strategic patch doesn't keep order of duplicated elements in the list

[redbaron]

/kind bug

What happened:
given list with duplicated keys D1

mergingList:
 - name: D1
   value: URL1
 - name: A
   value: x
 - name: D1
   value: URL2

and a patch updating non-duplicated key A:

$setElementOrder/mergingList:
 - name: D1
 - name: A
 - name: D1
mergingList:
 - name: A
   value: z

it reorders keys:

mergingList: 
 - name: D1
   value: URL1
 - name: D1
   value: URL2
 - name: A
   value: z

What you expected to happen:

Order of duplicated keys stays the same

mergingList: 
 - name: D1
   value: URL1
 - name: A
   value: z
 - name: D1
   value: URL2

How to reproduce it (as minimally and precisely as possible):

I created test-case which fails on release-1.10 and master: redbaron@971c1b2

Anything else we need to know?:

I am trying to find out why kubectl creates incorrect patch on apply. Although I can't reproduce it with a simple test case like in this ticket, it look related to it. On a real resource kubectl apply it reorders keys in generated patch, so it generates something like:

$setElementOrder/mergingList:
 - name: D1
 - name: A
 - name: D1
mergingList:
 - name: D1
   value: URL1
 - name: D1
   value: URL2
 - name: A
   value: z

which makes it an invalid patch as order of keys differs from order of values. This reordering might or might not be result of the same underneath bug.

/sig api-machinery

[liggitt]

If name is the key, it is invalid to have the item twice in the list

[redbaron]

there is a testcase which explicitly allows it https://github.com/redbaron/kubernetes/blob/971c1b20e51dd2090a9fd585bafa18282c2984f5/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go#L4155-L4226

in our case duplicates where in a list of environment variables in container definition. I'll check tomorrow, but I think that pod was running with duplicates in env list just fine.

[redbaron]

confirmed, there are pods with duplicated env variable names, kubernetes is happy to run them, so I'd guess strategic patch should support this case

[liggitt]

there are pods with duplicated env variable names, kubernetes is happy to run them, so I'd guess strategic patch should support this case

They run, but are currently undefined with respect to ordering of the duplicate items on patch.

[redbaron]

hence this issue :) If k8s allows pod template to be accepted it should allow pod template to be updated on kubectl apply, for that strategic merge should support duplicated entries.

[liggitt]

If k8s allows pod template to be accepted it should allow pod template to be updated

The likely outcome is to tighten validation to not accept those templates. See #64907

[wenjiaswe]

/cc @jennybuckley

[redbaron]

Whats the conclusion? Is it NOTABUG?

[liggitt]

It is a bug, but the bug is that duplicate keys are allowed in the first place. Fixing that validation in the least disruptive way possible would likely be the best solution

[redbaron]

strategic patch and resource validation are on a different levels, each should do best to ensure correctness. Validating PODs for keys duplications is a good thing of course, but strategic patch should do it's part too: either detect unsupported case and return error or start supporting it, silently nulling key is the worst option IMHO

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[redbaron]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[redbaron]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

[redbaron]

/reopen

[k8s-ci-robot]

@strmdvr: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

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.

[redbaron]

/reopen

[k8s-ci-robot]

@redbaron: Reopened this issue.

In response to this:

/reopen

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.

[fedebongio]

/assign @jiahuif @apelisse @leilajal

/triage accepted

[apelisse]

We can keep this open, but I don't think it's anyone's priority to fix this. I'm not optimistic.

Also, if you want to keep it open, we should rename the title, maybe something like:

strategic patch doesn't fail on duplicated elements in the list

My understanding is that we can only have duplicates within the patch, which means that we only have to verify that there aren't duplicates when we generate the patch. I don't know how much we would see this as a backward incompatible change or as a bug fix too.

[redbaron]

strategic patch doesn't fail on duplicated elements in the list

If we make if fail on duplicates, wouldn't it stop apply to work for containers with duplicated elements? If so, when users try to remove duplicates they wont be able to do so.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

[redbaron]

/reopen

[k8s-ci-robot]

@redbaron: Reopened this issue.

In response to this:

/reopen

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.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

[deepflame]

/reopen

[k8s-ci-robot]

@deepflame: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

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.

[redbaron]

/reopen

Is it possible to freeze it? this reopen dance is pointless, bug is confirmed and just waits for resolution one way or another, closing issue doesn't make problem go away.

[k8s-ci-robot]

@redbaron: Reopened this issue.

In response to this:

/reopen

Is it possible to freeze it? this reopen dance is pointless, bug is confirmed and just waits for resolution one way or another, closing issue doesn't make problem go away.

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.

[regularvm]

what will be the final outcome here - it will be fixed or duplicated environment variables will not be accepted?

[apelisse]

I'd say it's fairly unlikely that we'll fix this in strategic-merge-patch, but at least we're addressing this in server-side apply right now.