(bug) deployment error

When Sveltos deploys Kubernetes resources using _PolicyRefs_
or _KustomizationRefs_, it should only remove stale resources
**after** the new resources have been successfully deployed.

If deployment fails for any reason, stale resources should not
be removed.

For example, if Sveltos previously deployed resources A, B, and C,
and a new update (with invalid data) attempts to update resource
B but fails, Sveltos currently treats B and C as stale. This is
because it successfully redeploys A, fails on B, and assumes only
A is needed—leading to incorrect cleanup.

Author: gianlucam76

api/v1beta1/zz_generated.deepcopy.go

@@ -146,6 +146,12 @@ func deployKustomizeRefs(ctx context.Context, c client.Client,
 		return err
 	}
 
+	// If a deployment error happened, do not try to clean stale resources. Because of the error potentially
+	// all resources might be considered stale at this time.
+	if deployError != nil {
+		return deployError
+	}
+
 	var undeployed []configv1beta1.ResourceReport
 	_, undeployed, err = cleanStaleKustomizeResources(ctx, remoteRestConfig, remoteClient, clusterSummary,
 		localResourceReports, remoteResourceReports, logger)
@@ -174,10 +180,6 @@ func deployKustomizeRefs(ctx context.Context, c client.Client,
 		return &configv1beta1.DryRunReconciliationError{}
 	}
 
-	if deployError != nil {
-		return deployError
-	}
-
 	return validateHealthPolicies(ctx, remoteRestConfig, clusterSummary, configv1beta1.FeatureKustomize, logger)
 }
 

controllers/handlers_kustomize.go

@@ -101,6 +101,12 @@ func deployResources(ctx context.Context, c client.Client,
 		return err
 	}
 
+	// If a deployment error happened, do not try to clean stale resources. Because of the error potentially
+	// all resources might be considered stale at this time.
+	if deployError != nil {
+		return deployError
+	}
+
 	var undeployed []configv1beta1.ResourceReport
 	_, undeployed, err = cleanStaleResources(ctx, remoteRestConfig, remoteClient, clusterSummary,
 		localResourceReports, remoteResourceReports, logger)
@@ -125,10 +131,6 @@ func deployResources(ctx context.Context, c client.Client,
 		return err
 	}
 
-	if deployError != nil {
-		return deployError
-	}
-
 	return validateHealthPolicies(ctx, remoteRestConfig, clusterSummary, configv1beta1.FeatureResources, logger)
 }
 

controllers/handlers_resources.go

@@ -0,0 +1,219 @@
+/*
+Copyright 2025. projectsveltos.io. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package fv_test
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+
+	configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1"
+	"github.com/projectsveltos/addon-controller/controllers"
+	libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1"
+)
+
+const (
+	serviceTemplate = `  apiVersion: v1
+  kind: Service
+  metadata:
+    namespace: %s
+    name: %s
+    labels:
+      app: my-app
+      environment: production
+  spec:
+    selector:
+      app: my-app
+    ports:
+      - port: 80
+        targetPort: 8080
+        protocol: TCP
+        name: http
+      - port: 443
+        targetPort: 8443
+        protocol: TCP
+        name: https
+    type: ClusterIP
+    sessionAffinity: None`
+
+	incorrectService = ` apiVersion: v1
+  kind: Service
+  metadata:
+    namespace: %s
+    name: %s
+    labels:
+      app: my-app
+      environment: production
+  spec:
+    # Missing required 'selector' field
+    ports:
+      - port: 80
+        targetPort: 8080
+        protocol: TCP
+        name: http
+      - port: 443
+        targetPort: 8443
+        protocol: TCP
+        name: https
+    type: ClusterIP
+    sessionAffinity: None`
+)
+
+// This test runs in Serial because it requires the addon controller to only onboard
+// capi cluster with onboard annotations
+var _ = Describe("Stale Resources", Serial, func() {
+	const (
+		namePrefix = "stale-resource"
+	)
+
+	It("Stale resources are removed only after successful deployment", Label("NEW-FV", "EXTENDED"), func() {
+		Byf("Create a ClusterProfile matching Cluster %s/%s", kindWorkloadCluster.Namespace, kindWorkloadCluster.Name)
+		clusterProfile := getClusterProfile(namePrefix, map[string]string{key: value})
+		clusterProfile.Spec.SyncMode = configv1beta1.SyncModeContinuous
+		Expect(k8sClient.Create(context.TODO(), clusterProfile)).To(Succeed())
+
+		verifyClusterProfileMatches(clusterProfile)
+
+		verifyClusterSummary(controllers.ClusterProfileLabelName,
+			clusterProfile.Name, &clusterProfile.Spec,
+			kindWorkloadCluster.Namespace, kindWorkloadCluster.Name)
+
+		configMapNs := randomString()
+		Byf("Create configMap's namespace %s", configMapNs)
+		ns := &corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: configMapNs,
+			},
+		}
+		Expect(k8sClient.Create(context.TODO(), ns)).To(Succeed())
+
+		service1 := randomString()
+		service2 := randomString()
+		service3 := randomString()
+
+		Byf("Create a configMap with Service")
+		configMap := createConfigMapWithPolicy(configMapNs, namePrefix+randomString(),
+			fmt.Sprintf(serviceTemplate, configMapNs, service1),
+			fmt.Sprintf(serviceTemplate, configMapNs, service2),
+			fmt.Sprintf(serviceTemplate, configMapNs, service3),
+		)
+		Expect(k8sClient.Create(context.TODO(), configMap)).To(Succeed())
+		currentConfigMap := &corev1.ConfigMap{}
+		Expect(k8sClient.Get(context.TODO(),
+			types.NamespacedName{Namespace: configMap.Namespace, Name: configMap.Name}, currentConfigMap)).To(Succeed())
+
+		Byf("Update ClusterProfile %s to reference ConfigMap %s/%s", clusterProfile.Name, configMap.Namespace, configMap.Name)
+		currentClusterProfile := &configv1beta1.ClusterProfile{}
+		Expect(k8sClient.Get(context.TODO(), types.NamespacedName{Name: clusterProfile.Name}, currentClusterProfile)).To(Succeed())
+		currentClusterProfile.Spec.PolicyRefs = []configv1beta1.PolicyRef{
+			{
+				Kind:      string(libsveltosv1beta1.ConfigMapReferencedResourceKind),
+				Namespace: configMap.Namespace,
+				Name:      configMap.Name,
+			},
+		}
+		Expect(k8sClient.Update(context.TODO(), currentClusterProfile)).To(Succeed())
+
+		clusterSummary := verifyClusterSummary(controllers.ClusterProfileLabelName,
+			currentClusterProfile.Name, &currentClusterProfile.Spec,
+			kindWorkloadCluster.Namespace, kindWorkloadCluster.Name)
+
+		Byf("Getting client to access the workload cluster")
+		workloadClient, err := getKindWorkloadClusterKubeconfig()
+		Expect(err).To(BeNil())
+		Expect(workloadClient).ToNot(BeNil())
+
+		for _, serviceName := range []string{service1, service2, service3} {
+			Byf("Verifying Service %s is created in the workload cluster", serviceName)
+			Eventually(func() error {
+				currentService := &corev1.Service{}
+				return workloadClient.Get(context.TODO(),
+					types.NamespacedName{Namespace: configMapNs, Name: serviceName}, currentService)
+			}, timeout, pollingInterval).Should(BeNil())
+		}
+
+		Byf("Verifying ClusterSummary %s status is set to Deployed for Resources feature", clusterSummary.Name)
+		verifyFeatureStatusIsProvisioned(kindWorkloadCluster.Namespace, clusterSummary.Name, configv1beta1.FeatureResources)
+
+		By("Updating ConfigMap to reference incorrect Service")
+		Expect(k8sClient.Get(context.TODO(),
+			types.NamespacedName{Namespace: configMap.Namespace, Name: configMap.Name}, currentConfigMap)).To(Succeed())
+		allClusterRoleName := randomString()
+		currentConfigMap = updateConfigMapWithPolicy(currentConfigMap, fmt.Sprintf(allClusterRole, allClusterRoleName))
+		Expect(k8sClient.Update(context.TODO(), currentConfigMap)).To(Succeed())
+
+		By("Updating ConfigMap to reference also an incorrect Service")
+		incorrectServiceName := randomString()
+		Expect(k8sClient.Get(context.TODO(),
+			types.NamespacedName{Namespace: configMap.Namespace, Name: configMap.Name}, currentConfigMap)).To(Succeed())
+		currentConfigMap = updateConfigMapWithPolicy(currentConfigMap,
+			fmt.Sprintf(serviceTemplate, configMapNs, service1),
+			fmt.Sprintf(incorrectService, configMapNs, incorrectServiceName),
+			fmt.Sprintf(serviceTemplate, configMapNs, service2),
+			fmt.Sprintf(serviceTemplate, configMapNs, service3))
+		Expect(k8sClient.Update(context.TODO(), currentConfigMap)).To(Succeed())
+
+		for _, serviceName := range []string{service1, service2, service3} {
+			Byf("Verifying Service %s is created in the workload cluster", serviceName)
+			Consistently(func() error {
+				currentService := &corev1.Service{}
+				return workloadClient.Get(context.TODO(),
+					types.NamespacedName{Namespace: configMapNs, Name: serviceName}, currentService)
+			}, time.Minute, pollingInterval).Should(BeNil())
+		}
+
+		Byf("Verifying Service %s is created in not the workload cluster", incorrectServiceName)
+		Consistently(func() error {
+			currentService := &corev1.Service{}
+			return workloadClient.Get(context.TODO(),
+				types.NamespacedName{Namespace: configMapNs, Name: incorrectServiceName}, currentService)
+		}, time.Minute, pollingInterval).ShouldNot(BeNil())
+
+		By("Updating ConfigMap to reference also a fourth Service")
+		service4 := randomString()
+		Expect(k8sClient.Get(context.TODO(),
+			types.NamespacedName{Namespace: configMap.Namespace, Name: configMap.Name}, currentConfigMap)).To(Succeed())
+		currentConfigMap = updateConfigMapWithPolicy(currentConfigMap,
+			fmt.Sprintf(serviceTemplate, configMapNs, service1),
+			fmt.Sprintf(serviceTemplate, configMapNs, service4),
+			fmt.Sprintf(serviceTemplate, configMapNs, service2),
+			fmt.Sprintf(serviceTemplate, configMapNs, service3))
+		Expect(k8sClient.Update(context.TODO(), currentConfigMap)).To(Succeed())
+
+		for _, serviceName := range []string{service1, service2, service3, service4} {
+			Byf("Verifying Service %s is created in the workload cluster", serviceName)
+			Eventually(func() error {
+				currentService := &corev1.Service{}
+				return workloadClient.Get(context.TODO(),
+					types.NamespacedName{Namespace: configMapNs, Name: serviceName}, currentService)
+			}, timeout, pollingInterval).Should(BeNil())
+		}
+
+		deleteClusterProfile(clusterProfile)
+
+		currentNs := &corev1.Namespace{}
+		Expect(k8sClient.Get(context.TODO(), types.NamespacedName{Name: configMapNs}, currentNs)).To(Succeed())
+		Expect(k8sClient.Delete(context.TODO(), currentNs)).To(Succeed())
+	})
+})