-
Notifications
You must be signed in to change notification settings - Fork 416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support validating internal list items on list types #342
Comments
@DirectXMan12 @damemi @sttts - Do any of you have any insight into this? Could this be implemented by updating the |
The current solution is to alias the string type and add marker for the alias. We had a discussion before to have a special syntax to apply something for items or additionalPropoerties. This was never implemented or fleshed out AFAIK. Something around the lines of // +k8s:openapi-gen=true
type CRSpec struct {
// +kubebuilder:validation:Items:MinLength=1
Field []string `json:"field"`
} |
While aliasing the string type and adding the marker to the type itself works, doing so requires significant controller-side changes since there's no way to cast between slices of different types. The ability to do something like you've shown above would be a huge improvement. |
It is painful, I understand. @DirectXMan12 can comment on that topic more than I can. |
we've thought about implementing We removed the way it used to work because it was very inconsistent and caused issues when you actually wanted a marker to apply to the field in general and not the item. |
I'd encourage new users to do the type aliasing, but I understand that that's clunky sometimes, and that it'd cause existing users some pain, so figuring out an alternate solution (like |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
I'd like for this issue to stay open since addressing it would be a huge quality of life improvement. Do only maintainers have the ability to use /remove-lifecycle-stale? |
/remove-lifecycle stale |
For what it's worth, I'm running into this now. My CRD is pretty large and will require a great number of type aliases (and associated casts) which I'd really rather not do. Was it decided not to do this or is there just not anyone willing at the moment? |
I think the latter. I took a quick look through the code to try to evaluate the level of effort, but wasn't familiar enough with it to make an accurate guess |
The core developers just don't really have bandwidth atm, and it's not super-high priority. If someone else wants to pick this up, that'd probably work. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
/lifecycle frozen |
This is still painful. |
When using kubernetes-sigs/controller-tools v2 and above, there's a know issue that the validation on intertal list items is no longer supported. See in: kubernetes-sigs/controller-tools#342. This issue is opened 2 years ago and currently frozen(seems will not fix in a short term). We can set a alia of the item type to introduce the validation instead, otherwise, we'll need to remove these validations. This commit gives an example about the effort to set the validation with alia type on the subfunctions for review. Note: there's still many validation of the items in a "StringList" need to implement. Signed-off-by: Yuxing Jiang <Yuxing.Jiang@windriver.com>
Due to the known issue in the upstream package: kubernetes-sigs/controller-tools#342, this commit removes the validation on the service types. Note: this is a temperary work around, need to add these validations back once we validate the fix for the subfunctions. Signed-off-by: Yuxing Jiang <Yuxing.Jiang@windriver.com>
controller-gen does not support validating internal list items on list types, see kubernetes-sigs/controller-tools#342 To add host pattern we used perl hack that is hard to extend to multiple validations. Also #16 added optional tls spec that contains hosts field for which perl hack worked by accident, see #16 (comment) This change replaces perl hack for a go hack and adds max length constraint. Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
We've added a post-processing step that adds validation fields to the generated CRD yaml, see szuecs/routegroup-client#38 |
just for everyones awareness, we'd highly appreciate a change to fix this and its going to benefit the ecosystem a lot more than downstream workarounds. |
Fixes kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
For kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Ok, I've created #897 |
Why not just use a type alias and apply the validation to the alias? That works well and you have a clear I think already people expect |
|
yeah I agree with the type alias, its impractical. We need to find a way to specify that a given validation is for the items of a list and not the list as a whole. |
For kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
For kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Ok, I've created #898 |
For kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
For kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
For kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
For kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
For kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
For kubernetes-sigs#342 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
The issue kubernetes-sigs/controller-tools#342 was fixed and CRD generator now supports validation markers for array items. This change uses items markers and removes hack used to add max length and pattern constraints to hosts. Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
/close PR is merged. Thx @AlexanderYastrebov |
@sbueringer: Closing this issue. In response to this:
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/test-infra repository. |
The issue kubernetes-sigs/controller-tools#342 was fixed and CRD generator now supports validation markers for array items. This change uses items markers and removes hack used to add max length and pattern constraints to hosts. Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Per https://book.kubebuilder.io/reference/markers/crd-validation.html, validating tags like
+kubebuilder:validation:MinLength
andkubebuilder:validation:Pattern
must be specified onstring
fields orstring
types. If one has a field of type[]string
, however, it is impossible to use such tags on the field directly to enforce validation on the individual list items. This forces users to alias the list item type and place the validation on the type itself like so:This leads to undesired consequences like having to export the type so that it can be referenced in controller packages and having to cast list items since there is no easy way to cast
[]NonEmptyString
to[]string
.It used to previously be possible to add the validation tag to the field itself like so:
Doing so, however, would generate the following CRD where the validation was present on both the list itself as well as its items:
Now, the CRD is generated as follows:
It'd be great if the validating tags could be specified on the field itself and result in the following CRD:
Previously, I was using
v0.9.0
of the Operator SDK which usedcontroller-tools
v0.1.11-0.20190411181648-9d55346c2bde
. Now, I'm usingv0.11.0
of the Operator SDK which usescontroller-tools
v0.2.0
The text was updated successfully, but these errors were encountered: