diff --git a/README.md b/README.md index 2b56182..47eec19 100644 --- a/README.md +++ b/README.md @@ -61,9 +61,13 @@ Check out the [Quick Start](./quick-start.md) for launching a cluster on STACKIT 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). diff --git a/cloud/fake/client.go b/cloud/fake/client.go index 0d08966..0642e49 100644 --- a/cloud/fake/client.go +++ b/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 } diff --git a/controller/constants.go b/controller/constants.go index 6a27a27..cc4e652 100644 --- a/controller/constants.go +++ b/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 ) diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index fd95fb3..c861246 100644 --- a/controller/controller_test_helpers_test.go +++ b/controller/controller_test_helpers_test.go @@ -87,15 +87,11 @@ 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, }, @@ -103,7 +99,8 @@ func createOwnerMachine(ctx context.Context, name, namespace, clusterName, stack 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, diff --git a/controller/stackitcluster_bastion.go b/controller/stackitcluster_bastion.go index 5e07cb4..3a3927d 100644 --- a/controller/stackitcluster_bastion.go +++ b/controller/stackitcluster_bastion.go @@ -39,25 +39,22 @@ 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 { @@ -65,7 +62,7 @@ func (r *StackitClusterReconciler) reconcileBastion( } 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,25 +96,30 @@ 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 @@ -125,7 +127,7 @@ func (r *StackitClusterReconciler) reconcileBastion( 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{} diff --git a/controller/stackitcluster_controller.go b/controller/stackitcluster_controller.go index 47ddd09..a7eaa81 100644 --- a/controller/stackitcluster_controller.go +++ b/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) } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index ef24ac0..4d391ad 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -203,11 +203,8 @@ var _ = Describe("StackitCluster Controller", func() { }) It("cleans up bastion resources during deletion even when bastion status was never persisted", func() { - // Regression test for debug/deletion-bug.md: the cloud-cleanup block used - // to be gated on persisted status for the bastion, while the load - // balancer was gated on its spec flag. A bastion created without its - // status patch landing (process restart, conflict) therefore skipped - // cleanup entirely and leaked server, public IP and security group. + // A bastion whose status patch never landed must still be cleaned up, so + // cleanup follows the spec flag rather than persisted status. createOwnerCluster(ctx, clusterName+"-nolb") defer deleteIfExists(ctx, &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-nolb", Namespace: namespace}, @@ -247,10 +244,8 @@ var _ = Describe("StackitCluster Controller", func() { }) It("finalizes deletion when the credentials Secret is already gone", func() { - // A missing credentials Secret cannot be recovered from, and it commonly - // disappears first during namespace teardown. Broadening the delete gate - // to spec.Bastion.Enabled made a working cloud client mandatory for every - // bastion cluster, which would strand such a cluster in Terminating. + // The Secret commonly disappears first during namespace teardown, and + // without it no cloud client can be built at all. createOwnerCluster(ctx, clusterName+"-nocreds") defer deleteIfExists(ctx, &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-nocreds", Namespace: namespace}, @@ -284,10 +279,8 @@ var _ = Describe("StackitCluster Controller", func() { }) It("tears the bastion down when disabled even if its status was never persisted", func() { - // Counterpart to the deletion path: disabling the bastion used to be - // gated on hasBastionStatus alone. With the status lost, nothing was torn - // down while the condition reported "bastion disabled" — leaving port 22 - // open for the rest of the cluster's life. + // With the status lost, a status-gated teardown would report the bastion + // as disabled while leaving port 22 open. got := &infrav1.StackitCluster{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) got.Spec.Bastion = validBastionSpec() @@ -316,8 +309,7 @@ var _ = Describe("StackitCluster Controller", func() { It("tears the bastion down when disabled even if only its status was lost", func() { // Narrower than the case above: the condition survives and still reports - // the bastion as available, only the bastion status fields are gone. - // Gating the cleanup on hasBastionStatus left the server running here. + // the bastion as available, only the status fields are gone. got := &infrav1.StackitCluster{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) got.Spec.Bastion = validBastionSpec() @@ -345,10 +337,8 @@ var _ = Describe("StackitCluster Controller", func() { }) It("cleans up the load balancer during deletion when it was disabled and its status was lost", func() { - // Counterpart to the bastion case: flipping apiServerLoadBalancer.enabled - // off neither deletes the load balancer nor clears its ID, so with the - // status patch lost the deletion gate matched nothing and the load - // balancer stayed behind. + // Flipping apiServerLoadBalancer.enabled off neither deletes the load + // balancer nor clears its ID, so deletion cannot rely on either. _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) @@ -499,19 +489,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() { @@ -580,6 +585,83 @@ var _ = Describe("StackitCluster Controller", func() { }).Should(BeTrue()) }) + // 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() { + var finalizersAtFirstCall []string + fakeCloud.BeforeGetNetwork = func() { + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + finalizersAtFirstCall = got.Finalizers + } + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) + + Expect(finalizersAtFirstCall).To(ContainElement(infrav1.ClusterFinalizer), + "cloud resources were created while the API server had no finalizer to clean them up") + }) + + // Machine controllers reach credentials and project context through this + // StackitCluster, so it has to outlive them. Cluster API orders this + // correctly when deletion starts at the Cluster, but a namespace teardown or + // a direct delete of this object bypasses that ordering. + It("keeps the finalizer while Machines still exist for the cluster", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + machineName := "machine-" + clusterName + createOwnerMachine(ctx, machineName, clusterName, "stackit-"+machineName) + DeferCleanup(func() { + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) + }) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + By("requeueing instead of tearing down shared infrastructure") + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(deleteRequeueAfter)) + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Finalizers).To(ContainElement(infrav1.ClusterFinalizer), + "the finalizer must survive while a Machine still needs the StackitCluster") + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1), + "the load balancer must not be deleted while Machines still exist") + }) + + It("removes the finalizer once the last Machine is gone", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + machineName := "machine-" + clusterName + createOwnerMachine(ctx, machineName, clusterName, "stackit-"+machineName) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(deleteRequeueAfter)) + + By("deleting the last Machine") + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: machineName, Namespace: namespace}}) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Expect(fakeCloud.LoadBalancerCount()).To(Equal(0)) + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitCluster{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) + It("keeps the finalizer when load balancer deletion returns a transient error", func() { _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 1380c0c..5e0c56d 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -12,6 +12,7 @@ package controller import ( "context" + "fmt" "net/netip" "time" @@ -20,6 +21,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -33,20 +35,27 @@ import ( func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { log := logf.FromContext(ctx) - cluster := clusterScope.StackitCluster + stackitCluster := clusterScope.StackitCluster - if !controllerutil.ContainsFinalizer(cluster, infrav1.ClusterFinalizer) { - controllerutil.AddFinalizer(cluster, infrav1.ClusterFinalizer) + if !controllerutil.ContainsFinalizer(stackitCluster, infrav1.ClusterFinalizer) { + controllerutil.AddFinalizer(stackitCluster, infrav1.ClusterFinalizer) + // Persisted immediately, before any cloud resource can be created, to + // ensure nothing is running behind an object that carries no finalizer to + // clean it up. + if err := clusterScope.PatchObject(ctx); err != nil { + return ctrl.Result{}, fmt.Errorf("persist finalizer: %w", err) + } } - cluster.Status.FailureDomains = stackitFailureDomains(cluster.Spec.Region) + stackitCluster.Status.FailureDomains = stackitFailureDomains(stackitCluster.Spec.Region) - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, cluster) + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, stackitCluster) if err != nil { - cluster.Status.Ready = false + stackitCluster.Status.Ready = false return util.CredentialFailureResult( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, err, + credentialsRetryRequeueAfter, infrav1.ClusterCredentialsReadyCondition, infrav1.ClusterReadyCondition, ) @@ -58,12 +67,12 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS infrav1.ClusterCredentialsReadyCondition, ) - network, err := cloudClient.GetNetwork(ctx, cluster.Spec.Network.ID) + network, err := cloudClient.GetNetwork(ctx, stackitCluster.Spec.Network.ID) if err != nil { - cluster.Status.Ready = false + stackitCluster.Status.Ready = false return util.CloudFailureResult( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, "NetworkNotFound", err, retryableErrorRequeueAfter, @@ -79,19 +88,19 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS infrav1.ClusterNetworkReadyCondition, ) - if cluster.Spec.APIServerLoadBalancer.Enabled { - lb, err := cloudClient.EnsureAPIServerLoadBalancer( + if stackitCluster.Spec.APIServerLoadBalancer.Enabled { + loadBalancer, err := cloudClient.EnsureAPIServerLoadBalancer( ctx, loadbalancerservice.APIServerInput( - cluster, + stackitCluster, []cloud.LoadBalancerTargetInput{loadbalancerservice.BootstrapTarget(bootstrapTargetIP(network))}, ), ) if err != nil { - cluster.Status.Ready = false + stackitCluster.Status.Ready = false return util.CloudFailureResult( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, "LoadBalancerError", err, retryableErrorRequeueAfter, @@ -100,17 +109,17 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS infrav1.ClusterReadyCondition, ) } - hadLoadBalancerID := cluster.Status.APIServerLoadBalancerID != "" - if lb != nil { - cluster.Status.APIServerLoadBalancerID = lb.ID - if !hadLoadBalancerID && lb.ID != "" && r.Recorder != nil { + hadLoadBalancerID := stackitCluster.Status.APIServerLoadBalancerID != "" + if loadBalancer != nil { + stackitCluster.Status.APIServerLoadBalancerID = loadBalancer.ID + if !hadLoadBalancerID && loadBalancer.ID != "" && r.Recorder != nil { r.Recorder.Eventf( - cluster, nil, corev1.EventTypeNormal, "LoadBalancerCreated", "Create", - "Created API server load balancer %s", lb.ID, + stackitCluster, nil, corev1.EventTypeNormal, "LoadBalancerCreated", "Create", + "Created API server load balancer %s", loadBalancer.ID, ) } } - if lb == nil || lb.IP == "" { + if loadBalancer == nil || loadBalancer.IP == "" { clusterScope.SetNotReady( "Provisioning", "waiting for API server load balancer IP address", @@ -120,7 +129,7 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS return ctrl.Result{RequeueAfter: 10 * time.Second}, nil } endpoint := clusterv1.APIEndpoint{ - Host: lb.IP, + Host: loadBalancer.IP, Port: defaultAPIServerPort, } clusterScope.SetAPIServerEndpoint(endpoint) @@ -132,12 +141,12 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS ) if r.Recorder != nil { r.Recorder.Eventf( - cluster, nil, corev1.EventTypeNormal, "LoadBalancerReady", "SetReady", - "API server load balancer is ready at %s", lb.IP, + stackitCluster, nil, corev1.EventTypeNormal, "LoadBalancerReady", "SetReady", + "API server load balancer is ready at %s", loadBalancer.IP, ) } - } else if cluster.Spec.ControlPlaneEndpoint.Host != "" { - cluster.Status.APIServerEndpoint = cluster.Spec.ControlPlaneEndpoint + } else if stackitCluster.Spec.ControlPlaneEndpoint.Host != "" { + stackitCluster.Status.APIServerEndpoint = stackitCluster.Spec.ControlPlaneEndpoint clusterScope.SetConditions( metav1.ConditionTrue, "Skipped", @@ -155,10 +164,10 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS } if result, ready, err := r.reconcileBastion(ctx, cloudClient, clusterScope); err != nil { - cluster.Status.Ready = false + stackitCluster.Status.Ready = false return util.CloudFailureResult( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, "BastionError", err, retryableErrorRequeueAfter, @@ -171,7 +180,7 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS } clusterScope.SetReady() - log.V(1).Info("StackitCluster ready", "endpoint", cluster.Status.APIServerEndpoint) + log.V(1).Info("StackitCluster ready", "endpoint", stackitCluster.Status.APIServerEndpoint) return ctrl.Result{}, nil } @@ -222,75 +231,89 @@ func bootstrapTargetIP(network *cloud.Network) string { return "10.0.0.1" } -func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) error { - cluster := clusterScope.StackitCluster +func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { + stackitCluster := clusterScope.StackitCluster + + // Machines resolve their credentials and project context through this + // StackitCluster, so the finalizer has to stay while any of them remain. + // Selects the same way util/collections.GetFilteredMachinesForCluster does, + // inlined because that package pulls in the kubeadm bootstrap API. + machines := &clusterv1.MachineList{} + if err := r.List(ctx, machines, + client.InNamespace(clusterScope.Cluster.Namespace), + client.MatchingLabels{clusterv1.ClusterNameLabel: clusterScope.Cluster.Name}, + ); err != nil { + return ctrl.Result{}, fmt.Errorf("list Machines for cluster %s: %w", clusterScope.Cluster.Name, err) + } + if len(machines.Items) > 0 { + logf.FromContext(ctx).Info( + "Waiting for Machines to be deleted before removing the StackitCluster finalizer", + "remainingMachines", len(machines.Items), + ) + return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil + } - // Cleanup runs unconditionally. Neither spec nor status is a trustworthy - // record of what exists in the cloud: a resource can be created before its - // status patch lands, and disabling the load balancer or the bastion leaves - // the running resource behind. ResolveID, DeleteBastion and - // DeleteNodeSSHAccess all fall back to tag lookups and tolerate NotFound, so - // asking for everything costs a handful of list calls once per cluster and - // removes every combination in which a resource could be missed. - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, cluster) + // Cleanup runs unconditionally: neither spec nor status is a trustworthy + // record of what exists in the cloud. ResolveID, DeleteBastion and + // DeleteNodeSSHAccess fall back to tag lookups and tolerate NotFound. + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, stackitCluster) if err != nil { - // A missing credentials Secret can never be recovered from — it commonly - // disappears first during namespace teardown. Blocking here would strand - // the cluster in Terminating forever, so finalize and make the possible - // leak loud instead. Any other credentials problem is fixable, so keep - // retrying for those. + // A missing credentials Secret cannot be recovered from and commonly + // disappears first during namespace teardown, so finalize and make the + // possible leak loud rather than stranding the cluster in Terminating. + // Every other credentials problem is fixable and keeps retrying. if apierrors.IsNotFound(err) { if r.Recorder != nil { - r.Recorder.Eventf(cluster, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", + r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", "Credentials Secret is gone; finalizing without cloud cleanup. "+ "Any remaining STACKIT resources for this cluster must be removed manually: %v", err) } - controllerutil.RemoveFinalizer(cluster, infrav1.ClusterFinalizer) - return nil + controllerutil.RemoveFinalizer(stackitCluster, infrav1.ClusterFinalizer) + return ctrl.Result{}, nil } util.SetConditions( - &cluster.Status.Conditions, - cluster.Generation, + &stackitCluster.Status.Conditions, + stackitCluster.Generation, metav1.ConditionFalse, "CredentialsInvalid", err.Error(), infrav1.ClusterCredentialsReadyCondition, ) - return err + return ctrl.Result{}, err } - loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, cluster) + loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, stackitCluster) if err != nil { - return err + return ctrl.Result{}, err } if loadBalancerID != "" { if err := cloudClient.DeleteAPIServerLoadBalancer(ctx, loadBalancerID); err != nil && !cloud.IsNotFound(err) { - return err + return ctrl.Result{}, err } - cluster.Status.APIServerLoadBalancerID = "" + stackitCluster.Status.APIServerLoadBalancerID = "" if r.Recorder != nil { r.Recorder.Eventf( - cluster, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", + stackitCluster, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", "Deleted API server load balancer %s", loadBalancerID, ) } } - if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil && !cloud.IsNotFound(err) { - return err + if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(stackitCluster)); err != nil && !cloud.IsNotFound(err) { + return ctrl.Result{}, err } - if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(cluster, nil), cloud.Bastion{ - ServerID: cluster.Status.Bastion.ServerID, - PublicIPID: cluster.Status.Bastion.PublicIPID, - PublicIP: cluster.Status.Bastion.PublicIP, - SecurityGroupID: cluster.Status.Bastion.SecurityGroupID, + if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(stackitCluster, nil), cloud.Bastion{ + ServerID: stackitCluster.Status.Bastion.ServerID, + PublicIPID: stackitCluster.Status.Bastion.PublicIPID, + PublicIP: stackitCluster.Status.Bastion.PublicIP, + SecurityGroupID: stackitCluster.Status.Bastion.SecurityGroupID, }); err != nil && !cloud.IsNotFound(err) { - return err + return ctrl.Result{}, err } - if hasBastionStatus(cluster.Status.Bastion) { + if hasBastionStatus(stackitCluster.Status.Bastion) { 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") } } - controllerutil.RemoveFinalizer(cluster, infrav1.ClusterFinalizer) - return nil + controllerutil.RemoveFinalizer(stackitCluster, infrav1.ClusterFinalizer) + return ctrl.Result{}, nil } diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index ed72106..0dfb3e2 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -69,7 +69,7 @@ var _ = Describe("StackitMachine Controller", func() { createCredentialsSecret(ctx, credentials, namespace, testProjectID) createOwnerCluster(ctx, clusterName) createReadyStackitCluster(ctx, clusterName, namespace, credentials) - createOwnerMachine(ctx, machineName, namespace, clusterName, stackitName, nil) + createOwnerMachine(ctx, machineName, clusterName, stackitName) stackitMach = newStackitMachine(stackitName, namespace, machineName) Expect(k8sClient.Create(ctx, stackitMach)).To(Succeed()) }) @@ -132,12 +132,9 @@ var _ = Describe("StackitMachine Controller", func() { }) It("does not silently recreate the server of an already-provisioned machine", func() { - // Regression test for debug/machine-recreate-bug.md: when the backing - // server disappears out-of-band, ensureServer used to call CreateServer - // again, replaying the original bootstrap data. The replacement either - // never rejoins (different IP) or rejoins while Machine and Node keep - // pointing at the deleted server (same IP) — neither restores the - // cluster, and both consume another VM unnoticed. + // Recreating it would replay bootstrap data pinned to the previous + // identity: the replacement either never rejoins or rejoins while Machine + // and Node still point at the deleted server. updateMachineBootstrapSecret(ctx, machineName, bootstrapName) createBootstrapSecret(ctx, bootstrapName) @@ -220,7 +217,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 +226,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() { @@ -386,6 +395,62 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) + // The hook observes API server state from inside the cloud call, so it proves + // the ordering rather than only the end result. + It("persists the finalizer before creating the server", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) + + var finalizersAtCreate []string + fakeCloud.BeforeCreateServer = func() { + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + finalizersAtCreate = got.Finalizers + } + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + Expect(finalizersAtCreate).To(ContainElement(infrav1.MachineFinalizer), + "the server was created while the API server had no finalizer to clean it up") + }) + + // A lost status patch leaves a running, tagged server that no field on the + // object names, so an empty status.instanceID is not proof that none exists. + It("deletes a tagged server whose instance ID was lost from the status", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + By("losing the persisted instance ID, as if the status patch never landed") + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Status.InstanceID = "" + got.Status.ProviderID = "" + Expect(k8sClient.Status().Update(ctx, got)).To(Succeed()) + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Spec.ProviderID = nil + Expect(k8sClient.Update(ctx, got)).To(Succeed()) + + By("deleting the StackitMachine") + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Expect(fakeCloud.ServerCount()).To(Equal(0), + "the tagged server leaked because deletion trusted the empty status") + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) + It("maps owning Machine events to StackitMachine reconcile requests", func() { machine := &clusterv1.Machine{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: machineName, Namespace: namespace}, machine)).To(Succeed()) @@ -402,6 +467,48 @@ var _ = Describe("StackitMachine Controller", func() { Expect(requests).To(ConsistOf(request)) }) + // Machine.spec.clusterName names the Cluster, not the StackitCluster, and a + // ClusterClass-generated infrastructureRef never shares its Cluster's name. + // Every other spec here uses matching names and would miss that. + It("maps StackitCluster events when the StackitCluster name differs from the Cluster name", func() { + suffix := time.Now().UnixNano() + ownerClusterName := fmt.Sprintf("owner-%d", suffix) + infraClusterName := ownerClusterName + "-infra" + otherMachineName := fmt.Sprintf("other-machine-%d", suffix) + otherStackitName := fmt.Sprintf("other-stackit-machine-%d", suffix) + + By("creating a Cluster whose infrastructureRef points at a differently named StackitCluster") + ownerCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: ownerClusterName, Namespace: namespace}, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: clusterv1.ContractVersionedObjectReference{ + APIGroup: infrav1.GroupVersion.Group, + Kind: "StackitCluster", + Name: infraClusterName, + }, + }, + } + Expect(k8sClient.Create(ctx, ownerCluster)).To(Succeed()) + + infraCluster := newStackitCluster(infraClusterName, namespace, false) + infraCluster.OwnerReferences[0].Name = ownerClusterName + Expect(k8sClient.Create(ctx, infraCluster)).To(Succeed()) + + createOwnerMachine(ctx, otherMachineName, ownerClusterName, otherStackitName) + + DeferCleanup(func() { + deleteIfExists(ctx, &clusterv1.Machine{ObjectMeta: metav1.ObjectMeta{Name: otherMachineName, Namespace: namespace}}) + deleteIfExists(ctx, infraCluster) + deleteIfExists(ctx, ownerCluster) + }) + + By("mapping the StackitCluster event onto the Machine owned by its Cluster") + requests := reconciler.stackitMachineRequestsForStackitCluster(ctx, infraCluster) + Expect(requests).To(ConsistOf(reconcile.Request{ + NamespacedName: types.NamespacedName{Namespace: namespace, Name: otherStackitName}, + }), "the StackitCluster watch must resolve its owning Cluster instead of matching on its own name") + }) + It("maps bootstrap Secret events to StackitMachine reconcile requests", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: bootstrapName, Namespace: namespace}} diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index 863b4e2..c549df3 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -32,51 +32,67 @@ import ( "github.com/stackitcloud/cluster-api-provider-stackit/util" ) -func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope.MachineScope) (ctrl.Result, error) { +func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineScope *scope.MachineScope) (ctrl.Result, error) { log := logf.FromContext(ctx) - sm := s.StackitMachine + stackitMachine := machineScope.StackitMachine - if !controllerutil.ContainsFinalizer(sm, infrav1.MachineFinalizer) { - controllerutil.AddFinalizer(sm, infrav1.MachineFinalizer) + if !controllerutil.ContainsFinalizer(stackitMachine, infrav1.MachineFinalizer) { + controllerutil.AddFinalizer(stackitMachine, infrav1.MachineFinalizer) + // Persisted immediately, before CreateServer can run, to ensure no servers + // are running behind an object that carries no finalizer to clean it up. + if err := machineScope.PatchObject(ctx); err != nil { + return ctrl.Result{}, fmt.Errorf("persist finalizer: %w", err) + } } - if !s.StackitCluster.Status.Ready { - s.SetNotReady("InfrastructureNotReady", "waiting for StackitCluster to be ready", infrav1.MachineReadyCondition) + if !machineScope.StackitCluster.Status.Ready { + machineScope.SetNotReady("InfrastructureNotReady", "waiting for StackitCluster to be ready", infrav1.MachineReadyCondition) return ctrl.Result{}, nil } - if err := validateMachineAvailabilityZone(s); err != nil { - s.SetNotReady("InvalidFailureDomain", err.Error(), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) + if err := validateMachineAvailabilityZone(machineScope); err != nil { + machineScope.SetNotReady( + "InvalidFailureDomain", + err.Error(), + infrav1.MachineInstanceReadyCondition, + infrav1.MachineReadyCondition, + ) return ctrl.Result{}, nil } - bootstrapData, condStatus, reason, msg := r.fetchBootstrapData(ctx, s.Machine) - s.SetConditions(condStatus, reason, msg, infrav1.MachineBootstrapReadyCondition) - if condStatus != metav1.ConditionTrue { - s.SetConditions(metav1.ConditionFalse, reason, msg, infrav1.MachineReadyCondition) + bootstrapData, conditionStatus, reason, message := r.fetchBootstrapData(ctx, machineScope.Machine) + machineScope.SetConditions(conditionStatus, reason, message, infrav1.MachineBootstrapReadyCondition) + if conditionStatus != metav1.ConditionTrue { + machineScope.SetConditions(metav1.ConditionFalse, reason, message, infrav1.MachineReadyCondition) if reason == util.BootstrapReasonInvalid { return ctrl.Result{}, nil } return ctrl.Result{RequeueAfter: retryableErrorRequeueAfter}, nil } - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, s.StackitCluster) + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, machineScope.StackitCluster) if err != nil { return util.CredentialFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, err, + credentialsRetryRequeueAfter, infrav1.MachineCredentialsReadyCondition, infrav1.MachineReadyCondition, ) } - s.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.MachineCredentialsReadyCondition) + machineScope.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.MachineCredentialsReadyCondition) - server, created, err := r.ensureServer(ctx, cloudClient, s, bootstrapData) + server, created, err := r.ensureServer(ctx, cloudClient, machineScope, bootstrapData) if err != nil { - s.SetNotReady("InstanceError", err.Error(), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) + machineScope.SetNotReady( + "InstanceError", + err.Error(), + infrav1.MachineInstanceReadyCondition, + infrav1.MachineReadyCondition, + ) return util.CloudFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, "InstanceError", err, retryableErrorRequeueAfter, @@ -87,24 +103,29 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope } if created && r.Recorder != nil { r.Recorder.Eventf( - sm, nil, corev1.EventTypeNormal, "InstanceCreated", "Create", "Created instance %s", server.ID, + stackitMachine, nil, corev1.EventTypeNormal, "InstanceCreated", "Create", "Created instance %s", server.ID, ) } - sm.Status.InstanceState = server.State - sm.Status.Addresses = machineAddressesFromCloud(server.Addresses) - providerID := s.SetInstance(server) + stackitMachine.Status.InstanceState = server.State + stackitMachine.Status.Addresses = machineAddressesFromCloud(server.Addresses) + providerID := machineScope.SetInstance(server) if server.State != "" && server.State != "ACTIVE" { - s.SetNotReady("Provisioning", fmt.Sprintf("server state is %s", server.State), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) + machineScope.SetNotReady( + "Provisioning", + fmt.Sprintf("server state is %s", server.State), + infrav1.MachineInstanceReadyCondition, + infrav1.MachineReadyCondition, + ) return ctrl.Result{RequeueAfter: 15 * time.Second}, nil } - if err := r.reconcileBastionNodeSSHAccess(ctx, cloudClient, s, server); err != nil { - s.SetNotReady("BastionSSHAccessError", err.Error(), infrav1.MachineReadyCondition) + if err := r.reconcileBastionNodeSSHAccess(ctx, cloudClient, machineScope, server); err != nil { + machineScope.SetNotReady("BastionSSHAccessError", err.Error(), infrav1.MachineReadyCondition) return util.CloudFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, "BastionSSHAccessError", err, retryableErrorRequeueAfter, @@ -113,11 +134,11 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope ) } - if err := r.reconcileAPIServerLoadBalancerTarget(ctx, cloudClient, s, server); err != nil { - s.SetNotReady("LoadBalancerTargetError", err.Error(), infrav1.MachineReadyCondition) + if err := r.reconcileAPIServerLoadBalancerTarget(ctx, cloudClient, machineScope, server); err != nil { + machineScope.SetNotReady("LoadBalancerTargetError", err.Error(), infrav1.MachineReadyCondition) return util.CloudFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, "LoadBalancerTargetError", err, retryableErrorRequeueAfter, @@ -126,17 +147,17 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope ) } - s.SetReady() + machineScope.SetReady() log.V(1).Info("StackitMachine ready", "providerID", providerID) return ctrl.Result{}, nil } -func validateMachineAvailabilityZone(s *scope.MachineScope) error { - availabilityZone := s.StackitMachine.Spec.AvailabilityZone - if availabilityZone == "" || len(s.StackitCluster.Status.FailureDomains) == 0 { +func validateMachineAvailabilityZone(machineScope *scope.MachineScope) error { + availabilityZone := machineScope.StackitMachine.Spec.AvailabilityZone + if availabilityZone == "" || len(machineScope.StackitCluster.Status.FailureDomains) == 0 { return nil } - for _, failureDomain := range s.StackitCluster.Status.FailureDomains { + for _, failureDomain := range machineScope.StackitCluster.Status.FailureDomains { if failureDomain.Name == availabilityZone { return nil } @@ -144,50 +165,78 @@ func validateMachineAvailabilityZone(s *scope.MachineScope) error { return fmt.Errorf("availabilityZone %q is not published in StackitCluster status.failureDomains", availabilityZone) } -func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, s *scope.MachineScope) error { - sm := s.StackitMachine - needsLoadBalancerCleanup := isControlPlaneMachine(s.Machine) && - s.StackitCluster.Spec.APIServerLoadBalancer.Enabled && - s.StackitCluster.Status.APIServerLoadBalancerID != "" - if sm.Status.InstanceID == "" && !needsLoadBalancerCleanup { - controllerutil.RemoveFinalizer(sm, infrav1.MachineFinalizer) - if r.Recorder != nil { - r.Recorder.Eventf(sm, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance") - } - return nil - } - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, s.StackitCluster) +func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineScope *scope.MachineScope) error { + stackitMachine := machineScope.StackitMachine + + // An empty status.instanceID is not proof that no server exists, so every + // deletion asks the cloud before dropping the finalizer. + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, machineScope.StackitCluster) if err != nil { + // A missing credentials Secret cannot be recovered from and commonly + // disappears first during namespace teardown, so finalize and make the + // possible leak loud rather than stranding the Machine in Terminating. + // Every other credentials problem is fixable and keeps retrying. + if apierrors.IsNotFound(err) { + if r.Recorder != nil { + r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", + "Credentials Secret is gone; finalizing without cloud cleanup. "+ + "Any remaining STACKIT server for this machine must be removed manually: %v", err) + } + controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) + return nil + } _, resultErr := util.CredentialFailureResult( - &sm.Status.Conditions, - sm.Generation, + &stackitMachine.Status.Conditions, + stackitMachine.Generation, err, + credentialsRetryRequeueAfter, infrav1.MachineCredentialsReadyCondition, ) return resultErr } - if err := r.deleteAPIServerLoadBalancerTarget(ctx, cloudClient, s); err != nil { + if err := r.deleteAPIServerLoadBalancerTarget(ctx, cloudClient, machineScope); err != nil { return err } - if sm.Status.InstanceID == "" { - controllerutil.RemoveFinalizer(sm, infrav1.MachineFinalizer) + + instanceID, err := r.resolveServerForDeletion(ctx, cloudClient, machineScope) + if err != nil { + return err + } + if instanceID != "" { + if err := cloudClient.DeleteServer(ctx, instanceID); err != nil && !cloud.IsNotFound(err) { + return err + } + machineScope.ClearInstance() if r.Recorder != nil { - r.Recorder.Eventf(sm, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance") + r.Recorder.Eventf( + stackitMachine, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance %s", instanceID, + ) } - return nil } - instanceID := sm.Status.InstanceID - if err := cloudClient.DeleteServer(ctx, instanceID); err != nil && !cloud.IsNotFound(err) { - return err + controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) + return nil +} + +// resolveServerForDeletion reports the ID of the server backing this machine, or +// an empty string once the cloud confirms none exists. It falls back to the tags, +// which carry the Machine UID, because a lost status patch can leave a running +// server that status.instanceID no longer names. +func (r *StackitMachineReconciler) resolveServerForDeletion( + ctx context.Context, + cloudClient cloud.Client, + machineScope *scope.MachineScope, +) (string, error) { + if instanceID := machineScope.StackitMachine.Status.InstanceID; instanceID != "" { + return instanceID, nil } - s.ClearInstance() - controllerutil.RemoveFinalizer(sm, infrav1.MachineFinalizer) - if r.Recorder != nil { - r.Recorder.Eventf( - sm, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance %s", instanceID, - ) + server, err := cloudClient.FindServerByTags(ctx, machineScope.Tags()) + if err != nil { + if cloud.IsNotFound(err) { + return "", nil + } + return "", err } - return nil + return server.ID, nil } func (r *StackitMachineReconciler) fetchBootstrapData(ctx context.Context, machine *clusterv1.Machine) ([]byte, metav1.ConditionStatus, string, string) { @@ -211,14 +260,14 @@ func (r *StackitMachineReconciler) fetchBootstrapData(ctx context.Context, machi func (r *StackitMachineReconciler) ensureServer( ctx context.Context, - c cloud.Client, - s *scope.MachineScope, + cloudClient cloud.Client, + machineScope *scope.MachineScope, userData []byte, ) (*cloud.Server, bool, error) { - sm := s.StackitMachine - tags := s.Tags() - if sm.Status.InstanceID != "" { - server, err := c.GetServer(ctx, sm.Status.InstanceID) + stackitMachine := machineScope.StackitMachine + tags := machineScope.Tags() + if stackitMachine.Status.InstanceID != "" { + server, err := cloudClient.GetServer(ctx, stackitMachine.Status.InstanceID) if err == nil { return server, false, nil } @@ -226,45 +275,40 @@ func (r *StackitMachineReconciler) ensureServer( return nil, false, err } } - if server, err := c.FindServerByTags(ctx, tags); err == nil { + if server, err := cloudClient.FindServerByTags(ctx, tags); err == nil { return server, false, nil } else if !cloud.IsNotFound(err) { return nil, false, err } - // The machine had already been provisioned and its server has since - // disappeared. Recreating it here would replay the original bootstrap data, - // which is pinned to the previous identity: the replacement either never - // rejoins (different IP) or rejoins while Machine/Node keep pointing at the - // deleted server (same IP). Neither restores the cluster, and both consume - // another VM silently. Surface it instead and let Cluster API decide to - // replace the Machine. - if sm.Status.Initialization.Provisioned { + // Recreating the server would replay bootstrap data pinned to the previous + // identity, so surface the loss and let Cluster API replace the Machine. + if stackitMachine.Status.Initialization.Provisioned { return nil, false, fmt.Errorf( "%w: server %s for already-provisioned machine no longer exists; the Machine must be replaced", - cloud.ErrNotFound, sm.Status.InstanceID, + cloud.ErrNotFound, stackitMachine.Status.InstanceID, ) } deleteOnTermination := true - if sm.Spec.RootVolume.DeleteOnTermination != nil { - deleteOnTermination = *sm.Spec.RootVolume.DeleteOnTermination - } - server, err := c.CreateServer(ctx, cloud.CreateServerInput{ - Name: sm.Name, - ProjectID: s.StackitCluster.Spec.ProjectID, - Region: s.StackitCluster.Spec.Region, - ImageID: sm.Spec.ImageID, - MachineType: sm.Spec.MachineType, - AvailabilityZone: sm.Spec.AvailabilityZone, - SSHKeyName: sm.Spec.SSHKeyName, - NetworkID: sm.Spec.Network.ID, - SecurityGroups: sm.Spec.SecurityGroups, + if stackitMachine.Spec.RootVolume.DeleteOnTermination != nil { + deleteOnTermination = *stackitMachine.Spec.RootVolume.DeleteOnTermination + } + server, err := cloudClient.CreateServer(ctx, cloud.CreateServerInput{ + Name: stackitMachine.Name, + ProjectID: machineScope.StackitCluster.Spec.ProjectID, + Region: machineScope.StackitCluster.Spec.Region, + ImageID: stackitMachine.Spec.ImageID, + MachineType: stackitMachine.Spec.MachineType, + AvailabilityZone: stackitMachine.Spec.AvailabilityZone, + SSHKeyName: stackitMachine.Spec.SSHKeyName, + NetworkID: stackitMachine.Spec.Network.ID, + SecurityGroups: stackitMachine.Spec.SecurityGroups, UserData: userData, Tags: tags, RootVolume: cloud.RootVolumeInput{ - SizeGiB: sm.Spec.RootVolume.SizeGiB, - PerformanceClass: sm.Spec.RootVolume.PerformanceClass, + SizeGiB: stackitMachine.Spec.RootVolume.SizeGiB, + PerformanceClass: stackitMachine.Spec.RootVolume.PerformanceClass, DeleteOnTermination: deleteOnTermination, }, }) @@ -273,70 +317,74 @@ func (r *StackitMachineReconciler) ensureServer( func (r *StackitMachineReconciler) reconcileBastionNodeSSHAccess( ctx context.Context, - c cloud.Client, - s *scope.MachineScope, + cloudClient cloud.Client, + machineScope *scope.MachineScope, server *cloud.Server, ) error { - if !s.StackitCluster.Spec.Bastion.Enabled { + if !machineScope.StackitCluster.Spec.Bastion.Enabled { return nil } - if s.StackitCluster.Status.Bastion.SecurityGroupID == "" { + if machineScope.StackitCluster.Status.Bastion.SecurityGroupID == "" { return fmt.Errorf("%w: bastion security group ID is empty", cloud.ErrTransient) } if server == nil || server.ID == "" { return fmt.Errorf("%w: server ID is empty", cloud.ErrTransient) } - _, err := c.EnsureNodeSSHAccess(ctx, cloud.NodeSSHAccessInput{ - Name: s.StackitCluster.Name + "-node-ssh", + _, err := cloudClient.EnsureNodeSSHAccess(ctx, cloud.NodeSSHAccessInput{ + Name: machineScope.StackitCluster.Name + "-node-ssh", ServerID: server.ID, - BastionSecurityGroupID: s.StackitCluster.Status.Bastion.SecurityGroupID, - Tags: bastionservice.NodeSSHAccessTags(s.StackitCluster), + BastionSecurityGroupID: machineScope.StackitCluster.Status.Bastion.SecurityGroupID, + Tags: bastionservice.NodeSSHAccessTags(machineScope.StackitCluster), }) return err } func (r *StackitMachineReconciler) reconcileAPIServerLoadBalancerTarget( ctx context.Context, - c cloud.Client, - s *scope.MachineScope, + cloudClient cloud.Client, + machineScope *scope.MachineScope, server *cloud.Server, ) error { - if !isControlPlaneMachine(s.Machine) || !s.StackitCluster.Spec.APIServerLoadBalancer.Enabled { + if !isControlPlaneMachine(machineScope.Machine) || !machineScope.StackitCluster.Spec.APIServerLoadBalancer.Enabled { return nil } loadBalancerID, err := loadbalancerservice.EnsureForMachine( ctx, - c, - s.StackitCluster, - s.Machine.Name, + cloudClient, + machineScope.StackitCluster, + machineScope.Machine.Name, server.Addresses, ) if err != nil { return err } - target, err := loadbalancerservice.TargetForMachine(s.Machine.Name, server.Addresses) + target, err := loadbalancerservice.TargetForMachine(machineScope.Machine.Name, server.Addresses) if err != nil { return err } target.LoadBalancerID = loadBalancerID - return c.EnsureAPIServerLoadBalancerTarget(ctx, target) + return cloudClient.EnsureAPIServerLoadBalancerTarget(ctx, target) } -func (r *StackitMachineReconciler) deleteAPIServerLoadBalancerTarget(ctx context.Context, c cloud.Client, s *scope.MachineScope) error { - if !isControlPlaneMachine(s.Machine) || !s.StackitCluster.Spec.APIServerLoadBalancer.Enabled { +func (r *StackitMachineReconciler) deleteAPIServerLoadBalancerTarget( + ctx context.Context, + cloudClient cloud.Client, + machineScope *scope.MachineScope, +) error { + if !isControlPlaneMachine(machineScope.Machine) || !machineScope.StackitCluster.Spec.APIServerLoadBalancer.Enabled { return nil } - loadBalancerID, err := loadbalancerservice.ResolveID(ctx, c, s.StackitCluster) + loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, machineScope.StackitCluster) if err != nil { return err } if loadBalancerID == "" { return nil } - err = c.DeleteAPIServerLoadBalancerTarget(ctx, cloud.LoadBalancerTargetInput{ + err = cloudClient.DeleteAPIServerLoadBalancerTarget(ctx, cloud.LoadBalancerTargetInput{ LoadBalancerID: loadBalancerID, - Name: s.Machine.Name, + Name: machineScope.Machine.Name, Port: defaultAPIServerPort, }) if cloud.IsNotFound(err) { diff --git a/controller/stackitmachine_watches.go b/controller/stackitmachine_watches.go index 266adfc..8853c7a 100644 --- a/controller/stackitmachine_watches.go +++ b/controller/stackitmachine_watches.go @@ -14,8 +14,10 @@ import ( "context" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + clusterutil "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -37,14 +39,28 @@ func (r *StackitMachineReconciler) stackitMachineRequestsForStackitCluster(ctx c return nil } + // Machine.spec.clusterName names the owning Cluster, not the StackitCluster, + // and a ClusterClass-generated infrastructureRef never shares its name. + cluster, err := clusterutil.GetOwnerCluster(ctx, r.Client, stackitCluster.ObjectMeta) + switch { + case apierrors.IsNotFound(err) || cluster == nil: + return nil + case err != nil: + logf.FromContext(ctx).Error(err, "Failed to get owning Cluster for StackitCluster watch", "stackitCluster", client.ObjectKeyFromObject(stackitCluster)) + return nil + } + machines := &clusterv1.MachineList{} - if err := r.List(ctx, machines, client.InNamespace(stackitCluster.Namespace)); err != nil { + if err := r.List(ctx, machines, + client.InNamespace(stackitCluster.Namespace), + client.MatchingLabels{clusterv1.ClusterNameLabel: cluster.Name}, + ); err != nil { logf.FromContext(ctx).Error(err, "Failed to list Machines for StackitCluster watch", "stackitCluster", client.ObjectKeyFromObject(stackitCluster)) return nil } return stackitMachineRequestsForMachines(machines.Items, func(machine clusterv1.Machine) bool { - return machine.Spec.ClusterName == stackitCluster.Name + return machine.Spec.ClusterName == cluster.Name }) } diff --git a/scope/cluster_scope.go b/scope/cluster_scope.go index 772499a..f8d67ea 100644 --- a/scope/cluster_scope.go +++ b/scope/cluster_scope.go @@ -41,17 +41,17 @@ type ClusterScope struct { func NewClusterScope( k8sClient client.Client, cluster *clusterv1.Cluster, - sc *infrav1.StackitCluster, + stackitCluster *infrav1.StackitCluster, ) (*ClusterScope, error) { - ph, err := patch.NewHelper(sc, k8sClient) + patchHelper, err := patch.NewHelper(stackitCluster, k8sClient) if err != nil { return nil, err } return &ClusterScope{ Client: k8sClient, Cluster: cluster, - StackitCluster: sc, - patchHelper: ph, + StackitCluster: stackitCluster, + patchHelper: patchHelper, }, nil } diff --git a/scope/machine_scope.go b/scope/machine_scope.go index 0e982e7..23918cd 100644 --- a/scope/machine_scope.go +++ b/scope/machine_scope.go @@ -44,7 +44,7 @@ func NewMachineScope( stackitCluster *infrav1.StackitCluster, stackitMachine *infrav1.StackitMachine, ) (*MachineScope, error) { - ph, err := patch.NewHelper(stackitMachine, k8sClient) + patchHelper, err := patch.NewHelper(stackitMachine, k8sClient) if err != nil { return nil, err } @@ -54,7 +54,7 @@ func NewMachineScope( Machine: machine, StackitCluster: stackitCluster, StackitMachine: stackitMachine, - patchHelper: ph, + patchHelper: patchHelper, }, nil } diff --git a/util/conditions.go b/util/conditions.go index 858e4f4..f60b2bf 100644 --- a/util/conditions.go +++ b/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 }