feat: service sccount to service account impersonation to support universe domain

[zhumin8]

for context: b/340602527

Changes in this pr:

• Override getUniverseDomain() to grab source credentials’s universe domain (UD) by default. Always use source credentials UD, not explicit provided UD. (In current design, impersonated credentials may not have universe domain in the outer layer. relay on UD from source credential. This may change in future)

• Fix isDefaultUniverseDomain() in GoogleCredentials to account for getUniverseDomain() overrides in child classes.

• In refreshAccessToken(), use endpoint url pattern to account for TPC case.

  • note that I choose to bypass this refreshIfExpired step because it wrongly steps into code path meant only for OAuth2 token request (GDU flow). Filed ServiceAccountCredentials with SSJ flow will get error when invoking refreshAccessToke() #1534 to address this separately. But for GDU flow here, this refresh step is redundant because the SSJ will get re-generated at initialize request. Also skip this step for SA GDU with SSJ flow.

• Throw IllegalStateException if UD is explicitly set (with parent class setter) and not matching source credential's UD

• Fix toBuilder() to invoke super, and fix related issue with createScoped. (see fix: Invalidate the SA's AccessToken when createScoped() is called #1489, fix: ComputeEngineCredentials.createScoped should invalidate existing AccessToken #1428); Also fix equals() to compare super first.

Not in this pr:

• idtoken and signBlob endpoint changes are out-of-scope for this pr, will raise separate pr for it.

sa-to-sa impersonation is successfully E2E tested for TPC usage according to go/prptst-testing-service-account-impersonation.

[blakeli0]

I'm still not sure why this call is required for GDU but not required for nonGDU, is there any feature that is only supported in GDU?

[zhumin8]

I am convinced that this is not needed for nonGDU, reasons in description above.
For GDU, I am not 100% convinced that this is required before initialize step. But out of precautious, I do not want to change the flow for GDU in this pr. I will raise a separate issue to investigate for GDU flow.

[TimurSadykov]

to answer Blake's question - GDU has token endpoint, nonGDU does not :)

SSJ is a primary flow is GDU now as well. This should be a fallback scenario if SSJ is disabled, execute conditionally if SSJ flag is disabled.

[zhumin8]

updated condition to also skip for GDU SSJ flow.

[blakeli0]

I don't think we have to override this method as it is already public in the parent. I noticed that it seems we are overriding the builder methods in other subclasses of the builder, but I think that was a mistake.

[zhumin8]

I debated this in my mind. One benefit I see overriding the setter allows for a fluent builder API where method calls can be chained without requiring explicit casting to ImpersonatedCredentials. In this case, I feel it provides a slightly better experience for users setting explicit UD
So user can do

        ImpersonatedCredentials.newBuilder()
            .setUniverseDomain("explicit.domain.com");

instead of casting:

        (ImpersonatedCredentials) ImpersonatedCredentials.newBuilder()
            .setUniverseDomain("explicit.domain.com");

While generally it's best to avoid overriding public methods if possible, override setters in Builders is kind of a special case and more acceptable. .e.g. overriding Builder setters seems to be a pattern in other libraries like Guava (example)

One other concern is removing other overrides in other credential classes will be breaking change to customers code.
That said, I don't feel too strongly about this, and I believe exposing this setter is not a hard requirement for now. If you have any other concerns, I can remove this for now. What do you think?

[blakeli0]

I don't think customers have to cast the credential to ImpersonatedCredentials to setUniverseDomain(), I verified in my local that it works. Can you please double check it?
For the Guava example, yes they are doing it because there are some additional logics, but we are not doing anything special other than calling super.setUniverseDomain(). So I think we can delete it at this moment and add it later if needed.

[TimurSadykov]

Confirming that explicit overrides were added to avoid casting.

[blakeli0]

Min and I confirmed that we do need explicit casting right now. However, the problem is that the parent builder's setters should return the child types. I created a demo PR for a potential fix. Also see how protobuf's GeneratedMessageV3 does it.

That being said, this should be separate problem to solve, so it's OK to override the parent's setters right now.

[TimurSadykov]

What would be a breaking change?

[zhumin8]

added to comment, to throw IOException would be breaking.

[TimurSadykov]

update docs?

[TimurSadykov]

LGTM

[TimurSadykov]

Overall looks good, there is a pending discussion on explicit UD

[zhumin8]

@blakeli0 @TimurSadykov Can you please take another look? Changes since you last reviewed:
Updated with changes to reflect internal discussion to throw for explicit set universe domain not matching source credential's ud.
Also updated condition to skip refreshIfExpired() to include gdu w/ ssj flow.
Added tests for these changes.

note: kokoro tests are failing because trampoline is disabled at the moment.

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[TimurSadykov]

LGTM