Repository navigation
fix(api): leave failed golden images out of the image options - #4568
Conversation
The image option source lists every vm-default-images-* PVC in cozy-public. CDI creates that PVC as soon as an import starts, so a golden whose DataVolume ended in phase Failed is still offered, and a disk cloned from it never populates: the user picks an image the platform just suggested and ends up with a disk that never becomes usable. Read the DataVolumes next to the PVCs and drop the names whose import Failed. Filtering beats marking: the option can never be used, and OptionItem has no disabled state to show it in. A golden still importing stays, since CDI holds a clone until the source is populated, and so does a PVC with no DataVolume. When the DataVolumes cannot be read, without CDI or without the grant, the PVCs go out unfiltered: the Option list drops a source whose provider errors, and an empty picker would be a regression rather than a safeguard. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The vm-disk source picker listed vm-default-images-* PVCs in cozy-public directly, as the tenant, so it kept offering goldens whose import failed even once the image option source leaves them out. The Option list is what the other dynamic dropdowns already read, and it shares their request. With the console no longer listing those PVCs, the tenant dashboard role's read on cozy-public PVCs has no user left, so it goes too. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/api
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe image provider now excludes golden-image PVCs when their matching DataVolume has phase ChangesImage picker options
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The image picker now shows images from the tenant options source and hides images whose import failed. The change is small and tested, and no merge-blocking risk was identified. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The change reduces direct dashboard access to shared storage, but grants the API broader read access than its image picker needs. The picker also returns to its previous, unfiltered choices if import status cannot be read. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 5 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM
A clean, well-scoped fix. There are three ways a filter like this usually goes wrong, and I checked each one against the code.
The "failed" decision reads status.phase on the DataVolume and compares it to Failed (pkg/registry/core/option/providers.go:357). That is CDI's own phase field, not a status.conditions[] near-twin and not a frozen lastTransitionTime, so it follows the live import. Only Failed drops. An image still importing (ImportInProgress), one that succeeded, one still pending, or a PVC with no DataVolume at all, all stay in the list, and the tests name the ImportInProgress and no-DataVolume cases. The filter keys on the DataVolume name and matches it against the PVC name, which is sound because a golden's DataVolume and its target PVC in cozy-public carry the same name vm-default-images-<name> (packages/system/vm-default-images/templates/dv.yaml, usePopulator: "true"). Nothing drops a healthy image off by one.
I took this to the code rather than the PR description. Reverting the || failed[name] term reddens TestImageProviderSkipsGoldensWhoseImportFailed, so the test holds the behaviour instead of passing beside it, and go build / go test on the package are green. The RBAC count bound (20 to 21) and the dashboard-role flip from contains to notContains ... any: true track the diff, and the console now reads the same image Option the other dropdowns use, so the server-side filter actually reaches the picker.
Two notes inline, neither blocking.
| verbs: ["get", "watch", "list"] | ||
| # The image picker drops golden images whose import Failed. | ||
| - apiGroups: ["cdi.kubevirt.io"] | ||
| resources: ["datavolumes"] |
There was a problem hiding this comment.
[MINOR] cdi.kubevirt.io/datavolumes read grant is cluster-wide while the provider only reads cozy-public
This grant is cluster-wide, but imageProvider only ever lists DataVolumes in cozy-public (pkg/registry/core/option/providers.go:351). It is read-only and never reaches a tenant, and it matches the other cluster-scoped read grants on this ClusterRole (nodes, kubevirts, instancetypes), so I am not blocking on it. If no other API path needs the wider read, a namespaced Role on cozy-public would be tighter. Read-only cross-namespace exposure only, no tenant-facing path.
There was a problem hiding this comment.
Leaving this cluster-wide. The same ClusterRole already lets cozystack-api get and list every Secret in the cluster, so DataVolume read adds nothing it can not already see. A Role in cozy-public can not ship from this chart either: cozystack-basics creates that namespace and dependsOn cozystack-engine, so on a fresh install the Role would hit a missing namespace.
| // imageProvider lists the default image PVCs in cozy-public and strips the | ||
| // vm-default-images- prefix to get the catalog name used by the vm-disk chart. | ||
| // | ||
| // A golden whose DataVolume is in phase Failed is left out: CDI never |
There was a problem hiding this comment.
[NIT] the Failed filter covers less than #4553 assumed (documented, tracked in #4564)
Worth keeping visible next to the code: in CDI v1.66.1 a golden reaches Failed only on a checksum mismatch, while a broken source URL stays in ImportInProgress and is therefore never filtered out of the picker. This is called out in the PR body and tracked in #4564, so it is an acknowledged scope limit rather than a defect in this change.
There was a problem hiding this comment.
Yes, it stays open in #4564 until there is a better signal than Failed.
2 participants
What this PR does
The VM image picker no longer offers golden images whose import failed. A golden keeps its PVC in
cozy-publicafter the import fails, so a picker built on the PVC list offered an image whose clone never completes.The
imageoption source in cozystack-api now reads thevm-default-images-*DataVolumes next to the PVCs and drops names whose DataVolume is in phaseFailed. I filter rather than mark them, because such an option can never be used andOptionItemhas no disabled state. A golden that is still importing stays in the list, since CDI holds a clone until the source is populated, and so does a PVC with no DataVolume. When the DataVolumes cannot be read, the PVCs go out unfiltered. The Option list drops a source whose provider errors, and an empty picker would be worse than the old behaviour. cozystack-api gets a read grant oncdi.kubevirt.ioDataVolumes for this.The console did not use that source at all.
SourceFieldlisted the PVCs itself, as the tenant, so the fix above would not reach the picker users see. It now reads the same Option list the other dynamic dropdowns use. With that, nothing in the console listscozy-publicPVCs, so I removed the read grant on them from thecozy:tenant:dashboardRole. That is a visible change: a tenant loseskubectl get pvc --namespace cozy-public, which was only ever granted for this picker in #2843.One limitation: in CDI v1.66.1, which cozystack ships, a golden import reaches
Failedonly on a checksum mismatch, and a broken URL keeps it inImportInProgress. So this filter covers less than the issue assumed. I filed #4564 for that.Closes #4553
Screenshots
I have no screenshot. The picker looks the same; only the list behind it changes.
apps/console/src/components/SourceField.test.tsxcovers it: the picker shows the images the option source serves and not the raw PVC names.Downstream repositories
Release note
Summary by CodeRabbit