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: Verify Universe Domain's DirectPath Compatibility after Endpoint Resolution #2412

Merged
merged 3 commits into from Jan 24, 2024

Conversation

lqiu96
Copy link
Contributor

@lqiu96 lqiu96 commented Jan 23, 2024

Fixes: #2411

@lqiu96 lqiu96 requested a review from blakeli0 January 23, 2024 23:19
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Jan 23, 2024
@lqiu96
Copy link
Contributor Author

lqiu96 commented Jan 23, 2024

Looking into possible showcase tests to verify client initialization with certain settings.

@lqiu96
Copy link
Contributor Author

lqiu96 commented Jan 23, 2024

Looking into possible showcase tests to verify client initialization with certain settings.

I think the configurations with this would revolve around setting flags in InstantiatingGrpcChannelProvider which would be covered in InstantiatingGrpcChannelProviderTest. I think dedicated e2e tests would be better suited in the future.

@lqiu96 lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 23, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 23, 2024
@lqiu96 lqiu96 marked this pull request as ready for review January 23, 2024 23:35
@lqiu96 lqiu96 requested a review from a team as a code owner January 23, 2024 23:35
// If the endpoint is null, then the endpoint hasn't been fully resolved yet (client not
// initialized). Before a call goes through with DirectPath, this validation must succeed.
if (endpoint == null) {
return true;
Copy link
Collaborator

@blakeli0 blakeli0 Jan 24, 2024

Choose a reason for hiding this comment

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

This would fix the issue but this is a little misleading IMO. If endpoint is null and this method returns true, it does not mean we canUseDirectPathWithUniverseDomain, but rather we can not validate it.
Can we not log direct path misconfigure on client initialization? And only adding endpoint.contains(Credentials.GOOGLE_DEFAULT_UNIVERSE) in channel creation where endpoint is guaranteed not null? This should be good enough to meet the requirement for universeDomain validation with DirectPath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would fix the issue but this is a little misleading IMO. If endpoint is null and this method returns true, it does not mean we canUseDirectPathWithUniverseDomain, but rather we can not validate it.
Can we not log direct path misconfigure on client initialization?

Makes sense. I think I can just remove the call from logDirectPathMisconfig() instead of trying to force it in there.

And only adding endpoint.contains(Credentials.GOOGLE_DEFAULT_UNIVERSE) in channel creation where endpoint is guaranteed not null? This should be good enough to meet the requirement for TPC.

sg, will update.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 24, 2024
return true;
}
// This is only logged if DirectPath is enabled
LOG.log(Level.WARNING, "DirectPath will only work in the the googleapis.com Universe Domain");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept this log statement as it only triggers if these three conditions above are true:

if (isDirectPathEnabled()
&& isCredentialDirectPathCompatible()
&& isOnComputeEngine()

Might be beneficial since the user is has all the other DirectPath configurations correct except for Universe Domain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be beneficial, but this check is called on every channel resizing, which by default is every minute, hence could cause spam logs to customers. That's why the logDirectPathMisconfigure was moved to the constructor initially, so I think we can skip this logging for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Will remove then

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Jan 24, 2024
Copy link

sonarcloud bot commented Jan 24, 2024

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Jan 24, 2024

Quality Gate Failed Quality Gate failed for 'java_showcase_integration_tests'

Failed conditions

0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@lqiu96 lqiu96 merged commit e2de93b into main Jan 24, 2024
37 of 39 checks passed
@lqiu96 lqiu96 deleted the directpath-endpoint-fix branch January 24, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client fails to initialize when enabling DirectPath
2 participants