Support label and annotation selectors in replacement source/targets

[marshall007]

Is your feature request related to a problem? Please describe.

We have identified several scenarios where we were previously using strategic merge patches with hard-coded values that can be made more robust by using replacements targeting array values by name. For example, manipulating service and container port values.

Unfortunately, replacements does not support filtering sources and targets by labelSelector and annotationSelector.

Describe the solution you'd like

For consistency, I think it would make sense to have better parity between patch target and replacement source/target. I couldn't find any obvious reason why label and annotation selectors would not be available on replacements too.

[k8s-ci-robot]

@marshall007: This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[natasha41575]

I can see the use case for filtering replacement targets by labelSelector and annotationSelector (and implementation would be straightforward), but I'm curious why you would want it for replacement source values. Since the source selector must match exactly 1 resource, couldn't you always use its GVKN/N?

[marshall007]

I'm curious why you would want it for replacement source values. Since the source selector must match exactly 1 resource, couldn't you always use its GVKN/N?

@natasha41575 I was under the impression that a selector must match exactly 1 or 0 resources (the latter case resulting in a no-op). However, after further experimentation it seems like the behavior is inconsistent. It appears that there are cases where if the specified GVKNN source matches a single resource but the fieldPath does not exist you get the "no-op" behavior I'm describing. In most other cases an uncaught exception is thrown.

I can see the case for strictly enforcing that a single resource is matched by the source selector. This is how vars worked previously and it is useful to know when your selector is not matching what you expect. Maybe we should make it configurable?

In our case, we provide components that target Deployment/StatefulSet/DaemonSet (anything that uses a standard pod spec) based on annotations so we don't have to care what type of workloads the user is deploying. It is up to the end-user to apply the annotation(s) we look for on the single appropriate resource for their application.

The option to have the "no-op" behavior for unmatched replacement sources would be handy because we can wrap the user-provided resources/kustomization in a "last-mile" kustomization that conditionally applies patches/replacements if annotated by the user (and importantly, wouldn't break if they aren't provided).

Here is an example I threw together which demonstrates our use-case (essentially client-side sidecar injection + Service port remapping): https://gist.github.com/marshall007/ebaaea553c69d20395ffdc926a3f1d00

[marshall007]

Was just about to add that "no-op" for unmatched replacement source/targets should be the default behavior as this would align with the behavior of unmatched patch targets. However, there seems to be a regression from v4.1.2 to v4.2.0 and this is no longer the case. A pointer dereference exception is thrown in v4.2.0.

[edit]: never mind, I don't think there is a regression. Unmatched patch targets do still no-op in v4.2.0. The exception was due to an interaction with replacements.

I think we need to take a step back and determine what the desirable default behavior is for unmatched targets in general and apply that consistently to patches, replacements, etc. My opinion is that the default should be a "no-op" with a configurable boolean flag options.required to error when the target is unmatched.

[natasha41575]

The exception was due to an interaction with replacements

If you have the time, please do file a separate issue for what happened here. I have a hunch about what caused that to happen and I think it can be fixed.

I think we need to take a step back and determine what the desirable default behavior is for unmatched targets in general and apply that consistently to patches, replacements, etc.

I totally agree that it makes sense for an unmatched replacement target to be a no-op (which I believe is the current behavior) - and should be consistent with patch targets. But what would it mean to have an unmatched replacement source? In that case, you are asking kustomize to copy a field from a resource into other field(s). If that resource doesn't exist, does it make sense for kustomize to fail silently? Analogous to the vars feature it is replacing, does it make sense to define a var whose object reference doesn't exist? Analogous to patches, does it make sense to define a patches field with a patch file that doesn't exist? To me I would expect these all to be error cases and I see them all as variations as the same mistake: the source is missing.

It appears that there are cases where if the specified GVKNN source matches a single resource but the fieldPath does not exist you get the "no-op" behavior I'm describing.

IMO this should throw an error as well and it is a bug that it doesn't.

My opinion is that the default should be a "no-op" with a configurable boolean flag options.required to error when the target is unmatched.

I am hesitant to add configuration boolean flags. I think we should be very careful about what we make configurable; adding too many knobs will eventually pile up into something increasingly complex and difficult to understand.

[marshall007]

@natasha41575

If you have the time, please do file a separate issue for what happened here.

You bet, see #4078.

But what would it mean to have an unmatched replacement source? In that case, you are asking kustomize to copy a field from a resource into other field(s). If that resource doesn't exist, does it make sense for kustomize to fail silently?

I don't see this case as different from the unmatched patch source/target case. I think of replacements as basically "patches that can source values from resources". In the case of patches, the source and target resource are the same. When using replacements, the source and target resource may be the same or they can be different.

