open-policy-agent / gatekeeper

🐊 Gatekeeper - Policy Controller for Kubernetes
https://open-policy-agent.github.io/gatekeeper/
Apache License 2.0
3.68k stars 757 forks source link

AssignMetadata: Allow targeting objects by annotations as well as labels #2087

Open seh opened 2 years ago

seh commented 2 years ago

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

maxsmythe commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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] commented 2 years ago

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 commented 2 years ago

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

seh commented 2 years ago

Well, that was insulting.

maxsmythe commented 2 years ago

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 commented 2 years ago

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

seh commented 2 years ago

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

maxsmythe commented 2 years ago

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

stale[bot] commented 2 years ago

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 commented 2 years ago

What counts as "activity?"

stale[bot] commented 1 year ago

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 commented 1 year ago

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

stale[bot] commented 1 year ago

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 commented 1 year ago

This is not stale.

ritazh commented 1 year ago

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

thelabdude commented 1 year ago

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 commented 1 year ago

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

At what time does this call occur?

ritazh commented 1 year ago

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

seh commented 1 year ago

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 commented 1 year ago

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

stale[bot] commented 1 year ago

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 commented 1 year ago

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] commented 1 year ago

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 commented 1 year ago

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

stale[bot] commented 1 year ago

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 commented 1 year ago

As before, this issue remains relevant.

stale[bot] commented 12 months ago

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.