feat(config): reject unknown fields in NICo API configuration files - #4598
feat(config): reject unknown fields in NICo API configuration files#4598hwadekar-nv wants to merge 4 commits into
Conversation
Summary by CodeRabbit
WalkthroughConfiguration loading now detects unknown fields across merged sources and applies the ChangesConfiguration validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigSources
participant ConfigLoader
participant Figment
participant CarbideConfig
ConfigSources->>ConfigLoader: provide base, site, and environment values
ConfigLoader->>Figment: merge sources and extract unknown fields
Figment->>CarbideConfig: deserialize strict configuration
CarbideConfig-->>ConfigLoader: return configuration or validation error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/libmlx/src/firmware/config.rs (1)
539-551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a serde round-trip test for the profile.
The new test proves rejection of an unknown key. It does not prove that the manual deserializer stays aligned with the
Serializederive or with the three nested section structs. A field added later toFlashSpecand omitted fromFlatFirmwareFlasherProfilewould be silently unsupported in configuration, and no existing test would fail. The tests at lines 424-513 cover only the protobuf conversion.Add one assertion that serializes a fully populated profile and deserializes it back.
💚 Suggested round-trip guard
#[test] fn profile_toml_rejects_unknown_fields() {Add after the existing test:
#[test] fn profile_serde_round_trip_preserves_every_field() { let original = FirmwareFlasherProfile { firmware_spec: FirmwareSpec { part_number: "900-9D3B4-00CV-TA0".to_string(), psid: "MT_0000000884".to_string(), version: "32.43.1014".to_string(), }, flash_spec: FlashSpec { firmware_url: "https://artifacts.nvidia.com/fw.bin".to_string(), firmware_credentials: Some(Credentials::bearer_token("token123")), device_conf_url: Some("https://artifacts.nvidia.com/debug.conf".to_string()), device_conf_credentials: Some(Credentials::basic_auth("user", "pass")), verify_from_cache: true, cache_dir: Some(PathBuf::from("/var/cache/fw")), }, flash_options: FlashOptions { verify_image: true, verify_version: true, reset: true, reset_level: 5, }, }; let encoded = toml::to_string(&original).expect("profile serializes"); let decoded = FirmwareFlasherProfile::from_toml(&encoded) .expect("serialized profile must deserialize under the strict schema"); assert_eq!(decoded.firmware_spec.version, original.firmware_spec.version); assert_eq!(decoded.flash_spec.cache_dir, original.flash_spec.cache_dir); assert_eq!(decoded.flash_options.reset_level, original.flash_options.reset_level); }🤖 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/libmlx/src/firmware/config.rs` around lines 539 - 551, Add a new test beside profile_toml_rejects_unknown_fields that constructs a fully populated FirmwareFlasherProfile, serializes it with toml::to_string, and deserializes it through FirmwareFlasherProfile::from_toml. Assert representative fields from FirmwareSpec, FlashSpec, and FlashOptions—including nested credentials, cache_dir, and reset_level—match the original, guarding manual deserialization against future schema drift.crates/api-core/src/cfg/file.rs (1)
3649-3674: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rejection reason in the strict-schema table.
The
runclosure discards the error with.map_err(drop). ThereforeFailsproves only that extraction failed, not that it failed because of the unknown field. Several rows are vulnerable to this:[host_models.test-model]and[mlx-config-profiles.test]supply hand-written required fields, so a schema drift inFirmwareorMlxConfigProfilewould keep the test green for the wrong reason.Map the error to its
Kindso the assertion binds to the intended failure. The dedicated test at lines 3726-3751 already demonstrates the pattern.♻️ Suggested strengthening of the failure assertion
- run = |patch| Figment::new() - .merge(Toml::file(format!("{TEST_DATA_DIR}/min_config.toml"))) - .merge(Toml::string(patch)) - .extract::<CarbideConfig>() - .map(drop) - .map_err(drop); + run = |patch| Figment::new() + .merge(Toml::file(format!("{TEST_DATA_DIR}/min_config.toml"))) + .merge(Toml::string(patch)) + .extract::<CarbideConfig>() + .map(drop) + .map_err(|error| match error.kind { + Kind::UnknownField(field, _) => field, + other => panic!("expected an unknown-field rejection, got {other:?}"), + });🤖 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/cfg/file.rs` around lines 3649 - 3674, Update the strict-schema table’s `run` closure to preserve and inspect extraction errors instead of discarding them with `.map_err(drop)`. Map the error to its `Kind` and assert that each `Fails` case specifically reports the unknown-field validation failure, following the pattern in the dedicated test around the existing strict-schema tests; ensure rows such as `host_models.test-model` and `mlx-config-profiles.test` cannot pass due to unrelated schema errors.
🤖 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-core/src/cfg/file.rs`:
- Around line 2896-2907: Expand deserialize_shipped_deployment_config to
strictly extract every shipped CarbideConfig, including the Helm and development
API configurations and the deployed site overlay. Rename the deployment
configuration consistently from nico-api-config.toml to carbide-api-config.toml
across the test and deployment references so the base Kustomization assembles
the existing artifact. Reuse the integration fixture extraction coverage where
applicable.
In `@crates/libmlx/src/firmware/credentials.rs`:
- Line 49: Update the enum containing the SshAgent variant to represent it as an
empty struct variant instead of a unit variant, ensuring Serde’s
deny_unknown_fields rejects extra fields. Add deserialization tests covering
valid SshAgent input and rejection of unknown fields, while preserving the
existing tagged snake_case representation.
In `@deploy/nico-base/api/config-files/nico-api-config.toml`:
- Line 37: The DPU NIC firmware allowlists in
deploy/nico-base/api/config-files/nico-api-config.toml lines 37-37 and
helm/charts/nico-api/files/carbide-api-config.toml lines 32-32 must align with
the configured BF2/BF3 firmware baseline. Update both
dpu_nic_firmware_update_versions lists to the intended 24.47.2682 and 32.47.2682
releases, or explicitly configure matching firmware defaults if the older
compatibility boundary is deliberate.
---
Nitpick comments:
In `@crates/api-core/src/cfg/file.rs`:
- Around line 3649-3674: Update the strict-schema table’s `run` closure to
preserve and inspect extraction errors instead of discarding them with
`.map_err(drop)`. Map the error to its `Kind` and assert that each `Fails` case
specifically reports the unknown-field validation failure, following the pattern
in the dedicated test around the existing strict-schema tests; ensure rows such
as `host_models.test-model` and `mlx-config-profiles.test` cannot pass due to
unrelated schema errors.
In `@crates/libmlx/src/firmware/config.rs`:
- Around line 539-551: Add a new test beside profile_toml_rejects_unknown_fields
that constructs a fully populated FirmwareFlasherProfile, serializes it with
toml::to_string, and deserializes it through FirmwareFlasherProfile::from_toml.
Assert representative fields from FirmwareSpec, FlashSpec, and
FlashOptions—including nested credentials, cache_dir, and reset_level—match the
original, guarding manual deserialization against future schema drift.
🪄 Autofix
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: b9ceb9e3-b65d-4395-b671-a583ba4ff3b4
📒 Files selected for processing (41)
book/src/configuration/configurability.mdcrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/cfg/test_data/full_config.tomlcrates/api-core/src/cfg/test_data/full_config_post_migration.tomlcrates/api-core/src/cfg/test_data/site_config.tomlcrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/machine/mod.rscrates/api-model/src/network_security_group/mod.rscrates/api-model/src/network_segment/mod.rscrates/api-model/src/rack_type.rscrates/api-model/src/resource_pool/define.rscrates/api-model/src/vpc/mod.rscrates/api-test-helper/src/api_server.rscrates/authn/src/config.rscrates/component-manager/src/config.rscrates/dpa-manager/src/config.rscrates/dpf/src/types.rscrates/ib-fabric/src/config.rscrates/libmlx/src/firmware/config.rscrates/libmlx/src/firmware/credentials.rscrates/libmlx/src/profile/serialization.rscrates/machine-controller/src/config/bom_validation.rscrates/machine-controller/src/config/controller.rscrates/machine-controller/src/config/firmware_global.rscrates/machine-controller/src/config/machine_validation.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/config/power_manager.rscrates/nras/src/lib.rscrates/nvlink-manager/src/config.rscrates/rack-controller/src/config.rscrates/site-explorer/src/config.rscrates/site-explorer/tests/site_explorer.rscrates/state-controller-common/src/config.rsdeploy/nico-base/api/config-files/nico-api-config.tomldev/docker-env/carbide-api-config.tomlhelm/charts/nico-api/files/carbide-api-config.toml
💤 Files with no reviewable changes (6)
- crates/api-test-helper/src/api_server.rs
- crates/api-core/src/cfg/test_data/site_config.toml
- crates/api-core/src/cfg/test_data/full_config_post_migration.toml
- book/src/configuration/configurability.md
- crates/api-core/src/cfg/test_data/full_config.toml
- dev/docker-env/carbide-api-config.toml
4de3a12 to
7dd6392
Compare
|
I'm wondering about the case where someone might have a config, leave a deprecated setting in there, introduce the new setting (so they're both side by side), and then eventually if we get rid of the old setting, strict rejection would cause the API to fail to start (because they still had the old setting in there as a fallback). I guess that would come into the realm of backwards compatibility. If the idea is to be backwards compatible "forever", would that also mean old config options must persist forever? Or are we only concerned about backwards compatibility with the API (gRPC and REST)? Usually if I want to change a config parameter, I guess I'll serde alias the thing so the old one still works. There's also the case for pre-configuring something that you know is coming -- in the past, we'll have a new flag coming into the API server, so we'll pre-deploy value(s) out to all sites, knowing that the API server will just ignore unknown values, and then once the new build arrives, it will pick up the new value. It allows config + binary to go out independently and not break anything. Maybe we could have an actual |
These were the two points I thought of as well. This would make feature flagging a lot more difficult if we had to coordinate the |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4598.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/api-core/src/cfg/load.rs (1)
66-107: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueWell-guarded loop; consider bounding the retry cost.
The termination property is correct. Each iteration removes exactly one key, and the
remove_value_at_pathfailure path returns the original error instead of looping forever. That is the important detail and it is handled.One operational note: the function performs a full
T::deserializepass for every unknown field. For a configuration withkunknown keys, the cost isk + 1deserializations of the completeCarbideConfigtree. Site configurations are small, so this is acceptable today. If a migration ever introduces a large batch of removed keys, the boot path pays for it. A short comment recording the intended bound would help the next reader.🤖 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/cfg/load.rs` around lines 66 - 107, Add a concise comment immediately before the retry loop in extract_with_unknown_fields documenting that each iteration removes one unknown key, so deserialization runs at most once per removed key plus the final successful attempt. Do not change the existing loop behavior.crates/api-core/src/cfg/file.rs (1)
4820-4867: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoad the development configurations at compile time.
The helper mixes two access strategies.
rendered_helm_api_configandrendered_deployment_site_configuseinclude_str!, which fails the build if a file is renamed or moved.deploy_path,docker_path, andwebdev_pathuse runtime paths withToml::file. Figment treats a missing file as an empty provider, so a renamed development configuration produces a silent coverage gap rather than a failure. The deployment case is protected by the version assertion at line 4834; the Docker and web-development cases are not.Use
include_str!withToml::stringfor all three sources so the compiler pins the paths.♻️ Proposed change to pin the configuration paths
- let repository_root = concat!(env!("CARGO_MANIFEST_DIR"), "/../.."); - let deploy_path = - format!("{repository_root}/deploy/nico-base/api/config-files/nico-api-config.toml"); - let docker_path = format!("{repository_root}/dev/docker-env/carbide-api-config.toml"); - let webdev_path = format!("{repository_root}/dev/webdev-env/carbide-api-config.toml"); + let deploy_config_source = + include_str!("../../../../deploy/nico-base/api/config-files/nico-api-config.toml"); + let docker_config_source = + include_str!("../../../../dev/docker-env/carbide-api-config.toml"); + let webdev_config_source = + include_str!("../../../../dev/webdev-env/carbide-api-config.toml");Then replace each
Toml::file(&path)withToml::string(source).🤖 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/cfg/file.rs` around lines 4820 - 4867, Update deserialize_shipped_api_configurations to load deploy_path, docker_path, and webdev_path via compile-time include_str! sources and Toml::string providers instead of runtime path strings and Toml::file. Preserve the existing configuration combinations and deployment version assertion while ensuring renamed or missing configuration files fail compilation.
🤖 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/site-explorer/src/config.rs`:
- Line 34: Remove the deprecated override_target_port field from
SiteExplorerConfig deserialization and delete the corresponding check in the API
configuration loading logic that enables dynamic bmc_proxy changes. Ensure
site-explorer neither reads nor honors this setting, while preserving only the
documented force_dpu_nic_mode compatibility exception.
---
Nitpick comments:
In `@crates/api-core/src/cfg/file.rs`:
- Around line 4820-4867: Update deserialize_shipped_api_configurations to load
deploy_path, docker_path, and webdev_path via compile-time include_str! sources
and Toml::string providers instead of runtime path strings and Toml::file.
Preserve the existing configuration combinations and deployment version
assertion while ensuring renamed or missing configuration files fail
compilation.
In `@crates/api-core/src/cfg/load.rs`:
- Around line 66-107: Add a concise comment immediately before the retry loop in
extract_with_unknown_fields documenting that each iteration removes one unknown
key, so deserialization runs at most once per removed key plus the final
successful attempt. Do not change the existing loop behavior.
🪄 Autofix
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: b6faa139-906b-4edc-9d5a-63532c288982
📒 Files selected for processing (50)
book/src/configuration/configurability.mdcrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/cfg/load.rscrates/api-core/src/cfg/provenance.rscrates/api-core/src/cfg/test_data/full_config.tomlcrates/api-core/src/cfg/test_data/full_config_post_migration.tomlcrates/api-core/src/cfg/test_data/site_config.tomlcrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/machine/mod.rscrates/api-model/src/network_security_group/mod.rscrates/api-model/src/network_segment/mod.rscrates/api-model/src/rack_type.rscrates/api-model/src/resource_pool/define.rscrates/api-model/src/vpc/mod.rscrates/api-model/src/vpc/routing_profile.rscrates/api-test-helper/src/api_server.rscrates/api/src/run.rscrates/authn/src/config.rscrates/component-manager/src/config.rscrates/dpa-manager/src/config.rscrates/dpf/src/types.rscrates/ib-fabric/src/config.rscrates/libmlx/src/firmware/config.rscrates/libmlx/src/firmware/credentials.rscrates/libmlx/src/firmware/source.rscrates/libmlx/src/profile/serialization.rscrates/libmlx/tests/firmware/test_credentials.rscrates/machine-controller/src/config/bom_validation.rscrates/machine-controller/src/config/controller.rscrates/machine-controller/src/config/firmware_global.rscrates/machine-controller/src/config/machine_validation.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/config/power_manager.rscrates/nras/src/lib.rscrates/nvlink-manager/src/config.rscrates/rack-controller/src/config.rscrates/scout/src/mlx_device.rscrates/site-explorer/src/config.rscrates/site-explorer/tests/integration/site_explorer.rscrates/state-controller-common/src/config.rsdeploy/nico-base/api/config-files/nico-api-config.tomldeploy/nico-base/api/deployment.yamldeploy/nico-base/api/kustomization.yamldev/docker-env/carbide-api-config.tomldev/webdev-env/carbide-api-config.tomlhelm/charts/nico-api/files/carbide-api-config.toml
💤 Files with no reviewable changes (6)
- crates/api-core/src/cfg/test_data/full_config.toml
- crates/api-core/src/cfg/test_data/site_config.toml
- crates/api-test-helper/src/api_server.rs
- crates/api-core/src/cfg/test_data/full_config_post_migration.toml
- book/src/configuration/configurability.md
- dev/docker-env/carbide-api-config.toml
🚧 Files skipped from review as they are similar to previous changes (27)
- crates/api-model/src/network_security_group/mod.rs
- crates/libmlx/src/profile/serialization.rs
- crates/api-core/src/machine_update_manager/dpu_nic_firmware.rs
- crates/nvlink-manager/src/config.rs
- crates/authn/src/config.rs
- crates/api-model/src/resource_pool/define.rs
- crates/machine-controller/src/config/controller.rs
- crates/dpf/src/types.rs
- crates/ib-fabric/src/config.rs
- crates/machine-controller/src/config/power_manager.rs
- crates/component-manager/src/config.rs
- crates/machine-controller/src/config/firmware_global.rs
- deploy/nico-base/api/config-files/nico-api-config.toml
- crates/dpa-manager/src/config.rs
- crates/api-core/src/cfg/README.md
- crates/machine-controller/src/config/mod.rs
- crates/state-controller-common/src/config.rs
- crates/api-model/src/machine/mod.rs
- crates/machine-controller/src/config/bom_validation.rs
- crates/nras/src/lib.rs
- helm/charts/nico-api/files/carbide-api-config.toml
- crates/rack-controller/src/config.rs
- crates/api-model/src/network_segment/mod.rs
- crates/api-model/src/vpc/mod.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/api-model/src/rack_type.rs
- crates/machine-controller/src/config/machine_validation.rs
c8cda0e to
363c98a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-core/src/cfg/README.md`:
- Line 32: Update the deny_unknown_fields entry in the configuration README to
document that base TOML, site TOML, and CARBIDE_API_DENY_UNKNOWN_FIELDS may set
it, with precedence base TOML < site TOML < environment. Clarify that the
effective merged value applies to unknown fields from all merged sources: true
fails startup, while false logs warnings and continues.
🪄 Autofix
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: 09d7181d-2573-4f10-85ff-f7aa7e6d1cfa
📒 Files selected for processing (50)
book/src/configuration/configurability.mdcrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/cfg/load.rscrates/api-core/src/cfg/provenance.rscrates/api-core/src/cfg/test_data/full_config.tomlcrates/api-core/src/cfg/test_data/full_config_post_migration.tomlcrates/api-core/src/cfg/test_data/site_config.tomlcrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/machine/mod.rscrates/api-model/src/network_security_group/mod.rscrates/api-model/src/network_segment/mod.rscrates/api-model/src/rack_type.rscrates/api-model/src/resource_pool/define.rscrates/api-model/src/vpc/mod.rscrates/api-model/src/vpc/routing_profile.rscrates/api-test-helper/src/api_server.rscrates/api/src/run.rscrates/authn/src/config.rscrates/component-manager/src/config.rscrates/dpa-manager/src/config.rscrates/dpf/src/types.rscrates/ib-fabric/src/config.rscrates/libmlx/src/firmware/config.rscrates/libmlx/src/firmware/credentials.rscrates/libmlx/src/firmware/source.rscrates/libmlx/src/profile/serialization.rscrates/libmlx/tests/firmware/test_credentials.rscrates/machine-controller/src/config/bom_validation.rscrates/machine-controller/src/config/controller.rscrates/machine-controller/src/config/firmware_global.rscrates/machine-controller/src/config/machine_validation.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/config/power_manager.rscrates/nras/src/lib.rscrates/nvlink-manager/src/config.rscrates/rack-controller/src/config.rscrates/scout/src/mlx_device.rscrates/site-explorer/src/config.rscrates/site-explorer/tests/integration/site_explorer.rscrates/state-controller-common/src/config.rsdeploy/nico-base/api/config-files/nico-api-config.tomldeploy/nico-base/api/deployment.yamldeploy/nico-base/api/kustomization.yamldev/docker-env/carbide-api-config.tomldev/webdev-env/carbide-api-config.tomlhelm/charts/nico-api/files/carbide-api-config.toml
💤 Files with no reviewable changes (6)
- crates/api-test-helper/src/api_server.rs
- crates/api-core/src/cfg/test_data/site_config.toml
- crates/api-core/src/cfg/test_data/full_config_post_migration.toml
- book/src/configuration/configurability.md
- crates/api-core/src/cfg/test_data/full_config.toml
- dev/docker-env/carbide-api-config.toml
🚧 Files skipped from review as they are similar to previous changes (42)
- crates/machine-controller/src/config/mod.rs
- dev/webdev-env/carbide-api-config.toml
- crates/api-model/src/network_security_group/mod.rs
- crates/site-explorer/tests/integration/site_explorer.rs
- crates/machine-controller/src/config/power_manager.rs
- crates/machine-controller/src/config/controller.rs
- deploy/nico-base/api/deployment.yaml
- crates/api/src/run.rs
- crates/libmlx/tests/firmware/test_credentials.rs
- crates/dpf/src/types.rs
- crates/rack-controller/src/config.rs
- crates/scout/src/mlx_device.rs
- crates/machine-controller/src/config/bom_validation.rs
- crates/libmlx/src/profile/serialization.rs
- crates/authn/src/config.rs
- crates/api-model/src/resource_pool/define.rs
- crates/api-model/src/network_segment/mod.rs
- crates/machine-controller/src/config/machine_validation.rs
- crates/nvlink-manager/src/config.rs
- crates/api-model/src/rack_type.rs
- crates/libmlx/src/firmware/source.rs
- deploy/nico-base/api/config-files/nico-api-config.toml
- crates/component-manager/src/config.rs
- crates/ib-fabric/src/config.rs
- crates/libmlx/src/firmware/config.rs
- crates/machine-controller/src/config/firmware_global.rs
- deploy/nico-base/api/kustomization.yaml
- crates/nras/src/lib.rs
- crates/dpa-manager/src/config.rs
- helm/charts/nico-api/files/carbide-api-config.toml
- crates/api-core/src/cfg/provenance.rs
- crates/api-core/src/machine_update_manager/dpu_nic_firmware.rs
- crates/state-controller-common/src/config.rs
- crates/api-core/src/cfg/load.rs
- crates/api-model/src/firmware.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/site-explorer/src/config.rs
- crates/api-model/src/vpc/routing_profile.rs
- crates/libmlx/src/firmware/credentials.rs
- crates/api-model/src/vpc/mod.rs
- crates/api-model/src/machine/mod.rs
- crates/api-core/src/cfg/file.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/nico-base/api/config-files/carbide-api-config.toml (1)
58-62: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winComplete the relocated configuration-path contract.
The Deployment mounts
nico-api-config-filesat/etc/forge/carbide-api, butcasbin_policy_filestill points to/etc/nico/nico-api/casbin-policy.csv. The configured file does not resolve to the ConfigMap-provided policy.
deploy/nico-base/api/config-files/carbide-api-config.toml#L58-L62: Updateadmin_root_cafile_pathandcasbin_policy_fileto deployed mount paths, or add explicit mounts for the legacy paths.deploy/README.md#L102-L106: Specify the required Secret or ConfigMap key and its mount path. Distinguish the SPIFFEroot_cafile_pathfrom any external admin root CA file.As per coding guidelines, document interface contracts completely, including sources, paths, requiredness, and fallback behavior.
🤖 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 `@deploy/nico-base/api/config-files/carbide-api-config.toml` around lines 58 - 62, Update deploy/nico-base/api/config-files/carbide-api-config.toml at lines 58-62 so admin_root_cafile_path and casbin_policy_file use the deployed /etc/forge/carbide-api mount paths, or provide explicit legacy-path mounts. Update deploy/README.md at lines 102-106 to document each required Secret or ConfigMap key, its mount path, requiredness, and fallback behavior, clearly distinguishing the SPIFFE root_cafile_path from the external admin root CA file.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.
Outside diff comments:
In `@deploy/nico-base/api/config-files/carbide-api-config.toml`:
- Around line 58-62: Update
deploy/nico-base/api/config-files/carbide-api-config.toml at lines 58-62 so
admin_root_cafile_path and casbin_policy_file use the deployed
/etc/forge/carbide-api mount paths, or provide explicit legacy-path mounts.
Update deploy/README.md at lines 102-106 to document each required Secret or
ConfigMap key, its mount path, requiredness, and fallback behavior, clearly
distinguishing the SPIFFE root_cafile_path from the external admin root CA file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 074a4502-6b53-4770-8317-1ea288837573
📒 Files selected for processing (4)
crates/api-core/src/cfg/file.rsdeploy/README.mddeploy/nico-base/api/config-files/carbide-api-config.tomldeploy/nico-base/api/deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-core/src/cfg/file.rs
|
@coderabbitai full_review ! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/site-explorer/src/config.rs (1)
34-34:⚠️ Potential issue | 🟠 MajorRemove the deprecated
override_target_portcompatibility.
#[serde(deny_unknown_fields)]does not reject this key whileSiteExplorerConfigstill declares it. The loader also still uses it when enabling dynamicbmc_proxychanges. Remove the field from deserialization and remove the downstream check. Keep only the documentedforce_dpu_nic_modecompatibility exception.Based on learnings, site-explorer must not read, honor, or generate behavior based on
site_explorer.override_target_port.🤖 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/site-explorer/src/config.rs` at line 34, Remove the deprecated override_target_port field from SiteExplorerConfig deserialization and delete the downstream logic that checks or applies it when enabling dynamic bmc_proxy changes. Ensure site-explorer no longer reads, honors, or generates behavior from site_explorer.override_target_port, while preserving only the documented force_dpu_nic_mode compatibility exception.Source: Learnings
🧹 Nitpick comments (2)
crates/api-core/src/cfg/README.md (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine a removal boundary for the temporary
force_dpu_nic_modeexception.The paragraph documents the key as accepted and warned, but it gives no end date for that acceptance. The corresponding declaration in
crates/api-core/src/cfg/file.rs(lines 348-350) also describes the acceptance as temporary. Operators need a concrete boundary to plan migration. State the target release or the support boundary after which the key is rejected.📝 Proposed documentation change
The removed `force_dpu_nic_mode` key is explicitly recognized at the top level and under `[site_explorer]`, ignored, and reported as a deprecation warning. -Use `site_explorer.dpu_policy` instead. +Use `site_explorer.dpu_policy` instead. The key is accepted until <release>, +after which configurations that still set it fail to load.As per coding guidelines: "Temporary claims and hard-coded tool or dependency versions must identify a release, support boundary, tested compatibility boundary, or authoritative release URL".
🤖 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/cfg/README.md` around lines 16 - 18, Update the documentation paragraph describing the deprecated force_dpu_nic_mode key to state the concrete release or support boundary after which the key will be rejected, aligned with the temporary-acceptance declaration in the cfg handling. Retain the migration guidance to site_explorer.dpu_policy.Source: Coding guidelines
crates/api-core/src/cfg/file.rs (1)
4922-4931: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider rendering the chart with Helm instead of string replacement.
The line filter removes every line that begins with
{{-. Control-flow blocks such as{{- if ... }}are removed, but the guarded body lines are retained. The resulting document is therefore the union of all conditional branches, which may never be produced by a real values file. The assertion at line 4926 keeps the helper honest about unreplaced expressions, so the current failure mode is a loud test break.If the chart gains conditional sections, prefer a
helm templatestep in CI over extending this replacement table.🤖 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/cfg/file.rs` around lines 4922 - 4931, Replace the line-based Helm directive removal in the config-rendering helper with rendering the chart through Helm using the intended values file, rather than treating template control-flow lines as comments. Preserve the existing assertion that no Helm expressions remain in the rendered result, and return the actual Helm-rendered configuration so conditional branches match real chart output.
🤖 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-core/src/cfg/file.rs`:
- Around line 4956-4990: Update the development configuration checks around the
Docker and web development cases to compile-time include their TOML files with
include_str!, ensuring missing or renamed files fail compilation instead of
being treated as empty providers. Reuse the resulting contents through
Toml::string while preserving the existing database_url merge for the Docker
case and the current strict-schema extraction behavior.
In `@crates/api-model/src/network_security_group/mod.rs`:
- Line 197: Remove #[serde(deny_unknown_fields)] from NetworkSecurityGroupRule
and introduce a separate strict configuration type for validating
network_security_groups.rules input, while keeping persisted JSONB
deserialization backward-compatible with legacy or extra fields.
---
Duplicate comments:
In `@crates/site-explorer/src/config.rs`:
- Line 34: Remove the deprecated override_target_port field from
SiteExplorerConfig deserialization and delete the downstream logic that checks
or applies it when enabling dynamic bmc_proxy changes. Ensure site-explorer no
longer reads, honors, or generates behavior from
site_explorer.override_target_port, while preserving only the documented
force_dpu_nic_mode compatibility exception.
---
Nitpick comments:
In `@crates/api-core/src/cfg/file.rs`:
- Around line 4922-4931: Replace the line-based Helm directive removal in the
config-rendering helper with rendering the chart through Helm using the intended
values file, rather than treating template control-flow lines as comments.
Preserve the existing assertion that no Helm expressions remain in the rendered
result, and return the actual Helm-rendered configuration so conditional
branches match real chart output.
In `@crates/api-core/src/cfg/README.md`:
- Around line 16-18: Update the documentation paragraph describing the
deprecated force_dpu_nic_mode key to state the concrete release or support
boundary after which the key will be rejected, aligned with the
temporary-acceptance declaration in the cfg handling. Retain the migration
guidance to site_explorer.dpu_policy.
🪄 Autofix
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: 6b0749c2-db60-4ce3-9e37-b5c4bca878f7
📒 Files selected for processing (50)
book/src/configuration/configurability.mdcrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/cfg/load.rscrates/api-core/src/cfg/provenance.rscrates/api-core/src/cfg/test_data/full_config.tomlcrates/api-core/src/cfg/test_data/full_config_post_migration.tomlcrates/api-core/src/cfg/test_data/site_config.tomlcrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/machine/mod.rscrates/api-model/src/network_security_group/mod.rscrates/api-model/src/network_segment/mod.rscrates/api-model/src/rack_type.rscrates/api-model/src/resource_pool/define.rscrates/api-model/src/vpc/mod.rscrates/api-model/src/vpc/routing_profile.rscrates/api-test-helper/src/api_server.rscrates/api/src/run.rscrates/authn/src/config.rscrates/component-manager/src/config.rscrates/dpa-manager/src/config.rscrates/dpf/src/types.rscrates/ib-fabric/src/config.rscrates/libmlx/src/firmware/config.rscrates/libmlx/src/firmware/credentials.rscrates/libmlx/src/firmware/source.rscrates/libmlx/src/profile/serialization.rscrates/libmlx/tests/firmware/test_credentials.rscrates/machine-controller/src/config/bom_validation.rscrates/machine-controller/src/config/controller.rscrates/machine-controller/src/config/firmware_global.rscrates/machine-controller/src/config/machine_validation.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/config/power_manager.rscrates/nras/src/lib.rscrates/nvlink-manager/src/config.rscrates/rack-controller/src/config.rscrates/scout/src/mlx_device.rscrates/site-explorer/src/config.rscrates/site-explorer/tests/integration/site_explorer.rscrates/state-controller-common/src/config.rsdeploy/README.mddeploy/nico-base/api/config-files/carbide-api-config.tomldeploy/nico-base/api/deployment.yamldev/docker-env/carbide-api-config.tomldev/webdev-env/carbide-api-config.tomlhelm/charts/nico-api/files/carbide-api-config.toml
💤 Files with no reviewable changes (6)
- dev/docker-env/carbide-api-config.toml
- crates/api-core/src/cfg/test_data/full_config.toml
- book/src/configuration/configurability.md
- crates/api-core/src/cfg/test_data/full_config_post_migration.toml
- crates/api-test-helper/src/api_server.rs
- crates/api-core/src/cfg/test_data/site_config.toml
| let deploy_path = | ||
| format!("{repository_root}/deploy/nico-base/api/config-files/carbide-api-config.toml"); | ||
| let docker_path = format!("{repository_root}/dev/docker-env/carbide-api-config.toml"); | ||
| let webdev_path = format!("{repository_root}/dev/webdev-env/carbide-api-config.toml"); | ||
| let site_config = rendered_deployment_site_config(); | ||
| let helm_config = rendered_helm_api_config(); | ||
|
|
||
| let deploy_config = Figment::new() | ||
| .merge(Toml::file(&deploy_path)) | ||
| .extract::<CarbideConfig>() | ||
| .expect("the shipped deployment config must match the strict schema"); | ||
| assert_eq!( | ||
| deploy_config.dpu_config.dpu_nic_firmware_update_versions, | ||
| [BF2_NIC_VERSION.to_string(), BF3_NIC_VERSION.to_string()] | ||
| ); | ||
|
|
||
| for (name, figment) in [ | ||
| ( | ||
| "deployment base plus site override", | ||
| Figment::new() | ||
| .merge(Toml::file(&deploy_path)) | ||
| .merge(Toml::string(&site_config)), | ||
| ), | ||
| ( | ||
| "Docker development", | ||
| Figment::new() | ||
| .merge(Toml::file(&docker_path)) | ||
| .merge(Toml::string( | ||
| r#"database_url = "postgres://test:test@localhost/test""#, | ||
| )), | ||
| ), | ||
| ( | ||
| "web development", | ||
| Figment::new().merge(Toml::file(&webdev_path)), | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the development configuration files exist before extracting.
Toml::file treats a missing path as an empty provider rather than an error. The "Docker development" case merges a database_url on top of docker_path, so a renamed or moved file yields an empty base document and the case still passes. The strict-schema assertion then validates nothing.
The deploy_path case is protected by the dpu_nic_firmware_update_versions assertion at lines 4967-4970. Give the other file-based cases the same protection. include_str! fails at compile time and matches the approach already used by rendered_helm_api_config and rendered_deployment_site_config.
🛡️ Proposed fix using compile-time inclusion
- let docker_path = format!("{repository_root}/dev/docker-env/carbide-api-config.toml");
- let webdev_path = format!("{repository_root}/dev/webdev-env/carbide-api-config.toml");
+ let docker_config = include_str!("../../../../dev/docker-env/carbide-api-config.toml");
+ let webdev_config = include_str!("../../../../dev/webdev-env/carbide-api-config.toml"); (
"Docker development",
Figment::new()
- .merge(Toml::file(&docker_path))
+ .merge(Toml::string(docker_config))
.merge(Toml::string(
r#"database_url = "postgres://test:test@localhost/test""#,
)),
),
(
"web development",
- Figment::new().merge(Toml::file(&webdev_path)),
+ Figment::new().merge(Toml::string(webdev_config)),
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let deploy_path = | |
| format!("{repository_root}/deploy/nico-base/api/config-files/carbide-api-config.toml"); | |
| let docker_path = format!("{repository_root}/dev/docker-env/carbide-api-config.toml"); | |
| let webdev_path = format!("{repository_root}/dev/webdev-env/carbide-api-config.toml"); | |
| let site_config = rendered_deployment_site_config(); | |
| let helm_config = rendered_helm_api_config(); | |
| let deploy_config = Figment::new() | |
| .merge(Toml::file(&deploy_path)) | |
| .extract::<CarbideConfig>() | |
| .expect("the shipped deployment config must match the strict schema"); | |
| assert_eq!( | |
| deploy_config.dpu_config.dpu_nic_firmware_update_versions, | |
| [BF2_NIC_VERSION.to_string(), BF3_NIC_VERSION.to_string()] | |
| ); | |
| for (name, figment) in [ | |
| ( | |
| "deployment base plus site override", | |
| Figment::new() | |
| .merge(Toml::file(&deploy_path)) | |
| .merge(Toml::string(&site_config)), | |
| ), | |
| ( | |
| "Docker development", | |
| Figment::new() | |
| .merge(Toml::file(&docker_path)) | |
| .merge(Toml::string( | |
| r#"database_url = "postgres://test:test@localhost/test""#, | |
| )), | |
| ), | |
| ( | |
| "web development", | |
| Figment::new().merge(Toml::file(&webdev_path)), | |
| ), | |
| let deploy_path = | |
| format!("{repository_root}/deploy/nico-base/api/config-files/carbide-api-config.toml"); | |
| let docker_config = include_str!("../../../../dev/docker-env/carbide-api-config.toml"); | |
| let webdev_config = include_str!("../../../../dev/webdev-env/carbide-api-config.toml"); | |
| let site_config = rendered_deployment_site_config(); | |
| let helm_config = rendered_helm_api_config(); | |
| let deploy_config = Figment::new() | |
| .merge(Toml::file(&deploy_path)) | |
| .extract::<CarbideConfig>() | |
| .expect("the shipped deployment config must match the strict schema"); | |
| assert_eq!( | |
| deploy_config.dpu_config.dpu_nic_firmware_update_versions, | |
| [BF2_NIC_VERSION.to_string(), BF3_NIC_VERSION.to_string()] | |
| ); | |
| for (name, figment) in [ | |
| ( | |
| "deployment base plus site override", | |
| Figment::new() | |
| .merge(Toml::file(&deploy_path)) | |
| .merge(Toml::string(&site_config)), | |
| ), | |
| ( | |
| "Docker development", | |
| Figment::new() | |
| .merge(Toml::string(docker_config)) | |
| .merge(Toml::string( | |
| r#"database_url = "postgres://test:test@localhost/test""#, | |
| )), | |
| ), | |
| ( | |
| "web development", | |
| Figment::new().merge(Toml::string(webdev_config)), | |
| ), |
🤖 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/cfg/file.rs` around lines 4956 - 4990, Update the
development configuration checks around the Docker and web development cases to
compile-time include their TOML files with include_str!, ensuring missing or
renamed files fail compilation instead of being treated as empty providers.
Reuse the resulting contents through Toml::string while preserving the existing
database_url merge for the Docker case and the current strict-schema extraction
behavior.
| /// single rule that will be applied on a DPU to restrict | ||
| /// traffic. | ||
| #[derive(Debug, Clone, Deserialize, Serialize, PartialEq)] | ||
| #[serde(deny_unknown_fields)] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate every deserialization site for NetworkSecurityGroupRule.
set -eu
rg -n -C 6 'NetworkSecurityGroupRule' crates --type=rust -g '!**/target/**'Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- direct deserialization and persistence paths ---'
rg -n -C 4 \
'Json<Vec<NetworkSecurityGroupRule>>|serde_json::(from|to)_|rules:.*NetworkSecurityGroupRule|NetworkSecurityGroupRule.*rules|rules.*json' \
crates/api-model/src/network_security_group/mod.rs \
crates/api-db/src/network_security_group.rs \
crates/rpc/src/model/network_security_group.rs \
crates/api-core/src \
--type=rust
printf '%s\n' '--- request model and proto rule definitions ---'
rg -n -C 5 \
'NetworkSecurityGroupRuleAttributes|network_security_group_rule|NetworkSecurityGroupRule' \
crates/rpc crates/api-core crates/api-model \
--glob '*.proto' --glob '*.rs' | head -n 500
printf '%s\n' '--- database schema and migrations mentioning network_security_groups.rules ---'
rg -n -C 5 \
'network_security_groups|rules.*json|jsonb.*rules|rules.*jsonb' \
. --glob '*.sql' --glob '*.rs' --glob '*.toml' | head -n 500Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- model definition and database conversion ---'
sed -n '189,213p;315,350p' crates/api-model/src/network_security_group/mod.rs
sed -n '40,90p;410,455p' crates/api-db/src/network_security_group.rs
printf '%s\n' '--- all exact type references, without broad context ---'
rg -n --glob '*.rs' \
'NetworkSecurityGroupRule([, >]|::|$)|Json<Vec<NetworkSecurityGroupRule>>' \
crates/api-core crates/api-db crates/api-model crates/rpc
printf '%s\n' '--- request-to-model conversion symbols ---'
rg -n -C 8 --glob '*.rs' \
'NetworkSecurityGroupRuleAttributes|NetworkSecurityGroupAttributes|rules\(\)|\.rules|rules:' \
crates/api-core crates/rpc/src/model/network_security_group.rs | head -n 600
printf '%s\n' '--- migration definitions for the rules column ---'
rg -n -C 8 --glob '*.sql' \
'CREATE TABLE.*network_security_groups|network_security_groups|rules' \
migrations crates 2>/dev/null | head -n 400Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
import json
model = Path("crates/api-model/src/network_security_group/mod.rs").read_text()
rpc_model = Path("crates/rpc/src/model/network_security_group.rs").read_text()
handler = Path("crates/api-core/src/handlers/network_security_group.rs").read_text()
rule = re.search(
r"#\[derive\([^]]*Deserialize[^]]*\)\]\s*"
r"#\[serde\(deny_unknown_fields\)\]\s*"
r"pub struct NetworkSecurityGroupRule\s*\{(?P<body>.*?)\n\}",
model,
re.S,
)
assert rule, "strict NetworkSecurityGroupRule definition not found"
fields = set(re.findall(r"pub\s+([A-Za-z0-9_]+)\s*:", rule.group("body")))
assert "rules" not in fields
assert "priority" in fields
assert "sqlx::types::Json<Vec<NetworkSecurityGroupRule>>" in model
assert "impl TryFrom<rpc::NetworkSecurityGroupRuleAttributes> for NetworkSecurityGroupRule" in rpc_model
assert ".map(|r| r.try_into())" in handler
payload = {
name: None for name in fields
}
payload["priority"] = 100
payload["misspelled_priority"] = 100
unknown = set(payload) - fields
print(json.dumps({
"strict_model_fields": sorted(fields),
"persisted_json_deserializes_as": "Vec<NetworkSecurityGroupRule>",
"client_rule_conversion": "rpc::NetworkSecurityGroupRuleAttributes -> NetworkSecurityGroupRule",
"unknown_payload_fields": sorted(unknown),
"persisted_row_with_unknown_field": "rejected by deny_unknown_fields",
}, sort_keys=True))
PYRepository: NVIDIA/infra-controller
Length of output: 624
Use a separate strict configuration type
NetworkSecurityGroupRule is also deserialized from network_security_groups.rules JSONB. deny_unknown_fields rejects any existing row with legacy or extra fields. Use a dedicated strict configuration type, or migrate persisted rows before enabling this attribute.
🤖 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-model/src/network_security_group/mod.rs` at line 197, Remove
#[serde(deny_unknown_fields)] from NetworkSecurityGroupRule and introduce a
separate strict configuration type for validating network_security_groups.rules
input, while keeping persisted JSONB deserialization backward-compatible with
legacy or extra fields.
3 participants
Description
NICo currently ignores unknown TOML configuration fields without any diagnostic, allowing typos, removed settings, and unsupported options to silently produce unintended runtime behavior.
This PR reports unknown fields across top-level and nested fixed-schema configuration types with an actionable key path and source. Unknown fields log warnings by default so configuration and binary rollouts remain independent. Operators can set
deny_unknown_fields = trueto reject them during startup.force_dpu_nic_modekey remains temporarily accepted with a warning.Related issues
Fixes #4527
Type of Change
Breaking Changes
Unknown fields continue to be accepted by default, but now emit warnings. Strict startup rejection is opt-in through
deny_unknown_fields = true. Known fields with invalid types or values remain fatal in both modes.Testing
Tests cover warn-by-default and strict modes, top-level and nested fields, site overrides, environment-derived configuration, shipped configuration, initial objects, NSG policy, SuperNIC, MLX profiles, and source attribution.
Additional Notes