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 properties to customize universe-domain and host in Storage #3287

Merged
merged 20 commits into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/src/main/asciidoc/storage.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ The Spring Boot Starter for Google Cloud Storage provides the following configur
Base64-encoded contents of OAuth2 account private key for authenticating with the Google Cloud Storage API, if different from the ones in the <<spring-cloud-gcp-core,Spring Framework on Google Cloud Core Module>> | No |
| `spring.cloud.gcp.storage.credentials.scopes` |
https://developers.google.com/identity/protocols/googlescopes[OAuth2 scope] for Spring Framework on Google Cloud Storage credentials | No | https://www.googleapis.com/auth/devstorage.read_write
| `spring.cloud.gcp.storage.universe-domain` | Universe domain of the Storage service. The universe domain is a part of the host that is formatted as https://${service}.${universeDomain} | No | Relies on client library’s default universe domain which is googleapis.com
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
| `spring.cloud.gcp.storage.host` | Host of the Storage service which expects https://${service}.${universeDomain} as the format. | No | Relies on client library’s default host which is `https://storage.googleapis.com`
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
|===


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public BigQueryTemplate bigQueryTemplate(
bigQuery, bigQueryWriteClient, bqInitSettings, bigQueryThreadPoolTaskScheduler);
}

private String resolveToHost(String endpoint) throws URISyntaxException {
private String resolveToHost(String endpoint) {
int portIndex = endpoint.indexOf(":");
if (portIndex != -1) {
return "https://" + endpoint.substring(0, portIndex) + "/";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,17 @@ public class GcpBigQueryProperties implements CredentialsSupplier {
/** The size of thread pool of ThreadPoolTaskScheduler used by GcpBigQueryAutoConfiguration */
private int threadPoolSize;

/**
* Universe domain of the Bigquery and BigQueryWriteClient which is part of the endpoint that is
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
* formatted as `{service}.{universeDomain}:${port}`.
*/
private String universeDomain;

/**
* Endpoint (formatted as `{service}.{universeDomain}:${port}`)
* Endpoint of the Bigquery and BigQueryWriteClient. Formatted as
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
* `{service}.{universeDomain}:${port}`. Note that endpoint will be reformatted in {@link
* GcpBigQueryAutoConfiguration} to follow the `https://${service}.${universeDomain}/` pattern
* before being applied to the Bigquery client.
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
*/
private String endpoint;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,14 @@ public class GcpKmsProperties implements CredentialsSupplier {

/** Overrides the GCP Project ID specified in the Core module. */
private String projectId;

/**
* Universe domain of the client which is part of the endpoint that is formatted as
* `${service}.${universeDomain}:${port}`
*/
private String universeDomain;

/** Enspoint of the KMS client which is formatted as`${service}.${universeDomain}:${port}` */
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
private String endpoint;

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import com.google.cloud.storage.Storage;
import com.google.cloud.storage.StorageOptions;
import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URL;
import org.springframework.boot.autoconfigure.AutoConfiguration;
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
Expand All @@ -52,6 +54,10 @@ public class GcpStorageAutoConfiguration { // NOSONAR squid:S1610 must be a clas

private final CredentialsProvider credentialsProvider;

private final String universeDomain;

private final String host;

public GcpStorageAutoConfiguration(
GcpProjectIdProvider coreProjectIdProvider,
CredentialsProvider credentialsProvider,
Expand All @@ -67,16 +73,39 @@ public GcpStorageAutoConfiguration(
gcpStorageProperties.getCredentials().hasKey()
? new DefaultCredentialsProvider(gcpStorageProperties)
: credentialsProvider;

this.universeDomain = gcpStorageProperties.getUniverseDomain();
this.host = gcpStorageProperties.getHost();
}

@Bean
@ConditionalOnMissingBean
public Storage storage() throws IOException {
return StorageOptions.newBuilder()
.setHeaderProvider(new UserAgentHeaderProvider(GcpStorageAutoConfiguration.class))
.setProjectId(this.gcpProjectIdProvider.getProjectId())
.setCredentials(this.credentialsProvider.getCredentials())
.build()
.getService();
StorageOptions.Builder storageOptionsBuilder =
StorageOptions.newBuilder()
.setHeaderProvider(new UserAgentHeaderProvider(GcpStorageAutoConfiguration.class))
.setProjectId(this.gcpProjectIdProvider.getProjectId())
.setCredentials(this.credentialsProvider.getCredentials());

if (this.universeDomain != null) {
storageOptionsBuilder.setUniverseDomain(this.universeDomain);
}
if (this.host != null) {
storageOptionsBuilder.setHost(verifyAndFetchHost(this.host));
}
return storageOptionsBuilder.build().getService();
}

private String verifyAndFetchHost(String host) {
URL url;
try {
url = new URL(host);
} catch (MalformedURLException e) {
throw new IllegalArgumentException(
"Invalid host format: "
+ host
+ ". Please verify that the specified host follows the 'https://${service}.${universeDomain}' format");
}
return url.getProtocol() + "://" + url.getHost() + "/";
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we get this format from Storage client library code?

Copy link
Contributor

@blakeli0 blakeli0 Oct 9, 2024

Choose a reason for hiding this comment

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

I know we have it in java-core, is it where get it from? But we don't have the ending slash in java-core

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 thanks for the question. From doing a check with breakpoints in a spring storage app, Storage calls getResolvedApiaryHost() when setting the host at https://github.com/googleapis/java-storage/blob/7d47307d10e285f92b3e2125c4993b6a292001bf/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpStorageOptions.java#L267.

getResolvedApiaryHost() returns a host with a / so applied the same behavior here.

Copy link
Contributor

@blakeli0 blakeli0 Oct 9, 2024

Choose a reason for hiding this comment

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

Thanks! Do you mind adding some comment regarding where we get the format from? I think a link to the implementation of getResolvedApiaryHost() in java-core should be good enough.
In addition, anywhere we mention the format of host, we should always have an ending slash, which I see that it is missing in the Javadoc of universeDomain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Added a javadoc and also modified other locations where we mention host follow a consistent style.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,36 @@ public Credentials getCredentials() {

private String projectId;

/**
* Universe domain of the client which is part of the host that is formatted as
* `https://${service}.${universeDomain}.
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
*/
private String universeDomain;

/** Host of the Storage client that is formatted as `https://${service}.${universeDomain}/. */
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
private String host;

public String getProjectId() {
return projectId;
}

public void setProjectId(String projectId) {
this.projectId = projectId;
}

public String getUniverseDomain() {
return universeDomain;
}

public void setUniverseDomain(String universeDomain) {
this.universeDomain = universeDomain;
}

public String getHost() {
return host;
}

public void setHost(String host) {
this.host = host;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.cloud.spring.autoconfigure.storage;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -48,27 +49,31 @@ class GcpStorageAutoConfigurationTests {
.withUserConfiguration(TestConfiguration.class);

@Test
void testValidObject() throws Exception {
this.contextRunner.run(
context -> {
Resource resource = context.getBean("mockResource", Resource.class);
assertThat(resource.contentLength()).isEqualTo(4096);
});
void testValidObject() {
this.contextRunner
.withUserConfiguration(TestStorageConfiguration.class)
.run(
context -> {
Resource resource = context.getBean("mockResource", Resource.class);
assertThat(resource.contentLength()).isEqualTo(4096);
});
}

@Test
void testAutoCreateFilesTrueByDefault() throws IOException {
this.contextRunner.run(
context -> {
Resource resource = context.getBean("mockResource", Resource.class);
assertThat(((GoogleStorageResource) resource).isAutoCreateFiles()).isTrue();
});
void testAutoCreateFilesTrueByDefault() {
this.contextRunner
.withUserConfiguration(TestStorageConfiguration.class)
.run(
context -> {
Resource resource = context.getBean("mockResource", Resource.class);
assertThat(((GoogleStorageResource) resource).isAutoCreateFiles()).isTrue();
});
}

@Test
void testAutoCreateFilesRespectsProperty() throws IOException {

void testAutoCreateFilesRespectsProperty() {
this.contextRunner
.withUserConfiguration(TestStorageConfiguration.class)
.withPropertyValues("spring.cloud.gcp.storage.auto-create-files=false")
.run(
context -> {
Expand All @@ -77,9 +82,85 @@ void testAutoCreateFilesRespectsProperty() throws IOException {
});
}

@Test
void testUniverseDomain() {
this.contextRunner
.withPropertyValues("spring.cloud.gcp.storage.universe-domain=example.com")
.run(
context -> {
Storage storage = context.getBean("storage", Storage.class);
assertThat(storage.getOptions().getUniverseDomain()).isEqualTo("example.com");
assertThat(storage.getOptions().getHost()).isEqualTo("https://storage.example.com/");
});
}

@Test
void testHost() {
this.contextRunner
.withPropertyValues("spring.cloud.gcp.storage.host=https://storage.example.com/")
.run(
context -> {
Storage storage = context.getBean("storage", Storage.class);
assertThat(storage.getOptions().getHost()).isEqualTo("https://storage.example.com/");
});
}

@Test
void testUniverseDomainAndHostSet() {
this.contextRunner
.withPropertyValues(
"spring.cloud.gcp.storage.universe-domain=example.com",
"spring.cloud.gcp.storage.host=https://storage.example.com")
.run(
context -> {
Storage storage = context.getBean("storage", Storage.class);
assertThat(storage.getOptions().getUniverseDomain()).isEqualTo("example.com");
assertThat(storage.getOptions().getHost()).isEqualTo("https://storage.example.com/");
});
}

@Test
void testNoUniverseDomainOrHostSet_useDefaults() {
this.contextRunner.run(
context -> {
Storage storage = context.getBean("storage", Storage.class);
assertThat(storage.getOptions().getUniverseDomain()).isNull();
assertThat(storage.getOptions().getHost()).isEqualTo("https://storage.googleapis.com/");
});
}

@Test
void testInvalidHost_throwsException() {
this.contextRunner
.withPropertyValues("spring.cloud.gcp.storage.host=storage.example.com")
.run(
context -> {
Exception exception =
assertThrows(Exception.class, () -> context.getBean("storage", Storage.class));
assertThat(exception).hasRootCauseInstanceOf(IllegalArgumentException.class);
assertThat(exception)
.hasRootCauseMessage(
"Invalid host format: storage.example.com. Please verify that the specified host follows the 'https://${service}.${universeDomain}' format");
});
}

@Configuration
static class TestConfiguration {

@Bean
public static CredentialsProvider googleCredentials() {
return () -> mock(Credentials.class);
}

@Bean
public static GcpProjectIdProvider gcpProjectIdProvider() {
return () -> "default-project";
}
}

@Configuration
static class TestStorageConfiguration {

@Value("gs://test-spring/images/spring.png")
private Resource remoteResource;

Expand All @@ -98,15 +179,5 @@ public static Storage mockStorage() throws Exception {
when(storage.get(validBlob)).thenReturn(mockedBlob);
return storage;
}

@Bean
public static CredentialsProvider googleCredentials() {
return () -> mock(Credentials.class);
}

@Bean
public static GcpProjectIdProvider gcpProjectIdProvider() {
return () -> "default-project";
}
}
}
Loading