OSSM-6128 Improve code for adding finalizer (#1715)

- handle conflicts - immediately stop reconciling when finalizer is added and continue when the next Reconcile is called
maistra · Mar 21, 2024 · ecbc663 · ecbc663
1 parent 762e4ce
commit ecbc663
Show file tree

Hide file tree

Showing 4 changed files with 125 additions and 15 deletions.
diff --git a/controllers/istiocni/istiocni_controller.go b/controllers/istiocni/istiocni_controller.go
@@ -105,11 +105,7 @@ func (r *IstioCNIReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	}
 
 	if !kube.HasFinalizer(&cni) {
-		err := kube.AddFinalizer(ctx, &cni, r.Client)
-		if err != nil {
-			log.Info("failed to add finalizer")
-			return ctrl.Result{}, err
-		}
+		return kube.AddFinalizer(ctx, &cni, r.Client)
 	}
 
 	if err := validateIstioCNI(cni); err != nil {

diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go
@@ -113,11 +113,7 @@ func (r *IstioRevisionReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	}
 
 	if !kube.HasFinalizer(&rev) {
-		err := kube.AddFinalizer(ctx, &rev, r.Client)
-		if err != nil {
-			log.Info("failed to add finalizer")
-			return ctrl.Result{}, err
-		}
+		return kube.AddFinalizer(ctx, &rev, r.Client)
 	}
 
 	if err := validateIstioRevision(rev); err != nil {

diff --git a/pkg/kube/finalizers.go b/pkg/kube/finalizers.go
@@ -59,7 +59,7 @@ func RemoveFinalizer(ctx context.Context, obj client.Object, cl client.Client) (
 	return ctrl.Result{}, nil
 }
 
-func AddFinalizer(ctx context.Context, obj client.Object, cl client.Client) error {
+func AddFinalizer(ctx context.Context, obj client.Object, cl client.Client) (ctrl.Result, error) {
 	log := logf.FromContext(ctx)
 	log.Info("Adding finalizer")
 
@@ -70,12 +70,17 @@ func AddFinalizer(ctx context.Context, obj client.Object, cl client.Client) erro
 
 	err := cl.Update(ctx, obj)
 	if errors.IsNotFound(err) {
-		// Object was deleted manually before we could add the finalizer to it. This is not an error.
-		return nil
+		log.Info("Resource no longer exists; nothing to do")
+		return ctrl.Result{}, nil
+	} else if errors.IsConflict(err) {
+		log.Info("Conflict while adding finalizer; Requeuing reconciliation")
+		return ctrl.Result{RequeueAfter: 2 * time.Second}, nil
 	} else if err != nil {
-		return pkgerrors.Wrapf(err, "Could not add finalizer to %s/%s", objectMeta.GetNamespace(), objectMeta.GetName())
+		return ctrl.Result{}, pkgerrors.Wrapf(err, "Could not add finalizer to %s/%s", objectMeta.GetNamespace(), objectMeta.GetName())
 	}
-	return nil
+
+	log.Info("Finalizer added")
+	return ctrl.Result{}, nil
 }
 
 func getObjectMeta(obj client.Object) meta.Object {

diff --git a/pkg/kube/finalizers_test.go b/pkg/kube/finalizers_test.go
@@ -194,3 +194,116 @@ func TestRemoveFinalizer(t *testing.T) {
 		})
 	}
 }
+
+func TestAddFinalizer(t *testing.T) {
+	tests := []struct {
+		name               string
+		obj                client.Object
+		interceptorFuncs   interceptor.Funcs
+		expectResult       ctrl.Result
+		expectError        bool
+		checkFinalizers    bool
+		expectedFinalizers []string
+	}{
+		{
+			name: "object not found",
+			obj: &v1alpha1.Istio{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			interceptorFuncs: interceptor.Funcs{
+				Get: func(ctx context.Context, client client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+					return errors.NewNotFound(schema.GroupResource{}, "Istio")
+				},
+			},
+			expectResult: ctrl.Result{},
+			expectError:  false,
+		},
+		{
+			name: "update conflict",
+			obj: &v1alpha1.Istio{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			interceptorFuncs: interceptor.Funcs{
+				Update: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
+					return errors.NewConflict(schema.GroupResource{}, "dummy", fmt.Errorf("simulated conflict error"))
+				},
+			},
+			expectResult: ctrl.Result{RequeueAfter: 2 * time.Second},
+			expectError:  false,
+		},
+		{
+			name: "update error",
+			obj: &v1alpha1.Istio{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			interceptorFuncs: interceptor.Funcs{
+				Update: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
+					return fmt.Errorf("simulated update error")
+				},
+			},
+			expectResult: ctrl.Result{},
+			expectError:  true,
+		},
+		{
+			name: "success with no previous finalizer",
+			obj: &v1alpha1.Istio{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			expectResult:       ctrl.Result{},
+			expectError:        false,
+			checkFinalizers:    true,
+			expectedFinalizers: []string{common.FinalizerName},
+		},
+		{
+			name: "success with other finalizers",
+			obj: &v1alpha1.Istio{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test",
+					Finalizers: []string{"example.com/some-finalizer"},
+				},
+			},
+			expectResult:       ctrl.Result{},
+			expectError:        false,
+			checkFinalizers:    true,
+			expectedFinalizers: []string{"example.com/some-finalizer", common.FinalizerName},
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			cl := fake.NewClientBuilder().
+				WithScheme(scheme.Scheme).
+				WithObjects(tc.obj).
+				WithInterceptorFuncs(tc.interceptorFuncs).
+				Build()
+
+			result, err := AddFinalizer(context.TODO(), tc.obj, cl)
+
+			assert.Equal(t, result, tc.expectResult)
+
+			if tc.expectError {
+				if err == nil {
+					t.Fatalf("Expected error, got nil")
+				}
+				return
+			}
+
+			if err != nil {
+				t.Fatalf("Expected no error, got %v", err)
+			}
+
+			if tc.checkFinalizers {
+				obj := &v1alpha1.Istio{}
+				assert.NilError(t, cl.Get(context.TODO(), types.NamespacedName{Name: tc.obj.GetName()}, obj))
+				assert.DeepEqual(t, sets.NewString(obj.GetFinalizers()...), sets.NewString(tc.expectedFinalizers...))
+			}
+		})
+	}
+}