feat: make readiness probe optional

[ThatsMrTalbot]

No description provided.

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from thatsmrtalbot. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[SgtCoDFish]

I've had a think about this and dug in a bit!

tls Check

The tls check doesn't seem hugely helpful to preserve as a readiness check in the pure runtime config scenario.

Once it passes it'll never fail by the looks of it. Only context cancellation (on shutdown) will set the tlsConfig to nil which is the only way that the check can fail.

server Check

Likewise, the other check, server only checks the "ready" bool, which is set to true after initial bootstrapping and then only unset on shutdown.

Overall

The checks aren't ideal. The TLS check would be better if it returned true if the cert was still valid and the server check would be better if it only returned true when an issuer was configured.

That makes it ok to disable them like this, I think. We're not losing much.

That said, I think the optics of fully disabling a readiness check which is already defined (which we'd have to do for pure runtime configuration) aren't great though. People will (reasonably) question it.

Alternative

An alternative is:

1. When using pure runtime config, we short-circuit the readiness checks we already have to return a healthy response until the first time an issuer is configured. This is basically just for Helm.
2. When an issuer is configured, we no longer short circuit and the readiness checks revert to what they are now.
3. Down the line (after all this is merged) we improve those checks to be more helpful. Specifically, we should return not ready if the configmap defining the issuer is then deleted (but that's a separate piece of work)

In that scenario, I think we'd want the readiness checks enabled and this new helm value wouldn't be used. That's a little more work for us now, but is it better for users?

What do you think? I feel like that's a lot of words, sorry 🙃

[ThatsMrTalbot]

@SgtCoDFish

I originally was planning to alter the checks if the issuer was not set, however we default the issuer in the Helm config. This means a user would have to explicitly unset three Helm values to get into this mode.

An alternative is we could add an option app.certmanager.issuer.enabled that defaults to true, then not set the flags if it is false.

From there we could make the changes to the checks you describe.

Thoughts?

[SgtCoDFish]

I originally was planning to alter the checks if the issuer was not set, however we default the issuer in the Helm config. This means a user would have to explicitly unset three Helm values to get into this mode.

I think this is a bit of a tangent to the issue at hand, no? Like, it's not a great UX and I rather that the issuer didn't have a default value but we're kinda forced to accept it now for backwards compat reasons.

Although I guess it's a better UX to have app.certmanager.issuer.enabled and just have to set it to false than having to do the blanking. I think I'd support that!

[ThatsMrTalbot]

Superseded by #395