open-policy-agent / gatekeeper

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

Attribute matching for mutation pathTests #3450

Open skaven81 opened 4 months ago

skaven81 commented 4 months ago

Describe the solution you'd like It is currently not possible to limit the application of a Gatekeeper mutator to only instances of a resource that have certain attributes set. A prime example of this would be creating an Assign mutator that sets spec.allocateLoadBalancerNodePorts=false for any LoadBalancer Services that are created. The match spec can only be as granular as matching v1.Service resources, so it will apply to all of ClusterIP, NodePort, and LoadBalancer type Services. And unfortunately the Kubernetes API refuses to allow spec.allocateLoadBalancerNodePorts to be present in the Service spec if spec.type!=LoadBalancer. Thus, we need a pathTest that can check for spec.type == LoadBalancer to gate the application of the mutation. The existing functionality of subPath with MustExist or MustNotExist does not work, as spec.type is not a prefix of spec.allocateLoadBalancerNodePorts. And even if subPath was possible to use in this case, the two existing operators MustExist and MustNotExist are insufficient to limit application to just LoadBalancer Services.

One way this could be implemented is with an additional type of pathTest that can check for arbitrary values in the review object:

parameters:
  pathTests:
  - subPath: "spec.containers[name: foo]"
    condition: MustExist
  - subPath: "spec.containers[name: foo].securityContext.capabilities"
    condition: MustNotExist
  - pathMatch: spec.type
    operator: In
    values: [ LoadBalancer ]

Adding a new pathMatch behavior to pathTests would allow for gating the activation of the mutator. Unlike subPath behavior, the path designators in pathMatch don't have to be prefixes of the target attribute path. The purpose of pathMatch is to check for specific attribute values, not the presence or absence of a path. They can be structured similarly to matchExpressions to make construction familiar, and would thus implement the same In, NotIn, Exists and DoesNotExist operators that matchExpressions do.

With this implementation in place, it would be possible to create the Assign mutator I need:

apiVersion: mutations.gatekeeper.sh/v1
kind: Assign
metadata:
  name: loadbalancer-nodeports
spec:
  applyTo:
  - groups: [""]
    kinds: [ Service ]
    versions: [ v1 ]
  match:
    kinds:
    - apiGroups: [ "" ]
      kinds: [ Service ]
  location: spec.allocateLoadBalancerNodePorts
  parameters:
    assign:
      value: false
    pathTests:
    - pathMatch: spec.type
      operator: In
      values: [ LoadBalancer ]
    - subPath: spec.allocateLoadBalancerNodePorts
      condition: MustNotExist

Anything else you would like to add:

I can see how implementing this functionality under match might be a better option, as the whole point is to determine whether or not the object under review is even eligible for mutation. I'd be equally happy with an implementation in match or pathTests.

Environment:

pikehuang commented 4 months ago

oh, I met the same question: I use assign mutator to add iptable rules to pod in create phase. however for hostnetwork pod, the iptable rule in pod would take effect in node scope, thus leading to other pod in this node works uncorrectly.

spandan541 commented 4 months ago

I have faced similar troubles with adding an annotation to LoadBalancer services - https://github.com/orgs/open-policy-agent/discussions/457

I agree with @skaven81, there is a need to support pathMatch to test the values of a field or match resources to narrow down resources eligible for a mutation.

As far as I know, the two existing operators MustExist and MustNotExist are not capable enough to filter out Loadbalancer Services for applying mutations.

From #1548 by @maxsmythe , I understand that testing values of a field (assignIf) was supported earlier but was removed later on due to concerns of circular mutations. But, in certain cases like these it would be a helpful feature to have.

I would love to discuss about how we can add such a feature while continuing to maintaining idempotent & linear mutations. Looking forward to hearing from the OPA Gatekeeper team.

stale[bot] commented 2 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.

skaven81 commented 1 month ago

not stale

JaydipGabani commented 1 month ago

For anyone visiting this issue, a workaround could be -

  1. Create Mutation:
    • Use match.labelSelector (or other relevant selectors) to identify the resources for mutation.
    • Exclude any subpath checks.
  2. Validation Policy:
    • Ensure that resources not intended for mutation do not satisfy the "matching criteria" (e.g., labelSelector or other selectors).
    • Confirm that resources targeted for mutation have the appropriate field values as per the defined mutation criteria.