Skip to content

feat(opentelemetry-operator): auto detect presence of cert-manager CRDs #2021

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.

Sign up for GitHub

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

Open
theSuess wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(opentelemetry-operator): auto detect presence of cert-manager CRDs #2021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions charts/opentelemetry-operator/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ a cert is loaded from an existing secret or is provided via `.Values`
{{- $caCertEnc := "" }}
{{- $certCrtEnc := "" }}
{{- $certKeyEnc := "" }}
{{- if .Values.admissionWebhooks.autoGenerateCert.enabled }}
{{- if or (eq .Values.admissionWebhooks.certificateSource "helm-generated") (eq .Values.admissionWebhooks.certificateSource "prefer-cert-manager") }}
{{- $prevSecret := (lookup "v1" "Secret" (include "opentelemetry-operator.namespace" .) (default (printf "%s-controller-manager-service-cert" (include "opentelemetry-operator.fullname" .)) .Values.admissionWebhooks.secretName )) }}
{{- if and (not .Values.admissionWebhooks.autoGenerateCert.recreate) $prevSecret }}
{{- $certCrtEnc = index $prevSecret "data" "tls.crt" }}
Expand All @@ -140,7 +140,10 @@ a cert is loaded from an existing secret or is provided via `.Values`
{{- $certKeyEnc = b64enc $cert.Key }}
{{- $caCertEnc = b64enc $ca.Cert }}
{{- end }}
{{- else }}
{{- else if eq .Values.admissionWebhooks.certificateSource "self-signed" }}
{{- if or (or (eq .Values.admissionWebhooks.certFile "") (eq .Values.admissionWebhooks.keyFile "")) (eq .Values.admissionWebhooks.caFile "")}}
{{ fail "certFile, keyFile and caFile must be set when using self-signed as certificateSource" }}
{{- end }}
{{- $certCrtEnc = .Files.Get .Values.admissionWebhooks.certFile | b64enc }}
{{- $certKeyEnc = .Files.Get .Values.admissionWebhooks.keyFile | b64enc }}
{{- $caCertEnc = .Files.Get .Values.admissionWebhooks.caFile | b64enc }}
Expand Down
8 changes: 7 additions & 1 deletion ...entelemetry-operator/templates/admission-webhooks/operator-webhook-with-cert-manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{{- if and (.Values.admissionWebhooks.create) (.Values.admissionWebhooks.certManager.enabled) }}
{{- if .Capabilities.APIVersions.Has "cert-manager.io/v1" -}}
{{- if or (eq .Values.admissionWebhooks.certificateSource "cert-manager") (eq .Values.admissionWebhooks.certificateSource "prefer-cert-manager") }}
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
Expand Down Expand Up @@ -235,3 +236,8 @@ webhooks:
sideEffects: None
timeoutSeconds: {{ .Values.admissionWebhooks.timeoutSeconds }}
{{- end }}
{{- else -}}
{{ if eq .Values.admissionWebhooks.certificateSource "cert-manager" }}
{{ fail "certificateSource is set to cert-manager but cert manager APIs are not available" }}
{{- end }}
{{- end }}
2 changes: 1 addition & 1 deletion charts/opentelemetry-operator/templates/admission-webhooks/operator-webhook.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{{- if and (.Values.admissionWebhooks.create) (not .Values.admissionWebhooks.certManager.enabled) }}
{{- if or (ne .Values.admissionWebhooks.certificateSource "cert-manager") (and (eq .Values.admissionWebhooks.certificateSource "prefer-cert-manager") (not (.Capabilities.APIVersions.Has "cert-manager.io/v1"))) }}
{{- $cert := fromYaml (include "opentelemetry-operator.WebhookCert" .) }}
{{- $caCertEnc := $cert.ca }}
{{- $certCrtEnc := $cert.crt }}
Expand Down
2 changes: 1 addition & 1 deletion charts/opentelemetry-operator/templates/certmanager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{{- if and .Values.admissionWebhooks.create .Values.admissionWebhooks.certManager.enabled }}
{{- if and .Values.admissionWebhooks.create .Values.admissionWebhooks.certManager.enabled (.Capabilities.APIVersions.Has "cert-manager.io/v1") }}

@petewall petewall Jan 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My concern here is that if the user says they want it:

