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 to Java-Core #2329

Merged
merged 24 commits into from Jan 4, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
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
Expand Up @@ -46,6 +46,7 @@
import com.google.auth.oauth2.ServiceAccountCredentials;
import com.google.cloud.spi.ServiceRpcFactory;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
Expand Down Expand Up @@ -81,6 +82,7 @@ public abstract class ServiceOptions<
implements Serializable {

public static final String CREDENTIAL_ENV_NAME = "GOOGLE_APPLICATION_CREDENTIALS";
public static final String GOOGLE_DEFAULT_UNIVERSE = "googleapis.com";
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved

private static final String DEFAULT_HOST = "https://www.googleapis.com";
private static final String LEGACY_PROJECT_ENV_NAME = "GCLOUD_PROJECT";
Expand All @@ -95,6 +97,7 @@ public abstract class ServiceOptions<
protected final String clientLibToken;

private final String projectId;
private final String universeDomain;
private final String host;
private final RetrySettings retrySettings;
private final String serviceRpcFactoryClassName;
Expand Down Expand Up @@ -125,6 +128,7 @@ public abstract static class Builder<
private final ImmutableSet<String> allowedClientLibTokens =
ImmutableSet.of(ServiceOptions.getGoogApiClientLibName());
private String projectId;
private String universeDomain;
private String host;
protected Credentials credentials;
private RetrySettings retrySettings;
Expand All @@ -142,6 +146,7 @@ protected Builder() {}
@InternalApi("This class should only be extended within google-cloud-java")
protected Builder(ServiceOptions<ServiceT, OptionsT> options) {
projectId = options.projectId;
universeDomain = options.universeDomain;
host = options.host;
credentials = options.credentials;
retrySettings = options.retrySettings;
Expand Down Expand Up @@ -199,6 +204,17 @@ public B setHost(String host) {
return self();
}

/**
* Universe Domain is the domain for Google Cloud Services. It follows the format of
suztomo marked this conversation as resolved.
Show resolved Hide resolved
* `{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`.
Copy link
Member

Choose a reason for hiding this comment

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

Would you add a paragraph that explains the validity of the argument in terms of isValidUniverseDomain?

*/
public B setUniverseDomain(String universeDomain) {
this.universeDomain = universeDomain;
suztomo marked this conversation as resolved.
Show resolved Hide resolved
return self();
}

/**
* Sets the service authentication credentials. If no credentials are set, {@link
* GoogleCredentials#getApplicationDefault()} will be used to attempt getting credentials from
Expand Down Expand Up @@ -306,6 +322,7 @@ protected ServiceOptions(
"A project ID is required for this service but could not be determined from the builder "
+ "or the environment. Please set a project ID using the builder.");
}
universeDomain = builder.universeDomain;
host = firstNonNull(builder.host, getDefaultHost());
credentials = builder.credentials != null ? builder.credentials : defaultCredentials();
retrySettings = firstNonNull(builder.retrySettings, getDefaultRetrySettings());
Expand Down Expand Up @@ -582,6 +599,16 @@ public String getProjectId() {
return projectId;
}

/**
* 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`.
suztomo marked this conversation as resolved.
Show resolved Hide resolved
*/
public String getUniverseDomain() {
return universeDomain;
}

/** Returns the service host. */
public String getHost() {
return host;
Expand Down Expand Up @@ -767,4 +794,40 @@ public String getClientLibToken() {
public String getQuotaProjectId() {
return quotaProjectId;
}

/** Returns the resolved endpoint for the Service to connect to Google Cloud */
public String getResolvedEndpoint(String serviceName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think host in java-core is basically endpoint in Gax without the port, based on the value of DEFAULT_HOST. So we should either rename this to getResolvedHost and remove the port concatenation in formatEndpoint, or return host:port if host is specified.
Can you please verify the format of host?

Copy link
Contributor Author

@lqiu96 lqiu96 Jan 3, 2024

Choose a reason for hiding this comment

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

I think the format of host is intended to be scheme://host:port. For gRPC, the channel is expecting something in format of domain:port and httpjson is open to anything valid.

I see that the handwritten libraries handle these inside their own codebase:

  1. Storage gRPC (strips the scheme and adds a port if missing): https://github.com/googleapis/java-storage/blob/2b5b27eac3279db815b36b252830d0905ade0665/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java#L163-L170
  2. Storage HttpJson (sets it directly): https://github.com/googleapis/java-storage/blob/2b5b27eac3279db815b36b252830d0905ade0665/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java#L143
  3. Spanner (enforces that a port exists): https://github.com/googleapis/java-spanner/blob/21f5eba7d46c9011803accf0aaf87e6072066419/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java#L1469-L1471
  4. Bigquery (sets it directly): https://github.com/googleapis/java-bigquery/blob/7a1bbd2d33e71644d83c42b71c67bac099882a4a/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/spi/v2/HttpBigQueryRpc.java#L108

I think it should be that the gRPC clients use getResolvedEndpoint() and expect something in the format host:port. Apiary clients (httpjson) will use getResolvedApiaryEndpoint() and that will return the just the host.

if (universeDomain == null) {
return formatEndpoint(serviceName, GOOGLE_DEFAULT_UNIVERSE);
} else if (Strings.isNullOrEmpty(universeDomain)) {
throw new IllegalArgumentException("Universe Domain cannot be empty");
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
} else {
if (host == null) {
return formatEndpoint(serviceName, getUniverseDomain());
}
return host;
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
}
}

// Temporarily used for Apiary Wrapped Libraries. To be removed in the future.
@InternalApi
public String getResolvedApiaryEndpoint(String serviceName) {
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
String resolvedUniverseDomain =
getUniverseDomain() != null ? getUniverseDomain() : GOOGLE_DEFAULT_UNIVERSE;
return formatEndpoint(serviceName, resolvedUniverseDomain);
}

/** Validates that Credentials' Universe Domain and user configured Universe Domain matches. */
public boolean isValidUniverseDomain() throws IOException {
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
suztomo marked this conversation as resolved.
Show resolved Hide resolved
Credentials credentials = getCredentials();
String universeDomain = getUniverseDomain();
String resolvedUniverseDomain =
universeDomain != null ? universeDomain : GOOGLE_DEFAULT_UNIVERSE;
return true;
// return credentials.getUniverseDomain() != resolvedUniverseDomain;
}

private String formatEndpoint(String serviceName, String universeDomain) {
return serviceName + "." + universeDomain + ":443";
}
}
Expand Up @@ -22,6 +22,7 @@
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

Expand Down Expand Up @@ -471,6 +472,74 @@ public void testResponseHeaderDoesNotContainMetaDataFlavor() throws Exception {
assertThat(ServiceOptions.headerContainsMetadataFlavor(httpResponse)).isFalse();
}

@Test
public void testGetResolvedEndpoint_noUniverseDomain() {
TestServiceOptions options = TestServiceOptions.newBuilder().setProjectId("project-id").build();
assertThat(options.getResolvedEndpoint("service")).isEqualTo("service.googleapis.com:443");
}

@Test
public void testGetResolvedEndpoint_emptyUniverseDomain() {
TestServiceOptions options =
TestServiceOptions.newBuilder().setUniverseDomain("").setProjectId("project-id").build();
IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () -> options.getResolvedEndpoint("service"));
assertThat(exception.getMessage()).isEqualTo("Universe Domain cannot be empty");
}

@Test
public void testGetResolvedEndpoint_customUniverseDomain_noHost() {
TestServiceOptions options =
TestServiceOptions.newBuilder()
.setUniverseDomain("test.com")
.setHost(null)
.setProjectId("project-id")
.build();
// `https://www.googleapis.com` is the DEFAULT_HOST value for ServiceOptions. The ServiceOptions
// builder will set as it as the host if host is null
assertThat(options.getResolvedEndpoint("service")).isEqualTo("https://www.googleapis.com");
}

@Test
public void testGetResolvedEndpoint_customUniverseDomain_customHost() {
TestServiceOptions options =
TestServiceOptions.newBuilder()
.setUniverseDomain("test.com")
.setHost("service.random.com:443")
.setProjectId("project-id")
.build();
assertThat(options.getResolvedEndpoint("service")).isEqualTo("service.random.com:443");
}

@Test
public void testGetResolvedApiaryEndpoint_noUniverseDomain() {
TestServiceOptions options = TestServiceOptions.newBuilder().setProjectId("project-id").build();
assertThat(options.getResolvedApiaryEndpoint("service"))
.isEqualTo("service.googleapis.com:443");
}

@Test
public void testGetResolvedApiaryEndpoint_customUniverseDomain_noHost() {
TestServiceOptions options =
TestServiceOptions.newBuilder()
.setUniverseDomain("test.com")
.setHost(null)
.setProjectId("project-id")
.build();
assertThat(options.getResolvedApiaryEndpoint("service")).isEqualTo("service.test.com:443");
}

@Test
public void testGetResolvedApiaryEndpoint_customUniverseDomain_customHost() {
TestServiceOptions options =
TestServiceOptions.newBuilder()
.setUniverseDomain("test.com")
.setHost("service.random.com:443")
.setProjectId("project-id")
.build();
assertThat(options.getResolvedEndpoint("service")).isEqualTo("service.random.com:443");
}

private HttpResponse createHttpResponseWithHeader(final Multimap<String, String> headers)
throws Exception {
HttpTransport mockHttpTransport =
Expand Down