On Thu, Apr 1, 2021 at 3:22 PM Lorenzo Bianconi
<[email protected]> wrote:
>
> >
> > Also improve the tests to make sure this doesn't break again in the
> > future.
> >
> > Fixes: 225426081f85 ("northd: introduce build_lrouter_in_dnat_flow routine")
> > CC: Lorenzo Bianconi <[email protected]>
> > Reported-by: Ilya Maximets <[email protected]>
> > Signed-off-by: Dumitru Ceara <[email protected]>
> > ---
> > Note: the regression was only in the ovn-northd C implementation, there
> > are no ovn-northd-ddlog changes required.
> > ---
> > northd/ovn-northd.c | 26 ++++++++++++--------------
> > tests/ovn-northd.at | 40 +++++++++++++++++++++++++++-------------
> > 2 files changed, 39 insertions(+), 27 deletions(-)
>
> Thx for fixing this:
>
> Acked-by: Lorenzo Bianconi <[email protected]>
Thanks Ilya and Dumitru for reporting and fixing this.
I applied this patch to master.
Numan
>
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 57df62b92..9839b8c4f 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -11240,20 +11240,6 @@ build_lrouter_in_dnat_flow(struct hmap *lflows,
> > struct ovn_datapath *od,
> > &nat->header_);
> > }
> > }
> > -
> > - if (!od->l3dgw_port) {
> > - /* For gateway router, re-circulate every packet through
> > - * the DNAT zone. This helps with the following.
> > - *
> > - * Any packet that needs to be unDNATed in the reverse
> > - * direction gets unDNATed. Ideally this could be done in
> > - * the egress pipeline. But since the gateway router
> > - * does not have any feature that depends on the source
> > - * ip address being external IP address for IP routing,
> > - * we can do it here, saving a future re-circulation. */
> > - ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50,
> > - "ip", "flags.loopback = 1; ct_dnat;");
> > - }
> > }
> >
> > static void
> > @@ -11716,6 +11702,18 @@ build_lrouter_nat_defrag_and_lb(struct
> > ovn_datapath *od,
> > od->lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb");
> > }
> > }
> > +
> > + /* For gateway router, re-circulate every packet through
> > + * the DNAT zone. This helps with the following.
> > + *
> > + * Any packet that needs to be unDNATed in the reverse
> > + * direction gets unDNATed. Ideally this could be done in
> > + * the egress pipeline. But since the gateway router
> > + * does not have any feature that depends on the source
> > + * ip address being external IP address for IP routing,
> > + * we can do it here, saving a future re-circulation. */
> > + ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50,
> > + "ip", "flags.loopback = 1; ct_dnat;");
> > }
> >
> > /* Load balancing and packet defrag are only valid on
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 47f2662e4..96476497d 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -2700,7 +2700,7 @@ wait_row_count nb:Logical_Switch_Port 1 up=false
> > name=lsp1
> > AT_CLEANUP
> >
> > OVN_FOR_EACH_NORTHD([
> > -AT_SETUP([ovn -- lb_force_snat_ip for Gateway Routers])
> > +AT_SETUP([ovn -- Load Balancers and lb_force_snat_ip for Gateway Routers])
> > ovn_start
> >
> > check ovn-nbctl ls-add sw0
> > @@ -2740,11 +2740,11 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort],
> > [0], [dnl
> > table=5 (lr_in_unsnat ), priority=0 , match=(1), action=(next;)
> > ])
> >
> > -AT_CHECK([grep "lr_in_dnat" lr0flows | grep force_snat_for_lb | sort],
> > [0], [dnl
> > -])
> > -
> > -
> > -AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort],
> > [0], [dnl
> > +AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> > + table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
> > + table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip &&
> > ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80), action=(ct_dnat;)
> > + table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip &&
> > ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80),
> > action=(ct_lb(backends=10.0.0.4:8080);)
> > + table=6 (lr_in_dnat ), priority=50 , match=(ip),
> > action=(flags.loopback = 1; ct_dnat;)
> > ])
> >
> > check ovn-nbctl --wait=sb set logical_router lr0
> > options:lb_force_snat_ip="20.0.0.4 aef0::4"
> > @@ -2759,14 +2759,18 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort],
> > [0], [dnl
> > table=5 (lr_in_unsnat ), priority=110 , match=(ip6 && ip6.dst ==
> > aef0::4), action=(ct_snat;)
> > ])
> >
> > -AT_CHECK([grep "lr_in_dnat" lr0flows | grep force_snat_for_lb | sort],
> > [0], [dnl
> > +AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> > + table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
> > table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip &&
> > ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80),
> > action=(flags.force_snat_for_lb = 1; ct_dnat;)
> > table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip &&
> > ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80),
> > action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.4:8080);)
> > + table=6 (lr_in_dnat ), priority=50 , match=(ip),
> > action=(flags.loopback = 1; ct_dnat;)
> > ])
> >
> > -AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort],
> > [0], [dnl
> > +AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl
> > + table=1 (lr_out_snat ), priority=0 , match=(1), action=(next;)
> > table=1 (lr_out_snat ), priority=100 ,
> > match=(flags.force_snat_for_lb == 1 && ip4), action=(ct_snat(20.0.0.4);)
> > table=1 (lr_out_snat ), priority=100 ,
> > match=(flags.force_snat_for_lb == 1 && ip6), action=(ct_snat(aef0::4);)
> > + table=1 (lr_out_snat ), priority=120 , match=(nd_ns),
> > action=(next;)
> > ])
> >
> > check ovn-nbctl --wait=sb set logical_router lr0
> > options:lb_force_snat_ip="router_ip"
> > @@ -2784,15 +2788,19 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort],
> > [0], [dnl
> > table=5 (lr_in_unsnat ), priority=110 , match=(inport ==
> > "lr0-sw1" && ip4.dst == 20.0.0.1), action=(ct_snat;)
> > ])
> >
> > -AT_CHECK([grep "lr_in_dnat" lr0flows | grep force_snat_for_lb | sort],
> > [0], [dnl
> > +AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> > + table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
> > table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip &&
> > ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80),
> > action=(flags.force_snat_for_lb = 1; ct_dnat;)
> > table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip &&
> > ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80),
> > action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.4:8080);)
> > + table=6 (lr_in_dnat ), priority=50 , match=(ip),
> > action=(flags.loopback = 1; ct_dnat;)
> > ])
> >
> > -AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort],
> > [0], [dnl
> > +AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl
> > + table=1 (lr_out_snat ), priority=0 , match=(1), action=(next;)
> > table=1 (lr_out_snat ), priority=110 ,
> > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"),
> > action=(ct_snat(172.168.0.100);)
> > table=1 (lr_out_snat ), priority=110 ,
> > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"),
> > action=(ct_snat(10.0.0.1);)
> > table=1 (lr_out_snat ), priority=110 ,
> > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"),
> > action=(ct_snat(20.0.0.1);)
> > + table=1 (lr_out_snat ), priority=120 , match=(nd_ns),
> > action=(next;)
> > ])
> >
> > check ovn-nbctl --wait=sb remove logical_router lr0 options chassis
> > @@ -2804,7 +2812,9 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0],
> > [dnl
> > table=5 (lr_in_unsnat ), priority=0 , match=(1), action=(next;)
> > ])
> >
> > -AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort],
> > [0], [dnl
> > +AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl
> > + table=1 (lr_out_snat ), priority=0 , match=(1), action=(next;)
> > + table=1 (lr_out_snat ), priority=120 , match=(nd_ns),
> > action=(next;)
> > ])
> >
> > check ovn-nbctl set logical_router lr0 options:chassis=ch1
> > @@ -2821,16 +2831,20 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort],
> > [0], [dnl
> > table=5 (lr_in_unsnat ), priority=110 , match=(inport ==
> > "lr0-sw1" && ip6.dst == bef0::1), action=(ct_snat;)
> > ])
> >
> > -AT_CHECK([grep "lr_in_dnat" lr0flows | grep force_snat_for_lb | sort],
> > [0], [dnl
> > +AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> > + table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
> > table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip &&
> > ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80),
> > action=(flags.force_snat_for_lb = 1; ct_dnat;)
> > table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip &&
> > ip4.dst == 10.0.0.10 && tcp && tcp.dst == 80),
> > action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.4:8080);)
> > + table=6 (lr_in_dnat ), priority=50 , match=(ip),
> > action=(flags.loopback = 1; ct_dnat;)
> > ])
> >
> > -AT_CHECK([grep "lr_out_snat" lr0flows | grep force_snat_for_lb | sort],
> > [0], [dnl
> > +AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl
> > + table=1 (lr_out_snat ), priority=0 , match=(1), action=(next;)
> > table=1 (lr_out_snat ), priority=110 ,
> > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"),
> > action=(ct_snat(172.168.0.100);)
> > table=1 (lr_out_snat ), priority=110 ,
> > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"),
> > action=(ct_snat(10.0.0.1);)
> > table=1 (lr_out_snat ), priority=110 ,
> > match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw1"),
> > action=(ct_snat(20.0.0.1);)
> > table=1 (lr_out_snat ), priority=110 ,
> > match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-sw1"),
> > action=(ct_snat(bef0::1);)
> > + table=1 (lr_out_snat ), priority=120 , match=(nd_ns),
> > action=(next;)
> > ])
> >
> > AT_CLEANUP
> > --
> > 2.27.0
> >
>
> _______________________________________________
> 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
