fix: deletion defects and resource orphans (#17)

Author: Herbaert

README.md

@@ -81,9 +81,13 @@ Check out the [Quick Start](docs/src/quick-start.md) for launching a cluster on
 This provider's versions are compatible with the following versions of Cluster API
 and support all Kubernetes versions that is supported by its compatible Cluster API version:
 
-|                          | Cluster API v1alpha4 (v0.4) | Cluster API v1beta1 (v1.x) |
-| ------------------------ | :-------------------------: | :------------------------: |
-| CAPSTK v1alpha1 `(main)` |              x              |             ✓              |
+|                          | Cluster API v1alpha4 (v0.4) | Cluster API v1beta1 | Cluster API v1beta2 |
+| ------------------------ | :-------------------------: | :-----------------: | :-----------------: |
+| CAPSTK v1alpha1 `(main)` |              x              |          x          |          ✓          |
+
+This provider implements the **v1beta2** contract, as declared in
+[`metadata.yaml`](metadata.yaml), and is built against `sigs.k8s.io/cluster-api`
+v1.13.2.
 
 (See [Kubernetes support matrix](https://cluster-api.sigs.k8s.io/reference/versions.html) of Cluster API versions).
 

cloud/fake/client.go

@@ -55,6 +55,12 @@ type Client struct {
 	FailNextEnsureNodeSSH error
 	FailNextDeleteNodeSSH error
 
+	// Before* hooks, if non-nil, run before the call they belong to. They let a
+	// test observe API server state at the exact moment a cloud call would
+	// happen. Unlike FailNext*, they are not consumed.
+	BeforeCreateServer func()
+	BeforeGetNetwork   func()
+
 	// CreateServerCalls counts successful CreateServer calls (for idempotency
 	// assertions).
 	CreateServerCalls  int
@@ -171,6 +177,9 @@ func (c *Client) CreateServer(_ context.Context, input cloud.CreateServerInput)
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
+	if c.BeforeCreateServer != nil {
+		c.BeforeCreateServer()
+	}
 	if err := consume(&c.FailNextCreateServer); err != nil {
 		return nil, err
 	}
@@ -215,6 +224,9 @@ func (c *Client) GetNetwork(_ context.Context, id string) (*cloud.Network, error
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
+	if c.BeforeGetNetwork != nil {
+		c.BeforeGetNetwork()
+	}
 	if err := consume(&c.FailNextGetNetwork); err != nil {
 		return nil, err
 	}

controller/constants.go

@@ -18,4 +18,11 @@ const (
 	cloudInitRefKindSecret = "Secret"
 
 	retryableErrorRequeueAfter = 5 * time.Second
+
+	// 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/controller_test_helpers_test.go

@@ -87,23 +87,20 @@ func createOwnerCluster(ctx context.Context, name string) {
 	Expect(k8sClient.Create(ctx, cluster)).To(Succeed())
 }
 
-func createOwnerMachine(ctx context.Context, name, namespace, clusterName, stackitMachineName string, bootstrapSecretName *string) {
-	if bootstrapSecretName == nil {
-		empty := ""
-		bootstrapSecretName = &empty
-	}
+func createOwnerMachine(ctx context.Context, name, clusterName, stackitMachineName string) {
 	machine := &clusterv1.Machine{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
-			Namespace: namespace,
+			Namespace: "default",
 			Labels: map[string]string{
 				clusterv1.ClusterNameLabel: clusterName,
 			},
 		},
 		Spec: clusterv1.MachineSpec{
 			ClusterName: clusterName,
 			Bootstrap: clusterv1.Bootstrap{
-				DataSecretName: bootstrapSecretName,
+				// Specs that need bootstrap data attach it with updateMachineBootstrapSecret.
+				DataSecretName: new(""),
 			},
 			InfrastructureRef: clusterv1.ContractVersionedObjectReference{
 				APIGroup: infrav1.GroupVersion.Group,

controller/stackitcluster_bastion.go

@@ -39,33 +39,30 @@ func (r *StackitClusterReconciler) reconcileBastion(
 	cloudClient cloud.Client,
 	clusterScope *scope.ClusterScope,
 ) (ctrl.Result, bool, error) {
-	cluster := clusterScope.StackitCluster
-	input := bastionservice.Input(cluster, nil)
+	stackitCluster := clusterScope.StackitCluster
+	input := bastionservice.Input(stackitCluster, nil)
 	status := cloud.Bastion{
-		ServerID:        cluster.Status.Bastion.ServerID,
-		PublicIPID:      cluster.Status.Bastion.PublicIPID,
-		PublicIP:        cluster.Status.Bastion.PublicIP,
-		SecurityGroupID: cluster.Status.Bastion.SecurityGroupID,
-	}
-
-	if !cluster.Spec.Bastion.Enabled {
-		// The condition carries this reason only after a cleanup has succeeded,
-		// so anything else means we may still own bastion resources — including
-		// the case where EnsureBastion succeeded but its status patch was lost.
-		// Keying on it instead of on the status keeps the tag-based cleanup to
-		// once per cluster rather than once per reconcile, which matters because
-		// this path runs for every cluster without a bastion.
-		condition := meta.FindStatusCondition(cluster.Status.Conditions, infrav1.ClusterBastionReadyCondition)
+		ServerID:        stackitCluster.Status.Bastion.ServerID,
+		PublicIPID:      stackitCluster.Status.Bastion.PublicIPID,
+		PublicIP:        stackitCluster.Status.Bastion.PublicIP,
+		SecurityGroupID: stackitCluster.Status.Bastion.SecurityGroupID,
+	}
+
+	if !stackitCluster.Spec.Bastion.Enabled {
+		// The reason is set only after a cleanup has succeeded, so anything else
+		// means bastion resources may still exist. Keying on it rather than on
+		// the status keeps the tag-based cleanup to once per cluster.
+		condition := meta.FindStatusCondition(stackitCluster.Status.Conditions, infrav1.ClusterBastionReadyCondition)
 		if condition == nil || condition.Reason != bastionDisabledReason {
-			if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil {
+			if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(stackitCluster)); err != nil {
 				return ctrl.Result{}, false, err
 			}
 			if err := cloudClient.DeleteBastion(ctx, input, status); err != nil {
 				return ctrl.Result{}, false, err
 			}
 			clusterScope.ClearBastionStatus()
 			if r.Recorder != nil {
-				r.Recorder.Eventf(cluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion")
+				r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion")
 			}
 		}
 		clusterScope.SetConditions(
@@ -77,7 +74,7 @@ func (r *StackitClusterReconciler) reconcileBastion(
 		return ctrl.Result{}, true, nil
 	}
 
-	if err := validateBastionSpec(cluster.Spec.Bastion); err != nil {
+	if err := validateBastionSpec(stackitCluster.Spec.Bastion); err != nil {
 		clusterScope.SetNotReady(
 			"InvalidBastionSpec",
 			err.Error(),
@@ -87,7 +84,7 @@ func (r *StackitClusterReconciler) reconcileBastion(
 		return ctrl.Result{}, false, nil
 	}
 
-	cloudInit, err := r.resolveBastionCloudInit(ctx, cluster)
+	cloudInit, err := r.resolveBastionCloudInit(ctx, stackitCluster)
 	if err != nil {
 		clusterScope.SetNotReady(
 			"CloudInitRefError",
@@ -99,33 +96,38 @@ func (r *StackitClusterReconciler) reconcileBastion(
 	}
 	input.CloudInit = cloudInit
 
-	if bastionNeedsRecreate(cluster, cloudInit) {
-		if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil && !cloud.IsNotFound(err) {
+	if bastionNeedsRecreate(stackitCluster, cloudInit) {
+		if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(stackitCluster)); err != nil && !cloud.IsNotFound(err) {
 			return ctrl.Result{}, false, err
 		}
 		if err := cloudClient.DeleteBastion(ctx, input, status); err != nil && !cloud.IsNotFound(err) {
 			return ctrl.Result{}, false, err
 		}
 		clusterScope.ClearBastionStatus()
-		clusterScope.SetNotReady("Recreating", "recreating bastion because cloudInitRef content changed", infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition)
+		clusterScope.SetNotReady(
+			"Recreating",
+			"recreating bastion because cloudInitRef content changed",
+			infrav1.ClusterBastionReadyCondition,
+			infrav1.ClusterReadyCondition,
+		)
 		if r.Recorder != nil {
 			r.Recorder.Eventf(
-				cluster, nil, corev1.EventTypeNormal, "BastionRecreating", "Recreate",
+				stackitCluster, nil, corev1.EventTypeNormal, "BastionRecreating", "Recreate",
 				"Recreating bastion because cloudInitRef content changed",
 			)
 		}
 		return ctrl.Result{RequeueAfter: retryableErrorRequeueAfter}, false, nil
 	}
 
-	hadBastionStatus := hasBastionStatus(cluster.Status.Bastion)
+	hadBastionStatus := hasBastionStatus(stackitCluster.Status.Bastion)
 	bastion, err := cloudClient.EnsureBastion(ctx, input)
 	if err != nil {
 		return ctrl.Result{}, false, err
 	}
 	clusterScope.SetBastionStatus(bastion, bastionCloudInitHash(cloudInit))
 	if !hadBastionStatus && r.Recorder != nil {
 		r.Recorder.Eventf(
-			cluster, nil, corev1.EventTypeNormal, "BastionCreated", "Create", "Created bastion %s", bastion.ServerID,
+			stackitCluster, nil, corev1.EventTypeNormal, "BastionCreated", "Create", "Created bastion %s", bastion.ServerID,
 		)
 	}
 	if bastion.ServerState != "" && bastion.ServerState != "ACTIVE" {
@@ -180,11 +182,11 @@ func hasBastionStatus(status infrav1.StackitBastionStatus) bool {
 	return status.ServerID != "" || status.PublicIPID != "" || status.PublicIP != "" || status.SecurityGroupID != ""
 }
 
-func bastionNeedsRecreate(sc *infrav1.StackitCluster, cloudInit []byte) bool {
-	if !hasBastionStatus(sc.Status.Bastion) {
+func bastionNeedsRecreate(stackitCluster *infrav1.StackitCluster, cloudInit []byte) bool {
+	if !hasBastionStatus(stackitCluster.Status.Bastion) {
 		return false
 	}
-	return sc.Status.Bastion.CloudInitHash != bastionCloudInitHash(cloudInit)
+	return stackitCluster.Status.Bastion.CloudInitHash != bastionCloudInitHash(cloudInit)
 }
 
 func bastionCloudInitHash(cloudInit []byte) string {
@@ -194,12 +196,12 @@ func bastionCloudInitHash(cloudInit []byte) string {
 	return fmt.Sprintf("%x", sha256.Sum256(cloudInit))
 }
 
-func (r *StackitClusterReconciler) resolveBastionCloudInit(ctx context.Context, sc *infrav1.StackitCluster) ([]byte, error) {
-	ref := sc.Spec.Bastion.CloudInitRef
+func (r *StackitClusterReconciler) resolveBastionCloudInit(ctx context.Context, stackitCluster *infrav1.StackitCluster) ([]byte, error) {
+	ref := stackitCluster.Spec.Bastion.CloudInitRef
 	if ref == nil {
 		return nil, nil
 	}
-	key := types.NamespacedName{Namespace: sc.Namespace, Name: ref.Name}
+	key := types.NamespacedName{Namespace: stackitCluster.Namespace, Name: ref.Name}
 	switch ref.Kind {
 	case "ConfigMap":
 		configMap := &corev1.ConfigMap{}

controller/stackitcluster_controller.go

@@ -54,6 +54,7 @@ type StackitClusterReconciler struct {
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=stackitclusters/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=stackitclusters/finalizers,verbs=update
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines,verbs=get;list;watch
 // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch
 // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch
 // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch
@@ -94,7 +95,7 @@ func (r *StackitClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	util.SetPausedCondition(&stackitCluster.Status.Conditions, stackitCluster.Generation, false, "")
 
 	if !stackitCluster.DeletionTimestamp.IsZero() {
-		return ctrl.Result{}, r.reconcileDelete(ctx, clusterScope)
+		return r.reconcileDelete(ctx, clusterScope)
 	}
 	return r.reconcileNormal(ctx, clusterScope)
 }