From bfa033e2bffdee39b7bde43b927183dd0431384d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Corentin=20N=C3=A9au?= Date: Thu, 7 Nov 2024 17:58:44 +0100 Subject: [PATCH 1/2] Fix namespace target customization support with no defaults (#3052) * Improve namespace target customization tests These tests now verify that the created namespace does bear expected labels and annotations. This commit also paves the way for additional tests with customizations over unconfigured namespace labels and annotations, which currently cause a panic. * Initialise options maps when empty This prevents panics when namespace labels or annotations are configured as target customizations over nonexistent defaults. * Use main branch of `rancher/fleet-test-data The required changes made in that repo have been merged. --- ...bels-target-customization-no-defaults.yaml | 10 ++ ...ml => ns-labels-target-customization.yaml} | 2 +- .../targetCustomization_test.go | 101 ----------- .../target_customization_test.go | 166 ++++++++++++++++++ internal/cmd/controller/target/target.go | 23 +++ 5 files changed, 200 insertions(+), 102 deletions(-) create mode 100644 e2e/assets/single-cluster/ns-labels-target-customization-no-defaults.yaml rename e2e/assets/single-cluster/{namespaceLabels_targetCustomization.yaml => ns-labels-target-customization.yaml} (73%) delete mode 100644 e2e/single-cluster/targetCustomization_test.go create mode 100644 e2e/single-cluster/target_customization_test.go diff --git a/e2e/assets/single-cluster/ns-labels-target-customization-no-defaults.yaml b/e2e/assets/single-cluster/ns-labels-target-customization-no-defaults.yaml new file mode 100644 index 0000000000..fac66d415e --- /dev/null +++ b/e2e/assets/single-cluster/ns-labels-target-customization-no-defaults.yaml @@ -0,0 +1,10 @@ +kind: GitRepo +apiVersion: fleet.cattle.io/v1alpha1 +metadata: + name: test + namespace: fleet-local +spec: + repo: https://github.com/rancher/fleet-test-data + branch: master + paths: + - target-customization-namespace-labels/without-default-values diff --git a/e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml b/e2e/assets/single-cluster/ns-labels-target-customization.yaml similarity index 73% rename from e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml rename to e2e/assets/single-cluster/ns-labels-target-customization.yaml index 337746ae93..779a616ec5 100644 --- a/e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml +++ b/e2e/assets/single-cluster/ns-labels-target-customization.yaml @@ -7,4 +7,4 @@ spec: repo: https://github.com/rancher/fleet-test-data branch: master paths: - - target-customization-namespace-labels + - target-customization-namespace-labels/with-default-values diff --git a/e2e/single-cluster/targetCustomization_test.go b/e2e/single-cluster/targetCustomization_test.go deleted file mode 100644 index 651563e06a..0000000000 --- a/e2e/single-cluster/targetCustomization_test.go +++ /dev/null @@ -1,101 +0,0 @@ -package singlecluster_test - -import ( - "strings" - - "github.com/rancher/fleet/e2e/testenv" - "github.com/rancher/fleet/e2e/testenv/kubectl" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("Helm deploy options", func() { - var ( - asset string - k kubectl.Command - ) - BeforeEach(func() { - k = env.Kubectl.Namespace(env.Namespace) - }) - - JustBeforeEach(func() { - out, err := k.Apply("-f", testenv.AssetPath(asset)) - Expect(err).ToNot(HaveOccurred(), out) - }) - - AfterEach(func() { - out, err := k.Delete("-f", testenv.AssetPath(asset)) - Expect(err).ToNot(HaveOccurred(), out) - }) - - Describe("namespaceLabels TargetCustomization", func() { - BeforeEach(func() { - asset = "single-cluster/namespaceLabels_targetCustomization.yaml" - }) - When("namespaceLabels and namespaceAnnotations are set as targetCustomization ", func() { - It("deploys the bundledeployment with the merged namespaceLabels and namespaceAnnotations", func() { - out, err := k.Get("cluster", "local", "-o", "jsonpath={.status.namespace}") - Expect(err).ToNot(HaveOccurred(), out) - - clusterNS := strings.TrimSpace(out) - clusterK := k.Namespace(clusterNS) - Eventually(func() string { - bundleDeploymentNames, _ := clusterK.Get( - "bundledeployments", - "-o", - "jsonpath={.items[*].metadata.name}", - ) - - var bundleDeploymentName string - for _, podName := range strings.Split(bundleDeploymentNames, " ") { - if strings.HasPrefix(podName, "test-target-customization-namespace-labels") { - bundleDeploymentName = podName - break - } - } - if bundleDeploymentName == "" { - return "nil" - } - - bundleDeploymentNamespacesLabels, _ := clusterK.Get( - "bundledeployments", - bundleDeploymentName, - "-o", - "jsonpath={.spec.options.namespaceLabels}", - ) - return bundleDeploymentNamespacesLabels - }).Should(Equal(`{"foo":"bar","this.is/a":"test"}`)) - - Eventually(func() string { - bundleDeploymentNames, _ := clusterK.Get( - "bundledeployments", - "-o", - "jsonpath={.items[*].metadata.name}", - ) - - var bundleDeploymentName string - for _, podName := range strings.Split(bundleDeploymentNames, " ") { - if strings.HasPrefix(podName, "test-target-customization-namespace-labels") { - bundleDeploymentName = podName - break - } - } - if bundleDeploymentName == "" { - return "nil" - } - - bundleDeploymentNamespacesLabels, _ := clusterK.Get( - "bundledeployments", - bundleDeploymentName, - "-o", - "jsonpath={.spec.options.namespaceAnnotations}", - ) - return bundleDeploymentNamespacesLabels - }).Should(Equal(`{"foo":"bar","this.is/a":"test"}`)) - - }) - }) - }) - -}) diff --git a/e2e/single-cluster/target_customization_test.go b/e2e/single-cluster/target_customization_test.go new file mode 100644 index 0000000000..797ff9940c --- /dev/null +++ b/e2e/single-cluster/target_customization_test.go @@ -0,0 +1,166 @@ +package singlecluster_test + +import ( + "strings" + + "github.com/rancher/fleet/e2e/testenv" + "github.com/rancher/fleet/e2e/testenv/kubectl" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Helm deploy options", func() { + var ( + asset string + k kubectl.Command + ) + BeforeEach(func() { + k = env.Kubectl.Namespace(env.Namespace) + }) + + JustBeforeEach(func() { + out, err := k.Apply("-f", testenv.AssetPath(asset)) + Expect(err).ToNot(HaveOccurred(), out) + }) + + AfterEach(func() { + out, err := k.Delete("-f", testenv.AssetPath(asset)) + Expect(err).ToNot(HaveOccurred(), out) + }) + + Describe("namespace labels and annotations in target customizations", func() { + When("namespaceLabels and namespaceAnnotations override existing root configuration", func() { + BeforeEach(func() { + asset = "single-cluster/ns-labels-target-customization.yaml" + }) + + It("deploys the bundledeployment with the merged namespaceLabels and namespaceAnnotations", func() { + By("setting the namespaces and annotations on the bundle deployment") + out, err := k.Get("cluster", "local", "-o", "jsonpath={.status.namespace}") + Expect(err).ToNot(HaveOccurred(), out) + + clusterNS := strings.TrimSpace(out) + clusterK := k.Namespace(clusterNS) + + var bundleDeploymentName string + + Eventually(func(g Gomega) { + bundleDeploymentNames, _ := clusterK.Get( + "bundledeployments", + "-o", + "jsonpath={.items[*].metadata.name}", + ) + + for _, bdName := range strings.Split(bundleDeploymentNames, " ") { + if strings.HasPrefix(bdName, "test-target-customization-namespace-labels") { + bundleDeploymentName = bdName + break + } + } + + g.Expect(bundleDeploymentName).ToNot(BeEmpty()) + }).Should(Succeed()) + + Eventually(func(g Gomega) { + labels, err := clusterK.Get( + "bundledeployments", + bundleDeploymentName, + "-o", + "jsonpath={.spec.options.namespaceLabels}", + ) + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(labels).To(Equal(`{"foo":"bar","this.is/a":"test"}`)) + + annot, err := clusterK.Get( + "bundledeployments", + bundleDeploymentName, + "-o", + "jsonpath={.spec.options.namespaceAnnotations}", + ) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(annot).To(Equal(`{"foo":"bar","this.is/another":"test"}`)) + }).Should(Succeed()) + + By("setting the namespaces and annotations on the created namespace") + Eventually(func(g Gomega) { + labels, err := k.Get("ns", "ns-1", "-o", "jsonpath={.metadata.labels}") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(labels).To(Equal(`{"foo":"bar","kubernetes.io/metadata.name":"ns-1","this.is/a":"test"}`)) + + ann, err := k.Get("ns", "ns-1", "-o", "jsonpath={.metadata.annotations}") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ann).To(Equal(`{"foo":"bar","this.is/another":"test"}`)) + }).Should(Succeed()) + }) + }) + + When("namespaceLabels and namespaceAnnotations override empty root configuration", func() { + BeforeEach(func() { + asset = "single-cluster/ns-labels-target-customization-no-defaults.yaml" + }) + + It("deploys the bundledeployment with the merged namespaceLabels and namespaceAnnotations", func() { + By("setting the namespaces and annotations on the bundle deployment") + out, err := k.Get("cluster", "local", "-o", "jsonpath={.status.namespace}") + Expect(err).ToNot(HaveOccurred(), out) + + clusterNS := strings.TrimSpace(out) + clusterK := k.Namespace(clusterNS) + + var bundleDeploymentName string + + Eventually(func(g Gomega) { + bundleDeploymentNames, _ := clusterK.Get( + "bundledeployments", + "-o", + "jsonpath={.items[*].metadata.name}", + ) + + for _, bdName := range strings.Split(bundleDeploymentNames, " ") { + if strings.HasPrefix(bdName, "test-target-customization-namespace-labels") { + bundleDeploymentName = bdName + break + } + } + + g.Expect(bundleDeploymentName).ToNot(BeEmpty()) + }).Should(Succeed()) + + Eventually(func(g Gomega) { + labels, err := clusterK.Get( + "bundledeployments", + bundleDeploymentName, + "-o", + "jsonpath={.spec.options.namespaceLabels}", + ) + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(labels).To(Equal(`{"this.is/a":"test"}`)) + + annot, err := clusterK.Get( + "bundledeployments", + bundleDeploymentName, + "-o", + "jsonpath={.spec.options.namespaceAnnotations}", + ) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(annot).To(Equal(`{"this.is/another":"test"}`)) + }).Should(Succeed()) + + By("setting the namespaces and annotations on the created namespace") + Eventually(func(g Gomega) { + labels, err := k.Get("ns", "no-defaults-ns-1", "-o", "jsonpath={.metadata.labels}") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(labels).To(Equal(`{"kubernetes.io/metadata.name":"no-defaults-ns-1","this.is/a":"test"}`)) + + ann, err := k.Get("ns", "no-defaults-ns-1", "-o", "jsonpath={.metadata.annotations}") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ann).To(Equal(`{"this.is/another":"test"}`)) + }).Should(Succeed()) + }) + }) + }) + +}) diff --git a/internal/cmd/controller/target/target.go b/internal/cmd/controller/target/target.go index 510f23c938..00442ba7e1 100644 --- a/internal/cmd/controller/target/target.go +++ b/internal/cmd/controller/target/target.go @@ -59,6 +59,8 @@ func (t *Target) BundleDeployment() *fleet.BundleDeployment { } bd.Spec.Paused = t.IsPaused() + initialiseOptionsMaps(bd) + for _, bundleTarget := range t.Bundle.Spec.Targets { if bundleTarget.NamespaceLabels != nil { for key, value := range *bundleTarget.NamespaceLabels { @@ -140,3 +142,24 @@ func (t *Target) state() fleet.BundleState { func (t *Target) message() string { return summary.MessageFromDeployment(t.Deployment) } + +// initialiseOptionsMaps initialises options and staged options maps of namespace labels and annotations on bd, if +// needed. +// Assumes that bd is not nil. +func initialiseOptionsMaps(bd *fleet.BundleDeployment) { + if bd.Spec.Options.NamespaceLabels == nil { + bd.Spec.Options.NamespaceLabels = map[string]string{} + } + + if bd.Spec.Options.NamespaceAnnotations == nil { + bd.Spec.Options.NamespaceAnnotations = map[string]string{} + } + + if bd.Spec.StagedOptions.NamespaceLabels == nil { + bd.Spec.StagedOptions.NamespaceLabels = map[string]string{} + } + + if bd.Spec.StagedOptions.NamespaceAnnotations == nil { + bd.Spec.StagedOptions.NamespaceAnnotations = map[string]string{} + } +} From edb773c41bdb7b92b4ab4adb0cd29c6fb0cb7a67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Corentin=20N=C3=A9au?= Date: Fri, 15 Nov 2024 13:30:59 +0100 Subject: [PATCH 2/2] Fix typos This fixes `requeueing` as `requeuing` and should make our typos Github action happy. --- .../controller/agentmanagement/controllers/cluster/import.go | 2 +- .../cmd/controller/reconciler/bundledeployment_controller.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/cmd/controller/agentmanagement/controllers/cluster/import.go b/internal/cmd/controller/agentmanagement/controllers/cluster/import.go index 1df456a652..b6e3dc5277 100644 --- a/internal/cmd/controller/agentmanagement/controllers/cluster/import.go +++ b/internal/cmd/controller/agentmanagement/controllers/cluster/import.go @@ -292,7 +292,7 @@ func (i *importHandler) importCluster(cluster *fleet.Cluster, status fleet.Clust TTL: &metav1.Duration{Duration: durations.ClusterImportTokenTTL}, }, }) - logrus.Debugf("Failed to create ClusterRegistrationToken for cluster %s/%s: %v (requeueing)", cluster.Namespace, cluster.Name, err) + logrus.Debugf("Failed to create ClusterRegistrationToken for cluster %s/%s: %v (requeuing)", cluster.Namespace, cluster.Name, err) i.clusters.EnqueueAfter(cluster.Namespace, cluster.Name, durations.TokenClusterEnqueueDelay) return status, nil } diff --git a/internal/cmd/controller/reconciler/bundledeployment_controller.go b/internal/cmd/controller/reconciler/bundledeployment_controller.go index 966de38d51..d0a5f91344 100644 --- a/internal/cmd/controller/reconciler/bundledeployment_controller.go +++ b/internal/cmd/controller/reconciler/bundledeployment_controller.go @@ -109,7 +109,7 @@ func (r *BundleDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Req err = r.Status().Update(ctx, bd) if err != nil { - logger.V(1).Error(err, "Reconcile failed update to bundle deployment status, requeueing", "status", bd.Status) + logger.V(1).Error(err, "Reconcile failed update to bundle deployment status, requeuing", "status", bd.Status) return ctrl.Result{}, err }