Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/
---

Review request for cloudstack, Girish Shilamkar and Prasanna Santhanam.


Repository: cloudstack-git


Description
---

Tests for egress firewall rules for advance zone.


Diffs
-

  test/integration/component/test_egress_fw_rules.py PRE-CREATION 

Diff: https://reviews.apache.org/r/12934/diff/


Testing
---


Thanks,

Ashutosh Kelkar

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Prasanna Santhanam]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/
---

(Updated July 25, 2013, 4:39 a.m.)


Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
Santhanam.


Changes
---

Jayapal, need your help reviewing this buddy.


Repository: cloudstack-git


Description
---

Tests for egress firewall rules for advance zone.


Diffs
-

  test/integration/component/test_egress_fw_rules.py PRE-CREATION 

Diff: https://reviews.apache.org/r/12934/diff/


Testing
---


Thanks,

Ashutosh Kelkar

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Jayapal Reddy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review23820
---



test/integration/component/test_egress_fw_rules.py


Please check the below configurable option for egress firewall rules.

https://issues.apache.org/jira/browse/CLOUDSTACK-1578

For the default offering egress is block.
For the newly creating offering the egress default is allow and block the 
traffic for the created rules.

So update the test cases for the above



test/integration/component/test_egress_fw_rules.py


create two network offering one with 'egress_policy'= true and other is 
'egress_policy'= false
Add the test cases for both



test/integration/component/test_egress_fw_rules.py


Add 'egress_policy' entry in the network offering.



test/integration/component/test_egress_fw_rules.py


1. create a network offering without redundant router
2. You can have another offering/test case with redundant router



test/integration/component/test_egress_fw_rules.py


1. deploy VM should ' deploy vm with network offering 



test/integration/component/test_egress_fw_rules.py


Please refer 
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Egress+firewall+rules+-+Ability+to+change+the+default
 

update the test case accordingly 


- Jayapal Reddy


On July 25, 2013, 4:39 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 25, 2013, 4:39 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Jenkins Cloudstack.org]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review23828
---


Review 12934 PASSED the build test
The url of build cloudstack-master-with-patch #33 is : 
http://jenkins.cloudstack.org/job/cloudstack-master-with-patch/33/

- Jenkins Cloudstack.org


On July 25, 2013, 4:39 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 25, 2013, 4:39 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/
---

(Updated July 26, 2013, 6:27 a.m.)


Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
Santhanam.


Changes
---

Code review changes: 
1) Updated egress_policy in network_offering definition.
2) Added test for egress_policy=true
Added test for DB table value check.
Added test for RVR.


Repository: cloudstack-git


Description
---

Tests for egress firewall rules for advance zone.


Diffs (updated)
-

  test/integration/component/test_egress_fw_rules.py PRE-CREATION 

Diff: https://reviews.apache.org/r/12934/diff/


Testing
---


Thanks,

Ashutosh Kelkar

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Sheng Yang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review23920
---



test/integration/component/test_egress_fw_rules.py


Missing clean up for account and domain.



test/integration/component/test_egress_fw_rules.py


Missing tearDown()



test/integration/component/test_egress_fw_rules.py


I think we already have alike mechanism for executing inside user VM? Check 
get_ssh_client. If it doesn't fit, we need some generic mechanism rather than 
one method for one test case.



test/integration/component/test_egress_fw_rules.py


You need to wait a while (e.g. 60 seconds) for mgmt server complete the 
updating of RvR status.


- Sheng Yang


On July 26, 2013, 6:27 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 26, 2013, 6:27 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

> On July 25, 2013, 6:25 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 99
> > 
> >
> > Please check the below configurable option for egress firewall rules.
> > 
> > https://issues.apache.org/jira/browse/CLOUDSTACK-1578
> > 
> > For the default offering egress is block.
> > For the newly creating offering the egress default is allow and block 
> > the traffic for the created rules.
> > 
> > So update the test cases for the above

Added Test for this use case.


> On July 25, 2013, 6:25 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 100
> > 
> >
> > create two network offering one with 'egress_policy'= true and other is 
> > 'egress_policy'= false
> > Add the test cases for both

Updated the service data to cover egress policy.


> On July 25, 2013, 6:25 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 106
> > 
> >
> > Add 'egress_policy' entry in the network offering.

Done


> On July 25, 2013, 6:25 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 121
> > 
> >
> > 1. create a network offering without redundant router
> > 2. You can have another offering/test case with redundant router

Done


> On July 25, 2013, 6:25 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 363
> > 
> >
> > Please refer 
> > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Egress+firewall+rules+-+Ability+to+change+the+default
> >  
> > 
> > update the test case accordingly

Done


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review23820
---


On July 26, 2013, 6:27 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 26, 2013, 6:27 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

> On July 26, 2013, 8:46 a.m., Sheng Yang wrote:
> > test/integration/component/test_egress_fw_rules.py, line 253
> > 
> >
> > I think we already have alike mechanism for executing inside user VM? 
> > Check get_ssh_client. If it doesn't fit, we need some generic mechanism 
> > rather than one method for one test case.

Yes, that method works when VM is created with mode=advance. Here we don't want 
to create PF rule and setup public ip for a VM. we just want to test the egress 
rule and policy. In this case we have to take hopes to Hypervisor Host-> 
Router-> VM instance to reach out VM for checking the public network access.


> On July 26, 2013, 8:46 a.m., Sheng Yang wrote:
> > test/integration/component/test_egress_fw_rules.py, line 619
> > 
> >
> > You need to wait a while (e.g. 60 seconds) for mgmt server complete the 
> > updating of RvR status.

VirtualMachine.Create() waits till VM is up and VM comes up only after Network 
is up leading RVR up. no need to put a wait here.


> On July 26, 2013, 8:46 a.m., Sheng Yang wrote:
> > test/integration/component/test_egress_fw_rules.py, line 183
> > 
> >
> > Missing tearDown()

No, teamDown is not missing. it's there on line 352.


> On July 26, 2013, 8:46 a.m., Sheng Yang wrote:
> > test/integration/component/test_egress_fw_rules.py, line 166
> > 
> >
> > Missing clean up for account and domain.

No, cleanup for account and domain is in teamDownClass.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review23920
---


On July 26, 2013, 6:27 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 26, 2013, 6:27 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Sheng Yang]

> On July 26, 2013, 8:46 a.m., Sheng Yang wrote:
> > test/integration/component/test_egress_fw_rules.py, line 166
> > 
> >
> > Missing clean up for account and domain.
> 
> Ashutosh Kelkar wrote:
> No, cleanup for account and domain is in teamDownClass.

I didn't saw adding account or domain last time, but I found it now. I must be 
tired…


> On July 26, 2013, 8:46 a.m., Sheng Yang wrote:
> > test/integration/component/test_egress_fw_rules.py, line 183
> > 
> >
> > Missing tearDown()
> 
> Ashutosh Kelkar wrote:
> No, teamDown is not missing. it's there on line 352.

Yes.


> On July 26, 2013, 8:46 a.m., Sheng Yang wrote:
> > test/integration/component/test_egress_fw_rules.py, line 253
> > 
> >
> > I think we already have alike mechanism for executing inside user VM? 
> > Check get_ssh_client. If it doesn't fit, we need some generic mechanism 
> > rather than one method for one test case.
> 
> Ashutosh Kelkar wrote:
> Yes, that method works when VM is created with mode=advance. Here we 
> don't want to create PF rule and setup public ip for a VM. we just want to 
> test the egress rule and policy. In this case we have to take hopes to 
> Hypervisor Host-> Router-> VM instance to reach out VM for checking the 
> public network access.
>

Got it. But still it should be a good idea to add it to generic mechanism 
rather than implement it for each test case.


> On July 26, 2013, 8:46 a.m., Sheng Yang wrote:
> > test/integration/component/test_egress_fw_rules.py, line 619
> > 
> >
> > You need to wait a while (e.g. 60 seconds) for mgmt server complete the 
> > updating of RvR status.
> 
> Ashutosh Kelkar wrote:
> VirtualMachine.Create() waits till VM is up and VM comes up only after 
> Network is up leading RVR up. no need to put a wait here.

No, the detection of RvR state is an separate thread, it won't change as soon 
as the VR is up. So you need to wait for a while.

But in fact the VM's start up would likely take more than more minute, so it 
should be fine.


- Sheng


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review23920
---


On July 26, 2013, 6:27 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 26, 2013, 6:27 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

