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

@@ -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

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)
 		}

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 = ""
+				}
+			}
+		}
+	}
+}

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()
+			}
+		})
+	}
+}

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
+}

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"})
+}

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}