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