Message ID | 20200319122641.473776-1-numans@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,ovn] ovn-northd: Don't add arp responder flows for lports with 'unknown' address. | expand |
On Thu, Mar 19, 2020 at 5:27 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > If a logical port has 'unknown' address, it means it can send and receive > packet with any IP and MAC and generally port security is not set for > such logical ports. If an lport has addresses set to - ["MAC1 IP1", unknown], > right now we add arp responder flows for IP1 and respond MAC1 in the arp > response. But it's possible that the VIF of the logical port can use the IP1 > with a different MAC. This patch supports this usecase. When another logical port > sends ARP request for IP1, the VIF of the logical port will anyway respond. > > Reported-by: Maciej Józefczyk <mjozefcz@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/ovn-northd.8.xml | 5 +++-- > northd/ovn-northd.c | 13 ++++++++----- > tests/ovn.at | 16 ++++++++++++---- > 3 files changed, 23 insertions(+), 11 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 9b44720d1..7d03cbc83 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -699,8 +699,9 @@ output; > > <p> > These flows are omitted for logical ports (other than router ports or > - <code>localport</code> ports) that are down and for logical ports of > - type <code>virtual</code>. > + <code>localport</code> ports) that are down, for logical ports of > + type <code>virtual</code> and for logical ports with 'unknown' > + address set. > </p> > </li> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 4f94680b5..f648d2ea7 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -1152,7 +1152,7 @@ struct ovn_port { > > bool derived; /* Indicates whether this is an additional port > * derived from nbsp or nbrp. */ > - > + bool has_unknown; /* If the addresses have 'unknown' defined. */ > /* The port's peer: > * > * - A switch port S of type "router" has a router port R as a peer, > @@ -2059,8 +2059,11 @@ join_logical_ports(struct northd_context *ctx, > op->lsp_addrs > = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses); > for (size_t j = 0; j < nbsp->n_addresses; j++) { > - if (!strcmp(nbsp->addresses[j], "unknown") > - || !strcmp(nbsp->addresses[j], "router")) { > + if (!strcmp(nbsp->addresses[j], "unknown")) { > + op->has_unknown = true; > + continue; > + } > + if (!strcmp(nbsp->addresses[j], "router")) { > continue; > } > if (is_dynamic_lsp_address(nbsp->addresses[j])) { > @@ -6127,7 +6130,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, > } else { > /* > * Add ARP/ND reply flows if either the > - * - port is up or > + * - port is up and it doesn't have 'unknown' address defined or > * - port type is router or > * - port type is localport > */ > @@ -6136,7 +6139,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, > continue; > } > > - if (lsp_is_external(op->nbsp)) { > + if (lsp_is_external(op->nbsp) || op->has_unknown) { > continue; > } > > diff --git a/tests/ovn.at b/tests/ovn.at > index 8cdbad743..1b6073ff0 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1758,11 +1758,13 @@ for is in 1 2 3; do > sip=`ip_to_hex 192 168 0 $is$js` > tip=`ip_to_hex 192 168 0 $id$jd` > tip_unknown=`ip_to_hex 11 11 11 11` > + reply_ha=; > if test $d != $s; then > - reply_ha=f000000000$d > - else > - reply_ha= > + if test $jd != 1; then > + reply_ha=f000000000$d > + fi > fi > + > test_arp $s f000000000$s $sip $tip $reply_ha #9 > test_arp $s f000000000$s $sip $tip_unknown #10 > > @@ -2199,7 +2201,13 @@ for s in 1 2 3; do > sip=192.168.0.$s > tip=192.168.0.$d > tip_unknown=11.11.11.11 > - if test $d != $s; then reply_ha=f0:00:00:00:00:0$d; else reply_ha=; fi > + reply_ha=; > + if test $d != $s; then > + if test $d != 1; then > + reply_ha=f0:00:00:00:00:0$d; > + fi > + fi > + > test_arp $s f0:00:00:00:00:0$s $sip $tip $reply_ha #9 > test_arp $s f0:00:00:00:00:0$s $sip $tip_unknown #10 > > -- > 2.24.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Acked-by: Han Zhou <hzhou@ovn.org>
On Thu, Mar 19, 2020 at 9:51 PM Han Zhou <zhouhan@gmail.com> wrote: > > On Thu, Mar 19, 2020 at 5:27 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@ovn.org> > > > > If a logical port has 'unknown' address, it means it can send and receive > > packet with any IP and MAC and generally port security is not set for > > such logical ports. If an lport has addresses set to - ["MAC1 IP1", > unknown], > > right now we add arp responder flows for IP1 and respond MAC1 in the arp > > response. But it's possible that the VIF of the logical port can use the > IP1 > > with a different MAC. This patch supports this usecase. When another > logical port > > sends ARP request for IP1, the VIF of the logical port will anyway > respond. > > > > Reported-by: Maciej Józefczyk <mjozefcz@redhat.com> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > northd/ovn-northd.8.xml | 5 +++-- > > northd/ovn-northd.c | 13 ++++++++----- > > tests/ovn.at | 16 ++++++++++++---- > > 3 files changed, 23 insertions(+), 11 deletions(-) > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index 9b44720d1..7d03cbc83 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -699,8 +699,9 @@ output; > > > > <p> > > These flows are omitted for logical ports (other than router > ports or > > - <code>localport</code> ports) that are down and for logical > ports of > > - type <code>virtual</code>. > > + <code>localport</code> ports) that are down, for logical ports > of > > + type <code>virtual</code> and for logical ports with 'unknown' > > + address set. > > </p> > > </li> > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 4f94680b5..f648d2ea7 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -1152,7 +1152,7 @@ struct ovn_port { > > > > bool derived; /* Indicates whether this is an additional port > > * derived from nbsp or nbrp. */ > > - > > + bool has_unknown; /* If the addresses have 'unknown' defined. */ > > /* The port's peer: > > * > > * - A switch port S of type "router" has a router port R as a > peer, > > @@ -2059,8 +2059,11 @@ join_logical_ports(struct northd_context *ctx, > > op->lsp_addrs > > = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses); > > for (size_t j = 0; j < nbsp->n_addresses; j++) { > > - if (!strcmp(nbsp->addresses[j], "unknown") > > - || !strcmp(nbsp->addresses[j], "router")) { > > + if (!strcmp(nbsp->addresses[j], "unknown")) { > > + op->has_unknown = true; > > + continue; > > + } > > + if (!strcmp(nbsp->addresses[j], "router")) { > > continue; > > } > > if (is_dynamic_lsp_address(nbsp->addresses[j])) { > > @@ -6127,7 +6130,7 @@ build_lswitch_flows(struct hmap *datapaths, struct > hmap *ports, > > } else { > > /* > > * Add ARP/ND reply flows if either the > > - * - port is up or > > + * - port is up and it doesn't have 'unknown' address > defined or > > * - port type is router or > > * - port type is localport > > */ > > @@ -6136,7 +6139,7 @@ build_lswitch_flows(struct hmap *datapaths, struct > hmap *ports, > > continue; > > } > > > > - if (lsp_is_external(op->nbsp)) { > > + if (lsp_is_external(op->nbsp) || op->has_unknown) { > > continue; > > } > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 8cdbad743..1b6073ff0 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -1758,11 +1758,13 @@ for is in 1 2 3; do > > sip=`ip_to_hex 192 168 0 $is$js` > > tip=`ip_to_hex 192 168 0 $id$jd` > > tip_unknown=`ip_to_hex 11 11 11 11` > > + reply_ha=; > > if test $d != $s; then > > - reply_ha=f000000000$d > > - else > > - reply_ha= > > + if test $jd != 1; then > > + reply_ha=f000000000$d > > + fi > > fi > > + > > test_arp $s f000000000$s $sip $tip $reply_ha > #9 > > test_arp $s f000000000$s $sip $tip_unknown > #10 > > > > @@ -2199,7 +2201,13 @@ for s in 1 2 3; do > > sip=192.168.0.$s > > tip=192.168.0.$d > > tip_unknown=11.11.11.11 > > - if test $d != $s; then reply_ha=f0:00:00:00:00:0$d; else > reply_ha=; fi > > + reply_ha=; > > + if test $d != $s; then > > + if test $d != 1; then > > + reply_ha=f0:00:00:00:00:0$d; > > + fi > > + fi > > + > > test_arp $s f0:00:00:00:00:0$s $sip $tip $reply_ha > #9 > > test_arp $s f0:00:00:00:00:0$s $sip $tip_unknown > #10 > > > > -- > > 2.24.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Acked-by: Han Zhou <hzhou@ovn.org> Thanks. I applied this patch to master. Numan > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 9b44720d1..7d03cbc83 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -699,8 +699,9 @@ output; <p> These flows are omitted for logical ports (other than router ports or - <code>localport</code> ports) that are down and for logical ports of - type <code>virtual</code>. + <code>localport</code> ports) that are down, for logical ports of + type <code>virtual</code> and for logical ports with 'unknown' + address set. </p> </li> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 4f94680b5..f648d2ea7 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -1152,7 +1152,7 @@ struct ovn_port { bool derived; /* Indicates whether this is an additional port * derived from nbsp or nbrp. */ - + bool has_unknown; /* If the addresses have 'unknown' defined. */ /* The port's peer: * * - A switch port S of type "router" has a router port R as a peer, @@ -2059,8 +2059,11 @@ join_logical_ports(struct northd_context *ctx, op->lsp_addrs = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses); for (size_t j = 0; j < nbsp->n_addresses; j++) { - if (!strcmp(nbsp->addresses[j], "unknown") - || !strcmp(nbsp->addresses[j], "router")) { + if (!strcmp(nbsp->addresses[j], "unknown")) { + op->has_unknown = true; + continue; + } + if (!strcmp(nbsp->addresses[j], "router")) { continue; } if (is_dynamic_lsp_address(nbsp->addresses[j])) { @@ -6127,7 +6130,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, } else { /* * Add ARP/ND reply flows if either the - * - port is up or + * - port is up and it doesn't have 'unknown' address defined or * - port type is router or * - port type is localport */ @@ -6136,7 +6139,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, continue; } - if (lsp_is_external(op->nbsp)) { + if (lsp_is_external(op->nbsp) || op->has_unknown) { continue; } diff --git a/tests/ovn.at b/tests/ovn.at index 8cdbad743..1b6073ff0 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1758,11 +1758,13 @@ for is in 1 2 3; do sip=`ip_to_hex 192 168 0 $is$js` tip=`ip_to_hex 192 168 0 $id$jd` tip_unknown=`ip_to_hex 11 11 11 11` + reply_ha=; if test $d != $s; then - reply_ha=f000000000$d - else - reply_ha= + if test $jd != 1; then + reply_ha=f000000000$d + fi fi + test_arp $s f000000000$s $sip $tip $reply_ha #9 test_arp $s f000000000$s $sip $tip_unknown #10 @@ -2199,7 +2201,13 @@ for s in 1 2 3; do sip=192.168.0.$s tip=192.168.0.$d tip_unknown=11.11.11.11 - if test $d != $s; then reply_ha=f0:00:00:00:00:0$d; else reply_ha=; fi + reply_ha=; + if test $d != $s; then + if test $d != 1; then + reply_ha=f0:00:00:00:00:0$d; + fi + fi + test_arp $s f0:00:00:00:00:0$s $sip $tip $reply_ha #9 test_arp $s f0:00:00:00:00:0$s $sip $tip_unknown #10