[ovs-dev] [PATCH v4] ovn-northd: Force SNAT for multiple gateway routers.

Guru Shetty guru at ovn.org
Tue Nov 29 17:12:19 UTC 2016


On 28 November 2016 at 19:33, Mickey Spiegel <mickeys.dev at gmail.com> wrote:

> Acked-by: Mickey Spiegel <mickeys.dev at gmail.com>
>

Thank you for the thorough review and ideas.  It looks a lot better now
than when I started.


>
> A few comments and nits below.
>
>
>>
>>
> I never noticed before, but I am unsure why the "continue" in line 3693
> (without changes), line 3768 (with changes), should be there. In order to
> generate ARP replies for SNAT external IPs, this seems to assume that
> all external IPs for SNAT are router interface IPs, and that all ARP
> requests for those IPs will arrive over the corresponding router interface.
> The latter condition is true for gateway routers, given that they connect
> to the distributed router over a single join network.
>

> If the continue were removed, then both assumptions would go away.
>
> What I am pointing out is old code, it just gets emphasized more with
> this new functionality. Explicit SNAT rules point outward and so uses
> an IP address on a router interface pointing outward. With the new
> force_snat functionality, due to the "continue" in line 3693, you would
> have to use an IP address on the gateway router's interface to the join
> network, which is exactly what you have done in the tests below.
>

Right. The general assumption so far (which I think is kind of fair) has
been that you only need additional ARP entries for floating IP addresses
which may not be a IP address of your gateway router. The gateway router's
configured IP addresses already get their own ARP entries automatically.
And a pure SNAT IP address is always one of the gateway IP addresses (when
I say 'pure', I mean not the 'dnat_and_snat' case).

If you remove the "continue", I think there will be duplicate flows
(ovn-controller will complain about it) and that will need to be handled
carefully with additional logic.


>
> I can revisit this in the distributed router patch set if you feel that it
> is
> not relevant to NAT on a gateway router.
>

Sure.


