[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 &&
- 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 && 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 &&
- 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 && 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 &&
+ 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 && 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