bug: handle errors when SyncMode is set to DryRun

Makefile

@@ -80,7 +80,7 @@ KUBECTL := $(TOOLS_BIN_DIR)/kubectl
 
 GOVULNCHECK_VERSION := "v1.1.4"
 GOLANGCI_LINT_VERSION := "v2.12.2"
-CLUSTERCTL_VERSION := v1.13.3
+CLUSTERCTL_VERSION := v1.13.4
 KIND_VERSION := v0.32.0
 
 KUSTOMIZE_VER := v5.8.0

config/crd/bases/config.projectsveltos.io_clusterreports.yaml

@@ -71,6 +71,7 @@ spec:
                       - Update
                       - Delete
                       - Conflict
+                      - Error
                       type: string
                     message:
                       description: |-
@@ -138,6 +139,7 @@ spec:
                       - Update
                       - Delete
                       - Conflict
+                      - Error
                       type: string
                     message:
                       description: |-
@@ -246,6 +248,7 @@ spec:
                       - Update
                       - Delete
                       - Conflict
+                      - Error
                       type: string
                     message:
                       description: |-

controllers/export_test.go

@@ -124,8 +124,9 @@ var (
 	AddExtraAnnotations = addExtraAnnotations
 	AdjustNamespace     = adjustNamespace
 
-	ResourcesHash   = resourcesHash
-	GetResourceRefs = getResourceRefs
+	ResourcesHash                 = resourcesHash
+	GetResourceRefs               = getResourceRefs
+	PersistResourceReportsOnError = persistResourceReportsOnError
 
 	UndeployKustomizeRefs             = undeployKustomizeRefs
 	KustomizationHash                 = kustomizationHash

controllers/handlers_resources.go

@@ -160,7 +160,8 @@ func postProcessDeployedResources(ctx context.Context, remoteRestConfig *rest.Co
 	// 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
+		return persistResourceReportsOnError(ctx, c, clusterSummary, isPullMode, remoteResourceReports,
+			featureHandler.id, deployError)
 	}
 
 	// Agent in the managed cluster will take care of this for SveltosClusters in pull mode. We still need to
@@ -697,6 +698,19 @@ func getResourceRefs(clusterSummary *configv1beta1.ClusterSummary) []configv1bet
 	return clusterSummary.Spec.ClusterProfileSpec.PolicyRefs
 }
 
+// persistResourceReportsOnError still writes whatever ResourceReports were generated before returning
+// deployError, so DryRun's ClusterReport isn't left unpopulated just because one resource failed to apply.
+func persistResourceReportsOnError(ctx context.Context, c client.Client, clusterSummary *configv1beta1.ClusterSummary,
+	isPullMode bool, remoteResourceReports []libsveltosv1beta1.ResourceReport, featureID libsveltosv1beta1.FeatureID,
+	deployError error) error {
+
+	if reportErr := updateClusterReportWithResourceReports(ctx, c, clusterSummary, isPullMode, remoteResourceReports,
+		featureID); reportErr != nil {
+		return reportErr
+	}
+	return deployError
+}
+
 // updateClusterReportWithResourceReports updates ClusterReport Status with ResourceReports.
 // This is no-op unless mode is DryRun.
 func updateClusterReportWithResourceReports(ctx context.Context, c client.Client,

controllers/handlers_resources_test.go

@@ -32,6 +32,8 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog/v2/textlogger"
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1"
 	"github.com/projectsveltos/addon-controller/controllers"
@@ -314,6 +316,49 @@ var _ = Describe("HandlersResource", func() {
 		Expect(clusterSummary.Status.DeployedGVKs[0].DeployedGroupVersionKind).To(ContainElement(
 			fmt.Sprintf("%s.%s.%s", remoteReports[0].Resource.Kind, remoteReports[0].Resource.Version, remoteReports[0].Resource.Group)))
 	})
+
+	It("persistResourceReportsOnError still persists ResourceReports on ClusterReport before returning the deploy error", func() {
+		clusterSummary.Spec.ClusterProfileSpec.SyncMode = configv1beta1.SyncModeDryRun
+
+		clusterReport := &configv1beta1.ClusterReport{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: clusterops.GetClusterReportName(configv1beta1.ClusterProfileKind,
+					clusterProfile.Name, clusterSummary.Spec.ClusterName, clusterSummary.Spec.ClusterType),
+				Namespace: clusterSummary.Spec.ClusterNamespace,
+			},
+			Spec: configv1beta1.ClusterReportSpec{
+				ClusterNamespace: clusterSummary.Spec.ClusterNamespace,
+				ClusterName:      clusterSummary.Spec.ClusterName,
+			},
+		}
+
+		initObjects := []client.Object{clusterSummary, clusterReport}
+		c := fake.NewClientBuilder().WithScheme(scheme).
+			WithStatusSubresource(initObjects...).WithObjects(initObjects...).Build()
+
+		remoteReports := []libsveltosv1beta1.ResourceReport{
+			{
+				Action:   string(libsveltosv1beta1.CreateResourceAction),
+				Resource: libsveltosv1beta1.Resource{Name: randomString(), Kind: randomString()},
+			},
+			{
+				Action:   string(libsveltosv1beta1.ErrorResourceAction),
+				Message:  "StatefulSet.apps is invalid: spec: Forbidden: updates to statefulset spec for fields other than allowed ones are forbidden",
+				Resource: libsveltosv1beta1.Resource{Name: randomString(), Kind: "StatefulSet"},
+			},
+		}
+		deployErr := fmt.Errorf("deploying resource StatefulSet %s/%s failed", namespace, randomString())
+
+		err := controllers.PersistResourceReportsOnError(context.TODO(), c, clusterSummary, false, remoteReports,
+			libsveltosv1beta1.FeatureResources, deployErr)
+		Expect(err).To(Equal(deployErr))
+
+		currentClusterReport := &configv1beta1.ClusterReport{}
+		Expect(c.Get(context.TODO(),
+			types.NamespacedName{Name: clusterReport.Name, Namespace: clusterReport.Namespace}, currentClusterReport)).To(Succeed())
+		Expect(currentClusterReport.Status.ResourceReports).To(HaveLen(2))
+		Expect(currentClusterReport.Status.ResourceReports).To(ContainElement(remoteReports[1]))
+	})
 })
 
 var _ = Describe("Hash methods", func() {

controllers/handlers_utils.go

@@ -524,6 +524,7 @@ func deployUnstructured(ctx context.Context, deployingToMgmtCluster bool, destCo
 		if err != nil {
 			logger.Error(err, fmt.Sprintf("failed to deploy resource %s %s/%s",
 				policy.GetKind(), policy.GetNamespace(), policy.GetName()))
+			reports = recordResourceApplyError(reports, clusterSummary.Spec.ClusterProfileSpec.SyncMode, resource, err)
 			if clusterSummary.Spec.ClusterProfileSpec.ContinueOnError {
 				errorMsg += fmt.Sprintf("%s: %v", errorPrefix, err)
 				continue
@@ -536,6 +537,17 @@ func deployUnstructured(ctx context.Context, deployingToMgmtCluster bool, destCo
 		clusterSummary.Spec.ClusterProfileSpec.SyncMode == configv1beta1.SyncModeDryRun)
 }
 
+// recordResourceApplyError appends a ResourceReport documenting a resource's apply failure when in DryRun
+// mode, so the failure surfaces in ClusterReport instead of the resource silently missing from it.
+func recordResourceApplyError(reports []libsveltosv1beta1.ResourceReport, syncMode configv1beta1.SyncMode,
+	resource *libsveltosv1beta1.Resource, err error) []libsveltosv1beta1.ResourceReport {
+
+	if syncMode != configv1beta1.SyncModeDryRun {
+		return reports
+	}
+	return append(reports, *deployer.GenerateErrorResourceReport(resource, err))
+}
+
 // requeueAllOldOwners gets the list of all ClusterProfile/Profile instances currently owning the resource in the
 // managed cluster (profiles). For each one, it finds the corresponding ClusterSummary and via requeueOldOwner reset
 // the Status so a new reconciliation happens.

controllers/handlers_utils_test.go

@@ -111,6 +111,26 @@ spec:
         ports:
         - containerPort: 80`
 
+	statefulSetTemplate = `apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  serviceName: %s
+  replicas: 1
+  selector:
+    matchLabels:
+      app: %s
+  template:
+    metadata:
+      labels:
+        app: %s
+    spec:
+      containers:
+      - name: main
+        image: nginx:latest`
+
 	deplTemplateWithStatus = `apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -566,7 +586,7 @@ var _ = Describe("HandlersUtils", func() {
 			textlogger.NewLogger(textlogger.NewConfig()))
 		Expect(err).To(BeNil())
 		By("Validating action for all resourceReports is Create")
-		validateResourceReports(resourceReports, 2, 0, 0, 0)
+		validateResourceReports(resourceReports, 2, 0, 0, 0, 0)
 
 		// Create services in the workload cluster and have their content exactly match
 		// the content contained in the secret referenced by ClusterProfile.
@@ -598,7 +618,7 @@ var _ = Describe("HandlersUtils", func() {
 			textlogger.NewLogger(textlogger.NewConfig()))
 		Expect(err).To(BeNil())
 		By("Validating action for all resourceReports is NoAction")
-		validateResourceReports(resourceReports, 0, 0, 2, 0)
+		validateResourceReports(resourceReports, 0, 0, 2, 0, 0)
 
 		// Update the secret referenced by ClusterProfile by changing the content of the
 		// two services by adding extra label
@@ -636,7 +656,7 @@ var _ = Describe("HandlersUtils", func() {
 			textlogger.NewLogger(textlogger.NewConfig()))
 		Expect(err).To(BeNil())
 		By("Validating action for all resourceReports is Update")
-		validateResourceReports(resourceReports, 0, 2, 0, 0)
+		validateResourceReports(resourceReports, 0, 2, 0, 0, 0)
 
 		// Pass a different secret to DeployContent, which means the services are contained in a different Secret
 		// and that is the one referenced by ClusterSummary. DeployContent will report conflicts in this case.
@@ -647,7 +667,7 @@ var _ = Describe("HandlersUtils", func() {
 			textlogger.NewLogger(textlogger.NewConfig()))
 		Expect(err).To(BeNil())
 		By("Validating action for all resourceReports is Conflict")
-		validateResourceReports(resourceReports, 0, 0, 0, 2)
+		validateResourceReports(resourceReports, 0, 0, 0, 2, 0)
 		for i := range resourceReports {
 			rr := &resourceReports[i]
 
@@ -658,6 +678,44 @@ var _ = Describe("HandlersUtils", func() {
 		}
 	})
 
+	It("deployContent in DryRun mode reports a resource whose dry-run apply is rejected as Error", func() {
+		statefulSetName := randomString()
+		serviceName := randomString()
+
+		clusterSummary.Spec.ClusterProfileSpec.SyncMode = configv1beta1.SyncModeDryRun
+
+		// Create the StatefulSet directly (outside of Sveltos), so a later DryRun apply attempting to
+		// change its (immutable) selector is rejected by the API server, exactly like a real StatefulSet
+		// spec update Sveltos did not create would be.
+		initialStatefulSet := fmt.Sprintf(statefulSetTemplate, statefulSetName, namespace, serviceName, "original", "original")
+		initialPolicy, err := k8s_utils.GetUnstructured([]byte(initialStatefulSet))
+		Expect(err).To(BeNil())
+		Expect(testEnv.Create(context.TODO(), initialPolicy)).To(Succeed())
+		Expect(waitForObject(context.TODO(), testEnv.Client, initialPolicy)).To(Succeed())
+
+		changedStatefulSet := fmt.Sprintf(statefulSetTemplate, statefulSetName, namespace, serviceName, "changed", "changed")
+		secret := createSecretWithPolicy(namespace, randomString(), changedStatefulSet)
+
+		Expect(addTypeInformationToObject(testEnv.Scheme(), secret)).To(Succeed())
+		Expect(addTypeInformationToObject(testEnv.Scheme(), clusterSummary)).To(Succeed())
+
+		clusterObjects, err := controllers.FetchClusterObjects(context.TODO(), testEnv.Config, testEnv.Client,
+			clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName, libsveltosv1beta1.ClusterTypeCapi,
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).To(BeNil())
+
+		resourceReports, err := controllers.DeployContent(context.TODO(), false,
+			testEnv.Config, testEnv.Client, secret, map[string]string{"statefulset": changedStatefulSet},
+			defaultTier, false, false, controllers.NewDeploymentContext(clusterSummary, clusterObjects, nil),
+			textlogger.NewLogger(textlogger.NewConfig()))
+		Expect(err).ToNot(BeNil())
+		Expect(err.Error()).To(ContainSubstring("Forbidden"))
+
+		By("Validating the failing resource is still reported, with action Error")
+		validateResourceReports(resourceReports, 0, 0, 0, 0, 1)
+		Expect(resourceReports[0].Message).To(ContainSubstring("Forbidden"))
+	})
+
 	It("deployContentOfSecret deploys all policies contained in a ConfigMap", func() {
 		services := fmt.Sprintf(serviceTemplate, namespace, namespace)
 		depl := fmt.Sprintf(deplTemplate, namespace)
@@ -1576,9 +1634,9 @@ var _ = Describe("prepareBundleSettersWithResourceInfo", func() {
 // validateResourceReports validates that number of resourceResources with certain actions
 // match the expected number per action
 func validateResourceReports(resourceReports []libsveltosv1beta1.ResourceReport,
-	created, updated, noAction, conflict int) {
+	created, updated, noAction, conflict, errored int) {
 
-	var foundCreated, foundUpdated, foundNoAction, foundConflict int
+	var foundCreated, foundUpdated, foundNoAction, foundConflict, foundErrored int
 	for i := range resourceReports {
 		rr := &resourceReports[i]
 		if rr.Action == string(libsveltosv1beta1.CreateResourceAction) {
@@ -1589,11 +1647,14 @@ func validateResourceReports(resourceReports []libsveltosv1beta1.ResourceReport,
 			foundNoAction++
 		} else if rr.Action == string(libsveltosv1beta1.ConflictResourceAction) {
 			foundConflict++
+		} else if rr.Action == string(libsveltosv1beta1.ErrorResourceAction) {
+			foundErrored++
 		}
 	}
 
 	Expect(foundCreated).To(Equal(created))
 	Expect(foundUpdated).To(Equal(updated))
 	Expect(foundNoAction).To(Equal(noAction))
 	Expect(foundConflict).To(Equal(conflict))
+	Expect(foundErrored).To(Equal(errored))
 }