From c8d26becdaf0a11d354bc422eec7483260387744 Mon Sep 17 00:00:00 2001 From: Jade Guiton Date: Wed, 5 Feb 2025 13:01:20 +0100 Subject: [PATCH] Remove `configauth` default, use nil in `configgrpc` default (#12271) #### Context The `configauth.NewDefaultAuthentication` returns a zero-initialized `Authentication` value ([link](https://github.com/open-telemetry/opentelemetry-collector/blob/477e4d3959932234e71f4e68c9d4e2290015a1df/config/configauth/configauth.go#L25)), which is used in `configgrpc.NewDefaultClientConfig` ([link](https://github.com/open-telemetry/opentelemetry-collector/blob/477e4d3959932234e71f4e68c9d4e2290015a1df/config/configgrpc/configgrpc.go#L109)) and `configgrpc.NewDefaultServerConfig` ([link](https://github.com/open-telemetry/opentelemetry-collector/blob/477e4d3959932234e71f4e68c9d4e2290015a1df/config/configgrpc/configgrpc.go#L196)). However, this default value is problematic, as it will cause the Collector to crash on startup with the error `Error: cannot start pipelines: failed to resolve authenticator "": authenticator not found`. There is no way for the `Authentication` struct to represent "no authentication" (which is presumably the intended default). The `configgrpc` code uses a `nil` `*Authentication` pointer to represent that case ([link](https://github.com/open-telemetry/opentelemetry-collector/blob/477e4d3959932234e71f4e68c9d4e2290015a1df/config/configgrpc/configgrpc.go#L310)). Regarding the use of these APIs: - Across Github, it looks like `configauth.NewDefaultAuthentication` is not used outside of `configgrpc`. - I haven't found any use of `configgrpc.NewDefaultServerConfig` on Github, and `configgrpc.NewDefaultClientConfig` has [one use in the Elastic OTel components repo](https://github.com/elastic/opentelemetry-collector-components/blob/3cfc4ac3a58e5ac9823bf2ccc966e2f306cff5c9/processor/ratelimitprocessor/config.go#L164), although I suspect the `Auth` field may get overriden anyway, so the crash may never actually occur. - It looks like the gRPC receivers/exporters in Core build their default `ClientConfig`/`ServerConfig` manually instead of using those functions, leaving the `Auth` field `nil`, which is why the crash does not occur there either. #### Description This PR: - Removes `configauth.NewDefaultAuthentication` since there doesn't seem to be a useful "default" value for this struct. Because it doesn't seem to be used outside this repo, I decided to skip the deprecation step. - Replaces its use in `configgrpc.NewDefaultClient/NewDefaultServerConfig` by `nil`. I think this would be considered a bug fix rather than a breaking change, since the current value causes an error if not overriden. #### Link to tracking issue Resolves #12223 (the original issue was about making the function signature more consistent, but removing the function entirely makes it no longer an issue I would say) #### Testing I experimentally confirmed that the above crash occurs with default config if we change the OTLP receiver's `createDefaultConfig` to use `configgrpc.NewDefaultServerConfig`, and that the above fix to said function prevents the crash. --- .chloggen/configauth-remove-default.yaml | 27 ++++++++++++++++++++++++ .chloggen/configgrpc-noauth-default.yaml | 26 +++++++++++++++++++++++ config/configauth/configauth.go | 5 ----- config/configauth/configauth_test.go | 6 ------ config/configgrpc/configgrpc.go | 2 -- config/configgrpc/configgrpc_test.go | 2 -- 6 files changed, 53 insertions(+), 15 deletions(-) create mode 100644 .chloggen/configauth-remove-default.yaml create mode 100644 .chloggen/configgrpc-noauth-default.yaml diff --git a/.chloggen/configauth-remove-default.yaml b/.chloggen/configauth-remove-default.yaml new file mode 100644 index 00000000000..59e0a4c9cd8 --- /dev/null +++ b/.chloggen/configauth-remove-default.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: configauth + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Remove NewDefaultAuthentication + +# One or more tracking issues or pull requests related to the change +issues: [12223] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + The value returned by this function will always cause an error on startup. + In `configgrpc.Client/ServerConfig.Auth`, `nil` should be used instead to disable authentication. + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [api] diff --git a/.chloggen/configgrpc-noauth-default.yaml b/.chloggen/configgrpc-noauth-default.yaml new file mode 100644 index 00000000000..1244682ea2a --- /dev/null +++ b/.chloggen/configgrpc-noauth-default.yaml @@ -0,0 +1,26 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: configgrpc + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Set Auth to nil in NewDefaultClientConfig/NewDefaultServerConfig" + +# One or more tracking issues or pull requests related to the change +issues: [12223] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + The value that was used previously would always cause an error on startup. + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [api] diff --git a/config/configauth/configauth.go b/config/configauth/configauth.go index a3501cd0bed..96c3e9c092b 100644 --- a/config/configauth/configauth.go +++ b/config/configauth/configauth.go @@ -27,11 +27,6 @@ type Authentication struct { AuthenticatorID component.ID `mapstructure:"authenticator"` } -// NewDefaultAuthentication returns a default authentication configuration. -func NewDefaultAuthentication() *Authentication { - return &Authentication{} -} - // GetServerAuthenticator attempts to select the appropriate auth.Server from the list of extensions, // based on the requested extension name. If an authenticator is not found, an error is returned. func (a Authentication) GetServerAuthenticator(_ context.Context, extensions map[component.ID]component.Component) (auth.Server, error) { diff --git a/config/configauth/configauth_test.go b/config/configauth/configauth_test.go index 89551d952ab..77cc1c8bb5d 100644 --- a/config/configauth/configauth_test.go +++ b/config/configauth/configauth_test.go @@ -17,12 +17,6 @@ import ( var mockID = component.MustNewID("mock") -func TestNewDefaultAuthentication(t *testing.T) { - auth := NewDefaultAuthentication() - assert.NotNil(t, auth) - assert.Empty(t, auth) -} - func TestGetServer(t *testing.T) { testCases := []struct { name string diff --git a/config/configgrpc/configgrpc.go b/config/configgrpc/configgrpc.go index 9f71e01ab7a..fc558227b4f 100644 --- a/config/configgrpc/configgrpc.go +++ b/config/configgrpc/configgrpc.go @@ -110,7 +110,6 @@ func NewDefaultClientConfig() *ClientConfig { return &ClientConfig{ TLSSetting: configtls.NewDefaultClientConfig(), Keepalive: NewDefaultKeepaliveClientConfig(), - Auth: configauth.NewDefaultAuthentication(), BalancerName: BalancerName(), } } @@ -196,7 +195,6 @@ type ServerConfig struct { func NewDefaultServerConfig() *ServerConfig { return &ServerConfig{ Keepalive: NewDefaultKeepaliveServerConfig(), - Auth: configauth.NewDefaultAuthentication(), } } diff --git a/config/configgrpc/configgrpc_test.go b/config/configgrpc/configgrpc_test.go index e979b7a1f49..7ce243df27a 100644 --- a/config/configgrpc/configgrpc_test.go +++ b/config/configgrpc/configgrpc_test.go @@ -47,7 +47,6 @@ func TestNewDefaultClientConfig(t *testing.T) { expected := &ClientConfig{ TLSSetting: configtls.NewDefaultClientConfig(), Keepalive: NewDefaultKeepaliveClientConfig(), - Auth: configauth.NewDefaultAuthentication(), BalancerName: BalancerName(), } @@ -83,7 +82,6 @@ func TestNewDefaultKeepaliveServerConfig(t *testing.T) { func TestNewDefaultServerConfig(t *testing.T) { expected := &ServerConfig{ Keepalive: NewDefaultKeepaliveServerConfig(), - Auth: configauth.NewDefaultAuthentication(), } result := NewDefaultServerConfig()