Suffix not added to ConfigMap name

tcurdt commented 6 months ago

What happened?

I am trying to use kustomize to rewrite ConfiMap names to include a hash suffix. Ideally the suffix is based on the content of the ConfigMap.

What did you expect to happen?

Since I am merging an existing ConfigMap with an empty one from the generator, I would have expected for the content hash of the final content to be added as a suffix.

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

# kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- caddy.yaml

generatorOptions:
  disableNameSuffixHash: false

configMapGenerator:
  - name: caddy
    namespace: infra
    behavior: merge

# caddy.yaml
---
apiVersion: v1
kind: ConfigMap
metadata:
  namespace: infra
  name: caddy
  labels:
    app: caddy
data:
  Caddyfile: |
    {
    }

---
apiVersion: apps/v1
kind: Deployment
metadata:
  namespace: infra
  name: caddy
  labels:
    app: caddy
spec:
  replicas: 1
  selector:
    matchLabels:
      app: caddy
  template:
    metadata:
      labels:
        app: caddy
    spec:
      containers:
        - name: caddy
          image: caddy:2.7.6-alpine
      volumes:
        - name: caddy-config
          configMap:
            name: caddy

Expected output

apiVersion: v1
data:
  caddy-configmap.yaml: |
    apiVersion: v1
    kind: ConfigMap
    metadata:
      namespace: infra
      name: caddy
      labels:
        app: caddy
    data:
      Caddyfile: |
        {
        }
kind: ConfigMap
metadata:
  labels:
    app: caddy
  name: caddy-AB1234
  namespace: infra
---
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: caddy
  name: caddy
  namespace: infra
spec:
  replicas: 1
  selector:
    matchLabels:
      app: caddy
  template:
    metadata:
      labels:
        app: caddy
    spec:
      containers:
      - image: caddy:2.7.6-alpine
        name: caddy
      volumes:
      - configMap:
          name: caddy-AB1234
        name: caddy-config

Actual output

apiVersion: v1
data:
  Caddyfile: |
    {
    }
kind: ConfigMap
metadata:
  labels:
    app: caddy
  name: caddy
  namespace: infra
---
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: caddy
  name: caddy
  namespace: infra
spec:
  replicas: 1
  selector:
    matchLabels:
      app: caddy
  template:
    metadata:
      labels:
        app: caddy
    spec:
      containers:
      - image: caddy:2.7.6-alpine
        name: caddy
      volumes:
      - configMap:
          name: caddy
        name: caddy-config

Kustomize version

v5.4.2

Operating system

MacOS

tcurdt commented 6 months ago

This seems like a similar problem as described here https://github.com/kubernetes-sigs/kustomize/issues/5223

tcurdt commented 6 months ago

Some more testing. No matter what strategy is used (merge or even replace), when there is a previous ConfigMap exists, the suffix appending does not work.

tcurdt commented 6 months ago

Seems what I want is a HashTransformer, the question is what defines NeedHashSuffix.

Based on BuildAnnotationsGenAddHashSuffix = konfig.ConfigAnnoDomain + "/needsHashSuffix" I tried to just add an annotation of

  annotations:
    internal.config.kubernetes.io/needsHashSuffix: true

but I don't see how to add the HashTransformer yet.

tcurdt commented 6 months ago

This seems to be more than related https://github.com/kubernetes-sigs/kustomize/issues/4833

tcurdt commented 6 months ago

I can confirm that the following annotation

internal.config.kubernetes.io/needsHashSuffix: enabled

makes it work with

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- caddy.yaml

generatorOptions:
  disableNameSuffixHash: false

stormqueen1990 commented 6 months ago

Hi there, @tcurdt! Thanks for reporting this issue!

I am a little confused with your expected output. Did you expect the existing ConfigMap to be embedded on a new ConfigMap generated by Kustomize?

/triage needs-information

tcurdt commented 6 months ago

I am a little confused with your expected output. Did you expect the existing ConfigMap to be embedded on a new ConfigMap generated by Kustomize?

Hey there, @stormqueen1990, when it says behavior: merge I would have thought it would merge the existing and the generated ConfigMap. In the initial example it would be merging the existing ConfigMap with an empty ConfigMap from the MapGenerator (since no new values are provided). Since the hashing is enabled I would have thought this also add a hash suffix to the new ConfigMap.

Does that make things clearer?

I still think that's something to fix.

But conceptually, using a transformer with an annotation seems like the cleaner way for my use case. That said: internal.config.kubernetes.io/needsHashSuffix: enabled really is not a great annotation to rely on. (given the internal and the true vs enabled problem).

But it would be great to officially support such an annotation.

ephesused commented 5 months ago

A quick note that the documentation states:

Name suffixing with overlay configMapGenerator When using configMapGenerator to override values of an existing ConfigMap, the overlay configMapGenerator does not cause suffixing of the existing ConfigMap’s name to occur. To take advantage of name suffixing, use configMapGenerator in the base, and the overlay generator will correctly update the suffix based on the new content.

stormqueen1990 commented 4 months ago

