-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix incorrect use of fsnotify in configtls #8439
Conversation
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.
Thanks for the fix! Please add a changelog entry for the bug fix 👍🏻
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
@@ -112,7 +112,7 @@ func (r *clientCAsFileReloader) handleWatcherEvents() { | |||
// NOTE: k8s configmaps uses symlinks, we need this workaround. | |||
// original configmap file is removed. | |||
// SEE: https://martensson.io/go-fsnotify-and-kubernetes-configmaps/ | |||
if event.Op == fsnotify.Remove || event.Op == fsnotify.Chmod { | |||
if event.Has(fsnotify.Remove) || event.Has(fsnotify.Chmod) { |
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.
My confusion here is why we need to monitor for the chmod...
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.
Possibly because of this fsnotify/fsnotify#421?
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.
Inotify generates IN_DELETE_SELF
for the watched file only when its link count is reduces to 0. Looks like it is used to handle this situation. 🤔
https://github.com/fsnotify/fsnotify/blob/v1.6.0/backend_inotify.go#L26-L33
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.
Thanks @haoqixu!
Description: fix #8438
Link to tracking Issue: #8438