Rework traffic-rules

Previous implementation did not take how VLANs are configured into
account, this lead to a pretty hefty change.

VLANs are using fake bridge, this bridge is then included in a parent
brige. As long as packets are inside that bridge and are not going out
of it, the packets are not tagged. Therefore, the vlanid cannot be used
to match the packets in OVS datapath. The only workaround found is to
create the rules for each ports, but this also means rules for
non-tagged traffic as they were done previously would apply to the VLAN
ports as well. We therefore have to apply rules to each matching ports
in all cases.

So what that version is doing is:
- TODO

Author: bleader

SOURCES/etc/xapi.d/plugins/sdncontroller.py

@@ -9,19 +9,26 @@
 
 OPENFLOW_PROTOCOL = "OpenFlow11"
 OVS_OFCTL_CMD = "ovs-ofctl"
+OVS_VSCTL_CMD = "ovs-vsctl"
 VALID_PROTOCOLS = {"tcp", "udp", "icmp", "ip", "arp"}
 PROTOCOLS_WITH_PORTS = {"tcp", "udp"}
 
 # error codes
 E_PARSER = 1
 E_PARAMS = 2
-E_BUILDER = 3
-E_OVSCALL = 4
+E_OVSCALL = 3
+E_PORTS = 4
+
+# rules names per direction
+TO = 0
+FROM = 1
+
 
 def log_and_raise_error(code, desc):
     _LOGGER.error(desc)
     raise_plugin_error(code, desc)
 
+
 # All functions starting with `parse_` are helper functions compatible with the `Parser` class.
 # Each should accept `args` as input and return either (result, None) on success,
 # or (None, error_message) on failure.
@@ -39,7 +46,9 @@ def parse_bridge(self):
             log_and_raise_error(E_PARSER, "bridge parameter is missing")
 
         if not BRIDGE_REGEX.match(bridge):
-            log_and_raise_error(E_PARSER, "'{}' is not a valid bridge name".format(bridge))
+            log_and_raise_error(
+                E_PARSER, "'{}' is not a valid bridge name".format(bridge)
+            )
 
         return bridge
 
@@ -50,7 +59,10 @@ def parse_mac(self):
         if mac_addr is None:
             return None
 
-        if not MAC_REGEX.match(mac_addr) or MAC_REGEX.match(mac_addr).group(0) != mac_addr:
+        if (
+            not MAC_REGEX.match(mac_addr)
+            or MAC_REGEX.match(mac_addr).group(0) != mac_addr
+        ):
             log_and_raise_error(E_PARSER, "'{}' is not a valid MAC".format(mac_addr))
 
         return mac_addr
@@ -60,13 +72,15 @@ def parse_iprange(self):
             r"^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}"
             r"(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(/\d{1,2})?$"
         )
-        ip_range = self.args.get("iprange")
+        ip_range = self.args.get("ipRange")
 
         if ip_range is None:
-            log_and_raise_error(E_PARSER, "ip range parameter is missing")
+            log_and_raise_error(E_PARSER, "ipRange parameter is missing")
 
         if not IPRANGE_REGEX.match(ip_range):
-            log_and_raise_error(E_PARSER, "'{}' is not a valid IP range".format(ip_range))
+            log_and_raise_error(
+                E_PARSER, "'{}' is not a valid IP range".format(ip_range)
+            )
 
         return ip_range
 
@@ -81,7 +95,9 @@ def parse_direction(self):
         has_from = "from" in dir
 
         if not (has_to or has_from):
-            log_and_raise_error(E_PARSER, "'{}' is not a valid direction".format(direction))
+            log_and_raise_error(
+                E_PARSER, "'{}' is not a valid direction".format(direction)
+            )
 
         return (has_to, has_from)
 
@@ -94,7 +110,9 @@ def parse_protocol(self):
         protocol = protocol.lower()
 
         if protocol not in VALID_PROTOCOLS:
-            log_and_raise_error(E_PARSER, "'{}' is not a supported protocol".format(protocol))
+            log_and_raise_error(
+                E_PARSER, "'{}' is not a supported protocol".format(protocol)
+            )
 
         return protocol
 
@@ -117,7 +135,10 @@ def parse_allow(self):
             log_and_raise_error(E_PARSER, "allow parameter is missing")
 
         if allow.lower() not in ["true", "false"]:
