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

[theSuess]

This PR adds a check for the presence of the cert-manager.io/v1 API in the target cluster. Using this approach, the helm chart can be installed without any customization in clusters without cert manager, simplifying the installation.

[petewall]

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]

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]

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]

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]

you found much better names than me, I love it

[theSuess]

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

[theSuess]

Updated this PR to use certificateSource with the possible values of:

• prefer-cert-manager
• cert-manager
• helm-generated
• self-signed
  to be very explicit about the choice

also added validation errors if CRDs are not available or self-signed is chosen but no files are provided

[theSuess]

If this approach is good for everyone, I'll also update the docs

[cyrille-leclerc]

If I understand correctly, we would have the following developer experience:

TODAY

# TODAY - SELF SIGNED SETUP, discouraged in production
admissionWebhooks:
  ...
  certManager:
    enabled: false # Don't use cert-manager (default: true)
  autoGenerateCert:
    enabled: true # activate cert auto-generation (default: true)
    recreate: true # prevent self-signed certs trust problems that are frequent in dev  (default: true)

# TODAY - CERT MANAGER SETUP, recommended in production
admissionWebhooks:
  ...
  certManager:
    enabled: true #  (default: true)
    ... add more config if needed
  autoGenerateCert:
    enabled: false #  (default: true)

PR PROPOSAL

introduce

certificateSource: 'prefer-cert-manager', 'cert-manager', 'helm-generated', or 'self-signed'

# PR PROPOSAL - CERT MANAGER REQUIRED, recommended in production
admissionWebhooks:
  ...
  certificateSource: cert-manager # Require cert-manager
  certManager:
    ... add more config if needed

# PR PROPOSAL - CERT MANAGER PREFERED, FALL BACK TO SELF SIGNED, ok in production, great to get started
admissionWebhooks:
  ...
  certificateSource: prefer-cert-manager # use cert-manager if discovered during install, fallback to self signed
  certManager:
    ... add more config if needed

# PR PROPOSAL - SELF SIGNED, FALL BACK TO SELF SIGNED, discouraged in production
admissionWebhooks:
  ...
  certificateSource: self-signed # use self signed

---

I also have a question: what about the migration path? shouldn't we deprecated rather than immediately removing the entries in values.yaml ?

[theSuess]

If we can come up with a good priority ordering, I'm fine with deprecating the fields instead, but the templating logic will grow a lot more complex

[cyrille-leclerc]

If we can come up with a good priority ordering, I'm fine with deprecating the fields instead

I would say that certificateSource has the priority when defined and otherwise, we honor admissionWebhooks/certManager/enabled and admissionWebhooks/autoGenerateCert/enabled.
WDYT?

but the templating logic will grow a lot more complex

Would the prioritization above make the logic as simple as possible to offer backward compatibility?

[theSuess]

If I understand you correctly, this would make certificateSource optional. This would work and shouldn't be too complicated but in that case the default behavior would be the deprecated one which doesn't feel quite right.

As an alternative we can set certificateSource to cert-manager by default until the fields are removed. This effectively gives us the same behavior with stricter validation and better error messages

[cyrille-leclerc]

If I understand you correctly, this would make certificateSource optional. This would work and shouldn't be too complicated but in that case the default behavior would be the deprecated one which doesn't feel quite right.

As an alternative we can set certificateSource to cert-manager by default until the fields are removed. This effectively gives us the same behavior with stricter validation and better error messages

You have a good point. I'm thinking of an incremental approach:

• First, introduce certificateSource: null, recommending it while not deprecating the existing /certManager/enabled and /autoGenerateCert/enabled and make certificateSource overwrite /certManager/enabled and /autoGenerateCert/enabled when defined
• Later, deprecate /certManager/enabled and /autoGenerateCert/enabled
• Eventually remove /certManager/enabled and /autoGenerateCert/enabled, mking he helm deployment fail if one of them is defined, and define the default value for certificateSource: cert-manager as you rightfully suggested to be equivalent to the current setup or consider defaulting to prefer-cert-manager to be moe lenient.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[theSuess]

Rebased on main and marked as ready for review. From the implementation side, the templates in this PR work but we still need to reach consensus on how this behavior can be best introduced in a backwards compatible way