[ovs-dev] [External] : [PATCH ovn v2] ovn-northd.c: Add proxy ARP support to OVN

Brendan Doyle brendan.doyle at oracle.com
Tue Jun 15 12:34:00 UTC 2021



On 15/06/2021 13:26, Numan Siddique wrote:
> On Tue, Jun 15, 2021 at 7:50 AM Brendan Doyle <brendan.doyle at oracle.com> wrote:
>>
>>
>> On 14/06/2021 21:17, Numan Siddique wrote:
>>> On Mon, Jun 14, 2021 at 5:19 AM Brendan Doyle <brendan.doyle at oracle.com> wrote:
>>>> Hi,
>>>>
>>>> Just wondering how to move this along. It's been in the queue a while now.
>>>> The product I'm working on requires this feature and If I can't get it
>>>> upstream
>>>> I'll need to look at other options.
>>> Hi Brendan,
>>>
>>> Sorry for the late reply.  For some reason the patch doesn't apply to me.
>> Sorry I'm not sure what you mean, the patch did not apply, it works when
>> I apply
>> to my repo?
>>> Please see below for a few comments.
>>>
>>>
>>>> Thanks
>>>>
>>>> Brendan
>>>>
>>>> On 04/06/2021 15:51, Brendan Doyle wrote:
>>>>>   From 07ecd5d00f82658e094132102575d8d576161a6b Mon Sep 17 00:00:00 2001
>>>>> From: Brendan Doyle <brendan.doyle at oracle.com>
>>>>> Date: Fri, 28 May 2021 10:01:17 -0700
>>>>> Subject: [PATCH ovn] ovn-northd.c: Add proxy ARP support to OVN
>>>>>
>>>>> This patch provides the ability to configure proxy ARP IPs on a Logical
>>>>> Switch Router port. The IPs are added as Options for router ports. This
>>>>> provides a useful feature where traffic for a service must be sent to an
>>>>> address in a logical network address space, but the service is provided
>>>>> in a different network. For example an NFS service is provide to Logical
>>>>> networks at an address in their Logical network space, but the NFS
>>>>> server resides in a physical network. A Logical switch Router port can
>>>>> be configured to respond to ARP requests sent to the service "Logical
>>>>> address", the Logical Router/Gateway can then be configured to forward
>>>>> the traffic to the underlay/physical network.
>>>>>
>>>>> Signed-off-by: Brendan Doyle <brendan.doyle at oracle.com>
>>>>> ---
>>>>>    northd/ovn-northd.c |  44 ++++++++++++++++++++++
>>>>>    ovn-nb.xml          |   9 +++++
>>>>>    tests/ovn.at        | 103
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    3 files changed, 156 insertions(+)
>>>>>
>>>>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>>>>> index 0e5092a..9b686d9 100644
>>>>> --- a/northd/ovn-northd.c
>>>>> +++ b/northd/ovn-northd.c
>>>>> @@ -6943,6 +6943,7 @@ build_lswitch_arp_nd_responder_known_ips(struct
>>>>> ovn_port *op,
>>>>>                                             struct ds *match)
>>>>>    {
>>>>>        if (op->nbsp) {
>>>>> +        const char *arp_proxy;
>>>>>            if (!strcmp(op->nbsp->type, "virtual")) {
>>>>>                /* Handle
>>>>>                 *  - GARPs for virtual ip which belongs to a logical port
>>>>> @@ -7096,6 +7097,49 @@ build_lswitch_arp_nd_responder_known_ips(struct
>>>>> ovn_port *op,
>>>>>                    }
>>>>>                }
>>>>>            }
>>>>> +
>>>>> +        /*
>>>>> +         * Add responses for ARP proxies.
>>>>> +         */
>>>>> +        arp_proxy = smap_get(&op->nbsp->options,"arp_proxy");
>>> In my opinion instead of defining these proxy IPs in the logical
>>> switch port options,
>>> it is better to define them in the router port as generally a router
>>> would handle proxy arps (correct me if I'm wrong here).
>>>
>>> I'd suggest to add a new column "proxy_ips" (or something more appropriate)
>>> in the Logical_Router_Port and this column would point to an address set.
>>>
>>> CMS can define the list of IPs in the address set.  ovn-northd can add
>>> a logical flow like
>>>
>>> match = "inport == <router_port> && arp.tpa == $proxy_ip, action = {....}
>>> This would result in just one logical flow.
>>>
>>> CMS would do something like
>>>
>>> addr_set_uuid=$(ovn-nbctl create address_set name=proxy_arps
>>> addresses="10.0.0.4, 10.0.0.5")
>>> ovn-nbctl set logical_router_port <router_port> proxy_ip=$addr_set_uuid
>>>
>>> What do you think ?
>> In both cases only one OVS flows is created (after I made the changes)
>> suggested by Dan.
>> I can see the point with respect to proxy ARP and routers, in physical
>> equipment proxy ARP is done
>> by routers.  But I think doing the proxy ARP on the LRP of the LS is
>> consistent with the use of
>> the "options : nat-addresses" entry in the Logical_Switch_Port TABLE .
>> The patch makes it clear
>> that this feature is associated with a router, It is after all in the
>> "Options  for router ports". I think
>> it is also more efficient in that we'd respond to the ARP request
>> earlier in the datapath flows.
> Even if the proxy arps are associated with a router port, the flows can still be
> added in the logical switch pipeline of the peer to avoid the extra hop from
> logical switch pipeline to router pipeline.
>
> In my opinion,  it is better that the proxy arps are associated with a
> router port
> rather than a logical switch peer port.  Also having a separate column makes
> more sense to me than defining a string of IP addresses in the options smap.
I see your point, but  "options : nat-addresses" is a list of IPs on a 
router peer
port so it is at least consistent with that. Like "options : 
nat-addresses" I don't
expect it to be a large list.
> I defer to other reviewers if they are fine with your patch.

Ok thanks.
> Thanks
> Numan
>
>> The suggestion you make is a lot more involved, touching much more code
>> and changes the DB
>> schema, so I'm hesitant to go down that path.
>>
>> Brendan.
>>
>>> Thanks
>>> Numan
>>>
>>>>> +        if (arp_proxy && op->peer) {
>>>>> +            char *ips, *ip, *rest;
>>>>> +            int i = 0;
>>>>> +
>>>>> +            ips = xstrdup(arp_proxy);
>>>>> +            rest = ips;
>>>>> +
>>>>> +            /*
>>>>> +             * Match rule on all proxy ARP IPs.
>>>>> +             */
>>>>> +            ds_clear(match);
>>>>> +            ds_put_cstr(match, "arp.op == 1 && (");
>>>>> +            while ((ip = strtok_r(rest,",", &rest))) {
>>>>> +                if (i++ > 0) {
>>>>> +                        ds_put_cstr(match, " || ");
>>>>> +                };
>>>>> +                ds_put_format(match, "arp.tpa == %s", ip);
>>>>> +            }
>>>>> +            ds_put_cstr(match, ")");
>>>>> +
>>>>> +            ds_clear(actions);
>>>>> +            ds_put_format(actions,
>>>>> +                "eth.dst = eth.src; "
>>>>> +                "eth.src = %s; "
>>>>> +                "arp.op = 2; /* ARP reply */ "
>>>>> +                "arp.tha = arp.sha; "
>>>>> +                "arp.sha = %s; "
>>>>> +                "arp.tpa <-> arp.spa; "
>>>>> +                "outport = inport; "
>>>>> +                "flags.loopback = 1; "
>>>>> +                "output;",
>>>>> +                op->peer->lrp_networks.ea_s,
>>>>> +                op->peer->lrp_networks.ea_s);
>>>>> +
>>>>> +            ovn_lflow_add_with_hint(lflows, op->od,
>>>>> S_SWITCH_IN_ARP_ND_RSP,
>>>>> +                50, ds_cstr(match), ds_cstr(actions),
>>>>> &op->nbsp->header_);
>>>>> +            free(ips);
>>>>> +        }
>>>>>        }
>>>>>    }
>>>>>
>>>>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>>>>> index 02fd216..4b6c183 100644
>>>>> --- a/ovn-nb.xml
>>>>> +++ b/ovn-nb.xml
>>>>> @@ -848,6 +848,15 @@
>>>>>                </dd>
>>>>>              </dl>
>>>>>            </column>
>>>>> +
>>>>> +        <column name="options" key="arp_proxy">
>>>>> +          Optional. A comma separated list IPv4 addresses that this
>>>>> +          logical switch <code>router</code> port will reply to ARP
>>>>> requests.
>>>>> +          Example: <code>169.254.239.254,169.254.239.2</code>. The
>>>>> +          <ref column="options" key="router-port"/>'s logical router
>>>>> should
>>>>> +          have a route to forward packets sent to configured proxy
>>>>> ARP IPs to
>>>>> +          an appropriate destination.
>>>>> +        </column>
>>>>>          </group>
>>>>>
>>>>>          <group title="Options for localnet ports">
>>>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>>>> index 2c3c36d..4befe4a 100644
>>>>> --- a/tests/ovn.at
>>>>> +++ b/tests/ovn.at
>>>>> @@ -26527,3 +26527,106 @@ AT_CHECK([test $(ovn-appctl -t
>>>>> ovn-controller coverage/read-counter lflow_run) =
>>>>>    OVN_CLEANUP([hv1])
>>>>>    AT_CLEANUP
>>>>>    ])
>>>>> +
>>>>> +OVN_FOR_EACH_NORTHD([
>>>>> +AT_SETUP([ovn -- proxy-arp: 1 HVs, 1 LSs, 1 lport/LS, 1 LR])
>>>>> +AT_KEYWORDS([proxy-arp])
>>>>> +ovn_start
>>>>> +
>>>>> +# Logical network:
>>>>> +# One LR - lr1 has switch ls1 (192.16.1.0/24) connected to it,
>>>>> +# and and one HV with IP 192.16.1.6.
>>>>> +
>>>>> +ovn-nbctl lr-add lr1
>>>>> +ovn-nbctl ls-add ls1
>>>>> +
>>>>> +# Connect ls1 to lr1
>>>>> +ovn-nbctl lrp-add lr1 ls1 00:00:00:01:02:f1 192.16.1.1/24
>>>>> +ovn-nbctl lsp-add ls1 rp-ls1 -- set Logical_Switch_Port rp-ls1 \
>>>>> +    type=router options:router-port=ls1 addresses=\"00:00:00:01:02:f1\"
>>>>> +
>>>>> +# Create logical port ls1-lp1 in ls1
>>>>> +ovn-nbctl lsp-add ls1 ls1-lp1 \
>>>>> +-- lsp-set-addresses ls1-lp1 "00:00:00:01:02:03 192.16.1.6"
>>>>> +
>>>>> +
>>>>> +# Create one hypervisor and create OVS ports corresponding to logical
>>>>> ports.
>>>>> +net_add n1
>>>>> +
>>>>> +sim_add pa-hv
>>>>> +as pa-hv
>>>>> +ovs-vsctl add-br br-phys
>>>>> +ovn_attach n1 br-phys 192.16.0.1
>>>>> +
>>>>> +# Note: tx/rx are with respect to the LS port, so
>>>>> +# tx on switch port is HV rx, etc.
>>>>> +ovs-vsctl -- add-port br-int vif1 -- \
>>>>> +    set interface vif1 external-ids:iface-id=ls1-lp1 \
>>>>> +    options:tx_pcap=pa-hv/vif1-tx.pcap \
>>>>> +    options:rxq_pcap=pa-hv/vif1-rx.pcap \
>>>>> +    ofport-request=1
>>>>> +
>>>>> +# And proxy ARP flows for 69.254.239.254 and 169.254.239.2
>>>>> +# and check that SB flows have been added.
>>>>> +ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
>>>>> +options arp_proxy='"169.254.239.254,169.254.239.2"'
>>>>> +ovn-sbctl dump-flows > sbflows
>>>>> +AT_CAPTURE_FILE([sbflows])
>>>>> +
>>>>> +AT_CHECK([ovn-sbctl dump-flows | grep ls_in_arp_rsp | grep
>>>>> "169.254.239.2" | wc -l], [0], [dnl
>>>>> +1
>>>>> +])
>>>>> +
>>>>> +# Remove and check that the flows have been removed
>>>>> +ovn-nbctl --wait=hv remove Logical_Switch_Port rp-ls1 options
>>>>> arp_proxy='"169.254.239.254,169.254.239.2"'
>>>>> +
>>>>> +AT_CHECK([ovn-sbctl dump-flows | grep ls_in_arp_rsp | grep
>>>>> "169.254.239.2" | wc -l], [0], [dnl
>>>>> +0
>>>>> +])
>>>>> +
>>>>> +# Add the flows back send arp request and check we see an ARP response
>>>>> +ovn-nbctl --wait=hv add Logical_Switch_Port rp-ls1 \
>>>>> +options arp_proxy='"169.254.239.254,169.254.239.2"'
>>>>> +
>>>>> +ls1_p1_mac=00:00:00:01:02:03
>>>>> +ls1_p1_ip=192.16.1.6
>>>>> +
>>>>> +ls1_ro_mac=00:00:00:01:02:f1
>>>>> +ls1_ro_ip=192.168.1.1
>>>>> +
>>>>> +proxy_ip1=169.254.239.254
>>>>> +proxy_ip2=169.254.239.2
>>>>> +
>>>>> +bcast_mac=ff:ff:ff:ff:ff:ff
>>>>> +
>>>>> +# Send ARP request for 169.254.239.254
>>>>> +packet="inport==\"ls1-lp1\" && eth.src==$ls1_p1_mac &&
>>>>> eth.dst==$bcast_mac &&
>>>>> +       arp.op==1 && arp.sha==$ls1_p1_mac && arp.spa==$ls1_p1_ip &&
>>>>> +       arp.tha==$bcast_mac && arp.tpa==$proxy_ip1"
>>>>> +
>>>>> +as pa-hv ovs-appctl -t ovn-controller inject-pkt "$packet"
>>>>> +
>>>>> +ovs-ofctl dump-flows br-int| grep 169.254.239.254 | grep priority=50
>>>>>> debug1
>>>>> +AT_CAPTURE_FILE([debug1])
>>>>> +
>>>>> +
>>>>> +# Check if packet hit the ARP reply ovs flow
>>>>> +AT_CHECK([ovs-ofctl dump-flows br-int | \
>>>>> +    grep "169.254.239.254" | \
>>>>> +    grep "priority=50" | \
>>>>> +    grep "arp_op=1" | \
>>>>> +    grep "n_packets=1" | wc -l], [0], [dnl
>>>>> +1
>>>>> +])
>>>>> +
>>>>> +# Check that the HV gets an ARP reply
>>>>> +expected="eth.src==$ls1_ro_mac && eth.dst==$ls1_p1_mac &&
>>>>> +       arp.op==2 && arp.sha==$ls1_ro_mac && arp.spa==$proxy_ip1 &&
>>>>> +       arp.tha==$ls1_p1_mac && arp.tpa==$ls1_p1_ip"
>>>>> +echo $expected | ovstest test-ovn expr-to-packets > expected
>>>>> +
>>>>> +OVN_CHECK_PACKETS([pa-hv/vif1-tx.pcap], [expected])
>>>>> +
>>>>> +OVN_CLEANUP([pa-hv])
>>>>> +AT_CLEANUP
>>>>> +])
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev at openvswitch.org
>>>> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!GqivPVa7Brio!LVM6snHeQdP5rdldKTqBZu_C7h5JiR3HZtfLAmyQisBZUJ2TMpQCtj8JY49M3ZRRqmU$
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!GqivPVa7Brio!NrcJS3mjIV9x8Gh00JLaF0SagZXDLIzbYhxTxeL6cZuW1gbERdi7pe3ht80s-40RSQw$



More information about the dev mailing list