Name in a configMapRef is not updated to include the hash as a suffix (nor a prefix) #5047

jonathanlking · 2023-02-15T17:08:59Z

What happened?

While upgrading from an older version kustomize I discovered a breaking change in output between v3.9.2 and v3.9.3, where a configMapRef is no longer valid.

While this behaviour appeared in v3.9.3, it can be reverted by applying the --enable_kyaml=false flag and from looking at the release notes for v3.9.3 I believe the difference is actually due to the switch to the kyaml library. (I was wrong about this!)

I think it’s different to other GitHub issues I’ve found (e.g. #1301 or #2609) as I can still replicate this on v5.0.0.

What did you expect to happen?

A hash and suffix to still be included in the configMapRef, i.e. a-example-configmap-6ct58987ht rather than -example-configmap.

Interestingly building a or b independently (or removing either - a or - b from the kustomization.yaml) still gives the expected behaviour. It's only when there are multiple resources that this unexpected behaviour occurs.

Perhaps I was previously relying on undefined/buggy behaviour?

How can we reproduce it (as minimally and precisely as possible)?

# kustomization.yaml
resources:
- a
- b

# a/kustomization.yaml
resources:
- ../common

namePrefix: a

# b/kustomization.yaml
resources:
- ../common

namePrefix: b

# common/kustomization.yaml
resources:
- service

configMapGenerator:
- name: "-example-configmap"

# common/service/deployment.yaml
kind: Deployment
apiVersion: apps/v1

metadata:
  name: "-"

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

# common/service/kustomization.yaml
resources:
- deployment.yaml

nameSuffix: api

I also have a copy of these files here https://github.com/jonathanlking/kustomize-example.

Expected output

apiVersion: v1
kind: ConfigMap
metadata:
  name: a-example-configmap-6ct58987ht
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: b-example-configmap-6ct58987ht
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: a-api
spec:
  template:
    spec:
      containers:
      - envFrom:
        - configMapRef:
            name: a-example-configmap-6ct58987ht
        name: app
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: b-api
spec:
  template:
    spec:
      containers:
      - envFrom:
        - configMapRef:
            name: b-example-configmap-6ct58987ht
        name: app

Actual output

apiVersion: v1
kind: ConfigMap
metadata:
  name: a-example-configmap-6ct58987ht
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: b-example-configmap-6ct58987ht
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: a-api
spec:
  template:
    spec:
      containers:
      - envFrom:
        - configMapRef:
            name: -example-configmap
        name: app
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: b-api
spec:
  template:
    spec:
      containers:
      - envFrom:
        - configMapRef:
            name: -example-configmap
        name: app

Kustomize version

v3.9.3 onwards (including v5.0.0)

Operating system

Linux

The text was updated successfully, but these errors were encountered:

Serializator · 2023-03-15T08:02:40Z

/assign

Serializator · 2023-03-15T13:53:50Z

A note for self and also anyone else who tries to reproduce. The patch included does not affect the outcome and can thus be removed and "-example-configmap" directly added to the deployment manifest.

It is important to use the most minimalistic manifest as this simplifies the code path to debug as well 😉

Please let me know if I'm wrong @jonathanlking and the patch does actually matter for the issue!

jonathanlking · 2023-03-15T14:21:15Z

@Serializator Ah, yes I did misunderstand! 🤦‍♂️

You're saying that the patch could be replace and instead the deployment could be changed to:

kind: Deployment
apiVersion: apps/v1

metadata:
  name: "-"

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

I agree, you're right, the patch included does not affect the outcome.
Apologies for this, I'm not sure how I missed it, I will update the example.

Serializator · 2023-03-15T14:24:40Z

Though this is not yet conclusive, I was able to pinpoint the PR in which the regression was introduced.
Pull Request: #3529

Gotta ❤️ Git with its bisect command!

I will go through the changes in this PR and try to identify which specific change is causing this behaviour.

@jonathanlking, thanks for the specific versions in the description, this helped a lot!

natasha41575 · 2023-03-15T16:19:35Z

/triage accepted

I can reproduce the regression between v3.9.2 and v3.9.3, though enabling/disabling the --enable_kyaml flag doesn't appear to make a difference for me when using v3.9.3.

jonathanlking · 2023-03-15T16:39:40Z

@natasha41575 Apologies (for the second time today!), you're right about the --enable_kyaml flag, I've just tried again and it doesn't appear to make a difference for me.

I'm not sure why I originally thought it did 🤔

Serializator · 2023-03-15T18:32:36Z

Appendix 1. below is taken from (resource.Resource).PrefixesSuffixesEquals, this function checks if the prefixes and suffixes of the resource itself and provided resource.ResCtx are equal.

In the original code (before the change from #3529) utils.SameEndingSubSlice would return true in case if one of the two slices were empty. Take a look at the first comparison in the appendix.

Right now this results in false, though with the previous logic (see appendix 2.) it would result in true as at least the prefixes are the same, basically an OR logic.

What my suggestion would be to bring this OR logic into (resource.Resource).PrefixesSuffixesEquals as currently it does an AND check on the prefixes and suffixes. Changing this to an OR would retain the logic before the PR whilst retaining the original "logic" for this part of the code.

@natasha41575, what do you think of this suggestion?

Appendix 1.

Resource Name Prefixes: [a]
Object Name Prefixes: [a]

Resource Name Suffixes: []
Object Name Suffixes: [api]

Result: false
--------
Resource Name Prefixes: [b]
Object Name Prefixes: [a]

Resource Name Suffixes: []
Object Name Suffixes: [api]

Result: false
--------
Resource Name Prefixes: [a]
Object Name Prefixes: [b]

Resource Name Suffixes: []
Object Name Suffixes: [api]

Result: false
--------
Resource Name Prefixes: [b]
Object Name Prefixes: [b]

Resource Name Suffixes: []
Object Name Suffixes: [api]

Result: false

Appendix 2.

func SameEndingSubarray(shortest, longest []string) bool {
	if len(shortest) > len(longest) {
		longest, shortest = shortest, longest
	}
	diff := len(longest) - len(shortest)
	if len(shortest) == 0 {
		return diff == 0
	}
	for i := len(shortest) - 1; i >= 0; i-- {
		if longest[i+diff] != shortest[i] {
			return false
		}
	}
	return true
}

jsitu777 · 2023-03-23T21:47:58Z

I'm facing the same issue even in kustomize v5.0.1

jonathanlking · 2023-04-27T22:43:41Z

@Serializator thanks again for your investigation! I've had a go at trying what I believe you suggested:

diff --git i/api/resource/resource.go w/api/resource/resource.go
index 53a62dffd..3585593d3 100644
--- i/api/resource/resource.go
+++ w/api/resource/resource.go
@@ -321,7 +321,7 @@ func (r *Resource) getCsvAnnotation(name string) []string {
 // as OutermostPrefixSuffix but performs a deeper comparison
 // of the suffix and prefix slices.
 func (r *Resource) PrefixesSuffixesEquals(o ResCtx) bool {
-	return SameEndingSubarray(r.GetNamePrefixes(), o.GetNamePrefixes()) && SameEndingSubarray(r.GetNameSuffixes(), o.GetNameSuffixes())
+	return SameEndingSubarray(r.GetNamePrefixes(), o.GetNamePrefixes()) || SameEndingSubarray(r.GetNameSuffixes(), o.GetNameSuffixes())
 }
 
 // RemoveBuildAnnotations removes annotations created by the build process.

This does seem to fix the issue reported! 🎉
However I think it introduces a new issue; a minimal example of this is:

# kustomization.yaml
resources:
- a
- b

# a/kustomization.yaml
resources:
- ../common

namePrefix: a

# b/kustomization.yaml
resources:
- ../common

namePrefix: b

# common/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: "-example"
spec:
  template:
    spec:
      containers:
      - name: app
        envFrom:
        - configMapRef:
            name: "-example-configmap"

# common/kustomization.yaml
resources:
- deployment.yaml

configMapGenerator:
- name: "-example-configmap"

Prior to this change we get (tested on v3.9.3-4.5.7):

apiVersion: v1
kind: ConfigMap
metadata:
  name: a-example-configmap-6ct58987ht
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: b-example-configmap-6ct58987ht
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: a-example
spec:
  template:
    spec:
      containers:
      - envFrom:
        - configMapRef:
            name: a-example-configmap-6ct58987ht
        name: app
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: b-example
spec:
  template:
    spec:
      containers:
      - envFrom:
        - configMapRef:
            name: b-example-configmap-6ct58987ht
        name: app

However after the change we now get:

**** Too many possible referral targets to referrer:
apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
    config.kubernetes.io/prefixes: a
    config.kubernetes.io/previousNames: -example
    config.kubernetes.io/previousNamespaces: default
  name: a-example
spec:
  template:
    spec:
      containers:
      - envFrom:
        - configMapRef:
            name: -example-configmap
        name: app

--- possible referral 0:
apiVersion: v1
kind: ConfigMap                    
metadata:
  annotations:
    config.kubernetes.io/prefixes: a
    config.kubernetes.io/previousNames: -example-configmap,a-example-configmap
    config.kubernetes.io/previousNamespaces: default,default
  name: a-example-configmap-6ct58987ht
------
--- possible referral 1:
apiVersion: v1
kind: ConfigMap
metadata:
  annotations:                     
    config.kubernetes.io/prefixes: b
    config.kubernetes.io/previousNames: -example-configmap,b-example-configmap
    config.kubernetes.io/previousNamespaces: default,default
  name: b-example-configmap-6ct58987ht
------
Error: updating name reference in 'spec/template/spec/containers/envFrom/configMapRef/name' field of 'apps_v1_Deployment|~X|a-example': considering field 'spec/template/spec/containers/envFrom/configMapRef/name' of object
apiVersion: apps/v1
kind: Deployment
metadata:
  name: "a-example"                
  annotations:
    config.kubernetes.io/prefixes: a
    config.kubernetes.io/previousNames: -example
    config.kubernetes.io/previousNamespaces: default
spec:                              
  template:                        
    spec:                          
      containers:                  
      - name: app                  
        envFrom:
        - configMapRef:
            name: "-example-configmap"
: visit traversal on path: [envFrom configMapRef name]: visit traversal on path: [configMapRef name]:  found multiple possible referrals: ~G_v1_ConfigMap|~X|a-example-configmap-6ct58987ht, ~G_v1_ConfigMap|~X|b-example-configmap-6ct58987ht

I've added this new example and also the patched version of kustomize to my example repository.

Any ideas/suggestions would be greatly appreciated! (and please say if I've misunderstood something/made a silly error again 😅)

jonathanlking · 2023-04-28T00:38:30Z

I re-read your comment and I think this patch might work (it still fixes the original issue, but doesn't introduce the new one):

diff --git i/api/resource/resource.go w/api/resource/resource.go
index 53a62dffd..3d471e6d9 100644
--- i/api/resource/resource.go
+++ w/api/resource/resource.go
@@ -320,8 +320,8 @@ 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 SameEndingSubarray(r.GetNamePrefixes(), o.GetNamePrefixes()) && SameEndingSubarray(r.GetNameSuffixes(), o.GetNameSuffixes())
+func(r * Resource) PrefixesSuffixesEquals(o ResCtx) bool {
+    return (len(r.GetNamePrefixes()) == 0 || len(o.GetNamePrefixes()) == 0 || SameEndingSubarray(r.GetNamePrefixes(), o.GetNamePrefixes())) && (len(r.GetNameSuffixes()) == 0 || len(o.GetNameSuffixes()) == 0 || SameEndingSubarray(r.GetNameSuffixes(), o.GetNameSuffixes()))
 }
 
 // RemoveBuildAnnotations removes annotations created by the build process.

If either of the prefixes/suffixes are empty then it treats that prefix/suffix as equal, which is what the previous behaviour was (I think?).

jonathanlking · 2023-07-08T22:53:39Z

While trying to implement a fix, I think I’m getting a better understanding of the problem.
For the example I shared, in selectReferral, the value of candidates before doSieve(candidates, prefixSuffixEquals(f.Referrer) is

kustomize/api/filters/nameref/nameref.go

Line 328 in da4e881

candidates = doSieve(candidates, prefixSuffixEquals(f.Referrer))

[
  {
    "apiVersion": "v1",
    "kind": "ConfigMap",
    "metadata": {
      "annotations": {
        "internal.config.kubernetes.io/generatorBehavior": "unspecified",
        "internal.config.kubernetes.io/needsHashSuffix": "enabled",
        "internal.config.kubernetes.io/prefixes": "a",
        "internal.config.kubernetes.io/previousKinds": "ConfigMap,ConfigMap",
        "internal.config.kubernetes.io/previousNames": "-example-configmap,a-example-configmap",
        "internal.config.kubernetes.io/previousNamespaces": "default,default"
      },
      "name": "a-example-configmap-6ct58987ht"
    }
  },
  {
    "apiVersion": "v1",
    "kind": "ConfigMap",
    "metadata": {
      "annotations": {
        "internal.config.kubernetes.io/generatorBehavior": "unspecified",
        "internal.config.kubernetes.io/needsHashSuffix": "enabled",
        "internal.config.kubernetes.io/prefixes": "b",
        "internal.config.kubernetes.io/previousKinds": "ConfigMap,ConfigMap",
        "internal.config.kubernetes.io/previousNames": "-example-configmap,b-example-configmap",
        "internal.config.kubernetes.io/previousNamespaces": "default,default"
      },
      "name": "b-example-configmap-6ct58987ht"
    }
  }
]

and the resource is:

{
  "apiVersion": "apps/v1",
  "kind": "Deployment",
  "metadata": {
    "annotations": {
      "internal.config.kubernetes.io/prefixes": "b",
      "internal.config.kubernetes.io/previousKinds": "Deployment,Deployment",
      "internal.config.kubernetes.io/previousNames": "-,-api",
      "internal.config.kubernetes.io/previousNamespaces": "default,default",
      "internal.config.kubernetes.io/suffixes": "api"
    },
    "name": "b-api"
  },
  "spec": {
    "template": {
      "spec": {
        "containers": [
          {
            "envFrom": [
              {
                "configMapRef": {
                  "name": "-example-configmap"
                }
              }
            ],
            "name": "app"
          }
        ]
      }
    }
  }
}

The second candidate is what I would like to get picked, however while it has the prefix "b", it doesn't have a suffix and therefore is filtered away.
Before #3529 this didn't matter

For some time, SameEndingSubarray (formerly private, with no unit test) was treating an empty array and a one entry array as having the same "ending subarray"

as this is exactly that scenario.

jonathanlking · 2023-07-08T23:22:03Z

If we change the logic to “OR” as @Serializator suggested, we run into the same issue that #3529 was trying to fix, which is that we're left with more than one candidate in some scenarios (which is caught by some existing unit tests, e.g. TestReusableCustomTransformers).

If we break the prefixSuffixEquals filter step into more granular steps, where you progressively get more aggressive in filtering while there is more than one candidate, I think this could solve the problem.

I think there are a few different heuristics you could use, e.g. initially

Allow either (inclusive) a prefix or suffix to match (like what @Serializator suggested)
Treat an empty prefix/suffix as having the same SameEndingSubarray as a one element prefix/suffix (the behaviour before Fix 3489 #3529)
Treat an empty prefix/suffix as having the same SameEndingSubarray as any prefix/suffix (what I have on Fix name in a configMapRef missing hash #5047 #5236)

This approach does feel a bit hacky to me, however if we’re only searching for the best, rather than an exact match (which I get the impression we are) I'm more comfortable. My branch in #5236 does seem to pass the krusty unit tests, which is promising.

I'd really appreciate feedback on whether this approach makes sense/if you think I've misunderstood the problem/have any other suggestions! 🙏

BishuSinha · 2023-10-14T12:10:47Z

Any fix on v5.1.1, still facing the issue

jonathanlking · 2023-10-14T12:18:23Z

@BishuSinha I'm waiting for feedback on whether my suggested approach makes sense/would be accepted. We've been using a patched version of Kustomize at work (based on #5236) and not had any issues, which is promising.

BishuSinha · 2023-10-14T12:33:30Z

@BishuSinha I'm waiting for feedback on whether my suggested approach makes sense/would be accepted. We've been using a patched version of Kustomize at work (based on #5236) and not had any issues, which is promising.

Thanks mate , will use the patch for now , cheers

* Add regression tests * Update PrefixesSuffixesEquals function * Try empty prefix/suffix but fall back on duplicates * Run gofmt * Remove newline * Revert unnecessary gofmt change * Add comment

jonathanlking added the kind/bug Categorizes issue or PR as related to a bug. label Feb 15, 2023

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 15, 2023

k8s-ci-robot assigned Serializator Mar 15, 2023

This comment was marked as outdated.

Sign in to view

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 15, 2023

jsitu777 mentioned this issue Mar 17, 2023

Update Kubeflow Pipeline ECR image and Helm charts awslabs/kubeflow-manifests#626

Merged

2 tasks

jonathanlking mentioned this issue Jul 7, 2023

Fix name in a configMapRef missing hash #5047 #5236

Merged

CameronHudson8 mentioned this issue Jan 24, 2024

env.valueFrom.secretKeyRef.name not changed in overlay when using namePrefix #1563

Closed

k8s-ci-robot closed this as completed in #5236 Apr 2, 2024

safizn mentioned this issue Dec 21, 2024

Name in a configMapRef is not updated to include suffix/prefix when namespace is defined #5827

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Name in a configMapRef is not updated to include the hash as a suffix (nor a prefix) #5047

Name in a configMapRef is not updated to include the hash as a suffix (nor a prefix) #5047

jonathanlking commented Feb 15, 2023 •

edited

Loading

Serializator commented Mar 15, 2023

Serializator commented Mar 15, 2023 •

edited

Loading

This comment was marked as outdated.

jonathanlking commented Mar 15, 2023 •

edited

Loading

Serializator commented Mar 15, 2023 •

edited

Loading

natasha41575 commented Mar 15, 2023

jonathanlking commented Mar 15, 2023

Serializator commented Mar 15, 2023 •

edited

Loading

jsitu777 commented Mar 23, 2023

jonathanlking commented Apr 27, 2023

jonathanlking commented Apr 28, 2023

jonathanlking commented Jul 8, 2023

jonathanlking commented Jul 8, 2023

BishuSinha commented Oct 14, 2023

jonathanlking commented Oct 14, 2023

BishuSinha commented Oct 14, 2023

Name in a configMapRef is not updated to include the hash as a suffix (nor a prefix) #5047

Name in a configMapRef is not updated to include the hash as a suffix (nor a prefix) #5047

Comments

jonathanlking commented Feb 15, 2023 • edited Loading

What happened?

What did you expect to happen?

How can we reproduce it (as minimally and precisely as possible)?

Expected output

Actual output

Kustomize version

Operating system

Serializator commented Mar 15, 2023

Serializator commented Mar 15, 2023 • edited Loading

This comment was marked as outdated.

jonathanlking commented Mar 15, 2023 • edited Loading

Serializator commented Mar 15, 2023 • edited Loading

natasha41575 commented Mar 15, 2023

jonathanlking commented Mar 15, 2023

Serializator commented Mar 15, 2023 • edited Loading

jsitu777 commented Mar 23, 2023

jonathanlking commented Apr 27, 2023

jonathanlking commented Apr 28, 2023

jonathanlking commented Jul 8, 2023

jonathanlking commented Jul 8, 2023

BishuSinha commented Oct 14, 2023

jonathanlking commented Oct 14, 2023

BishuSinha commented Oct 14, 2023

jonathanlking commented Feb 15, 2023 •

edited

Loading

Serializator commented Mar 15, 2023 •

edited

Loading

jonathanlking commented Mar 15, 2023 •

edited

Loading

Serializator commented Mar 15, 2023 •

edited

Loading

Serializator commented Mar 15, 2023 •

edited

Loading