Updating a secret with --asm will return an error, but appears to work

[wesbillman]

When attempting to update an ASM secret, we get the following errors.

❯ ftl --endpoint=https://[redacted] secret set --asm [redacted].FTL_DSN_[redacted]_[redacted]
Secret:
ftl: error: unknown: failed to set secret: failed to set secret URL: could not set secret URL: duplicate key value violates unique constraint "module_secrets_module_name_key": conflict

❯ ftl --endpoint=https://[redacted] secret unset --asm [redacted].FTL_DSN_[redacted]_[redacted]

❯ ftl --endpoint=https://[redacted] secret set --asm [redacted].FTL_DSN_[redacted]_[redacted]
Secret:
ftl: error: unknown: failed to set secret: asm/leader: unable to store secret in ASM: operation error Secrets Manager: CreateSecret, https response error StatusCode: 400, RequestID: 498ad9de-f3d5-466d-a0b1-733ee040cb03, InvalidRequestException: You can't create this secret because a secret with this name is already scheduled for deletion.

❯ ftl --endpoint=https://[redacted] secret set --asm [redacted].FTL_DSN_[redacted]_[redacted]
Secret:
ftl: error: unknown: failed to set secret: failed to set secret URL: could not set secret URL: duplicate key value violates unique constraint "module_secrets_module_name_key": conflict

Likely coming from here when a secret already exists:

https://github.com/TBD54566975/ftl/blob/main/common/configuration/asm_leader.go#L139-L164

[matt2e]

Putting this back into the next pile because I didn't get to it today

[deniseli]

Looks like duplicate key value violates unique constraint "module_secrets_module_name_key": conflict comes from the module_secrets table's uniqueness constraint coupled with the query SetModuleSecretURL just running INSERT INTO without an ON CONFLICT clause. This logic was largely copied from the module_config, so we'll need to fix that too (though lower priority). (Totally my fault since I introduced the module_config bug...)

[deniseli]

If setting the secret appears to work despite the error, then I'm guessing that postgres reports an error when the uniqueness constraint is violated, but it doesn't actually kill or revert the operation. So if the postgresql engine executes lookups by scanning starting from the most recent rows instead of scanning from the oldest, then it would pick up the most recently set value.

[deniseli]

Notes on the ASM issue:

The premise behind You can't create this secret because a secret with this name is already scheduled for deletion. is a bit flawed. If someone is trying to recreate a secret that they previously executed a deletion for, they're almost certainly just trying to replace the value, so instead of erroring for that case, we should instead remove the deletion request from the queue and instead treat the insertion as a replace operation. Since secrets are held to the uniqueness constraint anyway, so we should always replace if one already exists anyway, it would be sufficient to just remove the deletion request from the queue. This is something we should fix.

That said:

• Fixing the above probably isn't urgent enough to prioritize now because the only reason we even hit that issue was because we were trying to work around the other issue: the postgresql error. So it probably makes the most sense to just fix the sql query bug and stop there for now.
• I couldn't find the "You can't create this secret because a secret with this name is already scheduled for deletion" error anywhere in the FTL codebase by grepping for it... did we already remove/fix this, or is it somehow coming from somewhere else?

[deniseli]

Another thought - if we've been inserting duplicate key pairs into the module_secrets table this whole time, then we'll need to clear out the duplicates (unless we're already wiping everything)... the duplicate clearing would need to keep only the last-inserted record for each (module, name) pair and delete all prior ones to get rid of the duplicates.