Skip to content

Commit 0fb018b

Browse files
committed
fix(bgp): the ExaBGP api stage, and the panic it was hiding
The ExaBGP compatibility api stage goes 28/40 to 40/40. The encoding stage stays 42/42. Three cases exited 2 with an EMPTY stderr. Exit 2 is Go's status for an unrecovered panic, and the trace reached nobody: crashlog.Init dup2s fd 2 onto a pipe a reader goroutine drains, the runtime writes the trace and ends the process, so the reader is never scheduled. main called flushCrashlog only on the ordinary return, which a panic skips. Every ze binary was affected. main now ALSO defers it, so fd 2 is restored while the panic unwinds; the ordinary call stays because os.Exit runs no deferred function. The trace it was hiding named the defect in one frame. WireUpdate.Attrs answers (nil, nil) for a withdraw-only UPDATE, which carries no attribute section, and the bridge's three overlays read it. Checking the error alone is the nil a caller cannot tell from an answer. The withdraw path never went through the bridge's renames: the announce arm called exabgpNLRIObject and the del arm passed the object through, so one route read as nlri/label/65000:1 outbound and prefix/labels/0:65000:1 back. A withdraw also KEEPS its next hop in the attribute object, where an announce drops it: ExaBGP reads include_nexthop = bool(minus), because a withdraw has no announce key to hang it under. A Path Identifier of ZERO was dropped as if ADD-PATH were off. RFC 7911 Section 3 reserves no value, and appendIPv4PrefixesFromWire HELD the negotiated flag and still wrote it only when the identifier was non-zero. ParseINET had the same shape one layer down: it consumed the four octets and threw the flag away, so an *INET could not say whether it carried an identifier at all. interface-set is named in both directions now. It was the fifth extended community ze could encode and not read back; the config migration wrote it as raw hex and said ze had no name for it. It is also the one place the two vocabularies genuinely diverge rather than differ: the draft defines the community twice, transitive and non-transitive, and ze carries every extended community in one flat list, so its text is the only place that bit can go. ExaBGP has a scope block, so it prints four fields and puts transitivity in a JSON member, the only community it writes that member for. Three api fixtures stated peer 127.0.0.1 where their own ze config declares 127.0.0.2. That config header records the move and RFC 4271 Section 5.1.3 as its reason, and says the expected frames are unchanged, which held for the hex and not for the json lines. ExaBGP renders the member from the CONFIGURED neighbor, not the socket.
1 parent fc78786 commit 0fb018b

28 files changed

Lines changed: 655 additions & 92 deletions

File tree

‎cmd/ze/crash_flush_test.go‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// Design: docs/architecture/diagnostics/crash-capture.md -- the stderr a panic reaches
2+
3+
package main
4+
5+
import (
6+
"go/ast"
7+
"go/parser"
8+
"go/token"
9+
"testing"
10+
)
11+
12+
// TestMainDefersTheCrashlogFlush reads main.go and asserts that main defers
13+
// flushCrashlog.
14+
//
15+
// VALIDATES: a panic that nothing recovers still prints its trace.
16+
// PREVENTS: exit status 2 with no output at all.
17+
//
18+
// crashlog.Init replaces fd 2 with a pipe, and a reader goroutine copies that
19+
// pipe to the real stderr. The runtime writes a panic trace to fd 2 and then
20+
// ends the process, so that goroutine is never scheduled and the trace dies in
21+
// the pipe. Measured on 2026-09-20: the ExaBGP compatibility mock server
22+
// panicked on three cases and the suite reported `exit status 2` with an empty
23+
// stderr for each, which cost a whole bisection to recover
24+
// (plan/journal/failing-gate-prints-no-cause.md).
25+
//
26+
// The check is structural because the behavior needs a process that dies. A
27+
// deferred call runs while the panic unwinds, before the runtime prints, and
28+
// Flush restores fd 2 to the real stderr. The ordinary path cannot rely on the
29+
// defer, because os.Exit runs no deferred function, so main calls Flush twice
30+
// and Flush acts once.
31+
func TestMainDefersTheCrashlogFlush(t *testing.T) {
32+
fset := token.NewFileSet()
33+
file, err := parser.ParseFile(fset, "main.go", nil, 0)
34+
if err != nil {
35+
t.Fatalf("parse main.go: %v", err)
36+
}
37+
38+
body := mainFunctionBody(t, file)
39+
for _, statement := range body.List {
40+
deferred, isDefer := statement.(*ast.DeferStmt)
41+
if !isDefer {
42+
continue
43+
}
44+
name, isIdentifier := deferred.Call.Fun.(*ast.Ident)
45+
if !isIdentifier {
46+
continue
47+
}
48+
if name.Name == "flushCrashlog" {
49+
return
50+
}
51+
}
52+
t.Fatal("main does not defer flushCrashlog, so a panic exits 2 with its trace stuck in the crashlog pipe")
53+
}
54+
55+
// mainFunctionBody returns the body of func main, and fails the test when the
56+
// file holds no such function.
57+
func mainFunctionBody(t *testing.T, file *ast.File) *ast.BlockStmt {
58+
t.Helper()
59+
for _, declaration := range file.Decls {
60+
function, isFunction := declaration.(*ast.FuncDecl)
61+
if !isFunction {
62+
continue
63+
}
64+
if function.Recv != nil || function.Name.Name != "main" {
65+
continue
66+
}
67+
if function.Body == nil {
68+
t.Fatal("func main has no body")
69+
}
70+
return function.Body
71+
}
72+
t.Fatal("main.go declares no func main")
73+
return nil
74+
}

‎cmd/ze/main.go‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,21 @@ var (
5555
)
5656

5757
func main() {
58+
// crashlog.Init replaces fd 2 with a pipe that a reader goroutine copies to
59+
// the real stderr. A panic that nothing recovers writes its trace into that
60+
// pipe and the process dies immediately, so the reader is never scheduled
61+
// and the operator gets exit status 2 with no output at all. A deferred
62+
// flush runs while the panic unwinds, restores fd 2 to the real stderr and
63+
// drains what the pipe already holds, and the runtime then prints the trace
64+
// where somebody can read it.
65+
//
66+
// The call below it is still owed: os.Exit runs no deferred function, so the
67+
// ordinary path flushes for itself. Flush acts once, so the pair is safe.
68+
defer flushCrashlog()
69+
5870
code := dispatchMain(os.Args[1:])
5971
flushCrashlog()
60-
os.Exit(code)
72+
// gocritic's exitAfterDefer names the shape above, and it is the intended
73+
// one: the defer exists for the PANIC path, which os.Exit never reaches.
74+
os.Exit(code) //nolint:gocritic // exitAfterDefer: the flush on the line above serves this path
6175
}

‎docs/architecture/api/json-format.md‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,13 @@ With ADD-PATH:
275275
```json
276276
"nlri": [{"prefix": "10.0.0.0/24", "path-id": 1}]
277277
```
278-
<!-- source: internal/component/bgp/format/text.go -- NLRI formatting per family -->
278+
279+
The negotiated layout decides the shape, never the identifier's value. RFC 7911
280+
Section 3 reserves no value for the Path Identifier, so a route whose identifier
281+
is zero is written as `{"prefix": "10.0.0.0/24", "path-id": 0}`. The bare string
282+
form says the session carried no identifier for that route.
283+
284+
<!-- source: internal/component/bgp/format/text_json.go -- appendNLRIJSONValue -->
279285

280286
### Labeled Unicast (MPLS)
281287

‎docs/architecture/testing/ci-format.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -985,6 +985,20 @@ an exabgp-compat config is left alone for the same reason.
985985
<!-- source: internal/test/runner/loopback_darwin.go -- SIOCAIFADDR on BSD -->
986986
<!-- source: internal/le/setup/actions.go -- Answer -->
987987

