-
Notifications
You must be signed in to change notification settings - Fork 2
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 a command line option to limit which resources can be applied #101
Add a command line option to limit which resources can be applied #101
Conversation
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.
Can't we deal with CPU and Mem the same way for the filtering, having them by default in the allowedList ?
@@ -69,6 +70,7 @@ var ( | |||
registerWebhook = flag.Bool("register-webhook", true, "If set to true, admission webhook object will be created on start up to register with the API server.") | |||
registerByURL = flag.Bool("register-by-url", false, "If set to true, admission webhook will be registered by URL (webhookAddress:webhookPort) instead of by service name") | |||
vpaObjectNamespace = flag.String("vpa-object-namespace", apiv1.NamespaceAll, "Namespace to search for VPA objects. Empty means all namespaces will be used.") | |||
allowedResources = flag.String("allowed-resources", "", "Comma-separated allow list of resources that can be applied by this controller. Empty means any recommendation can be applied") |
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.
can we default to CPU and Memory ?
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.
Indeed, I was originally thinking avoid changing behavior by treating ""
as allow everything, but that should be the same as setting cpu/memory as the default options, updated
|
||
func filterResourceList(resourceList core.ResourceList, allowedResources []core.ResourceName) core.ResourceList { | ||
if len(allowedResources) == 0 { | ||
return resourceList |
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.
I would have expected that an empty allowedResources would result in an empty output: everything filtered out
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.
Yep, removed this check and now the entire list is filtered out if allowedResources is empty
a56945e
to
94a6a13
Compare
94a6a13
to
c3213da
Compare
LGTM, ready for final review ? |
After revisiting a bit, I decided this was better to implement as a separate |
Add the ability to prevent all VPA recommendations from being applied to a target pod. This adds a new command line optoin
--allowedResources
that can be set likeallowedResources=cpu,memory
to control which resources can be applied to pods.This should prevent an issue where values we're setting in our VPA objects for non-standard resources are attempting to be applied, but are not advertised extended resources, so they cannot be set.