Please consider opening the mutations api to support advanced rego scripts

[manelpb]

Hi folks, I do appreciate all the work being done at this project, great stuff.

I'm setting up my opa-gatekeeper and ran into a roadblock I'm unable to easily solve.

TL;DR: Opa-gatekeeper works great for constraints and basic mutations (currently alpha feature), but I really wish I could write my own rego script for mutations, same as we can do with constraints. I was reading some issues in the gatekeeper repo and looks like the main concern apparently is for circular mutations. I really like the simplicity of mutations with pure CRDs, which is great, but at the same time, it limits ourselves when it comes to complex mutations.

This is what I've been trying to do:

• I need to create a mutation that would add automatically podAffinity/Anti-Affinity to my deployments. As we know, k8s needs a unique label to match the deployment in the node and we can't use a generic label for all deployments. So the only solution I found was to use the opa/k8s-mgnt deployment to only deal with mutations, as they support rego for any operation (mutation or admission).

Is there a way I could do this? I think If we could use some of the deployment metadata as a variable, that could work. For example, let's say that the opa-gatekeeper exposes the metadata.name, which is the deployment/pod/etc name, then we could reference that in our mutation template as a {metadata.name} or something, so gatekeeper would automatically replace that. I think this way, it could allow us to write some more complex mutations, but honestly having the support to write our own rego script would be great.

[maxsmythe]

It sounds like what you're asking for is a way to add an anti-affinity rule to deployments such that none of the deployment's pods are scheduled on the same failure domain? And you want this anti-affinity to be on a per-deployment basis so you need a unique label per deployment?

WRT using deployment metadata (specifically metadata.name), that's something that we were thinking of while designing the external data schema for mutation, per the Mutate metadata.labels.owner to add organization for a user section of the doc...

apiVersion: mutations.gatekeeper.sh/v1alpha1
kind: AssignMetadata
metadata:
  name: k8saadownerlabel
spec:
  match:
    scope: Namespaced
    kinds:
      - apiGroups: [""]
        kinds: ["Namespace"]
  location: "metadata.labels.owner"
  parameters:
    externalData:
      provider: aad-provider
      dataSource: username  # kubernetes user name
    assign:
      value: "unknown"  # fallback value

Is one way this might be done, where instead of externalData there may be a metadata field, or this could be tucked into assign (some of this is up in the air).

TL;DR there should be a way, it hasn't been implemented yet though.

WRT the ability to write custom mutators in Rego...

This is a common request and something that is worth taking seriously. It also needs to be handled carefully as the implications of mutation webhooks are complex and often not obvious.

There are some security concerns that it is easy to fall into, like non-convergence being a sign of potential Turing completeness per:

open-policy-agent/gatekeeper#1388 (comment)

This issue also comments on how such cycles can be subtle and hard to detect, as well as some of the use cases a syntax that always converges unblocks (mainly around the ability to easily combine mutators written by a bunch of different people without worrying about convergence):

open-policy-agent/gatekeeper#1544 (comment)

Not mentioned was the idea of "mutation drift detection", where there could be an audit-like process that queries existing resources and applies mutations if necessary to bring them into conformance with the current set of mutators. Something like that would not be well-behaved with a non-convergent set of mutators.

The subtleties and use cases are also addressed in this comment:

open-policy-agent/gatekeeper#1176 (comment)

Finally, there is the kubernetes best practice that mutation webhooks must be idempotent. This is emphasized in a couple places:

https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#reinvocation-policy
https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#idempotence

Because of the complexities and risk, my hope is that most (or all) use cases that satisfy the idempotence requirement can be expressed using mutators that are convergent-by-design (a requirement for idempotence), so that we can assume idempotence and build features around that as well as lower the cost of administering mutation policies.

This is only a theory, however. There may be some use case that is idempotent but cannot be expressed by a mutator that is convergent-by-design. There may also be some use cases where the user is fine sacrificing idempotence and the features and safety it provides for some operational goal that is more valuable to them, and the feature request is common enough (or cheap enough) to implement and can be implemented in a way that doesn't sacrifice some of these more forward looking goals.

Unfortunately, the only way to know if such feature requests exist is via user feedback, stress-testing the current design via usage, and seeing if the current use-cases can be satisfied in a convergent-by-design way (as I think this one can).

[manelpb]

Thanks @maxsmythe, appreciate the time answering my question.

What would you recommend for my project in short term? Is the opa/K8s-mgnt the only solution?

I understand that folks are still designing/planning how this should be accomplished on the best and safest way possible, wish I could help somehow, but golang is not my strength :(

[maxsmythe]

Thanks @maxsmythe, appreciate the time answering my question.

Absolutely!

What would you recommend for my project in short term? Is the opa/K8s-mgnt the only solution?

I think it's the only current solution. I can try to write a PR next week to add the ability to reference metadata like resource name/namespace name when assigning a value, though I can't guarantee a timeline for merging it. Also, you would need to build from HEAD until we cut a new release

I understand that folks are still designing/planning how this should be accomplished on the best and safest way possible, wish I could help somehow, but golang is not my strength :(

Feedback is extremely helpful and much appreciated :)

[bartmeuris]

Is there an update on this? I'm trying to achieve the same thing with pod topology spread constraints.

[maxsmythe]

Are you asking about the ability to reference resource name/namespace?

That change is live. Documented here

[bartmeuris]

Yes, but in a more complex structure. I would need to add/enforce the topologySpreadConstraint to certain workloads where I'm either not in control of the manifests being pushed, or am using upstream helm charts I preferably don't want to maintain my own fork of. I was hoping to be able to use gatekeeper to enforce this for specific workloads.

This consists of 2 things:

• Ensuring a label is set with the name of the deployment, this can usually be set, and could be added as a pathTest to ensure this label exists.
• Adding a topologySpreadConstraint to the spec.template.spec, which requires adding a more complex structure to a list, in a deployment under spec.template.spec.topologySpreadConstraints:

- maxSkew: 1
  topologyKey: zone
  whenUnsatisfiable: DoNotSchedule
  labelSelector:
    matchLabels:
      # ... add labels here containing name and namespace

I was considering of handling this in 2 or more separate policies, one adding topologySpreadConstraints entry, one for injecting the matchLabels the label, but not sure this could be reliably handled?

[maxsmythe]

I think the two mutator approach should work. Basically:

1. Assign mutator against spec.template.spec.topologySpreadConstraints[topologyKey: "default-topology"].labelSelector.matchLabels.name-label, which sets the value to metadata.name (maybe another mutator for namespace name)

2. Assign mutator that sets spec.template.spec.topologySpreadConstraints[topologyKey: "default-topology"] to [[insert the rest of topology here]]

The only trick is mutator (1) always needs to be called after mutator (2) so that it's not clobbered by mutator (2). Currently you can do this by making sure mutator (2)'s name come before mutator (1)'s name alphabetically (technically this is not part of the API contract but will work, there is some risk of brittleness on upgrade).

There is also a bit of a race condition when the mutators are first applied... there will be a slight time difference between when mutators (1) and (2) are ingested, and in that window objects will only be mutated according to whichever mutators have been ingested, there are thoughts on how this can be addressed, but they haven't been implemented.

This also sounds like:

1. A compelling case for ordered mutators
2. A compelling reason to address the "multiple mutators ingested atomically" case, described here: Mutation: Un-handled use-cases gatekeeper#1348