Strategic merge patches change the order of containers

[shapirus]

What happened?

If only one container out of multiple is referenced in the patch, then the order of the containers in the output is changed.

Refer to #3912, it has all the info.

That issue was auto-closed for inactivity, but the bug still exists.

What did you expect to happen?

The original order of containers has to be preserved.

How can we reproduce it (as minimally and precisely as possible)?

See the original ticket.

Expected output

See the original ticket.

Actual output

See the original ticket.

Kustomize version

5.2.1

Operating system

None

[k8s-ci-robot]

This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

[tsekine354]

Not just container, but the order of env variables is also important.

If you use $(VAR_NAME) reference, VAR_NAME must be declared before its reference, as explained in https://kubernetes.io/docs/tasks/inject-data-application/define-interdependent-environment-variables/#define-an-environment-dependent-variable-for-a-container

But strategic merge patch changes the order of env variable, then it could brake dependent variable reference.

[shapirus]

Correct. It is apparently the case for list elements in general.

[tsekine354]

As per source code reading + some help by debugger, I guess elementValues() in https://github.com/kubernetes-sigs/kustomize/blob/master/kyaml/yaml/walk/associative_sequence.go#L312 is the cause.

In strategic patch merge mode, new items might be supposed to get inserted first, as per https://github.com/kubernetes-sigs/kustomize/blob/master/api/filters/patchstrategicmerge/patchstrategicmerge.go#L25

				ListIncreaseDirection: yaml.MergeOptionsListPrepend,

Then, elementValues() unconditionally read second source first

	beginIdx := 0
	if l.MergeOptions.ListIncreaseDirection == yaml.MergeOptionsListPrepend {
		beginIdx = 1
	}
	for i := range l.Sources {
		src := l.Sources[(i+beginIdx)%len(l.Sources)]
...

It should read Sources sequentially always. Then, if there are new items in Sources[1], insert those items into top of returnValues.

[tsekine354]

tsekine354@1ad9646
may be able to the fix this issue. But unfortunately, there are many unrelated test cases which depend on current behavior. I need feedback from project team before fixing those test cases.