Repository navigation
fix: peer template review follow-ups - #30
Merged
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
The bulk select's first option, and so its default, was "Attach to <first template>", 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 "<template>_example", which exceeds BIRD's 63-character limit for any template name of 56 characters or more, so those templates showed "Fix the errors above" with nothing to fix. The template part is now trimmed to fit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a template rename failed validation (say IX_PEERS to IX_RS with a bad community), the form re-rendered with the typed name and built its action from it: /peers/templates/IX_RS/edit. The corrected resubmit went to a template that did not exist, got "not found", and lost the edit. The peer form had the same bug. Both now post to the name the record is stored under while still showing what the operator typed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DeletePolicy refuses a policy a template's chain holds, but the list counted only peer chains. A policy used only by a template with no peers linked showed "Used by: nothing", and Delete then failed with "attached to 1 peer template(s)" without the list ever naming it. The list now shows template use beside peer use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failed template delete always appended "Detach those peers first", including for a database error or a template that had just been deleted elsewhere, sending the operator to detach peers from a template with none linked. DeletePeerTemplate now returns a TemplateInUseError for linked peers, and only that gets the hint. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The save-time name check covered only the template and peer forms. A prefix set, AS set, community, RPKI server or BMP station named like a template, a session imported from BIRD under a template's name, or data that already clashed, all still reached `bird -p` and failed with "Symbol already defined". The renderer is the one place that sees every symbol, so Sections now scans what it rendered for declarations (define, function, filter, template bgp, named protocols and tables, plus BIRD's own master4 and master6) and refuses a duplicate with a message naming both declarations. Preview, Changes and apply all show it before BIRD is asked. Raw configuration is left to BIRD: it is free text, and a commented-out block there is not a clash. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SymbolUses refused any name starting with imp_, exp_, ebgp_in_, originate_ and so on, though only an exact derived name clashes: `exp_transit` was refused although no policy "transit" existed. A derived name now counts as taken only while the policy, peer or prefix set it derives from exists. BIRD's own master4/master6 tables join the built-ins, which they were missing. Library community validation kept its own shorter list of reserved names and let BOGON_ASNS, rpki4 and the rest through. It now checks the same built-in list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The save-time check ran only on the template and peer forms. A prefix set, AS set, community, RPKI server or BMP station named like a template saved, and so did a session imported from BIRD under a template's name; each then declared the name twice. The renderer now catches that, but the form is where it should be refused. refuseTemplateName runs on each of those saves, on the peer form, and on import (which skips the session). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…its tx Review of the transactional attach found two hazards. It opened a deferred transaction and read before writing: under WAL, a commit from another connection in between makes the first write fail with SQLITE_BUSY_SNAPSHOT, which busy_timeout does not retry. It now opens by touching the template's row, which takes the write lock (waiting its turn) and proves the template exists, as the store's other transactions already do by writing first. And chainFor read each export policy's set IDs through the pool, not the transaction: a second connection held while the first waited, which deadlocks once the pool is exhausted. policySetIDs now takes the querier (TestAttachPeersReadsOnlyThroughItsTransaction hangs without it). LinkPeerToTemplate(peer, 0) silently detached, because 0 is AttachPeers' detach; it is now ErrNotFound. DetachPeer had no callers left and is gone. The bulk handler turns a peer or template deleted mid-request into "nothing was changed" instead of a 500. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
handleCommunityDelete went ahead with the delete whenever communityInUse returned an error, so a transient failure in that check, which now also reads templates, deleted a community that peers or templates still referenced. A failed check now refuses the delete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rename fix moved only the form's action to the stored name. "Save as template" (?from=), and on a template the "peers linked to it" and "add a new peer from it" links, still used the typed name, which matches no peer or template until the rename saves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The policies list loaded every template with its chains (a query per template, and one per chained export policy for its sets) only to count how many templates chain each policy. One GROUP BY over template_policies gives the same counts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
Review follow-up (pushed):
Left as is:
Re-verified locally: gofmt, vet (including |
3 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups from the review of #21 (peer templates). Each finding was checked against
mainfirst. The name clash was reproduced with realbird -p. Each fix has a test that was watched failing. One commit per fix:template bgp TRANSITbesideprotocol bgp TRANSIT, a set, a community or a generated name failed every apply with "Symbol already defined".store.SymbolUsesfinds every owner of a name; template saves refuse a taken name, and peer saves refuse a template's.af861d80e3f540updatePeerShapeclears it for iBGP, likevalidateShape.51baf44IX_PEERS: rs3_v4, rs7_v4and fold only across 2+ distinct peers.cec6003detach; an empty choice is refused.40370adstore.AttachPeersdoes the whole selection in one transaction, reading inside it; link and detach are its one-peer cases.edca30bbc78ff04054ed293730b9TemplateInUseError, and only that gets the hint.904b757Not changed, deliberately:
templates/attachget no migration. That's vanishingly rare, and renaming would restart live BGP sessions.Verification. Actions is billing-locked, so this was run locally with the
ci.ymlsteps (go1.26.8,GOTOOLCHAIN=local): gofmt clean, vet (including-tags integration) ok, golangci-lint 0 issues,go test -race ./...ok, govulncheck with nothing reachable, the build ok, and both BIRD 2.14 integration tests passing.🤖 Generated with Claude Code