fix: reject NVOS MACs already claimed by another expected switch#3293
Open
chet wants to merge 1 commit into
Open
fix: reject NVOS MACs already claimed by another expected switch#3293chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ヽ(・∀・)ノ ✅ Action performedFull review finished. |
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. Summary by CodeRabbit
WalkthroughThis PR enforces NVOS MAC uniqueness across ExpectedSwitch create and update flows. It adds duplicate-MAC error handling, transaction-guarded database checks, normalized filtering in the Go DAO, and REST pre-flight conflict validation with tests. ChangesNVOS MAC Uniqueness Enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-09 00:33:13 UTC | Commit: d41a5e4 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-db/src/expected_switch.rs`:
- Around line 57-99: `find_nvos_mac_claimed_elsewhere` is only doing a pre-write
overlap check, so concurrent `create`/`update` paths can still commit the same
NVOS MAC on multiple switches. Add an atomic guard around the check-and-write
flow in `expected_switch.rs` by either taking a transaction-scoped advisory lock
keyed on the canonicalized NVOS MAC set before calling
`find_nvos_mac_claimed_elsewhere`, or by moving NVOS MACs into a normalized
table with a `UNIQUE` constraint and enforcing the claim there. Ensure the
lock/constraint is applied in the `create` and `update` code paths that rely on
this helper.
In `@rest-api/api/pkg/api/handler/expectedswitch.go`:
- Around line 144-167: The NVOS MAC duplicate check in the Expected Switch write
flow is only a pre-validation and can race with concurrent requests, so the real
uniqueness enforcement must be moved into the persistence layer. Keep the
existing fast-fail lookup in the handler logic around expectedSwitch validation,
but add deterministic protection in the actual create/update path used by the
ExpectedSwitch DAO/service, such as a unique constraint on site plus NVOS MACs
or a transaction-scoped lock/advisory lock, and make the handler surface the
resulting conflict cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f2d4f06-79c1-44d6-8f5e-966b4a33665e
📒 Files selected for processing (9)
crates/api-core/src/errors.rscrates/api-core/src/tests/expected_switch.rscrates/api-db/src/expected_switch.rscrates/api-db/src/expected_switch/tests.rscrates/api-db/src/lib.rsrest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/expectedswitch_test.gorest-api/db/pkg/db/model/expectedswitch.gorest-api/db/pkg/db/model/expectedswitch_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rest-api/db/pkg/db/model/expectedswitch.go (1)
419-429: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFunctional transform blocks index usage on
nvos_mac_addresses.
lower(replace(m.mac, '-', ':'))inside theEXISTS/unnestsubquery is a per-row functional transform, so no index onnvos_mac_addressescan be leveraged — everyNvosMacAddressesfilter forces a full unnest+scan across allexpected_switchesrows. At current expected scale (switches per site) this is likely fine, but if this table grows large consider a functional/GIN expression index matching the normalized form, or normalizing and storing the MACs in a canonical format at write time to allow direct equality/array-overlap lookups.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/db/pkg/db/model/expectedswitch.go` around lines 419 - 429, The `ExpectedSwitchDAO` filter on `NvosMacAddresses` is applying `lower(replace(...))` inside the `EXISTS`/`unnest` query, which prevents index use and forces a full scan. Update the `query.Where(...)` logic in `expectedswitch.go` to use a canonical stored form or an indexable comparison strategy, and if normalization must remain, add a matching functional/GIN index for the normalized MAC representation. Keep the `NvosMacAddresses` tracing attribute unchanged.crates/api-db/src/expected_switch.rs (1)
57-99: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider narrowing the
SELECT *to only what's needed.
find_nvos_mac_claimed_elsewherefetches the entireexpected_switchesrow viaSELECT *just to recompute the intersecting MAC in Rust. A tighter query (e.g. selecting onlynvos_mac_addresses, or computing the overlapping element directly in SQL viaunnest/INTERSECT) would avoid materializing unrelated columns (credentials, metadata, etc.) on every create/update call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/expected_switch.rs` around lines 57 - 99, The query in find_nvos_mac_claimed_elsewhere is overfetching with SELECT * when only the overlapping nvos_mac_addresses value is needed. Update the sqlx::query_as call and the SQL strings to fetch only the minimal data required, or compute the shared MAC directly in SQL, so the ExpectedSwitch row is not fully materialized on every create/update path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/api-db/src/expected_switch.rs`:
- Around line 57-99: The query in find_nvos_mac_claimed_elsewhere is
overfetching with SELECT * when only the overlapping nvos_mac_addresses value is
needed. Update the sqlx::query_as call and the SQL strings to fetch only the
minimal data required, or compute the shared MAC directly in SQL, so the
ExpectedSwitch row is not fully materialized on every create/update path.
In `@rest-api/db/pkg/db/model/expectedswitch.go`:
- Around line 419-429: The `ExpectedSwitchDAO` filter on `NvosMacAddresses` is
applying `lower(replace(...))` inside the `EXISTS`/`unnest` query, which
prevents index use and forces a full scan. Update the `query.Where(...)` logic
in `expectedswitch.go` to use a canonical stored form or an indexable comparison
strategy, and if normalization must remain, add a matching functional/GIN index
for the normalized MAC representation. Keep the `NvosMacAddresses` tracing
attribute unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f83c2a1-0352-46e7-a1e1-19d9db296c35
📒 Files selected for processing (9)
crates/api-core/src/errors.rscrates/api-core/src/tests/expected_switch.rscrates/api-db/src/expected_switch.rscrates/api-db/src/expected_switch/tests.rscrates/api-db/src/lib.rsrest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/expectedswitch_test.gorest-api/db/pkg/db/model/expectedswitch.gorest-api/db/pkg/db/model/expectedswitch_test.go
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(=^･ω･^=)ﾉ ✅ Action performedFull review finished. |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/expected_switch.rs (1)
71-113: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNarrow the conflict-check query — avoid pulling credentials into memory for a MAC-only check.
find_nvos_mac_claimed_elsewhererunsSELECT *and deserializes a fullExpectedSwitch— includingbmc_passwordandnvos_password— solely to readnvos_mac_addresses. This is wasted I/O per call (now on the hot path of everycreate/update) and needlessly widens the surface area handling plaintext credentials in memory for a helper whose only output is a MAC address.♻️ Proposed fix: select only the column that's needed
let (sql, exclude_key) = match switch.expected_switch_id { Some(id) => ( - "SELECT * FROM expected_switches WHERE expected_switch_id != $1::uuid AND nvos_mac_addresses && $2::macaddr[] LIMIT 1", + "SELECT nvos_mac_addresses FROM expected_switches WHERE expected_switch_id != $1::uuid AND nvos_mac_addresses && $2::macaddr[] LIMIT 1", id.to_string(), ), None => ( - "SELECT * FROM expected_switches WHERE bmc_mac_address != $1::macaddr AND nvos_mac_addresses && $2::macaddr[] LIMIT 1", + "SELECT nvos_mac_addresses FROM expected_switches WHERE bmc_mac_address != $1::macaddr AND nvos_mac_addresses && $2::macaddr[] LIMIT 1", switch.bmc_mac_address.to_string(), ), }; - let other: Option<ExpectedSwitch> = sqlx::query_as(sql) + let other_macs: Option<Vec<MacAddress>> = sqlx::query_scalar(sql) .bind(exclude_key) .bind(nvos_mac_addresses) .fetch_optional(txn) .await .map_err(|err| DatabaseError::query(sql, err))?; - Ok(other.map(|other| { + Ok(other_macs.map(|other_macs| { nvos_mac_addresses .iter() - .find(|mac| other.nvos_mac_addresses.contains(mac)) + .find(|mac| other_macs.contains(mac)) .copied() // The SQL overlap guarantees a shared entry; the first requested // MAC is a safe stand-in if equality ever disagrees. .unwrap_or(nvos_mac_addresses[0]) }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/expected_switch.rs` around lines 71 - 113, find_nvos_mac_claimed_elsewhere currently uses sqlx::query_as with SELECT * and deserializes a full ExpectedSwitch even though it only needs nvos_mac_addresses; narrow the query to fetch just the needed column and avoid loading bmc_password/nvos_password into memory. Keep the existing conflict logic and return type the same, but update the query, result mapping, and any related binding/row handling in find_nvos_mac_claimed_elsewhere so it only reads the MAC data required for the overlap check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/api-db/src/expected_switch.rs`:
- Around line 71-113: find_nvos_mac_claimed_elsewhere currently uses
sqlx::query_as with SELECT * and deserializes a full ExpectedSwitch even though
it only needs nvos_mac_addresses; narrow the query to fetch just the needed
column and avoid loading bmc_password/nvos_password into memory. Keep the
existing conflict logic and return type the same, but update the query, result
mapping, and any related binding/row handling in find_nvos_mac_claimed_elsewhere
so it only reads the MAC data required for the overlap check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f94102b1-3771-4ffd-be73-dde54070e28e
📒 Files selected for processing (11)
crates/api-core/src/errors.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/tests/expected_switch.rscrates/api-db/src/expected_switch.rscrates/api-db/src/expected_switch/tests.rscrates/api-db/src/lib.rsrest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/expectedswitch_test.gorest-api/api/pkg/api/model/expectedswitch.gorest-api/db/pkg/db/model/expectedswitch.gorest-api/db/pkg/db/model/expectedswitch_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/api-db/src/lib.rs
- rest-api/db/pkg/db/model/expectedswitch_test.go
- rest-api/api/pkg/api/handler/expectedswitch.go
- crates/api-core/src/errors.rs
- rest-api/db/pkg/db/model/expectedswitch.go
- rest-api/api/pkg/api/handler/expectedswitch_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rest-api/db/pkg/db/model/expectedswitch.go (1)
413-437: 🚀 Performance & Scalability | 🔵 TrivialCase-insensitive
EXISTS/unnestpredicate can't use an index onnvos_mac_addresses.The
lower(replace(m.mac, '-', ':'))transformation inside the subquery forces a per-row function scan over the array for every candidate switch. Today this is always paired with aSiteIDsfilter in the call sites, which bounds the scan to a single site's switches, so this is fine at current scale. IfNvosMacAddressesis ever used unscoped (e.g. a future cross-site search), consider a functional/GIN index or storing a normalized column to keep this indexable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/db/pkg/db/model/expectedswitch.go` around lines 413 - 437, The `NvosMacAddresses` filter in `expectedswitch.go` uses a case-insensitive `EXISTS`/`unnest` predicate in the expected switch query, and the `lower(replace(...))` expression prevents index use on `nvos_mac_addresses`. Keep the current behavior in `NormalizeMacAddress`/the `ExpectedSwitch` query path, but if `filter.NvosMacAddresses` is ever used without the existing `SiteIDs` constraint, update the model/query strategy by adding a functional or GIN index, or by storing a normalized MAC field, so the lookup stays indexable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rest-api/db/pkg/db/model/expectedswitch.go`:
- Around line 413-437: The `NvosMacAddresses` filter in `expectedswitch.go` uses
a case-insensitive `EXISTS`/`unnest` predicate in the expected switch query, and
the `lower(replace(...))` expression prevents index use on `nvos_mac_addresses`.
Keep the current behavior in `NormalizeMacAddress`/the `ExpectedSwitch` query
path, but if `filter.NvosMacAddresses` is ever used without the existing
`SiteIDs` constraint, update the model/query strategy by adding a functional or
GIN index, or by storing a normalized MAC field, so the lookup stays indexable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 26f6bb96-174e-48fa-beab-f45ffb0a9063
📒 Files selected for processing (11)
crates/api-core/src/errors.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/tests/expected_switch.rscrates/api-db/src/expected_switch.rscrates/api-db/src/expected_switch/tests.rscrates/api-db/src/lib.rsrest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/expectedswitch_test.gorest-api/api/pkg/api/model/expectedswitch.gorest-api/db/pkg/db/model/expectedswitch.gorest-api/db/pkg/db/model/expectedswitch_test.go
Expected-switch create and update now enforce that an NVOS MAC address belongs to at most one expected switch, at both entry layers: Core's `AddExpectedSwitch`/`UpdateExpectedSwitch` reject with `FailedPrecondition` (covering the CLI, config-driven seeding, and REST via the site workflow), and the REST API pre-checks with a `409 Conflict` naming the conflicting record's `id`. Core's discovery path assumes exactly this invariant -- `find_by_nvos_mac_address` resolves a DHCPing NVOS port to a single expected switch -- so two switches claiming one MAC make static `nvos_ip_address` reservations land on an arbitrary switch and fan out `--nvos-mac` switch searches; a copy-paste error at registration time that only surfaces much later as a mis-addressed management port. - The Core guard lives in `db::expected_switch::create`/`update` (a `find_nvos_mac_claimed_elsewhere` overlap query plus the `ExpectedSwitchDuplicateNvosMacAddress` error), so every writer inherits it and a transaction-scoped advisory lock keeps the check-then-write deterministic under concurrent registrations; Postgres `macaddr` comparison canonicalizes case and separator differences, and `update_nvos_mac_addresses` stays unguarded since it records hardware-observed truth from site-explorer. - The REST handlers run the conflict read before the transaction opens and return the conflicting `id` in the 409 payload; `ExpectedSwitchFilterInput` gains `NvosMacAddresses` (case- and separator-insensitive `unnest` overlap) and `ExcludeExpectedSwitchIDs` so updates can exempt the switch being updated. - Updates only check newly claimed MACs, so re-asserting a list a switch already holds stays valid at both layers -- even for overlaps that predate the guard -- and an omitted or empty `nvosMacAddresses` skips the checks entirely. Tests added! This supports NVIDIA#3267 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An NVOS MAC is how discovery resolves a DHCPing switch management port to its expected switch -- the lookup assumes exactly one owner, but nothing enforced that. Two expected switches could silently claim the same MAC at registration, and the mistake only surfaced much later as a mis-addressed management port or duplicated switch-search rows. This PR makes the invariant real at both entry layers: Core rejects the conflict at the source of truth, and the REST API fails fast with a friendly
409 Conflict.Core (
crates/)db::expected_switch::create/update, so every writer inherits it -- the CLI, config-driven seeding, and REST via the site workflow.find_nvos_mac_claimed_elsewhereoverlap query backs the check; Postgresmacaddrcomparison canonicalizes case and separator differences.FailedPreconditionvia the newExpectedSwitchDuplicateNvosMacAddresserror.update_nvos_mac_addressesstays unguarded on purpose: it records hardware-observed truth from site-explorer.ReplaceAllExpectedSwitches) surfaces an intra-list duplicate as the sameFailedPreconditionconflict.REST API (
rest-api/)409 Conflictnaming the conflicting record'sid.ExpectedSwitchFilterInputgainsNvosMacAddresses(case- and separator-insensitive overlap) andExcludeExpectedSwitchIDsso updates can exempt the switch being updated.Behavior notes
nvosMacAddressesskips the checks, since nothing new is claimed.Tests added!
This supports #3267