Skip to content

Commit dc96976

Browse files
thomas-manginclaude
andcommitted
refactor: Apply packed-bytes-first pattern to INET (Phase 1)
INET class now stores complete wire format in _packed: - Replace path_info slot with _has_addpath bool slot - _packed contains [addpath:4?][mask:1][prefix:var] - path_info is now a property that extracts from _packed - cidr property uses _mask_offset for correct extraction NLRI immutability enforced: - Remove labels setter from Label class - Remove rd setter from IPVPN class - Remove assign() method from NLRI base class - Factory methods must receive all values upfront Configuration parsing updated: - Nested route syntax now uses settings mode - pre() creates INETSettings, post() creates immutable NLRI - Flow NLRI kept mutable (separate hierarchy) Documentation updated: - Add NLRI immutability rule to PACKED_BYTES_FIRST_PATTERN.md - Add NLRI immutability rule to NLRI_CLASS_HIERARCHY.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 8c4c92e commit dc96976

File tree

13 files changed

+425
-239
lines changed

13 files changed

+425
-239
lines changed

.claude/exabgp/NLRI_CLASS_HIERARCHY.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,18 @@ This document maps the NLRI class inheritance structure, showing where class var
44

55
---
66

7+
## 🚨 NLRI Immutability Rule
8+
9+
**NLRI instances are IMMUTABLE after creation. NO SETTERS ALLOWED.**
10+
11+
- Never add property setters for NLRI fields
12+
- Never assign to NLRI fields after creation
13+
- Use factory methods with all values provided upfront
14+
15+
See `PACKED_BYTES_FIRST_PATTERN.md` for full rationale and migration guidance.
16+
17+
---
18+
719
## Inheritance Diagram
820