988+
An exabgp-compat case runs TWO processes, ze and the mock BGP server, and its
989+
verdict names which one ended the case:
990+
991+
- `ze exited before the mock BGP server finished its script: <error>` is a
992+
daemon defect. Read the client stderr in the failure block.
993+
- `the mock BGP server failed: <error>` is a wire or document disagreement. The
994+
mock prints the frame or the JSON member that did not match.
995+
- `the mock BGP server ended without reporting success` means its script did not
996+
complete and it said nothing about why.
997+
998+
Both used to print the bare `exit status N` of whichever process failed, which
999+
told a reader nothing about which of the two to open.
1000+
<!-- source: internal/test/cli/cmd_exabgp.go -- exaBGPFailure -->
1001+
9881002
## Expectations
9891003

9901004
```

‎docs/architecture/wire/nlri.md‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,14 @@ NLRI (interface)
9191
- `family` - AFI/SAFI address family
9292
- `prefix` - IP prefix (netip.Prefix)
9393
- `pathID` - RFC 7911 ADD-PATH identifier
94+
- `addPath` - true when the wire carried a Path Identifier, read through `HasAddPath()`
95+
96+
RFC 7911 Section 3 reserves no value for the Path Identifier, so `pathID` of
97+
zero identifies a path and says nothing about whether one arrived. `addPath`
98+
answers that, and only the parser can set it, because the negotiation is what
99+
told the parser how to read the octets. A locally built NLRI reports false: it
100+
has no wire layout yet, and `WriteNLRI` takes the layout from the session's
101+
EncodingContext.
94102

95103
**RDNLRIBase** (base.go) - Shared by RD-based types:
96104
- `rd` - Route Distinguisher (8 bytes)
@@ -147,6 +155,11 @@ NLRI (interface)
147155
| 192.168.1.0/24 | `18 C0 A8 01` | mask=24, 3 bytes |
148156
| 10.0.0.1/32 | `20 0A 00 00 01` | mask=32, 4 bytes (full IP) |
149157

158+
`ParseINET` takes the ADD-PATH flag the session negotiated, consumes the 4-octet
159+
Path Identifier when it is set, and records the flag on the NLRI it returns. A
160+
reader of a parsed INET therefore asks `HasAddPath()` for the layout and reads
161+
`PathID()` for the value, including when that value is zero.
162+
150163
<!-- source: internal/core/bgp/nlri/inet.go -- ParseINET, INET.Len, INET.WriteTo -->
151164

152165
### ExaBGP Implementation
@@ -572,6 +585,11 @@ The plugin text command `decode nlri <family> <hex>` and the `ze bgp decode`
572585
CLI both read a hex blob with no session behind it, so both decode with no Path
573586
Identifier.
574587

588+
A core family takes the same route to the same answer. `INET` records the flag
589+
`ParseINET` was given, so `appendNLRIJSONValue` reads one interface for every
590+
family and renders `{"prefix": ..., "path-id": 0}` rather than a bare prefix
591+
string when the session carried a Path Identifier of zero.
592+
575593
<!-- source: internal/core/bgp/nlri/nlri.go -- AddPathAware, SplitPathID -->
576594
<!-- source: internal/component/bgp/format/text_json.go -- appendNLRIJSONValue -->
577595
<!-- source: internal/component/plugin/registry/registry.go -- DecodeNLRIByFamily -->

‎docs/architecture/wire/qualifiers.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,12 @@ class PathInfo:
318318
"path-information": "1.2.3.4"
319319
```
320320

321+
The member is written for every route the session carried a Path Identifier for,
322+
and RFC 7911 Section 3 reserves no value, so an identifier of zero renders as
323+
`"path-information": "0.0.0.0"`. `PathID()` returns the value alone, so a reader
324+
deciding whether the member is owed asks `HasAddPath()` instead of comparing
325+
that value against zero.
326+
321327
<!-- source: internal/core/bgp/nlri/nlri.go -- WriteNLRI, LenWithContext -->
322328
<!-- source: internal/core/bgp/nlri/base.go -- PrefixNLRI.PathID -->
323329

