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

Enforce container resources without max/min values #185

Merged

Conversation

olegy2008
Copy link
Contributor

@olegy2008 olegy2008 commented Apr 6, 2022

I've created this template to only enforce the presence of resources defined in the constraint without specifying min/max values like in containerrequests and containerlimits.
That's my first time using rego so would appreciate any suggestions to make it look and work better if possible.

@olegy2008 olegy2008 force-pushed the enforce-container-resources branch from 6ecb737 to 769257d Compare April 6, 2022 10:46
@olegy2008
Copy link
Contributor Author

@maxsmythe @ritazh is there any chance to get your review/opinion about this?

@maxsmythe
Copy link
Contributor

Sorry for the slow response :/ I like the idea of this CT!

I haven't had time to look through the tests, but the overall code LGTM.

Any chance this could be updated to also support ephemeral containers, per #191 ?

@olegy2008
Copy link
Contributor Author

Hey @maxsmythe!
I checked ephemeralContainers docs and it says that setting resources for ephemeralContainers is disallowed and ephemeral containers use spare resources already allocated to the pod.

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Thank you for checking WRT the ephemeral container!

The code looks good, I have a couple nits WRT test names and parameter validation.

object: samples/container-must-have-limits-and-requests/example_allowed1.yaml
assertions:
- violations: no
- name: example-disallowed1
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I wonder if we could name these tests to explain what they are testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed tests, but not sure if it's more readable/clear now, do you have any naming conventions?

Copy link
Contributor

Choose a reason for hiding this comment

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

No conventions as yet, since the test function is fairly new, but this is much more clear. Thank you for doing this!

It is recommended that users use the fully-qualified Docker image name (e.g. start with a domain name)
in order to avoid unexpectedly exempting images from an untrusted repository.
type: array
items:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a regex check for the string so that users get an error if they type anything other than "cpu" or "memory"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added enum to validate the values.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

olegy2008 added 2 commits May 8, 2022 17:15
Signed-off-by: Oleg Vorobev <[email protected]>
@maxsmythe
Copy link
Contributor

LGTM

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

@willbeason willbeason merged commit 259ad1b into open-policy-agent:master May 11, 2022
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.

4 participants