Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds Argo Workflows support: API webhook + handler mapping Argo phases to internal job statuses, new workspace-engine Argo job-agent (template rendering, submission with retries), OpenAPI/schema/models for agent config, Go dependency updates, server routing, TRPC typing, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as External Argo Workflow
participant API as API Server<br/>/api/argo/webhook
participant Handler as Workflow Handler
participant DB as Database
participant Queue as Job Queue
Client->>API: POST /webhook (Authorization)
API->>API: verifyRequest() (compare header vs SECRET)
alt Verification Fails
API->>Client: 401 Unauthorized
else Verification Succeeds
API->>Handler: handleArgoWorkflow(payload)
Handler->>Handler: extract uid, phase, timestamps
Handler->>Handler: mapTriggerToStatus(phase)
alt Phase not mapped
Handler->>Handler: exit without changes
else Phase mapped
Handler->>DB: UPDATE schema.job (externalId, status, startedAt, completedAt, updatedAt)
alt No row updated
Handler->>Handler: exit
else Job updated
Handler->>DB: SELECT workspaceId via releaseJob→release→deployment
alt WorkspaceId found
Handler->>Queue: enqueueAllReleaseTargetsDesiredVersion(workspaceId)
else No workspaceId
Handler->>Handler: exit
end
end
end
API->>Client: 200 OK
end
sequenceDiagram
participant Controller as Job Dispatch<br/>Controller
participant Agent as Argo Workflow<br/>Job Agent
participant Submitter as Workflow<br/>Submitter
participant ArgoAPI as Argo Workflows<br/>API
participant Setter as Job Status<br/>Setter
Controller->>Agent: Dispatch(job)
Agent->>Agent: ParseJobAgentConfig()
Agent->>Agent: TemplateApplication() / render workflow
Agent->>Agent: MakeApplicationK8sCompatible() / set job-id label
par Async Submission
Agent->>Submitter: SubmitWorkflow(serverUrl, apiKey, workflow)
Submitter->>ArgoAPI: Create Workflow
alt Retryable Error
Submitter->>Submitter: retry with backoff (up to 5 attempts)
Submitter->>ArgoAPI: Create Workflow
else Success
ArgoAPI->>Submitter: created workflow
Submitter->>Agent: success
Agent->>Setter: UpdateJob(status=InProgress, metadata=Argo links)
else Non-retryable Error
ArgoAPI->>Submitter: error
Agent->>Setter: UpdateJob(status=Failure)
end
end
Agent->>Controller: return (no error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mleonidas
force-pushed
the
777-feat-add-argo-workflows-job-agent-integration
branch
from
f48bd05 to
55aac41
Compare
* to have some ability to have a normal name, also stores the job id on the workflow in a label so it's searchable in the workflow ui in argo workflows
* adds some tests for generating and sanitizing workflow names based upon if we're using the auto generate feature of the workflow
* based upon the job agent params we either tempalte an inline workflow and submit or we crate a workflow object that templates the workflowTemplate parameters and creates the workflow object to send to argo workflow to call the template by name
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
apps/api/src/config.ts (1)
43-44: Tighten validation for the new Argo env vars.Both values currently accept any string, so an empty secret or malformed endpoint only fails later at runtime. This schema is the best place to fail fast.
Possible schema tightening
- ARGO_WORKFLOW_WEBHOOK_SECRET: z.string().optional(), - ARGO_WORKFLOW_HTTP_ENDPOINT: z.string().optional(), + ARGO_WORKFLOW_WEBHOOK_SECRET: z.string().trim().min(1).optional(), + ARGO_WORKFLOW_HTTP_ENDPOINT: z.string().trim().url().optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/config.ts` around lines 43 - 44, Tighten the Zod schema for ARGO_WORKFLOW_WEBHOOK_SECRET and ARGO_WORKFLOW_HTTP_ENDPOINT so they fail-fast: replace ARGO_WORKFLOW_WEBHOOK_SECRET: z.string().optional() with ARGO_WORKFLOW_WEBHOOK_SECRET: z.string().nonempty().optional() to reject empty secrets, and replace ARGO_WORKFLOW_HTTP_ENDPOINT: z.string().optional() with ARGO_WORKFLOW_HTTP_ENDPOINT: z.string().url().optional() (or z.string().url({ protocols: ['http','https'] }).optional() if you want to restrict schemes) so malformed endpoints are rejected at validation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/argoworkflow/index.ts`:
- Around line 18-27: The webhook handler handleWebhookRequest must validate
req.body before calling handleArgoWorkflow: after verifyRequest() succeeds,
parse and validate the payload shape (required fields such as workflow ID,
status, and a valid timestamp or other expected properties) and return
res.status(400).json({ message: "Bad Request" }) if validation fails; only pass
the validated payload to handleArgoWorkflow. Use the existing validation
utilities or a small inline schema/validator (e.g., zod/JSON schema or manual
checks) to ensure types and date formats are correct and reject malformed bodies
at this boundary.
- Around line 11-15: The verifyRequest function currently uses a plain equality
check (authHeader === secret) which is vulnerable to timing attacks; update
verifyRequest to first ensure both authHeader and secret are defined and have
equal byte lengths, then perform a constant-time comparison using
crypto.timingSafeEqual (convert both to Buffers) and return that result;
reference verifyRequest, authHeader, secret and use timingSafeEqual from Node's
crypto to mirror the pattern used in the tfe route.
In `@apps/api/src/routes/argoworkflow/workflow.ts`:
- Around line 29-34: getJobId currently falls back to payload.workflowName which
is not a reliable UUID-backed job identifier; change
getJobId(ArgoWorkflowPayload) to only return payload.jobId (and if missing
either throw a clear Error or return null/undefined depending on your codebase
convention) and remove the workflowName fallback; update call sites that rely on
getJobId (e.g., the code paths that perform UUID-backed joins at lines that
query by jobId) to handle the new missing-jobId case and/or query by the actual
Argo workflow identifier field instead.
- Around line 19-27: The statusMap and mapTriggerToStatus currently omit Argo's
"Error" phase so terminal error workflows are ignored; update the statusMap
(used by mapTriggerToStatus) to include "Error": JobStatus.Failure so Error
returns a failure and triggers reconciliation, and decide how to handle
"Suspended" (either map "Suspended" to JobStatus.InProgress if you want it
treated as non-terminal, or leave it unmapped for special handling) while not
adding non-Argo phases like "Skipped" or "Omitted".
In `@apps/workspace-engine/oapi/spec/schemas/jobs.jsonnet`:
- Around line 163-174: ArgoWorkflowJobAgentConfig in the OpenAPI schema is too
strict: it currently requires both inline and namespace for all configs but
workflow.go only requires namespace when inline == false (and treats missing
inline as false). Update the schema to model two variants (inline template vs
template reference) instead of a single required set—e.g., split into
ArgoWorkflowJobAgentConfigInline and ArgoWorkflowJobAgentConfigRef (or use
oneOf) where the inline variant requires inline: true and template, and the ref
variant requires inline: false (or omits inline) and namespace (and template
reference fields); ensure properties like name, serverUrl, apiKey remain common,
then regenerate openapi.json / oapi.gen.go to reflect the new contract
consistent with workflow.go.
In
`@apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow_submitter.go`:
- Around line 26-32: The code currently hardcodes InsecureSkipVerify: true when
calling argoapiclient.NewClientFromOptsWithContext (using argoapiclient.Opts ->
ArgoServerOpts), which disables TLS verification; change this to use a
configurable option that defaults to false (e.g., add/use a config flag like
ArgoServerOpts.InsecureSkipVerify or UseInsecureTLS that defaults to false) and
wire that flag into the NewClientFromOptsWithContext call instead of the literal
true; alternatively accept a CA bundle option (e.g., ArgoServerOpts.CABundlePath
or RootCAs) and prefer loading trusted CA certs when provided, only enabling
InsecureSkipVerify when the explicit opt-in config is true.
- Around line 47-73: Create an idempotency guard around wfClient.CreateWorkflow
to avoid duplicate executions: before retrying, derive a deterministic workflow
name from the job id (instead of relying on GenerateName) or query existing
workflows by the job-id label (using the Argo list API) and treat an existing
match as success; modify the retry closure around CreateWorkflow (and any
related helper isRetryableError handling) so that on AlreadyExists or when an
existing workflow with the same job-id label is found it returns nil (no retry),
while preserving retry behavior for truly retryable network/server errors.
In
`@apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow.go`:
- Around line 267-271: BuildArgoLinks currently constructs appURL by
concatenating serverAddr with the workflow path then prepending "https://" which
yields malformed URLs when serverAddr already includes a scheme; update
BuildArgoLinks to normalize serverAddr before joining the path (remove any
existing "http://" or "https://" and any trailing slashes, or parse and
recompose the URL) and then build appURL using the cleaned base plus
"/workflows/{namespace}/{name}" for wf.Namespace and wf.Name so links in
ctrlplane/links are valid; ensure the final URL uses the desired scheme (e.g.,
"https://") exactly once.
- Around line 118-125: The calls to a.setter.UpdateJob in both the error branch
and the success branch are currently discarding returned errors; capture the
error returned from a.setter.UpdateJob (the calls around fmt.Sprintf("failed to
submit workflow: %s", err.Error()) and the call that sets JobStatusInProgress
with metadata from BuildArgoLinks(created)), log it with contextual fields
(job.Id, workflow id/created, and the underlying error) via the component logger
on the receiver (e.g., a.logger.Errorf) and surface it as a retryable failure
path instead of silently swallowing it (for example by returning the error from
Dispatch or enqueueing a retryable job update) so failures in UpdateJob are
visible and can be retried.
In `@packages/trpc/src/routes/deployments.ts`:
- Around line 33-45: The three agent schemas (deploymentArgoCdConfig,
deploymentTfeConfig, deploymentArgoWorkflowConfig) all share the same shape so
getTypedJobAgentConfig's sequential safeParse will misclassify argo-workflow as
argo-cd; fix this by adding an explicit discriminant field (e.g., type:
z.literal("argo-cd") / "tfe" / "argo-workflow") to each schema and update
deploymentJobAgentConfig to use those discriminated schemas (or alternatively
derive the type from the agent source earlier in getTypedJobAgentConfig) so
parsing yields the correct type.
---
Nitpick comments:
In `@apps/api/src/config.ts`:
- Around line 43-44: Tighten the Zod schema for ARGO_WORKFLOW_WEBHOOK_SECRET and
ARGO_WORKFLOW_HTTP_ENDPOINT so they fail-fast: replace
ARGO_WORKFLOW_WEBHOOK_SECRET: z.string().optional() with
ARGO_WORKFLOW_WEBHOOK_SECRET: z.string().nonempty().optional() to reject empty
secrets, and replace ARGO_WORKFLOW_HTTP_ENDPOINT: z.string().optional() with
ARGO_WORKFLOW_HTTP_ENDPOINT: z.string().url().optional() (or z.string().url({
protocols: ['http','https'] }).optional() if you want to restrict schemes) so
malformed endpoints are rejected at validation time.
🪄 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: Pro
Run ID: f065e543-1705-4c0d-8ecb-575aa2ffe9d5
⛔ Files ignored due to path filters (1)
apps/workspace-engine/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
apps/api/src/config.tsapps/api/src/routes/argoworkflow/index.tsapps/api/src/routes/argoworkflow/workflow.tsapps/api/src/server.tsapps/workspace-engine/go.modapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/schemas/jobs.jsonnetapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/svc/controllers/jobdispatch/controller.goapps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow.goapps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow_submitter.goapps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow_test.gopackages/trpc/src/routes/deployments.ts
Comment on lines
+26
to
+32
| ctx, apiClient, err := argoapiclient.NewClientFromOptsWithContext(ctx, argoapiclient.Opts{ | ||
| ArgoServerOpts: argoapiclient.ArgoServerOpts{ | ||
| URL: serverAddr, | ||
| Secure: true, | ||
| HTTP1: true, | ||
| InsecureSkipVerify: true, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -path "*/apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow_submitter.go"Repository: ctrlplanedev/ctrlplane
Length of output: 163
🏁 Script executed:
# Read the file to see the full context around lines 26-32
cat -n apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow_submitter.go | head -80Repository: ctrlplanedev/ctrlplane
Length of output: 2589
🏁 Script executed:
# Check if InsecureSkipVerify is configurable elsewhere or if there are related options
rg "InsecureSkipVerify|self.?signed|tls|Secure" apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/ -A 2 -B 2Repository: ctrlplanedev/ctrlplane
Length of output: 944
🏁 Script executed:
# Look for where this NewClientFromOptsWithContext is called and how Opts are constructed
rg "NewClientFromOptsWithContext|Opts\{" apps/workspace-engine/ -A 5 -B 2Repository: ctrlplanedev/ctrlplane
Length of output: 1225
🏁 Script executed:
# Check the argoapiclient package to understand the design intent
find . -path "*/argoapiclient*" -type f | grep -E "\.(go|md)$" | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check if there are any configuration files, env vars, or flags that control this behavior
rg "InsecureSkipVerify|skip.*verify|self.?signed" apps/workspace-engine/ -iRepository: ctrlplanedev/ctrlplane
Length of output: 486
🏁 Script executed:
# Look for any tests or documentation that explain the security decision
find apps/workspace-engine -name "*.md" -o -name "*test*.go" | xargs grep -l "InsecureSkipVerify\|self.*signed" 2>/dev/nullRepository: ctrlplanedev/ctrlplane
Length of output: 50
🏁 Script executed:
# Check the git history or comments around this code for context
git log -p --all -S "InsecureSkipVerify" -- "apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow_submitter.go" | head -100Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check if there's any configuration struct that might control this
rg "type.*Config|type.*Opts" apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/ -A 10Repository: ctrlplanedev/ctrlplane
Length of output: 1258
🏁 Script executed:
# Check if GoWorkflowSubmitter is used in production or if there's a mock version
rg "GoWorkflowSubmitter|WorkflowSubmitter" apps/workspace-engine/ -B 2 -A 2 | head -50Repository: ctrlplanedev/ctrlplane
Length of output: 3217
🏁 Script executed:
# Look at the full workflow.go to see how this submitter is instantiated and if there's any env var or config for TLS
cat -n apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow.go | head -100Repository: ctrlplanedev/ctrlplane
Length of output: 3126
Disable TLS verification bypass for production API client.
InsecureSkipVerify: true is hardcoded and disables server certificate validation for every Argo API call, exposing the bearer token to MITM attacks. This cannot be overridden by configuration. Either use proper certificates signed by a trusted CA, accept self-signed certificates via a CA bundle, or add an explicit opt-in configuration field that defaults to false instead of bypassing all verification by default.
Minimum safe change
ArgoServerOpts: argoapiclient.ArgoServerOpts{
URL: serverAddr,
Secure: true,
HTTP1: true,
- InsecureSkipVerify: true,
+ InsecureSkipVerify: false,
},📝 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.
Suggested change
| ctx, apiClient, err := argoapiclient.NewClientFromOptsWithContext(ctx, argoapiclient.Opts{ | |
| ArgoServerOpts: argoapiclient.ArgoServerOpts{ | |
| URL: serverAddr, | |
| Secure: true, | |
| HTTP1: true, | |
| InsecureSkipVerify: true, | |
| }, | |
| ctx, apiClient, err := argoapiclient.NewClientFromOptsWithContext(ctx, argoapiclient.Opts{ | |
| ArgoServerOpts: argoapiclient.ArgoServerOpts{ | |
| URL: serverAddr, | |
| Secure: true, | |
| HTTP1: true, | |
| InsecureSkipVerify: false, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow_submitter.go`
around lines 26 - 32, The code currently hardcodes InsecureSkipVerify: true when
calling argoapiclient.NewClientFromOptsWithContext (using argoapiclient.Opts ->
ArgoServerOpts), which disables TLS verification; change this to use a
configurable option that defaults to false (e.g., add/use a config flag like
ArgoServerOpts.InsecureSkipVerify or UseInsecureTLS that defaults to false) and
wire that flag into the NewClientFromOptsWithContext call instead of the literal
true; alternatively accept a CA bundle option (e.g., ArgoServerOpts.CABundlePath
or RootCAs) and prefer loading trusted CA certs when provided, only enabling
InsecureSkipVerify when the explicit opt-in config is true.
apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow_submitter.go
Outdated
Show resolved
Hide resolved
apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow.go
Show resolved
Hide resolved
apps/workspace-engine/svc/controllers/jobdispatch/jobagents/argo-workflow/workflow.go
Show resolved
Hide resolved
* add generated files and update schema
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests