fix: remove a label key dropped from spec.classifierLabels

controllers/classifier_controller.go

@@ -556,6 +556,7 @@ func (r *ClassifierReconciler) cleanUpManagedResources(ctx context.Context,
 	}
 
 	oldMatchingClusters := make(map[corev1.ObjectReference]bool)
+	oldManagedLabels := make(map[corev1.ObjectReference][]string)
 	for i := range reports {
 		report := &reports[i]
 		if report.Spec.ClusterNamespace == "" {
@@ -564,9 +565,20 @@ func (r *ClassifierReconciler) cleanUpManagedResources(ctx context.Context,
 		if len(report.Status.ManagedLabels) > 0 || len(report.Status.UnManagedLabels) > 0 {
 			cluster := getClusterRefFromClassifierReport(report)
 			oldMatchingClusters[*cluster] = true
+			oldManagedLabels[*cluster] = report.Status.ManagedLabels
 		}
 	}
 
+	// A key can be dropped from spec.classifierLabels while a cluster still matches. Remove it
+	// from that cluster now, while this Classifier still owns the key's registration: once
+	// registerMatchingClusters (called right after this, for every still-matching cluster) drops
+	// stale registrations, CanManageLabel would refuse the removal and the key would be stranded.
+	err = r.removeStaleLabelsFromMatchingClusters(ctx, classifierScope.Classifier,
+		matchingClusters, oldManagedLabels, logger)
+	if err != nil {
+		return fmt.Errorf("failed to remove stale labels from matching clusters: %w", err)
+	}
+
 	err = r.cleanUpNonMatchingClusters(ctx, classifierScope.Classifier,
 		matchingClusters, oldMatchingClusters, logger)
 	if err != nil {
@@ -576,6 +588,58 @@ func (r *ClassifierReconciler) cleanUpManagedResources(ctx context.Context,
 	return nil
 }
 
+// removeStaleLabelsFromMatchingClusters removes, from each still-matching cluster, any label key
+// this Classifier managed on a previous reconcile (per oldManagedLabels, sourced from
+// ClassifierReport.Status.ManagedLabels) that is no longer present in classifier.Spec.ClassifierLabels.
+// Without this, a key dropped from spec while a cluster keeps matching is never revisited: the next
+// steps in the reconcile only add/update keys currently in spec, and registerMatchingClusters drops
+// this Classifier's ownership of the dropped key, so no later pass could remove it either.
+func (r *ClassifierReconciler) removeStaleLabelsFromMatchingClusters(ctx context.Context,
+	classifier *libsveltosv1beta1.Classifier, matchingClusters map[corev1.ObjectReference]bool,
+	oldManagedLabels map[corev1.ObjectReference][]string, logger logr.Logger) error {
+
+	currentKeys := make(map[string]bool, len(classifier.Spec.ClassifierLabels))
+	for i := range classifier.Spec.ClassifierLabels {
+		currentKeys[classifier.Spec.ClassifierLabels[i].Key] = true
+	}
+
+	var errs []error
+	for c := range matchingClusters {
+		staleKeys := make([]string, 0)
+		for _, key := range oldManagedLabels[c] {
+			if !currentKeys[key] {
+				staleKeys = append(staleKeys, key)
+			}
+		}
+		if len(staleKeys) == 0 {
+			continue
+		}
+
+		clusterType := clusterproxy.GetClusterType(&c)
+		clusterLogger := logger.WithValues("cluster", fmt.Sprintf("%s/%s", c.Namespace, c.Name))
+
+		cluster, err := clusterproxy.GetCluster(ctx, r.Client, c.Namespace, c.Name, clusterType)
+		if err != nil {
+			if apierrors.IsNotFound(err) {
+				continue // Cluster is gone; cleanUpNonMatchingClusters/next reconcile will handle it
+			}
+			wrappedErr := fmt.Errorf("failed to get cluster %s/%s to remove stale labels: %w", c.Namespace, c.Name, err)
+			clusterLogger.Error(wrappedErr, "lookup failed, skipping stale label removal")
+			errs = append(errs, wrappedErr)
+			continue
+		}
+
+		if err := r.removeLabelsFromCluster(ctx, classifier, cluster, clusterType, staleKeys, clusterLogger); err != nil {
+			wrappedErr := fmt.Errorf("failed to remove stale labels %v from cluster %s/%s: %w",
+				staleKeys, c.Namespace, c.Name, err)
+			clusterLogger.Error(wrappedErr, "stale label removal failed")
+			errs = append(errs, wrappedErr)
+		}
+	}
+
+	return errors.Join(errs...)
+}
+
 func (r *ClassifierReconciler) updateClassifierSet(classifierScope *scope.ClassifierScope, hasUnManaged bool) {
 	r.Mux.Lock()
 	defer r.Mux.Unlock()
@@ -624,12 +688,11 @@ func (r *ClassifierReconciler) updateLabelsOnMatchingClusters(ctx context.Contex
 	return errors.Join(errs...)
 }
 
-// removeLabelsFromCluster removes all labels from the provided cluster that were
-// specifically managed by this Classifier instance. It uses the KeyManager
-// to ensure it only deletes labels it has permission to manage.
+// removeLabelsFromCluster removes, from the provided cluster, every key in keysToConsider that
+// this specific Classifier instance is authorized to manage (per the KeyManager).
 func (r *ClassifierReconciler) removeLabelsFromCluster(ctx context.Context,
 	classifier *libsveltosv1beta1.Classifier, cluster client.Object, clusterType libsveltosv1beta1.ClusterType,
-	logger logr.Logger) error {
+	keysToConsider []string, logger logr.Logger) error {
 
 	manager, err := keymanager.GetKeyManagerInstance(ctx, r.Client)
 	if err != nil {
@@ -643,15 +706,11 @@ func (r *ClassifierReconciler) removeLabelsFromCluster(ctx context.Context,
 	}
 
 	labelsChanged := false
-	for i := range classifier.Spec.ClassifierLabels {
-		label := classifier.Spec.ClassifierLabels[i]
-
+	for _, key := range keysToConsider {
 		// Only remove the label if this specific Classifier is authorized to manage it
-		if manager.CanManageLabel(classifier, cluster.GetNamespace(), cluster.GetName(), label.Key,
-			clusterType) {
-
-			if _, exists := labels[label.Key]; exists {
-				delete(labels, label.Key)
+		if manager.CanManageLabel(classifier, cluster.GetNamespace(), cluster.GetName(), key, clusterType) {
+			if _, exists := labels[key]; exists {
+				delete(labels, key)
 				labelsChanged = true
 			}
 		}
@@ -786,7 +845,7 @@ func (r *ClassifierReconciler) removeLabelsFromClusters(ctx context.Context,
 		clusterLogger.V(logs.LogDebug).Info("removing managed labels from cluster")
 
 		if err := r.removeLabelsFromCluster(ctx, classifierScope.Classifier, cluster,
-			clusterproxy.GetClusterType(ref), clusterLogger); err != nil {
+			clusterproxy.GetClusterType(ref), report.Status.ManagedLabels, clusterLogger); err != nil {
 			clusterLogger.Error(err, "failed to remove labels")
 			errs = append(errs, err)
 		}
@@ -887,8 +946,11 @@ func (r *ClassifierReconciler) cleanUpNonMatchingClusters(ctx context.Context,
 		}
 
 		// 2. Attempt to remove the managed labels
-		// Passing the classifier object as the owner to removeLabelsFromCluster
-		if err := r.removeLabelsFromCluster(ctx, classifier, cluster, clusterType, clusterLogger); err != nil {
+		keysToConsider := make([]string, len(classifier.Spec.ClassifierLabels))
+		for i := range classifier.Spec.ClassifierLabels {
+			keysToConsider[i] = classifier.Spec.ClassifierLabels[i].Key
+		}
+		if err := r.removeLabelsFromCluster(ctx, classifier, cluster, clusterType, keysToConsider, clusterLogger); err != nil {
 			wrappedErr := fmt.Errorf("failed to remove labels from cluster %s/%s: %w", c.Namespace, c.Name, err)
 			clusterLogger.Error(wrappedErr, "label removal failed, skipping registration removal")
 			errs = append(errs, wrappedErr)

controllers/classifier_controller_test.go

@@ -610,6 +610,74 @@ var _ = Describe("Classifier: Reconciler", func() {
 		Expect(updatedReport.Status.ManagedLabels).To(BeEmpty())
 	})
 
+	It("removeStaleLabelsFromMatchingClusters removes a key dropped from spec.classifierLabels while the cluster still matches", func() {
+		keptKey := randomString()
+		keptValue := randomString()
+		droppedKey := randomString()
+		droppedValue := randomString()
+		clusterNamespace := randomString()
+		clusterName := randomString()
+
+		// Simulate that the key was already removed from spec.classifierLabels: only keptKey remains.
+		classifier.Spec.ClassifierLabels = []libsveltosv1beta1.ClassifierLabel{
+			{Key: keptKey, Value: keptValue},
+		}
+
+		cluster := &libsveltosv1beta1.SveltosCluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: clusterNamespace,
+				Name:      clusterName,
+				Labels: map[string]string{
+					keptKey:    keptValue,
+					droppedKey: droppedValue,
+				},
+			},
+		}
+
+		initObjects := []client.Object{classifier, cluster}
+		c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(initObjects...).Build()
+
+		// Both keys were managed on a previous reconcile, before droppedKey left the spec.
+		manager, err := keymanager.GetKeyManagerInstance(context.TODO(), c)
+		Expect(err).To(BeNil())
+		manager.RegisterClassifierForLabels(classifier, clusterNamespace, clusterName, libsveltosv1beta1.ClusterTypeSveltos)
+		classifier.Spec.ClassifierLabels = append(classifier.Spec.ClassifierLabels,
+			libsveltosv1beta1.ClassifierLabel{Key: droppedKey, Value: droppedValue})
+		manager.RegisterClassifierForLabels(classifier, clusterNamespace, clusterName, libsveltosv1beta1.ClusterTypeSveltos)
+		// Restore spec to reflect droppedKey no longer being declared.
+		classifier.Spec.ClassifierLabels = []libsveltosv1beta1.ClassifierLabel{
+			{Key: keptKey, Value: keptValue},
+		}
+
+		reconciler := &controllers.ClassifierReconciler{
+			Client:        c,
+			Scheme:        scheme,
+			ClusterMap:    make(map[corev1.ObjectReference]*libsveltosset.Set),
+			ClassifierMap: make(map[corev1.ObjectReference]*libsveltosset.Set),
+			Mux:           sync.Mutex{},
+			Logger:        logger,
+		}
+
+		clusterRef := corev1.ObjectReference{
+			Kind:       libsveltosv1beta1.SveltosClusterKind,
+			APIVersion: libsveltosv1beta1.GroupVersion.String(),
+			Namespace:  clusterNamespace,
+			Name:       clusterName,
+		}
+		matchingClusters := map[corev1.ObjectReference]bool{clusterRef: true}
+		oldManagedLabels := map[corev1.ObjectReference][]string{clusterRef: {keptKey, droppedKey}}
+
+		err = controllers.RemoveStaleLabelsFromMatchingClusters(reconciler, context.TODO(), classifier,
+			matchingClusters, oldManagedLabels, logger)
+		Expect(err).ToNot(HaveOccurred())
+
+		currentCluster := &libsveltosv1beta1.SveltosCluster{}
+		Expect(c.Get(context.TODO(),
+			client.ObjectKey{Namespace: clusterNamespace, Name: clusterName}, currentCluster)).To(Succeed())
+		Expect(currentCluster.Labels).ToNot(HaveKey(droppedKey))
+		Expect(currentCluster.Labels).To(HaveKeyWithValue(keptKey, keptValue))
+	})
+
 	It("classifyLabels divides labels in managed and unmanaged", func() {
 		clusterNamespace := randomString()
 		clusterName := randomString()

controllers/export_test.go

@@ -86,6 +86,8 @@ var (
 	RemoveAllRegistrations                 = (*ClassifierReconciler).removeAllRegistrations
 	ClassifyLabels                         = (*ClassifierReconciler).classifyLabels
 	CleanUpNonMatchingClusters             = (*ClassifierReconciler).cleanUpNonMatchingClusters
+	RemoveStaleLabelsFromMatchingClusters  = (*ClassifierReconciler).removeStaleLabelsFromMatchingClusters
+	CleanUpManagedResources                = (*ClassifierReconciler).cleanUpManagedResources
 )
 
 var (

test/fv/labels_test.go

@@ -37,6 +37,11 @@ var _ = Describe("Classifier: update cluster labels", func() {
 	It("Cluster labels are updated", Label("FV", "PULLMODE"), func() {
 		verifyFlow(namePrefix)
 	})
+
+	It("A key removed from spec.classifierLabels is removed from a still-matching cluster",
+		Label("FV", "PULLMODE"), func() {
+			verifyDroppedLabelKeyFlow(namePrefix)
+		})
 })
 
 func verifyFlow(namePrefix string) {
@@ -129,3 +134,50 @@ func verifyFlow(namePrefix string) {
 
 	removeLabels(classifier)
 }
+
+// verifyDroppedLabelKeyFlow covers https://github.com/projectsveltos/classifier/issues/513:
+// a key removed from spec.classifierLabels, while the cluster keeps matching, must be removed
+// from the cluster right away, not left stranded with no owner.
+func verifyDroppedLabelKeyFlow(namePrefix string) {
+	keptKey, keptValue := randomString(), randomString()
+	droppedKey, droppedValue := randomString(), randomString()
+	clusterLabels := map[string]string{keptKey: keptValue, droppedKey: droppedValue}
+	classifier := getClassifier(namePrefix, clusterLabels)
+
+	Byf("Creating classifier instance %s in the management cluster", classifier.Name)
+	Expect(k8sClient.Create(context.TODO(), classifier)).To(Succeed())
+
+	verifyClassfierIsProvisioned(classifier)
+	verifyClassifierReport(classifier.Name, true)
+	verifyClusterLabels(classifier)
+	verifyClassifierReportManagedLabels(classifier)
+
+	Byf("Removing key %s from spec.classifierLabels, leaving the cluster still matching", droppedKey)
+	currentClassifier := &libsveltosv1beta1.Classifier{}
+	Expect(k8sClient.Get(context.TODO(), types.NamespacedName{Name: classifier.Name},
+		currentClassifier)).To(Succeed())
+	currentClassifier.Spec.ClassifierLabels = []libsveltosv1beta1.ClassifierLabel{
+		{Key: keptKey, Value: keptValue},
+	}
+	Expect(k8sClient.Update(context.TODO(), currentClassifier)).To(Succeed())
+
+	Byf("Verifying cluster is still a match")
+	verifyClassifierReport(classifier.Name, true)
+
+	verifyClusterLabelKeyIsGone(droppedKey)
+	verifyClusterLabels(currentClassifier)
+	verifyClassifierReportManagedLabels(currentClassifier)
+
+	Byf("Deleting classifier instance %s in the management cluster", classifier.Name)
+	Expect(k8sClient.Delete(context.TODO(), currentClassifier)).To(Succeed())
+
+	Byf("Verifying Classifier instance is removed from the management cluster")
+	Eventually(func() bool {
+		instance := &libsveltosv1beta1.Classifier{}
+		err := k8sClient.Get(context.TODO(), types.NamespacedName{Name: classifier.Name}, instance)
+		return err != nil && apierrors.IsNotFound(err)
+	}, timeout, pollingInterval).Should(BeTrue())
+
+	verifyClusterLabelsAreGone(currentClassifier)
+	removeLabels(currentClassifier)
+}

test/fv/utils_test.go

@@ -264,6 +264,22 @@ func verifyClusterLabelsAreGone(classifier *libsveltosv1beta1.Classifier) {
 	}, timeout, pollingInterval).Should(BeTrue())
 }
 
+// verifyClusterLabelKeyIsGone verifies a single label key is absent from the cluster, regardless
+// of whether that key is still present in classifier.Spec.ClassifierLabels. Unlike
+// verifyClusterLabelsAreGone (which only checks keys currently in spec), this covers a key that
+// was already removed from spec.
+func verifyClusterLabelKeyIsGone(key string) {
+	Byf("Verifying label key %s is removed from cluster", key)
+	Eventually(func() bool {
+		currentCuster, err := getCluster()
+		if err != nil {
+			return false
+		}
+		_, ok := currentCuster.GetLabels()[key]
+		return !ok
+	}, timeout, pollingInterval).Should(BeTrue())
+}
+
 func removeLabels(classifier *libsveltosv1beta1.Classifier) {
 	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
 		currentCluster, err := getCluster()