-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Community #18
Community #18
Conversation
…er grain control of communities.
…sements with communities.
scripts/peering-prefix
Outdated
to multiple peers, but each peer will only see their | ||
respective community (e.g.: -c 1 means Peer 1 receives | ||
(47065,1)). [default: announce to all peers only if no | ||
communities attached] (see https://peering.usc.edu/peers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As these peer-id communities are filtered in BIRD, should we not mention the limit of 20 communities here and mention it only in the explanation of -C
? In other words, -c
will contribute at most one community after filtering, so the limit of 20 communities really applies to -C
.
scripts/peering-prefix
Outdated
|
||
-c id Attach community (47065,id) to the announcement. This will | ||
let the announcement through the peer with the given id | ||
(ids are < 1024). May be added multiple times to announce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Discussion:) I am in favor of bumping peer IDs to to 2048 to be extra future proof.
scripts/peering-prefix
Outdated
@@ -60,6 +66,7 @@ usage () { # {{{ | |||
Usage: peering prefix announce|withdraw [-m mux] | |||
[-p poison | [-P prepend] [-o origin]] | |||
[-c id1] ... [-c idN] | |||
[-C id1,id2] ... [-C idX,idY] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly write [-C as1,c1] ... [-C asY,cY]
. And rewrite the previous line as ... [-c idX]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid confusion, as in -c
one writes peer IDs, and in -C
one writes one of our AS numbers and arbitrary community values.
scripts/peering-prefix
Outdated
advertised. If no communities in the form of | ||
(47065,id<=1024) are included, announce to all peers. | ||
|
||
-C id1,id2 Attach community (id1,id2) to the announcement. If the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(as1,c1)
scripts/peering-prefix
Outdated
community is in the form of (47065,id2) the effect is | ||
identical to the -c option. Similarly, if any communities | ||
are in the form (47065,id2<=1024) the mux will only | ||
advertise to the peer with id2. May be added multiple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add: "and other peer IDs also added in the announcement" (-c
can be used multiple times).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to check in the bash script that -C
is not used with community values smaller than 1024. Probably easier to explain that -c
will add a special community (47065, id) to the announcement; and that these communities are reserved and cannot be used in -C
.
Changes made. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small changes. @cunha is the bash expert so he can comment on that side.
scripts/peering-prefix
Outdated
communities attached] (see https://peering.usc.edu/peers | ||
for the list of peers). Can be combined with the -C. | ||
|
||
-C as1,c2 Attach community (id1,id2) to the announcement. If the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You call it as1,c2
in the short hand, but then refer to them as id1
and id2
in the long description. In particular when you note "Only valid PEERING... are allowed for id1"
scripts/peering-prefix
Outdated
added multiple times to announce to multiple peers. | ||
|
||
Only valid PEERING 16-bit AS numbers are allowed for id1; | ||
the only exception is for the well-known communities: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't clear -- can I use any value for id1
as long as id2
is in set (65535, 65281 - 65284)
?
C) | ||
for comm in $(echo $OPTARG | tr "," "\n") | ||
do | ||
if [[ $comm -gt 65535 || $comm -lt 1 ]] ; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we only allow 16 bit numbers for these? Does BIRD not support BGP extended communities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think BIRD supports extended communities, but given it is a bit of a mess in the wide-area Internet, I would be in favor of starting with 16-bit communities only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BIRD supports extended communities, but I chose to only implement the 16 bit version for the first version of the feature.
Ok. That's acceptable
…On Oct 19, 2017 7:00 AM, "twabulldogg" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In scripts/peering-prefix
<#18 (comment)>:
> @@ -109,6 +138,15 @@ c)
fi
communities+=($OPTARG)
;;
+C)
+ for comm in $(echo $OPTARG | tr "," "\n")
+ do
+ if [[ $comm -gt 65535 || $comm -lt 1 ]] ; then
BIRD supports extended communities, but I chose to only implement the 16
bit version for the first version of the feature.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#18 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA_tN5fC7YC0SVmZUIhP3LIx0Du9YRvtks5st1XfgaJpZM4PkdXI>
.
|
…ier to understand.
No description provided.