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 2 commits
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
15 changes: 9 additions & 6 deletions ...java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Expand Up @@ -295,10 +295,6 @@ private void logDirectPathMisconfig() {
Level.WARNING,
"DirectPath is misconfigured. DirectPath is only available in a GCE environment.");
}
if (!canUseDirectPathWithUniverseDomain()) {
LOG.log(
Level.WARNING, "DirectPath will only work in the the googleapis.com Universe Domain");
}
}
}
}
Expand Down Expand Up @@ -334,8 +330,15 @@ 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() {
if (endpoint.contains(Credentials.GOOGLE_DEFAULT_UNIVERSE)) {
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.

}
// 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:

sdk-platform-java/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

Lines 372 to 374 in 909bdf9

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

return false;
}

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

@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 Expand Up @@ -565,22 +587,6 @@ public void testLogDirectPathMisconfigNotOnGCE() {
InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
}

@Test
public void testLogDirectPathMisconfigNotInGDU() {
FakeLogHandler logHandler = new FakeLogHandler();
InstantiatingGrpcChannelProvider.LOG.addHandler(logHandler);
InstantiatingGrpcChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPathXds()
.setAttemptDirectPath(true)
.setAllowNonDefaultServiceAccount(true)
.setEndpoint("test.random.endpoint.com:443")
.build();
assertThat(logHandler.getAllMessages())
.contains("DirectPath will only work in the the googleapis.com Universe Domain");
InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
}

private static class FakeLogHandler extends Handler {
List<LogRecord> records = new ArrayList<>();

Expand Down