Skip to content

fix(api): send the initial-events-end bookmark only to WatchList clients - #4467

Closed
Aleksei Sviridkin (lexfrei) wants to merge 1 commit into
mainfrom
fix/watch-initial-events-bookmark-needs-bookmarks
Closed

Aleksei Sviridkin (lexfrei) wants to merge 1 commit into
mainfrom
fix/watch-initial-events-bookmark-needs-bookmarks

Conversation

@lexfrei

@lexfrei Aleksei Sviridkin (lexfrei) commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

What this PR does

A plain kubectl get -w on any aggregated cozystack-api resource gets a bookmark it never asked for: the one that marks the end of WatchList initial events. The apiserver defaults sendInitialEvents=true onto every watch whose resourceVersion is empty or 0, without turning bookmarks on, and the storages treated that flag alone as a WatchList request.

On the k8s.io/apiserver v0.35 we build today this goes unnoticed. From v0.36 the watch handler compiled into cozystack-api treats that bookmark as the end of a WatchList and calls a completion hook it only sets up when both sendInitialEvents and allowWatchBookmarks are on, so it dereferences nil and the watch dies. That makes this a blocker for bumping the Kubernetes libraries past 0.35, whatever the cluster version.

The marker now needs both flags, the same condition the watch cache uses, and the backing watches only ask for bookmarks when the client did. This covers applications, tenant modules, tenant namespaces, tenant secrets and security groups. Security groups still forward sendInitialEvents to their backing watch so existing objects replay as ADDED.

Tests cover each storage for a plain watch, a WatchList watch and the backing watch options, plus one test that runs a plain watch and a WatchList watch through the real apiserver handler chain. Every storage test and the handler test fail on main.

Option and Tap never send the marker at all, which is a separate gap: #4465.

Screenshots

No UI changes.

Downstream repositories

Release note

fix(api): watches on cozystack-api resources no longer receive the WatchList initial-events-end bookmark unless the client asked for bookmarks.

Summary by CodeRabbit

  • Bug Fixes
    • Watch requests now receive an initial-events-end bookmark only when they request both initial events and bookmarks.
    • Plain watches no longer receive an initial-events-end bookmark. Backing watch bookmarks are forwarded only when requested by the client.

The apiserver defaults SendInitialEvents on for every watch whose
resourceVersion is empty or "0", and leaves AllowWatchBookmarks off. The
aggregated storages read SendInitialEvents alone as a WatchList request,
so a plain `kubectl get -w` on applications, tenant modules, namespaces,
secrets or security groups got a bookmark annotated as the end of
initial events.

The k8s.io/apiserver v0.35 watch handler lets that through. From v0.36
it treats the annotated bookmark as the end of a WatchList and calls the
WatchList completion hook, which it only sets up when SendInitialEvents
and AllowWatchBookmarks are both on, so the handler dereferences nil and
the watch dies. The handler is the one compiled into cozystack-api, so
bumping the library is what breaks it, whatever the cluster version.

The marker now requires both flags, the same condition the watch cache
uses, and the backing watches ask for bookmarks only when the client
did, since every backing bookmark is forwarded. Security groups still
forward SendInitialEvents to their backing watch so it replays existing
objects as ADDED.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <[email protected]>
@github-actions github-actions Bot added size/XL This PR changes 500-999 lines, ignoring generated files area/api Issues or PRs related to the cozystack-api aggregated API server kind/bug Categorizes issue or PR as related to a bug labels Sep 24, 2026
@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 12d21147-3684-4569-8133-6dd6ad82babf

📥 Commits

Reviewing files that changed from the base of the PR and between ab0c9f6 and caa3989.

📒 Files selected for processing (14)
  • pkg/cmd/server/watch_initial_events_test.go
  • pkg/registry/apps/application/rest.go
  • pkg/registry/apps/application/rest_watch_initial_events_test.go
  • pkg/registry/core/tenantmodule/rest.go
  • pkg/registry/core/tenantmodule/rest_watch_test.go
  • pkg/registry/core/tenantnamespace/rest.go
  • pkg/registry/core/tenantnamespace/rest_watch_test.go
  • pkg/registry/core/tenantsecret/rest.go
  • pkg/registry/core/tenantsecret/rest_watch_test.go
  • pkg/registry/registrytest/watch.go
  • pkg/registry/sdn/securitygroup/rest.go
  • pkg/registry/sdn/securitygroup/rest_watch_test.go
  • pkg/registry/utils.go
  • pkg/registry/utils_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

Watch handlers now use a shared condition to decide when to request an initial-events-end bookmark. Tests cover backing-watch options and event streams across several resource registries and the API server.

Changes

Watch bookmark handling

