Immutable secrets and consumer finalizers for TransportURL credential rotation - #608
Immutable secrets and consumer finalizers for TransportURL credential rotation#608lmiccini wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lmiccini The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test infra-operator-build-deploy-kuttl |
7bda5c0 to
aca54b9
Compare
|
Build failed (check pipeline). Post ❌ openstack-k8s-operators-content-provider FAILURE in 11m 20s |
|
recheck |
|
Build failed (check pipeline). Post ❌ openstack-k8s-operators-content-provider FAILURE in 10m 14s |
|
recheck |
aca54b9 to
98d388a
Compare
|
Build failed (check pipeline). Post ❌ openstack-k8s-operators-content-provider FAILURE in 11m 48s |
|
recheck |
|
/test infra-operator-build-deploy-kuttl |
2 similar comments
|
/test infra-operator-build-deploy-kuttl |
|
/test infra-operator-build-deploy-kuttl |
98d388a to
668c7bf
Compare
There was a problem hiding this comment.
The NodeSetSynced field is still present in the status type and is still cleared in releasePendingUser, but the two-phase logic that set it to "false" has been removed. It's only ever set to "" now. Is it still needed?
|
|
||
| if err := r.Create(ctx, secret); err != nil { | ||
| if !k8s_errors.IsAlreadyExists(err) { | ||
| return nil, "", fmt.Errorf("failed to create immutable transport secret %s: %w", secret.Name, err) |
There was a problem hiding this comment.
When r.Create(ctx, secret) returns AlreadyExists, the function returns the local secret object (no server metadata) and the locally-computed contentHash. If a prior reconcile already created the secret but the status patch failed, this works by accident because the name is deterministic. However, it would be cleaner to Get the existing secret on AlreadyExists to confirm the data matches, avoiding a silent mismatch if hash collisions ever occur:
if err := r.Create(ctx, secret); err != nil {
if !k8s_errors.IsAlreadyExists(err) {
return nil, "", fmt.Errorf("failed to create immutable transport secret %s: %w", secret.Name, err)
}
// Fetch the existing secret to confirm content matches
if err := r.Get(ctx, types.NamespacedName{Name: secret.Name, Namespace: secret.Namespace}, secret); err != nil {
return nil, "", fmt.Errorf("failed to get existing immutable transport secret %s: %w", secret.Name, err)
}
}
| if instance.Status.SecretName != "" && instance.Status.SecretName != secret.Name { | ||
| instance.Status.PreviousSecretName = instance.Status.SecretName | ||
| } | ||
| instance.Status.SecretName = secret.Name | ||
| instance.Status.SecretHash = hash |
There was a problem hiding this comment.
This sets PreviousSecretName without a corresponding PreviousRabbitmqUserRef, which then triggers the orphan cleanup block at line 624. This block checks for consumer finalizers on a secret that was never part of a consumer-driven rotation. The state machine has two independent cleanup paths (lines 608-618 for rotation, lines 624-642 for orphans) that overlap and can interact in unexpected ways. Should we either:
- Not setting PreviousSecretName in the normal path (only set it during rotation), or
- Adding a comment explaining this is intentional for cleaning up stale immutable secrets post-rotation?
… rotation
Enable safe credential rotation for TransportURL by creating immutable,
content-hashed secrets and coordinating cleanup across consuming operators.
Key changes:
- Create immutable transport secrets (rabbitmq-transport-url-{name}-{hash})
during rotation so consumers cannot accidentally mutate credentials mid-rollout
- Add per-consumer finalizers (turl.openstack.org/t-{name}) on shared
RabbitMQUser and RabbitMQVhost CRs to track which operators are still
using the old credentials
- Let consuming operators (e.g. Nova, Heat) signal rollout completion by
removing their finalizer, so TransportURL knows when it is safe to
delete the old user
- Gate old user cleanup on both consumer finalizer removal and NodeSet
secret hash synchronization — if the secret is not tracked by the
dataplane, release immediately; otherwise wait for a full NodeSet deploy
- Prevent SecretName flip-flop by comparing content hashes before creating
a new immutable secret
- Auto-delete orphaned RabbitMQUser CRs once all consumers have released
their finalizers
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
668c7bf to
43f4553
Compare
|
@lmiccini: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Enable safe credential rotation for TransportURL by creating immutable, content-hashed secrets and coordinating cleanup across consuming operators.