‎docs/exabgp/exabgp-differences.md‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ is gone.
107107

108108
## JSON member spellings the bridge folds
109109

110-
Four members carry the same fact under different words. Ze keeps its own word,
110+
Five members carry the same fact under different words. Ze keeps its own word,
111111
because each is load-bearing on Ze's side, and the bridge writes ExaBGP's word
112112
so a script reads one vocabulary.
113113

@@ -117,6 +117,7 @@ so a script reads one vocabulary.
117117
| RFC 7911 Path Identifier | `"path-id": 16909060` | `"path-information": "1.2.3.4"` | RFC 7911 Section 3 calls it a 4-octet value, and Ze's CLI and RIB read the number |
118118
| FlowSpec DSCP on IPv6 | `dscp` | `traffic-class` | RFC 8956 Section 8 registers component type 11 as `DSCP` under both its IPv4 and its IPv6 name |
119119
| FlowSpec traffic-marking and traffic-action | `mark:10`, `traffic-action:sample` | `mark 10`, `action sample` | the FlowSpec firewall lowering keys on the colon form |
120+
| Interface set transitivity | `interface-set:non-transitive:input:1234:10` | `interface-set:input:1234:10` plus `"transitive": false` | Ze carries every extended community in one flat list, and ExaBGP has a `scope` block to state it beside |
120121

121122
Nothing is lost either way, so each is a spelling rather than a conversion. The
122123
RFC governs the wire and says nothing about a JSON member, which is what leaves
@@ -128,7 +129,7 @@ quad in both projects, and so do `l2info` and the FlowSpec redirect-to-IP
128129
communities, because Ze's config parser already read those spellings and its
129130
renderer wrote something the parser refuses.
130131
<!-- source: internal/exabgp/bridge/bridge_event.go -- exabgpRD, exabgpPathInformation, exabgpFlowComponentName -->
131-
<!-- source: internal/exabgp/bridge/bridge_wire_json.go -- exabgpCommunitySpellings -->
132+
<!-- source: internal/exabgp/bridge/bridge_wire_json.go -- exabgpCommunitySpellings, exabgpInterfaceSet -->
132133

133134
---
134135

@@ -139,6 +140,19 @@ three members Ze's own JSON does not carry: the AS_PATH's segments and their
139140
types, the numeric value beside each extended community, and the flag octet in
140141
an unknown attribute's `attribute-0xCC-0xFF` name.
141142

143+
The community value is read for both widths, RFC 4360's 8 octets and RFC 5701's
144+
20, and it is built exactly and rounded once. RFC 5701's community is 160 bits
145+
wide and JSON has one number type, so accumulating the octets in a float rounds
146+
at every step: doing that put the member two thousand off an expectation whose
147+
own decimal literal rounds cleanly.
148+
149+
One community gains a second member here. ExaBGP writes `"transitive"` beside
150+
the interface set and beside nothing else, because that community
151+
(draft-ietf-idr-flowspec-interfaceset Section 5) is defined twice, transitive
152+
and non-transitive, differing by one bit of the type octet. Ze states that bit
153+
in the text instead, since it carries every extended community in one flat list
154+
and has no `scope` block to put it in.
155+
142156
The live bridge cannot. `translateAndForward` receives Ze's JSON event as text
143157
and has no wire to read, so an ExaBGP process attached to a running Ze gets Ze's
144158
flattened AS_PATH and no community values. The ExaBGP compatibility fixtures go

