fix: requeue invalid credentials instead of watching the Secret

Author: Herbaert

controller/constants.go

@@ -19,8 +19,10 @@ const (
 
 	retryableErrorRequeueAfter = 5 * time.Second
 
-	// deleteRequeueAfter paces the wait for dependent objects to disappear
-	// during deletion. Matches what Cluster API and the other infrastructure
-	// providers use for the same purpose.
+	// deleteRequeueAfter paces the wait for dependent objects to disappear during deletion.
 	deleteRequeueAfter = 5 * time.Second
+
+	// credentialsRetryRequeueAfter paces retries of invalid credentials, which
+	// need an operator to fix the Secret before they can succeed.
+	credentialsRetryRequeueAfter = time.Minute
 )

controller/stackitcluster_controller.go

@@ -150,52 +150,13 @@ func (r *StackitClusterReconciler) stackitClusterRequestsForCloudInitRef(ctx con
 	return requests
 }
 
-// stackitClusterRequestsForCredentialsSecret enqueues every StackitCluster whose
-// credentialsSecretRef points at the given Secret.
-//
-// Without it, correcting an invalid credentials Secret never reaches the
-// cluster: CredentialFailureResult deliberately returns without a requeue,
-// because retrying invalid credentials in a hot loop helps nobody — which only
-// works if fixing them triggers a reconcile.
-func (r *StackitClusterReconciler) stackitClusterRequestsForCredentialsSecret(ctx context.Context, obj client.Object) []reconcile.Request {
-	secret, ok := obj.(*corev1.Secret)
-	if !ok {
-		return nil
-	}
-
-	// Listed across all namespaces on purpose: CredentialsSecretRef.Namespace is
-	// optional, so the Secret may well live somewhere other than the
-	// StackitCluster that references it.
-	clusters := &infrav1.StackitClusterList{}
-	if err := r.List(ctx, clusters); err != nil {
-		logf.FromContext(ctx).Error(err, "Failed to list StackitClusters for credentials Secret watch", "secret", client.ObjectKeyFromObject(secret))
-		return nil
-	}
-
-	secretKey := client.ObjectKeyFromObject(secret)
-	requests := make([]reconcile.Request, 0, len(clusters.Items))
-	for _, cluster := range clusters.Items {
-		if util.CredentialsSecretKey(&cluster) != secretKey {
-			continue
-		}
-		requests = append(requests, reconcile.Request{
-			NamespacedName: types.NamespacedName{
-				Namespace: cluster.Namespace,
-				Name:      cluster.Name,
-			},
-		})
-	}
-	return requests
-}
-
 // SetupWithManager registers the controller with the manager.
 func (r *StackitClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&infrav1.StackitCluster{}).
 		Watches(&clusterv1.Cluster{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCluster)).
 		Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)).
 		Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCloudInitRef)).
-		Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForCredentialsSecret)).
 		Named("stackitcluster").
 		Complete(r)
 }

controller/stackitcluster_controller_test.go

@@ -22,7 +22,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	infrav1 "github.com/stackitcloud/cluster-api-provider-stackit/api/v1alpha1"
@@ -500,19 +499,34 @@ var _ = Describe("StackitCluster Controller", func() {
 		expectCondition(got.Status.Conditions, infrav1.ClusterReadyCondition, metav1.ConditionFalse, "NetworkNotFound")
 	})
 
