[ovs-dev,ovn,3/3] northd: interoduce logical flow for localnet egress shaping
diff mbox series

Message ID 8bea1e79bc209de4e413038bd7d79a84046a0ead.1569250466.git.lorenzo.bianconi@redhat.com
State New
Headers show
Series
  • Introduce localnet egress QoS support
Related show

Commit Message

Lorenzo Bianconi Sept. 23, 2019, 3:09 p.m. UTC
Add set_queue() action for qos capable localnet port in
S_SWITCH_OUT_PORT_SEC_L2 stage of logical swith pipeline
Introduce build_lswitch_port_sec and refactor lswitch_port_security code
in order to remove duplicated code

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 northd/ovn-northd.8.xml |   7 +-
 northd/ovn-northd.c     | 199 +++++++++++++++++++++-------------------
 2 files changed, 110 insertions(+), 96 deletions(-)

Comments

Dumitru Ceara Sept. 24, 2019, 10:38 a.m. UTC | #1
On Mon, Sep 23, 2019 at 5:09 PM Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> Add set_queue() action for qos capable localnet port in
> S_SWITCH_OUT_PORT_SEC_L2 stage of logical swith pipeline
> Introduce build_lswitch_port_sec and refactor lswitch_port_security code
> in order to remove duplicated code
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

There's a typo in the subject line: s/interoduce/Introduce.
> ---
>  northd/ovn-northd.8.xml |   7 +-
>  northd/ovn-northd.c     | 199 +++++++++++++++++++++-------------------
>  2 files changed, 110 insertions(+), 96 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 0f4f1c112..093b55438 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1150,10 +1150,15 @@ output;
>        <code>eth.dst</code> are always accepted instead of being subject to the
>        port security rules; this is implemented through a priority-100 flow that
>        matches on <code>eth.mcast</code> with action <code>output;</code>.
> -      Finally, to ensure that even broadcast and multicast packets are not
> +      Moreover, to ensure that even broadcast and multicast packets are not
>        delivered to disabled logical ports, a priority-150 flow for each
>        disabled logical <code>outport</code> overrides the priority-100 flow
>        with a <code>drop;</code> action.
> +      Finally if egress qos has been enabled on a localnet port, the outgoing
> +      queue id is set through <code>set_queue</code> action. Please remember to
> +      mark the corresponding physical interface with
> +      <code>ovn-egress-iface</code> set to true in <ref column="external_ids"
> +      table="Interface" db="Open_vSwitch"/>
>      </p>
>
>      <h2>Logical Router Datapaths</h2>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 633fb502b..4bacad572 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -3903,6 +3903,108 @@ has_stateful_acl(struct ovn_datapath *od)
>      return false;
>  }
>
> +static void
> +build_lswitch_port_sec(struct hmap *ports, struct hmap *datapaths,
> +                       struct hmap *lflows)
> +{

Isn't it better to split this function into two functions (one for
ingress and one for egress)? What do you think?

