ROX-33471: do not attach progs when no paths configured#371
ROX-33471: do not attach progs when no paths configured#371
Conversation
Molter73
left a comment
Molter73
left a comment
There was a problem hiding this comment.
Thanks for adding tests!
|
|
||
| paths_globset: GlobSet, | ||
|
|
||
| links: Vec<LsmLink>, |
There was a problem hiding this comment.
For future reference, if we ever decide to use programs other than lsm, aya provides a Link trait that can be used here to store different types of links.
| LpmTrie::try_from(path_prefix)?; | ||
|
|
||
| // Add the new prefixes | ||
| let paths_config = self.paths_config.borrow(); |
There was a problem hiding this comment.
We can probably move this borrow to line 122 and directly use that in the first if condition instead, just so we are not borrowing and dropping the borrow multiple times across the call.
This should also be safe because updating the self.paths_config value happens on a completely separate branch of the worker select! call.
| let mut links = Vec::new(); | ||
| for (_name, prog) in self.obj.programs_mut() { | ||
| match prog { | ||
| Program::Lsm(prog) => prog.attach()?, | ||
| Program::Lsm(prog) => { | ||
| let link_id = prog.attach()?; | ||
| links.push(prog.take_link(link_id)?); | ||
| } | ||
| u => unimplemented!("{u:?}"), | ||
| }; | ||
| } | ||
| self.links = links; |
There was a problem hiding this comment.
I'm gonna go full nerd here, iterators FTW!
| let mut links = Vec::new(); | |
| for (_name, prog) in self.obj.programs_mut() { | |
| match prog { | |
| Program::Lsm(prog) => prog.attach()?, | |
| Program::Lsm(prog) => { | |
| let link_id = prog.attach()?; | |
| links.push(prog.take_link(link_id)?); | |
| } | |
| u => unimplemented!("{u:?}"), | |
| }; | |
| } | |
| self.links = links; | |
| self.links = self | |
| .obj | |
| .programs_mut() | |
| .map(|(_, prog)| match prog { | |
| Program::Lsm(prog) => { | |
| let link_id = prog.attach()?; | |
| prog.take_link(link_id) | |
| } | |
| u => unimplemented!("{u:?}"), | |
| }) | |
| .collect::<Result<_, _>>()?; |
That final transmutation on .collect::<Result<_, _>>() also means that if we hit an error iteration will stop immediately, the links that had been created that far should also be dropped and the programs detached.
| # Remove all paths | ||
| config, config_file = fact_config | ||
| config['paths'] = [] | ||
| reload_config(fact, config, config_file, delay=0.5) |
There was a problem hiding this comment.
Is the extended delay necessary for the programs to be fully detached? Or is there something more to it? Maybe we can add a comment depending on the reason?
| sleep(1) | ||
|
|
||
| assert server.is_empty(), 'Should not receive events with no paths configured' |
There was a problem hiding this comment.
Consider using pytest.raises here, something like this:
| sleep(1) | |
| assert server.is_empty(), 'Should not receive events with no paths configured' | |
| e = Event(process=p, event_type=EventType.OPEN, | |
| file=fut, host_path=fut) | |
| with pytest.raises(TimeoutError): | |
| server.wait_events([e]) |
This will also fail if we get any other event, so it is a strict check.
https://docs.pytest.org/en/7.1.x/how-to/assert.html#assertions-about-expected-exceptions
| # Write to a file — should NOT produce events | ||
| with open(fut, 'w') as f: | ||
| f.write('This should be ignored') | ||
| sleep(1) | ||
|
|
||
| assert server.is_empty(), 'Should not receive events after removing paths' |
There was a problem hiding this comment.
Similar to my previous comment on using pytest.raises.
An additional is that server.wait_events has a timeout of 5 seconds I believe, we might want to make the timeout configurable and call it with just 1 second in these tests.
2 participants
Description
Ensures that Fact will not emit any events when there are no paths configured. Programs are loaded, but not attached. When a path is configured, they are attached and the links are stored. If there are no paths configured, the links are dropped (detaching the programs.)
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
New tests cover the important cases: no paths -> add path. some paths -> remove all.