From 6cd8a634f46e928f688ab9ebc951cf4ac1568653 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Corentin=20N=C3=A9au?= Date: Tue, 23 Jul 2024 10:33:28 +0200 Subject: [PATCH 1/5] Fix watched path for target customization tests Now that the necessary directory has been merged into `rancher/fleet-test-data`, this commit makes use of it instead of a fork's directory. --- .../single-cluster/namespaceLabels_targetCustomization.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml b/e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml index f53ed41b38..337746ae93 100644 --- a/e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml +++ b/e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml @@ -4,7 +4,7 @@ metadata: name: test namespace: fleet-local spec: - repo: https://github.com/Tommy12789/fleet-examples-tommy - branch: test + repo: https://github.com/rancher/fleet-test-data + branch: master paths: - - namespaceLabels_targetCustomization + - target-customization-namespace-labels From 7dfa6a6752dd366a5c87c06f9513ecefc1238f0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Corentin=20N=C3=A9au?= Date: Tue, 23 Jul 2024 10:59:48 +0200 Subject: [PATCH 2/5] Optimise labels and annotations target customisation population One only needs to loop over targets once to populate both. --- internal/cmd/controller/target/target.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/cmd/controller/target/target.go b/internal/cmd/controller/target/target.go index bc320086dc..510f23c938 100644 --- a/internal/cmd/controller/target/target.go +++ b/internal/cmd/controller/target/target.go @@ -58,6 +58,7 @@ func (t *Target) BundleDeployment() *fleet.BundleDeployment { Spec: t.Deployment.Spec, } bd.Spec.Paused = t.IsPaused() + for _, bundleTarget := range t.Bundle.Spec.Targets { if bundleTarget.NamespaceLabels != nil { for key, value := range *bundleTarget.NamespaceLabels { @@ -65,15 +66,15 @@ func (t *Target) BundleDeployment() *fleet.BundleDeployment { (*bd.Spec.StagedOptions.NamespaceLabels)[key] = value } } - } - for _, bundleTargets := range t.Bundle.Spec.Targets { - if bundleTargets.NamespaceAnnotations != nil { - for key, value := range *bundleTargets.NamespaceAnnotations { + + if bundleTarget.NamespaceAnnotations != nil { + for key, value := range *bundleTarget.NamespaceAnnotations { (*bd.Spec.Options.NamespaceAnnotations)[key] = value (*bd.Spec.StagedOptions.NamespaceAnnotations)[key] = value } } } + bd.Spec.DependsOn = t.Bundle.Spec.DependsOn bd.Spec.CorrectDrift = t.Options.CorrectDrift return bd From 83dd89c10b2c5b884d27855591135cadb57b342c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Corentin=20N=C3=A9au?= Date: Tue, 23 Jul 2024 11:00:57 +0200 Subject: [PATCH 3/5] Improve target customisation tests readability This fetches the cluster namespace from the cluster object itself, as opposed to using a hard-coded namespace with a seemingly arbitrary value. --- .../targetCustomization_test.go | 39 +++++++++++++++---- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/e2e/single-cluster/targetCustomization_test.go b/e2e/single-cluster/targetCustomization_test.go index 80c69e2144..651563e06a 100644 --- a/e2e/single-cluster/targetCustomization_test.go +++ b/e2e/single-cluster/targetCustomization_test.go @@ -33,14 +33,23 @@ var _ = Describe("Helm deploy options", func() { BeforeEach(func() { asset = "single-cluster/namespaceLabels_targetCustomization.yaml" }) - When("namespaceLabels set as targetCustomization ", func() { - It("deploy the bundledeployment with the merged namespaceLabels", func() { + 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, _ := k.Namespace("cluster-fleet-local-local-1a3d67d0a899").Get("bundledeployments", "-o", "jsonpath={.items[*].metadata.name}") + bundleDeploymentNames, _ := clusterK.Get( + "bundledeployments", + "-o", + "jsonpath={.items[*].metadata.name}", + ) var bundleDeploymentName string for _, podName := range strings.Split(bundleDeploymentNames, " ") { - if strings.HasPrefix(podName, "test-namespacelabels-targetcustomization") { + if strings.HasPrefix(podName, "test-target-customization-namespace-labels") { bundleDeploymentName = podName break } @@ -49,16 +58,25 @@ var _ = Describe("Helm deploy options", func() { return "nil" } - bundleDeploymentNamespacesLabels, _ := k.Namespace("cluster-fleet-local-local-1a3d67d0a899").Get("bundledeployments", bundleDeploymentName, "-o", "jsonpath={.spec.options.namespaceLabels}") + bundleDeploymentNamespacesLabels, _ := clusterK.Get( + "bundledeployments", + bundleDeploymentName, + "-o", + "jsonpath={.spec.options.namespaceLabels}", + ) return bundleDeploymentNamespacesLabels }).Should(Equal(`{"foo":"bar","this.is/a":"test"}`)) Eventually(func() string { - bundleDeploymentNames, _ := k.Namespace("cluster-fleet-local-local-1a3d67d0a899").Get("bundledeployments", "-o", "jsonpath={.items[*].metadata.name}") + bundleDeploymentNames, _ := clusterK.Get( + "bundledeployments", + "-o", + "jsonpath={.items[*].metadata.name}", + ) var bundleDeploymentName string for _, podName := range strings.Split(bundleDeploymentNames, " ") { - if strings.HasPrefix(podName, "test-namespacelabels-targetcustomization") { + if strings.HasPrefix(podName, "test-target-customization-namespace-labels") { bundleDeploymentName = podName break } @@ -67,7 +85,12 @@ var _ = Describe("Helm deploy options", func() { return "nil" } - bundleDeploymentNamespacesLabels, _ := k.Namespace("cluster-fleet-local-local-1a3d67d0a899").Get("bundledeployments", bundleDeploymentName, "-o", "jsonpath={.spec.options.namespaceAnnotations}") + bundleDeploymentNamespacesLabels, _ := clusterK.Get( + "bundledeployments", + bundleDeploymentName, + "-o", + "jsonpath={.spec.options.namespaceAnnotations}", + ) return bundleDeploymentNamespacesLabels }).Should(Equal(`{"foo":"bar","this.is/a":"test"}`)) From ce952ac34da82217c9a28e08056d9d7d1f33ca02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Corentin=20N=C3=A9au?= Date: Tue, 23 Jul 2024 11:14:02 +0200 Subject: [PATCH 4/5] Use maps for namespace labels and annotations Pointers to maps are superfluous, as iterating over an empty map will be a no-op, and we do not need to otherwise differentiate an empty map from an unset one. This still leaves original namespace labels and annotations as pointers to maps in bundle deployment options, for backward compatibility. --- internal/cmd/controller/target/target.go | 16 ++++++--------- .../fleet.cattle.io/v1alpha1/bundle_types.go | 4 ++-- .../v1alpha1/zz_generated.deepcopy.go | 20 ++++++------------- 3 files changed, 14 insertions(+), 26 deletions(-) diff --git a/internal/cmd/controller/target/target.go b/internal/cmd/controller/target/target.go index 510f23c938..d39e5227dd 100644 --- a/internal/cmd/controller/target/target.go +++ b/internal/cmd/controller/target/target.go @@ -60,18 +60,14 @@ func (t *Target) BundleDeployment() *fleet.BundleDeployment { bd.Spec.Paused = t.IsPaused() for _, bundleTarget := range t.Bundle.Spec.Targets { - if bundleTarget.NamespaceLabels != nil { - for key, value := range *bundleTarget.NamespaceLabels { - (*bd.Spec.Options.NamespaceLabels)[key] = value - (*bd.Spec.StagedOptions.NamespaceLabels)[key] = value - } + for key, value := range bundleTarget.NamespaceLabels { + (*bd.Spec.Options.NamespaceLabels)[key] = value + (*bd.Spec.StagedOptions.NamespaceLabels)[key] = value } - if bundleTarget.NamespaceAnnotations != nil { - for key, value := range *bundleTarget.NamespaceAnnotations { - (*bd.Spec.Options.NamespaceAnnotations)[key] = value - (*bd.Spec.StagedOptions.NamespaceAnnotations)[key] = value - } + for key, value := range bundleTarget.NamespaceAnnotations { + (*bd.Spec.Options.NamespaceAnnotations)[key] = value + (*bd.Spec.StagedOptions.NamespaceAnnotations)[key] = value } } diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/bundle_types.go b/pkg/apis/fleet.cattle.io/v1alpha1/bundle_types.go index a3431fbd37..e010f29373 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/bundle_types.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/bundle_types.go @@ -229,10 +229,10 @@ type BundleTarget struct { DoNotDeploy bool `json:"doNotDeploy,omitempty"` // NamespaceLabels are labels that will be appended to the namespace created by Fleet. // +nullable - NamespaceLabels *map[string]string `json:"namespaceLabels,omitempty"` + NamespaceLabels map[string]string `json:"namespaceLabels,omitempty"` // NamespaceAnnotations are annotations that will be appended to the namespace created by Fleet. // +nullable - NamespaceAnnotations *map[string]string `json:"namespaceAnnotations,omitempty"` + NamespaceAnnotations map[string]string `json:"namespaceAnnotations,omitempty"` } // BundleSummary contains the number of bundle deployments in each state and a diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go index e64aa50838..c21f9c91a0 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go @@ -581,24 +581,16 @@ func (in *BundleTarget) DeepCopyInto(out *BundleTarget) { } if in.NamespaceLabels != nil { in, out := &in.NamespaceLabels, &out.NamespaceLabels - *out = new(map[string]string) - if **in != nil { - in, out := *in, *out - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val - } + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val } } if in.NamespaceAnnotations != nil { in, out := &in.NamespaceAnnotations, &out.NamespaceAnnotations - *out = new(map[string]string) - if **in != nil { - in, out := *in, *out - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val - } + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val } } } From 8e71c25149a49a9db92283eb2f7bb2e068edf978 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Corentin=20N=C3=A9au?= Date: Mon, 29 Jul 2024 14:02:38 +0200 Subject: [PATCH 5/5] Use maps instead of pointers for namespace labels and annotations This simplifies dealing with bundle deployment options, since an empty map is semantically equivalent to a nil pointer. --- internal/cmd/agent/deployer/deployer.go | 4 +- internal/cmd/agent/deployer/deployer_test.go | 39 +++++++++++++++---- internal/cmd/controller/target/target.go | 8 ++-- .../v1alpha1/bundledeployment_types.go | 4 +- .../v1alpha1/zz_generated.deepcopy.go | 20 +++------- 5 files changed, 45 insertions(+), 30 deletions(-) diff --git a/internal/cmd/agent/deployer/deployer.go b/internal/cmd/agent/deployer/deployer.go index af025a90f6..bf4bcbfd80 100644 --- a/internal/cmd/agent/deployer/deployer.go +++ b/internal/cmd/agent/deployer/deployer.go @@ -176,13 +176,13 @@ func (d *Deployer) setNamespaceLabelsAndAnnotations(ctx context.Context, bd *fle } if bd.Spec.Options.NamespaceLabels != nil { - addLabelsFromOptions(ns.Labels, *bd.Spec.Options.NamespaceLabels) + addLabelsFromOptions(ns.Labels, bd.Spec.Options.NamespaceLabels) } if bd.Spec.Options.NamespaceAnnotations != nil { if ns.Annotations == nil { ns.Annotations = map[string]string{} } - addAnnotationsFromOptions(ns.Annotations, *bd.Spec.Options.NamespaceAnnotations) + addAnnotationsFromOptions(ns.Annotations, bd.Spec.Options.NamespaceAnnotations) } err = d.updateNamespace(ctx, ns) if err != nil { diff --git a/internal/cmd/agent/deployer/deployer_test.go b/internal/cmd/agent/deployer/deployer_test.go index c12c8fc76a..71f6e4c864 100644 --- a/internal/cmd/agent/deployer/deployer_test.go +++ b/internal/cmd/agent/deployer/deployer_test.go @@ -23,11 +23,34 @@ func TestSetNamespaceLabelsAndAnnotations(t *testing.T) { release string expectedNs corev1.Namespace }{ + "Empty sets of NamespaceLabels and NamespaceAnnotations are supported": { + bd: &fleet.BundleDeployment{Spec: fleet.BundleDeploymentSpec{ + Options: fleet.BundleDeploymentOptions{ + NamespaceLabels: nil, // equivalent to map[string]string{} + NamespaceAnnotations: nil, + }, + }}, + ns: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "namespace", + Labels: map[string]string{"kubernetes.io/metadata.name": "namespace"}, + }, + }, + release: "namespace/foo/bar", + expectedNs: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "namespace", + Labels: map[string]string{"kubernetes.io/metadata.name": "namespace"}, + Annotations: nil, + }, + }, + }, + "NamespaceLabels and NamespaceAnnotations are appended": { bd: &fleet.BundleDeployment{Spec: fleet.BundleDeploymentSpec{ Options: fleet.BundleDeploymentOptions{ - NamespaceLabels: &map[string]string{"optLabel1": "optValue1", "optLabel2": "optValue2"}, - NamespaceAnnotations: &map[string]string{"optAnn1": "optValue1"}, + NamespaceLabels: map[string]string{"optLabel1": "optValue1", "optLabel2": "optValue2"}, + NamespaceAnnotations: map[string]string{"optAnn1": "optValue1"}, }, }}, ns: corev1.Namespace{ @@ -49,8 +72,8 @@ func TestSetNamespaceLabelsAndAnnotations(t *testing.T) { "NamespaceLabels and NamespaceAnnotations removes entries that are not in the options, except the name label": { bd: &fleet.BundleDeployment{Spec: fleet.BundleDeploymentSpec{ Options: fleet.BundleDeploymentOptions{ - NamespaceLabels: &map[string]string{"optLabel": "optValue"}, - NamespaceAnnotations: &map[string]string{}, + NamespaceLabels: map[string]string{"optLabel": "optValue"}, + NamespaceAnnotations: map[string]string{}, }, }}, ns: corev1.Namespace{ @@ -73,8 +96,8 @@ func TestSetNamespaceLabelsAndAnnotations(t *testing.T) { "NamespaceLabels and NamespaceAnnotations updates existing values": { bd: &fleet.BundleDeployment{Spec: fleet.BundleDeploymentSpec{ Options: fleet.BundleDeploymentOptions{ - NamespaceLabels: &map[string]string{"bdLabel": "labelUpdated"}, - NamespaceAnnotations: &map[string]string{"bdAnn": "annUpdated"}, + NamespaceLabels: map[string]string{"bdLabel": "labelUpdated"}, + NamespaceAnnotations: map[string]string{"bdAnn": "annUpdated"}, }, }}, ns: corev1.Namespace{ @@ -130,8 +153,8 @@ func TestSetNamespaceLabelsAndAnnotations(t *testing.T) { func TestSetNamespaceLabelsAndAnnotationsError(t *testing.T) { bd := &fleet.BundleDeployment{Spec: fleet.BundleDeploymentSpec{ Options: fleet.BundleDeploymentOptions{ - NamespaceLabels: &map[string]string{"optLabel1": "optValue1", "optLabel2": "optValue2"}, - NamespaceAnnotations: &map[string]string{"optAnn1": "optValue1"}, + NamespaceLabels: map[string]string{"optLabel1": "optValue1", "optLabel2": "optValue2"}, + NamespaceAnnotations: map[string]string{"optAnn1": "optValue1"}, }, }} release := "test/foo/bar" diff --git a/internal/cmd/controller/target/target.go b/internal/cmd/controller/target/target.go index d39e5227dd..a24b7decee 100644 --- a/internal/cmd/controller/target/target.go +++ b/internal/cmd/controller/target/target.go @@ -61,13 +61,13 @@ func (t *Target) BundleDeployment() *fleet.BundleDeployment { for _, bundleTarget := range t.Bundle.Spec.Targets { for key, value := range bundleTarget.NamespaceLabels { - (*bd.Spec.Options.NamespaceLabels)[key] = value - (*bd.Spec.StagedOptions.NamespaceLabels)[key] = value + bd.Spec.Options.NamespaceLabels[key] = value + bd.Spec.StagedOptions.NamespaceLabels[key] = value } for key, value := range bundleTarget.NamespaceAnnotations { - (*bd.Spec.Options.NamespaceAnnotations)[key] = value - (*bd.Spec.StagedOptions.NamespaceAnnotations)[key] = value + bd.Spec.Options.NamespaceAnnotations[key] = value + bd.Spec.StagedOptions.NamespaceAnnotations[key] = value } } diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/bundledeployment_types.go b/pkg/apis/fleet.cattle.io/v1alpha1/bundledeployment_types.go index 31a3a7e266..4062a2c968 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/bundledeployment_types.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/bundledeployment_types.go @@ -103,11 +103,11 @@ type BundleDeploymentOptions struct { // NamespaceLabels are labels that will be appended to the namespace created by Fleet. // +nullable - NamespaceLabels *map[string]string `json:"namespaceLabels,omitempty"` + NamespaceLabels map[string]string `json:"namespaceLabels,omitempty"` // NamespaceAnnotations are annotations that will be appended to the namespace created by Fleet. // +nullable - NamespaceAnnotations *map[string]string `json:"namespaceAnnotations,omitempty"` + NamespaceAnnotations map[string]string `json:"namespaceAnnotations,omitempty"` // DeleteCRDResources deletes CRDs. Warning! this will also delete all your Custom Resources. DeleteCRDResources bool `json:"deleteCRDResources,omitempty"` diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go index c21f9c91a0..5721b34565 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go @@ -191,24 +191,16 @@ func (in *BundleDeploymentOptions) DeepCopyInto(out *BundleDeploymentOptions) { } if in.NamespaceLabels != nil { in, out := &in.NamespaceLabels, &out.NamespaceLabels - *out = new(map[string]string) - if **in != nil { - in, out := *in, *out - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val - } + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val } } if in.NamespaceAnnotations != nil { in, out := &in.NamespaceAnnotations, &out.NamespaceAnnotations - *out = new(map[string]string) - if **in != nil { - in, out := *in, *out - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val - } + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val } } }