opentelemetry-helm-charts icon indicating copy to clipboard operation
opentelemetry-helm-charts copied to clipboard

Can we omit this annotation entirely if `.Values.admissionWebhooks.certManager.enabled=false` instead of setting it to `none`?

Open TylerHelmuth opened this issue 1 year ago • 6 comments

          Can we omit this annotation entirely if `.Values.admissionWebhooks.certManager.enabled=false` instead of setting it to `none`?

Originally posted by @TylerHelmuth in https://github.com/open-telemetry/opentelemetry-helm-charts/pull/1175#discussion_r1595655023

TylerHelmuth avatar May 09 '24 16:05 TylerHelmuth

Hey, i would like to work on this issue.

suyash-811 avatar May 11 '24 23:05 suyash-811

@suyash-811 Great, we should wait until https://github.com/open-telemetry/opentelemetry-helm-charts/pull/1176 is merged.

TylerHelmuth avatar May 11 '24 23:05 TylerHelmuth

Just saw that the PR merged. I'll update the helm template to add the cert-manager annotation if .Values.admissionWebhooks.certManager.enabled=false

suyash-811 avatar May 13 '24 20:05 suyash-811

I took a look at the template admission-webhooks/operator-webhook-with-cert-manager.yaml. There is an ifblock at the start of the file which also uses the same field from the values file to generate the Webhook configurations (.Values.admissionWebhooks.certManager.enabled).

https://github.com/open-telemetry/opentelemetry-helm-charts/blob/870f23120c43a05f8d431fd8f3534e4af44cc104/charts/opentelemetry-operator/templates/admission-webhooks/operator-webhook-with-cert-manager.yaml#L1-L6

Thus, i am a bit unsure if this is the file that really needs the change, as the file isnt rendered at all when the cert-manager admissionWebhook is disabled. Or maybe i got something wrong? Either way, looking forward to some feedback!

suyash-811 avatar May 13 '24 21:05 suyash-811

@suyash-811 Look for the other places that use the opentelemetry-operator.webhookCertAnnotation helper template. I believe instead of setting none we can add more template logic to conditionally add the annotation.

This is going to break make update-operator-crds and make check-operator-crds and maybe shouldn't be done until we've got a better way to keep the CRD templates up to date.

TylerHelmuth avatar May 13 '24 21:05 TylerHelmuth

Ah i see. I'll prepare a PR for this. It can be merged once CRD management is done.

suyash-811 avatar May 14 '24 20:05 suyash-811