Repository navigation
Tap privileged-confirmation gate races source-watcher on a benign-to-privileged flip #4359
Description
Summary
The privileged-confirmation gate for tap auto-registrations is a content-derived decision (Install.Privileged in the PackageSource spec), enforced in the Package reconciler (skip the HelmRelease, prune a lingering one) and the PackageSource reconciler (withhold the privileged component's ExternalArtifact). Both derive the flag from the source revision's content, which source-watcher's ArtifactGenerator materializes from the same OCIRepository revision, independently. Nothing serializes "cozystack observes the flip and withholds" against "source-watcher materializes the new revision's content."
Impact
A component that was benign (already installed: HelmRelease + ExternalArtifact exist) and is flipped to privileged: true in a new source revision can have its privileged content materialized by source-watcher and upgraded onto the lingering HelmRelease before cozystack withholds it. The deployed release is not uninstalled afterward, so the exposure can persist. This defeats the confirmation gate's purpose (defending against a tap publisher escalating to privileged).
Reproduction
A variant that dependsOn a dependency which is transiently not-ready skips cleanupOrphanedHelmReleases entirely: the areDependenciesReady early-return precedes it. The lingering HelmRelease remains while source-watcher rebuilds the ExternalArtifact from the new revision using the ArtifactGenerator spec that still lists the component.
Current mitigations (best-effort)
- Package reconciler refuses a privileged component's HelmRelease for an unconfirmed Package and prunes a lingering one.
- PackageSource reconciler withholds a privileged component's
ExternalArtifactuntil confirmed, re-running on confirmation via a Package watch.
These narrow the window but do not close it, because they all lag the content observation.
Proposed fix
Decouple catalog registration from install: produce ApplicationDefinitions (catalog visibility) directly from PackageSource metadata rather than by deploying the registration Package's HelmReleases. An unconfirmed tap can then materialize nothing (no HelmReleases, no artifacts) until confirmed, gating on the confirmation label (known without parsing content) rather than the content-derived privileged flag. Race-free by construction, and preserves dashboard catalog visibility.
Activity
feat(marketplace): register a tapped repository's apps on connect
feat(marketplace): register a tapped repository's apps on connect
IvanHunters commentedon Sep 21, 2026
Design proposal: make the catalog a product of metadata, not of an install
Before the design itself, two properties of
ApplicationDefinitionthat the fix proposed in the issue body runs into. Both are read off the code on main.An ApplicationDefinition is a cluster-wide write capability over existing releases.
updateHelmReleasesForAppDef(internal/controller/applicationdefinition_helmreconciler.go:80) lists HelmReleases across the whole cluster selecting onapps.cozystack.io/application.kindandapps.cozystack.io/application.groupalone, then rewritesspec.chartRefon every match to the one the definition carries. No ownership relation ties a definition to the releases it rewrites. cozystack-api stamps both labels on every tenant application release it creates (pkg/registry/apps/application/rest.go:237). So a definition declaringapplication.kind: Redisrepoints every existing tenant Redis release at its own artifact, whatever the definition object itself is named, and the object name is the only thing a Helm ownership conflict would catch. Nothing checksapplication.kindfor uniqueness: not ininternal/marketplace/collision, not anywhere else in the tree.A malformed ApplicationDefinition stops cozystack-api from starting.
Completebuilds aconfig.Resourcefrom every definition and returns the first error (pkg/cmd/server/start.go:251), whichbuildResourceFromCRDproduces for an invalidrelease.cozystack.io/*annotation (start.go:310).ApplicationDefinitionReconcilerrestarts cozystack-api, debounced, on any definition change, so such a definition takes the API down when it lands rather than at some later restart.Neither is reachable from a tap in any release: the tap feature is unreleased, absent from v1.6.3 and v1.7.0-alpha.1. Without a tap, putting an ApplicationDefinition on the cluster takes the same access as installing a chart, so neither property crosses a trust boundary as things stand.
They matter here because producing ApplicationDefinitions for an unconfirmed tap, which is what the issue body proposes, hands both capabilities to a tap publisher at connect time. That closes the race and keeps the class: the gate stops depending on when we observe a content flip, and starts depending on how completely we validate content we apply on the publisher's behalf.
The proposal
Split the two jobs the registration Package does at once, and keep only one of them automatic.
PackageSourcein it (internal/operator/tapmaterializer_artifact.go). It also collects theApplicationDefinitiondocuments, and takes only the catalog fields:application.kind,plural,singular, and thedashboardblock. No ApplicationDefinition object is applied.PackageSource.Status, attributed to their component throughnaming.ArtifactName, the relationbuildTapalready uses to map a definition to a component. The icon is the only large field, so the recorded set needs a size cap.buildTapprefers a live ApplicationDefinition where one exists and falls back to the recorded metadata where none does, with a newTapPackage.Installedseparating the two. The marketplace view fills in on connect, which is what feat(marketplace): register a tapped repository's apps on connect #4308 set out to achieve.Package, throughcozypkg addor the dashboard's install action. It produces the install HelmReleases and, through the app's-rdcomponent, the ApplicationDefinition, exactly as a platform package does today. Platform and tap keep a single mechanism.What this removes
An unconfirmed tap has no Package, so there is nothing left to gate, and the best-effort layers in #4308 come out instead of growing another qualifier:
collision.PrivilegedConfirmed, the Package reconciler's privileged skip and its prune of a lingering HelmRelease, the PackageSource reconciler's artifact withholding, and the Package watch that re-runs it.The privileged decision after handover stays with #4358, as an allowlist of the component names the operator approved. That decision never depends on observing a flip in time, which is what makes it race-free rather than merely narrower.
Worth its own issue
application.kinduniqueness, and the missing ownership relation inApplicationDefinitionHelmReconciler, are wider than taps: the same takeover follows fromcozypkg addagainst any repository declaring a colliding kind. I would rather fix that separately than fold it into this one.What I would like agreement on before writing code
Whether catalog metadata belongs on
PackageSource.Statusor in a separate cluster-scoped object, and whether the dashboard is content to render an uninstalled package fromTapPackage.Installedrather than from an ApplicationDefinition.feat(marketplace): register a tapped repository's apps on connect (#4308
fix(api): let one ApplicationDefinition own each application kind (#4424
fix(marketplace): do not install a connected repository's components …
fix(marketplace): do not install a connected repository's components …