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.

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

api/resource: retain disableNameSuffixHash on merge and replace #4834

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 184 additions & 0 deletions api/krusty/generatormergeandreplace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,190 @@ metadata:
`)
}

func TestMergeAndReplaceDisableNameSuffixHashGenerators(t *testing.T) {
th := kusttest_test.MakeHarness(t)
th.WriteK("app", `
namePrefix: team-foo-
commonLabels:
app: mynginx
org: example.com
team: foo
commonAnnotations:
note: This is a test annotation
resources:
- deployment.yaml
configMapGenerator:
- name: configmap-in-base
literals:
- foo=bar
secretGenerator:
- name: secret-in-base
literals:
- username=admin
- password=somepw
`)
th.WriteF("app/deployment.yaml", `
apiVersion: apps/v1
kind: Deployment
metadata:
name: nginx
labels:
app: nginx
spec:
template:
metadata:
labels:
app: nginx
spec:
containers:
- name: nginx
image: nginx
volumeMounts:
- name: nginx-persistent-storage
mountPath: /tmp/ps
volumes:
- name: nginx-persistent-storage
emptyDir: {}
- configMap:
name: configmap-in-base
name: configmap-in-base
`)
th.WriteF("overlay/deployment.yaml", `
apiVersion: apps/v1
kind: Deployment
metadata:
name: nginx
spec:
template:
spec:
volumes:
- name: nginx-persistent-storage
emptyDir: null
gcePersistentDisk:
pdName: nginx-persistent-storage
- configMap:
name: configmap-in-overlay
name: configmap-in-overlay
`)
th.WriteK("overlay", `
namePrefix: staging-
commonLabels:
env: staging
team: override-foo
patchesStrategicMerge:
- deployment.yaml
resources:
- ../app
configMapGenerator:
- name: configmap-in-overlay
literals:
- hello=world
options:
disableNameSuffixHash: true
- name: configmap-in-base
behavior: replace
literals:
- foo=override-bar
options:
disableNameSuffixHash: true
KnVerey marked this conversation as resolved.
Show resolved Hide resolved
secretGenerator:
- name: secret-in-base
behavior: merge
literals:
- proxy=haproxy
options:
disableNameSuffixHash: true
`)
m := th.Run("overlay", th.MakeDefaultOptions())
th.AssertActualEqualsExpected(m, `
apiVersion: apps/v1
kind: Deployment
metadata:
annotations:
note: This is a test annotation
labels:
app: mynginx
env: staging
org: example.com
team: override-foo
name: staging-team-foo-nginx
spec:
selector:
matchLabels:
app: mynginx
env: staging
org: example.com
team: override-foo
template:
metadata:
annotations:
note: This is a test annotation
labels:
app: mynginx
env: staging
org: example.com
team: override-foo
spec:
containers:
- image: nginx
name: nginx
volumeMounts:
- mountPath: /tmp/ps
name: nginx-persistent-storage
volumes:
- gcePersistentDisk:
pdName: nginx-persistent-storage
name: nginx-persistent-storage
- configMap:
name: staging-configmap-in-overlay
name: configmap-in-overlay
- configMap:
name: staging-team-foo-configmap-in-base
name: configmap-in-base
---
apiVersion: v1
data:
foo: override-bar
kind: ConfigMap
metadata:
annotations:
note: This is a test annotation
labels:
app: mynginx
env: staging
org: example.com
team: override-foo
name: staging-team-foo-configmap-in-base
---
apiVersion: v1
data:
password: c29tZXB3
proxy: aGFwcm94eQ==
username: YWRtaW4=
kind: Secret
metadata:
annotations:
note: This is a test annotation
labels:
app: mynginx
env: staging
org: example.com
team: override-foo
name: staging-team-foo-secret-in-base
type: Opaque
---
apiVersion: v1
data:
hello: world
kind: ConfigMap
metadata:
labels:
env: staging
team: override-foo
name: staging-configmap-in-overlay
`)
}

func TestGeneratingIntoNamespaces(t *testing.T) {
th := kusttest_test.MakeHarness(t)
th.WriteK("app", `
Expand Down
44 changes: 44 additions & 0 deletions api/krusty/generatoroptions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,47 @@ metadata:
name: shouldHaveHash-c9867f8446
`)
}

func TestGeneratorOptionsOverlayDisableNameSuffixHash(t *testing.T) {
th := kusttest_test.MakeHarness(t)
th.WriteK("base", `
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
generatorOptions:
disableNameSuffixHash: false
labels:
foo: bar
configMapGenerator:
- name: baseShouldHaveHashButOverlayShouldNot
literals:
- foo=bar
`)
th.WriteK("overlay", `
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- ../base
generatorOptions:
disableNameSuffixHash: true
labels:
fruit: apple
configMapGenerator:
- name: baseShouldHaveHashButOverlayShouldNot
behavior: merge
literals:
- fruit=apple
`)
m := th.Run("overlay", th.MakeDefaultOptions())
th.AssertActualEqualsExpected(m, `
apiVersion: v1
data:
foo: bar
fruit: apple
kind: ConfigMap
metadata:
labels:
foo: bar
fruit: apple
name: baseShouldHaveHashButOverlayShouldNot
`)
}
11 changes: 9 additions & 2 deletions api/resource/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,17 @@ func (r *Resource) CopyMergeMetaDataFieldsFrom(other *Resource) error {
mergeStringMaps(other.GetLabels(), r.GetLabels())); err != nil {
return fmt.Errorf("copyMerge cannot set labels - %w", err)
}
if err := r.SetAnnotations(
mergeStringMapsWithBuildAnnotations(other.GetAnnotations(), r.GetAnnotations())); err != nil {

ra := r.GetAnnotations()
_, enableNameSuffixHash := ra[utils.BuildAnnotationsGenAddHashSuffix]
Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, I initially thought we should be checking the value of the annotation here, but upon further reflection I agree with your code: we shouldn't. If the annotation is present in the overlay, we should leave the value alone. If it isn't present, that means the user explicitly told us not to suffix, so we need to remove the defaulted annotation from the base. The reversed polarity of the internal tracking vs user specification gets confusing. 😓

merged := mergeStringMapsWithBuildAnnotations(other.GetAnnotations(), ra)
if !enableNameSuffixHash {
delete(merged, utils.BuildAnnotationsGenAddHashSuffix)
}
if err := r.SetAnnotations(merged); err != nil {
return fmt.Errorf("copyMerge cannot set annotations - %w", err)
}

if err := r.SetName(other.GetName()); err != nil {
return fmt.Errorf("copyMerge cannot set name - %w", err)
}
Expand Down