Optionally pass the context argument down to the OnDemand decision func

[ankon]

For further instrumenting our code base we would like to be able to pass the context we have when getting a certificate down into the "decision func" that checks whether a certificate should be obtained.

This PR is a bit of a "let's talk": I think this could just work, and wouldn't produce problems for existing code that only configures a DecisionFunc, but it can possibly be done nicer?

[ankon]

Quick update: We tested this, and it does what it should do -- we can now use this code inside our configuration:

    certmagicConfig.OnDemand = &certmagic.OnDemandConfig{
        DecisionContextFunc: func(ctx context.Context, name string) error {
            segment := newrelic.FromContext(ctx).StartSegment("TLS/OnDemandDecisionFunc")
            defer segment.End()
            // ...
        },
        // ...
    }

    tlsConfig.GetCertificate = func(clientHelloInfo *tls.ClientHelloInfo) (*tls.Certificate, error) {
        txn := newrelicApp.StartTransaction("TLS/GetCertificate")
        defer txn.End()
        // Add attributes related to the request
        txn.AddAttribute("hostname", clientHelloInfo.ServerName)

        // Get a certificate
        ctx := newrelic.NewContext(clientHelloInfo.Context(), txn)
        cert, err := certmagicConfig.GetCertificateWithContext(ctx, clientHelloInfo)
        // ...
    }

[francislavoie]

FWIW I do agree this is a good idea.

On the Caddy forums, someone recently inquired about getting the client IP as part of the ask request, which was impossible to add without having some kind of context in which we could pull from. Hopefully we can get at the current TLS handshake info (for the Conn/RemoteAddr) via context? That would be handy to have. (More of a reminder for @mholt since this is in the same ballpark)

[ankon]

The "ask" request is a caddy thing, right? As much as you can get all that data from the ClientHelloInfo, I guess you would need to change caddy then to pass it along as request parameters?

(Not to side-track this too much: This PR is just a tiny thing inside certmagic, so quite a step away from the "ask" I would assume)

[mholt]

Oh man my heart sank when I saw this notification come in because I thought I forgot about an open PR from Framer from like, months ago... but it's only from an hour ago :sweat_smile: So that's a relief.

Ok, yeah, this is a good change.

Technically, CertMagic isn't 1.0 yet so I've been OK with making breaking changes even recently, especially those which are very easy to work around (by adding an empty parameter for example). So I'd be OK with just changing DecisionFunc. I'd almost rather do that than have to deal with the removal and make people change their code twice, because I know I won't want both when we tag 1.0. Which I should look into doing in 2024. I think it'll be time soon...

Anyway, how do you feel about just changing DecisionFunc directly then?

EDIT: Just saw the reply above.

The "ask" request is a caddy thing, right? As much as you can get all that data from the ClientHelloInfo, I guess you would need to change caddy then to pass it along as request parameters?

Yeah, it's a Caddy thing that sets up a DecisionFunc. We would modify Caddy for that. I'll talk to Francis about it.

[ankon]

Anyway, how do you feel about just changing DecisionFunc directly then?

If we don't need compatibility here, then yes, absolutely: There is really no reason to have the non-context variant except for that compatibility/ease-of-upgrading. If you don't care about the context an implementor could easily ignore it, and it should be trivial to update.

[mholt]

Yeah, let's just replace DecisionFunc with the new signature. It's an easy change and to be expected before 1.0. Much better now than later.