Merge pull request kubernetes-sigs#5971 from killianmuldoon/fix/error…

…-reporting 🐛 Remove generated names from error messages to reduce reconciliation
shapeblue · Jan 25, 2022 · 5d126b1 · 5d126b1
2 parents 5b9922a + 78eec8e
commit 5d126b1
Show file tree

Hide file tree

Showing 2 changed files with 94 additions and 19 deletions.
diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go
@@ -19,6 +19,7 @@ package cluster
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
@@ -184,27 +185,26 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope)
 
 		cpInfraRef, err := contract.ControlPlane().MachineTemplate().InfrastructureRef().Get(s.Desired.ControlPlane.Object)
 		if err != nil {
-			return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.InfrastructureMachineTemplate})
+			return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: s.Desired.ControlPlane.InfrastructureMachineTemplate})
 		}
 
 		// Create or update the MachineInfrastructureTemplate of the control plane.
-		err = r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
+		if err = r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
 			cluster:              s.Current.Cluster,
 			ref:                  cpInfraRef,
 			current:              s.Current.ControlPlane.InfrastructureMachineTemplate,
 			desired:              s.Desired.ControlPlane.InfrastructureMachineTemplate,
 			compatibilityChecker: check.ObjectsAreCompatible,
 			templateNamePrefix:   controlPlaneInfrastructureMachineTemplateNamePrefix(s.Current.Cluster.Name),
 		},
-		)
-		if err != nil {
-			return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.InfrastructureMachineTemplate})
+		); err != nil {
+			return err
 		}
 
 		// The controlPlaneObject.Spec.machineTemplate.infrastructureRef has to be updated in the desired object
 		err = contract.ControlPlane().MachineTemplate().InfrastructureRef().Set(s.Desired.ControlPlane.Object, refToUnstructured(cpInfraRef))
 		if err != nil {
-			return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object})
+			return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object})
 		}
 	}
 
@@ -227,7 +227,7 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope)
 			},
 		},
 	}); err != nil {
-		return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object})
+		return err
 	}
 
 	// If the ControlPlane has defined a current or desired MachineHealthCheck attempt to reconcile it.
@@ -242,7 +242,7 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope)
 
 		// Reconcile the current and desired state of the MachineHealthCheck.
 		if err := r.reconcileMachineHealthCheck(ctx, s.Current.ControlPlane.MachineHealthCheck, s.Desired.ControlPlane.MachineHealthCheck); err != nil {
-			return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.MachineHealthCheck})
+			return err
 		}
 	}
 	return nil
@@ -403,21 +403,21 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, cluster *clust
 		cluster: cluster,
 		desired: md.InfrastructureMachineTemplate,
 	}); err != nil {
-		return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.Object})
+		return errors.Wrapf(err, "failed to create %s", md.Object.Kind)
 	}
 
 	bootstrapCtx, _ := log.WithObject(md.BootstrapTemplate).Into(ctx)
 	if err := r.reconcileReferencedTemplate(bootstrapCtx, reconcileReferencedTemplateInput{
 		cluster: cluster,
 		desired: md.BootstrapTemplate,
 	}); err != nil {
-		return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.Object})
+		return errors.Wrapf(err, "failed to create %s", md.Object.Kind)
 	}
 
 	log = log.WithObject(md.Object)
 	log.Infof(fmt.Sprintf("Creating %s", tlog.KObj{Obj: md.Object}))
 	if err := r.Client.Create(ctx, md.Object.DeepCopy()); err != nil {
-		return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.Object})
+		return createErrorWithoutObjectName(err, md.Object)
 	}
 	r.recorder.Eventf(cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: md.Object})
 
@@ -429,7 +429,7 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, cluster *clust
 			*ownerReferenceTo(md.Object),
 		})
 		if err := r.reconcileMachineHealthCheck(ctx, nil, md.MachineHealthCheck); err != nil {
-			return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.MachineHealthCheck})
+			return err
 		}
 	}
 	return nil
@@ -448,7 +448,7 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, cluster *clust
 		templateNamePrefix:   infrastructureMachineTemplateNamePrefix(cluster.Name, mdTopologyName),
 		compatibilityChecker: check.ObjectsAreCompatible,
 	}); err != nil {
-		return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: currentMD.Object})
+		return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: currentMD.Object})
 	}
 
 	bootstrapCtx, _ := log.WithObject(desiredMD.BootstrapTemplate).Into(ctx)
@@ -460,13 +460,13 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, cluster *clust
 		templateNamePrefix:   bootstrapTemplateNamePrefix(cluster.Name, mdTopologyName),
 		compatibilityChecker: check.ObjectsAreInTheSameNamespace,
 	}); err != nil {
-		return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: currentMD.Object})
+		return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: currentMD.Object})
 	}
 
 	// Patch MachineHealthCheck for the MachineDeployment.
 	if desiredMD.MachineHealthCheck != nil || currentMD.MachineHealthCheck != nil {
 		if err := r.reconcileMachineHealthCheck(ctx, currentMD.MachineHealthCheck, desiredMD.MachineHealthCheck); err != nil {
-			return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: desiredMD.MachineHealthCheck})
+			return err
 		}
 	}
 