>
>
>> @@ -3845,6 +3920,12 @@ build_lrouter_flows(struct hmap *datapaths, struct
>> hmap *ports,
>>              continue;
>>          }
>>
>> +        ovs_be32 snat_ip;
>> +        const char *dnat_force_snat_ip = get_force_snat_ip(od, "dnat",
>> +                                                           &snat_ip);
>> +        const char *lb_force_snat_ip = get_force_snat_ip(od, "lb",
>> +                                                         &snat_ip);
>> +
>>          /* A set to hold all ips that need defragmentation and tracking.
>> */
>>          struct sset all_ips = SSET_INITIALIZER(&all_ips);
>>
>> @@ -3867,14 +3948,16 @@ build_lrouter_flows(struct hmap *datapaths,
>> struct hmap *ports,
>>                      sset_add(&all_ips, ip_address);
>>                  }
>>
>> -                /* Higher priority rules are added in DNAT table to
>> match on
>> -                 * ct.new which in-turn have group id as an action for
>> load
>> -                 * balancing. */
>> +                /* Higher priority rules are added for load-balancing in
>> DNAT
>> +                 * table.  For every match (on a VIP[:port]), we add two
>> flows
>> +                 * via add_router_lb_flow().  One flow is for specific
>> matching
>> +                 * on ct.new with an action of "ct_lb($targets);".  The
>> other
>> +                 * flow is for ct.est with an action of "ct_dnat;". */
>>                  ds_clear(&actions);
>>                  ds_put_format(&actions, "ct_lb(%s);", node->value);
>>
>>                  ds_clear(&match);
>> -                ds_put_format(&match, "ct.new && ip && ip4.dst == %s",
>> +                ds_put_format(&match, "ip && ip4.dst == %s",
>>                                ip_address);
>>                  free(ip_address);
>>
>> @@ -3886,11 +3969,11 @@ build_lrouter_flows(struct hmap *datapaths,
>> struct hmap *ports,
>>                          ds_put_format(&match, " && tcp && tcp.dst == %d",
>>                                        port);
>>                      }
>> -                    ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT,
>> -                                  120, ds_cstr(&match),
>> ds_cstr(&actions));
>> +                    add_router_lb_flow(lflows, od, &match, &actions, 120,
>> +                                       lb_force_snat_ip);
>>                  } else {
>> -                    ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT,
>> -                                  110, ds_cstr(&match),
>> ds_cstr(&actions));
>> +                    add_router_lb_flow(lflows, od, &match, &actions, 110,
>> +                                       lb_force_snat_ip);
>>                  }
>>              }
>>          }
>> @@ -3981,7 +4064,13 @@ build_lrouter_flows(struct hmap *datapaths, struct
>> hmap *ports,
>>                  ds_clear(&match);
>>                  ds_put_format(&match, "ip && ip4.dst == %s",
>> nat->external_ip);
>>                  ds_clear(&actions);
>> -                ds_put_format(&actions,"flags.loopback = 1;
>> ct_dnat(%s);",
>> +                if (dnat_force_snat_ip) {
>> +                    /* Indicate to the future tables that a DNAT has
>> taken
>> +                     * place and a force SNAT needs to be done in the
>> Egress
>> +                     * SNAT table. */
>> +                    ds_put_format(&actions, "flags.force_snat_for_dnat =
>> 1; ");
>> +                }
>> +                ds_put_format(&actions, "flags.loopback = 1;
>> ct_dnat(%s);",
>>                                nat->logical_ip);
>>                  ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 100,
>>                                ds_cstr(&match), ds_cstr(&actions));
>> @@ -4006,8 +4095,47 @@ build_lrouter_flows(struct hmap *datapaths, struct
>> hmap *ports,
>>              }
>>          }
>>
>> +        /* Handle force SNAT options set in the gateway router. */
>> +        if (dnat_force_snat_ip) {
>> +            /* If a packet with destination IP address as that of the
>> +             * gateway router (as set in options:dnat_force_snat_ip) is
>> seen,
>> +             * UNSNAT it. */
>> +            ds_clear(&match);
>> +            ds_put_format(&match, "ip && ip4.dst == %s",
>> dnat_force_snat_ip);
>> +            ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 110,
>> +                          ds_cstr(&match), "ct_snat; next;");
>>
>
> I am unable to come up with a rationale for the different priority values
> in the
> S_ROUTER_IN_UNSNAT stage. Explicit SNAT rules use priority 100,
> dnat_force_snat uses priority 110, and lb_force_snat uses priority 100.
> It seems like there can be cases where the same IP address is specified for
> dnat_force_snat and lb_force_snat. With NAT on a distributed router, it
> would
> be possible for all 3 to use the same IP address. The actions are the same,
> so it will all work regardless. It just seems like it would be more
> consistent if
> all three cases use the same priority value, or all 3 cases use different
> priority
> values.
>

When they have same priority, ovn-controller will complain about duplicate
flows. I will change it to all have different priorities to be consistent
here and also change ovn-northd.8.xml to reflect correct behavior.


>
> Also note that the ovn-northd.8.xml changes describe all of these flows as
> priority 100 flows.
>
> +
>> +            /* Higher priority rules to force SNAT with the IP addresses
>> +             * configured in the Gateway router.  This only takes effect
>> +             * when the packet has already been DNATed once. */
>> +            ds_clear(&match);
>> +            ds_put_format(&match, "flags.force_snat_for_dnat == 1 &&
>> ip");
>> +            ds_clear(&actions);
>> +            ds_put_format(&actions, "ct_snat(%s);", dnat_force_snat_ip);
>> +            ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 110,
>> +                          ds_cstr(&match), ds_cstr(&actions));
>
> +        }
>> +        if (lb_force_snat_ip) {
>> +            /* If a packet with destination IP address as that of the
>> +             * gateway router (as set in options:lb_force_snat_ip) is
>> seen,
>> +             * UNSNAT it. */
>> +            ds_clear(&match);
>> +            ds_put_format(&match, "ip && ip4.dst == %s",
>> lb_force_snat_ip);
>> +            ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 100,
>> +                          ds_cstr(&match), "ct_snat; next;");
>> +
>> +            /* Load balanced traffic will have flags.force_snat_for_lb
>> set.
>> +             * Force SNAT it. */
>> +            ds_clear(&match);
>> +            ds_put_format(&match, "flags.force_snat_for_lb == 1 && ip");
>> +            ds_clear(&actions);
>> +            ds_put_format(&actions, "ct_snat(%s);", lb_force_snat_ip);
>> +            ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 110,
>> +                          ds_cstr(&match), ds_cstr(&actions));
>>
>
> The lb_force_snat priority value of 110 is not consistent with the
> ovn-northd.8.xml changes, which describe the use of priority 110 for
> dnat_force_snat and 100 for lb_force_snat.
>
> I could not figure out why priority 110 is used for the force_snat
> S_ROUTER_OUT_SNAT flows rather than 100, since I did not see
> any priority 100 flows.
>

