On Tue, Jun 15, 2021 at 8:34 AM Brendan Doyle <brendan.do...@oracle.com> wrote: > > > > On 15/06/2021 13:26, Numan Siddique wrote: > > On Tue, Jun 15, 2021 at 7:50 AM Brendan Doyle <brendan.do...@oracle.com> > > wrote: > >> > >> > >> On 14/06/2021 21:17, Numan Siddique wrote: > >>> On Mon, Jun 14, 2021 at 5:19 AM Brendan Doyle <brendan.do...@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.do...@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.do...@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() > >>>>> + > >>>>> + 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 > >>>> d...@openvswitch.org > >>>> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!GqivPVa7Brio!LVM6snHeQdP5rdldKTqBZu_C7h5JiR3HZtfLAmyQisBZUJ2TMpQCtj8JY49M3ZRRqmU$ > >> _______________________________________________ > >> dev mailing list > >> d...@openvswitch.org > >> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!GqivPVa7Brio!NrcJS3mjIV9x8Gh00JLaF0SagZXDLIzbYhxTxeL6cZuW1gbERdi7pe3ht80s-40RSQw$ > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev