Wildcard support for creation in ReplacementTransformer #4886

KnVerey · 2022-11-24T00:46:51Z

⚠️ Behaviour change: in kustomize <v4.5.7, #4761 silently ignored targets with out of range indexes (wrongly, IMO!). Kustomize = v4.5.7 panics as described in that issue (also very bad!). My previous PR turned the panic into an error (fine). This one will turn the error into doing what they asked for: error if create is disabled, or creating the fields one by one if create is enabled. The latter would be an output change for a user coming from v4.5.6 or earlier, but I would argue this is fixing something that's clearly a bug.

Makes the Replacements transformer support the use of wildcards in conjunction with the Create option.
Also makes the Replacements transformer support appending list elements (both "primitive" and map-based elements).
Does both by implementing these features on PathMatcher, and switching to it. I tried several other things first, which you can still see in the earlier commits if interested.
- In working on this, I discovered an existing bug where the Create option cannot work with multiple matches, which is possible without a wildcard (e.g. if you match on container image, that may not be unique in a valid object). That bug is demonstrated in the regression test in this commit: 273fdf0. Currently, it silently uses the first one. The other approaches I tried would at best not be able to fix this, which is what led me to conclude modifying and using PathMatcher was necessary.
Initially, I also added support for - meaning "append" without knowing the index in advance. However, the example in replacements: Unable to add/create a new array element #4469 made me think the semantics deserve separate consideration (if you use hyphens in that example, you'll append two new things, which is not the intent). Plus PathGetter uses - to mean "last", with untested/unclear behaviour on create. We can still do something like this if there's demand for it; I just decided it shouldn't be part of this PR.

Fixes #4561
Fixes #4469
Fixes #1493

