Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Hi @guoshimin. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Signed the CLA

/ok-to-test

Would you mind explaining how you found that bug? This code is complicated to understand and I'd really feel more confident if we could have better tests. I have the feeling that this is a good opportunity.

I wrote a tool which used the strategicpatch package. I encountered the error unable to find api field in struct Container for the json field "$setElementOrder/env" for some input, but only some of the times. From there, by tracing successful runs and failed runs, I was able to find the problem. The nondeterminism was caused by the random map traversal order.

Although I found the bug via my tool, I believe the bug affects kubectl apply too. The condition to trigger the bug is when you are

• deleting one or more elements from an array,
• modifying one or more elements from the same array, and
• modifying one or more elements from another array

Here's a minimum example:

import "k8s.io/api/core/v1"

func TestMergeMap(t *testing.T) {
        for i := 0; i < 100; i++ {
                original := map[string]interface{}{
                        "env": []interface{}{
                                map[string]interface{}{
                                        "$patch": "delete",
                                        "name":   "AAA",
                                },
                        },
                        "$setElementOrder/env": []interface{}{
                                map[string]interface{}{
                                        "name": "BBB",
                                },
                        },
                }
                patch := map[string]interface{}{
                        "$setElementOrder/volumeMounts": []interface{}{},
                        "env": []interface{}{
                                map[string]interface{}{
                                        "name":  "BBB",
                                        "value": "BBB",
                                },
                        },
                        "$setElementOrder/env": []interface{}{
                                map[string]interface{}{
                                        "name": "BBB",
                                },
                        },
                }
                typ := reflect.TypeOf(v1.Container{})
                _, err := mergeMap(original, patch, typ, MergeOptions{})
                if err != nil {
                        t.Errorf(err.Error())
                        t.FailNow()
                }
        }
}

Awesome @guoshimin! @mengqiy can you take a look and see if this can be included in the unit-test suite?

LGTM
@guoshimin Thanks for fixing this bug. We need a test to cover this.
You can include the test in the existing test framework:https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go#L1114.

IMO we don't need a release note for this fix, do we? Can you update the release-note section in the PR body.

Updated the release note

Can you add the test case in #49834 (comment) to prevent regression in the future?

Added test. PTAL.

/lgtm
Please squash commits for merging.

/assign @pwittrock

/test pull-kubernetes-e2e-gce-etcd3

/lgtm

/retest

gentle ping @pwittrock

/approve

Thanks!

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guoshimin, mengqiy, pwittrock

Associated issue: 50040

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

/test all [submit-queue is verifying that this PR is safe to merge]

/retest

Automatic merge from submit-queue

Now that this is merged, how do we include it in the next patch release?

This will be included in v1.8.0.
It you want it in 1.7, you will need to backport it to branch release-1.7.

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.