Merge pull request #723 from Deydra71/appcred-finalizer-split

Split appcred finalizer management

Author: openshift-merge-bot[bot]

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() {

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(

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,