Copilot commented on code in PR #3401:
URL: https://github.com/apache/dubbo-go/pull/3401#discussion_r3407608008
##########
remoting/getty/getty_server.go:
##########
@@ -55,62 +54,36 @@ func initServer(url *common.URL) {
return
}
- // load server config from rootConfig.Protocols
- // default use dubbo
- // TODO: Temporary compatibility with old APIs, can be removed later
- if url.GetParam(constant.ApplicationKey, "") == "" &&
config.GetApplicationConfig() == nil {
- return
- }
+ // Ensure application config is available via URL attribute
+ commonCfg.EnsureApplicationAttribute(url)
- // TODO: Temporary compatibility with old APIs, can be removed later
- if url.GetParam(constant.ProtocolKey, "") == "" &&
config.GetRootConfig().Protocols == nil {
+ protocolConfRaw, ok := url.GetAttribute(constant.ProtocolConfigKey)
+ if !ok || protocolConfRaw == nil {
+ logger.Warn("[Remoting][Getty] protocolConfig not found in URL
attributes")
return
}
-
- //TODO: Temporary compatibility with old APIs, can be removed later
- protocolConfMap :=
dubbo.CompatGlobalProtocolConfigMap(config.GetRootConfig().Protocols)
- if protocolConfMap == nil {
- if protocolConfRaw, ok :=
url.GetAttribute(constant.ProtocolConfigKey); ok {
- protocolConfig, ok :=
protocolConfRaw.(map[string]*global.ProtocolConfig)
- if !ok {
- logger.Warn("[Remoting][Getty] protocolConfig
assert failed")
- return
- }
- if protocolConfig == nil {
- logger.Warn("[Remoting][Getty] protocolConfig
is nil")
- return
- }
- protocolConfMap = protocolConfig
- }
+ protocolConfMap, ok :=
protocolConfRaw.(map[string]*global.ProtocolConfig)
+ if !ok || protocolConfMap == nil {
+ logger.Warn("[Remoting][Getty] protocolConfig assert failed or
is nil")
+ return
}
protocolConf := protocolConfMap[url.Protocol]
if protocolConf == nil {
logger.Debug("[Remoting][Getty] use default getty server
config")
return
} else {
- //server tls config
- tlsConfig :=
dubbo.CompatGlobalTLSConfig(config.GetRootConfig().TLSConfig)
-
- if tlsConfig == nil {
- if tlsConfRaw, ok :=
url.GetAttribute(constant.TLSConfigKey); ok {
- tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
- if !ok {
- logger.Error("[Remoting][Getty] getty
server initialized the TLSConfig configuration failed")
- return
+ // server tls config
+ if tlsConfRaw, ok := url.GetAttribute(constant.TLSConfigKey);
ok {
+ if tlsConf, ok := tlsConfRaw.(*global.TLSConfig); ok &&
dubbotls.IsServerTLSValid(tlsConf) {
Review Comment:
TLS enablement now depends on dubbotls.IsServerTLSValid(tlsConf) but there
is no unit test covering (1) valid TLS config enabling SSLEnabled/TLSBuilder
and (2) invalid/missing fields keeping TLS disabled. Adding coverage in
remoting/getty/getty_server_test.go would help prevent regressions during the
config-package removal.
##########
remoting/getty/getty_client.go:
##########
@@ -63,71 +62,30 @@ func initClient(url *common.URL) {
return
}
- // load client config from rootConfig.Protocols
- // default use dubbo
- // TODO: Temporary compatibility with old APIs, can be removed later
- if url.GetParam(constant.ApplicationKey, "") == "" &&
config.GetApplicationConfig() == nil {
- return
- }
+ // Ensure application config is available via URL attribute
+ commonCfg.EnsureApplicationAttribute(url)
- // TODO: Temporary compatibility with old APIs, can be removed later
- if url.GetParam(constant.ProtocolKey, "") == "" &&
config.GetRootConfig().Protocols == nil {
+ protocolConfRaw, ok := url.GetAttribute(constant.ProtocolConfigKey)
+ if !ok || protocolConfRaw == nil {
+ logger.Warn("[Remoting][Getty] protocolConfig not found in URL
attributes")
return
}
-
- // TODO: Temporary compatibility with old APIs, can be removed later
- protocolConfMap :=
dubbo.CompatGlobalProtocolConfigMap(config.GetRootConfig().Protocols)
- if protocolConfMap == nil {
- if protocolConfRaw, ok :=
url.GetAttribute(constant.ProtocolConfigKey); ok {
- protocolConfig, ok :=
protocolConfRaw.(map[string]*global.ProtocolConfig)
- if !ok {
- logger.Warn("[Remoting][Getty] protocolConfig
assert failed")
- return
- }
- if protocolConfig == nil {
- logger.Warn("[Remoting][Getty] protocolConfig
is nil")
- return
- }
- protocolConfMap = protocolConfig
- }
+ protocolConfMap, ok :=
protocolConfRaw.(map[string]*global.ProtocolConfig)
+ if !ok || protocolConfMap == nil {
+ logger.Warn("[Remoting][Getty] protocolConfig assert failed or
is nil")
+ return
}
protocolConf := protocolConfMap[url.Protocol]
if protocolConf == nil {
logger.Info("[Remoting][Getty] use default getty client config")
return
} else {
- //client tls config
- tlsConfig :=
dubbo.CompatGlobalTLSConfig(config.GetRootConfig().TLSConfig)
-
- if tlsConfig == nil {
- if tlsConfRaw, ok :=
url.GetAttribute(constant.TLSConfigKey); ok {
- tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
- if !ok {
- logger.Error("[Remoting][Getty] getty
client initialized the TLSConfig configuration failed")
- return
- }
- tlsConfig = tlsConf
- }
- }
-
- if tlsConfig != nil {
- clientConf.SSLEnabled = true
- clientConf.TLSBuilder = &getty.ClientTlsConfigBuilder{
- ClientKeyCertChainPath:
tlsConfig.TLSCertFile,
- ClientPrivateKeyPath:
tlsConfig.TLSKeyFile,
- ClientTrustCertCollectionPath:
tlsConfig.CACertFile,
- }
- } else if tlsConfRaw, ok :=
url.GetAttribute(constant.TLSConfigKey); ok {
- // use global TLSConfig handle tls
- tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
- if !ok {
- logger.Error("[Remoting][Getty] getty client
initialized the TLSConfig configuration failed")
- return
- }
- if dubbotls.IsClientTLSValid(tlsConf) {
- srvConf.SSLEnabled = true
- srvConf.TLSBuilder =
&getty.ClientTlsConfigBuilder{
+ // client tls config
+ if tlsConfRaw, ok := url.GetAttribute(constant.TLSConfigKey);
ok {
+ if tlsConf, ok := tlsConfRaw.(*global.TLSConfig); ok &&
dubbotls.IsClientTLSValid(tlsConf) {
+ clientConf.SSLEnabled = true
+ clientConf.TLSBuilder =
&getty.ClientTlsConfigBuilder{
Review Comment:
If TLSConfigKey is present but not a *global.TLSConfig, this code silently
ignores it and continues without TLS. That can mask configuration errors and
potentially downgrade transport security; consider logging and returning on
type-assert failure (consistent with other protocols).
##########
remoting/getty/getty_server.go:
##########
@@ -55,62 +54,36 @@ func initServer(url *common.URL) {
return
}
- // load server config from rootConfig.Protocols
- // default use dubbo
- // TODO: Temporary compatibility with old APIs, can be removed later
- if url.GetParam(constant.ApplicationKey, "") == "" &&
config.GetApplicationConfig() == nil {
- return
- }
+ // Ensure application config is available via URL attribute
+ commonCfg.EnsureApplicationAttribute(url)
- // TODO: Temporary compatibility with old APIs, can be removed later
- if url.GetParam(constant.ProtocolKey, "") == "" &&
config.GetRootConfig().Protocols == nil {
+ protocolConfRaw, ok := url.GetAttribute(constant.ProtocolConfigKey)
+ if !ok || protocolConfRaw == nil {
+ logger.Warn("[Remoting][Getty] protocolConfig not found in URL
attributes")
return
}
-
- //TODO: Temporary compatibility with old APIs, can be removed later
- protocolConfMap :=
dubbo.CompatGlobalProtocolConfigMap(config.GetRootConfig().Protocols)
- if protocolConfMap == nil {
- if protocolConfRaw, ok :=
url.GetAttribute(constant.ProtocolConfigKey); ok {
- protocolConfig, ok :=
protocolConfRaw.(map[string]*global.ProtocolConfig)
- if !ok {
- logger.Warn("[Remoting][Getty] protocolConfig
assert failed")
- return
- }
- if protocolConfig == nil {
- logger.Warn("[Remoting][Getty] protocolConfig
is nil")
- return
- }
- protocolConfMap = protocolConfig
- }
+ protocolConfMap, ok :=
protocolConfRaw.(map[string]*global.ProtocolConfig)
+ if !ok || protocolConfMap == nil {
+ logger.Warn("[Remoting][Getty] protocolConfig assert failed or
is nil")
+ return
}
protocolConf := protocolConfMap[url.Protocol]
if protocolConf == nil {
logger.Debug("[Remoting][Getty] use default getty server
config")
return
} else {
- //server tls config
- tlsConfig :=
dubbo.CompatGlobalTLSConfig(config.GetRootConfig().TLSConfig)
-
- if tlsConfig == nil {
- if tlsConfRaw, ok :=
url.GetAttribute(constant.TLSConfigKey); ok {
- tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
- if !ok {
- logger.Error("[Remoting][Getty] getty
server initialized the TLSConfig configuration failed")
- return
+ // server tls config
+ if tlsConfRaw, ok := url.GetAttribute(constant.TLSConfigKey);
ok {
+ if tlsConf, ok := tlsConfRaw.(*global.TLSConfig); ok &&
dubbotls.IsServerTLSValid(tlsConf) {
+ srvConf.SSLEnabled = true
+ srvConf.TLSBuilder =
&getty.ServerTlsConfigBuilder{
Review Comment:
If TLSConfigKey is present but not a *global.TLSConfig, this code silently
ignores it and continues without TLS. That can mask configuration errors and
potentially downgrade transport security; other protocols (e.g. gRPC) log an
error and abort initialization on type-assert failure.
##########
remoting/getty/getty_client.go:
##########
@@ -63,71 +62,30 @@ func initClient(url *common.URL) {
return
}
- // load client config from rootConfig.Protocols
- // default use dubbo
- // TODO: Temporary compatibility with old APIs, can be removed later
- if url.GetParam(constant.ApplicationKey, "") == "" &&
config.GetApplicationConfig() == nil {
- return
- }
+ // Ensure application config is available via URL attribute
+ commonCfg.EnsureApplicationAttribute(url)
- // TODO: Temporary compatibility with old APIs, can be removed later
- if url.GetParam(constant.ProtocolKey, "") == "" &&
config.GetRootConfig().Protocols == nil {
+ protocolConfRaw, ok := url.GetAttribute(constant.ProtocolConfigKey)
+ if !ok || protocolConfRaw == nil {
+ logger.Warn("[Remoting][Getty] protocolConfig not found in URL
attributes")
return
}
-
- // TODO: Temporary compatibility with old APIs, can be removed later
- protocolConfMap :=
dubbo.CompatGlobalProtocolConfigMap(config.GetRootConfig().Protocols)
- if protocolConfMap == nil {
- if protocolConfRaw, ok :=
url.GetAttribute(constant.ProtocolConfigKey); ok {
- protocolConfig, ok :=
protocolConfRaw.(map[string]*global.ProtocolConfig)
- if !ok {
- logger.Warn("[Remoting][Getty] protocolConfig
assert failed")
- return
- }
- if protocolConfig == nil {
- logger.Warn("[Remoting][Getty] protocolConfig
is nil")
- return
- }
- protocolConfMap = protocolConfig
- }
+ protocolConfMap, ok :=
protocolConfRaw.(map[string]*global.ProtocolConfig)
+ if !ok || protocolConfMap == nil {
+ logger.Warn("[Remoting][Getty] protocolConfig assert failed or
is nil")
+ return
}
protocolConf := protocolConfMap[url.Protocol]
if protocolConf == nil {
logger.Info("[Remoting][Getty] use default getty client config")
return
} else {
- //client tls config
- tlsConfig :=
dubbo.CompatGlobalTLSConfig(config.GetRootConfig().TLSConfig)
-
- if tlsConfig == nil {
- if tlsConfRaw, ok :=
url.GetAttribute(constant.TLSConfigKey); ok {
- tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
- if !ok {
- logger.Error("[Remoting][Getty] getty
client initialized the TLSConfig configuration failed")
- return
- }
- tlsConfig = tlsConf
- }
- }
-
- if tlsConfig != nil {
- clientConf.SSLEnabled = true
- clientConf.TLSBuilder = &getty.ClientTlsConfigBuilder{
- ClientKeyCertChainPath:
tlsConfig.TLSCertFile,
- ClientPrivateKeyPath:
tlsConfig.TLSKeyFile,
- ClientTrustCertCollectionPath:
tlsConfig.CACertFile,
- }
- } else if tlsConfRaw, ok :=
url.GetAttribute(constant.TLSConfigKey); ok {
- // use global TLSConfig handle tls
- tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
- if !ok {
- logger.Error("[Remoting][Getty] getty client
initialized the TLSConfig configuration failed")
- return
- }
- if dubbotls.IsClientTLSValid(tlsConf) {
- srvConf.SSLEnabled = true
- srvConf.TLSBuilder =
&getty.ClientTlsConfigBuilder{
+ // client tls config
+ if tlsConfRaw, ok := url.GetAttribute(constant.TLSConfigKey);
ok {
+ if tlsConf, ok := tlsConfRaw.(*global.TLSConfig); ok &&
dubbotls.IsClientTLSValid(tlsConf) {
Review Comment:
Client-side TLS is now gated by dubbotls.IsClientTLSValid(tlsConf), but
remoting/getty/getty_client_test.go doesn’t exercise the TLS attribute path.
Please add tests asserting that a valid TLSConfigKey attribute enables
SSLEnabled/TLSBuilder and that invalid configs do not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
