diff mbox series

[ovs-dev,ovn] ovn-northd: Don't add arp responder flows for lports with 'unknown' address.

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

Commit Message

Numan Siddique March 19, 2020, 12:26 p.m. UTC 
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(-)

Comments

Han Zhou March 19, 2020, 4:20 p.m. UTC | #1 
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>
Numan Siddique March 20, 2020, 8:57 a.m. UTC | #2 
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 mbox series

Patch

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