Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove configauth default, use nil in configgrpc default #12271

Merged

Conversation

jade-guiton-dd
Copy link
Contributor

@jade-guiton-dd jade-guiton-dd commented Feb 4, 2025

Context

The configauth.NewDefaultAuthentication returns a zero-initialized Authentication value (link), which is used in configgrpc.NewDefaultClientConfig (link) and configgrpc.NewDefaultServerConfig (link). 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).

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, 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.

Copy link

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.27%. Comparing base (477e4d3) to head (2e98f36).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12271      +/-   ##
==========================================
- Coverage   91.27%   91.27%   -0.01%     
==========================================
  Files         466      466              
  Lines       25586    25582       -4     
==========================================
- Hits        23354    23350       -4     
  Misses       1816     1816              
  Partials      416      416              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jade-guiton-dd jade-guiton-dd marked this pull request as ready for review February 5, 2025 10:42
@jade-guiton-dd jade-guiton-dd requested a review from a team as a code owner February 5, 2025 10:42
@mx-psi
Copy link
Member

mx-psi commented Feb 5, 2025

Since there are no default values I think this section from the coding guidelines does not really apply. If I am wrong, since this is something that can be fixed in a backwards compatible way, we can just open another PR. Merging!

@mx-psi mx-psi added this pull request to the merge queue Feb 5, 2025
Merged via the queue into open-telemetry:main with commit c8d26be Feb 5, 2025
72 checks passed
@jade-guiton-dd
Copy link
Contributor Author

jade-guiton-dd commented Feb 5, 2025

I didn't remember that section of the coding guidelines; I think it may actually apply here... but I think it's a bit of a silly rule in cases like this one where the only sensible default is "don't create the struct" and you would need to overwrite every field anyway, so I'll leave it to your judgment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[configauth] Return type of NewDefaultAuthentication should not be pointer
2 participants