EVEREST-1180 | TLS support #888
Conversation
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
|
|
||
| // Start the certificate files watcher until the context is closed. | ||
| func (w *certWatcher) Start(ctx context.Context) error { | ||
| watcher, err := fsnotify.NewWatcher() |
There was a problem hiding this comment.
are we really sure that you will get there filesystem notify signal in case certs mounted to Everest pod are changed?
We have re-check this with K8S because I remember earlier not everywhere it worked.
I remember NGINX recommended to implement it in the following way:
- use sidecar container
- subscribe in K8S API to event "secret changed"
- re-read and write new secret data to
- send signal to the main container to re-read certs.
There was a problem hiding this comment.
are we really sure that you will get there filesystem notify signal in case certs mounted to Everest pod are changed?
Yes, please see: https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-files-from-a-pod
When a volume contains data from a Secret, and that Secret is updated, Kubernetes tracks this and updates the data in the volume, using an eventually-consistent approach.
.. and I also tested this on GKE and k3d before opening this PR.
earlier not everywhere it worked.
Yes, earlier, I don't think it worked very well. I have also had to use some sidecar watcher. But Kubernetes has fixed it to make this work more reliably, I can't remember from which version, but its probably 1.18.
Another issue is that getting this signal takes about a minute or so, its not instant. However, for TLS certs, its fine, because usually they are renewed much ahead of the actual expiry. I would not complicate this by introducing a sidecar, etc.
This fsnotify mechanism is also used almost everywhere in most popular Kubernetes projects, like controller-runtime, so I wouldn't worry
| cert, err := tls.LoadX509KeyPair(w.certFile, w.keyFile) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
it seems w.mutex.Lock() should be only there, no need to lock it before loading certs
There was a problem hiding this comment.
should be only there
should be where?
no need to lock it before loading certs
Why not? If someone reads it while it is being written, they can potentially read corrupted or inconsistent data. By locking before loading the cert, we prevent readers from reading the cert until after the loading is complete. IMO this lock is needed here, otherwise it makes no sense to have just a read lock and not protect the writes.
Please note that the other lock we have (in GetCertificates) is a read lock - meaning that multiple reader goroutines can read without blocking each other. While we have only 1 writer routine, we still need the write lock here so that the readers don't read bad data. Let me know if I missed something
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
No description provided.