Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EVEREST-1180 | TLS support #888
base: main
Are you sure you want to change the base?
EVEREST-1180 | TLS support #888
Changes from all commits
dfaa2d2
e980bc0
f465bd7
ecde075
0caa254
5ef5aee
6ad2b4d
46fbf85
0615010
75d3aca
8dcec89
c6411e3
385a8c2
d6cafb5
c3172ed
eee377a
02a1661
9783325
15e9e06
beea433
1dc7fe6
8d6001b
327a819
5607d40
506d8eb
62a3f51
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems
w.mutex.Lock()
should be only there, no need to lock it before loading certsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be where?
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 somethingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please see: https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-files-from-a-pod
.. and I also tested this on GKE and k3d before opening this PR.
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