Merge pull request #1659 from gianlucam76/fix-hash-evaluation

Fix hash evaluation

Author: gianlucam76

api/v1beta1/zz_generated.deepcopy.go

@@ -781,16 +781,11 @@ func runInitContainerWork(ctx context.Context, config *rest.Config,
 
 func setupLogging() {
 	klog.InitFlags(nil)
-	_ = flag.Set("logtostderr", "false") // set default, but still overridable via CLI
+
 	initFlags(pflag.CommandLine)
 	pflag.CommandLine.SetNormalizeFunc(cliflag.WordSepNormalizeFunc)
 	pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
 	pflag.Parse()
 
-	if flag.Lookup("logtostderr").Value.String() == "false" {
-		klog.SetOutputBySeverity("INFO", os.Stdout)
-		klog.SetOutputBySeverity("WARNING", os.Stdout)
-		klog.SetOutputBySeverity("ERROR", os.Stderr)
-		klog.SetOutputBySeverity("FATAL", os.Stderr)
-	}
+	ctrl.SetLogger(klog.Background())
 }

cmd/main.go

@@ -155,7 +155,7 @@ type ClusterSummaryReconciler struct {
 //+kubebuilder:rbac:groups="source.toolkit.fluxcd.io",resources=buckets,verbs=get;watch;list
 //+kubebuilder:rbac:groups="source.toolkit.fluxcd.io",resources=buckets/status,verbs=get;watch;list
 
-func (r *ClusterSummaryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {
+func (r *ClusterSummaryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, reterr error) {
 	logger := ctrl.LoggerFrom(ctx)
 	logger.V(logs.LogDebug).Info("Reconciling")
 
@@ -210,14 +210,17 @@ func (r *ClusterSummaryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	}
 
 	// Always close the scope when exiting this function so we can persist any ClusterSummary
-	// changes. Conflict errors are swallowed because the watch event from whatever caused the
-	// conflict will re-enqueue this resource, and the next reconciliation will recompute status.
-	// Propagating the conflict would cause controller-runtime to immediately requeue, bypassing
-	// the intended NextReconcileTime backoff.
+	// changes. Conflict errors are swallowed and a 1-minute requeue is scheduled instead of
+	// propagating the error (which would cause controller-runtime to immediately requeue,
+	// bypassing the intended NextReconcileTime backoff). We cannot rely solely on a watch event
+	// to re-enqueue because the conflict may be caused by the controller's own logic (e.g. a
+	// tier change), in which case no further watch event will arrive.
 	defer func() {
 		if err = clusterSummaryScope.Close(ctx); err != nil {
 			if apierrors.IsConflict(err) {
-				logger.V(logs.LogDebug).Info("conflict patching ClusterSummary status, will reconcile on next event")
+				logger.V(logs.LogDebug).Info("conflict patching ClusterSummary status, will retry in 1 minute")
+				r.setNextReconcileTime(clusterSummaryScope, time.Minute)
+				result = ctrl.Result{RequeueAfter: time.Minute}
 				return
 			}
 			reterr = err
@@ -295,6 +298,11 @@ func (r *ClusterSummaryReconciler) updateDeletedInstancs(clusterSummaryScope *sc
 		Namespace: clusterSummaryScope.Namespace(),
 		Name:      clusterSummaryScope.Name(),
 	}] = time.Now()
+
+	delete(r.NextReconcileTimes, types.NamespacedName{
+		Namespace: clusterSummaryScope.Namespace(),
+		Name:      clusterSummaryScope.Name(),
+	})
 }
 
 func (r *ClusterSummaryReconciler) reconcileDelete(

controllers/clustersummary_controller.go

@@ -98,7 +98,8 @@ func (r *ClusterSummaryReconciler) deployFeature(ctx context.Context, clusterSum
 	// Get hash of current configuration (at this very precise moment)
 	currentHash, err := f.currentHash(ctx, r.Client, clusterSummary, logger)
 	if err != nil {
-		if !apierrors.IsNotFound(err) {
+		var nrErr *configv1beta1.NonRetriableError
+		if !errors.As(err, &nrErr) && !apierrors.IsNotFound(err) {
 			return err
 		}
 	}

controllers/clustersummary_deployer.go

@@ -171,11 +171,11 @@ var _ = AfterSuite(func() {
 
 func getClusterSummaryReconciler(c client.Client, dep deployer.DeployerInterface) *controllers.ClusterSummaryReconciler {
 	return &controllers.ClusterSummaryReconciler{
-		Client:           c,
-		Scheme:           scheme,
-		Deployer:         dep,
-		ClusterMap:       make(map[corev1.ObjectReference]*libsveltosset.Set),
-		ReferenceMap:     make(map[corev1.ObjectReference]*libsveltosset.Set),
+		Client:             c,
+		Scheme:             scheme,
+		Deployer:           dep,
+		ClusterMap:         make(map[corev1.ObjectReference]*libsveltosset.Set),
+		ReferenceMap:       make(map[corev1.ObjectReference]*libsveltosset.Set),
 		DeletedInstances:   make(map[types.NamespacedName]time.Time),
 		NextReconcileTimes: make(map[types.NamespacedName]time.Time),
 		PolicyMux:          sync.Mutex{},

controllers/controllers_suite_test.go

@@ -367,6 +367,7 @@ func kustomizationHash(ctx context.Context, c client.Client, clusterSummary *con
 			continue
 		}
 		config += string(result)
+		config += fmt.Sprintf("%d", kustomizationRef.Tier)
 
 		valueFromHash, err := getKustomizeReferenceResourceHash(ctx, c, clusterSummary,
 			kustomizationRef, logger)

controllers/handlers_kustomize.go

@@ -533,34 +533,7 @@ func resourcesHash(ctx context.Context, c client.Client, clusterSummary *configv
 	hash := sha256.Sum256(raw)
 	config += hex.EncodeToString((hash[:]))
 
-	referencedObjects := make([]corev1.ObjectReference, 0, len(clusterSummary.Spec.ClusterProfileSpec.PolicyRefs))
-	for i := range sortedPolicyRefs {
-		reference := &sortedPolicyRefs[i]
-		namespace, err := libsveltostemplate.GetReferenceResourceNamespace(ctx, c,
-			clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName, reference.Namespace,
-			clusterSummary.Spec.ClusterType)
-		if err != nil {
-			logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to instantiate namespace for %s %s/%s: %v",
-				reference.Kind, reference.Namespace, reference.Name, err))
-			// Ignore template instantiation error
-			continue
-		}
-
-		name, err := libsveltostemplate.GetReferenceResourceName(ctx, c, clusterSummary.Spec.ClusterNamespace,
-			clusterSummary.Spec.ClusterName, reference.Name, clusterSummary.Spec.ClusterType)
-		if err != nil {
-			logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to instantiate name for %s %s/%s: %v",
-				reference.Kind, reference.Namespace, reference.Name, err))
-			// Ignore template instantiation error
-			continue
-		}
-
-		referencedObjects = append(referencedObjects, corev1.ObjectReference{
-			Kind:      sortedPolicyRefs[i].Kind,
-			Namespace: namespace,
-			Name:      name,
-		})
-	}
+	referencedObjects, referencedObjectTiers := getInstantiatedPolicyRefInfo(ctx, c, clusterSummary, sortedPolicyRefs, logger)
 
 	sort.Sort(dependencymanager.SortedCorev1ObjectReference(referencedObjects))
 
@@ -573,12 +546,14 @@ func resourcesHash(ctx context.Context, c client.Client, clusterSummary *configv
 			err = c.Get(ctx, types.NamespacedName{Namespace: reference.Namespace, Name: reference.Name}, configmap)
 			if err == nil {
 				config += getConfigMapHash(configmap)
+				config += fmt.Sprintf("%d", referencedObjectTiers[*reference])
 			}
 		} else if reference.Kind == string(libsveltosv1beta1.SecretReferencedResourceKind) {
 			secret := &corev1.Secret{}
 			err = c.Get(ctx, types.NamespacedName{Namespace: reference.Namespace, Name: reference.Name}, secret)
 			if err == nil {
 				config += getSecretHash(secret)
+				config += fmt.Sprintf("%d", referencedObjectTiers[*reference])
 			}
 		} else {
 			var source client.Object
@@ -591,6 +566,7 @@ func resourcesHash(ctx context.Context, c client.Client, clusterSummary *configv
 				if source.GetAnnotations() != nil {
 					config += getDataSectionHash(source.GetAnnotations())
 				}
+				config += fmt.Sprintf("%d", referencedObjectTiers[*reference])
 			}
 		}
 		if err != nil {
@@ -623,6 +599,45 @@ func resourcesHash(ctx context.Context, c client.Client, clusterSummary *configv
 	return h.Sum(nil), nil
 }
 
+func getInstantiatedPolicyRefInfo(ctx context.Context, c client.Client, clusterSummary *configv1beta1.ClusterSummary,
+	sortedPolicyRefs []configv1beta1.PolicyRef, logger logr.Logger,
+) (referencedObjects []corev1.ObjectReference, referencedObjectTiers map[corev1.ObjectReference]int32) {
+
+	referencedObjects = make([]corev1.ObjectReference, 0, len(clusterSummary.Spec.ClusterProfileSpec.PolicyRefs))
+	referencedObjectTiers = make(map[corev1.ObjectReference]int32, len(clusterSummary.Spec.ClusterProfileSpec.PolicyRefs))
+	for i := range sortedPolicyRefs {
+		reference := &sortedPolicyRefs[i]
+		namespace, err := libsveltostemplate.GetReferenceResourceNamespace(ctx, c,
+			clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName, reference.Namespace,
+			clusterSummary.Spec.ClusterType)
+		if err != nil {
+			logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to instantiate namespace for %s %s/%s: %v",
+				reference.Kind, reference.Namespace, reference.Name, err))
+			// Ignore template instantiation error
+			continue
+		}
+
+		name, err := libsveltostemplate.GetReferenceResourceName(ctx, c, clusterSummary.Spec.ClusterNamespace,
+			clusterSummary.Spec.ClusterName, reference.Name, clusterSummary.Spec.ClusterType)
+		if err != nil {
+			logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to instantiate name for %s %s/%s: %v",
+				reference.Kind, reference.Namespace, reference.Name, err))
+			// Ignore template instantiation error
+			continue
+		}
+
+		obj := corev1.ObjectReference{
+			Kind:      sortedPolicyRefs[i].Kind,
+			Namespace: namespace,
+			Name:      name,
+		}
+		referencedObjects = append(referencedObjects, obj)
+		referencedObjectTiers[obj] = reference.Tier
+	}
+
+	return referencedObjects, referencedObjectTiers
+}
+
 func getResourceRefs(clusterSummary *configv1beta1.ClusterSummary) []configv1beta1.PolicyRef {
 	return clusterSummary.Spec.ClusterProfileSpec.PolicyRefs
 }

controllers/handlers_resources.go

@@ -398,7 +398,7 @@ func cleanClusterConfigurations(ctx context.Context, c client.Client, profileSco
 		}
 
 		err = cleanClusterConfiguration(ctx, c, profileScope.Profile, cc)
-		if err != nil {
+		if err != nil && !apierrors.IsNotFound(err) {
 			return err
 		}
 	}

controllers/profile_utils.go

@@ -236,7 +236,7 @@ func isNamespaced(ctx context.Context, r *unstructured.Unstructured, clusterName
 	// 2. RETRY LOOP: Give it 3 attempts with increasing wait times
 	// Total wait time: 1s + 2s + 3s = 6 seconds.
 	for i := range 3 {
-		// Log that we are attempting a refresh (MGIANLUC style)
+		// Log that we are attempting a refresh
 		logger.V(logs.LogInfo).Info(fmt.Sprintf("GVK %s not found, refreshing discovery (attempt %d)", gvk.String(), i+1))
 
 		// IMPORTANT: Invalidate the Discovery Client FIRST, then Reset the Mapper

controllers/utils.go

@@ -135,7 +135,7 @@ var _ = Describe("PolicyRef Tier", func() {
 				currentServiceAccount)
 		}, timeout, pollingInterval).Should(BeNil())
 
-		Byf("Verifying ServicdAccount has proper labels")
+		Byf("Verifying ServiceAccount has proper labels")
 		currentServiceAccount := &corev1.ServiceAccount{}
 		Expect(workloadClient.Get(context.TODO(),
 			types.NamespacedName{Namespace: saNamespace, Name: saName},
@@ -144,6 +144,8 @@ var _ = Describe("PolicyRef Tier", func() {
 		v, ok := currentServiceAccount.Labels[firstConfigMapLabelKey]
 		Expect(ok).To(BeTrue())
 		Expect(v).To(Equal(firstConfigMapLabelValue))
+		v, ok = currentServiceAccount.Labels[secondConfigMapLabelKey]
+		Expect(ok).To(BeFalse())
 
 		Byf("Verifying ClusterSummary %s status reports conflict for Resources feature", clusterSummary.Name)
 		Eventually(func() bool {
@@ -165,7 +167,7 @@ var _ = Describe("PolicyRef Tier", func() {
 			return false
 		}, timeout, pollingInterval).Should(BeTrue())
 
-		By("Updating second ConfigMap tier")
+		By(fmt.Sprintf("Updating ConfigMap %s/%s tier", secondConfigMap.Namespace, secondConfigMap.Name))
 		const lowerTier = 90
 		err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
 			Expect(k8sClient.Get(context.TODO(),
@@ -177,18 +179,22 @@ var _ = Describe("PolicyRef Tier", func() {
 					Name:      firstConfigMap.Name,
 				},
 				{
-					Tier:      lowerTier,
 					Kind:      string(libsveltosv1beta1.ConfigMapReferencedResourceKind),
 					Namespace: secondConfigMap.Namespace,
 					Name:      secondConfigMap.Name,
+					Tier:      lowerTier,
 				},
 			}
 			return k8sClient.Update(context.TODO(), currentClusterProfile)
 		})
 		Expect(err).To(BeNil())
 
-		Byf("Verifying ClusterSummary %s status is set to Deployed for Resources feature", clusterSummary.Name)
-		verifyFeatureStatusIsProvisioned(kindWorkloadCluster.GetNamespace(), clusterSummary.Name, libsveltosv1beta1.FeatureResources)
+		Expect(k8sClient.Get(context.TODO(),
+			types.NamespacedName{Name: clusterProfile.Name}, currentClusterProfile)).To(Succeed())
+
+		clusterSummary = verifyClusterSummary(clusterops.ClusterProfileLabelName,
+			currentClusterProfile.Name, &currentClusterProfile.Spec,
+			kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName(), getClusterType())
 
 		Byf("Verifying proper ServiceAccount is still present in the workload cluster with correct labels")
 		Eventually(func() bool {
@@ -203,12 +209,63 @@ var _ = Describe("PolicyRef Tier", func() {
 			if currentServiceAccount.Labels == nil {
 				return false
 			}
+			_, ok = currentServiceAccount.Labels[firstConfigMapLabelKey]
+			if ok {
+				return false
+			}
 			v, ok = currentServiceAccount.Labels[secondConfigMapLabelKey]
 			return ok && v == secondConfigMapLabelValue
 		}, timeout, pollingInterval).Should(BeTrue())
 
+		By("Changing first ConfigMap so there is no conflict anymore")
+		newSaNamespace := randomString()
+		firstConfigMap = createConfigMapWithPolicy(configMapNs, namePrefix+randomString(),
+			fmt.Sprintf(resource, newSaNamespace, saName, firstConfigMapLabelKey, firstConfigMapLabelValue))
+		Expect(k8sClient.Create(context.TODO(), firstConfigMap)).To(Succeed())
+
+		err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
+			Expect(k8sClient.Get(context.TODO(),
+				types.NamespacedName{Name: clusterProfile.Name}, currentClusterProfile)).To(Succeed())
+			currentClusterProfile.Spec.PolicyRefs = []configv1beta1.PolicyRef{
+				{
+					Kind:      string(libsveltosv1beta1.ConfigMapReferencedResourceKind),
+					Namespace: firstConfigMap.Namespace,
+					Name:      firstConfigMap.Name,
+				},
+				{
+					Kind:      string(libsveltosv1beta1.ConfigMapReferencedResourceKind),
+					Namespace: secondConfigMap.Namespace,
+					Name:      secondConfigMap.Name,
+					Tier:      lowerTier,
+				},
+			}
+			return k8sClient.Update(context.TODO(), currentClusterProfile)
+		})
+		Expect(err).To(BeNil())
+
+		Byf("Verifying new ServiceAccount is present in the workload cluster with correct labels")
+		Eventually(func() bool {
+			currentServiceAccount := &corev1.ServiceAccount{}
+			err = workloadClient.Get(context.TODO(),
+				types.NamespacedName{Namespace: newSaNamespace, Name: saName},
+				currentServiceAccount)
+			if err != nil {
+				return false
+			}
+
+			if currentServiceAccount.Labels == nil {
+				return false
+			}
+			v, ok = currentServiceAccount.Labels[firstConfigMapLabelKey]
+			return ok && v == firstConfigMapLabelValue
+		}, timeout, pollingInterval).Should(BeTrue())
+
+		Byf("Verifying ClusterSummary %s status is set to Deployed for Resources feature", clusterSummary.Name)
+		verifyFeatureStatusIsProvisioned(clusterSummary.Namespace, clusterSummary.Name, libsveltosv1beta1.FeatureResources)
+
 		policies := []policy{
 			{kind: "ServiceAccount", name: saName, namespace: saNamespace, group: ""},
+			{kind: "ServiceAccount", name: saName, namespace: newSaNamespace, group: ""},
 		}
 		verifyClusterConfiguration(configv1beta1.ClusterProfileKind, clusterProfile.Name,
 			clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName, libsveltosv1beta1.FeatureResources,
@@ -228,5 +285,14 @@ var _ = Describe("PolicyRef Tier", func() {
 				currentServiceAccount)
 			return err != nil && apierrors.IsNotFound(err)
 		}, timeout, pollingInterval).Should(BeTrue())
+
+		Byf("Verifying second ServiceAccount is removed from the workload cluster")
+		Eventually(func() bool {
+			currentServiceAccount := &corev1.ServiceAccount{}
+			err = workloadClient.Get(context.TODO(),
+				types.NamespacedName{Namespace: newSaNamespace, Name: saName},
+				currentServiceAccount)
+			return err != nil && apierrors.IsNotFound(err)
+		}, timeout, pollingInterval).Should(BeTrue())
 	})
 })