Skip to content
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

modified FORWARD policy name to ACCEPT and enabled by default ESTABLISHED #337

Merged
merged 5 commits into from
Nov 12, 2020

Conversation

smagnani96
Copy link
Collaborator

FIX #331
ENHANCE #334

Tried quickly with docker s41m0n/polycube:fulvio, if someone can help testing this and modifying all the tests accordingly would be amazing.

Signed-off-by: Simone Magnani simonemagnani.96@gmail.com

@goldenrye
Copy link
Contributor

goldenrye commented Sep 22, 2020 via email

@acloudiator acloudiator added this to the v.0.11.0 milestone Oct 4, 2020
@acloudiator acloudiator requested review from a team and FedeParola October 4, 2020 21:36
@acloudiator acloudiator added the bug Something isn't working label Oct 4, 2020
@goldenrye
Copy link
Contributor

Accept the 'accept' as default to minimize the impact to deployment

@frisso
Copy link
Contributor

frisso commented Nov 3, 2020

/rebase

…SHED

Signed-off-by: Simone Magnani <simonemagnani.96@gmail.com>
@polycube-bot
Copy link
Collaborator

Rebase status: success!

Briefly, before starting the test it has been added the DROP policy both to INGRESS and EGRESS path, in order to make the test run

Signed-off-by: Simone Magnani <simonemagnani.96@gmail.com>
Signed-off-by: Simone Magnani <simonemagnani.96@gmail.com>
@acloudiator acloudiator changed the base branch from master to release/v0.11.0 November 4, 2020 17:54
@acloudiator
Copy link
Contributor

/rebase

@polycube-bot
Copy link
Collaborator

Rebase status: success!

@smagnani96 smagnani96 force-pushed the firewall/fixes branch 3 times, most recently from 53147c1 to 74a61e7 Compare November 5, 2020 22:45
Signed-off-by: Simone Magnani <simonemagnani.96@gmail.com>
@smagnani96
Copy link
Collaborator Author

++++TEST ./../src/services/pcn-firewall/test/ping/test_ping_5.1.sh Failed++++
cause: unknown

++++TEST ./../src/services/pcn-firewall/test/tcp/test_tcp_4.sh Failed++++
cause: netcat connection time out

@FedeParola Can you help checking these two tests please? This is taking more time than expected

@FedeParola
Copy link
Collaborator

FedeParola commented Nov 6, 2020

I may have found the cause of the problem @s41m0n.
Jenkins slaves use a old version of the kernel (4.15) that doesn't have support for bounded loops and has the limit of 4096 instructions per ebpf program. To overcome the first problem the firewall performs loop unrolling and apparently the number of rules of the test are causing the size of the program to exceed the limit (at least this is what happens when I downgrade the kernel on my machine). Could you try lowering that number? In my tests 2000 rules seem to be safe.

I still don't understand why the same tests passed formerly... But it is worth giving a try

@FedeParola
Copy link
Collaborator

Maybe I found out why the tests are passing on the master: original tests use a batch file to carry rules and then pass this file to the polycubectl command, however probably this method doesn't work and no instruction is added at all, leaving only the default (forward) policy.

@smagnani96
Copy link
Collaborator Author

I may have found the cause of the problem @s41m0n.
Jenkins slaves use a old version of the kernel (4.15) that doesn't have support for bounded loops and has the limit of 4096 instructions per ebpf program. To overcome the first problem the firewall performs loop unrolling and apparently the number of rules of the test are causing the size of the program to exceed the limit (at least this is what happens when I downgrade the kernel on my machine). Could you try lowering that number? In my tests 2000 rules seem to be safe.

I still don't understand why the same tests passed formerly... But it is worth giving a try

The number of rules injected is the same, it hasn't changed.
Even when there was the "transactional" mode instead of batch rule injection we used to insert more than 4000 rules

@smagnani96
Copy link
Collaborator Author

Maybe I found out why the tests are passing on the master: original tests use a batch file to carry rules and then pass this file to the polycubectl command, however probably this method doesn't work and no instruction is added at all, leaving only the default (forward) policy.

I remember to have tested file injection with "polycubectl" with Fulvio and it seemed to work. Anyway, now the rules are correctly inserted, I am able to see them all, and locally the test passes :(

@FedeParola
Copy link
Collaborator

Maybe I found out why the tests are passing on the master: original tests use a batch file to carry rules and then pass this file to the polycubectl command, however probably this method doesn't work and no instruction is added at all, leaving only the default (forward) policy.

I remember to have tested file injection with "polycubectl" with Fulvio and it seemed to work. Anyway, now the rules are correctly inserted, I am able to see them all, and locally the test passes :(

Yes you are right, I didn't check too accurately, sorry.

I may have found the cause of the problem @s41m0n.
Jenkins slaves use a old version of the kernel (4.15) that doesn't have support for bounded loops and has the limit of 4096 instructions per ebpf program. To overcome the first problem the firewall performs loop unrolling and apparently the number of rules of the test are causing the size of the program to exceed the limit (at least this is what happens when I downgrade the kernel on my machine). Could you try lowering that number? In my tests 2000 rules seem to be safe.
I still don't understand why the same tests passed formerly... But it is worth giving a try

The number of rules injected is the same, it hasn't changed.
Even when there was the "transactional" mode instead of batch rule injection we used to insert more than 4000 rules

Ok then the overflow is probably caused by the DEBUG instructions, try instantiating the firewall with no loglevel (default=INFO) and the problem should be gone.

Signed-off-by: Simone Magnani <simonemagnani.96@gmail.com>
@smagnani96
Copy link
Collaborator Author

@FedeParola thanks, the debug option seemed to alter some test, it worked.

@frisso frisso marked this pull request as ready for review November 12, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] accept-established in firewall should be ON by default
6 participants