Skip to content

feat: Add workflow-level executor plugin settings definition#15440

Open
ntny wants to merge 8 commits intoargoproj:mainfrom
ntny:workflow-level-executor-plugin-configuration
Open

feat: Add workflow-level executor plugin settings definition#15440
ntny wants to merge 8 commits intoargoproj:mainfrom
ntny:workflow-level-executor-plugin-configuration

Conversation

@ntny
Copy link

@ntny ntny commented Jan 28, 2026

Fixes #15234

Motivation

Enable workflow-level configuration of the Argo Workflow Executor Plugin. When settings are specified in the workflow spec under executorPlugins.spec.sidecar, the plugin uses that configuration directly. If no sidecar is defined in the workflow spec, it falls back to the ConfigMap, preserving the previous behavior.

In other words, if executorPlugins.spec is provided in the workflow spec, the settings from the ConfigMap are ignored, and the agent pod will launch only the sidecars defined in the workflow spec.

Example Argo Workflow Spec

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: http-template
  namespace: default
spec:
  podSpecPatch: |
    nodeName: virtual-node
  entrypoint: main
  executorPlugins: #executor plugin settings
    -  metadata:
        name: test-sidecar
    spec:
        sidecar:
          container:
            name: test-sidecar
            image: busybox:1.35
            ports:
              - containerPort: 8080
            resources:
              requests:
                cpu: "100m"
                memory: "128Mi"
              limits:
                cpu: "200m"
                memory: "256Mi"
            securityContext:
              runAsUser: 1000
              runAsGroup: 1000
              runAsNonRoot: true
  templates:
    - name: main
      steps:
        ...

This feature enable via ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS=true controller env variable

How I tested

Sample of the Argo Workflow spec:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: wf-level-plugin
  namespace: argo
spec:
  entrypoint: hello-hello-hello
  executorPlugins:  # configuration for executor plugins
    - metadata:
       name:  print-message-plugin
    spec:
        sidecar:
          container:
            name: print-message-plugin
            image: print-message-plugin:latest
            imagePullPolicy: IfNotPresent
            ports:
              - containerPort: 8080
            resources:
              limits:
                cpu: 500m
                memory: 128Mi
              requests:
                cpu: 250m
                memory: 64Mi
            securityContext:
              runAsNonRoot: true
              runAsUser: 65534

  templates:
    - name: hello-hello-hello
      steps:
        - - name: hello1
            template: print-message
            arguments:
              parameters:
                - name: message
                  value: "hello1"
        - - name: hello2a
            template: print-message
            arguments:
              parameters:
                - name: message
                  value: "hello2a"
          - name: hello2b
            template: print-message
            arguments:
              parameters:
                - name: message
                  value: "hello2b"

    - name: print-message  # step that uses the plugin
      inputs:
        parameters:
          - name: message
      plugin:
        print-message-plugin:
          args: ["{{inputs.parameters.message}}"]
  1. ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS disabled in controller
  • Deployed the workflow controller without the ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS environment variable.

  • Submitted a workflow that defines executorPlugins at the workflow level:

kubectl -n argo apply -f workflows/wf-level-plugin.yaml

Result: The workflow failed as expected.
Снимок экрана 2026-02-01 в 00 16 53

argo workflow events:
`Events:
Type Reason Age From Message


Normal WorkflowRunning 2m6s workflow-controller Workflow Running
Normal WorkflowNodeRunning 2m6s workflow-controller Running node wf-level-plugin
Normal WorkflowNodeRunning 2m6s workflow-controller Running node wf-level-plugin[0]
Normal WorkflowNodeRunning 2m6s workflow-controller Running node wf-level-plugin[0].hello1: create agent pod failed with reason:"workflow-level executor plugins are disabled in the controller. To enable them, set the environment variable ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS=true"
Warning WorkflowNodeError 2m6s workflow-controller Error node wf-level-plugin[0].hello1: create agent pod failed with reason:"workflow-level executor plugins are disabled in the controller. To enable them, set the environment variable ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS=true"
Warning WorkflowFailed 2m6s workflow-controller child 'wf-level-plugin-1169000434' failed
Warning WorkflowNodeFailed 2m6s workflow-controller Failed node wf-level-plugin: child 'wf-level-plugin-1169000434' failed
Warning WorkflowNodeFailed 2m6s workflow-controller Failed node wf-level-plugin[0]: child 'wf-level-plugin-1169000434' failed`

  1. No workflow-level plugins, only plugin template (controller flag ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS = ON and OFF)
    Submitted a workflow without executorPlugins in the workflow spec, but using a executor plugin template spec.

