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: Add experimental S2A integration in client libraries grpc transport #3326
feat: Add experimental S2A integration in client libraries grpc transport #3326
Changes from 5 commits
aebd139
b456935
3510643
e799526
70ae8d0
7ef7313
6d98115
250fecc
8469a1a
e880fbe
28adb31
b3e2e06
90892ef
33b710c
b320cc2
035e17e
759c3df
c096cb7
dc4b61e
d4fcc71
393190a
30a37c2
c3b93a0
18a4cf2
0897730
9901656
257c515
e4565f4
33bd7a6
f9eef5b
a7af12e
eb70225
2a50985
943683d
34f030f
fb3d608
926faca
4ee2ee9
2f70bf8
cb5768f
c96c760
30ff3d6
791ab2c
a4ee6c5
6eeccb2
03eb991
91175ef
04d47a0
7d7b233
a42dcbd
56551c6
1ff7a92
2958fb4
c9a7edd
8bf770d
33faba1
924a4e4
b00fd53
ca5be85
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.
I guess these are well-known locations on GCE? Do we have any public or internal docs for these locations? If not, where do we get these? What I'm worried is that how do we get notified if they change?
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
I included a link down below in the logic, I've moved it up here as well: https://cloud.google.com/compute/docs/metadata/overview#https-mds. Specifically https://cloud.google.com/compute/docs/metadata/overview#https-mds-root-certs and https://cloud.google.com/compute/docs/metadata/overview#https-mds-client-certs
2958fb4
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.
hmm, I see
endpointOverride
is needed within transport channel. Is providing this override option a requirement for this feature or AIP?nit: Do you mind to separate these 2 scenarios in single condition if statements for a bit more clarity?
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.
Is providing this override option a requirement for this feature or AIP?
separating into 2 single condition if done in 33b710c
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 change this to:
to reduce nesting?
Also don't we need this for plaintextAddress 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.
The logic we want is to "fallback" to plaintextAddress in a few cases (basically on any failure to create mtls to S2A creds):
mtlsAddress.isEmpty()
TlsChannelCredentials
In 30a37c2 I reduced the nesting, let me know what you think. I also reversed the negation checks (e.g. `if x != null) to make it a bit more readable.
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 we can early exit with null in the exceptions above, I don't think we would need null check. mtlsToS2AChannelCredentials shouldn't be null as it would either return a TlsChannelCredentials or throw an IOException
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 makes sense, done in 18a4cf2
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.
Is this supposed to be the last check? Shouldn't this be one of the earliest checks? I assumed so since we are retrieving the S2AConfig in the first step.
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.
plaintextAddress
is a fallback, so we only really want to use if we fail to usemtlsAddress
to create mtls to S2A channel credentials, which is why the check is at the very end. I did some refactoring in 30a37c2 which may help the readability.