patch(v2.0): Decode DPU network policy overrides#4070
Conversation
Fixes an HTTP 500 when retrieving attached DPU details for machines with network security policy overrides by preserving protobuf fields through the Temporal workflow. The REST response stays the same, and the API documentation now makes clear that only selected DPU network configuration fields are exposed. None. - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) - [ ] **This PR contains breaking changes** - [x] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) Signed-off-by: Kyle Felter <kfelter@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. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
What changedThis backport carries the DPU list through Temporal in a protobuf wrapper so network-policy Scenario and setupI tested head The local site contained a simulated host with an attached DPU. For the regression case, Core was configured with both source and destination network-policy variants. The public REST model intentionally omits those internal fields, so the Core read below corroborates setup; the REST call is the behavior under test. The rollout-compatibility row used the exact API with the pre-fix site-agent and a network configuration without policy overrides, matching the compatibility contract from the original review. The commands below use local test values and require the caller to export short-lived tokens and simulated machine IDs: API_URL="${API_URL:?set to the local API URL}"
ORG="test-org"
HOST_ID="${HOST_ID:?set to the simulated host ID}"
DPU_ID="${DPU_ID:?set to its attached DPU ID}"
ADMIN_TOKEN="${ADMIN_TOKEN:?set from the local provider-admin login}"
TENANT_TOKEN="${TENANT_TOKEN:?set from the local unprivileged-tenant login}"
ROUTE="$API_URL/v2/org/$ORG/nico/machine/$HOST_ID/dpu"VerificationStep 1: Reproduce the pre-fix decode failureWhy this step exists: It establishes the original failure with both REST images built from the pre-fix release revision and a policy-bearing Core response. Runnable command: curl -sS -o pre-fix.json -w 'status=%{http_code}\n' "$ROUTE" \
-H "Authorization: Bearer $ADMIN_TOKEN"
jq -c '{message}' pre-fix.jsonObserved result: Why this proves the behavior: The real endpoint reproduced the Temporal JSON decode failure before the backport. Step 2: Retrieve the policy-bearing DPU at the exact headWhy this step exists: It exercises the changed REST-to-Temporal-to-site-agent-to-Core path with both nested protobuf variants present. Runnable command: curl -sS -o exact.json -w 'status=%{http_code}\n' "$ROUTE" \
-H "Authorization: Bearer $ADMIN_TOKEN"
jq -c --arg dpu "$DPU_ID" --arg host "$HOST_ID" '{
type: type,
count: length,
first_id_matches: (.[0].id == $dpu),
host_id_matches: (.[0].hostMachineId == $host),
network_config_present: (.[0].dpuNetworkConfig != null)
}' exact.json
kubectl -n nico-system exec deployment/nico-api -- \
/opt/carbide/nico-admin-cli --format json dpu network config \
--machine-id "$DPU_ID" |
jq -c '{
oneof_object_count: ([.. | objects | select(has("source_net") or has("destination_net"))] | length),
oneof_keys: ([.. | objects | keys[] | select(. == "source_net" or . == "destination_net")] | unique)
}'Observed result: Why this proves the behavior: The public endpoint returned the attached DPU instead of HTTP 500 while Core supplied the source and destination Step 3: Exercise repeat, authentication, authorization, and selector behaviorWhy this step exists: It verifies that the changed decode path preserves the route's existing repeat and error behavior. Runnable command: curl -sS -o repeat.json -w 'repeat=%{http_code}\n' "$ROUTE" \
-H "Authorization: Bearer $ADMIN_TOKEN"
cmp -s exact.json repeat.json && echo 'repeat_body_identical=true'
curl -sS -o /dev/null -w 'no_auth=%{http_code}\n' "$ROUTE"
curl -sS -o /dev/null -w 'tenant=%{http_code}\n' "$ROUTE" \
-H "Authorization: Bearer $TENANT_TOKEN"
curl -sS -o /dev/null -w 'dpu_as_host=%{http_code}\n' \
"$API_URL/v2/org/$ORG/nico/machine/$DPU_ID/dpu" \
-H "Authorization: Bearer $ADMIN_TOKEN"Observed result: Why this proves the behavior: Repeating the successful request was stable, and the unchanged access and host-selector cases returned their established statuses. Step 4: Exercise the legacy workflow-result fallbackWhy this step exists: It verifies the rollout path called out in the original review: an exact-head API decoding a simple top-level array from a pre-fix site-agent worker. Runnable command: EXACT_API_IMAGE="${EXACT_API_IMAGE:?set to the exact-head API image}"
LEGACY_SITE_AGENT_IMAGE="${LEGACY_SITE_AGENT_IMAGE:?set to the pre-fix site-agent image}"
SITE_AGENT_CONTAINER="$(kubectl -n nico-rest get statefulset nico-rest-site-agent \
-o jsonpath='{.spec.template.spec.containers[0].name}')"
docker image inspect "$EXACT_API_IMAGE" "$LEGACY_SITE_AGENT_IMAGE" \
--format '{{index .Config.Labels "verification.revision"}}'
kubectl -n nico-rest set image statefulset/nico-rest-site-agent \
"$SITE_AGENT_CONTAINER=$LEGACY_SITE_AGENT_IMAGE"
kubectl -n nico-rest rollout status statefulset/nico-rest-site-agent --timeout=180s
curl -sS -o legacy.json -w 'legacy_provider=%{http_code}\n' "$ROUTE" \
-H "Authorization: Bearer $ADMIN_TOKEN"
jq -c '{type:type,count:length,network_config_present:(.[0].dpuNetworkConfig != null)}' legacy.jsonObserved result: Why this proves the behavior: The exact API successfully applied its fallback to the legacy worker's top-level array through the real endpoint. Step 5: Run focused regression and contract checksWhy this step exists: It covers the wrapper, policy variants, legacy payload decode, public model, OpenAPI, and final diff. Runnable command: go test ./site-workflow/pkg/workflow -run '^TestGetDpuMachinesTestSuite$' -count=1
go test ./site-workflow/pkg/activity -run '^TestManageMachine_GetDpuMachinesByIDs$' -count=1
go test ./api/pkg/api/handler \
-run '^(TestGetDpuMachinesLegacyTemporalPayload|TestMachineHandler_GetDpuMachines)$' -count=1
go test ./api/pkg/api/model -count=1
npx @redocly/cli lint --skip-rule=no-ambiguous-paths ./openapi/spec.yaml
git diff --check a14089b7fce2c79858e398ea9de17b7537b026fb...HEADObserved result: Why this proves the behavior: The focused regression paths and generated public contract are consistent with the runtime results, and the final diff has no whitespace errors. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4070.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-24 18:28:33 UTC | Commit: 4e9867e |
![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: 4e9867e63f
ℹ️ 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".
|
|
||
| // GetDpuMachinesByIDs is an activity to retrieve DPU Machines by IDs with network configuration | ||
| func (mm *ManageMachine) GetDpuMachinesByIDs(ctx context.Context, dpuMachineIDs []string) ([]*cwssaws.DpuMachine, error) { | ||
| func (mm *ManageMachine) GetDpuMachinesByIDs(ctx context.Context, dpuMachineIDs []string) (*cwssaws.DpuMachineList, error) { |
There was a problem hiding this comment.
When site-agent is rolled with old and new pods on the same Temporal task queue, this keeps registering the same GetDpuMachinesByIDs activity name but changes its result from []*DpuMachine to DpuMachineList. Temporal can route an activity task scheduled by one workflow version to a worker running the other version, so the waiting workflow decodes the activity result into the wrong shape and fails before the API handler's legacy workflow-result fallback runs. That makes /machine/{id}/dpu return 500 during mixed-version rollouts; please version the activity name or keep/decode both result shapes inside the workflow until old workers are drained.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
So this is for a graceful zero down upgrade? I don't think we need to worry about that right now.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
Backports #3866 to prevent attached DPU detail requests from returning HTTP 500 when network security policy overrides are present. The release implementation preserves those protobuf fields through Temporal while retaining compatibility with legacy workflow payloads.
Related issues
Backport of #3866.
Type of Change
Breaking Changes
Testing
Passed focused handler decode-fallback and Temporal workflow tests, plus OpenAPI lint.
Additional Notes
Conflict resolution uses
release/v2.0's workflow-schema proto package instead ofmain's newer Core proto package.