Tested with ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS set to both true and false.

Result:
In both cases, the global agent configuration from the ConfigMap was reused as expected.

  1. Workflow-level plugins enabled in controller
  • Set ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS="true" in the workflow controller.
  • Submitted a workflow that defines its own executorPlugins:
    Result:
  • The agent pod was created according to the executorPlugins spec defined in the workflow.
  • The executor agent sidecar container received the incoming request as expected:
    arpechenin@MSK-GDC4DN4654 argo-executor-plugin-demo % kubectl -n argo logs -c print-message-plugin wf-level-plugin-1340600742-agent Skipping virtualenv creation, as specified in config file. INFO: Started server process [1] INFO: Waiting for application startup. INFO: Application startup complete. INFO: Uvicorn running on http://0.0.0.0:8080 (Press CTRL+C to quit) PRINT: ['hello1'] INFO: 127.0.0.1:57470 - "POST /api/v1/template.execute HTTP/1.1" 200 OK
  • The corresponding Argo Workflow finished successfully
    argo workflow events:
    `
    Events:
    Type Reason Age From Message
    Normal WorkflowRunning 8m31s workflow-controller Workflow Running
    Normal WorkflowNodeRunning 8m31s workflow-controller Running node wf-level-plugin
    Normal WorkflowNodeRunning 8m31s workflow-controller Running node wf-level-plugin[0]
    Normal WorkflowNodeRunning 8m10s workflow-controller Running node wf-level-plugin[0].hello1
    Normal WorkflowNodeSucceeded 8m10s workflow-controller Succeeded node wf-level-plugin[0].hello1
    Normal WorkflowNodeSucceeded 8m10s workflow-controller Succeeded node wf-level-plugin[0]
    Normal WorkflowNodeRunning 8m10s workflow-controller Running node wf-level-plugin[1]
    Normal WorkflowNodeRunning 8m workflow-controller Running node wf-level-plugin[1].hello2b
    Normal WorkflowNodeSucceeded 8m workflow-controller Succeeded node wf-level-plugin[1].hello2b
    Normal WorkflowNodeRunning 8m workflow-controller Running node wf-level-plugin[1].hello2a
    Normal WorkflowNodeSucceeded 8m workflow-controller Succeeded node wf-level-plugin[1].hello2a
    Normal WorkflowSucceeded 7m50s workflow-controller Workflow completed
    Normal WorkflowNodeSucceeded 7m50s workflow-controller Succeeded node wf-level-plugin[1]
    Normal WorkflowNodeSucceeded 7m50s workflow-controller Succeeded node wf-level-plugin
    `
    Additionally, global agent sidecars from the controller ConfigMap were ignored, as expected when workflow-level plugins are provided.
    сс @Joibel

Summary by CodeRabbit

  • New Features
    • Workflows can now declare per-workflow executor plugins via a new executorPlugins field; controller support gated by ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS.
  • Documentation
    • Docs updated with deployment/Helm examples, field references, and guidance on global vs workflow-level precedence.
  • Chores
    • CLI/start flow now exposes a WORKFLOW_LEVEL_PLUGINS flag and shows guidance/warnings when workflow-level plugins are not enabled.
  • Tests
    • Added tests validating workflow-defined executor plugin loading and agent pod sidecar behavior.

@ntny ntny force-pushed the workflow-level-executor-plugin-configuration branch 6 times, most recently from 0d862c1 to d5c06fb Compare January 28, 2026 22:27
@ntny ntny force-pushed the workflow-level-executor-plugin-configuration branch 4 times, most recently from d6596ec to 70858dd Compare February 2, 2026 23:58
@Joibel Joibel self-assigned this Feb 5, 2026
@Joibel
Copy link
Member