That was sloppiness from my part from changes over the iterations. I will
change both to 100 and apply this patch with the following total
incremental.


diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 11245c6..0e84f2f 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -1145,19 +1145,25 @@ icmp4 {
     <ul>
       <li>
         <p>
-          For each configuration in the OVN Northbound database, that asks
-          to change the source IP address of a packet from <var>A</var> to
-          <var>B</var>, a priority-100 flow matches <code>ip &amp;&amp;
-          ip4.dst == <var>B</var></code> with an action
-          <code>ct_snat; next;</code>.
+          If the Gateway router has been configured to force SNAT any
+          previously DNATted packets to <var>B</var>, a priority-110 flow
+          matches <code>ip &amp;&amp; ip4.dst == <var>B</var></code> with
+          an action <code>ct_snat; next;</code>.
         </p>

         <p>
-          If the Gateway router has been configured to force SNAT (any
-          previously DNATted or Load-balanced packets) to <var>B</var>,
-          a priority-100 flow matches <code>ip &amp;&amp;
-          ip4.dst == <var>B</var></code> with an action <code>ct_snat;
-          next;</code>.
+          If the Gateway router has been configured to force SNAT any
+          previously load-balanced packets to <var>B</var>, a priority-100
flow
+          matches <code>ip &amp;&amp; ip4.dst == <var>B</var></code> with
+          an action <code>ct_snat; next;</code>.
+        </p>
+
+        <p>
+          For each NAT configuration in the OVN Northbound database, that
asks
+          to change the source IP address of a packet from <var>A</var> to
+          <var>B</var>, a priority-90 flow matches <code>ip &amp;&amp;
+          ip4.dst == <var>B</var></code> with an action
+          <code>ct_snat; next;</code>.
         </p>

         <p>
@@ -1477,7 +1483,7 @@ arp {
         <p>
           If the Gateway router in the OVN Northbound database has been
           configured to force SNAT a packet (that has been previously
DNATted)
-          to <var>B</var>, a priority-110 flow matches
+          to <var>B</var>, a priority-100 flow matches
           <code>flags.force_snat_for_dnat == 1 &amp;&amp; ip</code> with an
           action <code>ct_snat(<var>B</var>);</code>.
         </p>
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 5afcbbc..3eb1023 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -4049,7 +4049,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
                 || !strcmp(nat->type, "dnat_and_snat")) {
                 ds_clear(&match);
                 ds_put_format(&match, "ip && ip4.dst == %s",
nat->external_ip);
-                ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 100,
+                ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 90,
                               ds_cstr(&match), "ct_snat; next;");
             }

@@ -4112,7 +4112,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
             ds_put_format(&match, "flags.force_snat_for_dnat == 1 && ip");
             ds_clear(&actions);
             ds_put_format(&actions, "ct_snat(%s);", dnat_force_snat_ip);
-            ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 110,
+            ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 100,
                           ds_cstr(&match), ds_cstr(&actions));
         }
         if (lb_force_snat_ip) {
@@ -4130,7 +4130,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
             ds_put_format(&match, "flags.force_snat_for_lb == 1 && ip");
             ds_clear(&actions);
             ds_put_format(&actions, "ct_snat(%s);", lb_force_snat_ip);
-            ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 110,
+            ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 100,
                           ds_cstr(&match), ds_cstr(&actions));
         }


More information about the dev mailing list