Merge existing and newly generated CRD OpenAPIv3 validation blocks

[estroz]

Whenever I modify a type, or add/remove/update +kubebuilder:validation: directives from api's in pkg/apis/<group>/<version>/*.go, a CRD is generated with a validation block containing these changes. However if I add some validation field in a CRD that exists on-disk for an existing api, that field should be preserved in certain situations. Example:

On-disk CRD:

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
...
spec:
  validation:
    openAPIV3Schema:
      properties:
        apiVersion:
          type: string
...
        spec:
          properties:
            size:
              format: int32
              type: integer
            test:
              type: string
          required:
          - size
          type: object
...

Then write a value to some field manually, ex. spec.validation.openAPIV3Shema.properties.spec.properties.size.title = "foo bar".

Desired CRD after generation:

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
...
spec:
  validation:
    openAPIV3Schema:
      properties:
        apiVersion:
          type: string
...
        spec:
          properties:
            size:
              format: int32
              type: integer
              title: foo bar # << added manually
            test:
              type: string
          required:
          - size
          type: object
...

As far as I can tell, the CRD generator will overwrite the title field, even though there is no +kubebuilder:validation:Title annotation according to the kubebuilder book. Is it possible to support persisting fields added manually, i.e. merge old and new CRD's, if:

• There is no +kubebuilder:validation: directive for that field
• The CRD generator did not generate a value for that field

[hasbro17]

/cc @mengqiy @DirectXMan12 @droot
Any idea if it's possible to preserve manually added validation fields?

[mengqiy]

It currently always rewrites the whole CRD.
It's hard to distinguish if a field is added by user manually or should be deleted. e.g. if there was +kubebuilder:validation:Maximum=100 and later it is removed. The generator should be know this field should be removed and not treat it as user added.

IMO having +kubebuilder:validation:title should be better.

[droot]

I agree with @mengqiy's reasoning here. Adding +kubebuilder:validation:title in the types.go seems to be the right approach.

[DirectXMan12]

Ack. We should have validation directives to support everything. I don't think it makes sense to try and merge.

[hasbro17]

Sounds good to me. If we can extend the supported tags to include title and any other missing directives then we can regenerate validation from scratch every time.

[estroz]

@mengqiy I will submit a PR.

[estroz]

@mengqiy @DirectXMan12 @droot what is meant by this code comment

Don't allow recursion until we support it through refs

Do you intend to support referencing other parsed types in +kubebuilder:validation annotations?

[DirectXMan12]

I think that's intended to prevent recursive types from blowing out the generator. We intend to eventually support embedding validation for other types. The k/k validation parser doesn't support cross-group references, so we'd have to embed.

[estroz]

@DirectXMan12 by "cross-group references" do you mean referencing validation using JSONSchemaProps.Ref? And "embedding validation" == embedding a string value that unmarshals into a JSONShemaProps?

WDYT about having users define Go variables of type JSONSchemaProps, and have +kubebuilder:validation values that reference variable names for schema object properties like AllOf? That would allow complex validation blocks to be easily defined and set in CRD's.

Example toy_types.go file:

package v1alpha1

import (
	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// ToyS3LogConfig defines logs configs using S3 backend
type ToyS3LogConfig struct {
	// +kubebuilder:validation:MinLength=1
	Bucket string `json:"bucket,omitempty"`
}

var toyS3LogConfigAllOfSchema = []apiextv1beta1.JSONSchemaProps{
	{...},
}

// ToySpec defines the desired state of Toy
type ToySpec struct {
	...
	// Used this for testing fieldNames with number.
	// +kubebuilder:validation:AllOf=toyS3LogConfigAllOfSchema
	S3Log ToyS3LogConfig `json:"s3Log"`
	...
}

type Toy struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Spec   ToySpec   `json:"spec,omitempty"`
	Status ToyStatus `json:"status,omitempty"`
}

[DirectXMan12]

@DirectXMan12 by "cross-group references" do you mean referencing validation using JSONSchemaProps.Ref

Yeah

And "embedding validation" == embedding a string value that unmarshals into a JSONSchemaProps

No, I mean recursively replacing the ref with the actual type by fetching and generating a schema for that type (optionally with some overloading for certain types in k/k that don't behave as expected).

See #190

WDYT about having

That makes our parsing a lot more complicated. I'm planning to support putting validation on types instead of just fields, which makes it easier to share validation (e.g. // +kubebuilder:validation:MinLength=1 ; type Bucket string), but I'm not a huge fan of also having to parse variable declarations as well -- the parsing code is hairy enough as it is.

[fejta-bot]

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

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten