add certutils.CertReloader #11
Conversation
certutils/reloader.go
Outdated
| } | ||
|
|
||
| func (cr *CertReloader) setCertificate() error { | ||
| log.Println("Loading TLS certificates...") |
There was a problem hiding this comment.
I don't think libraries should be logging. It should at least be a toggle. Could also make the constructor take a logger.
P.S. I really wish go authors had made log an interface.
There was a problem hiding this comment.
I agree and spent a better part of sunday thinking about it. I'll drop it for now.
CertReloader provides a background go-routine that will automatically reload TLS key+cert files when they're changed on disk. It implements the GetCertificate() and GetClientCertificate() funcs used by tls.Config to dynamically load key+cert data as needed. Example provided as usage/documentation.
joemiller
force-pushed
the
cert-reloader
branch
from
e302ebd to
0b2d840
Compare
There was a problem hiding this comment.
Really excited about this feature.
I left some comments/concerns in the review. Don't let my comments block a merge if its current form fixes a problem for us.
Thanks for doing this @joemiller ! 🚀
| if err != nil { | ||
| return err | ||
| } | ||
| err = cr.watcher.Add(cr.certFile) |
There was a problem hiding this comment.
It seems that if cr.watcher.Add(cr.certFile) errs, the watchCertificate() goroutine cannot recover. On the next iteration of the loop, it will wait forever to receive from either channel (and neither of which will ever send).
| if err != nil { | ||
| cr.Error <- err | ||
| } | ||
| err = cr.resetWatcher() |
There was a problem hiding this comment.
Why are we resetting the watcher after every event?
There was a problem hiding this comment.
Unknown. This code is copied from kelseyhightower's POC code (link is at top of the source code). I've poked around and I have not figured out why he was doing that yet.
There was a problem hiding this comment.
I couldn't find documentation to support my conjecture, but I suspect it's either a lazy debounce or a multi-platform support hack (it looks like it shouldn't be required on linux as it uses inotify_add_watch(2) which watches paths).
| return nil, err | ||
| } | ||
|
|
||
| go cr.watchCertificate() |
There was a problem hiding this comment.
Should we wrap cr.watchCertificate() with a func to catch its error?
| cr := &CertReloader{ | ||
| certFile: certFile, | ||
| keyFile: keyFile, | ||
| Error: make(chan error, 10), |
There was a problem hiding this comment.
The watchCertificate() goroutine will stop processing when this buffer fills. Perhaps we should add a timeout to sending on this channel so we drop errors when the user of the library fails to read from this channel?
There was a problem hiding this comment.
In general I am super uncomfortable with the Error channel. I spent half my labor day weekend trying to think of a better way to handle this. I would prefer to find a way to make the reloader uber simple and uber transparent to callers. That is, I want it to be just as easy as using TLS with the standard "load once" approach.
I'm open to suggestions on how to simplify the error handling.. some current thoughts, though not super happy about any of them:
- just fatal out on errors.. but having a library exit your app is no good.
- just log the errors ... optionally allow the user to supply a logger, else use a default logger
- continue to provide the Error channel for callers that want to implement their own handling of errors. For callers that don't consume the error channel, setup a go routine that simply logs errors for them.
- no Error channel, but force the caller to provide a callback func to execute on any errors.
all of these have some kind of additional problem/tradeoffs though.
thoughts? /cc @spheromak too
There was a problem hiding this comment.
# 1 has another option:
Rather than hiding the watcher goroutine from the user, require the user to call it. Then the watcher goroutine could bubble errors back to the user of the library merely by exiting (and leave it up to the user of the library how they want to handle that).
Other functions in the stdlib follow a similar pattern to the above (e.g. http.ListenAndServe).
# 2 and # 4 are similar, and either would be fine IMHO. Slight preference for the callback though as I think it offers some implicit intuitive flexibility (e.g. only call the callback if callback exists, otherwise ignore the error). This pattern is found in the stdlib too (it's what we're implementing in this PR with tls.Config)
# 3 would be a little quirky IMHO. I think I would expect a library I use not to log unless I ask it to.
|
Found this, https://github.com/cloudflare/certinel/blob/master/README.md at first glance it looks to meet the same objective. Open sourced by cloudflare. It implements the "provide a callback for file read errors" approach. I will try this out and if it meets the goal I'm going to close this pr |
|
@joemiller should we come back to this with the certinel or just close this out with that being our way forward ? |
CertReloader provides a background go-routine that will automatically
reload TLS key+cert files when they're changed on disk. It implements the
GetCertificate() and GetClientCertificate() funcs used by tls.Config to
dynamically load key+cert data as needed.
Example provided as usage/documentation.