-
Notifications
You must be signed in to change notification settings - Fork 229
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
fix: makes default token url universe aware #1383
Conversation
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
this.tokenUrl = DEFAULT_TOKEN_URL.replace("{UNIVERSE_DOMAIN}", this.getUniverseDomain()); | ||
} catch (IOException e) { | ||
// Throwing an IOException would be a breaking change, so wrap it here. | ||
// This should not happen for this credential type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a place we can read this value for this credential type that doesn't offer an exception (and this dead code)?
Can we override getUniverseDomain() and strip the exception if it is always available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we could override it, I think we would still have the dead code in the overridden function, but I may be misunderstanding what you mean by strip the exception, do you just mean doing this? -
@Override
public String getUniverseDomain() {
try {
return super.getUniverseDomain();
} catch (IOException e) {
throw new RuntimeException(e);
}
}
The other easier option would be to make universeDomain protected instead of private in GoogleCredentials so the external account credential could just read it directly instead of calling the parent getUniverseDomain() function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example you put is what I meant -- this implementation has a more precise definition of getUniverseDomain
that can embed the fact that it will not throw. This will help this codepath and any other users avoid a hard decision.
universeDomain
can't be protected
in GoogleCredentials because it isn't usable by subclasses of GCE credential types, and those credential types can't hide it. It probably isn't set until a successful request to MDS.
I understand the override is kind of ugly but it is a consequence of our deep type hierarchy.
Quality Gate failedFailed conditions |
Coverage test is complaining about new |
Makes the default sts token url logic in external account credentials universe aware.