> On July 26, 2013, 8:46 a.m., Sheng Yang wrote:
> > test/integration/component/test_egress_fw_rules.py, line 253
> > 
> >
> > I think we already have alike mechanism for executing inside user VM? 
> > Check get_ssh_client. If it doesn't fit, we need some generic mechanism 
> > rather than one method for one test case.
> 
> Ashutosh Kelkar wrote:
> Yes, that method works when VM is created with mode=advance. Here we 
> don't want to create PF rule and setup public ip for a VM. we just want to 
> test the egress rule and policy. In this case we have to take hopes to 
> Hypervisor Host-> Router-> VM instance to reach out VM for checking the 
> public network access.
>
> 
> Sheng Yang wrote:
> Got it. But still it should be a good idea to add it to generic mechanism 
> rather than implement it for each test case.

Sure, I will add an enhancement ticket and will update the base.py with 
VirtualMachine.exec_script_on_user_vm(). It will be help full in future test 
case dev.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review23920
---


On July 26, 2013, 6:27 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 26, 2013, 6:27 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Jayapal Reddy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review23939
---



test/integration/component/test_egress_fw_rules.py


Please add network offering details also here.

#1. deploy VM using network offering with egress policy true  



test/integration/component/test_egress_fw_rules.py


Created network offering with egress policy True. That means by default all 
the guest traffic is allowed. If you create egress rules (ex: icmp) then the 
icmp traffic is blocked.

So #4. Public Network should be reachable from the VM



test/integration/component/test_egress_fw_rules.py


I gone through the your test cases. I think you bit confused on the egress 
default policy and rules
. 
Please update you test cases and test case comments as per below.

1. Network offering with egress_policy = true.
  - By default guest network traffic is allowed.
  - Egress rules traffic will be blocked and other traffic is allowed Ex: 
if you create egress rule for icmp traffic then except icmp other traffic is 
allowed.

   - Rules with DROP target added. 
 -A FW_EGRESS_RULES -p icmp -j DROP

2. Network offering with egress_policy = false
   - By default the guest network traffic is blocked.
   - Egress rule traffic is allowed. If you create egress rule with icmp 
protocol then except icmp other traffic is blocked.
   -Rules added with target ACCEPT.
-A FW_EGRESS_RULES -p icmp -j ACCPT



The CIDR in the egress rules is guest network cidr. The traffic 
allowed/blocked for guest network CIDR. CIDR is not Public/destination network 
cidr.






test/integration/component/test_egress_fw_rules.py


In egress the CIDR is source CIDR (guest network CIDR). If you don't 
mention the CIDR it will take the default guest network CIDR.

Egress compares the source CIDR.



- Jayapal Reddy


On July 26, 2013, 6:27 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 26, 2013, 6:27 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/
---

(Updated July 29, 2013, 4:57 a.m.)


Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
Santhanam.


Changes
---

code review changes.


Repository: cloudstack-git


Description
---

Tests for egress firewall rules for advance zone.


Diffs (updated)
-

  test/integration/component/test_egress_fw_rules.py PRE-CREATION 

Diff: https://reviews.apache.org/r/12934/diff/


Testing
---


Thanks,

Ashutosh Kelkar

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

> On July 26, 2013, 12:28 p.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 370
> > 
> >
> > Please add network offering details also here.
> > 
> > #1. deploy VM using network offering with egress policy true

Done.


> On July 26, 2013, 12:28 p.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 373
> > 
> >
> > Created network offering with egress policy True. That means by default 
> > all the guest traffic is allowed. If you create egress rules (ex: icmp) 
> > then the icmp traffic is blocked.
> > 
> > So #4. Public Network should be reachable from the VM

Done.


> On July 26, 2013, 12:28 p.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 390
> > 
> >
> > I gone through the your test cases. I think you bit confused on the 
> > egress default policy and rules
> > . 
> > Please update you test cases and test case comments as per below.
> > 
> > 1. Network offering with egress_policy = true.
> >   - By default guest network traffic is allowed.
> >   - Egress rules traffic will be blocked and other traffic is allowed 
> > Ex: if you create egress rule for icmp traffic then except icmp other 
> > traffic is allowed.
> > 
> >- Rules with DROP target added. 
> >  -A FW_EGRESS_RULES -p icmp -j DROP
> > 
> > 2. Network offering with egress_policy = false
> >- By default the guest network traffic is blocked.
> >- Egress rule traffic is allowed. If you create egress rule with 
> > icmp protocol then except icmp other traffic is blocked.
> >-Rules added with target ACCEPT.
> > -A FW_EGRESS_RULES -p icmp -j ACCPT
> > 
> > 
> > 
> > The CIDR in the egress rules is guest network cidr. The traffic 
> > allowed/blocked for guest network CIDR. CIDR is not Public/destination 
> > network cidr.
> > 
> > 
> >

