Skip to content

fix(backend): detect circular component references and enforce reserved keywords in visitor.go. Fixes #13966 - #13967

Open
bhuvan-somisetty wants to merge 1 commit into
kubeflow:masterfrom
bhuvan-somisetty:fix/compiler-dfs-cycle-detection
Open

fix(backend): detect circular component references and enforce reserved keywords in visitor.go. Fixes #13966#13967
bhuvan-somisetty wants to merge 1 commit into
kubeflow:masterfrom
bhuvan-somisetty:fix/compiler-dfs-cycle-detection

Conversation

@bhuvan-somisetty

Copy link
Copy Markdown

Description of your changes:

Problem

In backend/src/v2/compiler/visitor.go, component dependency graphs are traversed using a depth-first search (pipelineDFS.dfs). Analysis of the compiler visitor pattern revealed multiple related defects:

  1. Uncaught Circular Component References: pipelineDFS used a single flat visited map[string]bool map. When a sub-DAG component referenced an ancestor component (circular dependency), if state.visited[name] { return nil } returned nil silently. This allowed invalid cyclic pipeline specs to bypass API server validation and generate incomplete/corrupted Argo Workflow specs that failed with cryptic errors in Argo.
  2. Unenforced Reserved root Component Name: RootComponentName ("root") was not validated against user component names, allowing user components named "root" to collide with root DAG traversal state.
  3. Opaque Error Context: Error messages for missing component refs or executors omitted task name and parent DAG component context.
  4. Redundant Proto Serialization Passes: GetPipelineSpec and GetDeploymentConfig performed double JSON round-tripping (protojson.Marshal -> string -> protojson.Unmarshal), adding memory allocations during large pipeline compilation.

Solution

  1. Active Call-Stack Cycle Detection: Added inStack map[string]bool to pipelineDFS to track active DFS recursion. If inStack[name] is true, dfs returns a clear cycle error (circular reference detected in component graph: <name>).
  2. Reserved Keyword Enforcement: Rejects user components named RootComponentName ("root") in Accept.
  3. Enriched Error Context: Included task name (task.GetTaskInfo().GetName()) and parent component name in componentError.
  4. Proto Unmarshaling Optimization: Direct unmarshaling from jsonBytes eliminating intermediate string conversions and buffer allocations.
  5. Unit Tests: Added visitor_test.go covering cycle detection, reserved keyword checks, and error message context.

Fixes #13966

Checklist:

@google-oss-prow
google-oss-prow Bot requested review from HumairAK and zazulam August 4, 2026 09:40
@google-oss-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign nsingla for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow

Copy link
Copy Markdown

Hi @bhuvan-somisetty. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@bhuvan-somisetty

Copy link
Copy Markdown
Author

Hi @hbelmiro @jeffspahr @droctothorpe,

Could you please review this backend compiler PR when you get a chance?

This PR refactors pipelineDFS in backend/src/v2/compiler/visitor.go to add active call-stack recursion tracking (inStack) for detecting circular component references during compilation. It also enforces reserved keyword checking for RootComponentName ("root"), enriches error messages with task context, and optimizes proto unmarshaling. Includes unit tests in visitor_test.go.

Thanks!

…ed keywords in visitor.go

Refactor pipelineDFS in backend/src/v2/compiler/visitor.go to maintain an active call-stack recursion set (inStack), preventing unhandled circular component references from emitting malformed Argo Workflows.

Enforces reserved RootComponentName ("root") check against user component names, enriches compiler error messages with task context, and eliminates redundant string conversions during proto unmarshaling.

Fixes kubeflow#13966

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
@bhuvan-somisetty
bhuvan-somisetty force-pushed the fix/compiler-dfs-cycle-detection branch from a05b62b to 6cc5f52 Compare August 4, 2026 09:42
@hbelmiro

hbelmiro commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

/ok-to-test

@github-actions github-actions Bot added the ci-passed All CI tests on a pull request have passed label Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-passed All CI tests on a pull request have passed ok-to-test size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[backend] Unhandled circular component references, keyword collisions, and inefficient proto unmarshaling in backend compiler DFS (visitor.go)

2 participants