From e1804cbc76d0bb0256b1fd174a0bef6ba2973a95 Mon Sep 17 00:00:00 2001 From: phani Date: Wed, 7 Jul 2021 10:12:44 -0700 Subject: [PATCH] Retain field order after running any arbitrary functions on resources (#4021) * Reorder resource fields * Fix comment conflict * Update e2e test ordering * Suggested changes --- cmd/config/internal/commands/e2e/e2e_test.go | 6 +- kyaml/fn/runtime/runtimeutil/runtimeutil.go | 10 +- kyaml/order/syncorder.go | 122 ++++++ kyaml/order/syncorder_test.go | 397 +++++++++++++++++++ kyaml/sliceutil/slice.go | 25 ++ kyaml/sliceutil/sliceutil_test.go | 25 ++ kyaml/yaml/rnode.go | 45 +++ 7 files changed, 624 insertions(+), 6 deletions(-) create mode 100644 kyaml/order/syncorder.go create mode 100644 kyaml/order/syncorder_test.go create mode 100644 kyaml/sliceutil/slice.go create mode 100644 kyaml/sliceutil/sliceutil_test.go diff --git a/cmd/config/internal/commands/e2e/e2e_test.go b/cmd/config/internal/commands/e2e/e2e_test.go index c74721d791..54749ed9f2 100644 --- a/cmd/config/internal/commands/e2e/e2e_test.go +++ b/cmd/config/internal/commands/e2e/e2e_test.go @@ -542,13 +542,13 @@ kind: Input metadata: name: foo annotations: - a-bool-value: true - a-int-value: 2 - a-string-value: a config.kubernetes.io/function: | starlark: path: script.star name: fn + a-bool-value: true + a-int-value: 2 + a-string-value: a data: boolValue: true intValue: 2 diff --git a/kyaml/fn/runtime/runtimeutil/runtimeutil.go b/kyaml/fn/runtime/runtimeutil/runtimeutil.go index 2454aa4cb2..f3f54606e4 100644 --- a/kyaml/fn/runtime/runtimeutil/runtimeutil.go +++ b/kyaml/fn/runtime/runtimeutil/runtimeutil.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/kustomize/kyaml/errors" "sigs.k8s.io/kustomize/kyaml/kio" "sigs.k8s.io/kustomize/kyaml/kio/kioutil" + "sigs.k8s.io/kustomize/kyaml/order" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -169,8 +170,8 @@ func (c *FunctionFilter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) { return nil, err } - // copy the comments from the inputs to the outputs - if err := c.setComments(output); err != nil { + // copy the comments and sync the order of fields from the inputs to the outputs + if err := c.copyCommentsAndSyncOrder(output); err != nil { return nil, err } @@ -210,7 +211,7 @@ func (c *FunctionFilter) setIds(nodes []*yaml.RNode) error { return nil } -func (c *FunctionFilter) setComments(nodes []*yaml.RNode) error { +func (c *FunctionFilter) copyCommentsAndSyncOrder(nodes []*yaml.RNode) error { for i := range nodes { node := nodes[i] anID, err := node.Pipe(yaml.GetAnnotation(idAnnotation)) @@ -229,6 +230,9 @@ func (c *FunctionFilter) setComments(nodes []*yaml.RNode) error { if err := comments.CopyComments(in, node); err != nil { return errors.Wrap(err) } + if err := order.SyncOrder(in, node); err != nil { + return errors.Wrap(err) + } if err := node.PipeE(yaml.ClearAnnotation(idAnnotation)); err != nil { return errors.Wrap(err) } diff --git a/kyaml/order/syncorder.go b/kyaml/order/syncorder.go new file mode 100644 index 0000000000..06d157a5de --- /dev/null +++ b/kyaml/order/syncorder.go @@ -0,0 +1,122 @@ +// Copyright 2021 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package order + +import ( + "sigs.k8s.io/kustomize/kyaml/errors" + "sigs.k8s.io/kustomize/kyaml/yaml" +) + +// SyncOrder recursively sorts the map node keys in 'to' node to match the order of +// map node keys in 'from' node at same tree depth, additional keys are moved to the end +// Field order might be altered due to round-tripping in arbitrary functions. +// This functionality helps to retain the original order of fields to avoid unnecessary diffs. +func SyncOrder(from, to *yaml.RNode) error { + if err := syncOrder(from, to); err != nil { + return errors.Errorf("failed to sync field order: %q", err.Error()) + } + rearrangeHeadCommentOfSeqNode(to.YNode()) + return nil +} + +func syncOrder(from, to *yaml.RNode) error { + if from.IsNilOrEmpty() || to.IsNilOrEmpty() { + return nil + } + switch from.YNode().Kind { + case yaml.DocumentNode: + // Traverse the child of the documents + return syncOrder(yaml.NewRNode(from.YNode()), yaml.NewRNode(to.YNode())) + case yaml.MappingNode: + return VisitFields(from, to, func(fNode, tNode *yaml.MapNode) error { + // Traverse each field value + if fNode == nil || tNode == nil { + return nil + } + return syncOrder(fNode.Value, tNode.Value) + }) + case yaml.SequenceNode: + return VisitElements(from, to, func(fNode, tNode *yaml.RNode) error { + // Traverse each list element + return syncOrder(fNode, tNode) + }) + } + return nil +} + +// VisitElements calls fn for each element in a SequenceNode. +// Returns an error for non-SequenceNodes +func VisitElements(from, to *yaml.RNode, fn func(fNode, tNode *yaml.RNode) error) error { + fElements, err := from.Elements() + if err != nil { + return errors.Wrap(err) + } + + tElements, err := to.Elements() + if err != nil { + return errors.Wrap(err) + } + for i := range fElements { + if i >= len(tElements) { + return nil + } + if err := fn(fElements[i], tElements[i]); err != nil { + return errors.Wrap(err) + } + } + return nil +} + +// VisitFields calls fn for each field in the RNode. +// Returns an error for non-MappingNodes. +func VisitFields(from, to *yaml.RNode, fn func(fNode, tNode *yaml.MapNode) error) error { + srcFieldNames, err := from.Fields() + if err != nil { + return nil + } + yaml.SyncMapNodesOrder(from, to) + // visit each field + for _, fieldName := range srcFieldNames { + if err := fn(from.Field(fieldName), to.Field(fieldName)); err != nil { + return errors.Wrap(err) + } + } + return nil +} + +// rearrangeHeadCommentOfSeqNode addresses a remote corner case due to moving a +// map node in a sequence node with a head comment to the top +func rearrangeHeadCommentOfSeqNode(node *yaml.Node) { + if node == nil { + return + } + switch node.Kind { + case yaml.DocumentNode: + for _, node := range node.Content { + rearrangeHeadCommentOfSeqNode(node) + } + + case yaml.MappingNode: + for _, node := range node.Content { + rearrangeHeadCommentOfSeqNode(node) + } + + case yaml.SequenceNode: + for _, node := range node.Content { + // for each child mapping node, transfer the head comment of it's + // first child scalar node to the head comment of itself + if len(node.Content) > 0 && node.Content[0].Kind == yaml.ScalarNode { + if node.HeadComment == "" { + node.HeadComment = node.Content[0].HeadComment + continue + } + + if node.Content[0].HeadComment != "" { + node.HeadComment += "\n" + node.Content[0].HeadComment + node.Content[0].HeadComment = "" + } + } + } + } +} diff --git a/kyaml/order/syncorder_test.go b/kyaml/order/syncorder_test.go new file mode 100644 index 0000000000..4244d503db --- /dev/null +++ b/kyaml/order/syncorder_test.go @@ -0,0 +1,397 @@ +// Copyright 2021 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package order + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" + "sigs.k8s.io/kustomize/kyaml/kio" + "sigs.k8s.io/kustomize/kyaml/yaml" +) + +func TestSyncOrder(t *testing.T) { + testCases := []struct { + name string + from string + to string + expected string + }{ + { + name: "sort data fields configmap with comments", + from: `apiVersion: v1 +kind: ConfigMap +metadata: + name: setters-config +data: + # This should be the name of your Config Controller instance + cluster-name: cluster-name + # This should be the project where you deployed Config Controller + project-id: project-id # pro + project-number: "1234567890123" + # You can leave these defaults + namespace: config-control + deployment-repo: deployment-repo + source-repo: source-repo +`, + to: `apiVersion: v1 +kind: ConfigMap +metadata: # kpt-merge: /setters-config + name: setters-config +data: + # You can leave these defaults + namespace: config-control + # This should be the name of your Config Controller instance + cluster-name: cluster-name + deployment-repo: deployment-repo + # This should be the project where you deployed Config Controller + project-id: project-id # project + project-number: "1234567890123" + source-repo: source-repo +`, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: # kpt-merge: /setters-config + name: setters-config +data: + # This should be the name of your Config Controller instance + cluster-name: cluster-name + # This should be the project where you deployed Config Controller + project-id: project-id # project + project-number: "1234567890123" + # You can leave these defaults + namespace: config-control + deployment-repo: deployment-repo + source-repo: source-repo +`, + }, + { + name: "sort data fields configmap but retain order of extra fields", + from: `apiVersion: v1 +kind: ConfigMap +data: + baz: bar + cluster-name: cluster-name + foo: config-control +`, + to: `kind: ConfigMap +apiVersion: v1 +metadata: + name: foo +data: + color: orange + foo: config-control + abc: def + cluster-name: cluster-name +`, + expected: `apiVersion: v1 +kind: ConfigMap +data: + cluster-name: cluster-name + foo: config-control + color: orange + abc: def +metadata: + name: foo +`, + }, + { + name: "sort containers list node with sequence head comments preservation", + from: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: before +spec: + containers: + - name: nginx + # nginx image + image: "nginx:1.16.1" + ports: + - protocol: TCP # tcp protocol + containerPort: 80 +`, + to: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: after +spec: + containers: + # ports comment + - ports: + - containerPort: 80 + protocol: TCP # tcp protocol + # nginx image + image: "nginx:1.16.2" + # nginx container + name: nginx +# end of resource +`, + expected: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: after +spec: + containers: + # ports comment + # nginx container + - name: nginx + # nginx image + image: "nginx:1.16.2" + ports: + - protocol: TCP # tcp protocol + containerPort: 80 +# end of resource +`, + }, + { + name: "Do not alter sequence order", + from: `apiVersion: v1 +kind: KRMFile +metadata: + name: before +pipeline: + mutators: + - image: apply-setters:v0.1 + configPath: setters.yaml + - image: set-namespace:v0.1 + configPath: ns.yaml +`, + to: `apiVersion: v1 +kind: KRMFile +metadata: + name: after +pipeline: + mutators: + - configPath: sr.yaml + image: search-replace:v0.1 + - image: apply-setters:v0.1 + configPath: setters.yaml + - image: set-namespace:v0.1 + configPath: ns.yaml +`, + expected: `apiVersion: v1 +kind: KRMFile +metadata: + name: after +pipeline: + mutators: + - image: search-replace:v0.1 + configPath: sr.yaml + - image: apply-setters:v0.1 + configPath: setters.yaml + - image: set-namespace:v0.1 + configPath: ns.yaml +`, + }, + { + name: "Complex ASM reorder example", + from: `apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: (unknown) + creationTimestamp: null + name: controlplanerevisions.mesh.cloud.google.com +spec: + group: mesh.cloud.google.com + names: + kind: ControlPlaneRevision + listKind: ControlPlaneRevisionList + plural: controlplanerevisions + singular: controlplanerevision + scope: Namespaced + subresources: + status: {} + validation: + openAPIV3Schema: + description: ControlPlaneRevision is the Schema for the ControlPlaneRevision API + properties: + apiVersion: + description: 'APIVersion' + type: string + kind: + description: 'Kind' + type: string + metadata: + type: object + spec: + description: ControlPlaneRevisionSpec defines the desired state of ControlPlaneRevision + properties: + channel: + description: ReleaseChannel determines the aggressiveness of upgrades. + enum: + - regular + - rapid + - stable + type: string + type: + description: ControlPlaneRevisionType determines how the revision should be managed. + enum: + - managed_service + type: string + type: object + status: + description: ControlPlaneRevisionStatus defines the observed state of ControlPlaneRevision. + properties: + conditions: + items: + description: ControlPlaneRevisionCondition is a repeated struct definining the current conditions of a ControlPlaneRevision. + properties: + lastTransitionTime: + description: Last time the condition transitioned from one status to another + format: date-time + type: string + message: + description: Human-readable message indicating details about last transition + type: string + reason: + description: Unique, one-word, CamelCase reason for the condition's last transition + type: string + status: + description: Status is the status of the condition. Can be True, False, or Unknown. + type: string + type: + description: Type is the type of the condition. + type: string + type: object + type: array + type: object + type: object + version: v1alpha1 + versions: + - name: v1alpha1 + served: true + storage: true +status: + acceptedNames: + kind: "" + plural: "" + conditions: [] + storedVersions: [] +`, + to: `apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + name: controlplanerevisions.mesh.cloud.google.com + annotations: + controller-gen.kubebuilder.io/version: (unknown) + creationTimestamp: null +spec: + group: mesh.cloud.google.com + names: + kind: ControlPlaneRevision + listKind: ControlPlaneRevisionList + plural: controlplanerevisions + singular: controlplanerevision + scope: Namespaced + subresources: + status: {} + validation: + openAPIV3Schema: + type: object + description: ControlPlaneRevision is the Schema for the ControlPlaneRevision API + properties: + apiVersion: + type: string + description: 'APIVersion' + kind: + type: string + description: 'Kind' + metadata: + type: object + spec: + type: object + description: ControlPlaneRevisionSpec defines the desired state of ControlPlaneRevision + properties: + type: + type: string + description: ControlPlaneRevisionType determines how the revision should be managed. + enum: + - managed_service + channel: + type: string + description: ReleaseChannel determines the aggressiveness of upgrades. + enum: + - regular + - rapid + - stable + status: + type: object + description: ControlPlaneRevisionStatus defines the observed state of ControlPlaneRevision. + properties: + conditions: + type: array + items: + type: object + description: ControlPlaneRevisionCondition is a repeated struct definining the current conditions of a ControlPlaneRevision. + properties: + type: + type: string + description: Type is the type of the condition. + status: + type: string + description: Status is the status of the condition. Can be True, False, or Unknown. + lastTransitionTime: + type: string + description: Last time the condition transitioned from one status to another + format: date-time + message: + type: string + description: Human-readable message indicating details about last transition + reason: + type: string + description: Unique, one-word, CamelCase reason for the condition's last transition + version: v1alpha1 + versions: + - name: v1alpha1 + served: true + storage: true +status: + acceptedNames: + kind: "" + plural: "" + conditions: [] + storedVersions: [] +`, + expected: `test.from`, + }, + } + + for i := range testCases { + tc := testCases[i] + t.Run(tc.name, func(t *testing.T) { + from, err := yaml.Parse(tc.from) + if !assert.NoError(t, err) { + t.FailNow() + } + + to, err := yaml.Parse(tc.to) + if !assert.NoError(t, err) { + t.FailNow() + } + + err = SyncOrder(from, to) + if !assert.NoError(t, err) { + t.FailNow() + } + + out := &bytes.Buffer{} + kio.ByteWriter{ + Writer: out, + KeepReaderAnnotations: false, + }.Write([]*yaml.RNode{to}) + + // this means "to" is just a reordered version of "from" and after syncing order, + // resultant "to" must be equal to "from" + if tc.expected == "test.from" { + tc.expected = tc.from + } + + if !assert.Equal(t, tc.expected, out.String()) { + t.FailNow() + } + }) + } +} diff --git a/kyaml/sliceutil/slice.go b/kyaml/sliceutil/slice.go new file mode 100644 index 0000000000..23e3ad7c21 --- /dev/null +++ b/kyaml/sliceutil/slice.go @@ -0,0 +1,25 @@ +// Copyright 2021 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package sliceutil + +// Contains return true if string e is present in slice s +func Contains(s []string, e string) bool { + for _, a := range s { + if a == e { + return true + } + } + return false +} + +// Remove removes the first occurrence of r in slice s +// and returns remaining slice +func Remove(s []string, r string) []string { + for i, v := range s { + if v == r { + return append(s[:i], s[i+1:]...) + } + } + return s +} diff --git a/kyaml/sliceutil/sliceutil_test.go b/kyaml/sliceutil/sliceutil_test.go new file mode 100644 index 0000000000..6e38fa6e6b --- /dev/null +++ b/kyaml/sliceutil/sliceutil_test.go @@ -0,0 +1,25 @@ +// Copyright 2021 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package sliceutil + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestContains(t *testing.T) { + assert.True(t, Contains([]string{"foo", "bar"}, "bar")) + assert.False(t, Contains([]string{"foo", "bar"}, "baz")) + assert.False(t, Contains([]string{}, "bar")) + assert.False(t, Contains([]string{}, "")) +} + +func TestRemove(t *testing.T) { + assert.Equal(t, Remove([]string{"foo", "bar"}, "bar"), []string{"foo"}) + assert.Equal(t, Remove([]string{"foo", "bar", "foo"}, "foo"), []string{"bar", "foo"}) + assert.Equal(t, Remove([]string{"foo"}, "foo"), []string{}) + assert.Equal(t, Remove([]string{}, "foo"), []string{}) + assert.Equal(t, Remove([]string{"foo", "bar", "foo"}, "baz"), []string{"foo", "bar", "foo"}) +} diff --git a/kyaml/yaml/rnode.go b/kyaml/yaml/rnode.go index f7e3f230c1..9d9cbcaf94 100644 --- a/kyaml/yaml/rnode.go +++ b/kyaml/yaml/rnode.go @@ -14,6 +14,7 @@ import ( "sigs.k8s.io/kustomize/kyaml/errors" "sigs.k8s.io/kustomize/kyaml/internal/forked/github.com/go-yaml/yaml" + "sigs.k8s.io/kustomize/kyaml/sliceutil" "sigs.k8s.io/kustomize/kyaml/yaml/internal/k8sgen/pkg/labels" ) @@ -146,6 +147,50 @@ func NewMapRNode(values *map[string]string) *RNode { return m } +// SyncMapNodesOrder sorts the map node keys in 'to' node to match the order of +// map node keys in 'from' node, additional keys are moved to the end +func SyncMapNodesOrder(from, to *RNode) { + to.Copy() + res := &RNode{value: &yaml.Node{ + Kind: to.YNode().Kind, + Style: to.YNode().Style, + Tag: to.YNode().Tag, + Anchor: to.YNode().Anchor, + Alias: to.YNode().Alias, + HeadComment: to.YNode().HeadComment, + LineComment: to.YNode().LineComment, + FootComment: to.YNode().FootComment, + Line: to.YNode().Line, + Column: to.YNode().Column, + }} + + fromFieldNames, err := from.Fields() + if err != nil { + return + } + + toFieldNames, err := to.Fields() + if err != nil { + return + } + + for _, fieldName := range fromFieldNames { + if !sliceutil.Contains(toFieldNames, fieldName) { + continue + } + // append the common nodes in the order defined in 'from' node + res.value.Content = append(res.value.Content, to.Field(fieldName).Key.YNode(), to.Field(fieldName).Value.YNode()) + toFieldNames = sliceutil.Remove(toFieldNames, fieldName) + } + + for _, fieldName := range toFieldNames { + // append the residual nodes which are not present in 'from' node + res.value.Content = append(res.value.Content, to.Field(fieldName).Key.YNode(), to.Field(fieldName).Value.YNode()) + } + + to.SetYNode(res.YNode()) +} + // NewRNode returns a new RNode pointer containing the provided Node. func NewRNode(value *yaml.Node) *RNode { return &RNode{value: value}