feat: Add Universe Domain Support #2435

lqiu96 · 2024-02-20T20:58:20Z

Universe Domain support can be done almost all inside this PR. There is an additional change required inside the google-api-client-services generator. Need to add the setter inside the {Service}.Builder implementation.

@Override
public Builder setUniverseDomain(String universeDomain) {
  return (Builder) super.setUniverseDomain(universeDomain);
}

The reason this needs to be in the generated client code is because it returns the parent's implementation returns a type of AbstractGoogleClient.Builder instead of Bigquery.Builder. Having this inside the child implementation would return the correct type (Bigquery.Builder).

Tested this locally with a local build of Bigquery Apiary client and a local build of the api-client library. Connection to the test environment works and the validation is called on each RPC invocation.

Changes required in Java-Storage and Java-Bigquery

{ApiaryClient}.Builder()
            .setRootUrl(options.getResolvedHost("{Apiary Client Service Name}"))
            .setUniverseDomain(options.getUniverseDomain())
.build()

Setting the rootUrl will now use the resolvedHost and no longer use the apiary workaround
Pass the universe domain in to the apiary client (even though setting the rootUrl will be the rootUrl used and the universe domain configuration value isn't used in resolving the endpoint, pass in the universe domain as this will allow users to call the universe domain getter).

Local Tests

Local Bigquery Apiary - Able to connect + Universe Domain Validation
Local Java-Bigquery client library - Able to connect + Universe Domain Validation

Next Steps:

PR in google-api-java-client-services: googleapis/google-api-java-client-services#19934

Requires merging in this PR and a release of a new feat (minor version bump).

blakeli0 · 2024-03-08T23:43:56Z

...api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClient.java

      this.httpRequestInitializer = httpRequestInitializer;
+      this.serviceName = parseServiceName(rootUrl);
+      this.isUserConfiguredEndpoint =


Why do we need to determine if the rootUrl is user configured based on if it ends with GDU in the Builder? Is setting this.isUserConfiguredEndpoint = true; in the setter not enough ?
Is it for the scenarios that customers may extend this class and call this Builder directly? Because I think in the public client, rootUrl is hidden. e.g. in Bigquery

Is it for the scenarios that customers may extend this class and call this Builder directly?

Yep, it's for the (hopefully) rare user use-case where users are directly extending this class and calling the builder. The apiary libraries don't expose the endpoint configurations in the Builder directly.

blakeli0 · 2024-03-08T23:49:46Z

...api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClient.java

+     * done via {@link #setRootUrl(String)}.
+     *
+     * <p>For other uses cases that touch this Builder's constructor directly, check if the rootUrl
+     * passed in references the Google Default Universe (GDU). Any rootUrl value that is not set in


I don't think this logic is correct, customers could set regional endpoints which also end with GDU, see https://cloud.google.com/storage/docs/regional-endpoints

Ah. Turns out I think this works, but for the wrong reasons. I believe we're parsing out the serviceName and then reconstructing it to build the the same endpoint later on. It's marked as a non-user configured endpoint when it should be.

I think it might be better if I change the isUserConfiguredEndpoint logic to match for a strict regex which should cover the regional endpoints edge case.

blakeli0 · 2024-03-11T14:21:52Z

Changes required in Java-Storage and Java-Bigquery
{ApiaryClient}.Builder()
            .setRootUrl(options.getResolvedHost("{Apiary Client Service Name}"))
            .setUniverseDomain(options.getUniverseDomain())
.build()
Setting the rootUrl will now use the resolvedHost and no longer use the apiary workaround

Pass the universe domain in to the apiary client (even though setting the rootUrl will be the rootUrl used and the universe domain configuration value isn't used in resolving the endpoint, pass in the universe domain as this will allow users to call the universe domain getter).

Why they have to set both rootUrl and universeDomain? I think setting universeDomain alone should be good enough?

lqiu96 · 2024-03-11T15:18:18Z

Changes required in Java-Storage and Java-Bigquery
{ApiaryClient}.Builder()
            .setRootUrl(options.getResolvedHost("{Apiary Client Service Name}"))
            .setUniverseDomain(options.getUniverseDomain())
.build()
Setting the rootUrl will now use the resolvedHost and no longer use the apiary workaround

Pass the universe domain in to the apiary client (even though setting the rootUrl will be the rootUrl used and the universe domain configuration value isn't used in resolving the endpoint, pass in the universe domain as this will allow users to call the universe domain getter).
Why they have to set both rootUrl and universeDomain? I think setting universeDomain alone should be good enough?

The apiary wrapped libraries (java-storage and java-bigquery) may have some user configurations where they change the rootUrl value via settings there (for emulators, local endpoint, etc). We'll need those configurations so that we won't override them when the apiary library tries to resolve the endpoint in the code here.

blakeli0 · 2024-03-11T19:56:38Z

Changes required in Java-Storage and Java-Bigquery
{ApiaryClient}.Builder()
            .setRootUrl(options.getResolvedHost("{Apiary Client Service Name}"))
            .setUniverseDomain(options.getUniverseDomain())
.build()
Setting the rootUrl will now use the resolvedHost and no longer use the apiary workaround

Pass the universe domain in to the apiary client (even though setting the rootUrl will be the rootUrl used and the universe domain configuration value isn't used in resolving the endpoint, pass in the universe domain as this will allow users to call the universe domain getter).
Why they have to set both rootUrl and universeDomain? I think setting universeDomain alone should be good enough?
The apiary wrapped libraries (java-storage and java-bigquery) may have some user configurations where they change the rootUrl value via settings there (for emulators, local endpoint, etc). We'll need those configurations so that we won't override them when the apiary library tries to resolve the endpoint in the code here.

I see. If that's the case, I would say we don't have to pass universeDomain, setting the rootUrl should be good enough. Because

The Apiary client is hidden from customers in Bigquery/Storage, even if customers need to, they would use ServiceOptions.getUniverseDomain().
This is less confusing to Bigquery/Storage developers and customers. As they may think it is required to pass both values.
This requires no change in Bigquery/Storage.

lqiu96 · 2024-03-11T20:29:39Z

I believe you're right regarding not needing the universe domain to resolve the host. I just realized that the universe domain is also needed as we need it to validate the universe domain. If not passed to the Apiary client, the resolved universe domain will always default to googleapis.com. Passing in the configured universe domain value will use that for validation.

Previously, the handwritten libraries were validating the universe domain, but since we're moving it the apiary client, I believe we'll need to pass the configured universe domain value here.

blakeli0 · 2024-03-11T21:44:39Z

Previously, the handwritten libraries were validating the universe domain, but since we're moving it the apiary client, I believe we'll need to pass the configured universe domain value here.

It maybe a scenario we missed before, do we still want to validate the universe domain if the endpoint/rootUrl is provided? I think we should not because the endpoint configuration would override the universe domain configuration if both are configured. Otherwise it would be confusing if we pass both to the client, use one for validation but use another one for making calls.

blakeli0 · 2024-03-11T21:47:42Z

...api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClient.java

+      expectedUniverseDomain = credentials.getUniverseDomain();
+    }
+    if (!expectedUniverseDomain.equals(getUniverseDomain())) {
+      throw new IOException(


Why is it an IOException? It is a comparison that does not involve IO operations, hence I think it should be a runtime exception.

Ah right. Will change.

blakeli0 · 2024-03-11T22:16:29Z

...api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClient.java

+     * <p>The roolUrl from the Discovery Docs will always follow the format of
+     * https://{serviceName}(.mtls).googleapis.com/
+     */
+    private String parseServiceName(String rootUrl) {


I think we can use the regex to extract the service name now.

Sounds good, will update,

lqiu96 · 2024-03-11T22:23:11Z

Previously, the handwritten libraries were validating the universe domain, but since we're moving it the apiary client, I believe we'll need to pass the configured universe domain value here.

It maybe a scenario we missed before, do we still want to validate the universe domain if the endpoint/rootUrl is provided? I think we should not because the endpoint configuration would override the universe domain configuration if both are configured. Otherwise it would be confusing if we pass both to the client, use one for validation but use another one for making calls.

It is a bit of weird scenario especially given Apiary's non-normal use case. I think we should be validating the universe domain even if we are providing the "correct" endpoint from ServiceOptions.

Off the top of my head, I can think of these scenarios in which I think validation is needed required:

User sets custom rootUrl (to the correct non-GDU endpoint) and no universeDomain. This will create a connection to the right place and the universe domain is resolved to GDU. They supply the correct Credentials that points to non-GDU. While this does technically work as the Credentials is valid, this does not share the same functionality with the other client libs. GAPICs would throw an exception that the explicitly configured universe domain does not match before the call goes through.
User sets custom rootUrl (to the correct non-GDU endpoint) and a universeDomain. However, they supply the incorrect Credentials that points to GDU. I believe this would fail once the RPC hits the server, but the call should not go through (it should fail during validation). GAPICs would throw an exception that the explicitly configured universe domain does not match.

Otherwise it would be confusing if we pass both to the client, use one for validation but use another one for making calls.

Agreed. Ideally the endpoint should be resolved inside the Apiary client itself for every use case. It's only a special case for these two handwritten apiary wrappers as Java-Core is used to resolved the endpoint.

I chose this path because Java-Core sets the the host value to be the value of DEFUALT_HOST (https://github.com/googleapis/sdk-platform-java/blob/4b44a7851dc1d4fd2ac21a54df6c24db5625223c/java-core/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java#L86) if nothing is configured. One option that I thought about would be to have the two libraries do some additional logic to filter this out.
i.e.

String host = options.getHost().equals(DEFAULT_HOST) ? null : options.getHost();
{ApiaryClient}.Builder()
            .setRootUrl(host)
            .setUniverseDomain(options.getUniverseDomain())
.build()

I didn't go with this path since it's additional work for the two handwritten libraries.

blakeli0 · 2024-03-11T22:43:17Z

...api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClient.java

+     * discovery doc. Follows the format of `https://{serviceName}(.mtls).googleapis.com/`
+     */
+    Pattern defaultEndpointRegex =
+        Pattern.compile("https://[a-zA-Z]*(\\.mtls)?\\.googleapis.com/?");


I did a few quick test but I don't think this regex works, what worked for me is
https:\/\/[a-zA-Z]*(\.mtls)?\.googleapis.com\/?
Can you please make sure the regex is correct and add a few tests just for the regex?

Oh huh, I did add unit tests for this regex and it did pass:

google-api-java-client/google-api-client/src/test/java/com/google/api/client/googleapis/services/AbstractGoogleClientTest.java

Lines 328 to 357 in 1cfd93a

@Test

public void testIsUserSetEndpoint_mtlsRootUrl() {

AbstractGoogleClient.Builder clientBuilder =

new MockGoogleClient.Builder(

TRANSPORT, "https://test.mtls.googleapis.com/", "", JSON_OBJECT_PARSER, null)

.setApplicationName("Test Application");

assertFalse(clientBuilder.isUserConfiguredEndpoint);

}

@Test

public void testIsUserSetEndpoint_nonGDURootUrl() {

AbstractGoogleClient.Builder clientBuilder =

new MockGoogleClient.Builder(

TRANSPORT, "https://test.random.com/", "", JSON_OBJECT_PARSER, null)

.setApplicationName("Test Application");

assertTrue(clientBuilder.isUserConfiguredEndpoint);

}

@Test

public void testIsUserSetEndpoint_regionalEndpoint() {

AbstractGoogleClient.Builder clientBuilder =

new MockGoogleClient.Builder(

TRANSPORT,

"https://us-east-4.coolservice.googleapis.com/",

"",

JSON_OBJECT_PARSER,

null)

.setApplicationName("Test Application");

assertTrue(clientBuilder.isUserConfiguredEndpoint);

}

. Do you have the values you tested with? I may have tested incorrectly.

blakeli0 · 2024-03-11T23:05:24Z

...api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClient.java

      this.httpRequestInitializer = httpRequestInitializer;
+      this.serviceName = parseServiceName(rootUrl);
+      this.isUserConfiguredEndpoint = !defaultEndpointRegex.matcher(this.rootUrl).matches();


I'm still not convinced that we need this. Considering the following scenarios:

rootUrl is set to non-GDU, serviceName would be parsed to null, hence we would use rootUrl as is in determiningEndpoint().

rootUrl is set to GDU, isUserConfiguredEndpoint would be false anyway, and we would re-create the rootUrl with serviceName and universeDomain.

This does work, but I think it has misleading logic inside for certain edge cases (i.e. regional endpoints). For example, when using a regional endpoint in speech: https://eu-speech.googleapis.com and passing that into the constructor, the logic in this PR would work. But I believe it sets the serviceName as eu-speech, when it should just be speech, and builds the endpoint, when it probably shouldn't because it is a custom endpoint.

The more I think about this, the more I'm leaning towards fixing the serviceName parsing logic the same way we did in GAPICs. The Apiary generator probably should be the one parsing the discovery doc and generating this value as a constant inside the library. That way we don't have worry about parsing the serviceName based on user params or user set values and the Apiary would have this value by default. I probably should have done that to begin with, but I didn't account for regional endpoints and I thought this way would be fine.

blakeli0 · 2024-03-13T17:23:37Z

...api-client/src/main/java/com/google/api/client/googleapis/services/AbstractGoogleClient.java

+   *     Domain in the Credentials
+   */
+  public void validateUniverseDomain() throws IOException {
+    if (httpRequestInitializer instanceof HttpCredentialsAdapter) {


nit: Change this to

if (!(httpRequestInitializer instanceof HttpCredentialsAdapter)) { return; }

for better readability.

blakeli0 · 2024-03-13T17:53:58Z

pom.xml

@@ -499,6 +513,7 @@
    <project.httpclient.version>4.5.14</project.httpclient.version>
    <project.commons-codec.version>1.16.0</project.commons-codec.version>
    <project.oauth.version>1.35.0</project.oauth.version>
+    <project.auth.version>1.22.0</project.auth.version>


We might discussed this in the past, but I'm a little surprised that this project is not dependent on auth library yet. We probably can not avoid it because we need to access the Credentials object for validation, but let's be more careful here as it may introduce dependency problem for customers. @suztomo Do you have any concerns regarding this new auth dependency?

This does not create a circular dependency. Looks good to me.

lqiu96 added 2 commits February 16, 2024 18:02

feat: Implement Universe Domain Support

b49eeca

chore: Clean up auth dependencies

e15caa2

product-auto-label bot added the size: m Pull request size is medium. label Feb 20, 2024

lqiu96 added 6 commits February 20, 2024 16:57

chore: Resolve checkstyle issues

4544811

chore: Determine service name from url

3b8a90a

chore: Return null for invalid rootUrl

544ee32

chore: Add javadocs for Universe Domain changes in AbstractGoogleClient

2ba2504

chore: Address checkstyle issues

25c9bc8

chore: Add tests for AbstractGoogleClient

13418de

product-auto-label bot added size: xl Pull request size is extra large. and removed size: m Pull request size is medium. labels Feb 29, 2024

lqiu96 added 2 commits February 29, 2024 17:35

chore: Fix tests

b8b233a

chore: Update docs for AbstractGoogleClient

dfd07a7

product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Mar 4, 2024

lqiu96 added 8 commits March 4, 2024 17:28

chore: validateUniverseDomain does not return bool

a9a64db

chore: Move validateUniverseDomain() to parent impl

0f905c0

chore: Update javadocs

b2088be

chore: Add Env Var tests for GOOGLE_CLOUD_UNIVERSE_DOMAIN

fd1182a

chore: Fix lint issues

3edfe45

chore: Fix env var tests

bba201a

chore: Update logic for isUserConfiguredEndpoint

7a0b3fa

chore: Throw IOException on validateUniverseDomain

83612b6

lqiu96 requested a review from blakeli0 March 6, 2024 21:52

lqiu96 marked this pull request as ready for review March 6, 2024 21:52

lqiu96 requested a review from a team as a code owner March 6, 2024 21:52

blakeli0 reviewed Mar 8, 2024

View reviewed changes

blakeli0 mentioned this pull request Mar 11, 2024

feat: Add Universe Domain Support for Apiary Clients googleapis/google-api-java-client-services#19934

Merged

chore: Address PR comments

1cfd93a

blakeli0 reviewed Mar 11, 2024

View reviewed changes

lqiu96 added 4 commits March 11, 2024 23:26

chore: Address PR comments

3fcb3c8

chore: Address PR comments

8d11d10

chore: Address PR comments

46781b1

chore: Validate on HttpCredentialsAdapter

9c8084c

blakeli0 reviewed Mar 13, 2024

View reviewed changes

chore: Address PR comments

94c42d1

blakeli0 approved these changes Mar 13, 2024

View reviewed changes

lqiu96 merged commit 4adfed9 into main Mar 13, 2024
16 checks passed

lqiu96 deleted the apiary-universe-domain branch March 13, 2024 19:23

release-please bot mentioned this pull request Mar 13, 2024

chore(main): release 2.4.0 #2439

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Universe Domain Support #2435

feat: Add Universe Domain Support #2435

lqiu96 commented Feb 20, 2024 •

edited

blakeli0 Mar 8, 2024

lqiu96 Mar 11, 2024

blakeli0 Mar 8, 2024

lqiu96 Mar 11, 2024 •

edited

blakeli0 commented Mar 11, 2024

Changes required in Java-Storage and Java-Bigquery

lqiu96 commented Mar 11, 2024 •

edited

Changes required in Java-Storage and Java-Bigquery

blakeli0 commented Mar 11, 2024

Changes required in Java-Storage and Java-Bigquery

lqiu96 commented Mar 11, 2024 •

edited

blakeli0 commented Mar 11, 2024

blakeli0 Mar 11, 2024

lqiu96 Mar 11, 2024

blakeli0 Mar 11, 2024

lqiu96 Mar 11, 2024

lqiu96 commented Mar 11, 2024

blakeli0 Mar 11, 2024

lqiu96 Mar 12, 2024

blakeli0 Mar 11, 2024

lqiu96 Mar 12, 2024 •

edited

blakeli0 Mar 13, 2024

blakeli0 Mar 13, 2024

suztomo Mar 13, 2024

	@Test
	public void testIsUserSetEndpoint_mtlsRootUrl() {
	AbstractGoogleClient.Builder clientBuilder =
	new MockGoogleClient.Builder(
	TRANSPORT, "https://test.mtls.googleapis.com/", "", JSON_OBJECT_PARSER, null)
	.setApplicationName("Test Application");
	assertFalse(clientBuilder.isUserConfiguredEndpoint);
	}

	@Test
	public void testIsUserSetEndpoint_nonGDURootUrl() {
	AbstractGoogleClient.Builder clientBuilder =
	new MockGoogleClient.Builder(
	TRANSPORT, "https://test.random.com/", "", JSON_OBJECT_PARSER, null)
	.setApplicationName("Test Application");
	assertTrue(clientBuilder.isUserConfiguredEndpoint);
	}

	@Test
	public void testIsUserSetEndpoint_regionalEndpoint() {
	AbstractGoogleClient.Builder clientBuilder =
	new MockGoogleClient.Builder(
	TRANSPORT,
	"https://us-east-4.coolservice.googleapis.com/",
	"",
	JSON_OBJECT_PARSER,
	null)
	.setApplicationName("Test Application");
	assertTrue(clientBuilder.isUserConfiguredEndpoint);
	}

feat: Add Universe Domain Support #2435

feat: Add Universe Domain Support #2435

Conversation

lqiu96 commented Feb 20, 2024 • edited

Changes required in Java-Storage and Java-Bigquery

Local Tests

Next Steps:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Mar 11, 2024 • edited

Choose a reason for hiding this comment

blakeli0 commented Mar 11, 2024

Changes required in Java-Storage and Java-Bigquery

lqiu96 commented Mar 11, 2024 • edited

Changes required in Java-Storage and Java-Bigquery

blakeli0 commented Mar 11, 2024

Changes required in Java-Storage and Java-Bigquery

lqiu96 commented Mar 11, 2024 • edited

blakeli0 commented Mar 11, 2024

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 commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Mar 12, 2024 • 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

lqiu96 commented Feb 20, 2024 •

edited

lqiu96 Mar 11, 2024 •

edited

lqiu96 commented Mar 11, 2024 •

edited

lqiu96 commented Mar 11, 2024 •

edited

lqiu96 Mar 12, 2024 •

edited