yuqi1129 commented on code in PR #13440:
URL: https://github.com/apache/gravitino/pull/13440#discussion_r4386487938
##########
core/src/main/java/org/apache/gravitino/secret/SecretMaterialsHolder.java:
##########
@@ -39,4 +44,33 @@ public List<SecretMaterial> get() {
public void set(List<SecretMaterial> materials) {
this.materials = materials;
}
+
+ /**
+ * Returns the URNs of write-through secrets replaced or removed by the
prepared changes. They are
+ * deleted only after the alter commits; on failure they must stay
resolvable because the
+ * persisted entity still references them.
+ */
+ public List<SecretUrn> getReplacedUrns() {
+ return replacedUrns;
+ }
+
+ /** Records the URNs of replaced write-through secrets for post-commit
deletion. */
+ public void setReplacedUrns(List<SecretUrn> replacedUrns) {
+ this.replacedUrns = replacedUrns;
+ }
+
+ /**
+ * Deletes the replaced write-through secrets after the alter commits. Best
effort: the committed
+ * entity no longer references them, so a failure only leaks material and
must not fail the
+ * committed alter.
+ *
+ * @param secretManager secret manager used for deletion
+ */
+ public void deleteReplaced(SecretManager secretManager) {
+ try {
+ secretManager.deleteSecrets(replacedUrns);
Review Comment:
[Blocker] The committed alter's property snapshot does not establish that
these deterministic URNs are still unreferenced when deletion actually runs.
Another server can rebind the same property between the metadata commit and
this provider call:
1. A removes the property and commits, collecting URN U for deletion.
2. B writes new material to the same deterministic U, rebinds the property,
and commits successfully.
3. A runs this deferred deletion and deletes B's live material.
I reproduced this through two `FilesetCatalogOperations` instances, the
actual H2 relational store, and the real in-memory provider, with a controlled
interleaving immediately before A's provider deletion. Both alters succeed; the
stored fileset still references B's U, but reading it then throws `Secret not
found`. This models two servers sharing a provider; the node-local tree lock
cannot exclude that ordering.
Please bind cleanup to the old material's version/unique identity, or
coordinate rebinding and deletion across nodes. Re-reading current properties
immediately before deletion alone still leaves a check/delete race. The same
cleanup mechanism is used by catalog and schema alters, so the protection needs
to cover those paths too.
##########
core/src/main/java/org/apache/gravitino/secret/SecretManager.java:
##########
@@ -395,36 +405,48 @@ public String alterSetSecretBinding(
if (current != null
&& !current.equals(newUrn)
&& SecretPropertyUtils.isWriteThroughForEntity(property, current,
entityType, entityId)) {
- deleteSecretsFromProperties(Map.of(property, current));
+ // Defer deletion until the alter commits: an aborted alter leaves the
persisted
+ // entity referencing this URN, so the material must stay resolvable.
+ replacedUrns.add(SecretUrn.parse(current));
}
List<SecretMaterial> materials = List.of(new SecretMaterial(urns.get(0),
binding.plaintext()));
writeSecrets(materials);
- written.addAll(materials);
+ if (!newUrn.equals(current)) {
Review Comment:
[Question] Could we explicitly document and test the remaining failure
semantics here? A same-provider rotation overwrites the original material
before commit, and skipping rollback preserves the URN but does not restore the
original value.
I strengthened the assertions in
`testFailedSameProviderRotationKeepsUrnResolvable`,
`testFailedRemoveAndRebindKeepsUrnResolvable`, and
`testPostPrepareRollbackKeepsReboundUrnResolvable` from non-null to the
original plaintext. All three report `expected: old, actual: new`: the alter is
rejected, yet the secret value has changed.
This is a remaining pre-existing behavior rather than a new regression. If
this PR intentionally guarantees only URN resolvability, please make the
retained new value after failure an explicit documented/tested tradeoff and
track restoring the original value separately. `assertNotNull` currently leaves
that user-visible outcome unspecified.
--
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]