Added test scenario for guest network access check.


> On July 26, 2013, 12:28 p.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 426
> > 
> >
> > In egress the CIDR is source CIDR (guest network CIDR). If you don't 
> > mention the CIDR it will take the default guest network CIDR.
> > 
> > Egress compares the source CIDR.
> >

Done, added test scenario for guest network access check.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review23939
---


On July 29, 2013, 4:57 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 29, 2013, 4:57 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Jayapal Reddy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review24101
---



test/integration/component/test_egress_fw_rules.py


I think you are not clear with the default egress policy. 

 - egress policy true
   All the guest network traffic to public n/w is allowed by default. 
Adding egress rule blocks only the rule specific traffic.
   
-egress policy false
By default guest network traffic to public n/w is block. if you add 
egress rule  then only egress rule specific traffic is allowed.

When egress policy is true, by default the guest network traffic is allow.

public network should be reachable



test/integration/component/test_egress_fw_rules.py


here ping google.com should success.



test/integration/component/test_egress_fw_rules.py


Please update all the comments in this file, network offering with egress 
policy .
# deploy vm with network offering egress policy true 



test/integration/component/test_egress_fw_rules.py


Please understand the basics and update the test cases.

ping should be blocked. 100% packet loss



test/integration/component/test_egress_fw_rules.py


mention the network offering with egress policy=false



test/integration/component/test_egress_fw_rules.py


Here ping is success. 
I did not get why 100% packet loss ?



test/integration/component/test_egress_fw_rules.py


In egress CIDR is not destination/public CIDR.
CIDR is guest network/source CIDR.

Please change the comments



test/integration/component/test_egress_fw_rules.py


if 10.2.2.2 exists then ping will be success. 

You test case should be like.
give CIDR 10.1.1.120/32 and try to access wget from the vm 10.1.1.100. Then 
ping should fail from the 10.1.1.100 and pass from the vm with ip 10.1.1.120



test/integration/component/test_egress_fw_rules.py


The purpose of the test is not correct.

The CIDR is source cidr, so testing public network is not needed.



test/integration/component/test_egress_fw_rules.py



wget will be success.Why we get failed here ?



test/integration/component/test_egress_fw_rules.py


here egress default policy is true. 
created egress rule for icmp , the icmp traffic is BLOCKED and other 
traffic is allowed.





test/integration/component/test_egress_fw_rules.py


connection to public should be allowed



test/integration/component/test_egress_fw_rules.py


do we need second vm here ?



test/integration/component/test_egress_fw_rules.py


here ping should fail here



test/integration/component/test_egress_fw_rules.py


ping should success here



test/integration/component/test_egress_fw_rules.py


Here you are passing valid CIDR.
The comment should be updated to 
'create egress rule with other than guest cidr'



test/integration/component/test_egress_fw_rules.py


remove this comment



test/integration/component/test_egress_fw_rules.py


Remove this comment


- Jayapal Reddy


On July 29, 2013, 4:57 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated July 29, 2013, 4:57 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/
---

(Updated Aug. 1, 2013, 6:19 a.m.)


Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
Santhanam.


Changes
---

As per discussion with Jaypal on GTM added following Tests.
1) Tests to cover egress_policy = true/false
2) Skipping tests for egress_policy = true. We can relabel these test once we 
get confirmed response from QA and Jayapal.


Repository: cloudstack-git


Description
---

Tests for egress firewall rules for advance zone.


Diffs (updated)
-

  test/integration/component/test_egress_fw_rules.py PRE-CREATION 

Diff: https://reviews.apache.org/r/12934/diff/


Testing
---


Thanks,

Ashutosh Kelkar

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

> On July 29, 2013, 12:32 p.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 374
> > 
> >
> > I think you are not clear with the default egress policy. 
> > 
> >  - egress policy true
> >All the guest network traffic to public n/w is allowed by default. 
> > Adding egress rule blocks only the rule specific traffic.
> >
> > -egress policy false
> > By default guest network traffic to public n/w is block. if you add 
> > egress rule  then only egress rule specific traffic is allowed.
> > 
> > When egress policy is true, by default the guest network traffic is 
> > allow.
> > 
> > public network should be reachable