@@ -525,7 +525,7 @@ func (r *Reconciler) deleteMachineDeployment(ctx context.Context, cluster *clust
 	// delete MachineHealthCheck for the MachineDeployment.
 	if md.MachineHealthCheck != nil {
 		if err := r.reconcileMachineHealthCheck(ctx, md.MachineHealthCheck, nil); err != nil {
-			return errors.Wrapf(err, "failed to delete %s", tlog.KObj{Obj: md.MachineHealthCheck})
+			return err
 		}
 	}
 	log.Infof("Deleting %s", tlog.KObj{Obj: md.Object})
@@ -587,7 +587,7 @@ func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcile
 			return errors.Wrapf(err, "failed to create a copy of %s with the managed field annotation", tlog.KObj{Obj: in.desired})
 		}
 		if err := r.Client.Create(ctx, desiredWithManagedFieldAnnotation); err != nil {
-			return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: desiredWithManagedFieldAnnotation})
+			return createErrorWithoutObjectName(err, desiredWithManagedFieldAnnotation)
 		}
 		r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: in.desired})
 		return nil
@@ -664,7 +664,7 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci
 			return errors.Wrapf(err, "failed to create a copy of %s with the managed field annotation", tlog.KObj{Obj: in.desired})
 		}
 		if err := r.Client.Create(ctx, desiredWithManagedFieldAnnotation); err != nil {
-			return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: desiredWithManagedFieldAnnotation})
+			return createErrorWithoutObjectName(err, desiredWithManagedFieldAnnotation)
 		}
 		r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: in.desired})
 		return nil
@@ -716,7 +716,7 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci
 		return errors.Wrapf(err, "failed to create a copy of %s with the managed field annotation", tlog.KObj{Obj: in.desired})
 	}
 	if err := r.Client.Create(ctx, desiredWithManagedFieldAnnotation); err != nil {
-		return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: desiredWithManagedFieldAnnotation})
+		return createErrorWithoutObjectName(err, desiredWithManagedFieldAnnotation)
 	}
 	r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q as a replacement for %q (template rotation)", tlog.KObj{Obj: in.desired}, in.ref.Name)
 
@@ -727,3 +727,29 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci
 
 	return nil
 }
+
+// createErrorWithoutObjectName removes the name of the object from the error message. As each new Create call involves an
+// object with a unique generated name each error appears to be a different error. As the errors are being surfaced in a condition
+// on the Cluster, the name is removed here to prevent each creation error from triggering a new reconciliation.
+func createErrorWithoutObjectName(err error, obj client.Object) error {
+	var statusError *apierrors.StatusError
+	if errors.As(err, &statusError) {
+		if statusError.Status().Details != nil {
+			var causes []string
+			for _, cause := range statusError.Status().Details.Causes {
+				causes = append(causes, fmt.Sprintf("%s: %s: %s", cause.Type, cause.Field, cause.Message))
+			}
+			var msg string
+			if len(causes) > 0 {
+				msg = fmt.Sprintf("failed to create %s.%s: %s", statusError.Status().Details.Kind, statusError.Status().Details.Group, strings.Join(causes, " "))
+			} else {
+				msg = fmt.Sprintf("failed to create %s.%s", statusError.Status().Details.Kind, statusError.Status().Details.Group)
+			}
+			// Replace the statusError message with the constructed message.
+			statusError.ErrStatus.Message = msg
+			return statusError
+		}
+	}
+	// If this isn't a StatusError return a more generic error with the object details.
+	return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: obj})
+}
diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go
@@ -19,6 +19,7 @@ package cluster
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"regexp"
 	"testing"
 	"time"
@@ -2081,3 +2082,51 @@ func TestReconciler_reconcileMachineHealthCheck(t *testing.T) {
 		})
 	}
 }
+
+func Test_createErrorWithoutObjectName(t *testing.T) {
+	originalError := &apierrors.StatusError{
+		ErrStatus: metav1.Status{
+			Status:  metav1.StatusFailure,
+			Code:    http.StatusUnprocessableEntity,
+			Reason:  metav1.StatusReasonInvalid,
+			Message: "DockerMachineTemplate.infrastructure.cluster.x-k8s.io \"docker-template-one\" is invalid: spec.template.spec.preLoadImages: Invalid value: \"array\": spec.template.spec.preLoadImages in body must be of type string: \"array\"",
+			Details: &metav1.StatusDetails{
+				Group: "infrastructure.cluster.x-k8s.io",
+				Kind:  "DockerMachineTemplate",
+				Name:  "docker-template-one",
+				Causes: []metav1.StatusCause{
+					{
+						Type:    "FieldValueInvalid",
+						Message: "Invalid value: \"array\": spec.template.spec.preLoadImages in body must be of type string: \"array\"",
+						Field:   "spec.template.spec.preLoadImages",
+					},
+				},
+			},
+		}}
+	wantError := &apierrors.StatusError{
+		ErrStatus: metav1.Status{
+			Status: metav1.StatusFailure,
+			Code:   http.StatusUnprocessableEntity,
+			Reason: metav1.StatusReasonInvalid,
+			// The only difference between the two objects should be in the Message section.
+			Message: "failed to create DockerMachineTemplate.infrastructure.cluster.x-k8s.io: FieldValueInvalid: spec.template.spec.preLoadImages: Invalid value: \"array\": spec.template.spec.preLoadImages in body must be of type string: \"array\"",
+			Details: &metav1.StatusDetails{
+				Group: "infrastructure.cluster.x-k8s.io",
+				Kind:  "DockerMachineTemplate",
+				Name:  "docker-template-one",
+				Causes: []metav1.StatusCause{
+					{
+						Type:    "FieldValueInvalid",
+						Message: "Invalid value: \"array\": spec.template.spec.preLoadImages in body must be of type string: \"array\"",
+						Field:   "spec.template.spec.preLoadImages",
+					},
+				},
+			},
+		},
+	}
+	t.Run("Transform a create error correctly", func(t *testing.T) {
+		g := NewWithT(t)
+		err := createErrorWithoutObjectName(originalError, nil)
+		g.Expect(err).To(Equal(wantError), cmp.Diff(err, wantError))
+	})
+}