lasdf1234 commented on code in PR #13204:
URL: https://github.com/apache/gravitino/pull/13204#discussion_r4025471913
##########
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:
Fixed in `3481e38f7`.
`CloudStorageCredentialPropertyKeys.STATIC_CREDENTIAL_KEYS` now only
includes secret-bearing keys (e.g. `*-secret-access-key`, Azure account key).
Non-secret identifiers such as `s3-access-key-id` / `oss-access-key-id` /
`cos-access-key-id` and `gcs-service-account-file` stay in `properties()` and
are no longer stripped by GVFS.
Also added a GVFS regression test
(`testKeepsPlaintextAccessKeyIdFromRestProperties`) to cover the merge path:
plaintext access-key-id from REST + secret recovered via `getSecrets()`.
##########
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:
Fixed in `3481e38f7`.
`SecretPropertyOperationDispatcher` now resolves properties metadata via
`CatalogWrapper.doWithPropertiesMeta(...)` (under the connector classloader),
instead of calling `wrapper.catalog().*PropertiesMetadata()` directly.
`UnsupportedOperationException` still falls back to `metadata=null` so
`getSecrets` does not fail for unsupported entity types.
##########
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:
Fixed in `3481e38f7`.
Restored the public two-argument `buildSecrets(SecretManager, Map)` overload
as a compatibility wrapper that delegates to the three-argument method with
`metadata=null` (historical fuzzy recovery behavior).
--
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]
