[ovs-dev,ovn,v1] northd: Allow /64 after ipv6_prefix
diff mbox series

Message ID 20200219155050.615473-1-russell@ovn.org
State New
Headers show
Series
  • [ovs-dev,ovn,v1] northd: Allow /64 after ipv6_prefix
Related show

Commit Message

Russell Bryant Feb. 19, 2020, 3:50 p.m. UTC
We recently hit a bug in ovn-kubernetes, where I accidentally added
/64 at the end of ipv6_prefix, to match the format we used for the
subnet option for IPv4.  This was not allowed.

This patch update ovn-northd to take the ipv6_prefix either with or
without a trailing "/64".  It still enforces a /64 CIDR prefix length.

A test case was updated to ensure that a prefix with "/64" is now
accepted.

Signed-off-by: Russell Bryant <russell@ovn.org>
---
 northd/ovn-northd.c | 31 +++++++++++++++++++++++++++++--
 tests/ovn.at        |  4 +++-
 2 files changed, 32 insertions(+), 3 deletions(-)

Comments

0-day Robot Feb. 19, 2020, 4:58 p.m. UTC | #1
Bleep bloop.  Greetings Russell Bryant, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 82 characters long (recommended limit is 79)
#41 FILE: northd/ovn-northd.c:676:
                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s", ipv6_prefix, error);

WARNING: Line is 88 characters long (recommended limit is 79)
#49 FILE: northd/ovn-northd.c:684:
                    VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be /64", ipv6_prefix);

Lines checked: 82, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Numan Siddique Feb. 20, 2020, 3:46 p.m. UTC | #2
On Wed, Feb 19, 2020 at 9:27 PM Russell Bryant <russell@ovn.org> wrote:
>
> We recently hit a bug in ovn-kubernetes, where I accidentally added
> /64 at the end of ipv6_prefix, to match the format we used for the
> subnet option for IPv4.  This was not allowed.
>
> This patch update ovn-northd to take the ipv6_prefix either with or
> without a trailing "/64".  It still enforces a /64 CIDR prefix length.
>
> A test case was updated to ensure that a prefix with "/64" is now
> accepted.
>
> Signed-off-by: Russell Bryant <russell@ovn.org>

With the below check patch warnings fixed

Acked-by: Numan Siddique <numans@ovn.org>

****
WARNING: Line is 82 characters long (recommended limit is 79)
#40 FILE: northd/ovn-northd.c:676:
                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s",
ipv6_prefix, error);

WARNING: Line is 88 characters long (recommended limit is 79)
#48 FILE: northd/ovn-northd.c:684:
                    VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be
/64", ipv6_prefix);

****

Thanks
Numan

> ---
>  northd/ovn-northd.c | 31 +++++++++++++++++++++++++++++--
>  tests/ovn.at        |  4 +++-
>  2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 2580b4ec9..59d085aa9 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -664,8 +664,35 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
>      const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix");
>
>      if (ipv6_prefix) {
> -        od->ipam_info.ipv6_prefix_set = ipv6_parse(
> -            ipv6_prefix, &od->ipam_info.ipv6_prefix);
> +        if (strstr(ipv6_prefix, "/")) {
> +            /* If a prefix length was specified, it must be 64. */
> +            struct in6_addr mask;
> +            char *error
> +                = ipv6_parse_masked(ipv6_prefix,
> +                                    &od->ipam_info.ipv6_prefix, &mask);
> +            if (error) {
> +                static struct vlog_rate_limit rl
> +                    = VLOG_RATE_LIMIT_INIT(5, 1);
> +                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s", ipv6_prefix, error);
> +                free(error);
> +            } else {
> +                if (ipv6_count_cidr_bits(&mask) == 64) {
> +                    od->ipam_info.ipv6_prefix_set = true;
> +                } else {
> +                    static struct vlog_rate_limit rl
> +                        = VLOG_RATE_LIMIT_INIT(5, 1);
> +                    VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be /64", ipv6_prefix);
> +                }
> +            }
> +        } else {
> +            od->ipam_info.ipv6_prefix_set = ipv6_parse(
> +                ipv6_prefix, &od->ipam_info.ipv6_prefix);
> +            if (!od->ipam_info.ipv6_prefix_set) {
> +                static struct vlog_rate_limit rl
> +                    = VLOG_RATE_LIMIT_INIT(5, 1);
> +                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s", ipv6_prefix);
> +            }
> +        }
>      }
>
>      if (!subnet_str) {
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 254645a3a..cbaa6d4a2 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -12289,8 +12289,10 @@ ovn-nbctl set Logical_Switch ls1 \
>      other_config:subnet=10.1.0.0/24 other_config:ipv6_prefix="2001:db8:1::"
>  ovn-nbctl set Logical_Switch ls2 \
>      other_config:subnet=10.2.0.0/24 other_config:ipv6_prefix="2001:db8:2::"
> +
> +# A prefix length may be specified, but only if it is /64.
>  ovn-nbctl set Logical_Switch ls3 \
> -    other_config:subnet=10.3.0.0/24 other_config:ipv6_prefix="2001:db8:3::"
> +    other_config:subnet=10.3.0.0/24 other_config:ipv6_prefix="2001:db8:3::/64"
>
>  ovn-nbctl lsp-add ls1 lp1
>  ovn-nbctl lsp-add ls2 lp2
> --
> 2.24.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Russell Bryant Feb. 20, 2020, 7:32 p.m. UTC | #3
On Thu, Feb 20, 2020 at 10:46 AM Numan Siddique <numans@ovn.org> wrote:

> On Wed, Feb 19, 2020 at 9:27 PM Russell Bryant <russell@ovn.org> wrote:
> >
> > We recently hit a bug in ovn-kubernetes, where I accidentally added
> > /64 at the end of ipv6_prefix, to match the format we used for the
> > subnet option for IPv4.  This was not allowed.
> >
> > This patch update ovn-northd to take the ipv6_prefix either with or
> > without a trailing "/64".  It still enforces a /64 CIDR prefix length.
> >
> > A test case was updated to ensure that a prefix with "/64" is now
> > accepted.
> >
> > Signed-off-by: Russell Bryant <russell@ovn.org>
>
> With the below check patch warnings fixed
>
> Acked-by: Numan Siddique <numans@ovn.org>
>
>
Thanks!  I fixed the line length issues and pushed to master.



> ****
> WARNING: Line is 82 characters long (recommended limit is 79)
> #40 FILE: northd/ovn-northd.c:676:
>                 VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s",
> ipv6_prefix, error);
>
> WARNING: Line is 88 characters long (recommended limit is 79)
> #48 FILE: northd/ovn-northd.c:684:
>                     VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be
> /64", ipv6_prefix);
>
> ****
>
> Thanks
> Numan
>
> > ---
> >  northd/ovn-northd.c | 31 +++++++++++++++++++++++++++++--
> >  tests/ovn.at        |  4 +++-
> >  2 files changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 2580b4ec9..59d085aa9 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -664,8 +664,35 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
> >      const char *ipv6_prefix = smap_get(&od->nbs->other_config,
> "ipv6_prefix");
> >
> >      if (ipv6_prefix) {
> > -        od->ipam_info.ipv6_prefix_set = ipv6_parse(
> > -            ipv6_prefix, &od->ipam_info.ipv6_prefix);
> > +        if (strstr(ipv6_prefix, "/")) {
> > +            /* If a prefix length was specified, it must be 64. */
> > +            struct in6_addr mask;
> > +            char *error
> > +                = ipv6_parse_masked(ipv6_prefix,
> > +                                    &od->ipam_info.ipv6_prefix, &mask);
> > +            if (error) {
> > +                static struct vlog_rate_limit rl
> > +                    = VLOG_RATE_LIMIT_INIT(5, 1);
> > +                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s",
> ipv6_prefix, error);
> > +                free(error);
> > +            } else {
> > +                if (ipv6_count_cidr_bits(&mask) == 64) {
> > +                    od->ipam_info.ipv6_prefix_set = true;
> > +                } else {
> > +                    static struct vlog_rate_limit rl
> > +                        = VLOG_RATE_LIMIT_INIT(5, 1);
> > +                    VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be
> /64", ipv6_prefix);
> > +                }
> > +            }
> > +        } else {
> > +            od->ipam_info.ipv6_prefix_set = ipv6_parse(
> > +                ipv6_prefix, &od->ipam_info.ipv6_prefix);
> > +            if (!od->ipam_info.ipv6_prefix_set) {
> > +                static struct vlog_rate_limit rl
> > +                    = VLOG_RATE_LIMIT_INIT(5, 1);
> > +                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s", ipv6_prefix);
> > +            }
> > +        }
> >      }
> >
> >      if (!subnet_str) {
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 254645a3a..cbaa6d4a2 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -12289,8 +12289,10 @@ ovn-nbctl set Logical_Switch ls1 \
> >      other_config:subnet=10.1.0.0/24
> other_config:ipv6_prefix="2001:db8:1::"
> >  ovn-nbctl set Logical_Switch ls2 \
> >      other_config:subnet=10.2.0.0/24
> other_config:ipv6_prefix="2001:db8:2::"
> > +
> > +# A prefix length may be specified, but only if it is /64.
> >  ovn-nbctl set Logical_Switch ls3 \
> > -    other_config:subnet=10.3.0.0/24
> other_config:ipv6_prefix="2001:db8:3::"
> > +    other_config:subnet=10.3.0.0/24
> other_config:ipv6_prefix="2001:db8:3::/64"
> >
> >  ovn-nbctl lsp-add ls1 lp1
> >  ovn-nbctl lsp-add ls2 lp2
> > --
> > 2.24.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
>
Numan Siddique Feb. 21, 2020, 11:08 a.m. UTC | #4
On Fri, Feb 21, 2020 at 1:03 AM Russell Bryant <rbryant@redhat.com> wrote:
>
> On Thu, Feb 20, 2020 at 10:46 AM Numan Siddique <numans@ovn.org> wrote:
>
> > On Wed, Feb 19, 2020 at 9:27 PM Russell Bryant <russell@ovn.org> wrote:
> > >
> > > We recently hit a bug in ovn-kubernetes, where I accidentally added
> > > /64 at the end of ipv6_prefix, to match the format we used for the
> > > subnet option for IPv4.  This was not allowed.
> > >
> > > This patch update ovn-northd to take the ipv6_prefix either with or
> > > without a trailing "/64".  It still enforces a /64 CIDR prefix length.
> > >
> > > A test case was updated to ensure that a prefix with "/64" is now
> > > accepted.
> > >
> > > Signed-off-by: Russell Bryant <russell@ovn.org>
> >
> > With the below check patch warnings fixed
> >
> > Acked-by: Numan Siddique <numans@ovn.org>
> >
> >
> Thanks!  I fixed the line length issues and pushed to master.

Hi Russell,
I applied this patch to branch-20.03.

Thanks
Numan

>
>
>
> > ****
> > WARNING: Line is 82 characters long (recommended limit is 79)
> > #40 FILE: northd/ovn-northd.c:676:
> >                 VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s",
> > ipv6_prefix, error);
> >
> > WARNING: Line is 88 characters long (recommended limit is 79)
> > #48 FILE: northd/ovn-northd.c:684:
> >                     VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be
> > /64", ipv6_prefix);
> >
> > ****
> >
> > Thanks
> > Numan
> >
> > > ---
> > >  northd/ovn-northd.c | 31 +++++++++++++++++++++++++++++--
> > >  tests/ovn.at        |  4 +++-
> > >  2 files changed, 32 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index 2580b4ec9..59d085aa9 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -664,8 +664,35 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
> > >      const char *ipv6_prefix = smap_get(&od->nbs->other_config,
> > "ipv6_prefix");
> > >
> > >      if (ipv6_prefix) {
> > > -        od->ipam_info.ipv6_prefix_set = ipv6_parse(
> > > -            ipv6_prefix, &od->ipam_info.ipv6_prefix);
> > > +        if (strstr(ipv6_prefix, "/")) {
> > > +            /* If a prefix length was specified, it must be 64. */
> > > +            struct in6_addr mask;
> > > +            char *error
> > > +                = ipv6_parse_masked(ipv6_prefix,
> > > +                                    &od->ipam_info.ipv6_prefix, &mask);
> > > +            if (error) {
> > > +                static struct vlog_rate_limit rl
> > > +                    = VLOG_RATE_LIMIT_INIT(5, 1);
> > > +                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s",
> > ipv6_prefix, error);
> > > +                free(error);
> > > +            } else {
> > > +                if (ipv6_count_cidr_bits(&mask) == 64) {
> > > +                    od->ipam_info.ipv6_prefix_set = true;
> > > +                } else {
> > > +                    static struct vlog_rate_limit rl
> > > +                        = VLOG_RATE_LIMIT_INIT(5, 1);
> > > +                    VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be
> > /64", ipv6_prefix);
> > > +                }
> > > +            }
> > > +        } else {
> > > +            od->ipam_info.ipv6_prefix_set = ipv6_parse(
> > > +                ipv6_prefix, &od->ipam_info.ipv6_prefix);
> > > +            if (!od->ipam_info.ipv6_prefix_set) {
> > > +                static struct vlog_rate_limit rl
> > > +                    = VLOG_RATE_LIMIT_INIT(5, 1);
> > > +                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s", ipv6_prefix);
> > > +            }
> > > +        }
> > >      }
> > >
> > >      if (!subnet_str) {
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index 254645a3a..cbaa6d4a2 100644
> > > --- a/tests/ovn.at
> > > +++ b/tests/ovn.at
> > > @@ -12289,8 +12289,10 @@ ovn-nbctl set Logical_Switch ls1 \
> > >      other_config:subnet=10.1.0.0/24
> > other_config:ipv6_prefix="2001:db8:1::"
> > >  ovn-nbctl set Logical_Switch ls2 \
> > >      other_config:subnet=10.2.0.0/24
> > other_config:ipv6_prefix="2001:db8:2::"
> > > +
> > > +# A prefix length may be specified, but only if it is /64.
> > >  ovn-nbctl set Logical_Switch ls3 \
> > > -    other_config:subnet=10.3.0.0/24
> > other_config:ipv6_prefix="2001:db8:3::"
> > > +    other_config:subnet=10.3.0.0/24
> > other_config:ipv6_prefix="2001:db8:3::/64"
> > >
> > >  ovn-nbctl lsp-add ls1 lp1
> > >  ovn-nbctl lsp-add ls2 lp2
> > > --
> > > 2.24.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
> >
>
> --
> Russell Bryant
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Patch
diff mbox series

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 2580b4ec9..59d085aa9 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -664,8 +664,35 @@  init_ipam_info_for_datapath(struct ovn_datapath *od)
     const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix");
 
     if (ipv6_prefix) {
-        od->ipam_info.ipv6_prefix_set = ipv6_parse(
-            ipv6_prefix, &od->ipam_info.ipv6_prefix);
+        if (strstr(ipv6_prefix, "/")) {
+            /* If a prefix length was specified, it must be 64. */
+            struct in6_addr mask;
+            char *error
+                = ipv6_parse_masked(ipv6_prefix,
+                                    &od->ipam_info.ipv6_prefix, &mask);
+            if (error) {
+                static struct vlog_rate_limit rl
+                    = VLOG_RATE_LIMIT_INIT(5, 1);
+                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s", ipv6_prefix, error);
+                free(error);
+            } else {
+                if (ipv6_count_cidr_bits(&mask) == 64) {
+                    od->ipam_info.ipv6_prefix_set = true;
+                } else {
+                    static struct vlog_rate_limit rl
+                        = VLOG_RATE_LIMIT_INIT(5, 1);
+                    VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be /64", ipv6_prefix);
+                }
+            }
+        } else {
+            od->ipam_info.ipv6_prefix_set = ipv6_parse(
+                ipv6_prefix, &od->ipam_info.ipv6_prefix);
+            if (!od->ipam_info.ipv6_prefix_set) {
+                static struct vlog_rate_limit rl
+                    = VLOG_RATE_LIMIT_INIT(5, 1);
+                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s", ipv6_prefix);
+            }
+        }
     }
 
     if (!subnet_str) {
diff --git a/tests/ovn.at b/tests/ovn.at
index 254645a3a..cbaa6d4a2 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -12289,8 +12289,10 @@  ovn-nbctl set Logical_Switch ls1 \
     other_config:subnet=10.1.0.0/24 other_config:ipv6_prefix="2001:db8:1::"
 ovn-nbctl set Logical_Switch ls2 \
     other_config:subnet=10.2.0.0/24 other_config:ipv6_prefix="2001:db8:2::"
+
+# A prefix length may be specified, but only if it is /64.
 ovn-nbctl set Logical_Switch ls3 \
-    other_config:subnet=10.3.0.0/24 other_config:ipv6_prefix="2001:db8:3::"
+    other_config:subnet=10.3.0.0/24 other_config:ipv6_prefix="2001:db8:3::/64"
 
 ovn-nbctl lsp-add ls1 lp1
 ovn-nbctl lsp-add ls2 lp2