Skip to content

Commit 9da8844

Browse files
thomas-manginclaude
andcommitted
refactor: Remove nexthop from Flow NLRI json() method (Phase 4 Step 7)
- Add v4_json() method to Flow NLRI for backward compatibility - Update JSON encoder to use use_v4_json flag for NLRI serialization - V4JSON encoder sets use_v4_json=True to call v4_json() on NLRIs - Add _get_json_encoder() to check.py for decode command API version support - Update test_json to support jsv4:/jsv6: expectations (v6-only for json:) - Update conf-flow.ci expectations to remove nexthop from v6 format - Add v4_json() base implementation to NLRI class (delegates to json()) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
1 parent 3f49680 commit 9da8844

File tree

8 files changed

+223
-77
lines changed

8 files changed

+223
-77
lines changed

qa/bin/test_json

Lines changed: 156 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,17 @@
33
44
This test suite validates JSON output from BGP message decoding.
55
6-
CI files can include expected JSON using the format:
6+
CI files can include expected JSON using three tag types:
77
88
option:file:config.conf
99
1:raw:FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:0040:02:...
10-
1:json:{"exabgp": "6.0.0", "type": "update", ...}
10+
1:json:{"exabgp": "6.0.0", ...} # Tests with both v4 and v6 encoders
11+
1:jsv4:{"exabgp": "6.0.0", ...} # Tests with v4 encoder only
12+
1:jsv6:{"exabgp": "6.0.0", ...} # Tests with v6 encoder only
13+
14+
Tag usage:
15+
- json: Use when v4 and v6 produce identical output (most cases)
16+
- jsv4: + jsv6: Use when v4 and v6 differ (e.g., Flow NLRI nexthop)
1117
1218
Usage:
1319
./qa/bin/test_json # Run all JSON regression tests
@@ -38,13 +44,16 @@ YELLOW = '\033[33m'
3844
RESET = '\033[0m'
3945

4046

41-
def decode_message(payload_hex: str, repo_root: Path, config_file: Path | None = None) -> dict | None:
47+
def decode_message(
48+
payload_hex: str, repo_root: Path, config_file: Path | None = None, api_version: int = 4
49+
) -> dict | None:
4250
"""Decode a BGP message payload to JSON.
4351
4452
Args:
4553
payload_hex: The BGP UPDATE payload in hex (without marker/length/type header)
4654
repo_root: Path to repository root
4755
config_file: Optional config file for context
56+
api_version: API version to use (4 or 6)
4857
4958
Returns:
5059
Parsed JSON dict or None on error
@@ -64,7 +73,7 @@ def decode_message(payload_hex: str, repo_root: Path, config_file: Path | None =
6473
text=True,
6574
timeout=30,
6675
cwd=str(repo_root),
67-
env={**os.environ, 'exabgp_log_enable': 'false'},
76+
env={**os.environ, 'exabgp_log_enable': 'false', 'exabgp_api_version': str(api_version)},
6877
)
6978
if result.returncode == 0 and result.stdout.strip():
7079
return json.loads(result.stdout.strip())
@@ -100,18 +109,28 @@ def normalize_json(data: dict) -> dict:
100109
return normalized
101110

102111

103-
def parse_ci_file(ci_path: Path) -> tuple[Path | None, list[tuple[str, str, str | None]]]:
112+
class JsonExpectation:
113+
"""Container for JSON expectations with version-specific support."""
114+
115+
def __init__(self) -> None:
116+
self.json: str | None = None # Universal (both v4 and v6)
117+
self.jsv4: str | None = None # v4-specific
118+
self.jsv6: str | None = None # v6-specific
119+
120+
121+
def parse_ci_file(ci_path: Path) -> tuple[Path | None, list[tuple[str, str, JsonExpectation]]]:
104122
"""Parse a CI file for raw messages and json expectations.
105123
106124
Returns:
107-
Tuple of (config_file_path, list of (step, payload_hex, expected_json))
125+
Tuple of (config_file_path, list of (step, payload_hex, expectations))
108126
"""
109127
config_file = None
110-
messages: list[tuple[str, str, str | None]] = []
128+
messages: list[tuple[str, str, JsonExpectation]] = []
111129

112130
# Track raw messages by step to match with json
113131
raw_by_step: dict[str, list[str]] = {}
114-
json_by_step: dict[str, list[str]] = {}
132+
# Track expectations by step, with lists for multiple raw messages per step
133+
expectations_by_step: dict[str, list[JsonExpectation]] = {}
115134

116135
with open(ci_path) as f:
117136
for line in f:
@@ -142,25 +161,95 @@ def parse_ci_file(ci_path: Path) -> tuple[Path | None, list[tuple[str, str, str
142161
raw_by_step[step] = []
143162
raw_by_step[step].append(payload)
144163

145-
# Parse N:json:{...}
146-
json_match = re.match(r'^(\d+):json:(.+)$', line)
147-
if json_match:
148-
step = json_match.group(1)
149-
json_str = json_match.group(2)
150-
if step not in json_by_step:
151-
json_by_step[step] = []
152-
json_by_step[step].append(json_str)
164+
# Parse N:json:, N:jsv4:, N:jsv6: using split
165+
parts = line.split(':', 2)
166+
if len(parts) == 3 and parts[0].isdigit() and parts[1] in ('json', 'jsv4', 'jsv6'):
167+
step, tag, json_str = parts
168+
if step not in expectations_by_step:
169+
expectations_by_step[step] = []
170+
171+
# Get or create expectation for this message index
172+
# Each raw message gets its own expectation
173+
exp_list = expectations_by_step[step]
174+
175+
# Determine if we need a new expectation or can add to existing one
176+
# - json: needs its own expectation (can't mix with jsv4/jsv6)
177+
# - jsv4:/jsv6: should pair together on same expectation
178+
need_new = False
179+
if len(exp_list) == 0:
180+
need_new = True
181+
elif tag == 'json':
182+
# json: starts new if previous has json, jsv4, or jsv6
183+
prev = exp_list[-1]
184+
if prev.json is not None or prev.jsv4 is not None or prev.jsv6 is not None:
185+
need_new = True
186+
elif tag == 'jsv4':
187+
# jsv4: starts new if previous already has jsv4 or json
188+
prev = exp_list[-1]
189+
if prev.jsv4 is not None or prev.json is not None:
190+
need_new = True
191+
elif tag == 'jsv6':
192+
# jsv6: pairs with jsv4 on same expectation
193+
# Start new if previous has json or jsv6 already
194+
prev = exp_list[-1]
195+
if prev.json is not None or prev.jsv6 is not None:
196+
need_new = True
197+
198+
if need_new:
199+
exp_list.append(JsonExpectation())
200+
201+
exp = exp_list[-1]
202+
if tag == 'json':
203+
exp.json = json_str
204+
elif tag == 'jsv4':
205+
exp.jsv4 = json_str
206+
elif tag == 'jsv6':
207+
exp.jsv6 = json_str
153208

154209
# Match raw messages with json expectations
155210
for step, payloads in sorted(raw_by_step.items(), key=lambda x: int(x[0])):
156-
json_list = json_by_step.get(step, [])
211+
exp_list = expectations_by_step.get(step, [])
157212
for i, payload in enumerate(payloads):
158-
expected = json_list[i] if i < len(json_list) else None
159-
messages.append((step, payload, expected))
213+
exp = exp_list[i] if i < len(exp_list) else JsonExpectation()
214+
messages.append((step, payload, exp))
160215

161216
return config_file, messages
162217

163218

219+
def compare_json(decoded: dict, expected_json: str, verbose: bool, step: str, label: str) -> bool:
220+
"""Compare decoded JSON with expected JSON string.
221+
222+
Returns:
223+
True if match, False otherwise
224+
"""
225+
try:
226+
expected = json.loads(expected_json)
227+
normalized_decoded = normalize_json(decoded)
228+
normalized_expected = normalize_json(expected)
229+
230+
if normalized_decoded == normalized_expected:
231+
return True
232+
else:
233+
print(f' {RED}FAIL{RESET} step {step} ({label}): JSON mismatch')
234+
if verbose:
235+
# Find the first difference
236+
exp_str = json.dumps(normalized_expected, sort_keys=True)
237+
got_str = json.dumps(normalized_decoded, sort_keys=True)
238+
for i, (e, g) in enumerate(zip(exp_str, got_str)):
239+
if e != g:
240+
print(f' first diff at char {i}:')
241+
print(f' expected: ...{exp_str[max(0,i-20):i+50]}...')
242+
print(f' got: ...{got_str[max(0,i-20):i+50]}...')
243+
break
244+
else:
245+
# Length difference
246+
print(f' length diff: expected {len(exp_str)}, got {len(got_str)}')
247+
return False
248+
except json.JSONDecodeError:
249+
print(f' {RED}FAIL{RESET} step {step} ({label}): invalid expected JSON')
250+
return False
251+
252+
164253
def test_ci_file(
165254
ci_path: Path, repo_root: Path, verbose: bool = False, generate: bool = False
166255
) -> tuple[int, int, list[str]]:
@@ -178,59 +267,67 @@ def test_ci_file(
178267
failed = 0
179268
generated_lines: list[str] = []
180269

181-
skipped = 0
182-
for step, payload, expected_json in messages:
183-
decoded = decode_message(payload, repo_root, config_file)
184-
185-
if decoded is None:
186-
if verbose:
187-
print(f' {YELLOW}SKIP{RESET} step {step}: decode failed')
188-
skipped += 1
189-
continue
270+
for step, payload, expectations in messages:
271+
# Determine which tests to run based on expectations
272+
# json: = test both v4 and v6 (must match same expectation)
273+
# jsv4: = test v4 only
274+
# jsv6: = test v6 only
275+
has_any = expectations.json or expectations.jsv4 or expectations.jsv6
190276

191277
if generate:
192-
# Generate json: line
193-
# Remove volatile fields for cleaner output
278+
# Generate json: line using v4 encoder (default)
279+
decoded = decode_message(payload, repo_root, config_file, api_version=4)
280+
if decoded is None:
281+
if verbose:
282+
print(f' {YELLOW}SKIP{RESET} step {step}: decode failed')
283+
continue
194284
clean = normalize_json(decoded)
195285
json_line = f'{step}:json:{json.dumps(clean, separators=(",", ":"))}'
196286
generated_lines.append(json_line)
197287
if verbose:
198288
print(f' {GREEN}GEN{RESET} step {step}')
199289
passed += 1
200-
elif expected_json:
201-
# Compare with expected
202-
try:
203-
expected = json.loads(expected_json)
204-
normalized_decoded = normalize_json(decoded)
205-
normalized_expected = normalize_json(expected)
206-
207-
if normalized_decoded == normalized_expected:
208-
passed += 1
290+
elif has_any:
291+
step_passed = True
292+
293+
if expectations.json:
294+
# json: tests v6 only (default API version)
295+
# Use jsv4:/jsv6: to test both API versions explicitly
296+
decoded = decode_message(payload, repo_root, config_file, api_version=6)
297+
if decoded is None:
209298
if verbose:
210-
print(f' {GREEN}PASS{RESET} step {step}')
211-
else:
212-
failed += 1
213-
print(f' {RED}FAIL{RESET} step {step}: JSON mismatch')
299+
print(f' {YELLOW}SKIP{RESET} step {step}: decode failed')
300+
elif not compare_json(decoded, expectations.json, verbose, step, 'json'):
301+
step_passed = False
302+
303+
if expectations.jsv4:
304+
# v4-specific
305+
decoded = decode_message(payload, repo_root, config_file, api_version=4)
306+
if decoded is None:
307+
if verbose:
308+
print(f' {YELLOW}SKIP{RESET} step {step} (jsv4): decode failed')
309+
elif not compare_json(decoded, expectations.jsv4, verbose, step, 'jsv4'):
310+
step_passed = False
311+
312+
if expectations.jsv6:
313+
# v6-specific
314+
decoded = decode_message(payload, repo_root, config_file, api_version=6)
315+
if decoded is None:
214316
if verbose:
215-
# Find the first difference
216-
exp_str = json.dumps(normalized_expected, sort_keys=True)
217-
got_str = json.dumps(normalized_decoded, sort_keys=True)
218-
for i, (e, g) in enumerate(zip(exp_str, got_str)):
219-
if e != g:
220-
print(f' first diff at char {i}:')
221-
print(f' expected: ...{exp_str[max(0,i-20):i+50]}...')
222-
print(f' got: ...{got_str[max(0,i-20):i+50]}...')
223-
break
224-
else:
225-
# Length difference
226-
print(f' length diff: expected {len(exp_str)}, got {len(got_str)}')
227-
except json.JSONDecodeError:
317+
print(f' {YELLOW}SKIP{RESET} step {step} (jsv6): decode failed')
318+
elif not compare_json(decoded, expectations.jsv6, verbose, step, 'jsv6'):
319+
step_passed = False
320+
321+
if step_passed:
322+
passed += 1
323+
if verbose:
324+
print(f' {GREEN}PASS{RESET} step {step}')
325+
else:
228326
failed += 1
229-
print(f' {RED}FAIL{RESET} step {step}: invalid expected JSON')
230327
else:
231328
# No expected json - skip or warn
232329
if verbose:
233-
print(f' {YELLOW}SKIP{RESET} step {step}: no json: expectation')
330+
print(f' {YELLOW}SKIP{RESET} step {step}: no json expectation')
234331

235332
return passed, failed, generated_lines
236333

@@ -357,7 +454,7 @@ def main() -> int:
357454
# Use --generate or --write to process all files
358455
if not args.generate and not args.write:
359456
content = ci_file.read_text()
360-
if ':json:' not in content:
457+
if ':json:' not in content and ':jsv4:' not in content and ':jsv6:' not in content:
361458
continue
362459

363460
rel_path = ci_file.relative_to(repo_root) if ci_file.is_relative_to(repo_root) else ci_file

qa/encoding/conf-flow.ci

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ option:file:conf-flow.conf
55

66
# flow source-ipv4 10.0.0.2/32 next-hop 1.2.3.4 extended-community copy-to-nexthop
77
1:raw:FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:0043:02:0000002C4001010040020040050400000064C010080800000000000001800E100001850401020304000602200A000002
8-
1:json:{"exabgp":"6.0.0","type":"update","neighbor":{"address":{"local":"127.0.0.1","peer":"127.0.0.1"},"asn":{"local":1,"peer":1},"direction":"in","message":{"update":{"attribute":{"origin":"igp","local-preference":100,"extended-community":[{"value":576460752303423489,"string":"copy-to-nexthop"}]},"announce":{"ipv4 flow":{"1.2.3.4":[{"source-ipv4":["10.0.0.2/32"],"next-hop":"1.2.3.4","string":"flow source-ipv4 10.0.0.2/32"}]}}}}}}
8+
1:json:{"exabgp":"6.0.0","type":"update","neighbor":{"address":{"local":"127.0.0.1","peer":"127.0.0.1"},"asn":{"local":1,"peer":1},"direction":"in","message":{"update":{"attribute":{"origin":"igp","local-preference":100,"extended-community":[{"value":576460752303423489,"string":"copy-to-nexthop"}]},"announce":{"ipv4 flow":{"1.2.3.4":[{"source-ipv4":["10.0.0.2/32"],"string":"flow source-ipv4 10.0.0.2/32"}]}}}}}}
99

1010
# flow destination-ipv4 10.0.0.2/32 source-ipv4 10.0.0.1/32 protocol =TCP destination-port =3128 community [ 30740:0 30740:30740 ] extended-community discard
1111
1:raw:FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:0057:02:000000404001010040020040050400000064C008087814000078147814C010088006000000000000800E1900018500001301200A00000202200A00000103810605910C38
@@ -31,11 +31,11 @@ option:file:conf-flow.conf
3131

3232
# flow route-distinguisher 65535:65536 source-ipv4 10.0.0.3/32 next-hop 5.6.7.8 extended-community redirect-to-nexthop
3333
1:raw:FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:004B:02:000000344001010040020040050400000064C010080800000000000000800E180001860405060708000E0000FFFF0001000002200A000003
34-
1:json:{"exabgp":"6.0.0","type":"update","neighbor":{"address":{"local":"127.0.0.1","peer":"127.0.0.1"},"asn":{"local":1,"peer":1},"direction":"in","message":{"update":{"attribute":{"origin":"igp","local-preference":100,"extended-community":[{"value":576460752303423488,"string":"redirect-to-nexthop"}]},"announce":{"ipv4 flow-vpn":{"5.6.7.8":[{"source-ipv4":["10.0.0.3/32"],"rd":"65535:65536","next-hop":"5.6.7.8","string":"flow source-ipv4 10.0.0.3/32 rd 65535:65536"}]}}}}}}
34+
1:json:{"exabgp":"6.0.0","type":"update","neighbor":{"address":{"local":"127.0.0.1","peer":"127.0.0.1"},"asn":{"local":1,"peer":1},"direction":"in","message":{"update":{"attribute":{"origin":"igp","local-preference":100,"extended-community":[{"value":576460752303423488,"string":"redirect-to-nexthop"}]},"announce":{"ipv4 flow-vpn":{"5.6.7.8":[{"source-ipv4":["10.0.0.3/32"],"rd":"65535:65536","string":"flow source-ipv4 10.0.0.3/32 rd 65535:65536"}]}}}}}}
3535

3636
# flow destination-ipv4 192.168.0.1/32 next-hop 1.2.3.4 extended-community redirect-to-nexthop
3737
1:raw:FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:0043:02:0000002C4001010040020040050400000064C010080800000000000000800E10000185040102030400060120C0A80001
38-
1:json:{"exabgp":"6.0.0","type":"update","neighbor":{"address":{"local":"127.0.0.1","peer":"127.0.0.1"},"asn":{"local":1,"peer":1},"direction":"in","message":{"update":{"attribute":{"origin":"igp","local-preference":100,"extended-community":[{"value":576460752303423488,"string":"redirect-to-nexthop"}]},"announce":{"ipv4 flow":{"1.2.3.4":[{"destination-ipv4":["192.168.0.1/32"],"next-hop":"1.2.3.4","string":"flow destination-ipv4 192.168.0.1/32"}]}}}}}}
38+
1:json:{"exabgp":"6.0.0","type":"update","neighbor":{"address":{"local":"127.0.0.1","peer":"127.0.0.1"},"asn":{"local":1,"peer":1},"direction":"in","message":{"update":{"attribute":{"origin":"igp","local-preference":100,"extended-community":[{"value":576460752303423488,"string":"redirect-to-nexthop"}]},"announce":{"ipv4 flow":{"1.2.3.4":[{"destination-ipv4":["192.168.0.1/32"],"string":"flow destination-ipv4 192.168.0.1/32"}]}}}}}}
3939

4040
# flow destination-ipv4 192.168.0.1/32 source-ipv4 10.0.0.2/32 protocol [ =udp =tcp ] port [ =80 =8080 ] destination-port [ >8080&<8088 =3128 ] source-port >1024 origin igp local-preference 100 extended-community redirect:258:33756718
4141
1:raw:FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF:005E:02:000000474001010040020040050400000064C01008800801020203162E800E2B0001850000250120C0A8000102200A0000020301118106040150911F9005121F90541F98910C3806920400

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

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -991,7 +991,8 @@ def __deepcopy__(self, memo: dict[Any, Any]) -> 'Flow':
991991
new._rd_override = deepcopy(self._rd_override, memo) if self._rd_override else None
992992
return new
993993

994-
def json(self, compact: bool = False) -> str:
994+
def _json_core(self, compact: bool = False) -> tuple[str, str, str]:
995+
"""Build JSON core components: rules, rd, compatibility string."""
995996
string: list[str] = []
996997
for index in sorted(self.rules):
997998
rules = self.rules[index]
@@ -1002,12 +1003,21 @@ def json(self, compact: bool = False) -> str:
10021003
s.append(', ')
10031004
s.append('"{}"'.format(rule))
10041005
string.append(' "{}": [ {} ]'.format(rules[0].NAME, ''.join(str(_) for _ in s).replace('""', '')))
1005-
# TODO: nexthop in NLRI.json() is deprecated - should be removed in API v6
1006-
# See plan/api-v6-nexthop-removal.md for migration strategy
1007-
nexthop = ', "next-hop": "{}"'.format(self.nexthop) if self.nexthop is not IP.NoNextHop else ''
1006+
rules_str = ','.join(string)
10081007
rd = '' if self.rd is RouteDistinguisher.NORD else ', {}'.format(self.rd.json())
10091008
compatibility = ', "string": "{}"'.format(self.extensive())
1010-
return '{' + ','.join(string) + rd + nexthop + compatibility + ' }'
1009+
return rules_str, rd, compatibility
1010+
1011+
def json(self, compact: bool = False) -> str:
1012+
"""Serialize Flow NLRI to JSON (v6 format - no nexthop)."""
1013+
rules_str, rd, compatibility = self._json_core(compact)
1014+
return '{' + rules_str + rd + compatibility + ' }'
1015+
1016+
def v4_json(self, compact: bool = False) -> str:
1017+
"""Serialize Flow NLRI to JSON for API v4 backward compatibility (includes nexthop)."""
1018+
rules_str, rd, compatibility = self._json_core(compact)
1019+
nexthop = ', "next-hop": "{}"'.format(self.nexthop) if self.nexthop is not IP.NoNextHop else ''
1020+
return '{' + rules_str + rd + nexthop + compatibility + ' }'
10111021

10121022
@classmethod
10131023
def unpack_nlri(

0 commit comments

Comments
 (0)