The Watch goroutine never closed the events channel it owns, and
ProxyWatcher.Stop does not close the channel it proxies. When the
backing watcher ended, consumers ranging over ResultChan blocked until
their own timeout instead of seeing end-of-stream. Closing the channel
on goroutine exit matches the watch.Interface contract and the pattern
already used by the tenantmodule and application registries.
Goroutine-termination tests now assert the result channel closes.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Four goroutine-termination tests duplicated the same IsStopped polling
loop; waitForFakeWatcherStop keeps the deadline logic in one place.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Manual runtime.NewScheme setups in the List/Get error-path tests silently
discarded AddToScheme errors; the shared helper fails the test instead.
Renamed from newWatchTestScheme since it now serves both test files.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Cover the per-event filtering paths of REST.Watch directly instead of
only transitively through the bookmark test: authorization filtering via
RoleBindings, privileged groups (system:masters, cozystack-cluster-admin),
non-tenant namespace and foreign object drops, backing bookmark
forwarding, field and label selector filtering, missing user rejection,
access-check error resilience, ADDED deduplication by resourceVersion,
terminating bookmark flush on backing watcher close, and goroutine
shutdown on Stop and context cancellation.
Also close the remaining gaps on the sibling read paths sharing the same
authorization logic: List/filterAccessible (per-user filtering, privileged
groups, missing user, RoleBindings outside the tenant set, backing list
errors) and the Get error branches.
Branches the controller-runtime fake client cannot drive (backing
bookmarks, non-Namespace objects, chosen resourceVersions, backing watch
errors) are exercised through a stub client.WithWatch backed by a
test-owned FakeWatcher.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
## What this PR does
While preparing for my CozySummit talk of last week (creating commits
from CozySummit edits with
[gitops-reverser](https://github.com/ConfigButler/gitops-reverser)) I
noticed warnings on my console:
```
Warning: event bookmark expired err="reflector.go:343: hasn't received required bookmark event marking the end of initial events stream, received last event 59.998946102s ago"
The warning fires on every reflector restart and continuously while the cache is unsynced from the WatchList perspective.
```
I've been searching a bit better and it turns out that `TenantSecret`,
`TenantModule`, and `TenantNamespace` (group `core.cozystack.io`) all
ignore `ListOptions.SendInitialEvents`. Interestingly enough
`apps.cozystack.io/Application` already does implement this in the right
way.
Under the [WatchList / streaming-list
protocol](https://kubernetes.io/docs/reference/using-api/api-concepts/#streaming-lists)
(client-go's `WatchListClient`, default-on since v1.35), clients expect
the initial `ADDED` events to be terminated by a
[`Bookmark`](https://kubernetes.io/docs/reference/using-api/api-concepts/#watch-bookmarks)
annotated with `k8s.io/initial-events-end: "true"`. These three
resources only forwarded the backing watcher's bookmarks (which lack the
annotation), so the marker was never sent and client-go reflectors never
reached `HasSynced` — looping on `reflector.go:343: hasn't received
required bookmark event marking the end of initial events stream`.
**Fix:** extracted the contract into a shared
`registry.InitialEventsBookmarker` helper (`pkg/registry/utils.go`) and
wired it into all four `Watch` implementations. It annotates the first
backing bookmark as the terminating marker, with fallbacks to emit it
before the first live event and on watcher close. The annotation reuses
the upstream `metav1.InitialEventsAnnotationKey` constant rather than a
string literal.
The full bookmark machinery in `k8s.io/apiserver/pkg/storage/cacher`
isn't reusable here: it's internal to the watch-cache layer and only
fires for cacher-backed storage, whereas these resources translate a
controller-runtime watch in custom REST storage that bypasses the
cacher.
`Application` is refactored onto the helper with its `WorkloadMonitor`
event-draining preserved. Adds unit tests for the helper and watch tests
for each core resource (`ADDED` → annotated `Bookmark` → live event).
### Screenshots
N/A — no UI changes.
### Release note
```release-note
fix(api): core.cozystack.io resources (TenantSecret, TenantModule, TenantNamespace) now emit the k8s.io/initial-events-end bookmark for SendInitialEvents=true (WatchList) watches, so client-go informers reach HasSynced instead of looping on the missing-bookmark warning.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Watch endpoints reliably emit the "initial-events-end" bookmark after
initial events, improving watch/list contract compliance and client
reflector synchronization.
* **Refactor**
* Watch implementations consolidated around a centralized initial-events
bookmarker and stop-aware event forwarding for consistent bookmark
behavior.
* **New Features**
* Introduced a reusable initial-events bookmarker to coordinate bookmark
emission and resourceVersion observation across translated watch
streams.
* **Tests**
* Added unit and integration tests validating bookmark emission,
ordering, resourceVersion seeding/observation, and close-time behavior.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
The aggregated apiserver resources in group core.cozystack.io
(TenantSecret, TenantModule, TenantNamespace) ignored the
SendInitialEvents field of ListOptions. Clients using the WatchList /
streaming-list protocol (client-go's WatchListClient feature, on by
default since v1.35) never received the required
k8s.io/initial-events-end bookmark, so their reflectors stayed stuck in
the initial-stream phase and logged "hasn't received required bookmark
event marking the end of initial events stream" every ~10s, never
reaching HasSynced.
apps.cozystack.io/Application already implemented this contract inline.
Extract that logic into a shared registry.InitialEventsBookmarker helper
(pkg/registry/utils.go) and wire it into all four translating Watch
implementations: after the initial ADDED events, the first backing
bookmark is annotated as the initial-events-end marker, with fallbacks
that emit it before the first live event and on watcher close. Application
is refactored onto the shared helper with its WorkloadMonitor
pending-event draining preserved.
Adds unit tests for the bookmarker and watch tests for each of the three
core resources asserting ADDED -> annotated Bookmark -> live event.
```release-note
[apiserver] core.cozystack.io resources (TenantSecret, TenantModule,
TenantNamespace) now emit the k8s.io/initial-events-end bookmark for
SendInitialEvents=true (WatchList) watches, so client-go informers reach
HasSynced instead of looping on the missing-bookmark warning.
```
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Simon Koudijs <simonkoudijs@gmail.com>
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 <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
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 <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
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 <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
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 <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
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 <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
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 <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
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 <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
- Gate defensive label post-filter on event type: DELETED events now always
pass through so cached clients can evict stale entries when a Secret's
labels mutate out of the watched selector
- Extract buildTenantSelector helper to eliminate duplicated selector-merge
code between List and Watch
- Add TestList_WithNothingSelector_ReturnsEmpty and
TestWatch_WithNothingSelector_ClosesImmediately to cover the previously
untested labels.Nothing() path
Signed-off-by: IvanHunters <ivan.okhotnikov@aenix.io>
Watch() in the TenantSecret REST handler hard-coded the selector to the
internal.cozystack.io/tenantresource=true label and discarded
opts.LabelSelector entirely. Watch streams therefore returned every
tenant Secret in the namespace regardless of the user-provided selector,
while List() filtered correctly. Clients that switch to Watch after the
initial List surfaced unrelated Secrets.
Merge opts.LabelSelector into the underlying watch selector the same
way List() already does, handle non-selectable selectors by returning a
closed watcher, and add a defensive post-filter in the streaming
goroutine so events never escape the requested selector even if a future
client implementation skips label filtering on Watch.
Adds rest_test.go covering label-selector merging in List and a Watch
test that creates Secrets after the watch starts (since fake.Client.Watch
does not emit initial ADDED events) to assert only matching events are
observed.
Signed-off-by: IvanHunters <ivan.okhotnikov@aenix.io>
- Hoist user identity extraction out of the Watch goroutine; reuse a
cached username and groups map across events instead of re-fetching
them per event. Watch now returns Unauthorized up front when no user
is present in the context, rather than failing silently per event.
- Switch the per-event access-check error log to structured klog.ErrorS
to comply with the project Go style guide.
- Strengthen TestGet_WithAccess to assert the concrete *TenantNamespace
type plus Name, Kind, and APIVersion, so type or metadata regressions
fail fast.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Address review feedback from sircthulhu and CodeRabbit:
1. Return Forbidden instead of NotFound for unauthorized access
- Get() now returns 403 Forbidden to follow standard K8s RBAC behavior
- Previously returned 404 NotFound for security-by-obscurity
- Updated test expectations to match new behavior
2. Propagate field and label selectors in Watch handler
- Pass opts.FieldSelector and opts.LabelSelector to upstream Watch
- Add defensive filtering before authorization to prevent RBAC bypass
- Fixes potential issue with resourceNames restrictions
3. Refactor subject-matching logic to eliminate duplication
- Extract matchesSubject() helper for Group/User/ServiceAccount checks
- Remove duplicated code from filterAccessible and hasAccessToNamespace
- Consolidates ServiceAccount namespace fallback logic
All tests pass successfully.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Added comprehensive unit tests for authorization logic:
- hasAccessToNamespace() tests:
- User subject access (positive and negative cases)
- Group subject access
- ServiceAccount subject access
- ServiceAccount with empty namespace (defaults to RoleBinding ns)
- Privileged groups (system:masters, cozystack-cluster-admin)
- Get() handler tests:
- Returns namespace when user has access
- Returns NotFound when user lacks access (not Forbidden)
- Returns NotFound for non-tenant namespaces
All tests verify that authorization correctly enforces RoleBinding-based
access control and prevents IDOR vulnerabilities.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
- Add resourceVersion handling for Watch requests by filtering ADDED events
based on the resourceVersion provided by the client
- Forward bookmark events from underlying HelmRelease watchers to clients
for proper resourceVersion synchronization
- Extract MaxResourceVersion helper using meta.EachListItem for cleaner code
- This ensures clients don't receive duplicate objects they already have
from List+Watch patterns
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Fix getVersion to parse "0.1.4+abcdef" format (with "+" separator)
instead of incorrectly looking for "sha256:" prefix.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Controller-runtime cache doesn't support field selectors, causing
incorrect filtering when using kubectl with field selectors like
--field-selector=metadata.namespace=tenant-kvaps or metadata.name=test.
Changes:
- Created pkg/registry/fields package with ParseFieldSelector utility
- Refactored field selector parsing logic in application, tenantmodule,
and tenantsecret registries to use common implementation
- Implemented manual filtering for metadata.name and metadata.namespace
in List() and Watch() methods
- Removed Raw field usage and field selectors from client.ListOptions
- Label selectors passed directly via LabelSelector field
Field selectors now properly filter resources by name and namespace
through manual post-processing after label-based filtering.
See: https://github.com/kubernetes-sigs/controller-runtime/issues/612
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Add sorting helper package with generic functions for sorting
resources by name (cluster-scoped) or namespace/name (namespace-scoped).
Apply sorting to:
- TenantNamespace (by name)
- TenantSecret (refactored to use helper)
- TenantModule (new)
- Application (new)
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
This commit refactors the apiserver REST handlers to use typed objects
(appsv1alpha1.Application) instead of unstructured.Unstructured, eliminating
the need for runtime conversions and simplifying the codebase.
Additionally, it fixes an issue where UnstructuredList objects were using
the first registered kind from typeToGVK instead of the kind from the
object's field when multiple kinds are registered with the same Go type.
This is a more comprehensive fix for the problem addressed in
https://github.com/cozystack/cozystack/pull/1630, which was reverted in
https://github.com/cozystack/cozystack/pull/1677.
The fix includes the upstream fix from kubernetes/kubernetes#135537,
which enables short-circuit path for UnstructuredList similar to regular
Unstructured objects, using GVK from the object field instead of
typeToGVK.
Changes:
- Refactored rest.go handlers to use typed Application objects
- Removed unstructured.Unstructured conversions
- Fixed UnstructuredList GVK handling
- Updated dependencies in go.mod/go.sum
- Added e2e test for OpenAPI validation
- Updated Dockerfile
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
This patch changes all clients in the Cozystack API server to typed
ones from the controller runtime. This should improve the performance of
the API server and simplifies the code by removing work with
unstructured objects and dynamic clients.
```release-note
[api] Use typed and cache-backed k8s clients in the Cozystack API to
improve performance. Get rid of operations on unstructured objects and
use of dynamic clients.
```
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
The Cozystack API server lists TenantNamespaces by running a
SubjectAccessReview against every single requested namespace to see if
the user can create a WorkloadMonitor there. Will this is robust in
terms of permissions, delegating the authorization decision to the k8s
API, this is incredibly inefficient and has caused high latency to the
API. This patch simplifies the logic by instead getting the user's
groups and checking if the namespace contains a rolebinding for that
group. That way listing TenantNamespaces is reduced to a list call to
the k8s API for namespaces and another list call for rolebindings across
all namespaces, while authorization is done on the Cozystack API server
instead of making further calls to the k8s API.
```release-note
[api] Optimize listing of TenantNamespaces, fixes a bug causing very
high latency to the k8s API.
```
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
This patch refactors the secret selectors to use the
`internal.cozystack.io/tenantresource` label for managing secret
visibility and removes any selectors based on it or the previous
`apps.cozystack.io/tenantresource` label, the idea being that this label
will only ever be set by the controller.
```
[controller,api] Refactor labels for the secret selector.
```
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
[cozystack-controller] Introduce new dashboard-controller
[dashboard] Introduce new dashboard based on openapi-ui
Co-authored-by: kklinch0 <kklinch0@gmail.com>
Signed-off-by: kklinch0 <kklinch0@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Aleksei Sviridkin