format, typo, refactoring

apache · Oct 28, 2024 · e2c296b · e2c296b
1 parent 7e9a17d
commit e2c296b
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 51 deletions.
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java
@@ -143,17 +143,18 @@ private StorageConfigInfo getStorageInfo(Map<String, String> internalProperties)
             .build();
       }
       if (configInfo instanceof S3CompatibleStorageConfigurationInfo) {
-        S3CompatibleStorageConfigurationInfo s3Config = (S3CompatibleStorageConfigurationInfo) configInfo;
+        S3CompatibleStorageConfigurationInfo s3Config =
+            (S3CompatibleStorageConfigurationInfo) configInfo;
         return S3CompatibleStorageConfigInfo.builder()
             .setStorageType(StorageConfigInfo.StorageTypeEnum.S3_COMPATIBLE)
             .setS3Endpoint(s3Config.getS3Endpoint())
             .setS3PathStyleAccess(s3Config.getS3PathStyleAccess())
             .setCredsVendingStrategy(
-                org.apache.polaris.core.admin.model.S3CompatibleStorageConfigInfo.CredsVendingStrategyEnum
-                    .valueOf(
-                        org.apache.polaris.core.admin.model.S3CompatibleStorageConfigInfo
-                            .CredsVendingStrategyEnum.class,
-                        s3Config.getCredsVendingStrategy().name()))
+                org.apache.polaris.core.admin.model.S3CompatibleStorageConfigInfo
+                    .CredsVendingStrategyEnum.valueOf(
+                    org.apache.polaris.core.admin.model.S3CompatibleStorageConfigInfo
+                        .CredsVendingStrategyEnum.class,
+                    s3Config.getCredsVendingStrategy().name()))
             .setCredsCatalogAndClientStrategy(
                 org.apache.polaris.core.admin.model.S3CompatibleStorageConfigInfo
                     .CredsCatalogAndClientStrategyEnum.valueOf(
@@ -278,15 +279,19 @@ public Builder setStorageConfigurationInfo(
             break;
 
           case S3_COMPATIBLE:
-            S3CompatibleStorageConfigInfo s3ConfigModel = (S3CompatibleStorageConfigInfo) storageConfigModel;
+            S3CompatibleStorageConfigInfo s3ConfigModel =
+                (S3CompatibleStorageConfigInfo) storageConfigModel;
             config =
                 new S3CompatibleStorageConfigurationInfo(
                     PolarisStorageConfigurationInfo.StorageType.S3_COMPATIBLE,
                     S3CompatibleStorageConfigInfo.CredsVendingStrategyEnum.valueOf(
-                        org.apache.polaris.core.storage.s3compatible.S3CompatibleStorageConfigurationInfo.CredsVendingStrategyEnum.class,
+                        org.apache.polaris.core.storage.s3compatible
+                            .S3CompatibleStorageConfigurationInfo.CredsVendingStrategyEnum.class,
                         s3ConfigModel.getCredsVendingStrategy().name()),
                     S3CompatibleStorageConfigInfo.CredsCatalogAndClientStrategyEnum.valueOf(
-                        org.apache.polaris.core.storage.s3compatible.S3CompatibleStorageConfigurationInfo.CredsCatalogAndClientStrategyEnum.class,
+                        org.apache.polaris.core.storage.s3compatible
+                            .S3CompatibleStorageConfigurationInfo.CredsCatalogAndClientStrategyEnum
+                            .class,
                         s3ConfigModel.getCredsCatalogAndClientStrategy().name()),
                     s3ConfigModel.getS3Endpoint(),
                     s3ConfigModel.getS3CredentialsCatalogAccessKeyId(),

diff --git a/...g/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java b/...g/apache/polaris/core/storage/s3compatible/S3CompatibleCredentialsStorageIntegration.java
@@ -24,6 +24,7 @@
 import org.apache.polaris.core.PolarisDiagnostics;
 import org.apache.polaris.core.storage.InMemoryStorageIntegration;
 import org.apache.polaris.core.storage.PolarisCredentialProperty;
+import org.apache.polaris.core.storage.s3compatible.S3CompatibleStorageConfigurationInfo.CredsCatalogAndClientStrategyEnum;
 import org.jetbrains.annotations.NotNull;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -54,7 +55,11 @@ public void createStsClient(S3CompatibleStorageConfigurationInfo s3storageConfig
         Region
             .US_WEST_1); // default region to avoid bug, because most (all?) S3 compatible softwares
     // do not care about regions
-    stsBuilder.endpointOverride(URI.create(s3storageConfig.getS3Endpoint())); //S3Compatible - AWS STS endpoint is unique and different from the S3 Endpoint
+    stsBuilder.endpointOverride(
+        URI.create(
+            s3storageConfig
+                .getS3Endpoint())); // S3Compatible - AWS STS endpoint is unique and different from
+    // the S3 Endpoint
     stsBuilder.credentialsProvider(
         StaticCredentialsProvider.create(
             AwsBasicCredentials.create(
@@ -83,21 +88,46 @@ public EnumMap<PolarisCredentialProperty, String> getSubscopedCreds(
 
     switch (storageConfig.getCredsVendingStrategy()) {
       case KEYS_SAME_AS_CATALOG:
-        propertiesMap.put(
-            PolarisCredentialProperty.AWS_KEY_ID,
-            storageConfig.getS3CredentialsCatalogAccessKeyId());
-        propertiesMap.put(
-            PolarisCredentialProperty.AWS_SECRET_KEY,
-            storageConfig.getS3CredentialsCatalogSecretAccessKey());
+        if (storageConfig.getCredsCatalogAndClientStrategy()
+            == CredsCatalogAndClientStrategyEnum.ENV_VAR_NAME) {
+          String cai = System.getenv(storageConfig.getS3CredentialsCatalogAccessKeyId());
+          String cas = System.getenv(storageConfig.getS3CredentialsCatalogSecretAccessKey());
+          if (cai == null || cas == null) {
+            throw new IllegalArgumentException(
+                "Expect valid environment variable for catalog keys");
+          } else {
+            propertiesMap.put(PolarisCredentialProperty.AWS_KEY_ID, cai);
+            propertiesMap.put(PolarisCredentialProperty.AWS_SECRET_KEY, cas);
+          }
+        } else {
+          propertiesMap.put(
+              PolarisCredentialProperty.AWS_KEY_ID,
+              storageConfig.getS3CredentialsCatalogAccessKeyId());
+          propertiesMap.put(
+              PolarisCredentialProperty.AWS_SECRET_KEY,
+              storageConfig.getS3CredentialsCatalogSecretAccessKey());
+        }
         break;
 
       case KEYS_DEDICATED_TO_CLIENT:
-        propertiesMap.put(
-            PolarisCredentialProperty.AWS_KEY_ID,
-            storageConfig.getS3CredentialsClientAccessKeyId());
-        propertiesMap.put(
-            PolarisCredentialProperty.AWS_SECRET_KEY,
-            storageConfig.getS3CredentialsClientSecretAccessKey());
+        if (storageConfig.getCredsCatalogAndClientStrategy()
+            == CredsCatalogAndClientStrategyEnum.ENV_VAR_NAME) {
+          String cli = System.getenv(storageConfig.getS3CredentialsClientAccessKeyId());
+          String cls = System.getenv(storageConfig.getS3CredentialsClientSecretAccessKey());
+          if (cli == null || cls == null) {
+            throw new IllegalArgumentException("Expect valid environment variable for client keys");
+          } else {
+            propertiesMap.put(PolarisCredentialProperty.AWS_KEY_ID, cli);
+            propertiesMap.put(PolarisCredentialProperty.AWS_SECRET_KEY, cls);
+          }
+        } else {
+          propertiesMap.put(
+              PolarisCredentialProperty.AWS_KEY_ID,
+              storageConfig.getS3CredentialsClientAccessKeyId());
+          propertiesMap.put(
+              PolarisCredentialProperty.AWS_SECRET_KEY,
+              storageConfig.getS3CredentialsClientSecretAccessKey());
+        }
         break;
 
       case TOKEN_WITH_ASSUME_ROLE:
@@ -107,14 +137,21 @@ public EnumMap<PolarisCredentialProperty, String> getSubscopedCreds(
         LOGGER.debug("S3Compatible - assumeRole !");
         AssumeRoleResponse response =
             stsClient.assumeRole(
-                AssumeRoleRequest.builder().roleSessionName("PolarisCredentialsSTS").build());
+                AssumeRoleRequest.builder()
+                    .roleSessionName("PolarisCredentialsSTS")
+                    // @TODO customize duration
+                    // .durationSeconds(1800)
+                    .build());
 
         propertiesMap.put(
             PolarisCredentialProperty.AWS_KEY_ID, response.credentials().accessKeyId());
         propertiesMap.put(
             PolarisCredentialProperty.AWS_SECRET_KEY, response.credentials().secretAccessKey());
         propertiesMap.put(
             PolarisCredentialProperty.AWS_TOKEN, response.credentials().sessionToken());
+        LOGGER.debug(
+            "S3Compatible - assumeRole - Token Expiration at : {}j",
+            response.credentials().expiration().toString());
         break;
 
         // @TODO implement the MinIO external OpenID Connect -

diff --git a/...va/org/apache/polaris/core/storage/s3compatible/S3CompatibleStorageConfigurationInfo.java b/...va/org/apache/polaris/core/storage/s3compatible/S3CompatibleStorageConfigurationInfo.java
@@ -79,7 +79,7 @@ public S3CompatibleStorageConfigurationInfo(
       @JsonProperty(value = "allowedLocations", required = true) @NotNull
           List<String> allowedLocations) {
 
-    // Classic super and constructor storing properties
+    // storing properties
     super(storageType, allowedLocations);
     validateMaxAllowedLocations(MAX_ALLOWED_LOCATIONS);
     this.credsVendingStrategy =
@@ -90,30 +90,10 @@ public S3CompatibleStorageConfigurationInfo(
             CredsCatalogAndClientStrategyEnum.class, credsCatalogAndClientStrategy.name());
     this.s3pathStyleAccess = s3PathStyleAccess;
     this.s3endpoint = s3Endpoint;
-
-    // The constructor is called multiple time during catalog life
-    // to do substitution only once, there is a basic if null test, otherwise affect the data from
-    // the "Polaris cache storage"
-    // this way the first time the value is retrived from the name of the variable
-    // next times the getenv will try to retrive a variable but is using the value as a name, it will
-    // be null, so the value provided by "Polaris cache storage" is affected
-    if (CredsCatalogAndClientStrategyEnum.ENV_VAR_NAME.equals(credsCatalogAndClientStrategy)) {
-      String cai = System.getenv(s3CredentialsCatalogAccessKeyId);
-      String cas = System.getenv(s3CredentialsCatalogSecretAccessKey);
-      String cli = System.getenv(s3CredentialsClientAccessKeyId);
-      String cls = System.getenv(s3CredentialsClientSecretAccessKey);
-      this.s3CredentialsCatalogAccessKeyId = (cai != null) ? cai : s3CredentialsCatalogAccessKeyId;
-      this.s3CredentialsCatalogSecretAccessKey =
-          (cas != null) ? cas : s3CredentialsCatalogSecretAccessKey;
-      this.s3CredentialsClientAccessKeyId = (cli != null) ? cli : s3CredentialsClientAccessKeyId;
-      this.s3CredentialsClientSecretAccessKey =
-          (cls != null) ? cls : s3CredentialsClientSecretAccessKey;
-    } else {
-      this.s3CredentialsCatalogAccessKeyId = s3CredentialsCatalogAccessKeyId;
-      this.s3CredentialsCatalogSecretAccessKey = s3CredentialsCatalogSecretAccessKey;
-      this.s3CredentialsClientAccessKeyId = s3CredentialsClientAccessKeyId;
-      this.s3CredentialsClientSecretAccessKey = s3CredentialsClientSecretAccessKey;
-    }
+    this.s3CredentialsCatalogAccessKeyId = s3CredentialsCatalogAccessKeyId;
+    this.s3CredentialsCatalogSecretAccessKey = s3CredentialsCatalogSecretAccessKey;
+    this.s3CredentialsClientAccessKeyId = s3CredentialsClientAccessKeyId;
+    this.s3CredentialsClientSecretAccessKey = s3CredentialsClientSecretAccessKey;
   }
 
   public @NotNull CredsVendingStrategyEnum getCredsVendingStrategy() {
@@ -151,7 +131,6 @@ public S3CompatibleStorageConfigurationInfo(
   @Override
   public String toString() {
     return MoreObjects.toStringHelper(this)
-        .add("storageType", getStorageType())
         .add("storageType", getStorageType().name())
         .add("allowedLocation", getAllowedLocations())
         .toString();

diff --git a/...c/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java b/...c/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java
@@ -67,7 +67,8 @@ PolarisStorageIntegration<T> getStorageIntegrationForConfig(
                 new AwsCredentialsStorageIntegration(stsClientSupplier.get());
         break;
       case S3_COMPATIBLE:
-        storageIntegration = (PolarisStorageIntegration<T>) new S3CompatibleCredentialsStorageIntegration();
+        storageIntegration =
+            (PolarisStorageIntegration<T>) new S3CompatibleCredentialsStorageIntegration();
         break;
       case GCS:
         storageIntegration =

diff --git a/regtests/run_spark_sql_s3compatible.sh b/regtests/run_spark_sql_s3compatible.sh
@@ -109,7 +109,7 @@ fi
 # creation of catalog
 
 
-# if "credsCatalogAndClientStrategy"=="ENV_VAR_NAME" and not "VALUE", then the following environnement variables have to be available to Polaris
+# if "credsCatalogAndClientStrategy"=="ENV_VAR_NAME" and not "VALUE", then the corresponding environnement variables have to be available to Polaris
 # CATALOG_ID=minio-user-catalog
 # CATALOG_SECRET=12345678-minio-catalog
 # CLIENT_ID=minio-user-client