Conversation
|
🥇 awesome @Bezbran |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
almost LGTM, thanks @Bezbran !
can you follow https://github.com/kubescape/helm-charts/tree/main/charts/kubescape-operator/tests#update for the unit test and check my comments?
We are still missing the new version.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves the standalone host-scanner DaemonSet and related Helm assets; adds nodeAgent hostSensor configuration, node-agent ConfigMap keys, multiple hostdata.kubescape.cloud CRD templates, and conditional RBAC rules; updates chart/version/docs and values to shift host scanning into node-agent. Changes
Sequence Diagram(s)sequenceDiagram
participant Helm as Helm (install/upgrade)
participant Chart as kubescape-operator chart
participant KubeAPI as Kubernetes API (CRDs & RBAC)
participant NodeAgent as node-agent
participant Kubescape as kubescape controller
Helm->>Chart: install/upgrade (components.nodeAgent.enabled + hostSensor.enabled)
Chart->>KubeAPI: create CRDs (hostdata.kubescape.cloud/*)
Chart->>KubeAPI: apply RBAC for kubescape & node-agent
KubeAPI-->>NodeAgent: permissions allow create/watch of host CRs
NodeAgent->>KubeAPI: create/update host data CRs (OsReleaseFile, OpenPortsList, ...)
Kubescape->>KubeAPI: read/watch host CRs for processing
Kubescape-->>KubeAPI: optionally create/patch resources using granted verbs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
🤖 Fix all issues with AI agents
In `@charts/kubescape-operator/values.yaml`:
- Around line 535-539: The chart defines a host-filesystem volume (name:
host-filesystem, hostPath: /) but it is not mounted; update
nodeAgent.volumeMounts to include a matching volumeMount (name: host-filesystem)
with the intended mountPath (e.g., mountPath: / or another appropriate path) and
any needed readOnly setting, or remove the host-filesystem volume block entirely
if it is unused; edit the nodeAgent.volumeMounts section and the host-filesystem
volume definition (the unique identifiers: nodeAgent.volumeMounts and
host-filesystem) so they are consistent.
🧹 Nitpick comments (3)
charts/kubescape-operator/templates/node-agent/clusterrole.yaml (1)
69-69: Nitpick: Extra blank line creates inconsistent spacing.The blank line at 69 between the conditional block and the next rule is inconsistent with the formatting elsewhere in the file.
Suggested fix
verbs: ["create", "get", "update", "patch", "list", "watch"] {{- end }} - - apiGroups: ["kubescape.io"]charts/kubescape-operator/templates/node-agent-crds/cloudproviderinfo-crd.yaml (2)
30-36: Consider adding theAgeprinter column for consistency.The
osreleasefile-crd.yamlincludes anAgecolumn using.metadata.creationTimestamp, but this CRD only hasNodeandLast Sensed. Adding it would provide a consistentkubectl getexperience across all hostdata CRDs.Suggested addition
- name: Last Sensed type: string jsonPath: .status.lastSensed + - name: Age + type: date + jsonPath: .metadata.creationTimestamp {{- end }}
6-8: Optional: Addcontroller-genannotation for consistency.The
osreleasefile-crd.yamlincludes acontroller-gen.kubebuilder.io/versionannotation in metadata. If this CRD is generated by controller-gen or you want to maintain consistency across all hostdata CRDs, consider adding it here as well.Suggested addition
metadata: name: cloudproviderinfos.hostdata.kubescape.cloud + annotations: + controller-gen.kubebuilder.io/version: v0.11.1 spec:
| # Host sensor configuration | ||
| hostSensor: | ||
| enabled: true | ||
| interval: 5m # duration string | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the kubescape-operator values file and explore the structure
find . -name "values.yaml" -path "*kubescape-operator*" | head -20Repository: kubescape/helm-charts
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Check file size and read the relevant sections
wc -l ./charts/kubescape-operator/values.yamlRepository: kubescape/helm-charts
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Read the hostSensor and host-filesystem sections
sed -n '530,545p' ./charts/kubescape-operator/values.yaml
echo "---"
sed -n '640,655p' ./charts/kubescape-operator/values.yamlRepository: kubescape/helm-charts
Length of output: 813
🏁 Script executed:
#!/bin/bash
# Search for volumeMounts in values.yaml
rg -n "volumeMounts" ./charts/kubescape-operator/values.yaml -A 5Repository: kubescape/helm-charts
Length of output: 2133
🏁 Script executed:
#!/bin/bash
# Read full nodeAgent volumeMounts and volumes sections
sed -n '570,610p' ./charts/kubescape-operator/values.yaml
echo "---VOLUMES---"
sed -n '635,660p' ./charts/kubescape-operator/values.yamlRepository: kubescape/helm-charts
Length of output: 1751
🏁 Script executed:
#!/bin/bash
# Read the complete volumes section
sed -n '635,700p' ./charts/kubescape-operator/values.yamlRepository: kubescape/helm-charts
Length of output: 2088
🏁 Script executed:
#!/bin/bash
# Search templates directory for references to host-filesystem, hostSensor, HOST_ROOT
find ./charts/kubescape-operator/templates -type f -name "*.yaml" -o -name "*.tpl" | head -20Repository: kubescape/helm-charts
Length of output: 1401
🏁 Script executed:
#!/bin/bash
# Find node-agent templates
find ./charts/kubescape-operator/templates -type f -name "*node*"
echo "---"
# Search for host-filesystem references in all templates
rg -l "host-filesystem" ./charts/kubescape-operator/templates/Repository: kubescape/helm-charts
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Read the node-agent template
wc -l ./charts/kubescape-operator/templates/node-agent/_node-agent.tpl
echo "---"
# Search for host-filesystem in the template
rg -n "host-filesystem|volumeMount" ./charts/kubescape-operator/templates/node-agent/_node-agent.tplRepository: kubescape/helm-charts
Length of output: 638
🏁 Script executed:
#!/bin/bash
# Read the template more carefully, focusing on volumes and volumeMounts
sed -n '120,230p' ./charts/kubescape-operator/templates/node-agent/_node-agent.tplRepository: kubescape/helm-charts
Length of output: 3405
🏁 Script executed:
#!/bin/bash
# Search for volumes definition in the template
rg -n "volumes:|^ volumes:" ./charts/kubescape-operator/templates/node-agent/_node-agent.tpl -A 15Repository: kubescape/helm-charts
Length of output: 1002
🏁 Script executed:
#!/bin/bash
# Search for the node-agent.volumes definition
rg -n "define \"node-agent.volumes\"" ./charts/kubescape-operator/templates/node-agent/_node-agent.tpl -A 50Repository: kubescape/helm-charts
Length of output: 1664
🏁 Script executed:
#!/bin/bash
# Search for HOST_ROOT and hostSensor environment variables in templates
rg -n "HOST_ROOT|hostSensor" ./charts/kubescape-operator/templates/node-agent/_node-agent.tpl -B 2 -A 2Repository: kubescape/helm-charts
Length of output: 184
🏁 Script executed:
#!/bin/bash
# Look for where HOST_ROOT is documented or if it's mentioned alongside hostSensor
rg -n "hostSensor" ./charts/kubescape-operator/values.yaml -B 5 -A 5
echo "---"
# Search entire repo for HOST_ROOT usage
rg -n "HOST_ROOT" . --type yaml --type md | head -20Repository: kubescape/helm-charts
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Search all templates for references to host-filesystem
rg -r "host-filesystem" ./charts/kubescape-operator/templates/
echo "---"
# Check values for any reference to how hostSensor uses volumes
rg -n "hostSensor" ./charts/kubescape-operator/ -rRepository: kubescape/helm-charts
Length of output: 499
🏁 Script executed:
#!/bin/bash
# Search all templates for references to host-filesystem
find ./charts/kubescape-operator -type f \( -name "*.yaml" -o -name "*.tpl" \) -exec rg "host-filesystem" {} +
echo "---"
# Check if host-filesystem appears in any meaningful context
rg "host-filesystem" ./charts/kubescape-operator/Repository: kubescape/helm-charts
Length of output: 1167
Add volumeMount for the host-filesystem volume or remove it if unused.
The host-filesystem volume defined at lines 645-648 (hostPath at /) has no corresponding volumeMount in nodeAgent.volumeMounts (lines 577-610). This volume appears in test snapshots but is never actually mounted into the pod. Either add the missing volumeMount entry with the appropriate mountPath, or remove the unused volume definition to maintain consistency.
🤖 Prompt for AI Agents
In `@charts/kubescape-operator/values.yaml` around lines 535 - 539, The chart
defines a host-filesystem volume (name: host-filesystem, hostPath: /) but it is
not mounted; update nodeAgent.volumeMounts to include a matching volumeMount
(name: host-filesystem) with the intended mountPath (e.g., mountPath: / or
another appropriate path) and any needed readOnly setting, or remove the
host-filesystem volume block entirely if it is unused; edit the
nodeAgent.volumeMounts section and the host-filesystem volume definition (the
unique identifiers: nodeAgent.volumeMounts and host-filesystem) so they are
consistent.
Signed-off-by: Bezalel Brandwine <bez@softwine.net> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
node-agent-replace-host-sensor
branch
from
691e0af to
c07edfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
charts/kubescape-operator/values.yaml (1)
645-648:⚠️ Potential issue | 🟡 Minor
host-filesystemis still orphaned/redundant.At Lines 645-648, the added
host-filesystemvolume has no matchingvolumeMount, and root/is already exposed via the existinghostvolume/mount (/host). Please remove this block or mount it intentionally.Proposed cleanup
- - hostPath: - path: / - type: Directory - name: host-filesystem🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubescape-operator/values.yaml` around lines 645 - 648, The new volume block named "host-filesystem" (hostPath path: /) is orphaned—there is no matching volumeMount and the root filesystem is already exposed via the existing "host" volume/mount (mounted at /host); either delete the "host-filesystem" volume block or, if you intended to use it, add a corresponding volumeMount that uses name: "host-filesystem" in the container spec with the intended mountPath. Locate the "host-filesystem" volume definition and remove it, or create a volumeMount entry referencing "host-filesystem" (instead of duplicating root) to resolve the redundancy.
🧹 Nitpick comments (2)
charts/kubescape-operator/templates/node-agent-crds/controlplaneinfo-crd.yaml (1)
1-38: Factor the hostdata CRD boilerplate into a shared helper.This template has the same gate, version block, schema skeleton, and printer columns as the sibling hostdata CRDs in this PR. Keeping that structure duplicated across many files will drift the next time one of those common fields changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubescape-operator/templates/node-agent-crds/controlplaneinfo-crd.yaml` around lines 1 - 38, This CRD duplicates boilerplate (the gate check using .Values.nodeAgent.config.hostSensor.enabled, the apiVersion/kind CustomResourceDefinition spec version block, openAPIV3Schema skeleton and additionalPrinterColumns) used by other hostdata CRDs; extract that repeated version/schema/printer-columns block into a shared helper template (e.g., _helpers.tpl) and replace the duplicated content in ControlPlaneInfo (the current template that uses include "components" and the gate .Values.nodeAgent.config.hostSensor.enabled) with a call to that helper, passing any unique values (kind/name/plural/singular and jsonPath values like .spec.nodeName and .status.lastSensed) so the CRD file becomes the minimal wrapper that only supplies resource-specific identifiers while reusing the shared boilerplate.charts/kubescape-operator/templates/node-agent-crds/osreleasefile-crd.yaml (1)
22-55: Consider keeping this CRD as open as the sibling hostdata CRDs.This is the only reviewed hostdata CRD with a closed
spec/statusschema. That makesOsReleaseFilemuch less tolerant of chart/node-agent skew: new fields from a newer agent will be pruned until the chart upgrades the CRD. If strict lockstep is not intentional, aligning this with thex-kubernetes-preserve-unknown-fieldspattern used by the other hostdata CRDs will be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubescape-operator/templates/node-agent-crds/osreleasefile-crd.yaml` around lines 22 - 55, The OsReleaseFile CRD currently defines closed schemas for spec and status (properties: content/nodeName and lastSensed/error), which will prune unknown fields; to match the other hostdata CRDs and tolerate agent/chart skew, make spec and status preserve unknown fields by adding x-kubernetes-preserve-unknown-fields: true to the spec and status object schemas (leave the existing properties like content, nodeName, lastSensed, error in place so they’re still documented) so the OsReleaseFile CRD aligns with the hostdata pattern and won’t drop extra fields from newer agents.
🤖 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-crds/cniinfo-crd.yaml`:
- Around line 2-3: The CRD rendering is currently gated by the host sensor flag
(.Values.nodeAgent.config.hostSensor.enabled) which can cause Helm to delete the
CRD when that runtime flag is disabled; update the template so the CRD is not
tied to hostSensor.enabled—only gate it by the broader node agent enablement
($components.nodeAgent.enabled) or render it unconditionally for cluster scope;
remove the inner if that checks .Values.nodeAgent.config.hostSensor.enabled
around the CRD (and mirror the same change for the other occurrence noted) so
CRD lifecycle is independent of the hostSensor runtime flag.
---
Duplicate comments:
In `@charts/kubescape-operator/values.yaml`:
- Around line 645-648: The new volume block named "host-filesystem" (hostPath
path: /) is orphaned—there is no matching volumeMount and the root filesystem is
already exposed via the existing "host" volume/mount (mounted at /host); either
delete the "host-filesystem" volume block or, if you intended to use it, add a
corresponding volumeMount that uses name: "host-filesystem" in the container
spec with the intended mountPath. Locate the "host-filesystem" volume definition
and remove it, or create a volumeMount entry referencing "host-filesystem"
(instead of duplicating root) to resolve the redundancy.
---
Nitpick comments:
In
`@charts/kubescape-operator/templates/node-agent-crds/controlplaneinfo-crd.yaml`:
- Around line 1-38: This CRD duplicates boilerplate (the gate check using
.Values.nodeAgent.config.hostSensor.enabled, the apiVersion/kind
CustomResourceDefinition spec version block, openAPIV3Schema skeleton and
additionalPrinterColumns) used by other hostdata CRDs; extract that repeated
version/schema/printer-columns block into a shared helper template (e.g.,
_helpers.tpl) and replace the duplicated content in ControlPlaneInfo (the
current template that uses include "components" and the gate
.Values.nodeAgent.config.hostSensor.enabled) with a call to that helper, passing
any unique values (kind/name/plural/singular and jsonPath values like
.spec.nodeName and .status.lastSensed) so the CRD file becomes the minimal
wrapper that only supplies resource-specific identifiers while reusing the
shared boilerplate.
In `@charts/kubescape-operator/templates/node-agent-crds/osreleasefile-crd.yaml`:
- Around line 22-55: The OsReleaseFile CRD currently defines closed schemas for
spec and status (properties: content/nodeName and lastSensed/error), which will
prune unknown fields; to match the other hostdata CRDs and tolerate agent/chart
skew, make spec and status preserve unknown fields by adding
x-kubernetes-preserve-unknown-fields: true to the spec and status object schemas
(leave the existing properties like content, nodeName, lastSensed, error in
place so they’re still documented) so the OsReleaseFile CRD aligns with the
hostdata pattern and won’t drop extra fields from newer agents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58b39180-b444-4276-82cc-cc0330778ad9
⛔ Files ignored due to path filters (1)
charts/kubescape-operator/tests/__snapshot__/snapshot_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
charts/kubescape-operator/.helmignorecharts/kubescape-operator/Chart.yamlcharts/kubescape-operator/README.mdcharts/kubescape-operator/assets/host-scanner-definition.yamlcharts/kubescape-operator/templates/_common.tplcharts/kubescape-operator/templates/kubescape/clusterrole.yamlcharts/kubescape-operator/templates/kubescape/deployment.yamlcharts/kubescape-operator/templates/node-agent-crds/README.mdcharts/kubescape-operator/templates/node-agent-crds/cloudproviderinfo-crd.yamlcharts/kubescape-operator/templates/node-agent-crds/cniinfo-crd.yamlcharts/kubescape-operator/templates/node-agent-crds/controlplaneinfo-crd.yamlcharts/kubescape-operator/templates/node-agent-crds/kernelversion-crd.yamlcharts/kubescape-operator/templates/node-agent-crds/kubeletinfo-crd.yamlcharts/kubescape-operator/templates/node-agent-crds/kubeproxyinfo-crd.yamlcharts/kubescape-operator/templates/node-agent-crds/linuxkernelvariables-crd.yamlcharts/kubescape-operator/templates/node-agent-crds/linuxsecurityhardening-crd.yamlcharts/kubescape-operator/templates/node-agent-crds/openports-crd.yamlcharts/kubescape-operator/templates/node-agent-crds/osreleasefile-crd.yamlcharts/kubescape-operator/templates/node-agent/clusterrole.yamlcharts/kubescape-operator/values.yaml
💤 Files with no reviewable changes (2)
- charts/kubescape-operator/templates/_common.tpl
- charts/kubescape-operator/assets/host-scanner-definition.yaml
✅ Files skipped from review due to trivial changes (2)
- charts/kubescape-operator/Chart.yaml
- charts/kubescape-operator/templates/node-agent-crds/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- charts/kubescape-operator/README.md
- charts/kubescape-operator/.helmignore
- charts/kubescape-operator/templates/kubescape/deployment.yaml
- charts/kubescape-operator/templates/kubescape/clusterrole.yaml
| {{- if $components.nodeAgent.enabled }} | ||
| {{- if .Values.nodeAgent.config.hostSensor.enabled }} |
There was a problem hiding this comment.
Avoid tying CRD lifecycle to hostSensor.enabled.
At Line 3, disabling nodeAgent.config.hostSensor.enabled on upgrade will stop rendering this CRD, and Helm can delete it (and its stored objects). For cluster-scoped host data, this is a risky lifecycle coupling to a runtime flag.
Proposed change
{{- $components := fromYaml (include "components" .) }}
{{- if $components.nodeAgent.enabled }}
-{{- if .Values.nodeAgent.config.hostSensor.enabled }}
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: cniinfos.hostdata.kubescape.cloud
@@
-{{- end }}
{{- end }}Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/kubescape-operator/templates/node-agent-crds/cniinfo-crd.yaml` around
lines 2 - 3, The CRD rendering is currently gated by the host sensor flag
(.Values.nodeAgent.config.hostSensor.enabled) which can cause Helm to delete the
CRD when that runtime flag is disabled; update the template so the CRD is not
tied to hostSensor.enabled—only gate it by the broader node agent enablement
($components.nodeAgent.enabled) or render it unconditionally for cluster scope;
remove the inner if that checks .Values.nodeAgent.config.hostSensor.enabled
around the CRD (and mirror the same change for the other occurrence noted) so
CRD lifecycle is independent of the hostSensor runtime flag.
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/configmap.yaml`:
- Around line 32-33: The JSON in the ConfigMap template renders
hostSensor.interval unquoted causing invalid JSON; update the template so both
"hostSensorEnabled" and "hostSensorInterval" use Helm's toJson filter (apply |
toJson to .Values.nodeAgent.config.hostSensor.enabled and to
.Values.nodeAgent.config.hostSensor.interval) so the values are properly
JSON-encoded (booleans and duration strings will be correctly quoted/formatted)
in the rendered config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e914763-48bd-43cd-8eb5-8859ffbfe7f7
⛔ Files ignored due to path filters (1)
charts/kubescape-operator/tests/__snapshot__/snapshot_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
charts/kubescape-operator/templates/node-agent/configmap.yaml
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
node-agent-replace-host-sensor
branch
from
f4307f9 to
050623a
Compare
3 participants
Overview
The
host-scanneris a K8s daemonset which sensing some basic stuff from a K8s node and expose them in a K8s YAML-like format via HTTP handlers and it runs by Kubescape only for the period of Kubescape scanning process.We want to merge host-scanner into node-agent and let the node-agent itself to sense the stuff and send it to K8s API server as new CRDs.
The motivation for this change is well explained in this slack thread
How to Test
As ususal
Related issues/PRs:
#773
kubescape/kubescape#1916
kubescape/node-agent#681
Summary by CodeRabbit
New Features
Chores