diff --git a/cmd/memberagent/main.go b/cmd/memberagent/main.go index eca9da748..32c979227 100644 --- a/cmd/memberagent/main.go +++ b/cmd/memberagent/main.go @@ -91,6 +91,7 @@ var ( driftDetectionInterval = flag.Int("drift-detection-interval", 15, "The interval in seconds between attempts to detect configuration drifts in the cluster.") watchWorkWithPriorityQueue = flag.Bool("enable-watch-work-with-priority-queue", false, "If set, the apply_work controller will watch/reconcile work objects that are created new or have recent updates") watchWorkReconcileAgeMinutes = flag.Int("watch-work-reconcile-age", 60, "maximum age (in minutes) of work objects for apply_work controller to watch/reconcile") + deletionWaitTime = flag.Int("deletion-wait-time", 5, "The time the work-applier will wait for work object to be deleted before updating the applied work owner reference") enablePprof = flag.Bool("enable-pprof", false, "enable pprof profiling") pprofPort = flag.Int("pprof-port", 6065, "port for pprof profiling") hubPprofPort = flag.Int("hub-pprof-port", 6066, "port for hub pprof profiling") @@ -395,6 +396,7 @@ func Start(ctx context.Context, hubCfg, memberConfig *rest.Config, hubOpts, memb parallelizer.DefaultNumOfWorkers, time.Second*time.Duration(*availabilityCheckInterval), time.Second*time.Duration(*driftDetectionInterval), + time.Minute*time.Duration(*deletionWaitTime), *watchWorkWithPriorityQueue, *watchWorkReconcileAgeMinutes, ) diff --git a/pkg/controllers/internalmembercluster/v1beta1/member_suite_test.go b/pkg/controllers/internalmembercluster/v1beta1/member_suite_test.go index bb5ed113b..a2f7c94c2 100644 --- a/pkg/controllers/internalmembercluster/v1beta1/member_suite_test.go +++ b/pkg/controllers/internalmembercluster/v1beta1/member_suite_test.go @@ -379,7 +379,7 @@ var _ = BeforeSuite(func() { // This controller is created for testing purposes only; no reconciliation loop is actually // run. - workApplier1 = workapplier.NewReconciler(hubClient, member1ReservedNSName, nil, nil, nil, nil, 0, 1, time.Second*5, time.Second*5, true, 60) + workApplier1 = workapplier.NewReconciler(hubClient, member1ReservedNSName, nil, nil, nil, nil, 0, 1, time.Second*5, time.Second*5, time.Minute, true, 60) propertyProvider1 = &manuallyUpdatedProvider{} member1Reconciler, err := NewReconciler(ctx, hubClient, member1Cfg, member1Client, workApplier1, propertyProvider1) @@ -402,7 +402,7 @@ var _ = BeforeSuite(func() { // This controller is created for testing purposes only; no reconciliation loop is actually // run. - workApplier2 = workapplier.NewReconciler(hubClient, member2ReservedNSName, nil, nil, nil, nil, 0, 1, time.Second*5, time.Second*5, true, 60) + workApplier2 = workapplier.NewReconciler(hubClient, member2ReservedNSName, nil, nil, nil, nil, 0, 1, time.Second*5, time.Second*5, time.Minute, true, 60) member2Reconciler, err := NewReconciler(ctx, hubClient, member2Cfg, member2Client, workApplier2, nil) Expect(err).NotTo(HaveOccurred()) diff --git a/pkg/controllers/workapplier/apply.go b/pkg/controllers/workapplier/apply.go index 95bfb6d25..45895d2b0 100644 --- a/pkg/controllers/workapplier/apply.go +++ b/pkg/controllers/workapplier/apply.go @@ -19,7 +19,6 @@ package workapplier import ( "context" "fmt" - "reflect" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/validation" @@ -518,7 +517,7 @@ func validateOwnerReferences( // expected AppliedWork object. For safety reasons, Fleet will still do a sanity check. found := false for _, ownerRef := range inMemberClusterObjOwnerRefs { - if reflect.DeepEqual(ownerRef, *expectedAppliedWorkOwnerRef) { + if areOwnerRefsEqual(&ownerRef, expectedAppliedWorkOwnerRef) { found = true break } diff --git a/pkg/controllers/workapplier/controller.go b/pkg/controllers/workapplier/controller.go index aca1e73e3..227c25e68 100644 --- a/pkg/controllers/workapplier/controller.go +++ b/pkg/controllers/workapplier/controller.go @@ -1,19 +1,3 @@ -/* -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. -*/ - /* Copyright 2025 The KubeFleet Authors. @@ -34,6 +18,7 @@ package workapplier import ( "context" + "fmt" "time" "go.uber.org/atomic" @@ -212,6 +197,7 @@ type Reconciler struct { availabilityCheckRequeueAfter time.Duration driftCheckRequeueAfter time.Duration + deletionWaitTime time.Duration } func NewReconciler( @@ -222,6 +208,7 @@ func NewReconciler( workerCount int, availabilityCheckRequestAfter time.Duration, driftCheckRequestAfter time.Duration, + deletionWaitTime time.Duration, watchWorkWithPriorityQueue bool, watchWorkReconcileAgeMinutes int, ) *Reconciler { @@ -251,6 +238,7 @@ func NewReconciler( joined: atomic.NewBool(false), availabilityCheckRequeueAfter: acRequestAfter, driftCheckRequeueAfter: dcRequestAfter, + deletionWaitTime: deletionWaitTime, } } @@ -417,7 +405,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu Kind: fleetv1beta1.AppliedWorkKind, Name: appliedWork.GetName(), UID: appliedWork.GetUID(), - BlockOwnerDeletion: ptr.To(false), + BlockOwnerDeletion: ptr.To(true), } // Set the default values for the Work object to avoid additional validation logic in the @@ -485,29 +473,112 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{RequeueAfter: r.driftCheckRequeueAfter}, nil } -// garbageCollectAppliedWork deletes the appliedWork and all the manifests associated with it from the cluster. func (r *Reconciler) garbageCollectAppliedWork(ctx context.Context, work *fleetv1beta1.Work) (ctrl.Result, error) { - deletePolicy := metav1.DeletePropagationBackground + deletePolicy := metav1.DeletePropagationForeground if !controllerutil.ContainsFinalizer(work, fleetv1beta1.WorkFinalizer) { return ctrl.Result{}, nil } - // delete the appliedWork which will remove all the manifests associated with it - // TODO: allow orphaned manifest - appliedWork := fleetv1beta1.AppliedWork{ + appliedWork := &fleetv1beta1.AppliedWork{ ObjectMeta: metav1.ObjectMeta{Name: work.Name}, } - err := r.spokeClient.Delete(ctx, &appliedWork, &client.DeleteOptions{PropagationPolicy: &deletePolicy}) - switch { - case apierrors.IsNotFound(err): - 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) + // Get the AppliedWork object + if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: work.Name}, appliedWork); err != nil { + if apierrors.IsNotFound(err) { + klog.V(2).InfoS("The appliedWork is already deleted, removing the finalizer from the work", "appliedWork", work.Name) + return r.removeWorkFinalizer(ctx, work) + } + klog.ErrorS(err, "Failed to get AppliedWork", "appliedWork", work.Name) return ctrl.Result{}, controller.NewAPIServerError(false, err) - default: - klog.InfoS("Successfully deleted the appliedWork", "appliedWork", work.Name) } - controllerutil.RemoveFinalizer(work, fleetv1beta1.WorkFinalizer) + // Handle stuck deletion after 5 minutes where the other owner references might not exist or are invalid. + if !appliedWork.DeletionTimestamp.IsZero() && time.Since(appliedWork.DeletionTimestamp.Time) >= r.deletionWaitTime { + klog.V(2).InfoS("AppliedWork deletion appears stuck; attempting to patch owner references", "appliedWork", work.Name) + if err := r.updateOwnerReference(ctx, work, appliedWork); err != nil { + klog.ErrorS(err, "Failed to update owner references for AppliedWork", "appliedWork", work.Name) + return ctrl.Result{}, controller.NewAPIServerError(false, err) + } + return ctrl.Result{}, fmt.Errorf("AppliedWork %s is being deleted, waiting for the deletion to complete", work.Name) + } + + if err := r.spokeClient.Delete(ctx, appliedWork, &client.DeleteOptions{PropagationPolicy: &deletePolicy}); err != nil { + if apierrors.IsNotFound(err) { + klog.V(2).InfoS("AppliedWork already deleted", "appliedWork", work.Name) + return r.removeWorkFinalizer(ctx, work) + } + klog.V(2).ErrorS(err, "Failed to delete the appliedWork", "appliedWork", work.Name) + return ctrl.Result{}, controller.NewAPIServerError(false, err) + } + + klog.V(2).InfoS("AppliedWork deletion in progress", "appliedWork", work.Name) + return ctrl.Result{}, fmt.Errorf("AppliedWork %s is being deleted, waiting for the deletion to complete", work.Name) +} + +// updateOwnerReference updates the AppliedWork owner reference in the manifest objects. +// It changes the blockOwnerDeletion field to false, so that the AppliedWork can be deleted in cases where +// the other owner references do not exist or are invalid. +// https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/#owner-references-in-object-specifications +func (r *Reconciler) updateOwnerReference(ctx context.Context, work *fleetv1beta1.Work, appliedWork *fleetv1beta1.AppliedWork) error { + appliedWorkOwnerRef := &metav1.OwnerReference{ + APIVersion: fleetv1beta1.GroupVersion.String(), + Kind: "AppliedWork", + Name: appliedWork.Name, + UID: appliedWork.UID, + } + + if err := r.hubClient.Get(ctx, types.NamespacedName{Name: work.Name, Namespace: work.Namespace}, work); err != nil { + if apierrors.IsNotFound(err) { + klog.V(2).InfoS("Work object not found, skipping owner reference update", "work", work.Name, "namespace", work.Namespace) + return nil + } + klog.ErrorS(err, "Failed to get Work object for owner reference update", "work", work.Name, "namespace", work.Namespace) + return controller.NewAPIServerError(false, err) + } + + for _, cond := range work.Status.ManifestConditions { + res := cond.Identifier + gvr := schema.GroupVersionResource{ + Group: res.Group, + Version: res.Version, + Resource: res.Resource, + } + + var obj *unstructured.Unstructured + var err error + if obj, err = r.spokeDynamicClient.Resource(gvr).Namespace(res.Namespace).Get(ctx, res.Name, metav1.GetOptions{}); err != nil { + if apierrors.IsNotFound(err) { + continue + } + klog.ErrorS(err, "Failed to get manifest", "gvr", gvr, "name", res.Name, "namespace", res.Namespace) + return err + } + // Check if there is more than one owner reference. If there is only one owner reference, it is the appliedWork itself. + // Otherwise, at least one other owner reference exists, and we need to leave resource alone. + if len(obj.GetOwnerReferences()) > 1 { + ownerRefs := obj.GetOwnerReferences() + updated := false + for idx := range ownerRefs { + if areOwnerRefsEqual(&ownerRefs[idx], appliedWorkOwnerRef) { + ownerRefs[idx].BlockOwnerDeletion = ptr.To(false) + updated = true + } + } + if updated { + obj.SetOwnerReferences(ownerRefs) + if _, err = r.spokeDynamicClient.Resource(gvr).Namespace(obj.GetNamespace()).Update(ctx, obj, metav1.UpdateOptions{}); err != nil { + klog.ErrorS(err, "Failed to update manifest owner references", "gvr", gvr, "name", res.Name, "namespace", res.Namespace) + return err + } + klog.V(4).InfoS("Patched manifest owner references", "gvr", gvr, "name", res.Name, "namespace", res.Namespace) + } + } + } + return nil +} + +// removeWorkFinalizer removes the finalizer from the work and updates it in the hub. +func (r *Reconciler) removeWorkFinalizer(ctx context.Context, work *fleetv1beta1.Work) (ctrl.Result, error) { + controllerutil.RemoveFinalizer(work, fleetv1beta1.WorkFinalizer) 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) diff --git a/pkg/controllers/workapplier/controller_integration_test.go b/pkg/controllers/workapplier/controller_integration_test.go index 776302d78..1cf36c982 100644 --- a/pkg/controllers/workapplier/controller_integration_test.go +++ b/pkg/controllers/workapplier/controller_integration_test.go @@ -26,6 +26,7 @@ import ( . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -178,7 +179,7 @@ func prepareAppliedWorkOwnerRef(workName string) *metav1.OwnerReference { Kind: "AppliedWork", Name: appliedWork.Name, UID: appliedWork.GetUID(), - BlockOwnerDeletion: ptr.To(false), + BlockOwnerDeletion: ptr.To(true), } } @@ -277,6 +278,39 @@ func regularDeploymentObjectAppliedActual(nsName, deployName string, appliedWork } } +func regularClusterRoleObjectAppliedActual(clusterRoleName string, appliedWorkOwnerRef *metav1.OwnerReference) func() error { + return func() error { + // Retrieve the ClusterRole object. + gotClusterRole := &rbacv1.ClusterRole{} + if err := memberClient.Get(ctx, client.ObjectKey{Name: clusterRoleName}, gotClusterRole); err != nil { + return fmt.Errorf("failed to retrieve the ClusterRole object: %w", err) + } + + // Check that the ClusterRole object has been created as expected. + + // To ignore default values automatically, here the test suite rebuilds the objects. + wantClusterRole := clusterRole.DeepCopy() + wantClusterRole.TypeMeta = metav1.TypeMeta{} + wantClusterRole.Name = clusterRoleName + wantClusterRole.OwnerReferences = []metav1.OwnerReference{ + *appliedWorkOwnerRef, + } + + rebuiltGotClusterRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: gotClusterRole.Name, + OwnerReferences: gotClusterRole.OwnerReferences, + }, + Rules: gotClusterRole.Rules, + } + + if diff := cmp.Diff(rebuiltGotClusterRole, wantClusterRole); diff != "" { + return fmt.Errorf("clusterRole diff (-got +want):\n%s", diff) + } + return nil + } +} + func regularConfigMapObjectAppliedActual(nsName, configMapName string, appliedWorkOwnerRef *metav1.OwnerReference) func() error { return func() error { // Retrieve the ConfigMap object. @@ -437,7 +471,22 @@ func appliedWorkStatusUpdated(workName string, appliedResourceMeta []fleetv1beta } } -func cleanupWorkObject(workName string) { +func workRemovedActual(workName string) func() error { + // Wait for the removal of the Work object. + return func() error { + work := &fleetv1beta1.Work{} + if err := hubClient.Get(ctx, client.ObjectKey{Name: workName, Namespace: memberReservedNSName}, work); !errors.IsNotFound(err) && err != nil { + return fmt.Errorf("work object still exists or an unexpected error occurred: %w", err) + } + if controllerutil.ContainsFinalizer(work, fleetv1beta1.WorkFinalizer) { + // The Work object is being deleted, but the finalizer is still present. + return fmt.Errorf("work object is being deleted, but the finalizer is still present") + } + return nil + } +} + +func deleteWorkObject(workName string) { // Retrieve the Work object. work := &fleetv1beta1.Work{ ObjectMeta: metav1.ObjectMeta{ @@ -446,26 +495,47 @@ func cleanupWorkObject(workName string) { }, } Expect(hubClient.Delete(ctx, work)).To(Succeed(), "Failed to delete the Work object") +} - // Wait for the removal of the Work object. - workRemovedActual := func() error { - work := &fleetv1beta1.Work{} - if err := hubClient.Get(ctx, client.ObjectKey{Name: workName, Namespace: memberReservedNSName}, work); !errors.IsNotFound(err) { - return fmt.Errorf("work object still exists or an unexpected error occurred: %w", err) - } - return nil +func checkNSOwnerReferences(workName, nsName string) { + // Retrieve the AppliedWork object. + appliedWork := &fleetv1beta1.AppliedWork{} + Expect(memberClient.Get(ctx, client.ObjectKey{Name: workName}, appliedWork)).To(Succeed(), "Failed to retrieve the AppliedWork object") + + // Check that the Namespace object has the AppliedWork as an owner reference. + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: nsName, + }, } - Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + Expect(memberClient.Get(ctx, client.ObjectKey{Name: nsName}, ns)).To(Succeed(), "Failed to retrieve the Namespace object") + Expect(ns.OwnerReferences).To(ContainElement(metav1.OwnerReference{ + APIVersion: fleetv1beta1.GroupVersion.String(), + Kind: "AppliedWork", + Name: appliedWork.Name, + UID: appliedWork.GetUID(), + BlockOwnerDeletion: ptr.To(true), + }), " AppliedWork OwnerReference not found in Namespace object") } -func appliedWorkRemovedActual(workName string) func() error { +func appliedWorkRemovedActual(workName, nsName string) func() error { return func() error { // Retrieve the AppliedWork object. appliedWork := &fleetv1beta1.AppliedWork{} - if err := memberClient.Get(ctx, client.ObjectKey{Name: workName}, appliedWork); !errors.IsNotFound(err) { - return fmt.Errorf("appliedWork object still exists or an unexpected error occurred: %w", err) + if err := memberClient.Get(ctx, client.ObjectKey{Name: workName}, appliedWork); err != nil { + if errors.IsNotFound(err) { + // The AppliedWork object has been deleted, which is expected. + return nil + } + return fmt.Errorf("failed to retrieve the AppliedWork object: %w", err) } - return nil + if !appliedWork.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(appliedWork, metav1.FinalizerDeleteDependents) { + // The AppliedWork object is being deleted, but the finalizer is still present. Remove the finalizer as there + // are no real built-in controllers in this test environment to handle garbage collection. + controllerutil.RemoveFinalizer(appliedWork, metav1.FinalizerDeleteDependents) + Expect(memberClient.Update(ctx, appliedWork)).To(Succeed(), "Failed to remove the finalizer from the AppliedWork object") + } + return fmt.Errorf("appliedWork object still exists") } } @@ -489,6 +559,25 @@ func regularDeployRemovedActual(nsName, deployName string) func() error { } } +func regularClusterRoleRemovedActual(clusterRoleName string) func() error { + return func() error { + // Retrieve the ClusterRole object. + clusterRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterRoleName, + }, + } + if err := memberClient.Delete(ctx, clusterRole); err != nil && !errors.IsNotFound(err) { + return fmt.Errorf("failed to delete the ClusterRole object: %w", err) + } + + if err := memberClient.Get(ctx, client.ObjectKey{Name: clusterRoleName}, clusterRole); !errors.IsNotFound(err) { + return fmt.Errorf("clusterRole object still exists or an unexpected error occurred: %w", err) + } + return nil + } +} + func regularConfigMapRemovedActual(nsName, configMapName string) func() error { return func() error { // Retrieve the ConfigMap object. @@ -698,15 +787,23 @@ var _ = Describe("applying manifests", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) + // Ensure applied manifest has been removed. regularDeployRemovedActual := regularDeployRemovedActual(nsName, deployName) Eventually(regularDeployRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -953,12 +1050,18 @@ var _ = Describe("applying manifests", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) + deleteWorkObject(workName) + + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -1122,40 +1225,829 @@ var _ = Describe("applying manifests", func() { Eventually(workStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update work status") }) - It("should update the AppliedWork object status", func() { - // Prepare the status information. - appliedResourceMeta := []fleetv1beta1.AppliedResourceMeta{ - { - WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ - Ordinal: 0, - Group: "", - Version: "v1", - Kind: "Namespace", - Resource: "namespaces", - Name: nsName, - }, - UID: regularNS.UID, - }, - } + It("should update the AppliedWork object status", func() { + // Prepare the status information. + appliedResourceMeta := []fleetv1beta1.AppliedResourceMeta{ + { + WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 0, + Group: "", + Version: "v1", + Kind: "Namespace", + Resource: "namespaces", + Name: nsName, + }, + UID: regularNS.UID, + }, + } + + appliedWorkStatusUpdatedActual := appliedWorkStatusUpdated(workName, appliedResourceMeta) + Eventually(appliedWorkStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update appliedWork status") + }) + + AfterAll(func() { + // Delete the Work object and related resources. + deleteWorkObject(workName) + + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + + // The environment prepared by the envtest package does not support namespace + // deletion; consequently this test suite would not attempt so verify its deletion. + }) + }) + + Context("can handle partial failures (pre-processing, decoding error)", Ordered, func() { + workName := fmt.Sprintf(workNameTemplate, utils.RandStr()) + // The environment prepared by the envtest package does not support namespace + // deletion; each test case would use a new namespace. + nsName := fmt.Sprintf(nsNameTemplate, utils.RandStr()) + + var appliedWorkOwnerRef *metav1.OwnerReference + var regularNS *corev1.Namespace + var decodingErredDeploy *appsv1.Deployment + var regularConfigMap *corev1.ConfigMap + + BeforeAll(func() { + // Prepare a NS object. + regularNS = ns.DeepCopy() + regularNS.Name = nsName + regularNSJSON := marshalK8sObjJSON(regularNS) + + // Prepare a mal-formed Deployment object. + decodingErredDeploy = deploy.DeepCopy() + decodingErredDeploy.TypeMeta = metav1.TypeMeta{ + APIVersion: "dummy/v10", + Kind: "Fake", + } + decodingErredDeploy.Namespace = nsName + decodingErredDeploy.Name = deployName + decodingErredDeployJSON := marshalK8sObjJSON(decodingErredDeploy) + + // Prepare a ConfigMap object. + regularConfigMap = configMap.DeepCopy() + regularConfigMap.Namespace = nsName + regularConfigMapJSON := marshalK8sObjJSON(regularConfigMap) + + // Create a new Work object with all the manifest JSONs. + createWorkObject(workName, nil, regularNSJSON, decodingErredDeployJSON, regularConfigMapJSON) + }) + + It("should add cleanup finalizer to the Work object", func() { + finalizerAddedActual := workFinalizerAddedActual(workName) + Eventually(finalizerAddedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to add cleanup finalizer to the Work object") + }) + + It("should prepare an AppliedWork object", func() { + appliedWorkCreatedActual := appliedWorkCreatedActual(workName) + Eventually(appliedWorkCreatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to prepare an AppliedWork object") + + appliedWorkOwnerRef = prepareAppliedWorkOwnerRef(workName) + }) + + It("should apply the manifests", func() { + // Ensure that the NS object has been applied as expected. + regularNSObjectAppliedActual := regularNSObjectAppliedActual(nsName, appliedWorkOwnerRef) + Eventually(regularNSObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the namespace object") + + Expect(memberClient.Get(ctx, client.ObjectKey{Name: nsName}, regularNS)).To(Succeed(), "Failed to retrieve the NS object") + + // Ensure that the ConfigMap object has been applied as expected. + regularConfigMapObjectAppliedActual := regularConfigMapObjectAppliedActual(nsName, configMapName, appliedWorkOwnerRef) + Eventually(regularConfigMapObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the ConfigMap object") + Expect(memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: configMapName}, regularConfigMap)).To(Succeed(), "Failed to retrieve the ConfigMap object") + }) + + It("should update the Work object status", func() { + // Prepare the status information. + workConds := []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionFalse, + Reason: WorkNotAllManifestsAppliedReason, + }, + } + manifestConds := []fleetv1beta1.ManifestCondition{ + { + Identifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 0, + Group: "", + Version: "v1", + Kind: "Namespace", + Resource: "namespaces", + Name: nsName, + }, + Conditions: []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingApplyResultTypeApplied), + ObservedGeneration: 0, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + ObservedGeneration: 0, + }, + }, + }, + { + Identifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 1, + Group: "dummy", + Version: "v10", + Kind: "Fake", + Name: deployName, + Namespace: nsName, + }, + Conditions: []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionFalse, + Reason: string(ManifestProcessingApplyResultTypeDecodingErred), + }, + }, + }, + { + Identifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 2, + Version: "v1", + Kind: "ConfigMap", + Resource: "configmaps", + Name: configMapName, + Namespace: nsName, + }, + Conditions: []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingApplyResultTypeApplied), + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + }, + }, + }, + } + + workStatusUpdatedActual := workStatusUpdated(workName, workConds, manifestConds, nil, nil) + Eventually(workStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update work status") + }) + + It("should update the AppliedWork object status", func() { + // Prepare the status information. + appliedResourceMeta := []fleetv1beta1.AppliedResourceMeta{ + { + WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 0, + Group: "", + Version: "v1", + Kind: "Namespace", + Resource: "namespaces", + Name: nsName, + }, + UID: regularNS.UID, + }, + { + WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 2, + Group: "", + Version: "v1", + Kind: "ConfigMap", + Resource: "configmaps", + Name: configMapName, + Namespace: nsName, + }, + UID: regularConfigMap.UID, + }, + } + + appliedWorkStatusUpdatedActual := appliedWorkStatusUpdated(workName, appliedResourceMeta) + Eventually(appliedWorkStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update appliedWork status") + }) + + AfterAll(func() { + // Delete the Work object and related resources. + deleteWorkObject(workName) + + // Ensure applied manifest has been removed. + regularConfigMapRemovedActual := regularConfigMapRemovedActual(nsName, configMapName) + Eventually(regularConfigMapRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the ConfigMap object") + + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + + // The environment prepared by the envtest package does not support namespace + // deletion; consequently this test suite would not attempt so verify its deletion. + }) + }) +}) + +var _ = Describe("work applier garbage collection", func() { + Context("update owner reference with blockOwnerDeletion to false (other owner reference does not exist)", Ordered, func() { + workName := fmt.Sprintf(workNameTemplate, utils.RandStr()) + // The environment prepared by the envtest package does not support namespace + // deletion; each test case would use a new namespace. + nsName := fmt.Sprintf(nsNameTemplate, utils.RandStr()) + anotherOwnerReference := metav1.OwnerReference{ + APIVersion: "another-api-version", + Kind: "another-kind", + Name: "another-owner", + UID: "another-uid", + } + + var appliedWorkOwnerRef *metav1.OwnerReference + var regularNS *corev1.Namespace + var regularDeploy *appsv1.Deployment + + BeforeAll(func() { + // Prepare a NS object. + regularNS = ns.DeepCopy() + regularNS.Name = nsName + regularNSJSON := marshalK8sObjJSON(regularNS) + + // Prepare a Deployment object. + regularDeploy = deploy.DeepCopy() + regularDeploy.Namespace = nsName + regularDeploy.Name = deployName + regularDeployJSON := marshalK8sObjJSON(regularDeploy) + + // Create a new Work object with all the manifest JSONs. + createWorkObject(workName, &fleetv1beta1.ApplyStrategy{AllowCoOwnership: true}, regularNSJSON, regularDeployJSON) + }) + + It("should add cleanup finalizer to the Work object", func() { + finalizerAddedActual := workFinalizerAddedActual(workName) + Eventually(finalizerAddedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to add cleanup finalizer to the Work object") + }) + + It("should prepare an AppliedWork object", func() { + appliedWorkCreatedActual := appliedWorkCreatedActual(workName) + Eventually(appliedWorkCreatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to prepare an AppliedWork object") + + appliedWorkOwnerRef = prepareAppliedWorkOwnerRef(workName) + }) + + It("should apply the manifests", func() { + // Ensure that the NS object has been applied as expected. + regularNSObjectAppliedActual := regularNSObjectAppliedActual(nsName, appliedWorkOwnerRef) + Eventually(regularNSObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the namespace object") + + Expect(memberClient.Get(ctx, client.ObjectKey{Name: nsName}, regularNS)).To(Succeed(), "Failed to retrieve the NS object") + + // Ensure that the Deployment object has been applied as expected. + regularDeploymentObjectAppliedActual := regularDeploymentObjectAppliedActual(nsName, deployName, appliedWorkOwnerRef) + Eventually(regularDeploymentObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the deployment object") + + Expect(memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, regularDeploy)).To(Succeed(), "Failed to retrieve the Deployment object") + }) + + It("can mark the deployment as available", func() { + markDeploymentAsAvailable(nsName, deployName) + }) + + It("should update the Work object status", func() { + // Prepare the status information. + workConds := []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: WorkAllManifestsAppliedReason, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: WorkAllManifestsAvailableReason, + }, + } + manifestConds := []fleetv1beta1.ManifestCondition{ + { + Identifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 0, + Group: "", + Version: "v1", + Kind: "Namespace", + Resource: "namespaces", + Name: nsName, + }, + Conditions: []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingApplyResultTypeApplied), + ObservedGeneration: 0, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + ObservedGeneration: 0, + }, + }, + }, + { + Identifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 1, + Group: "apps", + Version: "v1", + Kind: "Deployment", + Resource: "deployments", + Name: deployName, + Namespace: nsName, + }, + Conditions: []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingApplyResultTypeApplied), + ObservedGeneration: 1, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + ObservedGeneration: 1, + }, + }, + }, + } + + workStatusUpdatedActual := workStatusUpdated(workName, workConds, manifestConds, nil, nil) + Eventually(workStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update work status") + }) + + It("should update the AppliedWork object status", func() { + // Prepare the status information. + appliedResourceMeta := []fleetv1beta1.AppliedResourceMeta{ + { + WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 0, + Group: "", + Version: "v1", + Kind: "Namespace", + Resource: "namespaces", + Name: nsName, + }, + UID: regularNS.UID, + }, + { + WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 1, + Group: "apps", + Version: "v1", + Kind: "Deployment", + Resource: "deployments", + Name: deployName, + Namespace: nsName, + }, + UID: regularDeploy.UID, + }, + } + + appliedWorkStatusUpdatedActual := appliedWorkStatusUpdated(workName, appliedResourceMeta) + Eventually(appliedWorkStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update appliedWork status") + }) + + It("can update Deployment object to add another owner reference", func() { + // Retrieve the Deployment object. + gotDeploy := &appsv1.Deployment{} + Expect(memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy)).To(Succeed(), "Failed to retrieve the Deployment object") + + // Add another owner reference to the Deployment object. + gotDeploy.OwnerReferences = append(gotDeploy.OwnerReferences, anotherOwnerReference) + Expect(memberClient.Update(ctx, gotDeploy)).To(Succeed(), "Failed to update the Deployment object with another owner reference") + + // Ensure that the Deployment object has been updated as expected. + Eventually(func() error { + // Retrieve the Deployment object again. + if err := memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy); err != nil { + return fmt.Errorf("failed to retrieve the Deployment object: %w", err) + } + + // Check that the Deployment object has been updated as expected. + if len(gotDeploy.OwnerReferences) != 2 { + return fmt.Errorf("expected 2 owner references, got %d", len(gotDeploy.OwnerReferences)) + } + for _, ownerRef := range gotDeploy.OwnerReferences { + if ownerRef.APIVersion == anotherOwnerReference.APIVersion && + ownerRef.Kind == anotherOwnerReference.Kind && + ownerRef.Name == anotherOwnerReference.Name && + ownerRef.UID == anotherOwnerReference.UID { + return nil + } + } + return fmt.Errorf("another owner reference not found in the Deployment object") + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to find another owner reference on Deployment") + }) + + It("should start deleting the Work object", func() { + // Start deleting the Work object. + deleteWorkObject(workName) + }) + + It("should start deleting the AppliedWork object", func() { + // Ensure that the Work object is being deleted. + Eventually(func() error { + appliedWork := &fleetv1beta1.AppliedWork{} + if err := memberClient.Get(ctx, client.ObjectKey{Name: workName}, appliedWork); err != nil { + return err + } + if !appliedWork.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(appliedWork, metav1.FinalizerDeleteDependents) { + return fmt.Errorf("appliedWork object still is not being deleted") + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to start deleting the AppliedWork object") + + // Explicitly wait a minute to let the deletion timestamp progress + time.Sleep(30 * time.Second) + }) + + It("should update owner reference from the Deployment object", func() { + // Ensure that the Deployment object has been updated applied owner reference with blockOwnerDeletion to false. + Eventually(func() error { + // Retrieve the Deployment object again. + gotDeploy := &appsv1.Deployment{} + if err := memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy); err != nil { + return fmt.Errorf("failed to retrieve the Deployment object: %w", err) + } + // Check that the Deployment object has been updated as expected. + for _, ownerRef := range gotDeploy.OwnerReferences { + if ownerRef.APIVersion == fleetv1beta1.GroupVersion.String() && + ownerRef.Kind == fleetv1beta1.AppliedWorkKind && + ownerRef.Name == workName && + ownerRef.UID == appliedWorkOwnerRef.UID { + if *ownerRef.BlockOwnerDeletion { + return fmt.Errorf("owner reference from AppliedWork still has BlockOwnerDeletion set to true") + } + } + } + return nil + }, 2*eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from Deployment") + }) + + AfterAll(func() { + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, 2*time.Minute, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + + // Ensure that the Deployment object still exists. + Consistently(func() error { + return memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, regularDeploy) + }, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Deployment object has been removed unexpectedly") + // Delete objects created by the test suite so that the next test case can run without issues. + Expect(memberClient.Delete(ctx, regularDeploy)).To(Succeed(), "Failed to delete the Deployment object") + + // The environment prepared by the envtest package does not support namespace + // deletion; consequently this test suite would not attempt so verify its deletion. + }) + }) + + Context("update owner reference with blockOwnerDeletion to false (other owner reference invalid)", Ordered, func() { + workName := fmt.Sprintf(workNameTemplate, utils.RandStr()) + // The environment prepared by the envtest package does not support namespace + // deletion; each test case would use a new namespace. + nsName := fmt.Sprintf(nsNameTemplate, utils.RandStr()) + + var appliedWorkOwnerRef *metav1.OwnerReference + var regularNS *corev1.Namespace + var regularDeploy *appsv1.Deployment + var regularClusterRole *rbacv1.ClusterRole + + BeforeAll(func() { + // Prepare a NS object. + regularNS = ns.DeepCopy() + regularNS.Name = nsName + regularNSJSON := marshalK8sObjJSON(regularNS) + + // Prepare a Deployment object. + regularDeploy = deploy.DeepCopy() + regularDeploy.Namespace = nsName + regularDeploy.Name = deployName + regularDeployJSON := marshalK8sObjJSON(regularDeploy) + + // Prepare a ClusterRole object. + regularClusterRole = clusterRole.DeepCopy() + regularClusterRole.Name = clusterRoleName + regularClusterRoleJSON := marshalK8sObjJSON(regularClusterRole) + + // Create a new Work object with all the manifest JSONs. + createWorkObject(workName, &fleetv1beta1.ApplyStrategy{AllowCoOwnership: true}, regularNSJSON, regularDeployJSON, regularClusterRoleJSON) + }) + + It("should add cleanup finalizer to the Work object", func() { + finalizerAddedActual := workFinalizerAddedActual(workName) + Eventually(finalizerAddedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to add cleanup finalizer to the Work object") + }) + + It("should prepare an AppliedWork object", func() { + appliedWorkCreatedActual := appliedWorkCreatedActual(workName) + Eventually(appliedWorkCreatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to prepare an AppliedWork object") + + appliedWorkOwnerRef = prepareAppliedWorkOwnerRef(workName) + }) + + It("should apply the manifests", func() { + // Ensure that the NS object has been applied as expected. + regularNSObjectAppliedActual := regularNSObjectAppliedActual(nsName, appliedWorkOwnerRef) + Eventually(regularNSObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the namespace object") + + Expect(memberClient.Get(ctx, client.ObjectKey{Name: nsName}, regularNS)).To(Succeed(), "Failed to retrieve the NS object") + + // Ensure that the Deployment object has been applied as expected. + regularDeploymentObjectAppliedActual := regularDeploymentObjectAppliedActual(nsName, deployName, appliedWorkOwnerRef) + Eventually(regularDeploymentObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the deployment object") + + Expect(memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, regularDeploy)).To(Succeed(), "Failed to retrieve the Deployment object") + + // Ensure that the ClusterRole object has been applied as expected. + regularClusterRoleObjectAppliedActual := regularClusterRoleObjectAppliedActual(clusterRoleName, appliedWorkOwnerRef) + Eventually(regularClusterRoleObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the clusterRole object") + + Expect(memberClient.Get(ctx, client.ObjectKey{Name: clusterRoleName}, regularClusterRole)).To(Succeed(), "Failed to retrieve the clusterRole object") + }) + + It("can mark the deployment as available", func() { + markDeploymentAsAvailable(nsName, deployName) + }) + + It("should update the Work object status", func() { + // Prepare the status information. + workConds := []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: WorkAllManifestsAppliedReason, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: WorkAllManifestsAvailableReason, + }, + } + manifestConds := []fleetv1beta1.ManifestCondition{ + { + Identifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 0, + Group: "", + Version: "v1", + Kind: "Namespace", + Resource: "namespaces", + Name: nsName, + }, + Conditions: []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingApplyResultTypeApplied), + ObservedGeneration: 0, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + ObservedGeneration: 0, + }, + }, + }, + { + Identifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 1, + Group: "apps", + Version: "v1", + Kind: "Deployment", + Resource: "deployments", + Name: deployName, + Namespace: nsName, + }, + Conditions: []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingApplyResultTypeApplied), + ObservedGeneration: 1, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + ObservedGeneration: 1, + }, + }, + }, + { + Identifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 2, + Group: "rbac.authorization.k8s.io", + Version: "v1", + Kind: "ClusterRole", + Resource: "clusterroles", + Name: clusterRoleName, + }, + Conditions: []metav1.Condition{ + { + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingApplyResultTypeApplied), + ObservedGeneration: 0, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + ObservedGeneration: 0, + }, + }, + }, + } + + workStatusUpdatedActual := workStatusUpdated(workName, workConds, manifestConds, nil, nil) + Eventually(workStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update work status") + }) + + It("should update the AppliedWork object status", func() { + // Prepare the status information. + appliedResourceMeta := []fleetv1beta1.AppliedResourceMeta{ + { + WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 0, + Group: "", + Version: "v1", + Kind: "Namespace", + Resource: "namespaces", + Name: nsName, + }, + UID: regularNS.UID, + }, + { + WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 1, + Group: "apps", + Version: "v1", + Kind: "Deployment", + Resource: "deployments", + Name: deployName, + Namespace: nsName, + }, + UID: regularDeploy.UID, + }, + { + WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 2, + Group: "rbac.authorization.k8s.io", + Version: "v1", + Kind: "ClusterRole", + Resource: "clusterroles", + Name: clusterRoleName, + }, + UID: regularClusterRole.UID, + }, + } + + appliedWorkStatusUpdatedActual := appliedWorkStatusUpdated(workName, appliedResourceMeta) + Eventually(appliedWorkStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update appliedWork status") + }) + + It("can update ClusterRole object to add another owner reference", func() { + // Retrieve the ClusterRole object. + gotClusterRole := &rbacv1.ClusterRole{} + Expect(memberClient.Get(ctx, client.ObjectKey{Name: clusterRoleName}, gotClusterRole)).To(Succeed(), "Failed to retrieve the ClusterRole object") + + // Retrieve the Deployment object. + gotDeploy := &appsv1.Deployment{} + Expect(memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy)).To(Succeed(), "Failed to retrieve the Deployment object") + + // Add another owner reference to the ClusterRole object. + // Note: This is an invalid owner reference, as it adds a namespace-scoped object as an owner of a cluster-scoped object. + gotClusterRole.OwnerReferences = append(gotClusterRole.OwnerReferences, metav1.OwnerReference{ + APIVersion: appsv1.SchemeGroupVersion.String(), + Kind: "Deployment", + Name: gotDeploy.Name, + UID: gotDeploy.UID, + }) + Expect(memberClient.Update(ctx, gotClusterRole)).To(Succeed(), "Failed to update the ClusterRole object with another owner reference") + + // Ensure that the ClusterRole object has been updated as expected. + Eventually(func() error { + // Retrieve the ClusterRole object again. + if err := memberClient.Get(ctx, client.ObjectKey{Name: clusterRoleName}, gotClusterRole); err != nil { + return fmt.Errorf("failed to retrieve the ClusterRole object: %w", err) + } + + // Check that the ClusterRole object has been updated as expected. + if len(gotClusterRole.OwnerReferences) != 2 { + return fmt.Errorf("expected 2 owner references, got %d", len(gotClusterRole.OwnerReferences)) + } + for _, ownerRef := range gotClusterRole.OwnerReferences { + if ownerRef.APIVersion == appsv1.SchemeGroupVersion.String() && + ownerRef.Kind == "Deployment" && + ownerRef.Name == gotDeploy.Name && + ownerRef.UID == gotDeploy.UID { + return nil + } + } + return fmt.Errorf("another owner reference not found in the ClusterRole object") + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to find another owner reference on ClusterRole") + }) + + It("should start deleting the Work object", func() { + // Start deleting the Work object. + deleteWorkObject(workName) + }) + + It("should start deleting the AppliedWork object", func() { + // Ensure that the Work object is being deleted. + Eventually(func() error { + appliedWork := &fleetv1beta1.AppliedWork{} + if err := memberClient.Get(ctx, client.ObjectKey{Name: workName}, appliedWork); err != nil { + return err + } + if !appliedWork.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(appliedWork, metav1.FinalizerDeleteDependents) { + return fmt.Errorf("appliedWork object still is not being deleted") + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to start deleting the AppliedWork object") + + // Explicitly wait a minute to let the deletion timestamp progress + time.Sleep(30 * time.Second) + }) + + It("should update owner reference from the ClusterRole object", func() { + // Ensure that the ClusterRole object has been updated with AppliedWork owner reference to have BlockOwnerDeletion set to false. + Eventually(func() error { + // Retrieve the ClusterRole object again. + gotClusterRole := &rbacv1.ClusterRole{} + if err := memberClient.Get(ctx, client.ObjectKey{Name: clusterRoleName}, gotClusterRole); err != nil { + return fmt.Errorf("failed to retrieve the ClusterRole object: %w", err) + } + + // Check that the ClusterRole object has been updated as expected. + for _, ownerRef := range gotClusterRole.OwnerReferences { + if ownerRef.APIVersion == appliedWorkOwnerRef.APIVersion && + ownerRef.Kind == appliedWorkOwnerRef.Kind && + ownerRef.Name == appliedWorkOwnerRef.Name && + ownerRef.UID == appliedWorkOwnerRef.UID && *ownerRef.BlockOwnerDeletion { + return fmt.Errorf("owner reference from AppliedWork still has BlockOwnerDeletion set to true") + } + } - appliedWorkStatusUpdatedActual := appliedWorkStatusUpdated(workName, appliedResourceMeta) - Eventually(appliedWorkStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update appliedWork status") + return nil + }, 2*eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from ClusterRole") }) AfterAll(func() { - // Delete the Work object and related resources. - cleanupWorkObject(workName) + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure applied manifest has been removed. + regularDeployRemovedActual := regularDeployRemovedActual(nsName, deployName) + Eventually(regularDeployRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the deployment object") - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, 2*time.Minute, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + + // Ensure that the ClusterRole object still exists. + Consistently(func() error { + return memberClient.Get(ctx, client.ObjectKey{Name: clusterRoleName}, regularClusterRole) + }, consistentlyDuration, consistentlyInterval).Should(BeNil(), "ClusterRole object has been removed unexpectedly") + // Delete objects created by the test suite so that the next test case can run without issues. + Expect(memberClient.Delete(ctx, regularClusterRole)).To(Succeed(), "Failed to delete the clusterRole object") // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) }) - Context("can handle partial failures (pre-processing, decoding error)", Ordered, func() { + Context("update owner reference with blockOwnerDeletion to false (other owner reference valid)", Ordered, func() { workName := fmt.Sprintf(workNameTemplate, utils.RandStr()) // The environment prepared by the envtest package does not support namespace // deletion; each test case would use a new namespace. @@ -1163,8 +2055,8 @@ var _ = Describe("applying manifests", func() { var appliedWorkOwnerRef *metav1.OwnerReference var regularNS *corev1.Namespace - var decodingErredDeploy *appsv1.Deployment - var regularConfigMap *corev1.ConfigMap + var regularDeploy *appsv1.Deployment + var regularClusterRole *rbacv1.ClusterRole BeforeAll(func() { // Prepare a NS object. @@ -1172,23 +2064,19 @@ var _ = Describe("applying manifests", func() { regularNS.Name = nsName regularNSJSON := marshalK8sObjJSON(regularNS) - // Prepare a mal-formed Deployment object. - decodingErredDeploy = deploy.DeepCopy() - decodingErredDeploy.TypeMeta = metav1.TypeMeta{ - APIVersion: "dummy/v10", - Kind: "Fake", - } - decodingErredDeploy.Namespace = nsName - decodingErredDeploy.Name = deployName - decodingErredDeployJSON := marshalK8sObjJSON(decodingErredDeploy) + // Prepare a Deployment object. + regularDeploy = deploy.DeepCopy() + regularDeploy.Namespace = nsName + regularDeploy.Name = deployName + regularDeployJSON := marshalK8sObjJSON(regularDeploy) - // Prepare a ConfigMap object. - regularConfigMap = configMap.DeepCopy() - regularConfigMap.Namespace = nsName - regularConfigMapJSON := marshalK8sObjJSON(regularConfigMap) + // Prepare a ClusterRole object. + regularClusterRole = clusterRole.DeepCopy() + regularClusterRole.Name = clusterRoleName + regularClusterRoleJSON := marshalK8sObjJSON(regularClusterRole) // Create a new Work object with all the manifest JSONs. - createWorkObject(workName, nil, regularNSJSON, decodingErredDeployJSON, regularConfigMapJSON) + createWorkObject(workName, &fleetv1beta1.ApplyStrategy{AllowCoOwnership: true}, regularNSJSON, regularDeployJSON, regularClusterRoleJSON) }) It("should add cleanup finalizer to the Work object", func() { @@ -1210,10 +2098,21 @@ var _ = Describe("applying manifests", func() { Expect(memberClient.Get(ctx, client.ObjectKey{Name: nsName}, regularNS)).To(Succeed(), "Failed to retrieve the NS object") - // Ensure that the ConfigMap object has been applied as expected. - regularConfigMapObjectAppliedActual := regularConfigMapObjectAppliedActual(nsName, configMapName, appliedWorkOwnerRef) - Eventually(regularConfigMapObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the ConfigMap object") - Expect(memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: configMapName}, regularConfigMap)).To(Succeed(), "Failed to retrieve the ConfigMap object") + // Ensure that the Deployment object has been applied as expected. + regularDeploymentObjectAppliedActual := regularDeploymentObjectAppliedActual(nsName, deployName, appliedWorkOwnerRef) + Eventually(regularDeploymentObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the deployment object") + + Expect(memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, regularDeploy)).To(Succeed(), "Failed to retrieve the Deployment object") + + // Ensure that the ClusterRole object has been applied as expected. + regularClusterRoleObjectAppliedActual := regularClusterRoleObjectAppliedActual(clusterRoleName, appliedWorkOwnerRef) + Eventually(regularClusterRoleObjectAppliedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to apply the clusterRole object") + + Expect(memberClient.Get(ctx, client.ObjectKey{Name: clusterRoleName}, regularClusterRole)).To(Succeed(), "Failed to retrieve the clusterRole object") + }) + + It("can mark the deployment as available", func() { + markDeploymentAsAvailable(nsName, deployName) }) It("should update the Work object status", func() { @@ -1221,8 +2120,13 @@ var _ = Describe("applying manifests", func() { workConds := []metav1.Condition{ { Type: fleetv1beta1.WorkConditionTypeApplied, - Status: metav1.ConditionFalse, - Reason: WorkNotAllManifestsAppliedReason, + Status: metav1.ConditionTrue, + Reason: WorkAllManifestsAppliedReason, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: WorkAllManifestsAvailableReason, }, } manifestConds := []fleetv1beta1.ManifestCondition{ @@ -1253,39 +2157,49 @@ var _ = Describe("applying manifests", func() { { Identifier: fleetv1beta1.WorkResourceIdentifier{ Ordinal: 1, - Group: "dummy", - Version: "v10", - Kind: "Fake", + Group: "apps", + Version: "v1", + Kind: "Deployment", + Resource: "deployments", Name: deployName, Namespace: nsName, }, Conditions: []metav1.Condition{ { - Type: fleetv1beta1.WorkConditionTypeApplied, - Status: metav1.ConditionFalse, - Reason: string(ManifestProcessingApplyResultTypeDecodingErred), + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingApplyResultTypeApplied), + ObservedGeneration: 1, + }, + { + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + ObservedGeneration: 1, }, }, }, { Identifier: fleetv1beta1.WorkResourceIdentifier{ - Ordinal: 2, - Version: "v1", - Kind: "ConfigMap", - Resource: "configmaps", - Name: configMapName, - Namespace: nsName, + Ordinal: 2, + Group: "rbac.authorization.k8s.io", + Version: "v1", + Kind: "ClusterRole", + Resource: "clusterroles", + Name: clusterRoleName, }, Conditions: []metav1.Condition{ { - Type: fleetv1beta1.WorkConditionTypeApplied, - Status: metav1.ConditionTrue, - Reason: string(ManifestProcessingApplyResultTypeApplied), + Type: fleetv1beta1.WorkConditionTypeApplied, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingApplyResultTypeApplied), + ObservedGeneration: 0, }, { - Type: fleetv1beta1.WorkConditionTypeAvailable, - Status: metav1.ConditionTrue, - Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + Type: fleetv1beta1.WorkConditionTypeAvailable, + Status: metav1.ConditionTrue, + Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), + ObservedGeneration: 0, }, }, }, @@ -1311,15 +2225,26 @@ var _ = Describe("applying manifests", func() { }, { WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ - Ordinal: 2, - Group: "", + Ordinal: 1, + Group: "apps", Version: "v1", - Kind: "ConfigMap", - Resource: "configmaps", - Name: configMapName, + Kind: "Deployment", + Resource: "deployments", + Name: deployName, Namespace: nsName, }, - UID: regularConfigMap.UID, + UID: regularDeploy.UID, + }, + { + WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ + Ordinal: 2, + Group: "rbac.authorization.k8s.io", + Version: "v1", + Kind: "ClusterRole", + Resource: "clusterroles", + Name: clusterRoleName, + }, + UID: regularClusterRole.UID, }, } @@ -1327,17 +2252,113 @@ var _ = Describe("applying manifests", func() { Eventually(appliedWorkStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update appliedWork status") }) + It("can update Deployment object to add another owner reference", func() { + // Retrieve the ClusterRole object. + gotClusterRole := &rbacv1.ClusterRole{} + Expect(memberClient.Get(ctx, client.ObjectKey{Name: clusterRoleName}, gotClusterRole)).To(Succeed(), "Failed to retrieve the ClusterRole object") + + // Retrieve the Deployment object. + gotDeploy := &appsv1.Deployment{} + Expect(memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy)).To(Succeed(), "Failed to retrieve the Deployment object") + + // Add another owner reference to the Deployment object. + gotDeploy.OwnerReferences = append(gotDeploy.OwnerReferences, metav1.OwnerReference{ + APIVersion: rbacv1.SchemeGroupVersion.String(), + Kind: "ClusterRole", + Name: gotClusterRole.Name, + UID: gotClusterRole.UID, + }) + Expect(memberClient.Update(ctx, gotDeploy)).To(Succeed(), "Failed to update the Deployment object with another owner reference") + + // Ensure that the Deployment object has been updated as expected. + Eventually(func() error { + // Retrieve the Deployment object again. + if err := memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy); err != nil { + return fmt.Errorf("failed to retrieve the Deployment object: %w", err) + } + + // Check that the Deployment object has been updated as expected. + if len(gotDeploy.OwnerReferences) != 2 { + return fmt.Errorf("expected 2 owner references, got %d", len(gotDeploy.OwnerReferences)) + } + for _, ownerRef := range gotDeploy.OwnerReferences { + if ownerRef.APIVersion == rbacv1.SchemeGroupVersion.String() && + ownerRef.Kind == "ClusterRole" && + ownerRef.Name == gotClusterRole.Name && + ownerRef.UID == gotClusterRole.UID { + return nil + } + } + return fmt.Errorf("another owner reference not found in the Deployment object") + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to find another owner reference on Deployment") + }) + + It("should start deleting the Work object", func() { + // Start deleting the Work object. + deleteWorkObject(workName) + }) + + It("should start deleting the AppliedWork object", func() { + // Ensure that the Work object is being deleted. + Eventually(func() error { + appliedWork := &fleetv1beta1.AppliedWork{} + if err := memberClient.Get(ctx, client.ObjectKey{Name: workName}, appliedWork); err != nil { + return err + } + if !appliedWork.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(appliedWork, metav1.FinalizerDeleteDependents) { + return fmt.Errorf("appliedWork object still is not being deleted") + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to start deleting the AppliedWork object") + + // Explicitly wait a minute to let the deletion timestamp progress + time.Sleep(30 * time.Second) + }) + + It("should update owner reference from the Deployment object", func() { + // Ensure that the Deployment object has been updated with AppliedWork owner reference to have BlockOwnerDeletion set to false. + Eventually(func() error { + // Retrieve the Deployment object. + gotDeploy := &appsv1.Deployment{} + if err := memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, gotDeploy); err != nil { + return fmt.Errorf("failed to retrieve the ClusterRole object: %w", err) + } + + // Check that the Deployment object has been updated as expected. + for _, ownerRef := range gotDeploy.OwnerReferences { + if ownerRef.APIVersion == appliedWorkOwnerRef.APIVersion && + ownerRef.Kind == appliedWorkOwnerRef.Kind && + ownerRef.Name == appliedWorkOwnerRef.Name && + ownerRef.UID == appliedWorkOwnerRef.UID && *ownerRef.BlockOwnerDeletion { + return fmt.Errorf("owner reference from AppliedWork still has BlockOwnerDeletion set to true") + } + } + return nil + }, 2*eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from Deployment") + }) + AfterAll(func() { - // Delete the Work object and related resources. - cleanupWorkObject(workName) + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) + // Ensure applied manifest has been removed. + regularClusterRoleRemovedActual := regularClusterRoleRemovedActual(clusterRoleName) + Eventually(regularClusterRoleRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the ClusterRole object") + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") - regularConfigMapRemovedActual := regularConfigMapRemovedActual(nsName, configMapName) - Eventually(regularConfigMapRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the ConfigMap object") + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, 2*time.Minute, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // Ensure that the Deployment object still exists. + Consistently(func() error { + return memberClient.Get(ctx, client.ObjectKey{Namespace: nsName, Name: deployName}, regularDeploy) + }, consistentlyDuration, consistentlyInterval).Should(BeNil(), "Deployment object has been removed unexpectedly") + // Delete objects created by the test suite so that the next test case can run without issues. + Expect(memberClient.Delete(ctx, regularDeploy)).To(Succeed(), "Failed to delete the Deployment object") // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -1513,15 +2534,23 @@ var _ = Describe("drift detection and takeover", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) + // Ensure applied manifest has been removed. regularDeployRemovedActual := regularDeployRemovedActual(nsName, deployName) Eventually(regularDeployRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -1775,16 +2804,19 @@ var _ = Describe("drift detection and takeover", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that the AppliedWork object has been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) // Ensure that the Deployment object has been left alone. regularDeployNotRemovedActual := regularDeployNotRemovedActual(nsName, deployName) Consistently(regularDeployNotRemovedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -2050,16 +3082,19 @@ var _ = Describe("drift detection and takeover", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that the AppliedWork object has been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) // Ensure that the Deployment object has been left alone. regularDeployNotRemovedActual := regularDeployNotRemovedActual(nsName, deployName) Consistently(regularDeployNotRemovedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -2461,16 +3496,23 @@ var _ = Describe("drift detection and takeover", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that the AppliedWork object has been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) // Ensure that the Deployment object has been left alone. regularDeployNotRemovedActual := regularDeployNotRemovedActual(nsName, deployName) Consistently(regularDeployNotRemovedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -2700,12 +3742,15 @@ var _ = Describe("drift detection and takeover", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) + deleteWorkObject(workName) // Ensure that the AppliedWork object has been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -2951,12 +3996,19 @@ var _ = Describe("drift detection and takeover", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) + deleteWorkObject(workName) + + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) // Ensure that the AppliedWork object has been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -3305,12 +4357,19 @@ var _ = Describe("drift detection and takeover", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) + deleteWorkObject(workName) + + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) // Ensure that the AppliedWork object has been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -3573,6 +4632,25 @@ var _ = Describe("drift detection and takeover", func() { workStatusUpdatedActual := workStatusUpdated(workName, workConds, manifestConds, &driftObservedMustBeforeTimestamp, &firstDriftedMustBeforeTimestamp) Eventually(workStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update work status") }) + + AfterAll(func() { + // Delete the Work object and related resources. + deleteWorkObject(workName) + + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + + // The environment prepared by the envtest package does not support namespace + // deletion; consequently this test suite would not attempt so verify its deletion. + }) }) Context("never take over", Ordered, func() { @@ -3739,15 +4817,19 @@ var _ = Describe("drift detection and takeover", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) + // Ensure applied manifest has been removed. regularDeployRemovedActual := regularDeployRemovedActual(nsName, deployName) Eventually(regularDeployRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -3845,12 +4927,15 @@ var _ = Describe("report diff", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) + deleteWorkObject(workName) // Ensure that the AppliedWork object has been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -4166,16 +5251,19 @@ var _ = Describe("report diff", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that the AppliedWork object has been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) // Ensure that the Deployment object has been left alone. regularDeployNotRemovedActual := regularDeployNotRemovedActual(nsName, deployName) Consistently(regularDeployNotRemovedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -4378,15 +5466,19 @@ var _ = Describe("report diff", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) + // Ensure applied manifest has been removed. regularDeployRemovedActual := regularDeployRemovedActual(nsName, deployName) Eventually(regularDeployRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -4736,15 +5828,23 @@ var _ = Describe("switch apply strategies", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) + // Ensure applied manifest has been removed. regularDeployRemovedActual := regularDeployRemovedActual(nsName, deployName) Eventually(regularDeployRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -4989,15 +6089,23 @@ var _ = Describe("switch apply strategies", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) + // Ensure applied manifest has been removed. regularDeployRemovedActual := regularDeployRemovedActual(nsName, deployName) Eventually(regularDeployRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) @@ -5366,15 +6474,23 @@ var _ = Describe("switch apply strategies", func() { AfterAll(func() { // Delete the Work object and related resources. - cleanupWorkObject(workName) - - // Ensure that all applied manifests have been removed. - appliedWorkRemovedActual := appliedWorkRemovedActual(workName) - Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + deleteWorkObject(workName) + // Ensure applied manifest has been removed. regularDeployRemovedActual := regularDeployRemovedActual(nsName, deployName) Eventually(regularDeployRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the deployment object") + // Kubebuilder suggests that in a testing environment like this, to check for the existence of the AppliedWork object + // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). + checkNSOwnerReferences(workName, nsName) + + // Ensure that the AppliedWork object has been removed. + appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) + Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object") + + workRemovedActual := workRemovedActual(workName) + Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object") + // The environment prepared by the envtest package does not support namespace // deletion; consequently this test suite would not attempt so verify its deletion. }) diff --git a/pkg/controllers/workapplier/controller_test.go b/pkg/controllers/workapplier/controller_test.go index e6b6433ee..4cb8c8903 100644 --- a/pkg/controllers/workapplier/controller_test.go +++ b/pkg/controllers/workapplier/controller_test.go @@ -26,6 +26,7 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -41,9 +42,10 @@ import ( const ( workName = "work-1" - deployName = "deploy-1" - configMapName = "configmap-1" - nsName = "ns-1" + deployName = "deploy-1" + configMapName = "configmap-1" + nsName = "ns-1" + clusterRoleName = "clusterrole-1" ) var ( @@ -120,6 +122,23 @@ var ( }, } + clusterRole = &rbacv1.ClusterRole{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "rbac.authorization.k8s.io/v1", + Kind: "ClusterRole", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: clusterRoleName, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get", "list", "watch"}, + }, + }, + } + dummyOwnerRef = metav1.OwnerReference{ APIVersion: "dummy.owner/v1", Kind: "DummyOwner", diff --git a/pkg/controllers/workapplier/preprocess.go b/pkg/controllers/workapplier/preprocess.go index 9dd61080d..f3deb0ad1 100644 --- a/pkg/controllers/workapplier/preprocess.go +++ b/pkg/controllers/workapplier/preprocess.go @@ -19,7 +19,6 @@ package workapplier import ( "context" "fmt" - "reflect" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -544,7 +543,7 @@ func isInMemberClusterObjectDerivedFromManifestObj(inMemberClusterObj *unstructu // Verify if the owner reference still stands. curOwners := inMemberClusterObj.GetOwnerReferences() for idx := range curOwners { - if reflect.DeepEqual(curOwners[idx], *expectedAppliedWorkOwnerRef) { + if areOwnerRefsEqual(&curOwners[idx], expectedAppliedWorkOwnerRef) { return true } } @@ -558,9 +557,19 @@ func removeOwnerRef(obj *unstructured.Unstructured, expectedAppliedWorkOwnerRef // Re-build the owner references; remove the given one from the list. for idx := range ownerRefs { - if !reflect.DeepEqual(ownerRefs[idx], *expectedAppliedWorkOwnerRef) { - updatedOwnerRefs = append(updatedOwnerRefs, ownerRefs[idx]) + if areOwnerRefsEqual(&ownerRefs[idx], expectedAppliedWorkOwnerRef) { + // Skip the expected owner reference. + continue } + updatedOwnerRefs = append(updatedOwnerRefs, ownerRefs[idx]) } obj.SetOwnerReferences(updatedOwnerRefs) } + +// areOwnerRefsEqual returns true if two owner references are equal based on UID, Name, Kind, and APIVersion. +func areOwnerRefsEqual(a, b *metav1.OwnerReference) bool { + return a.UID == b.UID && + a.Name == b.Name && + a.Kind == b.Kind && + a.APIVersion == b.APIVersion +} diff --git a/pkg/controllers/workapplier/process.go b/pkg/controllers/workapplier/process.go index 1c623505f..9b660e528 100644 --- a/pkg/controllers/workapplier/process.go +++ b/pkg/controllers/workapplier/process.go @@ -19,7 +19,6 @@ package workapplier import ( "context" "fmt" - "reflect" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -254,7 +253,7 @@ func shouldInitiateTakeOverAttempt(inMemberClusterObj *unstructured.Unstructured // Check if the live object is owned by Fleet. curOwners := inMemberClusterObj.GetOwnerReferences() for idx := range curOwners { - if reflect.DeepEqual(curOwners[idx], *expectedAppliedWorkOwnerRef) { + if areOwnerRefsEqual(&curOwners[idx], expectedAppliedWorkOwnerRef) { // The live object is owned by Fleet; no takeover is needed. return false } @@ -354,7 +353,7 @@ func canApplyWithOwnership(inMemberClusterObj *unstructured.Unstructured, expect // Verify if the object is owned by Fleet. curOwners := inMemberClusterObj.GetOwnerReferences() for idx := range curOwners { - if reflect.DeepEqual(curOwners[idx], *expectedAppliedWorkOwnerRef) { + if areOwnerRefsEqual(&curOwners[idx], expectedAppliedWorkOwnerRef) { return true } } diff --git a/pkg/controllers/workapplier/suite_test.go b/pkg/controllers/workapplier/suite_test.go index d7491e25b..b631098f1 100644 --- a/pkg/controllers/workapplier/suite_test.go +++ b/pkg/controllers/workapplier/suite_test.go @@ -165,6 +165,7 @@ var _ = BeforeSuite(func() { workerCount, time.Second*5, time.Second*5, + 30*time.Second, true, 60, ) diff --git a/test/e2e/enveloped_object_placement_test.go b/test/e2e/enveloped_object_placement_test.go index 38f8e164c..09dfda55b 100644 --- a/test/e2e/enveloped_object_placement_test.go +++ b/test/e2e/enveloped_object_placement_test.go @@ -207,6 +207,7 @@ var _ = Describe("placing wrapped resources using a CRP", func() { AfterAll(func() { By(fmt.Sprintf("deleting envelope %s", testResourceEnvelope.Name)) Expect(hubClient.Delete(ctx, &testResourceEnvelope)).To(Succeed(), "Failed to delete ResourceEnvelope") + By(fmt.Sprintf("deleting envelope %s", testClusterResourceEnvelope.Name)) Expect(hubClient.Delete(ctx, &testClusterResourceEnvelope)).To(Succeed(), "Failed to delete testClusterResourceEnvelope") By(fmt.Sprintf("deleting placement %s and related resources", crpName)) ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters) diff --git a/test/e2e/placement_apply_strategy_test.go b/test/e2e/placement_apply_strategy_test.go index c2a8bebba..937f2c506 100644 --- a/test/e2e/placement_apply_strategy_test.go +++ b/test/e2e/placement_apply_strategy_test.go @@ -43,27 +43,29 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() { annotationValue := "annotation-value" annotationUpdatedValue := "annotation-updated-value" workNamespaceName := fmt.Sprintf(workNamespaceNameTemplate, GinkgoParallelProcess()) + anotherOwnerReference := metav1.OwnerReference{} BeforeAll(func() { By("creating work resources on hub cluster") createWorkResources() + + By("creating owner reference for the namespace") + anotherOwnerReference = createAnotherValidOwnerReference(fmt.Sprintf("owner-namespace-%d", GinkgoParallelProcess())) }) AfterAll(func() { By("deleting created work resources on hub cluster") cleanupWorkResources() + + By("deleting owner reference namespace") + cleanupAnotherValidOwnerReference(anotherOwnerReference.Name) }) Context("Test a CRP place objects successfully (client-side-apply and allow co-own)", Ordered, func() { BeforeAll(func() { ns := appNamespace() ns.SetOwnerReferences([]metav1.OwnerReference{ - { - APIVersion: "another-api-version", - Kind: "another-kind", - Name: "another-owner", - UID: "another-uid", - }, + anotherOwnerReference, }) ns.Annotations = map[string]string{ annotationKey: annotationValue, @@ -117,10 +119,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() { }) It("namespace should be kept on member cluster", func() { - Consistently(func() error { - ns := &corev1.Namespace{} - return allMemberClusters[0].KubeClient.Get(ctx, types.NamespacedName{Name: workNamespaceName}, ns) - }, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Namespace which is not owned by the CRP should not be deleted") + checkNamespaceExistsWithOwnerRefOnMemberCluster(workNamespaceName, crpName) }) }) @@ -231,12 +230,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() { BeforeAll(func() { ns := appNamespace() ns.SetOwnerReferences([]metav1.OwnerReference{ - { - APIVersion: "another-api-version", - Kind: "another-kind", - Name: "another-owner", - UID: "another-uid", - }, + anotherOwnerReference, }) By(fmt.Sprintf("creating namespace %s on member cluster", ns.Name)) Expect(allMemberClusters[0].KubeClient.Create(ctx, &ns)).Should(Succeed(), "Failed to create namespace %s", ns.Name) @@ -345,11 +339,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() { }) It("namespace should be kept on member cluster", func() { - Consistently(func() error { - workNamespaceName := fmt.Sprintf(workNamespaceNameTemplate, GinkgoParallelProcess()) - ns := &corev1.Namespace{} - return allMemberClusters[0].KubeClient.Get(ctx, types.NamespacedName{Name: workNamespaceName}, ns) - }, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Namespace which is not owned by the CRP should not be deleted") + checkNamespaceExistsWithOwnerRefOnMemberCluster(workNamespaceName, crpName) }) }) @@ -357,12 +347,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() { BeforeAll(func() { ns := appNamespace() ns.SetOwnerReferences([]metav1.OwnerReference{ - { - APIVersion: "another-api-version", - Kind: "another-kind", - Name: "another-owner", - UID: "another-uid", - }, + anotherOwnerReference, }) ns.Annotations = map[string]string{ annotationKey: annotationValue, @@ -430,10 +415,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() { }) It("namespace should be kept on member cluster", func() { - Consistently(func() error { - ns := &corev1.Namespace{} - return allMemberClusters[0].KubeClient.Get(ctx, types.NamespacedName{Name: workNamespaceName}, ns) - }, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Namespace which is not owned by the CRP should not be deleted") + checkNamespaceExistsWithOwnerRefOnMemberCluster(workNamespaceName, crpName) }) }) diff --git a/test/e2e/placement_pickfixed_test.go b/test/e2e/placement_pickfixed_test.go index 2e7d3a8ba..9979596ea 100644 --- a/test/e2e/placement_pickfixed_test.go +++ b/test/e2e/placement_pickfixed_test.go @@ -21,9 +21,11 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" "github.com/kubefleet-dev/kubefleet/test/e2e/framework" @@ -190,4 +192,117 @@ var _ = Describe("placing resources using a CRP of PickFixed placement type", fu ensureCRPAndRelatedResourcesDeleted(crpName, []*framework.Cluster{}) }) }) + + Context("switch to another cluster to simulate stuck deleting works", Ordered, func() { + crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess()) + workNamespaceName := fmt.Sprintf(workNamespaceNameTemplate, GinkgoParallelProcess()) + appConfigMapName := fmt.Sprintf(appConfigMapNameTemplate, GinkgoParallelProcess()) + var currentConfigMap corev1.ConfigMap + + BeforeAll(func() { + // Create the resources. + createWorkResources() + + // Create the CRP. + crp := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: crpName, + // Add a custom finalizer; this would allow us to better observe + // the behavior of the controllers. + Finalizers: []string{customDeletionBlockerFinalizer}, + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: workResourceSelector(), + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: &placementv1beta1.RollingUpdateConfig{ + UnavailablePeriodSeconds: ptr.To(2), + }, + }, + Policy: &placementv1beta1.PlacementPolicy{ + PlacementType: placementv1beta1.PickFixedPlacementType, + ClusterNames: []string{ + memberCluster1EastProdName, + }, + }, + }, + } + Expect(hubClient.Create(ctx, crp)).To(Succeed(), "Failed to create CRP") + }) + + It("should update CRP status as expected", func() { + crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), []string{memberCluster1EastProdName}, nil, "0") + Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected") + }) + + It("should place resources on specified clusters", func() { + resourcePlacedActual := workNamespaceAndConfigMapPlacedOnClusterActual(memberCluster1EastProd) + Eventually(resourcePlacedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to place resources on specified clusters") + }) + + It("should add finalizer to work resources on the specified clusters", func() { + Eventually(func() error { + if err := memberCluster1EastProd.KubeClient.Get(ctx, types.NamespacedName{Namespace: workNamespaceName, Name: appConfigMapName}, ¤tConfigMap); err != nil { + return err + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to get configmap") + // Add finalizer to block deletion to simulate work stuck + controllerutil.AddFinalizer(¤tConfigMap, "example.com/finalizer") + Expect(memberCluster1EastProd.KubeClient.Update(ctx, ¤tConfigMap)).To(Succeed(), "Failed to update configmap with finalizer") + }) + + It("update crp to pick another cluster", func() { + Eventually(func() error { + crp := &placementv1beta1.ClusterResourcePlacement{} + if err := hubClient.Get(ctx, types.NamespacedName{Name: crpName}, crp); err != nil { + return err + } + crp.Spec.Policy.ClusterNames = []string{memberCluster2EastCanaryName} + return hubClient.Update(ctx, crp) + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP") + }) + + It("should update CRP status as expected", func() { + // should successfully apply to the new cluster + crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), []string{memberCluster2EastCanaryName}, nil, "0") + Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected") + }) + + It("should have a deletion timestamp on work objects", func() { + work := &placementv1beta1.Work{} + Expect(hubClient.Get(ctx, types.NamespacedName{Namespace: fmt.Sprintf("fleet-member-%s", memberCluster1EastProdName), Name: fmt.Sprintf("%s-work", crpName)}, work)).Should(Succeed(), "Failed to get work") + Expect(work.DeletionTimestamp).ShouldNot(BeNil(), "Work should have a deletion timestamp") + + appliedWork := &placementv1beta1.AppliedWork{} + Expect(memberCluster1EastProd.KubeClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-work", crpName)}, appliedWork)).Should(Succeed(), "Failed to get appliedwork") + Expect(appliedWork.DeletionTimestamp).ShouldNot(BeNil(), "AppliedWork should have a deletion timestamp") + }) + + It("configmap should still exists on previously specified cluster and be in deleting state", func() { + configMap := &corev1.ConfigMap{} + Expect(memberCluster1EastProd.KubeClient.Get(ctx, types.NamespacedName{Namespace: workNamespaceName, Name: appConfigMapName}, configMap)).Should(Succeed(), "Failed to get configmap") + Expect(configMap.DeletionTimestamp).ShouldNot(BeNil(), "ConfigMap should have a deletion timestamp") + }) + + It("should remove finalizer from work resources on the specified clusters", func() { + configMap := &corev1.ConfigMap{} + Expect(memberCluster1EastProd.KubeClient.Get(ctx, types.NamespacedName{Namespace: workNamespaceName, Name: appConfigMapName}, configMap)).Should(Succeed(), "Failed to get configmap") + controllerutil.RemoveFinalizer(configMap, "example.com/finalizer") + Expect(memberCluster1EastProd.KubeClient.Update(ctx, configMap)).To(Succeed(), "Failed to update configmap with finalizer") + }) + + It("should remove resources from previously specified clusters", func() { + checkIfRemovedWorkResourcesFromMemberClusters([]*framework.Cluster{memberCluster1EastProd}) + }) + + It("should update CRP status as expected", func() { + crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), []string{memberCluster2EastCanaryName}, nil, "0") + Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected") + }) + + AfterAll(func() { + ensureCRPAndRelatedResourcesDeleted(crpName, []*framework.Cluster{memberCluster1EastProd, memberCluster2EastCanary}) + }) + }) }) diff --git a/test/e2e/utils_test.go b/test/e2e/utils_test.go index 60307f021..c8f5eab42 100644 --- a/test/e2e/utils_test.go +++ b/test/e2e/utils_test.go @@ -730,6 +730,36 @@ func setAllMemberClustersToLeave() { } } +func createAnotherValidOwnerReference(nsName string) metav1.OwnerReference { + // Create a namespace to be owner. + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: nsName, + }, + } + Expect(allMemberClusters[0].KubeClient.Create(ctx, ns)).Should(Succeed(), "Failed to create namespace %s", nsName) + + // Get the namespace to ensure to create a valid owner reference. + Expect(allMemberClusters[0].KubeClient.Get(ctx, types.NamespacedName{Name: nsName}, ns)).Should(Succeed(), "Failed to get namespace %s", nsName) + + return metav1.OwnerReference{ + APIVersion: "v1", + Kind: "Namespace", + Name: nsName, + UID: ns.UID, + } +} + +func cleanupAnotherValidOwnerReference(nsName string) { + // Cleanup the namespace created for the owner reference. + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: nsName, + }, + } + Expect(allMemberClusters[0].KubeClient.Delete(ctx, ns)).Should(Succeed(), "Failed to create namespace %s", nsName) +} + func checkIfAllMemberClustersHaveLeft() { for idx := range allMemberClusters { memberCluster := allMemberClusters[idx] @@ -788,6 +818,27 @@ func checkIfRemovedWorkResourcesFromMemberClustersConsistently(clusters []*frame Consistently(workResourcesRemovedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to remove work resources from member cluster %s consistently", memberCluster.ClusterName) } } +func checkNamespaceExistsWithOwnerRefOnMemberCluster(nsName, crpName string) { + Consistently(func() error { + ns := &corev1.Namespace{} + if err := allMemberClusters[0].KubeClient.Get(ctx, types.NamespacedName{Name: nsName}, ns); err != nil { + return fmt.Errorf("failed to get namespace %s: %w", nsName, err) + } + + if len(ns.OwnerReferences) > 0 { + for _, ownerRef := range ns.OwnerReferences { + if ownerRef.APIVersion == placementv1beta1.GroupVersion.String() && + ownerRef.Kind == placementv1beta1.AppliedWorkKind && + ownerRef.Name == fmt.Sprintf("%s-work", crpName) { + if *ownerRef.BlockOwnerDeletion { + return fmt.Errorf("namespace %s owner reference for AppliedWork should have been updated to have BlockOwnerDeletion set to false", nsName) + } + } + } + } + return nil + }, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Namespace which is not owned by the CRP should not be deleted") +} // cleanupCRP deletes the CRP and waits until the resources are not found. func cleanupCRP(name string) { @@ -815,6 +866,30 @@ func cleanupCRP(name string) { // Wait until the CRP is removed. removedActual := crpRemovedActual(name) Eventually(removedActual, workloadEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove CRP %s", name) + + // Check if work is deleted. Needed to ensure that the Work resource is cleaned up before the next CRP is created. + // This is because the Work resource is created with a finalizer that blocks deletion until the all applied work + // and applied work itself is successfully deleted. If the Work resource is not deleted, it can cause resource overlap + // and flakiness in subsequent tests. + By("Check if work is deleted") + var workNS string + work := &placementv1beta1.Work{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-work", name), + }, + } + Eventually(func() bool { + for i := range allMemberClusters { + workNS = fmt.Sprintf("fleet-member-%s", allMemberClusterNames[i]) + if err := hubClient.Get(ctx, types.NamespacedName{Name: work.Name, Namespace: workNS}, work); err != nil && k8serrors.IsNotFound(err) { + // Work resource is not found, which is expected. + continue + } + // Work object still exists, or some other error occurred, return false to retry. + return false + } + return true + }, workloadEventuallyDuration, eventuallyInterval).Should(BeTrue(), fmt.Sprintf("Work resource %s from namespace %s should be deleted from hub", work.Name, workNS)) } // createResourceOverrides creates a number of resource overrides. @@ -1234,6 +1309,7 @@ func readJobTestManifest(testManifest *batchv1.Job) { func readEnvelopeResourceTestManifest(testEnvelopeObj *placementv1beta1.ResourceEnvelope) { By("Read testEnvelopConfigMap resource") + testEnvelopeObj.ResourceVersion = "" err := utils.GetObjectFromManifest("resources/test-envelope-object.yaml", testEnvelopeObj) Expect(err).Should(Succeed()) } @@ -1283,7 +1359,7 @@ func buildOwnerReference(cluster *framework.Cluster, crpName string) *metav1.Own Kind: "AppliedWork", Name: workName, UID: appliedWork.UID, - BlockOwnerDeletion: ptr.To(false), + BlockOwnerDeletion: ptr.To(true), } }