Joibel commented Feb 10, 2026

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 10, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 10, 2026

📝 Walkthrough

Walkthrough

Adds optional workflow-level executor plugin configuration: new API types and WorkflowSpec field, controller flag and wiring, agent logic to prefer workflow-defined plugins, Makefile/run support, docs, and tests to exercise sidecar-based executor plugins.

Changes

Cohort / File(s) Summary
Feature Doc
.features/pending/15234-argo-workflow-level-executor-plugin-configuration.md, docs/executor_plugins.md, docs/fields.md
New documentation and examples describing how to enable and use workflow-level executor plugins (ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS) and fields added to WorkflowSpec.
API Schema & Protobuf
api/jsonschema/schema.json, pkg/apis/workflow/v1alpha1/generated.proto
Adds ExecutorPlugin, ExecutorPluginSpec, ExecutorPluginSidecar types and adds executorPlugins to WorkflowSpec/Workflow in JSON schema and protobuf (including GCSArtifact.key addition).
Kubernetes API Types
pkg/apis/workflow/v1alpha1/workflow_types.go
Adds public types ExecutorPlugin, ExecutorPluginSpec, ExecutorPluginSidecar, new WorkflowSpec.ExecutorPlugins field and conversion helper AsExecutorPluginSpec().
Controller CLI / Init
cmd/workflow-controller/main.go, workflow/controller/controller.go
Adds CLI flag to enable workflow-level executor plugins, extends NewWorkflowController signature and stores new enableWorkflowLevelExecutorPlugins field.
Agent Logic
workflow/controller/agent.go
Refactors executor plugin resolution to prefer workflow-level plugins when present, adds getExecutorPluginComponents helper to build sidecar container and optional token volume, and enforces the enablement flag.
Tests
workflow/controller/agent_test.go, workflow/controller/controller_test.go
Adds tests validating loading of executorPlugins from WorkflowSpec and agent pod creation with executor sidecar; updates controller tests to reflect new flag.
Build / Run
Makefile
Adds WORKFLOW_LEVEL_PLUGINS variable, exposes ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS in local runs, and updates start messaging/UI TASKS logic.
API Rules
pkg/apis/api-rules/violation_exceptions.list
Adds list_type_missing and related exceptions for new ExecutorPlugins and several WorkflowSpec fields.

Sequence Diagram(s)

sequenceDiagram
  participant Author as Workflow Author
  participant Controller as Workflow Controller
  participant Agent as Workflow Agent
  participant KubeAPI as Kubernetes API
  Author->>Controller: Submit Workflow with executorPlugins (sidecar spec)
  Controller->>Controller: validate flag enableWorkflowLevelExecutorPlugins
  Controller->>Agent: Start workflow operation context (woc) with spec
  Agent->>Agent: woc.Spec.AsExecutorPluginSpec() -> plugin specs
  Agent->>Agent: getExecutorPluginComponents(plugin) -> container (+optional volume)
  Agent->>KubeAPI: Create Pod with task container + plugin sidecar & volumes
  KubeAPI-->>Agent: Pod created
  Agent-->>Controller: Pod status updates / events
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • Joibel
  • terrytangyuan
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'feat: Add workflow-level executor plugin settings definition' clearly and specifically describes the main change—adding workflow-level configuration for executor plugins, matching the core objective of issue #15234.
Linked Issues check ✅ Passed The PR implementation fully satisfies the requirements from issue #15234: adds workflow-level executorPlugins field to WorkflowSpec, implements fallback to ConfigMap when not defined, gates the feature with ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS environment variable, and preserves backward compatibility.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing workflow-level executor plugin configuration. Minor exception: GCSArtifact received a new 'key' field, but this appears to be an incidental enhancement and does not compromise the scope of the primary feature.
Description check ✅ Passed The PR description comprehensively addresses all required template sections: Fixes #15234, clear motivation, detailed modifications, extensive verification with testing scenarios, and documentation updates.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
workflow/controller/agent.go (1)

331-337: ⚠️ Potential issue | 🟠 Major

