feat(kubescape-operator): add priorityClassName support for deployments#804
Closed
bl4ko wants to merge 2 commits intokubescape:mainfrom
Closed
feat(kubescape-operator): add priorityClassName support for deployments#804bl4ko wants to merge 2 commits intokubescape:mainfrom
bl4ko wants to merge 2 commits intokubescape:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds conditional rendering of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
bl4ko
force-pushed
the
feat/add-priorityclassname-deployments
branch
from
9fb6842 to
9f6052e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/kubescape-operator/templates/operator/deployment.yaml (1)
272-278: Consider centralizing this fallback into a shared helper template.This logic is duplicated across multiple Deployment templates; extracting it once will reduce drift and simplify future changes.
♻️ Suggested refactor
+# templates/_helpers.tpl +{{- define "kubescape-operator.priorityClassName" -}} +{{- with .component.priorityClassName }} +priorityClassName: {{ . | quote }} +{{- else }} +{{- with .global }} +priorityClassName: {{ . | quote }} +{{- end }} +{{- end }} +{{- end -}}- {{- with .Values.operator.priorityClassName }} - priorityClassName: {{ . }} - {{- else }} - {{- with .Values.customScheduling.priorityClassName }} - priorityClassName: {{ . }} - {{- end }} - {{- end }} + {{- include "kubescape-operator.priorityClassName" (dict "component" .Values.operator "global" .Values.customScheduling.priorityClassName) | nindent 6 }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubescape-operator/templates/operator/deployment.yaml` around lines 272 - 278, The priorityClassName fallback logic is duplicated; extract it into a shared Helm helper (e.g., create a template named "kubescape.priorityClassName" or similar) that checks .Values.operator.priorityClassName then falls back to .Values.customScheduling.priorityClassName, and replace the inline blocks in Deployment templates (where priorityClassName is currently set from .Values.operator.priorityClassName / .Values.customScheduling.priorityClassName) with a single call to that helper to centralize the behavior and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@charts/kubescape-operator/templates/operator/deployment.yaml`:
- Around line 272-278: The priorityClassName fallback logic is duplicated;
extract it into a shared Helm helper (e.g., create a template named
"kubescape.priorityClassName" or similar) that checks
.Values.operator.priorityClassName then falls back to
.Values.customScheduling.priorityClassName, and replace the inline blocks in
Deployment templates (where priorityClassName is currently set from
.Values.operator.priorityClassName / .Values.customScheduling.priorityClassName)
with a single call to that helper to centralize the behavior and avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2e52c46-4016-46cc-8a6c-eb7f33b8ebf8
📒 Files selected for processing (6)
charts/kubescape-operator/templates/kubescape/deployment.yamlcharts/kubescape-operator/templates/kubevuln/deployment.yamlcharts/kubescape-operator/templates/operator/deployment.yamlcharts/kubescape-operator/templates/prometheus-exporter/deployment.yamlcharts/kubescape-operator/templates/storage/deployment.yamlcharts/kubescape-operator/values.yaml
Add optional priorityClassName to all 5 Deployment workloads (kubescape, kubevuln, operator, prometheus-exporter, storage) using a shared helper template with customScheduling fallback, consistent with the existing nodeSelector/affinity/tolerations pattern. Signed-off-by: bl4ko <gasperoblak@bl4ko.com>
bl4ko
force-pushed
the
feat/add-priorityclassname-deployments
branch
from
9f6052e to
41e136f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
charts/kubescape-operator/values.yaml (1)
329-329: Unrelated change: operator image tag bump.The
operator.image.tagbump fromv0.2.128tov0.2.133appears unrelated to the priorityClassName feature. Consider splitting this into a separate commit or PR for clearer change tracking, or document the reason in the PR description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubescape-operator/values.yaml` at line 329, The values.yaml change unexpectedly bumps operator.image.tag from v0.2.128 to v0.2.133 which is unrelated to the priorityClassName work; revert the operator.image.tag change in charts/kubescape-operator/values.yaml (the operator.image.tag entry) so the PR only contains the priorityClassName changes, or move just this tag bump into its own commit/PR and document the reason in the commit message/PR description.charts/kubescape-operator/tests/snapshot_test.yaml (1)
524-525: Use unmistakably dummy fixture keys to reduce secret-scan noise.Lines 524–525 use UUID-like values for
accountandaccessKeythat secret scanners may flag. Consider using clearly synthetic placeholders (e.g.,test-account-id,00000000-0000-0000-0000-000000000000) instead, both here and at other test fixture locations throughout the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubescape-operator/tests/snapshot_test.yaml` around lines 524 - 525, Replace the UUID-like secret-looking values used for the test fixture keys account and accessKey in snapshot_test.yaml with unmistakably synthetic placeholders (for example set account: test-account-id and accessKey: 00000000-0000-0000-0000-000000000000) to avoid secret-scan noise; update the entries referenced by the keys account and accessKey in the snapshot data and scan the rest of the file for other similar fixtures to replace with the same clearly dummy placeholders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/kubescape-operator/tests/snapshot_test.yaml`:
- Around line 530-535: The snapshot’s capabilities block is missing
prometheusExporter, so the prometheus-exporter template (and its new
priorityClassName path) won’t render; update the capabilities map used in this
test (the block containing
configurationScan/nodeScan/vulnerabilityScan/relevancy/runtimeDetection) to
include prometheusExporter: enable so the prometheus-exporter deployment is
rendered and the priorityClassName change is exercised.
---
Nitpick comments:
In `@charts/kubescape-operator/tests/snapshot_test.yaml`:
- Around line 524-525: Replace the UUID-like secret-looking values used for the
test fixture keys account and accessKey in snapshot_test.yaml with unmistakably
synthetic placeholders (for example set account: test-account-id and accessKey:
00000000-0000-0000-0000-000000000000) to avoid secret-scan noise; update the
entries referenced by the keys account and accessKey in the snapshot data and
scan the rest of the file for other similar fixtures to replace with the same
clearly dummy placeholders.
In `@charts/kubescape-operator/values.yaml`:
- Line 329: The values.yaml change unexpectedly bumps operator.image.tag from
v0.2.128 to v0.2.133 which is unrelated to the priorityClassName work; revert
the operator.image.tag change in charts/kubescape-operator/values.yaml (the
operator.image.tag entry) so the PR only contains the priorityClassName changes,
or move just this tag bump into its own commit/PR and document the reason in the
commit message/PR description.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dd28b0a-2c6e-412d-9539-b5ec4acb6473
⛔ Files ignored due to path filters (1)
charts/kubescape-operator/tests/__snapshot__/snapshot_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
charts/kubescape-operator/templates/_helpers.tplcharts/kubescape-operator/templates/autoupdater/cronjob.yamlcharts/kubescape-operator/templates/grype-offline-db/cronjob.yamlcharts/kubescape-operator/templates/kubescape-scheduler/cronjob.yamlcharts/kubescape-operator/templates/kubescape/deployment.yamlcharts/kubescape-operator/templates/kubevuln-scheduler/cronjob.yamlcharts/kubescape-operator/templates/kubevuln/deployment.yamlcharts/kubescape-operator/templates/node-agent/_node-agent.tplcharts/kubescape-operator/templates/operator/deployment.yamlcharts/kubescape-operator/templates/prometheus-exporter/deployment.yamlcharts/kubescape-operator/templates/storage/deployment.yamlcharts/kubescape-operator/templates/synchronizer/deployment.yamlcharts/kubescape-operator/tests/snapshot_test.yamlcharts/kubescape-operator/values.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- charts/kubescape-operator/templates/operator/deployment.yaml
- charts/kubescape-operator/templates/storage/deployment.yaml
- charts/kubescape-operator/templates/kubevuln/deployment.yaml
- charts/kubescape-operator/templates/kubescape/deployment.yaml
Comment on lines
+530
to
+535
| capabilities: | ||
| configurationScan: enable | ||
| nodeScan: enable | ||
| vulnerabilityScan: enable | ||
| relevancy: enable | ||
| runtimeDetection: enable |
There was a problem hiding this comment.
Enable prometheusExporter in this scenario to actually cover the new deployment change.
Line 530’s capability set does not include prometheusExporter: enable, so this snapshot may skip rendering prometheus-exporter and miss validating the newly added priorityClassName path in that template.
✅ Suggested patch
capabilities:
configurationScan: enable
nodeScan: enable
vulnerabilityScan: enable
relevancy: enable
runtimeDetection: enable
+ prometheusExporter: enable📝 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.
Suggested change
| capabilities: | |
| configurationScan: enable | |
| nodeScan: enable | |
| vulnerabilityScan: enable | |
| relevancy: enable | |
| runtimeDetection: enable | |
| capabilities: | |
| configurationScan: enable | |
| nodeScan: enable | |
| vulnerabilityScan: enable | |
| relevancy: enable | |
| runtimeDetection: enable | |
| prometheusExporter: enable |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/kubescape-operator/tests/snapshot_test.yaml` around lines 530 - 535,
The snapshot’s capabilities block is missing prometheusExporter, so the
prometheus-exporter template (and its new priorityClassName path) won’t render;
update the capabilities map used in this test (the block containing
configurationScan/nodeScan/vulnerabilityScan/relevancy/runtimeDetection) to
include prometheusExporter: enable so the prometheus-exporter deployment is
rendered and the priorityClassName change is exercised.
…rage Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
feat/add-priorityclassname-deployments
branch
from
4cdf536 to
af43373
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/kubescape-operator/templates/node-agent/_node-agent.tpl`:
- Around line 446-451: The current precedence in the node-agent template uses
.Values.configurations.priorityClass.enabled before
.Values.customScheduling.priorityClassName, which prevents the global fallback
(kubescape-critical) from being masked correctly; update the condition order in
the priorityClassName block so it checks .Values.nodeAgent.priorityClassName
first, then .Values.customScheduling.priorityClassName, and only then
.Values.configurations.priorityClass.enabled to emit "priorityClassName:
kubescape-critical" — adjust the conditional sequence around the
priorityClassName lines to reflect this corrected precedence.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40e33dca-6cd3-423b-8c5a-059ac1ba3f8f
⛔ Files ignored due to path filters (1)
charts/kubescape-operator/tests/__snapshot__/snapshot_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
charts/kubescape-operator/templates/autoupdater/cronjob.yamlcharts/kubescape-operator/templates/grype-offline-db/cronjob.yamlcharts/kubescape-operator/templates/kubescape-scheduler/cronjob.yamlcharts/kubescape-operator/templates/kubevuln-scheduler/cronjob.yamlcharts/kubescape-operator/templates/node-agent/_node-agent.tplcharts/kubescape-operator/templates/synchronizer/deployment.yamlcharts/kubescape-operator/tests/snapshot_test.yamlcharts/kubescape-operator/values.yaml
✅ Files skipped from review due to trivial changes (2)
- charts/kubescape-operator/templates/grype-offline-db/cronjob.yaml
- charts/kubescape-operator/templates/kubescape-scheduler/cronjob.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/kubescape-operator/tests/snapshot_test.yaml
Comment on lines
+446
to
+451
| {{- if .Values.nodeAgent.priorityClassName }} | ||
| priorityClassName: {{ .Values.nodeAgent.priorityClassName }} | ||
| {{- else if .Values.configurations.priorityClass.enabled }} | ||
| priorityClassName: kubescape-critical | ||
| {{- else if .Values.customScheduling.priorityClassName }} | ||
| priorityClassName: {{ .Values.customScheduling.priorityClassName }} |
There was a problem hiding this comment.
Global fallback is effectively masked for node-agent.
At Line 448, configurations.priorityClass.enabled takes precedence over customScheduling.priorityClassName, so global scheduling won’t apply to node-agent under default settings. This is inconsistent with the component→global fallback model used elsewhere.
💡 Suggested precedence fix
{{- if .Values.nodeAgent.priorityClassName }}
priorityClassName: {{ .Values.nodeAgent.priorityClassName }}
-{{- else if .Values.configurations.priorityClass.enabled }}
-priorityClassName: kubescape-critical
{{- else if .Values.customScheduling.priorityClassName }}
priorityClassName: {{ .Values.customScheduling.priorityClassName }}
+{{- else if .Values.configurations.priorityClass.enabled }}
+priorityClassName: kubescape-critical
{{- end }}📝 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.
Suggested change
| {{- if .Values.nodeAgent.priorityClassName }} | |
| priorityClassName: {{ .Values.nodeAgent.priorityClassName }} | |
| {{- else if .Values.configurations.priorityClass.enabled }} | |
| priorityClassName: kubescape-critical | |
| {{- else if .Values.customScheduling.priorityClassName }} | |
| priorityClassName: {{ .Values.customScheduling.priorityClassName }} | |
| {{- if .Values.nodeAgent.priorityClassName }} | |
| priorityClassName: {{ .Values.nodeAgent.priorityClassName }} | |
| {{- else if .Values.customScheduling.priorityClassName }} | |
| priorityClassName: {{ .Values.customScheduling.priorityClassName }} | |
| {{- else if .Values.configurations.priorityClass.enabled }} | |
| priorityClassName: kubescape-critical | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/kubescape-operator/templates/node-agent/_node-agent.tpl` around lines
446 - 451, The current precedence in the node-agent template uses
.Values.configurations.priorityClass.enabled before
.Values.customScheduling.priorityClassName, which prevents the global fallback
(kubescape-critical) from being masked correctly; update the condition order in
the priorityClassName block so it checks .Values.nodeAgent.priorityClassName
first, then .Values.customScheduling.priorityClassName, and only then
.Values.configurations.priorityClass.enabled to emit "priorityClassName:
kubescape-critical" — adjust the conditional sequence around the
priorityClassName lines to reflect this corrected precedence.
Contributor
|
superseded by #809 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Add optional
priorityClassNamesupport to all 5 Deployment workloads in the kubescape-operator chart:The existing
priorityClassconfiguration only applies to the node-agent DaemonSet (hardcoded tokubescape-critical). Deployments currently have no way to set apriorityClassName, which causes policy violations in clusters enforcingrequire-priorityclasspolicies (e.g., Kyverno PSS).Follows the existing
customSchedulingfallback pattern used bynodeSelector,affinity, andtolerations:.Values.<component>.priorityClassName.Values.customScheduling.priorityClassNameHow to Test
Then verify with:
helm template kubescape-operator charts/kubescape-operator -f values.yaml | grep -A1 priorityClassNameRelated issues/PRs:
Summary by CodeRabbit
New Features
Chores
Tests