From ce8e9ac1ed217ae4b87313100627156393977286 Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Mon, 19 Jan 2026 21:56:41 +0100 Subject: [PATCH] Clean up managed labels when clusters no longer match Implements label cleanup logic for the Classifier controller. When a cluster is no longer a match for a Classifier instance, or when the Classifier itself is deleted, the controller now removes all labels it previously managed on those clusters. This ensures no stale metadata remains on the underlying cluster resources. The removal logic is guarded by the KeyManager to prevent the accidental deletion of labels managed by other Classifier instances. --- controllers/classifier_controller.go | 370 ++++++++++++++++------ controllers/classifier_controller_test.go | 133 ++++++-- controllers/classifier_predicates.go | 2 +- controllers/export_test.go | 3 +- test/fv/conflict_test.go | 2 +- test/fv/labels_test.go | 24 +- test/fv/report_match_change_test.go | 4 +- test/fv/utils_test.go | 23 +- 8 files changed, 432 insertions(+), 129 deletions(-) diff --git a/controllers/classifier_controller.go b/controllers/classifier_controller.go index 8f9085f..bb336bf 100644 --- a/controllers/classifier_controller.go +++ b/controllers/classifier_controller.go @@ -18,12 +18,12 @@ package controllers import ( "context" + "errors" "fmt" "sync" "time" "github.com/go-logr/logr" - "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -150,11 +150,10 @@ func (r *ClassifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) if apierrors.IsNotFound(err) { return reconcile.Result{}, nil } - logger.Error(err, "Failed to fetch Classifier") - return reconcile.Result{}, errors.Wrapf( - err, - "Failed to fetch Classifier %s", - req.NamespacedName, + logger.Error(err, "failed to fetch Classifier") + return reconcile.Result{}, fmt.Errorf( + "failed to fetch Classifier %s: %w", + req.NamespacedName, err, ) } @@ -167,11 +166,10 @@ func (r *ClassifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) ControllerName: "classifier", }) if err != nil { - logger.Error(err, "Failed to create classifierScope") - return reconcile.Result{}, errors.Wrapf( - err, - "unable to create classifier scope for %s", - req.NamespacedName, + logger.Error(err, "failed to create classifierScope") + return reconcile.Result{}, fmt.Errorf( + "unable to create classifier scope for %s: %w", + req.NamespacedName, err, ) } @@ -200,7 +198,13 @@ func (r *ClassifierReconciler) reconcileDelete( logger := classifierScope.Logger logger.V(logs.LogDebug).Info("Reconciling Classifier delete") - err := r.removeAllRegistrations(ctx, classifierScope, logger) + err := r.removeLabelsFromClusters(ctx, classifierScope, logger) + if err != nil { + logger.V(logs.LogInfo).Error(err, "failed to remove managed labels from clusters") + return reconcile.Result{}, err + } + + err = r.removeAllRegistrations(ctx, classifierScope, logger) if err != nil { logger.V(logs.LogInfo).Error(err, "failed to clear Classifier label registrations") return reconcile.Result{}, err @@ -270,21 +274,38 @@ func (r *ClassifierReconciler) reconcileNormal( } } - err := r.updateMatchingClustersAndRegistrations(ctx, classifierScope, logger) + // get list of clusters currently matching this Classifier instance + matchingClusters, err := r.getMatchingClusters(ctx, classifierScope, logger) if err != nil { - logger.V(logs.LogDebug).Info("failed to update matchingClusterRefs") + logger.V(logs.LogDebug).Error(err, "failed to get matching clusters") return reconcile.Result{Requeue: true, RequeueAfter: normalRequeueAfter}, nil } + // For clusters that no longer match, remove managed labels and clear internal registrations + err = r.cleanUpManagedResources(ctx, classifierScope, matchingClusters, logger) + if err != nil { + // Use Error level because this indicates a failure to clean up resources + logger.V(logs.LogDebug).Error(err, "failed to clean up labels/registrations for non-matching clusters") + return reconcile.Result{Requeue: true, RequeueAfter: normalRequeueAfter}, nil + } + + // For every currently matching cluster, update label ownership registrations + err = r.updateMatchingClustersAndRegistrations(ctx, classifierScope, matchingClusters, logger) + if err != nil { + logger.V(logs.LogDebug).Error(err, "failed to update status/registrations for matching clusters") + return reconcile.Result{Requeue: true, RequeueAfter: normalRequeueAfter}, nil + } + + // Apply the labels to the clusters err = r.updateLabelsOnMatchingClusters(ctx, classifierScope, logger) if err != nil { - logger.V(logs.LogDebug).Info("failed to update cluster labels") + logger.V(logs.LogDebug).Error(err, "failed to apply labels to matching clusters") return reconcile.Result{Requeue: true, RequeueAfter: normalRequeueAfter}, nil } err = r.updateClusterInfo(ctx, classifierScope) if err != nil { - logger.V(logs.LogDebug).Info("failed to update clusterInfo") + logger.V(logs.LogDebug).Error(err, "failed to update clusterInfo") return reconcile.Result{Requeue: true, RequeueAfter: normalRequeueAfter}, nil } @@ -344,7 +365,7 @@ func (r *ClassifierReconciler) SetupWithManager(ctx context.Context, ). Build(r) if err != nil { - return nil, errors.Wrap(err, "error creating controller") + return nil, fmt.Errorf("error creating controller: %w", err) } // At this point we don't know yet whether CAPI is present in the cluster. @@ -391,11 +412,10 @@ func (r *ClassifierReconciler) addFinalizer(ctx context.Context, classifierScope controllerutil.AddFinalizer(classifierScope.Classifier, libsveltosv1beta1.ClassifierFinalizer) // Register the finalizer immediately to avoid orphaning clusterprofile resources on delete if err := classifierScope.PatchObject(ctx); err != nil { - classifierScope.Error(err, "Failed to add finalizer") - return errors.Wrapf( - err, - "Failed to add finalizer for %s", - classifierScope.Name(), + classifierScope.Error(err, "failed to add finalizer") + return fmt.Errorf( + "failed to add finalizer for %s: %w", + classifierScope.Name(), err, ) } return nil @@ -439,11 +459,10 @@ func (r *ClassifierReconciler) updateClusterInfo(ctx context.Context, classifier return nil } -// updateMatchingClustersAndRegistrations does two things: -// - updates Classifier Status.MachingClusterStatuses -// - update label key registration with keymanager instance -func (r *ClassifierReconciler) updateMatchingClustersAndRegistrations(ctx context.Context, - classifierScope *scope.ClassifierScope, logger logr.Logger) error { +// getMatchingClusters returns the set of clusters currently matching the Classifier +// based on the existing ClassifierReport resources. +func (r *ClassifierReconciler) getMatchingClusters(ctx context.Context, + classifierScope *scope.ClassifierScope, logger logr.Logger) (map[corev1.ObjectReference]bool, error) { listOptions := []client.ListOption{ client.MatchingLabels{ @@ -452,75 +471,90 @@ func (r *ClassifierReconciler) updateMatchingClustersAndRegistrations(ctx contex } classifierReportList := &libsveltosv1beta1.ClassifierReportList{} - err := r.List(ctx, classifierReportList, listOptions...) - if err != nil { - logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to list ClassifierReports. Err: %v", err)) - return err + if err := r.List(ctx, classifierReportList, listOptions...); err != nil { + logger.V(logs.LogInfo).Error(err, "failed to list ClassifierReports") + return nil, err } - logger.V(logs.LogDebug).Info(fmt.Sprintf("found %d ClassifierReports for this Classifier instance", - len(classifierReportList.Items))) - - // create map of current matching clusters currentMatchingClusters := make(map[corev1.ObjectReference]bool) for i := range classifierReportList.Items { report := &classifierReportList.Items[i] - // If Sveltos is managing the management cluster as well, - // there will be two types of ClassifierReports: - // 1. created by sveltos-agent running in the management cluster. - // Those will have Spec.ClusterNamespace not set - // 2. pulled by classifier or pushed by sveltos-agent running - // in the managed cluster. Those will have Spec.ClusterNamespace set - // Consider only type #2 + + // Only consider reports with ClusterNamespace set (type #2 in your logic) if report.Spec.ClusterNamespace == "" { continue } + if report.Spec.Match { cluster := getClusterRefFromClassifierReport(report) - l := logger.WithValues("cluster", fmt.Sprintf("type: %s cluster %s/%s", report.Spec.ClusterType, cluster.Namespace, cluster.Name)) - l.V(logs.LogDebug).Info("is a match") currentMatchingClusters[*cluster] = true } } - // create map of old matching clusters - oldMatchingClusters := make(map[corev1.ObjectReference]bool) - for i := range classifierScope.Classifier.Status.MachingClusterStatuses { - ref := classifierScope.Classifier.Status.MachingClusterStatuses[i] - oldMatchingClusters[ref.ClusterRef] = true - } + return currentMatchingClusters, nil +} - err = r.handleLabelRegistrations(ctx, classifierScope.Classifier, currentMatchingClusters, - oldMatchingClusters, logger) - if err != nil { - return err +// updateMatchingClustersAndRegistrations synchronizes the KeyManager registrations +// and updates the Classifier's Status with the current set of matching clusters. +func (r *ClassifierReconciler) updateMatchingClustersAndRegistrations(ctx context.Context, + classifierScope *scope.ClassifierScope, matchingClusters map[corev1.ObjectReference]bool, + logger logr.Logger) error { + + // Sync registrations first so classifyLabels knows what we are allowed to manage + if err := r.registerMatchingClusters(ctx, classifierScope.Classifier, matchingClusters, logger); err != nil { + return fmt.Errorf("failed to register labels with keymanager: %w", err) } - matchingClusterStatus := make([]libsveltosv1beta1.MachingClusterStatus, len(currentMatchingClusters)) - i := 0 - unManaged := 0 - for c := range currentMatchingClusters { - tmpManaged, tmpUnmanaged, err := r.classifyLabels(ctx, classifierScope.Classifier, &c, logger) + matchingClusterStatus := make([]libsveltosv1beta1.MachingClusterStatus, 0, len(matchingClusters)) + var hasUnmanaged bool + + for c := range matchingClusters { + managed, unmanaged, err := r.classifyLabels(ctx, classifierScope.Classifier, &c, logger) if err != nil { - return err + return fmt.Errorf("failed to classify labels for cluster %s/%s: %w", c.Namespace, c.Name, err) } - unManaged += len(tmpUnmanaged) - matchingClusterStatus[i] = - libsveltosv1beta1.MachingClusterStatus{ - ClusterRef: c, - ManagedLabels: tmpManaged, - UnManagedLabels: tmpUnmanaged, - } - i++ - } - r.updateClassifierSet(classifierScope, unManaged != 0) + if len(unmanaged) > 0 { + hasUnmanaged = true + } + matchingClusterStatus = append(matchingClusterStatus, libsveltosv1beta1.MachingClusterStatus{ + ClusterRef: c, + ManagedLabels: managed, + UnManagedLabels: unmanaged, + }) + } + + // updateClassifierSet likely updates a boolean if any cluster has label conflicts + r.updateClassifierSet(classifierScope, hasUnmanaged) classifierScope.SetMachingClusterStatuses(matchingClusterStatus) return nil } +func (r *ClassifierReconciler) cleanUpManagedResources(ctx context.Context, + classifierScope *scope.ClassifierScope, matchingClusters map[corev1.ObjectReference]bool, + logger logr.Logger) error { + + // create map of old matching clusters from the Classifier Status + oldMatchingClusters := make(map[corev1.ObjectReference]bool) + for i := range classifierScope.Classifier.Status.MachingClusterStatuses { + ref := classifierScope.Classifier.Status.MachingClusterStatuses[i] + oldMatchingClusters[ref.ClusterRef] = true + } + + // Identify and clean up clusters that are no longer matches. + // This will remove managed labels from the clusters and, if successful, + // clear the registrations from the KeyManager. + err := r.cleanUpNonMatchingClusters(ctx, classifierScope.Classifier, + matchingClusters, oldMatchingClusters, logger) + if err != nil { + return fmt.Errorf("failed to clean up non-matching clusters: %w", err) + } + + return nil +} + func (r *ClassifierReconciler) updateClassifierSet(classifierScope *scope.ClassifierScope, hasUnManaged bool) { r.Mux.Lock() defer r.Mux.Unlock() @@ -535,37 +569,80 @@ func (r *ClassifierReconciler) updateClassifierSet(classifierScope *scope.Classi r.AllClassifierSet.Insert(classifierInfo) } -// updateLabelsOnMatchingClusters set labels on all matching clusters (only for clusters -// for which permission is granted by keymanager) +// updateLabelsOnMatchingClusters applies the desired labels to all clusters +// currently matching the Classifier, skipping clusters that are not found. func (r *ClassifierReconciler) updateLabelsOnMatchingClusters(ctx context.Context, classifierScope *scope.ClassifierScope, logger logr.Logger) error { - // Register Classifier instance as wanting to manage any labels in ClassifierLabels - // for all the clusters currently matching - for i := range classifierScope.Classifier.Status.MachingClusterStatuses { - ref := &classifierScope.Classifier.Status.MachingClusterStatuses[i].ClusterRef + var errs []error + statuses := classifierScope.Classifier.Status.MachingClusterStatuses + + for i := range statuses { + ref := &statuses[i].ClusterRef + cluster, err := clusterproxy.GetCluster(ctx, r.Client, ref.Namespace, ref.Name, clusterproxy.GetClusterType(ref)) if err != nil { if apierrors.IsNotFound(err) { - continue + continue // Cluster was likely deleted; cleanUpManagedResources will handle it next time } - logger.V(logs.LogInfo).Error(err, fmt.Sprintf("failed to get cluster %s/%s", ref.Namespace, ref.Name)) - return err + logger.Error(err, "failed to fetch cluster for label update", "cluster", ref.Name) + errs = append(errs, err) + continue } - l := logger.WithValues("cluster", fmt.Sprintf("%s/%s", cluster.GetNamespace(), cluster.GetName())) - l.V(logs.LogDebug).Info("update labels on cluster") - err = r.updateLabelsOnCluster(ctx, classifierScope, cluster, clusterproxy.GetClusterType(ref), l) - if err != nil { - // If cluster was removed before classifier had a chance to react to it, ignore the error - if apierrors.IsNotFound(err) { - return nil + clusterLogger := logger.WithValues("cluster", fmt.Sprintf("%s/%s", cluster.GetNamespace(), cluster.GetName())) + + // Attempt to update labels on the physical cluster + if err := r.updateLabelsOnCluster(ctx, classifierScope, cluster, clusterproxy.GetClusterType(ref), clusterLogger); err != nil { + if !apierrors.IsNotFound(err) { + clusterLogger.Error(err, "failed to apply labels to cluster") + errs = append(errs, err) + } + } + } + + 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. +func (r *ClassifierReconciler) removeLabelsFromCluster(ctx context.Context, + classifier *libsveltosv1beta1.Classifier, cluster client.Object, clusterType libsveltosv1beta1.ClusterType, + logger logr.Logger) error { + + manager, err := keymanager.GetKeyManagerInstance(ctx, r.Client) + if err != nil { + logger.V(logs.LogInfo).Error(err, "failed to get label key manager") + return err + } + + labels := cluster.GetLabels() + if labels == nil { + return nil // Nothing to remove + } + + labelsChanged := false + for i := range classifier.Spec.ClassifierLabels { + label := classifier.Spec.ClassifierLabels[i] + + // 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) + labelsChanged = true } - l.V(logs.LogDebug).Error(err, "failed to update labels on cluster") - return err } } + // Only trigger a cluster update if labels were actually modified + if labelsChanged { + cluster.SetLabels(labels) + return r.Update(ctx, cluster) + } + return nil } @@ -631,6 +708,49 @@ func (r *ClassifierReconciler) updateMaps(classifierScope *scope.ClassifierScope r.ClassifierMap[*classifierInfo] = currentClusters } +// removeLabelsFromClusters identifies all clusters currently matching the Classifier's +// selector and removes labels managed by this Classifier instance from each of them. +// It continues processing remaining clusters even if an individual cluster update fails, +// returning a combined error at the end. +func (r *ClassifierReconciler) removeLabelsFromClusters(ctx context.Context, + classifierScope *scope.ClassifierScope, logger logr.Logger) error { + + var errs []error + + // Iterate through all clusters currently tracked in the Classifier status + for i := range classifierScope.Classifier.Status.MachingClusterStatuses { + ref := &classifierScope.Classifier.Status.MachingClusterStatuses[i].ClusterRef + + cluster, err := clusterproxy.GetCluster(ctx, r.Client, ref.Namespace, ref.Name, + clusterproxy.GetClusterType(ref)) + if err != nil { + if apierrors.IsNotFound(err) { + continue + } + logger.V(logs.LogInfo).Error(err, "failed to get cluster", + "namespace", ref.Namespace, "name", ref.Name) + errs = append(errs, err) + continue + } + + clusterLogger := logger.WithValues( + "cluster", fmt.Sprintf("%s/%s", cluster.GetNamespace(), cluster.GetName()), + "clusterType", clusterproxy.GetClusterType(ref), + ) + + clusterLogger.V(logs.LogDebug).Info("removing managed labels from cluster") + + // Remove labels and collect error if the operation fails + if err := r.removeLabelsFromCluster(ctx, classifierScope.Classifier, cluster, + clusterproxy.GetClusterType(ref), clusterLogger); err != nil { + clusterLogger.Error(err, "failed to remove labels") + errs = append(errs, err) + } + } + + return errors.Join(errs...) +} + // removeAllRegistrations unregisters Classifier for all cluster labels // it used to manage (in any matching cluster) func (r *ClassifierReconciler) removeAllRegistrations(ctx context.Context, @@ -651,41 +771,83 @@ func (r *ClassifierReconciler) removeAllRegistrations(ctx context.Context, return nil } -// handleLabelRegistrations registers Classifier for all labels, considering all clusters -// currently matching Classifier -// Clear old registrations -func (r *ClassifierReconciler) handleLabelRegistrations(ctx context.Context, +// registerMatchingClusters updates the label ownership registrations for all clusters +// currently matching the Classifier. It also ensures stale registrations for these +// specific clusters are cleared. +func (r *ClassifierReconciler) registerMatchingClusters(ctx context.Context, classifier *libsveltosv1beta1.Classifier, - currentMatchingClusters, oldMatchingClusters map[corev1.ObjectReference]bool, + currentMatchingClusters map[corev1.ObjectReference]bool, logger logr.Logger) error { - // Register Classifier instance as wanting to manage any labels in ClassifierLabels - // for all the clusters currently matching manager, err := keymanager.GetKeyManagerInstance(ctx, r.Client) if err != nil { logger.V(logs.LogInfo).Error(err, "failed to get label key manager") return err } - matchingClusterRefs := make([]corev1.ObjectReference, len(currentMatchingClusters)) - i := 0 for c := range currentMatchingClusters { clusterType := clusterproxy.GetClusterType(&c) + // Ensure the manager is up-to-date for this specific cluster manager.RemoveStaleRegistrations(classifier, c.Namespace, c.Name, clusterType) manager.RegisterClassifierForLabels(classifier, c.Namespace, c.Name, clusterType) - matchingClusterRefs[i] = c - i++ } - // For every cluster which is not a match anymore, remove registations + return nil +} + +// cleanUpNonMatchingClusters identifies clusters that no longer match the Classifier. +// It removes managed labels from the clusters first; only upon successful label removal +// does it clear the internal registrations in the KeyManager. +func (r *ClassifierReconciler) cleanUpNonMatchingClusters(ctx context.Context, + classifier *libsveltosv1beta1.Classifier, + currentMatchingClusters, oldMatchingClusters map[corev1.ObjectReference]bool, + logger logr.Logger) error { + + manager, err := keymanager.GetKeyManagerInstance(ctx, r.Client) + if err != nil { + logger.V(logs.LogInfo).Error(err, "failed to get label key manager") + return err + } + + var errs []error for c := range oldMatchingClusters { - if _, ok := currentMatchingClusters[c]; !ok { - clusterType := clusterproxy.GetClusterType(&c) - manager.RemoveAllRegistrations(classifier, c.Namespace, c.Name, clusterType) + // Only process clusters that were in the old set but are NOT in the current set + if _, ok := currentMatchingClusters[c]; ok { + continue + } + + clusterType := clusterproxy.GetClusterType(&c) + clusterLogger := logger.WithValues("cluster", fmt.Sprintf("%s/%s", c.Namespace, c.Name)) + + // 1. Attempt to fetch the cluster + cluster, err := clusterproxy.GetCluster(ctx, r.Client, c.Namespace, c.Name, clusterType) + if err != nil { + if apierrors.IsNotFound(err) { + // Cluster is gone, so labels are effectively removed. Safe to clear registrations. + manager.RemoveAllRegistrations(classifier, c.Namespace, c.Name, clusterType) + continue + } + + wrappedErr := fmt.Errorf("failed to get cluster %s/%s for cleanup: %w", c.Namespace, c.Name, err) + clusterLogger.Error(wrappedErr, "lookup failed, skipping registration removal") + errs = append(errs, wrappedErr) + continue + } + + // 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 { + 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) + continue } + + // 3. Only remove registrations if label removal was successful + manager.RemoveAllRegistrations(classifier, c.Namespace, c.Name, clusterType) } - return nil + return errors.Join(errs...) } // classifyLabels divides labels in Managed and UnManaged diff --git a/controllers/classifier_controller_test.go b/controllers/classifier_controller_test.go index 0685efc..95653be 100644 --- a/controllers/classifier_controller_test.go +++ b/controllers/classifier_controller_test.go @@ -20,6 +20,7 @@ import ( "context" "sync" + "github.com/go-logr/logr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -45,9 +46,11 @@ import ( var _ = Describe("Classifier: Reconciler", func() { var classifier *libsveltosv1beta1.Classifier + var logger logr.Logger BeforeEach(func() { classifier = getClassifierInstance(randomString()) + logger = textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))) }) It("Adds finalizer", func() { @@ -131,7 +134,7 @@ var _ = Describe("Classifier: Reconciler", func() { Expect(c.Status().Update(context.TODO(), currentClassifier)).To(Succeed()) dep := fakedeployer.GetClient(context.TODO(), - textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))), testEnv.Client) + logger, testEnv.Client) Expect(dep.RegisterFeatureID(libsveltosv1beta1.FeatureClassifier)).To(Succeed()) reconciler := &controllers.ClassifierReconciler{ @@ -180,6 +183,14 @@ var _ = Describe("Classifier: Reconciler", func() { classifierReport1.Spec.Match = false classifierReport2 := getClassifierReport(randomString(), randomString(), randomString()) + matchingClusters := map[corev1.ObjectReference]bool{} + matchingClusters[corev1.ObjectReference{ + Namespace: clusterNamespace, + Name: clusterName, + APIVersion: clusterv1.GroupVersion.String(), + Kind: clusterv1.ClusterKind, + }] = true + initObjects := []client.Object{ classifier, classifierReport0, @@ -200,14 +211,14 @@ var _ = Describe("Classifier: Reconciler", func() { classifierScope, err := scope.NewClassifierScope(scope.ClassifierScopeParams{ Client: c, - Logger: textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))), + Logger: logger, Classifier: classifier, ControllerName: "classifier", }) Expect(err).To(BeNil()) - Expect(controllers.UpdateMatchingClustersAndRegistrations(reconciler, context.TODO(), classifierScope, - textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))))).To(Succeed()) + Expect(controllers.UpdateMatchingClustersAndRegistrations(reconciler, context.TODO(), classifierScope, matchingClusters, + logger)).To(Succeed()) Expect(classifier.Status.MachingClusterStatuses).ToNot(BeNil()) Expect(len(classifier.Status.MachingClusterStatuses)).To(Equal(1)) @@ -221,6 +232,14 @@ var _ = Describe("Classifier: Reconciler", func() { classifierReport0 := getClassifierReport(classifier.Name, clusterNamespace, clusterName) classifierReport0.Spec.Match = true + matchingClusters := map[corev1.ObjectReference]bool{} + matchingClusters[corev1.ObjectReference{ + Namespace: clusterNamespace, + Name: clusterName, + APIVersion: clusterv1.GroupVersion.String(), + Kind: clusterv1.ClusterKind, + }] = true + // Create a second classifier with same ClassifierLabels as first classifier classifier1 := &libsveltosv1beta1.Classifier{ ObjectMeta: metav1.ObjectMeta{ @@ -260,14 +279,14 @@ var _ = Describe("Classifier: Reconciler", func() { classifierScope, err := scope.NewClassifierScope(scope.ClassifierScopeParams{ Client: c, - Logger: textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))), + Logger: logger, Classifier: classifier, ControllerName: "classifier", }) Expect(err).To(BeNil()) - Expect(controllers.UpdateMatchingClustersAndRegistrations(reconciler, context.TODO(), classifierScope, - textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))))).To(Succeed()) + Expect(controllers.UpdateMatchingClustersAndRegistrations(reconciler, context.TODO(), classifierScope, matchingClusters, + logger)).To(Succeed()) Expect(classifier.Status.MachingClusterStatuses).ToNot(BeNil()) Expect(len(classifier.Status.MachingClusterStatuses)).To(Equal(1)) @@ -325,7 +344,7 @@ var _ = Describe("Classifier: Reconciler", func() { classifierScope, err := scope.NewClassifierScope(scope.ClassifierScopeParams{ Client: c, - Logger: textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))), + Logger: logger, Classifier: classifier, ControllerName: "classifier", }) @@ -337,12 +356,11 @@ var _ = Describe("Classifier: Reconciler", func() { currentMatchingClusters := map[corev1.ObjectReference]bool{ {Namespace: cluster.Namespace, Name: cluster.Name, APIVersion: cluster.APIVersion, Kind: cluster.Kind}: true, } - oldMatchingClusters := map[corev1.ObjectReference]bool{} - Expect(controllers.HandleLabelRegistrations(reconciler, context.TODO(), classifier, currentMatchingClusters, - oldMatchingClusters, textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))))).To(Succeed()) + Expect(controllers.RegisterMatchingClusters(reconciler, context.TODO(), classifier, currentMatchingClusters, + logger)).To(Succeed()) Expect(controllers.UpdateLabelsOnMatchingClusters(reconciler, context.TODO(), classifierScope, - textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))))).To(Succeed()) + logger)).To(Succeed()) currentCluster := &clusterv1.Cluster{} Expect(c.Get(context.TODO(), @@ -402,18 +420,97 @@ var _ = Describe("Classifier: Reconciler", func() { classifierScope, err := scope.NewClassifierScope(scope.ClassifierScopeParams{ Client: c, - Logger: textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))), + Logger: logger, Classifier: classifier, ControllerName: "classifier", }) Expect(err).To(BeNil()) Expect(controllers.RemoveAllRegistrations(reconciler, context.TODO(), classifierScope, - textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))))).To(Succeed()) + logger)).To(Succeed()) Expect(manager.CanManageLabel(classifier, clusterNamespace, clusterName, label, libsveltosv1beta1.ClusterTypeCapi)).To(BeFalse()) }) + It("cleanUpNonMatchingClusters removes labels from cluster which are not a match", func() { + labelKey := randomString() + labelValue := randomString() + clusterNamespace := randomString() + clusterName := randomString() + classifier.Spec.ClassifierLabels = []libsveltosv1beta1.ClassifierLabel{ + {Key: labelKey, Value: labelValue}, + } + + cluster := &libsveltosv1beta1.SveltosCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: clusterNamespace, + Name: clusterName, + Labels: map[string]string{ + labelKey: labelValue, + }, + }, + } + + clusterRef := corev1.ObjectReference{ + Kind: libsveltosv1beta1.SveltosClusterKind, + APIVersion: libsveltosv1beta1.GroupVersion.String(), + Namespace: clusterNamespace, + Name: clusterName, + } + + classifier.Status.MachingClusterStatuses = []libsveltosv1beta1.MachingClusterStatus{ + { + ClusterRef: corev1.ObjectReference{ + Namespace: clusterNamespace, + Name: clusterName, + Kind: clusterKind, + APIVersion: clusterv1.GroupVersion.String(), + }, + ManagedLabels: []string{labelKey}, + }, + } + + initObjects := []client.Object{ + classifier, cluster, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(initObjects...). + WithObjects(initObjects...).Build() + + reconciler := &controllers.ClassifierReconciler{ + Client: c, + Scheme: scheme, + ClusterMap: make(map[corev1.ObjectReference]*libsveltosset.Set), + ClassifierMap: make(map[corev1.ObjectReference]*libsveltosset.Set), + Mux: sync.Mutex{}, + } + + oldMatches := map[corev1.ObjectReference]bool{clusterRef: true} + currentMatches := map[corev1.ObjectReference]bool{} // No longer matches + + err := controllers.CleanUpNonMatchingClusters(reconciler, context.TODO(), classifier, currentMatches, oldMatches, logger) + Expect(err).ToNot(HaveOccurred()) + + // Since the Classifier is not managing this label, label remains on cluster + currentCluster := &libsveltosv1beta1.SveltosCluster{} + err = c.Get(context.TODO(), client.ObjectKey{Name: clusterRef.Name, Namespace: clusterRef.Namespace}, currentCluster) + Expect(err).ToNot(HaveOccurred()) + Expect(currentCluster.Labels).To(HaveKey(labelKey)) + + manager, err := keymanager.GetKeyManagerInstance(ctx, c) + Expect(err).To(BeNil()) + // Register classifier as managing labels on cluster + manager.RegisterClassifierForLabels(classifier, clusterNamespace, clusterName, libsveltosv1beta1.ClusterTypeSveltos) + + err = controllers.CleanUpNonMatchingClusters(reconciler, context.TODO(), classifier, currentMatches, oldMatches, logger) + Expect(err).ToNot(HaveOccurred()) + + // Since the Classifier is managing this label, label is removed from cluster + err = c.Get(context.TODO(), client.ObjectKey{Name: clusterRef.Name, Namespace: clusterRef.Namespace}, currentCluster) + Expect(err).ToNot(HaveOccurred()) + Expect(currentCluster.Labels).ToNot(HaveKey(labelKey)) + }) + It("classifyLabels divides labels in managed and unmanaged", func() { clusterNamespace := randomString() clusterName := randomString() @@ -466,7 +563,7 @@ var _ = Describe("Classifier: Reconciler", func() { } managed, unManaged, err := controllers.ClassifyLabels(reconciler, context.TODO(), classifier, - clusterRef, textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1)))) + clusterRef, logger) Expect(err).To(BeNil()) Expect(len(managed)).To(Equal(1)) Expect(len(unManaged)).To(Equal(1)) @@ -477,11 +574,12 @@ var _ = Describe("Classifier: Reconciler", func() { var _ = Describe("ClassifierReconciler: requeue methods", func() { var classifier *libsveltosv1beta1.Classifier var cluster *clusterv1.Cluster + var logger logr.Logger BeforeEach(func() { cluster = prepareCluster() controllers.SetVersion(version) - + logger = textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))) classifier = getClassifierInstance(randomString()) }) @@ -502,8 +600,7 @@ var _ = Describe("ClassifierReconciler: requeue methods", func() { Name: classifier.Name, } - dep := fakedeployer.GetClient(context.TODO(), textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(1))), - testEnv.Client) + dep := fakedeployer.GetClient(context.TODO(), logger, testEnv.Client) Expect(dep.RegisterFeatureID(libsveltosv1beta1.FeatureClassifier)).To(Succeed()) clusterProfileReconciler := getClassifierReconciler(testEnv.Client, dep) diff --git a/controllers/classifier_predicates.go b/controllers/classifier_predicates.go index 1973297..b32377f 100644 --- a/controllers/classifier_predicates.go +++ b/controllers/classifier_predicates.go @@ -252,7 +252,7 @@ func ClassifierReportPredicate(logger logr.Logger) predicate.Funcs { // return true if ClassifierReport.Spec.Match has changed if oldReport.Spec.Match != newReport.Spec.Match { log.V(logs.LogVerbose).Info( - "Cluster was unpaused. Will attempt to reconcile associated Classifiers.") + "ClassifierReport Spec.Match changed. Will attempt to reconcile associated Classifiers.") return true } diff --git a/controllers/export_test.go b/controllers/export_test.go index 4f87753..d63b2a3 100644 --- a/controllers/export_test.go +++ b/controllers/export_test.go @@ -54,10 +54,11 @@ var ( RequeueClassifierForClassifierReport = (*ClassifierReconciler).requeueClassifierForClassifierReport UpdateMatchingClustersAndRegistrations = (*ClassifierReconciler).updateMatchingClustersAndRegistrations UpdateLabelsOnMatchingClusters = (*ClassifierReconciler).updateLabelsOnMatchingClusters - HandleLabelRegistrations = (*ClassifierReconciler).handleLabelRegistrations + RegisterMatchingClusters = (*ClassifierReconciler).registerMatchingClusters UndeployClassifier = (*ClassifierReconciler).undeployClassifier RemoveAllRegistrations = (*ClassifierReconciler).removeAllRegistrations ClassifyLabels = (*ClassifierReconciler).classifyLabels + CleanUpNonMatchingClusters = (*ClassifierReconciler).cleanUpNonMatchingClusters ) var ( diff --git a/test/fv/conflict_test.go b/test/fv/conflict_test.go index 05d72e3..bc17d79 100644 --- a/test/fv/conflict_test.go +++ b/test/fv/conflict_test.go @@ -149,7 +149,7 @@ var _ = Describe("Classifier: update cluster labels", func() { return err != nil && apierrors.IsNotFound(err) }, timeout, pollingInterval).Should(BeTrue()) - verifyClusterLabels(classifier2) + verifyClusterLabelsAreGone(classifier2) removeLabels(classifier1) removeLabels(classifier2) diff --git a/test/fv/labels_test.go b/test/fv/labels_test.go index 6321a10..35af303 100644 --- a/test/fv/labels_test.go +++ b/test/fv/labels_test.go @@ -68,7 +68,29 @@ func verifyFlow(namePrefix string) { } verifyClassifierReport(classifier.Name, true) + verifyClusterLabels(classifier) + + Byf("Changing classifier so cluster is not a match anymore") + currentClassifer := &libsveltosv1beta1.Classifier{} + Expect(k8sClient.Get(context.TODO(), types.NamespacedName{Name: classifier.Name}, + currentClassifer)).To(Succeed()) + Expect(currentClassifer.Spec.KubernetesVersionConstraints).ToNot(BeNil()) + currentClassifer.Spec.KubernetesVersionConstraints[0].Comparison = + string(libsveltosv1beta1.ComparisonLessThan) + Expect(k8sClient.Update(context.TODO(), currentClassifer)).To(Succeed()) + + verifyClassifierReport(classifier.Name, false) + verifyClusterLabelsAreGone(classifier) + + Byf("Changing classifier so cluster is a match again") + Expect(k8sClient.Get(context.TODO(), types.NamespacedName{Name: classifier.Name}, + currentClassifer)).To(Succeed()) + Expect(currentClassifer.Spec.KubernetesVersionConstraints).ToNot(BeNil()) + currentClassifer.Spec.KubernetesVersionConstraints[0].Comparison = + string(libsveltosv1beta1.ComparisonGreaterThanOrEqualTo) + Expect(k8sClient.Update(context.TODO(), currentClassifer)).To(Succeed()) + verifyClassifierReport(classifier.Name, true) verifyClusterLabels(classifier) Byf("Deleting classifier instance %s in the management cluster", classifier.Name) @@ -96,7 +118,7 @@ func verifyFlow(namePrefix string) { }, timeout, pollingInterval).Should(BeTrue()) Byf("Verifying Cluster labels are not updated because of Classifier being deleted") - verifyClusterLabels(classifier) + verifyClusterLabelsAreGone(classifier) removeLabels(classifier) } diff --git a/test/fv/report_match_change_test.go b/test/fv/report_match_change_test.go index cb0da94..0102ffb 100644 --- a/test/fv/report_match_change_test.go +++ b/test/fv/report_match_change_test.go @@ -129,8 +129,8 @@ var _ = Describe("Classifier: update cluster labels", func() { return err != nil && apierrors.IsNotFound(err) }, timeout, pollingInterval).Should(BeTrue()) - Byf("Verifying Cluster labels are not updated because of Classifier being deleted") - verifyClusterLabels(classifier) + Byf("Verifying Cluster labels are removed because of Classifier being deleted") + verifyClusterLabelsAreGone(classifier) removeLabels(classifier) }) diff --git a/test/fv/utils_test.go b/test/fv/utils_test.go index 924ea7c..9d24ec2 100644 --- a/test/fv/utils_test.go +++ b/test/fv/utils_test.go @@ -66,7 +66,7 @@ func getClassifier(namePrefix string, clusterLabels map[string]string) *libsvelt ClassifierLabels: labels, KubernetesVersionConstraints: []libsveltosv1beta1.KubernetesVersionConstraint{ { - Version: "1.25.0", + Version: "1.30.0", Comparison: string(libsveltosv1beta1.ComparisonGreaterThanOrEqualTo), }, }, @@ -130,6 +130,27 @@ func verifyClusterLabels(classifier *libsveltosv1beta1.Classifier) { }, timeout, pollingInterval).Should(BeTrue()) } +func verifyClusterLabelsAreGone(classifier *libsveltosv1beta1.Classifier) { + Byf("Verifying Classifier labels are removed from cluster %s", classifier.Name) + Eventually(func() bool { + currentCuster, err := getCluster() + if err != nil { + return false + } + if currentCuster.GetLabels() == nil { + return false + } + for i := range classifier.Spec.ClassifierLabels { + cLabel := classifier.Spec.ClassifierLabels[i] + _, ok := currentCuster.GetLabels()[cLabel.Key] + if ok { + return false + } + } + return true + }, timeout, pollingInterval).Should(BeTrue()) +} + func removeLabels(classifier *libsveltosv1beta1.Classifier) { err := retry.RetryOnConflict(retry.DefaultRetry, func() error { currentCluster, err := getCluster()