fix: makes default token url universe aware#1383
Conversation
…ials.java Co-authored-by: Leo <39062083+lsirac@users.noreply.github.com>
| 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.
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.
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.
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.
|
|
Coverage test is complaining about new |


Makes the default sts token url logic in external account credentials universe aware.