[ovs-dev] [PATCH v4 1/2] ovn: Gratuitous ARP for centralized NAT rules on a distributed router

Guru Shetty guru at ovn.org
Wed Mar 22 16:03:57 UTC 2017


>
>
>
>>
>>> This is achieved by extending
>>> the syntax for "options:nat-addresses" in the southbound database,
>>> allowing the condition 'is_chassis_resident("LPORT_NAME")' to be
>>> appended
>>> after the MAC and IP addresses.  This condition is automatically inserted
>>> by ovn-northd when the northbound "options:nat-addresses" is set to
>>> "router" and the peer is a distributed gateway port.
>>>
>>> A separate patch will be required to support gratuitous ARP for
>>> distributed NAT rules that specify logical_port and external_mac.  Since
>>> the MAC address differs and the logical port often resides on a different
>>> chassis from the redirect-chassis, these addresses cannot be included in
>>> the same "nat-addresses" string as for centralized NAT rules.
>>>
>>> Signed-off-by: Mickey Spiegel <mickeys.dev at gmail.com>
>>> ---
>>>  ovn/controller/pinctrl.c | 104 ++++++++++++++++++++++++++++++
>>> ++++++++++++++---
>>>  ovn/lib/ovn-util.c       |  38 ++++++++++++++---
>>>  ovn/lib/ovn-util.h       |   2 +
>>>  ovn/northd/ovn-northd.c  |  52 +++++++++++++++++-------
>>>  ovn/ovn-nb.xml           |  33 ++++++++++++---
>>>  ovn/ovn-sb.xml           |  31 ++++++++++----
>>>  tests/ovn.at             |  70 +++++++++++++++++++++++++++++++
>>>  7 files changed, 289 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
>>> index b342189..08af792 100644
>>> --- a/ovn/controller/pinctrl.c
>>> +++ b/ovn/controller/pinctrl.c
>>> @@ -37,6 +37,7 @@
>>>  #include "lib/dhcp.h"
>>>  #include "ovn-controller.h"
>>>  #include "ovn/actions.h"
>>> +#include "ovn/lex.h"
>>>  #include "ovn/lib/logical-fields.h"
>>>  #include "ovn/lib/ovn-dhcp.h"
>>>  #include "ovn/lib/ovn-util.h"
>>> @@ -1049,7 +1050,8 @@ send_garp_update(const struct sbrec_port_binding
>>> *binding_rec,
>>>
>>>      volatile struct garp_data *garp = NULL;
>>>      /* Update GARP for NAT IP if it exists. */
>>> -    if (!strcmp(binding_rec->type, "l3gateway")) {
>>> +    if (!strcmp(binding_rec->type, "l3gateway")
>>> +        || !strcmp(binding_rec->type, "patch")) {
>>>
>> A comment above on why we should also look at "patch" will be useful.
>>
>
> Replace the comment above with something along the following lines?
> /* Update GARP for NAT IP if it exists. Consider port bindings with type
>  * "l3gateway" for logical switch ports attached to gateway routers, and
>  * port bindings with type "patch" for logical switch ports attached to
>  * distributed gateway ports. */
>

Looks good.

>
>
>>
>>>          struct lport_addresses *laddrs = NULL;
>>>          laddrs = shash_find_data(nat_addresses,
>>> binding_rec->logical_port);
>>>          if (!laddrs) {
>>> @@ -1202,24 +1204,101 @@ get_localnet_vifs_l3gwports(const struct
>>> ovsrec_bridge *br_int,
>>>
>>>      const struct local_datapath *ld;
>>>      HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
>>> -        if (!ld->has_local_l3gateway) {
>>> +        if (!ld->localnet_port) {
>>>              continue;
>>>          }
>>>
>>>          for (size_t i = 0; i < ld->ldatapath->n_lports; i++) {
>>>              const struct sbrec_port_binding *pb =
>>> ld->ldatapath->lports[i];
>>> -            if (!strcmp(pb->type, "l3gateway")
>>> -                /* && it's on this chassis */) {
>>> +            if ((ld->has_local_l3gateway && !strcmp(pb->type,
>>> "l3gateway"))
>>> +                || !strcmp(pb->type, "patch")) {
>>>
>> A comment above on why we are considering "patch" will be useful.
>>
>
> Something along the lines?
> /* Consider port bindings of type "l3gateway" that connect to gateway
> routers,
>  * and port bindings of type "patch" since they might connect to
> distributed
>  * gateway ports with NAT addresses. */
>
Looks good


>
>
>> Does local_datapaths include every patch port or is it only those that
>> have l2 distributed gateway port instantiated on it?
>>
>
> Local datapaths is constructed in ovn/controller/binding.c. Start with
> every local port (VIF, l3gateway, l2gateway, chassisredirect) and then find
> all connected datapaths by walking patch ports.
>
Okay.


> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
>>> index 46a25f6..1f0b003 100644
>>> --- a/ovn/ovn-nb.xml
>>> +++ b/ovn/ovn-nb.xml
>>> @@ -244,8 +244,7 @@
>>>          <column name="options" key="nat-addresses">
>>>            <p>
>>>              This is used to send gratuitous ARPs for SNAT and DNAT IP
>>> -            addresses via <code>localnet</code> and is valid for only L3
>>> -            gateway ports.
>>> +            addresses via <code>localnet</code>.
>>>
>>
>> localnet switch port is usually a separate logical_switch_port that is
>> attached to the same switch.  And this logical siwtch port is usually the
>> one that is connected to the router, right? I think if we clarify it above,
>> it will cause less confusion.
>>
>
> This is used to send gratuitous ARPs for SNAT and DNAT IP
> addresses via the <code>localnet</code> port that is attached
> to the same logical switch. This option is specified on a logical
> switch port that is connected to a gateway router, or a logical
> switch port that is connected to a distributed gateway port on
> a logical router.
>
Sounds good.


>
>
>>
>>
>>>            </p>
>>>
>>>            <p>
>>> @@ -264,6 +263,13 @@
>>>                </p>
>>>
>>>                <p>
>>> +                This form of <ref column="options"
>>> key="nat-addresses"/> is
>>> +                valid for L3 gateway ports, and for logical switch ports
>>> +                where <ref column="options" key="router-port"/> is the
>>> name
>>> +                of a distributed gateway port.
>>> +              </p>
>>> +
>>>
>> The above is a little confusing. I think if it read as below, it will be
>> more clear?
>>
>> This form of options:nat-addresses is valid for for  logical  switch
>>  ports  where
>> options:router-port is the port of a gateway router or the name of a
>> distributed gateway port.
>>
>
> That is better.
>
>
>>
>>
>>> +              <p>
>>>                  Supported only in OVN 2.8 and later.  Earlier versions
>>> required
>>>                  NAT addresses to be manually synchronized.
>>>                </p>
>>> @@ -271,10 +277,17 @@
>>>
>>>              <dt><code>Ethernet address followed by one or more IPv4
>>> addresses</code></dt>
>>>              <dd>
>>> -              Example: <code>80:fa:5b:06:72:b7 158.36.44.22
>>> -              158.36.44.24</code>. This would result in generation of
>>> -              gratuitous ARPs for IP addresses 158.36.44.22 and
>>> 158.36.44.24
>>> -              with a MAC address of 80:fa:5b:06:72:b7.
>>> +              <p>
>>> +                Example: <code>80:fa:5b:06:72:b7 158.36.44.22
>>> +                158.36.44.24</code>. This would result in generation of
>>> +                gratuitous ARPs for IP addresses 158.36.44.22 and
>>> 158.36.44.24
>>> +                with a MAC address of 80:fa:5b:06:72:b7.
>>> +              </p>
>>> +
>>> +              <p>
>>> +                This form of <ref column="options"
>>> key="nat-addresses"/> is
>>> +                only valid for L3 gateway ports.
>>>
>>
>> s/is only valid for L3 gateway ports/is only valid for a port of a
>> gateway router/  is more clear?
>>
>
> No opinion, so I can go with this.
>
>
>>
>>> +              </p>
>>>              </dd>
>>>            </dl>
>>>          </column>
>>> @@ -1166,6 +1179,14 @@
>>>            peer logical switch's destination lookup flow on the
>>>            <code>redirect-chassis</code>.
>>>          </p>
>>> +
>>> +        <p>
>>> +          When this option is specified and it is desired to generate
>>> +          gratuitous ARPs for NAT addresses, then the peer logical
>>> switch
>>> +          port's <ref column="options" key="nat-addresses"
>>> +          table="Logical_Switch_Port"/> should be set to
>>> +          <code>router</code>.
>>> +        </p>
>>>        </column>
>>>      </group>
>>>
>>> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
>>> index 4e95c80..2df45e8 100644
>>> --- a/ovn/ovn-sb.xml
>>> +++ b/ovn/ovn-sb.xml
>>> @@ -1862,6 +1862,20 @@ tcp.flags = RST;
>>>          ports must have reversed <ref column="logical_port"/> and
>>>          <code>peer</code> values.
>>>        </column>
>>> +
>>> +      <column name="options" key="nat-addresses">
>>> +        MAC address of the <code>patch</code> port followed by a list of
>>> +        SNAT and DNAT external IP addresses, followed by
>>> +        <code>is_chassis_resident("<var>lport</var>")</code>, where
>>> +        <var>lport</var> is the name of a logical port on the same
>>> chassis
>>> +        where the corresponding NAT rules are applied.  This is used to
>>> +        send gratuitous ARPs for SNAT and DNAT external IP addresses via
>>> +        <code>localnet</code>, from the chassis where <var>lport</var>
>>> +        resides.  Example: <code>80:fa:5b:06:72:b7 158.36.44.22
>>> +        158.36.44.24</code>.  This would result in generation of
>>> +        gratuitous ARPs for IP addresses 158.36.44.22 and 158.36.44.24
>>> +        with a MAC address of 80:fa:5b:06:72:b7.
>>>
>>
>> The example above does not include "is_chassis_resident". Can you add
>> that?
>>
>
> Yes, I can add that.
>
>
>>
>>
>>> +      </column>
>>>      </group>
>>>
>>>      <group title="L3 Gateway Options">
>>> @@ -1883,15 +1897,14 @@ tcp.flags = RST;
>>>          The <code>chassis</code> in which the port resides.
>>>        </column>
>>>
>>> -        <column name="options" key="nat-addresses">
>>> -          MAC address of the <code>l3gateway</code> port followed by a
>>> list of
>>> -          SNAT and DNAT IP addresses. This is used to send gratuitous
>>> ARPs for
>>> -          SNAT and DNAT IP addresses via <code>localnet</code> and is
>>> valid for
>>> -          only L3 gateway ports.  Example: <code>80:fa:5b:06:72:b7
>>> 158.36.44.22
>>> -          158.36.44.24</code>. This would result in generation of
>>> gratuitous
>>> -          ARPs for IP addresses 158.36.44.22 and 158.36.44.24 with a MAC
>>> -          address of 80:fa:5b:06:72:b7.
>>> -        </column>
>>> +      <column name="options" key="nat-addresses">
>>> +        MAC address of the <code>l3gateway</code> port followed by a
>>> list of
>>> +        SNAT and DNAT external IP addresses.  This is used to send
>>> gratuitous
>>> +        ARPs for SNAT and DNAT external IP addresses via
>>> <code>localnet</code>.
>>> +        Example: <code>80:fa:5b:06:72:b7 158.36.44.22
>>> 158.36.44.24</code>.
>>> +        This would result in generation of gratuitous ARPs for IP
>>> addresses
>>> +        158.36.44.22 and 158.36.44.24 with a MAC address of
>>> 80:fa:5b:06:72:b7.
>>> +      </column>
>>>      </group>
>>>
>>>      <group title="Localnet Options">
>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>> index bbbec90..0915f7a 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -6660,3 +6660,73 @@ OVN_CHECK_PACKETS([hv2/vif1-tx.pcap],
>>> [hv2-vif1.expected])
>>>  OVN_CLEANUP([hv1],[hv2],[hv3])
>>>
>>>  AT_CLEANUP
>>> +
>>> +AT_SETUP([ovn -- send gratuitous arp for NAT rules on distributed
>>> router])
>>> +AT_SKIP_IF([test $HAVE_PYTHON = no])
>>> +ovn_start
>>> +# Create logical switch
>>> +ovn-nbctl ls-add ls0
>>> +# Create gateway router
>>> +ovn-nbctl create Logical_Router name=lr0
>>> +# Add router port to gateway router
>>> +ovn-nbctl lrp-add lr0 lrp0 f0:00:00:00:00:01 192.168.0.1/24 \
>>> +    -- set Logical_Router_Port lrp0 options:redirect-chassis="hv2"
>>> +ovn-nbctl lsp-add ls0 lrp0-rp -- set Logical_Switch_Port lrp0-rp \
>>> +    type=router options:router-port=lrp0-rp addresses="router"
>>>
>>
>> A fix above for 'options:router-port' to point to the router port.
>>
>
> Should I create the patch to fix the other two places with this
> problem, or will you do that?
>
Please go ahead and do it.


More information about the dev mailing list