Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix name in a configMapRef missing hash #5047 #5236

Merged
merged 7 commits into from Apr 2, 2024
Merged

Fix name in a configMapRef missing hash #5047 #5236

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
12ad89b
Add regression tests
jonathanlking Jul 7, 2023
534ac51
Update PrefixesSuffixesEquals function
jonathanlking Jul 7, 2023
e052771
Try empty prefix/suffix but fall back on duplicates
jonathanlking Jul 8, 2023
8e03a67
Run gofmt
jonathanlking Mar 6, 2024
d6f988a
Remove newline
jonathanlking Mar 7, 2024
d146a81
Revert unnecessary gofmt change
jonathanlking Mar 7, 2024
218e3ea
Add comment
jonathanlking Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions api/filters/nameref/nameref.go
View file
Open in desktop
jonathanlking marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -284,9 +284,9 @@ func (f Filter) roleRefFilter() sieveFunc {
return previousIdSelectedByGvk(roleRefGvk)
}

func prefixSuffixEquals(other resource.ResCtx) sieveFunc {
func prefixSuffixEquals(other resource.ResCtx, allowEmpty bool) sieveFunc {
return func(r *resource.Resource) bool {
return r.PrefixesSuffixesEquals(other)
return r.PrefixesSuffixesEquals(other, allowEmpty)
}
}

Expand Down Expand Up @@ -325,7 +325,10 @@ func (f Filter) selectReferral(
if len(candidates) == 1 {
return candidates[0], nil
}
candidates = doSieve(candidates, prefixSuffixEquals(f.Referrer))
candidates = doSieve(candidates, prefixSuffixEquals(f.Referrer, true))
if len(candidates) > 1 {
candidates = doSieve(candidates, prefixSuffixEquals(f.Referrer, false))
}
if len(candidates) == 1 {
return candidates[0], nil
}
Expand Down
181 changes: 181 additions & 0 deletions api/krusty/configmaps_test.go
View file
Open in desktop
Expand Up @@ -591,3 +591,184 @@ metadata:
name: test-m8t7bmb6g2
`)
}

// Regression test for https://github.com/kubernetes-sigs/kustomize/issues/5047
func TestPrefixSuffix(t *testing.T) {
th := kusttest_test.MakeHarness(t)
th.WriteF("kustomization.yaml", `
resources:
- a
- b
`)

th.WriteF("a/kustomization.yaml", `
resources:
- ../common

namePrefix: a
`)

th.WriteF("b/kustomization.yaml", `
resources:
- ../common

namePrefix: b
`)

th.WriteF("common/kustomization.yaml", `
resources:
- service

configMapGenerator:
- name: "-example-configmap"
`)

th.WriteF("common/service/deployment.yaml", `
kind: Deployment
apiVersion: apps/v1

metadata:
name: "-"

spec:
template:
spec:
containers:
- name: app
envFrom:
- configMapRef:
name: "-example-configmap"
`)

th.WriteF("common/service/kustomization.yaml", `
resources:
- deployment.yaml

nameSuffix: api
`)

m := th.Run(".", th.MakeDefaultOptions())
th.AssertActualEqualsExpected(m, `
apiVersion: apps/v1
kind: Deployment
metadata:
name: a-api
spec:
template:
spec:
containers:
- envFrom:
- configMapRef:
name: a-example-configmap-6ct58987ht
name: app
---
apiVersion: v1
kind: ConfigMap
metadata:
name: a-example-configmap-6ct58987ht
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: b-api
spec:
template:
spec:
containers:
- envFrom:
- configMapRef:
name: b-example-configmap-6ct58987ht
name: app
---
apiVersion: v1
kind: ConfigMap
metadata:
name: b-example-configmap-6ct58987ht
`)
}

// Regression test for https://github.com/kubernetes-sigs/kustomize/issues/5047
func TestPrefixSuffix2(t *testing.T) {
th := kusttest_test.MakeHarness(t)
th.WriteF("kustomization.yaml", `
resources:
- a
- b
`)

th.WriteF("a/kustomization.yaml", `
resources:
- ../common

namePrefix: a
`)

th.WriteF("b/kustomization.yaml", `
resources:
- ../common

namePrefix: b
`)

th.WriteF("common/deployment.yaml", `
apiVersion: apps/v1
kind: Deployment
metadata:
name: "-example"
spec:
template:
spec:
containers:
- name: app
envFrom:
- configMapRef:
name: "-example-configmap"
`)

th.WriteF("common/kustomization.yaml", `
resources:
- deployment.yaml

configMapGenerator:
- name: "-example-configmap"
`)

m := th.Run(".", th.MakeDefaultOptions())
th.AssertActualEqualsExpected(m, `
apiVersion: apps/v1
kind: Deployment
metadata:
name: a-example
spec:
template:
spec:
containers:
- envFrom:
- configMapRef:
name: a-example-configmap-6ct58987ht
name: app
---
apiVersion: v1
kind: ConfigMap
metadata:
name: a-example-configmap-6ct58987ht
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: b-example
spec:
template:
spec:
containers:
- envFrom:
- configMapRef:
name: b-example-configmap-6ct58987ht
name: app
---
apiVersion: v1
kind: ConfigMap
metadata:
name: b-example-configmap-6ct58987ht
`)
}
14 changes: 11 additions & 3 deletions api/resource/resource.go
View file
Open in desktop
Expand Up @@ -290,9 +290,17 @@ func (r *Resource) getCsvAnnotation(name string) []string {
// PrefixesSuffixesEquals is conceptually doing the same task
// as OutermostPrefixSuffix but performs a deeper comparison
// of the suffix and prefix slices.
func (r *Resource) PrefixesSuffixesEquals(o ResCtx) bool {
return utils.SameEndingSubSlice(r.GetNamePrefixes(), o.GetNamePrefixes()) &&
utils.SameEndingSubSlice(r.GetNameSuffixes(), o.GetNameSuffixes())
func (r *Resource) PrefixesSuffixesEquals(o ResCtx, allowEmpty bool) bool {
if allowEmpty {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🙏🏻

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonathanlking this looks good, thanks for adding!

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()))
} else {
return utils.SameEndingSubSlice(r.GetNamePrefixes(), o.GetNamePrefixes()) &&
utils.SameEndingSubSlice(r.GetNameSuffixes(), o.GetNameSuffixes())
}
}

// RemoveBuildAnnotations removes annotations created by the build process.
Expand Down