HYPERFLEET-1123 - feat: define hyperfleet-critical PriorityClass and apply to maestro pods#51
HYPERFLEET-1123 - feat: define hyperfleet-critical PriorityClass and apply to maestro pods#51kuudori wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughA Kubernetes Sequence DiagramsequenceDiagram
participant Deployment as local-up-kind/gcp
participant Make as Make install-priority-classes
participant Kubectl
participant K8sAPI as Kubernetes API
participant HelmChart as Helm: Maestro Chart
participant Scheduler as Pod Scheduler
Deployment->>Make: Invoke target
Make->>Kubectl: kubectl apply priority-classes.yaml
Kubectl->>K8sAPI: Apply PriorityClass hyperfleet-critical
K8sAPI-->>Kubectl: Resource created (1000000000)
Kubectl-->>Make: Success
Deployment->>HelmChart: helm install with values.yaml
HelmChart->>Scheduler: Pod spec references priorityClassName
Scheduler->>Scheduler: Schedule with priority 1000000000
Security & Configuration Risk AnalysisPriorityClass value collision risk (CWE-400): Kubectl apply without input validation (CWE-345): The Overly broad safe-to-evict false policy (CWE-400): Setting No manifest schema validation before install: The Makefile target does not validate Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@helm/maestro/Chart.yaml`:
- Around line 9-16: The chart dependencies maestro-server and maestro-agent have
a reproducibility issue where the version fields are pinned to "0.1.1" but the
repository URLs use the mutable branch ref "?ref=main". To fix this, replace the
mutable branch references (?ref=main) in both the maestro-server and
maestro-agent repository URLs with an immutable reference that matches version
0.1.1, either by using an immutable git tag (such as ?ref=v0.1.1) or a specific
commit SHA, depending on what the upstream maestro repository provides for that
version.
In `@Makefile`:
- Line 117: The kubectl apply recipe on line 117 expands the $(MANIFESTS_DIR)
variable unquoted, creating a shell injection vulnerability (CWE-78) where
malicious input could execute arbitrary commands. Fix this by quoting the
variable reference in the file path passed to kubectl apply, changing
$(MANIFESTS_DIR) to "$(MANIFESTS_DIR)" to safely handle any input containing
special characters or shell metacharacters.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ebb6030d-47f8-49ca-99c2-3fa45aacd35a
📒 Files selected for processing (4)
Makefilehelm/maestro/Chart.yamlhelm/maestro/values.yamlmanifests/priority-classes.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
kuudori
force-pushed
the
HYPERFLEET-1123
branch
4 times, most recently
from
6a30649 to
c0d38fa
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Makefile (1)
117-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnquoted Make variable in recipe enables shell injection (CWE-78).
Line 117 expands
$(MANIFESTS_DIR)unquoted in a shell command; environment/CLI override can inject shell metacharacters on CI runners. Quote the path argument.As per coding guidelines,
**/Makefile: “Flag shell injection via unquoted variables in recipes.”🤖 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 `@Makefile` at line 117, The kubectl apply command on line 117 in the Makefile uses an unquoted Make variable $(MANIFESTS_DIR) which creates a shell injection vulnerability (CWE-78). Quote the entire path argument by wrapping $(MANIFESTS_DIR)/priority-classes.yaml in double quotes to prevent shell metacharacters in environment or CLI overrides from being interpreted as shell commands.Source: Coding guidelines
🤖 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 `@Makefile`:
- Line 117: The kubectl apply command on line 117 in the Makefile uses an
unquoted Make variable $(MANIFESTS_DIR) which creates a shell injection
vulnerability (CWE-78). Quote the entire path argument by wrapping
$(MANIFESTS_DIR)/priority-classes.yaml in double quotes to prevent shell
metacharacters in environment or CLI overrides from being interpreted as shell
commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8f6475a3-6afa-4167-a2d6-562530ac2f80
📒 Files selected for processing (4)
Makefilehelm/maestro/Chart.yamlhelm/maestro/values.yamlmanifests/priority-classes.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
✅ Files skipped from review due to trivial changes (1)
- helm/maestro/Chart.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- helm/maestro/values.yaml
- manifests/priority-classes.yaml
…apply to maestro pods
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
hyperfleet-criticalPriorityClass (value: 1000000000) as a standalone manifest atmanifests/priority-classes.yamlinstall-priority-classesMakefile target, hooked intoinstall-allandinstall-all-rabbitmqbeforeinstall-maestropriorityClassName: hyperfleet-criticalinto all four maestro pod specs (server, agent, db, mqtt)priorityClassNamehooksVerification
Tested on local kind cluster:
make install-priority-classescreates the PriorityClass with correct valuekubectl get priorityclass hyperfleet-criticalreturns value=1000000000Test plan
make install-priority-classessucceedskubectl get priorityclass hyperfleet-criticalshows value below system-cluster-critical (2B)kubectl get pods -n maestro -o custom-columns=NAME:.metadata.name,PRIORITY_CLASS:.spec.priorityClassNameshowshyperfleet-criticalon all four pods