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

Move validation to be part of the CRD #3319

Open
jaronoff97 opened this issue Oct 2, 2024 · 5 comments · May be fixed by #3321
Open

Move validation to be part of the CRD #3319

jaronoff97 opened this issue Oct 2, 2024 · 5 comments · May be fixed by #3321
Labels
area:auto-instrumentation Issues for auto-instrumentation area:collector Issues for deploying collector area:opamp Issues for the opamp resource area:target-allocator Issues for target-allocator enhancement New feature or request good first issue Good for newcomers

Comments

@jaronoff97
Copy link
Contributor

Component(s)

collector, auto-instrumentation, target allocator, opamp

Is your feature request related to a problem? Please describe.

CRDs can have validation rules now, it would be great to move our existing webhooks (for whatever we can) into these.

Describe the solution you'd like

Migrate to the kubebuilder's support for CRD validation rules. This would probably require us to bump our min supported version ...

Describe alternatives you've considered

No response

Additional context

No response

@jaronoff97 jaronoff97 added enhancement New feature or request good first issue Good for newcomers area:collector Issues for deploying collector area:auto-instrumentation Issues for auto-instrumentation area:target-allocator Issues for target-allocator area:opamp Issues for the opamp resource labels Oct 2, 2024
@mercybassey
Copy link

Hi @jaronoff97 I am an Outreachy applicant, and I would like to work on this issue. I'll review the project and keep you updated on my progress. In the meantime, I would appreciate any insights or guidance you could provide to help me address this issue.

@jaronoff97
Copy link
Contributor Author

@mercybassey sounds good! one issue with this is that feature went beta in Kubernetes 1.25 and stable in kubernetes 1.29 (link) and right now our minimum supported version is 1.23. We will probably need to bump that minimum version as part of this. We have an open issue here for that #2798

@mercybassey
Copy link

@jaronoff97 Thank you for the update! I’d like to clarify if I can go ahead with migrating the validation rules to the CRDs while keeping an eye on the progress of issue #2798 regarding the version bump.

@swiatekm
Copy link
Contributor

swiatekm commented Oct 3, 2024

Assuming the x-kubernetes-validations field is ignored on K8s <1.25 (it should be, but we need to verify), then I think we can add these validations, without removing existing ones from the webhook. There isn't really any downside to having both for users. It does represent additional maintenance burden for us, but I wouldn't expect it to be very noticable.

@mercybassey if you want to work on this, I'd limit the scope to just adding the existing webhook validations as validation rules, without removing the former. You can start by familiarizing yourself with the webhook logic, how we define and annotate our CRDs using kubebuilder, and kubebuilder's own documentation. What you want for CRD validation rules is the +kubebuilder:validation:XValidation marker.

Here's also a tutorial showing how to use CEL with kubebuilder in practice.

@mercybassey
Copy link

mercybassey commented Oct 3, 2024

Assuming the x-kubernetes-validations field is ignored on K8s <1.25 (it should be, but we need to verify), then I think we can add these validations, without removing existing ones from the webhook. There isn't really any downside to having both for users. It does represent additional maintenance burden for us, but I wouldn't expect it to be very noticable.

@mercybassey if you want to work on this, I'd limit the scope to just adding the existing webhook validations as validation rules, without removing the former. You can start by familiarizing yourself with the webhook logic, how we define and annotate our CRDs using kubebuilder, and kubebuilder's own documentation. What you want for CRD validation rules is the +kubebuilder:validation:XValidation marker.

Here's also a tutorial showing how to use CEL with kubebuilder in practice.

Thank you for your guidance. I'll start working on it right away.

@mercybassey mercybassey linked a pull request Oct 3, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:auto-instrumentation Issues for auto-instrumentation area:collector Issues for deploying collector area:opamp Issues for the opamp resource area:target-allocator Issues for target-allocator enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants