fix(rest-api): keep expected hardware BMC MACs immutable - #4642
Conversation
Reject physical BMC MAC replacements for expected switches and power shelves while accepting formatting-equivalent reassertions. Preserve the stored identity passed to Core and mark the update fields deprecated in OpenAPI and the generated SDK. Fixes NVIDIA#4362 Signed-off-by: Patrice Breton <pbreton@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
Summary by CodeRabbit
WalkthroughExpected Switch and Expected Power Shelf updates now reject physical BMC MAC changes before mutation. Equivalent MAC formatting remains compatible. ExpectedMachine helper names are generalized, and the OpenAPI contract documents immutable MAC fields. ChangesBMC MAC immutability
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review. |
|
✅ Action performedFull review finished. |
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rest-api/openapi/spec.yaml (1)
24258-24262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing "stored spelling preserved" and "HTTP 400" details to both
bmcMacAddressupdate descriptions.Both descriptions state that a formatting-equivalent MAC is accepted and a physical MAC change is rejected, but neither states two required facts:
- The response code for a rejected physical MAC change is HTTP 400.
- Resubmitting a formatting-equivalent MAC does not change the stored spelling; the originally stored value is preserved.
Add both facts to the
ExpectedPowerShelfUpdateRequest.bmcMacAddressdescription (Line 24260) and theExpectedSwitchUpdateRequest.bmcMacAddressdescription (Line 24824).As per path instructions for
rest-api/openapi/spec.yaml: "Update descriptions must state that clients should omit these fields, formatting-equivalent MAC values remain accepted for compatibility, physical MAC changes are rejected with HTTP 400, and the stored spelling is preserved."📝 Proposed description updates
bmcMacAddress: type: - string - 'null' pattern: '^([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})$' deprecated: true description: |- Immutable MAC address of the Expected Power Shelf's BMC (Baseboard Management Controller). - Omit this field when updating. A formatting-equivalent value is accepted for compatibility, - but changing the physical MAC address is rejected. + Omit this field when updating. A formatting-equivalent value is accepted for compatibility + and the stored spelling is preserved; changing the physical MAC address is rejected with + HTTP 400.Apply the equivalent change to
ExpectedSwitchUpdateRequest.bmcMacAddress.Also applies to: 24822-24826
🤖 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/openapi/spec.yaml` around lines 24258 - 24262, Update the bmcMacAddress descriptions in ExpectedPowerShelfUpdateRequest and ExpectedSwitchUpdateRequest to state that clients should omit the field when updating, formatting-equivalent MAC values remain accepted for compatibility, physical MAC changes are rejected with HTTP 400, and resubmission preserves the originally stored spelling.Source: Path instructions
🤖 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/openapi/spec.yaml`:
- Around line 24258-24262: Update the bmcMacAddress descriptions in
ExpectedPowerShelfUpdateRequest and ExpectedSwitchUpdateRequest to state that
clients should omit the field when updating, formatting-equivalent MAC values
remain accepted for compatibility, physical MAC changes are rejected with HTTP
400, and resubmission preserves the originally stored spelling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9acc80dd-a960-4a12-9b62-77e537c14591
⛔ Files ignored due to path filters (2)
rest-api/sdk/standard/model_expected_power_shelf_update_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_expected_switch_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (8)
rest-api/api/pkg/api/handler/expectedmachine.gorest-api/api/pkg/api/handler/expectedmachine_test.gorest-api/api/pkg/api/handler/expectedpowershelf.gorest-api/api/pkg/api/handler/expectedpowershelf_test.gorest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/expectedswitch_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yaml
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4642.docs.buildwithfern.com/infra-controller |
🔐 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-08-06 05:23:37 UTC | Commit: 159b73c |
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Looks good, thanks @pbreton
2 participants
Expected Switch and Expected Power Shelf PATCH requests currently accept a changed
bmcMacAddressand update Cloud storage, while Core identifies and updates these resources by UUID without changing their physical BMC MAC. A successful request can therefore leave Cloud, Core, and Flow disagreeing about the hardware identity.This change rejects physical BMC MAC replacements after site authorization and before any database or workflow mutation. Omitted values and formatting-equivalent reassertions remain accepted, and the stored spelling is preserved. The implementation reuses the normalized comparison already applied to Expected Machines and documents the update fields as deprecated and immutable in OpenAPI.
Related issues
Fixes #4362
Type of Change
Breaking Changes
Testing
Validation performed:
make rest-api/test-apigo vet ./api/...(cd rest-api/sdk/standard && go test ./... -count=1)make rest-api/lint-openapimake rest-api/generate-sdkmake rest-api/publish-openapioasdiffv1.18.2 (no breaking changes)git diff --check upstream/main...HEADAdditional Notes
This is an attempt at fully automated issue resolution using Codex with Sol 5.6 Medium.