NVSHAS-7554 - Added a catch to block identical filter paths from bein… #934
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…g stored through the REST API. The REST api will make sure the directory path for filters do not have any collisions. The bug we hit was that if a customer was allowed to add both `/tmp/` and `/tmp` but only one filter would be used. This caused confusion in the expected behaviour. If the filter has a trailing /, it is meant to be a filter for directories. If it is ommited, it is meant to be a filter for filepaths. When the two are used in conjection, it is confusing because we can only store one filter and the behaviour may not be what the user expects. Added comments to parseFileFilter so its clear what are the behaviours if you add a trailing /. I cleaned up getCoreFile() because it had duplicated code.
jaimeyu-suse
commented
Jul 31, 2023
| @@ -227,7 +238,7 @@ func handlerFileMonitorConfig(w http.ResponseWriter, r *http.Request, ps httprou | |||
| } | |||
|
|
|||
| for i, cfilter := range profConf.Filters { | |||
| if cfilter.Filter == filter.Filter { | |||
| if (cfilter.Filter == filter.Filter) || (cfilter.Path == base) { | |||
There was a problem hiding this comment.
Note that Filter is not the path, this is the pseudo regex expression. eg: /tmp/.*
While the additional check if Path is the same as base will throw an error if the path is the same. The reason is that the monitoring logic can only store one filter per directory.
There was a problem hiding this comment.
Please add some test cases and make sure this logic does help.
jaimeyu-suse
commented
Jul 31, 2023
| @@ -575,8 +575,7 @@ func (w *FileWatch) getCoreFile(cid string, pid int, profile *share.CLUSFileMoni | |||
| dirList := make(map[string]*osutil.FileInfoExt) | |||
| singleFiles := make([]*osutil.FileInfoExt, 0) | |||
|
|
|||
| // get files and dirs from all filters | |||
| for _, filter := range profile.Filters { | |||
| addFilterFn := func(filter share.CLUSFileMonitorFilter){ | |||
There was a problem hiding this comment.
This is cosmetic to reduce the copy paste code. No changes in the logic was done and will make it easier to make changes in the future.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…g stored through the REST API.
The REST api will make sure the directory path for filters do not have any collisions. The bug we hit was that if a customer was allowed to add both
/tmp/and/tmpbut only one filter would be used. This caused confusion in the expected behaviour.If the filter has a trailing /, it is meant to be a filter for directories. If it is ommited, it is meant to be a filter for filepaths. When the two are used in conjection, it is confusing because we can only store one filter and the behaviour may not be what the user expects.
Added comments to parseFileFilter so its clear what are the behaviours if you add a trailing /.
I cleaned up getCoreFile() because it had duplicated code.