diff --git a/api/internal/target/kusttarget_test.go b/api/internal/target/kusttarget_test.go index fbb19ebaf..a0a279e38 100644 --- a/api/internal/target/kusttarget_test.go +++ b/api/internal/target/kusttarget_test.go @@ -244,7 +244,7 @@ metadata: t.Fatalf("unexpected error %v", err) } } - assert.NoError(t, expected.RemoveIdAnnotations()) + expected.RemoveIdAnnotations() expYaml, err := expected.AsYaml() assert.NoError(t, err) @@ -252,7 +252,7 @@ metadata: assert.NoError(t, kt.Load()) actual, err := kt.MakeCustomizedResMap() assert.NoError(t, err) - assert.NoError(t, actual.RemoveIdAnnotations()) + actual.RemoveIdAnnotations() actYaml, err := actual.AsYaml() assert.NoError(t, err) assert.Equal(t, expYaml, actYaml) diff --git a/api/krusty/basic_io_test.go b/api/krusty/basic_io_test.go index 82a38e8fc..9700ec41a 100644 --- a/api/krusty/basic_io_test.go +++ b/api/krusty/basic_io_test.go @@ -4,13 +4,33 @@ package krusty_test import ( + "fmt" "testing" kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" ) -func TestBasicIO1(t *testing.T) { +func TestBasicIO_1(t *testing.T) { th := kusttest_test.MakeHarness(t) + opts := th.MakeDefaultOptions() + if !opts.UseKyaml { + // This test won't pass under apimachinery, because in the bowels of + // that code (see GetAnnotations in v0.17.0 of + // k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go) + // an error returned from NestedStringMap is discarded, and an + // empty annotation map is silently returned, making this test fail + // The swallowed error arises from code like: + // var v interface{} + // v = true + // if str, ok := v.(string); ok { + // save the value in a map (doesn't happen) + // } else { + // return an error (that is then ignored by GetAnnotations) + // } + // The error happens when any annotation value can be interpreted as + // a boolean or number. + t.SkipNow() + } th.WriteK(".", ` resources: - service.yaml @@ -27,7 +47,8 @@ metadata: spec: clusterIP: None `) - m := th.Run(".", th.MakeDefaultOptions()) + m := th.Run(".", opts) + // The annotations are sorted by key, hence the order change. th.AssertActualEqualsExpected( m, ` apiVersion: v1 @@ -42,3 +63,45 @@ spec: clusterIP: None `) } + +func TestBasicIO_2(t *testing.T) { + th := kusttest_test.MakeHarness(t) + opts := th.MakeDefaultOptions() + th.WriteK(".", ` +resources: +- service.yaml +`) + // All the annotation values are quoted. + // The apimachinery code path retains the quotes, the kyaml + // path drops them at the moment. + th.WriteF("service.yaml", ` +apiVersion: v1 +kind: Service +metadata: + annotations: + port: "8080" + happy: "true" + color: green + name: demo +spec: + clusterIP: None +`) + m := th.Run(".", opts) + // The annotations are sorted by key, hence the order change. + expFmt := ` +apiVersion: v1 +kind: Service +metadata: + annotations: + color: green + happy: %s + port: %s + name: demo +spec: + clusterIP: None +` + th.AssertActualEqualsExpected( + m, opts.IfApiMachineryElseKyaml( + fmt.Sprintf(expFmt, `"true"`, `"8080"`), + fmt.Sprintf(expFmt, `true`, `8080`))) +} diff --git a/api/krusty/configmaps_test.go b/api/krusty/configmaps_test.go index c698b3e15..3847f6d56 100644 --- a/api/krusty/configmaps_test.go +++ b/api/krusty/configmaps_test.go @@ -27,10 +27,6 @@ configMapGenerator: apiVersion: v1 kind: Service metadata: - annotations: - port: 8080 - happy: true - color: green name: demo spec: clusterIP: None @@ -41,10 +37,6 @@ spec: apiVersion: v1 kind: Service metadata: - annotations: - color: green - happy: true - port: 8080 name: demo spec: clusterIP: None diff --git a/api/krusty/kustomizer.go b/api/krusty/kustomizer.go index 1d354c68e..6e78628b8 100644 --- a/api/krusty/kustomizer.go +++ b/api/krusty/kustomizer.go @@ -93,9 +93,6 @@ func (b *Kustomizer) Run(path string) (resmap.ResMap, error) { } t.Transform(m) } - err = m.RemoveIdAnnotations() - if err != nil { - return nil, err - } + m.RemoveIdAnnotations() return m, nil } diff --git a/api/krusty/variableref_test.go b/api/krusty/variableref_test.go index cf73f27ad..41b55ffcf 100644 --- a/api/krusty/variableref_test.go +++ b/api/krusty/variableref_test.go @@ -4,6 +4,7 @@ package krusty_test import ( + "fmt" "strings" "testing" @@ -360,7 +361,7 @@ resources: } } -// TODO: varref has some quote issues +// TODO(#3449): varref has some quote issues // https://github.com/kubernetes-sigs/kustomize/issues/3449 func TestVarRefBig(t *testing.T) { th := kusttest_test.MakeHarness(t) @@ -682,7 +683,7 @@ resources: - ../../base `) m := th.Run("/app/overlay/staging", opts) - th.AssertActualEqualsExpected(m, ` + expFmt := ` apiVersion: v1 kind: ServiceAccount metadata: @@ -756,9 +757,9 @@ kind: Service metadata: annotations: prometheus.io/path: _status/vars - prometheus.io/port: 8080 - prometheus.io/scrape: true - service.alpha.kubernetes.io/tolerate-unready-endpoints: true + prometheus.io/port: %s + prometheus.io/scrape: %s + service.alpha.kubernetes.io/tolerate-unready-endpoints: %s labels: app: cockroachdb name: dev-base-cockroachdb @@ -769,8 +770,8 @@ spec: port: 26257 targetPort: 26257 - name: http - port: "8080" - targetPort: "8080" + port: %s + targetPort: %s selector: app: cockroachdb --- @@ -786,8 +787,8 @@ spec: port: 26257 targetPort: 26257 - name: http - port: "8080" - targetPort: "8080" + port: %s + targetPort: %s selector: app: cockroachdb --- @@ -824,8 +825,8 @@ spec: - --host $(hostname -f) - --http-host 0.0.0.0 - --join dev-base-cockroachdb-0.dev-base-cockroachdb,dev-base-cockroachdb-1.dev-base-cockroachdb,dev-base-cockroachdb-2.dev-base-cockroachdb - - --cache 25% - - --max-sql-memory 25% + - --cache 25%% + - --max-sql-memory 25%% image: cockroachdb/cockroach:v1.1.5 imagePullPolicy: IfNotPresent name: cockroachdb @@ -925,7 +926,16 @@ data: kind: ConfigMap metadata: name: dev-base-test-config-map-6b85g79g7g -`) +` + th.AssertActualEqualsExpected(m, + opts.IfApiMachineryElseKyaml( + fmt.Sprintf( + expFmt, + `"8080"`, `"true"`, `"true"`, `8080`, `8080`, `8080`, `8080`), + fmt.Sprintf( + expFmt, + `8080`, `true`, `true`, `"8080"`, `"8080"`, `"8080"`, `"8080"`), + )) } func TestVariableRefIngressBasic(t *testing.T) { diff --git a/api/resmap/resmap.go b/api/resmap/resmap.go index efdef0763..ad97212b7 100644 --- a/api/resmap/resmap.go +++ b/api/resmap/resmap.go @@ -246,5 +246,6 @@ type ResMap interface { ApplySmPatch( selectedSet *resource.IdSet, patch *resource.Resource) error - RemoveIdAnnotations() error + // Remove annotations used exclusively by the kustomize build process. + RemoveIdAnnotations() } diff --git a/api/resmap/reswrangler.go b/api/resmap/reswrangler.go index 2369245f2..d0ad72c9c 100644 --- a/api/resmap/reswrangler.go +++ b/api/resmap/reswrangler.go @@ -621,12 +621,8 @@ func (m *resWrangler) ApplySmPatch( return nil } -func (m *resWrangler) RemoveIdAnnotations() error { +func (m *resWrangler) RemoveIdAnnotations() { for _, r := range m.Resources() { - err := r.RemoveIdAnnotations() - if err != nil { - return err - } + r.RemoveIdAnnotations() } - return nil } diff --git a/api/resmap/reswrangler_test.go b/api/resmap/reswrangler_test.go index 8668a683f..317d09505 100644 --- a/api/resmap/reswrangler_test.go +++ b/api/resmap/reswrangler_test.go @@ -998,7 +998,7 @@ spec: return } assert.False(t, tc.errorExpected) - assert.NoError(t, m.RemoveIdAnnotations()) + m.RemoveIdAnnotations() yml, err := m.AsYaml() assert.NoError(t, err) assert.Equal(t, strings.Join(tc.expected, "---\n"), string(yml)) @@ -1111,7 +1111,7 @@ $patch: delete assert.NoError(t, err, name) assert.NoError(t, m.ApplySmPatch(idSet, p), name) assert.Equal(t, tc.finalMapSize, m.Size(), name) - assert.NoError(t, m.RemoveIdAnnotations()) + m.RemoveIdAnnotations() yml, err := m.AsYaml() assert.NoError(t, err, name) assert.Equal(t, tc.expected, string(yml), name) diff --git a/api/resource/resource.go b/api/resource/resource.go index e569496e2..4a20246d6 100644 --- a/api/resource/resource.go +++ b/api/resource/resource.go @@ -244,28 +244,26 @@ func copyStringSlice(s []string) []string { // Implements ResCtx AddNamePrefix func (r *Resource) AddNamePrefix(p string) { - annotations := r.GetAnnotations() - if annotations == nil { - annotations = make(map[string]string) - } - if _, ok := annotations[prefixAnnotation]; !ok { - annotations[prefixAnnotation] = p - } else { - annotations[prefixAnnotation] = annotations[prefixAnnotation] + "," + p - } - r.SetAnnotations(annotations) + r.addAdditiveAnnotation(prefixAnnotation, p) } // Implements ResCtx AddNameSuffix func (r *Resource) AddNameSuffix(s string) { + r.addAdditiveAnnotation(suffixAnnotation, s) +} + +func (r *Resource) addAdditiveAnnotation(name, value string) { + if value == "" { + return + } annotations := r.GetAnnotations() if annotations == nil { annotations = make(map[string]string) } - if _, ok := annotations[suffixAnnotation]; !ok { - annotations[suffixAnnotation] = s + if existing, ok := annotations[name]; ok { + annotations[name] = existing + "," + value } else { - annotations[suffixAnnotation] = annotations[suffixAnnotation] + "," + s + annotations[name] = value } r.SetAnnotations(annotations) } @@ -347,50 +345,16 @@ func (r *Resource) PrefixesSuffixesEquals(o ResCtx) bool { return sameEndingSubarray(r.GetNamePrefixes(), o.GetNamePrefixes()) && sameEndingSubarray(r.GetNameSuffixes(), o.GetNameSuffixes()) } -func (r *Resource) RemoveIdAnnotations() error { +func (r *Resource) RemoveIdAnnotations() { annotations := r.GetAnnotations() - if annotations == nil { - annotations = make(map[string]string) + if len(annotations) == 0 { + return } delete(annotations, nameAnnotation) delete(annotations, prefixAnnotation) delete(annotations, suffixAnnotation) delete(annotations, namespaceAnnotation) - if len(annotations) == 0 { - json, err := r.MarshalJSON() - if err != nil { - return err - } - - yml, err := yaml.JSONToYAML(json) - if err != nil { - return err - } - - rnode, err := kyaml.Parse(string(yml)) - if err != nil { - return err - } - - err = rnode.SetAnnotations(nil) - if err != nil { - return err - } - - b, err := rnode.MarshalJSON() - if err != nil { - return err - } - - err = r.UnmarshalJSON(b) - if err != nil { - return err - } - return nil - } - r.SetAnnotations(annotations) - return nil } func (r *Resource) GetOriginalName() string { diff --git a/api/testutils/kusttest/harness.go b/api/testutils/kusttest/harness.go index 7146899af..f39b035dd 100644 --- a/api/testutils/kusttest/harness.go +++ b/api/testutils/kusttest/harness.go @@ -127,10 +127,7 @@ func (th Harness) AssertActualEqualsExpected( } func (th Harness) AssertActualEqualsExpectedNoIdAnnotations(m resmap.ResMap, expected string) { - err := m.RemoveIdAnnotations() - if err != nil { - th.t.Fatalf("Err: %v", err) - } + m.RemoveIdAnnotations() th.AssertActualEqualsExpectedWithTweak(m, nil, expected) } diff --git a/plugin/builtin/prefixsuffixtransformer/PrefixSuffixTransformer_test.go b/plugin/builtin/prefixsuffixtransformer/PrefixSuffixTransformer_test.go index f6bc09860..9e3efcb34 100644 --- a/plugin/builtin/prefixsuffixtransformer/PrefixSuffixTransformer_test.go +++ b/plugin/builtin/prefixsuffixtransformer/PrefixSuffixTransformer_test.go @@ -137,7 +137,6 @@ metadata: annotations: config.kubernetes.io/originalName: deployment config.kubernetes.io/prefixes: test- - config.kubernetes.io/suffixes: null name: test-deployment spec: template: