Re: [ovs-dev] [PATCH ovn v5 2/3] northd: make default drops explicit

[Adrian Moreno]

On 11/18/22 15:55, Dumitru Ceara wrote:
On 11/4/22 16:49, Adrian Moreno wrote:
By default, traffic that doesn't match any configured flow will be dropped.
But having that behavior implicit makes those drops more difficult to
visualize.

Make default drops explicit both as default logical flows and as default
openflow flows (e.g: for physical tables).

Signed-off-by: Adrian Moreno <amore...@redhat.com>
---
  controller/physical.c   |  45 +++++++
  northd/northd.c         |  34 +++++-
  northd/ovn-northd.8.xml |  40 ++++++-
  tests/ovn-northd.at     |  84 +++++++++++++
  tests/ovn.at            | 256 +++++++++++++++++++++++++++++++++++-----
  5 files changed, 421 insertions(+), 38 deletions(-)

diff --git a/controller/physical.c b/controller/physical.c
index 705146316..415d16b76 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -833,6 +833,17 @@ put_zones_ofpacts(const struct zone_ids *zone_ids, struct 
ofpbuf *ofpacts_p)
      }
  }
  
+static void
+add_default_drop_flow(uint8_t table_id,
+                      struct ovn_desired_flow_table *flow_table)
+{
+    struct match match = MATCH_CATCHALL_INITIALIZER;
+    struct ofpbuf ofpacts;
+    ofpbuf_init(&ofpacts, 0);
+    ofctrl_add_flow(flow_table, table_id, 0, 0, &match,
+                    &ofpacts, hc_uuid);
+}
+
  static void
  put_local_common_flows(uint32_t dp_key,
                         const struct sbrec_port_binding *pb,
@@ -2114,6 +2125,13 @@ physical_run(struct physical_ctx *p_ctx,
          }
      }
  
+    /* Table 0, priority 0.
+     * ======================
+     *
+     * Drop packets tha do not match any tunnel in_port.
+     */
+    add_default_drop_flow(OFTABLE_PHY_TO_LOG, flow_table);
+
      /* Table 37, priority 150.
       * =======================
       *
@@ -2159,6 +2177,13 @@ physical_run(struct physical_ctx *p_ctx,
      ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 0, 0, &match,
                      &ofpacts, hc_uuid);
  
+    /* Table 38, priority 0.
+     * ======================
+     *
+     * Drop packets that do not match previous flows.
+     */
+    add_default_drop_flow(OFTABLE_LOCAL_OUTPUT, flow_table);
+
      /* Table 39, Priority 0.
       * =======================
       *
@@ -2185,5 +2210,25 @@ physical_run(struct physical_ctx *p_ctx,
      ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 0, 0, &match,
                      &ofpacts, hc_uuid);
  
+    /* Table 65, priority 0.
+     * ======================
+     *
+     * Drop packets that do not match previous flows.
+     */
+    add_default_drop_flow(OFTABLE_LOG_TO_PHY, flow_table);
+
+    /* Table 68, priority 0.
+     * ======================
+     *
+     * Drop packets that do not match previous flows.
+     */
+    add_default_drop_flow(OFTABLE_CHK_LB_HAIRPIN, flow_table);

We never drop in this table.  No need for a default drop flow.


Hi Dumitru,

What do you mean by "we never drop"?

The default behavior for a packet not matching any rule in a table is to drop 
(unless explicitly changed) so my rationale for putting default drop flows on 
every table was:
Even if traffic is never supposed to reach the default drop flow, adding it will 
help catch bugs and make maintaining this "good habit" easier: Unit test ensures 
all tables have at least one default rule.


+
+    /* Table 70, priority 0.
+     * ======================
+     *
+     * Drop packets that do not match previous flows.
+     */
+    add_default_drop_flow(OFTABLE_CT_SNAT_HAIRPIN, flow_table);

Same here.

      ofpbuf_uninit(&ofpacts);
  }

Thanks,
Dumitru


--
Adrián Moreno

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev