On Thu, Mar 23, 2023 at 3:38 PM Numan Siddique <[email protected]> wrote: > > On Wed, Mar 22, 2023 at 6:35 AM Dumitru Ceara <[email protected]> wrote: > > > > On 3/22/23 03:33, Numan Siddique wrote: > > > On Mon, Mar 20, 2023 at 2:30 PM Lorenzo Bianconi > > > <[email protected]> wrote: > > >> > > >> Drop ip packets with ct status set to invalid in post snat and > > >> lb_aff_learn router stages. > > >> Skip ICMPv{4,6} error messages packet in ct.inv rules in order to avoid > > >> to introduce too complicated code. > > >> > > >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2160685 > > >> Reviewed-by: Simon Horman <[email protected]> > > >> Signed-off-by: Lorenzo Bianconi <[email protected]> > > >> --- > > >> Changes since v3: > > >> - rebase on top of ovn main branch > > >> Changes since v2: > > >> - rebase on top of ovn main branch > > >> - cosmetics > > >> Changes since v1: > > >> - skip ICMPv{4,6} error messages packet in ct.inv rules > > >> - this series is based on the following series not yet applied in ovn > > >> master: > > >> https://patchwork.ozlabs.org/project/ovn/list/?series=343841 > > > > > > Hi Lorenzo, > > > > > > The patch overall LGTM. > > > > > > I've few minor comments. Please see below > > > > > > > > >> --- > > >> northd/northd.c | 30 ++++++++++++++++++++- > > >> northd/ovn-northd.8.xml | 43 +++++++++++++++++++++++++++-- > > >> tests/ovn-northd.at | 13 +++++++++ > > >> tests/ovn.at | 60 +++++++++++++++++++++-------------------- > > >> tests/system-ovn.at | 16 +++++------ > > >> 5 files changed, 122 insertions(+), 40 deletions(-) > > >> > > >> diff --git a/northd/northd.c b/northd/northd.c > > >> index 5f0b436c2..5884f50e1 100644 > > >> --- a/northd/northd.c > > >> +++ b/northd/northd.c > > >> @@ -13822,6 +13822,31 @@ build_lrouter_out_is_dnat_local(struct hmap > > >> *lflows, struct ovn_datapath *od, > > >> &nat->header_); > > >> } > > >> > > >> +static void > > >> +build_lrouter_drop_ct_inv_flow(struct ovn_datapath *od, struct hmap > > >> *lflows) > > > > > >> +{ > > >> + if (!od->nbr) { > > >> + return; > > >> + } > > >> + > > >> + ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 0, "1", "next;"); > > > > > > Small nit - Since the name of the function is - > > > build_lrouter_drop_ct_inv_flow(), I'd suggest moving the above logical > > > flow with prio-0 back to its original place. > > > > > > > > > > > > > > >> + > > >> + if (use_ct_inv_match) { > > > > > > Can you please add a few comments on the icmp type and codes used below. > > > It's not obvious the logical flow is added to advance the Packet too > > > big icmp packets. > > > > > > With these addressed : > > > > > > Acked-by: Numan Siddique <[email protected]> > > > > > > > > > I'm not too sure if it's a good idea to add a new predicate symbol in > > > the symbol table (in lib/logical-fields.c) for these icmp error types > > > and codes ? > > > > > > Like > > > > > > expr_symtab_add_predicate(symtab, "icmp4_too_big", "icmp4.type == 3 && > > > icmp4.code == 4"); > > > > > > I don't like the name - icmp4_too_big though. > > > > > > Thoughts ? > > > > > > @Dumitru Ceara wdyt ? > > > > > > I'm fine if we don't want to add this as this could result in > > > ovn-controllers rejecting the logical flow if ovn-northd is upgraded > > > first. > > > > I agree, this is probably good enough reason to not add a new predicate. > > I applied this patch with the below changes. > > --- > diff --git a/northd/northd.c b/northd/northd.c > index 5884f50e1d..76199a59f2 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -13825,23 +13825,20 @@ build_lrouter_out_is_dnat_local(struct hmap > *lflows, struct ovn_datapath *od, > static void > build_lrouter_drop_ct_inv_flow(struct ovn_datapath *od, struct hmap *lflows) > { > - if (!od->nbr) { > - return; > - } > - > - ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 0, "1", "next;"); > - > - if (use_ct_inv_match) { > + if (od->nbr && use_ct_inv_match) { > + /* Advance ICMP Destination Unreachable - Fragmentation Needed > + * packets and drop other packets which are ct tracked and invalid. > */ > ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 150, > "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||" > " (ip6 && icmp6.type == 2 && icmp6.code == 0))", > "next;"); > - ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 100, > - "ip && ct.trk && ct.inv", debug_drop_action()); > ovn_lflow_add(lflows, od, S_ROUTER_IN_LB_AFF_LEARN, 250, > "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||" > " (ip6 && icmp6.type == 2 && icmp6.code == 0))", > "next;"); > + > + ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 100, > + "ip && ct.trk && ct.inv", debug_drop_action()); > ovn_lflow_add(lflows, od, S_ROUTER_IN_LB_AFF_LEARN, 200, > "ip && ct.trk && ct.inv", debug_drop_action()); > } > @@ -14235,6 +14232,7 @@ build_lrouter_nat_defrag_and_lb(struct > ovn_datapath *od, struct hmap *lflows, > ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 0, "1", "next;"); > ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 0, "1", "next;"); > ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_UNDNAT, 0, "1", "next;"); > + ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 0, "1", "next;"); > ovn_lflow_add(lflows, od, S_ROUTER_OUT_EGR_LOOP, 0, "1", "next;"); > ovn_lflow_add(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 0, "1", "next;"); > > ----- > > > I'll backport to branch-23.03 and branch-22.12 once the CI passes for > these branches in my local repo.
Backported to branch-23.03. But the tests are failing for 22.12. If you want this to be backported to branch-22.12 please fix the test case failures and submit branch-22.12 only patch. Numan > > Thanks > Numan > > > > > Thanks, > > Dumitru > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
