Skip to content

Refactor configauth to support multiple extension auth interfaces #12702

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Mar 22, 2025

The only interface that looks a bit strange is the "GetHTTPHandler" which needs the "reqParams" attribute. Would have been better if we actually support "HttpServer" extension auth that returns a Handler maybe...

@bogdandrutu bogdandrutu requested a review from a team as a code owner March 22, 2025 17:29
@bogdandrutu bogdandrutu requested a review from codeboten March 22, 2025 17:29
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me (modulo fixing the tests) and avoids a public API dependency between configauth and extensionauth.

@jmacd
Copy link
Contributor

jmacd commented Mar 29, 2025

Please take a look at #12700.

I would like to suggest that extensionauth.HTTPClient use more-descriptive names so that middleware extensions and auth extensions do not conflict. As stated in #12700, the middleware extension uses different names. Still, I would recommend e.g., for HTTP client auth extension, ClientAuthRoundTripper() instead of RoundTripper().

@mx-psi
Copy link
Member

mx-psi commented Apr 9, 2025

I would like to suggest that extensionauth.HTTPClient use more-descriptive names so that middleware extensions and auth extensions do not conflict.

I am sorry I missed this :( It is now too late for this since we marked this as 1.x. For next time, could you block the "mark as stable" PR or add a comment on the tracking issue for a particular module if there are changes needed there?

@bogdandrutu
Copy link
Member Author

I am sorry I missed this :( It is now too late for this since we marked this as 1.x. For next time, could you block the "mark as stable" PR or add a comment on the tracking issue for a particular module if there are changes needed there?

@mx-psi lots of rushing to this 1.x for these packages... as I said multiple times, a stamp of a tag 1.x does not mean anything if the quality is not there.

@mx-psi
Copy link
Member

mx-psi commented Apr 11, 2025

lots of rushing to this 1.x for these packages...

Let me start by saying that, to be clear, I think the suggestion by @jmacd's is a good suggestion. I am frustrated that I did not think about it, and I appreciate him taking the time to state it.

However, I strongly disagree that this was 'rushed' in any way, and I don't think delaying things would make things any better in general. Since I assume #12702 (comment) relates to extensionauth, let me put it as an example. I think the timeline is similar to other modules, since the majority of the discussion is publicly available I encourage you to take a look and tell me what has been 'rushed' in any other module. I am happy to learn from it and to have a constructive conversation as to what exactly should be done to avoid this 'rushing'.

This is the work that was put into place for stabilizing extensionauth and the dates in which public announcements or PRs were made:

  1. On August 29, 2024 I filed an issue for stabilizing extensionauth (see Stabilize module extension/extensionauth #11006) which I shared on Slack on the same day on the #otel-collector channel (note that at the time it was called extension/auth) and also on the #opentelemetry channel. I also added it to the meeting notes of the 2024-09-04 meeting (I just added a bookmark here so you can check it). You can also check on the recording from the meeting happening on that date, starting at around 3 minutes, how Antoine kindly announced this for me.
  2. On September 4, 2024, I tried to stabilize these modules for the first time, as can be seen in [extension, extension/auth] Mark modules as stable #11047. I gave up on this after some discussion.
  3. Since late February 2025 up until three weeks ago I have worked on continuing the stabilization, you can check [extension] Deprecate extension/auth in favor of extension/extensionauth #12478, [extensionauth] Remove deprecated methods #12547, [extensionauthtest] Remove extensionauthtest.MockClient #12567, [extensionauth] Split extensionauth.Client by protocol type #12574, [extensionauth,configauth] Remove deprecated symbols #12672 for the work that I did, and you can see how this was reviewed by multiple community members. These were discussed in the #otel-collector-core-leads private Slack channel in multiple conversations which I am happy to share publicly if I get the consent of everybody involved.
  4. On March 19th, I shared a message on #otel-collector-core-leads with the following message: "I intend to open a PR for extensionauth 1.0 and configauth 1.0 after this one: [extensionauth,configauth] Remove deprecated symbols #12672 PTAL!"
  5. On the same day, March 19th 2025, after that PR was merged, I filed Stabilize extensionauth #12675 to mark this module as 1.x. I pinged the Collector approvers handle and got 6 approvals from Collector approvers in the priod from March 19th to March 22nd.
  6. On March 22nd I got this message Stabilize extensionauth #12675 (comment). We discussed it on the stability meeting on March 24th, and agreed to leave configauth for later (precisely because of this PR).
  7. Given the agreement on this point, two days later I merged Stabilize extensionauth #12675.
  8. Finally, on March 31st 2025 v1.29.0 was released which includes this change.

There was a total of 7 months and 2 days excluding the end date from August 29, 2024 to propose any changes. This was shared publicly, on Slack, meetings, Github, including pinging the approvers group. This was done following a process that was agreed for this. At no point this particular objection was raised, despite the multiple opportunities to do so. Respectfully, I don't think 7 months is 'rushing' it.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants