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

Proposal: Validation for CustomResources. #708

Merged
merged 1 commit into from Jul 6, 2017

Conversation

@nikhita
Member

nikhita commented Jun 12, 2017

ThirdPartyResource (TPR) is deprecated and CustomResourceDefinition (CRD) is the successor which solves the fundamental issues of TPRs to form a stable base for further features.

Currently we do not provide validation for CustomResources (CR). This proposal proposes the design and describes a way to add JSON-Schema based validation for CustomResources.

cc @sttts @deads2k @enisoc @xiao-zhou @erictune @lavalamp @brendanburns @philips @fabxc @frankgreco @sdminonne

@sttts

This comment has been minimized.

Show comment
Hide comment
@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k
Contributor

deads2k commented Jun 12, 2017

@enisoc

This comment has been minimized.

Show comment
Hide comment
@enisoc
Member

enisoc commented Jun 12, 2017

@enisoc

Looks great to me overall.

Show outdated Hide outdated contributors/design-proposals/customresources-validation.md Outdated
Show outdated Hide outdated contributors/design-proposals/customresources-validation.md Outdated
This is especially true in situations when CRs are used by components that are out of the control of the admin. Example: A user can create a database CR for a Database-As-A-Service. In this case, only server-side validation can give confidence that the CRs are well formed.
### Existing Instances and changing the Schema

This comment has been minimized.

@enisoc

enisoc Jun 12, 2017

Member

In the Google Doc, there was some discussion about ways to deal with CRs that exist before the introduction of validation on a given CRD for the first time. Can you summarize the findings in this proposal?

@enisoc

enisoc Jun 12, 2017

Member

In the Google Doc, there was some discussion about ways to deal with CRs that exist before the introduction of validation on a given CRD for the first time. Can you summarize the findings in this proposal?

This comment has been minimized.

@nikhita

nikhita Jun 13, 2017

Member

Added.

@nikhita

nikhita Jun 13, 2017

Member

Added.

Note: this is a workaround while we do not support multiple versions and conversion for custom resources.
### JSON-Schema Validation Runtime Complexity

This comment has been minimized.

@enisoc

enisoc Jun 12, 2017

Member

In general, it seems like this will be slower than native validation. We should consult with @kubernetes/sig-scalability-proprosals to define the performance bar this needs to meet and create regression tests for large numbers of objects.

@enisoc

enisoc Jun 12, 2017

Member

In general, it seems like this will be slower than native validation. We should consult with @kubernetes/sig-scalability-proprosals to define the performance bar this needs to meet and create regression tests for large numbers of objects.

This comment has been minimized.

@sttts

sttts Jun 13, 2017

Contributor

This is certainly slower than hand crafted validation. The main point of this paragraph is not that's it might be slower by a factor of 10 (a guess), but that it cannot be used to bring down an API server because the validation algorithm is quadratic of even exponential in the input size.

But of course, having a performance bar with tests would be great.

@sttts

sttts Jun 13, 2017

Contributor

This is certainly slower than hand crafted validation. The main point of this paragraph is not that's it might be slower by a factor of 10 (a guess), but that it cannot be used to bring down an API server because the validation algorithm is quadratic of even exponential in the input size.

But of course, having a performance bar with tests would be great.

The CRD JSON-Schema will be validated to have neither recursion, nor `uniqueItems=true` being set.
### Alternatives

This comment has been minimized.

@enisoc

enisoc Jun 12, 2017

Member

I think many people will see imperative validation (custom code inserted in the request path somehow) as the main alternative to this proposal. You mentioned above they are not mutually exclusive, but it would be good to document here why we think it makes sense to put our effort in creating the declarative version first.

@enisoc

enisoc Jun 12, 2017

Member

I think many people will see imperative validation (custom code inserted in the request path somehow) as the main alternative to this proposal. You mentioned above they are not mutually exclusive, but it would be good to document here why we think it makes sense to put our effort in creating the declarative version first.

This comment has been minimized.

@bgrant0607

bgrant0607 Jun 13, 2017

Member

Schema validation performed by kubectl is driven by Swagger/OpenAPI models. An equivalent capability needs to move to the server (kubernetes/kubernetes#5889) as part of the general effort to move functionality to the server in order to improve extensibility and to simplify clients (kubernetes/kubernetes#12143).

I would like to see us move towards declarative validation generally for the API, as much as possible:
kubernetes/kubernetes#25460

@mbohlool may have already started on a proposal for that.

Also, ideally, I'd like the same spec to be usable to serve an OpenAPI spec for the CRD APIs, which would mean it would have to be an OpenAPI-compatible flavor of JSON schema. I really don't want multiple different schema languages in the system.

As for a hook-based approach, (potentially multiple) special-purpose hooks per resource would be harder to understand, as well as being more fragile, slower, etc. I'd prefer to find a way to use admission-control extension for that use case, so as not to add yet another hook mechanism:

https://github.com/kubernetes/community/blob/master/contributors/design-proposals/admission_control_extension.md

@bgrant0607

bgrant0607 Jun 13, 2017

Member

Schema validation performed by kubectl is driven by Swagger/OpenAPI models. An equivalent capability needs to move to the server (kubernetes/kubernetes#5889) as part of the general effort to move functionality to the server in order to improve extensibility and to simplify clients (kubernetes/kubernetes#12143).

I would like to see us move towards declarative validation generally for the API, as much as possible:
kubernetes/kubernetes#25460

@mbohlool may have already started on a proposal for that.

Also, ideally, I'd like the same spec to be usable to serve an OpenAPI spec for the CRD APIs, which would mean it would have to be an OpenAPI-compatible flavor of JSON schema. I really don't want multiple different schema languages in the system.

As for a hook-based approach, (potentially multiple) special-purpose hooks per resource would be harder to understand, as well as being more fragile, slower, etc. I'd prefer to find a way to use admission-control extension for that use case, so as not to add yet another hook mechanism:

https://github.com/kubernetes/community/blob/master/contributors/design-proposals/admission_control_extension.md

This comment has been minimized.

@sttts

sttts Jun 13, 2017

Contributor

No need for another hook mechanism. We don't want that. I verified that CRDs play nicely with initializers in 1.7, same should be true for admission webhooks (not verified).

@sttts

sttts Jun 13, 2017

Contributor

No need for another hook mechanism. We don't want that. I verified that CRDs play nicely with initializers in 1.7, same should be true for admission webhooks (not verified).

This comment has been minimized.

@nikhita

nikhita Jun 13, 2017

Member

Added a note about intializers.

@nikhita

nikhita Jun 13, 2017

Member

Added a note about intializers.

This comment has been minimized.

@brendandburns

brendandburns Jun 14, 2017

Contributor

Commented above. The commentary assumes/implies that webhooks are harder and less useful than json-schema. I think exactly the opposite is true.

Personally, I would argue for webhook validation in the CRD resource because conceptually it is part of the resource. While it is possible to do it in an extension admission controller, the user experience is way worse, because suddenly I have to create/manage/synchronize two different objects to implement my desired experience.

Instead, I think we should add webhooks to the CRD, and then implement a controller that reflects that webhook into an admission control extension. (similar to how ReplicaSet creates Pod objects, etc)

IMHO we shouldn't mix our desires around implementation complexity (only one kind of webhook) with our designs around API complexity (everything relating to a particular object should be in that object)

@brendandburns

brendandburns Jun 14, 2017

Contributor

Commented above. The commentary assumes/implies that webhooks are harder and less useful than json-schema. I think exactly the opposite is true.

Personally, I would argue for webhook validation in the CRD resource because conceptually it is part of the resource. While it is possible to do it in an extension admission controller, the user experience is way worse, because suddenly I have to create/manage/synchronize two different objects to implement my desired experience.

Instead, I think we should add webhooks to the CRD, and then implement a controller that reflects that webhook into an admission control extension. (similar to how ReplicaSet creates Pod objects, etc)

IMHO we shouldn't mix our desires around implementation complexity (only one kind of webhook) with our designs around API complexity (everything relating to a particular object should be in that object)

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607
Member

bgrant0607 commented Jun 13, 2017

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607
Member

bgrant0607 commented Jun 13, 2017

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
Member

bgrant0607 commented Jun 13, 2017

## Goals
1. To provide validation for CustomResources using a declarative specification language for JSON data.
2. To keep open the door to add other validation mechanisms later.<sup id="f2">[2](#footnote2)</sup>

This comment has been minimized.

@fabiand

fabiand Jun 13, 2017

You might want to drop the <sup> tag

@fabiand

fabiand Jun 13, 2017

You might want to drop the <sup> tag

This comment has been minimized.

@nikhita

nikhita Jun 13, 2017

Member

Github Flavored Markdown does not support footnotes without the <sup> tags.

Please let me know if this is indeed possible and I'm missing something here!

@nikhita

nikhita Jun 13, 2017

Member

Github Flavored Markdown does not support footnotes without the <sup> tags.

Please let me know if this is indeed possible and I'm missing something here!

The implementation is planned in the following steps:
1. Add the proposed types to the v1beta1<sup id="f3">[3](#footnote3)</sup> version of the CRD type.

This comment has been minimized.

@fabiand

fabiand Jun 13, 2017

You probably want to drop <sup> here as well.

@fabiand

fabiand Jun 13, 2017

You probably want to drop <sup> here as well.

MaxItems *int64 `json:"maxItems,omitempty"`
MinItems *int64 `json:"minItems,omitempty"`
// disable uniqueItems for now because it can cause the validation runtime
// complexity to become quadratic.

This comment has been minimized.

@deads2k

deads2k Jun 14, 2017

Contributor

CRDs are inherently a cluster-admin resource since they affect the resources for the entire cluster. I don't feel bad about allowing a cluster-admin to make an expensive resource.

@deads2k

deads2k Jun 14, 2017

Contributor

CRDs are inherently a cluster-admin resource since they affect the resources for the entire cluster. I don't feel bad about allowing a cluster-admin to make an expensive resource.

This comment has been minimized.

@sttts

sttts Jun 14, 2017

Contributor

Will cluster-admins understand complexity?

@sttts

sttts Jun 14, 2017

Contributor

Will cluster-admins understand complexity?

This comment has been minimized.

@deads2k

deads2k Jun 14, 2017

Contributor

Will cluster-admins understand complexity?

Will they care? I suspect that first line of defense would simply be to scale out the masters.

@deads2k

deads2k Jun 14, 2017

Contributor

Will cluster-admins understand complexity?

Will they care? I suspect that first line of defense would simply be to scale out the masters.

The client-side validation is carried out before sending the request to the api-server, or even completely offline. This can be achieved while creating resources through the client i.e. kubectl using the --validate option.
If the API type serves the JSON-Schema in the swagger spec, the existing kubectl code will already be able to also validate CRs. This will be achieved as a follow-up.

This comment has been minimized.

@deads2k

deads2k Jun 14, 2017

Contributor

@mbohlool this is going to require changes to the openapi aggregation triggers.

@deads2k

deads2k Jun 14, 2017

Contributor

@mbohlool this is going to require changes to the openapi aggregation triggers.

This comment has been minimized.

@mbohlool

mbohlool Jul 31, 2017

Member

Sorry that I see this late. I have two points here:

  1. If we register CRDs with aggregation server (which I think we should), this will be aggregated automatically.
  2. OpenAPI supports a subset of json schema. I think instead of asking users to provide JSON Schema we should ask them to provide OpenAPI spec that has a Definition model in it (that is a subset of JSON Schema).
@mbohlool

mbohlool Jul 31, 2017

Member

Sorry that I see this late. I have two points here:

  1. If we register CRDs with aggregation server (which I think we should), this will be aggregated automatically.
  2. OpenAPI supports a subset of json schema. I think instead of asking users to provide JSON Schema we should ask them to provide OpenAPI spec that has a Definition model in it (that is a subset of JSON Schema).

This comment has been minimized.

@sttts

sttts Jul 31, 2017

Contributor

about 2) why should we restrict ourselfes to openapi? The openapi spec is a side-product of this validation and I don't want to loose expressivity because openapi hasn't updated to the JSON schema draft 4 yet.

Moreover, some day openapi v3 is finalized, we will support v3 and v2 for backward compatibility. Do you suggest that we stay at the common denominator, i.e. to that JSON schema subset supported by v2? In other words, at some point we will have to diverge anyway.

Is there anything that stops us from stripping the given spec from unsupported features for consumation by the openapi aggregator? As far as I see all JSON Schema fields are monotonic in the sense that stripping fields makes the spec weaker. That's what we need for easy openapi consumation.

@sttts

sttts Jul 31, 2017

Contributor

about 2) why should we restrict ourselfes to openapi? The openapi spec is a side-product of this validation and I don't want to loose expressivity because openapi hasn't updated to the JSON schema draft 4 yet.

Moreover, some day openapi v3 is finalized, we will support v3 and v2 for backward compatibility. Do you suggest that we stay at the common denominator, i.e. to that JSON schema subset supported by v2? In other words, at some point we will have to diverge anyway.

Is there anything that stops us from stripping the given spec from unsupported features for consumation by the openapi aggregator? As far as I see all JSON Schema fields are monotonic in the sense that stripping fields makes the spec weaker. That's what we need for easy openapi consumation.

1. This is the same behavior that we require for native resources. Validation cannot be made stricter in later Kubernetes versions without breaking compatibility.
2. For migration of CRDs with no validation to CRDs with validation, we can create a controller that will validate and annotate invalid CRs once the spec changes, so that the custom controller can choose to delete them (this is also essentially the status condition of the CRD). This can be achieved, but it is not part of the proposal.

This comment has been minimized.

@deads2k

deads2k Jun 14, 2017

Contributor

I'm not really a fan of this. It seems like it should mirror kube-apis: it's the responsibility of the APi author and fixups are performed client-side.

@deads2k

deads2k Jun 14, 2017

Contributor

I'm not really a fan of this. It seems like it should mirror kube-apis: it's the responsibility of the APi author and fixups are performed client-side.

The implementation is planned in the following steps:
1. Add the proposed types to the v1beta1<sup id="f3">[3](#footnote3)</sup> version of the CRD type.
2. Add a validation step to the CREATE and UPDATE REST handlers of the apiextensions-apiserver.

This comment has been minimized.

@deads2k

deads2k Jun 14, 2017

Contributor

I think you actually just want to update the strategy.

@deads2k

deads2k Jun 14, 2017

Contributor

I think you actually just want to update the strategy.

This comment has been minimized.

@sttts

sttts Jun 14, 2017

Contributor

yes, that's actually meant here.

@sttts

sttts Jun 14, 2017

Contributor

yes, that's actually meant here.

This comment has been minimized.

@nikhita

nikhita Jun 14, 2017

Member

Yes, will be passing the CRD to the strategy.

@nikhita

nikhita Jun 14, 2017

Member

Yes, will be passing the CRD to the strategy.

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jun 14, 2017

Contributor

minor comments. This lgtm.

Contributor

deads2k commented Jun 14, 2017

minor comments. This lgtm.

@sdminonne

This comment has been minimized.

Show comment
Hide comment
@sdminonne

sdminonne Jun 14, 2017

Contributor

I liked the declarative approach defining validation but I would love to have a way to reuse definitions similar to swagger multiple files
Even better would be to reuse swagger as already reported by @bgrant0607-nocc

The same mechanism should be used to reference native resources like PodSpec.
To be more clear: If my CDR contains a v1.PodSpec I would like references the swagger definition of the io.k8s.kubernetes.pkg.api.v1.PodSpec avoiding cut & paste of the swagger.json.

According to @sttts this mechanism may be added in a second step (brief discussion on slack). So I'm in general OK with this approach.

Contributor

sdminonne commented Jun 14, 2017

I liked the declarative approach defining validation but I would love to have a way to reuse definitions similar to swagger multiple files
Even better would be to reuse swagger as already reported by @bgrant0607-nocc

The same mechanism should be used to reference native resources like PodSpec.
To be more clear: If my CDR contains a v1.PodSpec I would like references the swagger definition of the io.k8s.kubernetes.pkg.api.v1.PodSpec avoiding cut & paste of the swagger.json.

According to @sttts this mechanism may be added in a second step (brief discussion on slack). So I'm in general OK with this approach.

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jun 14, 2017

Contributor

Adding to @sdminonne's comment:

The languages of native kube object specs in swagger and those proposed here are the same. Hence, a schema reference could be used to link a CRD to a native object spec, using a JSON-Schema reference with a certain URL.

The main conceptual problem to solve is to define this URL namespace. Note that we are coupling non-kube-CRDs with the kube type universe, maybe even with a number of user provided API servers. One could use the service names of those API servers to reference their specs, e.g. https://kubernetes/api/v1/pods/schema.json and https://kubernetes/apis/you.group.com/your-custom-resources/schema.json. But this needs deep thought as we are coupling independent components, after splitting them apart into single API servers with a lot of effort.

Hence, I see this as a follow-up to this proposal. Right now, we will keep schema references local and the schemas self-contained.

Contributor

sttts commented Jun 14, 2017

Adding to @sdminonne's comment:

The languages of native kube object specs in swagger and those proposed here are the same. Hence, a schema reference could be used to link a CRD to a native object spec, using a JSON-Schema reference with a certain URL.

The main conceptual problem to solve is to define this URL namespace. Note that we are coupling non-kube-CRDs with the kube type universe, maybe even with a number of user provided API servers. One could use the service names of those API servers to reference their specs, e.g. https://kubernetes/api/v1/pods/schema.json and https://kubernetes/apis/you.group.com/your-custom-resources/schema.json. But this needs deep thought as we are coupling independent components, after splitting them apart into single API servers with a lot of effort.

Hence, I see this as a follow-up to this proposal. Right now, we will keep schema references local and the schemas self-contained.

@philips

This comment has been minimized.

Show comment
Hide comment
@philips
Contributor

philips commented Jun 14, 2017

@brendandburns

This comment has been minimized.

Show comment
Hide comment
@brendandburns

brendandburns Jun 16, 2017

Contributor

I commented above, but a thing that just occurred to me is that we will want to support multiple different validations all running for the same CRD. That's because Schematic and Programattic validation are both valuable and users will likely want to combine them.

Contributor

brendandburns commented Jun 16, 2017

I commented above, but a thing that just occurred to me is that we will want to support multiple different validations all running for the same CRD. That's because Schematic and Programattic validation are both valuable and users will likely want to combine them.

@frankgreco

This comment has been minimized.

Show comment
Hide comment
@frankgreco

frankgreco Jun 16, 2017

With the declarative/static approach to validation, how would dynamic fields be checked. An example of this is what the Service spec does to check the uniquess of hard-coded NodePorts. I use dynamic validation all the time for TPRs.

frankgreco commented Jun 16, 2017

With the declarative/static approach to validation, how would dynamic fields be checked. An example of this is what the Service spec does to check the uniquess of hard-coded NodePorts. I use dynamic validation all the time for TPRs.

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jun 16, 2017

Contributor

With the declarative/static approach to validation, how would dynamic fields be checked.

This proposal explicitly notes that multiple validation approaches are a design goal https://github.com/kubernetes/community/pull/708/files#diff-9679c445e428c00f0907fa6fe5c6d8e4R46, but intentionally does not include that implementation. The CRD schema is structured to make adding that in the future easy/reasonable to do, but it can come later at a different time.

Contributor

deads2k commented Jun 16, 2017

With the declarative/static approach to validation, how would dynamic fields be checked.

This proposal explicitly notes that multiple validation approaches are a design goal https://github.com/kubernetes/community/pull/708/files#diff-9679c445e428c00f0907fa6fe5c6d8e4R46, but intentionally does not include that implementation. The CRD schema is structured to make adding that in the future easy/reasonable to do, but it can come later at a different time.

@brendanburns

This comment has been minimized.

Show comment
Hide comment
@brendanburns

brendanburns Jun 16, 2017

Contributor
Contributor

brendanburns commented Jun 16, 2017

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jun 16, 2017

Contributor

But it doesn't (as far as I could find) describe that multiple different
validations could be active at the same time. The API object should
probably be structured as an array of validation objects, not a single
object...

The the API has a validation stanza, which currently has one element for JSONSchema and could easily be existed to have another element called "webhook" or some such.

Contributor

deads2k commented Jun 16, 2017

But it doesn't (as far as I could find) describe that multiple different
validations could be active at the same time. The API object should
probably be structured as an array of validation objects, not a single
object...

The the API has a validation stanza, which currently has one element for JSONSchema and could easily be existed to have another element called "webhook" or some such.

@brendanburns

This comment has been minimized.

Show comment
Hide comment
@brendanburns

brendanburns Jun 16, 2017

Contributor
Contributor

brendanburns commented Jun 16, 2017

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jun 19, 2017

Contributor

Right, but my point is that I may actually want multiple web hooks, or a
JSON schema and a webhook. I think an array captures this more elegantly
than a single top-level struct.

The current struct does not preclude introducing a slice of webhooks. Your example also highlights a deficiency of an array of validators, since some combinations like multiple JSON-schemas don't make sense. Rather than enforce that in resource validation, I would make the struct represent the valid combinations.

Contributor

deads2k commented Jun 19, 2017

Right, but my point is that I may actually want multiple web hooks, or a
JSON schema and a webhook. I think an array captures this more elegantly
than a single top-level struct.

The current struct does not preclude introducing a slice of webhooks. Your example also highlights a deficiency of an array of validators, since some combinations like multiple JSON-schemas don't make sense. Rather than enforce that in resource validation, I would make the struct represent the valid combinations.

@agonzalezro

This comment has been minimized.

Show comment
Hide comment
@agonzalezro

agonzalezro Jun 20, 2017

If multiple validations are added (as @brendanburns mentioned) I suppose it's going to be a new struct field here: CRD.Spec.Validation.*.

Is the order of this validations important? For example, somebody would like to run JSON Schema validations before calling a webhook.

In case it's important what do you think about something like this (please, read this as pseudo code 😬):

type CustomResourceDefinitionSpec struct {
   ...
   Validations []CustomResourceValidation `json:"validation,omitempty"`
}

type CustomResourceValidation interface{
  Validate() []error
}

This would allow us to even repeat validations which can be good and bad at the same time:

  • good: we could validate with JSON Schema, webhook A (microservice from dev team), webhook B (microservice from finance team -> billing).
  • bad: you could as well add several JSON Schemas validation to the same CRD that could end up fighting between them (one returning true and the other false).

agonzalezro commented Jun 20, 2017

If multiple validations are added (as @brendanburns mentioned) I suppose it's going to be a new struct field here: CRD.Spec.Validation.*.

Is the order of this validations important? For example, somebody would like to run JSON Schema validations before calling a webhook.

In case it's important what do you think about something like this (please, read this as pseudo code 😬):

type CustomResourceDefinitionSpec struct {
   ...
   Validations []CustomResourceValidation `json:"validation,omitempty"`
}

type CustomResourceValidation interface{
  Validate() []error
}

This would allow us to even repeat validations which can be good and bad at the same time:

  • good: we could validate with JSON Schema, webhook A (microservice from dev team), webhook B (microservice from finance team -> billing).
  • bad: you could as well add several JSON Schemas validation to the same CRD that could end up fighting between them (one returning true and the other false).
Show outdated Hide outdated contributors/design-proposals/customresources-validation.md Outdated
Show outdated Hide outdated contributors/design-proposals/customresources-validation.md Outdated
Show outdated Hide outdated contributors/design-proposals/customresources-validation.md Outdated
}
// JSONSchemaProps is a JSON-Schema following Specification Draft 4 (http://json-schema.org/).
type JSONSchemaProps struct {

This comment has been minimized.

@jamiehannaford

jamiehannaford Jun 22, 2017

Member

Could we use a json schema lib like https://github.com/xeipuuv/gojsonschema instead of defining this ourselves? I know folks are wary of more imports but just thought I'd float the idea

@jamiehannaford

jamiehannaford Jun 22, 2017

Member

Could we use a json schema lib like https://github.com/xeipuuv/gojsonschema instead of defining this ourselves? I know folks are wary of more imports but just thought I'd float the idea

This comment has been minimized.

@nikhita

nikhita Jun 22, 2017

Member

I believe we don't really want to import here and end up making our types depend on external types.

Had considered using https://github.com/xeipuuv/gojsonschema but instead decided to go with go-openapi because we want to be able to express the schema via openapi docs.

@nikhita

nikhita Jun 22, 2017

Member

I believe we don't really want to import here and end up making our types depend on external types.

Had considered using https://github.com/xeipuuv/gojsonschema but instead decided to go with go-openapi because we want to be able to express the schema via openapi docs.

This comment has been minimized.

@jamiehannaford

jamiehannaford Jun 22, 2017

Member

Yeah, it's also defined here. Just wondering if we can avoid duplication and the risk of feature drift.

@liggitt should we be avoiding external types here?

@jamiehannaford

jamiehannaford Jun 22, 2017

Member

Yeah, it's also defined here. Just wondering if we can avoid duplication and the risk of feature drift.

@liggitt should we be avoiding external types here?

This comment has been minimized.

@liggitt

liggitt Jun 22, 2017

Member

external types don't have the protobuf annotations needed by our API types. tying our API to an external API is not something we want to do.

@liggitt

liggitt Jun 22, 2017

Member

external types don't have the protobuf annotations needed by our API types. tying our API to an external API is not something we want to do.

@nikhita

This comment has been minimized.

Show comment
Hide comment
@nikhita

nikhita Jun 22, 2017

Member

@agonzalezro That looks like a nice approach but what I am concerned about is that we can end up with multiple JSON schemas and allow invalid combinations. We need the JSON schema to only have one entry point (not multiple).

Member

nikhita commented Jun 22, 2017

@agonzalezro That looks like a nice approach but what I am concerned about is that we can end up with multiple JSON schemas and allow invalid combinations. We need the JSON schema to only have one entry point (not multiple).

## API Types
The schema is referenced in [`CustomResourceDefinitionSpec`](https://github.com/kubernetes/kubernetes/commit/0304ef60a210758ab4ac43a468f8a5e19f39ff5a#diff-0e64a9ef2cf809a2a611b16fd44d22f8). `Validation` is of the type `CustomResourceValidation`. The JSON-Schema is stored in a field of `Validation`. This way we can make the validation generic and add other validations in the future as well.

This comment has been minimized.

@sttts

sttts Jul 3, 2017

Contributor

To address @brendanburns's comment #708 (comment):

This way we can make the validation generic and add other validation options in the future. Even multiple validation options could be used at the same time within one CustomResourceDefinition.
@sttts

sttts Jul 3, 2017

Contributor

To address @brendanburns's comment #708 (comment):

This way we can make the validation generic and add other validation options in the future. Even multiple validation options could be used at the same time within one CustomResourceDefinition.
@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 3, 2017

Contributor

I can see value in @agonzalezro's example with multiple webhooks. Also the possibility to rely on static JSON-schema validation before calling a webhook sounds reasonable.

Though I am not so sure about introducing any more structure, i.e. a whole slice of validation mechanisms. As the next step we would want to define the parallalism, i.e. sequential or in parallel, eventually leading to complete pipeline semantics. I agree with @deads2k that having a struct { JSONSchema, []WebHook } is a good line to draw. If some day we have some embedded interpreter of some sort, we could have struct { JSONSchema, Script, []WebHook }.

Contributor

sttts commented Jul 3, 2017

I can see value in @agonzalezro's example with multiple webhooks. Also the possibility to rely on static JSON-schema validation before calling a webhook sounds reasonable.

Though I am not so sure about introducing any more structure, i.e. a whole slice of validation mechanisms. As the next step we would want to define the parallalism, i.e. sequential or in parallel, eventually leading to complete pipeline semantics. I agree with @deads2k that having a struct { JSONSchema, []WebHook } is a good line to draw. If some day we have some embedded interpreter of some sort, we could have struct { JSONSchema, Script, []WebHook }.

@agonzalezro

This comment has been minimized.

Show comment
Hide comment
@agonzalezro

agonzalezro Jul 3, 2017

I see struct { JSONSchema, []WebHook } reasonable. In the case that it's EXTREMELY important for somebody to to run the webhooks before the schema they could implement the schema validation in their latest webhook. Of course, this case would be like the 0.1% of the people :)

Looks pretty good, good job you all.

agonzalezro commented Jul 3, 2017

I see struct { JSONSchema, []WebHook } reasonable. In the case that it's EXTREMELY important for somebody to to run the webhooks before the schema they could implement the schema validation in their latest webhook. Of course, this case would be like the 0.1% of the people :)

Looks pretty good, good job you all.

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jul 5, 2017

Contributor

/lgtm

Contributor

deads2k commented Jul 5, 2017

/lgtm

@frankgreco

This comment has been minimized.

Show comment
Hide comment
@frankgreco

frankgreco Jul 5, 2017

I am very interested in contributing to this change. Is this the right issue to be following for that. I assume at some point this thread will turn into more of implementation discussion once the final design is finished.

frankgreco commented Jul 5, 2017

I am very interested in contributing to this change. Is this the right issue to be following for that. I assume at some point this thread will turn into more of implementation discussion once the final design is finished.

@enisoc

This comment has been minimized.

Show comment
Hide comment
@enisoc

enisoc Jul 5, 2017

Member

@frankgreco There is work in progress here: kubernetes/kubernetes#47263

Member

enisoc commented Jul 5, 2017

@frankgreco There is work in progress here: kubernetes/kubernetes#47263

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jul 5, 2017

Contributor

I am very interested in contributing to this change. Is this the right issue to be following for that. I assume at some point this thread will turn into more of implementation discussion once the final design is finished.

I figured I'd give a day for closing arguments and merge this tomorrow.

Contributor

deads2k commented Jul 5, 2017

I am very interested in contributing to this change. Is this the right issue to be following for that. I assume at some point this thread will turn into more of implementation discussion once the final design is finished.

I figured I'd give a day for closing arguments and merge this tomorrow.

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jul 6, 2017

Contributor

I figured I'd give a day for closing arguments and merge this tomorrow.

Ok. I'll merge this as a starting point. Specific issues are welcome as are comments on the WIP kubernetes/kubernetes#47263

Contributor

deads2k commented Jul 6, 2017

I figured I'd give a day for closing arguments and merge this tomorrow.

Ok. I'll merge this as a starting point. Specific issues are welcome as are comments on the WIP kubernetes/kubernetes#47263

@deads2k deads2k merged commit 05d4e60 into kubernetes:master Jul 6, 2017

1 check passed

cla/linuxfoundation nikhita authorized
Details
@mbohlool

This comment has been minimized.

Show comment
Hide comment
@mbohlool

mbohlool Jul 31, 2017

Member
Member

mbohlool commented Jul 31, 2017

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 31, 2017

Contributor

@mbohlool let's continue in the issue kubernetes/kubernetes#49879

Contributor

sttts commented Jul 31, 2017

@mbohlool let's continue in the issue kubernetes/kubernetes#49879

@nikhita nikhita referenced this pull request Aug 7, 2017

Merged

apiextensions: validation for customresources #47263

13 of 13 tasks complete

k8s-merge-robot added a commit to kubernetes/kubernetes that referenced this pull request Aug 30, 2017

Merge pull request #47263 from nikhita/crd-01-validation-types
Automatic merge from submit-queue

apiextensions: validation for customresources

- [x] Add types for validation of CustomResources
- [x] Fix conversion-gen: #49747
- [x] Fix defaulter-gen: kubernetes/gengo#61
- [x] Convert to OpenAPI types
- [x] Validate CR using go-openapi
- [x] Validate CRD Schema
- [x] Add integration tests
- [x] Fix round trip tests: #51204 
- [x] Add custom fuzzer functions
- [x] Add custom conversion functions
- [x] Fix data race while updating CRD: #50098 
- [x] Add feature gate for CustomResourceValidation
- [x] Fix protobuf generation

Proposal: kubernetes/community#708
Additional discussion: #49879, #50625

**Release note**:

```release-note
Add validation for CustomResources via JSON Schema.
```

/cc @sttts @deads2k

sttts pushed a commit to sttts/apiextensions-apiserver that referenced this pull request Sep 22, 2017

Merge pull request #47263 from nikhita/crd-01-validation-types
Automatic merge from submit-queue

apiextensions: validation for customresources

- [x] Add types for validation of CustomResources
- [x] Fix conversion-gen: #49747
- [x] Fix defaulter-gen: kubernetes/gengo#61
- [x] Convert to OpenAPI types
- [x] Validate CR using go-openapi
- [x] Validate CRD Schema
- [x] Add integration tests
- [x] Fix round trip tests: #51204
- [x] Add custom fuzzer functions
- [x] Add custom conversion functions
- [x] Fix data race while updating CRD: #50098
- [x] Add feature gate for CustomResourceValidation
- [x] Fix protobuf generation

Proposal: kubernetes/community#708
Additional discussion: kubernetes/kubernetes#49879, kubernetes/kubernetes#50625

**Release note**:

```release-note
Add validation for CustomResources via JSON Schema.
```

/cc @sttts @deads2k

Kubernetes-commit: 4457e43e7b789586096bfb564330295cf0438e70
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment