Merge pull request #3557 from natasha41575/PatchMergeIssue

fixed ports merging issue and refactored some kyaml/walk code

api/filters/patchstrategicmerge/patchstrategicmerge_test.go

@@ -723,12 +723,12 @@ spec:
       - name: consul
         image: "hashicorp/consul:1.9.1"
         ports:
+        - containerPort: 8301
+          protocol: "TCP"
+        - containerPort: 8301
+          protocol: "UDP"
         - containerPort: 8500
           name: http
-        - containerPort: 8301
-          protocol: "UDP"
-        - containerPort: 8301
-          protocol: "UDP"
 `,
 		},
 	}

api/krusty/multiplepatch_test.go

@@ -1124,3 +1124,92 @@ metadata:
 		}
 	}
 }
+
+// test for #3513
+func TestSmpWithDifferentKeysOnDifferentPorts(t *testing.T) {
+	th := kusttest_test.MakeHarness(t)
+	th.WriteK(".", `
+resources:
+  - resource.yaml
+patches:
+  - path: patch.yaml
+    target:
+      kind: StatefulSet
+      name: myapp
+`)
+	th.WriteF("resource.yaml", `
+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  name: myapp
+spec:
+  template:
+    spec:
+      containers:
+        - name: consul
+          image: "hashicorp/consul:1.9.1"
+          ports:
+            - containerPort: 8500
+              name: http
+            - containerPort: 8501
+              name: https
+            - containerPort: 8301
+              protocol: "TCP"
+              name: serflan-tcp
+            - containerPort: 8301
+              protocol: "UDP"
+              name: serflan-udp
+            - containerPort: 8302
+              name: serfwan
+            - containerPort: 8300
+              name: server
+            - containerPort: 8600
+              name: dns-tcp
+              protocol: "TCP"
+            - containerPort: 8600
+              name: dns-udp
+              protocol: "UDP"`)
+	th.WriteF("patch.yaml", `
+kind: StatefulSet
+metadata:
+  name: myapp
+  labels:
+    test: label
+`)
+	m := th.Run(".", th.MakeDefaultOptions())
+	th.AssertActualEqualsExpected(m, `
+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  labels:
+    test: label
+  name: myapp
+spec:
+  template:
+    spec:
+      containers:
+      - image: hashicorp/consul:1.9.1
+        name: consul
+        ports:
+        - containerPort: 8301
+          name: serflan-tcp
+          protocol: TCP
+        - containerPort: 8301
+          name: serflan-udp
+          protocol: UDP
+        - containerPort: 8600
+          name: dns-tcp
+          protocol: TCP
+        - containerPort: 8600
+          name: dns-udp
+          protocol: UDP
+        - containerPort: 8500
+          name: http
+        - containerPort: 8501
+          name: https
+        - containerPort: 8302
+          name: serfwan
+        - containerPort: 8300
+          name: server
+`)
+}

kyaml/yaml/fns.go

@@ -106,9 +106,7 @@ func (e ElementSetter) Filter(rn *RNode) (*RNode, error) {
 		var err error
 		found := true
 		for j := range e.Keys {
-			if j >= len(e.Values) {
+			if j < len(e.Values) {
-				val, err = newNode.Pipe(FieldMatcher{Name: e.Keys[j], StringValue: ""})
-			} else {
 				val, err = newNode.Pipe(FieldMatcher{Name: e.Keys[j], StringValue: e.Values[j]})
 			}
 			if err != nil {

kyaml/yaml/fns_test.go

@@ -127,15 +127,16 @@ func TestElementSetter(t *testing.T) {
 `, assertNoErrorString(t)(node.String()))
 
 	node = orig.Copy()
-	// Return error because ElementSetter doesn't support a single key
+	// When the element is not found, return an empty ist
-	// when there is a scalar value in the list
 	_, err = node.Pipe(ElementSetter{Keys: []string{"a"}})
-	assert.EqualError(t, err, "wrong Node Kind for  expected: MappingNode was ScalarNode: value: {scalarValue}")
+	assert.NoError(t, err)
+	assert.Equal(t, `[]
+`, assertNoErrorString(t)(node.String()))
 
-	// Return error because ElementSetter will assume all elements are scalar when
-	// there is only value provided.
 	_, err = node.Pipe(ElementSetter{Values: []string{"b"}})
