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

Apply constraints for immutable fields only to CREATE operations #346

Closed
ordovicia opened this issue May 22, 2023 · 10 comments · Fixed by #348
Closed

Apply constraints for immutable fields only to CREATE operations #346

ordovicia opened this issue May 22, 2023 · 10 comments · Fixed by #348

Comments

@ordovicia
Copy link
Contributor

Problem

Constraint templates for immutable fields (e.g. K8sPSPHostFilesystem validates pods' volumes) are enforced for UPDATE operations when the Gatekeeper's validating admission webhook is configured to be applied to UPDATE operation.
This can cause the following situation:

  1. Assume that a K8sPSPHostFilesystem constraint with allowedHostPaths: [{ pathPrefix: "/app" }] is on a cluster
  2. A user creates a job, which creates a pod with /app host path volume
  3. An administrator removes /app path prefix from allowedHostPaths of the K8sPSPHostFilesystem constraint
  4. The user deletes the job, which in turn makes the job controller tries to updates the pod's finalizers
  5. The pod's finalizers cannot be updated because the pod has /app host path volume and it now voilates the K8sPSPHostFilesystem constraint
  6. The /app host path volume also cannot be removed from the pod, because pods' volume config is immutable.
  7. As a result, the pod stuck in Terminating phase

Possible solution 1

Configuring the Gatekeeper's validating webhook to be applied only to CREATE operation.

This solution cannot be adopted if a cluster has constrains for mutable fields (e.g. K8sRequiredLabels).

Possible soultion 2

Modifying constraints templates for immutable fields to be applied only to CREATE operation.

This solution can be implemented by adding a input.review.operation condition to constraint definitions written in Rego.

What do you think?

@apeabody
Copy link
Contributor

Hi @ordovicia - Thanks for opening an issue!

Rather than limiting input.review.operation to CREATE operation, I would instead suggest excluding just UPDATE for the least template/violation blocks possible. That will also permits other operations, particularly audit, to work as intended.

@ordovicia
Copy link
Contributor Author

Thank you for your swift comment!
Excluding UPDATE operation in the minimum range seems good.
I am going to create a PR on that plan.

@maxsmythe
Copy link
Contributor

Related issue (which reaches no conclusions):

open-policy-agent/gatekeeper#1520

@stale
Copy link

stale bot commented Jul 22, 2023

This issue/PR 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.

@stale stale bot added the stale label Jul 22, 2023
@ordovicia
Copy link
Contributor Author

I created PR #348 that would fix this issue.

@stale stale bot removed the stale label Jul 25, 2023
@maxsmythe
Copy link
Contributor

Sorry for the delay in reviewing. @apeabody does the PR LGTY?

@maxsmythe
Copy link
Contributor

maxsmythe commented Jul 26, 2023

It would also be good to have a general solution to the "need to remove a finalizer" problem (IMO not a pre-requisite for this PR).

@apeabody
Copy link
Contributor

Sorry for the delay in reviewing. @apeabody does the PR LGTY?

Thanks @maxsmythe! The basics look good, I just had a question on the inclusion of "PATCH", and we should probably include template level test coverage of the new behavior. #348 (review)

@stale
Copy link

stale bot commented Sep 24, 2023

This issue/PR 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.

@stale stale bot added the stale label Sep 24, 2023
@ordovicia
Copy link
Contributor Author

#348 will fix this issue.

@stale stale bot removed the stale label Sep 25, 2023
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 a pull request may close this issue.

3 participants