🌱 Add basic validation for time duration #11257
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
This PR is currently missing an area label, which is used to identify the modified component when generating release notes. Area labels can be added by org members by writing Please see the labels list for possible areas. 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. |
k8s-ci-robot
added
do-not-merge/needs-area
|
Hi @Dhairya-Arora01. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Basic validation for time duration can be done using kubebuilder markers to match the pattern "^(([1-9][0-9]*(\\.[0-9]+)?|0?\\.[0-9]+)(ns|us|µs|ms|s|m|h))+$", Signed-off-by: Dhairya Arora <[email protected]>
Dhairya-Arora01
force-pushed
the
dhairya/durationValidation
branch
from
72a0a7c to
42e0fe9
Compare
k8s-ci-robot
added
size/S
Also now I think, this validation should be everywhere we use time.Duration type. wdyt? |
| // +kubebuilder:validation:Pattern="^(([1-9][0-9]*(\\.[0-9]+)?|0?\\.[0-9]+)(ns|us|µs|ms|s|m|h))+$" | ||
| // +kubebuilder:validation:Type:=string |
There was a problem hiding this comment.
If you were to use CEL here, you could create a custom error message which may be more helpful to end users, rather than just printing the regex as will happen when this fails, WDYT?
CEL pattern validation has been around since K8s 1.25 (beta originally) so I think all supported K8s versions CAPI targets should handle this
There was a problem hiding this comment.
1.28 is the min supported mgmt cluster version in CAPI 1.9.
Just have to make sure that whatever we do in CEL works with 1.28 (the Kubernetes CEL library is still adding new stuff with recent Kubernetes versions)
There was a problem hiding this comment.
how about this validation rule:
// +kubebuilder:validation:XValidation:rule="self.matches('^(([1-9][0-9]*(\\.[0-9]+)?|0?\\.[0-9]+)(ns|us|µs|ms|s|m|h))+$')",message="Invalid duration format. Must be a positive number followed by a unit (ns, us, µs, ms, s, m, h). Examples: 300ms, 2h45m, 1.5h"
this supports the error message too, but the pattern matching marker does not!
|
Should we have a discussion before going down the path of introducing a mix of open API and go validations? /hold |
fabriziopandini
added
the
do-not-merge/hold
|
@fabriziopandini we should definitely have this discussion and try and come up with a plan forward, my 0.02 below. In general, I think in the past, OpenAPI validation hasn't been up to scratch for the majority of our needs in terms of API validation. Modern CRD validation however, is much more powerful, and I think as a community we should start transitioning towards it more and more. From an end user perspective, what we need to achieve is validation that presents at admission time. That could be implemented either as a webhook, or through openapi. In terms of validating the end user experience, a test suite that executes envtest and applies resources and inspects output errors should allow us to test both webhook validations and openapi at the same time. Once we have this test suite in place, we could start moving validations from the webhook to the openapi validation to move more and more validation in-tree and reduce the usage of the webhook. This means less for us to maintain, and, when we can eventually use match conditions, we could even get to a point where the webhook is only called explicitly when it is needed. Which would further reduce usage of the webhook, and improve the runtime performance of our APIs, and hopefully improve the ability to scale management clusters. OpenAPI validation inside the CRD schema can only look at the object we have in question, so anything that relies on validating based on fields from other CRs, requires something more. This can either be a webhook, or validating admission policy which can use params as input from other resources. This would be another way that we could reduce webhook validations by providing a set of VAPs that are installed alongside CRDs, and again, this has the benefit of being in-tree validation. The other benefit of openapi based validation that I can think of is that it means that the CRD validations are at the point where the fields are defined. It's very easy to review the field context as a whole, you can see the godoc, and all of the validations applied to it in one small piece of code. This should help with comprehension of how the field behaves, and, IMO, helps us to improve the documentation. If I see a CEL rule that's not explained in the godoc, its easy to point out that this should be explained in the docs. If it's in a webhook, I'm less likely to pick up that it's not documented. There are of course trade offs to consider, primarily for us, I think, is the minimum supported version of Kube. This validation has been and still is evolving. Any API testing we do, must be against our lowest supported version, and therefore, some of the newer features of CRD validation we may not be able to use just yet, meaning this journey, if we choose to embark upon it, might take some time. |
|
@JoelSpeed I agree with most of your points. I just want to make sure that we making this to happen in an organic way, taking care of testing, of not breaking where we are using defaulting/validation logic outside of the webhooks, etc. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
Alrighty. Sorry for the delay, but I think we settled this debate now in: #6398 (comment) Min supported mgmt cluster version in CAPI v1.10 will be Kubernetes v1.29 This means we can rely on CEL (stable since v1.29), we can't rely on CRDValidationRatcheting (not stable as of now).
If we don't want to deny on pre-existing invalid values, this means we have to use CEL with a transition rule (#11257). In addition to that CEL allows us better error messages anyway. The main effort will be writing the transition rule correctly and adding envtest based unit test coverage @JoelSpeed Did I miss anything? :) |
k8s-ci-robot
added
ok-to-test
|
@Dhairya-Arora01: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
I think you captured it. In the future, we probably want to move to |
Is that this point?
Is the idea behind that, that other clients that read the field values - apart from our controllers - would have to implement go-compatible parsing of duration values? |
Yep, exactly. So in theory, anyone who works with this API and uses Go, is fine, as they'd be able to use the Go duration parsing to work with the field value. But if they happen to use another language, eg Python, then they would have to work out how to parse the duration string in a compatible way, which is non-trivial. So by removing the need to understand the units within the string (h|m|s|ns|...), you can make it easier for non-Go clients In this particular use case, I would expect all of these timeouts to be in the order of seconds, so it shouldn't be too hard of a lift to be able to make that change |
|
Yup agree. I think we also ~ only have 4 fields with Duration (duplicated in a few places):
Fine from my side to change them to *Seconds integer in v1beta2. I think then I wouldn't invest additional effort now to implement new validation for Duration. @fabriziopandini @chrischdi Fine from your side as well? @JoelSpeed Would be probably also something for KAL? |
|
Already tracking in JoelSpeed/kal#24, but not yet implemented. Should be pretty easy given the pattern established in other |
Fine from my side too :-) |
Basic validation for time duration can be done using kubebuilder markers to match the pattern "^(([1-9][0-9]*(\.[0-9]+)?|0?\.[0-9]+)(ns|us|µs|ms|s|m|h))+$",
What this PR does / why we need it: Basic validation for time duration can be done using kubebuilder markers to match the pattern "^(([1-9][0-9]*(\.[0-9]+)?|0?\.[0-9]+)(ns|us|µs|ms|s|m|h))+$"
Examples of valid matches:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #7104