controller-tools
controller-tools copied to clipboard
kubernetes-sigs
Support validating internal list items on list types
Per https://book.kubebuilder.io/reference/markers/crd-validation.html, validating tags like +kubebuilder:validation:MinLength and kubebuilder:validation:Pattern must be specified on string fields or string 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:
// +kubebuilder:validation:MinLength=1
type NonEmptyString string
// +k8s:openapi-gen=true
type CRSpec struct {
Field []NonEmptyString `json:"field"`
}
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:
// +k8s:openapi-gen=true
type CRSpec struct {
// +kubebuilder:validation:MinLength=1
Field []string `json:"field"`
}
Doing so, however, would generate the following CRD where the validation was present on both the list itself as well as its items:
field:
items:
minLength: 1
type: string
minLength: 1
type: array
Now, the CRD is generated as follows:
field:
items:
type: string
type: array
It'd be great if the validating tags could be specified on the field itself and result in the following CRD:
field:
items:
minLength: 1
type: string
type: array
Previously, I was using v0.9.0 of the Operator SDK which used controller-tools v0.1.11-0.20190411181648-9d55346c2bde. Now, I'm using v0.11.0 of the Operator SDK which uses controller-tools v0.2.0
@DirectXMan12 @damemi @sttts - Do any of you have any insight into this? Could this be implemented by updating the ApplyToSchema methods to interact with the Items field for the desired validation markers?
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 :items: before. It's not entirely clear how the internals would work beyond literally having a copy of every marker, but that's a discussion for the internals. Someone would need to do a prototype.
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 :items:) would be good.
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale
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?
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
Was it decided not to do this or is there just not anyone willing at the moment?
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.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale
I assume no one is working on this at the moment? I might pick this up if no else plans to.
Came here after searching to see if this feature request had already been created. The workaround to use a type definition should work well for my use case. I don't mind the exporting and type conversion. Thanks for posting it!
We've added a post-processing step that adds validation fields to the generated CRD yaml, see https://github.com/szuecs/routegroup-client/pull/38
We've added a post-processing step that adds validation fields to the generated CRD yaml, see https://github.com/szuecs/routegroup-client/pull/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.
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.
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 MaxLength on the alias, rather than having both MaxLength and MaxItems on a single field which is likely to cause confusion.
I think already people expect MaxLength to act as MaxItems on an array, at least presently they get an error in this case, with the change proposed, they won't get an error, but likely also won't get the schema they intended
Why not just use a type alias and apply the validation to the alias? That works well and you have a clear
MaxLengthon the alias, rather than having bothMaxLengthandMaxItemson a single field which is likely to cause confusion.
https://github.com/kubernetes-sigs/controller-tools/issues/342#issuecomment-542353437
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.
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.
Ok, I've created #898
@sbueringer: Closing this issue.
In response to this:
/close
PR is merged. Thx @AlexanderYastrebov
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.