> +    struct ds match = DS_EMPTY_INITIALIZER;
> +    struct ds actions = DS_EMPTY_INITIALIZER;
> +    struct ovn_datapath *od;
> +    struct ovn_port *op;
> +
> +    HMAP_FOR_EACH (op, key_node, ports) {
> +        if (!op->nbsp) {
> +            continue;
> +        }
> +
> +        if (lsp_is_external(op->nbsp)) {
> +            continue;
> +        }
> +
> +        ds_clear(&match);
> +        ds_put_format(&match, "outport == %s", op->json_key);
> +
> +        /* Egress table 8: Egress port security - IP (priorities 90 and 80)
> +         * if port security enabled.
> +         *
> +         * Egress table 9: Egress port security - L2 (priorities 50 and 150).
> +         *
> +         * Priority 50 rules implement port security for enabled logical port.
> +         *
> +         * Priority 150 rules drop packets to disabled logical ports, so that
> +         * they don't even receive multicast or broadcast packets.
> +         */
> +        if (!lsp_is_enabled(op->nbsp)) {
> +            ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 150,
> +                          ds_cstr(&match), "drop;");
> +            build_port_security_ip(P_OUT, op, lflows);

Do we still need to add port security flows for IP if the port is
disabled? Won't the drop flow above suffice?

Thanks,
Dumitru

> +            continue;
> +        }
> +
> +        ds_clear(&actions);
> +        if (!strcmp(op->nbsp->type, "localnet")) {
> +            const char *queue_id = smap_get(&op->sb->options,
> +                                            "qdisc_queue_id");
> +            if (queue_id) {
> +                ds_put_format(&actions, "set_queue(%s); ", queue_id);
> +            }
> +        }
> +        ds_put_cstr(&actions, "output;");
> +        build_port_security_l2("eth.dst", op->ps_addrs, op->n_ps_addrs,
> +                               &match);
> +        ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 50,
> +                          ds_cstr(&match), ds_cstr(&actions));
> +
> +        ds_clear(&match);
> +        ds_clear(&actions);
> +
> +        /* Logical switch ingress table 0: Ingress port security - L2
> +         *  (priority 50).
> +         *  Ingress table 1: Ingress port security - IP (priority 90 and 80)
> +         *  Ingress table 2: Ingress port security - ND (priority 90 and 80)
> +         */
> +        ds_put_format(&match, "inport == %s", op->json_key);
> +        build_port_security_l2("eth.src", op->ps_addrs, op->n_ps_addrs,
> +                               &match);
> +
> +        const char *queue_id = smap_get(&op->sb->options, "qdisc_queue_id");
> +        if (queue_id) {
> +            ds_put_format(&actions, "set_queue(%s); ", queue_id);
> +        }
> +        ds_put_cstr(&actions, "next;");
> +        ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_L2, 50,
> +                      ds_cstr(&match), ds_cstr(&actions));
> +
> +        if (op->nbsp->n_port_security) {
> +            build_port_security_ip(P_IN, op, lflows);
> +            build_port_security_ip(P_OUT, op, lflows);
> +            build_port_security_nd(op, lflows);
> +        }
> +    }
> +
> +    HMAP_FOR_EACH (od, key_node, datapaths) {
> +        if (!od->nbs) {
> +            continue;
> +        }
> +
> +        /* Ingress table 1 and 2: Port security - IP and ND, by default
> +         * goto next. (priority 0)
> +         */
> +        ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_ND, 0, "1", "next;");
> +        ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_IP, 0, "1", "next;");
> +
> +        /* Egress tables 8: Egress port security - IP (priority 0)
> +         * Egress table 9: Egress port security L2 - multicast/broadcast
> +         *                 (priority 100). */
> +        ovn_lflow_add(lflows, od, S_SWITCH_OUT_PORT_SEC_IP, 0, "1", "next;");
> +        ovn_lflow_add(lflows, od, S_SWITCH_OUT_PORT_SEC_L2, 100, "eth.mcast",
> +                      "output;");
> +    }
> +
> +    ds_destroy(&match);
> +    ds_destroy(&actions);
> +}
> +
>  static void
>  build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
>  {
> @@ -4974,61 +5076,12 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>           * to the next table if packet source is acceptable. */
>      }
>
> -    /* Logical switch ingress table 0: Ingress port security - L2
> -     *  (priority 50).
> -     *  Ingress table 1: Ingress port security - IP (priority 90 and 80)
> -     *  Ingress table 2: Ingress port security - ND (priority 90 and 80)
> -     */
> -    struct ovn_port *op;
> -    HMAP_FOR_EACH (op, key_node, ports) {
> -        if (!op->nbsp) {
> -            continue;
> -        }
> -
> -        if (!lsp_is_enabled(op->nbsp)) {
> -            /* Drop packets from disabled logical ports (since logical flow
> -             * tables are default-drop). */
> -            continue;
> -        }
> -
> -        if (lsp_is_external(op->nbsp)) {
> -            continue;
> -        }
> -
> -        ds_clear(&match);
> -        ds_clear(&actions);
> -        ds_put_format(&match, "inport == %s", op->json_key);
> -        build_port_security_l2("eth.src", op->ps_addrs, op->n_ps_addrs,
> -                               &match);
> -
> -        const char *queue_id = smap_get(&op->sb->options, "qdisc_queue_id");
> -        if (queue_id) {
> -            ds_put_format(&actions, "set_queue(%s); ", queue_id);
> -        }
> -        ds_put_cstr(&actions, "next;");
> -        ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_L2, 50,
> -                      ds_cstr(&match), ds_cstr(&actions));
> -
> -        if (op->nbsp->n_port_security) {
> -            build_port_security_ip(P_IN, op, lflows);
> -            build_port_security_nd(op, lflows);
> -        }
> -    }
> -
> -    /* Ingress table 1 and 2: Port security - IP and ND, by default goto next.
> -     * (priority 0)*/
> -    HMAP_FOR_EACH (od, key_node, datapaths) {
> -        if (!od->nbs) {
> -            continue;
> -        }
> -
> -        ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_ND, 0, "1", "next;");
> -        ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_IP, 0, "1", "next;");
> -    }
> +    build_lswitch_port_sec(ports, datapaths, lflows);
>
>      /* Ingress table 11: ARP/ND responder, skip requests coming from localnet
>       * and vtep ports. (priority 100); see ovn-northd.8.xml for the
>       * rationale. */
> +    struct ovn_port *op;
>      HMAP_FOR_EACH (op, key_node, ports) {
>          if (!op->nbsp) {
>              continue;
> @@ -5651,50 +5704,6 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>          }
>      }
>
> -    /* Egress tables 8: Egress port security - IP (priority 0)
> -     * Egress table 9: Egress port security L2 - multicast/broadcast
> -     *                 (priority 100). */
> -    HMAP_FOR_EACH (od, key_node, datapaths) {
> -        if (!od->nbs) {
> -            continue;
> -        }
> -
> -        ovn_lflow_add(lflows, od, S_SWITCH_OUT_PORT_SEC_IP, 0, "1", "next;");
> -        ovn_lflow_add(lflows, od, S_SWITCH_OUT_PORT_SEC_L2, 100, "eth.mcast",
> -                      "output;");
> -    }
> -
> -    /* Egress table 8: Egress port security - IP (priorities 90 and 80)
> -     * if port security enabled.
> -     *
> -     * Egress table 9: Egress port security - L2 (priorities 50 and 150).
> -     *
> -     * Priority 50 rules implement port security for enabled logical port.
> -     *
> -     * Priority 150 rules drop packets to disabled logical ports, so that they
> -     * don't even receive multicast or broadcast packets. */
> -    HMAP_FOR_EACH (op, key_node, ports) {
> -        if (!op->nbsp || lsp_is_external(op->nbsp)) {
> -            continue;
> -        }
> -
> -        ds_clear(&match);
> -        ds_put_format(&match, "outport == %s", op->json_key);
> -        if (lsp_is_enabled(op->nbsp)) {
> -            build_port_security_l2("eth.dst", op->ps_addrs, op->n_ps_addrs,
> -                                   &match);
> -            ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 50,
> -                          ds_cstr(&match), "output;");
> -        } else {
> -            ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 150,
> -                          ds_cstr(&match), "drop;");
> -        }
> -
> -        if (op->nbsp->n_port_security) {
> -            build_port_security_ip(P_OUT, op, lflows);
> -        }
> -    }
> -
>      ds_destroy(&match);
>      ds_destroy(&actions);
>  }
> --
> 2.21.0
>

Patch
diff mbox series

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 0f4f1c112..093b55438 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1150,10 +1150,15 @@  output;
       <code>eth.dst</code> are always accepted instead of being subject to the
       port security rules; this is implemented through a priority-100 flow that
       matches on <code>eth.mcast</code> with action <code>output;</code>.
-      Finally, to ensure that even broadcast and multicast packets are not
+      Moreover, to ensure that even broadcast and multicast packets are not
       delivered to disabled logical ports, a priority-150 flow for each
       disabled logical <code>outport</code> overrides the priority-100 flow
       with a <code>drop;</code> action.
+      Finally if egress qos has been enabled on a localnet port, the outgoing
+      queue id is set through <code>set_queue</code> action. Please remember to
+      mark the corresponding physical interface with
+      <code>ovn-egress-iface</code> set to true in <ref column="external_ids"
+      table="Interface" db="Open_vSwitch"/>
     </p>
 
     <h2>Logical Router Datapaths</h2>
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 633fb502b..4bacad572 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -3903,6 +3903,108 @@  has_stateful_acl(struct ovn_datapath *od)
     return false;
 }
 
+static void
+build_lswitch_port_sec(struct hmap *ports, struct hmap *datapaths,
+                       struct hmap *lflows)
+{
+    struct ds match = DS_EMPTY_INITIALIZER;
+    struct ds actions = DS_EMPTY_INITIALIZER;
+    struct ovn_datapath *od;
+    struct ovn_port *op;
+
+    HMAP_FOR_EACH (op, key_node, ports) {
+        if (!op->nbsp) {
+            continue;
+        }
+
+        if (lsp_is_external(op->nbsp)) {
+            continue;
+        }
+
+        ds_clear(&match);
+        ds_put_format(&match, "outport == %s", op->json_key);
+
+        /* Egress table 8: Egress port security - IP (priorities 90 and 80)
+         * if port security enabled.
+         *
+         * Egress table 9: Egress port security - L2 (priorities 50 and 150).
+         *
+         * Priority 50 rules implement port security for enabled logical port.
+         *
+         * Priority 150 rules drop packets to disabled logical ports, so that
+         * they don't even receive multicast or broadcast packets.
+         */
+        if (!lsp_is_enabled(op->nbsp)) {
+            ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 150,
+                          ds_cstr(&match), "drop;");
+            build_port_security_ip(P_OUT, op, lflows);
+            continue;
+        }
+
+        ds_clear(&actions);
+        if (!strcmp(op->nbsp->type, "localnet")) {
+            const char *queue_id = smap_get(&op->sb->options,
+                                            "qdisc_queue_id");
+            if (queue_id) {
+                ds_put_format(&actions, "set_queue(%s); ", queue_id);
+            }
+        }
+        ds_put_cstr(&actions, "output;");
+        build_port_security_l2("eth.dst", op->ps_addrs, op->n_ps_addrs,
+                               &match);
+        ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 50,
+                          ds_cstr(&match), ds_cstr(&actions));
+
+        ds_clear(&match);
+        ds_clear(&actions);
+
+        /* Logical switch ingress table 0: Ingress port security - L2
+         *  (priority 50).
+         *  Ingress table 1: Ingress port security - IP (priority 90 and 80)
+         *  Ingress table 2: Ingress port security - ND (priority 90 and 80)
+         */
+        ds_put_format(&match, "inport == %s", op->json_key);
+        build_port_security_l2("eth.src", op->ps_addrs, op->n_ps_addrs,
+                               &match);
+
+        const char *queue_id = smap_get(&op->sb->options, "qdisc_queue_id");
+        if (queue_id) {
+            ds_put_format(&actions, "set_queue(%s); ", queue_id);
+        }
+        ds_put_cstr(&actions, "next;");
+        ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_L2, 50,
+                      ds_cstr(&match), ds_cstr(&actions));
+
+        if (op->nbsp->n_port_security) {
+            build_port_security_ip(P_IN, op, lflows);
+            build_port_security_ip(P_OUT, op, lflows);
+            build_port_security_nd(op, lflows);
+        }
+    }
+
+    HMAP_FOR_EACH (od, key_node, datapaths) {
+        if (!od->nbs) {
+            continue;
+        }
+
+        /* Ingress table 1 and 2: Port security - IP and ND, by default
+         * goto next. (priority 0)
+         */
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_ND, 0, "1", "next;");
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_IP, 0, "1", "next;");
+
+        /* Egress tables 8: Egress port security - IP (priority 0)
+         * Egress table 9: Egress port security L2 - multicast/broadcast
+         *                 (priority 100). */
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_PORT_SEC_IP, 0, "1", "next;");
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_PORT_SEC_L2, 100, "eth.mcast",
+                      "output;");
+    }
+
+    ds_destroy(&match);
+    ds_destroy(&actions);
+}
+
 static void
 build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
 {
@@ -4974,61 +5076,12 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
          * to the next table if packet source is acceptable. */
     }
 
