[xds] Implement gRFC A101#41051
Conversation
markdroth
left a comment
There was a problem hiding this comment.
This code looks really good overall!
I have a bunch of comments, but the vast majority of them are minor cosmetic things. Mainly waiting on the tests -- and I'm happy to chat about the best way to write those if you're having trouble with them.
Please let me know if you have any questions. Thanks!
|
I think I addressed all of the comments that I didn't respond to individually. |
markdroth
left a comment
There was a problem hiding this comment.
This is looking really good! As before, comments are mostly minor; the main missing thing is the e2e tests.
Please let me know if you have any questions. Thanks!
|
|
||
| // Tls Context used by clients | ||
| CommonTlsContext common_tls_context; | ||
| UpstreamTlsContext upstream_tls_context; |
There was a problem hiding this comment.
Okay. Just please make sure there aren't any edge cases where the code that reads these new fields can't tell the difference between the TLS transport socket not being present and these fields not being set. I don't think there should be any such cases, but the ambiguity in the representation here makes me a little nervous.
markdroth
left a comment
There was a problem hiding this comment.
This is looking really good!
4c431ca to
7de742c
Compare
|
I added end2end tests, and they pass except for the Logical DNS test, which seems to be getting the wrong cert for some reason. |
|
For future reference, please don't force-push to a branch once code review has started, since that makes it almost impossible for the reviewer to see what has changed since their last review pass. Thanks! |
markdroth
left a comment
There was a problem hiding this comment.
This is looking really good! Remaining comments are relatively minor.
Please let me know if you have any questions. Thanks!
…51d23 Automated fix for refs/heads/xds_sni_support
markdroth
left a comment
There was a problem hiding this comment.
This looks great!
Remaining comments are minor; feel free to merge after addressing.
…41460) This option was added to the core credentials API in #41051. Now we add the same option to the corresponding C++ API. b/203822267 <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes #41460 COPYBARA_INTEGRATE_REVIEW=#41460 from murgatroid99:c++_creds_sni_override 7241f2a PiperOrigin-RevId: 859620187
…rpc#41460) This option was added to the core credentials API in grpc#41051. Now we add the same option to the corresponding C++ API. b/203822267 <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#41460 COPYBARA_INTEGRATE_REVIEW=grpc#41460 from murgatroid99:c++_creds_sni_override 7241f2a PiperOrigin-RevId: 859620187
This includes two major changes:
sni_overridewith the typeoptional<string>. Ifnullopt, it has no effect, and if set to the empty string it disables sending SNI entirely. Otherwise, the specified string will be sent.