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.

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

feat: Add Universe Domain Support #2435

Merged
merged 24 commits into from Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b49eeca
feat: Implement Universe Domain Support
lqiu96 Feb 16, 2024
e15caa2
chore: Clean up auth dependencies
lqiu96 Feb 20, 2024
4544811
chore: Resolve checkstyle issues
lqiu96 Feb 20, 2024
3b8a90a
chore: Determine service name from url
lqiu96 Feb 20, 2024
544ee32
chore: Return null for invalid rootUrl
lqiu96 Feb 22, 2024
2ba2504
chore: Add javadocs for Universe Domain changes in AbstractGoogleClient
lqiu96 Feb 29, 2024
25c9bc8
chore: Address checkstyle issues
lqiu96 Feb 29, 2024
13418de
chore: Add tests for AbstractGoogleClient
lqiu96 Feb 29, 2024
b8b233a
chore: Fix tests
lqiu96 Feb 29, 2024
dfd07a7
chore: Update docs for AbstractGoogleClient
lqiu96 Mar 4, 2024
a9a64db
chore: validateUniverseDomain does not return bool
lqiu96 Mar 4, 2024
0f905c0
chore: Move validateUniverseDomain() to parent impl
lqiu96 Mar 4, 2024
b2088be
chore: Update javadocs
lqiu96 Mar 5, 2024
fd1182a
chore: Add Env Var tests for GOOGLE_CLOUD_UNIVERSE_DOMAIN
lqiu96 Mar 6, 2024
3edfe45
chore: Fix lint issues
lqiu96 Mar 6, 2024
bba201a
chore: Fix env var tests
lqiu96 Mar 6, 2024
7a0b3fa
chore: Update logic for isUserConfiguredEndpoint
lqiu96 Mar 6, 2024
83612b6
chore: Throw IOException on validateUniverseDomain
lqiu96 Mar 6, 2024
1cfd93a
chore: Address PR comments
lqiu96 Mar 11, 2024
3fcb3c8
chore: Address PR comments
lqiu96 Mar 12, 2024
8d11d10
chore: Address PR comments
lqiu96 Mar 12, 2024
46781b1
chore: Address PR comments
lqiu96 Mar 12, 2024
9c8084c
chore: Validate on HttpCredentialsAdapter
lqiu96 Mar 12, 2024
94c42d1
chore: Address PR comments
lqiu96 Mar 13, 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
7 changes: 7 additions & 0 deletions .github/workflows/ci.yaml
Expand Up @@ -20,6 +20,13 @@ jobs:
- run: .kokoro/build.sh
env:
JOB_TYPE: test
# The `envVarTest` profile runs tests that require an environment variable
- name: Env Var Tests
run: |
mvn test -B -ntp -Dclirr.skip=true -Denforcer.skip=true -PenvVarTest
# Set the Env Var for this step only
env:
GOOGLE_CLOUD_UNIVERSE_DOMAIN: random.com
windows:
runs-on: windows-latest
steps:
Expand Down
39 changes: 38 additions & 1 deletion google-api-client/pom.xml
Expand Up @@ -111,6 +111,14 @@
<usedDependencies>commons-codec:commons-codec</usedDependencies>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<!-- These tests require an Env Var to be set. Use -PenvVarTest to ONLY run these tests -->
<test>!AbstractGoogleClientTest#testGoogleClientBuilder_noCustomUniverseDomain_universeDomainEnvVar+testGoogleClientBuilder_customUniverseDomain_universeDomainEnvVar</test>
</configuration>
</plugin>
</plugins>

<resources>
Expand All @@ -135,6 +143,14 @@
<groupId>com.google.oauth-client</groupId>
<artifactId>google-oauth-client</artifactId>
</dependency>
<dependency>
<groupId>com.google.auth</groupId>
<artifactId>google-auth-library-credentials</artifactId>
</dependency>
<dependency>
<groupId>com.google.auth</groupId>
<artifactId>google-auth-library-oauth2-http</artifactId>
</dependency>
<dependency>
<groupId>com.google.http-client</groupId>
<artifactId>google-http-client-gson</artifactId>
Expand Down Expand Up @@ -179,6 +195,27 @@
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<profiles>
<profile>
<id>envVarTest</id>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<test>AbstractGoogleClientTest#testGoogleClientBuilder_noCustomUniverseDomain_universeDomainEnvVar+testGoogleClientBuilder_customUniverseDomain_universeDomainEnvVar</test>
</configuration>
</plugin>
</plugins>
</build>
</profile>
</profiles>
</project>
Expand Up @@ -20,8 +20,12 @@
import com.google.api.client.util.ObjectParser;
import com.google.api.client.util.Preconditions;
import com.google.api.client.util.Strings;
import com.google.auth.Credentials;
import com.google.auth.http.HttpCredentialsAdapter;
import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
import java.util.logging.Logger;
import java.util.regex.Pattern;

