Re: [PATCH 2/3] openbgpd: Add support for 'unknown' well-known communities
ok
as wor the WELLKNOWN, what do other implementations do?
If others print it or accept it as input, lets keep it.
If not, remove it from both your diff and bgpctls output.
/Benno
Job Snijders(j...@instituut.net) on 2017.06.25 14:59:21 +0200:
> Small update.
>
> I renamed the 'msb' argument ('most significant bits') to 'part' to
> improve readability. In Community 15562:4, '15562' is part 0 and the '4'
> is part 1. Same type of logic might be useful down the road for Large
> Communities which would have 3 parts.
>
> - Job
>
> diff --git usr.sbin/bgpctl/parser.c usr.sbin/bgpctl/parser.c
> index 85300d1cd32..c9d63f9ade3 100644
> --- usr.sbin/bgpctl/parser.c
> +++ usr.sbin/bgpctl/parser.c
> @@ -413,7 +413,7 @@ intparse_addr(const char *,
> struct bgpd_addr *);
> int parse_asnum(const char *, size_t, u_int32_t *);
> int parse_number(const char *, struct parse_result *,
>enum token_type);
> -int getcommunity(const char *);
> +int getcommunity(const char *, int);
> int parse_community(const char *, struct parse_result *);
> u_int getlargecommunity(const char *);
> int parse_largecommunity(const char *, struct parse_result
> *);
> @@ -927,7 +927,7 @@ parse_number(const char *word, struct parse_result *r,
> enum token_type type)
> }
>
> int
> -getcommunity(const char *s)
> +getcommunity(const char *s, int part)
> {
> const char *errstr;
> u_int16_tuval;
> @@ -935,6 +935,9 @@ getcommunity(const char *s)
> if (strcmp(s, "*") == 0)
> return (COMMUNITY_ANY);
>
> + if (part == 0 && strcmp(s, "WELLKNOWN") == 0)
> + return (COMMUNITY_WELLKNOWN);
> +
> uval = strtonum(s, 0, USHRT_MAX, );
> if (errstr)
> errx(1, "Community is %s: %s", errstr, s);
> @@ -978,8 +981,8 @@ parse_community(const char *word, struct parse_result *r)
> }
> *p++ = 0;
>
> - as = getcommunity(word);
> - type = getcommunity(p);
> + as = getcommunity(word, 0);
> + type = getcommunity(p, 1);
>
> done:
> if (as == 0) {
> @@ -994,10 +997,6 @@ done:
> case COMMUNITY_BLACKHOLE:
> /* valid */
> break;
> - default:
> - /* unknown */
> - fprintf(stderr, "Unknown well-known community\n");
> - return (0);
> }
>
> if ((fs = calloc(1, sizeof(struct filter_set))) == NULL)
> diff --git usr.sbin/bgpd/parse.y usr.sbin/bgpd/parse.y
> index f0c96051e17..ec4ed956b60 100644
> --- usr.sbin/bgpd/parse.y
> +++ usr.sbin/bgpd/parse.y
> @@ -146,7 +146,7 @@ void copy_filterset(struct filter_set_head
> *,
> void merge_filter_lists(struct filter_head *, struct filter_head *);
> struct filter_rule *get_rule(enum action_types);
>
> -int getcommunity(char *);
> +int getcommunity(char *, int);
> int parsecommunity(struct filter_community *, char *);
> int64_t getlargecommunity(char *);
> int parselargecommunity(struct filter_largecommunity *, char *);
> @@ -2963,11 +2963,13 @@ symget(const char *nam)
> }
>
> int
> -getcommunity(char *s)
> +getcommunity(char *s, int part)
> {
> int val;
> const char *errstr;
>
> + if (part == 0 && strcmp(s, "WELLKNOWN") == 0)
> + return (COMMUNITY_WELLKNOWN);
> if (strcmp(s, "*") == 0)
> return (COMMUNITY_ANY);
> if (strcmp(s, "neighbor-as") == 0)
> @@ -3017,15 +3019,11 @@ parsecommunity(struct filter_community *c, char *s)
> }
> *p++ = 0;
>
> - if ((i = getcommunity(s)) == COMMUNITY_ERROR)
> + if ((i = getcommunity(s, 0)) == COMMUNITY_ERROR)
> return (-1);
> - if (i == COMMUNITY_WELLKNOWN) {
> - yyerror("Bad community AS number");
> - return (-1);
> - }
> as = i;
>
> - if ((i = getcommunity(p)) == COMMUNITY_ERROR)
> + if ((i = getcommunity(p, 1)) == COMMUNITY_ERROR)
> return (-1);
> c->as = as;
> c->type = i;
>
Re: [PATCH 2/3] openbgpd: Add support for 'unknown' well-known communities
On Sun, Jun 25, 2017 at 11:41:05PM +0200, Sebastian Benoit wrote:
> ok
>
> as wor the WELLKNOWN, what do other implementations do?
I'm not aware of other implementations that do a blanket replacement of
"65535:" with something like "WELLKNOWN:" in their CLI output.
Most implementations (after native support for a given well-known
community is added), do replace strings like "65535:666" with a human
readable version like "BLACKHOLE". Subsequently, they often accept both
the numeric and text version of the community as input. In this regard
openbsd is already aligned with the industry pattern.
> If others print it or accept it as input, lets keep it.
> If not, remove it from both your diff and bgpctls output.
ok, below:
diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
index 4d9701da35b..4242b3484ca 100644
--- usr.sbin/bgpctl/bgpctl.c
+++ usr.sbin/bgpctl/bgpctl.c
@@ -1548,7 +1548,7 @@ show_community(u_char *data, u_int16_t len)
printf("BLACKHOLE");
break;
default:
- printf("WELLKNOWN:%hu", v);
+ printf("%hu:%hu", a, v);
break;
}
else
diff --git usr.sbin/bgpctl/parser.c usr.sbin/bgpctl/parser.c
index 85300d1cd32..0846436a16b 100644
--- usr.sbin/bgpctl/parser.c
+++ usr.sbin/bgpctl/parser.c
@@ -994,10 +994,6 @@ done:
case COMMUNITY_BLACKHOLE:
/* valid */
break;
- default:
- /* unknown */
- fprintf(stderr, "Unknown well-known community\n");
- return (0);
}
if ((fs = calloc(1, sizeof(struct filter_set))) == NULL)
diff --git usr.sbin/bgpd/parse.y usr.sbin/bgpd/parse.y
index f0c96051e17..7eec31f2bda 100644
--- usr.sbin/bgpd/parse.y
+++ usr.sbin/bgpd/parse.y
@@ -3019,10 +3019,6 @@ parsecommunity(struct filter_community *c, char *s)
if ((i = getcommunity(s)) == COMMUNITY_ERROR)
return (-1);
- if (i == COMMUNITY_WELLKNOWN) {
- yyerror("Bad community AS number");
- return (-1);
- }
as = i;
if ((i = getcommunity(p)) == COMMUNITY_ERROR)
Re: [PATCH 2/3] openbgpd: Add support for 'unknown' well-known communities
Small update.
I renamed the 'msb' argument ('most significant bits') to 'part' to
improve readability. In Community 15562:4, '15562' is part 0 and the '4'
is part 1. Same type of logic might be useful down the road for Large
Communities which would have 3 parts.
- Job
diff --git usr.sbin/bgpctl/parser.c usr.sbin/bgpctl/parser.c
index 85300d1cd32..c9d63f9ade3 100644
--- usr.sbin/bgpctl/parser.c
+++ usr.sbin/bgpctl/parser.c
@@ -413,7 +413,7 @@ int parse_addr(const char *, struct
bgpd_addr *);
int parse_asnum(const char *, size_t, u_int32_t *);
int parse_number(const char *, struct parse_result *,
enum token_type);
-int getcommunity(const char *);
+int getcommunity(const char *, int);
int parse_community(const char *, struct parse_result *);
u_int getlargecommunity(const char *);
int parse_largecommunity(const char *, struct parse_result
*);
@@ -927,7 +927,7 @@ parse_number(const char *word, struct parse_result *r, enum
token_type type)
}
int
-getcommunity(const char *s)
+getcommunity(const char *s, int part)
{
const char *errstr;
u_int16_tuval;
@@ -935,6 +935,9 @@ getcommunity(const char *s)
if (strcmp(s, "*") == 0)
return (COMMUNITY_ANY);
+ if (part == 0 && strcmp(s, "WELLKNOWN") == 0)
+ return (COMMUNITY_WELLKNOWN);
+
uval = strtonum(s, 0, USHRT_MAX, );
if (errstr)
errx(1, "Community is %s: %s", errstr, s);
@@ -978,8 +981,8 @@ parse_community(const char *word, struct parse_result *r)
}
*p++ = 0;
- as = getcommunity(word);
- type = getcommunity(p);
+ as = getcommunity(word, 0);
+ type = getcommunity(p, 1);
done:
if (as == 0) {
@@ -994,10 +997,6 @@ done:
case COMMUNITY_BLACKHOLE:
/* valid */
break;
- default:
- /* unknown */
- fprintf(stderr, "Unknown well-known community\n");
- return (0);
}
if ((fs = calloc(1, sizeof(struct filter_set))) == NULL)
diff --git usr.sbin/bgpd/parse.y usr.sbin/bgpd/parse.y
index f0c96051e17..ec4ed956b60 100644
--- usr.sbin/bgpd/parse.y
+++ usr.sbin/bgpd/parse.y
@@ -146,7 +146,7 @@ void copy_filterset(struct filter_set_head
*,
voidmerge_filter_lists(struct filter_head *, struct filter_head *);
struct filter_rule *get_rule(enum action_types);
-int getcommunity(char *);
+int getcommunity(char *, int);
int parsecommunity(struct filter_community *, char *);
int64_t getlargecommunity(char *);
int parselargecommunity(struct filter_largecommunity *, char *);
@@ -2963,11 +2963,13 @@ symget(const char *nam)
}
int
-getcommunity(char *s)
+getcommunity(char *s, int part)
{
int val;
const char *errstr;
+ if (part == 0 && strcmp(s, "WELLKNOWN") == 0)
+ return (COMMUNITY_WELLKNOWN);
if (strcmp(s, "*") == 0)
return (COMMUNITY_ANY);
if (strcmp(s, "neighbor-as") == 0)
@@ -3017,15 +3019,11 @@ parsecommunity(struct filter_community *c, char *s)
}
*p++ = 0;
- if ((i = getcommunity(s)) == COMMUNITY_ERROR)
+ if ((i = getcommunity(s, 0)) == COMMUNITY_ERROR)
return (-1);
- if (i == COMMUNITY_WELLKNOWN) {
- yyerror("Bad community AS number");
- return (-1);
- }
as = i;
- if ((i = getcommunity(p)) == COMMUNITY_ERROR)
+ if ((i = getcommunity(p, 1)) == COMMUNITY_ERROR)
return (-1);
c->as = as;
c->type = i;
Re: [PATCH 2/3] openbgpd: Add support for 'unknown' well-known communities
On Sun, Jun 25, 2017 at 02:09:22PM +0200, Peter Hessler wrote:
> : $ bgpctl show rib community WELLKNOWN:0
> : ..
> : $ bgpctl show rib community WELLKNOWN:*
> : ..
>
> Eh, I don't really see a reason to have syntatic sugar for
> '65535'. In this case, I'm more likely to remember then number than
> which string to use ;).
>
> : $ doas cat /etc/bgpd.conf | grep set
> : match from any set { community WELLKNOWN:0 community 65535:1 }
>
> Same as before. OK for setting 65535:1, but 'E' for
> 'WELLKNOWN:0'.
>
> However, if we have one, then we need to have the other.
Current bgpd(8) on 6.1 uses the WELLKNOWN sugar in bgpctl output, i felt
it would be proper to allow it in bgpd.conf and bgpctl input too.
6.1 example:
[job@karen ~] $ bgpctl show rib detail 192.147.168.0/25
BGP routing table entry for 192.147.168.0/25
Nexthop 192.147.168.249 (via 192.147.168.249) from
192.147.168.249 (192.147.168.1)
Origin IGP, metric 0, localpref 100, weight 0, internal, valid,
best
Last update: 00:00:08 ago
Communities: WELLKNOWN:1
Kind regards,
Job
Re: [PATCH 2/3] openbgpd: Add support for 'unknown' well-known communities
On 2017 Jun 23 (Fri) at 16:01:58 +0200 (+0200), Job Snijders wrote:
:Dear team,
:
:This patch makes 'unknown' well-known communities more of a first-class
:citizen.
:
:A powerful property of well-known communities is that (often) operators
:can implement the feature associated with a given well-known community
:through their local routing policy, ahead of time before their vendor
:releasing native support in the implementation.
:
:Things that work now:
:
: $ bgpctl show rib community 65535:0
: ..
OK
: $ bgpctl show rib community WELLKNOWN:0
: ..
: $ bgpctl show rib community WELLKNOWN:*
: ..
:
Eh, I don't really see a reason to have syntatic sugar for '65535'.
In this case, I'm more likely to remember then number than which string
to use ;).
: $ doas cat /etc/bgpd.conf | grep set
: match from any set { community WELLKNOWN:0 community 65535:1 }
:
Same as before. OK for setting 65535:1, but 'E' for 'WELLKNOWN:0'.
However, if we have one, then we need to have the other.
:Kind regards,
:
:Job
:
:---
: usr.sbin/bgpctl/parser.c | 15 +++
: usr.sbin/bgpd/parse.y| 14 ++
: 2 files changed, 13 insertions(+), 16 deletions(-)
:
:diff --git a/usr.sbin/bgpctl/parser.c b/usr.sbin/bgpctl/parser.c
:index 85300d1cd32..0d1e5d9fb3a 100644
:--- a/usr.sbin/bgpctl/parser.c
:+++ b/usr.sbin/bgpctl/parser.c
:@@ -413,7 +413,7 @@ int parse_addr(const char *,
struct bgpd_addr *);
: intparse_asnum(const char *, size_t, u_int32_t *);
: intparse_number(const char *, struct parse_result *,
:enum token_type);
:-intgetcommunity(const char *);
:+intgetcommunity(const char *, int);
: intparse_community(const char *, struct parse_result *);
: u_int getlargecommunity(const char *);
: intparse_largecommunity(const char *, struct parse_result
*);
:@@ -927,7 +927,7 @@ parse_number(const char *word, struct parse_result *r,
enum token_type type)
: }
:
: int
:-getcommunity(const char *s)
:+getcommunity(const char *s, int msb)
: {
: const char *errstr;
: u_int16_tuval;
:@@ -935,6 +935,9 @@ getcommunity(const char *s)
: if (strcmp(s, "*") == 0)
: return (COMMUNITY_ANY);
:
:+ if (msb == 1 && strcmp(s, "WELLKNOWN") == 0)
:+ return (COMMUNITY_WELLKNOWN);
:+
: uval = strtonum(s, 0, USHRT_MAX, );
: if (errstr)
: errx(1, "Community is %s: %s", errstr, s);
:@@ -978,8 +981,8 @@ parse_community(const char *word, struct parse_result *r)
: }
: *p++ = 0;
:
:- as = getcommunity(word);
:- type = getcommunity(p);
:+ as = getcommunity(word, 1);
:+ type = getcommunity(p, 0);
:
: done:
: if (as == 0) {
:@@ -994,10 +997,6 @@ done:
: case COMMUNITY_BLACKHOLE:
: /* valid */
: break;
:- default:
:- /* unknown */
:- fprintf(stderr, "Unknown well-known community\n");
:- return (0);
: }
:
: if ((fs = calloc(1, sizeof(struct filter_set))) == NULL)
:diff --git a/usr.sbin/bgpd/parse.y b/usr.sbin/bgpd/parse.y
:index f0c96051e17..73bdb3a0cb9 100644
:--- a/usr.sbin/bgpd/parse.y
:+++ b/usr.sbin/bgpd/parse.y
:@@ -146,7 +146,7 @@ voidcopy_filterset(struct filter_set_head
*,
: void merge_filter_lists(struct filter_head *, struct filter_head *);
: struct filter_rule*get_rule(enum action_types);
:
:-intgetcommunity(char *);
:+intgetcommunity(char *, int);
: intparsecommunity(struct filter_community *, char *);
: int64_tgetlargecommunity(char *);
: intparselargecommunity(struct filter_largecommunity *, char *);
:@@ -2963,11 +2963,13 @@ symget(const char *nam)
: }
:
: int
:-getcommunity(char *s)
:+getcommunity(char *s, int msb)
: {
: int val;
: const char *errstr;
:
:+ if (msb == 1 && strcmp(s, "WELLKNOWN") == 0)
:+ return (COMMUNITY_WELLKNOWN);
: if (strcmp(s, "*") == 0)
: return (COMMUNITY_ANY);
: if (strcmp(s, "neighbor-as") == 0)
:@@ -3017,15 +3019,11 @@ parsecommunity(struct filter_community *c, char *s)
: }
: *p++ = 0;
:
:- if ((i = getcommunity(s)) == COMMUNITY_ERROR)
:+ if ((i = getcommunity(s, 1)) == COMMUNITY_ERROR)
: return (-1);
:- if (i == COMMUNITY_WELLKNOWN) {
:- yyerror("Bad community AS number");
:- return (-1);
:- }
: as = i;
:
:- if ((i = getcommunity(p)) == COMMUNITY_ERROR)
:+ if ((i = getcommunity(p, 0)) == COMMUNITY_ERROR)
: return (-1);
: c->as = as;
: c->type = i;
:
--
There is nothing wrong with
