replacements: fix issue with create: true option when there is an existing field (#4667)

* replacements: demonstrate broken behavior when using 'create: true' with an already existing field

* replacements: fix issue with 'create: true' option when there is an existing field

* Suggestion for PR 4667

* code review

* lint error

Co-authored-by: Katrina Verey <katrina.verey@shopify.com>
This commit is contained in:
Natasha Sarkar
2022-07-15 14:31:49 -07:00
committed by GitHub
parent ab09d27ec7
commit 2f2ba40876
2 changed files with 175 additions and 113 deletions

View File

@@ -38,22 +38,86 @@ func (f Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
return nodes, nil
}
func applyReplacement(nodes []*yaml.RNode, value *yaml.RNode, targets []*types.TargetSelector) ([]*yaml.RNode, error) {
for _, t := range targets {
if t.Select == nil {
return nil, fmt.Errorf("target must specify resources to select")
func getReplacement(nodes []*yaml.RNode, r *types.Replacement) (*yaml.RNode, error) {
source, err := selectSourceNode(nodes, r.Source)
if err != nil {
return nil, err
}
if r.Source.FieldPath == "" {
r.Source.FieldPath = types.DefaultReplacementFieldPath
}
fieldPath := kyaml_utils.SmarterPathSplitter(r.Source.FieldPath, ".")
rn, err := source.Pipe(yaml.Lookup(fieldPath...))
if err != nil {
return nil, fmt.Errorf("error looking up replacement source: %w", err)
}
if rn.IsNilOrEmpty() {
return nil, fmt.Errorf("fieldPath `%s` is missing for replacement source %s", r.Source.FieldPath, r.Source.ResId)
}
return getRefinedValue(r.Source.Options, rn)
}
// selectSourceNode finds the node that matches the selector, returning
// an error if multiple or none are found
func selectSourceNode(nodes []*yaml.RNode, selector *types.SourceSelector) (*yaml.RNode, error) {
var matches []*yaml.RNode
for _, n := range nodes {
ids, err := utils.MakeResIds(n)
if err != nil {
return nil, fmt.Errorf("error getting node IDs: %w", err)
}
if len(t.FieldPaths) == 0 {
t.FieldPaths = []string{types.DefaultReplacementFieldPath}
for _, id := range ids {
if id.IsSelectedBy(selector.ResId) {
if len(matches) > 0 {
return nil, fmt.Errorf(
"multiple matches for selector %s", selector)
}
matches = append(matches, n)
break
}
}
for _, n := range nodes {
ids, err := utils.MakeResIds(n)
}
if len(matches) == 0 {
return nil, fmt.Errorf("nothing selected by %s", selector)
}
return matches[0], nil
}
func getRefinedValue(options *types.FieldOptions, rn *yaml.RNode) (*yaml.RNode, error) {
if options == nil || options.Delimiter == "" {
return rn, nil
}
if rn.YNode().Kind != yaml.ScalarNode {
return nil, fmt.Errorf("delimiter option can only be used with scalar nodes")
}
value := strings.Split(yaml.GetValue(rn), options.Delimiter)
if options.Index >= len(value) || options.Index < 0 {
return nil, fmt.Errorf("options.index %d is out of bounds for value %s", options.Index, yaml.GetValue(rn))
}
n := rn.Copy()
n.YNode().Value = value[options.Index]
return n, nil
}
func applyReplacement(nodes []*yaml.RNode, value *yaml.RNode, targetSelectors []*types.TargetSelector) ([]*yaml.RNode, error) {
for _, selector := range targetSelectors {
if selector.Select == nil {
return nil, errors.New("target must specify resources to select")
}
if len(selector.FieldPaths) == 0 {
selector.FieldPaths = []string{types.DefaultReplacementFieldPath}
}
for _, possibleTarget := range nodes {
ids, err := utils.MakeResIds(possibleTarget)
if err != nil {
return nil, err
}
// filter targets by label and annotation selectors
selectByAnnoAndLabel, err := selectByAnnoAndLabel(n, t)
selectByAnnoAndLabel, err := selectByAnnoAndLabel(possibleTarget, selector)
if err != nil {
return nil, err
}
@@ -63,8 +127,8 @@ func applyReplacement(nodes []*yaml.RNode, value *yaml.RNode, targets []*types.T
// filter targets by matching resource IDs
for i, id := range ids {
if id.IsSelectedBy(t.Select.ResId) && !rejectId(t.Reject, &ids[i]) {
err := applyToNode(n, value, t)
if id.IsSelectedBy(selector.Select.ResId) && !rejectId(selector.Reject, &ids[i]) {
err := copyValueToTarget(possibleTarget, value, selector)
if err != nil {
return nil, err
}
@@ -113,61 +177,50 @@ func rejectId(rejects []*types.Selector, id *resid.ResId) bool {
return false
}
func applyToNode(node *yaml.RNode, value *yaml.RNode, target *types.TargetSelector) error {
for _, fp := range target.FieldPaths {
func copyValueToTarget(target *yaml.RNode, value *yaml.RNode, selector *types.TargetSelector) error {
for _, fp := range selector.FieldPaths {
fieldPath := kyaml_utils.SmarterPathSplitter(fp, ".")
var t *yaml.RNode
var err error
if target.Options != nil && target.Options.Create {
// create option is not supported in a wildcard matching.
// Because, PathMatcher is not supported create option.
// So, if create option is set, we fallback to PathGetter.
for _, f := range fieldPath {
if f == "*" {
return errors.New("cannot support create option in a multi-value target") //nolint:goerr113
}
}
t, err = node.Pipe(yaml.LookupCreate(value.YNode().Kind, fieldPath...))
} else {
t, err = node.Pipe(&yaml.PathMatcher{Path: fieldPath})
}
create, err := shouldCreateField(selector.Options, fieldPath)
if err != nil {
return err
}
if t != nil {
if err = applyToOneNode(target.Options, t, value); err != nil {
var targetFields []*yaml.RNode
if create {
createdField, createErr := target.Pipe(yaml.LookupCreate(value.YNode().Kind, fieldPath...))
if createErr != nil {
return fmt.Errorf("error creating replacement node: %w", createErr)
}
targetFields = append(targetFields, createdField)
} else {
// may return multiple fields, always wrapped in a sequence node
foundFieldSequence, lookupErr := target.Pipe(&yaml.PathMatcher{Path: fieldPath})
if lookupErr != nil {
return fmt.Errorf("error finding field in replacement target: %w", lookupErr)
}
targetFields, err = foundFieldSequence.Elements()
if err != nil {
return fmt.Errorf("error fetching elements in replacement target: %w", err)
}
}
for _, t := range targetFields {
if err := setFieldValue(selector.Options, t, value); err != nil {
return err
}
}
}
return nil
}
func applyToOneNode(options *types.FieldOptions, t *yaml.RNode, value *yaml.RNode) error {
if len(t.YNode().Content) == 0 {
if err := setTargetValue(options, t, value); err != nil {
return err
}
return nil
}
for _, scalarNode := range t.YNode().Content {
rn := yaml.NewRNode(scalarNode)
if err := setTargetValue(options, rn, value); err != nil {
return err
}
}
return nil
}
func setTargetValue(options *types.FieldOptions, t *yaml.RNode, value *yaml.RNode) error {
func setFieldValue(options *types.FieldOptions, targetField *yaml.RNode, value *yaml.RNode) error {
value = value.Copy()
if options != nil && options.Delimiter != "" {
if t.YNode().Kind != yaml.ScalarNode {
if targetField.YNode().Kind != yaml.ScalarNode {
return fmt.Errorf("delimiter option can only be used with scalar nodes")
}
tv := strings.Split(t.YNode().Value, options.Delimiter)
tv := strings.Split(targetField.YNode().Value, options.Delimiter)
v := yaml.GetValue(value)
// TODO: Add a way to remove an element
switch {
@@ -181,76 +234,25 @@ func setTargetValue(options *types.FieldOptions, t *yaml.RNode, value *yaml.RNod
value.YNode().Value = strings.Join(tv, options.Delimiter)
}
if t.YNode().Kind == yaml.ScalarNode {
if targetField.YNode().Kind == yaml.ScalarNode {
// For scalar, only copy the value (leave any type intact to auto-convert int->string or string->int)
t.YNode().Value = value.YNode().Value
targetField.YNode().Value = value.YNode().Value
} else {
t.SetYNode(value.YNode())
targetField.SetYNode(value.YNode())
}
return nil
}
func getReplacement(nodes []*yaml.RNode, r *types.Replacement) (*yaml.RNode, error) {
source, err := selectSourceNode(nodes, r.Source)
if err != nil {
return nil, err
func shouldCreateField(options *types.FieldOptions, fieldPath []string) (bool, error) {
if options == nil || !options.Create {
return false, nil
}
if r.Source.FieldPath == "" {
r.Source.FieldPath = types.DefaultReplacementFieldPath
}
fieldPath := kyaml_utils.SmarterPathSplitter(r.Source.FieldPath, ".")
rn, err := source.Pipe(yaml.Lookup(fieldPath...))
if err != nil {
return nil, err
}
if rn.IsNilOrEmpty() {
return nil, fmt.Errorf("fieldPath `%s` is missing for replacement source %s", r.Source.FieldPath, r.Source.ResId)
}
return getRefinedValue(r.Source.Options, rn)
}
func getRefinedValue(options *types.FieldOptions, rn *yaml.RNode) (*yaml.RNode, error) {
if options == nil || options.Delimiter == "" {
return rn, nil
}
if rn.YNode().Kind != yaml.ScalarNode {
return nil, fmt.Errorf("delimiter option can only be used with scalar nodes")
}
value := strings.Split(yaml.GetValue(rn), options.Delimiter)
if options.Index >= len(value) || options.Index < 0 {
return nil, fmt.Errorf("options.index %d is out of bounds for value %s", options.Index, yaml.GetValue(rn))
}
n := rn.Copy()
n.YNode().Value = value[options.Index]
return n, nil
}
// selectSourceNode finds the node that matches the selector, returning
// an error if multiple or none are found
func selectSourceNode(nodes []*yaml.RNode, selector *types.SourceSelector) (*yaml.RNode, error) {
var matches []*yaml.RNode
for _, n := range nodes {
ids, err := utils.MakeResIds(n)
if err != nil {
return nil, err
}
for _, id := range ids {
if id.IsSelectedBy(selector.ResId) {
if len(matches) > 0 {
return nil, fmt.Errorf(
"multiple matches for selector %s", selector)
}
matches = append(matches, n)
break
}
// create option is not supported in a wildcard matching
for _, f := range fieldPath {
if f == "*" {
return false, fmt.Errorf("cannot support create option in a multi-value target")
}
}
if len(matches) == 0 {
return nil, fmt.Errorf("nothing selected by %s", selector)
}
return matches[0], nil
return true, nil
}

View File

@@ -2354,6 +2354,66 @@ spec:
name: myapp-container
ports:
- containerPort: 80
`,
},
"replace an existing mapping value": {
input: `apiVersion: batch/v1
kind: Job
metadata:
name: hello
spec:
template:
spec:
containers:
- image: busybox
name: myapp-container
restartPolicy: old
---
apiVersion: v1
kind: Pod
metadata:
name: my-pod
spec:
containers:
- image: busybox
name: myapp-container
restartPolicy: new
`,
replacements: `replacements:
- source:
kind: Pod
name: my-pod
fieldPath: spec
targets:
- select:
name: hello
kind: Job
fieldPaths:
- spec.template.spec
options:
create: true
`,
expected: `apiVersion: batch/v1
kind: Job
metadata:
name: hello
spec:
template:
spec:
containers:
- image: busybox
name: myapp-container
restartPolicy: new
---
apiVersion: v1
kind: Pod
metadata:
name: my-pod
spec:
containers:
- image: busybox
name: myapp-container
restartPolicy: new
`,
},
}