As per your last email, and discussion over GTM. I have put in the tests for 
egress_policy = true. Currently all those tests are skipped from test run.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review24101
---


On Aug. 1, 2013, 6:19 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated Aug. 1, 2013, 6:19 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Jayapal Reddy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review24517
---



test/integration/component/test_egress_fw_rules.py


I appreciate your effort in correcting the test cases.
There are few comments from my side. Please fix those.
Also the script is failed to run.Also look into this. The below exception 
is thrown


integration.component.test_egress_fw_rules.log_test_exceptions ... ERROR

==
ERROR: integration.component.test_egress_fw_rules.log_test_exceptions
--
Traceback (most recent call last):
  File "/Library/Python/2.7/site-packages/nose/case.py", line 197, in 
runTest
self.test(*self.arg)
TypeError: log_test_exceptions() takes exactly 1 argument (0 given)




test/integration/component/test_egress_fw_rules.py


change the comment.
Egress policy is true



test/integration/component/test_egress_fw_rules.py


fix the comment.
access to public network for tcp port 80 is blocked.



test/integration/component/test_egress_fw_rules.py


Egress policy is not set to false, but comment says false. The above test 
case is already set to true



test/integration/component/test_egress_fw_rules.py


Fix the comment, invalid cidr is passed to create the rule



test/integration/component/test_egress_fw_rules.py


Invalid cidr is not passed to rule. fix the comment



test/integration/component/test_egress_fw_rules.py


Fix the comment



test/integration/component/test_egress_fw_rules.py


Fix the comment


- Jayapal Reddy


On Aug. 1, 2013, 6:19 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated Aug. 1, 2013, 6:19 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

> On Aug. 2, 2013, 10:29 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 676
> > 
> >
> > change the comment.
> > Egress policy is true

Done.


> On Aug. 2, 2013, 10:29 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 679
> > 
> >
> > fix the comment.
> > access to public network for tcp port 80 is blocked.

Done.


> On Aug. 2, 2013, 10:29 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 785
> > 
> >
> > Egress policy is not set to false, but comment says false. The above 
> > test case is already set to true

Done.


> On Aug. 2, 2013, 10:29 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 819
> > 
> >
> > Fix the comment, invalid cidr is passed to create the rule

Done.


> On Aug. 2, 2013, 10:29 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 837
> > 
> >
> > Invalid cidr is not passed to rule. fix the comment

Done.


> On Aug. 2, 2013, 10:29 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 856
> > 
> >
> > Fix the comment

Done


> On Aug. 2, 2013, 10:29 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 912
> > 
> >
> > Fix the comment

Done


> On Aug. 2, 2013, 10:29 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 55
> > 
> >
> > I appreciate your effort in correcting the test cases.
> > There are few comments from my side. Please fix those.
> > Also the script is failed to run.Also look into this. The below 
> > exception is thrown
> > 
> > 
> > integration.component.test_egress_fw_rules.log_test_exceptions ... ERROR
> > 
> > ==
> > ERROR: integration.component.test_egress_fw_rules.log_test_exceptions
> > --
> > Traceback (most recent call last):
> >   File "/Library/Python/2.7/site-packages/nose/case.py", line 197, in 
> > runTest
> > self.test(*self.arg)
> > TypeError: log_test_exceptions() takes exactly 1 argument (0 given)
> >

I am able to run the script without any issue.
python2.7 -m marvin.deployAndRun -c  $TESTPATH/run.cfg -d $TESTPATH -r 
/var/log/cs4.2.x-result.log -t /var/log/cs4.2.x-client.log -l &
testpid=$!
tail --pid $testpid -f /var/log/cs4.2.x-result.log /var/log/cs4.2.x-client.log
Let me know if you are still facing issues running the tests.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review24517
---


On Aug. 1, 2013, 6:19 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated Aug. 1, 2013, 6:19 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/
---

(Updated Aug. 5, 2013, 4:33 a.m.)


Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
Santhanam.


Changes
---

code review changes.


Repository: cloudstack-git


Description
---

Tests for egress firewall rules for advance zone.


Diffs (updated)
-

  test/integration/component/test_egress_fw_rules.py PRE-CREATION 

Diff: https://reviews.apache.org/r/12934/diff/


Testing
---


Thanks,

Ashutosh Kelkar

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Prasanna Santhanam]

