[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 13:54:07 UTC 2021



On 15/06/2021 14:33, Numan Siddique wrote:
> On Tue, Jun 15, 2021 at 8:34 AM Brendan Doyle <brendan.doyle at oracle.com> wrote:
>>
>>
>> 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.
> Ok.  I'd have no objections if other reviewers are fine.
>
> Please see below for one comment.
>
> Thanks
> Numan
>
>>> 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, ")");
> Instead of the above while loop please make use of the util function -
> extract_ip_address()
>

Sure will look in to it in a bit, just have to finish something else first.
Thanks
>>>>>>> +
>>>>>>> +            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$
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!GqivPVa7Brio!M1gGAG59qK2tuFqHQceuZtUEkfxgv3YLMmZOtNl97ZvcwkDz3bIAz-s9NgmJuQyJi48$



More information about the dev mailing list