Skip to content

feat: decode enterprise-specific Information Elements (RFC 5610) - #4

Merged
matthieugouel merged 3 commits into
mainfrom
feat/rfc5610-enterprise-elements
Nov 4, 2026
Merged

matthieugouel merged 3 commits into
mainfrom
feat/rfc5610-enterprise-elements

Conversation

@matthieugouel

Copy link
Copy Markdown
Member

Summary

Enterprise-specific (vendor) fields were always kept as FieldValue::Raw. Each Decoder now keeps a table of enterprise element definitions, filled from two sources:

  1. Registered by the caller with Decoder::register_enterprise_element, e.g. from vendor documentation. This is the Collecting Process's "own internal definition", which RFC 5610 §3.9 says type records must never replace. Registered definitions apply to all Observation Domains and survive clear(). Registering with PEN 0 also covers IANA elements assigned after the crate's registry snapshot.
  2. Learned from RFC 5610 type records sent by the Exporter. These are Options Data Records whose Scope Fields are privateEnterpriseNumber and informationElementId. They are scoped to the Transport Session and Observation Domain, and apply to the following Data Sets.

The RFC's rules (all MUSTs) are enforced:

  • type records never replace a registered definition or an IANA element
  • conflicting type records disable the element (its fields stay Raw)
  • invalid data type / semantics combinations (§3.10), unknown values, and names or descriptions containing NUL characters (§4) are ignored
  • reverse PEN 29305 cannot be described
  • the learned table is bounded (DecoderConfig::max_enterprise_elements, default 4096), and extra type records are ignored rather than failing the Message

New API: EnterpriseElement, ElementSource, Decoder::{register_enterprise_element, enterprise_element, field_name, learned_element_count}, DataType::from_value, DataTypeSemantics::from_value.

Registration is the part most users will need. None of the 13 vendor samples send type records: vendors document their elements out-of-band.

Test plan

  • 22 new unit tests (tests/unit/enterprise_elements.rs), one per rule above, plus a serde round-trip
  • fuzz_structured now generates type records (including PEN 0, PEN 29305, out-of-registry types and semantics) and enterprise fields with matching PENs: 4.0M runs, no crashes
  • fmt, clippy and rustdoc (-D warnings) clean; 140 tests pass including network ones; coverage 94% (99% on parsers/elements.rs)
  • CI green

🤖 Generated with Claude Code

Each Decoder now keeps a table of enterprise-specific Information Element
definitions, used to decode fields the IANA registry does not describe:

- definitions registered by the caller (register_enterprise_element), the
  Collecting Process's "own internal definition", which always take
  precedence;
- definitions learned from RFC 5610 Information Element type records,
  scoped to the Transport Session and Observation Domain, under the RFC's
  rules: they never replace registered or IANA definitions, conflicting
  records disable the element, invalid data type / semantics combinations
  (Section 3.10) and names with NUL characters are ignored, and the
  learned table is bounded (max_enterprise_elements).

Also adds Decoder::enterprise_element, field_name and
learned_element_count, DataType/DataTypeSemantics::from_value, unit and
serde tests, and RFC 5610 type records in the structured fuzzer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@matthieugouel

matthieugouel commented Nov 4, 2026 •

Copy link
Copy Markdown
Member Author

Claude finished @matthieugouel's task in 34s —— View job


Review of PR #4: enterprise-specific Information Elements (RFC 5610)

I read the src/ and CLAUDE.md diff. I did not build the crate or run the tests, and I did not read the test files.

The core logic looks sound:

  • Precedence: registered definitions win over IANA, and IANA wins over type records.
  • Conflicts: conflicting type records disable the element.
  • Invalid records: NUL characters, bad semantics and PEN 29305 are rejected.
  • Bounds: learning happens after the Data Set decodes, so a failed decode learns nothing, and the learned table is bounded.

I found no panics or unbounded allocations. The points below are in priority order.

CLAUDE.md convention gaps

These come from the "Implementing a New Specification" checklist.

  1. Step 3, "Known Specification Errata": src/parsers/elements.rs documents the RFC rules but has no "Known Specification Errata" section. Add one. If no RFC 5610 errata apply, say so. If you departed from the RFC, say so there too. The semantics table in has_valid_semantics is one such departure, because it extends RFC 5610 §3.10 to lists and unsigned256.
  2. Step 6, spec validation: tests/specs/rfc_structures.rs and tests/specs/download.rs are untouched. RFC 5610 is not in RFC_SPECS, and the type-record structure is not in RFC_STRUCTURES. The test plan also doesn't list make specs-validate. CLAUDE.md requires it to pass with no warnings, so please run it. If EnterpriseElement is deliberately not a wire structure, document that.
  3. Step 5, integration tests: tests/integration_test.rs is untouched. Please confirm no update is needed. A decode with a registered vendor element would be a cheap end-to-end check.

Behaviour worth a second look

  • Slot exhaustion (src/parsers/elements.rs, learn): self.learned.len() < max_learned counts Conflicting markers and all Observation Domains in one shared budget. A single exporter, or one Observation Domain, can send 4096 distinct type records and permanently block learning for every other domain until clear(). The cap protects memory, as intended. Consider a per-domain cap, or eviction on remove_observation_domain, so one noisy domain can't starve the rest.
  • PEN 0 with the enterprise bit set (ElementTable::data_type): a specifier with enterprise_number == Some(0) goes through get(domain, pen, id) with pen 0. That is the same table used for "IANA elements newer than the registry", and it skips the registry lookup. A field carrying the enterprise bit with PEN 0 is not an IANA field, so it gets the IANA-newer definition without the registry check. Decide whether Some(0) should be treated as IANA, and add a test for it.
  • has_valid_semantics (src/models/enterprise.rs): the signed and float arms are stricter than the unsigned arm. Please check them against the RFC 5610 §3.10 table, including whether identifier is allowed for signed types. Add a table-driven test that covers each data type against each semantics value.

Overall this looks good. The items above are mostly documentation and validation steps that CLAUDE.md requires.
· branch feat/rfc5610-enterprise-elements

@codecov

codecov Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

- A field with the Enterprise bit set and PEN 0 (reserved) is no longer
  decoded with PEN 0 definitions (IANA elements newer than the registry);
  it is kept raw, and has no name.
- Validate RFC 5610 Table 1 (data type values) and Table 4 (type record
  elements and Scope Fields) against the implementation in the specs tests.
  The type record element IDs move to the public registry::type_records
  module.
- Reproduce the RFC 5610 Appendix A example as a unit test, with Erratum
  4731. Document in SPEC_ISSUES.md that the example's Field Length of
  65536 should be 65535, which the erratum missed.
- Document RFC 5610 errata and the semantics-restriction deviation (lists,
  unsigned256), and add an exhaustive data type x semantics test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@matthieugouel

Copy link
Copy Markdown
Member Author

Thanks for the review. Here's how each point was handled in the latest push:

CLAUDE.md gaps

  1. Errata: src/parsers/elements.rs now has a "Known Specification Errata" section. It covers RFC 5610 errata 4731 and 1822 (both editorial; neither affects parsing) and documents the has_valid_semantics extension to lists and unsigned256 as a deliberate deviation.

  2. Spec validation: RFC 5610 has no packet figure, since a type record is a regular Options Data Record. It does have two tables the parser depends on, and both are now validated in tests/specs/rfc_tables.rs:

    • Table 1 is checked against DataType.
    • Table 4 is checked against registry::type_records, including which elements are Scope Fields.

    I confirmed the check fails when the scope set is mutated. make specs-validate passes with no warnings.

  3. Integration test: neither the softflowd capture nor the 13 vendor samples contain type records. The vendor samples' enterprise elements aren't publicly documented in a way I could verify, so I didn't register guessed definitions. Instead, RFC 5610 Appendix A (with Erratum 4731) is now reproduced byte for byte as test_rfc5610_appendix_a_example, which runs end to end through Decoder::decode. While doing that I found that the example's Field Length = 65536 should be 65535. The erratum missed it, so it's now in SPEC_ISSUES.md.

Behaviour

  • Shared budget: this is intentional and mirrors max_templates. A Decoder is one Transport Session, so all its Observation Domains belong to the same Exporter. remove_observation_domain already evicts that domain's learned entries. Counting Conflicting markers keeps memory bounded.
  • PEN 0 with the E bit: good catch. Fixed: such a field is now Raw and has no field_name, with a test.
  • has_valid_semantics: I re-checked it against §3.10. Signed types allow everything except flags, so identifier is allowed. Floats exclude identifier and flags. The code was correct, and test_semantics_restrictions_exhaustive now covers every data type against every semantics value.

Covers the 3 patch lines reported by Codecov on #4.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@matthieugouel
matthieugouel merged commit 956ba1d into main Nov 4, 2026
11 checks passed
@matthieugouel
matthieugouel deleted the feat/rfc5610-enterprise-elements branch November 4, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant