-
Notifications
You must be signed in to change notification settings - Fork 327
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
Add ephemeral containers to policies #191
Add ephemeral containers to policies #191
Conversation
9a18948
to
5600652
Compare
Currently having a look at the best way to implement tests for both init containers and ephemeral containers. |
There was a problem hiding this 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 PR @phillebaba! 🎉
Indeed, TY for the PR! LMK when you think you have the tests sorted out. Rego tests can make it easy to stamp tests out. Adding them to |
@@ -172,6 +172,10 @@ spec: | |||
general_violation[{"msg": msg, "field": "initContainers"}] | |||
} | |||
|
|||
violation[{"msg": msg}] { | |||
general_violation[{"msg": msg, "field": "ephemeralContainers"}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may have one question
It is said Pod resource allocations are immutable, so setting resources is disallowed.
in
https://kubernetes.io/docs/concepts/workloads/pods/ephemeral-containers/#what-is-an-ephemeral-container
So, do we need to add ephemeralContainer check in container limit policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so do the policy contaienrrequests and containerresourceratios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fseldow good catch, you are right. It is not needed so will remove. Going to have a second look to see if there are more of these.
efff6e4
to
0460431
Compare
Signed-off-by: Philip Laine <[email protected]>
0460431
to
eec34f2
Compare
@phillebaba How's the testing looking? |
8e23baa
to
e216525
Compare
Signed-off-by: Philip Laine <[email protected]>
e216525
to
7e16782
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for doing this!
@maxsmythe sorry I got a bit carried away with the testing, but started facing issues as certain rules have multiple violations and other do not. Maybe something to look more into in the future. I added the easiest test cases that I could. I just need to figure out why the integration tests are failing, do you have any insight into this? |
@phillebaba Looks like the kind cluster used in testing is using v1.21 where the I tested upgrading the kind cluster to v1.23 where I think you just can't integration-test this feature until we refactor |
To unblock this PR, perhaps remove the deployments with the ephemeral containers for now? |
The issue with removing the deployments using ephemeral containers is that it is part of the actual test. Could we not just update the kind version? |
@phillebaba even if you increase the kind version, the integration tests still fail because you can't "kubectl apply" a Pod with an ephemeralContainer in it: |
To exempt these ephemeral objects from on-cluster testing, can we rename them to remove the gatekeeper-library/test/bats/test.bats Lines 79 to 107 in 259ad1b
Then they will only be subject to OPA and |
@phillebaba are you able to make the suggested changes above and in #197 so we can get this PR merged soon? Please let us know how we can help. |
This change adds ephemeral containers to all policies that currently check the containers and init containers field. It is kind of a quick solution just to bring all the policies to a level where ephemeral containers do not introduce security concerns.
I have on purpose skipped the required probes policy as it currently only looks at the containers field. I have also only updated one of the mutation samples.
One thing that comes to mind when looking at the different policies is that there are a bunch of different ways to solve this problem. A couple of rules have a method which returns the container data from each field, and others duplicate code at different levels. I feel like it would be good to find a standard. Right now it would be easy to make mistakes and not include one of the containers field. At the same time I can see a situation where one would want to require read only root file systems for the init containers and containers fields, but not the ephemeral containers fields. Might be better to solve these challenges in the future.
Fixes #188