-	assert.EqualError(t, err, "wrong Node Kind for  expected: ScalarNode was MappingNode: value: {a: b}")
+	assert.NoError(t, err)
+	assert.Equal(t, `[]
+`, assertNoErrorString(t)(node.String()))
 
 	node = MustParse(`
 - a: b

kyaml/yaml/walk/associative_sequence.go

@@ -16,7 +16,7 @@ import (
 // src and dst should be both sequence node. key is used to call ElementSetter.
 // ElementSetter will use key-value pair to find and set the element in sequence
 // node.
-func appendListNode(dst, src *yaml.RNode, keys []string, merge3 bool) (*yaml.RNode, error) {
+func appendListNode(dst, src *yaml.RNode, keys []string) (*yaml.RNode, error) {
 	var err error
 	for _, elem := range src.Content() {
 		// If key is empty, we know this is a scalar value and we can directly set the
@@ -33,10 +33,6 @@ func appendListNode(dst, src *yaml.RNode, keys []string, merge3 bool) (*yaml.RNo
 			continue
 		}
 
-		if len(keys) > 1 && !merge3 {
-			continue
-		}
-
 		// we need to get the value for key so that we can find the element to set
 		// in sequence.
 		v := []string{}
@@ -59,7 +55,6 @@ func appendListNode(dst, src *yaml.RNode, keys []string, merge3 bool) (*yaml.RNo
 
 		// We use the key and value from elem to find the corresponding element in dst.
 		// Then we will use ElementSetter to replace the element with elem. If we cannot
-		// find the item, the element will be appended.
 		_, err = dst.Pipe(yaml.ElementSetter{
 			Element: elem,
 			Keys:    keys,
@@ -73,96 +68,37 @@ func appendListNode(dst, src *yaml.RNode, keys []string, merge3 bool) (*yaml.RNo
 	return dst, nil
 }
 
-// setPrimitiveSequenceElements sets elements in a primitive list
-func (l *Walker) setPrimitiveSequenceElements(values []string, key string, dest *yaml.RNode) (*yaml.RNode, error) {
-	// itemsToBeAdded contains the items that will be added to dest
-	itemsToBeAdded := yaml.NewListRNode()
-	var schema *openapi.ResourceSchema
-	if l.Schema != nil {
-		schema = l.Schema.Elements()
-	}
-
-	for _, value := range values {
-		val, err := Walker{
-			VisitKeysAsScalars:    l.VisitKeysAsScalars,
-			InferAssociativeLists: l.InferAssociativeLists,
-			Visitor:               l,
-			Schema:                schema,
-			Sources:               l.elementValue(key, value),
-			MergeOptions:          l.MergeOptions,
-		}.Walk()
-		if err != nil {
-			return nil, err
-		}
-		// delete the node from **dest** if it's null or empty
-		if yaml.IsMissingOrNull(val) || yaml.IsEmptyMap(val) {
-			_, err = dest.Pipe(yaml.ElementSetter{Keys: []string{key}, Values: []string{value}})
-			if err != nil {
-				return nil, err
-			}
-			continue
-		}
-
-		if val.Field(key) == nil {
-			// make sure the key is set on the field
-			_, err = val.Pipe(yaml.SetField(key, yaml.NewScalarRNode(value)))
-			if err != nil {
-				return nil, err
-			}
-		}
-
-		// Add the val to the sequence. val will replace the item in the sequence if
-		// there is an item that matches the key-value pair. Otherwise val will be appended
-		// the the sequence.
-		_, err = itemsToBeAdded.Pipe(yaml.ElementSetter{
-			Element: val.YNode(),
-			Keys:    []string{key},
-			Values:  []string{value},
-		})
-		if err != nil {
-			return nil, err
-		}
-	}
-	var err error
-	if l.MergeOptions.ListIncreaseDirection == yaml.MergeOptionsListPrepend {
-		// items from patches are needed to be prepended. so we append the
-		// dest to itemsToBeAdded
-		dest, err = appendListNode(itemsToBeAdded, dest, []string{""}, len(l.Sources) > 2)
-	} else {
-		// append the items
-		dest, err = appendListNode(dest, itemsToBeAdded, []string{""}, len(l.Sources) > 2)
-	}
-	if err != nil {
-		return nil, err
-	}
-	// sequence is empty
-	if yaml.IsMissingOrNull(dest) {
-		return nil, nil
-	}
-
-	return dest, nil
-}
-
 // validateKeys returns a list of valid key-value pairs
 // if secondary merge key values are missing, use only the available merge keys
-func validateKeys(value []string, keys []string) ([]string, []string) {
+func validateKeys(valuesList [][]string, values []string, keys []string) ([]string, []string) {
 	validKeys := make([]string, 0)
 	validValues := make([]string, 0)
-	for i, v := range value {
+	validKeySet := sets.String{}
+	for _, values := range valuesList {
+		for i, v := range values {
 			if v != "" {
-			validKeys = append(validKeys, keys[i])
+				validKeySet.Insert(keys[i])
+			}
+		}
+	}
+	if validKeySet.Len() == 0 { // if values missing, fall back to primary keys
+		return keys, values
+	}
+	for _, k := range keys {
+		if validKeySet.Has(k) {
+			validKeys = append(validKeys, k)
+		}
+	}
+	for i, v := range values {
+		if v != "" || validKeySet.Has(keys[i]) {
 			validValues = append(validValues, v)
 		}
 	}
-	if len(validKeys) == 0 { // if values missing, fall back to primary keys
-		validKeys = keys
-		validValues = value
-	}
 	return validKeys, validValues
 }
 
 // setAssociativeSequenceElements recursively set the elements in the list
-func (l *Walker) setAssociativeSequenceElements(values [][]string, keys []string, dest *yaml.RNode) (*yaml.RNode, error) {
+func (l *Walker) setAssociativeSequenceElements(valuesList [][]string, keys []string, dest *yaml.RNode) (*yaml.RNode, error) {
 	// itemsToBeAdded contains the items that will be added to dest
 	itemsToBeAdded := yaml.NewListRNode()
 	var schema *openapi.ResourceSchema
@@ -170,12 +106,20 @@ func (l *Walker) setAssociativeSequenceElements(values [][]string, keys []string
 		schema = l.Schema.Elements()
 	}
 
-	for _, value := range values {
+	// each element in valuesList is a list of values corresponding to the keys
-		if len(value) == 0 {
+	// for example, for the following yaml:
+	//        - containerPort: 8080
+	//          protocol: UDP
+	//        - containerPort: 8080
+	//          protocol: TCP
+	// `keys` would be [containerPort, protocol]
+	// and `valuesList` would be [ [8080, UDP], [8080, TCP] ]
+	for _, values := range valuesList {
+		if len(values) == 0 {
 			continue
 		}
 
-		validKeys, validValues := validateKeys(value, keys)
+		validKeys, validValues := validateKeys(valuesList, values, keys)
 		val, err := Walker{
 			VisitKeysAsScalars:    l.VisitKeysAsScalars,
 			InferAssociativeLists: l.InferAssociativeLists,
@@ -218,7 +162,7 @@ func (l *Walker) setAssociativeSequenceElements(values [][]string, keys []string
 		_, err = itemsToBeAdded.Pipe(yaml.ElementSetter{
 			Element: val.YNode(),
 			Keys:    validKeys,
-			Values:  validValues,
+			Values:  values,
 		})
 		if err != nil {
 			return nil, err
@@ -226,17 +170,18 @@ func (l *Walker) setAssociativeSequenceElements(values [][]string, keys []string
 	}
 
 	var err error
-	for _, v := range values {
+	if len(valuesList) > 0 {
-		validKeys, _ := validateKeys(v, keys)
+		validKeys, _ := validateKeys(valuesList, valuesList[0], keys)
 		if l.MergeOptions.ListIncreaseDirection == yaml.MergeOptionsListPrepend {
 			// items from patches are needed to be prepended. so we append the
 			// dest to itemsToBeAdded
-			dest, err = appendListNode(itemsToBeAdded, dest, validKeys, len(l.Sources) > 2)
+			dest, err = appendListNode(itemsToBeAdded, dest, validKeys)
 		} else {
 			// append the items
-			dest, err = appendListNode(dest, itemsToBeAdded, validKeys, len(l.Sources) > 2)
+			dest, err = appendListNode(dest, itemsToBeAdded, validKeys)
 		}
 	}
+
 	if err != nil {
 		return nil, err
 	}
@@ -278,7 +223,7 @@ func (l *Walker) walkAssociativeSequence() (*yaml.RNode, error) {
 	}
 
 	// primitive associative list -- merge the values
-	return l.setPrimitiveSequenceElements(l.elementPrimitiveValues(), "", dest)
+	return l.setAssociativeSequenceElements(l.elementPrimitiveValues(), []string{""}, dest)
 }
 
 // elementKey returns the merge key to use for the associative list
@@ -338,9 +283,9 @@ func (l Walker) elementValues(keys []string) [][]string {
 }
 
 // elementPrimitiveValues returns the primitive values in an associative list -- eg. finalizers
-func (l Walker) elementPrimitiveValues() []string {
+func (l Walker) elementPrimitiveValues() [][]string {
 	// use slice to to keep elements in the original order
-	var returnValues []string
+	var returnValues [][]string
 	seen := sets.String{}
 	// if we are doing append, dest node should be the first.
 	// otherwise dest node should be the last.
@@ -360,26 +305,13 @@ func (l Walker) elementPrimitiveValues() []string {
 			if seen.Has(item.Value) {
 				continue
 			}
-			returnValues = append(returnValues, item.Value)
+			returnValues = append(returnValues, []string{item.Value})
 			seen.Insert(item.Value)
 		}
 	}
 	return returnValues
 }
 
-// fieldValue returns a slice containing each source's value for fieldName
-func (l Walker) elementValue(key, value string) []*yaml.RNode {
-	var fields []*yaml.RNode
-	for i := range l.Sources {
-		if l.Sources[i] == nil {
-			fields = append(fields, nil)
-			continue
-		}
-		fields = append(fields, l.Sources[i].Element(key, value))
-	}
-	return fields
-}
-
 // fieldValue returns a slice containing each source's value for fieldName
 func (l Walker) elementValueList(keys []string, values []string) []*yaml.RNode {
 	var fields []*yaml.RNode

plugin/builtin/patchtransformer/PatchTransformer_test.go

@@ -803,10 +803,10 @@ spec:
         name: test-deployment
         ports:
         - containerPort: 8080
-          name: take-over-the-world
+          name: disappearing-act
           protocol: TCP
         - containerPort: 8080
-          name: disappearing-act
+          name: take-over-the-world
           protocol: TCP
 `, `
 apiVersion: apps/v1