fix: error being silenced during apply (#395)

* fix: error being silenced during apply

Signed-off-by: Tianpeng Wang <tpwang@alauda.io>

* add test

Signed-off-by: Tianpeng Wang <tpwang@alauda.io>

* Apply suggestions from code review

Co-authored-by: Marcin Owsiany <marcin@owsiany.pl>

* fix lint

Signed-off-by: Tianpeng Wang <tpwang@alauda.io>

* check error status message

Signed-off-by: Tianpeng Wang <tpwang@alauda.io>

---------

Signed-off-by: Tianpeng Wang <tpwang@alauda.io>
Co-authored-by: Marcin Owsiany <marcin@owsiany.pl>

Author: timonwong

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) (_ 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

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, 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) {
+												// workaround https://github.com/kubernetes/kubernetes/issues/124347
+												return apierrors.NewNotFound(gvk.GroupVersion().WithResource("testapps").GroupResource(), obj.GetName())
+											}
+										}
+										return nil
 									},
 								}).WithStatusSubresource(obj).Build()
 								r.client = cl
@@ -752,6 +759,44 @@ 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() {
+								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(_ context.Context, _ client.Client, _ string, _ client.Object, _ ...client.SubResourceUpdateOption) error {
+										return apierrors.NewBadRequest("XXXInvalidXXX")
+									},
+								}).WithStatusSubresource(obj).Build()
+								r.client = cl
+							})
+
+							By("propagating the error from status update", func() {
+								res, err := r.Reconcile(ctx, req)
+								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"))
+							})
+						})
+					})
 					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() {