bug: deploymentType Local and management cluster

controllers/export_test.go

@@ -121,6 +121,7 @@ var (
 	GetDeployedGroupVersionKinds         = getDeployedGroupVersionKinds
 	GetSecret                            = getSecret
 	ReadFiles                            = readFiles
+	GetClusterSummaryAnnotationValue     = getClusterSummaryAnnotationValue
 
 	AddExtraLabels      = addExtraLabels
 	AddExtraAnnotations = addExtraAnnotations
@@ -195,6 +196,9 @@ const (
 	HelmActionUpgrade   = upgrade
 	HelmActionDowngrade = downgrade
 	HelmActionUninstall = uninstall
+
+	ClusterSummaryAnnotation = clusterSummaryAnnotation
+	DeploymentTypeAnnotation = deploymentTypeAnnotation
 )
 
 var (

controllers/handlers_utils.go

@@ -65,6 +65,7 @@ import (
 
 const (
 	clusterSummaryAnnotation = "projectsveltos.io/clustersummary"
+	deploymentTypeAnnotation = "projectsveltos.io/deploymenttype"
 	subresourcesAnnotation   = "projectsveltos.io/subresources"
 	pathAnnotation           = "path"
 )
@@ -390,6 +391,29 @@ func applyPatches(ctx context.Context, clusterSummary *configv1beta1.ClusterSumm
 	return referencedUnstructured, nil
 }
 
+// addStaleResourceScopingAnnotations annotates policy with the information stale-resource
+// cleanup needs to scope its scan to resources this exact ClusterSummary/deploymentType pair
+// deployed, rather than to everything present in the cluster.
+//
+// Just setting (Cluster)Profile as OwnerReference is not enough: a SveltosCluster can be
+// self-managed (its remote client/config resolve back to the management cluster), in which
+// case the "clean the management cluster" and "clean the remote cluster" passes end up
+// scanning the same physical cluster. Without these annotations, each pass would consider the
+// other's just-deployed resources stale, since neither pass's desired-state set includes what
+// the other pass deployed.
+func addStaleResourceScopingAnnotations(policy *unstructured.Unstructured, deployingToMgmtCluster bool,
+	clusterSummary *configv1beta1.ClusterSummary) {
+
+	value := getClusterSummaryAnnotationValue(clusterSummary)
+	deployer.AddAnnotation(policy, clusterSummaryAnnotation, value)
+
+	deploymentTypeValue := string(configv1beta1.DeploymentTypeRemote)
+	if deployingToMgmtCluster {
+		deploymentTypeValue = string(configv1beta1.DeploymentTypeLocal)
+	}
+	deployer.AddAnnotation(policy, deploymentTypeAnnotation, deploymentTypeValue)
+}
+
 // deployUnstructured deploys referencedUnstructured objects.
 // Returns an error if one occurred. Otherwise it returns a slice containing the name of
 // the policies deployed in the form of kind.group:namespace:name for namespaced policies
@@ -484,15 +508,7 @@ func deployUnstructured(ctx context.Context, deployingToMgmtCluster bool, destCo
 		deployer.AddMetadata(policy, resourceInfo.GetResourceVersion(), profile,
 			clusterSummary.Spec.ClusterProfileSpec.ExtraLabels, clusterSummary.Spec.ClusterProfileSpec.ExtraAnnotations)
 
-		if deployingToMgmtCluster {
-			// When deploying resources in the management cluster, just setting (Cluster)Profile as OwnerReference is
-			// not enough. We also need to track which ClusterSummary is creating the resource. Otherwise while
-			// trying to clean stale resources those objects will be incorrectly removed.
-			// An extra annotation is added here to indicate the clustersummary, so the managed cluster, this
-			// resource was created for
-			value := getClusterSummaryAnnotationValue(clusterSummary)
-			deployer.AddAnnotation(policy, clusterSummaryAnnotation, value)
-		}
+		addStaleResourceScopingAnnotations(policy, deployingToMgmtCluster, clusterSummary)
 
 		if requeue {
 			if clusterSummary.Spec.ClusterProfileSpec.SyncMode != configv1beta1.SyncModeDryRun {
@@ -1110,14 +1126,42 @@ func processDeployedGVKs(ctx context.Context, isMgmtCluster bool, remoteConfig *
 
 		leavePolicies := isLeavePolicies(clusterSummary, logger)
 		isDryRun := clusterSummary.Spec.ClusterProfileSpec.SyncMode == configv1beta1.SyncModeDryRun
-		var skipAnnotationKey, skipAnnotationValue string
+		ownClusterSummaryValue := getClusterSummaryAnnotationValue(clusterSummary)
+
+		expectedDeploymentType := string(configv1beta1.DeploymentTypeRemote)
 		if isMgmtCluster {
-			skipAnnotationValue = getClusterSummaryAnnotationValue(clusterSummary)
-			skipAnnotationKey = clusterSummaryAnnotation
+			expectedDeploymentType = string(configv1beta1.DeploymentTypeLocal)
 		}
 
 		for j := range list.Items {
 			r := list.Items[j]
+
+			// Protective signal, same reasoning as the clustersummary annotation check below:
+			// skip a resource here when it is explicitly annotated for the *other* deployment
+			// type (relevant when this ClusterSummary's remote cluster is the self-managed
+			// management cluster: the Local and Remote cleanup passes then scan the same
+			// physical cluster, and each pass's currentPolicies only reflects its own half of
+			// what was just deployed, so without this check each pass would consider the
+			// other's resources stale). A resource missing the annotation predates it being
+			// set on every deploy and must still fall through to the checks below.
+			if v, ok := r.GetAnnotations()[deploymentTypeAnnotation]; ok && v != expectedDeploymentType {
+				continue
+			}
+
+			// Only rely on the clustersummary annotation as a protective signal: skip a
+			// resource here when it is explicitly annotated for a *different* ClusterSummary
+			// (relevant when this ClusterSummary's remote cluster is the self-managed
+			// management cluster, and another ClusterSummary deployed there via
+			// deploymentType: Local). A resource missing the annotation entirely predates it
+			// being set on every deploy, and must still fall through to the normal
+			// canDelete/isResourceOwner checks below, or upgraded ClusterSummaries would never
+			// detect their own pre-existing resources as stale again.
+			var skipAnnotationKey, skipAnnotationValue string
+			if _, ok := r.GetAnnotations()[clusterSummaryAnnotation]; ok {
+				skipAnnotationKey = clusterSummaryAnnotation
+				skipAnnotationValue = ownClusterSummaryValue
+			}
+
 			rr, err := deployer.UndeployStaleResource(ctx, skipAnnotationKey, skipAnnotationValue, localClient,
 				profile, leavePolicies, isDryRun, r, currentPolicies, logger)
 			if err != nil {

controllers/handlers_utils_test.go

@@ -1018,6 +1018,244 @@ var _ = Describe("HandlersUtils", func() {
 		}, timeout, pollingInterval).Should(BeTrue())
 	})
 
+	It(`undeployStaleResources scopes deletion to resources annotated for this ClusterSummary, even when isMgmtCluster is false`, func() {
+		// This reproduces the scenario of a self-managed SveltosCluster: this ClusterSummary's
+		// "remote" cluster is physically the same cluster other ClusterSummaries deploy Local
+		// resources into. The remote-cluster cleanup pass (isMgmtCluster=false) must still only
+		// ever touch resources this exact ClusterSummary created, identified via the
+		// clustersummary annotation, and must leave other ClusterSummaries' resources alone.
+		ownClusterRoleName := randomString()
+		ownClusterRole := &rbacv1.ClusterRole{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: ownClusterRoleName,
+				Labels: map[string]string{
+					deployer.ReasonLabel: string(libsveltosv1beta1.FeatureResources),
+				},
+				Annotations: map[string]string{
+					deployer.ReferenceKindAnnotation:      string(libsveltosv1beta1.ConfigMapReferencedResourceKind),
+					deployer.ReferenceNamespaceAnnotation: randomString(),
+					deployer.ReferenceNameAnnotation:      randomString(),
+					controllers.ClusterSummaryAnnotation:  controllers.GetClusterSummaryAnnotationValue(clusterSummary),
+				},
+			},
+		}
+
+		otherClusterRoleName := randomString()
+		otherClusterRole := &rbacv1.ClusterRole{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: otherClusterRoleName,
+				Labels: map[string]string{
+					deployer.ReasonLabel: string(libsveltosv1beta1.FeatureResources),
+				},
+				Annotations: map[string]string{
+					deployer.ReferenceKindAnnotation:      string(libsveltosv1beta1.ConfigMapReferencedResourceKind),
+					deployer.ReferenceNamespaceAnnotation: randomString(),
+					deployer.ReferenceNameAnnotation:      randomString(),
+					// Deployed (Local) by a different ClusterSummary for a different managed cluster.
+					controllers.ClusterSummaryAnnotation: randomString(),
+				},
+			},
+		}
+
+		// Simulates a resource deployed by an older addon-controller version, before the
+		// clustersummary annotation was set on every deploy. It must still be detected as
+		// stale and removed by this same ClusterSummary; otherwise upgrading would leave such
+		// resources undeletable forever.
+		legacyClusterRoleName := randomString()
+		legacyClusterRole := &rbacv1.ClusterRole{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: legacyClusterRoleName,
+				Labels: map[string]string{
+					deployer.ReasonLabel: string(libsveltosv1beta1.FeatureResources),
+				},
+				Annotations: map[string]string{
+					deployer.ReferenceKindAnnotation:      string(libsveltosv1beta1.ConfigMapReferencedResourceKind),
+					deployer.ReferenceNamespaceAnnotation: randomString(),
+					deployer.ReferenceNameAnnotation:      randomString(),
+				},
+			},
+		}
+
+		Expect(testEnv.Create(context.TODO(), ownClusterRole)).To(Succeed())
+		Expect(waitForObject(ctx, testEnv.Client, ownClusterRole)).To(Succeed())
+		Expect(testEnv.Create(context.TODO(), otherClusterRole)).To(Succeed())
+		Expect(waitForObject(ctx, testEnv.Client, otherClusterRole)).To(Succeed())
+		Expect(testEnv.Create(context.TODO(), legacyClusterRole)).To(Succeed())
+		Expect(waitForObject(ctx, testEnv.Client, legacyClusterRole)).To(Succeed())
+
+		currentClusterProfile := &configv1beta1.ClusterProfile{}
+		Expect(testEnv.Get(context.TODO(),
+			types.NamespacedName{Name: clusterProfile.Name},
+			currentClusterProfile)).To(Succeed())
+
+		addOwnerReference(context.TODO(), testEnv.Client, ownClusterRole, currentClusterProfile)
+		addOwnerReference(context.TODO(), testEnv.Client, otherClusterRole, currentClusterProfile)
+		addOwnerReference(context.TODO(), testEnv.Client, legacyClusterRole, currentClusterProfile)
+
+		currentClusterSummary := &configv1beta1.ClusterSummary{}
+		Expect(testEnv.Get(context.TODO(),
+			types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name},
+			currentClusterSummary)).To(Succeed())
+		currentClusterSummary.Status.FeatureSummaries = []configv1beta1.FeatureSummary{
+			{
+				FeatureID: libsveltosv1beta1.FeatureResources,
+				Status:    libsveltosv1beta1.FeatureStatusProvisioned,
+			},
+		}
+		currentClusterSummary.Status.DeployedGVKs = []libsveltosv1beta1.FeatureDeploymentInfo{
+			{
+				FeatureID: libsveltosv1beta1.FeatureResources,
+				DeployedGroupVersionKind: []string{
+					testClusterRoleKindV1,
+				},
+			},
+		}
+		Expect(testEnv.Status().Update(context.TODO(), currentClusterSummary)).To(Succeed())
+
+		deployedGKVs := controllers.GetDeployedGroupVersionKinds(currentClusterSummary, libsveltosv1beta1.FeatureResources)
+		Expect(deployedGKVs).ToNot(BeEmpty())
+
+		// None of the ClusterRoles is in currentPolicies (nil), so all are candidates for
+		// deletion were it not for the clustersummary-annotation scoping.
+		_, err := controllers.UndeployStaleResources(context.TODO(), false, testEnv.Config, testEnv.Client,
+			libsveltosv1beta1.FeatureResources, currentClusterSummary, deployedGKVs, nil,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+
+		// Own resource is no longer referenced: it must be removed.
+		Eventually(func() bool {
+			currentClusterRole := &rbacv1.ClusterRole{}
+			err = testEnv.Get(context.TODO(), types.NamespacedName{Name: ownClusterRoleName}, currentClusterRole)
+			return err != nil && apierrors.IsNotFound(err)
+		}, timeout, pollingInterval).Should(BeTrue())
+
+		// Legacy (pre-annotation) resource is no longer referenced either: it must still be
+		// detected as stale and removed.
+		Eventually(func() bool {
+			currentClusterRole := &rbacv1.ClusterRole{}
+			err = testEnv.Get(context.TODO(), types.NamespacedName{Name: legacyClusterRoleName}, currentClusterRole)
+			return err != nil && apierrors.IsNotFound(err)
+		}, timeout, pollingInterval).Should(BeTrue())
+
+		// Other ClusterSummary's resource must never be touched.
+		Consistently(func() error {
+			currentClusterRole := &rbacv1.ClusterRole{}
+			return testEnv.Get(context.TODO(), types.NamespacedName{Name: otherClusterRoleName}, currentClusterRole)
+		}, timeout, pollingInterval).Should(BeNil())
+	})
+
+	It(`undeployStaleResources does not remove resources deployed by the other deployment type of the same ClusterSummary`, func() {
+		// Further self-managed SveltosCluster scenario: this time both resources were deployed
+		// by the SAME ClusterSummary, one via deploymentType Local and one via deploymentType
+		// Remote. Because it is the same ClusterSummary, the clustersummary annotation alone
+		// cannot tell the two apart once the remote cluster is physically the management
+		// cluster. The deploymenttype annotation must protect the Local resource from the
+		// Remote cleanup pass, and the Remote resource from the Local cleanup pass.
+		localClusterRoleName := randomString()
+		localClusterRole := &rbacv1.ClusterRole{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: localClusterRoleName,
+				Labels: map[string]string{
+					deployer.ReasonLabel: string(libsveltosv1beta1.FeatureResources),
+				},
+				Annotations: map[string]string{
+					deployer.ReferenceKindAnnotation:      string(libsveltosv1beta1.ConfigMapReferencedResourceKind),
+					deployer.ReferenceNamespaceAnnotation: randomString(),
+					deployer.ReferenceNameAnnotation:      randomString(),
+					controllers.ClusterSummaryAnnotation:  controllers.GetClusterSummaryAnnotationValue(clusterSummary),
+					controllers.DeploymentTypeAnnotation:  string(configv1beta1.DeploymentTypeLocal),
+				},
+			},
+		}
+
+		remoteClusterRoleName := randomString()
+		remoteClusterRole := &rbacv1.ClusterRole{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: remoteClusterRoleName,
+				Labels: map[string]string{
+					deployer.ReasonLabel: string(libsveltosv1beta1.FeatureResources),
+				},
+				Annotations: map[string]string{
+					deployer.ReferenceKindAnnotation:      string(libsveltosv1beta1.ConfigMapReferencedResourceKind),
+					deployer.ReferenceNamespaceAnnotation: randomString(),
+					deployer.ReferenceNameAnnotation:      randomString(),
+					controllers.ClusterSummaryAnnotation:  controllers.GetClusterSummaryAnnotationValue(clusterSummary),
+					controllers.DeploymentTypeAnnotation:  string(configv1beta1.DeploymentTypeRemote),
+				},
+			},
+		}
+
+		Expect(testEnv.Create(context.TODO(), localClusterRole)).To(Succeed())
+		Expect(waitForObject(ctx, testEnv.Client, localClusterRole)).To(Succeed())
+		Expect(testEnv.Create(context.TODO(), remoteClusterRole)).To(Succeed())
+		Expect(waitForObject(ctx, testEnv.Client, remoteClusterRole)).To(Succeed())
+
+		currentClusterProfile := &configv1beta1.ClusterProfile{}
+		Expect(testEnv.Get(context.TODO(),
+			types.NamespacedName{Name: clusterProfile.Name},
+			currentClusterProfile)).To(Succeed())
+
+		addOwnerReference(context.TODO(), testEnv.Client, localClusterRole, currentClusterProfile)
+		addOwnerReference(context.TODO(), testEnv.Client, remoteClusterRole, currentClusterProfile)
+
+		currentClusterSummary := &configv1beta1.ClusterSummary{}
+		Expect(testEnv.Get(context.TODO(),
+			types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name},
+			currentClusterSummary)).To(Succeed())
+		currentClusterSummary.Status.FeatureSummaries = []configv1beta1.FeatureSummary{
+			{
+				FeatureID: libsveltosv1beta1.FeatureResources,
+				Status:    libsveltosv1beta1.FeatureStatusProvisioned,
+			},
+		}
+		currentClusterSummary.Status.DeployedGVKs = []libsveltosv1beta1.FeatureDeploymentInfo{
+			{
+				FeatureID: libsveltosv1beta1.FeatureResources,
+				DeployedGroupVersionKind: []string{
+					testClusterRoleKindV1,
+				},
+			},
+		}
+		Expect(testEnv.Status().Update(context.TODO(), currentClusterSummary)).To(Succeed())
+
+		deployedGKVs := controllers.GetDeployedGroupVersionKinds(currentClusterSummary, libsveltosv1beta1.FeatureResources)
+		Expect(deployedGKVs).ToNot(BeEmpty())
+
+		// Neither ClusterRole is in currentPolicies (nil), so both are candidates for deletion
+		// were it not for the deploymenttype-annotation scoping.
+
+		// isMgmtCluster=false simulates the Remote cleanup pass: it must remove the
+		// Remote-deployed resource, but leave the Local-deployed one alone.
+		_, err := controllers.UndeployStaleResources(context.TODO(), false, testEnv.Config, testEnv.Client,
+			libsveltosv1beta1.FeatureResources, currentClusterSummary, deployedGKVs, nil,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+
+		Eventually(func() bool {
+			currentClusterRole := &rbacv1.ClusterRole{}
+			err = testEnv.Get(context.TODO(), types.NamespacedName{Name: remoteClusterRoleName}, currentClusterRole)
+			return err != nil && apierrors.IsNotFound(err)
+		}, timeout, pollingInterval).Should(BeTrue())
+
+		Consistently(func() error {
+			currentClusterRole := &rbacv1.ClusterRole{}
+			return testEnv.Get(context.TODO(), types.NamespacedName{Name: localClusterRoleName}, currentClusterRole)
+		}, timeout, pollingInterval).Should(BeNil())
+
+		// isMgmtCluster=true simulates the Local cleanup pass: it must now remove the
+		// Local-deployed resource.
+		_, err = controllers.UndeployStaleResources(context.TODO(), true, testEnv.Config, testEnv.Client,
+			libsveltosv1beta1.FeatureResources, currentClusterSummary, deployedGKVs, nil,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+
+		Eventually(func() bool {
+			currentClusterRole := &rbacv1.ClusterRole{}
+			err = testEnv.Get(context.TODO(), types.NamespacedName{Name: localClusterRoleName}, currentClusterRole)
+			return err != nil && apierrors.IsNotFound(err)
+		}, timeout, pollingInterval).Should(BeTrue())
+	})
+
 	It("addExtraLabels adds extra labels on unstructured", func() {
 		u := &unstructured.Unstructured{}
 		extraLabels := map[string]string{