bugfix: vm and vmi add svc to dashboard #1161
Conversation
WalkthroughThis update increments the chart versions for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard
participant Kubernetes API
participant Service Resource
User->>Dashboard: Requests service info
Dashboard->>Kubernetes API: get/list/watch on specific Service (allowed by new RBAC rule)
Kubernetes API->>Service Resource: Access granted
Service Resource-->>Kubernetes API: Responds with data
Kubernetes API-->>Dashboard: Returns service data
Dashboard-->>User: Displays service info
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/apps/versions_map(2 hunks)packages/apps/virtual-machine/Chart.yaml(1 hunks)packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/vm-instance/Chart.yaml(1 hunks)packages/apps/vm-instance/templates/dashboard-resourcemap.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/apps/vm-instance/templates/dashboard-resourcemap.yaml (1)
Learnt from: NickVolynkin
PR: cozystack/cozystack#1120
File: packages/apps/clickhouse/README.md:60-67
Timestamp: 2025-07-03T05:54:51.264Z
Learning: The `cozy-lib.resources.sanitize` function in packages/library/cozy-lib/templates/_resources.tpl supports both standard Kubernetes resource format (with limits:/requests: sections) and flat format (direct resource specifications). The flat format takes priority over nested values. CozyStack apps include cozy-lib as a chart dependency through symlinks in packages/apps/*/charts/cozy-lib directories.
packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml (1)
Learnt from: NickVolynkin
PR: cozystack/cozystack#1120
File: packages/apps/clickhouse/README.md:60-67
Timestamp: 2025-07-03T05:54:51.264Z
Learning: The `cozy-lib.resources.sanitize` function in packages/library/cozy-lib/templates/_resources.tpl supports both standard Kubernetes resource format (with limits:/requests: sections) and flat format (direct resource specifications). The flat format takes priority over nested values. CozyStack apps include cozy-lib as a chart dependency through symlinks in packages/apps/*/charts/cozy-lib directories.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (3)
packages/apps/virtual-machine/Chart.yaml (1)
20-20: Patch-level bump OK – double-check frozenappVersion.Chart
versionwas bumped to0.12.1whileappVersionstayed at0.12.0.
That is perfectly valid if the underlying application code didn’t change, but please confirm that this mismatch is intentional and documented in the CHANGELOG so downstream tooling doesn’t mistake it for drift.packages/apps/vm-instance/Chart.yaml (1)
20-20: Chart version bumped – sameappVersioncaveat.
version➜0.9.1withappVersionunchanged (0.8.0). As with the VM chart, ensure this deliberately represents a chart-only change.packages/apps/versions_map (1)
156-157: Mapping update looks coherent – verify commit70f82667exists.Old
HEADentries were pinned to the specific commit and newHEADlines added for0.12.1and0.9.1. Just make sure commit70f82667is reachable in the repo history; CI will fail otherwise.Also applies to: 174-175
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - services | ||
| resourceNames: | ||
| - {{ include "virtual-machine.fullname" . }} | ||
| verbs: ["get", "list", "watch"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten RBAC rule – list/watch + resourceNames grants cluster-wide visibility.
When resourceNames is specified, the Kubernetes RBAC authorizer ignores it for list/watch verbs, effectively allowing the caller to observe all Services in the namespace – broader than the single-service intent.
Consider splitting the rule:
- resourceNames:
- - {{ include "virtual-machine.fullname" . }}
- verbs: ["get", "list", "watch"]
+# Narrow GET to the exact Service
+ resourceNames:
+ - {{ include "virtual-machine.fullname" . }}
+ verbs: ["get"]
+
+# Separate rule for list/watch (no resourceNames filter possible)
+- apiGroups:
+ - ""
+ resources: ["services"]
+ verbs: ["list", "watch"]This keeps observability while avoiding unnecessary data exposure.
📝 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.
| - apiGroups: | |
| - "" | |
| resources: | |
| - services | |
| resourceNames: | |
| - {{ include "virtual-machine.fullname" . }} | |
| verbs: ["get", "list", "watch"] | |
| - apiGroups: | |
| - "" | |
| resources: | |
| - services | |
| # Narrow GET to the exact Service | |
| resourceNames: | |
| - {{ include "virtual-machine.fullname" . }} | |
| verbs: ["get"] | |
| # Separate rule for list/watch (no resourceNames filter possible) | |
| - apiGroups: | |
| - "" | |
| resources: ["services"] | |
| verbs: ["list", "watch"] |
🤖 Prompt for AI Agents
In packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml around
lines 6 to 12, the RBAC rule uses resourceNames with list and watch verbs, which
Kubernetes ignores, granting broader access than intended. To fix this, split
the rule into two: one with get verb and resourceNames to restrict to the
specific service, and another with list and watch verbs without resourceNames to
allow observing all services in the namespace without overexposure.
There was a problem hiding this comment.
List/watch works correctly with resource names. kubectl get resource resourcename -w watches for changes on a single resource. Listing a specific resource is possible too: kubectl get resource --field-selector .metadata.name=resourcename and the k8s api server correctly distinguishes such requests from their less specific kubectl get resource -w and kubectl get resource counterparts.
There was a problem hiding this comment.
@lllamnyp Thank you for the correction! You're absolutely right. I was mistaken about how Kubernetes RBAC handles resourceNames with list/watch verbs. The original RBAC rule in the code is actually correct and properly restricts access to the specific service resource.
I appreciate you taking the time to clarify this with concrete examples. The kubectl examples you provided clearly demonstrate that the API server does indeed respect resourceNames filtering for list/watch operations, contrary to my initial analysis.
🐰✨ learns something new about RBAC
✏️ Learnings added
Learnt from: lllamnyp
PR: cozystack/cozystack#1161
File: packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml:6-12
Timestamp: 2025-07-10T12:14:27.174Z
Learning: Kubernetes RBAC rules with resourceNames work correctly for list/watch verbs. When resourceNames is specified in an RBAC rule, it properly restricts access to only those named resources, even for list and watch operations. Examples: `kubectl get resource resourcename -w` watches for changes on a single resource, and `kubectl get resource --field-selector .metadata.name=resourcename` lists a specific resource. The Kubernetes API server correctly distinguishes such requests from their less specific counterparts.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - services | ||
| resourceNames: | ||
| - {{ include "virtual-machine.fullname" . }} | ||
| verbs: ["get", "list", "watch"] |
There was a problem hiding this comment.
Build-breaking template reference – wrong fullname helper.
{{ include "virtual-machine.fullname" . }} is not defined inside the vm-instance chart (unless the chart explicitly declares virtual-machine as a dependency). Helm will error with:
template: vm-instance/templates/dashboard-resourcemap.yaml:...: execution error at <include "virtual-machine.fullname" .>: template: no template "virtual-machine.fullname" defined
Fix:
- resourceNames:
- - {{ include "virtual-machine.fullname" . }}
+ resourceNames:
+ - {{ include "vm-instance.fullname" . }}Apply the same separation of GET vs LIST/WATCH as noted for the VM chart to avoid over-privilege.
📝 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.
| - apiGroups: | |
| - "" | |
| resources: | |
| - services | |
| resourceNames: | |
| - {{ include "virtual-machine.fullname" . }} | |
| verbs: ["get", "list", "watch"] | |
| - apiGroups: | |
| - "" | |
| resources: | |
| - services | |
| resourceNames: | |
| - {{ include "vm-instance.fullname" . }} | |
| verbs: ["get", "list", "watch"] |
🤖 Prompt for AI Agents
In packages/apps/vm-instance/templates/dashboard-resourcemap.yaml around lines 6
to 12, the template incorrectly references {{ include "virtual-machine.fullname"
. }}, which is undefined in the vm-instance chart and causes a build error.
Replace this with the correct fullname helper defined within the vm-instance
chart, typically {{ include "vm-instance.fullname" . }}, and separate the
permissions for GET versus LIST/WATCH verbs to follow least privilege principles
as done in the VM chart.
There was a problem hiding this comment.
Check out https://github.com/cozystack/cozystack/blob/411a465b148ffcc415a83b87b06f1215202e42b9/packages/apps/vm-instance/templates/vm.yaml, it's referencing virtual-machine.fullname too.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the file mentioned by the user
cat packages/apps/vm-instance/templates/vm.yaml
# Also check if there are any chart dependencies that might explain this
cat packages/apps/vm-instance/Chart.yaml
# Search for all occurrences of virtual-machine.fullname in the vm-instance chart
rg "virtual-machine\.fullname" packages/apps/vm-instance/
# Check if there are any _helpers.tpl files that might define this template
find packages/apps/vm-instance/ -name "_helpers.tpl" -exec cat {} \;
Length of output: 9416
@NickVolynkin Thank you for the correction! You're absolutely right. I can see now that the vm-instance chart actually defines the virtual-machine.fullname template in its own _helpers.tpl file:
{{- define "virtual-machine.fullname" -}}
{{- if .Values.fullnameOverride }}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride }}
{{- if contains $name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}
{{- end }}This template is used consistently across multiple files in the vm-instance chart (vm.yaml, secret.yaml, service.yaml, etc.), so the usage in dashboard-resourcemap.yaml is intentional and correct.
My initial assessment was wrong - this is not a build-breaking error but rather a deliberate design choice where the vm-instance chart creates resources using the "virtual-machine" naming convention. The current implementation is working as intended.
I apologize for the incorrect suggestion and thank you for pointing out the broader context!
Signed-off-by: kklinch0 <kklinch0@gmail.com>
6b0ca9f to
f251cba
Compare
What this PR does
Release note
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores