AssignMetadata: Allow targeting objects by annotations as well as labels

[seh]

Desire

Per discussion that I started in the "opa-gatekeeper" channel of the "Open Policy Agent" Slack workspace, I would like to be able to select target objects for the AssginMetadata mutator by way of their annotations, and not just their labels.

Repeating what I wrote earlier, I noticed that the AssignMetadata mutator allows one to select target objects by label and set an annotation, but I can’t select target objects by annotation. Is that an efficiency concession, since the Kubernetes API allows filtering admission requests by label, or is it a safety concession to prevent oscillation? The latter doesn’t seem likely, as the criteria and the mutations would be equivalent, just targeting the “metadata.annotations” field rather than the “metadata.labels” field.

We have a situation where we’d like to add an annotation with a fixed value to objects that also bear a related annotation, but in the current system we can’t target objects by whether they have an annotation with a particular value.

We're not allowed to use path tests with AssignMetadata; its implementation synthesizes a single path test that asserts that the target field for the assignment does not exist. If we could use path tests, I could attempt to select the triggering annotation, or reject target objects that lack the intended matching annotation.

Motivation

Our motivating scenario involves automating a gradual transition from annotating objects with one longstanding annotation to a newer replacement. Essentially, we'd like to say, "For all objects annotated with 'original' and a value of 'true,' add a 'newer' annotation with a value of 'yes.'" Of course the names and values are notional, but I hope that shows that we couldn't induce oscillation this way—or at least I haven't been able to concoct a scenario that runs afoul of that threat.

Environment

• Gatekeeper version: 3.8.0
• Kubernetes version: 1.24.1

[maxsmythe]

The main issue with trying to implement annotation selectors is how to write annotation selectors. I think I wrote about this a while back, but I can't find it.

The thing is that annotations are structured whereas labels must have simple string values. E.g. if I have the annotation:

{"one": "a", "two": "b"}

and there is a selector that is expecting:

{"two": "b", "one": "a" }

should the annotation match? What about matching against a superset of an annotation? Formats that are not JSON?

K8s also mentions that annotations aren't/shouldn't be used as identifying metadata:

https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#attaching-metadata-to-objects

Really though the main thing is the arbitrary structure of the value, which means it may not be possible to write a one-size-fits-all selection criteria like label selectors are.

[seh]

The thing is that annotations are structured whereas labels must have simple string values. E.g. if I have the annotation:

{"one": "a", "two": "b"}

and there is a selector that is expecting:

{"two": "b", "one": "a" }

should the annotation match?

Where did you hear that annotation values are structured? The documentation notes that they are unstructured, and the validation function in the API machinery only checks that the keys are qualified names. There are no constraints on the values, apart from total size together with the keys.

[seh]

In an appeal to prior art, note that both kustomize and Kyverno allow selecting target objects by annotation selectors. Kyverno has an example in its documentation, and kustomize allows such target selection in its "patches" and "replacements" transformers.

I'm well aware of the distinction between the intended uses of labels and annotations on objects in the Kubernetes API. In this case, we're meeting manifests where they already are: They have some old annotation, and we need to help migrate them to use a newer annotation. You could say, "Well, you should have labeled them all first!" I'm not the author of these manifests, and there are many controllers out there consuming annotations that wind up with no correlation with the labels on the annotated objects.

Hewing to this stricture keeps Gatekeeper more in the theoretically useful realm rather than the pragmatic realm. This is the kind of problem we have to solve today. We're not responsible for having found the problem, having made some mistake in understanding how to best use Kubernetes. We'd like Gatekeeper's help in fixing it. We could write our own mutating admission Webhook, or we could use Kyverno, but we'd prefer to use Gatekeeper.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[seh]

Would the Gatekeeper project be amenable to a patch implementing this proposed capability, or are you collectively opposed to the idea?

[seh]

Well, that was insulting.

[maxsmythe]

Sorry, this slipped off my radar, also your reply should have prevented the bug from auto-closing. @sozercan any idea how to check for activity before the bug is closed? Or maybe it's that we need to re-send a warning?

Personally, I'm okay with it as long as it's clear it will never be more than a strict string match. The one risk here would be if mutators gained the ability to mutate annotations beyond assigning them a constant value if undefined. For instance, if mutators could mutate JSON inside of an annotation, this could lead to infinite recursion. OTOH, it's probably better for mutators to treat metadata lightly as a general rule for uniformity.