921
```

.claude/exabgp/PACKED_BYTES_FIRST_PATTERN.md

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,39 @@ def endpoint(self) -> int:
132132

133133
---
134134

135+
## 🚨 NLRI Immutability Rule
136+
137+
**NLRI instances are IMMUTABLE after creation. NO SETTERS ALLOWED.**
138+
139+
```python
140+
# ❌ WRONG - Never add property setters for NLRI fields
141+
@path_info.setter
142+
def path_info(self, value: PathInfo) -> None:
143+
self._packed = ... # FORBIDDEN
144+
145+
# ❌ WRONG - Never assign to NLRI fields after creation
146+
nlri.labels = Labels.make_labels([100]) # FORBIDDEN
147+
nlri.rd = RouteDistinguisher(...) # FORBIDDEN
148+
nlri.path_info = PathInfo(...) # FORBIDDEN
149+
150+
# ✅ CORRECT - Use factory methods with all values upfront
151+
nlri = Label.from_cidr(cidr, afi, labels=Labels.make_labels([100]))
152+
nlri = IPVPN.from_settings(settings) # settings has all values
153+
```
154+
155+
**Why immutability?**
156+
1. **Wire format integrity:** `_packed` is the source of truth; modifications would desync properties
157+
2. **Thread safety:** Immutable objects are inherently thread-safe
158+
3. **Hash stability:** NLRI are used as dict keys; mutable objects break hashing
159+
4. **Simplicity:** No need to handle partial construction states
160+
161+
**Backward compatibility during migration:**
162+
- Existing code that assigns to NLRI fields will break - this is intentional
163+
- Update callers to use factory methods with all values provided upfront
164+
- If code currently does `nlri.labels = X`, change to `from_cidr(..., labels=X)`
165+
166+
---
167+
135168
## Implementation Pattern
136169

137170
### 1. Class Structure
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
# Plan: INET/Label/IPVPN Packed-Bytes-First Pattern
2+
3+
**Status:** ✅ Phase 1 Complete (INET)
4+
**Created:** 2025-12-08
5+
**Last Updated:** 2025-12-08
6+
7+
---
8+
9+
## Progress
10+
11+
### Phase 1: INET Class - COMPLETE ✅
12+
13+
**Implemented:**
14+
1. Replaced `path_info` slot with `_has_addpath: bool` slot
15+
2. Updated `__init__` to accept `has_addpath` parameter
16+
3. Added `_mask_offset` property (returns 0 or 4 based on `_has_addpath`)
17+
4. Added `path_info` property (extracts from `_packed[0:4]` if `_has_addpath`)
18+
5. Updated `cidr` property to use `_mask_offset`
19+
6. Updated `pack_nlri()` with proper AddPath handling
20+
7. Updated `__hash__()`, `index()`, `__len__()` to use `_packed` directly
21+
8. Updated factory methods (`from_cidr`) to build wire format with AddPath
22+
23+
**Also completed:**
24+
- Removed `labels` setter from Label class
25+
- Removed `rd` setter from IPVPN class
26+
- Updated Label and IPVPN `__init__`, `from_cidr`, `from_settings` methods
27+
- Updated Label and IPVPN `__copy__` and `__deepcopy__` methods
28+
- Updated configuration parsing to use settings mode for nested route syntax
29+
- Removed legacy mode from configuration parsing (NLRI are now immutable)
30+
- Updated documentation with NLRI immutability rule
31+
32+
**Tests:** All 2877 unit tests pass, all 11 test suites pass
33+
34+
---
35+
36+
## Remaining Work
37+
38+
### Phase 2: Label Class - NOT STARTED
39+
40+
Store labels in `_packed` instead of separate `_labels_packed` slot.
41+
42+
1. Remove `_labels_packed` slot
43+
2. Update `__init__` to accept complete wire format including labels
44+
3. Add `_label_end_offset()` helper for BOS scanning
45+
4. Update `labels` property to extract from `_packed`
46+
5. Override `cidr` property to extract from after labels
47+
48+
### Phase 3: IPVPN Class - NOT STARTED
49+
50+
Store RD in `_packed` instead of separate `_rd_packed` slot.
51+
52+
1. Remove `_rd_packed` slot
53+
2. Update `__init__` to accept complete wire format including labels and RD
54+
3. Add `rd` property to extract from `_packed[label_end:label_end+8]`
55+
4. Override `cidr` property to extract from after RD
56+
5. Update `unpack_nlri()` to store complete wire format
57+
58+
---
59+
60+
## Key Changes Made
61+
62+
### NLRI Immutability
63+
64+
**All INET/Label/IPVPN NLRI are now immutable after creation.**
65+
66+
- No property setters for `path_info`, `labels`, `rd`
67+
- Factory methods (`from_cidr`, `from_settings`) must receive all values upfront
68+
- Configuration parsing uses settings mode (INETSettings) for deferred construction
69+
70+
### Configuration Parsing
71+
72+
**Nested route syntax now uses settings mode:**
73+
74+
```
75+
route 10.0.0.0/24 {
76+
rd 65000:1;
77+
label 100;
78+
next-hop 1.2.3.4;
79+
}
80+
```
81+
82+
- `pre()` creates INETSettings and enters settings mode
83+
- Parsed values populate the settings object
84+
- `post()` creates immutable NLRI from settings
85+
86+
**Flow NLRI kept mutable** - Flow is a separate hierarchy with its own setters.
87+
88+
---
89+
90+
## Wire Format
91+
92+
```
93+
INET: [addpath:4?][mask:1][prefix:var]
94+
Label: [addpath:4?][mask:1][labels:3n][prefix:var]
95+
IPVPN: [addpath:4?][mask:1][labels:3n][rd:8][prefix:var]
96+
```
97+
98+
Currently:
99+
- INET: `_packed` = `[addpath:4?][mask:1][prefix:var]`
100+
- Label: `_packed` = `[addpath:4?][mask:1][prefix:var]`, `_labels_packed` = labels (still separate)
101+
- IPVPN: `_packed` = `[addpath:4?][mask:1][prefix:var]`, `_labels_packed` = labels, `_rd_packed` = rd (still separate)
102+
103+
---
104+
105+
## Verification
106+
107+
```bash
108+
./qa/bin/test_everything # All 11 tests pass
109+
```

src/exabgp/bgp/message/update/nlri/inet.py

Lines changed: 98 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -101,33 +101,65 @@
101101
@NLRI.register(AFI.ipv4, SAFI.multicast)
102102
@NLRI.register(AFI.ipv6, SAFI.multicast)
103103
class INET(NLRI):
104-
__slots__ = ('path_info', 'labels', 'rd')
104+
"""INET NLRI using packed-bytes-first pattern.
105105
106-
def __init__(self, packed: bytes, afi: AFI, safi: SAFI = SAFI.unicast) -> None:
107-
"""Create an INET NLRI from packed CIDR bytes.
106+
Wire format stored in _packed: [addpath:4?][mask:1][prefix:var]
107+
- If _has_addpath is True: addpath bytes are at [0:4], mask at [4]
108+
- If _has_addpath is False: mask is at [0], no addpath bytes
109+
110+
Properties extract data from _packed lazily:
111+
- path_info: extracts PathInfo from [0:4] if _has_addpath, else DISABLED
112+
- cidr: extracts CIDR from [M:] where M = 4 if _has_addpath else 0
113+
"""
114+
115+
__slots__ = ('_has_addpath', 'labels', 'rd')
116+
117+
def __init__(self, packed: bytes, afi: AFI, safi: SAFI = SAFI.unicast, *, has_addpath: bool = False) -> None:
118+
"""Create an INET NLRI from packed wire format bytes.
108119
109120
Args:
110-
packed: CIDR wire format bytes [mask_byte][truncated_ip...]
121+
packed: Wire format bytes [addpath:4?][mask:1][prefix:var]
111122
afi: Address Family Identifier (required - cannot be reliably inferred)
112123
safi: Subsequent Address Family Identifier (defaults to unicast)
124+
has_addpath: If True, packed includes 4-byte path identifier at start
113125
114-
The AFI parameter is required because wire format is ambiguous for
115-
masks 0-32: both IPv4 /32 and IPv6 /32 have the same byte length.
116-
Use factory methods (from_cidr, make_route) when creating INET instances.
126+
The packed bytes include the complete wire format:
127+
- If has_addpath=True: [path_id:4][mask:1][prefix:var]
128+
- If has_addpath=False: [mask:1][prefix:var]
129+
130+
Use factory methods (from_cidr, from_settings) when creating INET instances.
117131
"""
118132
NLRI.__init__(self, afi, safi, Action.UNSET)
119-
self._packed = packed # CIDR wire format
120-
self.path_info = PathInfo.DISABLED
133+
self._packed = packed # Complete wire format
134+
self._has_addpath = has_addpath
121135
self.nexthop = IP.NoNextHop
122136
self.labels: Labels | None = None
123137
self.rd: RouteDistinguisher | None = None
124138

139+
@property
140+
def _mask_offset(self) -> int:
141+
"""Offset where mask byte starts (0 or 4 depending on AddPath)."""
142+
return PATH_INFO_SIZE if self._has_addpath else 0
143+
144+
@property
145+
def path_info(self) -> PathInfo:
146+
"""Extract PathInfo from wire bytes if AddPath present."""
147+
if not self._has_addpath:
148+
return PathInfo.DISABLED
149+
path_bytes = self._packed[:PATH_INFO_SIZE]
150+
# Return NOPATH singleton for all-zero path ID
151+
if path_bytes == b'\x00\x00\x00\x00':
152+
return PathInfo.NOPATH
153+
return PathInfo(path_bytes)
154+
125155
@property
126156
def cidr(self) -> CIDR:
127157
"""Unpack CIDR from stored wire format bytes."""
158+
offset = self._mask_offset
159+
cidr_bytes = self._packed[offset:]
128160
if self.afi == AFI.ipv4:
129-
return CIDR.from_ipv4(self._packed)
130-
return CIDR.from_ipv6(self._packed)
161+
return CIDR.from_ipv4(cidr_bytes)
162+
return CIDR.from_ipv6(cidr_bytes)
131163

132164
@classmethod
133165
def from_cidr(
@@ -148,12 +180,22 @@ def from_cidr(
148180
path_info: AddPath path identifier
149181
150182
Returns:
151-
New INET instance
183+
New INET instance with wire format in _packed
152184
"""
185+
# Build wire format: [addpath:4?][mask:1][prefix:var]
186+
cidr_packed = cidr.pack_nlri()
187+
188+
# Determine if AddPath should be included
189+
has_addpath = path_info is not PathInfo.DISABLED
190+
if has_addpath:
191+
packed = bytes(path_info.pack_path()) + cidr_packed
192+
else:
193+
packed = cidr_packed
194+
153195
instance = object.__new__(cls)
154196
NLRI.__init__(instance, afi, safi, action)
155-
instance._packed = cidr.pack_nlri()
156-
instance.path_info = path_info
197+
instance._packed = packed
198+
instance._has_addpath = has_addpath
157199
instance.nexthop = IP.NoNextHop
158200
instance.labels = None
159201
instance.rd = None
@@ -227,7 +269,8 @@ def from_settings(cls, settings: 'INETSettings') -> 'INET':
227269
return instance
228270

229271
def __len__(self) -> int:
230-
return len(self._packed) + len(self.path_info)
272+
# _packed includes AddPath if present
273+
return len(self._packed)
231274

232275
def __str__(self) -> str:
233276
return self.extensive()
@@ -236,13 +279,11 @@ def __repr__(self) -> str:
236279
return self.extensive()
237280

238281
def __hash__(self) -> int:
239-
if self.path_info is PathInfo.NOPATH:
240-
addpath = b'no-pi'
241-
elif self.path_info is PathInfo.DISABLED:
242-
addpath = b'disabled'
243-
else:
244-
addpath = bytes(self.path_info.pack_path())
245-
return hash(addpath + self._packed)
282+
# _packed includes AddPath if present; use _has_addpath as discriminator
283+
# for DISABLED vs actually having no path bytes
284+
if self._has_addpath:
285+
return hash(self._packed)
286+
return hash(b'disabled' + self._packed)
246287

247288
def __copy__(self) -> 'INET':
248289
new = self.__class__.__new__(self.__class__)
@@ -252,7 +293,7 @@ def __copy__(self) -> 'INET':
252293
# NLRI slots
253294
self._copy_nlri_slots(new)
254295
# INET slots
255-
new.path_info = self.path_info
296+
new._has_addpath = self._has_addpath
256297
new.labels = self.labels
257298
new.rd = self.rd
258299
return new
@@ -268,7 +309,7 @@ def __deepcopy__(self, memo: dict[Any, Any]) -> 'INET':
268309
# NLRI slots
269310
self._deepcopy_nlri_slots(new, memo)
270311
# INET slots
271-
new.path_info = self.path_info # Typically shared singleton
312+
new._has_addpath = self._has_addpath # bool - immutable
272313
new.labels = deepcopy(self.labels, memo) if self.labels else None
273314
new.rd = deepcopy(self.rd, memo) if self.rd else None
274315
return new
@@ -279,23 +320,37 @@ def feedback(self, action: Action) -> str:
279320
return ''
280321

281322
def pack_nlri(self, negotiated: 'Negotiated') -> bytes:
282-
if not negotiated.addpath.send(self.afi, self.safi):
283-
return self._packed # No addpath - return directly, no copy
284-
# ADD-PATH negotiated: MUST send 4-byte path ID
285-
if self.path_info is PathInfo.DISABLED:
286-
addpath = PathInfo.NOPATH.pack_path()
323+
"""Pack NLRI for wire transmission.
324+
325+
Handles AddPath based on negotiated capability vs stored format:
326+
- If negotiated.send=True AND _has_addpath: return _packed directly
327+
- If negotiated.send=True AND NOT _has_addpath: prepend NOPATH
328+
- If negotiated.send=False AND _has_addpath: strip AddPath (return from offset 4)
329+
- If negotiated.send=False AND NOT _has_addpath: return _packed directly
330+
"""
331+
send_addpath = negotiated.addpath.send(self.afi, self.safi)
332+
333+
if send_addpath:
334+
if self._has_addpath:
335+
return self._packed # Already has AddPath, return directly
336+
# Need to prepend NOPATH (4 zero bytes)
337+
return bytes(PathInfo.NOPATH.pack_path()) + self._packed
287338
else:
288-
addpath = self.path_info.pack_path()
289-
return bytes(addpath) + self._packed
339+
if self._has_addpath:
340+
# Strip AddPath bytes (first 4 bytes)
341+
return self._packed[PATH_INFO_SIZE:]
342+
return self._packed # No AddPath in either, return directly
290343

291344
def index(self) -> bytes:
292-
if self.path_info is PathInfo.NOPATH:
293-
addpath = b'no-pi'
294-
elif self.path_info is PathInfo.DISABLED:
295-
addpath = b'disabled'
296-
else:
297-
addpath = self.path_info.pack_path()
298-
return bytes(Family.index(self)) + addpath + self._packed
345+
"""Generate unique index for RIB lookup.
346+
347+
Includes family, AddPath status, and wire bytes.
348+
"""
349+
if self._has_addpath:
350+
# _packed already includes path bytes
351+
return bytes(Family.index(self)) + self._packed
352+
# No AddPath - add discriminator to distinguish from has_addpath=True with 0x00000000
353+
return bytes(Family.index(self)) + b'disabled' + self._packed
299354

300355
def prefix(self) -> str:
301356
return '{}{}'.format(self.cidr.prefix(), str(self.path_info))
@@ -400,12 +455,13 @@ def unpack_nlri(
400455
cidr = CIDR.from_ipv4(bytes([mask]) + bytes(network))
401456
else:
402457
cidr = CIDR.from_ipv6(bytes([mask]) + bytes(network))
403-
nlri = cls.from_cidr(cidr, afi, safi, action, path_info)
404458

405-
# Set optional attributes
459+
# Build kwargs for from_cidr - subclasses accept labels and rd
460+
kwargs: dict[str, Labels | RouteDistinguisher] = {}
406461
if labels_list is not None:
407-
nlri.labels = Labels.make_labels(labels_list)
462+
kwargs['labels'] = Labels.make_labels(labels_list)
408463
if rd is not None:
409-
nlri.rd = rd
464+
kwargs['rd'] = rd
410465

466+
nlri = cls.from_cidr(cidr, afi, safi, action, path_info, **kwargs)
411467
return nlri, data

0 commit comments

Comments
 (0)