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

Guru Shetty guru at ovn.org
Fri Nov 18 16:22:53 UTC 2016


On 16 November 2016 at 17:05, Mickey Spiegel <mickeys.dev at gmail.com> wrote:

>
> On Sun, Nov 13, 2016 at 11:15 PM, Gurucharan Shetty <guru at ovn.org> wrote:
>
>> When multiple gateway routers exist, a packet can
>> enter any gateway router. Once the packet reaches its
>> destination, its reverse direction should be via the
>> same gateway router.  This is achieved by doing a SNAT
>> of the packet in the inward direction (towards logical space)
>> with a IP address of the gateway router such that packet travels back
>> to the same gateway router.
>>
>> To do the above, we introduce two new options in the logical router.
>>
>> options:dnat_force_snat_ip=$IP will force SNAT any packet to $IP if
>> it has been previously DNATted.
>>
>> options:lb_force_snat_ip=$IP will force SNAT any packet to $IP if
>> it has been previously load-balanced.
>>
>
> Overall it looks very good. Three comments inline.
>
> Signed-off-by: Gurucharan Shetty <guru at ovn.org>
>> ---
>> v2->v3:
>> Adds a unit test for load-balancing and multiple gateway routers.
>> ---
>>  ovn/lib/logical-fields.c    |   4 +
>>  ovn/lib/logical-fields.h    |   5 +
>>  ovn/northd/ovn-northd.8.xml |  28 +++-
>>  ovn/northd/ovn-northd.c     |  98 ++++++++++-
>>  ovn/ovn-nb.xml              |  25 +++
>>  tests/system-ovn.at         | 386 ++++++++++++++++++++++++++++++
>> ++++++++++++++
>>  6 files changed, 543 insertions(+), 3 deletions(-)
>>
>> diff --git a/ovn/lib/logical-fields.c b/ovn/lib/logical-fields.c
>> index d4578c3..ef5188a 100644
>> --- a/ovn/lib/logical-fields.c
>> +++ b/ovn/lib/logical-fields.c
>> @@ -88,6 +88,10 @@ ovn_init_symtab(struct shash *symtab)
>>      char flags_str[16];
>>      snprintf(flags_str, sizeof flags_str, "flags[%d]",
>> MLF_ALLOW_LOOPBACK_BIT);
>>      expr_symtab_add_subfield(symtab, "flags.loopback", NULL, flags_str);
>> +    snprintf(flags_str, sizeof flags_str, "flags[%d]",
>> +             MLF_FORCE_SNAT_FOR_DNAT_BIT);
>> +    expr_symtab_add_subfield(symtab, "flags.force_snat_for_dnat", NULL,
>> +                             flags_str);
>>
>>      /* Connection tracking state. */
>>      expr_symtab_add_field(symtab, "ct_mark", MFF_CT_MARK, NULL, false);
>> diff --git a/ovn/lib/logical-fields.h b/ovn/lib/logical-fields.h
>> index a1f1da6..1681fff 100644
>> --- a/ovn/lib/logical-fields.h
>> +++ b/ovn/lib/logical-fields.h
>> @@ -47,6 +47,7 @@ void ovn_init_symtab(struct shash *symtab);
>>  enum mff_log_flags_bits {
>>      MLF_ALLOW_LOOPBACK_BIT = 0,
>>      MLF_RCV_FROM_VXLAN_BIT = 1,
>> +    MLF_FORCE_SNAT_FOR_DNAT_BIT = 2,
>>
>
> You chose to define an explicit bit for the DNAT case, but not for the LB
> case.
> If there is only going to be a bit defined for one of the cases, I prefer
> that the
> explicit bit be for the LB case. The LB bit will be required for the
> implementation of centralized load balancing on an otherwise distributed
> router.
>

I see. I was hoping to not use the LB bit as currently all established
connections of LB would simply be sent through DNAT table for NATting. But
based on the corner case you pointed out below, I decided to use 2 bits and
also add specific matching for LB's established traffic.

---snip.....


@@ -4006,6 +4061,45 @@ 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, 100,
>> +                          ds_cstr(&match), "ct_snat; next;");
>>
>
> Since the rest of the gateway router NAT rule code is organized by table,
> IMO it would be better to put the code above in the Ingress UNSNAT table
> section.
>

The rest of the code is inside a for loop as it is per NAT entry. Since
this rule is per-datapath, it cannot really stay at the same place.  So I
kept it separate.

I will send a v4.


More information about the dev mailing list