Merge pull request #1581 from pdube/network-acl-rules-order

CLOUDSTACK-9404 Fixed ordering of network ACL rules being sent to the VR. 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

* pr/1581:
  Added ASF license to unit test file
  Added unit test to verify ordering
  Fixed ordering of network ACL rules being sent to the VR. The comparator was inverted

Signed-off-by: Will Stevens <williamstevens@gmail.com>

Author: swill

core/src/com/cloud/agent/api/routing/SetNetworkACLCommand.java

@@ -45,12 +45,8 @@ public NetworkACLTO[] getRules() {
 
     public String[][] generateFwRules() {
         final List<NetworkACLTO> aclList = Arrays.asList(rules);
-        Collections.sort(aclList, new Comparator<NetworkACLTO>() {
-            @Override
-            public int compare(final NetworkACLTO acl1, final NetworkACLTO acl2) {
-                return acl1.getNumber() < acl2.getNumber() ? 1 : -1;
-            }
-        });
+
+        orderNetworkAclRulesByRuleNumber(aclList);
 
         final String[][] result = new String[2][aclList.size()];
         int i = 0;
@@ -97,6 +93,15 @@ public int compare(final NetworkACLTO acl1, final NetworkACLTO acl2) {
         return result;
     }
 
+    protected void orderNetworkAclRulesByRuleNumber(List<NetworkACLTO> aclList) {
+        Collections.sort(aclList, new Comparator<NetworkACLTO>() {
+            @Override
+            public int compare(final NetworkACLTO acl1, final NetworkACLTO acl2) {
+                return acl1.getNumber() > acl2.getNumber() ? 1 : -1;
+            }
+        });
+    }
+
     public NicTO getNic() {
         return nic;
     }

core/test/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java

@@ -0,0 +1,53 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package com.cloud.agent.api.routing;
+
+import static org.junit.Assert.assertEquals;
+
+import java.util.List;
+
+import org.junit.Test;
+
+import com.cloud.agent.api.to.NetworkACLTO;
+import com.google.common.collect.Lists;
+
+public class SetNetworkACLCommandTest {
+
+    @Test
+    public void testNetworkAclRuleOrdering(){
+
+        //given
+        List<NetworkACLTO> aclList = Lists.newArrayList();
+
+        aclList.add(new NetworkACLTO(3, null, null, null, null, false, false, null, null, null, null, false, 3));
+        aclList.add(new NetworkACLTO(1, null, null, null, null, false, false, null, null, null, null, false, 1));
+        aclList.add(new NetworkACLTO(2, null, null, null, null, false, false, null, null, null, null, false, 2));
+
+        SetNetworkACLCommand cmd = new SetNetworkACLCommand(aclList, null);
+
+        //when
+        cmd.orderNetworkAclRulesByRuleNumber(aclList);
+
+        //then
+        for(int i=0; i< aclList.size();i++){
+            assertEquals(aclList.get(i).getNumber(), i+1);
+        }
+    }
+}