-
Notifications
You must be signed in to change notification settings - Fork 225
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
feat: Support querying S2A Addresses from MDS #1400
base: main
Are you sure you want to change the base?
Conversation
0f96e86
to
ddac7aa
Compare
cc: @xmenxk |
Quality Gate passedIssues Measures |
@westarle, friendly ping :). Please review when you get a chance. |
@@ -143,6 +143,11 @@ private S2AConfig getS2AConfigFromMDS() { | |||
try { | |||
HttpResponse response = request.execute(); | |||
if (!response.isSuccessStatusCode()) { | |||
int statusCode = response.getStatusCode(); |
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.
fyi: @lqiu96 @zhumin8 , after chatting with @xmenxk (authored the Go implementation: https://github.com/googleapis/google-cloud-go/blob/main/auth/internal/transport/s2a.go), he mentioned that we shouldn't retry if the endpoint doesn't exist. This logic exists in Go impl, so adding it here as well.
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.
I am leaning toward switching to HttpRequest's built-in retry handlers, logic like to customize the retry behavior based on return code can go into HttpUnsuccessfulResponseHandler
. See other comment for details.
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.
we shouldn't retry if the endpoint doesn't exist
This means that we don't retry only on 404 status code, right?
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.
I am leaning toward switching to HttpRequest's built-in retry handlers, logic like to customize the retry behavior based on return code can go into HttpUnsuccessfulResponseHandler. See other comment for details.
SG! I'll address your other comment.
This means that we don't retry only on 404 status code, right?
Yes, we want to retry on everything but 404.
Update: I've inspected the list of error codes returned by MDS, and updated the list of retriable error codes to include the ones that we want to retry on; we only want to retry on 5xx error codes. I'll ping you these references internally.
Thanks for the second pass through @lqiu96! I've addressed the comments, please let me know of any followups / additional comments. |
return S2AConfig.createBuilder().build(); | ||
} | ||
|
||
for (int i = 0; i < MAX_MDS_PING_TRIES; i++) { |
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.
I only mentioned ExponentialBackOff
because that is used in one of the recent changes related to retry (https://github.com/googleapis/google-auth-library-java/pull/1452/files)
I suppose you can do similar, by using HttpRequest's built-in retry handlers and set request.setNumberOfRetries(OAuth2Utils.DEFAULT_NUMBER_OF_RETRIES);
? If you do not need ExponentialBackOff
, maybe something simple e.g.
// Set the number of retries
request.setNumberOfRetries(OAuth2Utils.DEFAULT_NUMBER_OF_RETRIES);
// Retry on specific status codes (you might want to adjust these)
request.setUnsuccessfulResponseHandler(
new HttpUnsuccessfulResponseHandler() {
@Override
public boolean handleResponse(
HttpRequest request, HttpResponse response, boolean supportsRetry)
throws IOException {
return RETRYABLE_STATUS_CODES.contains(response.getStatusCode());
}
});
About thread-safety, noticed HttpRequest is also marked as not safe here, likely because its member variable hold state information . But I don't see it a concern in this implementation, as executeAsync()
is not used and getS2AConfigFromMDS()
creates and executes a request each time.
@@ -143,6 +143,11 @@ private S2AConfig getS2AConfigFromMDS() { | |||
try { | |||
HttpResponse response = request.execute(); | |||
if (!response.isSuccessStatusCode()) { | |||
int statusCode = response.getStatusCode(); |
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.
I am leaning toward switching to HttpRequest's built-in retry handlers, logic like to customize the retry behavior based on return code can go into HttpUnsuccessfulResponseHandler
. See other comment for details.
|
||
@CanIgnoreReturnValue | ||
public Builder setPlaintextAddress(String plaintextAddress) { | ||
this.plaintextAddress = plaintextAddress; |
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.
Can you add this explanation as javadoc comment here or to getS2AConfigFromMDS()
for future references?
try { | ||
request = transportFactory.create().createRequestFactory().buildGetRequest(genericUrl); | ||
request.setParser(parser); | ||
request.getHeaders().set(METADATA_FLAVOR, GOOGLE); | ||
request.setThrowExceptionOnExecuteError(false); | ||
request.setNumberOfRetries(OAuth2Utils.DEFAULT_NUMBER_OF_RETRIES); | ||
|
||
ExponentialBackOff backoff = | ||
new ExponentialBackOff.Builder() | ||
.setInitialIntervalMillis(OAuth2Utils.INITIAL_RETRY_INTERVAL_MILLIS) | ||
.setRandomizationFactor(OAuth2Utils.RETRY_RANDOMIZATION_FACTOR) | ||
.setMultiplier(OAuth2Utils.RETRY_MULTIPLIER) | ||
.build(); | ||
|
||
// Retry on 5xx status codes. | ||
request.setUnsuccessfulResponseHandler( | ||
new HttpBackOffUnsuccessfulResponseHandler(backoff) | ||
.setBackOffRequired( | ||
response -> RETRYABLE_STATUS_CODES.contains(response.getStatusCode()))); | ||
request.setIOExceptionHandler(new HttpBackOffIOExceptionHandler(backoff)); | ||
} catch (IOException e) { | ||
return S2AConfig.createBuilder().build(); | ||
} |
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.
S2AConfig.createBuilder().build()
sets plainTextAddress and mtlsS2AAddress to ""
by default. Can we move this code in this try block into the one below (merging them)?
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.
Done in ed681f5
oauth2_http/javatests/com/google/auth/oauth2/MockMetadataServerTransport.java
Outdated
Show resolved
Hide resolved
// Create the JSON response | ||
GenericJson content = new GenericJson(); | ||
content.setFactory(OAuth2Utils.JSON_FACTORY); | ||
if (requestStatusCode < 400) { |
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.
nit: Can this be 200? Or are there other 2xx/ 3xx codes?
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 it can just be 200. We expect only 200 on successful responses. Done in 20825f7
private String plaintextS2AAddressJsonKey; | ||
|
||
private String plaintextS2AAddress; | ||
|
||
private String mtlsS2AAddressJsonKey; | ||
|
||
private String mtlsS2AAddress; |
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.
nit: Can these values be stored as a map?
perhaps Map<String, String> s2aContentMap
(or a different name)? there would be one setter setS2AContentMap()
instead of 4?
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.
Done in 594df7b
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.
Thanks for working with us to address the comments. I think this mostly looks good to me given the context.
Can you also do two additional things:
- Run
mvn fmt:format
on this PR? - Update the title to be
feat: {title}
? Something that describes the functionalityfeat: Support querying S2A Addresses from MDS
or whatever you think aptly describes it.
@lqiu96 , I've addressed your latest round of comments -- thanks for the feedback! Run mvn fmt:format on this PR?
Update the title to be feat: {title}? Something that describes the functionality feat: Support querying S2A Addresses from MDS or whatever you think aptly describes it.
|
transportFactory.transport.setS2AContentMap( | ||
"plaintextS2AAddressJsonKey", S2A.S2A_PLAINTEXT_ADDRESS_JSON_KEY); | ||
transportFactory.transport.setS2AContentMap("plaintextS2AAddress", S2A_PLAINTEXT_ADDRESS); | ||
transportFactory.transport.setS2AContentMap( | ||
"mtlsS2AAddressJsonKey", S2A.S2A_MTLS_ADDRESS_JSON_KEY); | ||
transportFactory.transport.setS2AContentMap("mtlsS2AAddress", S2A_MTLS_ADDRESS); |
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.
Ah, I was going to suggest something like:
For valid data:
transportFactory.transport.setS2AContentMap(
ImmutableMap.of(S2A.S2A_PLAINTEXT_ADDRESS_JSON_KEY, S2A_PLAINTEXT_ADDRESS,
S2A.S2A_MTLS_ADDRESS_JSON_KEY, S2A_MTLS_ADDRESS));
For invalid data:
transportFactory.transport.setS2AContentMap(
ImmutableMap.of(INVALID_JSON_KEY, S2A_PLAINTEXT_ADDRESS,
S2A.S2A_MTLS_ADDRESS_JSON_KEY, S2A_MTLS_ADDRESS));
And the content would be populated via iteration through tkey k/v's of the map. Something like:
if (requestStatusCode == 200) {
for (Map.Entry<String, String> entrySet: content.entrySet()) {
content.put(entrySet.getKey(), entrySet.getValue())
}
}
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.
This is more succinct -- thanks. Done in 1e6c058
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.
LGTM. Added a few nits. Please do let @zhumin8 to review and give an approval as well
return new Builder(); | ||
} | ||
|
||
public String getPlaintextAddress() { |
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.
I'd like Javadocs for all these public things, pointing to public docs for MDS.
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.
We don't have this documented in public MDS docs (e.g. https://cloud.google.com/compute/docs/metadata/predefined-metadata-keys). We do have an AIP: https://google.aip.dev/auth/4115 which discusses this autconfig endpoint and how it fits in the mTLS via S2A + bound tokens story. WDYT about 934679c?
Quality Gate passedIssues Measures |
Add utility to get S2A address from MDS MTLS autoconfiguration endpoint.
This utility will be used when creating mTLS channel using S2A Java Client, which takes S2A Address as input to create S2AChannelCredentials.
Parallel change in go: googleapis/google-api-go-client#1874
S2A Java client: grpc/grpc-java#11113