-            log_and_raise_error(E_PARSER, "allow parameter should be true or false, not '{}'".format(allow))
+            log_and_raise_error(
+                E_PARSER,
+                "allow parameter should be true or false, not '{}'".format(allow),
+            )
 
         if allow.lower() == "true":
             return True
@@ -135,7 +156,9 @@ def parse_priority(self):
                 raise ValueError
             return priority
         except ValueError:
-            log_and_raise_error(E_PARSER, "'{}' is not a valid priority".format(priority))
+            log_and_raise_error(
+                E_PARSER, "'{}' is not a valid priority".format(priority)
+            )
 
     def read(self, key, parse_fn, dests=None):
         # parse_fn can return a single value or a tuple of values.
@@ -147,13 +170,17 @@ def read(self, key, parse_fn, dests=None):
             if not isinstance(val, tuple):
                 log_and_raise_error(
                     E_PARSER,
-                    "Internal error: parse {}: doesn't return a tuple while dests is set".format(key)
+                    "Internal error: parse {}: doesn't return a tuple while dests is set".format(
+                        key
+                    ),
                 )
 
             if len(dests) != len(val):
                 log_and_raise_error(
                     E_PARSER,
-                    "Internal error: parse {}: dests is {} while {} was expected".format(key, len(dests), len(val))
+                    "Internal error: parse {}: dests is {} while {} was expected".format(
+                        key, len(dests), len(val)
+                    ),
                 )
 
             for d, v in zip(dests, val):
@@ -163,6 +190,7 @@ def read(self, key, parse_fn, dests=None):
 
         self.values[key] = val
 
+
 def json_error(name, desc):
     return json.dumps(
         {
@@ -173,41 +201,102 @@ def json_error(name, desc):
         }
     )
 
-def build_rules_string(args):
-    rules = []
 
-    #  tcp,dl_dst=26:bf:26:f0:4f:50,nw_src=192.168.38.0/24,tp_src=22 actions=NORMA
-    if args["has_from"]:
-        rule = ""
-        if "priority" in args and args["priority"]:
-            rule += "priority=" + args["priority"] + ","
-        rule += args["protocol"].lower()
-        if "mac" in args and args["mac"]:
-            rule += ",dl_dst=" + args["mac"]
-        rule += ",nw_src=" + args["iprange"]
-        if "protocol" in args and args["protocol"] in PROTOCOLS_WITH_PORTS:
-            rule += ",tp_src=" + args["port"]
-        if "allow" in args:
-            rule += ",actions=" + ("normal" if args["allow"] else "drop")
-        rules += [rule]
-
-    # tcp,in_port=3,dl_src=26:bf:26:f0:4f:50,nw_dst=192.168.38.0/24,tp_dst=2
-    if args["has_to"]:
-        rule = ""
-        if "priority" in args and args["priority"]:
-            rule += "priority=" + args["priority"] + ","
-        rule += args["protocol"].lower()
-        if "mac" in args and args["mac"]:
-            rule += ",dl_src=" + args["mac"]
-        rule += ",nw_dst=" + args["iprange"]
-        if "protocol" in args and args["protocol"] in PROTOCOLS_WITH_PORTS:
-            rule += ",tp_dst=" + args["port"]
-        if "allow" in args:
-            rule += ",actions=" + ("normal" if args["allow"] else "drop")
-        rules += [rule]
+def run_vsctl_cmd(args):
+    vsctl_cmd = [OVS_VSCTL_CMD] + args
+    cmd = run_command(vsctl_cmd, check=False)
+    if cmd["returncode"] != 0:
+        log_and_raise_error(
+            E_OVSCALL,
+            "Error running ovs-vsctl command: %s: %s"
+            % (format(vsctl_cmd), cmd["stderr"]),
+        )
+    return cmd["stdout"]
+
+
+def update_args_from_ovs(args):
+    # get parent bridge to apply ruels to
+    args["parent-bridge"] = run_vsctl_cmd(["br-to-parent", args["bridge"]]).rstrip()
 
+    # get ports names for our actual bridge, be it fake (vlan) or real (no vlan)
+    ifs_in_bridge = run_vsctl_cmd(["list-ports", args["bridge"]]).split()
+
+    # get the list of all ports and filter them with the ports names to get all interfaces
+    ports_j = json.loads(run_vsctl_cmd(["--format=json", "list", "port"]))
+    ports = [dict(zip(ports_j["headings"], row)) for row in ports_j["data"]]
+    interfaces = []
+    for port in ports:
+        if port["name"] in ifs_in_bridge:
+            interfaces.append(port["interfaces"])
+    if len(interfaces) == 0:
+        return
+
+    # get the list of all interfaces, filter with what we found previously and get their ofports number
+    if_j = json.loads(run_vsctl_cmd(["--format=json", "list", "interface"]))
+    ifs = [dict(zip(if_j["headings"], row)) for row in if_j["data"]]
+    ofports = []
+    for interface in ifs:
+        # port 65534 is the internal port for the bridge, we don't want to use it
+        if interface["_uuid"] in interfaces and interface["ofport"] != 65534:
+            ofports.append(interface["ofport"])
+
+    args["ofports"] = ofports
+
+
+def build_rules_strings(args):
+    rules = []
+    if args.get("has_to"):
+        if args.get("ofports"):
+            for ofport in args["ofports"]:
+                rules.append(build_rule_string(TO, ofport, args))
+
+    if args.get("has_from"):
+        if args.get("ofports"):
+            for ofport in args["ofports"]:
+                rules.append(build_rule_string(FROM, ofport, args))
     return rules
 
+
+def build_rule_string(direction, ofport, args):
+    rule = ""
+    # To / From
+    rule_parts = {
+        "priority": ("priority", "priority"),
+        "protocol": (None, None),
+        "ofport": ("in_port", "in_port"),
+        "mac": ("dl_src", "dl_dst"),
+        "iprange": ("nw_dst", "nw_src"),
+        "port": ("tp_dst", "tp_src"),
+        "allow": ("actions", "actions"),
+    }
+
+    if args.get("priority"):
+        rule += "priority={}".format(args["priority"]) + ","
+    rule += args["protocol"]
+    if ofport:
+        rule += "," + rule_parts["ofport"][direction] + "={}".format(ofport)
+    if args.get("mac"):
+        rule += "," + rule_parts["mac"][direction] + "={}".format(args["mac"])
+    rule += "," + rule_parts["iprange"][direction] + "={}".format(args["iprange"])
+    if args.get("port"):
+        rule += "," + rule_parts["port"][direction] + "={}".format(args["port"])
+    if "allow" in args:  # optional in case of del_rule
+        rule += ",actions={}".format("normal" if args["allow"] else "drop")
+    return rule
+
+
+def run_ofctl_cmd(cmd, bridge, rule):
+    ofctl_cmd = [OVS_OFCTL_CMD, "-O", OPENFLOW_PROTOCOL, cmd, bridge, rule]
+    cmd = run_command(ofctl_cmd, check=False)
+    if cmd["returncode"] != 0:
+        log_and_raise_error(
+            E_OVSCALL,
+            "Error running ovs-ofctl command: %s: %s"
+            % (format(ofctl_cmd), cmd["stderr"]),
+        )
+    _LOGGER.info("Applied rule: {}".format(ofctl_cmd))
+
+
 @error_wrapped
 def add_rule(_session, args):
     _LOGGER.info("Calling add rule with args {}".format(args))
@@ -223,31 +312,39 @@ def add_rule(_session, args):
         parser.read("allow", parser.parse_allow)
         parser.read("priority", parser.parse_priority)
     except XenAPIPlugin.Failure as e:
-        log_and_raise_error(E_PARSER, "add_rule: Failed to get parameters: {}".format(e.params[1]))
+        log_and_raise_error(
+            E_PARSER, "add_rule: Failed to get parameters: {}".format(e.params[1])
+        )
 
     if parser.errors:
-        log_and_raise_error(E_PARSER, "add_rule: Failed to get parameters: {}".format(parser.errors))
+        log_and_raise_error(
+            E_PARSER, "add_rule: Failed to get parameters: {}".format(parser.errors)
+        )
 
     rule_args = parser.values
-    _LOGGER.info("successfully parsed: {}".format(rule_args))
 
     # sanity check
     if rule_args["protocol"] in PROTOCOLS_WITH_PORTS and not rule_args["port"]:
-        log_and_raise_error(E_PARAMS, "add_rule: No port provided, tcp and udp requires one")
+        log_and_raise_error(
+            E_PARAMS, "add_rule: No port provided, tcp and udp requires one"
+        )
 
-    ofctl_cmd = [OVS_OFCTL_CMD, "-O", OPENFLOW_PROTOCOL, "add-flow", rule_args["bridge"]]
+    # update vlanid first, as bridge could be replaced by parent bridge
+    update_args_from_ovs(rule_args)
+    if rule_args["ofports"] is None:
+        log_and_raise_error(
+            E_PORTS, "No ports found for bridge: {}".format(rule_args["bridge"])
+        )
 
     # We can now build the open flow rule
-    rules = build_rules_string(rule_args)
+    rules = build_rules_strings(rule_args)
+    _LOGGER.info("Built rules: {}".format(rules))
 
     if not rules:
-        log_and_raise_error(E_BUILDER, "add_rule: No rules were build")
+        log_and_raise_error(E_PORTS, "add_rule: No rules were build")
 
     for rule in rules:
-        cmd = run_command(ofctl_cmd + [rule], check=False)
-        if cmd['returncode'] != 0:
-            log_and_raise_error(E_OVSCALL, "Error adding rule: %s: %s" % (format(ofctl_cmd + [rule]), cmd['stderr']))
-        _LOGGER.info("Added rule: {}".format(ofctl_cmd + [rule]))
+        run_ofctl_cmd("add-flow", rule_args["parent-bridge"], rule)
 
     return json.dumps(True)
 
@@ -265,28 +362,33 @@ def del_rule(_session, args):
         parser.read("iprange", parser.parse_iprange)
         parser.read("port", parser.parse_port)
     except XenAPIPlugin.Failure as e:
-        log_and_raise_error(E_PARSER, "del_rule: Failed to get parameters: {}".format(e.params[1]))
+        log_and_raise_error(
+            E_PARSER, "del_rule: Failed to get parameters: {}".format(e.params[1])
+        )
 
     if parser.errors:
-        log_and_raise_error(E_PARSER, "del_rule: Failed to get parameters: {}".format(parser.errors))
+        log_and_raise_error(
+            E_PARSER, "del_rule: Failed to get parameters: {}".format(parser.errors)
+        )
 
     rule_args = parser.values
     _LOGGER.info("successfully parsed: {}".format(rule_args))
 
     # sanity check
     if rule_args["protocol"] in PROTOCOLS_WITH_PORTS and not rule_args["port"]:
-        log_and_raise_error(E_PARAMS, "del_rule: No port provided, tcp and udp requires one")
+        log_and_raise_error(
+            E_PARAMS, "del_rule: No port provided, tcp and udp requires one"
+        )
 
-    ofctl_cmd = [OVS_OFCTL_CMD, "-O", OPENFLOW_PROTOCOL, "del-flows", rule_args["bridge"]]
+    update_args_from_ovs(rule_args)
 
     # We can now build the open flow rule
-    rules = build_rules_string(rule_args)
+    rules = build_rules_strings(rule_args)
+    _LOGGER.info("Built rules: {}".format(rules))
 
     for rule in rules:
-        cmd = run_command(ofctl_cmd + [rule], check=False)
-        if cmd['returncode']:
-            log_and_raise_error(E_OVSCALL, "Error deleting rule: %s: %s" % (format(ofctl_cmd + [rule]), cmd['stderr']))
-        _LOGGER.info("Deleted rule: {}".format(ofctl_cmd + [rule]))
+        run_ofctl_cmd("del-flows", rule_args["parent-bridge"], rule)
+
     return json.dumps(True)
 
 
@@ -298,12 +400,17 @@ def dump_flows(_session, args):
     try:
         bridge = parser.parse_bridge()
     except XenAPIPlugin.Failure as e:
-        log_and_raise_error(E_PARSER, "dump_flows: Failed to get parameters: {}".format(e.params[1]))
+        log_and_raise_error(
+            E_PARSER, "dump_flows: Failed to get parameters: {}".format(e.params[1])
+        )
 
     ofctl_cmd = [OVS_OFCTL_CMD, "-O", OPENFLOW_PROTOCOL, "dump-flows", bridge]
     cmd = run_command(ofctl_cmd, check=False)
-    if cmd['returncode']:
-        log_and_raise_error(E_OVSCALL, "Error dumping flows: %s: %s" % (format(ofctl_cmd), cmd['stderr']))
+    if cmd["returncode"]:
+        log_and_raise_error(
+            E_OVSCALL,
+            "Error dumping flows: %s: %s" % (format(ofctl_cmd), cmd["stderr"]),
+        )
     return json.dumps(cmd)