k8s-ci-robot · 2022-11-24T00:46:53Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2022-11-24T00:46:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [KnVerey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2022-11-24T21:06:31Z

@KnVerey: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

KnVerey · 2022-11-25T23:29:19Z

kyaml/yaml/match.go

-	val         *RNode
-	field       string
-	matchRegex  string
-	indexNumber int


This field is internal and was set in one place but unused.

This approach doesn't work when multiple existing sequence elements should match, i.e. because the sequence contains maps and we're searching on a key they all contain (target all containers with a certain image would be one use case for this). PathGetter just takes the first match in that case, which is not what we want.

…e query should match multiple elements

PathGetter treats it as meaning 'last' not 'append' and does not have test coverage for its handling of this when create is set. Semantics are dubious given that multiple Replacement fieldPaths may be specified, which would cause successive appends.

KnVerey · 2022-12-06T18:45:33Z

rebased and ready for review @natasha41575

natasha41575

The change to PathMatcher is very well done, thanks!

lgtm, couple of minor comments

natasha41575 · 2022-12-06T03:05:37Z

kyaml/yaml/match_test.go

+
+func TestPathMatcher_Filter_Create(t *testing.T) {
+	testCases := map[string]struct {
+		name                    string


nit: looks like this name field doesn't get used anywhere, do we need it?

natasha41575 · 2022-12-06T17:06:52Z

api/filters/replacement/replacement.go

-		}
+func fieldRetrievalError(fieldPath string, isCreate bool) string {
+	if isCreate {
+		return "unable to find or create field " + fieldPath + " in replacement target"


super nit: suggest quoting the fieldPath, since it's a user input value

KnVerey · 2022-12-06T19:51:11Z

Updated! I also realized the "find or create" error wasn't showing up in any tests, so I added another one.

The change to PathMatcher is very well done, thanks!

Thanks! It took me a while to get there, as the commit history still shows. 😅

/label tide/merge-method-squash

natasha41575

/lgtm

…igs#4886) * Ahead-of-time wildcard path expansion solution * Wrapped PathGetter solution This approach doesn't work when multiple existing sequence elements should match, i.e. because the sequence contains maps and we're searching on a key they all contain (target all containers with a certain image would be one use case for this). PathGetter just takes the first match in that case, which is not what we want. * Add creation support to PathMatcher * Regression test for existing bug when creation is enabled and sequence query should match multiple elements * PathMatcher Create tests and support for sequence appending * revert hyphen append support PathGetter treats it as meaning 'last' not 'append' and does not have test coverage for its handling of this when create is set. Semantics are dubious given that multiple Replacement fieldPaths may be specified, which would cause successive appends. * This also provides a solution to issue 1493 * Review feedback

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 24, 2022

k8s-ci-robot requested a review from annasong20 November 24, 2022 00:46

k8s-ci-robot requested a review from koba1t November 24, 2022 00:46

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 24, 2022

KnVerey force-pushed the replacements_wildcard branch from 042d65a to 4ce0e2b Compare November 24, 2022 21:06

KnVerey force-pushed the replacements_wildcard branch from 4ce0e2b to 273fdf0 Compare November 24, 2022 21:31

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 25, 2022

KnVerey commented Nov 25, 2022

View reviewed changes

KnVerey marked this pull request as ready for review November 25, 2022 23:35

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2022

k8s-ci-robot requested review from mortent and natasha41575 November 25, 2022 23:35

KnVerey added this to the v5.0.0 milestone Dec 1, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2022

KnVerey added 7 commits December 6, 2022 13:30

Ahead-of-time wildcard path expansion solution

eca2588

Add creation support to PathMatcher

bd95eb3

Regression test for existing bug when creation is enabled and sequenc…

a617210

…e query should match multiple elements

PathMatcher Create tests and support for sequence appending

263a965

revert hyphen append support

ce528f9

PathGetter treats it as meaning 'last' not 'append' and does not have test coverage for its handling of this when create is set. Semantics are dubious given that multiple Replacement fieldPaths may be specified, which would cause successive appends.

This also provides a solution to issue 1493

f9fbbc7

KnVerey force-pushed the replacements_wildcard branch from a5a5469 to f9fbbc7 Compare December 6, 2022 18:44

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2022

natasha41575 reviewed Dec 6, 2022

View reviewed changes

Review feedback

01e833e

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 6, 2022

natasha41575 reviewed Dec 6, 2022

View reviewed changes

k8s-ci-robot assigned natasha41575 Dec 6, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2022

k8s-ci-robot merged commit 903fbb6 into kubernetes-sigs:master Dec 6, 2022

KnVerey deleted the replacements_wildcard branch December 6, 2022 21:08

siegenthalerroger mentioned this pull request Jan 4, 2024

Support patches for containers #5354

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wildcard support for creation in ReplacementTransformer #4886

Wildcard support for creation in ReplacementTransformer #4886

KnVerey commented Nov 24, 2022 •

edited

k8s-ci-robot commented Nov 24, 2022

k8s-ci-robot commented Nov 24, 2022

k8s-ci-robot commented Nov 24, 2022

KnVerey Nov 25, 2022

KnVerey commented Dec 6, 2022

natasha41575 left a comment

natasha41575 Dec 6, 2022

natasha41575 Dec 6, 2022

KnVerey commented Dec 6, 2022

natasha41575 left a comment

Wildcard support for creation in ReplacementTransformer #4886

Wildcard support for creation in ReplacementTransformer #4886

Conversation

KnVerey commented Nov 24, 2022 • edited

k8s-ci-robot commented Nov 24, 2022

k8s-ci-robot commented Nov 24, 2022

k8s-ci-robot commented Nov 24, 2022

KnVerey Nov 25, 2022

Choose a reason for hiding this comment

KnVerey commented Dec 6, 2022

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 Dec 6, 2022

Choose a reason for hiding this comment

natasha41575 Dec 6, 2022

Choose a reason for hiding this comment

KnVerey commented Dec 6, 2022

natasha41575 left a comment

Choose a reason for hiding this comment

KnVerey commented Nov 24, 2022 •

edited