refactor: replace custom recorder with capi provided package and add additional error logging and event recording

cmd/manager/main.go

@@ -29,6 +29,7 @@
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
+	"sigs.k8s.io/cluster-api/util/record"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -182,11 +183,12 @@
 		os.Exit(1)
 	}
 
+	record.InitFromRecorder(mgr.GetEventRecorderFor("cluster-api-provider-stackit"))
+
 	if err := (&controller.StackitClusterReconciler{
 		Client:             mgr.GetClient(),
 		Scheme:             mgr.GetScheme(),
 		CloudClientFactory: cloud.NewClient,
-		Recorder:           mgr.GetEventRecorder("stackitcluster-controller"),
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "Failed to create controller", "controller", "stackitcluster")
 		os.Exit(1)
@@ -195,7 +197,6 @@
 		Client:             mgr.GetClient(),
 		Scheme:             mgr.GetScheme(),
 		CloudClientFactory: cloud.NewClient,
-		Recorder:           mgr.GetEventRecorder("stackitmachine-controller"),
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "Failed to create controller", "controller", "stackitmachine")
 		os.Exit(1)

controller/controller_test_helpers_test.go

@@ -18,7 +18,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/tools/events"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
@@ -204,16 +203,3 @@ func deleteIfExists(ctx context.Context, obj client.Object) {
 		Expect(apierrors.IsNotFound(err)).To(BeTrue())
 	}
 }
-
-// drainEvents returns all events recorded so far.
-func drainEvents(recorder *events.FakeRecorder) []string {
-	var out []string
-	for {
-		select {
-		case event := <-recorder.Events:
-			out = append(out, event)
-		default:
-			return out
-		}
-	}
-}

controller/stackitcluster_bastion.go

@@ -22,7 +22,9 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/cluster-api/util/record"
 	ctrl "sigs.k8s.io/controller-runtime"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
 	infrav1 "github.com/stackitcloud/cluster-api-provider-stackit/api/v1alpha1"
 	"github.com/stackitcloud/cluster-api-provider-stackit/cloud"