-    /* Logical switch ingress table 0: Ingress port security - L2
-     *  (priority 50).
-     *  Ingress table 1: Ingress port security - IP (priority 90 and 80)
-     *  Ingress table 2: Ingress port security - ND (priority 90 and 80)
-     */
-    struct ovn_port *op;
-    HMAP_FOR_EACH (op, key_node, ports) {
-        if (!op->nbsp) {
-            continue;
-        }
-
-        if (!lsp_is_enabled(op->nbsp)) {
-            /* Drop packets from disabled logical ports (since logical flow
-             * tables are default-drop). */
-            continue;
-        }
-
-        if (lsp_is_external(op->nbsp)) {
-            continue;
-        }
-
-        ds_clear(&match);
-        ds_clear(&actions);
-        ds_put_format(&match, "inport == %s", op->json_key);
-        build_port_security_l2("eth.src", op->ps_addrs, op->n_ps_addrs,
-                               &match);
-
-        const char *queue_id = smap_get(&op->sb->options, "qdisc_queue_id");
-        if (queue_id) {
-            ds_put_format(&actions, "set_queue(%s); ", queue_id);
-        }
-        ds_put_cstr(&actions, "next;");
-        ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_L2, 50,
-                      ds_cstr(&match), ds_cstr(&actions));
-
-        if (op->nbsp->n_port_security) {
-            build_port_security_ip(P_IN, op, lflows);
-            build_port_security_nd(op, lflows);
-        }
-    }
-
-    /* Ingress table 1 and 2: Port security - IP and ND, by default goto next.
-     * (priority 0)*/
-    HMAP_FOR_EACH (od, key_node, datapaths) {
-        if (!od->nbs) {
-            continue;
-        }
-
-        ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_ND, 0, "1", "next;");
-        ovn_lflow_add(lflows, od, S_SWITCH_IN_PORT_SEC_IP, 0, "1", "next;");
-    }
+    build_lswitch_port_sec(ports, datapaths, lflows);
 
     /* Ingress table 11: ARP/ND responder, skip requests coming from localnet
      * and vtep ports. (priority 100); see ovn-northd.8.xml for the
      * rationale. */
