fix(webhook): process InitContainers during admission and quota checks - #2560
fix(webhook): process InitContainers during admission and quota checks#2560Rickydama3 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: leodon33 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds a Draft-07 Helm values schema for HAMI charts. Updates the admission webhook to mutate init containers and apply Kubernetes init-container resource semantics during quota checks. Adds regression coverage for GPU requests in init containers. ChangesHelm values schema
Init-container admission handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
This fixes a bug where the mutating admission webhook completely ignored InitContainers, which allowed users to bypass the HAMi scheduler and GPU resource quotas if they only requested GPUs in an InitContainer. The webhook now correctly parses InitContainers, mutates the Pod scheduler name, and validates quotas using MAX(max(init_reqs), sum(container_reqs)). Fixes Project-HAMi#2558 Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
![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
🧹 Nitpick comments (1)
pkg/scheduler/webhook_test.go (1)
1099-1107: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd a quota-rejection case for init-container resources.
This init container requests only
hami.io/gpu. The configured default memory and core values are zero.GenerateResourceRequeststherefore produces zero memory and core demand, andfitResourceQuotaskipsFitQuota.The test verifies scheduler mutation only. It will still pass if init-container quota aggregation is removed. Configure non-zero init memory or cores, set a rejecting quota, and assert that admission is denied.
🤖 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 1099 - 1107, Extend the init-container resource test around GenerateResourceRequests and fitResourceQuota with non-zero memory or core requests, configure the resource quota to reject that demand, and assert admission is denied. Keep the existing GPU mutation coverage while ensuring quota rejection depends on init-container resource aggregation rather than zero-valued defaults.
🤖 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 `@charts/hami/values.schema.json`:
- Around line 46-48: Update the customURL.port schema property to validate TCP
ports from 1 through 65535 by adding the appropriate minimum and maximum
constraints alongside its integer type.
---
Nitpick comments:
In `@pkg/scheduler/webhook_test.go`:
- Around line 1099-1107: Extend the init-container resource test around
GenerateResourceRequests and fitResourceQuota with non-zero memory or core
requests, configure the resource quota to reject that demand, and assert
admission is denied. Keep the existing GPU mutation coverage while ensuring
quota rejection depends on init-container resource aggregation rather than
zero-valued defaults.
🪄 Autofix
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: 230a196b-7cf4-4a25-bd2c-02869354fa9f
📒 Files selected for processing (3)
charts/hami/values.schema.jsonpkg/scheduler/webhook.gopkg/scheduler/webhook_test.go
| "port": { | ||
| "type": "integer" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Constrain customURL.port to the valid TCP port range.
Line 47 accepts negative values, zero, and values above 65535. The webhook template renders this value directly into clientConfig.url. Reject invalid ports before Helm renders an unusable webhook endpoint.
Proposed fix
"port": {
- "type": "integer"
+ "type": "integer",
+ "minimum": 1,
+ "maximum": 65535
}📝 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.
| "port": { | |
| "type": "integer" | |
| } | |
| "port": { | |
| "type": "integer", | |
| "minimum": 1, | |
| "maximum": 65535 | |
| } |
🤖 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 `@charts/hami/values.schema.json` around lines 46 - 48, Update the
customURL.port schema property to validate TCP ports from 1 through 65535 by
adding the appropriate minimum and maximum constraints alongside its integer
type.
1c9605f to
1cfd78e
Compare
1 participant
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes a critical bug where the mutating admission webhook (
pkg/scheduler/webhook.go) completely ignoredInitContainers, leading to GPU quota bypasses and pod scheduling failures.The Problem
Previously, the
Handle()function and thefitResourceQuota()function in the webhook only iterated throughpod.Spec.Containers. This caused two major issues:InitContainer, the webhook'shasResourceflag remainedfalse. As a result, the webhook did not mutatepod.Spec.SchedulerNametohami-scheduler, causing the pod to be scheduled by the default Kubernetes scheduler without proper GPU allocation.fitResourceQuotafunction never parsed the requests ofInitContainers. A malicious or unaware user could request GPUs inside anInitContainerand bypass the configured HAMi cluster resource quotas entirely.Note: This was strictly an admission webhook issue; the scheduler backend (
pkg/device/devices.go) was already correctly parsing and allocating resources forInitContainers.The Solution
Handle()loop to iterate overpod.Spec.InitContainersin addition topod.Spec.Containers. This ensures that the webhook executesMutateAdmissionfor eachInitContainer, correctly settinghasResource = true, and mutating theSchedulerNameto use HAMi.fitResourceQuota()to calculate resource quotas forInitContainers. SinceInitContainersrun sequentially and complete before regular containers start, the effective resource limit for a Pod isMAX(max(initContainers), sum(regularContainers)).InitContainers.Containers.TestInitContainersWebhooktowebhook_test.goto explicitly verify that pods requesting resources only in anInitContainerare properly mutated, have their scheduler changed, and are verified against quotas.Which issue(s) this PR fixes:
Fixes #2558
Special notes for your reviewer:
N/A
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
New Features
Bug Fixes
Tests