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 <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, >
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. > >>> + >>> + /* 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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev