diff --git a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go index 82d427e98..1267b90fe 100644 --- a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go +++ b/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" `, }, } diff --git a/api/go.mod b/api/go.mod index b412ebf02..8333c6ae3 100644 --- a/api/go.mod +++ b/api/go.mod @@ -24,3 +24,5 @@ require ( sigs.k8s.io/kustomize/kyaml v0.10.7 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 dbba59ad8..0ba88075b 100644 --- a/api/krusty/multiplepatch_test.go +++ b/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 +`) +} diff --git a/kyaml/yaml/fns.go b/kyaml/yaml/fns.go index 2271cc0b4..ab01b9815 100644 --- a/kyaml/yaml/fns.go +++ b/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) { - val, err = newNode.Pipe(FieldMatcher{Name: e.Keys[j], StringValue: ""}) - } else { + if j < len(e.Values) { val, err = newNode.Pipe(FieldMatcher{Name: e.Keys[j], StringValue: e.Values[j]}) } if err != nil { diff --git a/kyaml/yaml/fns_test.go b/kyaml/yaml/fns_test.go index e3ee2aaaf..a4e5db98f 100644 --- a/kyaml/yaml/fns_test.go +++ b/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 there is a scalar value in the list + // When the element is not found, return an empty ist _, 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 diff --git a/kyaml/yaml/walk/associative_sequence.go b/kyaml/yaml/walk/associative_sequence.go index c35e6413c..3b6222a9a 100644 --- a/kyaml/yaml/walk/associative_sequence.go +++ 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 { - if v != "" { - validKeys = append(validKeys, keys[i]) - validValues = append(validValues, v) + validKeySet := sets.String{} + for _, values := range valuesList { + for i, v := range values { + if v != "" { + validKeySet.Insert(keys[i]) + } } } - if len(validKeys) == 0 { // if values missing, fall back to primary keys - validKeys = keys - validValues = value + 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) + } } 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 { - if len(value) == 0 { + // each element in valuesList is a list of values corresponding to the keys + // 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 { - validKeys, _ := validateKeys(v, keys) + 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 - 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 diff --git a/plugin/builtin/patchtransformer/PatchTransformer_test.go b/plugin/builtin/patchtransformer/PatchTransformer_test.go index cadd90e7d..846e3e332 100644 --- a/plugin/builtin/patchtransformer/PatchTransformer_test.go +++ b/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