-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Use fsnotify to detect model/policy files change in casbin plugin #614
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #614 +/- ##
==========================================
- Coverage 88.36% 88.19% -0.17%
==========================================
Files 115 115
Lines 5456 5457 +1
==========================================
- Hits 4821 4813 -8
- Misses 454 461 +7
- Partials 181 183 +2 ☔ View full report in Codecov by Sentry. |
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.
Could you update
htnn/plugins/tests/integration/casbin_test.go
Line 118 in 0483a64
time.Sleep(10 * time.Second) // TODO remove this once we switch the file change detector to inotify |
plugins/pkg/file/fs.go
Outdated
fs = &Fsnotify{ | ||
Watcher: watcher, | ||
} | ||
return |
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.
We don't need this return?
plugins/pkg/file/fs.go
Outdated
return | ||
} | ||
|
||
watcher := newFsnotify().Watcher |
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.
Why don't we use defaultFsnotify?
plugins/plugins/casbin/filter.go
Outdated
f.reloadEnforcer() | ||
}, conf.modelFile, conf.policyFile) | ||
if err != nil { | ||
api.LogErrorf("failed to update Enforcer: %v", err) |
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.
The msg is not precise. The code above doesn't update Enforcer
plugins/pkg/file/fs.go
Outdated
if !ok { | ||
return | ||
} | ||
logger.Info(fmt.Sprintf("event: %v", event)) |
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.
Just put the event as an argument is enough, we don't need to use Sprintf
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.
When I use logger.Info("event: %v", event)
, an error is occurred:
DPANIC file file/fs.go:100 odd number of arguments passed as key-value pairs for logging {"ignored key": "WRITE \"example3760250304\""}
so I use Sprintf.
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.
As the error message indicated, the arguments should be key-value pairs, which have even numbers, for example:
htnn/api/pkg/plugins/plugins.go
Line 73 in 1d5db16
logger.Info("register plugin type", "name", name) |
plugins/pkg/file/fs.go
Outdated
} | ||
logger.Info(fmt.Sprintf("event: %v", event)) | ||
onChange() | ||
return |
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.
We should keep watching, and should not exit after the first event. This is a waste of goroutine and watcher, and force the user to rewatch each time.
plugins/pkg/file/fs_test.go
Outdated
tmpfile.Close() | ||
assert.False(t, IsChanged(f)) | ||
tmpfile.Sync() | ||
mu.Lock() |
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.
Does the lock here guarantee the callback in WatchFiles run before assertion? The assertion may get the lock first.
plugins/pkg/file/fs.go
Outdated
logger.Error(err, "failed to close fsnotify watcher") | ||
} | ||
}(w) | ||
err := w.Add(files.Name) |
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.
Consider a case: people remove the file and then create a new file with the same name. The fsnotify won't be triggered in this case. What about watching the file's directory and filtering according to the name?
I think about it again. Using global fsnotify is tough for a beginner. We can implement a per-file fsnotify version first. |
plugins/pkg/file/fs.go
Outdated
) | ||
|
||
func IsChanged(files ...*File) bool { | ||
func Update(onChange func(), files ...*File) (err error) { |
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.
The Update is redundant, use WatchFiles is enough.
@lyt122 |
plugins/plugins/casbin/filter.go
Outdated
} | ||
}() | ||
if getChanged() { | ||
conf.reloadEnforcer() |
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.
Why don't we call this inside file.WatchFiles(func() {
?
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.
because if reloadenforcer
called in file.WatchFiles()
, the config won't be changed. So I need the flag
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.
I wonder why. Could you explain it?
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.
I don't know. In fact, this problem bother me for a long time.
plugins/plugins/casbin/filter.go
Outdated
@@ -35,33 +35,27 @@ type filter struct { | |||
config *config | |||
} | |||
|
|||
var ( | |||
Changed = false |
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.
We don't need this flag if we can do reloadEnforcer in the fsnotify's callback
plugins/plugins/casbin/filter.go
Outdated
ChangedMu.Unlock() | ||
role, _ := headers.Get(conf.Token.Name) // role can be "" | ||
url := headers.Url() | ||
err := file.WatchFiles(func() { |
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.
Why can't we move this part inside config.Init
?
// configuration is not changed, but file changed | ||
err = os.WriteFile(policyFile2.Name(), []byte(policy), 0755) | ||
require.Nil(t, err) | ||
|
||
//wait to run reloadEnforcer | ||
time.Sleep(5 * time.Second) |
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.
Ping @lyt122
Don't miss #614 (comment)
hdr := http.Header{} | ||
hdr.Set("customer", "alice") | ||
|
||
assert.Eventually(t, func() bool { | ||
resp, _ := dp.Post("/echo", hdr, strings.NewReader("any")) | ||
return resp != nil && resp.StatusCode == 200 | ||
}, 1*time.Second, 10*time.Millisecond) | ||
}, 3*time.Second, 1*time.Second) |
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.
}, 3*time.Second, 1*time.Second) | |
}, 3*time.Second, 100*time.Millisecond) |
Better to use shorter interval to reduce test time
@@ -88,6 +88,7 @@ func TestCasbin(t *testing.T) { | |||
assert.Equal(t, tt.status, 0) | |||
} else { | |||
assert.Equal(t, tt.status, lr.Code) | |||
assert.False(t, Changed) |
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.
When Changed
is removed, we can compare the field in conf
directly for test
plugins/pkg/file/fs.go
Outdated
func watchFiles(onChanged func(), file *File) { | ||
dir := filepath.Dir(file.Name) | ||
defer func() { | ||
storeWatchedFiles.lock.Lock() |
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.
The global storeWatchedFiles makes things complex. And the current impl is incorrect if multiple config watch the same file.
We can improve it like this:
- create a watcher which wraps the fsnotify when initing the config
- add files into the watcher
- start the watcher (remember to stop the goroutine once the watcher is closed)
@spacewander ready to review |
plugins/plugins/casbin/config.go
Outdated
runtime.SetFinalizer(conf, func(conf *config) { | ||
err := conf.watcher.Stop() | ||
if err != nil { | ||
api.LogErrorf("failed to close watcher, err: %v", err) |
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.
api.LogErrorf("failed to close watcher, err: %v", err) | |
api.LogErrorf("failed to stop watcher, err: %v", err) |
plugins/plugins/casbin/config.go
Outdated
e, err := casbin.NewEnforcer(conf.Rule.Model, conf.Rule.Policy) | ||
conf.watcher = watcher | ||
|
||
err = conf.watcher.AddFile(conf.modelFile, conf.policyFile) |
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.
Better to name it AddFiles if this method accept multiple files
plugins/pkg/file/fs.go
Outdated
return defaultFs.Stat(path) | ||
func (w *Watcher) Start(onChanged func()) { | ||
go func() { | ||
logger.Info("start watch files") |
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.
logger.Info("start watch files") | |
logger.Info("start watching files") |
plugins/pkg/file/fs.go
Outdated
for { | ||
select { | ||
case event, ok := <-w.watcher.Events: | ||
if !ok { |
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.
There is no need to add this branch as the watcher doesn't close the Events.
plugins/pkg/file/fs.go
Outdated
logger.Info("file changed: ", "event", event) | ||
onChanged() | ||
case err, ok := <-w.watcher.Errors: | ||
if !ok { |
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.
Ditto
@spacewander ready to review |
plugins/pkg/file/fs_test.go
Outdated
assert.False(t, IsChanged(f)) | ||
time.Sleep(1000 * time.Millisecond) | ||
|
||
tmpfile, _ := os.CreateTemp("./", "example") |
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.
tmpfile, _ := os.CreateTemp("./", "example") | |
tmpfile, _ := os.CreateTemp("", "example") |
would be better so that the test case won't pollute the git repo.
plugins/pkg/file/fs.go
Outdated
for { | ||
select { | ||
case event := <-w.watcher.Events: | ||
if _, exists := w.files[event.Name]; exists { |
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.
The test case doesn't work on Mac. I spent some time investigating it and found that the event.Name is example1104322652
and the map is map[./example1104322652:true]
. Let's convert the input file name to absolute path.
plugins/pkg/file/fs.go
Outdated
if _, exists := w.files[file.Name]; !exists { | ||
w.files[file.Name] = true | ||
} | ||
dir, err := filepath.Abs(file.Name) |
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.
Missing a Dir call?
plugins/pkg/file/fs.go
Outdated
for { | ||
select { | ||
case event := <-w.watcher.Events: | ||
if event.Op&fsnotify.Chmod == 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.
If the Op is Chmod | SomethingElse
, the SomethingElse event will be ignored.
plugins/pkg/file/fs.go
Outdated
for { | ||
select { | ||
case event := <-w.watcher.Events: | ||
if event.Op.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.
Does this solve the issue that the Op both has Chmod and SomethingElse flags? In this case, the SomethingElse event will be dropped.
plugins/pkg/file/fs.go
Outdated
if event.Op&fsnotify.Chmod != 0 { | ||
event.Op &= ^fsnotify.Chmod // Remove the Chmod bit | ||
if event.Op == 0 { | ||
continue // Skip if it was only a Chmod event |
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.
This code works but why don't we just use event.Op == fsnotify.Chmod
?
Merged. Thanks! |
To fix #556