Fix name in a configMapRef missing hash #5047

[jonathanlking]

Fixes #5047.

Until #3529, an empty prefix/suffix was considered to have the same SameEndingSubSlice (previously named SameEndingSubarray) as a one element prefix/suffix.

This allowed the example in #5047 (added as a test case in this PR), which has no longer worked since v3.9.3.

This PR adds back support for this behaviour, by making two passes in selectReferral:

1. If a prefix/suffix is empty, then it vacuously matches anything (i.e allowEmpty = true).
   Unlike before it also doesn't matter how many elements are in the non-empty prefix/suffix.

If there is still more than one candidate, then

2. Fall back to the current behaviour, requiring an exact match of the prefix and suffix (i.e. allowEmpty = false).

[k8s-ci-robot]

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.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jonathanlking / name: Jonathan King (e052771, 534ac51, 8e03a67, d146a81, 218e3ea, d6f988a, 12ad89b)

[k8s-ci-robot]

Welcome @jonathanlking!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @jonathanlking. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[jonathanlking]

I think this is ready to review.
The approach I’ve taken is described in #5047 (comment).
(The reason I left this as a draft until now was I was uncertain whether this was a correct approach to take, and wanted to leave the PR review process as more of a code review, but I’m not sure if this actually makes sense!)

[jonathanlking]

Just seen the failing Go / Lint action, I'll try and fix this now.

[ubyyj]

need this fix badly

[k8s-triage-robot]

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

This bot triages 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 PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

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

/lifecycle stale

[jonathanlking]

/remove-lifecycle stale

[erikgb]

/ok-to-test

[stormqueen1990]

Hi there, @jonathanlking! 👋🏻

First off, thank you very much for your contribution!

I'm having a little bit of trouble understanding the exact solution here, even after reading the referred issue 😅

Please feel free to correct me if I got this wrong, will try to summarize the issue in my own words to verify if we're in the same page: the problem in this use case is that the suffixes won't match because a nested kustomization.yaml provides a suffix for a subset of the resources being kustomized, and the outer set doesn't contain that same suffix. This discrepancy makes the comparison fail because it expects both prefixes and suffixes to be the same. On the other hand, if we simply replace that with an or, the list of candidates is ambiguous, which causes a different failure (Too many possible referral targets to referrer). Is this correct?

Your patch looks good, but I have a question arount the usage of the allowEmpty flag. It appears to me that flag is superfluous, since you're running the filter twice when the first filter doesn't disambiguate. Did you run into any specific issues if you simplified the condition to just allow empty on either side?

[stormqueen1990]

Do we really need this condition here?

From what I've seen in this PR, you'll run the function twice, one with the flag set to true, another one with the flag set to false. Is there a specific reason why applying the OR would be undesirable?

[jonathanlking]

Are you asking, could we just go with the following function?

func (r *Resource) PrefixesSuffixesEquals(o ResCtx) bool {                                                 
        eitherPrefixEmpty := len(r.GetNamePrefixes()) == 0 || len(o.GetNamePrefixes()) == 0                
        eitherSuffixEmpty := len(r.GetNameSuffixes()) == 0 || len(o.GetNameSuffixes()) == 0                
                                                                                                           
        return (eitherPrefixEmpty || utils.SameEndingSubSlice(r.GetNamePrefixes(), o.GetNamePrefixes())) &&
                (eitherSuffixEmpty || utils.SameEndingSubSlice(r.GetNameSuffixes(), o.GetNameSuffixes()))  
}                                                                                                          

This causes regressions with TestIssue3489Simplified, TestIssue3489 and TestNameReferenceDeploymentIssue3489, all with "found multiple possible referrals" errors.

I've tried to explain this problem/the motivation behind the allowEmpty option here #5047 (comment).

[jonathanlking]

Thanks for the review and apologies I didn't get around to writing a description beforehand! 🙏
My memory is slightly hazy, as it's been a while since I worked on this, but I think your description is accurate 🙂
Expanding on a couple of points:

if we simply replace that with an or

i.e. allow either (inclusive) a prefix or suffix to match

it expects both prefixes and suffixes to be the same

And if a prefix/suffix is missing it treats them as not the same (which wasn't the case before #3529, where an empty prefix/suffix was considered to have the same SameEndingSubarray as a one element prefix/suffix).

I also think #5047 (comment) and #5047 (comment) are the most useful/accurate comments I've left.

[stormqueen1990]

Hi there!! 👋🏻

Apologies for the confusion -- I took another look and understand the difference now. The second pass is only going to happen if/when the resulting list of candidates is bigger than one element, and that means the filter with "either side empty" wasn't strict enough to filter out candidates, whereas it's too strict to still apply the suffix on your use case. I totally missed that the first time around 🤦🏻‍♀️ my bad.

Appreciate the detailed description, and thanks for bearing with me on this! 🙏🏻

[stormqueen1990]

Just a small suggestion: I think it's worth adding a comment with a brief context of why the condition is needed, for future reference.

[jonathanlking]

The second pass is only going to happen if/when the resulting list of candidates is bigger than one element

Yep, exactly that!

I totally agree about adding a comment explaining this, I'll try and write something as soon as possible — I only held off before as I wasn't sure if this was even the correct/best approach.

Thanks again for the review! 🙏

[jonathanlking]

@stormqueen1990 I've added a comment and updated the PR description.
Let me know if you want any changes/think more the of PR description should live in the code as a comment 🙂

[stormqueen1990]

@jonathanlking this looks good, thanks for adding!

[jonathanlking]

I've just rebased off master and hopefully (actually this time!) fixed the linting failures 🤞

[jonathanlking]

Apologies for the noise, I'm pretty sure the lint step is now working (I've run the CI job on my own fork).

[stormqueen1990]

/lgtm
/label tide/merge-method-squash

/cc @koba1t
for maintainer review

[stormqueen1990]

Just a small suggestion: I think it's worth adding a comment with a brief context of why the condition is needed, for future reference.

[stormqueen1990]

/lgtm

[koba1t]

/assign

[koba1t]

Thanks @jonathanlking
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonathanlking, koba1t

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

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

[jonathanlking]

Thank you both @stormqueen1990 and @koba1t for the reviews and getting this merged! ☺️ 🙏