open-telemetry / opentelemetry-operator

Kubernetes Operator for OpenTelemetry Collector
Apache License 2.0
1.24k stars 445 forks source link

Move validation to be part of the CRD #3319

Open jaronoff97 opened 2 months ago

jaronoff97 commented 2 months ago

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

mercybassey commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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.

jbiers commented 3 days ago

@mercybassey Are you still planning on contributing to this issue? If not, let me know as I'd like to take a shot at this ;)

mercybassey commented 2 days ago

@mercybassey Are you still planning on contributing to this issue? If not, let me know as I'd like to take a shot at this ;)

You can go ahead.