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. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