‎internal/component/bgp/format/addpath_nlri_json_test.go‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,3 +106,71 @@ func TestAppendNLRIJSONValueCarriesAddPathToTheDecoder(t *testing.T) {
106106
})
107107
}
108108
}
109+
110+
// TestNLRIJSONKeepsAZeroPathIdentifier drives the two NLRI JSON writers with a
111+
// route whose Path Identifier is zero, which is the value that used to make the
112+
// formatter render an ADD-PATH route as a plain prefix.
113+
//
114+
// RFC 7911 Section 3: "In order to carry the Path Identifier in an UPDATE
115+
// message, the NLRI encoding MUST be extended by prepending the Path Identifier
116+
// field, which is of four octets."
117+
//
118+
// The section reserves no value, so zero identifies a path like any other
119+
// number. ExaBGP writes it as "path-information":"0.0.0.0" (PathInfo.json,
120+
// src/exabgp/bgp/message/update/nlri/qualifier/path.py) and omits the member
121+
// only when ADD-PATH is disabled.
122+
//
123+
// VALIDATES: appendNLRIJSONValue and appendIPv4PrefixesFromWire decide the shape
124+
// from the negotiated layout, and write "path-id" for an identifier of zero.
125+
// PREVENTS: an ADD-PATH route with identifier zero rendering as the bare string
126+
// a route carrying no identifier renders as, which leaves a reader of the JSON
127+
// unable to tell the two apart.
128+
func TestNLRIJSONKeepsAZeroPathIdentifier(t *testing.T) {
129+
// One /32 of 1.1.1.1, once behind a zero Path Identifier and once alone.
130+
addPathSection := []byte{0x00, 0x00, 0x00, 0x00, 0x20, 0x01, 0x01, 0x01, 0x01}
131+
plainSection := []byte{0x20, 0x01, 0x01, 0x01, 0x01}
132+
133+
cases := []struct {
134+
name string
135+
data []byte
136+
addPath bool
137+
want string
138+
}{
139+
{
140+
name: "add-path negotiated",
141+
data: addPathSection,
142+
addPath: true,
143+
want: `{"prefix":"1.1.1.1/32","path-id":0}`,
144+
},
145+
{
146+
name: "add-path not negotiated",
147+
data: plainSection,
148+
addPath: false,
149+
want: `"1.1.1.1/32"`,
150+
},
151+
}
152+
153+
for _, tc := range cases {
154+
t.Run(tc.name, func(t *testing.T) {
155+
parsed, rest, err := nlri.ParseINET(family.AFIIPv4, family.SAFIUnicast, tc.data, tc.addPath)
156+
if err != nil {
157+
t.Fatalf("parse NLRI: %v", err)
158+
}
159+
if len(rest) != 0 {
160+
t.Fatalf("want the whole section consumed, %d octets left", len(rest))
161+
}
162+
163+
got := string(appendNLRIJSONValue(nil, parsed, family.IPv4Unicast))
164+
if got != tc.want {
165+
t.Errorf("appendNLRIJSONValue wrote %s, want %s", got, tc.want)
166+
}
167+
168+
// The body NLRI section never builds an NLRI struct, so it carries
169+
// the same decision in its own loop and needs its own case.
170+
fromWire := string(appendIPv4PrefixesFromWire(nil, tc.data, tc.addPath))
171+
if fromWire != tc.want {
172+
t.Errorf("appendIPv4PrefixesFromWire wrote %s, want %s", fromWire, tc.want)
173+
}
174+
})
175+
}
176+
}

‎internal/component/bgp/format/json_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1498,7 +1498,7 @@ func TestJSONEncoderMPLSVPN(t *testing.T) {
14981498
pathID: 42,
14991499
wantRD: "2:65536:100",
15001500
wantLabels: `"labels":[[400,6401]]`, // each member is the label and the RFC 8277 stack entry it came from
1501-
wantPathID: false, // path-id is transport-level, not in plugin decode output
1501+
wantPathID: false, // path-id is transport-level, not in plugin decode output
15021502
},
15031503
}
15041504

0 commit comments

Comments
 (0)