From 067559127dabee78263bdc8d4ece54a076398ad6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Ortiz=20Garc=C3=ADa?= Date: Mon, 11 Oct 2021 19:16:08 -0500 Subject: [PATCH 1/3] test cases for framework wrapping/unwrapping bug --- kyaml/fn/framework/framework_test.go | 132 +++++++++++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/kyaml/fn/framework/framework_test.go b/kyaml/fn/framework/framework_test.go index 400f1ba7fc..82961ad199 100644 --- a/kyaml/fn/framework/framework_test.go +++ b/kyaml/fn/framework/framework_test.go @@ -90,3 +90,135 @@ results: tags: foo: bar`, strings.TrimSpace(out.String())) } + +func TestExecute_NoErrorResult(t *testing.T) { + singleDeployment := `kind: Deployment +apiVersion: v1 +metadata: + name: tester + namespace: default +spec: + replicas: 0` + resourceListDeployment := `kind: ResourceList +apiVersion: config.kubernetes.io/v1alpha1 +items: +- kind: Deployment + apiVersion: v1 + metadata: + name: tester + namespace: default + spec: + replicas: 0` + outputResourceList := `apiVersion: config.kubernetes.io/v1alpha1 +kind: ResourceList +items: +- kind: Deployment + apiVersion: v1 + metadata: + name: tester + namespace: default + spec: + replicas: 0 +results: + name: Incompatible config + items: + - message: bad value for replicas + severity: error + resourceRef: + apiVersion: v1 + kind: Deployment + name: tester + namespace: default + field: + path: .spec.Replicas + currentValue: "0" + suggestedValue: "3" + file: + path: /path/to/deployment.yaml + - message: some error + severity: error` + + testCases := []struct { + desc string + noWrap bool + wrapKind string + wrapAPIVersion string + input string + want string + }{ + { + desc: "resource list", + input: resourceListDeployment, + want: outputResourceList, + }, + { + desc: "individual resources", + input: singleDeployment, + want: singleDeployment, + }, + { + desc: "wrap resource list", + wrapKind: kio.ResourceListKind, + wrapAPIVersion: kio.ResourceListAPIVersion, + input: resourceListDeployment, + want: outputResourceList, + }, + { + desc: "unwrap resource list", + noWrap: true, + input: resourceListDeployment, + want: singleDeployment, + }, + { + desc: "wrap individual resources", + wrapKind: kio.ResourceListKind, + wrapAPIVersion: kio.ResourceListAPIVersion, + input: singleDeployment, + want: outputResourceList, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + p := framework.ResourceListProcessorFunc(func(rl *framework.ResourceList) error { + rl.Result = &framework.Result{ + Name: "Incompatible config", + Items: []framework.ResultItem{ + { + Message: "bad value for replicas", + Severity: framework.Error, + ResourceRef: yaml.ResourceIdentifier{ + TypeMeta: yaml.TypeMeta{APIVersion: "v1", Kind: "Deployment"}, + NameMeta: yaml.NameMeta{Name: "tester", Namespace: "default"}, + }, + Field: framework.Field{ + Path: ".spec.Replicas", + CurrentValue: "0", + SuggestedValue: "3", + }, + File: framework.File{ + Path: "/path/to/deployment.yaml", + Index: 0, + }, + }, + { + Message: "some error", + Severity: framework.Error, + }, + }, + } + return nil + }) + got := new(bytes.Buffer) + source := &kio.ByteReadWriter{ + Reader: bytes.NewBufferString(tc.input), + Writer: got, + WrappingAPIVersion: tc.wrapAPIVersion, + WrappingKind: tc.wrapKind, + NoWrap: tc.noWrap, + } + assert.NoError(t, framework.Execute(p, source)) + assert.Equal(t, tc.want, strings.TrimSpace(got.String())) + }) + } +} From ba4d83f75fc2c401087cfad73d167b84f481bede Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Ortiz=20Garc=C3=ADa?= Date: Mon, 11 Oct 2021 19:27:35 -0500 Subject: [PATCH 2/3] only override wrapping kind if it wasn't set already --- kyaml/fn/framework/framework_test.go | 96 ++++++++++++++++------------ kyaml/kio/byteio_reader.go | 17 +++-- 2 files changed, 65 insertions(+), 48 deletions(-) diff --git a/kyaml/fn/framework/framework_test.go b/kyaml/fn/framework/framework_test.go index 82961ad199..ab50ad033f 100644 --- a/kyaml/fn/framework/framework_test.go +++ b/kyaml/fn/framework/framework_test.go @@ -100,7 +100,7 @@ metadata: spec: replicas: 0` resourceListDeployment := `kind: ResourceList -apiVersion: config.kubernetes.io/v1alpha1 +apiVersion: config.kubernetes.io/v1 items: - kind: Deployment apiVersion: v1 @@ -109,7 +109,7 @@ items: namespace: default spec: replicas: 0` - outputResourceList := `apiVersion: config.kubernetes.io/v1alpha1 + outputResourceList := `apiVersion: config.kubernetes.io/v1 kind: ResourceList items: - kind: Deployment @@ -120,23 +120,25 @@ items: spec: replicas: 0 results: - name: Incompatible config - items: - - message: bad value for replicas - severity: error - resourceRef: - apiVersion: v1 - kind: Deployment - name: tester - namespace: default - field: - path: .spec.Replicas - currentValue: "0" - suggestedValue: "3" - file: - path: /path/to/deployment.yaml - - message: some error - severity: error` +- message: bad value for replicas + severity: error + resourceRef: + apiVersion: v1 + kind: Deployment + name: tester + namespace: default + field: + path: .spec.Replicas + currentValue: "0" + proposedValue: "3" + file: + path: /path/to/deployment.yaml +- message: some error + severity: error` + outputOtherWrap := strings.NewReplacer( + "kind: ResourceList", "kind: SomethingElse", + "apiVersion: config.kubernetes.io/v1", "apiVersion: fakeVersion", + ).Replace(outputResourceList) testCases := []struct { desc string @@ -176,36 +178,48 @@ results: input: singleDeployment, want: outputResourceList, }, + { + desc: "no wrap has precedence", + noWrap: true, + wrapKind: kio.ResourceListKind, + wrapAPIVersion: kio.ResourceListAPIVersion, + input: singleDeployment, + want: singleDeployment, + }, + { + desc: "honor specified wrapping kind", + wrapKind: "SomethingElse", + wrapAPIVersion: "fakeVersion", + input: resourceListDeployment, + want: outputOtherWrap, + }, } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { p := framework.ResourceListProcessorFunc(func(rl *framework.ResourceList) error { - rl.Result = &framework.Result{ - Name: "Incompatible config", - Items: []framework.ResultItem{ - { - Message: "bad value for replicas", - Severity: framework.Error, - ResourceRef: yaml.ResourceIdentifier{ - TypeMeta: yaml.TypeMeta{APIVersion: "v1", Kind: "Deployment"}, - NameMeta: yaml.NameMeta{Name: "tester", Namespace: "default"}, - }, - Field: framework.Field{ - Path: ".spec.Replicas", - CurrentValue: "0", - SuggestedValue: "3", - }, - File: framework.File{ - Path: "/path/to/deployment.yaml", - Index: 0, - }, + rl.Results = framework.Results{ + { + Message: "bad value for replicas", + Severity: framework.Error, + ResourceRef: yaml.ResourceIdentifier{ + TypeMeta: yaml.TypeMeta{APIVersion: "v1", Kind: "Deployment"}, + NameMeta: yaml.NameMeta{Name: "tester", Namespace: "default"}, + }, + Field: framework.Field{ + Path: ".spec.Replicas", + CurrentValue: "0", + ProposedValue: "3", }, - { - Message: "some error", - Severity: framework.Error, + File: framework.File{ + Path: "/path/to/deployment.yaml", + Index: 0, }, }, + { + Message: "some error", + Severity: framework.Error, + }, } return nil }) diff --git a/kyaml/kio/byteio_reader.go b/kyaml/kio/byteio_reader.go index 65a46d8228..18281a536d 100644 --- a/kyaml/kio/byteio_reader.go +++ b/kyaml/kio/byteio_reader.go @@ -67,12 +67,12 @@ func (rw *ByteReadWriter) Read() ([]*yaml.RNode, error) { WrapBareSeqNode: rw.WrapBareSeqNode, } val, err := b.Read() + rw.Results = b.Results + if rw.FunctionConfig == nil { rw.FunctionConfig = b.FunctionConfig } - rw.Results = b.Results - - if !rw.NoWrap { + if !rw.NoWrap && rw.WrappingKind == "" { rw.WrappingAPIVersion = b.WrappingAPIVersion rw.WrappingKind = b.WrappingKind } @@ -80,15 +80,18 @@ func (rw *ByteReadWriter) Read() ([]*yaml.RNode, error) { } func (rw *ByteReadWriter) Write(nodes []*yaml.RNode) error { - return ByteWriter{ + w := ByteWriter{ Writer: rw.Writer, KeepReaderAnnotations: rw.KeepReaderAnnotations, Style: rw.Style, FunctionConfig: rw.FunctionConfig, Results: rw.Results, - WrappingAPIVersion: rw.WrappingAPIVersion, - WrappingKind: rw.WrappingKind, - }.Write(nodes) + } + if !rw.NoWrap { + w.WrappingAPIVersion = rw.WrappingAPIVersion + w.WrappingKind = rw.WrappingKind + } + return w.Write(nodes) } // ParseAll reads all of the inputs into resources From 20c608989a841ced1baf1a5e04354c337753d28c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Ortiz=20Garc=C3=ADa?= Date: Mon, 8 Nov 2021 11:55:02 -0800 Subject: [PATCH 3/3] Move kio.ByteRW tests from framework_test to byteio_readwriter_test --- kyaml/fn/framework/framework_test.go | 146 --------------------------- kyaml/kio/byteio_readwriter_test.go | 119 ++++++++++++++++++++++ 2 files changed, 119 insertions(+), 146 deletions(-) diff --git a/kyaml/fn/framework/framework_test.go b/kyaml/fn/framework/framework_test.go index ab50ad033f..400f1ba7fc 100644 --- a/kyaml/fn/framework/framework_test.go +++ b/kyaml/fn/framework/framework_test.go @@ -90,149 +90,3 @@ results: tags: foo: bar`, strings.TrimSpace(out.String())) } - -func TestExecute_NoErrorResult(t *testing.T) { - singleDeployment := `kind: Deployment -apiVersion: v1 -metadata: - name: tester - namespace: default -spec: - replicas: 0` - resourceListDeployment := `kind: ResourceList -apiVersion: config.kubernetes.io/v1 -items: -- kind: Deployment - apiVersion: v1 - metadata: - name: tester - namespace: default - spec: - replicas: 0` - outputResourceList := `apiVersion: config.kubernetes.io/v1 -kind: ResourceList -items: -- kind: Deployment - apiVersion: v1 - metadata: - name: tester - namespace: default - spec: - replicas: 0 -results: -- message: bad value for replicas - severity: error - resourceRef: - apiVersion: v1 - kind: Deployment - name: tester - namespace: default - field: - path: .spec.Replicas - currentValue: "0" - proposedValue: "3" - file: - path: /path/to/deployment.yaml -- message: some error - severity: error` - outputOtherWrap := strings.NewReplacer( - "kind: ResourceList", "kind: SomethingElse", - "apiVersion: config.kubernetes.io/v1", "apiVersion: fakeVersion", - ).Replace(outputResourceList) - - testCases := []struct { - desc string - noWrap bool - wrapKind string - wrapAPIVersion string - input string - want string - }{ - { - desc: "resource list", - input: resourceListDeployment, - want: outputResourceList, - }, - { - desc: "individual resources", - input: singleDeployment, - want: singleDeployment, - }, - { - desc: "wrap resource list", - wrapKind: kio.ResourceListKind, - wrapAPIVersion: kio.ResourceListAPIVersion, - input: resourceListDeployment, - want: outputResourceList, - }, - { - desc: "unwrap resource list", - noWrap: true, - input: resourceListDeployment, - want: singleDeployment, - }, - { - desc: "wrap individual resources", - wrapKind: kio.ResourceListKind, - wrapAPIVersion: kio.ResourceListAPIVersion, - input: singleDeployment, - want: outputResourceList, - }, - { - desc: "no wrap has precedence", - noWrap: true, - wrapKind: kio.ResourceListKind, - wrapAPIVersion: kio.ResourceListAPIVersion, - input: singleDeployment, - want: singleDeployment, - }, - { - desc: "honor specified wrapping kind", - wrapKind: "SomethingElse", - wrapAPIVersion: "fakeVersion", - input: resourceListDeployment, - want: outputOtherWrap, - }, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - p := framework.ResourceListProcessorFunc(func(rl *framework.ResourceList) error { - rl.Results = framework.Results{ - { - Message: "bad value for replicas", - Severity: framework.Error, - ResourceRef: yaml.ResourceIdentifier{ - TypeMeta: yaml.TypeMeta{APIVersion: "v1", Kind: "Deployment"}, - NameMeta: yaml.NameMeta{Name: "tester", Namespace: "default"}, - }, - Field: framework.Field{ - Path: ".spec.Replicas", - CurrentValue: "0", - ProposedValue: "3", - }, - File: framework.File{ - Path: "/path/to/deployment.yaml", - Index: 0, - }, - }, - { - Message: "some error", - Severity: framework.Error, - }, - } - return nil - }) - got := new(bytes.Buffer) - source := &kio.ByteReadWriter{ - Reader: bytes.NewBufferString(tc.input), - Writer: got, - WrappingAPIVersion: tc.wrapAPIVersion, - WrappingKind: tc.wrapKind, - NoWrap: tc.noWrap, - } - assert.NoError(t, framework.Execute(p, source)) - assert.Equal(t, tc.want, strings.TrimSpace(got.String())) - }) - } -} diff --git a/kyaml/kio/byteio_readwriter_test.go b/kyaml/kio/byteio_readwriter_test.go index ce0c4e5c66..3f8d22c21a 100644 --- a/kyaml/kio/byteio_readwriter_test.go +++ b/kyaml/kio/byteio_readwriter_test.go @@ -747,3 +747,122 @@ func TestByteReadWriter_WrapBareSeqNode(t *testing.T) { }) } } + +func TestByteReadWriter_ResourceListWrapping(t *testing.T) { + singleDeployment := `kind: Deployment +apiVersion: v1 +metadata: + name: tester + namespace: default +spec: + replicas: 0` + resourceList := `apiVersion: config.kubernetes.io/v1 +kind: ResourceList +items: +- kind: Deployment + apiVersion: v1 + metadata: + name: tester + namespace: default + spec: + replicas: 0` + resourceListWithError := `apiVersion: config.kubernetes.io/v1 +kind: ResourceList +items: +- kind: Deployment + apiVersion: v1 + metadata: + name: tester + namespace: default + spec: + replicas: 0 +results: +- message: some error + severity: error` + resourceListDifferentWrapper := strings.NewReplacer( + "kind: ResourceList", "kind: SomethingElse", + "apiVersion: config.kubernetes.io/v1", "apiVersion: fakeVersion", + ).Replace(resourceList) + + testCases := []struct { + desc string + noWrap bool + wrapKind string + wrapAPIVersion string + input string + want string + }{ + { + desc: "resource list", + input: resourceList, + want: resourceList, + }, + { + desc: "individual resources", + input: singleDeployment, + want: singleDeployment, + }, + { + desc: "no nested wrapping", + wrapKind: kio.ResourceListKind, + wrapAPIVersion: kio.ResourceListAPIVersion, + input: resourceList, + want: resourceList, + }, + { + desc: "unwrap resource list", + noWrap: true, + input: resourceList, + want: singleDeployment, + }, + { + desc: "wrap individual resources", + wrapKind: kio.ResourceListKind, + wrapAPIVersion: kio.ResourceListAPIVersion, + input: singleDeployment, + want: resourceList, + }, + { + desc: "NoWrap has precedence", + noWrap: true, + wrapKind: kio.ResourceListKind, + wrapAPIVersion: kio.ResourceListAPIVersion, + input: singleDeployment, + want: singleDeployment, + }, + { + desc: "honor specified wrapping kind", + wrapKind: "SomethingElse", + wrapAPIVersion: "fakeVersion", + input: resourceList, + want: resourceListDifferentWrapper, + }, + { + desc: "passthrough results", + input: resourceListWithError, + want: resourceListWithError, + }, + } + + for i := range testCases { + tc := testCases[i] + t.Run(tc.desc, func(t *testing.T) { + var got bytes.Buffer + rw := kio.ByteReadWriter{ + Reader: strings.NewReader(tc.input), + Writer: &got, + NoWrap: tc.noWrap, + WrappingAPIVersion: tc.wrapAPIVersion, + WrappingKind: tc.wrapKind, + } + + rnodes, err := rw.Read() + assert.NoError(t, err) + + err = rw.Write(rnodes) + assert.NoError(t, err) + + assert.Equal(t, tc.want, strings.TrimSpace(got.String())) + }) + } +}