fix: Decode DPU network policy overrides#3866
Conversation
|
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. |
WalkthroughThe DPU machine workflow now returns a ChangesDPU machine list contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GetAllDpuMachineHandler
participant TemporalWorkflow
participant GetDpuMachinesByIDs
participant CoreGRPC
GetAllDpuMachineHandler->>TemporalWorkflow: execute GetDpuMachines
TemporalWorkflow->>GetDpuMachinesByIDs: request machines by IDs
GetDpuMachinesByIDs->>CoreGRPC: find DPU machines
CoreGRPC-->>GetDpuMachinesByIDs: return machine data
GetDpuMachinesByIDs-->>TemporalWorkflow: return DpuMachineList
TemporalWorkflow-->>GetAllDpuMachineHandler: return workflow result
GetAllDpuMachineHandler-->>GetAllDpuMachineHandler: decode wrapped or legacy result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Manual verificationManual verification was performed on branch SetupThe REST API, Temporal workers, site agent, Core API, and Machine-a-Tron inventory had to run together because the failure occurred while Temporal decoded a Core protobuf response containing nested git clone https://github.com/kfelternv/infra-controller.git
cd infra-controller
git checkout codex/fix-dpu-network-config-decode
test "$(git rev-parse HEAD)" = d7560b83545d7980d459b486ee171ef074ac1700
kind create cluster --name pr3866 --image kindest/node:v1.31.6
export KUBECONFIG="$PWD/pr3866.kubeconfig"
dev/deployment/devspace/bootstrap-prereqs.sh
devspace deploy -n nico-system --no-colors --max-concurrent-builds 3The charts deployed the exact-head images. The after-deploy helper still checks the obsolete The policy override was setup-only Core configuration. It used the source-defined enum representation so both the source and destination protobuf policy_override='
[[network_security_group.policy_overrides]]
direction = "Egress"
ipv6 = false
protocol = "Tcp"
src_net = { Prefix = "192.0.2.0/24" }
dst_net = { Prefix = "198.51.100.0/24" }
dst_port_start = 443
dst_port_end = 443
action = "Permit"
priority = 10
'
kubectl get configmap nico-api-config-files -n nico-system -o json |
jq --arg addition "$policy_override" '
.data["nico-api-config.toml"] += $addition |
.data["carbide-api-config.toml"] += $addition |
del(.metadata.creationTimestamp, .metadata.managedFields,
.metadata.resourceVersion, .metadata.uid)' |
kubectl apply -f -
kubectl rollout restart deployment/nico-api -n nico-system
kubectl rollout status deployment/nico-api -n nico-system --timeout=300sThe observed setup result was a registered, online local site with two REST-visible simulated hosts. Each host had attached simulated DPUs in Core. An admin CLI was used only for read-only corroboration because the public REST model intentionally omits internal Core policy fields; it was not counted as proof of the REST endpoint. API calls, responses, and proof
|
| Method | Route | Cases exercised | Result |
|---|---|---|---|
GET |
/v2/org/{org}/nico/machine/{machineId}/dpu |
provider success with nested Core policy overrides, repeated GET, unauthenticated, unauthorized tenant, non-host machine ID | PASS |
Read-only persistence corroboration
No direct database INSERT, UPDATE, or DELETE was used. No database query was needed for proof.
Immediately after the successful REST call, the read-only Core command for the returned DPU showed the nested variants that had crossed the Temporal boundary:
kubectl exec deployment/nico-api -n nico-system -- \
/opt/carbide/nico-admin-cli \
-a https://nico-api.nico-system.svc.cluster.local:1079 \
--root-ca-path /var/run/secrets/spiffe.io/ca.crt \
--client-cert-path /var/run/secrets/spiffe.io/tls.crt \
--client-key-path /var/run/secrets/spiffe.io/tls.key \
-f json dpu network config --machine-id "$DPU_ID" |
jq '.network_security_policy_overrides'[
{
"rule": {
"direction": "EGRESS",
"source_net": {"src_prefix": "192.0.2.0/24"},
"destination_net": {"dst_prefix": "198.51.100.0/24"},
"dst_port_start": 443,
"dst_port_end": 443,
"protocol": "TCP",
"action": "PERMIT",
"priority": 10
},
"src_prefixes": ["192.0.2.0/24"],
"dst_prefixes": ["198.51.100.0/24"]
}
]This corroborates that the success response was produced while both nested protobuf oneof values were present. The public REST response intentionally omits these internal Core-only fields.
The focused tests also passed at the exact revision:
go test ./site-workflow/pkg/workflow -run '^TestGetDpuMachinesTestSuite$' -count=1
PASS
go test ./site-workflow/pkg/activity -run '^TestManageMachine_GetDpuMachinesByIDs$' -count=1
PASS
go test ./api/pkg/api/handler -run '^TestMachineHandler_GetDpuMachines$' -count=1
PASS
Cleanup
The API and Keycloak port-forwards were stopped, then only the cluster created for this run was removed:
kind delete cluster --name pr3866Deleting cluster "pr3866" ...
Deleted nodes: ["pr3866-control-plane"]
The development host's pre-existing Kind clusters were still present afterward.
Reproduce
From a clean checkout at d7560b83545d7980d459b486ee171ef074ac1700:
- Create the isolated Kind cluster, bootstrap prerequisites, and deploy the exact revision using the setup commands above.
- Confirm the actual Core and REST deployments are rolled out. Run
rest-api/scripts/setup-local.sh site-agentwith local API and Keycloak port-forwards to register the site and start inventory sync. - Apply the source-defined policy override above and restart
deployment/nico-api. - Obtain the local admin token, list machines through the REST API, and select a returned host ID.
- Call
GET /v2/org/test-org/nico/machine/$HOST_ID/dpu; expect HTTP 200 and a JSON array containing at least one DPU. - Run the read-only Core command above for that DPU; expect both
source_net.src_prefixanddestination_net.dst_prefixinnetwork_security_policy_overrides. - Repeat the GET; expect HTTP 200 with the same DPU ID.
- Repeat without a token, with the local unprivileged tenant token, and with the DPU ID in the host-ID position; expect HTTP 401, 403, and 404 respectively.
- Run the three focused test commands above; each must pass.
- Stop local port-forwards and delete only the
pr3866Kind cluster.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@rest-api/api/pkg/api/model/dpumachine.go`:
- Line 15: Update the comments for the DPU network configuration API model,
including the corresponding comment at the other referenced declaration, to
state that these are network configuration fields exposed by the REST API and
that internal-only and sensitive Core fields are omitted. Keep the documentation
focused on the client-facing REST contract.
🪄 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: b56a2049-adab-4332-89c6-f3c5bbb4553a
⛔ Files ignored due to path filters (4)
rest-api/proto/core/gen/v1/dpu_machine.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/sdk/standard/api_machine.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/model_dpu_machine.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_dpu_network_config.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (9)
rest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/model/dpumachine.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/proto/core/src/v1/dpu_machine.protorest-api/site-workflow/pkg/activity/machine.gorest-api/site-workflow/pkg/workflow/machine.gorest-api/site-workflow/pkg/workflow/machine_test.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7560b8354
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3866.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-07-22 23:01:33 UTC | Commit: d7560b8 |
1 participant
Retrieving attached DPU details fails with HTTP 500 when the managed host network configuration contains network security policy overrides.
GetDpuMachinesreturned a top-level Go slice of protobuf messages, so Temporal selected its generic JSON converter instead of the configured protobuf converter. Generic JSON decoding cannot reconstruct protobufoneoffields such as source and destination prefixes.This change wraps the workflow and activity results in a protobuf
DpuMachineList. That lets the existing Temporal ProtoJSON converter preserve the network-policy overrides across the activity, workflow, and REST handler boundaries. The public REST response remains the same JSON array.The REST model intentionally exposes selected network configuration fields rather than the complete Core configuration, which also contains internal-only and sensitive fields. This change corrects the OpenAPI descriptions and regenerates the SDK comments and rendered API documentation that previously called the public response full or complete.
Related issues
None.
Type of Change
Breaking Changes
Testing
The workflow regression test includes network security policy overrides with source and destination prefix
oneofvalues. It reproduced the reported decode error before the fix and passes with the protobuf list result.Verified with:
go test ./proto/core/gen/v1 ./site-workflow/pkg/activity ./site-workflow/pkg/workflow -count=1go test ./api/pkg/api/model -count=1go test ./api/pkg/api/handler -run '^TestMachineHandler_GetDpuMachines$' -count=1go vet ./site-workflow/pkg/activity ./site-workflow/pkg/workflow ./api/pkg/api/handlermake lint-openapimake generate-sdkmake publish-openapibuf format --diff --exit-code proto/core/src/v1/dpu_machine.protomake check-source-headersgit diff --checkAdditional Notes
Manual verification compared exact pre-fix revision
0e70cc794c9dc2bdef33757de945991658d450b9with current PR headb15632bd6931a04dd474e3439b4081aa228d7cedin the same isolated local Kind and DevSpace deployment. Both revisions used the same simulated host and DPU inventory and the same Core policy override. Core confirmed that the DPU configuration contained bothsource_net.src_prefixanddestination_net.dst_prefixprotobufoneofvalues.The generated non-interactive command was run directly with NICo CLI 2.0.0:
At the pre-fix revision, the command exited 1 with HTTP 500 and the reported
cannot unmarshal ... SourceNetdecode error. At the current PR head, the command exited 0 and returned the attached DPU JSON array with the expected host association,Readystate, and network configuration. The pre-fix CLI binary also exited 0 and returned the same result when pointed at the current server, isolating the correction to the server-side Temporal workflow payload path rather than the CLI or public OpenAPI command shape.Earlier endpoint verification at revision
d7560b83545d7980d459b486ee171ef074ac1700also exercised the provider success, repeated-operation, unauthenticated, unauthorized tenant, and missing-machine cases. They returned HTTP 200, 200, 401, 403, and 404 respectively.