Add support for NFLOG groups and "Operation not permitted" error

[MrPeck]

This pull requests comes to solve two issues: #1166 and #1167

[codecov]

Codecov Report

Merging #1169 (d1ea3b0) into dev (5cdf3b7) will decrease coverage by 0.03%.
Report is 2 commits behind head on dev.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##              dev    #1169      +/-   ##
==========================================
- Coverage   82.18%   82.16%   -0.03%     
==========================================
  Files         154      154              
  Lines       19927    19929       +2     
  Branches     7523     7540      +17     
==========================================
- Hits        16377    16374       -3     
+ Misses       3042     3039       -3     
- Partials      508      516       +8     

Flag	Coverage Δ
alpine315	68.64% <37.50%> (-0.01%)	⬇️
centos7	72.03% <44.44%> (-0.02%)	⬇️
fedora34	68.47% <37.50%> (-0.01%)	⬇️
macos-11	60.12% <27.27%> (-0.07%)	⬇️
macos-12	60.17% <27.27%> (-0.08%)	⬇️
macos-ventura	60.12% <33.33%> (-0.05%)	⬇️
mingw32	68.90% <37.50%> (-0.06%)	⬇️
mingw64	68.90% <37.50%> (-0.08%)	⬇️
npcap	82.14% <70.00%> (-0.09%)	⬇️
ubuntu1804	72.25% <37.50%> (-0.03%)	⬇️
ubuntu2004	69.28% <37.50%> (-0.04%)	⬇️
ubuntu2204	68.28% <37.50%> (+0.01%)	⬆️
ubuntu2204-icpx	59.18% <27.27%> (-0.07%)	⬇️
unittest	82.16% <63.63%> (-0.03%)	⬇️
windows-2019	?
windows-2022	82.19% <70.00%> (-0.04%)	⬇️
winpcap	82.18% <70.00%> (-0.01%)	⬇️
zstd	72.02% <44.44%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
Pcap++/src/PcapLiveDevice.cpp	78.01% <60.00%> (-1.22%)	⬇️
Pcap++/header/PcapLiveDevice.h	100.00% <100.00%> (ø)

... and 19 files with indirect coverage changes

[MrPeck]

Hey! I don't understand what failed on the workflow and how I can fix it. Besides that, are there any additional steps I need to take to merge the changes?

[egecetin]

@MrPeck It was because the workflow is blocked for first time contributors. Approved the workflow the run all checks. If everything passes it looks good to me. For merging @seladb will check the PR and decide to merge or request additional changes but since its holiday session it may take a while.

[seladb]

I have a more general question: is it possible to add tests for this feature? Our Linux CI currently uses docker containers so I guess this would be hard because nflog is driven by the kernel, am I wrong? 🤔

[MrPeck]

I think it is possible to add tests for this feature. But we would need a few things:

• Run with elevated privileges so we can open nflog

nflog events are generated by netfilter when an IPTables rule is triggered:

• Create an IPTables rule inside the container
• Trigger the rule

I am not familiar with the testing infrastructure in the project, do you think it would be possible to implement the aforementioned steps in the CI workflow?

[seladb]

PcapPlusPlus uses GitHub Actions as the CI platform. You can see the workflow file here: https://github.com/seladb/PcapPlusPlus/blob/master/.github/workflows/build_and_test.yml

Currently all Linux jobs run on a docker container, but maybe we can add a new job that runs in a VM and not in a docker container. This should be pretty easy, here are some docs that explain how to do it. When we create this job we can add the IP table rules etc. This should be the easy part...

The more complex part is how to run the test only in the new "nflog" job and not in the other tests. In order to do that you need to understand the PcapPlusPlus testing infrastructure. Each test has 1 or more "tags" that can be included or excluded in test runs. You can see these tags here: https://github.com/seladb/PcapPlusPlus/blob/master/Tests/Pcap%2B%2BTest/main.cpp
What we can do is add a new "nflog" tag for this test, and run all other tests with -x "nflog". Then in this job we can run it with -t "nflog".

As I think more about it, it seems like a lot of work... maybe we can skip tests for now and keep it as a future enhancement. @egecetin @MrPeck what do you think?

[egecetin]

I think separate PR for tests is a better idea. So, we can merge this PR to fix error. Since it also needs configuration of VM, testing the workflow locally is not easy, it will probably take some time, you have to push and wait again, again and again...

[MrPeck]

I agree that keeping tests for the future would be a better idea. And I would gladly take that task upon myself whenever it comes up :)

[seladb]

Sounds good, thanks! 👍

[seladb]

Thank you @MrPeck for adding this great feature to PcapPlusPlus, very much appreciated! 🙏 ❤️