Skip to content

feat(auth): configure ALTS bound tokens in Options's validate() #11812

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rockspore
Copy link
Member

This is a different approach compared with #11665. The unexported field altsCredentials will be resolved during the validate() at most one time and used in subsequent dial() calls.

@quartzmo Could you PTAL at this? If this approach makes more sense to you, I feel we should do the same thing for the existing DetectDefault call especially with the introduction of MTLS_S2A feature. Let me know if there are any caveats I am missing here. Thanks.

@rockspore rockspore requested a review from a team as a code owner March 12, 2025 20:28
@quartzmo
Copy link
Member

quartzmo commented Mar 14, 2025

@rockspore Thank you for exploring the problems I raised in #11665. I think this PR suggests a good beginning for a larger cleanup of grpctransport. I would continue it as follows:

  1. Figure out why DetectDefault is potentially repeatedly called in dial instead of just once in Dial.
  2. An instance of grpctransport.Options should be considered mutable until its resolveDetectOptions method is called. After that, it should be considered an immutable record of the configuration that produced the credentials.DetectOptions. (This options object is subsequently used to call DetectDefault.)
  3. If another call to DetectDefault is needed, with a modified configuration, a new grpctransport.Options must be created. It can be a copy of the original. Its usage should also follow 2. above.
  4. Move all mutation of grpctransport.Options as well as all calls to DetectDefault out of dial and into Dial. This will improve efficiency as well as the readability of dial.
  5. Move the resolution of auth.Credentials out of dial and into Dial.

I think that these changes go far beyond your changes in #11665, which at the moment are consistent with the current design as well as nicely encapsulated in the configureDirectPath func. At this point, I feel we should merge #11665, as there is nothing in it that cannot be changed within the plan sketched out above. Also, I don't want step 1. above to delay you.

@quartzmo quartzmo added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Mar 14, 2025
@quartzmo
Copy link
Member

I'd like to keep this PR open until it can be expanded or replaced. However, I am converting it to a Draft since it is incomplete.

@quartzmo quartzmo marked this pull request as draft March 14, 2025 17:46
@rockspore
Copy link
Member Author

Thank you, @quartzmo! It makes a lot of sense.

gcf-merge-on-green bot pushed a commit that referenced this pull request Mar 14, 2025
This effectively reverts #11255 with the slight difference in that I refactored out the compute engine token provider check as an individual func, which I will utilize in pending #11812. Because the bound token credentials should only be configured if users didn't provide credentials explicitly or the given one is compute engine type.

Fixes #11821.
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.

2 participants