On 11/21/22 13:35, Dumitru Ceara wrote:
On 11/21/22 13:18, Adrian Moreno wrote: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 <[email protected]> --- 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,Hi Adrian,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.We use this table to detect if a packet is "hairpin" (that is, that its source and destination are the same). To do that, we resubmit to OFTABLE_CHK_LB_HAIRPIN where we have flows in place to match on all types of "hairpin" traffic. That's part of the chk_lb_hairpin() logical action. That is encoded as: encode_CHK_LB_HAIRPIN() --> encode_result_action__() https://github.com/ovn-org/ovn/blob/661094c40e6c7ef67778e0229a8861d33bb63bf5/lib/actions.c#L4019 This first sets the flags[7] bit (MLF_LOOKUP_LB_HAIRPIN_BIT) to 0 and then resubmits to OFTABLE_CHK_LB_HAIRPIN. Now there are two cases: a. a match happens in OFTABLE_CHK_LB_HAIRPIN with action load 1 to flags[7]. b. no match happens. We then continue the pipeline, after the resubmit, and set the destination register bit to the value of flags[7]. So there's never a drop in this table. And traffic reaching the default rule (or not matching any rule as a matter of fact) should be fine in this case.
Thanks for the explanation Dumitru.So, we don't need to explicitly visualize these 'drops'? so neither explicit drop nor "sample" action, right?
+ + /* Table 70, priority 0. + * ====================== + * + * Drop packets that do not match previous flows. + */ + add_default_drop_flow(OFTABLE_CT_SNAT_HAIRPIN, flow_table);Same here.Here we also don't drop any packets. This table is used as part the "ct_snat_to_vip;" logical action. If there's no match no snat happens and we advance to the next action. If there's no next action then there's indeed a drop. But that's in the "source" openflow table: https://github.com/ovn-org/ovn/blob/661094c40e6c7ef67778e0229a8861d33bb63bf5/northd/northd.c#L7095 Thanks, Dumitru
-- Adrián Moreno _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
