fix: handle nodes, kinds and defaults defined without a body by MaxFreedomPollard · Pull Request #3518 · srl-labs/containerlab

MaxFreedomPollard · 2026-10-23T02:35:56Z

A topology entry with nothing under it unmarshals into a nil *NodeDefinition: a node written as n1:, a kind whose settings are all commented out, or an empty defaults: block. #2835 and #2856 made most of the getters in types/topology.go handle nil node definitions, but a few places still don't.

Three getters return early on a nil node definition, so a node without a body misses part of what it should inherit:

getFieldPtr, behind privileged, enforce-startup-config, suppress-startup-config and auto-remove, returns the defaults value without looking at the group or the kind. For example, privileged: false on a kind leaves its nodes without a body privileged.
GetNodeGroup ignores the group set on the node's kind, so that group's settings never reach the node.
GetNodeConfigDispatcher returns nil, which drops inherited config.vars. A startup-config template that reads .Config.Vars then fails with nil pointer evaluating *types.ConfigDispatcher.Vars.

n1 and n2 below should come out the same, but the code on main resolves n1 to privileged with auto-remove off, and n2 to the kind's values:

topology:
  defaults:
    kind: linux
  kinds:
    linux:
      image: alpine:3
      privileged: false
      auto-remove: true
  nodes:
    n1:
    n2: {}

Kinds and defaults without a body panic. GetKind returns the nil definition of a kind with no body, and GetNodeGroup reads .Group from it. That is the panic in #2859. The issue was closed as fixed by #2856, but #2856 only handled nil nodes, and going by the code on main the topology from the issue still panics in GetNodeGroup, called from GetNodeRuntime in parseTopology. The inventory builders read the result of GetKind the same way. GetNodeKind and GetNodeConfigDispatcher also read t.Defaults directly instead of through GetDefaults(), so an empty defaults: block panics too.

Like #2833 and #2852, this goes back to the getter refactor in #2818. Before it, these lookups went through nil-safe NodeDefinition accessors.

The fix:

getFieldPtr, GetNodeGroup and GetNodeConfigDispatcher guard only the read of the node's own field, as getField and GetNodeType already do.
GetKind returns an empty definition for a kind without a body, as it already does for an unknown kind. An exact match still wins over a case-insensitive one. The GetNodeGroup change depends on this, because it now looks up the kind of nodes without a body.
GetNodeKind and GetNodeConfigDispatcher read the defaults through GetDefaults().

Entries that have a body resolve as before.

TestGetNodeFieldsOfNilNodeDefinition checks what a node with a nil definition inherits from its kind and group: the group, the four pointer-typed fields and the config vars. TestGetNodeFieldsOfNilKindAndDefaults uses a kind with a nil definition and no defaults, and checks GetKind, GetNodeGroup, GetNodeKind and GetNodeConfigDispatcher. Going by the code on main, every case of the first test fails and the second test panics.

I have not compiled or run this: there is no Go toolchain on the machine I worked on. The expected values come from reading the code, and both files parse cleanly with tree-sitter-go. gofmt and golangci-lint were not run.

🤖 Generated with Claude Code

A topology entry with nothing under it, such as a node written as `n1:` or a kind whose settings are all commented out, unmarshals into a nil *NodeDefinition. #2835 and #2856 made most getters cope with nil node definitions, but some places still do not: - getFieldPtr, GetNodeGroup and GetNodeConfigDispatcher return early on a nil node definition, so privileged, enforce-startup-config, suppress-startup-config and auto-remove set on a group or kind, the group set on the node's kind, and inherited config vars never reach the node; - GetKind returns the nil definition of a kind without a body and GetNodeGroup dereferences it, which panics as reported in #2859; - GetNodeKind and GetNodeConfigDispatcher read t.Defaults directly, so a defaults block without a body panics. Only guard the read of the node's own field, return an empty definition for a kind without a body, and read the defaults through GetDefaults(), as the other getters already do. Co-Authored-By: Claude Opus 5.5 <[email protected]>

hellt · 2026-10-23T06:41:26Z

/tofarev review

tofarev · 2026-10-23T06:41:48Z

ToFaRev review — completed

Reviewed eb2316dae741 · Session log · Workflow run

Finding	Location
P4 - Schema rejects bodyless `kinds:` entries that the runtime now treats as supported	schemas/clab.schema.json:3278-3905
P4 - Dead `kind != nil` guards now that `GetKind` never returns nil	types/topology.go:231-253

Detailed findings:

P4 - Schema rejects bodyless kinds: entries that the runtime now treats as supported

Location: schemas/clab.schema.json:3278-3905

Problem: The schema's nodes pattern properties explicitly accept null (oneOf: [{type: null}, node-config]), but every kinds entry is defined as allOf: [$ref node-config, ...] where node-config is "type": "object". A bodyless kind entry (kinds: linux:) therefore fails editor validation even though this PR establishes, and now tests via TestGetNodeFieldsOfNilKindAndDefaults, that the runtime handles nil kind definitions as empty definitions. This strictness predates the PR, but the PR elevates nil-kind handling to a documented-by-test behavior, widening the gap.

Trigger: A user writes kinds: linux: in a topology file and relies on editor live-validation, which per docs/manual/topo-def-file.md is the schema's primary use.

