[SURE-9137] Add template errors to bundle and gitrepo status (#3114) (#…

…3196)

* Import v1alpha1 package as fleet

* Show bundle errors in Bundle and GitRepo

Refers to #2943

* Add E2E tests

Refers to #2943

(cherry picked from commit 235e8ef)
(cherry picked from commit 3417071)

e2e/assets/status/chart-with-template-vars/Chart.yaml

@@ -0,0 +1,24 @@
+apiVersion: v2
+name: chart-with-template-vars
+description: A Helm chart for Kubernetes
+
+# A chart can be either an 'application' or a 'library' chart.
+#
+# Application charts are a collection of templates that can be packaged into versioned archives
+# to be deployed.
+#
+# Library charts provide useful utilities or functions for the chart developer. They're included as
+# a dependency of application charts to inject those utilities and functions into the rendering
+# pipeline. Library charts do not define any templates and therefore cannot be deployed.
+type: application
+
+# This is the chart version. This version number should be incremented each time you make changes
+# to the chart and its templates, including the app version.
+# Versions are expected to follow Semantic Versioning (https://semver.org/)
+version: 0.1.0
+
+# This is the version number of the application being deployed. This version number should be
+# incremented each time you make changes to the application. Versions are not expected to
+# follow Semantic Versioning. They should reflect the version the application is using.
+# It is recommended to use it with quotes.
+appVersion: "1.16.0"

e2e/assets/status/chart-with-template-vars/fleet.yaml

@@ -0,0 +1,4 @@
+helm:
+  values:
+    templatedLabel: "${ .ClusterLabels.foo }-foo"
+  releaseName: reproducer

e2e/assets/status/chart-with-template-vars/templates/configmap.yaml

@@ -0,0 +1,7 @@
+kind: ConfigMap
+apiVersion: v1
+metadata:
+  name: chart-with-template-vars-configmap
+  namespace: fleet-local
+data:
+  foo: bar

e2e/assets/status/gitrepo.yaml

@@ -0,0 +1,11 @@
+kind: GitRepo
+apiVersion: fleet.cattle.io/v1alpha1
+metadata:
+  name: {{.Name}}
+  namespace: fleet-local
+spec:
+  repo: {{.Repo}}
+  branch: {{.Branch}}
+  targetNamespace: {{.TargetNamespace}}
+  paths:
+  - examples

e2e/single-cluster/gitrepo_test.go

@@ -192,7 +192,7 @@ var _ = Describe("Monitoring Git repos via HTTP for change", Label("infra-setup"
 				},
 			}
 			Eventually(func(g Gomega) {
-				status := getGitRepoStatus(k, gitrepoName)
+				status := getGitRepoStatus(g, k, gitrepoName)
 				g.Expect(status).To(matchGitRepoStatus(expectedStatus))
 			}).Should(Succeed())
 
@@ -351,7 +351,7 @@ var _ = Describe("Monitoring Git repos via HTTP for change", Label("infra-setup"
 				},
 			}
 			Eventually(func(g Gomega) {
-				status := getGitRepoStatus(k, gitrepoName)
+				status := getGitRepoStatus(g, k, gitrepoName)
 				g.Expect(status).To(matchGitRepoStatus(expectedStatus))
 
 			}).Should(Succeed())
@@ -377,10 +377,10 @@ func replace(path string, s string, r string) {
 }
 
 // getGitRepoStatus retrieves the status of the gitrepo with the provided name.
-func getGitRepoStatus(k kubectl.Command, name string) fleet.GitRepoStatus {
+func getGitRepoStatus(g Gomega, k kubectl.Command, name string) fleet.GitRepoStatus {
 	gr, err := k.Get("gitrepo", name, "-o=json")
 
-	Expect(err).ToNot(HaveOccurred())
+	g.Expect(err).ToNot(HaveOccurred())
 
 	var gitrepo fleet.GitRepo
 	_ = json.Unmarshal([]byte(gr), &gitrepo)

e2e/single-cluster/status_test.go

@@ -1,13 +1,23 @@
 package singlecluster_test
 
 import (
+	"encoding/json"
 	"errors"
+	"fmt"
+	"math/rand"
+	"os"
+	"path"
 	"strings"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+
 	"github.com/rancher/fleet/e2e/testenv"
+	"github.com/rancher/fleet/e2e/testenv/githelper"
 	"github.com/rancher/fleet/e2e/testenv/kubectl"
+	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
+	"github.com/rancher/wrangler/v3/pkg/genericcondition"
+	corev1 "k8s.io/api/core/v1"
 )
 
 var _ = Describe("Checks status updates happen for a simple deployment", Ordered, func() {
@@ -108,3 +118,161 @@ var _ = Describe("Checks status updates happen for a simple deployment", Ordered
 		})
 	})
 })
