From e02da7745360d64ff12662c218b255e0b057fbff Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Thu, 14 Dec 2023 21:56:38 +0200 Subject: [PATCH 1/2] Prove that preserve-unknown-fields doesn't work with arrays If an array type is present in the CRD, the `parentPath` function fails to compose the full path to the spec parent for all fields declared after the array. Signed-off-by: Stefan Prodan --- internal/engine/importer.go | 6 ++-- internal/engine/importer_test.go | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/internal/engine/importer.go b/internal/engine/importer.go index ca509e65..eefbd2d6 100644 --- a/internal/engine/importer.go +++ b/internal/engine/importer.go @@ -252,7 +252,8 @@ func convertCRD(crd cue.Value) (*IntermediateCRD, error) { // all the way to the file root err = walkfn(defpath.Selectors(), rootosch) - // First pass of astutil.Apply to remove ellipses for fields not marked with x-kubernetes-embedded-resource: true + // First pass of astutil.Apply to remove ellipses for fields not marked with + // 'x-kubernetes-preserve-unknown-fields: true'. // Note that this implementation is only correct for CUE inputs that do not contain references. // It is safe to use in this context because CRDs already have that invariant. var stack []ast.Node @@ -278,6 +279,7 @@ func convertCRD(crd cue.Value) (*IntermediateCRD, error) { } stack = append(stack[:i], pc.Node()) pathstack = append(pathstack[:i], psel) + if !preserve[cue.MakePath(pathstack...).String()] { newlist := make([]ast.Decl, 0, len(x.Elts)) for _, elt := range x.Elts { @@ -366,7 +368,7 @@ func convertCRD(crd cue.Value) (*IntermediateCRD, error) { // - *ast.ListLit (index is the path) // - *ast.Field (label is the path) // -// If the there exceptions for the above two items, or the list should properly +// If there are exceptions for the above two items, or the list should properly // have more items, this func will be buggy func parentPath(c astutil.Cursor) (cue.Selector, astutil.Cursor) { p, prior := c.Parent(), c diff --git a/internal/engine/importer_test.go b/internal/engine/importer_test.go index ae3bfc1a..26a68a0c 100644 --- a/internal/engine/importer_test.go +++ b/internal/engine/importer_test.go @@ -293,6 +293,50 @@ func TestConvertCRD(t *testing.T) { innerField?: string } ... +}`, + }, + { + name: "array-xk-preserve", + spec: `type: "object" + properties: { + resources: { + properties: claims: { + items: { + properties: name: type: "string" + required: ["name"] + type: "object" + } + type: "array" + "x-kubernetes-list-map-keys": ["name"] + "x-kubernetes-list-type": "map" + } + type: "object" + } + spec: { + properties: template: { + properties: values: { + description: "Preserve unknown fields." + type: "object" + "x-kubernetes-preserve-unknown-fields": true + } + type: "object" + } + type: "object" + } + } + `, + expect: `{ + resources?: { + claims?: [...{ + name: string + }] + } + spec?: { + template?: { + // Preserve unknown fields. + values?: {} + } + } }`, }, } @@ -314,6 +358,9 @@ func TestConvertCRD(t *testing.T) { // remove the _#def injected by CUE's syntax formatter fn, err := format.Node(specNode.(*ast.StructLit).Elts[1].(*ast.Field).Value) g.Expect(err).ToNot(HaveOccurred()) + + t.Log(string(fn)) + diff := cmp.Diff(tt.expect, string(fn), multiline) if diff != "" { t.Fatal(diff) From c1d602d1dbf96bf9f738f69b8279c013d2538cc7 Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Fri, 15 Dec 2023 12:26:47 +0200 Subject: [PATCH 2/2] Workaround for preserve-unknown-fields arrays issue For CRDs with arrays the `parentPath` function doesn't walk back to the spec. To not miss `x-kubernetes-preserve-unknown-fields` we do partial matching which could open more fields than desired if the field names overlap. Signed-off-by: Stefan Prodan --- internal/engine/importer.go | 14 +++++++++++++- internal/engine/importer_test.go | 4 +++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/internal/engine/importer.go b/internal/engine/importer.go index eefbd2d6..109a31ac 100644 --- a/internal/engine/importer.go +++ b/internal/engine/importer.go @@ -280,7 +280,19 @@ func convertCRD(crd cue.Value) (*IntermediateCRD, error) { stack = append(stack[:i], pc.Node()) pathstack = append(pathstack[:i], psel) - if !preserve[cue.MakePath(pathstack...).String()] { + // Risk not closing up fields that are not marked with 'x-kubernetes-preserve-unknown-fields: true' + // if the current field name matches a path that is marked with preserve unknown. + // TODO: find a way to fix parentPath when arrays are involved. + currentPath := cue.MakePath(pathstack...).String() + found := false + for k, ok := range preserve { + if strings.HasSuffix(k, currentPath) && ok { + found = true + break + } + } + + if !found { newlist := make([]ast.Decl, 0, len(x.Elts)) for _, elt := range x.Elts { if _, is := elt.(*ast.Ellipsis); !is { diff --git a/internal/engine/importer_test.go b/internal/engine/importer_test.go index 26a68a0c..838cbde7 100644 --- a/internal/engine/importer_test.go +++ b/internal/engine/importer_test.go @@ -334,7 +334,9 @@ func TestConvertCRD(t *testing.T) { spec?: { template?: { // Preserve unknown fields. - values?: {} + values?: { + ... + } } } }`,