-
Notifications
You must be signed in to change notification settings - Fork 313
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
Conversation
...-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/kms/GcpKmsProperties.java
Outdated
Show resolved
Hide resolved
...gure/src/main/java/com/google/cloud/spring/autoconfigure/bigquery/GcpBigQueryProperties.java
Outdated
Show resolved
Hide resolved
...gure/src/main/java/com/google/cloud/spring/autoconfigure/bigquery/GcpBigQueryProperties.java
Outdated
Show resolved
Hide resolved
...gure/src/main/java/com/google/cloud/spring/autoconfigure/bigquery/GcpBigQueryProperties.java
Outdated
Show resolved
Hide resolved
...figure/src/main/java/com/google/cloud/spring/autoconfigure/storage/GcpStorageProperties.java
Outdated
Show resolved
Hide resolved
...figure/src/main/java/com/google/cloud/spring/autoconfigure/storage/GcpStorageProperties.java
Outdated
Show resolved
Hide resolved
LGTM with minor comment adjustments |
Co-authored-by: Burke Davison <[email protected]>
Co-authored-by: Burke Davison <[email protected]>
Co-authored-by: Burke Davison <[email protected]>
Co-authored-by: Burke Davison <[email protected]>
Co-authored-by: Burke Davison <[email protected]>
Co-authored-by: Burke Davison <[email protected]>
Co-authored-by: Burke Davison <[email protected]>
Co-authored-by: Burke Davison <[email protected]>
+ host | ||
+ ". Please verify that the specified host follows the 'https://${service}.${universeDomain}' format"); | ||
} | ||
return url.getProtocol() + "://" + url.getHost() + "/"; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
docs/src/main/asciidoc/storage.adoc
Outdated
@@ -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` | |||
| `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` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any documentation in Storage to describe the relationship between host
and universeDomain
? If yes, maybe we can link it here so customers know which one to configure? I know we don't have it yet in pure GAPICs(which is a gap).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any storage-specific documentation on universe domain and host but looks like this is the closest page there is to providing that info? https://cloud.google.com/java/docs/reference/google-cloud-core/latest/com.google.cloud.ServiceOptions.Builder#com_google_cloud_ServiceOptions_Builder_setUniverseDomain_java_lang_String_
*/ | ||
private String universeDomain; | ||
|
||
/** Host of the Storage client that is formatted as `https://${service}.${universeDomain}/`. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we have an ending slash here but not in the format example above for universeDomain, which one we should go for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, modified the documentation to be consistent.
Quality Gate passedIssues Measures |
Notable Changes
spring.cloud.gcp.storage.universe-domain
property to customize universe domain in StorageOptions, if provided.spring.cloud.gcp.storage.host
property to customize the host in StorageOptions. Note that it the host doesn't follow the ``https://${service}.${domain}/` format, we run into the following error:MalformedURLException
doesn't provide sufficient messaging to help troubleshoot host formatting issues, added validation to verify that the host starts withhttps://
or otherwise throws anIllegalArgumentException
.Integration testing has been manually recorded in a separate test plan document.