feat(rest-api): add generic Flow gRPC proxy - #4560
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThis change replaces separate Core proxy components with shared Core and Flow gRPC proxy contracts, dynamic unary transport, Temporal workflows and activities, encrypted secret handling, Flow registration, timeout validation, updated tests, and migration guidance. ChangesGeneric gRPC proxy
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RESTHandler
participant ExecuteFlowGRPC
participant InvokeFlowGRPC
participant InvokeFlowGRPCOnSite
participant FlowGrpcClient
RESTHandler->>ExecuteFlowGRPC: Submit method, request, workflow ID, and conflict policy
ExecuteFlowGRPC->>InvokeFlowGRPC: Start proxy workflow
InvokeFlowGRPC->>InvokeFlowGRPCOnSite: Execute proxy activity
InvokeFlowGRPCOnSite->>FlowGrpcClient: Merge secrets and invoke unary method
FlowGrpcClient-->>InvokeFlowGRPCOnSite: Return response JSON
InvokeFlowGRPCOnSite-->>InvokeFlowGRPC: Return grpcproxy.Response
InvokeFlowGRPC-->>ExecuteFlowGRPC: Return workflow result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 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-05 00:12:56 UTC | Commit: 2973f05 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
rest-api/common/pkg/flowproxy/flowproxy_test.go (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse named table-driven
t.Runsubtests.Each test executes one direct scenario. The repository test rule requires named table-driven subtests.
rest-api/common/pkg/flowproxy/flowproxy_test.go#L12-L15: put each timeout relation in a named table case.rest-api/common/pkg/coreproxy/coreproxy_test.go#L12-L14: put each timeout relation in a named table case.rest-api/site-workflow/pkg/workflow/flowproxy_test.go#L18-L42: put the workflow deadline scenario in a named table case.rest-api/api/pkg/api/handler/util/common/flowproxy_test.go#L22-L65: put the workflow-option and timeout-error scenario in a named table case.As per coding guidelines, “Use testify assertions and organize tests around the production function or method, with one top-level
Test...function and named table-drivent.Runsubtests.”🤖 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/common/pkg/flowproxy/flowproxy_test.go` around lines 12 - 15, Convert each listed test scenario into a named table-driven t.Run subtest while keeping one top-level Test... function and existing testify assertions: rest-api/common/pkg/flowproxy/flowproxy_test.go lines 12-15 should table-drive each timeout relation; rest-api/common/pkg/coreproxy/coreproxy_test.go lines 12-14 should do the same; rest-api/site-workflow/pkg/workflow/flowproxy_test.go lines 18-42 should wrap the workflow deadline scenario in a named case; and rest-api/api/pkg/api/handler/util/common/flowproxy_test.go lines 22-65 should wrap the workflow-option and timeout-error scenario in a named case.Source: Coding guidelines
rest-api/common/pkg/flowproxy/flowproxy.go (1)
52-61: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSensitive Data Exposure (CWE-312): Cleartext Storage of Sensitive Information
Reachability: Internal · Exploitability: Difficult
Reachability path
● Entry rest-api/site-workflow/pkg/workflow/flowproxy_test.go:18 TestInvokeFlowGRPCActivityDeadlinePrecedesWorkflowTimeout │ ▼ ● Hop rest-api/site-workflow/pkg/workflow/flowproxy.go:21 InvokeFlowGRPC: No automatic retries: a proxied call may be a non-idempotent mutation, so │ ▼ ● Hop rest-api/site-workflow/pkg/activity/flowproxy.go:39 InvokeFlowGRPCOnSite │ ▼ ● Sink rest-api/common/pkg/flowproxy/flowproxy.goFail closed in
ExecuteFlowGRPCwhen secret fields lack an encryption key.Current production callers pass no secret fields, so this is not an externally reachable leak today. However, a future secret-bearing caller could forward unredacted JSON to
flowproxy.Request.RequestJSONwhensecretKeyis empty. Return an error beforeExecuteWorkflowin this condition.🤖 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/common/pkg/flowproxy/flowproxy.go` around lines 52 - 61, Update ExecuteFlowGRPC to return an error before ExecuteWorkflow when secret fields are present but secretKey is empty, preventing unredacted JSON from being forwarded in RequestJSON. Preserve the existing workflow execution path when no secret fields are supplied or when an encryption key is available.rest-api/api/pkg/api/handler/taskrun_test.go (2)
75-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a checked type assertion in the mock
Runcallback.
args.Get(1).(*flowproxy.Response)panics if the production code ever passes a different pointer type towe.Get. A panic inside a testify callback reports a stack trace rather than the actual contract break. A checked assertion turns that into a readable failure.♻️ Proposed change
mockRun.Mock.On("Get", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { - out := args.Get(1).(*flowproxy.Response) + out, ok := args.Get(1).(*flowproxy.Response) + require.True(t, ok, "expected *flowproxy.Response, got %T", args.Get(1)) out.ResponseJSON = respJSON }).Return(nil)🤖 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/api/pkg/api/handler/taskrun_test.go` around lines 75 - 79, Update the mockRun Get callback to use a checked type assertion for args.Get(1) before assigning ResponseJSON, and report an assertion failure through the test context instead of panicking when the argument is not a *flowproxy.Response.
638-643: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the gateway-timeout branch.
executeRunFlowGRPCinrest-api/api/pkg/api/handler/taskrun.goLines 125-127 has a dedicated branch that callscommon.TerminateWorkflowOnTimeOutwhen the proxy returns 504. The migration preserved that behavior deliberately, but no case here exercises it. The current cases cover only success, validation failures, authorization failures, and a scheduling error.Extend
mockFlowProxyWorkflowto return a*temporal.TimeoutErrorfromGet, then assert the 504 status and thatTerminateWorkflowis called on the mock client with the expected workflow ID.🤖 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/api/pkg/api/handler/taskrun_test.go` around lines 638 - 643, Extend the task-run test cases around the existing success and failure entries to cover the gateway-timeout path: configure mockFlowProxyWorkflow.Get to return a *temporal.TimeoutError, expect http.StatusGatewayTimeout, and verify the mock client’s TerminateWorkflow call uses the expected workflow ID. Update mockFlowProxyWorkflow only as needed to support this response and assertion.rest-api/common/pkg/secretjson/secretjson_test.go (1)
14-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the
Redacttests into one table-driven top-level function.The file currently declares four top-level tests for
Redact. The repository test convention requires one top-levelTest...function per production function, with named table-drivent.Runsubtests. Group the round-trip, multi-field, no-match, and non-object cases as subtests of a singleTestRedact, and keepTestMergeseparate for the merge cases.As per path instructions: "Use
testifyassertions and organize tests around the production function or method, with one top-levelTest...function and named table-drivent.Runsubtests."🤖 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/common/pkg/secretjson/secretjson_test.go` around lines 14 - 78, Consolidate the four Redact-focused top-level tests into one table-driven TestRedact function with named t.Run subtests covering round-trip, multiple fields, no matches, and non-object payloads. Preserve each case’s existing testify assertions and behavior, while keeping the merge scenario in its separate TestMerge top-level function.Source: Path instructions
rest-api/site-workflow/pkg/grpc/client/flow_proxy.go (1)
74-79: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReconsider
DiscardUnknownon the request decode.The site decodes the cloud's request with
DiscardUnknown: true. If the cloud is built against a newerv1.Flowthan the site, an unknown request field is dropped and the call still succeeds with weaker semantics. A new filter or a new safety flag would be silently ignored rather than reported. Strict decoding on the request path converts version skew into an explicit error, while tolerant decoding stays correct for the response path.If the tolerant behavior is a deliberate rolling-upgrade requirement, state that in the doc comment so a future reader does not tighten it by accident.
♻️ Proposed change to fail fast on unknown request fields
in := dynamicpb.NewMessage(md.Input()) if len(reqJSON) > 0 { - if err := (protojson.UnmarshalOptions{DiscardUnknown: true}).Unmarshal(reqJSON, in); err != nil { + // Strict: an unknown field means the cloud and site Flow schemas have + // diverged, which must surface as an error rather than a weaker call. + if err := protojson.Unmarshal(reqJSON, in); err != nil { return nil, fmt.Errorf("decode request for %q: %w", md.Name(), err) } }🤖 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/site-workflow/pkg/grpc/client/flow_proxy.go` around lines 74 - 79, Update the request decode in the flow proxy around dynamicpb.NewMessage and protojson.UnmarshalOptions to use strict unknown-field handling by removing DiscardUnknown, so newer cloud request fields fail explicitly instead of being silently ignored; leave tolerant decoding for responses unchanged. If rolling upgrades intentionally require tolerant requests, document that requirement in the relevant doc comment instead.rest-api/site-workflow/pkg/grpc/client/flow_proxy_test.go (1)
16-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert these flat assertion sequences into named
t.Runsubtests.
TestResolveFlowMethodandTestFlowMethodNameassert several distinct cases in one flat body. A failure reports only the function name, so the failing case is not identifiable from the test output.TestInvokeFlowJSONConnalready uses the required shape; apply it here as well. Name each case for the exact behavior it exercises, for example "bare name resolves", "fully qualified name resolves", and "unknown method returns sentinel".As per path instructions: "Use
testifyassertions and organize tests around the production function or method, with one top-levelTest...function and named table-drivent.Runsubtests."Also applies to: 48-53
🤖 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/site-workflow/pkg/grpc/client/flow_proxy_test.go` around lines 16 - 28, Restructure TestResolveFlowMethod and TestFlowMethodName into named t.Run subtests, with separate cases for bare-name resolution, fully qualified-name resolution, and unknown-method sentinel errors. Keep each case’s existing testify assertions and organize the subtests around the behavior of the production function, matching the structure used by TestInvokeFlowJSONConn.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.
Inline comments:
In `@rest-api/api/pkg/api/handler/taskrun.go`:
- Around line 528-532: The lifecycle workflow ID in taskrun.go around
executeRunFlowGRPC must include a caller-supplied discriminator and
ExpectedPhaseIndex for advance requests, following GetTaskRunHandler’s
documented ID pattern, so concurrent requests do not coalesce under
USE_EXISTING. Update taskrun_test.go at lines 673-674 to expect the new ID shape
and add an advance case with a non-zero ExpectedPhaseIndex asserting a distinct
workflow ID.
In `@rest-api/api/pkg/api/handler/util/common/flowproxy.go`:
- Around line 58-68: Update the secret-handling flow before ExecuteWorkflow to
reject requests when secretFields is non-empty but secretKey is empty, returning
an appropriate API error instead of forwarding reqJSON unchanged. Keep the
existing Redact and EncryptData behavior for configurations where both
secretFields and secretKey are present.
In `@rest-api/site-workflow/pkg/activity/flowproxy.go`:
- Around line 20-23: Update the flowproxy secret-key initialization and usage so
EncryptedSecrets uses an independently generated per-site key loaded from secret
storage rather than ManagerAccess.Conf.EB.Temporal.ClusterID, while preserving
compatibility with EncryptData’s SHA-256 key derivation. Harden DecryptData’s
gcm.Open path to validate ciphertext length and return an error for invalid or
truncated input instead of panicking.
In `@rest-api/site-workflow/pkg/workflow/flowproxy.go`:
- Around line 25-31: Update the proxy contract and invocation flow around the
activity options so retry behavior is selected per Flow method: retain a single
attempt for non-idempotent mutations, while applying the intended bounded retry
policy to idempotent get and list operations. Add regression coverage verifying
both mutation and read/list invocations receive the correct retry configuration.
In `@rest-api/skills/rest-flow-grpc-proxy/SKILL.md`:
- Around line 33-44: Expand the ExecuteFlowGRPC contract documentation to
specify required and optional workflow options, including the default
WORKFLOW_ID_REUSE_POLICY_ALLOW_DUPLICATE, accepted conflict-policy enum values,
and links to the authoritative enum or implementation. Document behavior when
workflow IDs conflict or an existing workflow run is found, including resulting
outputs and errors, while preserving the existing handler-specific ID derivation
guidance.
- Around line 56-67: Document that secretFields requires a non-empty
siteIDSecretKey, and update the ExecuteFlowGRPC call path to reject the
combination of secretFields being provided with an empty secret key before
sending the original RequestJSON or persisting workflow history.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/taskrun_test.go`:
- Around line 75-79: Update the mockRun Get callback to use a checked type
assertion for args.Get(1) before assigning ResponseJSON, and report an assertion
failure through the test context instead of panicking when the argument is not a
*flowproxy.Response.
- Around line 638-643: Extend the task-run test cases around the existing
success and failure entries to cover the gateway-timeout path: configure
mockFlowProxyWorkflow.Get to return a *temporal.TimeoutError, expect
http.StatusGatewayTimeout, and verify the mock client’s TerminateWorkflow call
uses the expected workflow ID. Update mockFlowProxyWorkflow only as needed to
support this response and assertion.
In `@rest-api/common/pkg/flowproxy/flowproxy_test.go`:
- Around line 12-15: Convert each listed test scenario into a named table-driven
t.Run subtest while keeping one top-level Test... function and existing testify
assertions: rest-api/common/pkg/flowproxy/flowproxy_test.go lines 12-15 should
table-drive each timeout relation;
rest-api/common/pkg/coreproxy/coreproxy_test.go lines 12-14 should do the same;
rest-api/site-workflow/pkg/workflow/flowproxy_test.go lines 18-42 should wrap
the workflow deadline scenario in a named case; and
rest-api/api/pkg/api/handler/util/common/flowproxy_test.go lines 22-65 should
wrap the workflow-option and timeout-error scenario in a named case.
In `@rest-api/common/pkg/flowproxy/flowproxy.go`:
- Around line 52-61: Update ExecuteFlowGRPC to return an error before
ExecuteWorkflow when secret fields are present but secretKey is empty,
preventing unredacted JSON from being forwarded in RequestJSON. Preserve the
existing workflow execution path when no secret fields are supplied or when an
encryption key is available.
In `@rest-api/common/pkg/secretjson/secretjson_test.go`:
- Around line 14-78: Consolidate the four Redact-focused top-level tests into
one table-driven TestRedact function with named t.Run subtests covering
round-trip, multiple fields, no matches, and non-object payloads. Preserve each
case’s existing testify assertions and behavior, while keeping the merge
scenario in its separate TestMerge top-level function.
In `@rest-api/site-workflow/pkg/grpc/client/flow_proxy_test.go`:
- Around line 16-28: Restructure TestResolveFlowMethod and TestFlowMethodName
into named t.Run subtests, with separate cases for bare-name resolution, fully
qualified-name resolution, and unknown-method sentinel errors. Keep each case’s
existing testify assertions and organize the subtests around the behavior of the
production function, matching the structure used by TestInvokeFlowJSONConn.
In `@rest-api/site-workflow/pkg/grpc/client/flow_proxy.go`:
- Around line 74-79: Update the request decode in the flow proxy around
dynamicpb.NewMessage and protojson.UnmarshalOptions to use strict unknown-field
handling by removing DiscardUnknown, so newer cloud request fields fail
explicitly instead of being silently ignored; leave tolerant decoding for
responses unchanged. If rolling upgrades intentionally require tolerant
requests, document that requirement in the relevant doc comment instead.
🪄 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: fc11f3c8-ea6c-42f1-9c39-a9444033c666
📒 Files selected for processing (27)
rest-api/AGENTS.mdrest-api/api/pkg/api/handler/credentialrotation_test.gorest-api/api/pkg/api/handler/taskrun.gorest-api/api/pkg/api/handler/taskrun_test.gorest-api/api/pkg/api/handler/ueficredential_test.gorest-api/api/pkg/api/handler/util/common/coreproxy.gorest-api/api/pkg/api/handler/util/common/flowproxy.gorest-api/api/pkg/api/handler/util/common/flowproxy_test.gorest-api/common/pkg/coreproxy/coreproxy.gorest-api/common/pkg/coreproxy/coreproxy_test.gorest-api/common/pkg/flowproxy/flowproxy.gorest-api/common/pkg/flowproxy/flowproxy_test.gorest-api/common/pkg/secretjson/secretjson.gorest-api/common/pkg/secretjson/secretjson_test.gorest-api/site-agent/pkg/components/managers/flowgrpc/subscriber.gorest-api/site-workflow/pkg/activity/coreproxy.gorest-api/site-workflow/pkg/activity/flowproxy.gorest-api/site-workflow/pkg/activity/taskrun.gorest-api/site-workflow/pkg/activity/taskrun_test.gorest-api/site-workflow/pkg/grpc/client/flow_proxy.gorest-api/site-workflow/pkg/grpc/client/flow_proxy_test.gorest-api/site-workflow/pkg/workflow/flowproxy.gorest-api/site-workflow/pkg/workflow/flowproxy_test.gorest-api/site-workflow/pkg/workflow/taskrun.gorest-api/site-workflow/pkg/workflow/taskrun_test.gorest-api/skills/rest-core-grpc-proxy/SKILL.mdrest-api/skills/rest-flow-grpc-proxy/SKILL.md
💤 Files with no reviewable changes (4)
- rest-api/site-workflow/pkg/activity/taskrun_test.go
- rest-api/site-workflow/pkg/workflow/taskrun.go
- rest-api/site-workflow/pkg/workflow/taskrun_test.go
- rest-api/site-workflow/pkg/activity/taskrun.go
| var encryptedSecrets []byte | ||
| if secretKey != "" && len(secretFields) > 0 { | ||
| redacted, secretsJSON, rerr := secretjson.Redact(reqJSON, secretFields) | ||
| if rerr != nil { | ||
| return cutil.NewAPIError(http.StatusInternalServerError, "Failed to redact Flow proxy request", rerr) | ||
| } | ||
| reqJSON = redacted | ||
| if len(secretsJSON) > 0 { | ||
| encryptedSecrets = cutil.EncryptData(secretsJSON, secretKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no existing caller passes secret fields with an empty secret key.
set -euo pipefail
rg -n --type=go -C6 'ExecuteFlowGRPC\(|ExecuteCoreGRPC\('Repository: NVIDIA/infra-controller
Length of output: 161
Sensitive Data Exposure (CWE-922)
Reachability: Internal · Exploitability: Theoretical
Reachability path
● Entry
rest-api/api/pkg/api/handler/credentialrotation_test.go:37
TestRotateCredentialHandlerProxiesRequest
│
▼
● Hop
rest-api/api/pkg/api/handler/taskrun.go:124
ExecuteFlowGRPC
│
▼
● Sink
rest-api/api/pkg/api/handler/util/common/flowproxy.go
Reject mismatched secret configuration.
If secretFields is non-empty and secretKey is empty, return an error before ExecuteWorkflow. The current guard otherwise forwards reqJSON unchanged, which stores the secret in Temporal history.
🤖 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/api/pkg/api/handler/util/common/flowproxy.go` around lines 58 - 68,
Update the secret-handling flow before ExecuteWorkflow to reject requests when
secretFields is non-empty but secretKey is empty, returning an appropriate API
error instead of forwarding reqJSON unchanged. Keep the existing Redact and
EncryptData behavior for configurations where both secretFields and secretKey
are present.
| // secretKey decrypts the redacted secret fields carried in | ||
| // flowproxy.Request.EncryptedSecrets. It is the shared site key (the | ||
| // site/cluster ID), matching the key the cloud used to encrypt them. | ||
| secretKey string |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C6 --type go \
'func\s+(EncryptData|DecryptData)\b|\b(EncryptData|DecryptData)\s*\(' \
rest-api
rg -n -C6 --type go '\b(NewManageFlowProxy|ExecuteFlowGRPC)\s*\(' rest-api
rg -n -C6 --type go 'secretKey|site.?ID|cluster.?ID' \
rest-api/api rest-api/site-agent rest-api/site-workflowRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- crypto definitions and call sites ---'
rg -n -C8 --type go \
'func (EncryptData|DecryptData)\b|\b(EncryptData|DecryptData)\s*\(' \
rest-api/common rest-api/cloud* rest-api/site-workflow rest-api/site-agent 2>/dev/null \
| grep -E '(^[^:]+:[0-9]+:|EncryptData|DecryptData|secretKey)' \
| head -n 400
printf '%s\n' '--- flow proxy construction and secret-key arguments ---'
rg -n -C10 --type go \
'NewManageFlowProxy|secretKey:|SecretKey|site.?key|cluster.?key|SiteID|ClusterID' \
rest-api/site-agent rest-api/site-workflow rest-api/common \
| head -n 500Repository: NVIDIA/infra-controller
Length of output: 1731
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- encryption implementation ---'
cat -n rest-api/common/pkg/util/encryption.go
printf '%s\n' '--- encryption tests ---'
cat -n rest-api/common/pkg/util/encryption_test.go
printf '%s\n' '--- flow proxy constructors and key sources ---'
rg -n -C12 --type go \
'NewManageFlowProxy|InvokeFlowGRPCOnSite|Temporal\.ClusterID|ClusterID|secretKey' \
rest-api/site-agent rest-api/site-workflow \
| head -n 800Repository: NVIDIA/infra-controller
Length of output: 50379
Weak Cryptography (CWE-320)
Reachability: External · Exploitability: Moderate
Reachability path
● Entry
rest-api/site-workflow/pkg/workflow/flowproxy_test.go:18
TestInvokeFlowGRPCActivityDeadlinePrecedesWorkflowTimeout
│
▼
● Hop
rest-api/site-workflow/pkg/workflow/flowproxy.go:21
InvokeFlowGRPC: No automatic retries: a proxied call may be a non-idempotent mutation, so
│
▼
● Sink
rest-api/site-workflow/pkg/activity/flowproxy.go
Use a secret-managed key for EncryptedSecrets. secretKey is ManagerAccess.Conf.EB.Temporal.ClusterID, and EncryptData derives the AES key by hashing that identifier with SHA-256. Replace it with an independently generated per-site key from secret storage. DecryptData authenticates modified ciphertext through gcm.Open, but it should return an error for invalid or truncated ciphertext instead of panicking.
🤖 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/site-workflow/pkg/activity/flowproxy.go` around lines 20 - 23,
Update the flowproxy secret-key initialization and usage so EncryptedSecrets
uses an independently generated per-site key loaded from secret storage rather
than ManagerAccess.Conf.EB.Temporal.ClusterID, while preserving compatibility
with EncryptData’s SHA-256 key derivation. Harden DecryptData’s gcm.Open path to
validate ciphertext length and return an error for invalid or truncated input
instead of panicking.
| // No automatic retries: a proxied call may be a non-idempotent mutation, so | ||
| // the activity runs exactly once and the caller decides whether to retry. | ||
| options := workflow.ActivityOptions{ | ||
| StartToCloseTimeout: flowproxy.ActivityStartToCloseTimeout, | ||
| RetryPolicy: &temporal.RetryPolicy{ | ||
| MaximumAttempts: 1, | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve retries for idempotent Flow reads.
MaximumAttempts: 1 applies to every Flow method, including get and list methods. The PR objective limits this behavior to mutations. A transient activity or Flow transport failure now fails idempotent reads without retry.
Add per-invocation retry configuration to the proxy contract. Set one attempt only for mutations. Configure the intended bounded retry policy for read and list operations. Add regression tests for both paths.
🤖 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/site-workflow/pkg/workflow/flowproxy.go` around lines 25 - 31,
Update the proxy contract and invocation flow around the activity options so
retry behavior is selected per Flow method: retain a single attempt for
non-idempotent mutations, while applying the intended bounded retry policy to
idempotent get and list operations. Add regression coverage verifying both
mutation and read/list invocations receive the correct retry configuration.
Source: Path instructions
| `ExecuteFlowGRPC` requires the caller to supply: | ||
|
|
||
| 1. `workflowID` — deterministic for read/list dedup, fresh UUID for creates. | ||
| 2. `conflictPolicy` — typically `WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING` | ||
| for Flow handlers that coalesce identical in-flight requests. | ||
|
|
||
| Examples of ID derivation that must stay in the handler: | ||
|
|
||
| - `GetTaskRun`: include `includeStats` in the ID | ||
| (`task-run-get-{runID}-{true|false}`). | ||
| - List endpoints: hash query parameters (`QueryParamHash`) into the ID. | ||
| - `CreateTaskRun`: fresh UUID every request (`task-run-create-{uuid}`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document the complete workflow option contract.
Lines 33-44 do not define requiredness, accepted conflict-policy values, the WORKFLOW_ID_REUSE_POLICY_ALLOW_DUPLICATE default, or behavior for conflicts and existing workflow runs. Add these rules and link the authoritative enum or implementation.
As per coding guidelines, Markdown interface contracts must state requiredness, defaults, accepted values, interactions, outputs, and errors.
🤖 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/skills/rest-flow-grpc-proxy/SKILL.md` around lines 33 - 44, Expand
the ExecuteFlowGRPC contract documentation to specify required and optional
workflow options, including the default
WORKFLOW_ID_REUSE_POLICY_ALLOW_DUPLICATE, accepted conflict-policy enum values,
and links to the authoritative enum or implementation. Document behavior when
workflow IDs conflict or an existing workflow run is found, including resulting
outputs and errors, while preserving the existing handler-specific ID derivation
guidance.
Source: Coding guidelines
| - Secret fields that must not appear in Temporal history (top-level protojson | ||
| field names). | ||
| - Whether timeout should terminate the workflow (`TerminateWorkflowOnTimeOut`) | ||
| — TaskRun does this when `ExecuteFlowGRPC` returns `504`. | ||
|
|
||
| ## Implementation Workflow | ||
|
|
||
| 1. Keep auth, tenant/org membership, site lookup, Flow enablement, role checks, | ||
| request validation, and REST semantics in the REST handler. | ||
| 2. Build the typed Flow protobuf request before calling the proxy. | ||
| 3. Call `common.ExecuteFlowGRPC(ctx, siteTemporalClient, fullMethod, reqProto, | ||
| respProtoOrNil, workflowID, conflictPolicy, siteIDSecretKey, secretFields...)`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C 8 'func ExecuteFlowGRPC|secretjson\.Redact|secretFields|siteIDSecretKey' rest-api/api/pkg/api/handler rest-api --glob '*.go' --glob '*.md'Repository: NVIDIA/infra-controller
Length of output: 25223
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path("rest-api/api/pkg/api/handler/util/common/flowproxy.go")
lines = p.read_text().splitlines()
for start, end in [(39, 95)]:
for n in range(start, min(end, len(lines)) + 1):
print(f"{n}: {lines[n-1]}")
print("\nCall sites:")
for path in Path("rest-api").rglob("*.go"):
for n, line in enumerate(path.read_text(errors="ignore").splitlines(), 1):
if "ExecuteFlowGRPC(" in line and "func ExecuteFlowGRPC" not in line:
print(f"{path}:{n}:{line.strip()}")
PYRepository: NVIDIA/infra-controller
Length of output: 2943
🏁 Script executed:
sed -n '100,135p' rest-api/api/pkg/api/handler/taskrun.go
printf '\nAll Flow proxy invocations with argument context:\n'
rg -n -C 4 'ExecuteFlowGRPC\(' rest-api/api/pkg/api/handler --glob '*.go'Repository: NVIDIA/infra-controller
Length of output: 4124
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal · Exploitability: Theoretical
Require a non-empty secret key for secret fields.
ExecuteFlowGRPC sends the original RequestJSON when secretFields is non-empty and secretKey is empty. Document this precondition and return an error for this invalid combination instead of allowing secrets into Temporal history.
🤖 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/skills/rest-flow-grpc-proxy/SKILL.md` around lines 56 - 67, Document
that secretFields requires a non-empty siteIDSecretKey, and update the
ExecuteFlowGRPC call path to reject the combination of secretFields being
provided with an empty secret key before sending the original RequestJSON or
persisting workflow history.
Source: Coding guidelines
There was a problem hiding this comment.
Seems to be a fair ask.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline rest-api/api/pkg/api/handler/util/common/grpcproxy.go --items all
ast-grep outline rest-api/api/pkg/api/handler/util/common/grpcproxy_test.go --items all
sed -n '1,240p' rest-api/api/pkg/api/handler/util/common/grpcproxy.go
sed -n '1,280p' rest-api/api/pkg/api/handler/util/common/grpcproxy_test.goLength of output: 12184
@thossain-nv, the finding is confirmed.
ExecuteFlowGRPC delegates to executeGRPCProxy. The same helper also serves ExecuteCoreGRPC. With non-empty secretFields and an empty secretKey, the helper skips secretjson.Redact and sends the original RequestJSON to Temporal.
The implementation should add one shared preflight check in executeGRPCProxy:
- If
len(secretFields) > 0andsecretKey == "", return an internalAPIError. - Perform the check before request marshaling and before
ExecuteWorkflow. - Add a test that asserts
ExecuteWorkflowwas not called. - Document the
siteIDSecretKeyrequirement inrest-api/skills/rest-flow-grpc-proxy/SKILL.md.
This shared validation protects both the Flow and Core proxy paths.
🐇
You are interacting with an AI system.
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rest-api/api/internal/server/server_test.go (1)
34-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd named subtests for the timeout relationships.
Place each assertion in a named table-driven
t.Runsubtest. This identifies the failed relationship directly and follows the repository test structure.As per coding guidelines,
rest-api/**/*_test.gotests use one top-levelTest...function with named table-drivent.Runsubtests.🤖 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/api/internal/server/server_test.go` around lines 34 - 43, Update Test_ProxyTimeoutsFitWriteTimeout to use named table-driven t.Run subtests, placing each timeout relationship assertion in its own case with a descriptive name. Preserve the existing three Less assertions and keep a single top-level test function.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.
Inline comments:
In `@rest-api/skills/rest-flow-grpc-proxy/SKILL.md`:
- Around line 47-51: Update the migration guidance in the deterministic ID
section of SKILL.md to explicitly state that the flow-grpc- prefix is an
intentional contract change aligned with TaskRun IDs, and document the required
rollout compatibility when older workflow types may still be running. Preserve
the existing explanation of USE_EXISTING behavior and undecodable payloads.
---
Nitpick comments:
In `@rest-api/api/internal/server/server_test.go`:
- Around line 34-43: Update Test_ProxyTimeoutsFitWriteTimeout to use named
table-driven t.Run subtests, placing each timeout relationship assertion in its
own case with a descriptive name. Preserve the existing three Less assertions
and keep a single top-level test function.
🪄 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: 107b1c01-34fa-4b08-9d6e-308136f04da1
📒 Files selected for processing (10)
rest-api/api/internal/server/server.gorest-api/api/internal/server/server_test.gorest-api/api/pkg/api/handler/taskrun.gorest-api/api/pkg/api/handler/taskrun_test.gorest-api/api/pkg/api/handler/util/common/flowproxy.gorest-api/api/pkg/api/handler/util/common/flowproxy_test.gorest-api/common/pkg/flowproxy/flowproxy.gorest-api/common/pkg/flowproxy/flowproxy_test.gorest-api/site-workflow/pkg/workflow/flowproxy_test.gorest-api/skills/rest-flow-grpc-proxy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (6)
- rest-api/common/pkg/flowproxy/flowproxy_test.go
- rest-api/api/pkg/api/handler/util/common/flowproxy.go
- rest-api/api/pkg/api/handler/util/common/flowproxy_test.go
- rest-api/site-workflow/pkg/workflow/flowproxy_test.go
- rest-api/api/pkg/api/handler/taskrun.go
- rest-api/api/pkg/api/handler/taskrun_test.go
The generic Flow gRPC proxy needs the same redact-and-merge split the Core proxy already implements, so move it out of coreproxy rather than duplicate it. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Adds the InvokeFlowGRPC workflow, its on-site activity, the Flow JSON invoker, and the ExecuteFlowGRPC cloud helper, so a Flow-backed endpoint no longer needs a bespoke Temporal workflow per method. Unlike the Core proxy, the caller owns the workflow ID and conflict policy, keeping read dedup and create freshness under handler control. The site agent registers the proxy next to the per-method workflows it will replace, and no handler dispatches through it yet: a workflow type is known only to workers that registered it, and the cloud API and each site's agent ship as separate releases, so handlers can only switch over once every site runs an agent that serves the proxy. The timeout ladder is bounded by the API server's write deadline rather than by the on-site budget, since a response written past that deadline never reaches the client. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
976ac52 to
d4b8240
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. |
kunzhao-nv
changed the title
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/site-agent/pkg/components/managers/flowgrpc/subscriber.go`:
- Around line 229-232: Update the NewManageFlowProxy call in the subscriber flow
to pass the configured siteIDSecretKey instead of
ManagerAccess.Conf.EB.Temporal.ClusterID, matching the key used to encrypt
payload secrets. Add coverage for InvokeFlowGRPCOnSite with encrypted secrets to
verify decryption and request forwarding succeed.
🪄 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: 0ee44114-54fc-4cda-b300-8e0908f91c61
📒 Files selected for processing (23)
rest-api/AGENTS.mdrest-api/api/internal/server/server.gorest-api/api/internal/server/server_test.gorest-api/api/pkg/api/handler/credentialrotation_test.gorest-api/api/pkg/api/handler/ueficredential_test.gorest-api/api/pkg/api/handler/util/common/coreproxy.gorest-api/api/pkg/api/handler/util/common/flowproxy.gorest-api/api/pkg/api/handler/util/common/flowproxy_test.gorest-api/common/pkg/coreproxy/coreproxy.gorest-api/common/pkg/coreproxy/coreproxy_test.gorest-api/common/pkg/flowproxy/flowproxy.gorest-api/common/pkg/flowproxy/flowproxy_test.gorest-api/common/pkg/secretjson/secretjson.gorest-api/common/pkg/secretjson/secretjson_test.gorest-api/site-agent/pkg/components/managers/flowgrpc/subscriber.gorest-api/site-workflow/pkg/activity/coreproxy.gorest-api/site-workflow/pkg/activity/flowproxy.gorest-api/site-workflow/pkg/grpc/client/flow_proxy.gorest-api/site-workflow/pkg/grpc/client/flow_proxy_test.gorest-api/site-workflow/pkg/workflow/flowproxy.gorest-api/site-workflow/pkg/workflow/flowproxy_test.gorest-api/skills/rest-core-grpc-proxy/SKILL.mdrest-api/skills/rest-flow-grpc-proxy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (21)
- rest-api/common/pkg/flowproxy/flowproxy_test.go
- rest-api/api/pkg/api/handler/ueficredential_test.go
- rest-api/skills/rest-core-grpc-proxy/SKILL.md
- rest-api/site-workflow/pkg/activity/coreproxy.go
- rest-api/site-workflow/pkg/workflow/flowproxy.go
- rest-api/site-workflow/pkg/activity/flowproxy.go
- rest-api/site-workflow/pkg/grpc/client/flow_proxy_test.go
- rest-api/api/internal/server/server.go
- rest-api/api/internal/server/server_test.go
- rest-api/site-workflow/pkg/grpc/client/flow_proxy.go
- rest-api/AGENTS.md
- rest-api/api/pkg/api/handler/util/common/flowproxy_test.go
- rest-api/common/pkg/flowproxy/flowproxy.go
- rest-api/common/pkg/coreproxy/coreproxy_test.go
- rest-api/api/pkg/api/handler/util/common/coreproxy.go
- rest-api/common/pkg/secretjson/secretjson_test.go
- rest-api/api/pkg/api/handler/credentialrotation_test.go
- rest-api/site-workflow/pkg/workflow/flowproxy_test.go
- rest-api/common/pkg/secretjson/secretjson.go
- rest-api/common/pkg/coreproxy/coreproxy.go
- rest-api/api/pkg/api/handler/util/common/flowproxy.go
jw-nvidia
left a comment
jw-nvidia
left a comment
There was a problem hiding this comment.
The codes related to secrets are moved into its own package to be shared. There are still a lot of common codes between coreproxy and flowproxy, it'd be nice to consolidate those common codes.
The Core and Flow proxies were the same five files twice, differing only in the backend's name and its typed messages. Fold each layer into a single implementation selected by a grpcproxy.Backend value, leaving a wrapper per backend only where Temporal dispatches on the registered workflow or activity name. Merging the two payload contracts is safe for executions already in Temporal history because both types serialized to identical JSON. Tests pin that shape and the registered names, since either drifting would strand work an older worker enqueued. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/handler/util/common/grpcproxy.go`:
- Around line 120-122: The workflow-start error handling in the proxy workflow
path should map context.DeadlineExceeded from ExecuteWorkflow to HTTP 504,
matching the existing WorkflowRun.Get handling, while preserving HTTP 500 for
other errors. Add a mock test covering ExecuteWorkflow returning
context.DeadlineExceeded and asserting the 504 response.
- Around line 125-136: In the workflow response handling and proxy response
decoding blocks, split each inline error initializer from its condition. Assign
the result of we.Get to err before the first if, and assign
protojson.Unmarshal’s result before the second if, preserving the existing error
handling and return behavior.
In `@rest-api/site-workflow/pkg/grpc/client/proxy.go`:
- Around line 84-85: In the request decoding and response encoding checks around
protojson.UnmarshalOptions, separate each operation’s error assignment from its
conditional test: assign the returned error to err first, then use a standalone
if err != nil check while preserving the existing wrapped error messages and
return behavior.
In `@rest-api/site-workflow/pkg/workflow/grpcproxy.go`:
- Around line 63-66: In the workflow activity execution error path, update the
ExecuteActivity call in the workflow handler to assign the Get result to err
before the if statement. Preserve the existing logger.Error and return behavior
while removing the initializer clause from the condition.
🪄 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: 27f57b4d-878a-4838-8fe4-3fad8c6b7d4f
📒 Files selected for processing (30)
rest-api/api/internal/server/server_test.gorest-api/api/pkg/api/handler/bmccredential_test.gorest-api/api/pkg/api/handler/credentialrotation_test.gorest-api/api/pkg/api/handler/hostfirmwareconfig_test.gorest-api/api/pkg/api/handler/machinepower_test.gorest-api/api/pkg/api/handler/measuredboot_test.gorest-api/api/pkg/api/handler/operatingsystem_templated_proxy_test.gorest-api/api/pkg/api/handler/siteexplorerendpointaction_test.gorest-api/api/pkg/api/handler/sku_test.gorest-api/api/pkg/api/handler/ueficredential_test.gorest-api/api/pkg/api/handler/util/common/coreproxy.gorest-api/api/pkg/api/handler/util/common/coreproxy_test.gorest-api/api/pkg/api/handler/util/common/grpcproxy.gorest-api/api/pkg/api/handler/util/common/grpcproxy_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/common/pkg/coreproxy/coreproxy.gorest-api/common/pkg/grpcproxy/grpcproxy.gorest-api/common/pkg/grpcproxy/grpcproxy_test.gorest-api/site-workflow/pkg/activity/coreproxy.gorest-api/site-workflow/pkg/activity/grpcproxy.gorest-api/site-workflow/pkg/grpc/client/core_proxy.gorest-api/site-workflow/pkg/grpc/client/core_proxy_test.gorest-api/site-workflow/pkg/grpc/client/proxy.gorest-api/site-workflow/pkg/grpc/client/proxy_test.gorest-api/site-workflow/pkg/workflow/coreproxy.gorest-api/site-workflow/pkg/workflow/coreproxy_test.gorest-api/site-workflow/pkg/workflow/grpcproxy.gorest-api/site-workflow/pkg/workflow/grpcproxy_test.gorest-api/skills/rest-core-grpc-proxy/SKILL.mdrest-api/skills/rest-flow-grpc-proxy/SKILL.md
💤 Files with no reviewable changes (8)
- rest-api/site-workflow/pkg/workflow/coreproxy.go
- rest-api/site-workflow/pkg/workflow/coreproxy_test.go
- rest-api/common/pkg/coreproxy/coreproxy.go
- rest-api/site-workflow/pkg/activity/coreproxy.go
- rest-api/api/pkg/api/handler/util/common/coreproxy.go
- rest-api/site-workflow/pkg/grpc/client/core_proxy.go
- rest-api/site-workflow/pkg/grpc/client/core_proxy_test.go
- rest-api/api/pkg/api/handler/util/common/coreproxy_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- rest-api/skills/rest-flow-grpc-proxy/SKILL.md
- rest-api/skills/rest-core-grpc-proxy/SKILL.md
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Overall looks great @kunzhao-nv, left a few comments.
| - Secret fields that must not appear in Temporal history (top-level protojson | ||
| field names). | ||
| - Whether timeout should terminate the workflow (`TerminateWorkflowOnTimeOut`) | ||
| — TaskRun does this when `ExecuteFlowGRPC` returns `504`. | ||
|
|
||
| ## Implementation Workflow | ||
|
|
||
| 1. Keep auth, tenant/org membership, site lookup, Flow enablement, role checks, | ||
| request validation, and REST semantics in the REST handler. | ||
| 2. Build the typed Flow protobuf request before calling the proxy. | ||
| 3. Call `common.ExecuteFlowGRPC(ctx, siteTemporalClient, fullMethod, reqProto, | ||
| respProtoOrNil, workflowID, conflictPolicy, siteIDSecretKey, secretFields...)`. |
There was a problem hiding this comment.
Seems to be a fair ask.
Addresses PR review feedback. The redaction helpers lived in their own package only because Core and Flow were separate packages; with a single grpcproxy they belong beside the contract they serve. Naming secret fields without a key now fails instead of silently sending the fields unredacted into Temporal history. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rest-api/common/pkg/grpcproxy/secrets_test.go (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup the test cases by production function.
This file adds multiple top-level tests for
RedactSecretsandMergeSecrets. Use one top-levelTestRedactSecretsfunction and one top-levelTestMergeSecretsfunction. Put the cases in named table-drivent.Runsubtests.As per coding guidelines, files matching
rest-api/**/*_test.gomust organize tests by production function with one top-levelTestfunction and table-driven named subtests.Also applies to: 39-39, 53-53, 68-73
🤖 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/common/pkg/grpcproxy/secrets_test.go` at line 14, Restructure the tests by production function: replace the separate top-level tests for RedactSecrets and MergeSecrets with TestRedactSecrets and TestMergeSecrets, respectively. Move each scenario into named table-driven t.Run subtests under the corresponding function, preserving the existing case coverage and assertions.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.
Inline comments:
In `@rest-api/common/pkg/grpcproxy/secrets.go`:
- Around line 30-33: Update the JSON decoding in both MergeSecrets and the
related redaction function to reject a nil decoded map, including payloads
containing JSON null, before any map operations such as maps.Copy. Return a
descriptive error for each null input and add tests covering null payloads in
both functions.
---
Nitpick comments:
In `@rest-api/common/pkg/grpcproxy/secrets_test.go`:
- Line 14: Restructure the tests by production function: replace the separate
top-level tests for RedactSecrets and MergeSecrets with TestRedactSecrets and
TestMergeSecrets, respectively. Move each scenario into named table-driven t.Run
subtests under the corresponding function, preserving the existing case coverage
and assertions.
🪄 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: 96a9c80d-f480-43bd-8298-9bf592b14577
📒 Files selected for processing (12)
rest-api/api/pkg/api/handler/credentialrotation_test.gorest-api/api/pkg/api/handler/ueficredential_test.gorest-api/api/pkg/api/handler/util/common/grpcproxy.gorest-api/api/pkg/api/handler/util/common/grpcproxy_test.gorest-api/common/pkg/grpcproxy/grpcproxy.gorest-api/common/pkg/grpcproxy/secrets.gorest-api/common/pkg/grpcproxy/secrets_test.gorest-api/site-workflow/pkg/activity/grpcproxy.gorest-api/site-workflow/pkg/grpc/client/proxy.gorest-api/site-workflow/pkg/workflow/grpcproxy.gorest-api/skills/rest-core-grpc-proxy/SKILL.mdrest-api/skills/rest-flow-grpc-proxy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (8)
- rest-api/common/pkg/grpcproxy/grpcproxy.go
- rest-api/skills/rest-flow-grpc-proxy/SKILL.md
- rest-api/skills/rest-core-grpc-proxy/SKILL.md
- rest-api/site-workflow/pkg/workflow/grpcproxy.go
- rest-api/site-workflow/pkg/grpc/client/proxy.go
- rest-api/api/pkg/api/handler/util/common/grpcproxy.go
- rest-api/site-workflow/pkg/activity/grpcproxy.go
- rest-api/api/pkg/api/handler/ueficredential_test.go
|
/ok to test a173538 |
|
/ok to test a173538 |
Addresses PR review feedback. A null payload decodes into a nil map without an error, so merging secrets into one panicked in maps.Copy instead of reporting the malformed input. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
a173538 to
67812d5
Compare
3 participants
Description
Flow-backed REST endpoints each carry their own Temporal workflow, activity, and typed request, so adding or changing one means touching four packages and registering another workflow type on the site agent. NICo Core already has a generic proxy (
ExecuteCoreGRPC) that collapses that into a single reusable workflow; this adds the Flow equivalent so Flow endpoints can dispatch through one workflow type instead of one per method.InvokeFlowGRPCworkflow, itsInvokeFlowGRPCOnSiteactivity,FlowGrpcClient.InvokeJSON, and theExecuteFlowGRPCcloud helper. Requests and responses travel as protojson so they stay readable in the Temporal UI, and the final site-to-Flow hop is ordinary binary gRPC.common/pkg/coreproxybecomescommon/pkg/grpcproxy, and the workflow, activity, JSON-transcoding client, and cloud helper each keep a single body behind a thin per-backend wrapper. Everything Temporal can see on the Core side is unchanged — sameInvokeCoreGRPCworkflow type, sameInvokeCoreGRPCOnSiteactivity, samecore-grpc-<method>-<uuid>workflow IDs, same payload shape — so Core needs no coordinated rollout.ExecuteCoreGRPC, the caller supplies the workflow ID and the conflict policy. Flow read and list endpoints coalesce identical in-flight requests via a deterministic ID plusUSE_EXISTING, and creates must never coalesce, so that derivation has to stay in the handler that knows which query parameters change the response.ExecuteFlowGRPCyet. A workflow type is known only to the workers that registered it, andnico-restandnico-rest-site-agentare separate Helm releases that cannot be upgraded atomically — Temporal accepts a submission for an unregistered type, creates the execution, and nothing can advance it, so the caller learns about it only as a timeout. Handlers can therefore only switch over in a later release, once every site runs an agent that serves the proxy.WriteTimeout) rather than by the on-site budget, because a response written past that deadline never reaches the client.server.go's read and write timeouts become named constants andTest_ProxyTimeoutsFitWriteTimeoutguards the outer bound for both proxies.Related issues
Closes #4271
Type of Change
Breaking Changes
Testing
Additional Notes