Skip to content

chart: enable workloadmanager auth - #433

Closed
avinxshKD wants to merge 1 commit into
volcano-sh:mainfrom
avinxshKD:fix/workloadmanager-enable-auth
Closed

chart: enable workloadmanager auth#433
avinxshKD wants to merge 1 commit into
volcano-sh:mainfrom
avinxshKD:fix/workloadmanager-enable-auth

Conversation

@avinxshKD

Copy link
Copy Markdown

What type of PR is this?

/kind bug

What this PR does / why we need it:

Enables WorkloadManager auth in the base chart. The chart already has Router call WorkloadManager with its service account token, but WorkloadManager was not started with --enable-auth.

Also grants Router the minimal sandbox create/delete RBAC needed once WorkloadManager starts using the caller token.

Which issue(s) this PR fixes:
Fixes #432

Special notes for your reviewer:

Helm is not installed in my local env, so I could not run helm template or helm lint. I did run git diff --cached --check before committing.

Does this PR introduce a user-facing change?:

Base chart now enables WorkloadManager auth by default.

Copilot AI review requested due to automatic review settings July 9, 2026 10:51
@volcano-sh-bot volcano-sh-bot added the kind/bug Something isn't working label Jul 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@volcano-sh-bot

Copy link
Copy Markdown
Contributor

[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 hzxuzhonghu 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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces authentication configuration for the AgentCube Workload Manager, enabling the --enable-auth flag and setting up RBAC permissions for the router service account to manage sandboxes and sandboxclaims. The review feedback recommends replacing the proposed ClusterRole and ClusterRoleBinding with namespace-scoped Role and RoleBinding resources to adhere to the principle of least privilege and prevent naming conflicts in multi-namespace deployments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread manifests/charts/base/templates/rbac-router.yaml
@avinxshKD

Copy link
Copy Markdown
Author

small chart-side fix for this. Found that just passing --enable-auth was not enough, since WorkloadManager uses the caller token after auth is enabled. Added the minimal fix

@acsoto @JesseStutler PTAL, when get chance.

@codecov-commenter

codecov-commenter commented Jul 9, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.50%. Comparing base (3b19390) to head (fe295b6).
⚠️ Report is 21 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
+ Coverage   58.41%   58.50%   +0.08%     
==========================================
  Files          36       36              
  Lines        3463     3463              
==========================================
+ Hits         2023     2026       +3     
+ Misses       1231     1228       -3     
  Partials      209      209              
Flag Coverage Δ
unittests 58.50% <ø> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI review requested due to automatic review settings July 9, 2026 11:00
@avinxshKD
avinxshKD force-pushed the fix/workloadmanager-enable-auth branch from 88c3527 to 6bde635 Compare July 9, 2026 11:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@acsoto

acsoto commented Jul 13, 2026

Copy link
Copy Markdown
Member

With auth enabled, sandbox creation in any other namespace will fail even though the Router API accepts a workload namespace.

Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
Copilot AI review requested due to automatic review settings July 13, 2026 17:16
@avinxshKD
avinxshKD force-pushed the fix/workloadmanager-enable-auth branch from 6bde635 to fe295b6 Compare July 13, 2026 17:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@avinxshKD

Copy link
Copy Markdown
Author

@acsoto i agree, updated it and release-prefixed the ClusterRole/Binding names to avoid install collisions

@acsoto

acsoto commented Jul 14, 2026

Copy link
Copy Markdown
Member

I’m not sure enabling the current EnableAuth path is right. It couples caller authentication with credential delegation: WM starts using the Router service account for Kubernetes operations, which forces cluster-wide sandbox permissions onto the Router, while WM should use its own service account after authenticating the Router. we may align on the intended model before adding this?

@avinxshKD

Copy link
Copy Markdown
Author

I’ll rework this so WM uses the token only to authenticate the Router call and keeps using its own service account for sandbox create/delete. That should also let us drop the extra Router RBAC from this PR, What do u think?

@acsoto

acsoto commented Jul 14, 2026

Copy link
Copy Markdown
Member

I'm not sure what kind of permission behavior we're expecting right now, so I don't know how to make a recommendation.

@avinxshKD

Copy link
Copy Markdown
Author

@acsoto fair enough. since the expected auth/RBAC model isn’t clear yet, closing this PR for now. We can reopen this later once the intended WM auth model is settled.

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

Labels

kind/bug Something isn't working size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

workloadmanager chart does not enable auth

5 participants