diff --git a/CHANGELOG.md b/CHANGELOG.md index 41764d9..ead2782 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,7 +32,11 @@ follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - **Import from BIRD** offers a template per row, so adopted sessions arrive with the template's role, chains, limit and safeguards instead of bare identities. - Lint findings that are identical across peers of one template **fold into one - line** attributed to the template. + line** attributed to the template and naming the peers. + - A template's name has to be free across BIRD's single namespace: it may not + be a peer's, a prefix or AS set's, a community's or one birdy generates, and + a peer may not take a template's — a clash would fail every apply with + "Symbol already defined". - **Per-session BFD timers.** A peer (or template) with BFD on can set its own **interval** (ms) and **multiplier**, rendered as `bfd { interval …; multiplier …; };` on that session alone. BIRD's default 100 ms × 5 declares a session dead after @@ -51,6 +55,16 @@ 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. +- A peer rename that failed validation re-rendered the form posting to the new + name, so the corrected resubmit hit "not found" and the edit was lost. The form + now posts back to the name the peer is stored under. +- **Two objects with one BIRD name are caught before BIRD is.** BIRD keeps + defines, filters, functions, templates and protocols in one namespace, so a peer + and a prefix set (or community, AS set, RPKI server, BMP station) of the same + name failed `bird -p` with "Symbol already defined". The renderer now refuses + such a model with a message naming both, so Preview and Changes show it before + any apply. A library community also refuses every name birdy or BIRD defines + itself (`BOGON_ASNS`, `rpki4`, `master4`, …), not only the five it checked. - **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 diff --git a/docs/USAGE.md b/docs/USAGE.md index 1f9eb1b..b372275 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -517,7 +517,8 @@ until you have looked. deleted while a template chains it. A linked peer's protocol block in the rendered config carries a comment naming its template. On the Changes page, identical lint findings about peers of one template fold into a single line attributed to the - template — thirty peers missing an import limit is one thing to fix, once. + template and naming the peers — thirty peers missing an import limit is one thing + to fix, once. In the rendered config a template is BIRD's own `template bgp NAME { … }`, carrying the session options every linked peer shares — multihop, passive, BFD and its timers, GTSM, graceful restart, diff --git a/internal/render/lint.go b/internal/render/lint.go index b22aa3d..1534758 100644 --- a/internal/render/lint.go +++ b/internal/render/lint.go @@ -3,6 +3,7 @@ package render import ( "fmt" "net/netip" + "slices" "strings" "github.com/floreabogdan/birdy/internal/store" @@ -387,14 +388,25 @@ func foldByTemplate(ws []Warning, peers []store.Peer) []Warning { folded := map[int]Warning{} // first member's index -> the one finding that replaces the group drop := map[int]bool{} for k, idx := range members { - if len(idx) < 2 { + // Fold by distinct peer: one peer tripping the same check twice is not a + // pattern across the template. + var names []string + for _, i := range idx { + if !slices.Contains(names, ws[i].Peer) { + names = append(names, ws[i].Peer) + } + } + if len(names) < 2 { continue } - folded[idx[0]] = Warning{ - Severity: k.severity, - Peer: fmt.Sprintf("%s (%d peers)", k.template, len(idx)), - Message: k.message, + // The line still names its peers: the finding may be one the template + // cannot fix (a drain, a link-local neighbor), and the operator has to + // find the sessions either way. + label := k.template + ": " + strings.Join(names[:min(len(names), 5)], ", ") + if len(names) > 5 { + label += fmt.Sprintf(" and %d more", len(names)-5) } + folded[idx[0]] = Warning{Severity: k.severity, Peer: label, Message: k.message} for _, i := range idx[1:] { drop[i] = true } diff --git a/internal/render/peer_template_test.go b/internal/render/peer_template_test.go index 163a1c1..2604575 100644 --- a/internal/render/peer_template_test.go +++ b/internal/render/peer_template_test.go @@ -61,7 +61,7 @@ func TestLintFoldsIdenticalFindingsByTemplate(t *testing.T) { var folded, perPeer, ownLines, private int for _, w := range ws { switch { - case w.Peer == "IX_PEERS (3 peers)" && strings.HasPrefix(w.Message, "No import limit"): + case w.Peer == "IX_PEERS: rs1_v4, rs2_v4, rs3_v4" && strings.HasPrefix(w.Message, "No import limit"): folded++ case strings.HasPrefix(w.Peer, "rs") && strings.HasPrefix(w.Message, "No import limit"): perPeer++ @@ -264,3 +264,29 @@ func TestTemplateBlockCarriesBFDTimers(t *testing.T) { t.Errorf("a linked peer must leave BFD to the template:\n%s", blk) } } + +// A folded line still has to say which sessions it is about: the finding may be +// one the template cannot fix (a drain, a link-local neighbor), and the +// operator has to find the peers. It folds by distinct peer, so one peer that +// trips the same check twice is not "2 peers". +func TestFoldNamesThePeersAndCountsThemOnce(t *testing.T) { + var peers []store.Peer + for _, n := range []string{"rs1_v4", "rs2_v4", "rs3_v4", "rs4_v4", "rs5_v4", "rs6_v4", "rs7_v4"} { + peers = append(peers, store.Peer{Name: n, TemplateName: "IX_PEERS"}) + } + w := func(peer string) Warning { return Warning{Severity: "warning", Peer: peer, Message: "same thing"} } + + if got := foldByTemplate([]Warning{w("rs1_v4"), w("rs1_v4")}, peers); len(got) != 2 || got[0].Peer != "rs1_v4" { + t.Errorf("one peer's repeated finding must not fold as if it were two peers: %+v", got) + } + if got := foldByTemplate([]Warning{w("rs1_v4"), w("rs1_v4"), w("rs2_v4")}, peers); len(got) != 1 || got[0].Peer != "IX_PEERS: rs1_v4, rs2_v4" { + t.Errorf("two distinct peers should fold into one line naming both: %+v", got) + } + var all []Warning + for _, p := range peers { + all = append(all, w(p.Name)) + } + if got := foldByTemplate(all, peers); len(got) != 1 || got[0].Peer != "IX_PEERS: rs1_v4, rs2_v4, rs3_v4, rs4_v4, rs5_v4 and 2 more" { + t.Errorf("a long group should name the first five and count the rest: %+v", got) + } +} diff --git a/internal/render/render.go b/internal/render/render.go index 359f7c7..32d879f 100644 --- a/internal/render/render.go +++ b/internal/render/render.go @@ -273,6 +273,9 @@ func Sections(in Input) ([]Section, error) { if ferr != nil { return nil, ferr } + if err := checkSymbols(secs); err != nil { + return nil, err + } return secs, nil } diff --git a/internal/render/render_test.go b/internal/render/render_test.go index 7f1890b..cb9c608 100644 --- a/internal/render/render_test.go +++ b/internal/render/render_test.go @@ -1143,3 +1143,34 @@ func TestOnlyVetoRejectsCarryAMessage(t *testing.T) { } } } + +// BIRD keeps defines, functions, filters, templates and protocols in one +// namespace. Two of them sharing a name pass every per-object check but fail +// `bird -p` with "Symbol already defined"; the renderer is the one place that +// sees every symbol, so it refuses the model with a message naming both. +func TestRenderRefusesTwoSymbolsOfOneName(t *testing.T) { + in := baseInput() + p := ebgpPeer() + p.Name = "TRANSIT" + in.PrefixSets = append(bogonSets(), store.PrefixSet{ID: 40, Name: "TRANSIT", Family: store.FamilyV4, + Entries: []store.PrefixEntry{{Prefix: "192.0.2.0/24"}}}) + in.Peers = []store.Peer{p} + _, err := Config(in) + if err == nil || !strings.Contains(err.Error(), `"TRANSIT"`) || !strings.Contains(err.Error(), "define") || !strings.Contains(err.Error(), "protocol bgp") { + t.Fatalf("a peer and a prefix set of one name should be refused, naming both: %v", err) + } + + // BIRD's own implicit tables count too. + p.Name = "master4" + in.PrefixSets, in.Peers = bogonSets(), []store.Peer{p} + if _, err := Config(in); err == nil || !strings.Contains(err.Error(), "master4") { + t.Errorf("a peer named like BIRD's master4 table should be refused: %v", err) + } + + // Raw config is BIRD's to judge: a commented-out block there is not a clash. + p.Name = "edge_v4" + in.Peers, in.RawConfig = []store.Peer{p}, "/*\ndefine LOCAL_ASN = 1;\n*/" + if _, err := Config(in); err != nil { + t.Errorf("raw config must not be scanned for symbols: %v", err) + } +} diff --git a/internal/render/symbols.go b/internal/render/symbols.go new file mode 100644 index 0000000..b2ddde2 --- /dev/null +++ b/internal/render/symbols.go @@ -0,0 +1,41 @@ +package render + +import ( + "fmt" + "regexp" +) + +// symbolDecl matches a line that declares a BIRD symbol: a define, function, +// filter, BGP template, named protocol or table. Every writer starts these at +// the beginning of a line; a body line never does (it starts with a keyword +// such as "import" or "if", or a comment). +var symbolDecl = regexp.MustCompile(`(?m)^[ \t]*(define|function|filter|template bgp|protocol [a-z]+|roa[46] table|ipv[46] table)[ \t]+([A-Za-z_][A-Za-z0-9_]*)`) + +// birdSymbols are names BIRD defines on its own, before reading the file. +var birdSymbols = map[string]string{"master4": "BIRD's own master4 table", "master6": "BIRD's own master6 table"} + +// checkSymbols refuses a model whose rendered sections declare one name twice. +// BIRD keeps defines, functions, filters, templates and protocols in a single +// namespace, so such a config fails `bird -p` with "Symbol already defined" — +// every per-object check passes, and only here are all symbols in one place. +// Raw configuration is left to BIRD: it is free text, and a commented-out block +// there is not a clash. +func checkSymbols(secs []Section) error { + seen := map[string]string{} + for name, what := range birdSymbols { + seen[name] = what + } + for _, s := range secs { + if s.Path == "raw" { + continue + } + for _, m := range symbolDecl.FindAllStringSubmatch(s.Body, -1) { + what, name := m[1], m[2] + if first, dup := seen[name]; dup { + return fmt.Errorf("%q is declared twice, by %s and by %s: BIRD keeps every name in one namespace, so rename one of them", name, first, what) + } + seen[name] = what + } + } + return nil +} diff --git a/internal/store/communities.go b/internal/store/communities.go index 6ff572c..f63ed88 100644 --- a/internal/store/communities.go +++ b/internal/store/communities.go @@ -28,20 +28,14 @@ func (cd CommunityDef) Value() Community { // Pattern renders the value the way BIRD writes it, e.g. "(65535, 666)". func (cd CommunityDef) Pattern() string { return cd.Value().BIRD() } -// reservedSymbols are the define names birdy generates itself; a library -// community must not shadow one, or the rendered config has two defines of the -// same name. -var reservedSymbols = map[string]bool{ - "LOCAL_ASN": true, "FROM_UPSTREAM": true, "FROM_IX": true, - "FROM_CUSTOMER": true, "RPKI_INVALID": true, -} - // Validate checks the name and the community value, returning field-keyed errors. func (cd *CommunityDef) Validate() map[string]string { name, errs := validateNameDesc(cd.Name, cd.Description) cd.Name = name - if reservedSymbols[name] { - errs["name"] = name + " is a name birdy uses for a built-in define; pick another." + // A library community must not shadow a name birdy or BIRD defines itself, + // or the rendered config declares it twice. + if builtinSymbols[name] { + errs["name"] = name + " is a name birdy or BIRD defines itself; pick another." } parts := []int64{cd.A, cd.B} diff --git a/internal/store/communities_test.go b/internal/store/communities_test.go index d8495ed..493207a 100644 --- a/internal/store/communities_test.go +++ b/internal/store/communities_test.go @@ -17,3 +17,14 @@ func TestParseMatchCommunity(t *testing.T) { t.Error("out-of-range should be rejected") } } + +// A community renders `define NAME`, so it cannot take any name birdy defines +// itself — the same list the template check uses, not a shorter copy of it. +func TestCommunityCannotTakeABuiltinName(t *testing.T) { + for _, name := range []string{"LOCAL_ASN", "BOGON_ASNS", "rpki4", "master6"} { + cd := CommunityDef{Name: name, A: 65000, B: 1} + if _, bad := cd.Validate()["name"]; !bad { + t.Errorf("a community named %s should be refused", name) + } + } +} diff --git a/internal/store/peer_templates.go b/internal/store/peer_templates.go index 8d4aa7e..2f3f97b 100644 --- a/internal/store/peer_templates.go +++ b/internal/store/peer_templates.go @@ -180,14 +180,25 @@ func (s *Store) GetPeerTemplateByName(name string) (PeerTemplate, error) { } func (s *Store) getTemplate(query string, arg any) (PeerTemplate, error) { - t, err := scanTemplate(s.db.QueryRow(query, arg)) + return s.getTemplateIn(s.db, query, arg) +} + +// rowQuerier is querier plus single-row reads, so a template or a peer can be +// read inside the transaction that is about to write from it. +type rowQuerier interface { + querier + QueryRow(query string, args ...any) *sql.Row +} + +func (s *Store) getTemplateIn(q rowQuerier, query string, arg any) (PeerTemplate, error) { + t, err := scanTemplate(q.QueryRow(query, arg)) if err == sql.ErrNoRows { return PeerTemplate{}, ErrNotFound } if err != nil { return PeerTemplate{}, fmt.Errorf("store: get peer template: %w", err) } - if t.ImportPolicies, t.ExportPolicies, err = s.chainFor(s.db, "template_policies", "template_id", t.ID); err != nil { + if t.ImportPolicies, t.ExportPolicies, err = s.chainFor(q, "template_policies", "template_id", t.ID); err != nil { return PeerTemplate{}, err } return t, nil @@ -293,42 +304,71 @@ func (s *Store) UpdatePeerTemplate(t PeerTemplate, importIDs, exportIDs []int64) // template's shape over the peer's own. Overrides are cleared: a peer that // changes template takes the new template's word for everything. func (s *Store) LinkPeerToTemplate(peerID, templateID int64) error { - t, err := s.GetPeerTemplate(templateID) - if err != nil { - return err + if templateID == 0 { + return ErrNotFound // 0 is AttachPeers' "detach", never a template } - p, err := s.GetPeer(peerID) - if err != nil { - return err - } - p.TemplateOverrides = "" - t.ApplyTo(&p) + return s.AttachPeers([]int64{peerID}, templateID) +} + +// AttachPeers links every peer to a template — or, with templateID 0, detaches +// them — in one transaction. The template and each peer are read inside it, so +// a template save that commits meanwhile cannot leave a peer linked to the +// shape it replaced, and one peer that cannot be linked leaves the rest as +// they were rather than half the selection rewritten. +func (s *Store) AttachPeers(peerIDs []int64, templateID int64) error { tx, err := s.db.Begin() if err != nil { return err } defer tx.Rollback() - if err := updatePeerShape(tx, p); err != nil { - return err + var t PeerTemplate + if templateID != 0 { + // Write before reading: SQLite takes the write lock at the first write, + // and a transaction that read first cannot upgrade once another + // connection has committed since (SQLITE_BUSY_SNAPSHOT, which + // busy_timeout does not retry). Touching the template's own row takes + // the lock, waiting its turn, and proves the template exists. + res, err := tx.Exec(`UPDATE peer_templates SET updated_at = updated_at WHERE id = ?`, templateID) + if err != nil { + return fmt.Errorf("store: lock peer template: %w", err) + } + if err := affectedOne(res); err != nil { + return err + } + if t, err = s.getTemplateIn(tx, `SELECT `+templateCols+` FROM peer_templates WHERE id = ?`, templateID); err != nil { + return err + } } - if err := replaceChain(tx, "peer_policies", "peer_id", p.ID, PolicyIDs(t.ImportPolicies), PolicyIDs(t.ExportPolicies)); err != nil { - return err + for _, id := range peerIDs { + if templateID == 0 { + res, err := tx.Exec(`UPDATE peers SET template_id = NULL, template_overrides = '', updated_at = ? WHERE id = ?`, now(), id) + if err != nil { + return fmt.Errorf("store: detach peer: %w", err) + } + if err := affectedOne(res); err != nil { + return err + } + continue + } + p, err := scanPeer(tx.QueryRow(`SELECT `+peerCols+` FROM peers WHERE id = ?`, id)) + if err == sql.ErrNoRows { + return ErrNotFound + } + if err != nil { + return fmt.Errorf("store: get peer: %w", err) + } + p.TemplateOverrides = "" + t.ApplyTo(&p) + if err := updatePeerShape(tx, p); err != nil { + return err + } + if err := replaceChain(tx, "peer_policies", "peer_id", p.ID, PolicyIDs(t.ImportPolicies), PolicyIDs(t.ExportPolicies)); err != nil { + return err + } } return tx.Commit() } -// DetachPeer clears a peer's link and its overrides, leaving every value it -// inherited in place: the peer now owns the shape it had. The bulk "detach" -// on the peers list and nothing else goes through here; the form detaches by -// saving the peer with no template. -func (s *Store) DetachPeer(peerID int64) error { - res, err := s.db.Exec(`UPDATE peers SET template_id = NULL, template_overrides = '', updated_at = ? WHERE id = ?`, now(), peerID) - if err != nil { - return fmt.Errorf("store: detach peer: %w", err) - } - return affectedOne(res) -} - // PolicyIDs lists a chain's ids in order, for the chain writers. func PolicyIDs(chain []Policy) []int64 { out := make([]int64, 0, len(chain)) @@ -376,6 +416,37 @@ func (s *Store) TemplateUsage() (map[int64]int, error) { return out, rows.Err() } +// TemplatePolicyUsage counts, per policy id, the templates whose chain holds it. +func (s *Store) TemplatePolicyUsage() (map[int64]int, error) { + rows, err := s.db.Query(`SELECT policy_id, COUNT(*) FROM template_policies GROUP BY policy_id`) + if err != nil { + return nil, fmt.Errorf("store: template policy usage: %w", err) + } + defer rows.Close() + out := map[int64]int{} + for rows.Next() { + var id int64 + var n int + if err := rows.Scan(&id, &n); err != nil { + return nil, err + } + out[id] = n + } + return out, rows.Err() +} + +// TemplateInUseError is DeletePeerTemplate refusing a template that peers +// still link to — the one failure the operator fixes by detaching them. +type TemplateInUseError struct{ Peers []string } + +func (e *TemplateInUseError) Error() string { + shown := e.Peers + if len(shown) > 5 { + shown = append(shown[:5:5], "…") + } + return fmt.Sprintf("store: template is used by %d peer(s): %s", len(e.Peers), strings.Join(shown, ", ")) +} + // DeletePeerTemplate refuses while any peer still links to the template: // detaching them silently would leave thirty sessions owning a shape nobody // chose for them on purpose. The foreign key is the backstop; this is the @@ -386,11 +457,7 @@ func (s *Store) DeletePeerTemplate(id int64) error { return err } if len(users) > 0 { - shown := users - if len(shown) > 5 { - shown = append(shown[:5:5], "…") - } - return fmt.Errorf("store: template is used by %d peer(s): %s", len(users), strings.Join(shown, ", ")) + return &TemplateInUseError{Peers: users} } res, err := s.db.Exec(`DELETE FROM peer_templates WHERE id = ?`, id) if err != nil { diff --git a/internal/store/peer_templates_test.go b/internal/store/peer_templates_test.go index be626bb..92f0d72 100644 --- a/internal/store/peer_templates_test.go +++ b/internal/store/peer_templates_test.go @@ -2,11 +2,13 @@ package store import ( "database/sql" + "errors" "path/filepath" "reflect" "slices" "strings" "testing" + "time" ) // Every field of Peer is either identity — it stays the peer's own — or shape, @@ -376,6 +378,11 @@ func TestDeleteGuardsNameWhatStandsInTheWay(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "1 peer(s): edge_v4") { t.Errorf("deleting a linked template should refuse and name the peer, got %v", err) } + // Typed, so the UI can tell "detach these first" from any other failure. + var inUse *TemplateInUseError + if !errors.As(err, &inUse) || len(inUse.Peers) != 1 || inUse.Peers[0] != "edge_v4" { + t.Errorf("the refusal should be a TemplateInUseError naming edge_v4, got %#v", err) + } // NO_DEFAULT is in the template's chain and, through it, the peer's. err = s.DeletePolicy(noDefault.ID) if err == nil || !strings.Contains(err.Error(), "1 peer(s) and 1 peer template(s)") { @@ -599,3 +606,187 @@ func TestMigrateBFDTimersFromV38(t *testing.T) { t.Errorf("the template save should reach the linked peer: %+v", p) } } + +// BIRD keeps protocols, templates, defines and functions in one namespace, so +// a template named like a peer, a set or a community fails `bird -p` with +// "Symbol already defined" — after the save, on the next apply. SymbolUses is +// how a save finds out first. +func TestSymbolUsesFindsEveryOwnerOfAName(t *testing.T) { + s := openTest(t) + tmpl, _, _, _ := seedTemplateFixture(t, s) + p := validPeer() + p.Name = "TRANSIT" + if _, err := s.CreatePeer(p); err != nil { + t.Fatal(err) + } + + for _, tc := range []struct{ name, kind string }{ + {"TRANSIT", "peer"}, + {tmpl.Name, "peer template"}, + {"BOGONS_V4", "prefix set"}, + {"LOCAL_ASN", "built-in"}, + {"BOGON_ASNS", "built-in"}, + {"kernel4", "built-in"}, + {"master4", "built-in"}, + // Derived names clash only when what they derive from exists. + {"imp_IMPORT_SANITY_v4", "built-in"}, + {"ebgp_in_TRANSIT", "built-in"}, + {"originate_BOGONS_V4", "built-in"}, + } { + uses, err := s.SymbolUses(tc.name) + if err != nil { + t.Fatal(err) + } + if len(uses) != 1 || uses[0].Kind != tc.kind { + t.Errorf("%s: want one %s, got %+v", tc.name, tc.kind, uses) + } + } + for _, free := range []string{"FREE_NAME", "imp_NO_SUCH_POLICY_v4", "exp_transit", "ebgp_in_nobody", "originate_NOTHING"} { + if uses, err := s.SymbolUses(free); err != nil || len(uses) != 0 { + t.Errorf("%s should have no owner: %+v %v", free, uses, err) + } + } + if uses, _ := s.SymbolUses("TRANSIT"); uses[0].String() != `peer "TRANSIT"` { + t.Errorf("an owner should describe itself for the form error, got %q", uses[0].String()) + } +} + +// Drain is an eBGP signal; validateShape clears it when a peer is iBGP. A +// template can make a peer iBGP without the peer form ever running, so the +// shape write clears it too — or the iBGP filters would carry graceful +// shutdown and local-pref 0, from a switch the form then hides. +func TestTemplatePathsClearDrainWhenThePeerBecomesIBGP(t *testing.T) { + s := openTest(t) + ibgp := PeerTemplate{Name: "CORE", Role: RoleIBGP, ImportLimitAction: "restart"} + if errs := ibgp.Validate(); len(errs) != 0 { + t.Fatal(errs) + } + id, err := s.CreatePeerTemplate(ibgp, nil, nil) + if err != nil { + t.Fatal(err) + } + p := validPeer() + p.Drained = true + pid, err := s.CreatePeer(p) + if err != nil { + t.Fatal(err) + } + if err := s.LinkPeerToTemplate(pid, id); err != nil { + t.Fatal(err) + } + if got, _ := s.GetPeer(pid); got.Role != RoleIBGP || got.Drained { + t.Errorf("linking to an iBGP template should clear the drain: %+v", got) + } + + // The same through a template save that changes the role. + ix, sanity, _, _ := seedTemplateFixture(t, s) + q := validPeer() + q.Name, q.Drained = "rs1_v4", true + qid, err := s.CreatePeer(q) + if err != nil { + t.Fatal(err) + } + if err := s.LinkPeerToTemplate(qid, ix.ID); err != nil { + t.Fatal(err) + } + if got, _ := s.GetPeer(qid); !got.Drained { + t.Fatalf("an eBGP template should leave the drain alone: %+v", got) + } + ix.Role = RoleIBGP + if errs := ix.Validate(); len(errs) != 0 { + t.Fatal(errs) + } + if _, err := s.UpdatePeerTemplate(ix, []int64{sanity.ID}, nil); err != nil { + t.Fatal(err) + } + if got, _ := s.GetPeer(qid); got.Drained { + t.Errorf("a template turned iBGP should clear its peers' drain: %+v", got) + } +} + +// A bulk attach is all or nothing: one peer that cannot be linked leaves every +// other peer in the request as it was, instead of half the selection rewritten. +func TestAttachPeersIsAllOrNothing(t *testing.T) { + s := openTest(t) + tmpl, _, _, _ := seedTemplateFixture(t, s) + var ids []int64 + for _, name := range []string{"rs1_v4", "rs2_v4"} { + p := validPeer() + p.Name, p.ImportLimit = name, 7 + id, err := s.CreatePeer(p) + if err != nil { + t.Fatal(err) + } + ids = append(ids, id) + } + + if err := s.AttachPeers([]int64{ids[0], 9999}, tmpl.ID); err != ErrNotFound { + t.Fatalf("attaching a missing peer should be ErrNotFound, got %v", err) + } + if p, _ := s.GetPeer(ids[0]); p.TemplateID.Valid || p.ImportLimit != 7 { + t.Errorf("a failed bulk attach must not have linked anyone: %+v", p) + } + + if err := s.AttachPeers(ids, tmpl.ID); err != nil { + t.Fatal(err) + } + for _, id := range ids { + if p, _ := s.GetPeer(id); !p.TemplateID.Valid || p.ImportLimit != 50000 { + t.Errorf("both peers should be linked with the template's shape: %+v", p) + } + } + if err := s.AttachPeers(ids, 0); err != nil { + t.Fatal(err) + } + for _, id := range ids { + if p, _ := s.GetPeer(id); p.TemplateID.Valid || p.ImportLimit != 50000 { + t.Errorf("detaching keeps the values and drops the link: %+v", p) + } + } + if err := s.AttachPeers(ids, 9999); err != ErrNotFound { + t.Errorf("attaching to a missing template should be ErrNotFound, got %v", err) + } +} + +// Everything AttachPeers reads goes through its transaction. A read through the +// pool while the transaction holds a connection waits for a second one, and +// with the pool exhausted that wait never ends. +func TestAttachPeersReadsOnlyThroughItsTransaction(t *testing.T) { + s := openTest(t) + tmpl, _, _, _ := seedTemplateFixture(t, s) // its chain has an export policy + id, err := s.CreatePeer(validPeer()) + if err != nil { + t.Fatal(err) + } + s.db.SetMaxOpenConns(1) + done := make(chan error, 1) + go func() { done <- s.AttachPeers([]int64{id}, tmpl.ID) }() + select { + case err := <-done: + if err != nil { + t.Fatal(err) + } + case <-time.After(5 * time.Second): + t.Fatal("AttachPeers blocked on a second connection while holding its transaction") + } +} + +// Template 0 means "detach" only to AttachPeers, which says so. Linking to an +// unset template is a caller's mistake, not a detach. +func TestLinkingToTemplateZeroIsNotADetach(t *testing.T) { + s := openTest(t) + tmpl, _, _, _ := seedTemplateFixture(t, s) + id, err := s.CreatePeer(validPeer()) + if err != nil { + t.Fatal(err) + } + if err := s.LinkPeerToTemplate(id, tmpl.ID); err != nil { + t.Fatal(err) + } + if err := s.LinkPeerToTemplate(id, 0); err != ErrNotFound { + t.Errorf("linking to template 0 should be ErrNotFound, got %v", err) + } + if p, _ := s.GetPeer(id); !p.TemplateID.Valid { + t.Error("a failed link must leave the peer linked as it was") + } +} diff --git a/internal/store/peers.go b/internal/store/peers.go index a754f8a..dd848e9 100644 --- a/internal/store/peers.go +++ b/internal/store/peers.go @@ -499,18 +499,26 @@ func (s *Store) UpdatePeer(p Peer) error { // and the link itself. It is how a template save and a link reach into a peer // without touching its identity, password or operational state, and it runs // inside the caller's transaction so thirty peers change together or not at all. +// +// The one exception to "shape only" is the drain. It is the peer's own switch, +// but an eBGP one: validateShape clears it on an iBGP peer, and a template can +// make a peer iBGP without the peer form ever running, so the shape write +// clears it too rather than leave graceful shutdown on an internal session. func updatePeerShape(tx *sql.Tx, p Peer) error { + if p.IsIBGP() { + p.Drained = false + } res, err := tx.Exec(` UPDATE peers SET role = ?, multihop = ?, passive = ?, import_limit = ?, import_limit_action = ?, import_communities = ?, export_communities = ?, prepend_count = ?, enforce_first_as = ?, origin_peer_only = ?, bgp_role = ?, gtsm = ?, bfd = ?, bfd_interval = ?, bfd_multiplier = ?, graceful_restart = ?, - next_hop_self = ?, rr_client = ?, ibgp_export_default = ?, + next_hop_self = ?, rr_client = ?, ibgp_export_default = ?, drained = ?, template_id = ?, template_overrides = ?, updated_at = ? WHERE id = ?`, p.Role, p.Multihop, p.Passive, p.ImportLimit, p.ImportLimitAction, p.ImportCommunities, p.ExportCommunities, p.PrependCount, p.EnforceFirstAS, p.OriginPeerOnly, p.BGPRole, p.GTSM, p.BFD, p.BFDInterval, p.BFDMultiplier, p.GracefulRestart, - p.NextHopSelf, p.RRClient, p.IBGPExportDefault, + p.NextHopSelf, p.RRClient, p.IBGPExportDefault, p.Drained, p.TemplateID, p.TemplateOverrides, now(), p.ID) if err != nil { return fmt.Errorf("store: update peer shape: %w", err) diff --git a/internal/store/policies.go b/internal/store/policies.go index 6025690..6fd486b 100644 --- a/internal/store/policies.go +++ b/internal/store/policies.go @@ -212,7 +212,7 @@ func (s *Store) ListPolicies() ([]Policy, error) { if out[i].IsImport() { continue } - ids, err := s.policySetIDs(out[i].ID) + ids, err := s.policySetIDs(s.db, out[i].ID) if err != nil { return nil, err } @@ -231,15 +231,16 @@ func (s *Store) GetPolicyByName(name string) (Policy, error) { return Policy{}, fmt.Errorf("store: get policy: %w", err) } if !p.IsImport() { - if p.SetIDs, err = s.policySetIDs(p.ID); err != nil { + if p.SetIDs, err = s.policySetIDs(s.db, p.ID); err != nil { return Policy{}, err } } return p, nil } -func (s *Store) policySetIDs(policyID int64) ([]int64, error) { - rows, err := s.db.Query(`SELECT set_id FROM policy_prefix_sets WHERE policy_id = ? ORDER BY position, set_id`, policyID) +// policySetIDs reads through q, so a chain read inside a transaction stays in it. +func (s *Store) policySetIDs(q querier, policyID int64) ([]int64, error) { + rows, err := q.Query(`SELECT set_id FROM policy_prefix_sets WHERE policy_id = ? ORDER BY position, set_id`, policyID) if err != nil { return nil, fmt.Errorf("store: policy prefix sets: %w", err) } @@ -396,7 +397,7 @@ func (s *Store) chainFor(q querier, table, key string, id int64) (imports, expor return nil, nil, err } for i := range exports { - if exports[i].SetIDs, err = s.policySetIDs(exports[i].ID); err != nil { + if exports[i].SetIDs, err = s.policySetIDs(q, exports[i].ID); err != nil { return nil, nil, err } } diff --git a/internal/store/symbols.go b/internal/store/symbols.go new file mode 100644 index 0000000..a5a14e9 --- /dev/null +++ b/internal/store/symbols.go @@ -0,0 +1,98 @@ +package store + +import ( + "database/sql" + "fmt" + "regexp" +) + +// SymbolUse is one thing in the model that renders a BIRD symbol of a given +// name. BIRD keeps protocols, templates, defines, functions and filters in a +// single namespace, so two of them sharing a name fail `bird -p` with "Symbol +// already defined" — the whole config, not just the one block. +type SymbolUse struct { + Kind string // "peer", "peer template", "prefix set", "AS set", "community", "RPKI server", "BMP station" or "built-in" + ID int64 // 0 for a built-in + Name string +} + +func (u SymbolUse) String() string { + if u.Kind == "built-in" { + return "a name birdy generates itself" + } + return fmt.Sprintf("%s %q", u.Kind, u.Name) +} + +// builtinSymbols are the names birdy renders on its own, outside any table, +// and the tables BIRD defines before reading the file. +var builtinSymbols = map[string]bool{ + "LOCAL_ASN": true, "FROM_UPSTREAM": true, "FROM_IX": true, "FROM_CUSTOMER": true, "RPKI_INVALID": true, + "BOGON_ASNS": true, "BOGON_ASNS_EXCEPT_PRIVATE": true, + "rpki4": true, "rpki6": true, + "device1": true, "direct1": true, "kernel4": true, "kernel6": true, "bfd1": true, + "static_v4": true, "static_v6": true, + "master4": true, "master6": true, +} + +// derivedSymbols are the names birdy builds from another object's name — +// policy functions, per-peer filters, originator protocols. Such a name is +// taken only while the object it derives from exists. +var derivedSymbols = []struct { + pattern *regexp.Regexp + table string +}{ + {regexp.MustCompile(`^(?:imp|exp)_(.+)_v[46]$`), "policies"}, + {regexp.MustCompile(`^(?:ebgp|ibgp)_(?:in|out)_(.+)$`), "peers"}, + {regexp.MustCompile(`^originate_(.+)$`), "prefix_sets"}, +} + +// symbolTables are the tables whose rows each render a symbol named by their +// name column. +var symbolTables = []struct{ table, kind string }{ + {"peers", "peer"}, + {"peer_templates", "peer template"}, + {"prefix_sets", "prefix set"}, + {"as_sets", "AS set"}, + {"communities", "community"}, + {"rpki_servers", "RPKI server"}, + {"bmp_stations", "BMP station"}, +} + +// SymbolUses lists everything that already renders a BIRD symbol called name, +// so a save can refuse a name the config could not load with. The caller +// ignores its own row when renaming in place. +func (s *Store) SymbolUses(name string) ([]SymbolUse, error) { + var out []SymbolUse + if builtinSymbols[name] { + out = append(out, SymbolUse{Kind: "built-in", Name: name}) + } + for _, d := range derivedSymbols { + m := d.pattern.FindStringSubmatch(name) + if m == nil { + continue + } + var one int + // The table name comes from the fixed list above, never from input. + err := s.db.QueryRow(`SELECT 1 FROM `+d.table+` WHERE name = ?`, m[1]).Scan(&one) + if err == nil { + out = append(out, SymbolUse{Kind: "built-in", Name: name}) + break + } + if err != sql.ErrNoRows { + return nil, fmt.Errorf("store: symbol uses in %s: %w", d.table, err) + } + } + for _, st := range symbolTables { + var id int64 + // The table name comes from the fixed list above, never from input. + err := s.db.QueryRow(`SELECT id FROM `+st.table+` WHERE name = ?`, name).Scan(&id) + if err == nil { + out = append(out, SymbolUse{Kind: st.kind, ID: id, Name: name}) + continue + } + if err != sql.ErrNoRows { + return nil, fmt.Errorf("store: symbol uses in %s: %w", st.table, err) + } + } + return out, nil +} diff --git a/internal/web/assets.go b/internal/web/assets.go index 97bfddc..c6fad2e 100644 --- a/internal/web/assets.go +++ b/internal/web/assets.go @@ -117,6 +117,10 @@ func (s *Server) handleASSetSave(w http.ResponseWriter, r *http.Request) { } errs := as.Validate() + if err := s.refuseTemplateName(as.Name, errs); err != nil { + s.serverError(w, "check name", err) + return + } for k, v := range entryErrs { errs[k] = v } diff --git a/internal/web/bmp.go b/internal/web/bmp.go index 1eea196..cfd7563 100644 --- a/internal/web/bmp.go +++ b/internal/web/bmp.go @@ -86,6 +86,10 @@ func (s *Server) handleBMPSave(w http.ResponseWriter, r *http.Request) { } errs := st.Validate() + if err := s.refuseTemplateName(st.Name, errs); err != nil { + s.serverError(w, "check name", err) + return + } if len(errs) == 0 { var err error if isNew { diff --git a/internal/web/communities_handler_test.go b/internal/web/communities_handler_test.go index d30deb4..aa7f05b 100644 --- a/internal/web/communities_handler_test.go +++ b/internal/web/communities_handler_test.go @@ -106,7 +106,7 @@ func TestCommunityValidation(t *testing.T) { env := applyReady(t) body := env.do(t, "POST", "/library/communities/new", url.Values{"name": {"FROM_UPSTREAM"}, "value": {"65551:1"}}).Body.String() - if !strings.Contains(body, "built-in define") { + if !strings.Contains(body, "a name birdy or BIRD defines itself") { t.Error("a reserved name should be rejected") } if _, err := env.store.GetCommunityDefByName("FROM_UPSTREAM"); err != store.ErrNotFound { diff --git a/internal/web/helpers.go b/internal/web/helpers.go index d226743..653e366 100644 --- a/internal/web/helpers.go +++ b/internal/web/helpers.go @@ -146,3 +146,37 @@ func tabParam(r *http.Request, allowed ...string) string { func isUniqueViolation(err error) bool { return err != nil && strings.Contains(err.Error(), "UNIQUE constraint failed") } + +// symbolClash returns the form error for a name the config could not load +// with: BIRD keeps protocols, templates and defines in one namespace, so a +// second owner fails `bird -p` with "Symbol already defined". counts picks +// which existing owners matter to this save; "" means the name is free. +func (s *Server) symbolClash(name string, counts func(store.SymbolUse) bool) (string, error) { + uses, err := s.store.SymbolUses(name) + if err != nil { + return "", err + } + for _, u := range uses { + if counts(u) { + return "BIRD keeps every name in one namespace, and " + u.String() + " already uses this name.", nil + } + } + return "", nil +} + +// refuseTemplateName records a name error when a peer template already uses +// the name: the object being saved would render a second BIRD symbol of it. +// A name the form already rejected is left with that error. +func (s *Server) refuseTemplateName(name string, errs map[string]string) error { + if _, taken := errs["name"]; taken { + return nil + } + msg, err := s.symbolClash(name, func(u store.SymbolUse) bool { return u.Kind == "peer template" }) + if err != nil { + return err + } + if msg != "" { + errs["name"] = msg + } + return nil +} diff --git a/internal/web/library.go b/internal/web/library.go index 8298582..b598423 100644 --- a/internal/web/library.go +++ b/internal/web/library.go @@ -124,6 +124,10 @@ func (s *Server) handlePrefixSetSave(w http.ResponseWriter, r *http.Request) { } errs := ps.Validate() + if err := s.refuseTemplateName(ps.Name, errs); err != nil { + s.serverError(w, "check name", err) + return + } if len(errs) == 0 { var err error if isNew { diff --git a/internal/web/library_communities.go b/internal/web/library_communities.go index b4237df..2dfd2ab 100644 --- a/internal/web/library_communities.go +++ b/internal/web/library_communities.go @@ -92,6 +92,10 @@ func (s *Server) handleCommunitySave(w http.ResponseWriter, r *http.Request) { } errs := cd.Validate() + if err := s.refuseTemplateName(cd.Name, errs); err != nil { + s.serverError(w, "check name", err) + return + } if valueErr != "" { errs["value"] = valueErr } @@ -131,9 +135,15 @@ func (s *Server) handleCommunityDelete(w http.ResponseWriter, r *http.Request) { if !ok { return } - // Refuse to delete a community a peer or policy still references by name, or - // the next render would emit an undefined symbol. - if users, err := s.communityInUse(cd.Name); err == nil && len(users) > 0 { + // Refuse to delete a community a peer, template or policy still references + // by name, or the next render would emit an undefined symbol. If the check + // itself fails, refuse too: deleting blind is how that happens. + users, err := s.communityInUse(cd.Name) + if err != nil { + s.serverError(w, "check community use", err) + return + } + if len(users) > 0 { s.flashRedirect(w, r, "/library/communities", "Could not delete "+cd.Name+": still used by "+strings.Join(users, ", "), false) return } @@ -173,7 +183,9 @@ func (s *Server) checkCommunityRefs(text string) string { return "Unknown community: " + strings.Join(missing, ", ") + ". Define it under Library → Communities." } -// communityInUse lists the peers and policies that reference a community by name. +// communityInUse lists the peers, peer templates and policies that reference a +// community by name. A template counts even with no peers linked: attaching one +// later copies its references onto the peer. func (s *Server) communityInUse(name string) ([]string, error) { var users []string peers, err := s.store.ListPeers() @@ -188,6 +200,16 @@ func (s *Server) communityInUse(name string) ([]string, error) { users = append(users, "peer "+p.Name) } } + templates, err := s.store.ListPeerTemplates() + if err != nil { + return nil, err + } + for _, t := range templates { + if slices.Contains(store.NamedCommunityRefs(t.ExportCommunities), name) || + slices.Contains(store.NamedCommunityRefs(t.ImportCommunities), name) { + users = append(users, "peer template "+t.Name) + } + } policies, err := s.store.ListPolicies() if err != nil { return nil, err diff --git a/internal/web/peer_templates.go b/internal/web/peer_templates.go index 37d27c9..49b4257 100644 --- a/internal/web/peer_templates.go +++ b/internal/web/peer_templates.go @@ -2,6 +2,7 @@ package web import ( "database/sql" + "errors" "fmt" "net/http" "strings" @@ -186,6 +187,19 @@ func (s *Server) handlePeerTemplateSave(w http.ResponseWriter, r *http.Request) if msg := s.checkCommunityRefs(t.ImportCommunities); msg != "" { errs["importCommunities"] = msg } + // A template renders `template bgp NAME`, so its name must not already be a + // peer, a set, a community or anything birdy generates. Another template + // with it is the unique index's to report. + if _, taken := errs["name"]; !taken { + msg, err := s.symbolClash(t.Name, func(u store.SymbolUse) bool { return u.Kind != "peer template" }) + if err != nil { + s.serverError(w, "check template name", err) + return + } + if msg != "" { + errs["name"] = msg + } + } var rewrote int if len(errs) == 0 { @@ -240,7 +254,7 @@ func (s *Server) handlePeerTemplateSave(w http.ResponseWriter, r *http.Request) t.ImportPolicies, t.ExportPolicies = s.resolvePolicies(all, importIDs), s.resolvePolicies(all, exportIDs) } v := s.templateFormView(t, isNew) - v.Errs, v.LinkSource = errs, r.FormValue("from") + v.Errs, v.LinkSource, v.StoredName = errs, r.FormValue("from"), r.PathValue("name") s.renderPeerForm(w, v) } @@ -250,8 +264,13 @@ func (s *Server) handlePeerTemplateDelete(w http.ResponseWriter, r *http.Request return } if err := s.store.DeletePeerTemplate(t.ID); err != nil { - msg := strings.TrimPrefix(err.Error(), "store: ") - s.flashRedirect(w, r, "/peers/templates", "Could not delete "+t.Name+": "+msg+". Detach those peers first (choose \"none\" as their template).", true) + msg := "Could not delete " + t.Name + ": " + strings.TrimPrefix(err.Error(), "store: ") + "." + // Only a template peers still link to is fixed by detaching them. + var inUse *store.TemplateInUseError + if errors.As(err, &inUse) { + msg += " Detach those peers first (choose \"none\" as their template)." + } + s.flashRedirect(w, r, "/peers/templates", msg, true) return } s.audit(r, "deleted peer template "+t.Name) @@ -269,7 +288,7 @@ func (s *Server) handlePeerTemplatePreview(w http.ResponseWriter, r *http.Reques t := templateFromForm(r) t.ImportPolicies = s.resolvePolicies(policiesAll, idList(r.Form["importPolicyIds"])) t.ExportPolicies = s.resolvePolicies(policiesAll, idList(r.Form["exportPolicyIds"])) - sample := samplePeer(t) + sample := samplePeer(t, s.localASN()) preview, previewErr, warnings, err := s.previewWithLibrary(sample, policiesAll, []store.PeerTemplate{t}) if err != nil { writeJSON(w, previewResp{Err: "could not load the library"}) @@ -294,14 +313,29 @@ func attributeToTemplate(ws []birdconf.Warning, sampleName, templateName string) // preview renders, declared "from" the template so the preview shows exactly // what a linked peer's block will look like. It is named after the template // with an _example suffix: a protocol cannot share the template's own name. -func samplePeer(t store.PeerTemplate) store.Peer { +func samplePeer(t store.PeerTemplate, localASN int64) store.Peer { if t.Name == "" { t.Name = "template" } p := displayPeer(t) p.NeighborIP, p.RemoteASN = sampleNeighborIP, sampleRemoteASN - p.Name = t.Name + "_example" + // An internal session's neighbor is in our own AS; anything else renders an + // eBGP session and trips the "marked iBGP" lint on the template's own sample. + if t.IsIBGP() && localASN > 0 { + p.RemoteASN = localASN + } + // Named after the template, within BIRD's 63-character limit. + const suffix = "_example" + p.Name = t.Name[:min(len(t.Name), 63-len(suffix))] + suffix // A template being created has no id yet; the preview links to it by name. p.TemplateID, p.TemplateName = sql.NullInt64{Int64: max(t.ID, 1), Valid: true}, t.Name return p } + +// localASN is our own AS from settings, or 0 before the router identity is set. +func (s *Server) localASN() int64 { + if st, ok, err := s.store.GetSettings(); err == nil && ok && st.LocalASN.Valid { + return st.LocalASN.Int64 + } + return 0 +} diff --git a/internal/web/peer_templates_followups_test.go b/internal/web/peer_templates_followups_test.go index dd93db7..0d6e7b1 100644 --- a/internal/web/peer_templates_followups_test.go +++ b/internal/web/peer_templates_followups_test.go @@ -143,7 +143,9 @@ func TestBulkAttachAndDetach(t *testing.T) { } } body := env.do(t, "GET", "/peers", nil).Body.String() - for _, want := range []string{`id="bulk-attach"`, `name="peer" value="rs1_v4" form="bulk-attach"`, `Attach to IX_PEERS`, `data-check-all`} { + for _, want := range []string{`id="bulk-attach"`, `name="peer" value="rs1_v4" form="bulk-attach"`, `Attach to IX_PEERS`, `data-check-all`, + // No action is preselected: "Apply" with the default must not attach anything. + `{{end}}
diff --git a/internal/web/templates/peers.html b/internal/web/templates/peers.html index 5da2567..5cf3e20 100644 --- a/internal/web/templates/peers.html +++ b/internal/web/templates/peers.html @@ -104,9 +104,10 @@ per-row toggle forms rule out wrapping it. */}}
none selected - + {{range .Templates}}{{end}} - + Attaching gives each selected peer the template's chains, limit and safeguards; detaching leaves every value in place and makes the peer own it.{{if not .Templates}} No templates yet — create one.{{end}} diff --git a/internal/web/templates/policies.html b/internal/web/templates/policies.html index 6753975..f801740 100644 --- a/internal/web/templates/policies.html +++ b/internal/web/templates/policies.html @@ -61,7 +61,7 @@ {{if eq .BogonASNs "all"}}no bogon ASNs {{else if eq .BogonASNs "except_private"}}private ASNs ok{{end}} - {{$n := index $.InUse .ID}}{{if $n}}{{$n}} peer{{if ne $n 1}}s{{end}}{{else}}nothing{{end}} + {{$n := index $.InUse .ID}}{{$tn := index $.TemplateUse .ID}}{{if $n}}{{$n}} peer{{if ne $n 1}}s{{end}}{{end}}{{if $tn}}{{if $n}} {{end}}{{$tn}} template{{if ne $tn 1}}s{{end}}{{end}}{{if not (or $n $tn)}}nothing{{end}} {{template "icon-edit"}} @@ -105,7 +105,7 @@ {{if .AnnounceFromCustomer}}from customers{{end}} {{end}} - {{$n := index $.InUse .ID}}{{if $n}}{{$n}} peer{{if ne $n 1}}s{{end}}{{else}}nothing{{end}} + {{$n := index $.InUse .ID}}{{$tn := index $.TemplateUse .ID}}{{if $n}}{{$n}} peer{{if ne $n 1}}s{{end}}{{end}}{{if $tn}}{{if $n}} {{end}}{{$tn}} template{{if ne $tn 1}}s{{end}}{{end}}{{if not (or $n $tn)}}nothing{{end}} {{template "icon-edit"}}