-	It("marks credentials invalid without requeueing on unauthorized credentials", func() {
+	// The requeue is what lets a corrected Secret take effect: nothing else
+	// enqueues the cluster once the credentials are rejected.
+	It("requeues on unauthorized credentials and recovers once they are corrected", func() {
 		reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) {
 			return nil, fmt.Errorf("authenticate: %w", cloud.ErrUnauthorized)
 		}
 
 		result, err := reconciler.Reconcile(ctx, request)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(result).To(Equal(reconcile.Result{}))
+		Expect(result.RequeueAfter).To(Equal(credentialsRetryRequeueAfter))
 
 		got := &infrav1.StackitCluster{}
 		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		Expect(got.Status.Ready).To(BeFalse())
 		expectCondition(got.Status.Conditions, infrav1.ClusterCredentialsReadyCondition, metav1.ConditionFalse, "CredentialsInvalid")
 		expectCondition(got.Status.Conditions, infrav1.ClusterReadyCondition, metav1.ConditionFalse, "CredentialsInvalid")
+
+		By("correcting the credentials")
+		reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) {
+			return fakeCloud, nil
+		}
+
+		_, err = reconciler.Reconcile(ctx, request)
+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		Expect(got.Status.Ready).To(BeTrue())
+		expectCondition(got.Status.Conditions, infrav1.ClusterCredentialsReadyCondition, metav1.ConditionTrue, "Available")
 	})
 
 	It("does not call the cloud API when the owning Cluster is paused", func() {
@@ -723,48 +737,6 @@ var _ = Describe("StackitCluster Controller", func() {
 		Expect(requests).To(Equal([]reconcile.Request{request}))
 	})
 
-	It("maps credentials Secret events to StackitCluster reconcile requests", func() {
-		secret := &corev1.Secret{}
-		Expect(k8sClient.Get(ctx, types.NamespacedName{Name: credentials, Namespace: namespace}, secret)).To(Succeed())
-
-		requests := reconciler.stackitClusterRequestsForCredentialsSecret(ctx, secret)
-		Expect(requests).To(Equal([]reconcile.Request{request}))
-	})
-
-	It("maps credentials Secret events from a different namespace than the StackitCluster", func() {
-		otherNamespace := "credentials-elsewhere"
-		Expect(client.IgnoreAlreadyExists(k8sClient.Create(ctx, &corev1.Namespace{
-			ObjectMeta: metav1.ObjectMeta{Name: otherNamespace},
-		}))).To(Succeed())
-
-		otherCredentials := credentials + "-elsewhere"
-		createCredentialsSecret(ctx, otherCredentials, otherNamespace, testProjectID)
-		DeferCleanup(func() {
-			deleteIfExists(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: otherCredentials, Namespace: otherNamespace}})
-		})
-
-		got := &infrav1.StackitCluster{}
-		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
-		got.Spec.CredentialsSecretRef = corev1.SecretReference{Name: otherCredentials, Namespace: otherNamespace}
-		Expect(k8sClient.Update(ctx, got)).To(Succeed())
-
-		secret := &corev1.Secret{}
-		Expect(k8sClient.Get(ctx, types.NamespacedName{Name: otherCredentials, Namespace: otherNamespace}, secret)).To(Succeed())
-
-		requests := reconciler.stackitClusterRequestsForCredentialsSecret(ctx, secret)
-		Expect(requests).To(Equal([]reconcile.Request{request}),
-			"credentialsSecretRef.namespace is optional, so the mapper must not be limited to the Secret's own namespace")
-	})
-
-	It("ignores Secret events that no StackitCluster uses as credentials", func() {
-		secret := &corev1.Secret{}
-		Expect(k8sClient.Get(ctx, types.NamespacedName{Name: credentials, Namespace: namespace}, secret)).To(Succeed())
-		secret.Name = credentials + "-unused"
-
-		requests := reconciler.stackitClusterRequestsForCredentialsSecret(ctx, secret)
-		Expect(requests).To(BeEmpty())
-	})
-
 	It("ignores Cluster events for other infrastructure providers", func() {
 		cluster := &clusterv1.Cluster{
 			ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: namespace},

controller/stackitcluster_infrastructure.go

@@ -57,6 +57,7 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS
 			&stackitCluster.Status.Conditions,
 			stackitCluster.Generation,
 			err,
+			credentialsRetryRequeueAfter,
 			infrav1.ClusterCredentialsReadyCondition,
 			infrav1.ClusterReadyCondition,
 		)

controller/stackitmachine_controller_test.go

@@ -220,7 +220,7 @@ var _ = Describe("StackitMachine Controller", func() {
 		expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InvalidFailureDomain")
 	})
 
-	It("marks credentials invalid without requeueing on unauthorized credentials", func() {
+	It("requeues on unauthorized credentials and recovers once they are corrected", func() {
 		updateMachineBootstrapSecret(ctx, machineName, bootstrapName)
 		createBootstrapSecret(ctx, bootstrapName)
 		reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) {
@@ -229,13 +229,25 @@ var _ = Describe("StackitMachine Controller", func() {
 
 		result, err := reconciler.Reconcile(ctx, request)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(result).To(Equal(reconcile.Result{}))
+		Expect(result.RequeueAfter).To(Equal(credentialsRetryRequeueAfter))
 		Expect(fakeCloud.ServerCount()).To(Equal(0))
 
 		got := &infrav1.StackitMachine{}
 		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
 		expectCondition(got.Status.Conditions, infrav1.MachineCredentialsReadyCondition, metav1.ConditionFalse, "CredentialsInvalid")
 		expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "CredentialsInvalid")
+
+		By("correcting the credentials")
+		reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) {
+			return fakeCloud, nil
+		}
+
+		_, err = reconciler.Reconcile(ctx, request)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(fakeCloud.ServerCount()).To(Equal(1))
+
+		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		expectCondition(got.Status.Conditions, infrav1.MachineCredentialsReadyCondition, metav1.ConditionTrue, "Available")
 	})
 
 	It("does not call the cloud API when the owning Cluster is paused", func() {

controller/stackitmachine_infrastructure.go

@@ -78,6 +78,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS
 			&stackitMachine.Status.Conditions,
 			stackitMachine.Generation,
 			err,
+			credentialsRetryRequeueAfter,
 			infrav1.MachineCredentialsReadyCondition,
 			infrav1.MachineReadyCondition,
 		)
@@ -193,6 +194,7 @@ func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineS
 			&stackitMachine.Status.Conditions,
 			stackitMachine.Generation,
 			err,
+			credentialsRetryRequeueAfter,
 			infrav1.MachineCredentialsReadyCondition,
 		)
 		return resultErr

util/conditions.go

@@ -81,15 +81,21 @@ func SetConditions(
 	}
 }
 
+// CredentialFailureResult records the failure and decides how to retry.
+//
+// Invalid credentials need an operator to fix the Secret, so they requeue on a
+// slow timer rather than returning an error and spinning through the controller
+// backoff. The reconcile reads the Secret again, which is what picks up the fix.
 func CredentialFailureResult(
 	conditions *[]metav1.Condition,
 	generation int64,
 	err error,
+	requeueAfter time.Duration,
 	conditionTypes ...string,
 ) (ctrl.Result, error) {
 	SetConditions(conditions, generation, metav1.ConditionFalse, "CredentialsInvalid", err.Error(), conditionTypes...)
 	if cloud.IsUnauthorized(err) || cloud.IsInvalidInput(err) || errors.Is(err, ErrCredentialsInvalid) {
-		return ctrl.Result{}, nil
+		return ctrl.Result{RequeueAfter: requeueAfter}, nil
 	}
 	return ctrl.Result{}, err
 }