feat: add webhook labels flag to add labels in VPA admission controller webhook #7402

umagnus · 2024-10-16T08:40:19Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

feat: add webhook labels flag to add labels in VPA admission controller webhook
In AKS, VPA can't admit pods in kube-system namespace. Reference to Can admission controller webhooks impact kube-system and internal AKS namespaces?, VPA can add Label: "admissions.enforcer/disabled": "true" in admission controller webhook to impact kube-system namespace. Add a flag to add webhook labels.

Which issue(s) this PR fixes:

Fixes ##7392

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The --webhook-labels flag is added. Users can use webhook-labels flag in admission controller to add labels in vpa-webhook

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2024-10-16T08:40:29Z

Welcome @umagnus!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-10-16T08:40:30Z

Hi @umagnus. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

vertical-pod-autoscaler/pkg/admission-controller/main.go

vertical-pod-autoscaler/pkg/admission-controller/config.go

andyzhangx · 2024-10-16T08:46:34Z

/ok-to-test

adrianmoisey · 2024-10-16T08:49:39Z

Could you also update the documentation?

vertical-pod-autoscaler/pkg/admission-controller/config_test.go

adrianmoisey · 2024-10-16T09:13:40Z

I'm not sure how useful it is, but what about a test for selfRegistration() where the input is invalid?

adrianmoisey · 2024-10-16T09:20:12Z

The "Does this PR introduce a user-facing change?" section of the description should be filled in

umagnus · 2024-10-17T02:24:06Z

I'm not sure how useful it is, but what about a test for selfRegistration() where the input is invalid?

Previous I use klog.Fatal when label input is invalid, but I think the label may not be an error must be exited. Now I use klog.Warning to print a warning message for invalid labels and set empty labels in creating webhook. Also add the ut

adrianmoisey · 2024-10-17T06:23:33Z

Does this PR introduce a user-facing change?
action required: users can use webhook-labels flag in admission controller to add labels in vp

I don't think "action required" is true, since this change is backwards compatible. If a user upgrades they can ignore the flag and it shouldn't change their experience.

umagnus · 2024-10-17T06:58:01Z

Does this PR introduce a user-facing change?
action required: users can use webhook-labels flag in admission controller to add labels in vp
I don't think "action required" is true, since this change is backwards compatible. If a user upgrades they can ignore the flag and it shouldn't change their experience.

Sure, remove it

vertical-pod-autoscaler/pkg/admission-controller/config.go

vertical-pod-autoscaler/pkg/admission-controller/config_test.go

adrianmoisey · 2024-10-18T08:20:32Z

/lgtm

voelzmo · 2024-10-18T09:25:05Z

Hey @umagnus, thanks for the PR! Just to clarify: you actually need VPA to modify Pods in kube-system? Are you deploying anything there, which is under control of VPA? Usually, in managed k8s distros, you want to leave that namespace alone and use your own namespaces for your components, to not interfere with whatever your provider deploys there, right?

I'm asking, because we recently introduced a feature to ignore namespaces (other manages k8s distributions like GKE were also not happy with the VPA applying to Pods in kube-system), such that you can add the option --ignored-vpa-object-namespaces=kube-system to the vpa-recommender, vpa-updater and vpa-admission-webhook deployments. The necessary ignore matchers are automatically added to the VPA's webhook and you don't get bothered by your managed k8s distro's vendor.

umagnus · 2024-10-21T01:59:34Z

Hey @umagnus, thanks for the PR! Just to clarify: you actually need VPA to modify Pods in kube-system? Are you deploying anything there, which is under control of VPA? Usually, in managed k8s distros, you want to leave that namespace alone and use your own namespaces for your components, to not interfere with whatever your provider deploys there, right?

I'm asking, because we recently introduced a feature to ignore namespaces (other manages k8s distributions like GKE were also not happy with the VPA applying to Pods in kube-system), such that you can add the option --ignored-vpa-object-namespaces=kube-system to the vpa-recommender, vpa-updater and vpa-admission-webhook deployments. The necessary ignore matchers are automatically added to the VPA's webhook and you don't get bothered by your managed k8s distro's vendor.

Yes, I want to use VPA to modify Pods in kube-system in AKS, and it need to add a label for vpa-admission-webhook deployments. AKS ignore pods in kube-system by default.

umagnus · 2024-10-22T07:15:01Z

Hi, @adrianmoisey Could you please approve it? Thanks!

adrianmoisey · 2024-10-22T07:16:16Z

Hi, @adrianmoisey Could you please approve it? Thanks!

I can't approve, I can only /lgtm.

umagnus · 2024-10-22T07:20:00Z

Hi, @voelzmo Could you please approve it? Thank you!

voelzmo

Thanks for the PR, just small clarification regarding logging and error behavior.

voelzmo · 2024-10-22T08:28:24Z

vertical-pod-autoscaler/pkg/admission-controller/config.go

@@ -141,9 +141,15 @@ func selfRegistration(clientset kubernetes.Interface, caCert []byte, webHookDela
 			},
 		}
 	}
+	webhookLabelsMap, err := convertLabelsToMap(webHookLabels)
+	if err != nil {
+		klog.Warningf("Unable to parse webhook labels. Creating webhook without labels: %v\n", err)


We're switching to structured logging currently (see #7226 for the admission-controller PR), therefore I'd like this to follow the same pattern. As warnings don't exist in structured logging, this probably should be logged with klog.ErrorS?

I'm also wondering if we should continue with the selfRegistration here or not? Currently this error just goes into the logs and everything still get registered – is this intentional?

I change it to klog.ErrorS. I think use labels is an option but not necessary, so when cx use invalid labels format, selfRegistration will continue and just print a error log.

voelzmo · 2024-10-22T08:37:50Z

vertical-pod-autoscaler/pkg/admission-controller/config_test.go

+	webhookConfigInterface := testClientSet.AdmissionregistrationV1().MutatingWebhookConfigurations()
+	webhookConfig, err := webhookConfigInterface.Get(context.TODO(), webhookConfigName, metav1.GetOptions{})
+
+	assert.NoError(t, err, "expected invalid labels error fetching webhook configuration")


I'm not sure what this assertion is supposed to do. Currently, it asserts that no error is returned, but the message sounds like it should assert that there will be an error returned. What should be the expected behavior? See also my comment above about this.

I think webhook labels format error is an option but not necessary, so it don't need to let webhook register failed. It just need to print an error message in log and use empty labels in registering.

voelzmo · 2024-10-24T11:43:49Z

Thanks for the PR!

/lgtm
/approve

k8s-ci-robot · 2024-10-24T11:44:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umagnus, voelzmo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~vertical-pod-autoscaler/OWNERS~~ [voelzmo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. area/vertical-pod-autoscaler labels Oct 16, 2024

k8s-ci-robot requested review from krzysied and raywainman October 16, 2024 08:40

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 16, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 16, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 16, 2024

umagnus mentioned this pull request Oct 16, 2024

VPA admission controller can't admit pods in kube-system namespace #7392

Closed