@sozercan @ritazh thoughts on the feature and/or the tradeoff WRT mutation?

[ritazh]

@seh would adding support to select target objects by annotations with strict string match address your use case?

[seh]

Yes, that would satisfy my needs. What other options would we be eschewing there? Matching by regular expression?

[maxsmythe]

Or some kind of structure-aware matching -- a lot of annotations are serialized JSON

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[seh]

What counts as "activity?"

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[maxsmythe]

@seh pinging of the bug (though not this message)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[seh]

This is not stale.

[ritazh]

Let’s discuss this in the next community call on Wednesday. @seh would be great if you could join.

[thelabdude]

Figured I'd chime in here too, as we also need this capability. Currently, we run the Cluster Autoscaler, and I have many workloads on the cluster that use the "cluster-autoscaler.kubernetes.io/safe-to-evict": "false" annotation on their pods to prevent the CA from down-scaling nodes that host pods with that annotation. We're considering a migration to Karpenter, which has the same concept, but uses a different annotation karpenter.sh/do-not-evict: "true"! I'd prefer to not have to tell 100's of job owners across a large fleet of clusters to go re-configure their job settings to change the annotation. This is a perfect use of OPA assign metadata where I can find pods with the safe-to-evict annotation and swap to using the do-not-evict annotation transparently.

[seh]

Let’s discuss this in the next community call on Wednesday.

At what time does this call occur?

[ritazh]

Today, it will be at 2PM PT. I have already added it to the weekly meeting agenda

[seh]

Today, it will be at 2PM PT.

Unfortunately, I won't be able to attend the meeting, as I'll be traveling at that time. I'll follow up later to review notes or watch a recording. Perhaps @thelabdude can attend to lend support for this cause.

[maxsmythe]

We talked about this at the meeting some today. Some points that were raised:

• Assuming convergence as a requirement, this feature is mutually exclusive with other potential features that may edit annotations, including:
  • mutators that are able to deserialize/mutate/reserialize annotations (e.g. a JSON mutator), which could be used to customize the behavior of any controller that works from annotations.
  • Mutators that are able to replace the value of an annotation, e.g. changing "safe-to-evict" from "true" to "false" (the current suggestion of adding a second annotation based off the value of the first would be compatible though... essentially annotations would work like labels
• For the above, it's not clear which use case is necessarily more valuable to users (though annotations-as-selectors does at least have this open issue), but whatever decision should be very clear-eyed about the tradeoff being made
• There is a KEP being worked on for in-tree mutation
  • This KEP is a WIP, so its final syntax is unknown
  • In-process is a much better place for most mutations to take place due to mutation's recursive nature and in-process's lower latency profile
    • Because of this we should probably be cognizant of how to either add value by operating alongside the mutation KEP, carve out a functional niche that the mutation KEP wouldn't cover (like external data), or expect mutation to be replaced by the current KEP
    • Which of these is true is hard to know until the KEP is more solid, and may justify more or less investment into mutation (hard to justify spending time developing a feature if a better-positioned one is coming down the pipe from a platform you're building on top of)
  • Having a stance on convergence from core K8s and seeing its impact would provide good signal on how larger companies view this problem (e.g. will large cloud providers disable in-process mutation if it's poorly-behaved?), having this data could better inform the most useful direction.
    • One possible value proposition is to have core K8s have more expressive mutation primitives, with Gatekeeper providing a more easy-to-manage mutation primitives on top that facilitate delegation.
  • Once the shape of the KEP is better-defined, it will be easier to know what design decisions are more or less likely to have Gatekeeper mutation and in-tree mutation play nicely together.
  • The existence of the KEP does not necessarily preclude doing something here, but it's always good to develop with an awareness of the direction of the larger ecosystem.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[seh]

It sounds like we're waiting to see how KEP 3962 (in kubernetes/enhancements#3963) progresses, but that still only addresses selection by annotation obliquely by way of the proposed "spec.matchConditions[*].expression" fields.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[seh]

As we watch for progress with the aforementioned KEP, this issue remains relevant.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[seh]

As before, this issue remains relevant.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.