Merge pull request #317 from llxp/feature/fix-deleted-cluster-skipping-reconcile

🐛 fix skipping reconciliation, when a referenced cluster is already deleted

Author: k8s-ci-robot

internal/controllers/ipaddressclaim_test.go

@@ -1903,32 +1903,47 @@ var _ = Describe("IPAddressClaimReconciler", func() {
 		})
 
 		Context("When the cluster can not be retrieved", func() {
-			AfterEach(func() {
-				deleteClaim("test", namespace)
-				deleteNamespacedPool(poolName, namespace)
+			When("the claim is not fulfilled", func() {
+				AfterEach(func() {
+					deleteClaim("test", namespace)
+					deleteNamespacedPool(poolName, namespace)
+				})
+				It("does not allocate an address", func() {
+					claim := newClaim("test", namespace, "InClusterIPPool", poolName)
+					claim.Spec.ClusterName = clusterName
+					claim.Finalizers = []string{ipamutil.ReleaseAddressFinalizer}
+					Expect(k8sClient.Create(context.Background(), &claim)).To(Succeed())
+					Eventually(Get(&claim)).Should(Succeed())
+
+					addresses := ipamv1.IPAddressList{}
+					Consistently(ObjectList(&addresses, client.InNamespace(namespace))).
+						WithTimeout(5 * time.Second).WithPolling(100 * time.Millisecond).Should(
+						HaveField("Items", HaveLen(0)))
+				})
 			})
-			It("When the cluster cannot be retrieved", func() {
-				claim := ipamv1.IPAddressClaim{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test",
-						Namespace: namespace,
-					},
-					Spec: ipamv1.IPAddressClaimSpec{
-						ClusterName: clusterName,
-						PoolRef: corev1.TypedLocalObjectReference{
-							APIGroup: ptr.To("ipam.cluster.x-k8s.io"),
-							Kind:     "InClusterIPPool",
-							Name:     poolName,
+
+			When("the claim was fulfilled and is deleted", func() {
+				It("releases the IP address", func() {
+					// does not need to be paused, as the cluster just exists to allow the claim to be fulfilled before deleting the cluster again
+					cluster := clusterv1.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      clusterName,
+							Namespace: namespace,
 						},
-					},
-				}
-				Expect(k8sClient.Create(context.Background(), &claim)).To(Succeed())
-				Eventually(Get(&claim)).Should(Succeed())
+					}
+					Expect(k8sClient.Create(context.Background(), &cluster)).To(Succeed())
+					Eventually(Get(&cluster)).Should(Succeed())
+					claim := newClaim("test", namespace, "InClusterIPPool", poolName)
+					claim.Spec.ClusterName = clusterName
+					claim.Finalizers = []string{ipamutil.ReleaseAddressFinalizer}
+					Expect(k8sClient.Create(context.Background(), &claim)).To(Succeed())
+					Eventually(Get(&claim)).Should(Succeed())
 
-				addresses := ipamv1.IPAddressList{}
-				Consistently(ObjectList(&addresses, client.InNamespace(namespace))).
-					WithTimeout(5 * time.Second).WithPolling(100 * time.Millisecond).Should(
-					HaveField("Items", HaveLen(0)))
+					deleteCluster(clusterName, namespace)
+
+					Expect(k8sClient.Delete(context.Background(), &claim)).To(Succeed())
+					Eventually(Get(&claim)).ShouldNot(Succeed())
+				})
 			})
 		})
 

pkg/ipamutil/reconciler.go

@@ -140,7 +140,15 @@ func (r *ClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ct
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			if !claim.ObjectMeta.DeletionTimestamp.IsZero() {
-				return ctrl.Result{}, r.reconcileDelete(ctx, claim)
+				patch := client.MergeFrom(claim.DeepCopy())
+				if err := r.reconcileDelete(ctx, claim); err != nil {
+					return ctrl.Result{}, fmt.Errorf("reconcile delete: %w", err)
+				}
+				// we'll need to explicitly patch the claim here since we haven't set up a patch helper yet.
+				if err := r.Client.Patch(ctx, claim, patch); err != nil {
+					return ctrl.Result{}, fmt.Errorf("patch after reconciling delete: %w", err)
+				}
+				return ctrl.Result{}, nil
 			}
 			log.Info("IPAddressClaim linked to a cluster that is not found, unable to determine cluster's paused state, skipping reconciliation")
 			return ctrl.Result{}, nil
@@ -265,8 +273,9 @@ func (r *ClaimReconciler) reconcileDelete(ctx context.Context, claim *ipamv1.IPA
 
 	if address.Name != "" {
 		var err error
+		patch := client.MergeFrom(address.DeepCopy())
 		if controllerutil.RemoveFinalizer(address, ProtectAddressFinalizer) {
-			if err = r.Client.Update(ctx, address); err != nil && !apierrors.IsNotFound(err) {
+			if err = r.Client.Patch(ctx, address, patch); err != nil && !apierrors.IsNotFound(err) {
 				return errors.Wrap(err, "failed to remove address finalizer")
 			}
 		}