collado-mike commented on code in PR #370:
URL: https://github.com/apache/polaris/pull/370#discussion_r1800204471
##########
polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java:
##########
@@ -208,4 +215,58 @@ public void setAwsSecretKey(String awsSecretKey) {
public void setDefaultRealms(List<String> defaultRealms) {
this.defaultRealms = defaultRealms;
}
+
+ public Supplier<GoogleCredentials> getGcpCredentialsProvider() {
+ return gcpAccessToken != null
+ ? () -> GoogleCredentials.create(gcpAccessToken)
+ : () -> {
+ try {
+ return GoogleCredentials.getApplicationDefault();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to get GCP credentials", e);
+ }
+ };
+ }
+
+ @JsonProperty("gcp_credentials")
+ void setGcpCredentials(GcpAccessToken token) {
+ this.gcpAccessToken =
+ new AccessToken(
+ token.getAccessToken(),
+ new Date(System.currentTimeMillis() + token.getExpiresIn() *
1000));
+ }
+
+ /**
+ * A static AccessToken representation used to store a static token and
expiration date. This
+ * should strictly be used for testing.
Review Comment:
There's a pretty substantial number of classes that are only used in tests -
e.g., the DefaultContextResolver is only for tests, as is the in-memory
metastore manager.
TBH, I _do_ really want to have better support for customizable
cloud-specific credential loading - e.g., read the GCP credentials from vault
or assume an AWS IAM role using an instance profile - but adding support for
that is more than just defining an interface. We have to design an interface
that subclasses `Discoverable`, hash out the details around that interface,
then add the `META-INF/services` files in the `main` and `test` structures,
then override the integration-test yml file to specify which one to use and...
that's more work than I think ought to go into this bugfix.
I think the barrier to adding a test for a bugfix should be low. We can
refactor to better support for injectable credential providers later, but I
don't think it makes sense to hamstring a bugfix PR because we didn't properly
abstract this code in the first place. I will happily create an issue and add a
TODO in the code, but I think fixing the bug is more important to get done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
