feat: Full Endpoint Resolution from EndpointContext #2313

lqiu96 · 2023-12-19T19:24:11Z

Features:

Introduces the concept of Universe Domain
Endpoint + Universe Domain resolution is determined solely by the EndpointContext
Support and compatibility for GDC-H, DirectPath, and mTLS

lqiu96 · 2023-12-19T22:48:18Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

+      } else if (!Strings.isNullOrEmpty(endpoint)) {
+        audienceString = endpoint;


Confirm this behavior is correct

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.

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.

blakeli0 · 2023-12-26T16:17:48Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

+      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;


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.

Sounds good, will do.

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

Do we still need resolvedUniverseDomain?

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.

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.

blakeli0 · 2023-12-26T17:12:46Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

@@ -104,6 +104,9 @@ public abstract class ClientContext {
  @Nullable
  abstract String getServiceName();

+  @Nullable
+  public abstract String getUniverseDomain();


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

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.

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

gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

blakeli0 · 2023-12-28T22:00:47Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

+    private String determineUniverseDomain() {
+      // Do not set the universe domain for GDC-H
+      if (usingGDCH()) {
+        return null;


Can we return GDU here instead of null? And make resolvedUniverseDomain not nullable? I see that resolvedUniverseDomain is used multiple places later, it is probably good now but may easily introduce null pointer exceptions later.

The resolvedUniverseDomain is what is stored in the ClientContext and StubSettings (not the user configured universe domain). Following the GDC-H case above, it's possible to manually construct the StubSettings and ClientContext and each would construct the EndpointContext. If we set the resolvedUniverseDomain to the GDU by default, then it would fail every case as the Universe Domain can never be set for GDC-H (must be null).

--
Edit: I updated the logic so that it is non-null

blakeli0 · 2023-12-28T22:07:01Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

+        throw new IllegalArgumentException(
+            "Universe domain configuration is incompatible with GDC-H");
+      } else if (customEndpoint == null) {
+        return buildEndpointTemplate(serviceName(), GOOGLE_DEFAULT_UNIVERSE);


Can we use resolvedUniverseDomain if we default it to GDU?

Updated. Thanks!

blakeli0 · 2023-12-28T22:11:08Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

+      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;


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.

blakeli0 · 2023-12-28T22:12:38Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

@@ -104,6 +104,9 @@ public abstract class ClientContext {
  @Nullable
  abstract String getServiceName();

+  @Nullable
+  public abstract String getUniverseDomain();


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

blakeli0 · 2023-12-28T22:14:52Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

-    return new AutoValue_EndpointContext.Builder().setSwitchToMtlsEndpointAllowed(false);
-  }
+  @Nullable
+  public abstract String resolvedUniverseDomain();


Do we expect resolvedUniverseDomain to be used by anything outside of this class? If not, I think we can make the this method and the setter in the builder package private (I don't think we can make it completed private though due to AutoValue limitations).

Actually on second thought, I think you're right. I've made it package private. I originally used it as the value I set in the ClientContext, but I can just follow what we are doing with endpoint and pass the user set values for that. This way the resolvedUniverseDomain can be non-null (default to GDU).

blakeli0

LGTM. Please add Javadocs before merging.

blakeli0 · 2024-01-02T15:44:29Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

@@ -48,6 +51,9 @@ public abstract class EndpointContext {
  @Nullable
  public abstract String serviceName();

+  @Nullable


Please add some Javadocs to the getter here and setter below

Done. Thanks

sonarcloud · 2024-01-02T16:54:59Z

Quality Gate failed for 'gapic-generator-java-root'

Failed conditions

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

See analysis details on SonarCloud

sonarcloud · 2024-01-02T16:58:58Z

Quality Gate failed for 'java_showcase_integration_tests'

Failed conditions

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

See analysis details on SonarCloud

product-auto-label bot added the size: l Pull request size is large. label Dec 19, 2023

lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 19, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 19, 2023

lqiu96 commented Dec 19, 2023

View reviewed changes

lqiu96 requested a review from blakeli0 December 19, 2023 22:48

feat: Full endpoint resolution

bdbaab1

lqiu96 force-pushed the endpoint-resolution branch 2 times, most recently from 3bc8392 to bdbaab1 Compare December 20, 2023 21:02

chore: Fix lint issues

bf878b9

lqiu96 marked this pull request as ready for review December 20, 2023 21:29

lqiu96 requested a review from a team as a code owner December 20, 2023 21:29

lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 20, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 20, 2023

blakeli0 reviewed Dec 27, 2023

View reviewed changes

lqiu96 added 2 commits December 27, 2023 21:51

chore: Address PR comments

13aa12e

chore: Do not set resolved universe domain for gdch

1857882

lqiu96 requested a review from blakeli0 December 28, 2023 16:14

chore: Move helper methods to the Builder class

15d510e

blakeli0 reviewed Dec 28, 2023

View reviewed changes

chore: Address PR comments

653db8e

blakeli0 approved these changes Jan 2, 2024

View reviewed changes

chore: Add javadocs for universe domain

3d3d7a5

lqiu96 merged commit f499ced into main Jan 2, 2024
36 of 38 checks passed

lqiu96 deleted the endpoint-resolution branch January 2, 2024 17:29

release-please bot mentioned this pull request Jan 2, 2024

chore(main): release 2.31.0 #2290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Full Endpoint Resolution from EndpointContext #2313

feat: Full Endpoint Resolution from EndpointContext #2313

lqiu96 commented Dec 19, 2023

lqiu96 Dec 19, 2023

lqiu96 Dec 20, 2023 •

edited

lqiu96 Dec 20, 2023 •

edited

blakeli0 Dec 26, 2023

lqiu96 Dec 28, 2023

blakeli0 Dec 28, 2023

lqiu96 Dec 28, 2023

blakeli0 Dec 28, 2023

blakeli0 Dec 26, 2023

lqiu96 Dec 27, 2023

blakeli0 Dec 28, 2023

blakeli0 Dec 28, 2023

lqiu96 Dec 29, 2023 •

edited

blakeli0 Dec 28, 2023

lqiu96 Dec 29, 2023

blakeli0 Dec 28, 2023

blakeli0 Dec 28, 2023

blakeli0 Dec 28, 2023

lqiu96 Dec 29, 2023

blakeli0 left a comment

blakeli0 Jan 2, 2024

lqiu96 Jan 2, 2024

sonarcloud bot commented Jan 2, 2024

sonarcloud bot commented Jan 2, 2024

		} else if (!Strings.isNullOrEmpty(endpoint)) {
		audienceString = endpoint;

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

feat: Full Endpoint Resolution from EndpointContext #2313

feat: Full Endpoint Resolution from EndpointContext #2313

Conversation

lqiu96 commented Dec 19, 2023

Choose a reason for hiding this comment

lqiu96 Dec 20, 2023 • edited

Choose a reason for hiding this comment

lqiu96 Dec 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Dec 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 2, 2024

Quality Gate failed for 'gapic-generator-java-root'

sonarcloud bot commented Jan 2, 2024

Quality Gate failed for 'java_showcase_integration_tests'

lqiu96 Dec 20, 2023 •

edited

lqiu96 Dec 20, 2023 •

edited

lqiu96 Dec 29, 2023 •

edited