fix(api): preserve automatic networking on legacy instance updates#3594
Conversation
Walkthrough
ChangesLegacy network configuration compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RPCRequest
participant update_instance_config
participant ExistingInstance
RPCRequest->>update_instance_config: legacy auto=true request
update_instance_config->>ExistingInstance: read stored auto_config
ExistingInstance-->>update_instance_config: auto_config or missing
update_instance_config->>update_instance_config: restore auto_config or return InvalidArgument
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇✨ ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/tests/instance_config_update.rs (1)
472-525: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
scenarios!for this rejection matrix. Replace the customRejectCaseloop with the standard table-driven helper so these negative RPC cases match the rest of the test suite.🤖 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-core/src/tests/instance_config_update.rs` around lines 472 - 525, Replace the custom RejectCase struct and loop with the repository’s standard scenarios! table-driven helper, preserving all four network inputs, scenario descriptions, expected InvalidArgument status, and message assertions in the existing rejection test.Source: Coding guidelines
🤖 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-core/src/tests/instance_config_update.rs`:
- Around line 472-525: Replace the custom RejectCase struct and loop with the
repository’s standard scenarios! table-driven helper, preserving all four
network inputs, scenario descriptions, expected InvalidArgument status, and
message assertions in the existing rejection test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b775efeb-d8f3-4d59-a581-5bec9589d6d4
📒 Files selected for processing (2)
crates/api-core/src/handlers/instance.rscrates/api-core/src/tests/instance_config_update.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/tests/instance_config_update.rs (1)
472-525: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
scenarios!orcheck_casesfor the invalid-network cases. The customRejectCaseloop is harder to scan; a table-driven helper would preserve per-row reporting and align this test with the repository’s convention.🤖 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-core/src/tests/instance_config_update.rs` around lines 472 - 525, Replace the custom RejectCase loop with the repository’s scenarios! or check_cases helper for these invalid-network cases. Preserve each case’s scenario name, network input, expected InvalidArgument code, and expected-message assertion, including per-row failure reporting.Source: Coding guidelines
🤖 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-core/src/tests/instance_config_update.rs`:
- Around line 472-525: Replace the custom RejectCase loop with the repository’s
scenarios! or check_cases helper for these invalid-network cases. Preserve each
case’s scenario name, network input, expected InvalidArgument code, and
expected-message assertion, including per-row failure reporting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 600e5935-6f15-4336-8709-c583308ad8c1
📒 Files selected for processing (2)
crates/api-core/src/handlers/instance.rscrates/api-core/src/tests/instance_config_update.rs
Clients that still send the deprecated `InstanceNetworkConfig.auto` boolean can now update an automatically networked instance without losing its immutable VPC. Core recognizes the legacy `auto=true` request and restores the stored `auto_config`, rather than treating the omitted replacement field as a requested network change. Explicit `auto_config` requests remain authoritative, and legacy requests cannot enable or disable automatic networking. Using the instance's stored VPC also keeps the path independent of whether a Flat `HostInband` segment is bound. Tests added! This supports NVIDIA#3592 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
CI note: the ARM ephemeral-image job failed twice in |
2 participants
Instance config updates sent in the legacy
InstanceNetworkConfig.autowire format now preserve the instance's automatic networking, instead of being rejected.The
autoboolean was deprecated in favor ofauto_config, and the allocation path translates a legacyauto: trueinto a realauto_config-- but the update path never did. On update, RPC->model conversion derives auto-ness solely fromauto_config, so a legacy request arrives asauto_config: None; the immutable-network check then compares that against the storedSome(vpc_id)and rejects an otherwise no-op change. It's a rolling-upgrade gap -- any component still emitting the deprecated boolean (an older REST build, or anico-rest-site-agentthat drops the unknown field over Temporal) trips it, which is also why re-sending--auto-network truedoesn't help.The fix extends the allocation-path compatibility to updates, but sources the value differently: instead of re-resolving from the host's segment, it restores the instance's stored
auto_config-- the authoritative value Core already holds. It's scoped tightly to the legacy shape (auto: truewith noauto_configand no explicit interfaces), and the existing guardrails stand:auto: false, a different explicitauto_config.vpc_id, and explicit interfaces all still reject.HostInbandsegment isn't bound to a VPC.Related issues
This supports #3592
Type of Change
Breaking Changes
Testing
test_update_instance_config_restores_deprecated_auto_configcovers the legacy update preserving the VPC, plus the auto-enable / auto-disable / different-VPC / incomplete-auto_configrejects, and asserts the segment is unbound so the compatibility path can't lean on segment resolution. A deprecated-auto reject case was added totest_reject_invalid_instance_config_updates.Additional Notes
cargo test -p carbide-api-core test_update_instance_config_restores_deprecated_auto_config --libcargo test -p carbide-api-core test_reject_invalid_instance_config_updates --lib -- --nocapturecargo make format-nightlycargo make clippycargo make carbide-lintsCloses #3592