diff --git a/pkg/processor/daemonset/daemonset.go b/pkg/processor/daemonset/daemonset.go index 7caac534..4ca18815 100644 --- a/pkg/processor/daemonset/daemonset.go +++ b/pkg/processor/daemonset/daemonset.go @@ -67,9 +67,12 @@ func (d daemonset) Process(appMeta helmify.AppMetadata, obj *unstructured.Unstru name := appMeta.TrimName(obj.GetName()) - matchLabels, err := yamlformat.Marshal(map[string]interface{}{"matchLabels": dae.Spec.Selector.MatchLabels}, 0) - if err != nil { - return true, nil, err + matchLabels := "matchLabels:" + if stripped := processor.StripSelectorLabels(dae.Spec.Selector.MatchLabels); len(stripped) > 0 { + matchLabels, err = yamlformat.Marshal(map[string]interface{}{"matchLabels": stripped}, 0) + if err != nil { + return true, nil, err + } } matchExpr := "" if dae.Spec.Selector.MatchExpressions != nil { @@ -82,11 +85,14 @@ func (d daemonset) Process(appMeta helmify.AppMetadata, obj *unstructured.Unstru selector = strings.Trim(selector, " \n") selector = string(yamlformat.Indent([]byte(selector), 4)) - podLabels, err := yamlformat.Marshal(dae.Spec.Template.ObjectMeta.Labels, 8) - if err != nil { - return true, nil, err + podLabels := fmt.Sprintf(" {{- include \"%s.selectorLabels\" . | nindent 8 }}", appMeta.ChartName()) + if strippedPodLabels := processor.StripSelectorLabels(dae.Spec.Template.ObjectMeta.Labels); len(strippedPodLabels) > 0 { + podLabels, err = yamlformat.Marshal(strippedPodLabels, 8) + if err != nil { + return true, nil, err + } + podLabels += fmt.Sprintf("\n {{- include \"%s.selectorLabels\" . | nindent 8 }}", appMeta.ChartName()) } - podLabels += fmt.Sprintf("\n {{- include \"%s.selectorLabels\" . | nindent 8 }}", appMeta.ChartName()) podAnnotations := "" if len(dae.Spec.Template.ObjectMeta.Annotations) != 0 { diff --git a/pkg/processor/deployment/deployment.go b/pkg/processor/deployment/deployment.go index ad7c9234..965a9a42 100644 --- a/pkg/processor/deployment/deployment.go +++ b/pkg/processor/deployment/deployment.go @@ -93,9 +93,12 @@ func (d deployment) Process(appMeta helmify.AppMetadata, obj *unstructured.Unstr return true, nil, err } - matchLabels, err := yamlformat.Marshal(map[string]interface{}{"matchLabels": depl.Spec.Selector.MatchLabels}, 0) - if err != nil { - return true, nil, err + matchLabels := "matchLabels:" + if stripped := processor.StripSelectorLabels(depl.Spec.Selector.MatchLabels); len(stripped) > 0 { + matchLabels, err = yamlformat.Marshal(map[string]interface{}{"matchLabels": stripped}, 0) + if err != nil { + return true, nil, err + } } matchExpr := "" if depl.Spec.Selector.MatchExpressions != nil { @@ -108,11 +111,14 @@ func (d deployment) Process(appMeta helmify.AppMetadata, obj *unstructured.Unstr selector = strings.Trim(selector, " \n") selector = string(yamlformat.Indent([]byte(selector), 4)) - podLabels, err := yamlformat.Marshal(depl.Spec.Template.ObjectMeta.Labels, 8) - if err != nil { - return true, nil, err + podLabels := fmt.Sprintf(" {{- include \"%s.selectorLabels\" . | nindent 8 }}", appMeta.ChartName()) + if strippedPodLabels := processor.StripSelectorLabels(depl.Spec.Template.ObjectMeta.Labels); len(strippedPodLabels) > 0 { + podLabels, err = yamlformat.Marshal(strippedPodLabels, 8) + if err != nil { + return true, nil, err + } + podLabels += fmt.Sprintf("\n {{- include \"%s.selectorLabels\" . | nindent 8 }}", appMeta.ChartName()) } - podLabels += fmt.Sprintf("\n {{- include \"%s.selectorLabels\" . | nindent 8 }}", appMeta.ChartName()) podAnnotations := "" if len(depl.Spec.Template.ObjectMeta.Annotations) != 0 { diff --git a/pkg/processor/deployment/deployment_test.go b/pkg/processor/deployment/deployment_test.go index 07a2046f..ddce945e 100644 --- a/pkg/processor/deployment/deployment_test.go +++ b/pkg/processor/deployment/deployment_test.go @@ -1,6 +1,7 @@ package deployment import ( + "bytes" "testing" "github.com/arttor/helmify/pkg/metadata" @@ -24,10 +25,13 @@ spec: type: Recreate selector: matchLabels: + app.kubernetes.io/name: my-operator control-plane: controller-manager template: metadata: labels: + app.kubernetes.io/name: my-operator + app.kubernetes.io/instance: my-operator control-plane: controller-manager spec: containers: @@ -125,9 +129,21 @@ func Test_deployment_Process(t *testing.T) { t.Run("processed", func(t *testing.T) { obj := internal.GenerateObj(strDepl) - processed, _, err := testInstance.Process(&metadata.Service{}, obj) + processed, tpl, err := testInstance.Process(&metadata.Service{}, obj) assert.NoError(t, err) assert.Equal(t, true, processed) + + // issue #196: app.kubernetes.io/name and app.kubernetes.io/instance from + // selector.matchLabels and pod-template labels are provided by the + // chart.selectorLabels helper and must not be emitted hardcoded, otherwise + // the rendered manifest has duplicate YAML mapping keys. + var buf bytes.Buffer + assert.NoError(t, tpl.Write(&buf)) + rendered := buf.String() + assert.Contains(t, rendered, "selectorLabels") + assert.Contains(t, rendered, "control-plane: controller-manager") + assert.NotContains(t, rendered, "app.kubernetes.io/name: my-operator") + assert.NotContains(t, rendered, "app.kubernetes.io/instance: my-operator") }) t.Run("skipped", func(t *testing.T) { obj := internal.TestNs diff --git a/pkg/processor/meta.go b/pkg/processor/meta.go index 6593e161..7422af89 100644 --- a/pkg/processor/meta.go +++ b/pkg/processor/meta.go @@ -49,6 +49,32 @@ func WithAnnotations(values helmify.Values) MetaOpt { } } +// selectorLabelsKeys are the label keys emitted by the ".selectorLabels" helper. +// They are stripped from selectors and pod-template labels so they are not rendered +// twice (once hardcoded from the source manifest, once from the helper), which would +// produce duplicate YAML mapping keys rejected by strict parsers (e.g. ArgoCD). +var selectorLabelsKeys = map[string]struct{}{ + "app.kubernetes.io/name": {}, + "app.kubernetes.io/instance": {}, +} + +// StripSelectorLabels returns a copy of labels with the keys provided by the +// ".selectorLabels" helper removed. Use it on selectors and pod-template +// labels before rendering them next to a selectorLabels include. +func StripSelectorLabels(labels map[string]string) map[string]string { + if len(labels) == 0 { + return labels + } + stripped := make(map[string]string, len(labels)) + for k, v := range labels { + if _, drop := selectorLabelsKeys[k]; drop { + continue + } + stripped[k] = v + } + return stripped +} + // ProcessObjMeta - returns object apiVersion, kind and metadata as helm template. func ProcessObjMeta(appMeta helmify.AppMetadata, obj *unstructured.Unstructured, opts ...MetaOpt) (string, error) { options := &options{} diff --git a/pkg/processor/meta_test.go b/pkg/processor/meta_test.go index 9460b33e..01a64cd6 100644 --- a/pkg/processor/meta_test.go +++ b/pkg/processor/meta_test.go @@ -17,3 +17,54 @@ func TestProcessObjMeta(t *testing.T) { assert.Contains(t, res, "chart-name.labels") assert.Contains(t, res, "chart-name.fullname") } + +func TestStripSelectorLabels(t *testing.T) { + tests := []struct { + name string + labels map[string]string + want map[string]string + }{ + { + name: "nil labels returned as-is", + labels: nil, + want: nil, + }, + { + name: "empty labels returned as-is", + labels: map[string]string{}, + want: map[string]string{}, + }, + { + name: "drops only keys provided by selectorLabels", + labels: map[string]string{ + "app.kubernetes.io/name": "myapp", + "app.kubernetes.io/instance": "myapp", + "app.kubernetes.io/version": "1.0", // kept: not part of selectorLabels + "control-plane": "controller-manager", + }, + want: map[string]string{ + "app.kubernetes.io/version": "1.0", + "control-plane": "controller-manager", + }, + }, + { + name: "only selectorLabels keys yields empty map", + labels: map[string]string{ + "app.kubernetes.io/name": "myapp", + "app.kubernetes.io/instance": "myapp", + }, + want: map[string]string{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + input := tt.labels + got := StripSelectorLabels(input) + assert.Equal(t, tt.want, got) + // helper must not mutate the input map. + if len(input) > 0 { + assert.Contains(t, input, "app.kubernetes.io/name") + } + }) + } +} diff --git a/pkg/processor/poddisruptionbudget/pdb.go b/pkg/processor/poddisruptionbudget/pdb.go index 99635d14..747572e9 100644 --- a/pkg/processor/poddisruptionbudget/pdb.go +++ b/pkg/processor/poddisruptionbudget/pdb.go @@ -61,9 +61,20 @@ func (r pdb) Process(appMeta helmify.AppMetadata, obj *unstructured.Unstructured name := appMeta.TrimName(obj.GetName()) nameCamel := strcase.ToLowerCamel(name) - selector, _ := yaml.Marshal(pdb.Spec.Selector) - selector = yamlformat.Indent(selector, 4) - selector = bytes.TrimRight(selector, "\n ") + var selector []byte + if pdb.Spec.Selector != nil { + sel := pdb.Spec.Selector.DeepCopy() + sel.MatchLabels = processor.StripSelectorLabels(sel.MatchLabels) + if len(sel.MatchLabels) == 0 && len(sel.MatchExpressions) == 0 { + // selector only had keys provided by selectorLabels; keep the matchLabels + // key so the include nests correctly under it (avoids an empty "{}"). + selector = []byte(" matchLabels:") + } else { + raw, _ := yaml.Marshal(sel) + selector = yamlformat.Indent(raw, 4) + selector = bytes.TrimRight(selector, "\n ") + } + } if spec.MaxUnavailable != nil { _, err := values.Add(spec.MaxUnavailable.IntValue(), nameCamel, "maxUnavailable") diff --git a/pkg/processor/service/service.go b/pkg/processor/service/service.go index 0cc77354..76f6c373 100644 --- a/pkg/processor/service/service.go +++ b/pkg/processor/service/service.go @@ -80,9 +80,12 @@ func (r svc) Process(appMeta helmify.AppMetadata, obj *unstructured.Unstructured shortName := strings.TrimPrefix(name, "controller-manager-") shortNameCamel := strcase.ToLowerCamel(shortName) - selector, _ := yaml.Marshal(service.Spec.Selector) - selector = yamlformat.Indent(selector, 4) - selector = bytes.TrimRight(selector, "\n ") + var selector []byte + if stripped := processor.StripSelectorLabels(service.Spec.Selector); len(stripped) > 0 { + selector, _ = yaml.Marshal(stripped) + selector = yamlformat.Indent(selector, 4) + selector = bytes.TrimRight(selector, "\n ") + } values := helmify.Values{} svcType := service.Spec.Type diff --git a/test_data/sample-app.yaml b/test_data/sample-app.yaml index 0000d7c8..5896faee 100644 --- a/test_data/sample-app.yaml +++ b/test_data/sample-app.yaml @@ -11,10 +11,12 @@ spec: selector: matchLabels: app: myapp + app.kubernetes.io/name: myapp template: metadata: labels: app: myapp + app.kubernetes.io/name: myapp spec: initContainers: - name: init-container @@ -145,6 +147,7 @@ spec: targetPort: https selector: app: myapp + app.kubernetes.io/name: myapp --- apiVersion: v1 kind: Service @@ -274,10 +277,12 @@ spec: selector: matchLabels: name: fluentd-elasticsearch + app.kubernetes.io/name: fluentd-elasticsearch template: metadata: labels: name: fluentd-elasticsearch + app.kubernetes.io/name: fluentd-elasticsearch spec: priorityClassName: system-node-critical tolerations: @@ -404,3 +409,4 @@ spec: selector: matchLabels: app: nginx + app.kubernetes.io/name: nginx