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

Support annotation and label selection in replacement targets #4223

Closed
seh opened this issue Oct 4, 2021 · 8 comments · Fixed by #4229
Closed

Support annotation and label selection in replacement targets #4223

seh opened this issue Oct 4, 2021 · 8 comments · Fixed by #4229
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@seh
Copy link
Contributor

seh commented Oct 4, 2021

Description

When populating just the "replacements.targets.select.annotationSelector" field for restricting the targets of a replacement, kustomize mistakenly chooses all candidate resources as targets.

Test Case

Input

File objects.yaml

apiVersion: v1
kind: Secret
metadata:
  name: s-1
  namespace: ns-1
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm-1
  namespace: ns-1
  annotations:
    cert-manager.io/inject-ca-from-secret: ns-1/s-1

File kustomization.yaml

Though my actual intention is to match only objects annotated with "cert-manager.io/inject-ca-from-secret" with a specific value, I discovered that no matter what value I use for the "annotationSelector" field, including the empty string, kustomize will apply the replacement to any target object that has the "cert-manager.io/inject-ca-from-secret" annotation present—or at least those are the only targets for which the replacement has consequences. Here I've used the annotation selector "xyz=123" to make it obvious that it should not match any of the objects.

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

resources:
- objects.yaml

namespace: ns-2

replacements:
- source:
    group: ""
    kind: Secret
    name: s-1
    fieldPath: metadata.namespace
  targets:
  - select:
      annotationSelector: xyz=123
    fieldPaths:
    - metadata.annotations.cert-manager\.io/inject-ca-from-secret
    options:
      delimiter: /

Expected Output

I'm expecting the "cm-1" ConfigMap to retain its original annotation value.

apiVersion: v1
kind: ConfigMap
metadata:
  annotations:
    cert-manager.io/inject-ca-from-secret: ns-1/s-1
  name: cm-1
  namespace: ns-2
---
apiVersion: v1
kind: Secret
metadata:
  name: s-1
  namespace: ns-2

Actual Output

Note that the "cm-1" ConfigMap's annotation value changed to reflect the "ns-2" namespace in which the "s-1" Secret now sits.

apiVersion: v1
kind: ConfigMap
metadata:
  annotations:
    cert-manager.io/inject-ca-from-secret: ns-2/s-1
  name: cm-1
  namespace: ns-2
---
apiVersion: v1
kind: Secret
metadata:
  name: s-1
  namespace: ns-2

Context

Kustomize Version

{Version:kustomize/v4.4.0 GitCommit:63ec6bdb3d737a7c66901828c5743656c49b60e1 BuildDate:2021-09-27T16:13:36Z GoOs:darwin GoArch:amd64}

Platform

macOS

Additional

My real goal is to select certain objects annotated with particular values of "cert-manager.io/inject-ca-from-secret," and to replace those values. That works, but it works too liberally: kustomize applies the replacement to objects with that annotation, but even those with different values that don't match the selector.

To make the example more realistic, consider the following.

Input

File objects.yaml

apiVersion: v1
kind: Secret
metadata:
  name: s-1
  namespace: ns-1
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm-1
  namespace: ns-1
  annotations:
    cert-manager.io/inject-ca-from-secret: ns-1/s-1
---
apiVersion: v1
kind: Secret
metadata:
  name: s-2
  namespace: ns-3
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm-2
  namespace: ns-3
  annotations:
    cert-manager.io/inject-ca-from-secret: ns-3/s-2

File kustomization.yaml

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

resources:
- objects.yaml

namespace: ns-2

replacements:
- source:
    group: ""
    kind: Secret
    name: s-1
    fieldPath: metadata.namespace
  targets:
  - select:
      annotationSelector: cert-manager.io/inject-ca-from-secret=ns-1/s-1
    fieldPaths:
    - metadata.annotations.cert-manager\.io/inject-ca-from-secret
    options:
      delimiter: /

Expected Output

apiVersion: v1
kind: ConfigMap
metadata:
  annotations:
    cert-manager.io/inject-ca-from-secret: ns-2/s-1
  name: cm-1
  namespace: ns-2
