Repository navigation
feat(api): add Option resource and x-cozystack-options for dynamic form dropdowns - #2778
Merged
Aleksei Sviridkin (lexfrei) merged 20 commits intoJun 2, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a read-only core Option API and provider registry that compute dropdown option values on demand, implements REST storage (List/Get/Watch/Table), registers dynamic providers in the apiserver, updates RBAC, and annotates charts and CRDs with x-cozystack-options metadata linking fields to provider sources. ChangesDropdown Options API and Provider Infrastructure
Application Chart and CRD Schema Annotations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
area/api
Andrei Kvapil (kvaps)
marked this pull request as ready for review
June 1, 2026 23:07
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust mechanism for populating dashboard form dropdowns dynamically from live cluster state. By adding an Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new read-only virtual resource, Option, to dynamically serve dropdown choices (such as storage classes, instance types, and backup plans) for the dashboard UI. It implements the REST storage, dynamic providers, and registers the resource in the core API group, while updating various application schemas with x-cozystack-options annotations. The review feedback highlights a critical omission of RBAC permissions for listing plans, backups, and applicationdefinitions in the cozystack-api ServiceAccount, which would cause runtime failures. Additionally, it is recommended to use standard Kubernetes API markers (// +listType=map and // +listMapKey=value) on OptionSpec.Items to eliminate the need for an API rule violation exception.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+46
to
+51
| - apiGroups: ["backups.cozystack.io"] | ||
| resources: ["backupclasses"] | ||
| verbs: ["get", "watch", "list"] | ||
| - apiGroups: ["apps.cozystack.io"] | ||
| resources: ["vmdisks"] | ||
| verbs: ["get", "watch", "list"] |
Contributor
There was a problem hiding this comment.
The cozystack-api ServiceAccount is missing RBAC permissions to list plans and backups in the backups.cozystack.io API group, and applicationdefinitions in the apps.cozystack.io API group. Without these permissions, the plan, backup, and appkind option providers will fail with a Forbidden error at runtime when attempting to list these resources.
- apiGroups: ["backups.cozystack.io"]
resources: ["backupclasses", "plans", "backups"]
verbs: ["get", "watch", "list"]
- apiGroups: ["apps.cozystack.io"]
resources: ["vmdisks", "applicationdefinitions"]
verbs: ["get", "watch", "list"]
Comment on lines
+20
to
+23
| // OptionSpec holds the computed option items for a source. | ||
| type OptionSpec struct { | ||
| Items []OptionItem `json:"items,omitempty"` | ||
| } |
Contributor
There was a problem hiding this comment.
Instead of adding an exception to api/api-rules/cozystack_api_violation_exceptions.list for list_type_missing on OptionSpec.Items, you can use standard Kubernetes API markers // +listType=map and // +listMapKey=value directly on the struct field. This is cleaner, adheres to Kubernetes API conventions, and allows you to remove the exception from the violation list.
Suggested change
| // OptionSpec holds the computed option items for a source. | |
| type OptionSpec struct { | |
| Items []OptionItem `json:"items,omitempty"` | |
| } | |
| // OptionSpec holds the computed option items for a source. | |
| type OptionSpec struct { | |
| // +listType=map | |
| // +listMapKey=value | |
| Items []OptionItem `json:"items,omitempty"` | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/registry/core/option/providers.go (1)
56-71: Consider short-TTL caching for provider results.Every read of an Option recomputes its source by issuing live dynamic
Listcalls (e.g.gpuProviderlists KubeVirt + all Nodes,imageProviderlists PVCs,storageClassProviderlists all StorageClasses). Since the dashboard polls these dropdowns on each form open/edit and viaWatchre-list, concurrent tenants can generate noticeable read load on the apiserver. A small bounded TTL cache (or shared informer/lister) per source would cut redundant cluster reads without changing the read-only contract.🤖 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 `@pkg/registry/core/option/providers.go` around lines 56 - 71, The provider functions (e.g., gpuProvider, imageProvider, storageClassProvider, nameListProvider/nameListNamespacedProvider used in DefaultProviders) currently call the dynamic client on every Option read; wrap these providers with a small TTL cache or replace their internals with a shared informer/lister to avoid repeated dynamic.List calls. Concretely, add a generic caching wrapper (e.g., cacheProvider(providerFunc, ttl)) and use it in DefaultProviders to return cached results for a short duration (safely guarded by a mutex and invalidated after ttl or on explicit watch events), or alternatively implement per-source informer/lister usage inside the specific providers (gpuProvider, imageProvider, storageClassProvider, vmDiskProvider, etc.) so they serve from a lister instead of re-listing on every invocation.pkg/apiserver/apiserver.go (1)
199-219: Heads-up: provider reads are uncached and privileged.The dynamic client backs every Option list/get with direct, un-cached API reads of the underlying source kinds (nodes, KubeVirt, storageclasses, …). If the dashboard polls these dropdowns frequently, this could add measurable load on the kube-apiserver. Consider a short-TTL cache or informer-backed reads for the hottest sources if list latency/QPS becomes a concern in production.
🤖 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 `@pkg/apiserver/apiserver.go` around lines 199 - 219, The dynamic client created by dynamic.NewForConfig (dyn) is used by optionstorage.DefaultProviders and wired into optionstorage.NewREST, which performs uncached, privileged list/get calls for provider-backed Option resources; to mitigate load, wrap or replace the direct dynamic-backed providers with a caching layer or informer-backed reads for hot sources (e.g., implement a short-TTL in-memory cache or use informer listers for Node, KubeVirt, StorageClass kinds) and update optionstorage.DefaultProviders to accept the cached/provider-lister implementation instead of raw dyn so optionstorage.NewREST uses the cached provider reads.
🤖 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 @.github/workflows/pre-commit.yml:
- Line 50: The workflow currently pipes the release tarball directly into tar,
which skips integrity checks; instead modify the Install generate step to
download the release to a temporary file (use variables like VERSION and ASSET),
compute/verify its SHA256 against a pinned EXPECTED_SHA256 (use TMPDIR for the
temp location), and only after a successful checksum run tar to extract
cozyvalues-gen into /usr/local/bin; ensure the step uses strict shell options
(set -euo pipefail) and fails the job if the checksum does not match.
In `@packages/system/cozystack-api/templates/rbac.yaml`:
- Around line 27-51: The RBAC block in templates/rbac.yaml misses some read-only
GVRs used by the Option dropdown providers: add "plans" and "backups" to the
apiGroup "backups.cozystack.io" and add "applicationdefinitions" to the apiGroup
"apps.cozystack.io" so the providers referenced in
pkg/registry/core/option/providers.go (the plan, backup and appkind option
providers) can list/get/watch those resources; keep verbs ["get","watch","list"]
for these entries to match the other read-only providers.
---
Nitpick comments:
In `@pkg/apiserver/apiserver.go`:
- Around line 199-219: The dynamic client created by dynamic.NewForConfig (dyn)
is used by optionstorage.DefaultProviders and wired into optionstorage.NewREST,
which performs uncached, privileged list/get calls for provider-backed Option
resources; to mitigate load, wrap or replace the direct dynamic-backed providers
with a caching layer or informer-backed reads for hot sources (e.g., implement a
short-TTL in-memory cache or use informer listers for Node, KubeVirt,
StorageClass kinds) and update optionstorage.DefaultProviders to accept the
cached/provider-lister implementation instead of raw dyn so
optionstorage.NewREST uses the cached provider reads.
In `@pkg/registry/core/option/providers.go`:
- Around line 56-71: The provider functions (e.g., gpuProvider, imageProvider,
storageClassProvider, nameListProvider/nameListNamespacedProvider used in
DefaultProviders) currently call the dynamic client on every Option read; wrap
these providers with a small TTL cache or replace their internals with a shared
informer/lister to avoid repeated dynamic.List calls. Concretely, add a generic
caching wrapper (e.g., cacheProvider(providerFunc, ttl)) and use it in
DefaultProviders to return cached results for a short duration (safely guarded
by a mutex and invalidated after ttl or on explicit watch events), or
alternatively implement per-source informer/lister usage inside the specific
providers (gpuProvider, imageProvider, storageClassProvider, vmDiskProvider,
etc.) so they serve from a lister instead of re-listing on every invocation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eca96040-70be-4c0d-b1b0-2eeac5a18879
⛔ Files ignored due to path filters (1)
pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (71)
.github/workflows/pre-commit.ymlapi/api-rules/cozystack_api_violation_exceptions.listpackages/apps/bucket/values.schema.jsonpackages/apps/bucket/values.yamlpackages/apps/clickhouse/values.schema.jsonpackages/apps/clickhouse/values.yamlpackages/apps/foundationdb/values.schema.jsonpackages/apps/foundationdb/values.yamlpackages/apps/harbor/values.schema.jsonpackages/apps/harbor/values.yamlpackages/apps/http-cache/values.schema.jsonpackages/apps/http-cache/values.yamlpackages/apps/kafka/values.schema.jsonpackages/apps/kafka/values.yamlpackages/apps/kubernetes/values.schema.jsonpackages/apps/kubernetes/values.yamlpackages/apps/mariadb/values.schema.jsonpackages/apps/mariadb/values.yamlpackages/apps/mongodb/values.schema.jsonpackages/apps/mongodb/values.yamlpackages/apps/nats/values.schema.jsonpackages/apps/nats/values.yamlpackages/apps/openbao/values.schema.jsonpackages/apps/openbao/values.yamlpackages/apps/opensearch/values.schema.jsonpackages/apps/opensearch/values.yamlpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/apps/qdrant/values.schema.jsonpackages/apps/qdrant/values.yamlpackages/apps/rabbitmq/values.schema.jsonpackages/apps/rabbitmq/values.yamlpackages/apps/redis/values.schema.jsonpackages/apps/redis/values.yamlpackages/apps/vm-disk/values.schema.jsonpackages/apps/vm-disk/values.yamlpackages/apps/vm-instance/Makefilepackages/apps/vm-instance/values.schema.jsonpackages/apps/vm-instance/values.yamlpackages/system/backup-controller/definitions/backups.cozystack.io_backupjobs.yamlpackages/system/backup-controller/definitions/backups.cozystack.io_backups.yamlpackages/system/backup-controller/definitions/backups.cozystack.io_plans.yamlpackages/system/backup-controller/definitions/backups.cozystack.io_restorejobs.yamlpackages/system/bucket-rd/cozyrds/bucket.yamlpackages/system/clickhouse-rd/cozyrds/clickhouse.yamlpackages/system/cozystack-api/templates/rbac.yamlpackages/system/cozystack-basics/templates/clusterroles.yamlpackages/system/foundationdb-rd/cozyrds/foundationdb.yamlpackages/system/harbor-rd/cozyrds/harbor.yamlpackages/system/http-cache-rd/cozyrds/http-cache.yamlpackages/system/kafka-rd/cozyrds/kafka.yamlpackages/system/kubernetes-rd/cozyrds/kubernetes.yamlpackages/system/mariadb-rd/cozyrds/mariadb.yamlpackages/system/mongodb-rd/cozyrds/mongodb.yamlpackages/system/nats-rd/cozyrds/nats.yamlpackages/system/openbao-rd/cozyrds/openbao.yamlpackages/system/opensearch-rd/cozyrds/opensearch.yamlpackages/system/postgres-rd/cozyrds/postgres.yamlpackages/system/qdrant-rd/cozyrds/qdrant.yamlpackages/system/rabbitmq-rd/cozyrds/rabbitmq.yamlpackages/system/redis-rd/cozyrds/redis.yamlpackages/system/vm-disk-rd/cozyrds/vm-disk.yamlpackages/system/vm-instance-rd/cozyrds/vm-instance.yamlpkg/apis/core/fuzzer/fuzzer.gopkg/apis/core/v1alpha1/option_types.gopkg/apis/core/v1alpha1/register.gopkg/apis/core/v1alpha1/zz_generated.deepcopy.gopkg/apiserver/apiserver.gopkg/registry/core/option/providers.gopkg/registry/core/option/providers_test.gopkg/registry/core/option/rest.go
💤 Files with no reviewable changes (1)
- packages/apps/vm-instance/Makefile
| - name: Install generate | ||
| run: | | ||
| curl -sSL https://github.com/cozystack/cozyvalues-gen/releases/download/v1.5.0/cozyvalues-gen-linux-amd64.tar.gz | tar -xzvf- -C /usr/local/bin/ cozyvalues-gen | ||
| curl -sSL https://github.com/cozystack/cozyvalues-gen/releases/download/v1.6.0/cozyvalues-gen-linux-amd64.tar.gz | tar -xzvf- -C /usr/local/bin/ cozyvalues-gen |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== pre-commit workflow around line 50 ==="
if [ -f .github/workflows/pre-commit.yml ]; then
nl -ba .github/workflows/pre-commit.yml | sed -n '35,75p'
else
echo "Missing: .github/workflows/pre-commit.yml"
fi
echo
echo "=== find cozyvalues-gen in repo ==="
rg -n "cozyvalues-gen" -S . || true
echo
echo "=== inspect referenced Makefile section if present ==="
if [ -f packages/apps/vm-instance/Makefile ]; then
nl -ba packages/apps/vm-instance/Makefile | sed -n '1,60p'
else
echo "Missing: packages/apps/vm-instance/Makefile"
fi
echo
echo "=== check for checksum/install hardening patterns in workflow ==="
rg -n "sha256|gpg|cosign|sums\.txt|checksums|verify" .github/workflows/pre-commit.yml -S || trueRepository: cozystack/cozystack
Length of output: 167
🌐 Web query:
cozystack cozyvalues-gen v1.6.0 checksums.txt release asset name
💡 Result:
I can’t find any authoritative source stating that cozystack/cozyvalues-gen has a v1.6.0 release asset named checksums.txt. What I found instead: - The cozystack/cozyvalues-gen repository exists, but its latest release shown is v1.3.0 (not v1.6.0) on GitHub Releases pages. [1] - Searches for “v1.6.0” and “checksums.txt” within the cozyvalues-gen release assets did not return a matching release asset. If you meant a different repo/component (e.g., cozy/cozy-stack rather than cozyvalues-gen), or if the version tag is formatted differently (for example, v1.6.0 vs 1.6.0), tell me and I’ll re-check. [1] https://github.com/cozystack/cozyvalues-gen
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== pre-commit workflow around line 50 ==="
if [ -f .github/workflows/pre-commit.yml ]; then
awk 'NR>=35 && NR<=80 {printf "%5d:%s\n", NR, $0}' .github/workflows/pre-commit.yml
else
echo "Missing: .github/workflows/pre-commit.yml"
fi
echo
echo "=== find cozyvalues-gen in repo ==="
rg -n "cozyvalues-gen" -S . || true
echo
echo "=== check for checksum/signature verification patterns in workflow ==="
rg -n "sha256|gpg|cosign|sums\.txt|checksums|verify" .github/workflows/pre-commit.yml -S || true
echo
echo "=== search all workflows for integrity/verify patterns near cozyvalues-gen ==="
rg -n "cozyvalues-gen.*(sha256|gpg|cosign|verify|checksums|sum)" -S .github/workflows || trueRepository: cozystack/cozystack
Length of output: 8648
Verify cozyvalues-gen release artifact integrity before extracting binary.
In .github/workflows/pre-commit.yml (Install generate, line 50), CI pipes a GitHub release tarball directly into tar and installs cozyvalues-gen into /usr/local/bin with no checksum/signature verification—this is a supply-chain risk.
🔐 Suggested hardening
set -euo pipefail
VERSION="v1.6.0"
ASSET="cozyvalues-gen-linux-amd64.tar.gz"
TMPDIR="$(mktemp -d)"
curl -fsSL "https://github.com/cozystack/cozyvalues-gen/releases/download/${VERSION}/${ASSET}" -o "${TMPDIR}/${ASSET}"
# Replace with the known SHA256 for that exact asset version.
echo "${EXPECTED_SHA256} ${TMPDIR}/${ASSET}" | sha256sum -c -
tar -xzf "${TMPDIR}/${ASSET}" -C /usr/local/bin🤖 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 @.github/workflows/pre-commit.yml at line 50, The workflow currently pipes
the release tarball directly into tar, which skips integrity checks; instead
modify the Install generate step to download the release to a temporary file
(use variables like VERSION and ASSET), compute/verify its SHA256 against a
pinned EXPECTED_SHA256 (use TMPDIR for the temp location), and only after a
successful checksum run tar to extract cozyvalues-gen into /usr/local/bin;
ensure the step uses strict shell options (set -euo pipefail) and fails the job
if the checksum does not match.
Comment on lines
+27
to
+51
| # Read-only sources for the Option (dropdown) resource providers. | ||
| - apiGroups: [""] | ||
| resources: ["nodes", "persistentvolumeclaims"] | ||
| verbs: ["get", "watch", "list"] | ||
| - apiGroups: ["kubevirt.io"] | ||
| resources: ["kubevirts"] | ||
| verbs: ["get", "watch", "list"] | ||
| - apiGroups: ["instancetype.kubevirt.io"] | ||
| resources: ["virtualmachineclusterinstancetypes", "virtualmachineclusterpreferences"] | ||
| verbs: ["get", "watch", "list"] | ||
| - apiGroups: ["k8s.cni.cncf.io"] | ||
| resources: ["network-attachment-definitions"] | ||
| verbs: ["get", "watch", "list"] | ||
| - apiGroups: ["objectstorage.k8s.io"] | ||
| resources: ["bucketclasses"] | ||
| verbs: ["get", "watch", "list"] | ||
| - apiGroups: ["storage.k8s.io"] | ||
| resources: ["storageclasses"] | ||
| verbs: ["get", "watch", "list"] | ||
| - apiGroups: ["backups.cozystack.io"] | ||
| resources: ["backupclasses"] | ||
| verbs: ["get", "watch", "list"] | ||
| - apiGroups: ["apps.cozystack.io"] | ||
| resources: ["vmdisks"] | ||
| verbs: ["get", "watch", "list"] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the option providers and extract the GVRs they read, to compare with RBAC rules.
fd providers.go | rg 'core/option' || fd providers.go
f=$(fd providers.go | rg 'core/option' | head -n1)
echo "=== provider file: $f ==="
# Show GroupVersionResource definitions / Group/Resource literals
rg -nP '(GroupVersionResource|Group|Resource|Version)\s*:\s*"' "$f"
# Show any group/resource string constants used for dynamic Resource() calls
rg -nP 'schema\.GroupVersionResource|\.Resource\(' "$f" -A2 -B2Repository: cozystack/cozystack
Length of output: 5984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/system/cozystack-api/templates/rbac.yaml"
echo "=== File: $file (lines 1-120) ==="
nl -ba "$file" | sed -n '1,120p'
echo
echo "=== Search RBAC for option provider GVR resources ==="
rg -n "plans|backups\b|applicationdefinitions|backupclasses|helmreleases|vmdisks|gvr" "$file" || true
echo
echo "=== Show any rules block that might cover dynamic option providers (around lines 20-70) ==="
nl -ba "$file" | sed -n '20,90p'Repository: cozystack/cozystack
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/system/cozystack-api/templates/rbac.yaml"
echo "=== wc -l $file ==="
wc -l "$file"
echo
echo "=== Show $file line 1-160 with line numbers ==="
awk '{printf "%6d %s\n", NR, $0}' "$file" | sed -n '1,160p'
echo
echo "=== Search RBAC template for option-provider related resources ==="
rg -n --fixed-strings "plans" "$file" || true
rg -n --fixed-strings "backups" "$file" || true
rg -n --fixed-strings "applicationdefinitions" "$file" || true
rg -n --fixed-strings "backupclasses" "$file" || true
rg -n --fixed-strings "vmdisks" "$file" || true
rg -n "helmreleases|nodes|persistentvolumeclaims|kubevirts|virtualmachineclusterinstancetypes|virtualmachineclusterpreferences|network-attachment-definitions|storageclasses" "$file" || trueRepository: cozystack/cozystack
Length of output: 2914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/registry/core/option/providers.go"
echo "=== Relevant GVR definitions ==="
sed -n '30,70p' "$file" | nl -ba
echo
echo "=== Where dyn.Resource(...).List is called for plans/backups/app defs/helm releases ==="
rg -n "gvrPlans|gvrBackups|gvrAppDefs|gvrHelmReleases" "$file"
echo
echo "=== Show the provider functions around those usages ==="
for line in $(rg -n "gvrPlans|gvrBackups|gvrAppDefs|gvrHelmReleases" "$file" | cut -d: -f1 | sort -n | uniq); do
start=$((line-15)); end=$((line+25));
echo "---- $file:$start-$end (around line $line) ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {print NR ":" $0}' "$file" | sed -n '1,120p'
doneRepository: cozystack/cozystack
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate providers.go ==="
fd -a "providers.go" -t f . | head -n 20
echo
echo "=== Use pkg/registry/core/option/providers.go if it exists ==="
file="pkg/registry/core/option/providers.go"
if [ -f "$file" ]; then
echo "Found: $file"
else
echo "Missing: $file"
exit 1
fi
echo
echo "=== Relevant GVR definitions ==="
awk 'NR>=30 && NR<=70 {print NR ":" $0}' "$file"
echo
echo "=== Where dyn.Resource(...).List is called for plans/backups/app defs/helm releases ==="
rg -n "gvrPlans|gvrBackups|gvrAppDefs|gvrHelmReleases" "$file"
echo
echo "=== Show provider functions around those usages ==="
lines=$(rg -n "gvrPlans|gvrBackups|gvrAppDefs|gvrHelmReleases" "$file" | cut -d: -f1 | sort -n | uniq)
for line in $lines; do
start=$((line-20)); end=$((line+20));
echo
echo "---- $file:$start-$end (around line $line) ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {print NR ":" $0}' "$file" | sed -n '1,180p'
doneRepository: cozystack/cozystack
Length of output: 23766
Fix RBAC read permissions for Option dropdown providers (missing GVRs).
packages/system/cozystack-api/templates/rbac.yamlgrantsbackups.cozystack.ioread-only access only forbackupclasses, butpkg/registry/core/option/providers.goalso listsbackups.cozystack.io/v1alpha1plansandbackupsfor theplanandbackupoption providers.- The same RBAC block grants
apps.cozystack.ioread-only access only forvmdisks, but theappkindoption provider listsapps.cozystack.io/v1alpha1applicationdefinitions.
🤖 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 `@packages/system/cozystack-api/templates/rbac.yaml` around lines 27 - 51, The
RBAC block in templates/rbac.yaml misses some read-only GVRs used by the Option
dropdown providers: add "plans" and "backups" to the apiGroup
"backups.cozystack.io" and add "applicationdefinitions" to the apiGroup
"apps.cozystack.io" so the providers referenced in
pkg/registry/core/option/providers.go (the plan, backup and appkind option
providers) can list/get/watch those resources; keep verbs ["get","watch","list"]
for these entries to match the other read-only providers.
The roundtrip test for the core API group panicked (Filler.Funcs: customFuncs' second argument must be a randfill.Continue) because the fuzzer still used github.com/google/gofuzz while apimachinery switched to sigs.k8s.io/randfill. Port the fuzz functions to randfill.Continue / FillNoCustom. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Introduce a read-only, namespaced virtual resource core.cozystack.io/v1alpha1 Option that serves named lists of dropdown options for the dashboard. Each object (one per source: gpu, instancetype, instanceprofile, network, image, storagepool, storageclass, vmdisk, backupclass) is computed on read by an in-process provider using a privileged dynamic client, so tenants need no direct access to Nodes, the KubeVirt CR, cluster instancetypes, etc. Tenants get read access via the aggregated cozy:tenant roles; the apiserver ServiceAccount gains read on the underlying sources. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Mark fields that reference cluster resources with the x-cozystack-options vendor keyword so the dashboard renders them as dynamic dropdowns: GPU, instanceType, instanceProfile, network (vm-instance), GPU/ instanceType (kubernetes nodeGroups), image (vm-disk), storagePool (bucket), storageClass (all apps) and disks[].name (vm-instance). Drop the stale static instanceProfile enum. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Add three dropdown sources to the Option provider registry so the dashboard's backup forms can use the unified x-cozystack-options mechanism instead of injecting client-side enum arrays: - plan / backup: namespaced, list Plans / Backups in the tenant namespace - appkind: cluster-scoped, lists application kinds from ApplicationDefinitions Annotate the backups.cozystack.io CRD schemas (Plan, BackupJob, Backup, RestoreJob) with x-cozystack-options on backupClassName (backupclass), applicationRef.kind / targetApplicationRef.kind (appkind), planRef.name (plan) and backupRef.name (backup). This also gives the previously unused backupclass source its first consumer. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Move the dynamic-dropdown x-cozystack-options vendor extensions from the generated values.schema.json into the values.yaml source for vm-instance, kubernetes, vm-disk and bucket so they survive schema regeneration. The new cozyvalues-gen @x-<keyword> directive emits these extensions from the source annotations. Annotations on typedef @field entries (Disk, Network, GPU, NodeGroup) are declared once and cover every place the typedef is reused. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
…es.yaml Add the storageclass dynamic-dropdown x-cozystack-options annotation to the storageClass field of the data apps (openbao, clickhouse, mariadb, redis, harbor, opensearch, rabbitmq, mongodb, postgres, nats, kafka, qdrant, foundationdb, http-cache) at the values.yaml source, so the extension is emitted by cozyvalues-gen instead of hand-edited into values.schema.json. kafka carries two (Kafka and ZooKeeper structs); foundationdb is nested under the Storage struct. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
The instanceProfile field is now driven by an x-cozystack-options dynamic dropdown (source: instanceprofile), so the Makefile generate step no longer needs to post-inject a static enum into values.schema.json via yq. Remove that injection along with the already-commented-out instanceType enum injection so the dynamic dropdown is not fought by a stale static enum. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Options were granted on cozy:tenant:use:base, leaving view-only tenant users (cozy:tenant:view) without read access — so their create/edit forms could not populate dynamic dropdowns. Move the grant down to cozy:tenant:view:base, which aggregates up into use/admin/super-admin, covering every human tenant access level. The tenant ServiceAccount role keeps its grant via cozy:tenant:base. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
…ystack-options Regenerate values.schema.json and embedded openAPISchema in the -rd ApplicationDefinitions so the x-cozystack-options dynamic-source directives declared in each app values.yaml are carried into the generated artifacts. Brings the -rd schemas in sync with the hand-edited values.schema.json files (CI runs make generate + git diff --exit-code). Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
… group The appkind option provider queried applicationdefinitions in the apps.cozystack.io group, but the ApplicationDefinition CRD is served from cozystack.io. The provider listed a resource that does not exist, so the appkind source resolved empty on every cluster (tolerated and dropped by List, 500 on Get), and the backup form fields that reference it never populated. Point the provider at cozystack.io, which the existing cozystack.io read grant already covers. Add a guard test pinning the CRD group, since the existing fake-client test is built from the same GVR and cannot catch a wrong group. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The plan and backup option providers list namespaced plans and backups in the backups.cozystack.io group, but the cozystack-api ClusterRole only granted backupclasses there. The apiserver's privileged client runs as this ServiceAccount, so both providers got forbidden — List silently dropped the sources and Get returned 500, leaving the plan/backup dropdowns in the backup forms empty. Add plans and backups to the backups.cozystack.io rule and cover the grant with a chart test. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei)
force-pushed
the
feat/dynamic-options-dropdowns
branch
from
June 2, 2026 13:44
eb275f1 to
4cbe5b6
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/system/cozystack-basics/templates/clusterroles.yaml (1)
42-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
optionsout ofcozy:tenant:base.This widens a privileged virtual resource from view-role users to every tenant ServiceAccount. The same read rule is already added in
cozy:tenant:view:baseat Lines 123-127, which matches the stated dashboard dropdown use case without exposing option listings to workloads.Suggested RBAC trim
- apiGroups: - core.cozystack.io resources: - tenantmodules - tenantsecrets - - options verbs: ["get", "list", "watch"]🤖 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 `@packages/system/cozystack-basics/templates/clusterroles.yaml` around lines 42 - 48, Remove the "options" resource from the cozy:tenant:base ClusterRole's rules so tenant workloads don't get read access; locate the cluster role definition that lists apiGroups: core.cozystack.io with resources tenantmodules, tenantsecrets, options and delete "options" from that resources array, leaving the verbs unchanged, while leaving the existing "options" read rule in the cozy:tenant:view:base ClusterRole intact (it already grants the dashboard view use case).
🤖 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.
Outside diff comments:
In `@packages/system/cozystack-basics/templates/clusterroles.yaml`:
- Around line 42-48: Remove the "options" resource from the cozy:tenant:base
ClusterRole's rules so tenant workloads don't get read access; locate the
cluster role definition that lists apiGroups: core.cozystack.io with resources
tenantmodules, tenantsecrets, options and delete "options" from that resources
array, leaving the verbs unchanged, while leaving the existing "options" read
rule in the cozy:tenant:view:base ClusterRole intact (it already grants the
dashboard view use case).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c01a5fcb-f98d-43d3-abc9-62d58ae57b43
⛔ Files ignored due to path filters (1)
pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (71)
api/api-rules/cozystack_api_violation_exceptions.listpackages/apps/bucket/values.schema.jsonpackages/apps/bucket/values.yamlpackages/apps/clickhouse/values.schema.jsonpackages/apps/clickhouse/values.yamlpackages/apps/foundationdb/values.schema.jsonpackages/apps/foundationdb/values.yamlpackages/apps/harbor/values.schema.jsonpackages/apps/harbor/values.yamlpackages/apps/http-cache/values.schema.jsonpackages/apps/http-cache/values.yamlpackages/apps/kafka/values.schema.jsonpackages/apps/kafka/values.yamlpackages/apps/kubernetes/values.schema.jsonpackages/apps/kubernetes/values.yamlpackages/apps/mariadb/values.schema.jsonpackages/apps/mariadb/values.yamlpackages/apps/mongodb/values.schema.jsonpackages/apps/mongodb/values.yamlpackages/apps/nats/values.schema.jsonpackages/apps/nats/values.yamlpackages/apps/openbao/values.schema.jsonpackages/apps/openbao/values.yamlpackages/apps/opensearch/values.schema.jsonpackages/apps/opensearch/values.yamlpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/apps/qdrant/values.schema.jsonpackages/apps/qdrant/values.yamlpackages/apps/rabbitmq/values.schema.jsonpackages/apps/rabbitmq/values.yamlpackages/apps/redis/values.schema.jsonpackages/apps/redis/values.yamlpackages/apps/vm-disk/values.schema.jsonpackages/apps/vm-disk/values.yamlpackages/apps/vm-instance/Makefilepackages/apps/vm-instance/values.schema.jsonpackages/apps/vm-instance/values.yamlpackages/system/backup-controller/definitions/backups.cozystack.io_backupjobs.yamlpackages/system/backup-controller/definitions/backups.cozystack.io_backups.yamlpackages/system/backup-controller/definitions/backups.cozystack.io_plans.yamlpackages/system/backup-controller/definitions/backups.cozystack.io_restorejobs.yamlpackages/system/bucket-rd/cozyrds/bucket.yamlpackages/system/clickhouse-rd/cozyrds/clickhouse.yamlpackages/system/cozystack-api/templates/rbac.yamlpackages/system/cozystack-api/tests/rbac_test.yamlpackages/system/cozystack-basics/templates/clusterroles.yamlpackages/system/foundationdb-rd/cozyrds/foundationdb.yamlpackages/system/harbor-rd/cozyrds/harbor.yamlpackages/system/http-cache-rd/cozyrds/http-cache.yamlpackages/system/kafka-rd/cozyrds/kafka.yamlpackages/system/kubernetes-rd/cozyrds/kubernetes.yamlpackages/system/mariadb-rd/cozyrds/mariadb.yamlpackages/system/mongodb-rd/cozyrds/mongodb.yamlpackages/system/nats-rd/cozyrds/nats.yamlpackages/system/openbao-rd/cozyrds/openbao.yamlpackages/system/opensearch-rd/cozyrds/opensearch.yamlpackages/system/postgres-rd/cozyrds/postgres.yamlpackages/system/qdrant-rd/cozyrds/qdrant.yamlpackages/system/rabbitmq-rd/cozyrds/rabbitmq.yamlpackages/system/redis-rd/cozyrds/redis.yamlpackages/system/vm-disk-rd/cozyrds/vm-disk.yamlpackages/system/vm-instance-rd/cozyrds/vm-instance.yamlpkg/apis/core/fuzzer/fuzzer.gopkg/apis/core/v1alpha1/option_types.gopkg/apis/core/v1alpha1/register.gopkg/apis/core/v1alpha1/zz_generated.deepcopy.gopkg/apiserver/apiserver.gopkg/registry/core/option/providers.gopkg/registry/core/option/providers_test.gopkg/registry/core/option/rest.go
💤 Files with no reviewable changes (1)
- packages/apps/vm-instance/Makefile
✅ Files skipped from review due to trivial changes (21)
- packages/apps/http-cache/values.yaml
- packages/system/harbor-rd/cozyrds/harbor.yaml
- packages/apps/qdrant/values.yaml
- packages/apps/kafka/values.yaml
- packages/apps/rabbitmq/values.yaml
- packages/system/kafka-rd/cozyrds/kafka.yaml
- packages/apps/postgres/values.yaml
- packages/apps/opensearch/values.yaml
- packages/apps/openbao/values.yaml
- packages/apps/mariadb/values.yaml
- packages/apps/redis/values.yaml
- packages/apps/qdrant/values.schema.json
- packages/apps/bucket/values.yaml
- packages/apps/nats/values.yaml
- packages/apps/foundationdb/values.yaml
- packages/system/backup-controller/definitions/backups.cozystack.io_plans.yaml
- packages/apps/mongodb/values.schema.json
- packages/apps/opensearch/values.schema.json
- packages/apps/kubernetes/values.schema.json
- pkg/apis/core/v1alpha1/zz_generated.deepcopy.go
- packages/apps/vm-disk/values.yaml
🚧 Files skipped from review as they are similar to previous changes (43)
- packages/apps/clickhouse/values.yaml
- packages/apps/mongodb/values.yaml
- packages/apps/postgres/values.schema.json
- packages/apps/clickhouse/values.schema.json
- packages/apps/mariadb/values.schema.json
- api/api-rules/cozystack_api_violation_exceptions.list
- packages/system/postgres-rd/cozyrds/postgres.yaml
- packages/apps/rabbitmq/values.schema.json
- packages/system/qdrant-rd/cozyrds/qdrant.yaml
- packages/system/rabbitmq-rd/cozyrds/rabbitmq.yaml
- packages/apps/http-cache/values.schema.json
- packages/system/mongodb-rd/cozyrds/mongodb.yaml
- packages/system/nats-rd/cozyrds/nats.yaml
- packages/apps/vm-disk/values.schema.json
- packages/system/mariadb-rd/cozyrds/mariadb.yaml
- packages/apps/bucket/values.schema.json
- packages/system/vm-disk-rd/cozyrds/vm-disk.yaml
- packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml
- packages/system/backup-controller/definitions/backups.cozystack.io_backupjobs.yaml
- packages/system/foundationdb-rd/cozyrds/foundationdb.yaml
- packages/system/redis-rd/cozyrds/redis.yaml
- packages/apps/nats/values.schema.json
- packages/apps/kafka/values.schema.json
- pkg/apiserver/apiserver.go
- packages/apps/openbao/values.schema.json
- pkg/apis/core/v1alpha1/register.go
- packages/system/clickhouse-rd/cozyrds/clickhouse.yaml
- pkg/apis/core/v1alpha1/option_types.go
- packages/apps/harbor/values.yaml
- packages/system/kubernetes-rd/cozyrds/kubernetes.yaml
- packages/system/openbao-rd/cozyrds/openbao.yaml
- packages/system/opensearch-rd/cozyrds/opensearch.yaml
- packages/apps/vm-instance/values.yaml
- packages/system/cozystack-api/tests/rbac_test.yaml
- pkg/registry/core/option/providers_test.go
- packages/system/vm-instance-rd/cozyrds/vm-instance.yaml
- packages/system/backup-controller/definitions/backups.cozystack.io_restorejobs.yaml
- packages/system/cozystack-api/templates/rbac.yaml
- packages/apps/kubernetes/values.yaml
- packages/apps/foundationdb/values.schema.json
- pkg/registry/core/option/rest.go
- packages/apps/redis/values.schema.json
- pkg/registry/core/option/providers.go
The Option providers RBAC block granted read on objectstorage.k8s.io/bucketclasses, but no provider in the registry lists BucketClasses and no schema field references a bucketclass source — the bucket app's storage pool field uses the storagepool provider, which reads seaweedfs HelmReleases. Drop the unused grant and pin its absence in the chart test. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…n provider error The Option REST deliberately makes List tolerate a single failing provider (it logs and skips it, so a partial RBAC config drops one dropdown instead of breaking the whole list) while Get surfaces the error as a 500. Neither path was covered. Add tests that inject a failing provider and assert List keeps the healthy sources and omits the failing one, and that Get returns an InternalError. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…ntroller-gen controller-gen cannot emit the x-cozystack-options vendor extension, so the annotations on the backup CRDs (backupjobs/backups/plans/restorejobs) were hand-edited into generated output — the next make generate would strip them, breaking the appkind/plan/backup form dropdowns and failing the codegen-drift check. Add a deterministic awk pass that re-applies the extension after controller-gen, driven by per-CRD field targets. It inserts lines without reserializing the YAML, so it reproduces the committed CRDs byte-for-byte and is independent of the host yq version. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
fuzzer.go imports sigs.k8s.io/randfill directly after the gofuzz-to-randfill migration, but go.mod still listed it under the indirect block. go mod tidy moves it into the direct require block; commit the result so the metadata matches the import and a contributor running go mod tidy gets no diff. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…test passes
The core fuzzer was migrated from gofuzz to randfill, but the apps fuzzer was
left on gofuzz. TestRoundTripTypes runs both fuzzers (apps first), so
registering a gofuzz.Continue func against the randfill-based roundtrip harness
panics ("customFuncs' second argument must be a randfill.Continue").
Migrate the apps fuzzer the same way (randfill.Continue / FillNoCustom). With
no gofuzz consumers left, go mod tidy drops the dependency. TestRoundTripTypes
now passes.
Assisted-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
The gpu, image, storagepool, storageclass and vmdisk providers carry the real parsing logic (whitelist-intersect-allocatable, prefix strip, nested pool walk, default-class marking, size label) but had no tests — and List tolerates a provider error by skipping it, so a logic bug ships as a silently empty or wrong dropdown rather than a failure. Add table tests against the fake dynamic client for each, raising option-package coverage from 39% to 77%. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…tions The tenant view aggregation grants read on the core.cozystack.io/options virtual resource so dashboard dropdowns can be populated. Add a helm-unittest on the cozy:tenant:view:base ClusterRole so a silent RBAC regression on the tenant side is caught, mirroring the apiserver-side rbac test. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The one-shot option watch emitted the current state once then blocked on ctx.Done(). If the initial List failed (e.g. every provider forbidden by a broken RBAC config) it blocked silently, leaving the client with an empty stream indistinguishable from "no options". Log the error so an operator can tell the two apart. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
instancetype/instanceprofile replaced the static vm-instance enum with live dropdowns sourced from instancetype.kubevirt.io. A wrong group/version would make those dropdowns silently empty in every cluster (List tolerates the error) — the failure mode the appkind GVR guard already catches. Pin both GVRs to the served API. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/registry/core/option/providers_test.go (1)
195-200:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert NotFound for unknown option source
The test only fails onerr == nil, so a regression returning a different error type would slip through; checkapierrors.IsNotFound(err)instead.Suggested tightening
func TestRESTGetUnknownSourceNotFound(t *testing.T) { dyn := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), listKinds()) r := NewREST(DefaultProviders(dyn)) if _, err := r.Get(context.Background(), "does-not-exist", &metav1.GetOptions{}); err == nil { t.Fatal("expected NotFound for unknown source") + } else if !apierrors.IsNotFound(err) { + t.Fatalf("expected NotFound for unknown source, got %v", err) } }🤖 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 `@pkg/registry/core/option/providers_test.go` around lines 195 - 200, The test TestRESTGetUnknownSourceNotFound should verify the error is a NotFound error instead of only checking for non-nil; update the assertion after calling r.Get(...) to use apierrors.IsNotFound(err) (importing k8s.io/apimachinery/pkg/api/errors as apierrors) so the test fails if a different error type is returned; keep the rest of the test and the same setup using NewREST and DefaultProviders(dyn).
🤖 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 `@pkg/registry/core/option/providers_test.go`:
- Around line 355-357: The test currently calls
DefaultProviders(dyn)["storagepool"](context.Background(), "") and only asserts
len(empty)==0 while ignoring the returned error; update the assertion to also
capture and assert that the error is nil so failures where the provider returns
an unexpected error are caught. Locate the call to DefaultProviders and the
"storagepool" provider invocation in providers_test.go and change the test to
check both the slice length is zero and that the error value returned is nil
(i.e., fail the test if err != nil).
---
Outside diff comments:
In `@pkg/registry/core/option/providers_test.go`:
- Around line 195-200: The test TestRESTGetUnknownSourceNotFound should verify
the error is a NotFound error instead of only checking for non-nil; update the
assertion after calling r.Get(...) to use apierrors.IsNotFound(err) (importing
k8s.io/apimachinery/pkg/api/errors as apierrors) so the test fails if a
different error type is returned; keep the rest of the test and the same setup
using NewREST and DefaultProviders(dyn).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec94e353-2f5d-43d9-9add-f5b22c599663
📒 Files selected for processing (9)
go.modhack/inject-cozystack-options.awkhack/update-codegen.shpackages/system/cozystack-api/templates/rbac.yamlpackages/system/cozystack-api/tests/rbac_test.yamlpackages/system/cozystack-basics/tests/clusterroles-options_test.yamlpkg/apis/apps/fuzzer/fuzzer.gopkg/registry/core/option/providers_test.gopkg/registry/core/option/rest.go
💤 Files with no reviewable changes (1)
- packages/system/cozystack-api/templates/rbac.yaml
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/system/cozystack-api/tests/rbac_test.yaml
- pkg/registry/core/option/rest.go
Comment on lines
+355
to
+357
| // Namespaced provider opts out without a namespace. | ||
| if empty, _ := DefaultProviders(dyn)["storagepool"](context.Background(), ""); len(empty) != 0 { | ||
| t.Fatalf("storagepool: expected no items without a namespace") |
Contributor
There was a problem hiding this comment.
Don’t ignore the error on the empty-namespace branch.
If storagepool starts returning an error instead of cleanly opting out, this assertion still passes because it only checks len(empty). That weakens the contract this test is trying to pin.
Suggested tightening
// Namespaced provider opts out without a namespace.
- if empty, _ := DefaultProviders(dyn)["storagepool"](context.Background(), ""); len(empty) != 0 {
+ empty, err := DefaultProviders(dyn)["storagepool"](context.Background(), "")
+ if err != nil {
+ t.Fatalf("storagepool provider empty ns: %v", err)
+ }
+ if len(empty) != 0 {
t.Fatalf("storagepool: expected no items without a namespace")
}📝 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
| // Namespaced provider opts out without a namespace. | |
| if empty, _ := DefaultProviders(dyn)["storagepool"](context.Background(), ""); len(empty) != 0 { | |
| t.Fatalf("storagepool: expected no items without a namespace") | |
| // Namespaced provider opts out without a namespace. | |
| empty, err := DefaultProviders(dyn)["storagepool"](context.Background(), "") | |
| if err != nil { | |
| t.Fatalf("storagepool provider empty ns: %v", err) | |
| } | |
| if len(empty) != 0 { | |
| t.Fatalf("storagepool: expected no items without a namespace") | |
| } |
🤖 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 `@pkg/registry/core/option/providers_test.go` around lines 355 - 357, The test
currently calls DefaultProviders(dyn)["storagepool"](context.Background(), "")
and only asserts len(empty)==0 while ignoring the returned error; update the
assertion to also capture and assert that the error is nil so failures where the
provider returns an unexpected error are caught. Locate the call to
DefaultProviders and the "storagepool" provider invocation in providers_test.go
and change the test to check both the slice length is zero and that the error
value returned is nil (i.e., fail the test if err != nil).
Aleksei Sviridkin (lexfrei)
approved these changes
Jun 2, 2026
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Read-only Option resource (core.cozystack.io) computing dropdown items from live cluster state via privileged apiserver-side providers, plus the x-cozystack-options schema keyword carried from app values into the served OpenAPI. The design keeps tenants off direct reads on Nodes/KubeVirt/instancetypes while scoping namespaced sources to the request namespace — no cross-tenant leak on an empty-namespace list.
Verified: build/vet/lint clean; provider and REST logic are tested (whitelist-intersect-allocatable, prefix strip, default-class marking, pool/zone walk, namespaced scoping, List-tolerate vs Get-500); RBAC grants match every provider GVR; the generation chain reproduces (codegen-drift green), including the backup-CRD x-cozystack-options re-injection.
Fixes I pushed on top:
- appkind provider queried the wrong API group (apps.cozystack.io -> cozystack.io) — it was silently empty in every cluster.
- RBAC was missing plans/backups in backups.cozystack.io — added.
- removed an unused objectstorage/bucketclasses grant.
- backup-CRD x-cozystack-options were hand-edited into generated files; added a deterministic codegen step so
make generatereproduces them (codegen-drift would otherwise fail). - finished the gofuzz->randfill migration (the apps fuzzer was left behind and panicked the roundtrip test); marked randfill as a direct dependency.
- added provider/REST test coverage and GVR guard tests.
Non-blocking: the PR body's "bumps cozyvalues-gen to v1.6.0" line is stale — that bump already landed on main, there is no version diff here; and the option watch logs an all-providers-failed case at V(2), worth raising to a warning so a total RBAC failure is not quiet.
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.
What this PR does
Adds a generic mechanism for runtime-populated dropdowns in dashboard create/edit forms. Many fields are references to live cluster resources (GPU device names, KubeVirt instancetypes/preferences, Multus networks, VM images, bucket storage pools, storage classes, backup classes/plans) but were free-text or static enums that drift from reality.
Optionresource —core.cozystack.io/v1alpha1, namespaced, read-only (list/get/watch). Items are computed on read by an in-process provider registry using a privileged client, so tenants get curated option lists without broad cluster reads. Providers:gpu,instancetype,instanceprofile,network,image,storagepool,storageclass, and backup-relatedbackupclass,plan,backup,appkind.x-cozystack-optionsschema keyword — appvalues.yamldeclare it via the new cozyvalues-gen@x-<keyword>directive (e.g.## @x-cozystack-options {source: instancetype}). Schemas, Go types, and the-rdApplicationDefinitions are regenerated from source so the extension is carried into the served OpenAPI. The dashboard renders annotated fields as dropdowns sourced from the live cluster (UI widget ships in a separate cozystack-ui PR).optionsin their namespace via the aggregated tenant ClusterRoles (granted atcozy:tenant:view:base, so every access level from view upward can populate dropdowns).instanceProfileenum injection from vm-instance's Makefile (now driven dynamically).Dependency on cozyvalues-gen
The
x-cozystack-optionsannotations require the new vendor-extension directive from cozystack/cozyvalues-gen#27. This PR bumps the pinned cozyvalues-gen to v1.6.0. A cozyvalues-gen v1.6.0 release must be cut before CI can pass — until then the pre-commit schema-generation job cannot download the binary and will fail.Screenshots
N/A — no UI changes in this repository. The consuming dashboard widget ships in a separate cozystack-ui PR.
Release note
Summary by CodeRabbit
New Features
Chores
Tests