+    struct ovn_port *op;
     HMAP_FOR_EACH (op, key_node, ports) {
         if (!op->nbsp) {
             continue;
@@ -5651,50 +5704,6 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
         }
     }
 
-    /* Egress tables 8: Egress port security - IP (priority 0)
-     * Egress table 9: Egress port security L2 - multicast/broadcast
-     *                 (priority 100). */
-    HMAP_FOR_EACH (od, key_node, datapaths) {
-        if (!od->nbs) {
-            continue;
-        }
-
-        ovn_lflow_add(lflows, od, S_SWITCH_OUT_PORT_SEC_IP, 0, "1", "next;");
-        ovn_lflow_add(lflows, od, S_SWITCH_OUT_PORT_SEC_L2, 100, "eth.mcast",
-                      "output;");
-    }
-
-    /* Egress table 8: Egress port security - IP (priorities 90 and 80)
-     * if port security enabled.
-     *
-     * Egress table 9: Egress port security - L2 (priorities 50 and 150).
-     *
-     * Priority 50 rules implement port security for enabled logical port.
-     *
-     * Priority 150 rules drop packets to disabled logical ports, so that they
-     * don't even receive multicast or broadcast packets. */
-    HMAP_FOR_EACH (op, key_node, ports) {
-        if (!op->nbsp || lsp_is_external(op->nbsp)) {
-            continue;
-        }
-
-        ds_clear(&match);
-        ds_put_format(&match, "outport == %s", op->json_key);
-        if (lsp_is_enabled(op->nbsp)) {
-            build_port_security_l2("eth.dst", op->ps_addrs, op->n_ps_addrs,
-                                   &match);
-            ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 50,
-                          ds_cstr(&match), "output;");
-        } else {
-            ovn_lflow_add(lflows, op->od, S_SWITCH_OUT_PORT_SEC_L2, 150,
-                          ds_cstr(&match), "drop;");
-        }
-
-        if (op->nbsp->n_port_security) {
-            build_port_security_ip(P_OUT, op, lflows);
-        }
-    }
-
     ds_destroy(&match);
     ds_destroy(&actions);
 }