fsnotify-backed recursive watcher for Linux #5833
Conversation
adriansr
added
Auditbeat
in progress
|
|
||
| type nonRecursiveWatcher fsnotify.Watcher | ||
|
|
||
| func (_ *nonRecursiveWatcher) Start(done <-chan struct{}) error { |
There was a problem hiding this comment.
receiver name should not be an underscore
| {"/lib/libz.a", false}, | ||
| } | ||
|
|
||
| result := make([]visitParams, 0) |
There was a problem hiding this comment.
can probably use "var result []visitParams" instead
| {"/usr/bin/tar", false}, | ||
| } | ||
|
|
||
| result := make([]visitParams, 0) |
There was a problem hiding this comment.
can probably use "var result []visitParams" instead
| {"/a/b", true}}}, | ||
| } { | ||
| failMsg := fmt.Sprintf("test at index %d", idx) | ||
| result := make([]visitParams, 0) |
There was a problem hiding this comment.
can probably use "var result []visitParams" instead
| } | ||
|
|
||
| for _, order := range []VisitOrder{PreOrder, PostOrder} { | ||
| result := make([]visitParams, 0) |
There was a problem hiding this comment.
can probably use "var result []visitParams" instead
|
|
||
| type FileTree map[string]FileTree | ||
|
|
||
| type VisitFunc func(path string, isDir bool) error |
There was a problem hiding this comment.
exported type VisitFunc should have comment or be unexported
| PathSeparator = string(os.PathSeparator) | ||
| ) | ||
|
|
||
| type FileTree map[string]FileTree |
There was a problem hiding this comment.
exported type FileTree should have comment or be unexported
| ) | ||
|
|
||
| const ( | ||
| PathSeparator = string(os.PathSeparator) |
There was a problem hiding this comment.
exported const PathSeparator should have comment (or a comment on this block) or be unexported
| type VisitOrder int8 | ||
|
|
||
| const ( | ||
| PreOrder VisitOrder = iota |
There was a problem hiding this comment.
exported const PreOrder should have comment (or a comment on this block) or be unexported
| "strings" | ||
| ) | ||
|
|
||
| type VisitOrder int8 |
There was a problem hiding this comment.
exported type VisitOrder should have comment or be unexported
adriansr
changed the title
| ) | ||
|
|
||
| var ( | ||
| PathSeparator = string(os.PathSeparator) |
There was a problem hiding this comment.
exported var PathSeparator should have comment or be unexported
adriansr
added
enhancement
and removed
in progress
adriansr
force-pushed
the
recursive/linux
branch
2 times, most recently
from
75f5afe to
b2235a7
Compare
| PathSeparator = string(os.PathSeparator) | ||
| ) | ||
|
|
||
| // FileTree represents a directory in a filesystem-tree structure |
There was a problem hiding this comment.
Please close your public godoc sentences with periods. https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences
|
|
||
| func TestNonRecursive(t *testing.T) { | ||
| dir, err := ioutil.TempDir("", "monitor") | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
assert.NoError doesn't immediately fail the test case so the test will continue running after the error. In most cases this isn't what you want so you should either not use assert.NoError or you need to add in some logic like if assert.NoError { assert something is correct } . I've seen this result in lots of test case panics and I mostly use if err != nil { t.Fatal(err) } now.
| func newRecursiveWatcher(inner *fsnotify.Watcher) *recursiveWatcher { | ||
| return &recursiveWatcher{ | ||
| inner: inner, | ||
| tree: FileTree{}, |
There was a problem hiding this comment.
I think the map needs to be initialised otherwise I get insert to nil map panics.
There was a problem hiding this comment.
Totally. I didn't realize that FileTree was a map when I made that comment.
| func (watcher *recursiveWatcher) Close() error { | ||
| if watcher.done != nil { | ||
| // has been Started(), goroutine takes care of cleanup | ||
| watcher.done <- true |
There was a problem hiding this comment.
Closing the done channel is the most common way to signal since it works to stop an unbounded number of goroutines.
| return errors.Wrap(err, "file tree recursion failed") | ||
| } | ||
| if info.IsDir() { | ||
| // TODO: can be a symlink? |
There was a problem hiding this comment.
You could add a test case for this to find out.
| switch event.Op { | ||
| case fsnotify.Create: | ||
| if err := watcher.addRecursive(event.Name); err != nil { | ||
| watcher.inner.Errors <- errors.Wrap(err, fmt.Sprintf("unable to recurse path '%s'", event.Name)) |
There was a problem hiding this comment.
There is a Wrapf method for doing this.
| } | ||
| } | ||
|
|
||
| func fixPath(path string) string { |
There was a problem hiding this comment.
I think you could use filepath.Clean().
adriansr
force-pushed
the
recursive/linux
branch
from
b2235a7 to
1b45737
Compare
|
Can you please add a change log entry for this. |
Adds a wrapper on top of fsnotify that allows to place recursive watches on directories. Exhibits similar behavior to FSEvents.
adriansr
force-pushed
the
recursive/linux
branch
from
1b45737 to
123c9fa
Compare
|
Also added a small explanation of the recursive flag to |
| @@ -85,4 +85,7 @@ | |||
| # sha224, sha256, sha384, sha512, sha512_224, sha512_256, sha3_224, sha3_256, | |||
| # sha3_384 and sha3_512. Default is sha1. | |||
| file.hash_types: [sha1] | |||
|
|
|||
| # Detect changes to files included in subdirectories. Disabled by default. | |||
| file.recursive: false | |||
There was a problem hiding this comment.
If you're adding it here then I would also put it into https://github.com/elastic/beats/blob/master/auditbeat/module/audit/file/_meta/docs.asciidoc#configuration-options.
This adds an explanation to the `file.recursive` flag and also updates the description of the underlying implementation for different OSes, to account for recent changes.
| * macOS (Darwin) - Uses the `FSEvents` API, present since macOS 10.5. This API | ||
| coalesces multiple changes to a file into a single event. {beatname_uc} translates | ||
| this coalesced changes into a meaningful sequence of actions. However, | ||
| in rare situations the reported events may have a different ordering than what |
Adds a wrapper on top of fsnotify that allows placing recursive watches on directories.
Exhibits similar behavior to FSEvents:
Same for removal vs moving out of a watched dir.
Issue #5421