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

Kustomize doesn't append nameSuffix from overlay when patch is applied in patchesJson6902 section in version 4.2.0 #4111

Closed
Art3mK opened this issue Aug 10, 2021 · 3 comments · Fixed by #4266
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@Art3mK
Copy link

Art3mK commented Aug 10, 2021

nameSuffix isn't applied to serviceAccountName for daemonset, if patch is specified in patchesJson6902 section, the same patch in section patches works fine.

See sample setup in https://github.com/Art3mK/kustomize-bug

Expected output

➜  kustomize build overlays/prod
apiVersion: v1
kind: ServiceAccount
metadata:
  annotations:
    eks.amazonaws.com/role-arn: arn:aws:iam::12345678910:role/fluentd-cw-logs
  name: fluentd-sa-abc
  namespace: abc
---
apiVersion: apps/v1
kind: DaemonSet
metadata:
  name: fluentd-abc
  namespace: abc
spec:
  template:
    spec:
      containers:
      - image: fluentd:latest
        name: fluentd
      serviceAccountName: fluentd-sa-abc

Actual output

➜  kustomize build overlays/prod
apiVersion: v1
kind: ServiceAccount
metadata:
  annotations:
    eks.amazonaws.com/role-arn: arn:aws:iam::12345678910:role/fluentd-cw-logs
  name: fluentd-sa-abc
  namespace: abc
---
apiVersion: apps/v1
kind: DaemonSet
metadata:
  name: fluentd-abc
  namespace: abc
spec:
  template:
    spec:
      containers:
      - image: fluentd:latest
        name: fluentd
      serviceAccountName: fluentd-sa # <- missing nameSuffix

Kustomize version

{Version:kustomize/v4.2.0 GitCommit:d53a2ad45d04b0264bcee9e19879437d851cb778 BuildDate:2021-07-01T00:44:28+01:00 GoOs:darwin GoArch:amd64}

Platform

macOs

Additional context

adding random suffix to target in patch results in correct template

➜  cat overlays/prod/kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

bases:
  - "../../base"

namespace: abc
nameSuffix: -abc

# works fine with
#patches:
patchesJson6902:
  - path: patch-sa.yaml
    target:
      name: fluentd-sa-random # <- should give error, but it doesn't?
      kind: ServiceAccount
      version: v1
➜  kustomize build overlays/prod
apiVersion: v1
kind: ServiceAccount
metadata:
  name: fluentd-sa-abc
  namespace: abc
---
apiVersion: apps/v1
kind: DaemonSet
metadata:
  name: fluentd-abc
  namespace: abc
spec:
  template:
    spec:
      containers:
      - image: fluentd:latest
        name: fluentd
      serviceAccountName: fluentd-sa-abc # <- lolwhut
@Art3mK Art3mK added the kind/bug Categorizes issue or PR as related to a bug. label Aug 10, 2021
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Aug 10, 2021
@natasha41575 natasha41575 added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Sep 24, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 24, 2021
Serializator added a commit to Serializator/kustomize that referenced this issue Nov 2, 2021
@Serializator
Copy link
Contributor

/assign @Serializator

@Serializator
Copy link
Contributor

Just a status update since I assigned the issue to myself. I think to have found the problem and pushed the necessary change to my branch. I will create a pull request shortly. I only signed the CLA yesterday and I didn't know if creating a pull request right after would work.

@Serializator
Copy link
Contributor

I created a pull request (#4266) in which a proposed a solution which I neglected to run tests on locally, which turned out to indicate that the proposed solution would introduce at least one regression.

I dove into the commit history and using bisect discovered out that this bug was introduced in a commit (bd4580d) that changed in-memory fields in the resource.Resource struct with annotations.

Commenting out the delete(...) calls in resource.(*Resource).RemoveIdAnnotations made clear that the actual problem is that these annotations are replaced with nothing, due to the patch "overriding" the /metadata/annotations path in some way. Thus the prefix and suffix are lost and not applied.

apiVersion: v1
kind: ServiceAccount
metadata:
    annotations:
        eks.amazonaws.com/role-arn: arn:aws:iam::12345678910:role/fluentd-cw-logs
    name: fluentd-sa-abc
    namespace: abc

"patches" uses a strategic merge, which properly merges the annotations from Kustomize and the ones defined within the patch itself.

apiVersion: v1
kind: ServiceAccount
metadata:
    annotations:
        config.kubernetes.io/originalName: fluentd-sa
        config.kubernetes.io/originalNs: default
        config.kubernetes.io/prefixes: null
        config.kubernetes.io/suffixes: -abc
        eks.amazonaws.com/role-arn: arn:aws:iam::12345678910:role/fluentd-cw-logs
    name: fluentd-sa-abc
    namespace: abc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants