On 11/21/22 14:32, Adrian Moreno wrote: > > > 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? >
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 >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
