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 4 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
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 as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new Kubernetes Security notesPriority class value exposure: PriorityClass Kubectl apply in Makefile without validation: The Safe-to-evict false on non-critical paths: Setting 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)
|
|
||
| .PHONY: install-priority-classes | ||
| install-priority-classes: check-kubectl ## Install PriorityClasses for critical infrastructure pods | ||
| kubectl apply -f $(MANIFESTS_DIR)/priority-classes.yaml |
There was a problem hiding this comment.
Quote recipe variable to prevent shell injection (CWE-78).
Line 117 expands $(MANIFESTS_DIR) unquoted inside a shell command. If that variable is injected via environment/CLI in CI, this can execute arbitrary shell fragments on the runner.
Proposed fix
- kubectl apply -f $(MANIFESTS_DIR)/priority-classes.yaml
+ kubectl apply -f "$(MANIFESTS_DIR)/priority-classes.yaml"As per coding guidelines, **/Makefile: “Flag shell injection via unquoted variables in recipes.”
📝 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.
| kubectl apply -f $(MANIFESTS_DIR)/priority-classes.yaml | |
| kubectl apply -f "$(MANIFESTS_DIR)/priority-classes.yaml" |
🤖 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 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.
Source: Coding guidelines
kuudori
force-pushed
the
HYPERFLEET-1123
branch
2 times, most recently
from
9fd5209 to
0209e53
Compare
ref=main allows upstream drift between dependency updates. Pin to the exact commit that introduced chart version 0.1.1.
Version field guards against upstream drift. SHA pin adds friction without matching team's track-main workflow.
…apply to maestro pods Add a cluster-scoped PriorityClass (value: 1000000000) to protect maestro infrastructure pods under resource pressure, and wire priorityClassName into all four maestro pod specs (server, agent, db, mqtt). - Add manifests/priority-classes.yaml with hyperfleet-critical PriorityClass - Add install-priority-classes Makefile target, hooked into install-all and install-all-rabbitmq before install-maestro - Set priorityClassName on all four maestro pods in umbrella values - Bump chart dependency versions to 0.1.1 for upstream priorityClassName hooks
|
@kuudori: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
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