From f1b6dc2d44ab2fda36dc82ce9bcb40b32a8c0875 Mon Sep 17 00:00:00 2001 From: Max Goltzsche Date: Sun, 7 Mar 2021 17:33:07 +0100 Subject: [PATCH] Fix function output when annotation field empty. * When the `annotations` field is empty it gets removed before setting the path annotations to make sure `kyaml.LookupCreate` creates the mapping node. * Correspondingly, when forcefully setting a namespace, the namespace field is removed now before setting the namespace. Closes #13 --- cmd/khelm/fn.go | 45 ++++++++++++--- cmd/khelm/fn_test.go | 56 ++++++++++++------- cmd/khelm/template_test.go | 6 +- example/empty-annotations/Chart.yaml | 4 ++ .../empty-annotations/templates/template.yaml | 10 ++++ example/force-namespace/Chart.yaml | 4 ++ example/force-namespace/generator.yaml | 5 +- .../templates/clusterrolebinding.yaml | 12 ++++ .../force-namespace/templates/configmap.yaml | 30 ++++++++++ example/namespace/generator.yaml | 2 +- pkg/helm/render_test.go | 12 ++-- pkg/helm/transform.go | 10 +++- 12 files changed, 156 insertions(+), 40 deletions(-) create mode 100644 example/empty-annotations/Chart.yaml create mode 100644 example/empty-annotations/templates/template.yaml create mode 100644 example/force-namespace/Chart.yaml create mode 100644 example/force-namespace/templates/clusterrolebinding.yaml create mode 100644 example/force-namespace/templates/configmap.yaml diff --git a/cmd/khelm/fn.go b/cmd/khelm/fn.go index 5db9ef5..e9a575e 100644 --- a/cmd/khelm/fn.go +++ b/cmd/khelm/fn.go @@ -2,6 +2,7 @@ package main import ( "encoding/json" + "log" "path" "sort" "strconv" @@ -41,7 +42,7 @@ func kptFnCommand(h *helm.Helm) *cobra.Command { return errors.Errorf("no outputPath specified for outputMapping[%d]", i) } if len(m.ResourceSelectors) == 0 { - return errors.Errorf("no resourceSelectors specified for outputMapping[%d] -> %q", i, m.OutputPath) + return errors.Errorf("no selectors specified for outputMapping[%d] -> %q", i, m.OutputPath) } } @@ -53,7 +54,7 @@ func kptFnCommand(h *helm.Helm) *cobra.Command { } // Apply output path mappings and annotate resources - kustomizationDirs, err := mapOutputPaths(rendered, fnCfg.Data.OutputPathMapping, outputPath) + kustomizationDirs, err := mapOutputPaths(rendered, fnCfg.Data.OutputPathMapping, outputPath, h.Settings.Debug) if err != nil { return err } @@ -72,7 +73,10 @@ func kptFnCommand(h *helm.Helm) *cobra.Command { if err != nil { return errors.Wrapf(err, "generate %s", kustomizationPath) } - setKptAnnotations(kustomization, kustomizationPath, 0) + err = setKptAnnotations(kustomization, kustomizationPath, 0, h.Settings.Debug) + if err != nil { + return errors.Wrap(err, "set kpt annotations on kustomization") + } kustomizationResources[i] = kustomization } rendered = append(kustomizationResources, rendered...) @@ -80,7 +84,6 @@ func kptFnCommand(h *helm.Helm) *cobra.Command { // Apply output resourceList.Items = filterByOutputPath(resourceList.Items, outputPaths) resourceList.Items = append(resourceList.Items, rendered...) - return nil }) return cmd @@ -136,7 +139,7 @@ func isGeneratedOutputPath(path string, outputPaths []string) bool { return false } -func mapOutputPaths(resources []*yaml.RNode, outputMappings []kptFnOutputMapping, defaultOutputPath string) (map[string][]*yaml.RNode, error) { +func mapOutputPaths(resources []*yaml.RNode, outputMappings []kptFnOutputMapping, defaultOutputPath string, debug bool) (map[string][]*yaml.RNode, error) { matchers := make([]matcher.ResourceMatchers, len(outputMappings)) for i, m := range outputMappings { matchers[i] = matcher.FromResourceSelectors(m.ResourceSelectors) @@ -162,7 +165,10 @@ func mapOutputPaths(resources []*yaml.RNode, outputMappings []kptFnOutputMapping kustomizationDirs[outPath] = append(kustomizationDirs[outPath], o) outPath = output.ResourcePath(meta, outPath) } - setKptAnnotations(o, outPath, i) + err = setKptAnnotations(o, outPath, i, debug) + if err != nil { + return nil, errors.Wrapf(err, "set annotations on %s/%s", meta.Kind, meta.Name) + } } for _, m := range matchers { @@ -175,10 +181,31 @@ func mapOutputPaths(resources []*yaml.RNode, outputMappings []kptFnOutputMapping return kustomizationDirs, nil } -func setKptAnnotations(o *yaml.RNode, path string, index int) { +func setKptAnnotations(o *yaml.RNode, path string, index int, debug bool) error { + if debug { + m, err := o.GetMeta() + if err != nil { + return err + } + log.Printf("Mapping %s %s to path %s", m.Kind, m.Name, path) + } + // Remove annotations field if empty. + // This is required because LookupCreate() doesn't create the MappingNode if it exists but is empty (#13). + err := o.PipeE(yaml.LookupCreate(yaml.MappingNode, yaml.MetadataField), yaml.FieldClearer{Name: yaml.AnnotationsField, IfEmpty: true}) + if err != nil { + return err + } + // Add path annotations lookupAnnotations := yaml.LookupCreate(yaml.MappingNode, yaml.MetadataField, yaml.AnnotationsField) - _ = o.PipeE(lookupAnnotations, yaml.FieldSetter{Name: annotationIndex, StringValue: strconv.Itoa(index)}) - _ = o.PipeE(lookupAnnotations, yaml.FieldSetter{Name: annotationPath, StringValue: path}) + err = o.PipeE(lookupAnnotations, yaml.FieldSetter{Name: annotationIndex, StringValue: strconv.Itoa(index)}) + if err != nil { + return err + } + err = o.PipeE(lookupAnnotations, yaml.FieldSetter{Name: annotationPath, StringValue: path}) + if err != nil { + return err + } + return nil } func resourceNames(resources []*yaml.RNode, outputBasePath string) []string { diff --git a/cmd/khelm/fn_test.go b/cmd/khelm/fn_test.go index c992186..5dfe210 100644 --- a/cmd/khelm/fn_test.go +++ b/cmd/khelm/fn_test.go @@ -41,7 +41,7 @@ func TestKptFnCommand(t *testing.T) { name string input kptFnConfig mustContainObj int - mustContain string + mustContain []string }{ { "chart path only", @@ -50,7 +50,7 @@ func TestKptFnCommand(t *testing.T) { Chart: filepath.Join(exampleDir, "namespace"), }, }}, - 3, "myconfiga", + 3, []string{"myconfiga"}, }, { "latest cluster scoped remote chart", @@ -60,7 +60,7 @@ func TestKptFnCommand(t *testing.T) { Chart: "cert-manager", }, }}, - -1, "acme.cert-manager.io", + -1, []string{"acme.cert-manager.io"}, }, { "remote chart with version", @@ -71,7 +71,7 @@ func TestKptFnCommand(t *testing.T) { Version: "0.9.x", }, }}, - 34, "chart: cainjector-v0.9.1", + 34, []string{"chart: cainjector-v0.9.1"}, }, { "release name", @@ -83,7 +83,7 @@ func TestKptFnCommand(t *testing.T) { Name: "myrelease", }, }}, - 1, "myrelease-config", + 1, []string{"myrelease-config"}, }, { "valueFiles", @@ -94,7 +94,7 @@ func TestKptFnCommand(t *testing.T) { RendererConfig: config.RendererConfig{ ValueFiles: []string{filepath.Join(exampleDir, "values-inheritance", "values.yaml")}, }}}, - 1, " valueoverwrite: overwritten by file", + 1, []string{" valueoverwrite: overwritten by file"}, }, { "values", @@ -107,7 +107,7 @@ func TestKptFnCommand(t *testing.T) { "example": map[string]string{"overrideValue": "explicitly"}, }, }}}, - 1, " valueoverwrite: explicitly", + 1, []string{" valueoverwrite: explicitly"}, }, { "values override", @@ -121,7 +121,7 @@ func TestKptFnCommand(t *testing.T) { "example": map[string]string{"overrideValue": "explicitly"}, }, }}}, - 1, " valueoverwrite: explicitly", + 1, []string{" valueoverwrite: explicitly"}, }, { "apiversions", @@ -132,7 +132,7 @@ func TestKptFnCommand(t *testing.T) { RendererConfig: config.RendererConfig{ APIVersions: []string{"myfancyapi/v1", ""}, }}}, - 1, "fancycr", + 1, []string{"fancycr"}, }, { "kubeversion", @@ -143,7 +143,7 @@ func TestKptFnCommand(t *testing.T) { RendererConfig: config.RendererConfig{ KubeVersion: "1.12", }}}, - 1, "k8sVersion: v1.12.0", + 1, []string{"k8sVersion: v1.12.0"}, }, { "expand-list", @@ -152,7 +152,7 @@ func TestKptFnCommand(t *testing.T) { Chart: filepath.Join(exampleDir, "expand-list"), }, }}, - 3, "\n name: myserviceaccount2\n", + 3, []string{"\n name: myserviceaccount2\n"}, }, { "namespace", @@ -164,7 +164,7 @@ func TestKptFnCommand(t *testing.T) { Namespace: "mynamespace", }, }}, - 3, " namespace: mynamespace\n", + 3, []string{" namespace: mynamespace\n"}, }, { "force namespace", @@ -176,7 +176,7 @@ func TestKptFnCommand(t *testing.T) { ForceNamespace: "forced-namespace", }, }}, - 3, " namespace: forced-namespace\n", + 3, []string{" namespace: forced-namespace\n"}, }, { "exclude", @@ -194,7 +194,7 @@ func TestKptFnCommand(t *testing.T) { }, }, }}, - 2, "myconfigb", + 2, []string{"myconfigb"}, }, { "include", @@ -218,10 +218,10 @@ func TestKptFnCommand(t *testing.T) { }, }, }}, - 1, "myconfigb", + 1, []string{"myconfigb"}, }, { - "output path", + "annotate output path", kptFnConfig{ ChartConfig: &config.ChartConfig{ LoaderConfig: config.LoaderConfig{ @@ -230,7 +230,23 @@ func TestKptFnCommand(t *testing.T) { }, OutputPath: "my/output/manifest.yaml", }, - 3, " config.kubernetes.io/path: my/output/manifest.yaml\n", + 3, []string{" annotations:\n config.kubernetes.io/index: 1\n config.kubernetes.io/path: my/output/manifest.yaml\n"}, + }, + { + "annotate output path when annotations empty", + kptFnConfig{ + ChartConfig: &config.ChartConfig{ + LoaderConfig: config.LoaderConfig{ + Chart: filepath.Join(exampleDir, "empty-annotations"), + }, + }, + OutputPath: "my/output/path/", + }, + 3, []string{ + "\n config.kubernetes.io/path: my/output/path/kustomization.yaml\n", + "\n config.kubernetes.io/path: my/output/path/serviceaccount_sa1.yaml\n", + "\n config.kubernetes.io/path: my/output/path/serviceaccount_sa2.yaml\n", + }, }, { "output kustomization", @@ -242,7 +258,7 @@ func TestKptFnCommand(t *testing.T) { }, OutputPath: "my/output/path/", }, - 4, "resources:\n - configmap_myconfiga.yaml\n - configmap_myconfigb.yaml\n", + 4, []string{"resources:\n - configmap_myconfiga.yaml\n - configmap_myconfigb.yaml\n"}, }, } { t.Run(c.name, func(t *testing.T) { @@ -285,7 +301,9 @@ func TestKptFnCommand(t *testing.T) { t.Log("\n" + out.String()) } } - require.Contains(t, out.String(), c.mustContain, "output of %#v", c.input) + for _, mustContain := range c.mustContain { + require.Contains(t, out.String(), mustContain, "output of %#v", c.input) + } }) } } diff --git a/cmd/khelm/template_test.go b/cmd/khelm/template_test.go index 6a3929b..072aa13 100644 --- a/cmd/khelm/template_test.go +++ b/cmd/khelm/template_test.go @@ -80,9 +80,9 @@ func TestTemplateCommand(t *testing.T) { 3, "namespace: mynamespace", }, { - "namespace", - []string{filepath.Join(exampleDir, "namespace"), "--force-namespace=forced-namespace"}, - 3, "namespace: forced-namespace", + "force-namespace", + []string{filepath.Join(exampleDir, "force-namespace"), "--force-namespace=forced-namespace"}, + 5, "namespace: forced-namespace", }, } { t.Run(c.name, func(t *testing.T) { diff --git a/example/empty-annotations/Chart.yaml b/example/empty-annotations/Chart.yaml new file mode 100644 index 0000000..747f6b4 --- /dev/null +++ b/example/empty-annotations/Chart.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +description: example chart with resource with empty annotations field. This is to verify that the kpt function maps paths reliably +name: empty-annotations +version: 0.1.0 diff --git a/example/empty-annotations/templates/template.yaml b/example/empty-annotations/templates/template.yaml new file mode 100644 index 0000000..bc7c216 --- /dev/null +++ b/example/empty-annotations/templates/template.yaml @@ -0,0 +1,10 @@ +apiVersion: v1 +kind: ServiceAccount +metadata: + name: sa1 +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: sa2 + annotations: diff --git a/example/force-namespace/Chart.yaml b/example/force-namespace/Chart.yaml new file mode 100644 index 0000000..a1d6ff6 --- /dev/null +++ b/example/force-namespace/Chart.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +description: example chart with resources with and without namespace +name: namespace +version: 0.1.0 diff --git a/example/force-namespace/generator.yaml b/example/force-namespace/generator.yaml index 215473b..38be008 100644 --- a/example/force-namespace/generator.yaml +++ b/example/force-namespace/generator.yaml @@ -1,6 +1,7 @@ apiVersion: khelm.mgoltzsche.github.com/v1 kind: ChartRenderer metadata: - name: force-namespace -chart: ../namespace + name: namespace-test + namespace: default-namespace +chart: . forceNamespace: forced-namespace diff --git a/example/force-namespace/templates/clusterrolebinding.yaml b/example/force-namespace/templates/clusterrolebinding.yaml new file mode 100644 index 0000000..52a8d0f --- /dev/null +++ b/example/force-namespace/templates/clusterrolebinding.yaml @@ -0,0 +1,12 @@ +apiVersion: rbac.authorization.k8s.io/v1beta1 +kind: ClusterRoleBinding +metadata: + name: jenkins-role-binding +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: cluster-admin +subjects: +- kind: ServiceAccount + name: jenkins + namespace: cluster-role-binding-ns diff --git a/example/force-namespace/templates/configmap.yaml b/example/force-namespace/templates/configmap.yaml new file mode 100644 index 0000000..028958f --- /dev/null +++ b/example/force-namespace/templates/configmap.yaml @@ -0,0 +1,30 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: myconfiga + namespace: {{ .Release.Namespace }} +data: + key: a +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: myconfigb +data: + key: b +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: myconfigc-with-empty-namespace + namespace: +data: + key: c +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: myconfigd-with-namespace-mappingnode + namespace: {} +data: + key: d diff --git a/example/namespace/generator.yaml b/example/namespace/generator.yaml index 96ec8c4..00cedfa 100644 --- a/example/namespace/generator.yaml +++ b/example/namespace/generator.yaml @@ -2,5 +2,5 @@ apiVersion: khelm.mgoltzsche.github.com/v1 kind: ChartRenderer metadata: name: namespace-test - namespace: install-namespace + namespace: default-namespace chart: . diff --git a/pkg/helm/render_test.go b/pkg/helm/render_test.go index 181feb9..5583d70 100644 --- a/pkg/helm/render_test.go +++ b/pkg/helm/render_test.go @@ -48,7 +48,7 @@ func TestRender(t *testing.T) { {"cert-manager", "example/cert-manager/generator.yaml", []string{"cert-manager", "kube-system"}, " name: cert-manager-webhook"}, {"apiversions-condition", "example/apiversions-condition/generator.yaml", []string{}, " config: fancy-config"}, {"expand-list", "example/expand-list/generator.yaml", []string{"ns1", "ns2", "ns3"}, "\n name: myserviceaccount2\n"}, - {"namespace", "example/namespace/generator.yaml", []string{"install-namespace", "cluster-role-binding-ns"}, " key: b"}, + {"namespace", "example/namespace/generator.yaml", []string{"default-namespace", "cluster-role-binding-ns"}, " key: b"}, {"force-namespace", "example/force-namespace/generator.yaml", []string{"forced-namespace"}, " key: b"}, {"kubeVersion", "example/release-name/generator.yaml", []string{}, " k8sVersion: v1.17.0"}, {"release-name", "example/release-name/generator.yaml", []string{}, " name: my-release-name-config"}, @@ -72,10 +72,14 @@ func TestRender(t *testing.T) { require.Contains(t, rendered.String(), c.expectedContained, "%syaml", cached) found := map[string]struct{}{} for _, o := range l { - ns, ok := o["metadata"].(map[string]interface{})["namespace"].(string) + ns := "" + meta := o["metadata"].(map[string]interface{}) + nsVal, ok := meta["namespace"] if ok { - require.NotEmpty(t, ns, "%s%s: output resource has empty namespace set explicitly", cached, c.file) - found[ns] = struct{}{} + if ns, ok = nsVal.(string); ok { + found[ns] = struct{}{} + } + require.NotEmpty(t, ns, "%s%s: output resource declares empty namespace field", cached, c.file) } subjects, ok := o["subjects"].([]interface{}) if ok && len(subjects) > 0 { diff --git a/pkg/helm/transform.go b/pkg/helm/transform.go index 77d14dc..ef5fe3f 100644 --- a/pkg/helm/transform.go +++ b/pkg/helm/transform.go @@ -105,8 +105,14 @@ func (t *manifestTransformer) applyNamespace(o *yaml.RNode, clusterScopedResourc namespaced, knownKind := openapi.IsNamespaceScoped(meta.TypeMeta) if t.ForceNamespace != "" && (namespaced || !knownKind) { // Forcefully set namespace on resource - err = o.PipeE(yaml.LookupCreate( - yaml.ScalarNode, yaml.MetadataField, yaml.NamespaceField), + err = o.PipeE( + yaml.LookupCreate(yaml.MappingNode, yaml.MetadataField), + yaml.FieldClearer{Name: yaml.NamespaceField}) + if err != nil { + return err + } + err = o.PipeE( + yaml.LookupCreate(yaml.ScalarNode, yaml.MetadataField, yaml.NamespaceField), yaml.FieldSetter{StringValue: t.ForceNamespace}) if err != nil { return err