From bcf30e960acfbeb53df839b5fba0d41158a536bd Mon Sep 17 00:00:00 2001 From: Veronika Fisarova Date: Mon, 25 May 2026 14:49:04 +0200 Subject: [PATCH] Split appcred finalizer management The AC finalizer management is now split into two phases: Early phase: adds consumer finalizer to the new AC secret immediately (protects it from premature revocation) Late phase: removes consumer finalizer from the old AC secret only after AllSubConditionIsTrue() This prevents a race condition where rapid AC rotations could revoke credentials still in use by running pods. Signed-off-by: Veronika Fisarova --- internal/controller/ironic_controller.go | 46 +++++++++++++++++++---- test/functional/base_test.go | 22 +++++++++++ test/functional/ironic_controller_test.go | 11 ++++++ 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/internal/controller/ironic_controller.go b/internal/controller/ironic_controller.go index 4f250c2c..08bdaf2c 100644 --- a/internal/controller/ironic_controller.go +++ b/internal/controller/ironic_controller.go @@ -508,11 +508,12 @@ func (r *IronicReconciler) reconcileNormal(ctx context.Context, instance *ironic } // Create ConfigMaps and Secrets - end - // Manage consumer finalizer for ironic AC secret - if instance.Spec.Auth.ApplicationCredentialSecret != "" || instance.Status.ApplicationCredentialSecret != "" { + // Add consumer finalizer to the new ironic AC secret (early phase of the + // split pattern -- old secret finalizer removal is deferred to end of reconcile). + if instance.Spec.Auth.ApplicationCredentialSecret != "" { if err := keystonev1.ManageACSecretFinalizer(ctx, helper, instance.Namespace, instance.Spec.Auth.ApplicationCredentialSecret, - instance.Status.ApplicationCredentialSecret, + "", ironic.ACConsumerFinalizer); err != nil { instance.Status.Conditions.Set(condition.FalseCondition( condition.ServiceConfigReadyCondition, @@ -523,13 +524,12 @@ func (r *IronicReconciler) reconcileNormal(ctx context.Context, instance *ironic return ctrl.Result{}, err } } - instance.Status.ApplicationCredentialSecret = instance.Spec.Auth.ApplicationCredentialSecret - // Manage consumer finalizer for ironic-inspector AC secret - if instance.Spec.IronicInspector.Auth.ApplicationCredentialSecret != "" || instance.Status.InspectorApplicationCredentialSecret != "" { + // Add consumer finalizer to the new ironic-inspector AC secret. + if instance.Spec.IronicInspector.Auth.ApplicationCredentialSecret != "" { if err := keystonev1.ManageACSecretFinalizer(ctx, helper, instance.Namespace, instance.Spec.IronicInspector.Auth.ApplicationCredentialSecret, - instance.Status.InspectorApplicationCredentialSecret, + "", ironic.InspectorACConsumerFinalizer); err != nil { instance.Status.Conditions.Set(condition.FalseCondition( condition.ServiceConfigReadyCondition, @@ -540,7 +540,6 @@ func (r *IronicReconciler) reconcileNormal(ctx context.Context, instance *ironic return ctrl.Result{}, err } } - instance.Status.InspectorApplicationCredentialSecret = instance.Spec.IronicInspector.Auth.ApplicationCredentialSecret instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage) @@ -793,6 +792,37 @@ func (r *IronicReconciler) reconcileNormal(ctx context.Context, instance *ironic return ctrl.Result{}, err } + // Late phase of the AC split pattern: remove the old AC secret's finalizer + // and update status only after all sub-services are ready with the new + // credentials. This prevents premature revocation during rapid rotations. + isIronicRotation := instance.Status.ApplicationCredentialSecret != "" && + instance.Status.ApplicationCredentialSecret != instance.Spec.Auth.ApplicationCredentialSecret + if isIronicRotation { + if instance.Status.Conditions.AllSubConditionIsTrue() { + if err := keystonev1.RemoveACSecretConsumerFinalizer(ctx, helper, instance.Namespace, + instance.Status.ApplicationCredentialSecret, ironic.ACConsumerFinalizer); err != nil { + return ctrl.Result{}, err + } + instance.Status.ApplicationCredentialSecret = instance.Spec.Auth.ApplicationCredentialSecret + } + } else { + instance.Status.ApplicationCredentialSecret = instance.Spec.Auth.ApplicationCredentialSecret + } + + isInspectorRotation := instance.Status.InspectorApplicationCredentialSecret != "" && + instance.Status.InspectorApplicationCredentialSecret != instance.Spec.IronicInspector.Auth.ApplicationCredentialSecret + if isInspectorRotation { + if instance.Status.Conditions.AllSubConditionIsTrue() { + if err := keystonev1.RemoveACSecretConsumerFinalizer(ctx, helper, instance.Namespace, + instance.Status.InspectorApplicationCredentialSecret, ironic.InspectorACConsumerFinalizer); err != nil { + return ctrl.Result{}, err + } + instance.Status.InspectorApplicationCredentialSecret = instance.Spec.IronicInspector.Auth.ApplicationCredentialSecret + } + } else { + instance.Status.InspectorApplicationCredentialSecret = instance.Spec.IronicInspector.Auth.ApplicationCredentialSecret + } + // We reached the end of the Reconcile, update the Ready condition based on // the sub conditions if instance.Status.Conditions.AllSubConditionIsTrue() { diff --git a/test/functional/base_test.go b/test/functional/base_test.go index 5456ffd2..7163f37f 100644 --- a/test/functional/base_test.go +++ b/test/functional/base_test.go @@ -734,6 +734,28 @@ func GetSampleTopologySpec(label string) (map[string]any, []corev1.TopologySprea return topologySpec, topologySpecObj } +func simulateIronicSubServicesReady(names IronicNames) { + th.SimulateJobSuccess(names.IronicDBSyncJobName) + keystone.SimulateKeystoneServiceReady(names.IronicName) + keystone.SimulateKeystoneEndpointReady(names.IronicName) + infra.GetTransportURL(names.InspectorTransportURLName) + infra.SimulateTransportURLReady(names.InspectorTransportURLName) + mariadb.GetMariaDBDatabase(names.InspectorDatabaseName) + mariadb.SimulateMariaDBAccountCompleted(names.InspectorDatabaseAccount) + mariadb.SimulateMariaDBDatabaseCompleted(names.InspectorDatabaseName) + th.SimulateJobSuccess(names.InspectorDBSyncJobName) + keystone.SimulateKeystoneServiceReady(names.InspectorName) + keystone.SimulateKeystoneEndpointReady(names.InspectorName) + nestedINATransportURLName := names.INATransportURLName + nestedINATransportURLName.Name = names.IronicName.Name + "-" + nestedINATransportURLName.Name + infra.GetTransportURL(nestedINATransportURLName) + infra.SimulateTransportURLReady(nestedINATransportURLName) + th.SimulateDeploymentReplicaReady(names.IronicName) + th.SimulateStatefulSetReplicaReady(names.ConductorName) + th.SimulateStatefulSetReplicaReady(names.InspectorName) + th.SimulateDeploymentReplicaReady(names.INAName) +} + // CreateIronicInvalidSecret creates a secret with an invalid password for testing func CreateIronicInvalidSecret(namespace string, name string) *corev1.Secret { return th.CreateSecret( diff --git a/test/functional/ironic_controller_test.go b/test/functional/ironic_controller_test.go index b42d5a7c..4253fb01 100644 --- a/test/functional/ironic_controller_test.go +++ b/test/functional/ironic_controller_test.go @@ -1736,6 +1736,7 @@ var _ = Describe("Ironic controller", func() { }) It("should track the consumed AC secrets in status", func() { + th.SimulateJobSuccess(ironicNames.IronicDBSyncJobName) Eventually(func(g Gomega) { i := GetIronic(ironicNames.IronicName) g.Expect(i.Status.ApplicationCredentialSecret).To(Equal(acIronicSecretName)) @@ -1753,6 +1754,12 @@ var _ = Describe("Ironic controller", func() { ContainElement(ironic.ACConsumerFinalizer)) }, timeout, interval).Should(Succeed()) + simulateIronicSubServicesReady(ironicNames) + Eventually(func(g Gomega) { + i := GetIronic(ironicNames.IronicName) + g.Expect(i.Status.Conditions.IsTrue(condition.ReadyCondition)).To(BeTrue()) + }, timeout, interval).Should(Succeed()) + newACSecretName := "ac-ironic-rotated-x9y8z7-secret" //nolint:gosec // G101 newSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -1783,6 +1790,10 @@ var _ = Describe("Ironic controller", func() { }, timeout, interval).Should(Succeed()) Eventually(func(g Gomega) { + th.SimulateDeploymentReplicaReady(ironicNames.IronicName) + th.SimulateStatefulSetReplicaReady(ironicNames.ConductorName) + th.SimulateStatefulSetReplicaReady(ironicNames.InspectorName) + th.SimulateDeploymentReplicaReady(ironicNames.INAName) secret := th.GetSecret(types.NamespacedName{ Namespace: ironicNames.Namespace, Name: acIronicSecretName,