fixed disappearing ports issue

kubernetes-sigs · Mar 9, 2021 · 397744f · 397744f
1 parent 9e8e7a7
commit 397744f
Show file tree

Hide file tree

Showing 5 changed files with 270 additions and 15 deletions.
diff --git a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go
@@ -723,12 +723,12 @@ spec:
       - name: consul
         image: "dashicorp/consul:1.9.1"
         ports:
+        - containerPort: 8500
+          name: http
         - containerPort: 8301
           protocol: "TCP"
         - containerPort: 8301
           protocol: "UDP"
-        - containerPort: 8500
-          name: http
 `,
 		},
 	}

diff --git a/api/go.mod b/api/go.mod
@@ -16,3 +16,5 @@ require (
 	sigs.k8s.io/kustomize/kyaml v0.10.15
 	sigs.k8s.io/yaml v1.2.0
 )
+
+replace sigs.k8s.io/kustomize/kyaml => ../kyaml
diff --git a/api/krusty/multiplepatch_test.go b/api/krusty/multiplepatch_test.go
@@ -1191,26 +1191,26 @@ spec:
       - image: dashicorp/consul:1.9.1
         name: consul
         ports:
+        - containerPort: 8500
+          name: http
+        - containerPort: 8501
+          name: https
         - containerPort: 8301
           name: serflan-tcp
           protocol: TCP
         - containerPort: 8301
           name: serflan-udp
           protocol: UDP
+        - containerPort: 8302
+          name: serfwan
+        - containerPort: 8300
+          name: server
         - 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
 `)
 }
 
@@ -1262,3 +1262,113 @@ spec:
   - name: rabbitmq.rules
 `)
 }
+
+// test for #3620
+func TestPatchPortHasNoProtocol(t *testing.T) {
+	th := kusttest_test.MakeHarness(t)
+	th.WriteK(".", `
+resources:
+  - service.yaml
+patchesStrategicMerge:
+  - patch.yaml
+`)
+	th.WriteF("service.yaml", `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+spec:
+  ports:
+    - port: 30900
+      targetPort: 30900
+      protocol: TCP
+  type: NodePort
+`)
+	th.WriteF("patch.yaml", `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+  labels:
+    service: web
+spec:
+  ports:
+    - port: 30900
+      targetPort: 30900
+  selector:
+    service: web
+`)
+	m := th.Run(".", th.MakeDefaultOptions())
+	th.AssertActualEqualsExpected(m, `
+apiVersion: v1
+kind: Service
+metadata:
+  labels:
+    service: web
+  name: web
+spec:
+  ports:
+  - port: 30900
+    protocol: TCP
+    targetPort: 30900
+  selector:
+    service: web
+  type: NodePort
+`)
+}
+
+// test for #3620
+func TestPatchAddNewServicePort(t *testing.T) {
+	th := kusttest_test.MakeHarness(t)
+	th.WriteK(".", `
+resources:
+  - service.yaml
+patchesStrategicMerge:
+  - patch.yaml
+`)
+	th.WriteF("service.yaml", `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+spec:
+  ports:
+    - port: 30900
+      targetPort: 30900
+      protocol: TCP
+  type: NodePort
+`)
+	th.WriteF("patch.yaml", `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+  labels:
+    service: web
+spec:
+  ports:
+    - port: 30901
+      targetPort: 30901
+  selector:
+    service: web
+`)
+	m := th.Run(".", th.MakeDefaultOptions())
+	th.AssertActualEqualsExpected(m, `
+apiVersion: v1
+kind: Service
+metadata:
+  labels:
+    service: web
+  name: web
+spec:
+  ports:
+  - port: 30901
+    targetPort: 30901
+  - port: 30900
+    protocol: TCP
+    targetPort: 30900
+  selector:
+    service: web
+  type: NodePort
+`)
+}
diff --git a/kyaml/yaml/merge2/map_test.go b/kyaml/yaml/merge2/map_test.go
@@ -333,6 +333,90 @@ spec:
 `,
 		expected: `
 kind: Deployment
+`,
+		mergeOptions: yaml.MergeOptions{
+			ListIncreaseDirection: yaml.MergeOptionsListAppend,
+		},
+	},
+
+	//
+	// Test Case
+	//
+	{description: `port patch has no protocol`,
+		source: `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+spec:
+  ports:
+    - port: 30900
+      targetPort: 30900
+`,
+		dest: `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+spec:
+  ports:
+    - port: 30900
+      targetPort: 30900
+      protocol: TCP
+`,
+		expected: `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+spec:
+  ports:
+    - port: 30900
+      targetPort: 30900
+      protocol: TCP
+`,
+		mergeOptions: yaml.MergeOptions{
+			ListIncreaseDirection: yaml.MergeOptionsListAppend,
+		},
+	},
+
+	//
+	// Test Case
+	//
+	{description: `port patch has no protocol and different port number`,
+		source: `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+spec:
+  ports:
+    - port: 30901
+      targetPort: 30901
+`,
+		dest: `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+spec:
+  ports:
+    - port: 30900
+      targetPort: 30900
+      protocol: TCP
+`,
+		expected: `
+apiVersion: v1
+kind: Service
+metadata:
+  name: web
+spec:
+  ports:
+    - port: 30900
+      targetPort: 30900
+      protocol: TCP
+    - port: 30901
+      targetPort: 30901
 `,
 		mergeOptions: yaml.MergeOptions{
 			ListIncreaseDirection: yaml.MergeOptionsListAppend,

diff --git a/kyaml/yaml/walk/associative_sequence.go b/kyaml/yaml/walk/associative_sequence.go
@@ -53,14 +53,27 @@ func appendListNode(dst, src *yaml.RNode, keys []string) (*yaml.RNode, error) {
 			v = append(v, valueNode.YNode().Value)
 		}
 
+		// When there are multiple keys, ElementSetter appends the node to dst
+		// even if the output is already in dst. We remove the node from dst to
+		// prevent duplicates.
+		if len(keys) > 1 {
+			_, err = dst.Pipe(yaml.ElementSetter{
+				Keys:   keys,
+				Values: v,
+			})
+			if err != nil {
+				return nil, err
+			}
+		}
+
 		// 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,
 			Values:  v,
 		})