Impact: Low. Users get a spurious validation error for a syntax the runtime silently accepts; no deploy-time breakage since the CLI does not enforce the schema at runtime.

Suggested correction: Wrap the kinds entry refs the same way nodes does (oneOf: [{type: null}, $ref node-config]), or explicitly document that kind entries require a body. Either direction is fine; consistency is the goal.

P4 - Dead kind != nil guards now that GetKind never returns nil

Location: types/topology.go:231-253

Problem: The PR's rewritten GetKind returns new(NodeDefinition) for the nil-kdef case, so the method can no longer return nil by any path. The ten existing if kind != nil guards in types/topology.go (lines 49, 104, 145, 186, 355, 942, 973, 1019, 1076, 1109) are now unreachable conditions. They are behaviorally harmless, since evaluating a field on a zero-value NodeDefinition yields the same result the nil path produced, but they mislead readers into assuming GetKind has a nilable contract, which is the exact confusion this PR's bug stems from.

Trigger: Static: any call to GetKind inside types/topology.go.

Impact: Maintenance only. Future readers or contributors may add redundant nil handling around GetKind results or misjudge which code paths are reachable.

Suggested correction: Either drop the redundant kind != nil clause from the ten guards (keeping secondary conditions such as kind.Config != nil at lines 942/973), or add a doc comment on GetKind stating it always returns a non-nil definition and treat it as the single point of nil-normalization. A one-line comment is the smaller change and preserves the guards as cheap defense.

review conditions

Source inspection only; PR code, tests, and benchmarks were not executed.

All 2 changed paths (types/topology.go, types/topology_test.go) were fully reviewed; all four hunks were diffed against the base revision and all GetKind/GetNodeGroup/GetNodeConfigDispatcher call sites were traced (core/inventory.go:258,466, core/config.go:292-466, and types/topology.go internals).
The two new tests were traced manually against the implementation and pass logically, but were not executed: this is a source-only review snapshot, and no build, tests, or benchmarks were run.
Intentional behavior change (not a defect): bodyless nodes now yield a non-nil NodeConfig.Config with inherited kind/group/defaults vars where base produced nil, so exported node configs for such labs include the inherited config block; no consumer relies on the nil-vs-empty distinction.
The ten dead kind != nil guards evaluate identically under the new zero-value contract; no behavioral regression was found in any caller, including the previously panicking unconditional kindDef.Credentials dereferences in core/inventory.go.
new(expr) in the tests requires Go 1.26; go.mod declares go 1.26.0 and the pattern is pervasive in pre-existing code.
Docs: no user-visible regression; the bodyless-node syntax predates this PR (the schema already allowed null nodes in base) and the fix aligns runtime behavior with the documented inheritance precedence. The bodyless-kinds schema gap is noted as a P4 finding.
Schema: no two-directional breakage introduced; the kinds null-acceptance asymmetry is pre-existing and flagged as an optional improvement.
Long lines in head:docs/images/clabnew.svg exceed the read tool's per-line display limit.
99 additional coverage limitations were recorded.

Powered by

MaxFreedomPollard · 2026-10-27T21:53:09Z

Thanks for running the review. Both findings are P4 and outside the change itself: the schema strictness for bodyless kinds predates this PR, and the kind != nil guards still behave the same. Happy to allow null for kinds in the schema or add a doc comment on GetKind saying it never returns nil, if you want either here. The red fdio_vpp job passed its tests; only the coverage artifact upload failed with ECONNRESET, so a rerun should clear it.

codecov · 2026-11-01T14:07:30Z

Codecov Report

❌ Patch coverage is 26.31579% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.91%. Comparing base (97f7e1d) to head (eb2316d).
⚠️ Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
types/topology.go	26.31%	7 Missing and 7 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3518       +/-   ##
===========================================
- Coverage   61.81%   19.91%   -41.90%     
===========================================
  Files         299      296        -3     
  Lines       36726    35875      -851     
===========================================
- Hits        22701     7144    -15557     
- Misses      11727    27676    +15949     
+ Partials     2298     1055     -1243

Files with missing lines	Coverage Δ
types/topology.go	`71.65% <26.31%> (-21.27%)`	⬇️

... and 233 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

hellt merged commit 91d71cd into srl-labs:main Nov 6, 2026
150 of 152 checks passed

Navigation Menu

Repository navigation

fix: handle nodes, kinds and defaults defined without a body - #3518

fix: handle nodes, kinds and defaults defined without a body#3518
hellt merged 1 commit into
srl-labs:mainfrom
MaxFreedomPollard:fix/definitions-without-body

MaxFreedomPollard commented Oct 23, 2026

Uh oh!

hellt commented Oct 23, 2026

Uh oh!

tofarev Bot commented Oct 23, 2026 •

edited

Loading

Uh oh!

MaxFreedomPollard commented Oct 27, 2026

Uh oh!

codecov Bot commented Nov 1, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

MaxFreedomPollard commented Oct 23, 2026

Uh oh!

hellt commented Oct 23, 2026

Uh oh!

tofarev Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ToFaRev review — completed

Detailed findings:

Uh oh!

MaxFreedomPollard commented Oct 27, 2026

Uh oh!

codecov Bot commented Nov 1, 2026

Codecov Report

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tofarev Bot commented Oct 23, 2026 •

edited

Loading