Generate prefix-set instead of prefix macro

ledeuns commented 5 years ago

With OpenBGPd, prefix-sets are faster and are not limited in length. Be aware it breaks compatibility with current deployments though.

snar commented 5 years ago

On Wed, Sep 05, 2018 at 01:55:15AM -0700, Denis Fondras wrote:

With OpenBGPd, prefix-sets are faster and are not limited in length. Be aware it breaks compatibility with current deployments though.

Breaking changes will not be accepted, sorry: you're not the only person running bgpq3 with OpenBGPD and I do not like idea of ruining someone else operations.

On the other hand, I think that repurposing -E flag to generate prefix-sets instead of prefix-lists will be non-breaking, so this approach is implemented and pushed to github.

Can you please check that:

it does not break your current (previous) setup with prefix-lists.
with added flag -E it generates correct configuration for prefix-sets (according to documentation there shall be ',' separator between prefix entries, so I added this separator too).
empty prefix-sets behave in the same way as empty prefix-lists. If empty prefix-sets are accepted as "deny all" filters (just like it works on JunOS, for example), I shall further modify bgpq3 to remove warning.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

You can view, comment on, or merge this pull request online at:

https://github.com/snar/bgpq3/pull/40

Commit Summary

• Generate prefix-set instead of prefix macro

File Changes

• M bgpq3_printer.c (12)

Patch Links:

• https://github.com/snar/bgpq3/pull/40.patch • https://github.com/snar/bgpq3/pull/40.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.*

ledeuns commented 5 years ago

Thank you. (1) It does not break current setup wrt prefix-lists. (2) Since https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.sbin/bgpd/parse.y.diff?r1=1.337&r2=1.338&f=h you should remove \ on the first line. (3) Empty prefix-set (prefix-set NN { }) is a syntax error.

ledeuns commented 5 years ago

Speaking of commas : https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/etc/examples/bgpd.conf?rev=1.12&content-type=text/x-cvsweb-markup

ledeuns commented 5 years ago

This generates configuration suitable for openbgpd-current (no commas and no '\' on prefix-sets) :

index 7346f8f..f01fe0f 100644
--- a/bgpq3_printer.c
+++ b/bgpq3_printer.c
@@ -685,7 +685,6 @@ void
 bgpq3_print_openbgpd_prefix(struct sx_radix_node* n, void* ff)
 {
        char prefix[128];
-       int  pc = needscomma & prefixset;
        FILE* f=(FILE*)ff;
        if(n->isGlue)
                goto checkSon;
@@ -693,18 +692,17 @@ bgpq3_print_openbgpd_prefix(struct sx_radix_node* n, void* ff)
                f=stdout;
        sx_prefix_snprintf(&n->prefix, prefix, sizeof(prefix));
        if (!n->isAggregate) {
-               fprintf(f, "%s\n\t%s", pc ? "," : " \\", prefix);
+               fprintf(f, "%s\n\t%s", prefixset ? "" : " \\", prefix);
        } else if (n->aggregateLow == n->aggregateHi) {
                fprintf(f, "%s\n\t%s prefixlen = %u",
-                       pc ? "," : " \\", prefix, n->aggregateHi);
+                       prefixset ? "" : " \\", prefix, n->aggregateHi);
        } else if (n->aggregateLow > n->prefix.masklen) {
                fprintf(f, "%s\n\t%s prefixlen %u - %u",
-                       pc ? "," : " \\", prefix, n->aggregateLow, n->aggregateHi);
+                       prefixset ? "" : " \\", prefix, n->aggregateLow, n->aggregateHi);
        } else {
                fprintf(f, "%s\n\t%s prefixlen %u - %u",
-                       pc ? "," : " \\", prefix, n->prefix.masklen, n->aggregateHi);
+                       prefixset ? "," : " \\", prefix, n->prefix.masklen, n->aggregateHi);
        };
-       needscomma=1;
 checkSon:
        if(n->son)
                bgpq3_print_openbgpd_prefix(n->son, ff);

snar commented 5 years ago

On Fri, Sep 07, 2018 at 02:17:26PM -0700, Denis Fondras wrote:

Thank you. (1) It does not break current setup wrt prefix-lists. (2) -E yields invalid prefix-set. \ is missing after the prefix. (3) Empty prefix-set (prefix-set NN { }) is a syntax error.

Thanks for checking. Hope, this time I dit it right.

(2) can be fixed with something like : diff --git a/bgpq3_printer.c b/bgpq3_printer.c index 7346f8f..93c3415 100644 --- a/bgpq3_printer.c +++ b/bgpq3_printer.c @@ -693,16 +693,16 @@ bgpq3_print_openbgpd_prefix(struct sx_radix_node n, void ff) f=stdout; sx_prefix_snprintf(&n->prefix, prefix, sizeof(prefix)); if (!n->isAggregate) {

• fprintf(f, "%s\n\t%s", pc ? "," : " \", prefix);

• fprintf(f, "%s\n\t%s", pc ? ", \" : " \", prefix); } else if (n->aggregateLow == n->aggregateHi) { fprintf(f, "%s\n\t%s prefixlen = %u",

• pc ? "," : " \", prefix, n->aggregateHi);

• pc ? ", \" : " \", prefix, n->aggregateHi); } else if (n->aggregateLow > n->prefix.masklen) { fprintf(f, "%s\n\t%s prefixlen %u - %u",

• pc ? "," : " \", prefix, n->aggregateLow, n->aggregateHi);

• pc ? ", \" : " \", prefix, n->aggregateLow, n->aggregateHi); } else { fprintf(f, "%s\n\t%s prefixlen %u - %u",

• pc ? "," : " \", prefix, n->prefix.masklen, n->aggregateHi);

• pc ? ", \" : " \", prefix, n->prefix.masklen, n->aggregateHi); }; needscomma=1;

checkSon: @@ -1043,7 +1043,7 @@ bgpq3_print_openbgpd_prefixset(FILE f, struct bgpq_expander b) fprintf(f,"prefix-set %s { ", b->name); prefixset=1; sx_radix_tree_foreach(b->tree,bgpq3_print_openbgpd_prefix,f);

• fprintf(f, "\n\t}\n");

• fprintf(f, " \\n\t}\n"); } else { fprintf(f, "deny from AS %u\n", b->asnumber); };

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.*

snar / bgpq3

Generate prefix-set instead of prefix macro #40