mchades commented on code in PR #12366:
URL: https://github.com/apache/gravitino/pull/12366#discussion_r3733336388
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -101,84 +106,99 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
* @throws SchemaAlreadyExistsException If a schema with the same identifier
already exists.
*/
@Override
- public Schema createSchema(NameIdentifier ident, String comment, Map<String,
String> properties)
+ public Schema createSchema(
+ NameIdentifier ident,
+ String comment,
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
+ Map<String, SecretReference> secretReferences)
throws NoSuchCatalogException, SchemaAlreadyExistsException {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ long uid = idGenerator.nextId();
+ Map<String, String> entityProperties =
SecretPropertyUtils.copyEntityProperties(properties);
+ List<SecretUrn> secretUrns =
+ secretManager.assembleSecretUrns(
+ properties, entityProperties, "schema", uid, secretBindings,
secretReferences);
doWithCatalog(
catalogIdent,
c ->
c.doWithPropertiesMeta(
p -> {
- validatePropertyForCreate(p.schemaPropertiesMetadata(),
properties);
+ validatePropertyForCreate(p.schemaPropertiesMetadata(),
entityProperties);
return null;
}),
IllegalArgumentException.class);
- long uid = idGenerator.nextId();
+ secretManager.writeSecrets(secretBindings, secretUrns);
// Add StringIdentifier to the properties, the specific catalog will
handle this
// StringIdentifier to make sure only when the operation is successful,
the related
// SchemaEntity will be visible.
StringIdentifier stringId = StringIdentifier.fromId(uid);
Map<String, String> updatedProperties =
- StringIdentifier.newPropertiesWithId(stringId, properties);
-
- return TreeLockUtils.doWithTreeLock(
- catalogIdent,
- LockType.WRITE,
- () -> {
- // we do not retrieve the schema again (to obtain some values
generated by underlying
- // catalog)
- // since some catalogs' API is async and the schema may not be
created immediately
- Schema schema =
- doWithCatalog(
- catalogIdent,
- c -> c.doWithSchemaOps(s -> s.createSchema(ident, comment,
updatedProperties)),
- NoSuchCatalogException.class,
- SchemaAlreadyExistsException.class);
+ StringIdentifier.newPropertiesWithId(stringId, entityProperties);
- // If the Schema is maintained by the Gravitino's store, we don't
have to store again.
- boolean isManagedSchema = isManagedEntity(catalogIdent,
Capability.Scope.SCHEMA);
- if (isManagedSchema) {
- return EntityCombinedSchema.of(schema)
- .withHiddenProperties(
- getHiddenPropertyNames(
- catalogIdent,
- HasPropertyMetadata::schemaPropertiesMetadata,
- schema.properties()));
- }
+ try {
+ return TreeLockUtils.doWithTreeLock(
+ catalogIdent,
+ LockType.WRITE,
+ () -> {
+ // we do not retrieve the schema again (to obtain some values
generated by underlying
+ // catalog)
+ // since some catalogs' API is async and the schema may not be
created immediately
+ Schema schema =
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithSchemaOps(s -> s.createSchema(ident, comment,
updatedProperties)),
Review Comment:
`updatedProperties` contains the secret URNs assembled above, but it is
passed unchanged to the schema connector. There is no schema/fileset equivalent
of `CatalogManager.toPlaintextProperties`; the fileset catalog later merges
schema/fileset properties into filesystem configuration, so a credential
remains a `urn:...` string and authentication fails. Please provide a resolved
copy at the connector/runtime consumption boundary while keeping persisted
metadata as URNs, and apply the same fix to `FilesetOperationDispatcher`.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -593,12 +596,20 @@ public Catalog createCatalog(
Catalog.Type type,
String provider,
String comment,
- Map<String, String> properties)
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
+ Map<String, SecretReference> secretReferences)
throws NoSuchMetalakeException, CatalogAlreadyExistsException {
NameIdentifier metalakeIdent =
NameIdentifier.of(ident.namespace().levels());
- Map<String, String> mergedConfig = buildCatalogConf(provider, properties);
+ final Map<String, String> mergedConfig = new
HashMap<>(buildCatalogConf(provider, properties));
long uid = idGenerator.nextId();
+
+ List<SecretUrn> secretUrns =
+ secretManager.assembleSecretUrns(
+ properties, mergedConfig, "catalog", uid, secretBindings,
secretReferences);
+ secretManager.writeSecrets(secretBindings, secretUrns);
Review Comment:
`writeSecrets` executes before `checkMetalake` and before the cleanup
`try/finally`. If the metalake is missing or concurrently deleted, this method
exits at `checkMetalake` without rollback and leaves provider material
orphaned. Move the write under the lock, after metalake validation, and keep it
inside the cleanup scope.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -101,84 +106,99 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
* @throws SchemaAlreadyExistsException If a schema with the same identifier
already exists.
*/
@Override
- public Schema createSchema(NameIdentifier ident, String comment, Map<String,
String> properties)
+ public Schema createSchema(
+ NameIdentifier ident,
+ String comment,
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
+ Map<String, SecretReference> secretReferences)
throws NoSuchCatalogException, SchemaAlreadyExistsException {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ long uid = idGenerator.nextId();
+ Map<String, String> entityProperties =
SecretPropertyUtils.copyEntityProperties(properties);
+ List<SecretUrn> secretUrns =
+ secretManager.assembleSecretUrns(
+ properties, entityProperties, "schema", uid, secretBindings,
secretReferences);
doWithCatalog(
catalogIdent,
c ->
c.doWithPropertiesMeta(
p -> {
- validatePropertyForCreate(p.schemaPropertiesMetadata(),
properties);
+ validatePropertyForCreate(p.schemaPropertiesMetadata(),
entityProperties);
return null;
}),
IllegalArgumentException.class);
- long uid = idGenerator.nextId();
+ secretManager.writeSecrets(secretBindings, secretUrns);
Review Comment:
Once this write succeeds, a successful `dropSchema` has no corresponding
path that reads the stored URNs and deletes the write-through secret. A
create-then-drop sequence therefore permanently leaves provider material
behind. Please add drop cleanup for write-through URNs owned by the schema;
`FilesetOperationDispatcher` has the same gap. External-reference URNs must
remain untouched.
##########
core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java:
##########
@@ -91,7 +98,8 @@ public Schema createSchema(NameIdentifier ident, String
comment, Map<String, Str
// ancestor's owner is never overwritten.
List<NameIdentifier> newAncestors =
findMissingAncestors(normalizedIdent);
- Schema schema = dispatcher.createSchema(ident, comment, properties);
+ Schema schema =
+ dispatcher.createSchema(ident, comment, properties,
secretBindings, secretReferences);
Review Comment:
`dispatcher.createSchema` has already persisted the entity and written its
secrets when it returns. If the later `ownerManager.setOwners` call throws, the
API reports a create failure but this hook has no compensating drop or secret
cleanup, so the entity and secret remain and a retry sees already-exists.
Please add hook-level compensation; `FilesetHookDispatcher` has the same
sequence.
##########
design-docs/gravitino-entity-secrets.md:
##########
@@ -650,12 +650,12 @@ sibling maps). Existing **`setProperty`** stays a
**string** `value` (plaintext
| `@type` | Fields (flat; not nested under `value`)
| Behavior
|
| -------------------- |
----------------------------------------------------------------------------------------
|
---------------------------------------------------------------------------------------------------------------------------------------------------------
|
| `setProperty` | `value` (**string** plaintext)
| Today’s plaintext set. If the **current**
value matches the URN recognition rule, in-place `writeSecret` via provider in
the current URN; persist new URN |
-| `setSecretBinding` | `provider` (instance name) + `value` (plaintext
string) | Write-through bind/re-bind
(`writeSecret`); persist returned URN
|
+| `setSecretBinding` | `provider` (instance name) + `plaintext` (plaintext
string) | Write-through bind/re-bind
(`writeSecret`); persist returned URN
|
Review Comment:
This alter row now correctly uses typed `provider` plus `plaintext`, but the
create contract in §5.9.2/TC-1 still says `secretBindings` is
`map<string,string>` and places plaintext in `properties`. The current
DTO/OpenAPI require `property -> {provider, plaintext}` and reject overlap with
`properties`, so requests following the document fail. Please update the create
rules and examples in the same change.
##########
api/src/main/java/org/apache/gravitino/file/FilesetCatalog.java:
##########
@@ -113,6 +116,10 @@ default Fileset createFileset(
* @param type The type of the fileset.
* @param storageLocations The location names and storage locations of the
fileset.
* @param properties The properties of the fileset.
+ * @param secretBindings optional property key → binding ({@code provider} +
{@code plaintext})
Review Comment:
The new secret-aware entry point exists only for
`createMultipleLocationFileset`. The common single-location `createFileset`
overload cannot accept either secret map, so Java callers must switch APIs and
manufacture a singleton location map. Please add a symmetric secret-aware
overload and delegate internally.
##########
core/src/main/java/org/apache/gravitino/secret/SecretManager.java:
##########
@@ -87,7 +155,7 @@ public List<SecretUrn> getSecretReferenceUrns(Map<String,
SecretReference> secre
SecretProvider provider = registry.getProvider(providerName);
try {
SecretUrn urn = provider.buildReferenceUrn(key, locator.attributes());
Review Comment:
For an external-reference provider that reads a shared secret namespace with
a service identity, this accepts caller-controlled `provider`/`attributes`
without authorizing the referenced secret for the current principal or target
entity. A catalog creator could therefore make Gravitino read or use a secret
outside their scope; a caller-controlled connector target may then disclose or
misuse it. Please add reference-level authorization/scoping when binding and
again when reading or using the URN.
##########
api/src/main/java/org/apache/gravitino/SupportsCatalogs.java:
##########
@@ -84,23 +88,58 @@ default boolean catalogExists(String catalogName) {
* the created catalog is the managed catalog, like model, fileset catalog.
For the details of the
* provider definition, see {@link CatalogProvider}.
*
- * @param catalogName the name of the catalog.
- * @param type the type of the catalog.
- * @param provider the provider of the catalog, or null if the catalog is a
managed catalog.
- * @param comment the comment of the catalog.
- * @param properties the properties of the catalog.
- * @return The created catalog.
- * @throws NoSuchMetalakeException If the metalake does not exist.
- * @throws CatalogAlreadyExistsException If the catalog already exists.
+ * @param catalogName the name of the catalog
+ * @param type the type of the catalog
+ * @param provider the provider of the catalog, or null if the catalog is a
managed catalog
+ * @param comment the comment of the catalog
+ * @param properties the properties of the catalog
+ * @param secretBindings optional property key → binding ({@code provider} +
{@code plaintext})
+ * for write-through
+ * @param secretReferences optional property key → secret locator ({@code
provider} plus
+ * provider-specific attributes)
+ * @return the created catalog
+ * @throws NoSuchMetalakeException if the metalake does not exist
+ * @throws CatalogAlreadyExistsException if the catalog already exists
*/
Catalog createCatalog(
String catalogName,
Catalog.Type type,
String provider,
String comment,
- Map<String, String> properties)
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
Review Comment:
I realize this interface is `@Evolving`, but making the new secret-aware
overload abstract while turning the old method into a default forces every
existing third-party implementation to add the new method; invoking the new API
on an old implementation can produce `AbstractMethodError`. Could we preserve
the old abstract method and make the new overload a safe default that delegates
only for empty secret maps, or explicitly rejects non-empty maps?
`SupportsSchemas` has the same compatibility reversal.
##########
common/src/main/java/org/apache/gravitino/dto/requests/CatalogCreateRequest.java:
##########
@@ -62,18 +94,26 @@ public class CatalogCreateRequest implements RESTRequest {
* @param provider The provider of the catalog.
* @param comment The comment for the catalog.
* @param properties The properties for the catalog.
+ * @param secretBindings Optional property key → binding DTO ({@code
provider} + {@code
+ * plaintext}) for write-through secrets.
+ * @param secretReferences Optional property key → secret locator DTO
({@code provider} plus
+ * provider-specific attributes).
*/
@JsonCreator
public CatalogCreateRequest(
@JsonProperty("name") String name,
@JsonProperty("type") Catalog.Type type,
@JsonProperty("provider") String provider,
@JsonProperty("comment") String comment,
- @JsonProperty("properties") Map<String, String> properties) {
+ @JsonProperty("properties") Map<String, String> properties,
+ @JsonProperty("secretBindings") Map<String, SecretBindingDTO>
secretBindings,
+ @JsonProperty("secretReferences") Map<String, SecretReferenceDTO>
secretReferences) {
this.name = name;
this.type = type;
this.comment = comment;
this.properties = properties;
+ this.secretBindings = secretBindings;
+ this.secretReferences = secretReferences;
Review Comment:
`NON_EMPTY` omits the empty maps created by the legacy constructor, but
missing JSON fields arrive as `null` and are stored unchanged here. A JSON
round trip therefore changes empty maps to `null`, currently failing
`TestCatalogCreateRequest.testCatalogCreateRequestSerDe` and
`TestRequestJsonSerDe.testCatalogCreateRequestSerDe` in `build (17)`. Normalize
absent secret maps to empty here, or otherwise make the serialization/equality
contract consistent.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -1303,7 +1320,10 @@ private Map<String, String>
getResolvedProperties(CatalogEntity entity) {
private BaseCatalog<?> createBaseCatalog(IsolatedClassLoader classLoader,
CatalogEntity entity) {
// Load Catalog class instance
BaseCatalog<?> catalog = createCatalogInstance(classLoader,
entity.getProvider());
- catalog.withCatalogConf(entity.getProperties()).withCatalogEntity(entity);
+ // Resolve secret URNs to plaintext for connector init only; entity
storage keeps URNs.
+ catalog
+
.withCatalogConf(secretManager.toPlaintextProperties(entity.getProperties()))
Review Comment:
`GET .../catalogs?details=true` calls `listCatalogsInfo` for every catalog
before the REST layer performs per-item authorization filtering. This eager
resolution therefore performs provider reads and initializes cached catalog
configuration with plaintext even for catalogs filtered from the response; one
provider failure can also fail the entire listing. Filter authorized
identifiers before loading/resolving wrappers, and avoid retaining plaintext
longer than connector use.
##########
common/src/main/java/org/apache/gravitino/dto/requests/FilesetCreateRequest.java:
##########
@@ -64,6 +67,16 @@ public class FilesetCreateRequest implements RESTRequest {
@JsonProperty("properties")
private Map<String, String> properties;
+ @Nullable
+ @JsonInclude(JsonInclude.Include.NON_EMPTY)
+ @JsonProperty("secretBindings")
+ private Map<String, SecretBindingDTO> secretBindings;
Review Comment:
Because this class exposes Lombok's `@AllArgsConstructor`, adding these
fields changes the public constructor from six parameters to eight. Existing
source calls stop compiling and existing binaries can fail with
`NoSuchMethodError`. Please preserve an explicit six-argument overload, as the
catalog/schema request DTOs do, or avoid treating the generated all-args
constructor as the compatibility contract.
--
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]