-
 		if err != nil {
 			return nil, err
 		}
@@ -97,6 +110,47 @@ func validateKeys(valuesList [][]string, values []string, keys []string) ([]stri
 	return validKeys, validValues
 }
 
+// mergeValues merges values together - e.g. if two containerPorts
+// have the same port and targetPort but one has an empty protocol
+// and the other doesn't, they are treated as the same containerPort
+func mergeValues(valuesList [][]string) [][]string {
+	for i, values1 := range valuesList {
+		for j, values2 := range valuesList {
+			if matched, values := match(values1, values2); matched {
+				valuesList[i] = values
+				valuesList[j] = values
+			}
+		}
+	}
+	return valuesList
+}
+
+// two values match if they have at least one common element and
+// corresponding elements only differ if one is an empty string
+func match(values1 []string, values2 []string) (bool, []string) {
+	if len(values1) != len(values2) {
+		return false, nil
+	}
+	var commonElement bool
+	var res []string
+	for i := range values1 {
+		if values1[i] == values2[i] {
+			commonElement = true
+			res = append(res, values1[i])
+			continue
+		}
+		if values1[i] != "" && values2[i] != "" {
+			return false, nil
+		}
+		if values1[i] != "" {
+			res = append(res, values1[i])
+		} else {
+			res = append(res, values2[i])
+		}
+	}
+	return commonElement, res
+}
+
 // setAssociativeSequenceElements recursively set the elements in the list
 func (l *Walker) setAssociativeSequenceElements(valuesList [][]string, keys []string, dest *yaml.RNode) (*yaml.RNode, error) {
 	// itemsToBeAdded contains the items that will be added to dest
@@ -105,6 +159,9 @@ func (l *Walker) setAssociativeSequenceElements(valuesList [][]string, keys []st
 	if l.Schema != nil {
 		schema = l.Schema.Elements()
 	}
+	if len(keys) > 1 {
+		valuesList = mergeValues(valuesList)
+	}
 
 	// each element in valuesList is a list of values corresponding to the keys
 	// for example, for the following yaml:
@@ -114,12 +171,14 @@ func (l *Walker) setAssociativeSequenceElements(valuesList [][]string, keys []st
 	//          protocol: TCP
 	// `keys` would be [containerPort, protocol]
 	// and `valuesList` would be [ [8080, UDP], [8080, TCP] ]
+	var validKeys []string
+	var validValues []string
 	for _, values := range valuesList {
 		if len(values) == 0 {
 			continue
 		}
 
-		validKeys, validValues := validateKeys(valuesList, values, keys)
+		validKeys, validValues = validateKeys(valuesList, values, keys)
 		val, err := Walker{
 			VisitKeysAsScalars:    l.VisitKeysAsScalars,
 			InferAssociativeLists: l.InferAssociativeLists,
@@ -144,7 +203,7 @@ func (l *Walker) setAssociativeSequenceElements(valuesList [][]string, keys []st
 					return nil, err
 				}
 				exit = true
-			} else if val.Field(key) == nil {
+			} else if val.Field(key) == nil && validValues[i] != "" {
 				// make sure the key is set on the field
 				_, err = val.Pipe(yaml.SetField(key, yaml.NewScalarRNode(validValues[i])))
 				if err != nil {
@@ -162,7 +221,7 @@ func (l *Walker) setAssociativeSequenceElements(valuesList [][]string, keys []st
 		_, err = itemsToBeAdded.Pipe(yaml.ElementSetter{
 			Element: val.YNode(),
 			Keys:    validKeys,
-			Values:  values,
+			Values:  validValues,
 		})
 		if err != nil {
 			return nil, err
@@ -171,7 +230,6 @@ func (l *Walker) setAssociativeSequenceElements(valuesList [][]string, keys []st
 
 	var err error
 	if len(valuesList) > 0 {
-		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
@@ -314,6 +372,7 @@ func (l Walker) elementPrimitiveValues() [][]string {
 
 // fieldValue returns a slice containing each source's value for fieldName
 func (l Walker) elementValueList(keys []string, values []string) []*yaml.RNode {
+	keys, values = validateKeys([][]string{values}, values, keys)
 	var fields []*yaml.RNode
 	for i := range l.Sources {
 		if l.Sources[i] == nil {