fix webhook to deny privileged containers - #2139
Conversation
Signed-off-by: Jay2006sawant <jay242902@gmail.com>
Signed-off-by: Jay2006sawant <jay242902@gmail.com>
|
Welcome @Jay2006sawant! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
📝 WalkthroughWalkthroughThe admission webhook detects privileged init or regular containers and denies Pods containing them when GPU resources are processed. Tests cover privileged-only, mixed GPU, init-container, and scheduler-name scenarios. ChangesPrivileged admission enforcement
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/scheduler/webhook.go (1)
73-79: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEvaluate privileged containers before allowing scheduler bypass.
pkg/scheduler/webhook.goallows pods with a different scheduler before any privilege check, and only inspectspod.Spec.Containers, so privileged init containers and privileged PODs assigned to another scheduler can reach the workload. Add privilege checks for both initial/sidecar containers before the scheduler-name early return and add regression cases for the missing paths.Relevant production code
if pod.Spec.SchedulerName != "" && (pod.Spec.SchedulerName != corev1.DefaultSchedulerName || !config.ForceOverwriteDefaultScheduler) && (len(config.SchedulerName) == 0 || pod.Spec.SchedulerName != config.SchedulerName) { klog.V(3).Infof(template+" - Pod already has different scheduler assigned", req.Namespace, req.Name, req.UID) return admission.Allowed("pod already has different scheduler assigned") }🤖 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 `@pkg/scheduler/webhook.go` around lines 73 - 79, Move privilege validation ahead of the scheduler-name early return in the webhook admission flow, and apply the same validation to both pod.Spec.InitContainers and pod.Spec.Containers before allowing any scheduler bypass. Update the regression cases in pkg/scheduler/webhook_test.go covering privileged init containers and privileged pods assigned to another scheduler.
🤖 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 `@pkg/scheduler/webhook_test.go`:
- Around line 647-664: Update the test setup around InitDevicesWithConfig to
snapshot the existing global device map and handler list before initialization,
then restore both in t.Cleanup alongside config.SchedulerName. Preserve the
test’s current device initialization while preventing global registry state from
leaking into subsequent tests.
---
Outside diff comments:
In `@pkg/scheduler/webhook.go`:
- Around line 73-79: Move privilege validation ahead of the scheduler-name early
return in the webhook admission flow, and apply the same validation to both
pod.Spec.InitContainers and pod.Spec.Containers before allowing any scheduler
bypass. Update the regression cases in pkg/scheduler/webhook_test.go covering
privileged init containers and privileged pods assigned to another scheduler.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf53636a-14a6-43b1-8b3f-dff1e2fa2933
📒 Files selected for processing (2)
pkg/scheduler/webhook.gopkg/scheduler/webhook_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/scheduler/webhook_test.go (1)
789-795: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the denial is privilege-specific and patch-free.
A different denial with a non-empty message would pass, and a denied response carrying scheduler/GPU patches is not rejected by this test. Assert that the message identifies privilege and
len(resp.Patches) == 0.Proposed test assertions
+ if len(resp.Patches) != 0 { + t.Fatalf("Expected no patches for privileged pod, got %d", len(resp.Patches)) + } - if resp.Result == nil || resp.Result.Message == "" { + if resp.Result == nil || !strings.Contains(resp.Result.Message, "is privileged") { t.Fatalf("Expected denial message, got: %+v", resp.Result) }🤖 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 `@pkg/scheduler/webhook_test.go` around lines 789 - 795, Strengthen the denied-response assertions in the webhook test around wh.Handle by requiring resp.Result.Message to identify the privilege-related denial, while preserving the existing non-empty check, and assert that len(resp.Patches) equals zero so no scheduler or GPU patches accompany the denial.
🤖 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.
Outside diff comments:
In `@pkg/scheduler/webhook_test.go`:
- Around line 789-795: Strengthen the denied-response assertions in the webhook
test around wh.Handle by requiring resp.Result.Message to identify the
privilege-related denial, while preserving the existing non-empty check, and
assert that len(resp.Patches) equals zero so no scheduler or GPU patches
accompany the denial.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b034784-ea8d-4c98-bf0a-3beb55ae1f3c
📒 Files selected for processing (2)
pkg/scheduler/webhook.gopkg/scheduler/webhook_test.go
Signed-off-by: Jay2006sawant <jay242902@gmail.com>
b9c203e to
d9bb61e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/scheduler/webhook_test.go`:
- Line 764: Add an hami.io/gpu resource request to the privileged container in
the different-scheduler bypass fixture in webhook_test.go, while keeping
allowed: true, so the test validates scheduler-gate behavior for GPU-mutating
Pods.
In `@pkg/scheduler/webhook.go`:
- Around line 72-75: Update the admission handler around privilegedContainerName
and MutateAdmission to immediately return admission.Denied whenever
hasPrivileged is true, before any mutation or scheduler-gate checks. Remove the
hasResource requirement so privileged-only and unrecognized-GPU pods are denied,
and update TestPrivilegedContainerDenied expectations accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e4bd097-2359-45bd-900f-efb9f0e31d6e
📒 Files selected for processing (2)
pkg/scheduler/webhook.gopkg/scheduler/webhook_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/scheduler/webhook.go (1)
72-75:⚠️ Potential issue | 🟠 MajorReturn
admission.Deniedimmediately for privileged containers.The privileged check still occurs after the scheduler gate and all
MutateAdmissioncalls, and denial is incorrectly gated byhasResource. This admits privileged-only pods, privileged pods with unrecognized GPU requests, and privileged pods using another scheduler; mutation errors can also replace the required denial. Move the check before the scheduler gate/mutation and remove thehasResourcecondition. Align the related test expectations as well.Also applies to: 85-88
🤖 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 `@pkg/scheduler/webhook.go` around lines 72 - 75, Update the admission flow around privilegedContainerName and MutateAdmission so any privileged container returns admission.Denied immediately, before the scheduler gate or mutation calls, regardless of GPU resource detection or scheduler selection. Remove the hasResource requirement from this denial path, ensure mutation errors cannot supersede it, and update related test expectations.
🤖 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.
Duplicate comments:
In `@pkg/scheduler/webhook.go`:
- Around line 72-75: Update the admission flow around privilegedContainerName
and MutateAdmission so any privileged container returns admission.Denied
immediately, before the scheduler gate or mutation calls, regardless of GPU
resource detection or scheduler selection. Remove the hasResource requirement
from this denial path, ensure mutation errors cannot supersede it, and update
related test expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 20edd6e8-bd99-48d4-9c7b-711cfe74be43
📒 Files selected for processing (2)
pkg/scheduler/webhook.gopkg/scheduler/webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/scheduler/webhook_test.go
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, Jay2006sawant The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
The mutating admission webhook in
pkg/scheduler/webhook.gologged "Denying admission" for privileged containers but usedcontinueinstead of returningadmission.Denied. When a pod had both a privileged container and a GPU-requesting container, the webhook still allowed the pod and applied scheduler/GPU patches.This PR returns
admission.Deniedimmediately when any container is privileged, and adds unit tests for privileged-only and mixed privileged+GPU pods.Which issue(s) this PR fixes:
Fixes #2138
Special notes for your reviewer:
webhook.goandwebhook_test.gomake testandmake verifypass locallyDoes this PR introduce a user-facing change?:
Yes. Pods containing any privileged container are now rejected by the HAMi mutating webhook. Previously, a pod with a privileged sidecar plus a GPU workload could be admitted.
Summary by CodeRabbit
Bug Fixes
Tests