[#12297] feat(secret): Add catalog and schema create-time secrets - #12420
[#12297] feat(secret): Add catalog and schema create-time secrets#12420lasdf1234 wants to merge 32 commits into
Conversation
Follow-up to the fileset-only create secrets PR: wire secretBindings and secretReferences through catalog/schema API, REST, clients, OpenAPI, dispatchers, drop cleanup, hook rollback, and listCatalogsInfo auth before resolve. Co-authored-by: Cursor <cursoragent@cursor.com>
…cret helpers Rebase onto the fileset create-secrets tip, switch to rollbackBindings / deleteBindingsFromProperties / checkSecretKeys, and shorten catalog/schema secret unit tests to match apache#12366 cleanup. Co-authored-by: Cursor <cursoragent@cursor.com>
…rial APIs Use assembleSecretMaterials, writeSecrets(List), rollbackSecrets, and deleteSecretsFromProperties after rebasing onto main post-apache#12366. Co-authored-by: Cursor <cursoragent@cursor.com>
…che#12366 style Match fileset create-time secrets conventions: empty secret maps in Java and Python DTOs/clients, remove schema posthook drop on setOwner failure, and drop the unrelated GlueCatalogOperations Javadoc-only change. Co-authored-by: Cursor <cursoragent@cursor.com>
Authorize details=true via listCatalogs then loadCatalog so secrets resolve only for authorized catalogs, without a new API method. Co-authored-by: Cursor <cursoragent@cursor.com>
…eset Match FilesetCatalog defaults, SchemaCreateRequest Lombok/builder, and Python empty-map defaults / response validate. Co-authored-by: Cursor <cursoragent@cursor.com>
Fold secret rollback into needClean, with an explicit rollback on EntityAlreadyExists where the existing catalog must be kept. Co-authored-by: Cursor <cursoragent@cursor.com>
…unchanged Remove the extra checkSecretKeys calls; testConnection only probes connectivity and should not take part in create-time secret wiring. Co-authored-by: Cursor <cursoragent@cursor.com>
…elds Match CatalogCreateRequest style: final fields, JsonCreator, and constructor-based client usage instead of Lombok builder. Co-authored-by: Cursor <cursoragent@cursor.com>
…lizeDispatcher createSchema now validates assembled entityProperties (empty map when request properties is null), so the test must supply required key k1. Co-authored-by: Cursor <cursoragent@cursor.com>
…no secrets copyEntityProperties returns null only when properties and both secret maps are absent, and assembleSecretMaterials skips URN writes in that case so validatePropertyForCreate keeps historical null-skip behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
FrontendIT testClickCatalogLink timed out waiting for hive schema "default" while metastore was still warming; prior FrontendIT on this PR passed. Empty commit to re-run checks. Co-authored-by: Cursor <cursoragent@cursor.com>
Code Coverage Report
Files
|
| }); | ||
| }); | ||
| } catch (RuntimeException e) { | ||
| secretManager.rollbackSecrets(secretMaterials); |
There was a problem hiding this comment.
Will this delete a secret that is currently referenced by an existing schema?
There was a problem hiding this comment.
Each schema's secret is exclusively owned by that schema and there is no sharing involved, so this issue does not exist.
There was a problem hiding this comment.
The concern isn't sharing between schemas but liveness: the catch also covers the code after s.createSchema() has already succeeded (isManagedEntity, getHiddenPropertyNames), so a failure there deletes the secret while the schema still exists in the underlying catalog holding that URN — suggest rolling back only when the schema was not created.
| .withCreator(PrincipalUtils.getCurrentPrincipal().getName()) | ||
| .withCreateTime(Instant.now()) | ||
| .build()) | ||
| .build(); |
There was a problem hiding this comment.
Schema properties are not stored in Gravitino's entity store at all; their only source is the underlying catalog. Will they be lost for catalogs that don't support them?
| // the cache with stale data between invalidate and delete. | ||
| Map<String, String> catalogProperties = | ||
| catalogWrapper.catalog().entity().getProperties(); | ||
| boolean deleted = store.delete(ident, EntityType.CATALOG, true); |
There was a problem hiding this comment.
Cascade deletion does not clean up the secrets of child entities at all.
There was a problem hiding this comment.
Before deleting the catalog/schema, first note down the properties (including URNs) of the sub-schemas and filesets. After successfully deleting the entity, call the function "deleteSecretsFromProperties" to remove their write-through secrets.
| catalogIdent, | ||
| c -> c.doWithSchemaOps(s -> s.loadSchema(ident)), | ||
| NoSuchSchemaException.class); | ||
| schemaProperties = schema.properties(); |
There was a problem hiding this comment.
If load throws any other exception, the drop operation will fail.
There was a problem hiding this comment.
Thank you very much for your review. I have added a "catch(Exception)" block to solve this problem.
There was a problem hiding this comment.
schema.properties() is not read properties from entity store, some catalog‘s properties does not contain URN
…iew comments - Roll back create-time secrets only when catalog create did not succeed - Persist schema properties (incl. URNs) on SchemaEntity for unmanaged schemas - Clean schema/fileset write-through secrets on catalog cascade drop - Do not fail schema drop when pre-drop catalog load throws unexpected errors Co-authored-by: Cursor <cursoragent@cursor.com>
…anup review comments" This reverts commit c32ecdb.
Keep schema secret URNs in the entity store when the underlying catalog does not retain custom properties, so drop-time cleanup can still find them. Co-authored-by: Cursor <cursoragent@cursor.com>
store.delete(cascade) only soft-deletes meta rows. Snapshot schema and fileset properties first, drop entities, then delete write-through secrets so provider material is not left orphaned. Co-authored-by: Cursor <cursoragent@cursor.com>
…snapshot Co-authored-by: Cursor <cursoragent@cursor.com>
dropSchema cascade removes filesets without FilesetOperationDispatcher. Snapshot fileset properties before drop and delete write-through secrets after the schema drop succeeds. Co-authored-by: Cursor <cursoragent@cursor.com>
… errors Secret property capture before drop is best-effort; catch unexpected load failures so dropSchema can still proceed. Co-authored-by: Cursor <cursoragent@cursor.com>
| Map<String, SecretBinding> secretBindings, | ||
| Map<String, SecretReference> secretReferences) | ||
| throws NoSuchMetalakeException, CatalogAlreadyExistsException { | ||
| throw new UnsupportedOperationException("Not implemented"); |
There was a problem hiding this comment.
Could u use complete error message here? For example. xxxx is not implemented.
There was a problem hiding this comment.
Got. Use 'Creating xxx with secrets is not supported' instead.(Consistent with the original code of the project)
| */ | ||
| Schema createSchema(String schemaName, String comment, Map<String, String> properties) | ||
| throws NoSuchCatalogException, SchemaAlreadyExistsException; | ||
| default Schema createSchema( |
There was a problem hiding this comment.
Got. Use 'Creating xxx with secrets is not supported' instead.(Consistent with the original code of the project)
| * @throws SchemaAlreadyExistsException if the schema with specified identifier already exists. | ||
| */ | ||
| @Override | ||
| public Schema createSchema(String schemaName, String comment, Map<String, String> properties) |
There was a problem hiding this comment.
Do u modify the method signature? We should keep backwards compatibility.
There was a problem hiding this comment.
Thank you for your review. This issue has been fixed.
| Map<String, SecretReference> secretReferences) | ||
| throws NoSuchMetalakeException, CatalogAlreadyExistsException { | ||
| return getMetalake().createCatalog(catalogName, type, provider, comment, properties); | ||
| return getMetalake() |
There was a problem hiding this comment.
Thank you for your review. This issue has been fixed.
…ages Use descriptive UnsupportedOperationException text consistent with other API defaults, instead of a bare "Not implemented". Co-authored-by: Cursor <cursoragent@cursor.com>
…n messages Align FilesetCatalog/Fileset default messages with other API defaults instead of a bare "Not implemented". Co-authored-by: Cursor <cursoragent@cursor.com>
…ted message Co-authored-by: Cursor <cursoragent@cursor.com>
…ventions Use Fileset/ViewCatalog-style wording for fileset defaults, and keep create-with-secrets messages consistent across api and core dispatchers. Co-authored-by: Cursor <cursoragent@cursor.com>
| schemaEntity -> | ||
| SchemaEntity.builder() | ||
| .withId(schemaEntity.id()) | ||
| .withName(schemaEntity.name()) |
There was a problem hiding this comment.
alterSchema will remove the persisted properties.
There was a problem hiding this comment.
Thank you very much for your review. I chage my code and now alter also has properties.
| }); | ||
| }); | ||
| } catch (RuntimeException e) { | ||
| secretManager.rollbackSecrets(secretMaterials); |
There was a problem hiding this comment.
The concern isn't sharing between schemas but liveness: the catch also covers the code after s.createSchema() has already succeeded (isManagedEntity, getHiddenPropertyNames), so a failure there deletes the secret while the schema still exists in the underlying catalog holding that URN — suggest rolling back only when the schema was not created.
| catalogIdent, | ||
| c -> c.doWithSchemaOps(s -> s.loadSchema(ident)), | ||
| NoSuchSchemaException.class); | ||
| schemaProperties = schema.properties(); |
There was a problem hiding this comment.
schema.properties() is not read properties from entity store, some catalog‘s properties does not contain URN
| * Deletes write-through secrets for a schema and its fileset children using properties snapped | ||
| * before the entities were dropped. Entity drop must succeed before calling this. | ||
| */ | ||
| private void deleteSecretsFromPropertySnapshots( |
There was a problem hiding this comment.
The logic is duplicate with SchemaOperationDispatcher.java snapshotFilesetProperties and deleteSecretsAfterSchemaDrop
There was a problem hiding this comment.
- The concern isn't sharing between schemas but liveness: the catch also covers the code after s.createSchema() has already succeeded (isManagedEntity, getHiddenPropertyNames), so a failure there deletes the secret while the schema still exists in the underlying catalog holding that URN — suggest rolling back only when the schema was not created.
It has been resolved. The solution is similar to that for the catalog.
-
schema.properties() is not read properties from entity store, some catalog‘s properties does not contain URN
Read properties from SchemaEntity (entity store) before dropping -
The logic is duplicate with SchemaOperationDispatcher.java snapshotFilesetProperties and deleteSecretsAfterSchemaDrop
This method has been removed.
…ility Restore the original createSchema/createCatalog/createMultipleLocationFileset methods on Java clients and add secret-parameter overloads. Old methods delegate to the new ones with empty secret maps. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep entity-store properties (including secret URNs) when rebuilding SchemaEntity after alter, instead of dropping them. Co-authored-by: Cursor <cursoragent@cursor.com>
…atalog Update SchemaEntity properties from existing store props plus SchemaChange only, matching CatalogManager.alterCatalog. Co-authored-by: Cursor <cursoragent@cursor.com>
Align schema/fileset create with CatalogManager: set needClean=false after the underlying create succeeds, and roll back write-through secrets only in finally when create did not succeed. Co-authored-by: Cursor <cursoragent@cursor.com>
Match CatalogManager: write secrets inside the locked try/finally so partial create failures share the same needClean rollback path. Co-authored-by: Cursor <cursoragent@cursor.com>
Align with CatalogManager.dropCatalog: capture write-through URNs from SchemaEntity in the store instead of catalog loadSchema().properties(). Co-authored-by: Cursor <cursoragent@cursor.com>
…op cleanup Use getEntity directly in dropSchema instead of a dedicated helper. Co-authored-by: Cursor <cursoragent@cursor.com>
…ade delete Let schema/catalog cascade drop list filesets and call FilesetDispatcher so each fileset cleans its own secrets, instead of snapshotting and deleting fileset secrets from the parent path. Co-authored-by: Cursor <cursoragent@cursor.com>
… drop Wire FilesetDispatcher into SchemaOperationDispatcher via constructor and CatalogManager via setter, instead of looking it up from GravitinoEnv. Co-authored-by: Cursor <cursoragent@cursor.com>
What changes were proposed in this pull request?
Follow-up to #12366 (fileset create-time secrets). Adds create-time
secretBindings/secretReferencesfor catalog and schema:assembleSecretMaterials→writeSecrets; failure rollback viarollbackSecrets; drop cleanup viadeleteSecretsFromPropertiestestConnectionrejects raw secret URNs viacheckSecretKeyssetOwnerfailure (aligned with fileset / [#12297] feat(secret): Support fileset create-time secretBindings/secretReferences #12366)listCatalogsInfoauthorize-before-resolveWhy are the changes needed?
Fix: #12297
Create-time secret bindings/references were added for fileset in #12366;
catalog and schema need the same create-path support.
Does this PR introduce any user-facing change?
secretBindings/secretReferences(same model as fileset).How was this patch tested?