fix: peer template review follow-ups

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

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,

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
 		}

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)
+	}
+}

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
 }
 

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)
+	}
+}

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
+}

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}

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)
+		}
+	}
+}