diff --git a/e2e/assets/status/chart-with-template-vars/Chart.yaml b/e2e/assets/status/chart-with-template-vars/Chart.yaml new file mode 100644 index 0000000000..017949df22 --- /dev/null +++ b/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" diff --git a/e2e/assets/status/chart-with-template-vars/fleet.yaml b/e2e/assets/status/chart-with-template-vars/fleet.yaml new file mode 100644 index 0000000000..01e3b9bbaf --- /dev/null +++ b/e2e/assets/status/chart-with-template-vars/fleet.yaml @@ -0,0 +1,4 @@ +helm: + values: + templatedLabel: "${ .ClusterLabels.foo }-foo" + releaseName: reproducer diff --git a/e2e/assets/status/chart-with-template-vars/templates/configmap.yaml b/e2e/assets/status/chart-with-template-vars/templates/configmap.yaml new file mode 100644 index 0000000000..2327303f39 --- /dev/null +++ b/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 diff --git a/e2e/assets/status/gitrepo.yaml b/e2e/assets/status/gitrepo.yaml new file mode 100644 index 0000000000..c34afdb185 --- /dev/null +++ b/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 diff --git a/e2e/single-cluster/gitrepo_test.go b/e2e/single-cluster/gitrepo_test.go index 14102801f7..ae07cb38aa 100644 --- a/e2e/single-cluster/gitrepo_test.go +++ b/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) diff --git a/e2e/single-cluster/status_test.go b/e2e/single-cluster/status_test.go index 13d60e44dd..fbc8bbf06e 100644 --- a/e2e/single-cluster/status_test.go +++ b/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 +} diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index ecc4ea6109..51c7d9c338 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/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 +} diff --git a/internal/cmd/controller/reconciler/bundle_controller.go b/internal/cmd/controller/reconciler/bundle_controller.go index 72ae6ce040..a61cb7e2cc 100644 --- a/internal/cmd/controller/reconciler/bundle_controller.go +++ b/internal/cmd/controller/reconciler/bundle_controller.go @@ -5,8 +5,10 @@ package reconciler import ( "context" "fmt" + "time" "github.com/go-logr/logr" + "github.com/rancher/fleet/internal/cmd/controller/finalize" "github.com/rancher/fleet/internal/cmd/controller/summary" "github.com/rancher/fleet/internal/cmd/controller/target" @@ -15,8 +17,10 @@ import ( "github.com/rancher/fleet/internal/ociwrapper" fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" "github.com/rancher/fleet/pkg/sharding" + "github.com/rancher/wrangler/v3/pkg/genericcondition" corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -130,6 +134,7 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if err := r.Get(ctx, req.NamespacedName, bundle); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } + bundleOrig := bundle.DeepCopy() if bundle.Labels[fleet.RepoLabel] != "" { logger = logger.WithValues( @@ -215,6 +220,18 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr matchedTargets, err := r.Builder.Targets(ctx, bundle, manifestID) if err != nil { + // When targeting fails, we don't want to continue and we make the error message visible in + // the UI. For that we use a status condition of type Ready. + bundle.Status.Conditions = []genericcondition.GenericCondition{ + { + Type: string(fleet.Ready), + Status: v1.ConditionFalse, + Message: "Targeting error: " + err.Error(), + LastUpdateTime: metav1.Now().UTC().Format(time.RFC3339), + }, + } + + err := r.updateStatus(ctx, bundleOrig, bundle) return ctrl.Result{}, err } @@ -257,9 +274,8 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr summary.SetReadyConditions(&bundle.Status, "Cluster", bundle.Status.Summary) bundle.Status.ObservedGeneration = bundle.Generation - // build BundleDeployments out of targets discarding Status, replacing - // DependsOn with the bundle's DependsOn (pure function) and replacing - // the labels with the bundle's labels + // build BundleDeployments out of targets discarding Status, replacing DependsOn with the + // bundle's DependsOn (pure function) and replacing the labels with the bundle's labels for _, target := range matchedTargets { bd, err := r.createBundleDeployment(ctx, logger, target, contentsInOCI, manifestID) if err != nil { @@ -275,20 +291,7 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } updateDisplay(&bundle.Status) - - err = retry.RetryOnConflict(retry.DefaultRetry, func() error { - t := &fleet.Bundle{} - if err := r.Get(ctx, req.NamespacedName, t); err != nil { - return err - } - t.Status = bundle.Status - return r.Status().Update(ctx, t) - }) - if err != nil { - logger.V(1).Error(err, "Reconcile failed final update to bundle status", "status", bundle.Status) - } else { - metrics.BundleCollector.Collect(ctx, bundle) - } + err = r.updateStatus(ctx, bundleOrig, bundle) return ctrl.Result{}, err } @@ -340,22 +343,20 @@ func (r *BundleReconciler) createBundleDeployment( bd.Spec.OCIContents = contentsInOCI - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - if contentsInOCI { - return nil // no contents resources stored in etcd, no finalizers to add here. - } - + // contents resources stored in etcd, finalizers to add here. + if !contentsInOCI { content := &fleet.Content{} - if err := r.Get(ctx, types.NamespacedName{Name: manifestID}, content); err != nil { - return client.IgnoreNotFound(err) + err := r.Get(ctx, types.NamespacedName{Name: manifestID}, content) + if client.IgnoreNotFound(err) != nil { + logger.Error(err, "Reconcile failed to get content", "content ID", manifestID) + return nil, err } - controllerutil.AddFinalizer(content, bd.Name) - - return r.Update(ctx, content) - }) - if err != nil { - logger.Error(err, "Reconcile failed to add content finalizer", "content ID", manifestID) + if added := controllerutil.AddFinalizer(content, bd.Name); added { + if err := r.Update(ctx, content); err != nil { + logger.Error(err, "Reconcile failed to add content finalizer", "content ID", manifestID) + } + } } updated := bd.DeepCopy() @@ -450,3 +451,18 @@ func (r *BundleReconciler) getOCIReference(ctx context.Context, bundle *fleet.Bu // this is not a valid reference, it is only for display return fmt.Sprintf("oci://%s/%s:latest", string(ref), bundle.Spec.ContentsID), nil } + +// updateStatus patches the status of the bundle and collects metrics upon a successful update of +// the bundle status. It returns nil if the status update is successful, otherwise it returns an +// error. +func (r *BundleReconciler) updateStatus(ctx context.Context, orig *fleet.Bundle, bundle *fleet.Bundle) error { + logger := log.FromContext(ctx).WithName("bundle - updateStatus") + statusPatch := client.MergeFrom(orig) + err := r.Status().Patch(ctx, bundle, statusPatch) + if err != nil { + logger.V(1).Info("Reconcile failed update to bundle status", "status", bundle.Status, "error", err) + return err + } + metrics.BundleCollector.Collect(ctx, bundle) + return nil +}