[configtls] [config/configgrpc] Use configtls.NewDefaultClientConfig(), configtls.NewDefaultConfig() and configtls.NewDefaultServerConfig() #11638
Conversation
…instead of manually creating struct. See: open-telemetry#11383 Signed-off-by: wasim-nihal <[email protected]>
| @@ -102,11 +102,10 @@ func TestDefaultGrpcClientSettings(t *testing.T) { | |||
| tt, err := componenttest.SetupTelemetry(componentID) | |||
| require.NoError(t, err) | |||
| t.Cleanup(func() { require.NoError(t, tt.Shutdown(context.Background())) }) | |||
|
|
|||
| clientConfig := configtls.NewDefaultClientConfig() | |||
| clientConfig.Insecure = true | |||
| gcs := &ClientConfig{ | |||
There was a problem hiding this comment.
In most/all of these first cases, wouldn't you want to use the NewDefaultClientConfig at the outer level, rather than only on the inner TLSSetting?
i.e. using this function:
https://github.com/nokia/open-telemetry-opentelemetry-collector/blob/1703ce6df6979d84720b5826c97d579cf11a59bb/config/configgrpc/configgrpc.go#L111
There was a problem hiding this comment.
In most/all of these first cases, wouldn't you want to use the NewDefaultClientConfig at the outer level, rather than only on the inner TLSSetting?
Thanks for taking a look! This suggested change should be done for all the test cases or only the initial ones? PR includes changes only to migrate to new configtls.NewDefaultClientConfig(). If the community is okay, I am willing to create an issue to track this and create separate PR addressing this.
Please share your thoughts.
There was a problem hiding this comment.
Please ignore my comment if it doesn't match the requirements. I just thought it odd that we'd use one and not the other. I'm new here. :)
config/configgrpc/configgrpc_test.go
Outdated
| @@ -102,11 +102,10 @@ func TestDefaultGrpcClientSettings(t *testing.T) { | |||
| tt, err := componenttest.SetupTelemetry(componentID) | |||
| require.NoError(t, err) | |||
| t.Cleanup(func() { require.NoError(t, tt.Shutdown(context.Background())) }) | |||
|
|
|||
| clientConfig := configtls.NewDefaultClientConfig() | |||
There was a problem hiding this comment.
nit: The term "clientConfig" is highly overloaded in this context, I wonder if it would be better to rename this local "tlsSettings" or something similar.
|
@mackjmr can you please review this change? |
config/configgrpc/configgrpc_test.go
Outdated
| @@ -139,7 +137,8 @@ func TestAllGrpcClientSettings(t *testing.T) { | |||
| tt, err := componenttest.SetupTelemetry(componentID) | |||
| require.NoError(t, err) | |||
| t.Cleanup(func() { require.NoError(t, tt.Shutdown(context.Background())) }) | |||
|
|
|||
| clientConfig := configtls.NewDefaultClientConfig() | |||
| clientConfig.Insecure = false | |||
config/configgrpc/configgrpc_test.go
Outdated
| @@ -375,15 +368,15 @@ func TestGrpcServerAuthSettings(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestGrpcClientConfigInvalidBalancer(t *testing.T) { | |||
| clientConfig := configtls.NewDefaultClientConfig() | |||
| clientConfig.Insecure = false | |||
There was a problem hiding this comment.
Insecure is false by default
…nd configtls.NewDefaultServerConfig() Signed-off-by: wasim-nihal <[email protected]>
Signed-off-by: wasim-nihal <[email protected]>
|
@atoulme , thanks for your review. I have addressed the comments in the latest commit. Could you please check? |
Description
[config/configgrpc] Changes to move away from manually creating configtls.ClientConfig, configtls.Config and configtls.ServerConfig in favor of using configtls.NewDefaultClientConfig, configtls.NewDefaultClientConfig(), configtls.NewDefaultConfig() and configtls.NewDefaultServerConfig() respectively.
Link to tracking issue
Fixes #11383