Skip to content
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

Adopt CRD native validation #2951

Merged

Conversation

howardjohn
Copy link
Member

Part of istio/istio#46151.

This is the first PR ramping up our CRD native validation. The intent here is to provide the same validation but without a webhook dependency.

To keep things small and start in a low-risk API, this PR introduces validation only for WasmPlugin. Most of this is pretty straightforward; the Go webhook validation logic is 1:1 mapped into native CRD concepts.

Testing this change (and future) ones has three levels:

  • In this PR, introduced a new test suite. There is a big folder of CR yamls, and each one can declare itself valid or invalid. Each of these is run through validation and asserts the results.
  • In a followup PR in istio/istio, the same test inputs are tested comparing the webook response to the CRD validation response. This ensures we are not accidentally too strict or too lenient in our CRD validation; our goal is to be identical.
  • In addition to ^, a fuzz test will be added that does the same but with randomized inputs. This will give us pretty solid evidence that this change is safe.

blocked by: istio/tools#2687
blocked by: me running the fuzzer a bit locally to get confidence

@howardjohn howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Oct 6, 2023
@istio-testing
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 6, 2023
Copy link
Contributor

@costinm costinm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the change - but I think we are over-complicating things. Great to have a test suite, but I don't think it needs to match 100% what the current validator does. If we can convert any tests the validator had to the new suite - it should be sufficient.

@howardjohn howardjohn marked this pull request as ready for review October 12, 2023 18:52
@howardjohn howardjohn requested review from a team as code owners October 12, 2023 18:52
@howardjohn howardjohn force-pushed the gen/new-crd-generator-validation branch from 28eb25f to c96447a Compare October 12, 2023 18:52
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 12, 2023
Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does cue intergrate with kubebuilder

@howardjohn
Copy link
Member Author

howardjohn commented Oct 13, 2023 via email

Comment on lines +451 to +452
// +kubebuilder:validation:MaxLength=256
// +kubebuilder:validation:MinLength=1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: sometimes the min is before the max, sometimes its after.

@istio-testing istio-testing merged commit a53bf82 into istio:master Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes-none Indicates a PR that does not require release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants