Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Support querying S2A Addresses from MDS #1400
Changes from 19 commits
c96cb4a
f90be0b
0c64a0a
993663d
0f96e86
3d68cef
6d75a4e
d932e0c
6aa071b
ddac7aa
2c26736
67f9462
36d4cd1
fc2b246
359fd43
bce602e
32caef5
b82790a
05aa9cc
1466f0d
be1cfd2
c89b56c
544d9d1
c3ede1d
8238d50
36ab0a9
36a0ac7
ae545c8
47b3f2e
fb577a1
1f333b4
0bbd320
0e6f5ce
e786886
12b248d
4d05638
7447f0b
ed681f5
20825f7
594df7b
16fd964
1e6c058
934679c
257ed12
0e1631a
6644d50
8ca8d69
699bed7
8e5ccb0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we use the default retry value here:
google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/OAuth2Utils.java
Line 110 in d4a3763
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 c3ede1d
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 empty constructor seems a bit unnecessary as Java compiler provides it as default? Is it added to show this class can be instantiated without arguments?
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.
Yes, that is why the constructor was included, but thank you for pointing that out Min, I've removed it in 544d9d1
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 a more descriptive javadocs that details when empty addresses occur and what users are expected to do with it? I know it's a private method, but I think it would be helpful for future reference, especially given the information detailed in this comment: https://github.com/googleapis/google-auth-library-java/pull/1400/files#r1785153627
Also, what if only plaintextAddress is empty or mtsAddress is empty? Is that different when both are empty?
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 fb577a1
Also, what if only plaintextAddress is empty or mtsAddress is empty? Is that different when both are empty?
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.
If the response data is something like:
then I believe an IOException would be thrown and mtls_address wouldn't be set even though it's valid. Just want confirm the behavior.
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.
Yes this behavior could happen where an IOException is thrown and mtls_address is not set even though it is able to be parsed (I don't think it's too likely since we own the MDS mTLS autoconfiguration endpoint, and the json format is tested).
In 0e6f5ce I've adjusted the code so that it catches and ignores any exception thrown when parsing the addresses. So in this case, if for whatever reason, parsing the plaintext_address address fails, we still try and parse the mtls_address address. I chatted with @xmenxk, and we agree that in this case we shouldn't really retry since if we get a successful response from the MDS, it is unlikely to change if we query it again.
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 not super familiar with google-http-client, but can this retry logic be handled by maybe
HttpRequestRetryHandler
?ExponentialBackOff
used in retrys here is not thread-safe.cc.@lqiu96 who might be more familiar
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 pointing this out Min! A few followup questions:
ExponentialBackOff
in the retries, is their still a thread-safety problem? The retry logic here is mirrored fromgoogle-auth-library-java/oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java
Line 435 in dde6876
HttpRequestRetryHandler
. Should we instead use theorg.apache.http.impl.client
implementation so we can useorg.apache.http.impl.client.DefaultHttpRequestRetryHandler
or something similar?Apologies if I'm misunderstanding something here!
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 needExponentialBackOff
, maybe something simple e.g.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 andgetS2AConfigFromMDS()
creates and executes a request each time.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 I agree and I would prefer if the loops could be modified to use the unsuccessfulResponseHandler and exponential backoff (configured with a list of status codes that can be retried on).
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 the detail @zhumin8 ! In 12b248d I have modified to set the number of retries on the request and use
ExponentialBackOff
andHttpUnsuccessfulResponseHandler
. We only want to be retrying on 5xx error codes (503 is the expected one, but I listed all 5xx part of theHttpStatusCodes
package), so I have specified them as retryable error 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.
Can you add comment on why these are ignored?
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 added a comment in 6644d50. In general, if there is any error in this function, we ignore, and populate empty addresses in the
S2AConfig
.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?
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.
Please include the documentation for each public method here. The AIP isn't a great landing place -- can you please look into improving the public MDS docs at least with a bug to the metadata service team?
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 8ca8d69
The AIP isn't a great landing place -- can you please look into improving the public MDS docs at least with a bug to the metadata service team?
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.
Does these addresses need any validation or format check when setting with builder?
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.
No validation/format check is necessary here, because we own the MDS endpoint that is being queried to get the address, and it is up to the client which consumes the address (S2A client) to return error if there is a problem connecting to the S2A at that 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.
Can you add this explanation as javadoc comment here or to
getS2AConfigFromMDS()
for future references?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 4d05638