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

Suffix not added to ConfigMap name #5712

Open
tcurdt opened this issue Jun 2, 2024 · 14 comments
Open

Suffix not added to ConfigMap name #5712

tcurdt opened this issue Jun 2, 2024 · 14 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@tcurdt
Copy link

tcurdt commented Jun 2, 2024

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 tcurdt added the kind/bug Categorizes issue or PR as related to a bug. label Jun 2, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jun 2, 2024
@tcurdt
Copy link
Author

tcurdt commented Jun 2, 2024

This seems like a similar problem as described here #5223

@tcurdt tcurdt changed the title ConfigMap reference not adjusted suffix not added to ConfigMap name Jun 2, 2024
@tcurdt tcurdt changed the title suffix not added to ConfigMap name Suffix not added to ConfigMap name Jun 2, 2024
@tcurdt
Copy link
Author

tcurdt commented Jun 2, 2024

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
Copy link
Author

tcurdt commented Jun 3, 2024

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
Copy link
Author

tcurdt commented Jun 3, 2024

This seems to be more than related #4833

@tcurdt
Copy link
Author

tcurdt commented Jun 3, 2024

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
Copy link
Member

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

@k8s-ci-robot k8s-ci-robot added triage/needs-information Indicates an issue needs more information in order to work on it. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 5, 2024
@tcurdt
Copy link
Author

tcurdt commented Jun 5, 2024

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Author

tcurdt commented Jul 9, 2024

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
Copy link
Author

tcurdt commented Jul 9, 2024

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
Copy link
Contributor

@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
Copy link
Author

tcurdt commented Jul 9, 2024

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
Development

No branches or pull requests

4 participants