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
+489 −89
Merged

feat: Full Endpoint Resolution from EndpointContext #2313

Show file tree
Hide file tree
Changes from 2 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.getResolvedEndpoint();
String universeDomain = endpointContext.getResolvedUniverseDomain();

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
82 changes: 75 additions & 7 deletions gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java
View file
Open in desktop
Expand Up @@ -33,13 +33,18 @@
import com.google.api.gax.rpc.mtls.MtlsProvider;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import java.io.IOException;
import javax.annotation.Nullable;

/** Contains the fields required to resolve the endpoint */
/** Contains the fields required to resolve the endpoint and Universe Domain */
@InternalApi
@AutoValue
public abstract class EndpointContext {
// Default to port 443 for HTTPS. Using HTTP requires explicitly setting the endpoint
private static final String ENDPOINT_TEMPLATE = "SERVICE_NAME.UNIVERSE_DOMAIN:443";
static final String GOOGLE_DEFAULT_UNIVERSE = "googleapis.com";
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved

/**
* ServiceName is host URI for Google Cloud Services. It follows the format of
* `{ServiceName}.googleapis.com`. For example, speech.googleapis.com would have a ServiceName of
Expand All @@ -48,6 +53,9 @@ public abstract class EndpointContext {
@Nullable
public abstract String serviceName();

@Nullable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some Javadocs to the getter here and setter below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

public abstract String universeDomain();

/**
* ClientSettingsEndpoint is the endpoint value set via the ClientSettings/StubSettings classes.
*/
Expand All @@ -69,28 +77,76 @@ public abstract class EndpointContext {
@Nullable
public abstract MtlsProvider mtlsProvider();

public abstract boolean usingGDCH();

public abstract Builder toBuilder();

private String resolvedEndpoint;
private String resolvedUniverseDomain;

public static Builder newBuilder() {
return new AutoValue_EndpointContext.Builder().setSwitchToMtlsEndpointAllowed(false);
return new AutoValue_EndpointContext.Builder()
.setSwitchToMtlsEndpointAllowed(false)
.setUsingGDCH(false);
}

/** Determines the fully resolved endpoint and universe domain values */
@VisibleForTesting
void determineEndpoint() throws IOException {
MtlsProvider mtlsProvider = mtlsProvider() == null ? new MtlsProvider() : mtlsProvider();
// TransportChannelProvider's endpoint will override the ClientSettings' endpoint
String customEndpoint =
transportChannelProviderEndpoint() == null
? clientSettingsEndpoint()
: transportChannelProviderEndpoint();
resolvedEndpoint =
mtlsEndpointResolver(
customEndpoint, mtlsEndpoint(), switchToMtlsEndpointAllowed(), mtlsProvider);

// GDC-H has a separate flow
if (usingGDCH()) {
determineGDCHEndpoint(customEndpoint);
return;
}
// Check for "" (empty string)
if (universeDomain() != null && universeDomain().isEmpty()) {
throw new IllegalArgumentException("The universe domain value cannot be empty.");
}
// Universe Domain defaults to the GDU if it's not provided by the user.
resolvedUniverseDomain = universeDomain() != null ? universeDomain() : GOOGLE_DEFAULT_UNIVERSE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we default the universe domain to GDU instead of introducing a new field resolvedUniverseDomain? In addition, can we move the universe domain resolution logic to build()? The validation above can be done there as well. I know this method is already being called from build(), but determining universe domain feels like a separate step that should be done before determining endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we default the universe domain to GDU instead of introducing a new field resolvedUniverseDomain?

Do we still need resolvedUniverseDomain?

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 planned to remove it and use GDU by default, but I think there is a small edge case. Found it when running the IT GDCH tests, specifically regarding:

sdk-platform-java/showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITGdch.java

Lines 189 to 192 in 3d885ba

settings = settings.toBuilder().setGdchApiAudience(testAudience).build();
context = ClientContext.create(settings);
stubSettings = EchoStubSettings.newBuilder(context).build();
client = EchoClient.create(stubSettings.createStub());
.

L190's ClientContext.create(...) creates an EndpointContext. Nothing is passed in via the universeDomain getter as it's a GDC-H flow, but the universe domain would be set to GDU by default.

L192's createStub() will create the Stub and it takes the params from the ClientContext (which has the GDU as the universe domain) and it will fail the GDC-H logic block.

I'm not sure the way that the IT GDC-H tests are set is makes sense (or if I'm missing something), specifically regarding explicitly creating the ClientSettings -> ClientContext -> StubSettings steps. But I think it shows that doing this is possible by the user.

I learned towards keeping the resolvedUniverseDomain so that the user can differentiate between what the user configuration and what the resolved universe domain is (null, GDU, user configured). This would possibly be done in a future enhancement as a getter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I learned towards keeping the resolvedUniverseDomain so that the user can differentiate between what the user configuration and what the resolved universe domain is (null, GDU, user configured)

Makes sense. Please see my new comments regarding resolvedUniverseDomain, I think it's better to make it not nullable, but maybe there is a edge case preventing us from doing so.


if (Strings.isNullOrEmpty(customEndpoint)) {
customEndpoint = buildEndpointTemplate(serviceName(), resolvedUniverseDomain);
resolvedEndpoint =
mtlsEndpointResolver(
customEndpoint, mtlsEndpoint(), switchToMtlsEndpointAllowed(), mtlsProvider);
// Check if mTLS is configured with non-GDU
if (resolvedEndpoint.equals(mtlsEndpoint())
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
&& !resolvedUniverseDomain.equals(GOOGLE_DEFAULT_UNIVERSE)) {
throw new IllegalArgumentException(
"mTLS is not supported in any universe other than googleapis.com");
}
} else {
resolvedEndpoint = customEndpoint;
}
}

// GDC-H has no concept of Universe Domain. Do not set the resolvedUniverseDomain value
private void determineGDCHEndpoint(String customEndpoint) {
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
if (universeDomain() != null) {
throw new IllegalArgumentException(
"Universe domain configuration is incompatible with GDC-H");
} else if (customEndpoint == null) {
resolvedEndpoint = buildEndpointTemplate(serviceName(), GOOGLE_DEFAULT_UNIVERSE);
} else {
resolvedEndpoint = customEndpoint;
}
}

// Construct the endpoint with the template
private String buildEndpointTemplate(String serviceName, String resolvedUniverseDomain) {
return ENDPOINT_TEMPLATE
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
.replace("SERVICE_NAME", serviceName)
.replace("UNIVERSE_DOMAIN", resolvedUniverseDomain);
}

// This takes in parameters because determineEndpoint()'s logic will be updated
// to pass custom values in.
// Follows https://google.aip.dev/auth/4114 for resolving the endpoint
@VisibleForTesting
String mtlsEndpointResolver(
Expand Down Expand Up @@ -123,6 +179,14 @@ public String getResolvedEndpoint() {
return resolvedEndpoint;
}

/**
* The resolved Universe Domain is the computed Universe Domain after accounting for the custom
* Universe Domain
*/
public String getResolvedUniverseDomain() {
return resolvedUniverseDomain;
}

@AutoValue.Builder
public abstract static class Builder {
/**
Expand All @@ -132,6 +196,8 @@ public abstract static class Builder {
*/
public abstract Builder setServiceName(String serviceName);

public abstract Builder setUniverseDomain(String universeDomain);

/**
* ClientSettingsEndpoint is the endpoint value set via the ClientSettings/StubSettings classes.
*/
Expand All @@ -149,6 +215,8 @@ public abstract static class Builder {

public abstract Builder setMtlsProvider(MtlsProvider mtlsProvider);

public abstract Builder setUsingGDCH(boolean usingGDCH);

abstract EndpointContext autoBuild();

public EndpointContext build() throws IOException {
Expand Down