Repository navigation
fix(api): apply labelSelector in TenantSecret Watch - #2542
Conversation
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 <[email protected]>
📝 WalkthroughWalkthroughCentralizes tenant label-selector construction with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
github-actions
Bot
added
area/api
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an inconsistency where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
Bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements proper label selector support for the TenantSecret REST implementation in both List and Watch operations. It ensures that user-provided label selectors are correctly merged with the internal tenant resource filter. Additionally, it adds a defensive post-filtering step in the Watch event loop and includes a comprehensive suite of unit tests to verify these behaviors and prevent regressions. I have no feedback to provide.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — the new defensive post-filter on the Watch event stream silently drops legitimate transition-out events, which breaks cache consistency for the very dashboard use case this PR is meant to fix.
Business context: TenantSecret.Watch ignored opts.LabelSelector and emitted every tenant Secret in the namespace; the goal is to merge the user's selector into the underlying watch and stream only matching events.
Blockers
B1: defensive post-filter drops transition-out DELETED events
File: pkg/registry/core/tenantsecret/rest.go:517-522
Issue: The defensive ls.Matches(labels.Set(sec.Labels)) post-filter at line 520 drops every event whose object no longer matches the selector. That includes the legitimate DELETED event the apiserver emits when a Secret's labels mutate out of the watched selector — for example when apps.cozystack.io/application.name is changed from harbor-prod to harbor-staging.
Evidence: Standard apiserver watch semantics (https://kubernetes.io/docs/reference/using-api/api-concepts/#filtering-during-watch) require that, when an UPDATE results in an object no longer matching the filter, the apiserver synthesizes a DELETED event with the post-update labels. The post-filter at rest.go:520 evaluates the object's current labels — which by definition do not match the selector — so the DELETED event is dropped. Cached client subscriptions (e.g. dashboards that rely on watch-driven local stores) never learn the object left their selected set and retain stale TenantSecrets indefinitely.
Impact: Selector-aware watch consumers — the exact use case the PR description names — accumulate stale entries whenever a tenant Secret's labels change. The bug only manifests against a real apiserver, not the fake client used in TestWatch_WithLabelSelector_FiltersEvents, so the existing test won't catch the regression.
Fix: Either drop the post-filter entirely (trust the underlying watch's selector handling) or gate it on event type so DELETED events pass through untouched: if ev.Type != watch.Deleted && !ls.Matches(labels.Set(sec.Labels)) { continue }. The apiserver will not emit ADDED/MODIFIED for non-matching objects, and emits DELETED specifically for transitions out — those must be forwarded so clients can evict stale entries.
Non-blocking follow-ups
-
No test exercises
labels.Nothing()(or any non-selectable selector) for either List (rest.go:259-262) or Watch (rest.go:456-460). Thelabels.Nothing()branch in Watch in particular is the only path that returns a closed-channel ProxyWatcher, and it is currently untested. -
Selector-merge logic (
labels.NewSelector(), internal-tenant requirement,Requirements()withselectablecheck) is duplicated between List (rest.go:244-266) and Watch (rest.go:450-465). A helper likebuildTenantSelector(opts) (labels.Selector, bool /*selectable*/)would centralize the rule. Not urgent.
| // Defensive: post-filter against the merged selector. The underlying | ||
| // watch already filters by label, but this guards against any client | ||
| // implementation that doesn't honor LabelSelector on Watch. | ||
| if !ls.Matches(labels.Set(sec.Labels)) { |
There was a problem hiding this comment.
This post-filter drops legitimate transition-out DELETED events. When a Secret's labels mutate out of the selector (e.g. application.name changes from harbor-prod to harbor-staging), the apiserver synthesizes a DELETED event with the post-update labels — which by construction don't match ls. The current ls.Matches(...) check drops that event, so cached client subscriptions never learn the object left their watched set.
Suggested fix: gate the filter on event type so DELETED passes through:
if ev.Type != watch.Deleted && !ls.Matches(labels.Set(sec.Labels)) {
continue
}The apiserver only emits ADDED/MODIFIED for objects in the selected set, so dropping non-matching ones in those cases is still safe defense-in-depth.
…te selector logic - 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 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/registry/core/tenantsecret/rest_test.go (1)
302-337: Avoid fixed timeout waiting when expected events are already observed.
collectAddedNamescurrently drains until timeout/channel close, so this test usually waits the full timeout even after getting the needed event. Consider early-return once expected ADDED events are collected to reduce latency/flakiness.Suggested refactor
- got := collectAddedNames(t, w, 500*time.Millisecond) + got := collectAddedNames(t, w, 1, 2*time.Second) -func collectAddedNames(t *testing.T, w watch.Interface, timeout time.Duration) []string { +func collectAddedNames(t *testing.T, w watch.Interface, expected int, timeout time.Duration) []string { t.Helper() names := make([]string, 0) deadline := time.NewTimer(timeout) defer deadline.Stop() for { select { case ev, ok := <-w.ResultChan(): if !ok { sort.Strings(names) return names } if ev.Type != watch.Added { continue } ts, ok := ev.Object.(*corev1alpha1.TenantSecret) if !ok { t.Fatalf("expected *TenantSecret in event, got %T", ev.Object) } names = append(names, ts.Name) + if expected > 0 && len(names) >= expected { + sort.Strings(names) + return names + } case <-deadline.C: sort.Strings(names) return names } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/registry/core/tenantsecret/rest_test.go` around lines 302 - 337, collectAddedNames currently always waits the full timeout or channel close even when the test already observed the expected ADDED events; change collectAddedNames to accept an expectedCount (or a boolean to early-return on first match) and in the loop after appending ts.Name check if len(names) >= expectedCount then sort and return immediately. Update the test call that does got := collectAddedNames(t, w, 500*time.Millisecond) to pass expectedCount 1 (since the test expects "harbor-test-credentials"), keeping the existing behavior of ignoring non-Added events and type-checking TenantSecret.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/registry/core/tenantsecret/rest_test.go`:
- Around line 194-196: The test silently ignores the error from labels.Parse
when building sel; change the call to capture the error (e.g., sel, err :=
labels.Parse(...)) and assert/fail if err != nil (for example use
t.Fatalf("failed to parse selector: %v", err) or the test helper you use such as
require.NoError) so parsing failures surface during test runs; update the sel
variable usage accordingly in rest_test.go.
---
Nitpick comments:
In `@pkg/registry/core/tenantsecret/rest_test.go`:
- Around line 302-337: collectAddedNames currently always waits the full timeout
or channel close even when the test already observed the expected ADDED events;
change collectAddedNames to accept an expectedCount (or a boolean to
early-return on first match) and in the loop after appending ts.Name check if
len(names) >= expectedCount then sort and return immediately. Update the test
call that does got := collectAddedNames(t, w, 500*time.Millisecond) to pass
expectedCount 1 (since the test expects "harbor-test-credentials"), keeping the
existing behavior of ignoring non-Added events and type-checking TenantSecret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51bbc893-bc33-4363-b38a-87a66468970c
📒 Files selected for processing (2)
pkg/registry/core/tenantsecret/rest.gopkg/registry/core/tenantsecret/rest_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/registry/core/tenantsecret/rest.go
| sel, _ := labels.Parse( | ||
| "apps.cozystack.io/application.kind=Bucket,apps.cozystack.io/application.name=test", | ||
| ) |
There was a problem hiding this comment.
Handle selector parse errors instead of ignoring them.
At Line 194, labels.Parse error is discarded. Even in tests, silently ignoring parse failures can mask regressions.
Proposed fix
- sel, _ := labels.Parse(
+ sel, err := labels.Parse(
"apps.cozystack.io/application.kind=Bucket,apps.cozystack.io/application.name=test",
)
+ if err != nil {
+ t.Fatalf("parse selector: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sel, _ := labels.Parse( | |
| "apps.cozystack.io/application.kind=Bucket,apps.cozystack.io/application.name=test", | |
| ) | |
| sel, err := labels.Parse( | |
| "apps.cozystack.io/application.kind=Bucket,apps.cozystack.io/application.name=test", | |
| ) | |
| if err != nil { | |
| t.Fatalf("parse selector: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/registry/core/tenantsecret/rest_test.go` around lines 194 - 196, The test
silently ignores the error from labels.Parse when building sel; change the call
to capture the error (e.g., sel, err := labels.Parse(...)) and assert/fail if
err != nil (for example use t.Fatalf("failed to parse selector: %v", err) or the
test helper you use such as require.NoError) so parsing failures surface during
test runs; update the sel variable usage accordingly in rest_test.go.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the post-filter fix lands exactly as suggested (DELETED events pass through, ADDED/MODIFIED still gated by the selector), the selector-merge logic is now factored into buildTenantSelector, and both labels.Nothing() paths (List + Watch) are covered by tests.
Business context: TenantSecret.Watch ignored opts.LabelSelector and emitted every tenant Secret in the namespace; the goal is to merge the user's selector into the underlying watch and stream only matching events.
Resolved since previous review
- B1 (defensive post-filter dropping transition-out DELETED) — fixed in 4e97cb3 with
if ev.Type != watch.Deleted && !ls.Matches(...) { continue }plus an inline comment explaining that the apiserver synthesizes DELETED with the post-mutation labels. - Non-blocking #1 (no test for
labels.Nothing()) — addressed byTestList_WithNothingSelector_ReturnsEmptyandTestWatch_WithNothingSelector_ClosesImmediately. - Non-blocking #2 (selector-merge duplication between List and Watch) — addressed by extracting
buildTenantSelector(opts).
2 participants
What this PR does
opts.LabelSelectorinto the underlying watch selector inTenantSecret.Watch(), mirroring the behaviourList()already has.labels.Nothing()) by returning a watcher with a closed result channel.rest_test.gocovering label-selector merging inListand the newWatchfiltering behaviour.Watch()previously ignoredopts.LabelSelectorentirely; only the requiredinternal.cozystack.io/tenantresource=truelabel was passed through. As a result, watch consumers (e.g. dashboards using cached client subscriptions) saw every tenant Secret in the namespace regardless of the selector they requested.List()already merged the user selector correctly, so the inconsistency was easy to miss.The
Watchunit test creates Secrets after the watch starts becausesigs.k8s.io/controller-runtime/pkg/client/fake.Client.Watchdoes not emit initial ADDED events for objects already in the tracker; this still exercises the merged-selector path through the defensive post-filter.Screenshots
Not applicable — backend-only change.
Release note
Summary by CodeRabbit
Bug Fixes
Tests