+
+var _ = Describe("Checks that template errors are shown in bundles and gitrepos", Ordered, Label("infra-setup"), func() {
+	var (
+		tmpDir           string
+		cloneDir         string
+		k                kubectl.Command
+		gh               *githelper.Git
+		repoName         string
+		inClusterRepoURL string
+		gitrepoName      string
+		r                = rand.New(rand.NewSource(GinkgoRandomSeed()))
+		targetNamespace  string
+	)
+
+	BeforeEach(func() {
+		k = env.Kubectl.Namespace(env.Namespace)
+		repoName = "repo"
+	})
+
+	JustBeforeEach(func() {
+		// Build git repo URL reachable _within_ the cluster, for the GitRepo
+		host, err := githelper.BuildGitHostname(env.Namespace)
+		Expect(err).ToNot(HaveOccurred())
+
+		addr, err := githelper.GetExternalRepoAddr(env, port, repoName)
+		Expect(err).ToNot(HaveOccurred())
+		gh = githelper.NewHTTP(addr)
+
+		inClusterRepoURL = gh.GetInClusterURL(host, port, repoName)
+
+		tmpDir, _ = os.MkdirTemp("", "fleet-")
+		cloneDir = path.Join(tmpDir, repoName)
+
+		gitrepoName = testenv.RandomFilename("status-test", r)
+
+		_, err = gh.Create(cloneDir, testenv.AssetPath("status/chart-with-template-vars"), "examples")
+		Expect(err).ToNot(HaveOccurred())
+
+		err = testenv.ApplyTemplate(k, testenv.AssetPath("status/gitrepo.yaml"), struct {
+			Name            string
+			Repo            string
+			Branch          string
+			TargetNamespace string
+		}{
+			gitrepoName,
+			inClusterRepoURL,
+			gh.Branch,
+			targetNamespace, // to avoid conflicts with other tests
+		})
+		Expect(err).ToNot(HaveOccurred())
+	})
+
+	AfterEach(func() {
+		_ = os.RemoveAll(tmpDir)
+
+		_, err := k.Delete("gitrepo", gitrepoName)
+		Expect(err).ToNot(HaveOccurred())
+
+		// Check that the bundle deployment resource has been deleted
+		Eventually(func(g Gomega) {
+			out, _ := k.Get(
+				"bundledeployments",
+				"-A",
+				"-l",
+				fmt.Sprintf("fleet.cattle.io/repo-name=%s", gitrepoName),
+			)
+			g.Expect(out).To(ContainSubstring("No resources found"))
+		}).Should(Succeed())
+
+		// Deleting the targetNamespace is not necessary when the GitRepo did not successfully
+		// render, as in a few test cases here. If no targetNamespace was created, trying to delete
+		// the namespace will result in an error, which is why we are not checking for errors when
+		// deleting namespaces here.
+		_, _ = k.Delete("ns", targetNamespace)
+	})
+
+	expectNoError := func(g Gomega, conditions []genericcondition.GenericCondition) {
+		for _, condition := range conditions {
+			if condition.Type == string(fleet.Ready) {
+				g.Expect(condition.Status).To(Equal(corev1.ConditionTrue))
+				g.Expect(condition.Message).To(BeEmpty())
+				break
+			}
+		}
+	}
+
+	expectTargetingError := func(g Gomega, conditions []genericcondition.GenericCondition) {
+		found := false
+		for _, condition := range conditions {
+			if condition.Type == string(fleet.Ready) {
+				g.Expect(condition.Status).To(Equal(corev1.ConditionFalse))
+				g.Expect(condition.Message).To(ContainSubstring("Targeting error"))
+				g.Expect(condition.Message).To(
+					ContainSubstring(
+						"<.ClusterLabels.foo>: map has no entry for key \"foo\""))
+				found = true
+				break
+			}
+		}
+		g.Expect(found).To(BeTrue())
+	}
+
+	ensureClusterHasLabelFoo := func() (string, error) {
+		return k.Namespace("fleet-local").
+			Patch("cluster", "local", "--type", "json", "--patch",
+				`[{"op": "add", "path": "/metadata/labels/foo", "value": "bar"}]`)
+	}
+
+	ensureClusterHasNoLabelFoo := func() (string, error) {
+		return k.Namespace("fleet-local").
+			Patch("cluster", "local", "--type", "json", "--patch",
+				`[{"op": "remove", "path": "/metadata/labels/foo"}]`)
+	}
+
+	When("a git repository is created that contains a template error", func() {
+		BeforeEach(func() {
+			targetNamespace = testenv.NewNamespaceName("target", r)
+		})
+
+		It("should have an error in the bundle", func() {
+			_, _ = ensureClusterHasNoLabelFoo()
+			Eventually(func(g Gomega) {
+				status := getBundleStatus(g, k, gitrepoName+"-examples")
+				expectTargetingError(g, status.Conditions)
+			}).Should(Succeed())
+		})
+
+		It("should have an error in the gitrepo", func() {
+			_, _ = ensureClusterHasNoLabelFoo()
+			Eventually(func(g Gomega) {
+				status := getGitRepoStatus(g, k, gitrepoName)
+				expectTargetingError(g, status.Conditions)
+			}).Should(Succeed())
+		})
+	})
+
+	When("a git repository is created that contains no template error", func() {
+		It("should have no error in the bundle", func() {
+			_, _ = ensureClusterHasLabelFoo()
+			Eventually(func(g Gomega) {
+				status := getBundleStatus(g, k, gitrepoName+"-examples")
+				expectNoError(g, status.Conditions)
+			}).Should(Succeed())
+		})
+	})
+})
+
+// getBundleStatus retrieves the status of the bundle with the provided name.
+func getBundleStatus(g Gomega, k kubectl.Command, name string) fleet.BundleStatus {
+	gr, err := k.Get("bundle", name, "-o=json")
+
+	g.Expect(err).ToNot(HaveOccurred())
+
+	var bundle fleet.Bundle
+	_ = json.Unmarshal([]byte(gr), &bundle)
+
+	return bundle.Status
+}

