Copilot commented on code in PR #2891:
URL: https://github.com/apache/dubbo-go/pull/2891#discussion_r2117870981
##########
protocol/grpc/client.go:
##########
@@ -102,7 +107,28 @@ func NewClient(url *common.URL) (*Client, error) {
return nil, err
}
dialOpts = append(dialOpts,
grpc.WithTransportCredentials(credentials.NewTLS(cfg)))
+ } else if tlsConfRaw, ok := url.GetAttribute(constant.TLSConfigKey); ok
{
+ // use global TLSConfig handle tls
+ tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
+ if !ok {
+ logger.Errorf("DUBBO3 Server initialized the TLSConfig
configuration failed")
Review Comment:
Error message refers to 'Server' in a client context; update to 'Client' for
clarity.
```suggestion
logger.Errorf("DUBBO3 Client initialized the TLSConfig
configuration failed")
```
##########
protocol/triple/server.go:
##########
@@ -88,24 +90,23 @@ func (s *Server) Start(invoker base.Invoker, info
*common.ServiceInfo) {
}
// todo: support opentracing interceptor
- var cfg *tls.Config
- var err error
- // handle tls config
- // TODO: think about a more elegant way to configure tls,
- // Maybe we can try to create a ServerOptions for unified settings,
- // after this function becomes bloated.
-
- // TODO: Once the global replacement of the config is completed,
- // replace config with global.
- tlsConfig := config.GetRootConfig().TLSConfig
- if tlsConfig != nil {
- cfg, err = config.GetServerTlsConfig(&config.TLSConfig{
- CACertFile: tlsConfig.CACertFile,
- TLSCertFile: tlsConfig.TLSCertFile,
- TLSKeyFile: tlsConfig.TLSKeyFile,
- TLSServerName: tlsConfig.TLSServerName,
- })
+ // TODO: move tls config to handleService
+
+ var tlsConf *global.TLSConfig
+
+ // handle tls
+ tlsConfRaw, ok := URL.GetAttribute(constant.TLSConfigKey)
+ if ok {
+ tlsConf, ok = tlsConfRaw.(*global.TLSConfig)
+ if !ok {
+ logger.Errorf("TRIPLE Server inintialized the TLSConfig
configuration failed")
+ return
+ }
+ }
+ if dubbotls.IsServerTLSValid(tlsConf) {
+ cfg, err := dubbotls.GetServerTlsConfig(tlsConf)
if err != nil {
+ logger.Errorf("TRIPLE Server inintialized the TLSConfig
configuration failed. err: %v", err)
Review Comment:
Typo in log message: 'inintialized' should be spelled 'initialized'.
```suggestion
logger.Errorf("TRIPLE Server initialized the TLSConfig
configuration failed")
return
}
}
if dubbotls.IsServerTLSValid(tlsConf) {
cfg, err := dubbotls.GetServerTlsConfig(tlsConf)
if err != nil {
logger.Errorf("TRIPLE Server initialized the TLSConfig
configuration failed. err: %v", err)
```
##########
protocol/grpc/client.go:
##########
@@ -102,7 +107,28 @@ func NewClient(url *common.URL) (*Client, error) {
return nil, err
}
dialOpts = append(dialOpts,
grpc.WithTransportCredentials(credentials.NewTLS(cfg)))
+ } else if tlsConfRaw, ok := url.GetAttribute(constant.TLSConfigKey); ok
{
+ // use global TLSConfig handle tls
+ tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
+ if !ok {
+ logger.Errorf("DUBBO3 Server initialized the TLSConfig
configuration failed")
+ return nil, errors.New("DUBBO3 Client initialized the
TLSConfig configuration failed")
+ }
+
+ if dubbotls.IsClientTLSValid(tlsConf) {
+ cfg, err := dubbotls.GetServerTlsConfig(tlsConf)
Review Comment:
For client-side TLS, 'GetClientTlsConfig' should be used instead of
'GetServerTlsConfig'.
```suggestion
cfg, err := dubbotls.GetClientTlsConfig(tlsConf)
```
##########
protocol/grpc/server.go:
##########
@@ -116,11 +120,32 @@ func (s *Server) Start(url *common.URL) {
if err != nil {
return
}
- logger.Infof("Grpc Server initialized the TLSConfig
configuration")
+ logger.Infof("gRPC Server initialized the TLSConfig
configuration")
serverOpts = append(serverOpts,
grpc.Creds(credentials.NewTLS(cfg)))
+ } else if tlsConfRaw, ok := url.GetAttribute(constant.TLSConfigKey); ok
{
+ // use global TLSConfig handle tls
+ tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
+ if !ok {
+ logger.Errorf("gRPC Server initialized the TLSConfig
configuration failed")
+ return
+ }
+ if dubbotls.IsServerTLSValid(tlsConf) {
+ cfg, tlsErr := dubbotls.GetServerTlsConfig(tlsConf)
+ if tlsErr != nil {
+ return
+ }
+ if cfg != nil {
+ logger.Infof("gRPC Server initialized the
TLSConfig configuration")
+ serverOpts = append(serverOpts,
grpc.Creds(credentials.NewTLS(cfg)))
+ }
+ } else {
+ serverOpts = append(serverOpts,
grpc.Creds(insecure.NewCredentials()))
+ }
} else {
+ // TODO: remove this else
Review Comment:
[nitpick] The nested if-else structure is complex and contains duplicate
insecure credential branches; consider simplifying the logic to avoid redundant
else blocks.
```suggestion
} else {
tlsConfRaw, ok := url.GetAttribute(constant.TLSConfigKey)
if ok {
// use global TLSConfig handle tls
tlsConf, ok := tlsConfRaw.(*global.TLSConfig)
if !ok {
logger.Errorf("gRPC Server initialized the
TLSConfig configuration failed")
return
}
if dubbotls.IsServerTLSValid(tlsConf) {
cfg, tlsErr :=
dubbotls.GetServerTlsConfig(tlsConf)
if tlsErr != nil {
return
}
if cfg != nil {
logger.Infof("gRPC Server initialized
the TLSConfig configuration")
serverOpts = append(serverOpts,
grpc.Creds(credentials.NewTLS(cfg)))
return
}
}
}
// Append insecure credentials if no valid TLS configuration is
found
```
--
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]
