From 9df4ffd5f655683746642e220f20c7948b37c4d6 Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Wed, 30 Sep 2026 16:31:22 +0300 Subject: [PATCH 1/2] fix(render): stop logging every route the export catch-all drops Every generated export filter ended in `reject "not permitted by any export policy"`, and BIRD logs a reject's message once per route. On a full-table router that line drops nearly the whole table on each export to an upstream or iBGP peer, so each session (re)establishment and routine churn floods syslog. One production router logged 9.5 million such lines in three days, and journald's rate limiter then dropped the BGP/BFD session events alongside them. The catch-all is now a bare `reject;`. The targeted rejects inside policy functions keep their reasons; they fire rarely and say something useful. `show route noexport`, behind the peer page's "Rejected on export" tab, still answers which routes were withheld. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 +++++++++ internal/render/render.go | 5 ++++- internal/render/render_test.go | 11 +++++++++-- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef3cb77..34e70fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,15 @@ follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - The peer form's role-specific checkbox group for the *other* role (the iBGP switches on an eBGP peer, and vice versa) was visible, greyed out, instead of hidden: the group's flex layout overrode the `hidden` attribute. +- **Export filters no longer log every route they drop.** The catch-all at the end + of each generated export filter was `reject "not permitted by any export + policy"`, and BIRD logs a reject's message once per route. On a full-table + router that is nearly the whole table on every export to every upstream and iBGP + peer, again on each session re-establishment and on routine table churn — + millions of syslog lines a day, enough for journald to rate-limit `bird.service` + and drop the session events worth reading. It is now a bare `reject;`. Which + routes an export withholds is still one click away under a peer's *Rejected on + export* tab (`show route noexport`). ## [0.5.0] - 2026-07-22 diff --git a/internal/render/render.go b/internal/render/render.go index 4c5b0dd..c21e972 100644 --- a/internal/render/render.go +++ b/internal/render/render.go @@ -1287,7 +1287,10 @@ func writePeerExportFilter(b *strings.Builder, in Input, p store.Peer, fam famil for _, pol := range p.ExportPolicies { fmt.Fprintf(b, "\t%s;\n", policyFunc(pol, fam)) } - b.WriteString("\treject \"not permitted by any export policy\";\n}\n\n") + // A bare reject: BIRD logs a reject's message for every route it drops, and + // this line drops nearly the whole table on every export to an upstream or + // iBGP peer. `show route noexport` answers "why was this not announced". + b.WriteString("\treject;\n}\n\n") } // IsPrivateASN reports whether asn falls in one of the ranges the operator has diff --git a/internal/render/render_test.go b/internal/render/render_test.go index 729043f..13ddd90 100644 --- a/internal/render/render_test.go +++ b/internal/render/render_test.go @@ -336,8 +336,15 @@ func TestExportChainEndsInReject(t *testing.T) { if first < 0 || second < 0 || first > second { t.Errorf("export policies must be called in attachment order:\n%s", f) } - if !strings.Contains(f, `reject "not permitted by any export policy";`) { - t.Error("the export filter must reject anything no policy accepted") + if !strings.HasSuffix(f, "\n\treject;") { + t.Errorf("the export filter must end by rejecting anything no policy accepted:\n%s", f) + } + // Without a message: BIRD logs a reject's text for every route it drops, and + // on a full-table router the catch-all drops nearly every route on every + // export — millions of syslog lines, enough for journald to start + // suppressing the ones that matter. + if strings.Contains(f, `reject "`) { + t.Errorf("the catch-all reject must not log a message per route:\n%s", f) } } From 9f7bd00207231798ce6cbdac981294a40a8abb7a Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Wed, 30 Sep 2026 17:16:01 +0300 Subject: [PATCH 2/2] fix(render): keep every bulk catch-all reject out of the log Review of the export fix found the same flood on the import side: an allow-list (`not in SET`), the default-only rule, an origin AS set, the origin-only check, and the fail-closed rejects for a disabled or other-family allow-list all rejected everything not explicitly allowed with a message, and BIRD logs that message once per route. USAGE even recommends an allow-list import on iBGP, where the far end sends a full table. These catch-alls are now bare `reject;` with the reason as a config comment, as is the export catch-all. Vetoes (bogon, RPKI invalid, AS-path, prefix length, first AS) keep their messages: they name one thing wrong with a route. TestOnlyVetoRejectsCarryAMessage renders every catch-all and fails on any messaged reject not on the veto list, so a new one has to be added on purpose. The code comment and CHANGELOG no longer claim `show route noexport` says why a route was withheld (it lists which), and the CHANGELOG notes the change lands on the next apply. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 22 ++++--- internal/render/origin_test.go | 4 +- internal/render/prefixset_disable_test.go | 2 +- internal/render/render.go | 21 ++++--- internal/render/render_test.go | 71 +++++++++++++++++++++-- 5 files changed, 96 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34e70fb..e1a9cb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,15 +43,19 @@ follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - The peer form's role-specific checkbox group for the *other* role (the iBGP switches on an eBGP peer, and vice versa) was visible, greyed out, instead of hidden: the group's flex layout overrode the `hidden` attribute. -- **Export filters no longer log every route they drop.** The catch-all at the end - of each generated export filter was `reject "not permitted by any export - policy"`, and BIRD logs a reject's message once per route. On a full-table - router that is nearly the whole table on every export to every upstream and iBGP - peer, again on each session re-establishment and on routine table churn — - millions of syslog lines a day, enough for journald to rate-limit `bird.service` - and drop the session events worth reading. It is now a bare `reject;`. Which - routes an export withholds is still one click away under a peer's *Rejected on - export* tab (`show route noexport`). +- **Generated filters no longer log every route a catch-all drops.** BIRD logs a + reject's message once per route it drops. Export filters ended in + `reject "not permitted by any export policy"`, and import policies rejected + everything outside their allow-list, default-only rule, origin AS set or + origin-only check with a message too. On a full-table router such a catch-all + matches nearly the whole table on every session start and every UPDATE — + millions of syslog lines a day, enough for journald to rate-limit + `bird.service` and drop the session events worth reading. Catch-alls are now a + bare `reject;` with the reason kept as a config comment; vetoes (bogon, RPKI + invalid, AS-path, prefix-length and first-AS checks) still log why. A peer's + *Rejected on export* tab (`show route noexport`) still lists what was withheld. + The quieter filters take effect on the next apply, so after upgrading every + install with policies shows unapplied changes until then. ## [0.5.0] - 2026-07-22 diff --git a/internal/render/origin_test.go b/internal/render/origin_test.go index 4b23722..6d78f4f 100644 --- a/internal/render/origin_test.go +++ b/internal/render/origin_test.go @@ -32,7 +32,7 @@ func TestOriginPeerOnlyRendersOnTheSession(t *testing.T) { in.PrefixSets, in.Policies, in.Peers = bogonSets(), []store.Policy{sanityPolicy()}, []store.Peer{p} f := block(t, mustRender(t, in), "filter ebgp_in_edge_v4") - if !strings.Contains(f, `if bgp_path.last != 64600 then reject "prefix not originated by this peer";`) { + if !strings.Contains(f, "\tif bgp_path.last != 64600 then reject;\t# not originated by this peer\n") { t.Errorf("origin-peer-only guard missing:\n%s", f) } // It must land on the peer's filter, not inside a shared policy function: @@ -65,7 +65,7 @@ func TestOriginASSetRendersInThePolicy(t *testing.T) { } // It applies to both families: an ASN has no address family. for _, fn := range []string{"function imp_IMPORT_SANITY_v4()", "function imp_IMPORT_SANITY_v6()"} { - if !strings.Contains(block(t, out, fn), `if ! (bgp_path.last ~ AS_CUSTOMER_A) then reject "origin AS not in AS_CUSTOMER_A";`) { + if !strings.Contains(block(t, out, fn), "\tif ! (bgp_path.last ~ AS_CUSTOMER_A) then reject;\t# origin AS not in AS_CUSTOMER_A\n") { t.Errorf("%s should filter the origin AS", fn) } } diff --git a/internal/render/prefixset_disable_test.go b/internal/render/prefixset_disable_test.go index 34c9ddb..798cd51 100644 --- a/internal/render/prefixset_disable_test.go +++ b/internal/render/prefixset_disable_test.go @@ -93,7 +93,7 @@ func TestDisabledImportAllowListFailsClosed(t *testing.T) { if strings.Contains(fn, "! (net ~ CUST_IN)") { t.Errorf("a disabled allow-list must not render its membership check:\n%s", fn) } - if !strings.Contains(fn, `reject "CUST_IN is disabled`) { + if !strings.Contains(fn, "CUST_IN is disabled, so this policy permits nothing here.\n\treject;") { t.Errorf("a disabled allow-list must fail closed (reject all):\n%s", fn) } } diff --git a/internal/render/render.go b/internal/render/render.go index c21e972..e01cb96 100644 --- a/internal/render/render.go +++ b/internal/render/render.go @@ -756,13 +756,13 @@ func writeImportBody(b *strings.Builder, in Input, pol store.Policy, fam family, // permit nothing — rather than dropping the check, which would turn // "accept only these prefixes" into "accept anything" and leak the table. fmt.Fprintf(b, "\t# %s is disabled, so this policy permits nothing here.\n", ps.Name) - fmt.Fprintf(b, "\treject \"%s is disabled; no prefixes are permitted\";\n", ps.Name) + b.WriteString("\treject;\n") return nil case len(ps.Entries) == 0: return fmt.Errorf("accept-only prefix set %q is empty", ps.Name) case familyOf(ps) != fam.suffix: fmt.Fprintf(b, "\t# %s is %s, so this policy permits nothing here.\n", ps.Name, ps.Family) - fmt.Fprintf(b, "\treject \"no %s prefixes are permitted by this policy\";\n", fam.channel) + b.WriteString("\treject;\n") return nil } } @@ -790,11 +790,16 @@ func writeImportBody(b *strings.Builder, in Input, pol store.Policy, fam family, fmt.Fprintf(b, "\tif %s then {\n\t\tdest = RTD_BLACKHOLE;\n\t\taccept;\n\t}\n", cond) } + // Rejects that drop everything not explicitly allowed stay bare, with the + // reason as a comment: BIRD logs a reject's message once per route, and a + // catch-all can match most of a full table on every session start and + // every UPDATE. Only vetoes — one specific thing wrong with a route — say + // why in the log. switch pol.DefaultRoute { case store.DefaultReject: fmt.Fprintf(b, "\tif net = %s then reject \"default route not accepted\";\n", fam.anyRoute) case store.DefaultOnly: - fmt.Fprintf(b, "\tif net != %s then reject \"only the default route is accepted\";\n", fam.anyRoute) + fmt.Fprintf(b, "\tif net != %s then reject;\t# only the default route is accepted\n", fam.anyRoute) case store.DefaultAccept: fmt.Fprintf(b, "\t# the default route is accepted like any other prefix\n") } @@ -833,7 +838,7 @@ func writeImportBody(b *strings.Builder, in Input, pol store.Policy, fam family, } if pol.AcceptOnlySetID.Valid { name := sets[pol.AcceptOnlySetID.Int64].Name - fmt.Fprintf(b, "\tif ! (net ~ %s) then reject \"not in %s\";\n", name, name) + fmt.Fprintf(b, "\tif ! (net ~ %s) then reject;\t# not in %s\n", name, name) } // bgp_path.last is the AS that originated the route. Restricting it to the // members of an expanded IRR AS-SET is how a transit provider says "announce @@ -846,7 +851,7 @@ func writeImportBody(b *strings.Builder, in Input, pol store.Policy, fam family, if len(as.Entries) == 0 { return fmt.Errorf("origin AS set %q is empty", as.Name) } - fmt.Fprintf(b, "\tif ! (bgp_path.last ~ %s) then reject \"origin AS not in %s\";\n", as.Name, as.Name) + fmt.Fprintf(b, "\tif ! (bgp_path.last ~ %s) then reject;\t# origin AS not in %s\n", as.Name, as.Name) } // RPKI route-origin validation. roa_check compares the route's origin AS // against the ROAs its prefix holder published. Invalid means the origin is @@ -1206,7 +1211,7 @@ func writePeerImportFilter(b *strings.Builder, in Input, p store.Peer, fam famil // AS must be the peer itself. Prepending still works, because the origin is // the last ASN in the path, not the first. if p.OriginPeerOnly { - fmt.Fprintf(b, "\tif bgp_path.last != %d then reject \"prefix not originated by this peer\";\n", p.RemoteASN) + fmt.Fprintf(b, "\tif bgp_path.last != %d then reject;\t# not originated by this peer\n", p.RemoteASN) } if p.IsIBGP() { // The opposite of the eBGP rule below, and the reason iBGP gets its own @@ -1289,8 +1294,8 @@ func writePeerExportFilter(b *strings.Builder, in Input, p store.Peer, fam famil } // A bare reject: BIRD logs a reject's message for every route it drops, and // this line drops nearly the whole table on every export to an upstream or - // iBGP peer. `show route noexport` answers "why was this not announced". - b.WriteString("\treject;\n}\n\n") + // iBGP peer. `show route noexport` still lists what was withheld. + b.WriteString("\treject;\t# not permitted by any export policy\n}\n\n") } // IsPrivateASN reports whether asn falls in one of the ranges the operator has diff --git a/internal/render/render_test.go b/internal/render/render_test.go index 13ddd90..7356bfa 100644 --- a/internal/render/render_test.go +++ b/internal/render/render_test.go @@ -2,6 +2,7 @@ package render import ( "database/sql" + "regexp" "strings" "testing" "time" @@ -155,7 +156,7 @@ func TestDefaultRouteModes(t *testing.T) { in := baseInput() in.PrefixSets, in.Policies = sets, []store.Policy{only} fn := block(t, mustRender(t, in), "function imp_P_v4()") - if !strings.Contains(fn, `if net != 0.0.0.0/0 then reject "only the default route is accepted";`) { + if !strings.Contains(fn, "\tif net != 0.0.0.0/0 then reject;\t# only the default route is accepted") { t.Errorf("accept-only-default must reject everything else:\n%s", fn) } @@ -336,7 +337,7 @@ func TestExportChainEndsInReject(t *testing.T) { if first < 0 || second < 0 || first > second { t.Errorf("export policies must be called in attachment order:\n%s", f) } - if !strings.HasSuffix(f, "\n\treject;") { + if !strings.HasSuffix(f, "\n\treject;\t# not permitted by any export policy") { t.Errorf("the export filter must end by rejecting anything no policy accepted:\n%s", f) } // Without a message: BIRD logs a reject's text for every route it drops, and @@ -390,11 +391,11 @@ func TestAcceptOnlySetRejectsTheOtherFamily(t *testing.T) { in.Policies = []store.Policy{pol} out := mustRender(t, in) - if v4 := block(t, out, "function imp_CUST_v4()"); !strings.Contains(v4, `if ! (net ~ CUST_A_V4) then reject "not in CUST_A_V4";`) { + if v4 := block(t, out, "function imp_CUST_v4()"); !strings.Contains(v4, "\tif ! (net ~ CUST_A_V4) then reject;\t# not in CUST_A_V4") { t.Errorf("v4 function should permit only the set:\n%s", v4) } v6 := block(t, out, "function imp_CUST_v6()") - if !strings.Contains(v6, `reject "no ipv6 prefixes are permitted by this policy";`) { + if !strings.Contains(v6, "so this policy permits nothing here.\n\treject;") { t.Errorf("v6 function must reject everything, not silently accept:\n%s", v6) } } @@ -1039,3 +1040,65 @@ func TestBlackholeKeepsOriginTag(t *testing.T) { t.Errorf("origin tag must be added BEFORE the policy call so a blackhole accept keeps it:\n%s", f) } } + +// BIRD logs a reject's message once for every route it drops. For a veto — one +// specific thing wrong with this route — that is worth it: such routes are rare +// and the line says why. A catch-all that drops everything not explicitly +// allowed can match most of a full table on every session start and every +// UPDATE, and its log lines crowd out the session events. Catch-alls stay +// bare, with the reason as a config comment. A new messaged reject must be +// added to the veto list here on purpose. +func TestOnlyVetoRejectsCarryAMessage(t *testing.T) { + vetoes := map[string]bool{ + "default route not accepted": true, "prefix length out of bounds": true, "bogon prefix": true, + "RPKI invalid": true, "AS path too long": true, "our own ASN in AS path": true, + "bogon ASN in AS path": true, "first AS is not the peer AS": true, + } + + cust := store.PrefixSet{ID: 30, Name: "CUST_A_V4", Family: store.FamilyV4, + Entries: []store.PrefixEntry{{Prefix: "198.51.100.0/24", Modifier: "+"}}} + off := store.PrefixSet{ID: 31, Name: "OFF_V4", Family: store.FamilyV4, Disabled: true, + Entries: []store.PrefixEntry{{Prefix: "203.0.113.0/24"}}} + as := custASSet() + onlyDefault := store.Policy{ID: 10, Name: "ONLY_DEFAULT", Direction: store.DirImport, + DefaultRoute: store.DefaultOnly, BogonASNs: store.BogonASNsOff} + allow := store.Policy{ID: 11, Name: "CUST", Direction: store.DirImport, DefaultRoute: store.DefaultReject, + BogonASNs: store.BogonASNsOff, AcceptOnlySetID: sql.NullInt64{Int64: cust.ID, Valid: true}, + OriginASSetID: sql.NullInt64{Int64: as.ID, Valid: true}} + disabled := store.Policy{ID: 12, Name: "OFF", Direction: store.DirImport, DefaultRoute: store.DefaultReject, + BogonASNs: store.BogonASNsOff, AcceptOnlySetID: sql.NullInt64{Int64: off.ID, Valid: true}} + export := store.Policy{ID: 13, Name: "EXPORT_CUST", Direction: store.DirExport, AnnounceFromCustomer: true} + + p := ebgpPeer() + p.EnforceFirstAS, p.OriginPeerOnly = true, true + p.ImportPolicies = []store.Policy{sanityPolicy(), allow} + p.ExportPolicies = []store.Policy{export} + + in := baseInput() + in.PrefixSets = append(bogonSets(), cust, off) + in.ASSets = []store.ASSet{as} + in.Policies = []store.Policy{sanityPolicy(), onlyDefault, allow, disabled, export} + in.Peers = []store.Peer{p} + out := mustRender(t, in) + + msg := regexp.MustCompile(`reject "([^"]*)"`) + for _, m := range msg.FindAllStringSubmatch(out, -1) { + if !vetoes[m[1]] { + t.Errorf("reject %q logs once per route it drops; make it a bare reject with a comment, or add it to the vetoes if it is one", m[1]) + } + } + // And the catch-alls are all there, bare — the test covers what it claims to. + for _, want := range []string{ + "then reject;\t# only the default route is accepted\n", + "then reject;\t# not in CUST_A_V4\n", + "then reject;\t# origin AS not in AS_CUSTOMER_A\n", + "then reject;\t# not originated by this peer\n", + "\treject;\t# not permitted by any export policy\n", + "OFF_V4 is disabled, so this policy permits nothing here.\n\treject;\n", + "CUST_A_V4 is ipv4, so this policy permits nothing here.\n\treject;\n", + } { + if !strings.Contains(out, want) { + t.Errorf("expected the bare catch-all %q in:\n%s", want, out) + } + } +}