@@ -39,6 +41,7 @@ func (r *StackitClusterReconciler) reconcileBastion(
 	cloudClient cloud.Client,
 	clusterScope *scope.ClusterScope,
 ) (ctrl.Result, bool, error) {
+	log := logf.FromContext(ctx)
 	stackitCluster := clusterScope.StackitCluster
 	input := bastionservice.Input(stackitCluster, nil)
 	status := cloud.Bastion{
@@ -61,9 +64,7 @@ func (r *StackitClusterReconciler) reconcileBastion(
 				return ctrl.Result{}, false, err
 			}
 			clusterScope.ClearBastionStatus()
-			if r.Recorder != nil {
-				r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion")
-			}
+			record.Event(stackitCluster, "BastionDeleted", "Deleted bastion")
 		}
 		clusterScope.SetConditions(
 			metav1.ConditionTrue,
@@ -75,6 +76,8 @@ func (r *StackitClusterReconciler) reconcileBastion(
 	}
 
 	if err := validateBastionSpec(stackitCluster.Spec.Bastion); err != nil {
+		log.Error(err, "Invalid bastion spec", "cluster", stackitCluster.Name)
+		record.Warnf(stackitCluster, "InvalidBastionSpec", "Invalid bastion spec: %v", err)
 		clusterScope.SetNotReady(
 			"InvalidBastionSpec",
 			err.Error(),
@@ -86,6 +89,8 @@ func (r *StackitClusterReconciler) reconcileBastion(
 
 	cloudInit, err := r.resolveBastionCloudInit(ctx, stackitCluster)
 	if err != nil {
+		log.Error(err, "Failed to resolve bastion cloud-init", "cluster", stackitCluster.Name)
+		record.Warnf(stackitCluster, "CloudInitRefError", "Failed to resolve bastion cloudInitRef: %v", err)
 		clusterScope.SetNotReady(
 			"CloudInitRefError",
 			err.Error(),
@@ -110,12 +115,10 @@ func (r *StackitClusterReconciler) reconcileBastion(
 			infrav1.ClusterBastionReadyCondition,
 			infrav1.ClusterReadyCondition,
 		)
-		if r.Recorder != nil {
-			r.Recorder.Eventf(
-				stackitCluster, nil, corev1.EventTypeNormal, "BastionRecreating", "Recreate",
-				"Recreating bastion because cloudInitRef content changed",
-			)
-		}
+		record.Event(
+			stackitCluster, "BastionRecreating",
+			"Recreating bastion because cloudInitRef content changed",
+		)
 		return ctrl.Result{RequeueAfter: retryableErrorRequeueAfter}, false, nil
 	}
 
@@ -125,9 +128,9 @@ func (r *StackitClusterReconciler) reconcileBastion(
 		return ctrl.Result{}, false, err
 	}
 	clusterScope.SetBastionStatus(bastion, bastionCloudInitHash(cloudInit))
-	if !hadBastionStatus && r.Recorder != nil {
-		r.Recorder.Eventf(
-			stackitCluster, nil, corev1.EventTypeNormal, "BastionCreated", "Create", "Created bastion %s", bastion.ServerID,
+	if !hadBastionStatus {
+		record.Eventf(
+			stackitCluster, "BastionCreated", "Created bastion %s", bastion.ServerID,
 		)
 	}
 	if ready, reason, message, warn := serverStateCondition(bastion.ServerState, bastion.ServerPowerStatus); !ready {
@@ -137,8 +140,8 @@ func (r *StackitClusterReconciler) reconcileBastion(
 		}
 		clusterScope.SetNotReady(reason, "bastion "+message, infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition)
 		// Only warn on entering the state, not on every requeue.
-		if warn && r.Recorder != nil && previousReason != reason {
-			r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeWarning, reason, "Reconcile", "Bastion server %s: %s", bastion.ServerID, message)
+		if warn && previousReason != reason {
+			record.Warnf(stackitCluster, reason, "Bastion server %s: %s", bastion.ServerID, message)
 		}
 		return ctrl.Result{RequeueAfter: 15 * time.Second}, false, nil
 	}

controller/stackitcluster_controller.go

@@ -27,7 +27,6 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/tools/events"
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
 	clusterutil "sigs.k8s.io/cluster-api/util"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -53,7 +52,6 @@ type StackitClusterReconciler struct {
 	// CloudClientFactory builds a cloud.Client from parsed credentials. It is
 	// injected so tests can swap in the in-memory fake.
 	CloudClientFactory cloud.Factory
-	Recorder           events.EventRecorder
 }
 
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=stackitclusters,verbs=get;list;watch;create;update;patch;delete

controller/stackitcluster_controller_test.go

@@ -22,7 +22,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/tools/events"
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -140,7 +139,7 @@ var _ = Describe("StackitCluster Controller", func() {
 		expectCondition(got.Status.Conditions, infrav1.ClusterBastionReadyCondition, metav1.ConditionTrue, "Available")
 	})
 
-	It("reports a failed bastion server with its own reason and warns", func() {
+	It("reports a failed bastion server with its own reason", func() {
 		got := &infrav1.StackitCluster{}
 		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
 		got.Spec.Bastion = validBastionSpec()
@@ -150,8 +149,6 @@ var _ = Describe("StackitCluster Controller", func() {
 
 		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
 		fakeCloud.SetServerState(got.Status.Bastion.ServerID, "ERROR", "")
-		recorder := events.NewFakeRecorder(10)
-		reconciler.Recorder = recorder
 
 		result, err := reconciler.Reconcile(ctx, request)
 		Expect(err).NotTo(HaveOccurred())
@@ -161,7 +158,6 @@ var _ = Describe("StackitCluster Controller", func() {
 		expectCondition(got.Status.Conditions, infrav1.ClusterBastionReadyCondition, metav1.ConditionFalse, "InstanceFailed")
 		condition := meta.FindStatusCondition(got.Status.Conditions, infrav1.ClusterBastionReadyCondition)
 		Expect(condition.Message).To(Equal("bastion server failed (state ERROR)"))
-		Expect(drainEvents(recorder)).To(ContainElement(HavePrefix("Warning InstanceFailed")))
 	})
 
 	It("deletes existing bastion resources when bastion is disabled", func() {
@@ -502,6 +498,36 @@ var _ = Describe("StackitCluster Controller", func() {
 		expectCondition(got.Status.Conditions, infrav1.ClusterReadyCondition, metav1.ConditionFalse, "NetworkNotFound")
 	})
 
+	It("marks conditions not ready when API server load balancer creation fails", func() {
+		fakeCloud.FailNextEnsureLB = fmt.Errorf("load balancer creation rejected: %w", cloud.ErrInvalidInput)
+
+		result, err := reconciler.Reconcile(ctx, request)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(result).To(Equal(reconcile.Result{}))
+
+		got := &infrav1.StackitCluster{}
+		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		Expect(got.Status.Ready).To(BeFalse())
+		expectCondition(got.Status.Conditions, infrav1.ClusterLoadBalancerReadyCondition, metav1.ConditionFalse, "LoadBalancerError")
+	})
+
+	It("marks conditions not ready when bastion creation fails", func() {
+		got := &infrav1.StackitCluster{}
+		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		got.Spec.Bastion = validBastionSpec()
+		Expect(k8sClient.Update(ctx, got)).To(Succeed())
+
+		fakeCloud.FailNextEnsureBastion = fmt.Errorf("bastion creation rejected: %w", cloud.ErrInvalidInput)
+
+		result, err := reconciler.Reconcile(ctx, request)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(result).To(Equal(reconcile.Result{}))
+
+		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		Expect(got.Status.Ready).To(BeFalse())
+		expectCondition(got.Status.Conditions, infrav1.ClusterBastionReadyCondition, metav1.ConditionFalse, "BastionError")
+	})
+
 	It("requeues when network lookup returns a transient error", func() {
 		fakeCloud.FailNextGetNetwork = fmt.Errorf("temporary network lookup failure: %w", cloud.ErrTransient)
 
@@ -612,6 +638,22 @@ var _ = Describe("StackitCluster Controller", func() {
 		}).Should(BeTrue())
 	})
 
+	It("returns an error when API server load balancer deletion fails", func() {
+		_, err := reconciler.Reconcile(ctx, request)
+		Expect(err).NotTo(HaveOccurred())
+
+		got := &infrav1.StackitCluster{}
+		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		Expect(got.Status.APIServerLoadBalancerID).NotTo(BeEmpty())
+
+		Expect(k8sClient.Delete(ctx, got)).To(Succeed())
+
+		fakeCloud.FailNextDeleteLB = fmt.Errorf("delete load balancer timeout: %w", cloud.ErrTransient)
+
+		_, err = reconciler.Reconcile(ctx, request)
+		Expect(err).To(HaveOccurred())
+	})
+
 	// GetNetwork is the first cloud call of the reconcile, so a hook there proves
 	// the ordering rather than only the end result.
 	It("persists the finalizer before the first cloud call", func() {
@@ -723,32 +765,6 @@ var _ = Describe("StackitCluster Controller", func() {
 		expectCondition(got.Status.Conditions, infrav1.ClusterLoadBalancerReadyCondition, metav1.ConditionTrue, "Available")
 	})
 
-	It("only publishes the LoadBalancerReady event when the load balancer becomes available or its IP changes", func() {
-		recorder := events.NewFakeRecorder(10)
-		reconciler.Recorder = recorder
-
-		_, err := reconciler.Reconcile(ctx, request)
-		Expect(err).NotTo(HaveOccurred())
-
-		Expect(drainEvents(recorder)).To(ContainElement(HavePrefix("Normal LoadBalancerReady")))
-
-		// A subsequent reconcile with target changes should not republish LoadBalancerReady.
-		createControlPlaneMachine(ctx, "cp-event-test-"+clusterName, clusterName, "10.0.0.99")
-		_, err = reconciler.Reconcile(ctx, request)
-		Expect(err).NotTo(HaveOccurred())
-
-		Expect(drainEvents(recorder)).NotTo(ContainElement(HavePrefix("Normal LoadBalancerReady")))
-
-		// If the load balancer IP changes, LoadBalancerReady should be published again.
-		got := &infrav1.StackitCluster{}
-		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
-		fakeCloud.SetLoadBalancerIP(got.Status.APIServerLoadBalancerID, "203.0.113.99")
-		_, err = reconciler.Reconcile(ctx, request)
-		Expect(err).NotTo(HaveOccurred())
-
-		Expect(drainEvents(recorder)).To(ContainElement(Equal("Normal LoadBalancerReady API server load balancer is ready at 203.0.113.99")))
-	})
-
 	It("keeps the bootstrap placeholder while no control plane machine has an address", func() {
 		// STACKIT rejects an empty target pool.
 		createControlPlaneMachine(ctx, "cp-0-"+clusterName, clusterName, "")