[ovs-dev] [PATCH ovn 2/2] northd: Fix the missing force_snat_for_lb flows when router_ip is configured.

[Ben Pfaff]

On Fri, Mar 05, 2021 at 06:46:55AM +0530, Numan Siddique wrote:
> On Fri, Mar 5, 2021 at 2:30 AM Ben Pfaff <blp at ovn.org> wrote:
> >
> > On Wed, Mar 03, 2021 at 11:32:39PM +0530, numans at ovn.org wrote:
> > > From: Numan Siddique <numans at ovn.org>
> > >
> > > The commit c6e21a23bd8 which supported the option 'lb_force_snat_ip=router_ip'
> > > on a gateway router, missed out on
> > >   - updating the flows in 'lr_in_dnat' to set 'flags.force_snat_for_lb = 1'.
> > >   - removing the flow to drop if ip.dst == router port IP in 'lr_in_ip_input'
> > >     stage.
> > >
> > > This patch fixes these issue and adds a system test to cover the
> > > hairpin load balancer traffic for the gateway routers.
> > >
> > > Fixes: c6e21a23bd8("northd: Provide the Gateway router option 'lb_force_snat_ip' to take router port ips.")
> > >
> > > CC: Ben Pfaff <blp at ovn.org>
> > > Signed-off-by: Numan Siddique <numans at ovn.org>
> >
> > Thanks!  I took a look at the differences between the C and the DDlog
> > versions.  I have a few comments.
> >
> > The C version adds a new flow to the output.  The DDlog version doesn't
> > appear to, yet I do see a similar flow in the DDlog code.  I am not sure
> > how that happened.
> 
> Thanks for the review.
> 
> Can you please tell me in which table the new flow is seen?
> 
> If a logical router has the option 'lb_force_nat_ip=router_ip' is set, then both
> the C version and ddlog version do the below
> 
>   (1) Update the flow in the table lr_in_dnat to add
> "flags.force_snat_for_lb = 1" in the action
>     for the packets which are destined to load balancer vips.
> 
>   (2) Don't add the priority-60 flow in 'lr_in_ip_input' to drop the
> packets for router ips.
>     The below suggestion provided by you to replace "not
> force_lb_snat" with  ".force_lb_snat = false"
>     does that.

If they do the same thing, then that's great and no change is needed.
What surprises me is that the C version of the changes has the following
hunk to add a new flow, but there's no corresponding change to the DDlog
version to add a new flow.

@@ -9153,10 +9153,17 @@ build_lrouter_force_snat_flows_op(struct ovn_port *op,
 
     if (op->lrp_networks.n_ipv4_addrs) {
         ds_clear(match);
         ds_clear(actions);
 
+        ds_put_format(match, "inport == %s && ip4.dst == %s",
+                      op->json_key, op->lrp_networks.ipv4_addrs[0].addr_s);
+        ovn_lflow_add(lflows, op->od, S_ROUTER_IN_UNSNAT, 110,
+                      ds_cstr(match), "ct_snat;");
+
+        ds_clear(match);
+
         /* Higher priority rules to force SNAT with the router port ip.
          * This only takes effect when the packet has already been
          * load balanced once. */
         ds_put_format(match, "flags.force_snat_for_lb == 1 && ip4 && "
                       "outport == %s", op->json_key);

> > You can optimize it by writing
> > > +                                  .force_lb_snat = false,
> > and then omitting the later clause.  With the way DDlog currently
> > works, this isn't just cosmetic, it will actually yield faster
> > code because DDlog will discard nonmatching records earlier.  (If
> > that's an actual performance problem it will come out later in
> > profiling, I'm just giving a tip here that I only recently learned
> > myself.)
> 
> I tried like below as per your suggestion and the compilation failed

Oh, I meant like the following incremental.  Sorry I wasn't clear!

diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 0e208c1f5160..21092cc1bcea 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -5142,12 +5142,11 @@ Flow(.logical_datapath = lr_uuid,
      .external_ids = stage_hint(lrp_uuid)) :-
     &RouterPort(.lrp = nb::Logical_Router_Port{._uuid = lrp_uuid},
                 .router = &Router{.snat_ips = snat_ips,
-                                  .force_lb_snat = force_lb_snat,
+                                  .force_lb_snat = false,
                                   .lr = nb::Logical_Router{._uuid = lr_uuid}},
                 .networks = networks),
     var addr = FlatMap(networks.ipv4_addrs),
     not snat_ips.contains_key(IPv4{addr.addr}),
-    not force_lb_snat,
     var match_ips = "${addr.addr}".group_by((lr_uuid, lrp_uuid)).to_vec().
 Flow(.logical_datapath = lr_uuid,
      .stage = router_stage(IN, IP_INPUT),
@@ -5157,12 +5156,11 @@ Flow(.logical_datapath = lr_uuid,
      .external_ids = stage_hint(lrp_uuid)) :-
     &RouterPort(.lrp = nb::Logical_Router_Port{._uuid = lrp_uuid},
                 .router = &Router{.snat_ips = snat_ips,
-                                  .force_lb_snat = force_lb_snat,
+                                  .force_lb_snat = false,
                                   .lr = nb::Logical_Router{._uuid = lr_uuid}},
                 .networks = networks),
     var addr = FlatMap(networks.ipv6_addrs),
     not snat_ips.contains_key(IPv6{addr.addr}),
-    not force_lb_snat,
     var match_ips = "${addr.addr}".group_by((lr_uuid, lrp_uuid)).to_vec().
 
 for (RouterPortNetworksIPv4Addr(