diff --git a/CHANGELOG.md b/CHANGELOG.md index ef3cb77..e1a9cb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +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. +- **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 4c5b0dd..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 @@ -1287,7 +1292,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` 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 729043f..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,8 +337,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# 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 + // 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) } } @@ -383,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) } } @@ -1032,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) + } + } +}