Hey there, @stormqueen1990, when it says behavior: merge I would have thought it would merge the existing and the generated ConfigMap. In the initial example it would be merging the existing ConfigMap with an empty ConfigMap from the MapGenerator (since no new values are provided). Since the hashing is enabled I would have thought this also add a hash suffix to the new ConfigMap.

Does that make things clearer?

Hi @tcurdt, sorry for the delay in getting back to this. I still don't follow why your expected output lists an embedded ConfigMap in a ConfigMap:

apiVersion: v1
data:
  caddy-configmap.yaml: |
    apiVersion: v1
    kind: ConfigMap
    metadata:
      namespace: infra
      name: caddy
      labels:
        app: caddy
    data:
      Caddyfile: |
        {
        }
kind: ConfigMap
metadata:
  labels:
    app: caddy
  name: caddy-AB1234
  namespace: infra

since I don't think that's the goal of behavior: merge. You can, however, achieve this behaviour with Kustomize by creating a ConfigMap with the --from-file option.

I still think that's something to fix.

But conceptually, using a transformer with an annotation seems like the cleaner way for my use case. That said: internal.config.kubernetes.io/needsHashSuffix: enabled really is not a great annotation to rely on. (given the internal and the true vs enabled problem).

But it would be great to officially support such an annotation.

This part is what @ephesused mentioned in the comment prior to this one. Could you give that a try and let me know if it addresses your use case?

tcurdt commented 4 months ago

I still don't follow why your expected output lists an embedded ConfigMap in a ConfigMap

Sorry, map in a map? I have a hard time following that sentence :)

Let me try again:

I have two maps. One map empty, one map given. I have hash suffixes enabled. So I would expect the result of the merge of the two to have a suffix.

For my use case making internal.config.kubernetes.io/needsHashSuffix: enable an external contract/annotation would be good enough. But I still think the current merge behaviour is broken.

tcurdt commented 4 months ago

This part is what @ephesused mentioned in the comment prior to this one. Could you give that a try and let me know if it addresses your use case?

TBH I have no idea what "To take advantage of name suffixing, use configMapGenerator in the base, and the overlay generator will correctly update the suffix based on the new content." is meant to look like.

But it sure sounds like overcomplicating things.

In the end I just want every ConfigMap to have a content hash suffix to allow for a transparent deployment behaviour.

ephesused commented 4 months ago

@tcurdt, as far as I'm aware, the issue is that the merge behavior honors the original ConfigMap. If the original ConfigMap was generated, then a merge rebuilds the suffix based on the merged content. If the original was a plain ConfigMap, then merge does not add a suffix. So if you want every ConfigMap to have a hashed suffix, then you'll need to define those original ConfigMaps by generators.

Here's a simple example of what I was suggesting earlier:

kustomization.yaml

resources:
- caddy

configMapGenerator:
- name: caddy
  namespace: infra
  behavior: merge
  literals:
  - added-key=added-value

caddy/kustomization.yaml

configMapGenerator:
- name: caddy
  namespace: infra
  options:
    labels:
      app: caddy
  literals:
  - "Caddyfile={\n}"

With these results:

$ kustomize build .
apiVersion: v1
data:
  Caddyfile: |-
    {
    }
  added-key: added-value
kind: ConfigMap
metadata:
  labels:
    app: caddy
  name: caddy-5dddm7dgkt
  namespace: infra

$

Note that kustomize updates the suffix based on the newly added entry in the top-level kustomization.yaml. You can see that by building only the subdir:

$ kustomize build caddy
apiVersion: v1
data:
  Caddyfile: |-
    {
    }
kind: ConfigMap
metadata:
  labels:
    app: caddy
  name: caddy-t5fckhgd66
  namespace: infra

$

tcurdt commented 4 months ago

Thanks for example, @ephesused. What is the base and what is the override here? Just to be explicit.

Can you expand on what you mean by "Note that kustomize updates the suffix based on the newly added entry in the top-level kustomization.yaml"?

And while the example might describe the behaviour, I think the behaviour is "not great". The current behaviour really does feels to stray away from KISS - at least from a user perspective IMO. At least I am not sure I understand why the behaviour is like it is, or why it should be as it is.

For my use case I don't want to have anything in the kustomize other than "add a content hash suffix to these ConfigMaps". I'd rather want the yaml files be self-contained. That's what the annotation allows for. And if it wasn't prefixed as "internal" and was supported, it would be all that I need TBH.

The overall goal is: If the ConfigMap content changes, also update the deployments (etc) depending on it.

ephesused commented 4 months ago

I follow. I don't think it's worth digging down into my example. It was to show how to leverage current behavior to get the result you want, but that's not the point here. You're looking for a feature that kustomize currently does not have (allowing a simple way for a merging configMapGenerator to add a suffix to the resulting ConfigMap).

I don't know the "why" that would explain the current behavior, but my guess is what I put in my previous update.

k8s-triage-robot commented 1 month ago

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

This bot triages un-triaged issues 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 issue is closed

You can:

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

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

/lifecycle stale

k8s-triage-robot commented 3 weeks ago

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

This bot triages un-triaged issues 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 issue is closed

You can:

Mark this issue as fresh with /remove-lifecycle rotten
Close this issue with /close
Offer to help out with Issue Triage

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

/lifecycle rotten

kubernetes-sigs / kustomize