kube-openapi icon indicating copy to clipboard operation
kube-openapi copied to clipboard

Allow marker comments to extend validations for subschemas

Open alexzielenski opened this issue 1 year ago • 5 comments
trafficstars

This extends current marker syntax to support extending the subschemas of a struct inside items, properties, or additionalProperties. This is useful in cases like declarative validation for native types where we have many shared types whose validations change at point-of-use, so it does not make sense to annotate the type. And using an alias is too disruptive.

Example:

package foo

// +k8s:openapi-gen=true
// +k8s:validation:properties:field:maxLength=10
// +k8s:validation:properties:aliasMap:additionalProperties:pattern>^foo$
type Blah struct {
	// +k8s:validation:items:cel[0]:rule="self.length() % 2 == 0"
        // +k8s:validation:items:cel[0]:message="length must be even"
	Field MyAlias `json:"field"`

	// +k8s:validation:additionalProperties:maxLength=10
	AliasMap MyAliasMap `json:"aliasMap"`
}
		
type MyAliasMap map[string]MyAlias
type MyAlias []string

In the above example we can override subschemas at multiple different levels, with type checking supported.

This yields schema:

type: object
allOf:
    properties:
        field: 
            items: {maxLength: 10}
        aliasMap: {pattern: ^foo$}
properties:
    field:
        type: array
        items:
            type: string
        allOf:
            items:
                x-kubernetes-validations:
                - rule: self.length() % 2 == 0
                  message: length must be even
    aliasMap:
        additionalProperties: {type: string}
        allOf:
            additionalProperties:
                maxLength: 10

The value validations are placed in an allOf inside the object they were attached to and override subproperties. New structure is not allowed to be placed in these tags, only validations. We validate that any named properties are also in the structure of the Go type.

alexzielenski avatar Apr 12 '24 19:04 alexzielenski

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski

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 /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Apr 12 '24 19:04 k8s-ci-robot

/cc @Jefftree @jpbetz

alexzielenski avatar Apr 12 '24 19:04 alexzielenski

I feel like there are quite a few implicit changes here, would you be able to separate them out into another PR/commit to explain the relevance? The changes move the codebase in a good direction but I'm a bit confused about the interdependency between them and the PR description goal.

OTOH

  • a set of fields being transformed into ptrs
  • deterministic output
  • validations such as minItems/etc should only be attached corresponding types

I split the deterministic output change into a separate commit (no test changes, since theres only 1 possible element so it is more of a future-proofing change)

I also removed the MinProperties/MaxProperties behavior change, so diff is much smaller.

ready for another pass

alexzielenski avatar Apr 16 '24 22:04 alexzielenski

/hold

@jpbetz While trying to use this I've ran into the problem that CEL expressions inside allOf are not validated, since it is not part of the "structural" portion of the schema. Is this something that will ever change in the future? I'm also curious why this is the case, since CEL expressions are definitely value validations and don't impact schema structure unlike other X-extensions

alexzielenski avatar Apr 17 '24 20:04 alexzielenski

Ive tested this extensively as part of https://github.com/kubernetes/kubernetes/pull/123163

To use CEL within these still requires https://github.com/kubernetes/kubernetes/pull/124381 but I think this is ready to merge

/cc @jpbetz /remove-hold

alexzielenski avatar May 07 '24 01:05 alexzielenski

lgtm from me, I'll leave lgtm with @jpbetz if he wants another pass

Jefftree avatar May 21 '24 08:05 Jefftree

One question about the removed test cases then this looks ready to merge.

jpbetz avatar May 21 '24 18:05 jpbetz

/approve /lgtm Leaning on @Jefftree's review here, but it all looked reasonable to me.

jpbetz avatar May 21 '24 19:05 jpbetz

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, jpbetz

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:
  • ~~OWNERS~~ [alexzielenski,jpbetz]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar May 21 '24 19:05 k8s-ci-robot