Allow marker comments to extend validations for subschemas

[alexzielenski]

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]

/cc @Jefftree @jpbetz

[Jefftree]

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

[jpbetz]

Looks good. A few very minor comments.

[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

[Jefftree]

question: What's the purpose of changing this set of fields to pointers?

[alexzielenski]

It allows us to disambiguate empty from unset. It doesnt affect anything right now but I thought that while I was copying the fields over it'd be better to have this ability.

[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]

Ive tested this extensively as part of kubernetes/kubernetes#123163

To use CEL within these still requires kubernetes/kubernetes#124381 but I think this is ready to merge

/cc @jpbetz
/remove-hold

[Jefftree]

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

[jpbetz]

Sorry if this is obvious, but why are we removing these tags from this test input?

[alexzielenski]

Before this PR enforcement of invalid marker tags on structs was skipped:

https://github.com/kubernetes/kube-openapi/pull/469/files/8d2dde0ae8c4bdb1b57dfa5c0a417383dc2dc0d9#diff-72f8260914152fe9557b05d756ec94854184645e991ff755855628428d6d7a18L177-R295

So tags like exclusiveMinimum and exclusiveMaximum now throw errors for structs. minProperties and maxProperties are allowed on structs, so their removal was likely overzealous. I'll add it back

[alexzielenski]

added back

[jpbetz]

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

[jpbetz]

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

[k8s-ci-robot]

[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