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.
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
