From 0a3b4550a123c20fec0f598be528f2a637180800 Mon Sep 17 00:00:00 2001 From: rashmigottipati Date: Mon, 5 Apr 2021 13:02:18 -0400 Subject: [PATCH 1/7] Initial commit for finalizer helper library Signed-off-by: rashmigottipati --- pkg/finalizer/finalizer.go | 52 ++++++++++++++++++++++++++++++++++++++ pkg/finalizer/types.go | 39 ++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 pkg/finalizer/finalizer.go create mode 100644 pkg/finalizer/types.go diff --git a/pkg/finalizer/finalizer.go b/pkg/finalizer/finalizer.go new file mode 100644 index 0000000000..5d242e9faa --- /dev/null +++ b/pkg/finalizer/finalizer.go @@ -0,0 +1,52 @@ +/* +Copyright 2021 The Kubernetes Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package finalizer + +import ( + "context" + "fmt" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +func NewFinalizers() (Finalizers, error) { + return finalizers(map[string]Finalizer{}), nil +} + +func (f finalizers) Register(key string, finalizer Finalizer) error { + if _, ok := f[key]; ok { + return fmt.Errorf("finalizer for key %q already registered", key) + } + f[key] = finalizer + return nil +} + +func (f finalizers) Finalize(ctx context.Context, obj client.Object) (bool, error) { + needsUpdate := false + for key, finalizer := range f { + if obj.GetDeletionTimestamp() == nil && !controllerutil.ContainsFinalizer(obj, key) { + controllerutil.AddFinalizer(obj, key) + needsUpdate = true + } else if obj.GetDeletionTimestamp() != nil && controllerutil.ContainsFinalizer(obj, key) { + _, err := finalizer.Finalize(ctx, obj) + if err != nil { + return true, fmt.Errorf("finalize failed for key %q: %v", key, err) + } + controllerutil.RemoveFinalizer(obj, key) + needsUpdate = true + } + } + return needsUpdate, nil +} diff --git a/pkg/finalizer/types.go b/pkg/finalizer/types.go new file mode 100644 index 0000000000..eac28d3708 --- /dev/null +++ b/pkg/finalizer/types.go @@ -0,0 +1,39 @@ +/* +Copyright 2021 The Kubernetes Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package finalizer + +import ( + "context" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type Registerer interface { + Register(key string, f Finalizer) error +} + +type Finalizer interface { + Finalize(context.Context, client.Object) (needsUpdate bool, err error) +} + +type finalizers map[string]Finalizer + +type Finalizers interface { + // implements Registerer and Finalizer to finalize + // all registered finalizers if the provided object + // has a deletion timestamp or set all registered + // finalizers if it doesn't + Registerer + Finalizer +} From c6fecbb15597c61eed3c163e3f7f746354598617 Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Tue, 20 Apr 2021 05:35:17 -0400 Subject: [PATCH 2/7] Add unit tests Signed-off-by: Rashmi Gottipati --- pkg/finalizer/finalizer.go | 11 ++-- pkg/finalizer/finalizer_test.go | 92 +++++++++++++++++++++++++++++++++ pkg/finalizer/types.go | 13 +++-- 3 files changed, 107 insertions(+), 9 deletions(-) create mode 100644 pkg/finalizer/finalizer_test.go diff --git a/pkg/finalizer/finalizer.go b/pkg/finalizer/finalizer.go index 5d242e9faa..d3a54f266c 100644 --- a/pkg/finalizer/finalizer.go +++ b/pkg/finalizer/finalizer.go @@ -21,8 +21,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -func NewFinalizers() (Finalizers, error) { - return finalizers(map[string]Finalizer{}), nil +// NewFinalizers returns the Finalizers interface +func NewFinalizers() Finalizers { + return finalizers(map[string]Finalizer{}) } func (f finalizers) Register(key string, finalizer Finalizer) error { @@ -36,13 +37,13 @@ func (f finalizers) Register(key string, finalizer Finalizer) error { func (f finalizers) Finalize(ctx context.Context, obj client.Object) (bool, error) { needsUpdate := false for key, finalizer := range f { - if obj.GetDeletionTimestamp() == nil && !controllerutil.ContainsFinalizer(obj, key) { + if obj.GetDeletionTimestamp().IsZero() && !controllerutil.ContainsFinalizer(obj, key) { controllerutil.AddFinalizer(obj, key) needsUpdate = true } else if obj.GetDeletionTimestamp() != nil && controllerutil.ContainsFinalizer(obj, key) { - _, err := finalizer.Finalize(ctx, obj) + ret, err := finalizer.Finalize(ctx, obj) if err != nil { - return true, fmt.Errorf("finalize failed for key %q: %v", key, err) + return ret, fmt.Errorf("finalize failed for key %q: %v", key, err) } controllerutil.RemoveFinalizer(obj, key) needsUpdate = true diff --git a/pkg/finalizer/finalizer_test.go b/pkg/finalizer/finalizer_test.go new file mode 100644 index 0000000000..6d110db454 --- /dev/null +++ b/pkg/finalizer/finalizer_test.go @@ -0,0 +1,92 @@ +package finalizer + +import ( + "context" + "testing" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest/printer" +) + +type mockFinalizer struct{} + +func (f mockFinalizer) Finalize(context.Context, client.Object) (needsUpdate bool, err error) { + return true, nil +} +func TestFinalizer(t *testing.T) { + RegisterFailHandler(Fail) + suiteName := "Finalizer Suite" + RunSpecsWithDefaultAndCustomReporters(t, suiteName, []Reporter{printer.NewlineReporter{}, printer.NewProwReporter(suiteName)}) +} + +var _ = Describe("TestFinalizer", func() { + var err error + var pod *corev1.Pod + var finalizers Finalizers + var f mockFinalizer + BeforeEach(func() { + pod = &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{"finalizers.sigs.k8s.io/testfinalizer"}, + }, + } + Expect(pod).NotTo(BeNil()) + + finalizers = NewFinalizers() + Expect(finalizers).NotTo(BeNil()) + + f := mockFinalizer{} + Expect(f).NotTo(BeNil()) + + }) + Describe("Finalizer helper library", func() { + It("Register should successfully register a finalizer", func() { + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) + Expect(err).To(BeNil()) + }) + + It("Register should return an error when it is called twice with the same key", func() { + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) + Expect(err).To(BeNil()) + + // calling Register again with the same key should return an error + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring("already registered")) + + }) + + It("Finalize should return no error and indicate false for needsUpdate if a finalizer is not registered", func() { + ret, err := finalizers.Finalize(context.TODO(), pod) + Expect(err).To(BeNil()) + Expect(ret).To(BeFalse()) + }) + + It("Finalize should return no error when deletion timestamp is not nil and the finalizer exists", func() { + now := metav1.Now() + pod.DeletionTimestamp = &now + + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) + Expect(err).To(BeNil()) + + _, err := finalizers.Finalize(context.TODO(), pod) + Expect(err).To(BeNil()) + }) + + It("Finalize should return no error when deletion timestamp is nil and finalizer does not exist", func() { + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) + Expect(err).To(BeNil()) + + pod.DeletionTimestamp = nil + pod.Finalizers = []string{} + + needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + Expect(err).To(BeNil()) + Expect(needsUpdate).To(BeTrue()) + }) + }) +}) diff --git a/pkg/finalizer/types.go b/pkg/finalizer/types.go index eac28d3708..edca7f9661 100644 --- a/pkg/finalizer/types.go +++ b/pkg/finalizer/types.go @@ -19,21 +19,26 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +// Registerer holds Register that will check if a key is already registered +// and error out and it does; and if not registered, it will add the finalizer +// to the finalizers map as the value for the provided key type Registerer interface { Register(key string, f Finalizer) error } +// Finalizer holds Finalize that will add/remove a finalizer based on the +// deletion timestamp being set and return an indication of whether the +// obj needs an update or not type Finalizer interface { Finalize(context.Context, client.Object) (needsUpdate bool, err error) } type finalizers map[string]Finalizer +// Finalizers implements Registerer and Finalizer to finalize all registered +// finalizers if the provided object has a deletion timestamp or set all +// registered finalizers if it does not type Finalizers interface { - // implements Registerer and Finalizer to finalize - // all registered finalizers if the provided object - // has a deletion timestamp or set all registered - // finalizers if it doesn't Registerer Finalizer } From 9f610897c7e4969994273b3477c2387f6eccbdea Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Thu, 22 Apr 2021 12:17:09 -0400 Subject: [PATCH 3/7] Address review comments Signed-off-by: Rashmi Gottipati --- pkg/finalizer/finalizer.go | 26 +++++++++----- pkg/finalizer/finalizer_test.go | 61 +++++++++++++++++++++++++++------ pkg/finalizer/types.go | 2 -- 3 files changed, 69 insertions(+), 20 deletions(-) diff --git a/pkg/finalizer/finalizer.go b/pkg/finalizer/finalizer.go index d3a54f266c..583fdd1547 100644 --- a/pkg/finalizer/finalizer.go +++ b/pkg/finalizer/finalizer.go @@ -17,13 +17,16 @@ import ( "context" "fmt" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) +type finalizers map[string]Finalizer + // NewFinalizers returns the Finalizers interface func NewFinalizers() Finalizers { - return finalizers(map[string]Finalizer{}) + return finalizers{} } func (f finalizers) Register(key string, finalizer Finalizer) error { @@ -35,19 +38,26 @@ func (f finalizers) Register(key string, finalizer Finalizer) error { } func (f finalizers) Finalize(ctx context.Context, obj client.Object) (bool, error) { + var errList []error needsUpdate := false for key, finalizer := range f { - if obj.GetDeletionTimestamp().IsZero() && !controllerutil.ContainsFinalizer(obj, key) { + if dt := obj.GetDeletionTimestamp(); dt.IsZero() && !controllerutil.ContainsFinalizer(obj, key) { controllerutil.AddFinalizer(obj, key) needsUpdate = true - } else if obj.GetDeletionTimestamp() != nil && controllerutil.ContainsFinalizer(obj, key) { - ret, err := finalizer.Finalize(ctx, obj) + } else if !dt.IsZero() && controllerutil.ContainsFinalizer(obj, key) { + finalizerNeedsUpdate, err := finalizer.Finalize(ctx, obj) if err != nil { - return ret, fmt.Errorf("finalize failed for key %q: %v", key, err) + // Even when the finalizer fails, it may need to signal to update the primary + // object (e.g. it may set a condition and need a status update). + needsUpdate = needsUpdate || finalizerNeedsUpdate + errList = append(errList, fmt.Errorf("finalizer %q failed: %v", key, err)) + } else { + // If the finalizer succeeds, we remove the finalizer from the primary + // object's metadata, so we know it will need an update. + needsUpdate = true + controllerutil.RemoveFinalizer(obj, key) } - controllerutil.RemoveFinalizer(obj, key) - needsUpdate = true } } - return needsUpdate, nil + return needsUpdate, utilerrors.NewAggregate(errList) } diff --git a/pkg/finalizer/finalizer_test.go b/pkg/finalizer/finalizer_test.go index 6d110db454..80a197ec04 100644 --- a/pkg/finalizer/finalizer_test.go +++ b/pkg/finalizer/finalizer_test.go @@ -43,13 +43,13 @@ var _ = Describe("TestFinalizer", func() { Expect(f).NotTo(BeNil()) }) - Describe("Finalizer helper library", func() { - It("Register should successfully register a finalizer", func() { + Describe("Register", func() { + It("successfully registers a finalizer", func() { err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) Expect(err).To(BeNil()) }) - It("Register should return an error when it is called twice with the same key", func() { + It("should fail when trying to register a finalizer that was already registered", func() { err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) Expect(err).To(BeNil()) @@ -59,31 +59,72 @@ var _ = Describe("TestFinalizer", func() { Expect(err.Error()).To(ContainSubstring("already registered")) }) - - It("Finalize should return no error and indicate false for needsUpdate if a finalizer is not registered", func() { + }) + Describe("Finalize", func() { + It("should return no error and return false for needsUpdate if a finalizer is not registered", func() { ret, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(ret).To(BeFalse()) }) - It("Finalize should return no error when deletion timestamp is not nil and the finalizer exists", func() { + It("successfully finalizes and returns true for needsUpdate when deletion timestamp is nil and finalizer does not exist", func() { + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) + Expect(err).To(BeNil()) + + pod.DeletionTimestamp = nil + pod.Finalizers = []string{} + + needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + Expect(err).To(BeNil()) + Expect(needsUpdate).To(BeTrue()) + }) + + It("successfully finalizes and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() { now := metav1.Now() pod.DeletionTimestamp = &now err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) Expect(err).To(BeNil()) - _, err := finalizers.Finalize(context.TODO(), pod) + pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer"} + + needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) + Expect(needsUpdate).To(BeTrue()) }) - It("Finalize should return no error when deletion timestamp is nil and finalizer does not exist", func() { - err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) + It("should return no error and return false for needsUpdate when deletion timestamp is nil and finalizer doesn't exist", func() { + pod.DeletionTimestamp = nil + pod.Finalizers = []string{} + + needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) + Expect(needsUpdate).To(BeFalse()) + }) - pod.DeletionTimestamp = nil + It("should return no error and return false for needsUpdate when deletion timestamp is not nil and the finalizer doesn't exist", func() { + now := metav1.Now() + pod.DeletionTimestamp = &now pod.Finalizers = []string{} + needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + Expect(err).To(BeNil()) + Expect(needsUpdate).To(BeFalse()) + + }) + + It("successfully finalizes multiple finalizers and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() { + now := metav1.Now() + pod.DeletionTimestamp = &now + + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) + Expect(err).To(BeNil()) + + err = finalizers.Register("finalizers.sigs.k8s.io/newtestfinalizer", f) + Expect(err).To(BeNil()) + + pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer", "finalizers.sigs.k8s.io/newtestfinalizer"} + needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeTrue()) diff --git a/pkg/finalizer/types.go b/pkg/finalizer/types.go index edca7f9661..1020ad157c 100644 --- a/pkg/finalizer/types.go +++ b/pkg/finalizer/types.go @@ -33,8 +33,6 @@ type Finalizer interface { Finalize(context.Context, client.Object) (needsUpdate bool, err error) } -type finalizers map[string]Finalizer - // Finalizers implements Registerer and Finalizer to finalize all registered // finalizers if the provided object has a deletion timestamp or set all // registered finalizers if it does not From b2693223fb31846535f351caadaa0e55e978613e Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Wed, 19 May 2021 09:53:29 -0400 Subject: [PATCH 4/7] Add needsUpdate and err fields on mockFinalizer struct for customizations Signed-off-by: Rashmi Gottipati --- pkg/finalizer/finalizer_test.go | 68 ++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/pkg/finalizer/finalizer_test.go b/pkg/finalizer/finalizer_test.go index 80a197ec04..f809f52028 100644 --- a/pkg/finalizer/finalizer_test.go +++ b/pkg/finalizer/finalizer_test.go @@ -2,6 +2,7 @@ package finalizer import ( "context" + "fmt" "testing" . "github.com/onsi/ginkgo" @@ -12,10 +13,13 @@ import ( "sigs.k8s.io/controller-runtime/pkg/envtest/printer" ) -type mockFinalizer struct{} +type mockFinalizer struct { + needsUpdate bool + err error +} func (f mockFinalizer) Finalize(context.Context, client.Object) (needsUpdate bool, err error) { - return true, nil + return f.needsUpdate, f.err } func TestFinalizer(t *testing.T) { RegisterFailHandler(Fail) @@ -129,5 +133,65 @@ var _ = Describe("TestFinalizer", func() { Expect(err).To(BeNil()) Expect(needsUpdate).To(BeTrue()) }) + + It("should return needsUpdate as false and a non-nil error", func() { + now := metav1.Now() + pod.DeletionTimestamp = &now + pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer"} + + f.needsUpdate = false + f.err = fmt.Errorf("finalizer failed for %q", pod.Finalizers[0]) + + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) + Expect(err).To(BeNil()) + + needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring("finalizer failed")) + Expect(needsUpdate).To(BeFalse()) + }) + + It("should return expected needsUpdate and error values when registering multiple finalizers", func() { + now := metav1.Now() + pod.DeletionTimestamp = &now + pod.Finalizers = []string{ + "finalizers.sigs.k8s.io/testfinalizer1", + "finalizers.sigs.k8s.io/testfinalizer2", + "finalizers.sigs.k8s.io/testfinalizer3", + } + + // registering multiple finalizers with different return values + // test for needsUpdate as true, and nil error + f.needsUpdate = true + f.err = nil + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer1", f) + Expect(err).To(BeNil()) + + result, err := finalizers.Finalize(context.TODO(), pod) + Expect(err).To(BeNil()) + Expect(result).To(BeTrue()) + + // test for needsUpdate as false, and non-nil error + f.needsUpdate = false + f.err = fmt.Errorf("finalizer failed") + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer2", f) + Expect(err).To(BeNil()) + + result, err = finalizers.Finalize(context.TODO(), pod) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring("finalizer failed")) + Expect(result).To(BeFalse()) + + // test for needsUpdate as true, and non-nil error + f.needsUpdate = true + f.err = fmt.Errorf("finalizer failed") + err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer3", f) + Expect(err).To(BeNil()) + + result, err = finalizers.Finalize(context.TODO(), pod) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring("finalizer failed")) + Expect(result).To(BeTrue()) + }) }) }) From ac093b0af5c2eb6a9ece8a68331f9e9916a85240 Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Thu, 20 May 2021 22:58:38 -0400 Subject: [PATCH 5/7] Address review feedback #2 Signed-off-by: Rashmi Gottipati --- pkg/finalizer/finalizer_test.go | 41 ++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/pkg/finalizer/finalizer_test.go b/pkg/finalizer/finalizer_test.go index f809f52028..d6e49453d0 100644 --- a/pkg/finalizer/finalizer_test.go +++ b/pkg/finalizer/finalizer_test.go @@ -34,18 +34,10 @@ var _ = Describe("TestFinalizer", func() { var f mockFinalizer BeforeEach(func() { pod = &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Finalizers: []string{"finalizers.sigs.k8s.io/testfinalizer"}, - }, + ObjectMeta: metav1.ObjectMeta{}, } - Expect(pod).NotTo(BeNil()) - finalizers = NewFinalizers() - Expect(finalizers).NotTo(BeNil()) - - f := mockFinalizer{} - Expect(f).NotTo(BeNil()) - + f = mockFinalizer{} }) Describe("Register", func() { It("successfully registers a finalizer", func() { @@ -65,12 +57,6 @@ var _ = Describe("TestFinalizer", func() { }) }) Describe("Finalize", func() { - It("should return no error and return false for needsUpdate if a finalizer is not registered", func() { - ret, err := finalizers.Finalize(context.TODO(), pod) - Expect(err).To(BeNil()) - Expect(ret).To(BeFalse()) - }) - It("successfully finalizes and returns true for needsUpdate when deletion timestamp is nil and finalizer does not exist", func() { err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) Expect(err).To(BeNil()) @@ -81,6 +67,10 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeTrue()) + // when deletion timestamp is nil and finalizer is not present, the registered finalizer would be added to the obj + Expect(len(pod.Finalizers)).To(Equal(1)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer")) + }) It("successfully finalizes and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() { @@ -95,6 +85,8 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeTrue()) + // finalizer will be removed from the obj upon successful finalization + Expect(len(pod.Finalizers)).To(Equal(0)) }) It("should return no error and return false for needsUpdate when deletion timestamp is nil and finalizer doesn't exist", func() { @@ -104,6 +96,8 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeFalse()) + Expect(len(pod.Finalizers)).To(Equal(0)) + }) It("should return no error and return false for needsUpdate when deletion timestamp is not nil and the finalizer doesn't exist", func() { @@ -114,6 +108,7 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeFalse()) + Expect(len(pod.Finalizers)).To(Equal(0)) }) @@ -132,6 +127,7 @@ var _ = Describe("TestFinalizer", func() { needsUpdate, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(needsUpdate).To(BeTrue()) + Expect(len(pod.Finalizers)).To(Equal(0)) }) It("should return needsUpdate as false and a non-nil error", func() { @@ -149,6 +145,8 @@ var _ = Describe("TestFinalizer", func() { Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("finalizer failed")) Expect(needsUpdate).To(BeFalse()) + Expect(len(pod.Finalizers)).To(Equal(1)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer")) }) It("should return expected needsUpdate and error values when registering multiple finalizers", func() { @@ -170,6 +168,11 @@ var _ = Describe("TestFinalizer", func() { result, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) Expect(result).To(BeTrue()) + // `finalizers.sigs.k8s.io/testfinalizer1` will be removed from the list + // of finalizers, so length will be 2. + Expect(len(pod.Finalizers)).To(Equal(2)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2")) + Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3")) // test for needsUpdate as false, and non-nil error f.needsUpdate = false @@ -181,6 +184,9 @@ var _ = Describe("TestFinalizer", func() { Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("finalizer failed")) Expect(result).To(BeFalse()) + Expect(len(pod.Finalizers)).To(Equal(2)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2")) + Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3")) // test for needsUpdate as true, and non-nil error f.needsUpdate = true @@ -192,6 +198,9 @@ var _ = Describe("TestFinalizer", func() { Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("finalizer failed")) Expect(result).To(BeTrue()) + Expect(len(pod.Finalizers)).To(Equal(2)) + Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2")) + Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3")) }) }) }) From 60ab6fdc8f6db577324820b7b8f29c616794b141 Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Sun, 6 Jun 2021 22:30:47 -0400 Subject: [PATCH 6/7] Split needsUpdate into two separate variables and update unit tests Signed-off-by: Rashmi Gottipati --- pkg/finalizer/finalizer.go | 26 +++++++---- pkg/finalizer/finalizer_test.go | 76 +++++++++++++++++++-------------- pkg/finalizer/types.go | 2 +- 3 files changed, 62 insertions(+), 42 deletions(-) diff --git a/pkg/finalizer/finalizer.go b/pkg/finalizer/finalizer.go index 583fdd1547..bbcd46abf6 100644 --- a/pkg/finalizer/finalizer.go +++ b/pkg/finalizer/finalizer.go @@ -24,6 +24,12 @@ import ( type finalizers map[string]Finalizer +// Result struct holds Updated and StatusUpdated fields +type Result struct { + Updated bool + StatusUpdated bool +} + // NewFinalizers returns the Finalizers interface func NewFinalizers() Finalizers { return finalizers{} @@ -37,27 +43,31 @@ func (f finalizers) Register(key string, finalizer Finalizer) error { return nil } -func (f finalizers) Finalize(ctx context.Context, obj client.Object) (bool, error) { - var errList []error - needsUpdate := false +func (f finalizers) Finalize(ctx context.Context, obj client.Object) (Result, error) { + var ( + res Result + errList []error + ) + res.Updated = false for key, finalizer := range f { if dt := obj.GetDeletionTimestamp(); dt.IsZero() && !controllerutil.ContainsFinalizer(obj, key) { controllerutil.AddFinalizer(obj, key) - needsUpdate = true + res.Updated = true } else if !dt.IsZero() && controllerutil.ContainsFinalizer(obj, key) { - finalizerNeedsUpdate, err := finalizer.Finalize(ctx, obj) + finalizerRes, err := finalizer.Finalize(ctx, obj) if err != nil { // Even when the finalizer fails, it may need to signal to update the primary // object (e.g. it may set a condition and need a status update). - needsUpdate = needsUpdate || finalizerNeedsUpdate + res.Updated = res.Updated || finalizerRes.Updated + res.StatusUpdated = res.StatusUpdated || finalizerRes.StatusUpdated errList = append(errList, fmt.Errorf("finalizer %q failed: %v", key, err)) } else { // If the finalizer succeeds, we remove the finalizer from the primary // object's metadata, so we know it will need an update. - needsUpdate = true + res.Updated = true controllerutil.RemoveFinalizer(obj, key) } } } - return needsUpdate, utilerrors.NewAggregate(errList) + return res, utilerrors.NewAggregate(errList) } diff --git a/pkg/finalizer/finalizer_test.go b/pkg/finalizer/finalizer_test.go index d6e49453d0..944acd595a 100644 --- a/pkg/finalizer/finalizer_test.go +++ b/pkg/finalizer/finalizer_test.go @@ -14,12 +14,12 @@ import ( ) type mockFinalizer struct { - needsUpdate bool - err error + result Result + err error } -func (f mockFinalizer) Finalize(context.Context, client.Object) (needsUpdate bool, err error) { - return f.needsUpdate, f.err +func (f mockFinalizer) Finalize(context.Context, client.Object) (Result, error) { + return f.result, f.err } func TestFinalizer(t *testing.T) { RegisterFailHandler(Fail) @@ -56,24 +56,25 @@ var _ = Describe("TestFinalizer", func() { }) }) + Describe("Finalize", func() { - It("successfully finalizes and returns true for needsUpdate when deletion timestamp is nil and finalizer does not exist", func() { + It("successfully finalizes and returns true for Updated when deletion timestamp is nil and finalizer does not exist", func() { err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) Expect(err).To(BeNil()) pod.DeletionTimestamp = nil pod.Finalizers = []string{} - needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + result, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) - Expect(needsUpdate).To(BeTrue()) + Expect(result.Updated).To(BeTrue()) // when deletion timestamp is nil and finalizer is not present, the registered finalizer would be added to the obj Expect(len(pod.Finalizers)).To(Equal(1)) Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer")) }) - It("successfully finalizes and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() { + It("successfully finalizes and returns true for Updated when deletion timestamp is not nil and the finalizer exists", func() { now := metav1.Now() pod.DeletionTimestamp = &now @@ -82,37 +83,37 @@ var _ = Describe("TestFinalizer", func() { pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer"} - needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + result, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) - Expect(needsUpdate).To(BeTrue()) + Expect(result.Updated).To(BeTrue()) // finalizer will be removed from the obj upon successful finalization Expect(len(pod.Finalizers)).To(Equal(0)) }) - It("should return no error and return false for needsUpdate when deletion timestamp is nil and finalizer doesn't exist", func() { + It("should return no error and return false for Updated when deletion timestamp is nil and finalizer doesn't exist", func() { pod.DeletionTimestamp = nil pod.Finalizers = []string{} - needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + result, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) - Expect(needsUpdate).To(BeFalse()) + Expect(result.Updated).To(BeFalse()) Expect(len(pod.Finalizers)).To(Equal(0)) }) - It("should return no error and return false for needsUpdate when deletion timestamp is not nil and the finalizer doesn't exist", func() { + It("should return no error and return false for Updated when deletion timestamp is not nil and the finalizer doesn't exist", func() { now := metav1.Now() pod.DeletionTimestamp = &now pod.Finalizers = []string{} - needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + result, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) - Expect(needsUpdate).To(BeFalse()) + Expect(result.Updated).To(BeFalse()) Expect(len(pod.Finalizers)).To(Equal(0)) }) - It("successfully finalizes multiple finalizers and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() { + It("successfully finalizes multiple finalizers and returns true for Updated when deletion timestamp is not nil and the finalizer exists", func() { now := metav1.Now() pod.DeletionTimestamp = &now @@ -124,32 +125,35 @@ var _ = Describe("TestFinalizer", func() { pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer", "finalizers.sigs.k8s.io/newtestfinalizer"} - needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + result, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) - Expect(needsUpdate).To(BeTrue()) + Expect(result.Updated).To(BeTrue()) + Expect(result.StatusUpdated).To(BeFalse()) Expect(len(pod.Finalizers)).To(Equal(0)) }) - It("should return needsUpdate as false and a non-nil error", func() { + It("should return result as false and a non-nil error", func() { now := metav1.Now() pod.DeletionTimestamp = &now pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer"} - f.needsUpdate = false + f.result.Updated = false + f.result.StatusUpdated = false f.err = fmt.Errorf("finalizer failed for %q", pod.Finalizers[0]) err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f) Expect(err).To(BeNil()) - needsUpdate, err := finalizers.Finalize(context.TODO(), pod) + result, err := finalizers.Finalize(context.TODO(), pod) Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("finalizer failed")) - Expect(needsUpdate).To(BeFalse()) + Expect(result.Updated).To(BeFalse()) + Expect(result.StatusUpdated).To(BeFalse()) Expect(len(pod.Finalizers)).To(Equal(1)) Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer")) }) - It("should return expected needsUpdate and error values when registering multiple finalizers", func() { + It("should return expected result values and error values when registering multiple finalizers", func() { now := metav1.Now() pod.DeletionTimestamp = &now pod.Finalizers = []string{ @@ -159,23 +163,26 @@ var _ = Describe("TestFinalizer", func() { } // registering multiple finalizers with different return values - // test for needsUpdate as true, and nil error - f.needsUpdate = true + // test for Updated as true, and nil error + f.result.Updated = true + f.result.StatusUpdated = false f.err = nil err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer1", f) Expect(err).To(BeNil()) result, err := finalizers.Finalize(context.TODO(), pod) Expect(err).To(BeNil()) - Expect(result).To(BeTrue()) + Expect(result.Updated).To(BeTrue()) + Expect(result.StatusUpdated).To(BeFalse()) // `finalizers.sigs.k8s.io/testfinalizer1` will be removed from the list // of finalizers, so length will be 2. Expect(len(pod.Finalizers)).To(Equal(2)) Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2")) Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3")) - // test for needsUpdate as false, and non-nil error - f.needsUpdate = false + // test for Updated and StatusUpdated as false, and non-nil error + f.result.Updated = false + f.result.StatusUpdated = false f.err = fmt.Errorf("finalizer failed") err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer2", f) Expect(err).To(BeNil()) @@ -183,13 +190,15 @@ var _ = Describe("TestFinalizer", func() { result, err = finalizers.Finalize(context.TODO(), pod) Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("finalizer failed")) - Expect(result).To(BeFalse()) + Expect(result.Updated).To(BeFalse()) + Expect(result.StatusUpdated).To(BeFalse()) Expect(len(pod.Finalizers)).To(Equal(2)) Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2")) Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3")) - // test for needsUpdate as true, and non-nil error - f.needsUpdate = true + // test for result as true, and non-nil error + f.result.Updated = true + f.result.StatusUpdated = true f.err = fmt.Errorf("finalizer failed") err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer3", f) Expect(err).To(BeNil()) @@ -197,7 +206,8 @@ var _ = Describe("TestFinalizer", func() { result, err = finalizers.Finalize(context.TODO(), pod) Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("finalizer failed")) - Expect(result).To(BeTrue()) + Expect(result.Updated).To(BeTrue()) + Expect(result.StatusUpdated).To(BeTrue()) Expect(len(pod.Finalizers)).To(Equal(2)) Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2")) Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3")) diff --git a/pkg/finalizer/types.go b/pkg/finalizer/types.go index 1020ad157c..29d3d1dcc9 100644 --- a/pkg/finalizer/types.go +++ b/pkg/finalizer/types.go @@ -30,7 +30,7 @@ type Registerer interface { // deletion timestamp being set and return an indication of whether the // obj needs an update or not type Finalizer interface { - Finalize(context.Context, client.Object) (needsUpdate bool, err error) + Finalize(context.Context, client.Object) (Result, error) } // Finalizers implements Registerer and Finalizer to finalize all registered From 5eb033dacaf45820517fcf9a2736a4ad3937dfd5 Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Mon, 7 Jun 2021 13:21:43 -0400 Subject: [PATCH 7/7] Address review feedback #3 Signed-off-by: Rashmi Gottipati --- pkg/finalizer/finalizer.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/finalizer/finalizer.go b/pkg/finalizer/finalizer.go index bbcd46abf6..1261b73994 100644 --- a/pkg/finalizer/finalizer.go +++ b/pkg/finalizer/finalizer.go @@ -24,9 +24,13 @@ import ( type finalizers map[string]Finalizer -// Result struct holds Updated and StatusUpdated fields +// Result struct holds information about what parts of an object were updated by finalizer(s). type Result struct { - Updated bool + // Updated will be true if at least one of the object's non-status field + // was updated by some registered finalizer. + Updated bool + // StatusUpdated will be true if at least one of the object's status' fields + // was updated by some registered finalizer. StatusUpdated bool } @@ -66,6 +70,8 @@ func (f finalizers) Finalize(ctx context.Context, obj client.Object) (Result, er // object's metadata, so we know it will need an update. res.Updated = true controllerutil.RemoveFinalizer(obj, key) + // The finalizer may have updated the status too. + res.StatusUpdated = res.StatusUpdated || finalizerRes.StatusUpdated } } }