> On Aug. 2, 2013, 10:29 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 55
> > 
> >
> > I appreciate your effort in correcting the test cases.
> > There are few comments from my side. Please fix those.
> > Also the script is failed to run.Also look into this. The below 
> > exception is thrown
> > 
> > 
> > integration.component.test_egress_fw_rules.log_test_exceptions ... ERROR
> > 
> > ==
> > ERROR: integration.component.test_egress_fw_rules.log_test_exceptions
> > --
> > Traceback (most recent call last):
> >   File "/Library/Python/2.7/site-packages/nose/case.py", line 197, in 
> > runTest
> > self.test(*self.arg)
> > TypeError: log_test_exceptions() takes exactly 1 argument (0 given)
> >
> 
> Ashutosh Kelkar wrote:
> I am able to run the script without any issue.
> python2.7 -m marvin.deployAndRun -c  $TESTPATH/run.cfg -d $TESTPATH -r 
> /var/log/cs4.2.x-result.log -t /var/log/cs4.2.x-client.log -l &
> testpid=$!
> tail --pid $testpid -f /var/log/cs4.2.x-result.log 
> /var/log/cs4.2.x-client.log
> Let me know if you are still facing issues running the tests.

The reason why this won't run using nosetests is that the testrunner in nose 
needs the testnames to start with the test_ prefix. When you wrap the decorator 
over each test the name of the method changes. Check the test_assign_vm.py 
where I made the fix. Since you are using the logger decorator often why not 
also move it to the utils/common module?


- Prasanna


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review24517
---


On Aug. 1, 2013, 6:19 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated Aug. 1, 2013, 6:19 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Jayapal Reddy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review24640
---



test/integration/component/test_egress_fw_rules.py


remove the white space (red colour).
Please apply the patch in your local and make sure there is no warning.
# git apply patchName.patch



test/integration/component/test_egress_fw_rules.py


do we need specify vlan here ?



test/integration/component/test_egress_fw_rules.py


Hard coding vlan may not work for others setups.
If specify vlan is set then query the free vlan id


- Jayapal Reddy


On Aug. 1, 2013, 6:19 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated Aug. 1, 2013, 6:19 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

> On Aug. 5, 2013, 9:55 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 27
> > 
> >
> > remove the white space (red colour).
> > Please apply the patch in your local and make sure there is no warning.
> > # git apply patchName.patch

Removed the trailing white space.


> On Aug. 5, 2013, 9:55 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 108
> > 
> >
> > do we need specify vlan here ?

No need to specify the VLAN ID here.


> On Aug. 5, 2013, 9:55 a.m., Jayapal Reddy wrote:
> > test/integration/component/test_egress_fw_rules.py, line 130
> > 
> >
> > Hard coding vlan may not work for others setups.
> > If specify vlan is set then query the free vlan id

Removed VLAN ID value from service data.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review24640
---


On Aug. 1, 2013, 6:19 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated Aug. 1, 2013, 6:19 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Ashutosh Kelkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/
---

(Updated Aug. 6, 2013, 4:38 a.m.)


Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
Santhanam.


Changes
---

Removed hard coded value of VLAN ID from network service data.
White space cleanup.


Repository: cloudstack-git


Description
---

Tests for egress firewall rules for advance zone.


Diffs (updated)
-

  test/integration/component/test_egress_fw_rules.py PRE-CREATION 

Diff: https://reviews.apache.org/r/12934/diff/


Testing
---


Thanks,

Ashutosh Kelkar

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Jayapal Reddy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review24701
---

Ship it!


Ship It!

- Jayapal Reddy


On Aug. 6, 2013, 4:38 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated Aug. 6, 2013, 4:38 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>

Re: Review Request 12934: Tests for egress firewall rules for advance zone

[Prasanna Santhanam]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12934/#review24709
---

Ship it!


Applied on master and 4.2 and some minor changes.
Please watch the runs for failures/errors from the next run onwards:
http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regression-matrix/




- Prasanna Santhanam


On Aug. 6, 2013, 4:38 a.m., Ashutosh Kelkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12934/
> ---
> 
> (Updated Aug. 6, 2013, 4:38 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar, Jayapal Reddy, and Prasanna 
> Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Tests for egress firewall rules for advance zone.
> 
> 
> Diffs
> -
> 
>   test/integration/component/test_egress_fw_rules.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/12934/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashutosh Kelkar
> 
>