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 all 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.
maybe do try/catch block around these to be more specific about the exceptions catching? Nested try/catch blocks are a bit hard to read.
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 think I was the one that suggested this, but am open to having the try-catch blocks be more specific or a different implementation.
I suggested just have one try-catch block since this code doesn't catch or return the error message back to the user. It just catches any IO error and returns an empty S2AConfig back. Unfortunately, this doesn't provide any useful feedback to the user. Given that this is how it behaves already in Go, I thought it would just be simpler to have a catch-all (one big try block) for this logic.
Ideally, we would have more specific try-catch blocks which could tell the user exactly what went wrong (i.e. failed to build the request because x, or the request failed because of y).
I believe there are two spots where we potentially would need to add try-catches:
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.
There are a few places in this block that throw an
IOException
(theparseAs
andbuildGetRequest
also throwIOException
), so I removed the current nested try/catch block structure and added two separate try/catch blocks around the lines that throw an exception. Because we are getting rid of the big block, I also moved around variable definitions closer to where they are used.6644d50
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.
@lqiu96 , I think my changes and your comment happened at same time. Reading through your comment, the changes I just push match your suggestion. I removed the big try/catch and just did 2 smaller ones around building the request and executing it.
@zhumin8 , @lqiu96: I am fine with either option -- let me know if you prefer reverting to the 1 big/try catch block, or if you prefer the current state which has no nesting, but a few smaller try/catch blocks
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 fine with splitting it up to reduce nesting.
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