Add validation for workflow-level plugin containers in addresses() function.

The addresses() function directly accesses c.Ports[0].ContainerPort (line 334) without checking if the container has any ports. While validation exists in Sidecar.Validate() that enforces "at least one port is mandatory" for ConfigMap-based plugins, workflow-level plugins created via AsExecutorPluginSpec() bypass this validation. User-authored workflow plugins can therefore reach this function without ports, causing an index-out-of-range panic.

Add a bounds check before accessing Ports[0] or ensure workflow-level plugins are validated at the point they are converted in AsExecutorPluginSpec().

🤖 Fix all issues with AI agents
In
@.features/pending/15234-argo-workflow-level-executor-plugin-configuration.md:
- Line 7: Fix the typo in the sentence by changing "This feature enable via
`ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS=true` controller env variable" to a
grammatically correct form such as "This feature is enabled via the controller
env variable `ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS=true`" so the sentence reads
clearly and includes the environment variable in context.

In `@docs/executor_plugins.md`:
- Around line 5-9: Update the wording to state that enabling plugins is
controlled by the environment variables ARGO_EXECUTOR_PLUGINS and
ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS (not the ConfigMap), and clarify that the
Workflow Controller ConfigMap only stores plugin definitions; explicitly state
whether ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS is independent of
ARGO_EXECUTOR_PLUGINS (e.g., “These flags are independent: ARGO_EXECUTOR_PLUGINS
enables plugins cluster-wide, while ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS allows
per-workflow enabling regardless of the global flag”) so readers know how to set
each flag correctly.

In `@docs/fields.md`:
- Line 892: Update the docs entry for executorPlugins to reflect that
workflow-level presence (not just non-empty array) disables ConfigMap usage and
that the entire feature is gated by the ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS
flag; specifically, change the description for `executorPlugins` to state that
if a workflow-level `executorPlugins` field is present (regardless of array
length) the operator will ignore executor plugin settings from the ConfigMap,
and note that this behavior only applies when the environment feature flag
`ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS` is enabled.

In `@Makefile`:
- Around line 666-668: The echo message guarded by the WORKFLOW_LEVEL_PLUGINS
check contains a typo ("worklow-level plugins"); update the user-facing string
in the Makefile where the conditional uses WORKFLOW_LEVEL_PLUGINS to read
"workflow-level plugins" instead, preserving the exact surrounding message and
formatting so the warning remains: "@echo \"⚠️  not starting workflow-level
plugins. If you want to test plugins, run 'make start
WORKFLOW_LEVEL_PLUGINS=true' to start it\"".

In `@pkg/apis/workflow/v1alpha1/generated.proto`:
- Around line 672-686: Update the comment for the ExecutorPlugin protobuf
message to correct grammar: change the text from "ExecutorPlugin describe
workflow level executor plugin settings" to "ExecutorPlugin describes
workflow-level executor plugin settings" (adjusting subject-verb agreement and
adding the hyphen). Make this edit on the comment immediately preceding the
ExecutorPlugin message definition so the generated comment for the
ExecutorPlugin type is corrected.

In `@pkg/apis/workflow/v1alpha1/workflow_types.go`:
- Around line 1242-1255: Add metadata to the workflow-level plugin type so the
plugin Name is available: update the ExecutorPlugin struct to include
metav1.ObjectMeta (metadata) in addition to Spec so AsExecutorPluginSpec and
consumers like getExecutorPluginComponents can access plug.Name; also fix the
comment on ExecutorPluginSidecar to read "AutomountServiceAccountToken" to match
the AutomountServiceAccountToken field. Ensure AsExecutorPluginSpec fills or
preserves metadata/Name when converting so service account token volume names
are generated correctly.
- Around line 531-546: The AsExecutorPluginSpec conversion in
WorkflowSpec.AsExecutorPluginSpec() only sets Spec.Sidecar and leaves
execplugin.Plugin.TypeMeta/ObjectMeta zero-valued, so populate metadata: set
execplugin.Plugin.ObjectMeta.Name (derive from source ExecutorPlugin.Name or
fallback to "executor-plugin-<index>") and optional Namespace if available, and
set TypeMeta.Kind = "Plugin" and TypeMeta.APIVersion to the execplugin API
version constant; ensure you reference the source ExecutorPlugin fields (e.g.,
plugin.Name) when deriving the name and include these assignments when
constructing the execplugin.Plugin before appending to plugins so downstream
code can identify the converted plugin.
🧹 Nitpick comments (10)
docs/fields.md (3)

