-
Notifications
You must be signed in to change notification settings - Fork 403
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
✨ Generate schema for types with custom JSON marshaling as Any type #427
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions 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. |
Welcome @skaslev! |
985db0a
to
c92ebff
Compare
/assign @DirectXMan12 |
/retest |
@skaslev: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions 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. |
/check-cla |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a test case for this (just add something to the big integration test)? Otherwise LGTM
Thanks! We've wanted something like this for a while. |
@skaslev any chance to update the test and get this in? thank you! |
@yuzisun (or anyone else) -- if skaslev doesn't get back to us and you feel like writing the tests, feel free to add an additional commit and submit a new PR (and link to this one). |
To generate CRD validation schemas (also known as structual validation schemas[1]), we bring in controller-tools. This houses a tool called controller-gen which performs various K8s related code generation. Note that we are using a fork of controller-tools due to the upstream (as of v0.3.0) lacking support in two main areas. 1) A type which implements custom JSON marshalling has its validation schema generation skipped. In our case, api.Rule (which is set as the Spec field in CNP and CCNP) implements custom JSON marshalling. With the upstream tool, this skips the validation generation for it. In our fork, we revert this feature. See PR: kubernetes-sigs/controller-tools#427 2) For CNP and CCNP, we require `oneOf` validation for the endpointSelector and nodeSelector fields. This is because we want to validate that either endpointSelector OR nodeSelector is present, but not both, and at least one. In the upstream, there's currently an open PR that implements support for this. We have manually merged it in our fork. See PR: kubernetes-sigs/controller-tools#298 This commit also brings updates to our dependencies: - gomega v1.7.0 -> v1.8.1 - golang.org/x/internal/{tools,event...} [1]: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#specifying-a-structural-schema Co-authored-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: André Martins <andre@cilium.io> Signed-off-by: Chris Tarazi <chris@isovalent.com>
To generate CRD validation schemas (also known as structual validation schemas[1]), we bring in controller-tools. This houses a tool called controller-gen which performs various K8s related code generation. Note that we are using a fork of controller-tools due to the upstream (as of v0.3.0) lacking support in two main areas. 1) A type which implements custom JSON marshalling has its validation schema generation skipped. In our case, api.Rule (which is set as the Spec field in CNP and CCNP) implements custom JSON marshalling. With the upstream tool, this skips the validation generation for it. In our fork, we revert this feature. See PR: kubernetes-sigs/controller-tools#427 2) For CNP and CCNP, we require `oneOf` validation for the endpointSelector and nodeSelector fields. This is because we want to validate that either endpointSelector OR nodeSelector is present, but not both, and at least one. In the upstream, there's currently an open PR that implements support for this. We have manually merged it in our fork. See PR: kubernetes-sigs/controller-tools#298 This commit also brings updates to our dependencies: - gomega v1.7.0 -> v1.8.1 - golang.org/x/internal/{tools,event...} [1]: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#specifying-a-structural-schema Co-authored-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: André Martins <andre@cilium.io> Signed-off-by: Chris Tarazi <chris@isovalent.com>
@DirectXMan12 @skaslev Using the tip of the main branch, I'm now getting an invalid CRD:
|
We should improve the error message saying that when using this capability, folks should manually specify the validation type |
|
Currently controller-gen complains about structs with missing json tags even
when those structs implement custom JSON marshalling.
With this change we check if a type implements custom JSON marshalling and if it
does, we output schema for Any type. This still allows the validation type to
overriden with a marker.
Fixes #391