internal/cmd/controller/gitops/reconciler/gitjob_controller.go

@@ -22,10 +22,12 @@ import (
 	"github.com/rancher/fleet/pkg/sharding"
 
 	"github.com/rancher/wrangler/v3/pkg/condition"
+	"github.com/rancher/wrangler/v3/pkg/genericcondition"
 	"github.com/rancher/wrangler/v3/pkg/name"
 
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -227,6 +229,29 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 
 	SetCondition(&gitrepo.Status, nil)
 
+	// We're explicitly setting the ready status from a bundle here, but only if it isn't ready.
+	//
+	// - If the bundle has no deployments, there is no status to be copied from the setStatus
+	// function, so that we won't overwrite anything.
+	//
+	// - If the bundle has rendering issues and there are deployments of which there is at least one
+	// in a failed state, the status of the bundle deployments would be overwritten by the bundle
+	// status.
+	//
+	// - If the bundle has no rendering issues but there are deployments in a failed state, the code
+	// will overwrite the gitrepo's ready status condition with the ready status condition coming
+	// from the bundle. Because both have the same content, we can unconditionally set the status
+	// from the bundle.
+	//
+	// So we're basically just making sure the status from the bundle is being set on the gitrepo,
+	// even if there are no bundle deployments, which is the case for issues with rendering the
+	// manifests, for instance. In that case no bundle deployments are created, but an error is set
+	// in a ready status condition on the bundle.
+	err = r.setReadyStatusFromBundle(ctx, gitrepo)
+	if err != nil {
+		return result(repoPolled, gitrepo), err
+	}
+
 	err = updateStatus(ctx, r.Client, req.NamespacedName, gitrepo.Status)
 	if err != nil {
 		logger.Error(err, "Reconcile failed final update to git repo status", "status", gitrepo.Status)
@@ -1241,3 +1266,60 @@ func jobUpdatedPredicate() predicate.Funcs {
 		},
 	}
 }
+
+// setReadyStatusFromBundle fetches all bundles from a given gitrepo, checks the ready status conditions
+// from the bundles and applies one on the gitrepo if it isn't ready. The purpose is to make
+// rendering issues visible in the gitrepo status. Those issues need to be made explicitly visible
+// since the other statuses are calculated from bundle deployments, which do not exist when
+// rendering manifests fail. Should an issue be on the bundle, it will be copied to the gitrepo.
+func (r *GitJobReconciler) setReadyStatusFromBundle(ctx context.Context, gitrepo *v1alpha1.GitRepo) error {
+	bList := &v1alpha1.BundleList{}
+	err := r.List(ctx, bList, client.MatchingLabels{
+		v1alpha1.RepoLabel: gitrepo.Name,
+	}, client.InNamespace(gitrepo.Namespace))
+	if err != nil {
+		return err
+	}
+
+	found := false
+	// Find a ready status condition in a bundle which is not ready.
+	var condition genericcondition.GenericCondition
+bundles:
+	for _, bundle := range bList.Items {
+		if bundle.Status.Conditions == nil {
+			continue
+		}
+
+		for _, c := range bundle.Status.Conditions {
+			if c.Type == string(v1alpha1.Ready) && c.Status == v1.ConditionFalse {
+				condition = c
+				found = true
+				break bundles
+			}
+		}
+	}
+
+	// No ready condition found in any bundle, nothing to do here.
+	if !found {
+		return nil
+	}
+
+	found = false
+	newConditions := make([]genericcondition.GenericCondition, 0, len(gitrepo.Status.Conditions))
+	for _, c := range gitrepo.Status.Conditions {
+		if c.Type == string(v1alpha1.Ready) {
+			// Replace the ready condition with the one from the bundle
+			newConditions = append(newConditions, condition)
+			found = true
+			continue
+		}
+		newConditions = append(newConditions, c)
+	}
+	if !found {
+		// Add the ready condition from the bundle to the gitrepo.
+		newConditions = append(newConditions, condition)
+	}
+	gitrepo.Status.Conditions = newConditions
+
+	return nil
+}