🐛 Fix RemoveOwnerRef unit test to use fresh ownerRefs for each test case

[dlipovetsky]

What this PR does / why we need it:
Fixes the RemoveOwnerRef unit test so that one test case does not mutate the ownerRefs test fixture used in another test case.

The RemoveOwnerRef function mutates its input. As far as I understand, it is designed to not mutate its input. This seems to be by design. I explain why I think this is poor design; namely, that it is easy to misuse, and allows subtle bugs like #7310.

The reason that it mutates its input is that it calls append, which re-uses the underlying array. This behavior came as a surprise to me, although it is noted in the "fine print":

If the capacity of s is not large enough to fit the additional values, append allocates a new, sufficiently large underlying array that fits both the existing slice elements and the additional values. Otherwise, append re-uses the underlying array. -- https://go.dev/ref/spec#Appending_and_copying_slices

NOTE: The unit tests will fail until a related bug fix, #7310 is merged.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[dlipovetsky]

I think this needs to be fixed in 1.2

/cherrypick release-1.2

[k8s-infra-cherrypick-robot]

@dlipovetsky: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

In response to this:

I think this needs to be fixed in 1.2

/cherrypick release-1.2

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.

[dlipovetsky]

If RemoveOwnerRef is designed to mutate its input, then the test needs to be fixed, because the first call to RemoveOwnerRef mutates the test fixture; the test fixture gets an extra copy of one element.

I think the issue is not at all clear, so here's a simplified example that demonstrates how append mutates its input: https://go.dev/play/p/CEzvqsTHquy

We need to make it clear to callers that they must not use the input slice after calling RemoveOwnerRef.

Personally, I find it very easy to misuse this function in its current implementation , and I would prefer that it does not mutate its input, at the expense of allocating memory for a copy.

The bug in #7310 was possible precisely because RemoveOwnerRef mutates its input.

[dlipovetsky]

I also recognize that the complementary functions from this package (EnsureOwnerRef and ReplaceOwnerRef) also mutate their input, and if we decide to change any of them, we should change all of them.

[killianmuldoon]

Agree these functions would be better designed if they did not mutate their input, but as they're public and exported I'm not sure if we can change the behaviour directly, we'd have to deprecate and replace (which should be fine.).

Why do you think the functions are designed to work like this?

[dlipovetsky]

Agree these functions would be better designed if they did not mutate their input, but as they're public and exported I'm not sure if we can change the behaviour directly, we'd have to deprecate and replace (which should be fine.).

You're right.

Why do you think the functions are designed to work like this?

That's a good question. I suspect it's because the functions use append, which sometimes allocates a new array, and other times modifies the underlying array.

I see two alternative designs: (a) make a copy of the source array, or (b) require the caller to pass the source array by reference. Here's a go playground example of both.

[dlipovetsky]

Since the unit test has the same bug as the one in #7310, I will update this PR to fix the unit test only.

I will open a separate PR with an alternate implementation of these functions.

[killianmuldoon]

I will open a separate PR with an alternate implementation of these functions.

It might be best to wait to see if we can get consensus on:

1. Whether this behaviour should be changed
2. Whether or not this is a bug which should be fixed in the existing functions
3. What the correct implementation of these functions should be

IMO the answers are yes, yes, and (a) make a copy of the source array, but let's see if others want to chime in.

[dlipovetsky]

I've updated this PR to just fix the unit test.

It might be best to wait to see if we can get consensus

Agreed. I'll wait for others to chime in here, but I can also start GitHub Discussion.

[vincepri]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 [vincepri]

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

[k8s-infra-cherrypick-robot]

@dlipovetsky: new pull request created: #7313

In response to this:

I think this needs to be fixed in 1.2

/cherrypick release-1.2

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.

[killianmuldoon]

@dlipovetsky Seeing as this is merged do you mind opening an issue to capture the broader problem with this set of funcitons?