Skip to content

fix: validate expected interface roles and fixed addresses - #4255

Merged
chet merged 1 commit into
NVIDIA:mainfrom
chet:gh-issue-3990-followup
Jul 28, 2026
Merged

fix: validate expected interface roles and fixed addresses#4255
chet merged 1 commit into
NVIDIA:mainfrom
chet:gh-issue-3990-followup

Conversation

@chet

@chet chet commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Reject duplicate expected interface entries when one MAC names different roles, since DHCP and Site Explorer would otherwise interpret the same interface differently.

Explicit fixed allocation policies and DPU roles now require fixed_ip to fall within a configured managed prefix. Existing exact reservations must also belong to that segment, while legacy Host entries that omit the new policy fields keep the static-assignments fallback.

Related issues

This supports #3990

Follow-up to #3991.

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Validated with focused model, Core, database, and generated Go protobuf tests, plus cargo make format-nightly, cargo make check-format-nightly, cargo make clippy, and cargo make carbide-lints.

The uncommitted diff was also reviewed with CodeRabbit CLI and the local Claude review helper. Their actionable findings are included in this commit.

Additional Notes

This PR has no database migration or protobuf schema change. The generated Go protobuf diff contains comment updates only.

ExpectedMachine declarations may still be stored before their network segments exist. Strict managed-prefix validation happens when the reservation is materialized or updated.

@chet
chet requested a review from a team as a code owner July 28, 2026 18:58

chet commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full_review, thanks!

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@chet, I’ll perform a complete re-review of #4255, including the interface-role conflict handling and managed-prefix validation paths.

🐇🔍

✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Expected-interface allocation now distinguishes legacy Host behavior from managed-prefix-required fixed allocation. Segment resolution, preallocation reconciliation, duplicate-MAC role validation, API updates, tests, and related CLI/protobuf documentation are updated accordingly.

Changes

Expected interface allocation

Layer / File(s) Summary
Allocation contracts and input validation
crates/api-model/src/expected_machine.rs, crates/api-core/src/handlers/expected_machine.rs, crates/admin-cli/src/expected_machines/add/args.rs, crates/rpc/proto/forge.proto, rest-api/proto/core/src/v1/nico_nico.proto
Legacy Host allocation is identified explicitly, managed-prefix semantics are documented, and duplicate MAC entries are accepted only when their roles match.
Managed segment resolution and preallocation
crates/api-db/src/network_segment.rs, crates/api-db/src/machine_interface.rs, crates/api-db/src/machine_interface/tests.rs
Strict managed-prefix lookup is separated from static-assignments fallback, while preallocation reconciliation enforces the resolved target segment.
Expected-machine update integration
crates/api-core/src/handlers/machine_interface_address.rs, crates/api-core/src/tests/expected_machine.rs
Expected-machine updates propagate the managed-prefix requirement and verify single and batch fixed-interface behavior, including rejection and persistence cases.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ExpectedMachineUpdate
  participant validate_expected_interface_role_and_allocation
  participant update_preallocated_expected_machine_interface
  participant preallocate_expected_machine_interface
  participant for_managed_static_address
  ExpectedMachineUpdate->>validate_expected_interface_role_and_allocation: validate host NIC roles and allocations
  ExpectedMachineUpdate->>update_preallocated_expected_machine_interface: apply expected interface settings
  update_preallocated_expected_machine_interface->>preallocate_expected_machine_interface: pass require_managed_prefix
  preallocate_expected_machine_interface->>for_managed_static_address: resolve fixed IP segment
  for_managed_static_address-->>preallocate_expected_machine_interface: return managed segment or InvalidArgument
  preallocate_expected_machine_interface-->>ExpectedMachineUpdate: materialize or reject interface update
Loading

Suggested labels: networking, api

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the core change: validating expected interface roles and fixed addresses.
Description check ✅ Passed The description matches the change set, covering duplicate-role validation, managed-prefix checks, and legacy fallback behavior.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Reject duplicate expected interface entries when one MAC names different roles, since DHCP and Site Explorer would otherwise interpret the same interface differently.

Explicit fixed allocation policies and DPU roles now require fixed_ip to fall within a configured managed prefix. Existing exact reservations must also belong to that segment, while legacy Host entries that omit the new policy fields keep the static-assignments fallback.

This supports NVIDIA#3990

Signed-off-by: Chet Nichols III <chetn@nvidia.com>
@chet
chet force-pushed the gh-issue-3990-followup branch from ca0f83d to b34f01d Compare July 28, 2026 18:59
@github-actions

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-07-28 19:02:30 UTC | Commit: b34f01d

@chet
chet enabled auto-merge (squash) July 28, 2026 20:45
@chet
chet merged commit 55f1a03 into NVIDIA:main Jul 28, 2026
120 checks passed
@chet
chet deleted the gh-issue-3990-followup branch July 28, 2026 20:48
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.

2 participants