From c913c9dcc2b645f58abea68fdb06cb1580211329 Mon Sep 17 00:00:00 2001 From: IvanHunters Date: Wed, 29 Apr 2026 20:48:58 +0300 Subject: [PATCH 1/2] fix(api): apply labelSelector in TenantSecret Watch 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 --- pkg/registry/core/tenantsecret/rest.go | 50 ++- pkg/registry/core/tenantsecret/rest_test.go | 318 ++++++++++++++++++++ 2 files changed, 360 insertions(+), 8 deletions(-) create mode 100644 pkg/registry/core/tenantsecret/rest_test.go diff --git a/pkg/registry/core/tenantsecret/rest.go b/pkg/registry/core/tenantsecret/rest.go index 1f95c397..bd296cc0 100644 --- a/pkg/registry/core/tenantsecret/rest.go +++ b/pkg/registry/core/tenantsecret/rest.go @@ -245,8 +245,22 @@ func (r *REST) List(ctx context.Context, opts *metainternal.ListOptions) (runtim req, _ := labels.NewRequirement(tsLabelKey, selection.Equals, []string{tsLabelValue}) ls = ls.Add(*req) + emptyList := func() *corev1alpha1.TenantSecretList { + return &corev1alpha1.TenantSecretList{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1alpha1.SchemeGroupVersion.String(), + Kind: kindTenantSecretList, + }, + } + } + if opts.LabelSelector != nil { - if reqs, _ := opts.LabelSelector.Requirements(); len(reqs) > 0 { + reqs, selectable := opts.LabelSelector.Requirements() + if !selectable { + // labels.Nothing() and other non-selectable selectors match no objects. + return emptyList(), nil + } + if len(reqs) > 0 { ls = ls.Add(reqs...) } } @@ -261,12 +275,7 @@ func (r *REST) List(ctx context.Context, opts *metainternal.ListOptions) (runtim // If field selector specifies namespace different from context, return empty list if fieldFilter.Namespace != "" && ns != "" && ns != fieldFilter.Namespace { - return &corev1alpha1.TenantSecretList{ - TypeMeta: metav1.TypeMeta{ - APIVersion: corev1alpha1.SchemeGroupVersion.String(), - Kind: kindTenantSecretList, - }, - }, nil + return emptyList(), nil } list := &corev1.SecretList{} @@ -436,8 +445,26 @@ func (r *REST) Watch(ctx context.Context, opts *metainternal.ListOptions) (watch return nil, err } + // Build the same selector as List() does: required tenant-resource label + // plus any user-provided requirements. + ls := labels.NewSelector() + tsReq, _ := labels.NewRequirement(tsLabelKey, selection.Equals, []string{tsLabelValue}) + ls = ls.Add(*tsReq) + + if opts.LabelSelector != nil { + reqs, selectable := opts.LabelSelector.Requirements() + if !selectable { + // labels.Nothing(): match no objects, return a watcher that closes immediately. + ch := make(chan watch.Event) + close(ch) + return watch.NewProxyWatcher(ch), nil + } + if len(reqs) > 0 { + ls = ls.Add(reqs...) + } + } + secList := &corev1.SecretList{} - ls := labels.Set{tsLabelKey: tsLabelValue}.AsSelector() base, err := r.w.Watch(ctx, secList, &client.ListOptions{ Namespace: ns, LabelSelector: ls, @@ -487,6 +514,13 @@ func (r *REST) Watch(ctx context.Context, opts *metainternal.ListOptions) (watch continue } + // 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)) { + continue + } + tenant := secretToTenant(sec) // Skip ADDED events based on resourceVersion comparison diff --git a/pkg/registry/core/tenantsecret/rest_test.go b/pkg/registry/core/tenantsecret/rest_test.go new file mode 100644 index 00000000..a010c589 --- /dev/null +++ b/pkg/registry/core/tenantsecret/rest_test.go @@ -0,0 +1,318 @@ +// SPDX-License-Identifier: Apache-2.0 + +package tenantsecret + +import ( + "context" + "sort" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + metainternal "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/watch" + "k8s.io/apiserver/pkg/endpoints/request" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + corev1alpha1 "github.com/cozystack/cozystack/pkg/apis/core/v1alpha1" +) + +const testNamespace = "tenant-root" + +func newTestREST(t *testing.T, secrets ...*corev1.Secret) *REST { + t.Helper() + + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("add corev1 to scheme: %v", err) + } + + objs := make([]client.Object, 0, len(secrets)) + for _, s := range secrets { + objs = append(objs, s) + } + fc := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objs...). + Build() + + return &REST{ + c: fc, + w: fc, + gvr: schema.GroupVersionResource{ + Group: corev1alpha1.GroupName, + Version: "v1alpha1", + Resource: "tenantsecrets", + }, + } +} + +// makeTenantSecret produces a Secret already labeled as a tenant resource plus any extra labels. +func makeTenantSecret(name string, extra map[string]string) *corev1.Secret { + lbls := map[string]string{ + corev1alpha1.TenantResourceLabelKey: corev1alpha1.TenantResourceLabelValue, + } + for k, v := range extra { + lbls[k] = v + } + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testNamespace, + Labels: lbls, + }, + Type: corev1.SecretTypeOpaque, + } +} + +func itemNames(items []corev1alpha1.TenantSecret) []string { + out := make([]string, len(items)) + for i, it := range items { + out[i] = it.Name + } + sort.Strings(out) + return out +} + +func listTenantSecrets(t *testing.T, r *REST, opts *metainternal.ListOptions) *corev1alpha1.TenantSecretList { + t.Helper() + ctx := request.WithNamespace(context.Background(), testNamespace) + out, err := r.List(ctx, opts) + if err != nil { + t.Fatalf("List returned error: %v", err) + } + list, ok := out.(*corev1alpha1.TenantSecretList) + if !ok { + t.Fatalf("expected *TenantSecretList, got %T", out) + } + return list +} + +func TestList_NoSelector_ReturnsOnlyTenantSecrets(t *testing.T) { + bucket := makeTenantSecret("bucket-creds", map[string]string{ + "apps.cozystack.io/application.kind": "Bucket", + "apps.cozystack.io/application.name": "test", + }) + monitoring := makeTenantSecret("monitoring-creds", map[string]string{ + "apps.cozystack.io/application.kind": "Monitoring", + "apps.cozystack.io/application.name": "monitoring", + }) + // Plain Secret without the tenant marker — must be excluded from the list. + plain := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plain", + Namespace: testNamespace, + }, + } + + r := newTestREST(t, bucket, monitoring, plain) + + list := listTenantSecrets(t, r, &metainternal.ListOptions{}) + + got := itemNames(list.Items) + want := []string{"bucket-creds", "monitoring-creds"} + if !equalStrings(got, want) { + t.Fatalf("unexpected items: got %v, want %v", got, want) + } +} + +func TestList_WithLabelSelector_FiltersToMatchingApp(t *testing.T) { + bucket := makeTenantSecret("bucket-creds", map[string]string{ + "apps.cozystack.io/application.kind": "Bucket", + "apps.cozystack.io/application.name": "test", + }) + monitoring := makeTenantSecret("monitoring-creds", map[string]string{ + "apps.cozystack.io/application.kind": "Monitoring", + "apps.cozystack.io/application.name": "monitoring", + }) + other := makeTenantSecret("other-bucket", map[string]string{ + "apps.cozystack.io/application.kind": "Bucket", + "apps.cozystack.io/application.name": "other", + }) + + r := newTestREST(t, bucket, monitoring, other) + + 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) + } + + list := listTenantSecrets(t, r, &metainternal.ListOptions{LabelSelector: sel}) + + got := itemNames(list.Items) + want := []string{"bucket-creds"} + if !equalStrings(got, want) { + t.Fatalf("expected only bucket-creds, got %v", got) + } +} + +func TestList_WithLabelSelector_NoMatch_ReturnsEmpty(t *testing.T) { + r := newTestREST(t, makeTenantSecret("bucket-creds", map[string]string{ + "apps.cozystack.io/application.kind": "Bucket", + "apps.cozystack.io/application.name": "monitoring", + })) + + sel, err := labels.Parse("apps.cozystack.io/application.name=does-not-exist") + if err != nil { + t.Fatalf("parse selector: %v", err) + } + + list := listTenantSecrets(t, r, &metainternal.ListOptions{LabelSelector: sel}) + + if len(list.Items) != 0 { + t.Fatalf("expected empty list, got %v", itemNames(list.Items)) + } +} + +func TestList_WithLabelSelector_PreservesTenantFilter(t *testing.T) { + // A non-tenant Secret carrying the same user labels must NOT leak through + // the label-selector filter. + leaked := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "leaked", + Namespace: testNamespace, + Labels: map[string]string{ + "apps.cozystack.io/application.kind": "Bucket", + "apps.cozystack.io/application.name": "test", + }, + }, + } + bucket := makeTenantSecret("bucket-creds", map[string]string{ + "apps.cozystack.io/application.kind": "Bucket", + "apps.cozystack.io/application.name": "test", + }) + + r := newTestREST(t, leaked, bucket) + + sel, _ := labels.Parse( + "apps.cozystack.io/application.kind=Bucket,apps.cozystack.io/application.name=test", + ) + + list := listTenantSecrets(t, r, &metainternal.ListOptions{LabelSelector: sel}) + + got := itemNames(list.Items) + want := []string{"bucket-creds"} + if !equalStrings(got, want) { + t.Fatalf("non-tenant Secret leaked through filter: got %v, want %v", got, want) + } +} + +func TestList_WithEverythingSelector_BehavesLikeNoSelector(t *testing.T) { + bucket := makeTenantSecret("bucket-creds", map[string]string{ + "apps.cozystack.io/application.name": "test", + }) + monitoring := makeTenantSecret("monitoring-creds", map[string]string{ + "apps.cozystack.io/application.name": "monitoring", + }) + + r := newTestREST(t, bucket, monitoring) + + list := listTenantSecrets(t, r, &metainternal.ListOptions{LabelSelector: labels.Everything()}) + + got := itemNames(list.Items) + want := []string{"bucket-creds", "monitoring-creds"} + if !equalStrings(got, want) { + t.Fatalf("expected all tenant secrets, got %v", got) + } +} + +// TestWatch_WithLabelSelector_FiltersEvents reproduces issue +// cozystack/cozystack#2527: TenantSecret Watch ignored opts.LabelSelector +// and streamed every tenant Secret in the namespace, regardless of the +// user-provided selector. +// +// fake.Client.Watch does not emit initial ADDED events for objects already in +// the tracker — only events for subsequent CREATE/UPDATE/DELETE. So we start +// the watch first and then create two secrets; only the one matching the +// selector should be observed. +func TestWatch_WithLabelSelector_FiltersEvents(t *testing.T) { + r := newTestREST(t) + + sel, err := labels.Parse( + "apps.cozystack.io/application.kind=Harbor,apps.cozystack.io/application.name=test", + ) + if err != nil { + t.Fatalf("parse selector: %v", err) + } + + ctx, cancel := context.WithCancel(request.WithNamespace(context.Background(), testNamespace)) + defer cancel() + + w, err := r.Watch(ctx, &metainternal.ListOptions{LabelSelector: sel}) + if err != nil { + t.Fatalf("Watch returned error: %v", err) + } + defer w.Stop() + + matching := makeTenantSecret("harbor-test-credentials", map[string]string{ + "apps.cozystack.io/application.kind": "Harbor", + "apps.cozystack.io/application.name": "test", + }) + other := makeTenantSecret("postgres-new-credentials", map[string]string{ + "apps.cozystack.io/application.kind": "Postgres", + "apps.cozystack.io/application.name": "new", + }) + if err := r.c.Create(ctx, matching); err != nil { + t.Fatalf("create matching secret: %v", err) + } + if err := r.c.Create(ctx, other); err != nil { + t.Fatalf("create other secret: %v", err) + } + + got := collectAddedNames(t, w, 500*time.Millisecond) + want := []string{"harbor-test-credentials"} + + if !equalStrings(got, want) { + t.Fatalf("Watch ignored labelSelector: got %v, want %v", got, want) + } +} + +// collectAddedNames drains ADDED events from a watch until the timeout fires, +// returning the sorted list of object names. Bookmarks are ignored. Used to +// assert that nothing extra leaks past a label-selector filter. +func collectAddedNames(t *testing.T, w watch.Interface, 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) + case <-deadline.C: + sort.Strings(names) + return names + } + } +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} From 4e97cb304be57d0e49a5d7f4f27fa4750bde9ada Mon Sep 17 00:00:00 2001 From: IvanHunters Date: Thu, 30 Apr 2026 00:25:43 +0300 Subject: [PATCH 2/2] fix(api): pass DELETED events through watch post-filter and deduplicate 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 --- pkg/registry/core/tenantsecret/rest.go | 65 +++++++++++---------- pkg/registry/core/tenantsecret/rest_test.go | 33 +++++++++++ 2 files changed, 68 insertions(+), 30 deletions(-) diff --git a/pkg/registry/core/tenantsecret/rest.go b/pkg/registry/core/tenantsecret/rest.go index bd296cc0..c75255ed 100644 --- a/pkg/registry/core/tenantsecret/rest.go +++ b/pkg/registry/core/tenantsecret/rest.go @@ -192,6 +192,26 @@ func (r *REST) GroupVersionKind(_ schema.GroupVersion) schema.GroupVersionKind { } func (*REST) GetSingularName() string { return singularName } +// buildTenantSelector merges the required tenant-resource label with any +// user-provided requirements from opts.LabelSelector. +// Returns (selector, true) on success; (nil, false) when the user selector is +// non-selectable (e.g. labels.Nothing()) — callers should return an empty result. +func buildTenantSelector(opts *metainternal.ListOptions) (labels.Selector, bool) { + ls := labels.NewSelector() + req, _ := labels.NewRequirement(tsLabelKey, selection.Equals, []string{tsLabelValue}) + ls = ls.Add(*req) + if opts.LabelSelector != nil { + reqs, selectable := opts.LabelSelector.Requirements() + if !selectable { + return nil, false + } + if len(reqs) > 0 { + ls = ls.Add(reqs...) + } + } + return ls, true +} + // ----------------------------------------------------------------------------- // CRUD // ----------------------------------------------------------------------------- @@ -241,9 +261,7 @@ func (r *REST) List(ctx context.Context, opts *metainternal.ListOptions) (runtim return nil, err } - ls := labels.NewSelector() - req, _ := labels.NewRequirement(tsLabelKey, selection.Equals, []string{tsLabelValue}) - ls = ls.Add(*req) + ls, selectable := buildTenantSelector(opts) emptyList := func() *corev1alpha1.TenantSecretList { return &corev1alpha1.TenantSecretList{ @@ -254,15 +272,9 @@ func (r *REST) List(ctx context.Context, opts *metainternal.ListOptions) (runtim } } - if opts.LabelSelector != nil { - reqs, selectable := opts.LabelSelector.Requirements() - if !selectable { - // labels.Nothing() and other non-selectable selectors match no objects. - return emptyList(), nil - } - if len(reqs) > 0 { - ls = ls.Add(reqs...) - } + if !selectable { + // labels.Nothing() and other non-selectable selectors match no objects. + return emptyList(), nil } // Parse field selector for manual filtering @@ -445,23 +457,12 @@ func (r *REST) Watch(ctx context.Context, opts *metainternal.ListOptions) (watch return nil, err } - // Build the same selector as List() does: required tenant-resource label - // plus any user-provided requirements. - ls := labels.NewSelector() - tsReq, _ := labels.NewRequirement(tsLabelKey, selection.Equals, []string{tsLabelValue}) - ls = ls.Add(*tsReq) - - if opts.LabelSelector != nil { - reqs, selectable := opts.LabelSelector.Requirements() - if !selectable { - // labels.Nothing(): match no objects, return a watcher that closes immediately. - ch := make(chan watch.Event) - close(ch) - return watch.NewProxyWatcher(ch), nil - } - if len(reqs) > 0 { - ls = ls.Add(reqs...) - } + ls, selectable := buildTenantSelector(opts) + if !selectable { + // labels.Nothing(): match no objects, return a watcher that closes immediately. + ch := make(chan watch.Event) + close(ch) + return watch.NewProxyWatcher(ch), nil } secList := &corev1.SecretList{} @@ -517,7 +518,11 @@ func (r *REST) Watch(ctx context.Context, opts *metainternal.ListOptions) (watch // 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)) { + // DELETED events must always pass through: when a Secret's labels mutate + // out of the selector, the apiserver synthesizes a DELETED with the new + // (non-matching) labels — dropping it would leave cached clients with + // stale entries. + if ev.Type != watch.Deleted && !ls.Matches(labels.Set(sec.Labels)) { continue } diff --git a/pkg/registry/core/tenantsecret/rest_test.go b/pkg/registry/core/tenantsecret/rest_test.go index a010c589..3e53071f 100644 --- a/pkg/registry/core/tenantsecret/rest_test.go +++ b/pkg/registry/core/tenantsecret/rest_test.go @@ -223,6 +223,39 @@ func TestList_WithEverythingSelector_BehavesLikeNoSelector(t *testing.T) { } } +func TestList_WithNothingSelector_ReturnsEmpty(t *testing.T) { + r := newTestREST(t, makeTenantSecret("bucket-creds", map[string]string{ + "apps.cozystack.io/application.kind": "Bucket", + })) + + list := listTenantSecrets(t, r, &metainternal.ListOptions{LabelSelector: labels.Nothing()}) + + if len(list.Items) != 0 { + t.Fatalf("expected empty list for Nothing() selector, got %v", itemNames(list.Items)) + } +} + +func TestWatch_WithNothingSelector_ClosesImmediately(t *testing.T) { + r := newTestREST(t) + + ctx, cancel := context.WithCancel(request.WithNamespace(context.Background(), testNamespace)) + defer cancel() + + w, err := r.Watch(ctx, &metainternal.ListOptions{LabelSelector: labels.Nothing()}) + if err != nil { + t.Fatalf("Watch returned error: %v", err) + } + + select { + case _, ok := <-w.ResultChan(): + if ok { + t.Fatal("expected closed channel for Nothing() selector, got an event") + } + case <-time.After(500 * time.Millisecond): + t.Fatal("timed out waiting for channel to close") + } +} + // TestWatch_WithLabelSelector_FiltersEvents reproduces issue // cozystack/cozystack#2527: TenantSecret Watch ignored opts.LabelSelector // and streamed every tenant Secret in the namespace, regardless of the