fix: Do not clear drift reported while a ResourceSummary is processed

controllers/resourcesummary_collection.go

@@ -749,18 +749,14 @@ func containsDriftedResource(resources []configv1beta1.DriftedResourceRef, resou
 	return false
 }
 
+// resetResourceSummaryStatus clears the drift recorded in rs, the ResourceSummary that has been processed.
+// The update carries the resourceVersion of rs, so if drift-detection-manager reported a drift after rs was
+// read, the update fails with a Conflict instead of clearing a drift that was never processed. The next
+// collection reads the ResourceSummary again and processes it.
 func resetResourceSummaryStatus(ctx context.Context, remoteClient client.Client,
 	rs *libsveltosv1beta1.ResourceSummary, logger logr.Logger) error {
 
-	resourceSummary := &libsveltosv1beta1.ResourceSummary{}
-
-	err := remoteClient.Get(ctx, types.NamespacedName{Namespace: rs.Namespace, Name: rs.Name}, resourceSummary)
-	if err != nil {
-		if apierrors.IsNotFound(err) {
-			return nil
-		}
-		return err
-	}
+	resourceSummary := rs.DeepCopy()
 
 	logger.V(logs.LogDebug).Info("reset resourceSummary status")
 	resourceSummary.Status.ResourcesChanged = false
@@ -771,7 +767,12 @@ func resetResourceSummaryStatus(ctx context.Context, remoteClient client.Client,
 	// a resource already in it, so keeping the entries would pin their detection time to the first drift.
 	resourceSummary.Status.DriftedResources = nil
 	resourceSummary.Status.DriftedResourcesTruncated = false
-	return remoteClient.Status().Update(ctx, resourceSummary)
+
+	err := remoteClient.Status().Update(ctx, resourceSummary)
+	if err != nil && apierrors.IsNotFound(err) {
+		return nil
+	}
+	return err
 }
 
 // markDriftedHelmCharts sets NeedsRedeploy on the HelmReleaseSummaries entries matching

controllers/resourcesummary_collection_test.go

@@ -268,6 +268,86 @@ var _ = Describe("ResourceSummary Collection", func() {
 		}, timeout, pollingInterval).Should(BeTrue())
 	})
 
+	It("processResourceSummary does not clear a drift reported after the ResourceSummary was listed", func() {
+		cluster := prepareCluster()
+
+		clusterSummary := &configv1beta1.ClusterSummary{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: cluster.Namespace,
+				Name:      clusterProfileNamePrefix + randomString(),
+				Labels:    map[string]string{clusterops.ClusterProfileLabelName: randomString()},
+			},
+			Spec: configv1beta1.ClusterSummarySpec{
+				ClusterType: libsveltosv1beta1.ClusterTypeCapi,
+			},
+		}
+		Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed())
+		Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed())
+
+		currentClusterSummary := &configv1beta1.ClusterSummary{}
+		Expect(testEnv.Get(context.TODO(),
+			types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name},
+			currentClusterSummary)).To(Succeed())
+		currentClusterSummary.Status.FeatureSummaries = []configv1beta1.FeatureSummary{
+			{
+				FeatureID: libsveltosv1beta1.FeatureResources,
+				Status:    libsveltosv1beta1.FeatureStatusProvisioned,
+				Hash:      []byte(randomString()),
+			},
+			{
+				FeatureID: libsveltosv1beta1.FeatureHelm,
+				Status:    libsveltosv1beta1.FeatureStatusProvisioned,
+				Hash:      []byte(randomString()),
+			},
+		}
+		Expect(testEnv.Status().Update(context.TODO(), currentClusterSummary)).To(Succeed())
+
+		resourceSummary := getResourceSummary()
+		resourceSummary.Annotations = map[string]string{
+			libsveltosv1beta1.ClusterSummaryNameAnnotation:      clusterSummary.Name,
+			libsveltosv1beta1.ClusterSummaryNamespaceAnnotation: clusterSummary.Namespace,
+		}
+		Expect(testEnv.Create(context.TODO(), resourceSummary)).To(Succeed())
+		Expect(waitForObject(context.TODO(), testEnv.Client, resourceSummary)).To(Succeed())
+
+		resourceSummaryKey := types.NamespacedName{Namespace: resourceSummary.Namespace, Name: resourceSummary.Name}
+
+		// The copy the collector listed: only resourcesChanged is set
+		listedResourceSummary := &libsveltosv1beta1.ResourceSummary{}
+		Expect(testEnv.Get(context.TODO(), resourceSummaryKey, listedResourceSummary)).To(Succeed())
+		listedResourceSummary.Status.ResourcesChanged = true
+		Expect(testEnv.Status().Update(context.TODO(), listedResourceSummary)).To(Succeed())
+
+		// drift-detection-manager reports a Helm drift after the ResourceSummary was listed
+		// Eventual loop so testEnv Cache is synced and the update below does not use a stale resourceVersion
+		latestResourceSummary := &libsveltosv1beta1.ResourceSummary{}
+		Eventually(func() bool {
+			err := testEnv.Get(context.TODO(), resourceSummaryKey, latestResourceSummary)
+			return err == nil && latestResourceSummary.Status.ResourcesChanged
+		}, timeout, pollingInterval).Should(BeTrue())
+		latestResourceSummary.Status.HelmResourcesChanged = true
+		Expect(testEnv.Status().Update(context.TODO(), latestResourceSummary)).To(Succeed())
+
+		// Processing the stale copy must not reset the ResourceSummary: the Helm drift was never processed
+		err := controllers.ProcessResourceSummary(context.TODO(), testEnv.Client, listedResourceSummary,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).ToNot(BeNil())
+		Expect(apierrors.IsConflict(err)).To(BeTrue())
+
+		// Eventual loop so testEnv Cache is synced
+		Eventually(func() bool {
+			currentResourceSummary := &libsveltosv1beta1.ResourceSummary{}
+			err := testEnv.Get(context.TODO(), resourceSummaryKey, currentResourceSummary)
+			return err == nil && currentResourceSummary.Status.HelmResourcesChanged
+		}, timeout, pollingInterval).Should(BeTrue())
+
+		Consistently(func() bool {
+			currentResourceSummary := &libsveltosv1beta1.ResourceSummary{}
+			err := testEnv.Get(context.TODO(), resourceSummaryKey, currentResourceSummary)
+			return err == nil && currentResourceSummary.Status.HelmResourcesChanged
+		}, 2*time.Second, pollingInterval).Should(BeTrue())
+	})
+
 	It("processResourceSummary records the drifted resources of each feature in DriftHistory", func() {
 		cluster := prepareCluster()