Layer / File(s) Summary
Shared initial-events-end condition
pkg/registry/utils.go, pkg/registry/utils_test.go
InitialEventsEndBookmarkRequested returns true only when initial events are explicitly requested and watch bookmarks are allowed. The bookmarker uses this condition to determine whether an end bookmark remains to be sent.
Application and tenant-module watches
pkg/registry/apps/application/*, pkg/registry/core/tenantmodule/*, pkg/registry/registrytest/watch.go
These watch handlers use the shared condition for backing-watch options and initial-events bookmark handling. Tests check option forwarding and event order. RawRecordingWatch records raw options for backing-watch requests.
Tenant and security-group watches
pkg/registry/core/tenantnamespace/*, pkg/registry/core/tenantsecret/*, pkg/registry/sdn/securitygroup/*
These handlers update bookmark handling, and their tests check backing-watch options and emitted events. Security-group watches forward the client’s AllowWatchBookmarks option to the backing watch.
API server watch tests
pkg/cmd/server/watch_initial_events_test.go
Integration tests check that a plain watch receives a modified event without an initial-events-end bookmark, while a WatchList request receives the annotated bookmark.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to caa39

The watch changes are mergeable after normal checks; no concrete remaining issue has been established.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.97% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 14 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: initial-events-end bookmarks are sent only to WatchList clients.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lexfrei

Copy link
Copy Markdown
Contributor Author

Folded into #4473 together with the rest of the stack, so the Kubernetes bump lands as one PR.

Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Oct 2, 2026
…hinery fork (#4473)

## What this PR does

This moves the root module's `k8s.io/*` from v0.35 to v0.37.1 and
controller-runtime from v0.23 to v0.25.1, and drops the
`k8s.io/apimachinery` fork on the way. Supersedes #3001, #4467 and
#4472; those were a stack of the same commits.

The four commits are meant to be read one by one, and each builds and
passes the tests on its own:

1. **Plain watches stop getting the initial-events-end bookmark.** The
apiserver defaults `sendInitialEvents=true` onto any watch with an empty
or `0` resourceVersion and leaves bookmarks off, and our aggregated
storages took that flag alone as a WatchList request. From
`k8s.io/apiserver` v0.36 the watch handler compiled into cozystack-api
panics on that bookmark, so `kubectl get -w` on any cozystack-api
resource would die after the bump. The marker now needs both flags, the
same condition the watch cache uses. There are tests per storage and one
through the real handler chain.
2. **The apimachinery fork goes.** Every apps kind shares the
`Application` Go type, and stock `Scheme.ConvertToVersion` picks the
first kind registered for it. The fork patched that; the apps group now
encodes through a serializer that keeps the kind the object carries. A
handler-chain test covers create, get, list, update, watch, apply and
table for two kinds and fails without it.
3. **MutatingAdmissionPolicy stays out of admission.** From v0.36 the
plugin is on by default and refuses every write until it has synced
`mutatingadmissionpolicies`, which cozystack-api has no RBAC to list and
which clusters before 1.36 do not serve. Without this, every create,
update and delete would fail after the bump. It is also re-applied in
`Complete`, since `--disable-admission-plugins` replaces the list.
4. **The bump itself**, with the regenerated clients, openapi and
strategy CRDs.

Commits 2 and 4 are where the review effort goes; 1 and 3 are small.

Known changes in behaviour:

- Without the fork, the row objects of a Table requested with
`includeObject=Object` carry the first registered app kind. kubectl asks
for that only with `--sort-by`, and it sorts on fields, not on the kind.
Nothing at this layer can fix it, because the handler converts through
`APIGroupInfo.Scheme`, a concrete `*runtime.Scheme`. Admission webhooks
and ValidatingAdmissionPolicy still see the right kind.
- The alpha PodSpec field `workloadRef` became `schedulingGroup`
upstream. Once the new strategy CRDs are applied, the apiserver prunes
`workloadRef` from stored strategies it serves, and their next write
erases it. It only ever worked behind an alpha feature gate.

The branch is rebased onto the Renovate rollup (#4447), with `go mod
tidy` and codegen rerun on top of it.

### Screenshots

No UI changes.

### Downstream repositories

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
- [ ] [cozystack/community](https://github.com/cozystack/community) -
follow-up:

### Release note

```release-note
chore(deps): cozystack-api and the controllers build against the Kubernetes 1.37 libraries without the apimachinery fork. Plain watches no longer receive the WatchList initial-events-end bookmark, and objects in Table rows requested with includeObject=Object may carry another app's kind.
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Application API responses now preserve the requested resource kind
across operations, including lists and watches.
* Backup strategy resources support additional Kubernetes pod settings,
including probe protocols, volume ownership options, and scheduling
groups.
* Backup strategy schemas include updated Kubernetes pod fields and
descriptions.
* **Bug Fixes**
* Initial-event watch bookmarks are sent only when requested; plain
watches continue without an ending bookmark.
* Mutating Admission Policy is disabled by default, including when
admission plugin options are customized.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api Issues or PRs related to the cozystack-api aggregated API server kind/bug Categorizes issue or PR as related to a bug size/XL This PR changes 500-999 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant