| Message ID | 20200219155050.615473-1-russell@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn,v1] northd: Allow /64 after ipv6_prefix | expand |
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
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 >
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 > > > >
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 >
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
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(-)