Analogous to the vars feature it is replacing, does it make sense to define a var whose object reference doesn't exist?

I think the big difference between replacements and vars is that vars required the use of placeholder values that were not suitable for actual use. In other words, if they were not substituted, the generated resources were not valid for deployment. This is not necessarily the case with replacements, particularly if you are using options.create. On the contrary, this ability to introduce pseudo-conditional behaviors in your kustomizations is potentially very powerful.

I am hesitant to add configuration boolean flags. I think we should be very careful about what we make configurable; adding too many knobs will eventually pile up into something increasingly complex and difficult to understand.

Definitely agree, but in this particular case I see the addition of .options.required: <bool> (or perhaps .behavior: <enum>, inline with secret/configmap generator syntax) as adding more overall consistency to the API.

Consider if you have replacements targeting fields that are being applied by patches. If these patches are (silently) not applied because a suitable target cannot be found then the corresponding replacements will always fail because the behavior of these transformers are not aligned.

.behavior: on generators allow the user to opt out of the safer "error if already exists" default. Why limit this to generators and not make it available on patches and replacements as well?

[marshall007]

@natasha41575 I'm noticing today additional syntax for selectors that work in patches but do not work in replacements. I think we should make the spec and behavior for patches and replacements targets identical.

replacements:
- source:
    # ...
  targets:
  - select:
      kind: ValidatingWebhookConfiguration|MutatingWebhookConfiguration
      name: cert-manager-*
    fieldPaths:
    - metadata.annotations.[cert-manager.io/inject-ca-from-secret]
    options:
      delimiter: /
      index: 0

[natasha41575]

I think we should make the spec and behavior for patches and replacements targets identical.

I'm open to this, consistency across targets makes sense to me.

For having a missing source be a no-op instead of an error, I'd like to know @monopole's and @KnVerey's thoughts.

[KnVerey]

I agree with making target config consistent across replacements and patches, but aren't label and annotation selectors already supported in targets? The TargetSelector type embeds a Selector, which has those fields. Could this just be a test/docs gap?

I'm not convinced about source though. I think @natasha41575 is right that source is analogous to the patch itself, which in the patches field isn't a selectable resource at all, but rather a hardcoded value that would cause an error if missing. Looking at the provided gist, it seems like even if we allowed annotation selection in the source, the Kustomization would still be fragile in that it would break if the end user wanted to use gatekeeper on more than one resource. In the first replacement, the source value isn't dynamically determined by the user, but rather statically determined in the patch above in the same file. In the second replacement, what seems to be needed is to copy the port from one container in a given resource to the env of another container in the same resource. It's easy to imagine someone wanting to do that across a group of resources, and I don't think it's something replacements could support. Replacements are powerful, but they're not going to fulfill every use case elegantly.

Somewhat of a sidenote: The desired behavior could be built fairly easily with the kyaml functions framework. We need to make custom transformers easier to use (as briefly outlined in our roadmap) so that we can feel confident in recommending them as a solution to use cases that have complex, effectively conditional, logic to apply.

[marshall007]

@KnVerey I don't disagree with your conclusion regarding custom transformers being the preferred solution here, but just wanted to clarify a couple points:

the Kustomization would still be fragile in that it would break if the end user wanted to use gatekeeper on more than one resource.

Our intent with this component is that users would only apply it to groups of resources where each group contains only one "public endpoint" (i.e. annotated resource). I could be wrong, but I think if you included these replacements as a component in multiple child kustomizations that they would not "bubble up" and conflict with each other.

But yes, to have a robust solution we would need some way to select each annotated Deployment resource and for each of those, sub-select only the Service(s) with a matching field selector for that Deployment.

In the first replacement, the source value isn't dynamically determined by the user, but rather statically determined in the patch above in the same file. In the second replacement, what seems to be needed is to copy the port from one container in a given resource to the env of another container in the same resource.

We do the first replacement to guard against a user-defined patch or replacement on top of the 8081 port statically defined in our patch. I don't think it makes any difference right now, but it would be correctly tracked once #4034 is fixed.

It's easy to imagine someone wanting to do that across a group of resources, and I don't think it's something replacements could support.

You could imagine a generic mechanism that would just allow us to sub-select source/target resources based on matching values between resources (rather than the static k8s.appdat.jsc.nasa.gov/gatekeeper=enabled).

That all seems well out of scope for replacements right now, but may be a topic worth discussing in the future.

[k8s-triage-robot]

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

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

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

/lifecycle stale

[natasha41575]

#4229 has merged which supports label and annotation selectors in targets.

As discussed above, we don't think it makes sense to support in sources, so I am closing this issue as fixed.