1661-1668: Tighten the ExecutorPlugin type description.

The current sentence is ungrammatical and doesn’t explain the relationship to WorkflowSpec.executorPlugins.

📝 Suggested wording update
-ExecutorPlugin describe workflow level executor plugin settings
+ExecutorPlugin defines workflow-level executor plugin settings referenced by WorkflowSpec.executorPlugins.

2523-2531: Add a brief description for ExecutorPluginSpec.

Leaving it as “No description available” makes the new API harder to understand.

📝 Suggested wording update
-_No description available_
+ExecutorPluginSpec holds configuration for an executor plugin instance.

3942-3951: Verify the service-account naming constraint + fix wording.

The description says the service account “must have the same name as the plugin” and refers to “AutomountServiceAccount” instead of AutomountServiceAccountToken. Please confirm this constraint is enforced in code; if not, reword to avoid over-promising.

#!/bin/bash
# Search for validation or enforcement around executor plugin sidecar SA naming.
rg -n "ExecutorPluginSidecar|executorPlugins|automountServiceAccountToken|service account.*plugin" -S
workflow/controller/controller.go (1)

194-194: Constructor parameter list is growing long — consider an options struct in the future.

NewWorkflowController now accepts 12 parameters. This is consistent with the existing pattern but is becoming unwieldy. A future refactor to use an options/config struct would improve readability and reduce the risk of parameter ordering mistakes.

api/jsonschema/schema.json (2)

5257-5293: Clarify how plugin identity is determined (and container name requirement).

The schema doesn’t state how a plugin is identified (likely via container.name). Please document this so users know which field controls plugin identity and any naming constraints.

