Exclude kube-system namespace from seed VPA webhook

[andrerun]

How to categorize this PR?
/area auto-scaling
/kind improvement

What this PR does / why we need it:
Before this PR, Gardener uses the same mutating webhook for VPA on shoots and seeds. In result, seed VPA performs admission control to the seed's kube-system namespace. This has no benefits, as seed VPA has no targets in that namespace, and can only cause problems. Specifically, it fails a sanity check currently implemented by GKE and leads to GKE issuing warning messages.

This PR excludes the kube-system namespace from the seed VPA webhook.

Release note:

The `kube-system` namespace is now excluded by the seed VPA webhook.

[gardener-prow]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[gardener-prow]

@andrerun: The label(s) kind/improvement cannot be applied, because the repository doesn't have them.

In response to this:

How to categorize this PR?
/area auto-scaling
/kind improvement

What this PR does / why we need it:
Before this PR, Gardener uses the same mutating webhook for VPA on shoots and seeds. In result, seed VPA performs admission control to the seed's kube-system namespace. This has no benefits, as seed VPA has no targets in that namespace, and can only cause problems. Specifically, it fails a sanity check currently implemented by GKE and leads to GKE issuing warning messages.

This PR excludes the kube-system namespace from the seed VPA webhook.

Release note:

The `kube-system` namespace is now excluded by the seed VPA webhook.

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.

[ialidzhikov]

/assign

[vpnachev]

The key should be kubernetes.io/metadata.name, and there are constants that can be used here.

Suggested change

	{Key: "name", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"kube-system"}},
	{Key: corev1.LabelMetadataName, Operator: metav1.LabelSelectorOpNotIn, Values: []string{metav1.NamespaceSystem}},

[andrerun]

Done. Thanks for pointing me to the specific constant definitions!

[ialidzhikov]

/kind enhancement

[vpnachev]

I am just wondering is this the right approach? If a GKE cluster has VPA enabled then isn't it better to not install VPA via Gardener, i.e. set seed.spec.verticalPodAutoscaler.enabled=false?

The feature to install VPA on seeds was implemented for k8s clusters that are not equipped with VPA by the cluster vendor. With this change, such clusters will get the VPA from Gardener, however any component in the kube-system namespace will not be able to use it.

[gardener-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ialidzhikov. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[ialidzhikov]

I am just wondering is this the right approach? If a GKE cluster has VPA enabled then isn't it better to not install VPA via Gardener, i.e. set seed.spec.verticalPodAutoscaler.enabled=false?

Is this related to this PR? When VPA is enabled for a Seed gardenlet installs the VPA CRDs and I think we can assume that there are no other VPAs in the cluster except the ones Gardener creates.

The feature to install VPA on seeds was implemented for k8s clusters that are not equipped with VPA by the cluster vendor. With this change, such clusters will get the VPA from Gardener, however any component in the kube-system namespace will not be able to use it.

I am not sure this is something we have to consider or support. As I shared gardenlet is the component that installs the VPA CRDs on Seed bootstrap. So, the case you describe could only happen if GKE or any other K8s cluster provider for some reason initially does not create a VPA in kube-system, then gardenlet installs the CRDs and afterwards it decides to create a VPA in the kube-system namespace.

cc @voelzmo

[vpnachev]

I am just wondering is this the right approach? If a GKE cluster has VPA enabled then isn't it better to not install VPA via Gardener, i.e. set seed.spec.verticalPodAutoscaler.enabled=false?

Is this related to this PR? When VPA is enabled for a Seed gardenlet installs the VPA CRDs and I think we can assume that there are no other VPAs in the cluster except the ones Gardener creates.

The questions should be "Does Gardener have to install VPA controllers, CRDs, admission webhooks, etc. in the seeds that have VPA installed and maintained by the vendor?" My understanding is "no" as two different instances of controllers are acting on the same set of resources which leads to conflicts. So, for GKE clusters the landscape administrator must instruct Gardener to not install the VPA controllers, CRDs, webhooks, etc. in favor of the ones managed by Google. Alternatively, it should be explored if GKE can be configured to not enable the VPA for the cluster and then Gardener can install its own version of the controller.

In both cases, I think the change in this PR is not needed.

[ialidzhikov]

We had a short sync with @vpnachev and we found killer arguments against this PR:

• I wrongly assumed that we don't deploy VPAs in the kube-system Namespace of a Seed. We found that there is an internal component (image signatures' verification) that deploys a VPA in the kube-system Namespace of Seed. For such a component, this PR would cause troubles as vpa-recommender/vpa-updated will evict the Pod to have the recommendations applied but the webhook will never apply the recommendations.
• According to @vpnachev the webhook is not really problematic:

- admissionReviewVersions:
  - v1
  clientConfig:
    caBundle: <omitted>
    service:
      name: vpa-webhook
      namespace: garden
      port: 443
  failurePolicy: Ignore
  matchPolicy: Exact
  name: vpa.k8s.io
  namespaceSelector: {}
  objectSelector: {}
  reinvocationPolicy: IfNeeded
  rules:
  - apiGroups:
    - ""
    apiVersions:
    - v1
    operations:
    - CREATE
    resources:
    - pods
    scope: '*'
  - apiGroups:
    - autoscaling.k8s.io
    apiVersions:
    - '*'
    operations:
    - CREATE
    - UPDATE
    resources:
    - verticalpodautoscalers
    scope: '*'
  sideEffects: None
  timeoutSeconds: 10

Due to failurePolicy: Ignore and the low timeout (10s) the Seed cluster's API server should not fail the request even if the webhook is unavailable.
GKE docs for this warning - they don't check the failurePolicy of the webhook.

/hold

Any other opinions? Otherwise I would suggest to close this PR. If the warning in GKE is problem, we should rather open a ticket to them.

[vpnachev]

there is an internal component (image signatures' verification)

Actually, it is public component:

https://github.com/gardener/gardener-extension-shoot-lakom-service/blob/7bcc4d901171721712e2cb112c1d5a5d77e642ba/pkg/controller/seed/reconciler.go#L426-L429

[voelzmo]

If the warning in GKE is problem, we should rather open a ticket to them.

This was also discussed in the upstream VPA issue, with the same line of arguments: GKE shouldn't care about a webhook in kube-system, as long as failurePolicy: ignore is set.

And if we do put VPAs into kube-system, there is no way that we can make this change, as Ismail pointed out. So I guess let's close it for now and we can try to talk to our GKE contacts about this.

[ialidzhikov]

/close

[gardener-prow]

@ialidzhikov: Closed this PR.

In response to this:

/close

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.