Retain the error stack if checkIfCertShouldBeObtained returns an error

[ankon]

This allows an outside caller of GetCertificate to use errors.As to check for their own response and react accordingly.

In our case: The ondemand decision func checks a blocklist and might return something like BlockedDomainError{message:"This is blocked"}. This isn't an "error", it's just something that so happens to look like an error because that's what the decision func wants us to return.

In the GetCertificate implementation we call certmagic, and check the error it returns. If the error is "expected", then we don't really care about it and might just count it. If it is something interesting, we might want to tell a human about it.
The way that works is by having the GetCertificate function in a NewRelic "Transaction", and when we receive an error from certmagic we check whether the error is of one of the "normal" types, and if so call txn.NoticeExpectedError(err). If it is interesting, we call NoticeError(err).

This doesn't work for the errors from the decision func, because those lose their causal trace. The PR fixes that by using %w instead of %v to transmit the error outwards.

[ankon]

Example code illustrating what we do:

1. Control-flow error type BlockedHost:

type BlockedHost struct {
  name string
}

func (bh *BlockedHost) Error() string {
	return fmt.Sprintf("%s is blocked", bh.name)
}

2. Return that from the decision func, and check in GetCertificate:

	certmagicConfig.OnDemand = &certmagic.OnDemandConfig{
		DecisionContextFunc: func(ctx context.Context, name string) error {
			// ...				
			if (shouldBlockHost(name)) {
				return &BlockedHost{name}
			}
			// ...
		},
		// ...
	}

	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)
		if err != nil {
			var bhe *BlockedHost
			if errors.As(err, &bhe) {
				// Fine, happens once in while.
				txn.NoticeExpectedError(err)
			} else {
				// Something else!
				txn.NoticeError(err)
			}
		}
		// ...
	}

[mholt]

This is reasonable, thanks for the explanation!

My default is to not wrap errors because then they become part of the exported API but in this case I think it makes sense, since the function returning the error is one defined by the user/caller.

Edit: Oops, merge conflict -- @ankon do you want to tackle it, or I can if you're busy! :)

[ankon]

Will do it quickly.

FWIW: Instead of wrapping we could also just let the error get passed through completely with modification, which seems to be the case in most other places?

[mholt]

FWIW: Instead of wrapping we could also just let the error get passed through completely with modification, which seems to be the case in most other places?

Do you mean without modification? i.e. return err? That's definitely possible, the only downside is that we lose the natural "stack trace" that the error string gives us, like where in the code the errors are coming from.

Personally I'm happy with this as-is now, but am happy to approve another way if you'd prefer.

[ankon]

Do you mean without modification?

Indeed, I meant that. But, for now this is great, too -- there's always room to iterate further here if needed :)