📝 Suggested doc tweak
 "io.argoproj.workflow.v1alpha1.ExecutorPluginSidecar": {
   "properties": {
     "automountServiceAccountToken": {
       "description": "AutomountServiceAccount mounts the service account's token. The service account must have the same name as the plugin.",
       "type": "boolean"
     },
     "container": {
+      "description": "Sidecar container spec. The container name is used as the executor plugin name.",
       "$ref": "#/definitions/io.k8s.api.core.v1.Container"
     }
   },

8074-8080: Add feature-flag note to executorPlugins description.

Schema allows the field even when the controller feature flag is off; users may otherwise get runtime errors without realizing it’s gated.

📝 Suggested doc tweak
 "executorPlugins": {
-  "description": "ExecutorPlugins specifies a list of executor plugins at the workflow level. If any plugin is defined here, the corresponding settings from the ConfigMap are ignored.",
+  "description": "ExecutorPlugins specifies a list of executor plugins at the workflow level. If any plugin is defined here, the corresponding settings from the ConfigMap are ignored. Requires ARGO_WORKFLOW_LEVEL_EXECUTOR_PLUGINS=true.",
   "items": {
     "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.ExecutorPlugin"
   },
.features/pending/15234-argo-workflow-level-executor-plugin-configuration.md (1)

9-47: YAML code blocks should be fenced for consistent rendering.

The YAML examples rely on indentation-based Markdown code blocks. Fencing them with ```yaml would improve readability, enable syntax highlighting, and be more consistent with typical Argo docs.

workflow/controller/agent_test.go (1)

247-270: Subtest doesn't assert the sidecar was actually found; failure mode is unclear.

If the sidecar isn't in the pod (e.g., due to the feature flag issue above), executorSidecar stays nil and the test only reports assert.NotNil(t, executorSidecar) with no context. Consider using require.NotNil with a message, or require.NotNilf(t, executorSidecar, "expected test-sidecar container in pod") so failures are immediately diagnosable.

workflow/controller/agent.go (2)

275-276: Minor: unconventional variable naming wFPlugins.

Go convention is camelCase — wfPlugins would be more idiomatic.

Proposed fix
-	wFPlugins := woc.execWf.Spec.AsExecutorPluginSpec()
-	isGetPluginsFromWorkflow := len(wFPlugins) > 0
+	wfPlugins := woc.execWf.Spec.AsExecutorPluginSpec()
+	isGetPluginsFromWorkflow := len(wfPlugins) > 0

And update Line 283 accordingly:

-		for _, plugin := range wFPlugins {
+		for _, plugin := range wfPlugins {

15-16: Import grouping: spec package is separated into its own group.

The spec import (an internal project package) is isolated between the stdlib/external group and the other project imports, creating a third import group. This likely should be grouped with the other github.com/argoproj/argo-workflows/v3/... imports on Lines 17–24.

Proposed fix
 	"k8s.io/utils/ptr"
 
-	"github.com/argoproj/argo-workflows/v3/pkg/plugins/spec"
-
 	"github.com/argoproj/argo-workflows/v3/errors"
 	"github.com/argoproj/argo-workflows/v3/pkg/apis/workflow"
 	wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
+	"github.com/argoproj/argo-workflows/v3/pkg/plugins/spec"
 	"github.com/argoproj/argo-workflows/v3/util/env"

@ntny ntny force-pushed the workflow-level-executor-plugin-configuration branch 4 times, most recently from 97c00dd to 5396654 Compare February 16, 2026 16:15
arpechenin added 5 commits February 16, 2026 19:50
Signed-off-by: arpechenin <arpechenin@avito.ru>
Signed-off-by: arpechenin <arpechenin@avito.ru>
Signed-off-by: arpechenin <arpechenin@avito.ru>
Signed-off-by: arpechenin <arpechenin@avito.ru>
- fix comments

Signed-off-by: arpechenin <arpechenin@avito.ru>
@ntny ntny force-pushed the workflow-level-executor-plugin-configuration branch from 5396654 to 3b6c1fe Compare February 17, 2026 06:09
Signed-off-by: arpechenin <arpechenin@avito.ru>
@ntny ntny force-pushed the workflow-level-executor-plugin-configuration branch from 3b6c1fe to a2e59c6 Compare February 17, 2026 06:46
@ntny
Copy link
Author

ntny commented Feb 17, 2026

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 17, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (4)
docs/fields.md (1)

1661-1669: Add brief descriptions for the new public types.

The new sections render as “No description available”, which makes the docs less useful. Consider adding short type-level summaries for ExecutorPlugin, ExecutorPluginSpec, and ExecutorPluginSidecar.

Also applies to: 2524-2532, 3943-3946

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/fields.md` around lines 1661 - 1669, Add short, one-sentence type
summaries for the public types so the generated docs stop showing "_No
description available_": update the ExecutorPlugin section to include a brief
description of what an ExecutorPlugin represents, add a short summary to
ExecutorPluginSpec describing the configuration fields it contains, and add a
sentence for ExecutorPluginSidecar explaining its role (e.g., sidecar container
config for plugin runtime). Locate the doc entries for the symbols
ExecutorPlugin, ExecutorPluginSpec, and ExecutorPluginSidecar and replace the
placeholder "_No description available_" with these concise summaries.
workflow/controller/agent_test.go (1)

247-270: Test does not assert sidecar properties (resources, securityContext, ports) that are specified in the workflow YAML.

The workflow YAML specifies resources, securityContext, and ports on the sidecar container, but the test only checks that the container exists (not-nil). Consider asserting at least one of these fields to validate they are correctly propagated.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workflow/controller/agent_test.go` around lines 247 - 270, The test
AgentPodCreatedWithExecutorPluginSidecar only checks the sidecar exists; update
the test (inside the t.Run) to assert that the sidecar's resources,
securityContext, and ports from the workflow YAML are propagated: after locating
executorSidecar in the pod returned by woc.createAgentPod(ctx) assert
executorSidecar.Resources (e.g., Requests/Limits non-empty or matching expected
values), executorSidecar.SecurityContext (non-nil and expected fields), and
executorSidecar.Ports (contains the expected port name/number); keep these
assertions next to the existing NotNil checks so failures point to
createAgentPod/Agent sidecar propagation issues.
workflow/controller/agent.go (2)

269-284: Dead code: namespaces map is computed but unused when workflow-level plugins are present.

Lines 272–274 build the namespaces map on every call, but it is only used in the else branch (line 297). Consider moving it inside the else block to avoid unnecessary work and improve readability.

♻️ Suggested refactor
 func (woc *wfOperationCtx) getExecutorPlugins(ctx context.Context) ([]apiv1.Container, []apiv1.Volume, error) {
 	var sidecars []apiv1.Container
 	var volumes []apiv1.Volume
-	namespaces := map[string]bool{} // de-dupes executorPlugins when their namespaces are the same
-	namespaces[woc.controller.namespace] = true
-	namespaces[woc.wf.Namespace] = true
 	wFPlugins, err := woc.execWf.Spec.AsExecutorPluginSpec()
 	if err != nil {
 		return nil, nil, err
@@ ...
 	if isGetPluginsFromWorkflow {
 		...
 	} else {
+		namespaces := map[string]bool{}
+		namespaces[woc.controller.namespace] = true
+		namespaces[woc.wf.Namespace] = true
 		for namespace := range namespaces {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workflow/controller/agent.go` around lines 269 - 284, The namespaces map is
created unconditionally in wfOperationCtx.getExecutorPlugins but only used when
isGetPluginsFromWorkflow is false; move the declaration and initialization of
namespaces (including adding woc.controller.namespace and woc.wf.Namespace) into
the else branch where executorPlugins are collected to avoid wasted work and
improve readability, and remove the earlier unused namespaces variable; ensure
any subsequent code that relied on namespaces still has access after the move
(i.e., declare it just before the de-duplication logic that references it).

275-284: Feature-gate check happens after AsExecutorPluginSpec() — consider checking the flag first to short-circuit early.

If the feature is disabled, there's no need to parse/convert the workflow-level plugins at all. Checking the flag before calling AsExecutorPluginSpec() would be slightly more efficient and make the intent clearer.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@workflow/controller/agent.go` around lines 275 - 284, Move the feature-gate
check before calling AsExecutorPluginSpec(): check
woc.controller.enableWorkflowLevelExecutorPlugins first and if it's false, do
not call woc.execWf.Spec.AsExecutorPluginSpec() — instead set
isGetPluginsFromWorkflow to false (skip parsing/conversion) to short-circuit;
only call AsExecutorPluginSpec() when the feature flag is true and then use the
returned wFPlugins and isGetPluginsFromWorkflow as before. Ensure you reference
AsExecutorPluginSpec(), woc.execWf.Spec, and
woc.controller.enableWorkflowLevelExecutorPlugins in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@api/jsonschema/schema.json`:
- Around line 5329-5370: The schema allows an ExecutorPlugin without a
metadata.name, which contradicts ExecutorPluginSidecar's requirement that the
service account name match the plugin name; update the
"io.argoproj.workflow.v1alpha1.ExecutorPlugin" definition to require
metadata.name by changing the metadata $ref requirement to enforce "name" (i.e.,
ensure metadata is present and add a required property constraint for "name"
within the referenced io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta usage or
add a local required override) so that ExecutorPlugin objects always include
metadata.name; touch the related
"io.argoproj.workflow.v1alpha1.ExecutorPluginSpec" and "ExecutorPluginSidecar"
only if needed to keep consistency.

In `@docs/fields.md`:
- Around line 3950-3951: The doc entry for the field
`automountServiceAccountToken` uses the inconsistent phrase
“AutomountServiceAccount …”; update the description to match the actual field
name and intent. Replace the current description with a concise, matching phrase
such as "automountServiceAccountToken controls whether the service account's
token is mounted; the service account must have the same name as the plugin."
Keep the `container` line unchanged.

In `@pkg/apis/workflow/v1alpha1/workflow_types.go`:
- Around line 539-564: The AsExecutorPluginSpec conversion currently doesn't
validate plugin.ObjectMeta.Name, which allows unnamed ExecutorPlugins to pass
and causes ambiguous/invalid downstream resources; update
WorkflowSpec.AsExecutorPluginSpec to check each plugin.ObjectMeta.Name (or
plugin.Name) is non-empty before conversion, return a clear error if missing,
reuse the already-validated sidecar variable (instead of reconstructing it) when
building execplugin.PluginSpec, and keep the existing sidecar.Validate() call to
ensure sidecar validity.

---

Duplicate comments:
In `@pkg/apis/workflow/v1alpha1/workflow_types.go`:
- Around line 1260-1275: Update the field comment on ExecutorPluginSidecar to
reference the actual field name AutomountServiceAccountToken instead of
“AutomountServiceAccount”: locate the ExecutorPluginSidecar struct and change
the comment for the AutomountServiceAccountToken bool field so it accurately
describes that it mounts the service account's token and that the service
account must have the same name as the plugin (keep existing phrasing but rename
the identifier in the comment to AutomountServiceAccountToken).

---

Nitpick comments:
In `@docs/fields.md`:
- Around line 1661-1669: Add short, one-sentence type summaries for the public
types so the generated docs stop showing "_No description available_": update
the ExecutorPlugin section to include a brief description of what an
ExecutorPlugin represents, add a short summary to ExecutorPluginSpec describing
the configuration fields it contains, and add a sentence for
ExecutorPluginSidecar explaining its role (e.g., sidecar container config for
plugin runtime). Locate the doc entries for the symbols ExecutorPlugin,
ExecutorPluginSpec, and ExecutorPluginSidecar and replace the placeholder "_No
description available_" with these concise summaries.

In `@workflow/controller/agent_test.go`:
- Around line 247-270: The test AgentPodCreatedWithExecutorPluginSidecar only
checks the sidecar exists; update the test (inside the t.Run) to assert that the
sidecar's resources, securityContext, and ports from the workflow YAML are
propagated: after locating executorSidecar in the pod returned by
woc.createAgentPod(ctx) assert executorSidecar.Resources (e.g., Requests/Limits
non-empty or matching expected values), executorSidecar.SecurityContext (non-nil
and expected fields), and executorSidecar.Ports (contains the expected port
name/number); keep these assertions next to the existing NotNil checks so
failures point to createAgentPod/Agent sidecar propagation issues.

In `@workflow/controller/agent.go`:
- Around line 269-284: The namespaces map is created unconditionally in
wfOperationCtx.getExecutorPlugins but only used when isGetPluginsFromWorkflow is
false; move the declaration and initialization of namespaces (including adding
woc.controller.namespace and woc.wf.Namespace) into the else branch where
executorPlugins are collected to avoid wasted work and improve readability, and
remove the earlier unused namespaces variable; ensure any subsequent code that
relied on namespaces still has access after the move (i.e., declare it just
before the de-duplication logic that references it).
- Around line 275-284: Move the feature-gate check before calling
AsExecutorPluginSpec(): check woc.controller.enableWorkflowLevelExecutorPlugins
first and if it's false, do not call woc.execWf.Spec.AsExecutorPluginSpec() —
instead set isGetPluginsFromWorkflow to false (skip parsing/conversion) to
short-circuit; only call AsExecutorPluginSpec() when the feature flag is true
and then use the returned wFPlugins and isGetPluginsFromWorkflow as before.
Ensure you reference AsExecutorPluginSpec(), woc.execWf.Spec, and
woc.controller.enableWorkflowLevelExecutorPlugins in the change.

Signed-off-by: arpechenin <arpechenin@avito.ru>
@ntny ntny force-pushed the workflow-level-executor-plugin-configuration branch 2 times, most recently from 2bd4839 to e648104 Compare February 17, 2026 12:47
Signed-off-by: arpechenin <arpechenin@avito.ru>
@ntny ntny force-pushed the workflow-level-executor-plugin-configuration branch from e648104 to a1ba8cc Compare February 17, 2026 13:37
@ntny
Copy link
Author

ntny commented Feb 17, 2026

Hi @Joibel
Could you take a look when you have a chance?
I’ve resolved the coderabbital comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Optional Argo Workflow–Level Configuration for Executor Plugins

2 participants