VPC egress ACL: end ACL_OUTBOUND with RETURN so its rules stay in order

systemvm/debian/opt/cloud/bin/cs/CsAddress.py

@@ -561,6 +561,10 @@ def fw_vpcrouter(self):
                     ["mangle", "front", "-A ACL_OUTBOUND_%s -d 225.0.0.50/32 -j ACCEPT" % self.dev])
                 self.fw.append(
                     ["mangle", "front", "-A ACL_OUTBOUND_%s -d 224.0.0.18/32 -j ACCEPT" % self.dev])
+                # ACL rules are inserted ahead of the chain's last rule, so like ACL_INBOUND's DROP
+                # this has to stay last; RETURN is what falling off the end of the chain does anyway
+                self.fw.append(
+                    ["mangle", "", "-A ACL_OUTBOUND_%s -j RETURN" % self.dev])
             else:
                 self.fw.append(["filter", "", "-A FORWARD -d %s -o %s -j ACCEPT" % (guestNetworkCidr, self.dev)])
 
@@ -614,13 +618,19 @@ def fw_vpcrouter(self):
                     self.fw.append(["filter", "front",
                                     "-A FORWARD -d %s -o %s -m state --state RELATED,ESTABLISHED -j ACCEPT" %
                                     (static_route['network'], self.dev)])
+                    if self.get_type() in ["guest"] and not self.config.has_public_network():
+                        # Without a public network a tier's ACL chains get no last rule of their own, and are
+                        # only jumped to from here; close them too, so the ACL rules stay in order
+                        self.fw.append(["filter", "", "-A ACL_INBOUND_%s -j RETURN" % self.dev])
+                        self.fw.append(["mangle", "", "-A ACL_OUTBOUND_%s -j RETURN" % self.dev])
 
         if self.is_private_gateway():
             self.fw.append(["filter", "front", "-A FORWARD -d %s -o %s -j ACL_INBOUND_%s" %
                             (self.address['network'], self.dev, self.dev)])
             self.fw.append(["filter", "front", "-A FORWARD -d %s -o %s -m state --state RELATED,ESTABLISHED -j ACCEPT" %
                             (self.address['network'], self.dev)])
             self.fw.append(["filter", "", "-A ACL_INBOUND_%s -j DROP" % self.dev])
+            self.fw.append(["mangle", "", "-A ACL_OUTBOUND_%s -j RETURN" % self.dev])
             self.fw.append(["mangle", "",
                             "-A PREROUTING -m state --state NEW -i %s -s %s ! -d %s/32 -j ACL_OUTBOUND_%s" %
                             (self.dev, self.address['network'], self.address['gateway'], self.dev)])

systemvm/test/TestCsAddress.py

@@ -16,10 +16,42 @@
 # under the License.
 
 import unittest
-from cs.CsAddress import CsAddress
+from unittest import mock
+from cs.CsAddress import CsAddress, CsIP
 import merge
 
 
+class FakeConfig:
+
+    def __init__(self):
+        self.fw = []
+        self.nft_ipv4_fw = []
+        self.nft_ipv4_acl = []
+
+    def get_fw(self):
+        return self.fw
+
+    def get_nft_ipv4_fw(self):
+        return self.nft_ipv4_fw
+
+    def get_nft_ipv4_acl(self):
+        return self.nft_ipv4_acl
+
+    def cmdline(self):
+        cl = mock.Mock()
+        cl.get_vpccidr.return_value = "10.0.0.0/16"
+        return cl
+
+    def is_vpc(self):
+        return True
+
+    def is_routed(self):
+        return False
+
+    def has_public_network(self):
+        return True
+
+
 class TestCsAddress(unittest.TestCase):
 
     def setUp(self):
@@ -38,6 +70,49 @@ def test_get_guest_ip(self):
     def test_get_guest_netmask(self):
         self.assertTrue(self.csaddress.get_guest_netmask() == "255.255.255.0")
 
+    def acl_rules(self, address, chain="ACL_OUTBOUND_eth3", public_network=True, static_routes=None):
+        config = FakeConfig()
+        config.has_public_network = lambda: public_network
+        with mock.patch.object(CsIP, "list"):
+            ip = CsIP("eth3", config)
+        ip.setAddress(address)
+        routes = mock.Mock()
+        routes.get_bag.return_value = static_routes or {}
+        with mock.patch("cs.CsAddress.CsStaticRoutes", return_value=routes):
+            ip.fw_vpcrouter()
+        return [fw for fw in config.fw if "-A %s " % chain in fw[2]]
+
+    def acl_outbound_rules(self, address):
+        return self.acl_rules(address)
+
+    def test_acl_outbound_ends_with_return_on_guest_tier(self):
+        # ACL rules are inserted ahead of the last rule of the chain, so that rule must be a
+        # terminal one, or it would end up behind the ACL rules
+        rules = self.acl_outbound_rules({"nw_type": "guest", "network": "10.0.1.0/24", "gateway": "10.0.1.1"})
+        self.assertEqual(rules[-1], ["mangle", "", "-A ACL_OUTBOUND_eth3 -j RETURN"])
+        self.assertTrue(all(fw[1] == "front" for fw in rules[:-1]))
+
+    def test_acl_chains_end_with_return_on_static_route_tier_without_public_network(self):
+        # such a tier's ACL chains are only jumped to for the static route, and get no last rule otherwise
+        address = {"nw_type": "guest", "network": "10.0.1.0/24", "gateway": "10.0.1.1", "public_ip": "10.0.1.1"}
+        routes = {"id": "staticroutes", "192.168.50.0/24": {"network": "192.168.50.0/24", "ip_address": "10.0.1.1", "revoke": False}}
+        for chain, table in (("ACL_INBOUND_eth3", "filter"), ("ACL_OUTBOUND_eth3", "mangle")):
+            rules = self.acl_rules(address, chain, public_network=False, static_routes=routes)
+            self.assertEqual(rules, [[table, "", "-A %s -j RETURN" % chain]])
+
+    def test_acl_chains_get_one_last_rule_on_static_route_tier_with_public_network(self):
+        address = {"nw_type": "guest", "network": "10.0.1.0/24", "gateway": "10.0.1.1", "public_ip": "10.0.1.1"}
+        routes = {"id": "staticroutes", "192.168.50.0/24": {"network": "192.168.50.0/24", "ip_address": "10.0.1.1", "revoke": False}}
+        inbound = self.acl_rules(address, "ACL_INBOUND_eth3", static_routes=routes)
+        self.assertEqual([fw[2] for fw in inbound if fw[1] == ""], ["-A ACL_INBOUND_eth3 -j DROP"])
+        outbound = self.acl_rules(address, "ACL_OUTBOUND_eth3", static_routes=routes)
+        self.assertEqual([fw[2] for fw in outbound if fw[1] == ""], ["-A ACL_OUTBOUND_eth3 -j RETURN"])
+
+    def test_acl_outbound_ends_with_return_on_private_gateway(self):
+        rules = self.acl_outbound_rules({"nw_type": "public", "is_private_gateway": True, "network": "172.16.0.0/24",
+                                         "gateway": "172.16.0.1", "public_ip": "172.16.0.10", "source_nat": False})
+        self.assertEqual(rules, [["mangle", "", "-A ACL_OUTBOUND_eth3 -j RETURN"]])
+
 
 if __name__ == '__main__':
     unittest.main()