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

feat: Full Endpoint Resolution from EndpointContext #2313

Merged
merged 7 commits into from Jan 2, 2024
Merged

feat: Full Endpoint Resolution from EndpointContext #2313

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bdbaab1
feat: Full endpoint resolution
lqiu96 Dec 20, 2023
bf878b9
chore: Fix lint issues
lqiu96 Dec 20, 2023
13aa12e
chore: Address PR comments
lqiu96 Dec 28, 2023
1857882
chore: Do not set resolved universe domain for gdch
lqiu96 Dec 28, 2023
15d510e
chore: Move helper methods to the Builder class
lqiu96 Dec 28, 2023
653db8e
chore: Address PR comments
lqiu96 Dec 29, 2023
3d3d7a5
chore: Add javadocs for universe domain
lqiu96 Jan 2, 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: 12 additions & 1 deletion ...java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Expand Up @@ -295,6 +295,10 @@ 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 @@ -325,6 +329,10 @@ static boolean isOnComputeEngine() {
return false;
}

private boolean canUseDirectPathWithUniverseDomain() {
return endpoint.contains("googleapis.com");
}

@VisibleForTesting
ChannelCredentials createMtlsChannelCredentials() throws IOException, GeneralSecurityException {
if (mtlsProvider.useMtlsClientCertificate()) {
Expand Down Expand Up @@ -356,7 +364,10 @@ private ManagedChannel createSingleChannel() throws IOException {

// Check DirectPath traffic.
boolean useDirectPathXds = false;
if (isDirectPathEnabled() && isNonDefaultServiceAccountAllowed() && isOnComputeEngine()) {
if (isDirectPathEnabled()
&& isNonDefaultServiceAccountAllowed()
&& isOnComputeEngine()
&& canUseDirectPathWithUniverseDomain()) {
CallCredentials callCreds = MoreCallCredentials.from(credentials);
ChannelCredentials channelCreds =
GoogleDefaultChannelCredentials.newBuilder().callCredentials(callCreds).build();
Expand Down
19 changes: 19 additions & 0 deletions .../gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Expand Up @@ -290,6 +290,7 @@ public void testDirectPathXdsEnabled() throws IOException {
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPath(true)
.setAttemptDirectPathXds()
.setEndpoint("test.googleapis.com:443")
.build();

assertThat(provider.isDirectPathXdsEnabled()).isTrue();
Expand Down Expand Up @@ -528,6 +529,7 @@ public void testLogDirectPathMisconfigWrongCredential() {
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPathXds()
.setAttemptDirectPath(true)
.setEndpoint("test.googleapis.com:443")
.build();
assertThat(logHandler.getAllMessages())
.contains(
Expand All @@ -545,6 +547,7 @@ public void testLogDirectPathMisconfigNotOnGCE() {
.setAttemptDirectPathXds()
.setAttemptDirectPath(true)
.setAllowNonDefaultServiceAccount(true)
.setEndpoint("test.googleapis.com:443")
.build();
if (!InstantiatingGrpcChannelProvider.isOnComputeEngine()) {
assertThat(logHandler.getAllMessages())
Expand All @@ -554,6 +557,22 @@ 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
6 changes: 6 additions & 0 deletions gax-java/gax/clirr-ignored-differences.xml
View file
Open in desktop
Expand Up @@ -25,4 +25,10 @@
<className>com/google/api/gax/rpc/TransportChannelProvider</className>
<method>* getEndpoint()</method>
</difference>
<!-- Add Universe Domain to ClientContext -->
<difference>
<differenceType>7013</differenceType>
<className>com/google/api/gax/rpc/ClientContext*</className>
<method>* *UniverseDomain*(*)</method>
</difference>
</differences>
36 changes: 23 additions & 13 deletions gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java
View file
Open in desktop
Expand Up @@ -104,6 +104,9 @@ public abstract class ClientContext {
@Nullable
abstract String getServiceName();

@Nullable
public abstract String getUniverseDomain();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to expose a getter for universe domain? I guess it is for StubSettings?

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 is for the for logic that connects ClientContext <-> StubSettings. I believe it was added a while back for reasons and this getter is (ideally) only for StubSettings usage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, for every new ClientSettings, it seems that we have to duplicate it in StubSettings as well.


@Nullable
public abstract String getEndpoint();

Expand Down Expand Up @@ -157,15 +160,29 @@ public static ClientContext create(StubSettings settings) throws IOException {
final ScheduledExecutorService backgroundExecutor = backgroundExecutorProvider.getExecutor();

Credentials credentials = settings.getCredentialsProvider().getCredentials();
boolean usingGDCH = credentials instanceof GdchCredentials;
EndpointContext endpointContext =
EndpointContext.newBuilder()
.setServiceName(settings.getServiceName())
.setUniverseDomain(settings.getUniverseDomain())
.setClientSettingsEndpoint(settings.getEndpoint())
.setTransportChannelProviderEndpoint(
settings.getTransportChannelProvider().getEndpoint())
.setMtlsEndpoint(settings.getMtlsEndpoint())
.setSwitchToMtlsEndpointAllowed(settings.getSwitchToMtlsEndpointAllowed())
.setUsingGDCH(usingGDCH)
.build();
String endpoint = endpointContext.resolvedEndpoint();
String universeDomain = endpointContext.resolvedUniverseDomain();

String settingsGdchApiAudience = settings.getGdchApiAudience();
if (credentials instanceof GdchCredentials) {
if (usingGDCH) {
// We recompute the GdchCredentials with the audience
String audienceString;
if (!Strings.isNullOrEmpty(settingsGdchApiAudience)) {
audienceString = settingsGdchApiAudience;
} else if (!Strings.isNullOrEmpty(settings.getEndpoint())) {
audienceString = settings.getEndpoint();
} else if (!Strings.isNullOrEmpty(endpoint)) {
audienceString = endpoint;
Comment on lines +183 to +184
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirm this behavior is correct

Copy link
Contributor Author

@lqiu96 lqiu96 Dec 20, 2023 •
edited

Choose a reason for hiding this comment

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

The GDC-H audienceString's endpoint should match with endpoint set inside the TransportChannelProvider.

The logic is that for a GDC-H flow, the resolved endpoint will be either the clientsettings endpoint or transportchannel endpoint if set. This matches the behavior below as either the clientSettings or transportchannel's endpoint.

ClientSettings will only be set to the Transportchannel if it doesn't have an endpoint already set by the user. EndpointContext's resolvedEndpoint already takes this into consideration.

Copy link
Contributor Author

@lqiu96 lqiu96 Dec 20, 2023 •
edited

Choose a reason for hiding this comment

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

The else block is kept as the custom endpoint could be set as "" (empty string). URI.create("") does not throw an exception, so it is caught in this else block.

} else {
throw new IllegalArgumentException("Could not infer GDCH api audience from settings");
}
Expand Down Expand Up @@ -204,16 +221,6 @@ public static ClientContext create(StubSettings settings) throws IOException {
if (transportChannelProvider.needsCredentials() && credentials != null) {
transportChannelProvider = transportChannelProvider.withCredentials(credentials);
}
EndpointContext endpointContext =
EndpointContext.newBuilder()
.setServiceName(settings.getServiceName())
.setClientSettingsEndpoint(settings.getEndpoint())
.setTransportChannelProviderEndpoint(
settings.getTransportChannelProvider().getEndpoint())
.setMtlsEndpoint(settings.getMtlsEndpoint())
.setSwitchToMtlsEndpointAllowed(settings.getSwitchToMtlsEndpointAllowed())
.build();
String endpoint = endpointContext.getResolvedEndpoint();
if (transportChannelProvider.needsEndpoint()) {
transportChannelProvider = transportChannelProvider.withEndpoint(endpoint);
}
Expand Down Expand Up @@ -264,6 +271,7 @@ public static ClientContext create(StubSettings settings) throws IOException {
.setClock(clock)
.setDefaultCallContext(defaultCallContext)
.setServiceName(settings.getServiceName())
.setUniverseDomain(universeDomain)
.setEndpoint(settings.getEndpoint())
.setQuotaProjectId(settings.getQuotaProjectId())
.setStreamWatchdog(watchdog)
Expand Down Expand Up @@ -332,6 +340,8 @@ public abstract static class Builder {
// Package-Private scope for internal use only. Shared between StubSettings and ClientContext
abstract Builder setServiceName(String serviceName);

public abstract Builder setUniverseDomain(String universeDomain);

public abstract Builder setEndpoint(String endpoint);

public abstract Builder setQuotaProjectId(String QuotaProjectId);
Expand Down