Skip to content
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

fix: plumb mtlsEndpoint to gRPC Channel provider instead of setting it in EndpointContext. #3386

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rmehta19
Copy link
Contributor

@rmehta19 rmehta19 commented Nov 19, 2024

Fix a problem where if DirectPath is to be used (canUseDirectPath is going to return true), but endpoint resolution determines conditions are met to use S2A, the endpoint incorrectly gets set to mtls endpoint.

We only try S2A logic (e.g we only look at useS2A), if we have ruled out DirectPath as an option (canUseDirectPathI() returns false), but endpoint resolution logic doesn't know this, and will set endpoint to mtls endpoint if it determines S2A can be used, even if DirectPath can be used.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Nov 19, 2024
Copy link

conventional-commit-lint-gcf bot commented Nov 19, 2024

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@rmehta19 rmehta19 force-pushed the directpath-endpoint-fix branch from 364ea86 to 0f517a9 Compare November 19, 2024 22:32
@rmehta19
Copy link
Contributor Author

@lqiu96 @zhumin8 @blakeli0

@@ -272,10 +272,6 @@ private String determineUniverseDomain() {

/** Determines the fully resolved endpoint and universe domain values */
private String determineEndpoint() throws IOException {
if (shouldUseS2A()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of renaming this to canUseS2A(), since that's what the result of this function is really indicating.

For example, it is possible that this function returns true and canUseDirectPath in InstantiatingGrpcChannelProvider` returns true, and we therefore end up not using S2A.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant