diff --git a/controllers/classifier_controller.go b/controllers/classifier_controller.go index eaba356..9931dba 100644 --- a/controllers/classifier_controller.go +++ b/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) diff --git a/controllers/classifier_controller_test.go b/controllers/classifier_controller_test.go index a3d1673..97ba08c 100644 --- a/controllers/classifier_controller_test.go +++ b/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() diff --git a/controllers/export_test.go b/controllers/export_test.go index cc6d08d..a128394 100644 --- a/controllers/export_test.go +++ b/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 ( diff --git a/test/fv/labels_test.go b/test/fv/labels_test.go index 9997639..52dce33 100644 --- a/test/fv/labels_test.go +++ b/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) +} diff --git a/test/fv/utils_test.go b/test/fv/utils_test.go index 9a64b9e..879f386 100644 --- a/test/fv/utils_test.go +++ b/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()