Start moving vmi verifications to the CRD #11156
Conversation
kubevirt-bot
added
release-note
| @@ -146,16 +146,6 @@ func ValidateVirtualMachineInstanceSpec(field *k8sfield.Path, spec *v1.VirtualMa | |||
| volumeNameMap := make(map[string]*v1.Volume) | |||
| networkNameMap := make(map[string]*v1.Network) | |||
|
|
|||
| maxNumberOfDisksExceeded := len(spec.Domain.Devices.Disks) > arrayLenMax | |||
There was a problem hiding this comment.
seems like you can delete const arrayLenMax right?
The CRD language supports many validations. Using this verification from the CRD instead of from the webhook code has few main advantages: 1. The K8s API-server verifies the CR instead of the webhook, reducing load from the webhook. 2. Transparency: the validations are visible to the user as part of the CRD. 3. remove code duplication. For example, same validation in create and update webhook. For example, the validations in this PR only implemented in the create validation but not in the update validation. Adding the validation to the CRD promises that both cases are covered. This PR adds maxItems for several arrays in VMI and VMI preset: * `Volumes` array * `Networks` array * `AccessCredentials` array * `Disks` array * `Interfaces` array Then the PR removes the validation from the vmi create validator, and the relevant unit test. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
nunnatsa
force-pushed
the
poc-start-crd-validation
branch
from
b487fc3
to
2c2c86b
Compare
|
/retest |
|
/cc alaypatel07 |
|
Any reason you left the "POC" prefix on the PR and commit? |
Because it's very preliminary. The amount of changes is tiny, relatively to all the places we can use CRD verification instead of webhook logic. I want to see which parts of the source are affected and so on. |
|
/test all |
nunnatsa
changed the title
|
I like this direction. In the past there were limitations of why the CRD based validation was not working for KubeVirt. Do we have an understanding of the historic reasons? |
Not really. What can go wrong? Asking because we're using standard CRD fields for the verification.
Most of the logic is more complex than length of lists. I guess some of it can be implemented with CEL, and some of it will have to stay as code in the webhook. However, CEL is only GA in K8s 1.29, so we can't use it while we still support 1.27 & 1.28 |
|
/retest-required |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acardace 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:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
|
Required labels detected, running phase 2 presubmits: |
So how do we justify this split where we do one thing in multiple places? Is the server validation enabled by default already in Kubernetes? I remember this was only client-side a while ago. Do we know if this has a performance penalty or any other constraints? @fabiand Have your questions been answered? |
|
@xpivarc I have a counter question, why to betray CRD validation by disabling server validation in k8s? why the CRD validation is allowed at all if it may introduce severe performance degradation? |
A while back when I was working with this, the server validation did not exist or it was disabled. That is why I am asking if all supported Kubernetes versions have the server-side validation GA-ed and on by default. This is what I am missing in the PR description or the comments.
Any work/feature has an associated cost, I am simply asking what is the cost. Note I am also asking for general disadvantages. |
|
It's very hard to understand from K8s documentation, but I think that these verification are already supported. The "next level", the CEL verification, is only GA in 1.29, so we can't use it. As for the performance question, this is very simple - less logic == better performance. This PR is only the start. we can remove meaningful amount of verification logic by using CRD validation (https://book.kubebuilder.io/reference/markers/crd-validation) |
|
The doc here says:
As far as I understand this (bit crypt) text, is that OpenAPI v3 schemas validation can always be used, CEL can be used only if the feature gate is set (1.27-1.28) or from 1.29, and then you can also use webhook. |
|
I was under the impression this has been verified. If the validation is not occurring, we need to revert this change. UPDATE: Or add to the webhook a general check that validates the schema based on the CRD. I think that is possible. |
|
Here is the same segment from 1.26 documentation (before the introduction of CEL): https://v1-26.docs.kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#specifying-a-structural-schema There is no condition here, like a feature gate or other configuration. It can just been used. |
|
See ServerSideFieldValidation.
Less is more, but in this case, we move one thing from one place to another. This often doesn't mean it is more performant. It can be less, same, or more performant. Did we do any experiments?
Issue or design doc outlining the path forward, pros and cons would be much appreciated. |
|
I think this is the relevant feature in our case: https://kubernetes.io/blog/2023/04/24/openapi-v3-field-validation-ga/ But still, it needs verification to make sure we are good. |
|
Running on kubevirtci cluster with KUBEVIRT_PROVIDER=1.28 I did this: I added fake pattern validation to the network.name field: name:
description: 'Network name. Must be a DNS_LABEL and unique ...'
pattern: ^[a-fA-F]+$
type: stringThen I created a vm with kubectl v1.24.0, with This is the output. The error is clearly coming from the api-server: By the way, this is what I get from a new kubectl as well. |
|
As for justification, my view is a bit different. I think we need to justify any custom validation in the webhook, if it can be done by what is now the kubernetes way. And this is something I can't justify. |
What this PR does
The CRD language supports many validations. Using this verification from the CRD instead of from the webhook code has few main advantages:
This PR adds maxItems for several arrays in VMI and VMI preset:
VolumesarrayNetworksarrayAccessCredentialsarrayDisksarrayInterfacesarrayThen the PR removes the validation from the vmi create validator, and the relevant unit test.
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note