/**
* Abstract thread-safe Google client.
Expand All @@ -33,6 +37,8 @@ public abstract class AbstractGoogleClient {

private static final Logger logger = Logger.getLogger(AbstractGoogleClient.class.getName());

private static final String GOOGLE_CLOUD_UNIVERSE_DOMAIN = "GOOGLE_CLOUD_UNIVERSE_DOMAIN";

/** The request factory for connections to the server. */
private final HttpRequestFactory requestFactory;

Expand Down Expand Up @@ -68,13 +74,18 @@ public abstract class AbstractGoogleClient {
/** Whether discovery required parameter checks should be suppressed. */
private final boolean suppressRequiredParameterChecks;

private final String universeDomain;

private final HttpRequestInitializer httpRequestInitializer;

/**
* @param builder builder
* @since 1.14
*/
protected AbstractGoogleClient(Builder builder) {
googleClientRequestInitializer = builder.googleClientRequestInitializer;
rootUrl = normalizeRootUrl(builder.rootUrl);
universeDomain = determineUniverseDomain(builder);
rootUrl = normalizeRootUrl(determineEndpoint(builder));
servicePath = normalizeServicePath(builder.servicePath);
batchPath = builder.batchPath;
if (Strings.isNullOrEmpty(builder.applicationName)) {
Expand All @@ -88,6 +99,74 @@ protected AbstractGoogleClient(Builder builder) {
objectParser = builder.objectParser;
suppressPatternChecks = builder.suppressPatternChecks;
suppressRequiredParameterChecks = builder.suppressRequiredParameterChecks;
httpRequestInitializer = builder.httpRequestInitializer;
}

/**
* Resolve the Universe Domain to be used when resolving the endpoint. The logic for resolving the
* universe domain is the following order: 1. Use the user configured value is set, 2. Use the
* Universe Domain Env Var if set, 3. Default to the Google Default Universe
*/
private String determineUniverseDomain(Builder builder) {
String resolvedUniverseDomain = builder.universeDomain;
if (resolvedUniverseDomain == null) {
resolvedUniverseDomain = System.getenv(GOOGLE_CLOUD_UNIVERSE_DOMAIN);
}
return resolvedUniverseDomain == null
? Credentials.GOOGLE_DEFAULT_UNIVERSE
: resolvedUniverseDomain;
}

/**
* Resolve the endpoint based on user configurations. If the user has configured a custom rootUrl,
* use that value. Otherwise, construct the endpoint based on the serviceName and the
* universeDomain.
*/
private String determineEndpoint(Builder builder) {
boolean mtlsEnabled = builder.rootUrl.contains(".mtls.");
// mTLS configurations is not compatible with anything other than the GDU
if (mtlsEnabled && !universeDomain.equals(Credentials.GOOGLE_DEFAULT_UNIVERSE)) {
throw new IllegalArgumentException(
"mTLS is not supported in any universe other than googleapis.com");
}
// If the serviceName is null, we cannot construct a valid resolved endpoint. Simply return
// the rootUrl as this was custom configuration passed in.
if (builder.isUserConfiguredEndpoint || builder.serviceName == null) {
return builder.rootUrl;
}
if (mtlsEnabled) {
return "https://" + builder.serviceName + ".mtls." + universeDomain + "/";
}
return "https://" + builder.serviceName + "." + universeDomain + "/";
}

/**
* Check that the User configured universe domain matches the Credentials' universe domain. This
* uses the HttpRequestInitializer to get the Credentials and is enforced that the
* HttpRequestInitializer is of the {@see <a
* href="https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/http/HttpCredentialsAdapter.java">HttpCredentialsAdapter</a>}
* from the google-auth-library. If the HttpRequestInitializer is not used, the configured
* Universe Domain is validated against the Google Default Universe (GDU): `googleapis.com`.
*
* @throws IOException if the configured Universe Domain does not match the Universe Domain in the
* Credentials or there is an error reading the Universe Domain from the credentials
*/
public void validateUniverseDomain() throws IOException {
String expectedUniverseDomain;
if (!(httpRequestInitializer instanceof HttpCredentialsAdapter)) {
expectedUniverseDomain = Credentials.GOOGLE_DEFAULT_UNIVERSE;
} else {
Credentials credentials = ((HttpCredentialsAdapter) httpRequestInitializer).getCredentials();
expectedUniverseDomain = credentials.getUniverseDomain();
}
if (!expectedUniverseDomain.equals(getUniverseDomain())) {
throw new IOException(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right. Will change.

String.format(
"The configured universe domain (%s) does not match the universe domain found"
+ " in the credentials (%s). If you haven't configured the universe domain"
+ " explicitly, `googleapis.com` is the default.",
getUniverseDomain(), expectedUniverseDomain));
}
}

/**
Expand Down Expand Up @@ -139,6 +218,18 @@ public final GoogleClientRequestInitializer getGoogleClientRequestInitializer()
return googleClientRequestInitializer;
}

/**
* Universe Domain is the domain for Google Cloud Services. It follows the format of
* `{ServiceName}.{UniverseDomain}`. For example, speech.googleapis.com would have a Universe
* Domain value of `googleapis.com` and cloudasset.test.com would have a Universe Domain of
* `test.com`. If this value is not set, this will default to `googleapis.com`.
*
* @return The configured Universe Domain or the Google Default Universe (googleapis.com)
*/
public final String getUniverseDomain() {
return universeDomain;
}

/**
* Returns the object parser or {@code null} for none.
*
Expand Down Expand Up @@ -173,6 +264,7 @@ public ObjectParser getObjectParser() {
* @param httpClientRequest Google client request type
*/
protected void initialize(AbstractGoogleClientRequest<?> httpClientRequest) throws IOException {
validateUniverseDomain();
if (getGoogleClientRequestInitializer() != null) {
getGoogleClientRequestInitializer().initialize(httpClientRequest);
}
Expand Down Expand Up @@ -311,6 +403,33 @@ public abstract static class Builder {
/** Whether discovery required parameter checks should be suppressed. */
boolean suppressRequiredParameterChecks;

/** User configured Universe Domain. Defaults to `googleapis.com`. */
String universeDomain;

/**
* Regex pattern to check if the URL passed in matches the default endpoint confgured from a
* discovery doc. Follows the format of `https://{serviceName}(.mtls).googleapis.com/`
*/
Pattern defaultEndpointRegex =
Pattern.compile("https://[a-zA-Z]*(\\.mtls)?\\.googleapis.com/?");
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@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.


/**
* Whether the user has configured an endpoint via {@link #setRootUrl(String)}. This is added in
* because the rootUrl is set in the Builder's constructor. ,
*
* <p>Apiary clients don't allow user configurations to this Builder's constructor, so this
* would be set to false by default for Apiary libraries. User configuration to the rootUrl is
* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@lqiu96 lqiu96 Mar 11, 2024

Choose a reason for hiding this comment

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

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.

* the GDU is a user configured endpoint.
*/
boolean isUserConfiguredEndpoint;

/** The parsed serviceName value from the rootUrl from the Discovery Doc. */
String serviceName;

/**
* Returns an instance of a new builder.
*
Expand All @@ -328,9 +447,34 @@ protected Builder(
HttpRequestInitializer httpRequestInitializer) {
this.transport = Preconditions.checkNotNull(transport);
this.objectParser = objectParser;
setRootUrl(rootUrl);
setServicePath(servicePath);
this.rootUrl = normalizeRootUrl(rootUrl);
this.servicePath = normalizeServicePath(servicePath);
this.httpRequestInitializer = httpRequestInitializer;
this.serviceName = parseServiceName(rootUrl);
this.isUserConfiguredEndpoint = !defaultEndpointRegex.matcher(this.rootUrl).matches();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. rootUrl is set to non-GDU, serviceName would be parsed to null, hence we would use rootUrl as is in determiningEndpoint().
  2. rootUrl is set to GDU, isUserConfiguredEndpoint would be false anyway, and we would re-create the rootUrl with serviceName and universeDomain.

Copy link
Contributor Author

@lqiu96 lqiu96 Mar 12, 2024

Choose a reason for hiding this comment

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

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.

}

/**
* This is intended to invoked once on the initial Builder's constructor call. This parses the
* rootUrl value (set from the Discovery Doc) to use for the serviceName. The serviceName is
* used to construct an endpoint when the user passes in a custom Universe Domain value.
*
* <p>The roolUrl from the Discovery Docs will always follow the format of
* https://{serviceName}(.mtls).googleapis.com/
*/
private String parseServiceName(String rootUrl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use the regex to extract the service name 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.

Sounds good, will update,

// len of "https://"
int startIndex = 8;
if (rootUrl.contains(".mtls.")) {
return rootUrl.substring(startIndex, rootUrl.indexOf(".mtls"));
} else if (rootUrl.contains(".googleapis.com")) {
return rootUrl.substring(startIndex, rootUrl.indexOf(".googleapis.com"));
} else {
// Return null to not break behavior for any non-google users or any use
// case without a discovery doc. There may be certain use cases for this
// as the Builder's constructor is only protected scope
return null;
}
}

/** Builds a new instance of {@link AbstractGoogleClient}. */
Expand Down Expand Up @@ -371,6 +515,7 @@ public final String getRootUrl() {
* changing the return type, but nothing else.
*/
public Builder setRootUrl(String rootUrl) {
this.isUserConfiguredEndpoint = true;
this.rootUrl = normalizeRootUrl(rootUrl);
return this;
}
Expand Down Expand Up @@ -515,5 +660,24 @@ public Builder setSuppressRequiredParameterChecks(boolean suppressRequiredParame
public Builder setSuppressAllChecks(boolean suppressAllChecks) {
return setSuppressPatternChecks(true).setSuppressRequiredParameterChecks(true);
}

/**
* Sets the user configured Universe Domain value. This value will be used to try and construct
* the endpoint to connect to GCP services.
*
* @throws IllegalStateException if universeDomain is passed in with an empty string ("")
*/
public Builder setUniverseDomain(String universeDomain) {
if (universeDomain != null && universeDomain.isEmpty()) {
throw new IllegalArgumentException("The universe domain value cannot be empty.");
}
this.universeDomain = universeDomain;
return this;
}

@VisibleForTesting
String getServiceName() {
return serviceName;
}
}
}