apiextensions: validation for customresources#47263
apiextensions: validation for customresources#47263k8s-github-robot merged 6 commits intokubernetes:masterfrom
Conversation
|
Hi @nikhita. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. DetailsInstructions 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. I understand the commands that are listed here. |
1cdc672 to
6ef70fe
Compare
5044557 to
5e1c126
Compare
5e1c126 to
3bf86ac
Compare
3bf86ac to
c70f918
Compare
4ce3290 to
8192977
Compare
|
This will not pass tests until kubernetes/gengo#61 is merged. |
8192977 to
604f48b
Compare
| return true | ||
| case in.Kind == out.Kind: | ||
| // if the type exists already, return early to avoid recursion | ||
| if existingTypes[in] { |
There was a problem hiding this comment.
also here: alreadyVisitedTypes
| } | ||
|
|
||
| // JSONSchemaProps is a JSON-Schema following Specification Draft 4 (http://json-schema.org/). | ||
| type JSONSchemaProps struct { |
There was a problem hiding this comment.
Presumably this was covered in a design doc that I didn't manage to look at, but: is it really a good idea to recreate a spec type in our API like this? Was there a debate on the pros and cons? Sorry to bring this up at this late stage. But stuff we put in our API is forever.
There was a problem hiding this comment.
What exactly is the question? Yes, there was debate. Even if we reused types from a library, it would be in our API forever. The point is that this way we can guarantee it forever. By using a library which meanwhile switched to OpenAPI v6 we maybe couldn't.
|
cc @deads2k ready for review. |
|
/retest |
|
/test pull-kubernetes-bazel-build |
|
@smarterclayton @deads2k protobuf support is added now, with a stripped down variant of |
| } | ||
|
|
||
| func (s JSON) MarshalJSON() ([]byte, error) { | ||
| if len(s.Raw) > 0 { |
There was a problem hiding this comment.
note that you're compressing empty into "null". That's reasonable given the rest of the API, it just catches my eye.
| func Convert_v1beta1_JSON_To_apiextensions_JSON(in *JSON, out *apiextensions.JSON, s conversion.Scope) error { | ||
| if in != nil { | ||
| var i interface{} | ||
| if err := json.Unmarshal(in.Raw, &i); err != nil { |
There was a problem hiding this comment.
This makes sense. The end result is a protobuf shell with a delicious json nougat-y center, right?
|
As the JSON protobuf change is squashed already, here are the relevant lines of code:
@smarterclayton ^^^ |
This looks fairly straightforward. protobuf wrapping a limited json field with simple deserialization and a little validation. This may actually have been what the service catalog was looking for a while back. |
| } | ||
|
|
||
| // JSONSchemaProps is a JSON-Schema following Specification Draft 4 (http://json-schema.org/). | ||
| type JSONSchemaProps struct { |
There was a problem hiding this comment.
Maybe move this into its own types.go file - types_jsonschema.go
|
Looks like I expected. This has high level approval from me. |
Remove protobuf generation because of the interface type Add custom fuzzer funcs Add custom marshalling Add custom conversion functions move jsonschema types to separate file
update generated proto
* convert our types to openAPI types * update strategy to include crd * use strategy to validate customresource * add helper funcs * Fix conversion of empty ref field * add validation for forbidden fields * add defaulting for schema field * Validate CRD Schema
Update test schema Add polling for TestCRValidationOnCRDUpdate Add tests for forbidden fields Enable featureGate for CustomResourceValidation
update feature gates for generic apiserver Add apiextensions-apiserver features to golint_failures Ignore alpha feature if gate is disabled
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, nikhita, smarterclayton, sttts Associated issue: 49747 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
@nikhita: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
|
Automatic merge from submit-queue |
Proposal: kubernetes/community#708
Additional discussion: #49879, #50625
Release note:
/cc @sttts @deads2k