Copilot commented on code in PR #13204:
URL: https://github.com/apache/gravitino/pull/13204#discussion_r4024832255
##########
core/src/main/java/org/apache/gravitino/secret/SecretPropertyUtils.java:
##########
@@ -116,7 +143,7 @@ public static Map<String, String> buildSecrets(
}
if (isSecretProperty(key, value)) {
secrets.put(key, secretManager.readSecret(SecretUrn.parse(value)));
- } else if (isSensitivePropertyKey(key)) {
+ } else if (shouldRecoverSensitiveNamedSecret(key, metadata)) {
Review Comment:
This exclusion breaks the GVFS static-credential path. `BaseGVFSOperations`
first removes all keys in
`CloudStorageCredentialPropertyKeys.STATIC_CREDENTIAL_KEYS` from `properties()`
and then overlays `getSecrets()`; after access-key IDs and
`gcs-service-account-file` become declared non-hidden, this method no longer
returns them, so server-stored static credentials are dropped before Hadoop
configuration is built. Update the filtering/merge contract (and add a GVFS
regression test) so those non-hidden values are preserved or returned to that
consumer.
##########
core/src/main/java/org/apache/gravitino/secret/SecretPropertyUtils.java:
##########
@@ -99,10 +123,13 @@ public static boolean isSecretProperty(@Nullable String
key, @Nullable String va
*
* @param secretManager secret manager used to resolve URNs
* @param rawProperties raw entity properties (may be null)
+ * @param metadata entity properties metadata, or null when unavailable
* @return a new secret plaintext property map; never null
*/
public static Map<String, String> buildSecrets(
- SecretManager secretManager, @Nullable Map<String, String>
rawProperties) {
+ SecretManager secretManager,
+ @Nullable Map<String, String> rawProperties,
+ @Nullable PropertiesMetadata metadata) {
Review Comment:
This replaces an existing public two-argument overload, which breaks source
and binary compatibility for downstream users of `SecretPropertyUtils`. Keep
the two-argument overload as a delegating compatibility method (passing null
metadata); callers without metadata retain the historical fuzzy recovery
behavior while the new overload serves the dispatcher.
##########
core/src/main/java/org/apache/gravitino/cloud/storage/COSPropertiesMetadata.java:
##########
@@ -48,6 +48,22 @@ public class COSPropertiesMetadata {
false /* immutable */,
null /* defaultValue */,
true /* hidden */))
+ .put(
+ COSProperties.GRAVITINO_COS_REGION,
+ stringOptionalPropertyEntry(
+ COSProperties.GRAVITINO_COS_REGION,
+ "Tencent Cloud COS region",
+ false /* immutable */,
+ null /* defaultValue */,
+ false /* hidden */))
+ .put(
+ COSProperties.GRAVITINO_COS_ENDPOINT,
+ stringOptionalPropertyEntry(
+ COSProperties.GRAVITINO_COS_ENDPOINT,
+ "Tencent Cloud COS endpoint",
+ false /* immutable */,
+ null /* defaultValue */,
+ false /* hidden */))
Review Comment:
This metadata map still omits `GRAVITINO_COS_ROLE_ARN`,
`GRAVITINO_COS_EXTERNAL_ID`, and `GRAVITINO_COS_APP_ID`, even though they are
official COS credential-vending properties defined in
`COSProperties`/`COSCredentialConfig`. They therefore remain undeclared for
metadata validation and UI handling; add non-hidden entries for all three
alongside region and endpoint.
##########
core/src/main/java/org/apache/gravitino/secret/SecretPropertyOperationDispatcher.java:
##########
@@ -82,97 +85,96 @@ public SecretPropertyOperationDispatcher(
* @return secret plaintext properties; never null
*/
public Map<String, String> getSecrets(NameIdentifier identifier,
Entity.EntityType entityType) {
- Map<String, String> rawProperties = loadRawProperties(identifier,
entityType);
- return SecretPropertyUtils.buildSecrets(secretManager, rawProperties);
+ RawPropertiesAndMetadata loaded = loadRawPropertiesAndMetadata(identifier,
entityType);
+ return SecretPropertyUtils.buildSecrets(secretManager,
loaded.rawProperties, loaded.metadata);
}
- private Map<String, String> loadRawProperties(
+ /**
+ * Loads raw properties and matching properties metadata in one catalog
lease when possible.
+ *
+ * <p>If the catalog does not expose properties metadata for the entity type
({@link
+ * UnsupportedOperationException}), metadata is {@code null} so {@link
+ * SecretPropertyUtils#buildSecrets} falls back to metadata-unaware fuzzy
recovery instead of
+ * failing the whole {@code getSecrets} call.
+ */
+ private RawPropertiesAndMetadata loadRawPropertiesAndMetadata(
NameIdentifier identifier, Entity.EntityType entityType) {
switch (entityType) {
case METALAKE:
- return loadMetalakeRawProperties(identifier);
+ return new
RawPropertiesAndMetadata(loadMetalakeRawProperties(identifier), null);
case CATALOG:
- return loadCatalogRawProperties(identifier);
+ return doWithCatalog(
+ identifier,
+ wrapper -> {
+ wrapper.catalog().checkMetalakeInUse();
+ return new RawPropertiesAndMetadata(
+ wrapper.catalog().entity().getProperties(),
+ propertiesMetadataOrNull(() ->
wrapper.catalog().catalogPropertiesMetadata()));
+ },
+ NoSuchCatalogException.class);
case SCHEMA:
- return loadSchemaRawProperties(identifier);
+ return loadSchemaRawPropertiesAndMetadata(identifier);
case FILESET:
- return loadFilesetRawProperties(identifier);
+ return loadFilesetRawPropertiesAndMetadata(identifier);
case TABLE:
- return loadTableRawProperties(identifier);
+ return loadTableRawPropertiesAndMetadata(identifier);
case TOPIC:
- return loadTopicRawProperties(identifier);
+ return loadTopicRawPropertiesAndMetadata(identifier);
case VIEW:
- return loadViewRawProperties(identifier);
+ return loadViewRawPropertiesAndMetadata(identifier);
case MODEL:
- return loadModelRawProperties(identifier);
+ return loadModelRawPropertiesAndMetadata(identifier);
case MODEL_VERSION:
- return loadModelVersionRawProperties(identifier);
+ return loadModelVersionRawPropertiesAndMetadata(identifier);
default:
throw new NotSupportedException(
"Doesn't support secret property operations for entity type: " +
entityType);
}
}
- private Map<String, String> loadMetalakeRawProperties(NameIdentifier
identifier) {
- try {
- BaseMetalake entity = store.get(identifier, Entity.EntityType.METALAKE,
BaseMetalake.class);
- return entity.properties() == null ? Map.of() : entity.properties();
- } catch (NoSuchEntityException e) {
- throw new NoSuchMetalakeException(e, "Metalake %s does not exist",
identifier);
- } catch (IOException e) {
- throw new RuntimeException("Failed to load metalake entity " +
identifier, e);
- }
- }
-
- private Map<String, String> loadCatalogRawProperties(NameIdentifier
identifier) {
- return doWithCatalog(
- identifier,
- wrapper -> {
- wrapper.catalog().checkMetalakeInUse();
- return wrapper.catalog().entity().getProperties();
- },
- NoSuchCatalogException.class);
- }
-
- private Map<String, String> loadSchemaRawProperties(NameIdentifier
identifier) {
+ private RawPropertiesAndMetadata
loadSchemaRawPropertiesAndMetadata(NameIdentifier identifier) {
NameIdentifier catalogIdent =
NameIdentifierUtil.getCatalogIdentifier(identifier);
- doWithCatalog(
- catalogIdent,
- wrapper -> {
- wrapper.catalog().checkMetalakeInUse();
- return null;
- },
- NoSuchCatalogException.class);
+ PropertiesMetadata metadata =
+ doWithCatalog(
+ catalogIdent,
+ wrapper -> {
+ wrapper.catalog().checkMetalakeInUse();
+ return propertiesMetadataOrNull(() ->
wrapper.catalog().schemaPropertiesMetadata());
Review Comment:
This metadata getter is invoked directly on the connector, outside
`CatalogWrapper.doWithPropertiesMeta`, so the isolated connector classloader is
not installed for the call. A plugin whose metadata implementation resolves
connector-only classes or resources can fail here; invoke this getter through
the wrapper's metadata callback.
--
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]