admissionWebhooks:
  create: true
  certManager:
    enabled: true

But the CRDs haven't been installed, this will silently proceed with the installation but wont deploy these objects.

Before, it was ugly, but at least it would fail.

Would it be worth adding the else for the capabilities check and then use a fail block to print a message like "Cert manager requested, but the CRDs are not installed. Please install the crd and try again."?

@theSuess theSuess Jan 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Failing when the CRDs aren't available is a good approach as well, but I think this doesn't solve the original issue of install not working for everyone by default.

IMHO the default should be certManager.enabled=false since it isn't a hard requirement (or included in k8s) but I don't think we want to change the defaults here.

@cyrille-leclerc might have an opinion here as well

@cyrille-leclerc cyrille-leclerc Jan 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i was wondering if we would introduce a preset like

certs-mode: use-existing-cert-manager | create-self-signed | auto-detect_if-crd-found-use-cert-manager_else-create-self-signed

would that resonate with you?

@petewall petewall Jan 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a good idea, and makes it much more explicit for the user to choose.

certificateSource: cert-manager | self-signed | prefer-cert-manager

re: certificateSource vs certs-mode, dashes in yaml keys are allowed, but a real pain in Helm templating context, because you have to use index.

I like the prefer-... logic because that matches other things in Kubernetes.

@cyrille-leclerc cyrille-leclerc Jan 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you found much better names than me, I love it

@theSuess theSuess Jan 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm happy with this as well, will update this PR and ping here once implemented

apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
Expand Down
13 changes: 11 additions & 2 deletions charts/opentelemetry-operator/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1479,6 +1479,7 @@
"timeoutSeconds",
"namespaceSelector",
"objectSelector",
"certificateSource",
"certManager",
"autoGenerateCert",
"certFile",
Expand Down Expand Up @@ -1582,12 +1583,21 @@
{}
]
},
"certificateSource": {
"type": "string",
"default": {},
"title": "The certificateSource Schema",
"required": [],
"properties": {},
"examples": [
{}
]
},
"certManager": {
"type": "object",
"default": {},
"title": "The certManager Schema",
"required": [
"enabled",
"issuerRef",
"certificateAnnotations",
"issuerAnnotations"
Expand Down Expand Up @@ -1665,7 +1675,6 @@
"default": {},
"title": "The autoGenerateCert Schema",
"required": [
"enabled",
"recreate"
],
"additionalProperties": false,
Expand Down
14 changes: 8 additions & 6 deletions charts/opentelemetry-operator/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,11 +294,16 @@ admissionWebhooks:
namespaceSelector: {}
objectSelector: {}

## Choose the certificate approach to take
## - prefer-cert-manager (default): Uses cert manager if available, falls back to generated certificates
## - cert-manager: Use cert-manager to generate certificates
## - helm-generated: Generate certificates with helm
## - self-signed: Provide your own certificate
certificateSource: 'prefer-cert-manager'

## https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-operator/README.md#tls-certificate-requirement
## TLS Certificate Option 1: Use certManager to generate self-signed certificate.
## certManager must be enabled. If enabled, always takes precedence over options 2 and 3.
certManager:
enabled: true
## Provide the issuer kind and name to do the cert auth job.
## By default, OpenTelemetry Operator will use self-signer issuer.
issuerRef: {}
Expand All @@ -315,17 +320,14 @@ admissionWebhooks:
renewBefore: ""

## TLS Certificate Option 2: Use Helm to automatically generate self-signed certificate.
## certManager must be disabled and autoGenerateCert must be enabled.
## If true and certManager.enabled is false, Helm will automatically create a self-signed cert and secret for you.
autoGenerateCert:
enabled: true
# If set to true, new webhook key/certificate is generated on helm upgrade.
recreate: true
# Cert period time in days. The default is 365 days.
certPeriodDays: 365

## TLS Certificate Option 3: Use your own self-signed certificate.
## certManager and autoGenerateCert must be disabled and certFile, keyFile, and caFile must be set.
## certFile, keyFile, and caFile must be set.
## The chart reads the contents of the file paths with the helm .Files.Get function.
## Refer to this doc https://helm.sh/docs/chart_template_guide/accessing_files/ to understand
## limitations of file paths accessible to the chart.
Expand Down
Loading