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

Add user authentication to the ADC flow. #189

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jneem
Copy link
Contributor

@jneem jneem commented Nov 23, 2022

According to google's docs [1], the ADC strategy is

  1. env variable
  2. user credentials
  3. metadata server

Currently, this crate does 1 and 3; this PR inserts 2 in there.

[1] https://cloud.google.com/docs/authentication/application-default-credentials

Note that this is a breaking change, and it also breaks the test for me locally because I have user credentials configured. Presumably the CI doesn't so I guess they'll succeed there? But this isn't great so I've marked it as a draft.

@dermesser
Copy link
Owner

Great idea! Apologies for the delayed feedback. Let me know when this is ready for review.

@jneem
Copy link
Contributor Author

jneem commented Dec 12, 2022

Thanks for the encouragement! I'm not really sure what the best approach is, but here's one attempt.

The issue is that ApplicationDefaultCredentialsAuthenticator's behavior is dependent on the system state, which isn't really great for unit testing. So what I've done is made the instance metadata fallback into its own authenticator and just tested that. So now each of the three components can be used on their own, or you can use ApplicationDefaultCredentialsAuthenticator to automatically choose one.

One downside of this is extra breakage because of some renaming.

@jneem jneem marked this pull request as ready for review December 12, 2022 20:30
@rafael-arreola
Copy link

Will this feature be released soon?

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.

3 participants