Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude UPDATE operations in constraints for immutable fields #348

Merged
merged 26 commits into from
Sep 26, 2023

Conversation

ordovicia
Copy link
Contributor

@ordovicia ordovicia commented May 23, 2023

What this PR does / why we need it: This PR will fix a problem described in #346, by excluding UPDATE operations in constraint templates for immutable fields.

Which issue(s) does this PR fix (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #346

Special notes for your reviewer: None.

@ordovicia
Copy link
Contributor Author

Hi @apeabody @maxsmythe , could you take a look? Thank you!

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @ordovicia!

A few thoughts/questions:

@maxsmythe
Copy link
Contributor

maxsmythe commented Jul 27, 2023

PATCH is probably irrelevant. From the K8s docs:

Operations is the operations the admission hook cares about - CREATE, UPDATE, DELETE, CONNECT or * for all of those operations and any future admission operations that are added. If '*' is present, the length of the slice must be one. Required.

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#rulewithoperations-v1-admissionregistration-k8s-io

@ordovicia
Copy link
Contributor Author

Thank you for the comments!

Fixed to exclude UPDATE operation only.
Also added tests for constraint templates.
But I could not find how to add UPDATE operation tests for constraints.

@apeabody
Copy link
Contributor

apeabody commented Aug 1, 2023

Thank you for the comments!

Fixed to exclude UPDATE operation only. Also added tests for constraint templates. But I could not find how to add UPDATE operation tests for constraints.

Thanks @ordovicia! You should be able to use a AdmissionReview object in the test suite to include the operation type. For example with K8sPSPAutomountServiceAccountTokenPod you could add a new allowed similar to the existing disallowed but allowed for the UPDATE operation test. Instead of the using the Pod object directly, for object: you would use an AdmissionReview object similar to:

kind: AdmissionReview
apiVersion: admission.k8s.io/v1beta1
request:
  operation: "UPDATE"
  object:
    apiVersion: v1
    kind: Pod
    metadata:
      name: nginx-automountserviceaccounttoken-update-allowed
      labels:
        app: nginx-automountserviceaccounttoken
    spec:
      automountServiceAccountToken: true
      containers:
        - name: nginx
          image: nginx

The idea being this Pod should normally raise a violation, but not if the operation is "UPDATE".

@apeabody apeabody changed the title Exclude UPDATE and PATCH operations in constraints for immutable fields Exclude UPDATE operations in constraints for immutable fields Aug 1, 2023
@ordovicia
Copy link
Contributor Author

Thank you @apeabody !
I added tests for constraints.

@ordovicia ordovicia requested a review from apeabody August 22, 2023 04:33
@ordovicia
Copy link
Contributor Author

Hi @apeabody @maxsmythe , could you take a look again?

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ordovicia!

One question/comment on lib.exclude_update.

Cheers!

src/rego/lib_exclude_update/lib_exclude_update.rego Outdated Show resolved Hide resolved
website/docs/validation/volumes.md Show resolved Hide resolved
@ordovicia ordovicia requested a review from apeabody September 13, 2023 02:34
@ordovicia
Copy link
Contributor Author

Hi @apeabody please merge this if it's OK. Thank you!

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @ordovicia!

Another maintainer will need to review as well.

Co-authored-by: Andrew Peabody <[email protected]>
Signed-off-by: Hidehito Yabuuchi <[email protected]>
Signed-off-by: Hidehito Yabuuchi <[email protected]>
Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks for the fix @ordovicia! 🎉 I updated the PR description to remove patch.

@ritazh ritazh merged commit bd942ad into open-policy-agent:master Sep 26, 2023
15 checks passed
@ordovicia ordovicia deleted the exclude-update branch September 26, 2023 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply constraints for immutable fields only to CREATE operations
4 participants