diff --git a/controllers/istiocni/istiocni_controller.go b/controllers/istiocni/istiocni_controller.go index 1b5df3f7c8..22ba4a07ad 100644 --- 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 index fc0ec4b21a..956fdc31f8 100644 --- 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 index 99df996418..e6ba1bcb70 100644 --- 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 index ba83bde66d..ed808f115e 100644 --- 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...)) + } + }) + } +}