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

Message ID CAM_3v9J7gFPUPC4AvZkepH1+h0s0bS4WvJtqWQbrpk+TpW+5Sw@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Gurucharan Shetty Nov. 29, 2016, 5:12 p.m. UTC
On 28 November 2016 at 19:33, Mickey Spiegel <mickeys.dev@gmail.com> wrote:

> Acked-by: Mickey Spiegel <mickeys.dev@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.


             }

@@ -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));
         }

Comments

Mickey Spiegel Nov. 29, 2016, 6:38 p.m. UTC | #1
On Tue, Nov 29, 2016 at 9:12 AM, Guru Shetty <guru@ovn.org> wrote:

>
>
> On 28 November 2016 at 19:33, Mickey Spiegel <mickeys.dev@gmail.com>
> wrote:
>
>> Acked-by: Mickey Spiegel <mickeys.dev@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.
>

I will look at this again as part of distributed NAT. If I can assume that
there are no
duplicate IP addresses specified on a router interface, it is relatively
straightforward
to add code that avoids duplicate flows. However, I don't see any such
check, so I
probably cannot make that assumption.

<snip>


>> 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.
>>
>
<snip>


>
>> 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.
>
>
The incremental below looks good to me.

Mickey


>
> 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));
>          }
>
>
>
>

Patch
diff mbox

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;");