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
