From 1c63d8d16c174789e6901b0ceee3a2ba89223b10 Mon Sep 17 00:00:00 2001 From: Tianpeng Wang Date: Thu, 19 Sep 2024 11:28:54 +0800 Subject: [PATCH 1/5] fix: error being silenced during apply Signed-off-by: Tianpeng Wang --- pkg/reconciler/reconciler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index dd648196..ee47fdc5 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -556,13 +556,13 @@ type ControllerSetupFunc func(c ControllerSetup) error // - Deployed - a release for this CR is deployed (but not necessarily ready). // - ReleaseFailed - an installation or upgrade failed. // - Irreconcilable - an error occurred during reconciliation -func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, err error) { log := r.log.WithValues(strings.ToLower(r.gvk.Kind), req.NamespacedName) log.V(1).Info("Reconciliation triggered") obj := &unstructured.Unstructured{} obj.SetGroupVersionKind(*r.gvk) - err := r.client.Get(ctx, req.NamespacedName, obj) + err = r.client.Get(ctx, req.NamespacedName, obj) if apierrors.IsNotFound(err) { log.V(1).Info("Resource %s/%s not found, nothing to do", req.NamespacedName.Namespace, req.NamespacedName.Name) return ctrl.Result{}, nil From 8ad278b1e16fb0f623ad6b9da3c5fe89cf3741e4 Mon Sep 17 00:00:00 2001 From: Tianpeng Wang Date: Thu, 19 Sep 2024 14:09:56 +0800 Subject: [PATCH 2/5] add test Signed-off-by: Tianpeng Wang --- pkg/reconciler/reconciler_test.go | 46 +++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 9114ada3..d7168d3e 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -738,8 +738,15 @@ var _ = Describe("Reconciler", func() { Patch: func(ctx context.Context, _ client.WithWatch, fakeObj client.Object, patch client.Patch, opts ...client.PatchOption) error { return mgr.GetClient().Patch(ctx, fakeObj, patch, opts...) }, - SubResource: func(_ client.WithWatch, subresource string) client.SubResourceClient { - return mgr.GetClient().SubResource(subresource) + SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + // workaround https://github.com/kubernetes/kubernetes/issues/124347 + err := mgr.GetClient().SubResource(subResourceName).Update(ctx, obj, opts...) + if err != nil { + if apierrors.IsConflict(err) { + return apierrors.NewNotFound(gvk.GroupVersion().WithResource("testapps").GroupResource(), obj.GetName()) + } + } + return nil }, }).WithStatusSubresource(obj).Build() r.client = cl @@ -752,6 +759,41 @@ var _ = Describe("Reconciler", func() { }) }) }) + When("errors that were not 'NotFound' occurred while applying CR status", func() { + It("should propagate apply errors to reconciler", func() { + By("configuring a client that will error during apply", func() { + // Make a client that returns the stale CR, but sends writes to the real client. + cl := fake.NewClientBuilder().WithObjects(obj).WithInterceptorFuncs(interceptor.Funcs{ + Create: func(ctx context.Context, _ client.WithWatch, fakeObj client.Object, opts ...client.CreateOption) error { + return mgr.GetClient().Create(ctx, fakeObj, opts...) + }, + Delete: func(ctx context.Context, _ client.WithWatch, fakeObj client.Object, opts ...client.DeleteOption) error { + return mgr.GetClient().Delete(ctx, fakeObj, opts...) + }, + DeleteAllOf: func(ctx context.Context, _ client.WithWatch, fakeObj client.Object, opts ...client.DeleteAllOfOption) error { + return mgr.GetClient().DeleteAllOf(ctx, fakeObj, opts...) + }, + Update: func(ctx context.Context, _ client.WithWatch, fakeObj client.Object, opts ...client.UpdateOption) error { + return mgr.GetClient().Update(ctx, fakeObj, opts...) + }, + Patch: func(ctx context.Context, _ client.WithWatch, fakeObj client.Object, patch client.Patch, opts ...client.PatchOption) error { + return mgr.GetClient().Patch(ctx, fakeObj, patch, opts...) + }, + SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + return apierrors.NewBadRequest("XXXInvalidXXX") + }, + }).WithStatusSubresource(obj).Build() + r.client = cl + }) + + By("successfully ignoring not found errors and returning a nil error", func() { + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).To(HaveOccurred()) + Expect(apierrors.IsBadRequest(err)).To(BeTrue()) + }) + }) + }) When("CR is deleted, release is not present, but uninstall finalizer exists", func() { It("removes the finalizer", func() { By("adding the uninstall finalizer and deleting the CR", func() { From d4f8eeedbcc66ce1809471930506340bec6d0445 Mon Sep 17 00:00:00 2001 From: Timon Wong Date: Thu, 19 Sep 2024 16:12:01 +0800 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Marcin Owsiany --- pkg/reconciler/reconciler_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index d7168d3e..b1034a9b 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -739,10 +739,10 @@ var _ = Describe("Reconciler", func() { return mgr.GetClient().Patch(ctx, fakeObj, patch, opts...) }, SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { - // workaround https://github.com/kubernetes/kubernetes/issues/124347 err := mgr.GetClient().SubResource(subResourceName).Update(ctx, obj, opts...) if err != nil { if apierrors.IsConflict(err) { + // workaround https://github.com/kubernetes/kubernetes/issues/124347 return apierrors.NewNotFound(gvk.GroupVersion().WithResource("testapps").GroupResource(), obj.GetName()) } } @@ -762,7 +762,6 @@ var _ = Describe("Reconciler", func() { When("errors that were not 'NotFound' occurred while applying CR status", func() { It("should propagate apply errors to reconciler", func() { By("configuring a client that will error during apply", func() { - // Make a client that returns the stale CR, but sends writes to the real client. cl := fake.NewClientBuilder().WithObjects(obj).WithInterceptorFuncs(interceptor.Funcs{ Create: func(ctx context.Context, _ client.WithWatch, fakeObj client.Object, opts ...client.CreateOption) error { return mgr.GetClient().Create(ctx, fakeObj, opts...) @@ -786,7 +785,7 @@ var _ = Describe("Reconciler", func() { r.client = cl }) - By("successfully ignoring not found errors and returning a nil error", func() { + By("propagating the error from status update", func() { res, err := r.Reconcile(ctx, req) Expect(res).To(Equal(reconcile.Result{})) Expect(err).To(HaveOccurred()) From 7e5adfc5947c230064754d5bd7b7c9c7853d7e08 Mon Sep 17 00:00:00 2001 From: Tianpeng Wang Date: Thu, 19 Sep 2024 20:25:26 +0800 Subject: [PATCH 4/5] fix lint Signed-off-by: Tianpeng Wang --- pkg/reconciler/reconciler.go | 2 +- pkg/reconciler/reconciler_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index ee47fdc5..0b2ec2bd 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -556,7 +556,7 @@ type ControllerSetupFunc func(c ControllerSetup) error // - Deployed - a release for this CR is deployed (but not necessarily ready). // - ReleaseFailed - an installation or upgrade failed. // - Irreconcilable - an error occurred during reconciliation -func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, err error) { +func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, err error) { log := r.log.WithValues(strings.ToLower(r.gvk.Kind), req.NamespacedName) log.V(1).Info("Reconciliation triggered") diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index b1034a9b..dfdd1d28 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -738,7 +738,7 @@ var _ = Describe("Reconciler", func() { Patch: func(ctx context.Context, _ client.WithWatch, fakeObj client.Object, patch client.Patch, opts ...client.PatchOption) error { return mgr.GetClient().Patch(ctx, fakeObj, patch, opts...) }, - SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + SubResourceUpdate: func(ctx context.Context, _ client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { err := mgr.GetClient().SubResource(subResourceName).Update(ctx, obj, opts...) if err != nil { if apierrors.IsConflict(err) { @@ -778,7 +778,7 @@ var _ = Describe("Reconciler", func() { Patch: func(ctx context.Context, _ client.WithWatch, fakeObj client.Object, patch client.Patch, opts ...client.PatchOption) error { return mgr.GetClient().Patch(ctx, fakeObj, patch, opts...) }, - SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + SubResourceUpdate: func(_ context.Context, _ client.Client, _ string, _ client.Object, _ ...client.SubResourceUpdateOption) error { return apierrors.NewBadRequest("XXXInvalidXXX") }, }).WithStatusSubresource(obj).Build() From abbabcbf2aeb6fdcc50617f7c898967be21fa1cd Mon Sep 17 00:00:00 2001 From: Tianpeng Wang Date: Thu, 19 Sep 2024 20:29:21 +0800 Subject: [PATCH 5/5] check error status message Signed-off-by: Tianpeng Wang --- pkg/reconciler/reconciler_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index dfdd1d28..09dde645 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -790,6 +790,10 @@ var _ = Describe("Reconciler", func() { Expect(res).To(Equal(reconcile.Result{})) Expect(err).To(HaveOccurred()) Expect(apierrors.IsBadRequest(err)).To(BeTrue()) + + var statusErr apierrors.APIStatus + Expect(errors.As(err, &statusErr)).To(BeTrue()) + Expect(statusErr.Status().Message).To(Equal("XXXInvalidXXX")) }) }) })