-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
CLOUDSTACK-8688 - default policies for INPUT and FORWARD should be se… #765
Conversation
…t to DROP instead of ACCEPT - In order to be able to access the routers via the link local interface, we have to add a rules with NEW and ESTABLISHED state
cloudstack-pull-rats #454 SUCCESS |
VM Life Cycle tests (Advanced Zone)
VM Life Cycle tests (Basic Zone)
|
cloudstack-pull-analysis #387 SUCCESS |
@karuturi @bhaisaab @DaanHoogland @koushik-das Anyone with some time to have a look at this PR? Thanks in advance. Cheers, |
@wilderrodrigues I'm now testing your PR, but I have a question: how is SSHing into the VMs testing the default policy is set to DROP? |
SSH doesn't test it... I just did to make sure all works as before. To check the policies to iptables -L --verbose (you will see DROP for INPUT and FORWARD chains on all routers) You can also try connecting to a port that doesn't have a PF setup. |
ok, that's what I thought. |
@wilderrodrigues wouldn't it be better to have a Marvin test that checks the policy? |
@@ -414,7 +426,7 @@ def fw_router(self): | |||
self.fw.append(['', '', '-A NETWORK_STATS -i eth2 -o eth0']) | |||
self.fw.append(['', '', '-A NETWORK_STATS -o eth2 ! -i eth0 -p tcp']) | |||
self.fw.append(['', '', '-A NETWORK_STATS -i eth2 ! -o eth0 -p tcp']) | |||
|
|||
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.
trailing white space?
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 days of work to find the cause of the bugs and the thing goes with trailing spaces... crap.
Will remove it once I add a marvin test.
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.
@wilderrodrigues Seems like you hadn't removed the trailing white spaces. Maybe a good time to remove them when PRing CLOUDSTACK-8878 or CLOUDSTACK-8795? :)
changes look reasonable. have not tested, so I am going to trust @wilderrodrigues on this but @miguelaferreira his point on an automation sounds very promising to me. |
Okay for the Marvin test, but then it will make the thing wait for another day, at least. Which is fine, but I hope people LGTM it afterwards. I'm worried about the lack of reviews/tests by reviewers on PRs. Concerning the unit test, I won't add it because I want to refactor the code as a project and have it done in a way that we can add tests and refactor, as it was done with other components. I know it could be done in a way where I could refactor just 1 method, add a tests and push it. However, I do not want to mix styles in the Python code. By styles I mean: the way it was developed and the way I would have developed it. So, mixing styles by refactoring 1 method to add 1 test will not really improve it. Once we release 4.6, and if that okay with the team, I, we, will work on the python refactor. Cheers, |
@wilderrodrigues ok for the python unit tests, but I would really like a marvin test, or al least some way to automate setting up the environment you described. I'm trying to test this, and clicking around in the UI is just too inefficient. |
I will push a test today to cover the iptables default policies. Do you need help finding the options on the UI whilst the test gets cooked? Cheers, |
tested this on Xen 6.5 advanced zone with isolated and VPC. verified that the default policies are set to drop. I am not sure if its related to this. But, I found the below issue
|
Thanks for testing it, @karuturi, much appreciated! I'm writing marvin tests for this PR and the other issue (CLOUDSTACK-8759). Once done, I will have a look at the problem you reported. In order to keep things separate and move quicker with the PRs, could you please a separate issue with the details above? Thanks in advance. Cheers, |
Ok. Here is the new issue https://issues.apache.org/jira/browse/CLOUDSTACK-8795 👍 for this PR |
Thanks for the LGTM and for the new issue, @karuturi. :) I will push the test today and merge the PR after @miguelaferreira tests it. Cheers, |
cloudstack-pull-rats #489 SUCCESS |
cloudstack-pull-analysis #422 SUCCESS |
cloudstack-pull-rats #491 SUCCESS |
cloudstack-pull-analysis #424 FAILURE |
cloudstack-pull-rats #494 SUCCESS |
cloudstack-pull-rats #495 SUCCESS |
cloudstack-pull-analysis #427 FAILURE |
cloudstack-pull-rats #496 SUCCESS |
cloudstack-pull-rats #497 SUCCESS |
cloudstack-pull-rats #498 SUCCESS |
cloudstack-pull-rats #499 SUCCESS |
cloudstack-pull-analysis #434 UNSTABLE |
cloudstack-pull-analysis #435 SUCCESS |
cloudstack-pull-analysis #437 UNSTABLE |
cloudstack-pull-analysis #438 UNSTABLE |
cloudstack-pull-rats #508 SUCCESS |
cloudstack-pull-analysis #441 SUCCESS |
@miguelaferreira @wilderrodrigues waiting for the PR merge :) |
@karuturi Wilder will add marvin test for this PR, I will run that and post the results |
cloudstack-pull-rats #525 SUCCESS |
cloudstack-pull-analysis #459 UNSTABLE |
cloudstack-pull-rats #528 SUCCESS |
cloudstack-pull-analysis #462 SUCCESS |
cloudstack-pull-rats #533 SUCCESS |
cloudstack-pull-rats #534 SUCCESS |
cloudstack-pull-rats #535 SUCCESS |
cloudstack-pull-rats #536 SUCCESS |
…lied - Changing refactored the utils.get_process_status() function - Adding 2 tests: test_01_single_VPC_iptables_policies and test_02_routervm_iptables_policies
@miguelaferreira @remibergsma @karuturi @DaanHoogland The test is done! Results: Test iptables default INPUT/FORWARD policy on RouterVM ... === TestName: test_02_routervm_iptables_policies | Status : SUCCESS === Ran 2 tests in 663.540s OK The tests were done only for single VPC and Isolated Network because the python code executed is also used by Redundant VPC and Shared Network. We can come back to this test later and add more cases, I already added some service for the above mentioned networks in the test. You can run this test by doing so:
Make sure you do the following before running the test agains a KVM hypervisor:
Cheers, |
cloudstack-pull-analysis #467 SUCCESS |
cloudstack-pull-analysis #468 ABORTED |
cloudstack-pull-rats #538 SUCCESS |
cloudstack-pull-analysis #469 ABORTED |
cloudstack-pull-analysis #470 FAILURE |
cloudstack-pull-analysis #472 UNSTABLE |
Could one of you have a look at this PR, please? :) Cheers, |
LGTM |
CLOUDSTACK-8688 - default policies for INPUT and FORWARD should be set to DROP instead of ACCEPT - In order to be able to access the routers via the link local interface, we have to add a rules with NEW and ESTABLISHED state * pr/765: CLOUDSTACK-8688 - Adding Marvin tests in order to cover the fixes applied CLOUDSTACK-8688 - default policies for INPUT and FORWARD should be set to DROP instead of ACCEPT Signed-off-by: wilderrodrigues <wrodrigues@schubergphilis.com>
Thanks, @bhaisaab ! |
Reverter MR 295 Closes apache#765 See merge request scclouds/scclouds!333
…t to DROP instead of ACCEPT
Tests:
Deployed 2 zones, basic and advanced, using KVM as hypervisor
On the basic zone, created 1 security group, added ingress rules to open port 22 and deployed 1 VM
On the advanced zone, created 1 single VPC (with 2 tiers, 2 puc IPs, 2 VMs and 1 ACL), 1 redundant VPC ((with 2 tiers, 2 puc IPs, 2 VMs and 1 ACL)), 1 isolated network (with 1 VM and 1 pub IP), 1 redundant network (with 1 VM and 1 pub IP)
I'm now running some automated tests, will post the results here once they are complete.
@remibergsma @DaanHoogland @bhaisaab @miguelaferreira @wido @karuturi , could you guys please have a look?
Do not forget to replace the systemvm.iso in your hypervisor. Do the following for KVM:
Cheers,
Wilder