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()