CLOUDSTACK-9404 Fixed ordering of network ACL rules being sent to the VR.

[pdube]

The comparator was inverted.

Issue: https://issues.apache.org/jira/browse/CLOUDSTACK-9404

In this example, I created rules with the port numbers the same as the rule numbers.

Chain ACL_INBOUND_eth2 (1 references)
target prot opt source destination
ACCEPT all -- anywhere 225.0.0.50
ACCEPT all -- anywhere vrrp.mcast.net
DROP tcp -- anywhere anywhere tcp dpt:netstat
DROP tcp -- anywhere anywhere tcp dpt:10
DROP tcp -- anywhere anywhere tcp dpt:5
DROP tcp -- anywhere anywhere tcp dpt:3
DROP tcp -- anywhere anywhere tcp dpt:2
DROP all -- anywhere anywhere

We can see above that the rules are inverted.

After the fix:

Chain ACL_INBOUND_eth2 (1 references)
target prot opt source destination
ACCEPT all -- anywhere 225.0.0.50
ACCEPT all -- anywhere vrrp.mcast.net
DROP tcp -- anywhere anywhere tcp dpt:2
DROP tcp -- anywhere anywhere tcp dpt:3
DROP tcp -- anywhere anywhere tcp dpt:5
DROP tcp -- anywhere anywhere tcp dpt:10
DROP tcp -- anywhere anywhere tcp dpt:netstat
DROP all -- anywhere anywhere

[remibergsma]

@pdube Jenkins fails because this file has no license.

[pdube]

@remibergsma Thanks, updated it

[spellgen]

the changes look good to me

[remibergsma]

@pdube Travis timed out, you may want to try the Travis Lottery again ;-)

Is this ready for testing?

[pdube]

@remibergsma Yes, the rules should appear in the right order now.

[remibergsma]

@pdube Thanks, will give it a try soon

[pdube]

The Travis build has timed out 3 times now. Is there any known issue with Travis right now @swill ?

[aaronhurt]

Looks clean and the added test case should prevent this from happening again. According to the "git blame" the comparator was switched back in 2013 and has been broken since.

[swill]

The only known issue is that it this periodically happens. I have not had a chance to dig deeper...

[dmabry]

I just kicked off a build that includes this PR. I'm going to push this to our lab for testing and report back.

[dmabry]

I tested this in our lab with advanced networking verified the patch is working as expected. I used the following test process.

1. Created an acl and applied it to 1 VPC Network Tier.

10 192.168.10.0/24 Allow ALL Ingress 20 192.168.20.0/24 Allow ALL Ingress 30 192.168.30.0/24 Allow ALL Ingress 

2 iptables looked like the following on the VPC VR

Chain ACL_INBOUND_eth2 (1 references)
target prot opt source destination ACCEPT all -- 0.0.0.0/0 225.0.0.50 ACCEPT all -- 0.0.0.0/0 224.0.0.18 ACCEPT all -- 192.168.10.0/24 0.0.0.0/0 ACCEPT all -- 192.168.20.0/24 0.0.0.0/0 ACCEPT all -- 192.168.30.0/24 0.0.0.0/0 DROP all -- 0.0.0.0/0 0.0.0.0/0 

3 I added an additional rule of:

40 192.168.40.0/24 Allow TCP 80 80 Ingress 

4 iptables looked like the following on the VPC VR

Chain ACL_INBOUND_eth2 (1 references)
target prot opt source destination ACCEPT all -- 0.0.0.0/0 225.0.0.50 ACCEPT all -- 0.0.0.0/0 224.0.0.18 ACCEPT all -- 192.168.10.0/24 0.0.0.0/0 ACCEPT all -- 192.168.20.0/24 0.0.0.0/0 ACCEPT all -- 192.168.30.0/24 0.0.0.0/0 ACCEPT tcp -- 192.168.40.0/24 0.0.0.0/0 tcp dpt:80
DROP all -- 0.0.0.0/0 0.0.0.0/0 

In summary, it looks like this patch works verified by manual testing in my lab.

In short, LGTM based on testing.

[pdube]

Thanks @dmabry

[dmabry]

@pdube No problem. Think you could kick this off again and see if Travis comes back green?

[swill]

@pdube can you close and reopen this one? thx...

[swill]

CI RESULTS

Tests Run: 85
Skipped: 0
Failed: 0
Errors: 0
Duration: 4h 01m 52s

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__Jun_13_2016_17_06_42_SFRV4J:

• dc_entries.obj
• failed_plus_exceptions.txt
• runinfo.txt

/tmp/MarvinLogs/test_network_8S1TLA:

• failed_plus_exceptions.txt
• results.txt
• runinfo.txt

/tmp/MarvinLogs/test_vpc_routers_SYNGX8:

• failed_plus_exceptions.txt
• results.txt
• runinfo.txt

Uploads will be available until 2016-08-14 02:00:00 +0200 CEST

Comment created by upr comment.

[kishankavala]

@pdube though the fix looks good, the root of the issue is different.
Earlier when bash scripts were used to configure rules on VR, iptable rules for ACLs were inserted (-I option).
This changed to add (-A option) after VR refactor, resulting in rules being applied in the reverse order.

[swill]

I'm not sure what to make of this comment @kishankavala. Is that a LGTM?

[kishankavala]

@swill This is a regression caused by VR refactor. There could be more such issues. I would prefer a fix in the VR script. Current fix is more like reversing the order twice to make it correct.

[swill]

@pdube can you review @kishankavala's comments. I want to cut an RC soon, but we have other pending issues in the VR right now which @kiwiflyer, @dmabry and team are working on...

[aaronhurt]

Looking through the git blame for this section of code it appears the operator and the ordering of the list was the same as this patch prior to 2013 when the operator was switched.

[pdube]

@kishankavala I think that the ultimate fix will be in the VR. However, the inversion of the list is fixed with this patch, and does not require a VR update. This is a good enough fix for now, as the ordering inversion is a critical security bug, since the rule numbers you are giving are not being applied as expected.

[kishankavala]

@leprechau ACL rule ordering was introduced in 2013 in ACS 4.2. Since 4.2 release, the order was never changed.
Older commit your are referring to, was during development phase of 4.2.

[kishankavala]

@pdube Current fix is good enough. I'm only concerned about more such issues in VR.
can you please create a tracking bug for VR fix and make a note that this is a regression?

[aaronhurt]

@kishankavala Understood, thank you for the explanation. The long history of the project makes digging into some of these things difficult.

[bvbharatk]

ACS CI BVT Run

Sumarry:
Build Number 207
Hypervisor xenserver
NetworkType Advanced
Passed=68
Failed=5
Skipped=3

Link to logs Folder (search by build_no):https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0

Failed tests:

• test_vpc_vpn.py
  • ContextSuite context=TestRVPCSite2SiteVpn>:setup Failing since 24 runs
  • ContextSuite context=TestVpcRemoteAccessVpn>:setup Failing since 23 runs
  • ContextSuite context=TestVpcSite2SiteVpn>:setup Failing since 22 runs
• test_volumes.py
  • test_06_download_detached_volume Failed
• test_vm_life_cycle.py
  • test_10_attachAndDetach_iso Failed

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm

Passed test suits:
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_disk_offerings.py