jerryshao commented on code in PR #13354:
URL: https://github.com/apache/gravitino/pull/13354#discussion_r4061044670
##########
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:
[Question] The fallback uses `ALL_PROPERTY_ENTRIES` (storage keys **plus**
the AWS access-key pair), but this same PR deliberately keeps the AWS pair off
entity-level metadata: `FilesetPropertiesMetadata.java:78` and
`FilesetSchemaPropertiesMetadata.java:71` use `STORAGE_PROPERTY_ENTRIES`, and
the new `TestFilesetCloudPropertiesMetadata.java:93-105` locks that exclusion
in.
This single `INSTANCE` stands in for every entity type, not just catalogs:
`SecretPropertyOperationDispatcher.resolvePropertiesMetadata` (`:299`) is
reached from the SCHEMA / TABLE / FILESET / MODEL branches of
`loadRawPropertiesAndMetadata` (`:102-130`) as well as the CATALOG one. So on a
schema whose catalog has no schema metadata, `aws-access-key-id` counts as
declared and non-hidden and drops out of `getSecrets`
(`SecretPropertyUtils.java:112`), while on a fileset schema the same key is
undeclared, sensitive-named, and still fuzzy-recovered - the same name behaving
two ways, which is the class of inconsistency this PR is fixing.
Was including the AWS pair here intentional (fallback is also the catalog
stand-in), or should the non-catalog path use `STORAGE_PROPERTY_ENTRIES`?
Either way it is worth a sentence in the javadoc at `:30-33`.
Verified by: read `FallbackPropertiesMetadata` and every branch of
`SecretPropertyOperationDispatcher.loadRawPropertiesAndMetadata`, then compared
against the two fileset entity metadata classes and the new fileset test.
##########
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:
[Nit] Carried over from the previous review, now that the cloud block sits
directly above it: this loop skips a key already in `base`, but
`BASIC_CATALOG_PROPERTY_ENTRIES` is put unconditionally at `:130-135` and only
checks `base`, not what the cloud loop just added. There is no overlap today
(cloud keys are all `s3-` / `oss-` / `azure-` / `gcs-` / `cos-` / `aws-`
prefixed; the basic keys are `package`, `catalog-operation-impl`,
`authorization-provider`, `cloud.name`, `cloud.region-code`, `in-use`,
`metalake-in-use`), but if one ever appeared it would surface as Guava's opaque
`Multiple entries with same key` from `builder.build()` at `:136`, not the
explicit "Property metadata already exists" message one line above. Checking
the builder's own keys instead of `base` in both loops would keep the error
message useful.
Verified by: read `propertyEntries()` end to end and listed the key literals
on both sides; `ImmutableMap.Builder.build()` is what throws on a duplicate.
##########
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:
[Nit] The javadoc one line above still reads "(optional, sensitive)" for the
access key ID, which now contradicts the rest of this PR:
`AWSProperties.java:24` says "Not hidden", `AWSPropertiesMetadata.java:39` sets
`hidden=false`, and this PR's own doc change at
`design-docs/gravitino-glue-catalog.md:130` replaces "**Sensitive**: not
visible to catalog readers" with "Visible in cleartext to catalog readers".
Since the whole point of the change is that this key is an identifier rather
than a secret, the javadoc should say so too (the one on `:42` for the secret
key is accurate).
Verified by: compared the javadoc against the three other places this PR
describes the same key.
##########
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:
[Nit] These two `assertSame` calls cannot fail, so they do not pin what they
look like they pin. `GlueConstants.AWS_ACCESS_KEY_ID =
AWSProperties.GRAVITINO_AWS_ACCESS_KEY_ID` is a compile-time constant
expression (JLS 15.29), so javac inlines it into this test class as the literal
`"aws-access-key-id"`, and equal string literals are interned - the assertion
would pass just as well if `GlueConstants.java:40` went back to its own
literal, which is exactly the coupling the test is meant to guard.
An assertion with teeth would be on the objects rather than the names, e.g.
that the Glue catalog metadata's entry for the key is the same `PropertyEntry`
instance as `AWSPropertiesMetadata.AWS_ACCESS_KEY_ID` (that is what
`GlueCatalogPropertiesMetadata.java:61-62` now wires up). Otherwise these two
lines can just go.
Verified by: read the new test and `GlueConstants.java:40`/`:43`; both
constants are `static final String` initialised from constant expressions,
which javac inlines and interns.
--
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]
