lasdf1234 commented on code in PR #13354:
URL: https://github.com/apache/gravitino/pull/13354#discussion_r4061185947
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java:
##########
@@ -35,10 +37,10 @@ public final class GlueConstants {
public static final String AWS_GLUE_CATALOG_ID = "aws-glue-catalog-id";
/** AWS access key ID for static credential authentication (optional,
sensitive). */
- public static final String AWS_ACCESS_KEY_ID = "aws-access-key-id";
+ public static final String AWS_ACCESS_KEY_ID =
AWSProperties.GRAVITINO_AWS_ACCESS_KEY_ID;
Review Comment:
Done. The cloud merge only skipped a key when it was already in base, and
the basic-catalog loop checked the same map. Keys added by the cloud loop were
not visible to that check. There is no overlap today: cloud keys are s3- / oss-
/ azure- / gcs- / cos- / aws- prefixed, and the basic keys are package,
catalog-operation-impl, authorization-provider, cloud.name, cloud.region-code,
in-use, and metalake-in-use. A future collision would have failed inside
ImmutableMap.Builder.build() with Guava's "Multiple entries with same key", not
the "Property metadata already exists" message next to the basic-catalog loop.
propertyEntries() now keeps a set of names already placed, starting from
base. A cloud key that the catalog already declared is still skipped, so the
catalog's own entry wins. A basic catalog key that collides with base, or with
a cloud key just added, fails the checkArgument with "Property metadata already
exists".
##########
core/src/test/java/org/apache/gravitino/cloud/storage/TestCloudPropertiesMetadata.java:
##########
@@ -104,4 +108,41 @@ void
testDeclaredSensitiveNamedCredentialKeysAreNonHidden() {
SecretPropertyUtils.isSensitivePropertyKey(
AzureProperties.GRAVITINO_AZURE_STORAGE_ACCOUNT_NAME));
}
+
+ @Test
+ void testAwsCredentialPropertiesAreDeclared() {
+ var metadata = AWSPropertiesMetadata.PROPERTY_ENTRIES;
+
assertTrue(metadata.containsKey(AWSProperties.GRAVITINO_AWS_ACCESS_KEY_ID));
+
assertTrue(metadata.containsKey(AWSProperties.GRAVITINO_AWS_SECRET_ACCESS_KEY));
+
assertFalse(metadata.get(AWSProperties.GRAVITINO_AWS_ACCESS_KEY_ID).isHidden());
+
assertFalse(metadata.get(AWSProperties.GRAVITINO_AWS_ACCESS_KEY_ID).isRequired());
+
assertTrue(metadata.get(AWSProperties.GRAVITINO_AWS_SECRET_ACCESS_KEY).isHidden());
+
assertFalse(metadata.get(AWSProperties.GRAVITINO_AWS_SECRET_ACCESS_KEY).isRequired());
+ assertSame(AWSProperties.GRAVITINO_AWS_ACCESS_KEY_ID,
GlueConstants.AWS_ACCESS_KEY_ID);
+ assertSame(AWSProperties.GRAVITINO_AWS_SECRET_ACCESS_KEY,
GlueConstants.AWS_SECRET_ACCESS_KEY);
Review Comment:
Thank you for your review.
Done. Those two assertSame calls were comparing
GlueConstants.AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY with the
AWSProperties string constants. Both Glue constants are static final Strings
initialized from constant expressions, so javac inlines them into the test
class as the literals "aws-access-key-id" and "aws-secret-access-key". Equal
string literals are interned, so the assertions would still pass if
GlueConstants went back to its own literals. That does not pin the coupling
this change is supposed to keep.
I removed those two lines from TestCloudPropertiesMetadata. The check with
teeth is on the objects: TestGlueCatalogPropertiesMetadata now asserts that the
Glue catalog's propertyEntries() for those two names are the same PropertyEntry
instances as AWSPropertiesMetadata.AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY,
which is what GlueCatalogPropertiesMetadata wires in. A Glue-local copy of the
entry would fail that test. The assertion lives in the Glue module because the
core test cannot depend on the Glue catalog.
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalogPropertiesMetadata.java:
##########
@@ -102,17 +103,30 @@ 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 =
+ CloudPropertiesMetadata.ALL_PROPERTY_ENTRIES;
+
@Override
public Map<String, PropertyEntry<?>> propertyEntries() {
if (propertyEntries == null) {
synchronized (this) {
if (propertyEntries == null) {
- // Reuse BasePropertiesMetadata (specific + BASIC +
CredentialConfig), then add
- // catalog-only entries.
+ // Reuse BasePropertiesMetadata (specific + BASIC +
CredentialConfig), then add shared
+ // cloud credential keys and catalog-only entries.
Map<String, PropertyEntry<?>> base = buildBasePropertyEntries();
ImmutableMap.Builder<String, PropertyEntry<?>> builder =
ImmutableMap.builder();
builder.putAll(base);
+ CLOUD_PROPERTY_ENTRIES.forEach(
+ (name, entry) -> {
+ if (!base.containsKey(name)) {
+ builder.put(name, entry);
+ }
+ });
Review Comment:
Thank you for your review.
Done. The access key ID javadoc now says it is optional and not hidden: an
account identifier, not a secret. The secret key javadoc is unchanged.
##########
core/src/main/java/org/apache/gravitino/secret/FallbackPropertiesMetadata.java:
##########
@@ -42,13 +37,7 @@ final class FallbackPropertiesMetadata extends
BasePropertiesMetadata {
static final FallbackPropertiesMetadata INSTANCE = new
FallbackPropertiesMetadata();
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)
- .build();
+ CloudPropertiesMetadata.ALL_PROPERTY_ENTRIES;
Review Comment:
Good catch. Including the AWS pair in Fallback was inconsistent with keeping
it off fileset/schema metadata.
Fixed: FallbackPropertiesMetadata now uses STORAGE_PROPERTY_ENTRIES only.
The AWS access-key pair stays on BaseCatalogPropertiesMetadata (catalog merge).
Entity fallback treats aws-access-key-id the same as fileset/schema —
undeclared, so fuzzy mask/recover still applies.
TestSecretPropertyOperationDispatcher now asserts that.
--
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]
