| Message ID | 20210401092539.1009-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: Restore flows that recirculate packets in the router DNAT zone. | expand |
> > 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 <lorenzo.bianconi@redhat.com> > Reported-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > 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 <lorenzo.bianconi@redhat.com> > > 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 >
On Thu, Apr 1, 2021 at 3:22 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> 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 <lorenzo.bianconi@redhat.com> > > Reported-by: Ilya Maximets <i.maximets@ovn.org> > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > 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 <lorenzo.bianconi@redhat.com> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 4/1/21 12:23 PM, Numan Siddique wrote: > On Thu, Apr 1, 2021 at 3:22 PM Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> 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 <lorenzo.bianconi@redhat.com> >>> Reported-by: Ilya Maximets <i.maximets@ovn.org> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> --- >>> 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 <lorenzo.bianconi@redhat.com> > > Thanks Ilya and Dumitru for reporting and fixing this. > > I applied this patch to master. > > Numan > Thanks!
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
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 <lorenzo.bianconi@redhat.com> Reported-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- 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(-)