jerryshao commented on code in PR #13354:
URL: https://github.com/apache/gravitino/pull/13354#discussion_r4060234053
##########
core/src/main/java/org/apache/gravitino/secret/FallbackPropertiesMetadata.java:
##########
@@ -48,6 +49,7 @@ final class FallbackPropertiesMetadata extends
BasePropertiesMetadata {
.putAll(AzurePropertiesMetadata.PROPERTY_ENTRIES)
.putAll(GCSPropertiesMetadata.PROPERTY_ENTRIES)
.putAll(COSPropertiesMetadata.PROPERTY_ENTRIES)
+ .putAll(AWSPropertiesMetadata.PROPERTY_ENTRIES)
Review Comment:
[Question] The AWS pair is added at catalog level and here in the fallback,
but not to the entity-level metadata classes that already declare the other
five sets:
`catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetPropertiesMetadata.java:82-86`
and `FilesetSchemaPropertiesMetadata.java:75-79`. A fileset or schema property
literally named `aws-access-key-id` therefore stays fuzzy-masked as `******`
(undeclared, and the name contains `access`), while the same key on its catalog
now comes back in cleartext - the same name/level asymmetry this PR is fixing,
one level down. Intentional, because the AWS pair is Glue-catalog-only, or
worth adding there too?
Verified by: read both fileset metadata classes at those lines and
`HiddenPropertyMaskUtils.classifyHiddenProperties`
(`core/src/main/java/org/apache/gravitino/connector/HiddenPropertyMaskUtils.java:108-116`),
which this PR leaves unchanged.
##########
core/src/main/java/org/apache/gravitino/cloud/storage/AWSPropertiesMetadata.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.cloud.storage;
+
+import static
org.apache.gravitino.connector.PropertyEntry.stringOptionalPropertyEntry;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.gravitino.catalog.glue.GlueConstants;
Review Comment:
[Nit] `core` now owns a shared, every-catalog AWS credential definition, but
takes its key names from the Glue connector's constants class
(`GlueConstants.AWS_ACCESS_KEY_ID` / `AWS_SECRET_ACCESS_KEY`,
`catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java:38,41`).
The five sibling classes in this package take theirs from neutral holders in
`org.apache.gravitino.storage` (`S3Properties`, `OSSProperties`, ...). Consider
moving the two names to an `org.apache.gravitino.storage.AWSProperties` next to
`S3Properties` and letting `GlueConstants` delegate, so a key declared on every
catalog is not owned by one connector's constants file.
Verified by: read `AWSPropertiesMetadata.java:25,34,44` and
`GlueConstants.java:38,41`; `core/build.gradle.kts:33` declares
`implementation(project(":catalogs:catalog-common"))`, so this compiles today -
the concern is layering, not the build.
##########
core/src/main/java/org/apache/gravitino/cloud/storage/AWSPropertiesMetadata.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.cloud.storage;
+
+import static
org.apache.gravitino.connector.PropertyEntry.stringOptionalPropertyEntry;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.gravitino.catalog.glue.GlueConstants;
+import org.apache.gravitino.connector.PropertyEntry;
+
+/** Shared AWS credential {@link PropertyEntry} definitions for catalog
properties metadata. */
+public final class AWSPropertiesMetadata {
+
+ /** AWS access key ID. Not hidden. */
+ public static final PropertyEntry<String> AWS_ACCESS_KEY_ID =
+ stringOptionalPropertyEntry(
+ GlueConstants.AWS_ACCESS_KEY_ID,
+ "AWS access key ID for static credential authentication."
+ + " When omitted the default credential chain is used.",
+ false /* immutable */,
+ null /* defaultValue */,
+ false /* hidden */);
+
Review Comment:
[Question] `aws-access-key-id` keeps `hidden=false` and is now declared on
every catalog, so its value is returned in cleartext to anyone who can read the
catalog. That matches Glue's existing declaration on `main` and the
`s3-access-key-id` precedent, so I believe it is the right call - but
`design-docs/gravitino-glue-catalog.md:130-131` still documents both AWS keys
as "**Sensitive**: not visible to catalog readers via Gravitino API". That line
was already stale for Glue; this PR makes it wrong for every catalog. Worth
correcting alongside this change?
Verified by: read `design-docs/gravitino-glue-catalog.md:130-131`, and the
removed Glue block in this diff shows both keys already carried `hidden=false`
/ `hidden=true` before the change.
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalogPropertiesMetadata.java:
##########
@@ -102,17 +108,37 @@ protected Map<String, PropertyEntry<?>>
specificPropertyEntries() {
true /* hidden */)),
PropertyEntry::getName);
+ /**
+ * Cloud credential keys merged into every catalog. A catalog that already
declares a key wins.
+ */
+ private static final Map<String, PropertyEntry<?>> CLOUD_PROPERTY_ENTRIES =
+ ImmutableMap.<String, PropertyEntry<?>>builder()
+ .putAll(S3PropertiesMetadata.PROPERTY_ENTRIES)
+ .putAll(OSSPropertiesMetadata.PROPERTY_ENTRIES)
+ .putAll(AzurePropertiesMetadata.PROPERTY_ENTRIES)
+ .putAll(GCSPropertiesMetadata.PROPERTY_ENTRIES)
+ .putAll(COSPropertiesMetadata.PROPERTY_ENTRIES)
+ .putAll(AWSPropertiesMetadata.PROPERTY_ENTRIES)
+ .build();
Review Comment:
[Nit] This `putAll` block is now the seventh copy of the same list:
`core/src/main/java/org/apache/gravitino/secret/FallbackPropertiesMetadata.java:47-52`,
`catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalogPropertiesMetadata.java:131-135`,
`catalogs/catalog-lakehouse-iceberg/.../IcebergCatalogPropertiesMetadata.java:143-147`,
`catalogs/catalog-lakehouse-paimon/.../PaimonCatalogPropertiesMetadata.java:220-224`,
`catalogs/catalog-fileset/.../FilesetCatalogPropertiesMetadata.java:231-235`,
plus the two fileset entity-level classes. This PR had to edit two of the seven
to add AWS, which is exactly how they drift.
Two suggestions: (1) extract a single `ALL_PROPERTY_ENTRIES` constant in
`org.apache.gravitino.cloud.storage` and reference it from both core call
sites; (2) the four catalog-level copies listed above are now redundant - all
four extend `BaseCatalogPropertiesMetadata`, which supplies the same entries -
so they can be dropped here or in a follow-up.
Verified by: grepped `S3PropertiesMetadata.PROPERTY_ENTRIES` across the repo
and read every hit, plus the class declaration of each of the four catalogs.
##########
core/src/test/java/org/apache/gravitino/connector/TestBaseCatalogPropertiesMetadata.java:
##########
@@ -43,4 +46,59 @@ void
testCredentialPropertyEntriesAreDeclaredForAllCatalogs() {
assertFalse(metadata.isHiddenProperty(CredentialConstants.CREDENTIAL_PROVIDERS));
assertFalse(metadata.isHiddenProperty(CredentialConstants.S3_TOKEN_EXPIRE_IN_SECS));
}
+
+ @Test
+ void testSharedCloudCredentialKeysAreDeclaredForAllCatalogs() {
+
assertTrue(metadata.containsProperty(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID));
+
assertTrue(metadata.containsProperty(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY));
+
assertFalse(metadata.isHiddenProperty(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID));
+
assertTrue(metadata.isHiddenProperty(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY));
+ }
+
+ @Test
+ void testConnectorCredentialKeysAreDeclaredForAllCatalogs() {
+ assertTrue(metadata.containsProperty("aws-access-key-id"));
+ assertFalse(metadata.isHiddenProperty("aws-access-key-id"));
+ assertTrue(metadata.isHiddenProperty("aws-secret-access-key"));
+ assertFalse(metadata.containsProperty("jdbc-user"));
+ assertFalse(metadata.containsProperty("jdbc-password"));
+ assertFalse(metadata.containsProperty("token-provider"));
+ assertFalse(metadata.containsProperty("gcs.oauth2.token"));
+ assertFalse(metadata.containsProperty("s3.session-token"));
+ assertFalse(metadata.containsProperty("jdbc.user"));
+ }
+
+ @Test
+ void testRuntimeCopiedS3AccessKeyUsesSharedCloudMetadata() {
+ PropertiesMetadata glueLikeMetadata =
+ new BaseCatalogPropertiesMetadata() {
+ @Override
+ protected Map<String, PropertyEntry<?>> specificPropertyEntries() {
+ return ImmutableMap.of(
+ "aws-access-key-id",
+ PropertyEntry.stringOptionalPropertyEntry(
+ "aws-access-key-id", "AWS access key ID", false, null,
false),
+ "aws-secret-access-key",
+ PropertyEntry.stringOptionalPropertyEntry(
+ "aws-secret-access-key", "AWS secret access key", false,
null, true));
+ }
Review Comment:
[Nit] This case declares `aws-access-key-id` / `aws-secret-access-key` in
`specificPropertyEntries()` with exactly the same `hidden` flags as
`AWSPropertiesMetadata.java:32-48`, so it cannot observe the precedence rule
the description states ("a catalog that already declares a key keeps its own
entry"). A case where the catalog declares a shared key with a *different* flag
- for example `s3-access-key-id` with `hidden=true` - and asserts
`isHiddenProperty` stays `true` would pin the `!base.containsKey(name)` guard
at `BaseCatalogPropertiesMetadata.java:137`.
Verified by: compared the entries built in this test against
`AWSPropertiesMetadata.java:32-48`; both declare the same names with the same
flags.
--
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]
