From 2c9a14e6480e2068f8dcec508ee0f97c70ea2cef Mon Sep 17 00:00:00 2001 From: michaelawyu Date: Tue, 7 Jan 2025 16:53:14 +0800 Subject: [PATCH 1/3] Minor fixes --- pkg/controllers/workapplier/apply.go | 71 ++++++--- .../workapplier/availability_tracker.go | 20 ++- pkg/controllers/workapplier/controller.go | 17 ++- .../workapplier/drift_detection_takeover.go | 139 +++++------------- .../drift_detection_takeover_test.go | 115 --------------- pkg/controllers/workapplier/preprocess.go | 10 +- .../workapplier/preprocess_test.go | 2 +- pkg/controllers/workapplier/process.go | 1 + pkg/controllers/workapplier/utils.go | 97 ++++++++++++ pkg/controllers/workapplier/utils_test.go | 121 +++++++++++++++ 10 files changed, 338 insertions(+), 255 deletions(-) diff --git a/pkg/controllers/workapplier/apply.go b/pkg/controllers/workapplier/apply.go index a689c71f6..5fa4a6c88 100644 --- a/pkg/controllers/workapplier/apply.go +++ b/pkg/controllers/workapplier/apply.go @@ -25,6 +25,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" + "go.goms.io/fleet/pkg/utils/controller" "go.goms.io/fleet/pkg/utils/resource" ) @@ -35,7 +36,7 @@ func init() { _ = clientgoscheme.AddToScheme(builtInScheme) } -// applyInDryRunMode +// applyInDryRunMode dry-runs an apply op. func (r *Reconciler) applyInDryRunMode( ctx context.Context, gvr *schema.GroupVersionResource, @@ -74,8 +75,8 @@ func (r *Reconciler) apply( // Compute the hash of the manifest object. // - // Originally the manifest hash is kept only if three-way merge patch (client side apply esque - // strategy) is used; with the new drift detection and takeover capabilities, the manifest hash + // Originally the manifest hash is kept only if three-way merge patch (client side apply) + // is used; with the new drift detection and takeover capabilities, the manifest hash // will always be kept regardless of the apply strategy in use, as it is needed for // drift detection purposes. // @@ -141,12 +142,12 @@ func (r *Reconciler) apply( switch { case applyStrategy.Type == fleetv1beta1.ApplyStrategyTypeClientSideApply && isLastAppliedAnnotationSet: // The apply strategy dictates that three-way merge patch - // (client-side apply esque patch) should be used, and the last applied annotation + // (client-side apply) should be used, and the last applied annotation // has been set. return r.threeWayMergePatch(ctx, gvr, manifestObjCopy, inMemberClusterObj, isOptimisticLockEnabled, false) case applyStrategy.Type == fleetv1beta1.ApplyStrategyTypeClientSideApply: // The apply strategy dictates that three-way merge patch - // (client-side apply esque patch) should be used, but the last applied annotation + // (client-side apply) should be used, but the last applied annotation // cannot be set. Fleet will fall back to server-side apply. return r.serverSideApply( ctx, @@ -161,8 +162,11 @@ func (r *Reconciler) apply( applyStrategy.ServerSideApplyConfig.ForceConflicts, isOptimisticLockEnabled, false, ) default: - // An unexpected apply strategy has been set. - return nil, fmt.Errorf("unexpected apply strategy %s is found", applyStrategy.Type) + // An unexpected apply strategy has been set. Normally this will never run as the built-in + // validation would block invalid values. + wrappedErr := fmt.Errorf("unexpected apply strategy %s is found", applyStrategy.Type) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } } @@ -177,6 +181,7 @@ func (r *Reconciler) createManifestObject( } createdObj, err := r.spokeDynamicClient.Resource(*gvr).Namespace(manifestObject.GetNamespace()).Create(ctx, manifestObject, createOpts) if err != nil { + _ = controller.NewAPIServerError(false, err) return nil, fmt.Errorf("failed to create manifest object: %w", err) } return createdObj, nil @@ -212,7 +217,9 @@ func (r *Reconciler) threeWayMergePatch( data, err := patch.Data(manifestObj) if err != nil { // Fleet uses raw patch; this branch should never run. - return nil, fmt.Errorf("failed to get patch data: %w", err) + wrappedErr := fmt.Errorf("failed to get patch data: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } // Use three-way merge (similar to kubectl client side apply) to patch the object in the @@ -233,6 +240,7 @@ func (r *Reconciler) threeWayMergePatch( Resource(*gvr).Namespace(manifestObj.GetNamespace()). Patch(ctx, manifestObj.GetName(), patch.Type(), data, patchOpts) if err != nil { + _ = controller.NewAPIServerError(false, err) return nil, fmt.Errorf("failed to patch the manifest object: %w", err) } return patchedObj, nil @@ -274,6 +282,7 @@ func (r *Reconciler) serverSideApply( Resource(*gvr).Namespace(manifestObj.GetNamespace()). Apply(ctx, manifestObj.GetName(), manifestObj, applyOpts) if err != nil { + _ = controller.NewAPIServerError(false, err) return nil, fmt.Errorf("failed to apply the manifest object: %w", err) } return appliedObj, nil @@ -314,7 +323,9 @@ func buildThreeWayMergePatch(manifestObj, liveObj *unstructured.Unstructured) (c patchData, err = jsonmergepatch.CreateThreeWayJSONMergePatch( lastAppliedObjJSONBytes, manifestObjJSONBytes, liveObjJSONBytes, preconditions...) if err != nil { - return nil, err + wrappedErr := fmt.Errorf("failed to create three-way JSON merge patch: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } case err != nil: return nil, err @@ -323,11 +334,15 @@ func buildThreeWayMergePatch(manifestObj, liveObj *unstructured.Unstructured) (c patchType = types.StrategicMergePatchType lookupPatchMeta, err = strategicpatch.NewPatchMetaFromStruct(versionedObject) if err != nil { - return nil, err + wrappedErr := fmt.Errorf("failed to create patch meta from struct (strategic merge patch): %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } patchData, err = strategicpatch.CreateThreeWayMergePatch(lastAppliedObjJSONBytes, manifestObjJSONBytes, liveObjJSONBytes, lookupPatchMeta, true) if err != nil { - return nil, err + wrappedErr := fmt.Errorf("failed to create three-way strategic merge patch: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } } return client.RawPatch(patchType, patchData), nil @@ -346,7 +361,9 @@ func setFleetLastAppliedAnnotation(manifestObj *unstructured.Unstructured) (bool lastAppliedManifestJSONBytes, err := manifestObj.MarshalJSON() if err != nil { - return false, fmt.Errorf("failed to marshal the manifest object into JSON: %w", err) + wrappedErr := fmt.Errorf("failed to marshal the manifest object into JSON: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return false, wrappedErr } annotations[fleetv1beta1.LastAppliedConfigAnnotation] = string(lastAppliedManifestJSONBytes) isLastAppliedAnnotationSet := true @@ -388,7 +405,9 @@ func setManifestHashAnnotation(manifestObj *unstructured.Unstructured) error { cleanedManifestObj := discardFieldsIrrelevantInComparisonFrom(manifestObj) manifestObjHash, err := resource.HashOf(cleanedManifestObj.Object) if err != nil { - return err + wrappedErr := fmt.Errorf("failed to compute the hash of the manifest object: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return wrappedErr } annotations := manifestObj.GetAnnotations() @@ -404,13 +423,13 @@ func setManifestHashAnnotation(manifestObj *unstructured.Unstructured) error { // on a manifest to be applied. func setOwnerRef(obj *unstructured.Unstructured, expectedAppliedWorkOwnerRef *metav1.OwnerReference) { ownerRefs := obj.GetOwnerReferences() - if ownerRefs == nil { - ownerRefs = []metav1.OwnerReference{} - } + // Typically owner references is a system-managed field, and at this moment Fleet will // clear owner references (if any) set in the manifest object. However, for consistency // reasons, here Fleet will still assume that there might be some owner references set // in the manifest object. + // + // TO-DO (chenyu1): evaluate if user-set owner references should be kept. ownerRefs = append(ownerRefs, *expectedAppliedWorkOwnerRef) obj.SetOwnerReferences(ownerRefs) } @@ -431,7 +450,9 @@ func validateOwnerReferences( // perform sanitization on the manifest object before applying it, which removes all owner // references. if len(manifestObjOwnerRefs) > 0 && !applyStrategy.AllowCoOwnership { - return fmt.Errorf("manifest is set to have multiple owner references but co-ownership is disallowed") + wrappedErr := fmt.Errorf("manifest is set to have owner references but co-ownership is disallowed") + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return wrappedErr } // Do a sanity check to verify that no AppliedWork object is directly added as an owner @@ -440,7 +461,9 @@ func validateOwnerReferences( // references. for _, ownerRef := range manifestObjOwnerRefs { if ownerRef.APIVersion == fleetv1beta1.GroupVersion.String() && ownerRef.Kind == fleetv1beta1.AppliedWorkKind { - return fmt.Errorf("an AppliedWork object is unexpectedly added as an owner in the manifest object") + wrappedErr := fmt.Errorf("an AppliedWork object is unexpectedly added as an owner in the manifest object") + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return wrappedErr } } @@ -452,7 +475,9 @@ func validateOwnerReferences( // If the live object is co-owned but co-ownership is no longer allowed, the validation fails. if len(inMemberClusterObjOwnerRefs) > 1 && !applyStrategy.AllowCoOwnership { - return fmt.Errorf("object is co-owned by multiple objects but co-ownership has been disallowed") + wrappedErr := fmt.Errorf("object is co-owned by multiple objects but co-ownership has been disallowed") + _ = controller.NewUserError(wrappedErr) + return wrappedErr } // Note that at this point of execution, one of the owner references is guaranteed to be the @@ -465,7 +490,9 @@ func validateOwnerReferences( } } if !found { - return fmt.Errorf("object is not owned by the expected AppliedWork object") + wrappedErr := fmt.Errorf("object is not owned by the expected AppliedWork object") + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return wrappedErr } // If the object is already owned by another AppliedWork object, the validation fails. @@ -473,7 +500,9 @@ func validateOwnerReferences( // Normally this branch will never get executed as Fleet would refuse to take over an object // that has been owned by another AppliedWork object. if isPlacedByFleetInDuplicate(inMemberClusterObjOwnerRefs, expectedAppliedWorkOwnerRef) { - return fmt.Errorf("object is already owned by another AppliedWork object") + wrappedErr := fmt.Errorf("object is already owned by another AppliedWork object") + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return wrappedErr } return nil diff --git a/pkg/controllers/workapplier/availability_tracker.go b/pkg/controllers/workapplier/availability_tracker.go index 0c8fbfef3..22db9d43e 100644 --- a/pkg/controllers/workapplier/availability_tracker.go +++ b/pkg/controllers/workapplier/availability_tracker.go @@ -96,7 +96,9 @@ func trackDeploymentAvailability(inMemberClusterObj *unstructured.Unstructured) var deploy appv1.Deployment if err := runtime.DefaultUnstructuredConverter.FromUnstructured(inMemberClusterObj.Object, &deploy); err != nil { // Normally this branch should never run. - return ManifestProcessingAvailabilityResultTypeFailed, controller.NewUnexpectedBehaviorError(fmt.Errorf("failed to convert the unstructured object to a deployment: %w", err)) + wrappedErr := fmt.Errorf("failed to convert the unstructured object to a deployment: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return ManifestProcessingAvailabilityResultTypeFailed, wrappedErr } // Check if the deployment is available. @@ -119,7 +121,9 @@ func trackStatefulSetAvailability(inMemberClusterObj *unstructured.Unstructured) var statefulSet appv1.StatefulSet if err := runtime.DefaultUnstructuredConverter.FromUnstructured(inMemberClusterObj.Object, &statefulSet); err != nil { // Normally this branch should never run. - return ManifestProcessingAvailabilityResultTypeFailed, controller.NewUnexpectedBehaviorError(fmt.Errorf("failed to convert the unstructured object to a stateful set: %w", err)) + wrappedErr := fmt.Errorf("failed to convert the unstructured object to a stateful set: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return ManifestProcessingAvailabilityResultTypeFailed, wrappedErr } // Check if the stateful set is available. @@ -145,8 +149,10 @@ func trackStatefulSetAvailability(inMemberClusterObj *unstructured.Unstructured) func trackDaemonSetAvailability(inMemberClusterObj *unstructured.Unstructured) (ManifestProcessingAvailabilityResultType, error) { var daemonSet appv1.DaemonSet if err := runtime.DefaultUnstructuredConverter.FromUnstructured(inMemberClusterObj.Object, &daemonSet); err != nil { + wrappedErr := fmt.Errorf("failed to convert the unstructured object to a daemon set: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) // Normally this branch should never run. - return ManifestProcessingAvailabilityResultTypeFailed, controller.NewUnexpectedBehaviorError(fmt.Errorf("failed to convert the unstructured object to a daemon set: %w", err)) + return ManifestProcessingAvailabilityResultTypeFailed, wrappedErr } // Check if the daemonSet is available. @@ -168,7 +174,9 @@ func trackDaemonSetAvailability(inMemberClusterObj *unstructured.Unstructured) ( func trackServiceAvailability(inMemberClusterObj *unstructured.Unstructured) (ManifestProcessingAvailabilityResultType, error) { var svc corev1.Service if err := runtime.DefaultUnstructuredConverter.FromUnstructured(inMemberClusterObj.Object, &svc); err != nil { - return ManifestProcessingAvailabilityResultTypeFailed, controller.NewUnexpectedBehaviorError(fmt.Errorf("failed to convert the unstructured object to a service: %w", err)) + wrappedErr := fmt.Errorf("failed to convert the unstructured object to a service: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return ManifestProcessingAvailabilityResultTypeFailed, wrappedErr } switch svc.Spec.Type { case "": @@ -205,7 +213,9 @@ func trackServiceAvailability(inMemberClusterObj *unstructured.Unstructured) (Ma func trackCRDAvailability(inMemberClusterObj *unstructured.Unstructured) (ManifestProcessingAvailabilityResultType, error) { var crd apiextensionsv1.CustomResourceDefinition if err := runtime.DefaultUnstructuredConverter.FromUnstructured(inMemberClusterObj.Object, &crd); err != nil { - return ManifestProcessingAvailabilityResultTypeFailed, controller.NewUnexpectedBehaviorError(fmt.Errorf("failed to convert the unstructured object to a custom resource definition: %w", err)) + wrappedErr := fmt.Errorf("failed to convert the unstructured object to a custom resource definition: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return ManifestProcessingAvailabilityResultTypeFailed, wrappedErr } // If both conditions are True, the CRD has become available. diff --git a/pkg/controllers/workapplier/controller.go b/pkg/controllers/workapplier/controller.go index c831cee35..12268722c 100644 --- a/pkg/controllers/workapplier/controller.go +++ b/pkg/controllers/workapplier/controller.go @@ -338,12 +338,17 @@ func (r *Reconciler) garbageCollectAppliedWork(ctx context.Context, work *fleetv klog.V(2).InfoS("The appliedWork is already deleted", "appliedWork", work.Name) case err != nil: klog.ErrorS(err, "Failed to delete the appliedWork", "appliedWork", work.Name) - return ctrl.Result{}, err + return ctrl.Result{}, controller.NewAPIServerError(false, err) default: klog.InfoS("Successfully deleted the appliedWork", "appliedWork", work.Name) } controllerutil.RemoveFinalizer(work, fleetv1beta1.WorkFinalizer) - return ctrl.Result{}, r.hubClient.Update(ctx, work, &client.UpdateOptions{}) + + if err := r.hubClient.Update(ctx, work, &client.UpdateOptions{}); err != nil { + klog.ErrorS(err, "Failed to remove the finalizer from the work", "work", klog.KObj(work)) + return ctrl.Result{}, controller.NewAPIServerError(false, err) + } + return ctrl.Result{}, nil } // ensureAppliedWork makes sure that an associated appliedWork and a finalizer on the work resource exists on the cluster. @@ -377,12 +382,16 @@ func (r *Reconciler) ensureAppliedWork(ctx context.Context, work *fleetv1beta1.W } if err := r.spokeClient.Create(ctx, appliedWork); err != nil && !apierrors.IsAlreadyExists(err) { klog.ErrorS(err, "AppliedWork create failed", "appliedWork", workRef.Name) - return nil, err + return nil, controller.NewAPIServerError(false, err) } if !hasFinalizer { klog.InfoS("Add the finalizer to the work", "work", workRef) work.Finalizers = append(work.Finalizers, fleetv1beta1.WorkFinalizer) - return appliedWork, r.hubClient.Update(ctx, work, &client.UpdateOptions{}) + + if err := r.hubClient.Update(ctx, work, &client.UpdateOptions{}); err != nil { + klog.ErrorS(err, "Failed to add the finalizer to the work", "work", workRef) + return nil, controller.NewAPIServerError(false, err) + } } klog.InfoS("Recreated the appliedWork resource", "appliedWork", workRef.Name) return appliedWork, nil diff --git a/pkg/controllers/workapplier/drift_detection_takeover.go b/pkg/controllers/workapplier/drift_detection_takeover.go index cc541873f..f8a2d3c33 100644 --- a/pkg/controllers/workapplier/drift_detection_takeover.go +++ b/pkg/controllers/workapplier/drift_detection_takeover.go @@ -8,7 +8,6 @@ package workapplier import ( "context" "fmt" - "strings" "github.com/qri-io/jsonpointer" "github.com/wI2L/jsondiff" @@ -20,6 +19,7 @@ import ( "k8s.io/klog/v2" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" + "go.goms.io/fleet/pkg/utils/controller" ) const ( @@ -95,6 +95,7 @@ func (r *Reconciler) takeOverPreExistingObject( Resource(*gvr).Namespace(inMemberClusterObjCopy.GetNamespace()). Update(ctx, inMemberClusterObjCopy, metav1.UpdateOptions{}) if err != nil { + _ = controller.NewAPIServerError(false, err) return nil, nil, fmt.Errorf("failed to take over the object: %w", err) } @@ -165,12 +166,16 @@ func organizeJSONPatchIntoFleetPatchDetails(patch jsondiff.Patch, manifestObjMap pathPtr, err := jsonpointer.Parse(op.Path) if err != nil { // An invalid path is found; normally this should not happen. - return nil, fmt.Errorf("failed to parse the JSON path: %w", err) + wrappedErr := fmt.Errorf("failed to parse the JSON path %s: %w", op.Path, err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } fromPtr, err := jsonpointer.Parse(op.From) if err != nil { // An invalid path is found; normally this should not happen. - return nil, fmt.Errorf("failed to parse the JSON path: %w", err) + wrappedErr := fmt.Errorf("failed to parse the JSON path %s: %w", op.From, err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } switch op.Type { @@ -183,7 +188,9 @@ func organizeJSONPatchIntoFleetPatchDetails(patch jsondiff.Patch, manifestObjMap // Fleet here skips validation as the JSON data is just marshalled. hubValue, err := pathPtr.Eval(manifestObjMap) if err != nil { - return nil, fmt.Errorf("failed to evaluate the JSON path %s in the manifest object: %w", op.Path, err) + wrappedErr := fmt.Errorf("failed to evaluate the JSON path %s in the manifest object: %w", op.Path, err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } details = append(details, fleetv1beta1.PatchDetail{ Path: op.Path, @@ -193,7 +200,9 @@ func organizeJSONPatchIntoFleetPatchDetails(patch jsondiff.Patch, manifestObjMap // Fleet here skips validation as the JSON data is just marshalled. hubValue, err := pathPtr.Eval(manifestObjMap) if err != nil { - return nil, fmt.Errorf("failed to evaluate the JSON path %s in the manifest object: %w", op.Path, err) + wrappedErr := fmt.Errorf("failed to evaluate the JSON path %s in the manifest object: %w", op.Path, err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } details = append(details, fleetv1beta1.PatchDetail{ Path: op.Path, @@ -208,7 +217,9 @@ func organizeJSONPatchIntoFleetPatchDetails(patch jsondiff.Patch, manifestObjMap // Each Move operation will be parsed into two separate operations. hubValue, err := fromPtr.Eval(manifestObjMap) if err != nil { - return nil, fmt.Errorf("failed to evaluate the JSON path %s in the manifest object: %w", op.Path, err) + wrappedErr := fmt.Errorf("failed to evaluate the JSON path %s in the manifest object: %w", op.Path, err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } details = append(details, fleetv1beta1.PatchDetail{ Path: op.From, @@ -226,7 +237,9 @@ func organizeJSONPatchIntoFleetPatchDetails(patch jsondiff.Patch, manifestObjMap // Each Copy operation will be parsed into an Add operation. hubValue, err := fromPtr.Eval(manifestObjMap) if err != nil { - return nil, fmt.Errorf("failed to evaluate the JSON path %s in the applied object: %w", op.Path, err) + wrappedErr := fmt.Errorf("failed to evaluate the JSON path %s in the manifest object: %w", op.Path, err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } details = append(details, fleetv1beta1.PatchDetail{ Path: op.Path, @@ -236,7 +249,9 @@ func organizeJSONPatchIntoFleetPatchDetails(patch jsondiff.Patch, manifestObjMap // The Test op is a no-op in Fleet's use case. Normally it will not be returned, either. default: // An unexpected op is returned. - return nil, fmt.Errorf("an unexpected JSON patch operation is returned (%s)", op) + wrappedErr := fmt.Errorf("an unexpected JSON patch operation is returned (%s)", op) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } } @@ -253,18 +268,24 @@ func preparePatchDetails(srcObj, destObj *unstructured.Unstructured) ([]fleetv1b // Marshal the objects into JSON. srcObjJSONBytes, err := srcObjCopy.MarshalJSON() if err != nil { - return nil, fmt.Errorf("failed to marshal the source object into JSON: %w", err) + wrappedErr := fmt.Errorf("failed to marshal the source object into JSON: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } destObjJSONBytes, err := destObjCopy.MarshalJSON() if err != nil { - return nil, fmt.Errorf("failed to marshal the destination object into JSON: %w", err) + wrappedErr := fmt.Errorf("failed to marshal the destination object into JSON: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } // Compare the JSON representations. patch, err := jsondiff.CompareJSON(srcObjJSONBytes, destObjJSONBytes) if err != nil { - return nil, fmt.Errorf("failed to compare the JSON representations of the source and destination objects: %w", err) + wrappedErr := fmt.Errorf("failed to compare the JSON representations of the source and destination objects: %w", err) + _ = controller.NewUnexpectedBehaviorError(wrappedErr) + return nil, wrappedErr } details, err := organizeJSONPatchIntoFleetPatchDetails(patch, srcObjCopy.Object) @@ -291,6 +312,7 @@ func (r *Reconciler) removeLeftBehindAppliedWorkOwnerRefs(ctx context.Context, o switch { case err != nil && !errors.IsNotFound(err): // An unexpected error occurred. + _ = controller.NewAPIServerError(true, err) return nil, fmt.Errorf("failed to get the Work object: %w", err) case err == nil: // The AppliedWork owner reference is valid; no need for removal. @@ -311,98 +333,3 @@ func (r *Reconciler) removeLeftBehindAppliedWorkOwnerRefs(ctx context.Context, o return updatedOwnerRefs, nil } - -// discardFieldsIrrelevantInComparisonFrom discards fields that are irrelevant when comparing -// the manifest and live objects (or two manifest objects). -// -// Note that this method will return an object copy; the original object will be left untouched. -func discardFieldsIrrelevantInComparisonFrom(obj *unstructured.Unstructured) *unstructured.Unstructured { - // Create a deep copy of the object. - objCopy := obj.DeepCopy() - - // Remove object meta fields that are irrelevant in comparison. - - // Clear out the object's name/namespace. For regular objects, the names/namespaces will - // always be same between the manifest and the live objects, as guaranteed by the object - // retrieval step earlier, so a comparison on these fields is unnecessary anyway. - objCopy.SetName("") - objCopy.SetNamespace("") - - // Clear out the object's generate name. This is a field that is irrelevant in comparison. - objCopy.SetGenerateName("") - - // Remove certain labels and annotations. - // - // Fleet will remove labels/annotations that are reserved for Fleet own use cases, plus - // well-known Kubernetes labels and annotations, as these cannot (should not) be set by users - // directly. - annotations := objCopy.GetAnnotations() - cleanedAnnotations := map[string]string{} - for k, v := range annotations { - if strings.Contains(k, k8sReservedLabelAnnotationFullDomain) { - // Skip Kubernetes reserved annotations. - continue - } - - if strings.Contains(k, k8sReservedLabelAnnotationAbbrDomain) { - // Skip Kubernetes reserved annotations. - continue - } - - if strings.Contains(k, fleetReservedLabelAnnotationDomain) { - // Skip Fleet reserved annotations. - continue - } - cleanedAnnotations[k] = v - } - objCopy.SetAnnotations(cleanedAnnotations) - - labels := objCopy.GetLabels() - cleanedLabels := map[string]string{} - for k, v := range labels { - if strings.Contains(k, k8sReservedLabelAnnotationFullDomain) { - // Skip Kubernetes reserved labels. - continue - } - - if strings.Contains(k, k8sReservedLabelAnnotationAbbrDomain) { - // Skip Kubernetes reserved labels. - continue - } - - if strings.Contains(k, fleetReservedLabelAnnotationDomain) { - // Skip Fleet reserved labels. - continue - } - cleanedLabels[k] = v - } - objCopy.SetLabels(cleanedLabels) - - // Fields below are system-reserved fields in object meta. Technically speaking they can be - // set in the manifests, but this is a very uncommon practice, and currently Fleet will clear - // these fields (except for the finalizers) before applying the manifests]. - // As a result, for now Fleet will ignore them in the comparison process as well. - // - // TO-DO (chenyu1): evaluate if this is a correct assumption for most (if not all) Fleet - // users. - objCopy.SetFinalizers([]string{}) - objCopy.SetManagedFields([]metav1.ManagedFieldsEntry{}) - objCopy.SetOwnerReferences([]metav1.OwnerReference{}) - - // Fields below are read-only fields in object meta. Fleet will ignore them in the comparison - // process. - objCopy.SetCreationTimestamp(metav1.Time{}) - // Deleted objects are handled separately in the apply process; for comparison purposes, - // Fleet will ignore the deletion timestamp and grace period seconds. - objCopy.SetDeletionTimestamp(nil) - objCopy.SetDeletionGracePeriodSeconds(nil) - objCopy.SetGeneration(0) - objCopy.SetResourceVersion("") - objCopy.SetSelfLink("") - objCopy.SetUID("") - - // Remove the status field. - unstructured.RemoveNestedField(objCopy.Object, "status") - - return objCopy -} diff --git a/pkg/controllers/workapplier/drift_detection_takeover_test.go b/pkg/controllers/workapplier/drift_detection_takeover_test.go index 6dd5f1980..21fa5b468 100644 --- a/pkg/controllers/workapplier/drift_detection_takeover_test.go +++ b/pkg/controllers/workapplier/drift_detection_takeover_test.go @@ -7,7 +7,6 @@ package workapplier import ( "context" - "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -19,7 +18,6 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/kubernetes/scheme" @@ -456,119 +454,6 @@ func TestPreparePatchDetails(t *testing.T) { } } -// TestDiscardFieldsIrrelevantInComparisonFrom tests the discardFieldsIrrelevantInComparisonFrom function. -func TestDiscardFieldsIrrelevantInComparisonFrom(t *testing.T) { - // This test spec uses a Deployment object as the target. - generateName := "app-" - dummyManager := "dummy-manager" - now := metav1.Now() - dummyGeneration := int64(1) - dummyResourceVersion := "abc" - dummySelfLink := "self-link" - dummyUID := "123-xyz-abcd" - - deploy := &appsv1.Deployment{ - TypeMeta: metav1.TypeMeta{ - Kind: "Deployment", - APIVersion: "apps/v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: deployName, - Namespace: nsName, - GenerateName: generateName, - Annotations: map[string]string{ - fmt.Sprintf("%s/%s", k8sReservedLabelAnnotationFullDomain, dummyLabelKey): dummyLabelValue1, - fmt.Sprintf("%s/%s", k8sReservedLabelAnnotationAbbrDomain, dummyLabelKey): dummyLabelValue1, - fmt.Sprintf("%s/%s", fleetReservedLabelAnnotationDomain, dummyLabelKey): dummyLabelValue1, - dummyLabelKey: dummyLabelValue1, - }, - Labels: map[string]string{ - fmt.Sprintf("%s/%s", k8sReservedLabelAnnotationFullDomain, dummyLabelKey): dummyLabelValue1, - fmt.Sprintf("%s/%s", k8sReservedLabelAnnotationAbbrDomain, dummyLabelKey): dummyLabelValue1, - fmt.Sprintf("%s/%s", fleetReservedLabelAnnotationDomain, dummyLabelKey): dummyLabelValue1, - dummyLabelKey: dummyLabelValue2, - }, - Finalizers: []string{ - dummyLabelKey, - }, - ManagedFields: []metav1.ManagedFieldsEntry{ - { - Manager: dummyManager, - Operation: metav1.ManagedFieldsOperationUpdate, - APIVersion: "v1", - Time: &now, - FieldsType: "FieldsV1", - FieldsV1: &metav1.FieldsV1{}, - }, - }, - OwnerReferences: []metav1.OwnerReference{ - dummyOwnerRef, - }, - CreationTimestamp: now, - DeletionTimestamp: &now, - DeletionGracePeriodSeconds: ptr.To(int64(30)), - Generation: dummyGeneration, - ResourceVersion: dummyResourceVersion, - SelfLink: dummySelfLink, - UID: types.UID(dummyUID), - }, - Spec: appsv1.DeploymentSpec{ - Replicas: ptr.To(int32(1)), - }, - Status: appsv1.DeploymentStatus{ - Replicas: 1, - }, - } - wantDeploy := &appsv1.Deployment{ - TypeMeta: metav1.TypeMeta{ - Kind: "Deployment", - APIVersion: "apps/v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - dummyLabelKey: dummyLabelValue1, - }, - Labels: map[string]string{ - dummyLabelKey: dummyLabelValue2, - }, - }, - Spec: appsv1.DeploymentSpec{ - Replicas: ptr.To(int32(1)), - }, - Status: appsv1.DeploymentStatus{}, - } - - testCases := []struct { - name string - unstructuredObj *unstructured.Unstructured - wantUnstructuredObj *unstructured.Unstructured - }{ - { - name: "deploy", - unstructuredObj: toUnstructured(t, deploy), - wantUnstructuredObj: toUnstructured(t, wantDeploy), - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - got := discardFieldsIrrelevantInComparisonFrom(tc.unstructuredObj) - - // There are certain fields that need to be set manually as they might got omitted - // when being cast to an Unstructured object. - tc.wantUnstructuredObj.SetFinalizers([]string{}) - tc.wantUnstructuredObj.SetCreationTimestamp(metav1.Time{}) - tc.wantUnstructuredObj.SetManagedFields([]metav1.ManagedFieldsEntry{}) - tc.wantUnstructuredObj.SetOwnerReferences([]metav1.OwnerReference{}) - unstructured.RemoveNestedField(tc.wantUnstructuredObj.Object, "status") - - if diff := cmp.Diff(got, tc.wantUnstructuredObj, cmpopts.EquateEmpty()); diff != "" { - t.Errorf("discardFieldsIrrelevantInComparisonFrom() mismatches (-got, +want):\n%s", diff) - } - }) - } -} - // TestRemoveLeftBehindAppliedWorkOwnerRefs tests the removeLeftBehindAppliedWorkOwnerRefs method. func TestRemoveLeftBehindAppliedWorkOwnerRefs(t *testing.T) { ctx := context.Background() diff --git a/pkg/controllers/workapplier/preprocess.go b/pkg/controllers/workapplier/preprocess.go index a62bfdf78..066b3a848 100644 --- a/pkg/controllers/workapplier/preprocess.go +++ b/pkg/controllers/workapplier/preprocess.go @@ -166,6 +166,7 @@ func (r *Reconciler) writeAheadManifestProcessingAttempts( // skipped in the check above. Here Fleet simply skips the manifest. klog.ErrorS(err, "Failed to format the work resource identifier string", "ordinal", idx, "work", workRef) + _ = controller.NewUnexpectedBehaviorError(err) continue } if _, found := checked[wriStr]; found { @@ -178,7 +179,7 @@ func (r *Reconciler) writeAheadManifestProcessingAttempts( checked[wriStr] = true // Prepare the manifest conditions for the write-ahead process. - manifestCondForWA := prepareManifestCondForWA(wriStr, bundle.id, work.Generation, existingManifestCondQIdx, work.Status.ManifestConditions) + manifestCondForWA := prepareManifestCondForWriteAhead(wriStr, bundle.id, work.Generation, existingManifestCondQIdx, work.Status.ManifestConditions) manifestCondsForWA = append(manifestCondsForWA, manifestCondForWA) klog.V(2).InfoS("Prepared write-ahead information for a manifest", @@ -288,8 +289,8 @@ func prepareExistingManifestCondQIdx(existingManifestConditions []fleetv1beta1.M return existingManifestConditionQIdx } -// prepareManifestCondForWA prepares a manifest condition for the write-ahead process. -func prepareManifestCondForWA( +// prepareManifestCondForWriteAhead prepares a manifest condition for the write-ahead process. +func prepareManifestCondForWriteAhead( wriStr string, wri *fleetv1beta1.WorkResourceIdentifier, workGeneration int64, existingManifestCondQIdx map[string]int, @@ -453,6 +454,7 @@ func (r *Reconciler) removeOneLeftOverManifest( return nil case err != nil: // Failed to retrieve the object from the member cluster. + _ = controller.NewAPIServerError(true, err) return fmt.Errorf("failed to retrieve the object from the member cluster (gvr=%+v, manifestObj=%+v): %w", gvr, klog.KRef(manifestNamespace, manifestName), err) case inMemberClusterObj.GetDeletionTimestamp() != nil: // The object has been marked for deletion; no further action is needed. @@ -483,6 +485,7 @@ func (r *Reconciler) removeOneLeftOverManifest( removeOwnerRef(inMemberClusterObj, expectedAppliedWorkOwnerRef) if _, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestNamespace).Update(ctx, inMemberClusterObj, metav1.UpdateOptions{}); err != nil && !apierrors.IsNotFound(err) { // Failed to drop the ownership. + _ = controller.NewAPIServerError(false, err) return fmt.Errorf("failed to drop the ownership of the object (gvr=%+v, manifestObj=%+v, inMemberClusterObj=%+v, expectedAppliedWorkOwnerRef=%+v): %w", gvr, klog.KRef(manifestNamespace, manifestName), klog.KObj(inMemberClusterObj), *expectedAppliedWorkOwnerRef, err) } @@ -507,6 +510,7 @@ func (r *Reconciler) removeOneLeftOverManifest( } if err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestNamespace).Delete(ctx, manifestName, deleteOpts); err != nil && !apierrors.IsNotFound(err) { // Failed to delete the object from the member cluster. + _ = controller.NewAPIServerError(false, err) return fmt.Errorf("failed to delete the object (gvr=%+v, manifestObj=%+v, inMemberClusterObj=%+v, expectedAppliedWorkOwnerRef=%+v): %w", gvr, klog.KRef(manifestNamespace, manifestName), klog.KObj(inMemberClusterObj), *expectedAppliedWorkOwnerRef, err) } diff --git a/pkg/controllers/workapplier/preprocess_test.go b/pkg/controllers/workapplier/preprocess_test.go index 1728d0564..def67dcfb 100644 --- a/pkg/controllers/workapplier/preprocess_test.go +++ b/pkg/controllers/workapplier/preprocess_test.go @@ -463,7 +463,7 @@ func TestPrepareManifestCondForWA(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - manifestCondForWA := prepareManifestCondForWA(tc.wriStr, tc.wri, tc.workGeneration, tc.existingManifestCondQIdx, tc.existingManifestConds) + manifestCondForWA := prepareManifestCondForWriteAhead(tc.wriStr, tc.wri, tc.workGeneration, tc.existingManifestCondQIdx, tc.existingManifestConds) if diff := cmp.Diff(&manifestCondForWA, tc.wantManifestCondForWA, ignoreFieldConditionLTTMsg); diff != "" { t.Errorf("prepareManifestCondForWA() mismatches (-got +want):\n%s", diff) } diff --git a/pkg/controllers/workapplier/process.go b/pkg/controllers/workapplier/process.go index f63dd3b94..64cdde036 100644 --- a/pkg/controllers/workapplier/process.go +++ b/pkg/controllers/workapplier/process.go @@ -173,6 +173,7 @@ func (r *Reconciler) findInMemberClusterObjectFor( return false default: // An unexpected error has occurred. + _ = controller.NewAPIServerError(true, err) bundle.applyErr = fmt.Errorf("failed to find the corresponding object for the manifest object in the member cluster: %w", err) bundle.applyResTyp = ManifestProcessingApplyResultTypeFailedToFindObjInMemberCluster klog.ErrorS(err, diff --git a/pkg/controllers/workapplier/utils.go b/pkg/controllers/workapplier/utils.go index d03a8b3d3..b8829b2cc 100644 --- a/pkg/controllers/workapplier/utils.go +++ b/pkg/controllers/workapplier/utils.go @@ -7,9 +7,11 @@ package workapplier import ( "fmt" + "strings" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" "go.goms.io/fleet/pkg/utils/condition" @@ -55,3 +57,98 @@ func isPlacedByFleetInDuplicate(ownerRefs []metav1.OwnerReference, expectedAppli } return false } + +// discardFieldsIrrelevantInComparisonFrom discards fields that are irrelevant when comparing +// the manifest and live objects (or two manifest objects). +// +// Note that this method will return an object copy; the original object will be left untouched. +func discardFieldsIrrelevantInComparisonFrom(obj *unstructured.Unstructured) *unstructured.Unstructured { + // Create a deep copy of the object. + objCopy := obj.DeepCopy() + + // Remove object meta fields that are irrelevant in comparison. + + // Clear out the object's name/namespace. For regular objects, the names/namespaces will + // always be same between the manifest and the live objects, as guaranteed by the object + // retrieval step earlier, so a comparison on these fields is unnecessary anyway. + objCopy.SetName("") + objCopy.SetNamespace("") + + // Clear out the object's generate name. This is a field that is irrelevant in comparison. + objCopy.SetGenerateName("") + + // Remove certain labels and annotations. + // + // Fleet will remove labels/annotations that are reserved for Fleet own use cases, plus + // well-known Kubernetes labels and annotations, as these cannot (should not) be set by users + // directly. + annotations := objCopy.GetAnnotations() + cleanedAnnotations := map[string]string{} + for k, v := range annotations { + if strings.Contains(k, k8sReservedLabelAnnotationFullDomain) { + // Skip Kubernetes reserved annotations. + continue + } + + if strings.Contains(k, k8sReservedLabelAnnotationAbbrDomain) { + // Skip Kubernetes reserved annotations. + continue + } + + if strings.Contains(k, fleetReservedLabelAnnotationDomain) { + // Skip Fleet reserved annotations. + continue + } + cleanedAnnotations[k] = v + } + objCopy.SetAnnotations(cleanedAnnotations) + + labels := objCopy.GetLabels() + cleanedLabels := map[string]string{} + for k, v := range labels { + if strings.Contains(k, k8sReservedLabelAnnotationFullDomain) { + // Skip Kubernetes reserved labels. + continue + } + + if strings.Contains(k, k8sReservedLabelAnnotationAbbrDomain) { + // Skip Kubernetes reserved labels. + continue + } + + if strings.Contains(k, fleetReservedLabelAnnotationDomain) { + // Skip Fleet reserved labels. + continue + } + cleanedLabels[k] = v + } + objCopy.SetLabels(cleanedLabels) + + // Fields below are system-reserved fields in object meta. Technically speaking they can be + // set in the manifests, but this is a very uncommon practice, and currently Fleet will clear + // these fields (except for the finalizers) before applying the manifests. + // As a result, for now Fleet will ignore them in the comparison process as well. + // + // TO-DO (chenyu1): evaluate if this is a correct assumption for most (if not all) Fleet + // users. + objCopy.SetFinalizers([]string{}) + objCopy.SetManagedFields([]metav1.ManagedFieldsEntry{}) + objCopy.SetOwnerReferences([]metav1.OwnerReference{}) + + // Fields below are read-only fields in object meta. Fleet will ignore them in the comparison + // process. + objCopy.SetCreationTimestamp(metav1.Time{}) + // Deleted objects are handled separately in the apply process; for comparison purposes, + // Fleet will ignore the deletion timestamp and grace period seconds. + objCopy.SetDeletionTimestamp(nil) + objCopy.SetDeletionGracePeriodSeconds(nil) + objCopy.SetGeneration(0) + objCopy.SetResourceVersion("") + objCopy.SetSelfLink("") + objCopy.SetUID("") + + // Remove the status field. + unstructured.RemoveNestedField(objCopy.Object, "status") + + return objCopy +} diff --git a/pkg/controllers/workapplier/utils_test.go b/pkg/controllers/workapplier/utils_test.go index af285f27f..f9bd76e0f 100644 --- a/pkg/controllers/workapplier/utils_test.go +++ b/pkg/controllers/workapplier/utils_test.go @@ -9,6 +9,14 @@ import ( "fmt" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + appsv1 "k8s.io/api/apps/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" ) @@ -72,3 +80,116 @@ func TestIsPlacedByFleetInDuplicate(t *testing.T) { }) } } + +// TestDiscardFieldsIrrelevantInComparisonFrom tests the discardFieldsIrrelevantInComparisonFrom function. +func TestDiscardFieldsIrrelevantInComparisonFrom(t *testing.T) { + // This test spec uses a Deployment object as the target. + generateName := "app-" + dummyManager := "dummy-manager" + now := metav1.Now() + dummyGeneration := int64(1) + dummyResourceVersion := "abc" + dummySelfLink := "self-link" + dummyUID := "123-xyz-abcd" + + deploy := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: deployName, + Namespace: nsName, + GenerateName: generateName, + Annotations: map[string]string{ + fmt.Sprintf("%s/%s", k8sReservedLabelAnnotationFullDomain, dummyLabelKey): dummyLabelValue1, + fmt.Sprintf("%s/%s", k8sReservedLabelAnnotationAbbrDomain, dummyLabelKey): dummyLabelValue1, + fmt.Sprintf("%s/%s", fleetReservedLabelAnnotationDomain, dummyLabelKey): dummyLabelValue1, + dummyLabelKey: dummyLabelValue1, + }, + Labels: map[string]string{ + fmt.Sprintf("%s/%s", k8sReservedLabelAnnotationFullDomain, dummyLabelKey): dummyLabelValue1, + fmt.Sprintf("%s/%s", k8sReservedLabelAnnotationAbbrDomain, dummyLabelKey): dummyLabelValue1, + fmt.Sprintf("%s/%s", fleetReservedLabelAnnotationDomain, dummyLabelKey): dummyLabelValue1, + dummyLabelKey: dummyLabelValue2, + }, + Finalizers: []string{ + dummyLabelKey, + }, + ManagedFields: []metav1.ManagedFieldsEntry{ + { + Manager: dummyManager, + Operation: metav1.ManagedFieldsOperationUpdate, + APIVersion: "v1", + Time: &now, + FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{}, + }, + }, + OwnerReferences: []metav1.OwnerReference{ + dummyOwnerRef, + }, + CreationTimestamp: now, + DeletionTimestamp: &now, + DeletionGracePeriodSeconds: ptr.To(int64(30)), + Generation: dummyGeneration, + ResourceVersion: dummyResourceVersion, + SelfLink: dummySelfLink, + UID: types.UID(dummyUID), + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To(int32(1)), + }, + Status: appsv1.DeploymentStatus{ + Replicas: 1, + }, + } + wantDeploy := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + dummyLabelKey: dummyLabelValue1, + }, + Labels: map[string]string{ + dummyLabelKey: dummyLabelValue2, + }, + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To(int32(1)), + }, + Status: appsv1.DeploymentStatus{}, + } + + testCases := []struct { + name string + unstructuredObj *unstructured.Unstructured + wantUnstructuredObj *unstructured.Unstructured + }{ + { + name: "deploy", + unstructuredObj: toUnstructured(t, deploy), + wantUnstructuredObj: toUnstructured(t, wantDeploy), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := discardFieldsIrrelevantInComparisonFrom(tc.unstructuredObj) + + // There are certain fields that need to be set manually as they might got omitted + // when being cast to an Unstructured object. + tc.wantUnstructuredObj.SetFinalizers([]string{}) + tc.wantUnstructuredObj.SetCreationTimestamp(metav1.Time{}) + tc.wantUnstructuredObj.SetManagedFields([]metav1.ManagedFieldsEntry{}) + tc.wantUnstructuredObj.SetOwnerReferences([]metav1.OwnerReference{}) + unstructured.RemoveNestedField(tc.wantUnstructuredObj.Object, "status") + + if diff := cmp.Diff(got, tc.wantUnstructuredObj, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("discardFieldsIrrelevantInComparisonFrom() mismatches (-got, +want):\n%s", diff) + } + }) + } +} From d0a5bc70dcd1d1c640bd77e478ca9c71ecde6ecd Mon Sep 17 00:00:00 2001 From: michaelawyu Date: Thu, 9 Jan 2025 15:59:55 +0800 Subject: [PATCH 2/3] Minor fixes --- pkg/controllers/workapplier/apply_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/workapplier/apply_test.go b/pkg/controllers/workapplier/apply_test.go index ac963e0a6..ed5d7329a 100644 --- a/pkg/controllers/workapplier/apply_test.go +++ b/pkg/controllers/workapplier/apply_test.go @@ -203,7 +203,7 @@ func TestValidateOwnerReferences(t *testing.T) { AllowCoOwnership: false, }, wantErred: true, - wantErrMsgSubStr: "manifest is set to have multiple owner references but co-ownership is disallowed", + wantErrMsgSubStr: "manifest is set to have owner references but co-ownership is disallowed", }, { name: "unexpected AppliedWork owner ref", From 7af1d2f3b103810502ca7953d2a757405964adae Mon Sep 17 00:00:00 2001 From: michaelawyu Date: Wed, 15 Jan 2025 21:04:37 +0800 Subject: [PATCH 3/3] Minor fixes --- pkg/controllers/workapplier/apply.go | 12 ++++++------ .../workapplier/drift_detection_takeover.go | 8 ++++---- pkg/controllers/workapplier/preprocess.go | 12 ++++++------ pkg/controllers/workapplier/process.go | 6 +++--- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/controllers/workapplier/apply.go b/pkg/controllers/workapplier/apply.go index 5fa4a6c88..bfe92ea9d 100644 --- a/pkg/controllers/workapplier/apply.go +++ b/pkg/controllers/workapplier/apply.go @@ -181,8 +181,8 @@ func (r *Reconciler) createManifestObject( } createdObj, err := r.spokeDynamicClient.Resource(*gvr).Namespace(manifestObject.GetNamespace()).Create(ctx, manifestObject, createOpts) if err != nil { - _ = controller.NewAPIServerError(false, err) - return nil, fmt.Errorf("failed to create manifest object: %w", err) + wrappedErr := controller.NewAPIServerError(false, err) + return nil, fmt.Errorf("failed to create manifest object: %w", wrappedErr) } return createdObj, nil } @@ -240,8 +240,8 @@ func (r *Reconciler) threeWayMergePatch( Resource(*gvr).Namespace(manifestObj.GetNamespace()). Patch(ctx, manifestObj.GetName(), patch.Type(), data, patchOpts) if err != nil { - _ = controller.NewAPIServerError(false, err) - return nil, fmt.Errorf("failed to patch the manifest object: %w", err) + wrappedErr := controller.NewAPIServerError(false, err) + return nil, fmt.Errorf("failed to patch the manifest object: %w", wrappedErr) } return patchedObj, nil } @@ -282,8 +282,8 @@ func (r *Reconciler) serverSideApply( Resource(*gvr).Namespace(manifestObj.GetNamespace()). Apply(ctx, manifestObj.GetName(), manifestObj, applyOpts) if err != nil { - _ = controller.NewAPIServerError(false, err) - return nil, fmt.Errorf("failed to apply the manifest object: %w", err) + wrappedErr := controller.NewAPIServerError(false, err) + return nil, fmt.Errorf("failed to apply the manifest object: %w", wrappedErr) } return appliedObj, nil } diff --git a/pkg/controllers/workapplier/drift_detection_takeover.go b/pkg/controllers/workapplier/drift_detection_takeover.go index f8a2d3c33..b02cddcf9 100644 --- a/pkg/controllers/workapplier/drift_detection_takeover.go +++ b/pkg/controllers/workapplier/drift_detection_takeover.go @@ -95,8 +95,8 @@ func (r *Reconciler) takeOverPreExistingObject( Resource(*gvr).Namespace(inMemberClusterObjCopy.GetNamespace()). Update(ctx, inMemberClusterObjCopy, metav1.UpdateOptions{}) if err != nil { - _ = controller.NewAPIServerError(false, err) - return nil, nil, fmt.Errorf("failed to take over the object: %w", err) + wrappedErr := controller.NewAPIServerError(false, err) + return nil, nil, fmt.Errorf("failed to take over the object: %w", wrappedErr) } return takenOverInMemberClusterObj, nil, nil @@ -312,8 +312,8 @@ func (r *Reconciler) removeLeftBehindAppliedWorkOwnerRefs(ctx context.Context, o switch { case err != nil && !errors.IsNotFound(err): // An unexpected error occurred. - _ = controller.NewAPIServerError(true, err) - return nil, fmt.Errorf("failed to get the Work object: %w", err) + wrappedErr := controller.NewAPIServerError(true, err) + return nil, fmt.Errorf("failed to get the Work object: %w", wrappedErr) case err == nil: // The AppliedWork owner reference is valid; no need for removal. // diff --git a/pkg/controllers/workapplier/preprocess.go b/pkg/controllers/workapplier/preprocess.go index 066b3a848..f02b31511 100644 --- a/pkg/controllers/workapplier/preprocess.go +++ b/pkg/controllers/workapplier/preprocess.go @@ -454,8 +454,8 @@ func (r *Reconciler) removeOneLeftOverManifest( return nil case err != nil: // Failed to retrieve the object from the member cluster. - _ = controller.NewAPIServerError(true, err) - return fmt.Errorf("failed to retrieve the object from the member cluster (gvr=%+v, manifestObj=%+v): %w", gvr, klog.KRef(manifestNamespace, manifestName), err) + wrappedErr := controller.NewAPIServerError(true, err) + return fmt.Errorf("failed to retrieve the object from the member cluster (gvr=%+v, manifestObj=%+v): %w", gvr, klog.KRef(manifestNamespace, manifestName), wrappedErr) case inMemberClusterObj.GetDeletionTimestamp() != nil: // The object has been marked for deletion; no further action is needed. return nil @@ -485,9 +485,9 @@ func (r *Reconciler) removeOneLeftOverManifest( removeOwnerRef(inMemberClusterObj, expectedAppliedWorkOwnerRef) if _, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestNamespace).Update(ctx, inMemberClusterObj, metav1.UpdateOptions{}); err != nil && !apierrors.IsNotFound(err) { // Failed to drop the ownership. - _ = controller.NewAPIServerError(false, err) + wrappedErr := controller.NewAPIServerError(false, err) return fmt.Errorf("failed to drop the ownership of the object (gvr=%+v, manifestObj=%+v, inMemberClusterObj=%+v, expectedAppliedWorkOwnerRef=%+v): %w", - gvr, klog.KRef(manifestNamespace, manifestName), klog.KObj(inMemberClusterObj), *expectedAppliedWorkOwnerRef, err) + gvr, klog.KRef(manifestNamespace, manifestName), klog.KObj(inMemberClusterObj), *expectedAppliedWorkOwnerRef, wrappedErr) } default: // Fleet is the sole owner of the object; in this case, Fleet will delete the object. @@ -510,9 +510,9 @@ func (r *Reconciler) removeOneLeftOverManifest( } if err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestNamespace).Delete(ctx, manifestName, deleteOpts); err != nil && !apierrors.IsNotFound(err) { // Failed to delete the object from the member cluster. - _ = controller.NewAPIServerError(false, err) + wrappedErr := controller.NewAPIServerError(false, err) return fmt.Errorf("failed to delete the object (gvr=%+v, manifestObj=%+v, inMemberClusterObj=%+v, expectedAppliedWorkOwnerRef=%+v): %w", - gvr, klog.KRef(manifestNamespace, manifestName), klog.KObj(inMemberClusterObj), *expectedAppliedWorkOwnerRef, err) + gvr, klog.KRef(manifestNamespace, manifestName), klog.KObj(inMemberClusterObj), *expectedAppliedWorkOwnerRef, wrappedErr) } } return nil diff --git a/pkg/controllers/workapplier/process.go b/pkg/controllers/workapplier/process.go index 64cdde036..44a154cd2 100644 --- a/pkg/controllers/workapplier/process.go +++ b/pkg/controllers/workapplier/process.go @@ -173,10 +173,10 @@ func (r *Reconciler) findInMemberClusterObjectFor( return false default: // An unexpected error has occurred. - _ = controller.NewAPIServerError(true, err) - bundle.applyErr = fmt.Errorf("failed to find the corresponding object for the manifest object in the member cluster: %w", err) + wrappedErr := controller.NewAPIServerError(true, err) + bundle.applyErr = fmt.Errorf("failed to find the corresponding object for the manifest object in the member cluster: %w", wrappedErr) bundle.applyResTyp = ManifestProcessingApplyResultTypeFailedToFindObjInMemberCluster - klog.ErrorS(err, + klog.ErrorS(wrappedErr, "Failed to find the corresponding object for the manifest object in the member cluster", "work", klog.KObj(work), "GVR", *bundle.gvr, "manifestObj", klog.KObj(bundle.manifestObj), "expectedAppliedWorkOwnerRef", *expectedAppliedWorkOwnerRef)