From 397744f43613bd0bef8e08b54e04650d015959a5 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Mon, 8 Mar 2021 11:16:11 -0800 Subject: [PATCH] fixed disappearing ports issue --- .../patchstrategicmerge_test.go | 4 +- api/go.mod | 2 + api/krusty/multiplepatch_test.go | 126 ++++++++++++++++-- kyaml/yaml/merge2/map_test.go | 84 ++++++++++++ kyaml/yaml/walk/associative_sequence.go | 69 +++++++++- 5 files changed, 270 insertions(+), 15 deletions(-) diff --git a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go index 5c27c540b..1f96c8658 100644 --- 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 index ec5f75721..c2ec89663 100644 --- 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 index 723ff16b2..0f0f768eb 100644 --- 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 index 1631b0d40..783e81b73 100644 --- 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 index 3b6222a9a..7657ebc0f 100644 --- 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 {