search

kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.5k stars 1.3k forks source link

Implement validation for the KCP scale sub-resource #5466

Closed sbueringer closed 2 years ago

sbueringer commented 2 years ago

User Story

As a user I would like to be blocked when I try to scale to an invalid replica count of KCP via kubectl scale.

Detailed Description

This is already validated during update (e.g. kubectl edit), but the webhook is not called during kubectl scale as that uses the /scale sub-resource.

There was a previous implementation which was rolled back, xrefs:

Anything else you would like to add:

I think we could implement this by adding an additional validating webhook via the controller-runtime API shown here: https://github.com/kubernetes-sigs/controller-runtime/blob/master/examples/builtins/validatingwebhook.go#L38

So essentially we would have a new webhook which is only registered on the kcp/scale sub-resource. The webhook would decode the Scale resource and then validate the replicas (with the same logic as in the regular webhook).

It would be nice to have test coverage. I think it's not possible by "just" using testenv as that introduces a cyclic dependency between the KCP api package and the internal/envtest package. Maybe we already have an e2e test which does scale and we can switch to use the /scale subresource there

/kind feature /area control-plane

sbueringer commented 2 years ago

/help

k8s-ci-robot commented 2 years ago

@sbueringer: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/5466): >/help Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
dharmjit commented 2 years ago

Hi @sbueringer, I can pick this up. Let me know if it's okay.

sbueringer commented 2 years ago

@dharmjit Sounds great! Feel free to ping me if more context / background information would be helpful.

sbueringer commented 2 years ago

Just to be sure. @vincepri sounds the proposed solution okay to you?

So essentially we would have a new webhook which is only registered on the kcp/scale sub-resource. The webhook would decode the Scale resource and then validate the replicas (with the same logic as in the regular webhook).

dharmjit commented 2 years ago

/assign

vincepri commented 2 years ago

/milestone v1.1

vincepri commented 2 years ago

let's also backport

dharmjit commented 2 years ago

Hi @sbueringer, Submitted this Draft #5697 PR, Could you please let me know how could I backport this for v1alph4 and to start with unit tests/integration tests for the same. I had manually tested it

$ k scale kubeadmcontrolplane capi-quickstart-control-plane --replicas=0
Error from server (replicas cannot be zero): admission webhook "validation-scale.kubeadmcontrolplane.controlplane.cluster.x-k8s.io" denied the request: replicas cannot be zero

$ k scale kubeadmcontrolplane capi-quickstart-control-plane --replicas=2
Error from server (replicas cannot be an even number when using managed etcd): admission webhook "validation-scale.kubeadmcontrolplane.controlplane.cluster.x-k8s.io" denied the request: replicas cannot be an even number when using managed etcd

$ k scale kubeadmcontrolplane capi-quickstart-control-plane --replicas=3
kubeadmcontrolplane.controlplane.cluster.x-k8s.io/capi-quickstart-control-plane scaled
sbueringer commented 2 years ago

@dharmjit Sure, let's continue on the PR to avoid two parallel discussions.