From af861d87726e4b14f77e2bed7e4689462506a06d Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Thu, 1 Oct 2026 10:23:54 +0300 Subject: [PATCH 01/19] fix(templates): refuse a template name BIRD already has a symbol for BIRD keeps protocols, templates, defines, functions and filters in one namespace. A template named like a peer (`template bgp TRANSIT` beside `protocol bgp TRANSIT`), a prefix set, a community, an RPKI server or anything birdy generates saved without complaint, then failed every apply with "Symbol 'TRANSIT' already defined". store.SymbolUses lists every owner of a name: the tables whose rows render a symbol, the built-ins birdy renders itself, and the prefixes of derived names (imp_, ebgp_in_, originate_, ...). A template save refuses a name any of them holds, and a peer save refuses a template's name, so the clash cannot be built from either side. Co-Authored-By: Claude Opus 5.5 --- internal/store/peer_templates_test.go | 39 +++++++++++ internal/store/symbols.go | 78 +++++++++++++++++++++ internal/web/helpers.go | 17 +++++ internal/web/peer_templates.go | 13 ++++ internal/web/peer_templates_handler_test.go | 42 +++++++++++ internal/web/peers.go | 12 ++++ 6 files changed, 201 insertions(+) create mode 100644 internal/store/symbols.go diff --git a/internal/store/peer_templates_test.go b/internal/store/peer_templates_test.go index be626bb..cba9d48 100644 --- a/internal/store/peer_templates_test.go +++ b/internal/store/peer_templates_test.go @@ -599,3 +599,42 @@ 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"}, + {"imp_FOO_v4", "built-in"}, + {"ebgp_in_edge_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) + } + } + if uses, err := s.SymbolUses("FREE_NAME"); err != nil || len(uses) != 0 { + t.Errorf("an unused name should have no owner: %+v %v", 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()) + } +} diff --git a/internal/store/symbols.go b/internal/store/symbols.go new file mode 100644 index 0000000..78b7af3 --- /dev/null +++ b/internal/store/symbols.go @@ -0,0 +1,78 @@ +package store + +import ( + "database/sql" + "fmt" + "strings" +) + +// 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. +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, +} + +// generatedSymbolPrefixes start the names birdy derives from other objects: +// policy functions, per-peer filters, originator protocols. +var generatedSymbolPrefixes = []string{"imp_", "exp_", "ebgp_in_", "ebgp_out_", "ibgp_in_", "ibgp_out_", "originate_"} + +// 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 _, prefix := range generatedSymbolPrefixes { + if strings.HasPrefix(name, prefix) { + out = append(out, SymbolUse{Kind: "built-in", Name: name}) + break + } + } + 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/helpers.go b/internal/web/helpers.go index d226743..555e3d8 100644 --- a/internal/web/helpers.go +++ b/internal/web/helpers.go @@ -146,3 +146,20 @@ 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 +} diff --git a/internal/web/peer_templates.go b/internal/web/peer_templates.go index 37d27c9..0015ae7 100644 --- a/internal/web/peer_templates.go +++ b/internal/web/peer_templates.go @@ -186,6 +186,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 { diff --git a/internal/web/peer_templates_handler_test.go b/internal/web/peer_templates_handler_test.go index c913fdc..2cb9e39 100644 --- a/internal/web/peer_templates_handler_test.go +++ b/internal/web/peer_templates_handler_test.go @@ -437,3 +437,45 @@ func TestPeerTemplateBFDTimersShowInListAndPreview(t *testing.T) { t.Error("the template preview should render the BFD timers") } } + +// BIRD has one namespace for protocols, templates and defines; a clash only +// surfaced as "Symbol already defined" on the next apply. Both saves refuse it. +func TestTemplateAndPeerNamesCannotShareABIRDSymbol(t *testing.T) { + env := newTestEnv(t, false) + withIdentity(t, env) + peer := peerForm() + peer.Set("name", "TRANSIT") + if rec := env.do(t, "POST", "/peers/new", peer); rec.Code != http.StatusSeeOther { + t.Fatalf("peer create: %d %s", rec.Code, rec.Body) + } + + for _, clash := range []struct{ name, owner string }{{"TRANSIT", "peer"}, {"BOGONS_V4", "prefix set"}, {"LOCAL_ASN", "birdy generates"}} { + form := templateForm(env, t) + form.Set("name", clash.name) + rec := env.do(t, "POST", "/peers/templates/new", form) + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "already uses this name") || !strings.Contains(rec.Body.String(), clash.owner) { + t.Errorf("template %s should be refused for clashing with a %s: %d", clash.name, clash.owner, rec.Code) + } + if _, err := env.store.GetPeerTemplateByName(clash.name); err == nil { + t.Errorf("template %s must not be stored", clash.name) + } + } + + // Saving a template under its own name is not a clash with itself. + tmpl := createTemplate(env, t) + form := templateForm(env, t) + form.Set("importLimit", "60000") + if rec := env.do(t, "POST", "/peers/templates/"+tmpl.Name+"/edit", form); rec.Code != http.StatusSeeOther { + t.Errorf("re-saving a template under its own name: %d", rec.Code) + } + + // And from the other side: a peer cannot take a template's name. + peer.Set("name", tmpl.Name) + rec := env.do(t, "POST", "/peers/new", peer) + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "already uses this name") { + t.Errorf("a peer named like a template should be refused: %d", rec.Code) + } + if _, err := env.store.GetPeerByName(tmpl.Name); err == nil { + t.Error("the clashing peer must not be stored") + } +} diff --git a/internal/web/peers.go b/internal/web/peers.go index 36d31b3..f0f4c7e 100644 --- a/internal/web/peers.go +++ b/internal/web/peers.go @@ -300,6 +300,18 @@ func (s *Server) handlePeerSave(w http.ResponseWriter, r *http.Request) { if msg := s.checkCommunityRefs(p.ImportCommunities); msg != "" { errs["importCommunities"] = msg } + // Another peer with the name is the unique index's to report; a template + // with it would be a second `bgp` symbol of the same name. + if _, taken := errs["name"]; !taken { + msg, err := s.symbolClash(p.Name, func(u store.SymbolUse) bool { return u.Kind == "peer template" }) + if err != nil { + s.serverError(w, "check peer name", err) + return + } + if msg != "" { + errs["name"] = msg + } + } if len(errs) == 0 { var err error From 0e3f5407ed26f97b5ba0c39e7c64e9961dd1c4f3 Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Thu, 1 Oct 2026 10:24:33 +0300 Subject: [PATCH 02/19] fix(templates): a community a template references is in use The delete guard scanned peers and policies but not templates, so a community named only by a template (say one with no peers linked yet) could be deleted. Every peer attached to that template afterwards then referenced an undefined symbol: danger findings on Changes, `bird -p` failing the apply, and a template that would not save until someone recreated the community. Templates now count as users. Co-Authored-By: Claude Opus 5.5 --- internal/web/library_communities.go | 14 +++++++++++- internal/web/peer_templates_handler_test.go | 25 +++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/internal/web/library_communities.go b/internal/web/library_communities.go index b4237df..9014e26 100644 --- a/internal/web/library_communities.go +++ b/internal/web/library_communities.go @@ -173,7 +173,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 +190,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_handler_test.go b/internal/web/peer_templates_handler_test.go index 2cb9e39..5e339d1 100644 --- a/internal/web/peer_templates_handler_test.go +++ b/internal/web/peer_templates_handler_test.go @@ -479,3 +479,28 @@ func TestTemplateAndPeerNamesCannotShareABIRDSymbol(t *testing.T) { t.Error("the clashing peer must not be stored") } } + +// A template's community references are copied onto every peer linked later, +// so a community only a template names is still in use: deleting it would +// make the next attach render an undefined symbol. +func TestCommunityUsedOnlyByATemplateCannotBeDeleted(t *testing.T) { + env := newTestEnv(t, false) + withIdentity(t, env) + if rec := env.do(t, "POST", "/library/communities/new", url.Values{"name": {"NO_EXPORT_X"}, "value": {"65000:7"}}); rec.Code != http.StatusSeeOther { + t.Fatalf("community create: %d %s", rec.Code, rec.Body) + } + form := templateForm(env, t) + form.Set("exportCommunities", "NO_EXPORT_X") + if rec := env.do(t, "POST", "/peers/templates/new", form); rec.Code != http.StatusSeeOther { + t.Fatalf("template create: %d %s", rec.Code, rec.Body) + } + + env.do(t, "POST", "/library/communities/NO_EXPORT_X/delete", nil) + if _, err := env.store.GetCommunityDefByName("NO_EXPORT_X"); err != nil { + t.Errorf("a community a template references must not be deleted: %v", err) + } + users, err := env.srv.communityInUse("NO_EXPORT_X") + if err != nil || len(users) != 1 || users[0] != "peer template IX_PEERS" { + t.Errorf("the template should be named as the user: %v %v", users, err) + } +} From 51baf441eb121052487a0d697dfe9eccf99d44d2 Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Thu, 1 Oct 2026 10:25:03 +0300 Subject: [PATCH 03/19] fix(templates): clear the drain when a template makes a peer iBGP validateShape clears Drained on an iBGP peer, but a template save or a link rewrites a peer's shape without the peer form running. A drained eBGP peer attached to an iBGP template, or linked to a template whose role became ibgp, stayed drained. With chains attached, its iBGP filters then set local-pref 0 and tagged exports with GRACEFUL_SHUTDOWN, from a switch the form hides on iBGP. updatePeerShape now clears it for iBGP. Co-Authored-By: Claude Opus 5.5 --- internal/store/peer_templates_test.go | 53 +++++++++++++++++++++++++++ internal/store/peers.go | 12 +++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/internal/store/peer_templates_test.go b/internal/store/peer_templates_test.go index cba9d48..2e7361d 100644 --- a/internal/store/peer_templates_test.go +++ b/internal/store/peer_templates_test.go @@ -638,3 +638,56 @@ func TestSymbolUsesFindsEveryOwnerOfAName(t *testing.T) { 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) + } +} 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) From cec60038673623a0da6ae000357fb241ad7e7748 Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Thu, 1 Oct 2026 10:25:44 +0300 Subject: [PATCH 04/19] fix(lint): name the peers a template-folded finding is about Folding replaced the peer names with "IX_PEERS (2 peers)". That is fine for a finding the template can fix, but not for one it cannot: two drained sessions, or two link-local neighbors without an interface, folded into a line that sent the operator to the template and left no way to tell which sessions to fix. The count was also of findings, not peers, so one peer tripping a check twice showed as "(2 peers)". A folded line now reads "IX_PEERS: rs3_v4, rs7_v4" (the first five, then "and N more"), and a group folds only across two or more distinct peers. Co-Authored-By: Claude Opus 5.5 --- docs/USAGE.md | 3 ++- internal/render/lint.go | 22 ++++++++++++++++----- internal/render/peer_template_test.go | 28 ++++++++++++++++++++++++++- 3 files changed, 46 insertions(+), 7 deletions(-) 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) + } +} From 40370adffe629f8d5fa2d62a2aab88370cefb657 Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Thu, 1 Oct 2026 10:26:29 +0300 Subject: [PATCH 05/19] fix(peers): the bulk bar has no default action The bulk select's first option, and so its default, was "Attach to ", and "Detach" posted an empty value. Selecting twenty peers and pressing "Apply to selected" without touching the select, past a generic confirm, linked all twenty to whichever template sorted first, overwriting their chains, limits and safeguards. The select now starts on a disabled "Choose an action" placeholder and is required. Detach posts an explicit "detach". The handler refuses an empty choice instead of reading it as detach. Co-Authored-By: Claude Opus 5.5 --- internal/web/peer_templates_followups_test.go | 13 ++++++++++++- internal/web/peers.go | 16 ++++++++++++---- internal/web/templates/peers.html | 5 +++-- 3 files changed, 27 insertions(+), 7 deletions(-) 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. + ` + 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}} From edca30bd81d6de637fb45dedfd8c2a70587be66c Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Thu, 1 Oct 2026 10:27:33 +0300 Subject: [PATCH 06/19] fix(templates): attach and link in one transaction, reading inside it LinkPeerToTemplate read the template and the peer before opening its transaction, so a template save committing in between (which rewrites only the peers linked at its own commit) missed the newly linked peer. That peer kept the old chain and limit while rendering session options from the new template block, until the next template save. The bulk bar also committed one transaction per peer and audited only after the loop, so an error on peer 12 of 30 left 11 rewritten and none audited. store.AttachPeers links (or, with template 0, detaches) a whole selection in one transaction, reading the template and each peer through it. LinkPeerToTemplate and DetachPeer are its one-peer cases, and the bulk handler resolves names first, then makes one call. Co-Authored-By: Claude Opus 5.5 --- internal/store/peer_templates.go | 75 +++++++++++++++++++-------- internal/store/peer_templates_test.go | 44 ++++++++++++++++ internal/web/peers.go | 17 +++--- 3 files changed, 107 insertions(+), 29 deletions(-) diff --git a/internal/store/peer_templates.go b/internal/store/peer_templates.go index 8d4aa7e..448a1ea 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,26 +304,52 @@ 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 - } - 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 { + 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() } @@ -322,11 +359,7 @@ func (s *Store) LinkPeerToTemplate(peerID, templateID int64) error { // 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) + return s.AttachPeers([]int64{peerID}, 0) } // PolicyIDs lists a chain's ids in order, for the chain writers. diff --git a/internal/store/peer_templates_test.go b/internal/store/peer_templates_test.go index 2e7361d..e558304 100644 --- a/internal/store/peer_templates_test.go +++ b/internal/store/peer_templates_test.go @@ -691,3 +691,47 @@ func TestTemplatePathsClearDrainWhenThePeerBecomesIBGP(t *testing.T) { 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) + } +} diff --git a/internal/web/peers.go b/internal/web/peers.go index 92c64ec..1a42799 100644 --- a/internal/web/peers.go +++ b/internal/web/peers.go @@ -390,6 +390,7 @@ func (s *Server) handlePeersAttach(w http.ResponseWriter, r *http.Request) { } var done, missing []string + var ids []int64 for _, name := range names { p, err := s.store.GetPeerByName(name) if err == store.ErrNotFound { @@ -400,16 +401,16 @@ func (s *Server) handlePeersAttach(w http.ResponseWriter, r *http.Request) { s.serverError(w, "get peer", err) return } - if tmpl.ID != 0 { - err = s.store.LinkPeerToTemplate(p.ID, tmpl.ID) - } else { - err = s.store.DetachPeer(p.ID) - } - if err != nil { - s.serverError(w, "attach peer to template", err) + ids, done = append(ids, p.ID), append(done, name) + } + // One transaction for the whole selection: either every peer moves, or — + // on any error — none does, so a failure never leaves a half-attached, + // unaudited group. + if len(ids) > 0 { + if err := s.store.AttachPeers(ids, tmpl.ID); err != nil { + s.serverError(w, "attach peers to template", err) return } - done = append(done, name) } var msg string From bc78ff0b2bd717161ded81d12ad02532bd9545b2 Mon Sep 17 00:00:00 2001 From: Bogdan FLOREA Date: Thu, 1 Oct 2026 10:28:16 +0300 Subject: [PATCH 07/19] fix(templates): an iBGP template previews on an iBGP neighbor The template editor renders the shape on a sample neighbor, always AS64496. For an iBGP template that is an eBGP session: the preview showed `neighbor 192.0.2.1 as 64496` and a danger finding against the template, "marked iBGP but its remote AS is 64496", on every iBGP template. The sample now uses our own AS when the template is iBGP. The sample was also named "