---
apiVersion: v1
kind: ConfigMap
metadata:
  annotations:
    cert-manager.io/inject-ca-from-secret: ns-3/s-2
  name: cm-2
  namespace: ns-2
---
apiVersion: v1
kind: Secret
metadata:
  name: s-1
  namespace: ns-2
---
apiVersion: v1
kind: Secret
metadata:
  name: s-2
  namespace: ns-2

Actual Output

Note that the "cm-2" ConfigMap's annotation value starts with "ns-2"—the namespace of the "s-1" Secret—instead of retaining its original value of "ns-3/s-2."

(Let's ignore that the namespace for the "s-2" Secret changed from "ns-3" to "ns-2" by virtue of the "namespace" field in the kustomization. That's kustomize working as directed. My real scenario doesn't suffer that problem.)

apiVersion: v1
kind: ConfigMap
metadata:
  annotations:
    cert-manager.io/inject-ca-from-secret: ns-2/s-1
  name: cm-1
  namespace: ns-2
---
apiVersion: v1
kind: ConfigMap
metadata:
  annotations:
    cert-manager.io/inject-ca-from-secret: ns-2/s-2
  name: cm-2
  namespace: ns-2
---
apiVersion: v1
kind: Secret
metadata:
  name: s-1
  namespace: ns-2
---
apiVersion: v1
kind: Secret
metadata:
  name: s-2
  namespace: ns-2
@seh seh added the kind/bug Categorizes issue or PR as related to a bug. label Oct 4, 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 Oct 4, 2021
@natasha41575
Copy link
Contributor

natasha41575 commented Oct 4, 2021

There is currently no logic implemented in the Replacements Transformer to support annotation or label selection (though I agree that it should). If there are no disagreements from @monopole or @KnVerey, we can add annotation and label selection as a feature request for replacements.

I certainly think it makes sense as a feature and would be easy enough to support.

/kind feature
/triage accepted

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. 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 Oct 4, 2021
@natasha41575 natasha41575 removed the kind/bug Categorizes issue or PR as related to a bug. label Oct 4, 2021
@natasha41575
Copy link
Contributor

/retitle Support annotation and label selection in replacement targets

@k8s-ci-robot k8s-ci-robot changed the title Using "annotationSelector" in replacements targets yields false positive matches Support annotation and label selection in replacement targets Oct 4, 2021
@seh
Copy link
Contributor Author

seh commented Oct 4, 2021

Thank you, Natasha. Viewing the current behavior again knowing now that this selection isn't implemented, the behavior makes so much more sense. I've been using an annotation selector for a couple of months, but I only had one set of eligible targets, such that I couldn't tell that kustomize was being too "greedy." Today I tried to add a second set of targets with different replacement criteria, and first surprise then confusion plagued me for hours.

@KnVerey
Copy link
Contributor

KnVerey commented Oct 5, 2021

I agree with supporting label and annotation selection for targets. I don't think this is being requested here, but for clarity, I don't think it should be supported for sources.

@seh
Copy link
Contributor Author

seh commented Oct 5, 2021

Yes, to clarify, I was not expecting to be able to select sources that way.

It would help users if kustomize would reject selectors that it doesn't support. That would have saved me several hours today, had I encountered either an error or warning message telling me that kustomize couldn't accommodate my field values—yet.

@natasha41575
Copy link
Contributor

It would help users if kustomize would reject selectors that it doesn't support.

Yes, this is a very unfortunate oversight on our part, for having used an underlying object that had an annotationSelector field while neither implementing it nor having a check in case people tried to use it. Apologies and hopefully we won't make the same mistake again.

@natasha41575
Copy link
Contributor

#4229

@zmx
Copy link

zmx commented Nov 23, 2021

{Version:kustomize/v4.4.0 GitCommit:63ec6bdb3d737a7c66901828c5743656c49b60e1 BuildDate:2021-09-27T16:13:36Z GoOs:darwin GoArch:amd64}

In targets fieldPaths,

WORKS: metadata.annotations.[external-dns.alpha.kubernetes.io/hostname]
DON"T WORKS: metadata.annotations.external-dns\.alpha.\kubernetes\.io/hostname

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. 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.

5 participants