LuciferYang commented on code in PR #13440:
URL: https://github.com/apache/gravitino/pull/13440#discussion_r4396948971
##########
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:
Thanks for the repro. Fixed by giving each alter write its own URN:
`setSecretBinding`, and `setProperty` on a write-through secret the entity
owns, now write the value to `<entityType>:<entityId>:<writeId>:<propertyKey>`
with a random UUID `writeId`, so B never writes to the URN A deletes. Create
keeps the three-segment form, and drop and remove recognize both.
`testDeleteAfterCommitKeepsSecretRewrittenByLaterAlter` covers your
interleaving. This covers the catalog, schema and fileset alters, since they
all prepare through `SecretAlterChanges`. While at it, the external schema path
now deletes the old URN only after the schema entity is updated, because that
update can fail without failing the alter, and a catalog reads its own secrets
when it is built, so a request still using a catalog loaded before the rotation
does not read the deleted URN.
##########
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:
With the new URN per write, a failed same-provider rotation no longer
touches the old material, so the original value is kept. The three tests now
assert the old plaintext.
--
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]
