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.

Sign up for GitHub

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

Jump to bottom

fix: Verify Universe Domain's DirectPath Compatibility after Endpoint Resolution #2412

Merged
merged 3 commits into from Jan 24, 2024
Merged

fix: Verify Universe Domain's DirectPath Compatibility after Endpoint Resolution #2412

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
371a69a
fix: Verify Universe Domain's DirectPath Compatibility after Endpoint…
lqiu96 Jan 23, 2024
21eb9b3
chore: Address PR comments
lqiu96 Jan 24, 2024
33d1cbd
chore: Remove DirectPath logging statement
lqiu96 Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions ...java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Expand Up @@ -295,6 +295,7 @@ private void logDirectPathMisconfig() {
Level.WARNING,
"DirectPath is misconfigured. DirectPath is only available in a GCE environment.");
}
// Case 4: Universe Domain is non-GDU
if (!canUseDirectPathWithUniverseDomain()) {
LOG.log(
Level.WARNING, "DirectPath will only work in the the googleapis.com Universe Domain");
Expand Down Expand Up @@ -334,8 +335,16 @@ static boolean isOnComputeEngine() {
return false;
}

private boolean canUseDirectPathWithUniverseDomain() {
return endpoint.contains("googleapis.com");
// Universe Domain configuration is currently only supported in the GDU
@VisibleForTesting
boolean canUseDirectPathWithUniverseDomain() {
// ClientContext will set an endpoint if it isn't provided to the TransportChannelProvider
// 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 •
edited

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.

}
return endpoint.contains(Credentials.GOOGLE_DEFAULT_UNIVERSE);
}

@VisibleForTesting
Expand Down
32 changes: 32 additions & 0 deletions .../gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Expand Up @@ -292,6 +292,38 @@ public void testDirectPathDisallowNullCredentials() throws IOException {
assertThat(provider.isCredentialDirectPathCompatible()).isFalse();
}

@Test
public void testDirectPathWithNullEndpoint() {
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPath(true)
.setAttemptDirectPathXds()
.build();
assertThat(provider.canUseDirectPathWithUniverseDomain()).isTrue();
}

@Test
public void testDirectPathWithGDUEndpoint() {
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPath(true)
.setAttemptDirectPathXds()
.setEndpoint("test.googleapis.com:443")
.build();
assertThat(provider.canUseDirectPathWithUniverseDomain()).isTrue();
}

@Test
public void testDirectPathWithNonGDUEndpoint() {
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPath(true)
.setAttemptDirectPathXds()
.setEndpoint("test.random.com:443")
.build();
assertThat(provider.canUseDirectPathWithUniverseDomain()).isFalse();
}

@Test
public void testDirectPathXdsEnabled() throws IOException {
InstantiatingGrpcChannelProvider provider =
Expand Down