watch directories #2589
watch directories #2589
Conversation
drewwells
force-pushed
the
feature/fileEvents
branch
2 times, most recently
from
02a948c to
c1c1b38
Compare
|
If this looks like something that could be merged to the trunk, I will work on tests to verify expected behavior. |
Watching files only works in situations where standard files are in use. In k8s, configmaps are mounted via a set of symlinks. In those situations, you will only get file events when watching the directory containing the symlink. Since by default, configmaps are mounted as directories, I don't expect many people to find regressions when using this. Signed-off-by: Drew Wells <drew.wells00@gmail.com>
drewwells
force-pushed
the
feature/fileEvents
branch
from
c1c1b38 to
d996b48
Compare
There was a problem hiding this comment.
Thanks for the PR!
One aside for anyone who stumbles across this, generally speaking it isn't recommended to use the --watch feature for production usage for loading data/policies. Typically you would want to use the bundles: https://www.openpolicyagent.org/docs/latest/management/#bundles
The watch functionality is primarily intended for quick feedback/interactive policy authoring (eg. Start OPA and point to my local bundle source, edit files and either evaluate in the REPL or call some API to test the changes).
I left a few comments that we'll want to address, and then unit tests would be the last piece.
| // K8s likes to double symlink, resolve the symlink and fsnotify will resolve the next one | ||
| realPath, err := filepath.EvalSymlinks(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🤔 Couple things we should consider here. First is that OPA is by no means specific to K8s, so having behavior like this seems a little bit out of place. Second is that we are then relying on some implicit behavior on fsnotify to do a single resolve on our behalf.. which IMO isn't ideal.
If we move ahead with these changes we might as well go all the way and recursively resolve symlinks until we get to a "real" file/directory. AFAIK that should give the most consistent behavior across all the platforms OPA supports and runtimes (like k8s) where it gets deployed.
There was a problem hiding this comment.
The real file won't change either. K8s creates new files in different directories, and updates a symlink. The symlink event tends not to be registered so watching the directory is required. Watching a directory is hard to do with the implied notion that directories are bundles in the loader file.
| // Dirs converts all filepaths to directories to use in the file watcher. | ||
| // Some types of events will not register on the files themselves, so | ||
| // watch the directory to prevent this problem | ||
| func Dirs(paths []string) []string { |
There was a problem hiding this comment.
This function isn't specific to the file watcher, right? If its just giving the set of parent directories for some list of paths we should update the comment to basically explain that.
| unique := map[string]struct{}{} | ||
|
|
||
| for _, path := range paths { | ||
| // TODO: /dir/dir will register top level directory /dir |
There was a problem hiding this comment.
What is the action for the TODO here? Should it not return the parent directory?
There was a problem hiding this comment.
If you say watch a directory, which is supported today. The logic here will watch the parent directory instead. It may be fine, I'll convert this to a comment instead of a TODO.
| var u []string | ||
| for k := range unique { | ||
| u = append(u, k) | ||
| } | ||
| return u |
There was a problem hiding this comment.
🤔 Do we need to filter out unique paths? IIUC fsnotify handles that for us if we Add watches for duplicates.
|
@drewwells any update? |
|
Going to close this since there haven't been any updates. Please re-open if/when work continues on the changes. |
|
I'll update this, my email notifications appear to be broken. I can't re-open the PR. On the watch piece, we are using bundles with watch. We also plan to use this in production this way. Are there specific concerns with this or is it simply a less than ideal way to run OPA? For context, we are attempting to use the k8s API to manage the bundles for OPA. Since OPA does not support talking directly to k8s API, we are mounting a k8s configmap to a volume. As k8s updates the configmap, the mounted path gets an update and OPA reloads the bundle. Another option is to support speaking k8s inside OPA. There's some benefits to both approaches, but OPA is easier to integrate with the file mounts. Thoughts? |
|
@patrick-east can you open the PR? Tests are fixed. |
|
🤔 I don't have an option for it, github shows: Some quick googling seems to indicate there are work-arounds like: https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c |
|
My local git does not have the original sha, I'll open a new PR. |
Watching files only works in situations where standard files are in
use. In k8s, configmaps are mounted via a set of symlinks. In those
situations, you will only get file events when watching the directory
containing the symlink. Since by default, configmaps are mounted as
directories, I don't expect many people to find regressions when using this.
Signed-off-by: Drew Wells drew.wells00@gmail.com