Repository navigation
fix(api): send the initial-events-end bookmark only to WatchList clients - #4467
Aleksei Sviridkin (lexfrei) wants to merge 1 commit into
Conversation
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]>
|
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 (14)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughWatch 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. ChangesWatch bookmark handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The watch changes are mergeable after normal checks; no concrete remaining issue has been established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Folded into #4473 together with the rest of the stack, so the Kubernetes bump lands as one PR. |
…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 -->
What this PR does
A plain
kubectl get -won any aggregated cozystack-api resource gets a bookmark it never asked for: the one that marks the end of WatchList initial events. The apiserver defaultssendInitialEvents=trueonto every watch whose resourceVersion is empty or0, without turning bookmarks on, and the storages treated that flag alone as a WatchList request.On the
k8s.io/apiserverv0.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 bothsendInitialEventsandallowWatchBookmarksare 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
sendInitialEventsto 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
Summary by CodeRabbit