From 578c583c736322e02552dbb855806d588be15594 Mon Sep 17 00:00:00 2001 From: Jonny Date: Wed, 9 Sep 2026 15:53:04 +0100 Subject: [PATCH 1/2] fix: treat no-op SSA apply as cache-sync success Re-applying an unchanged ValidatingAdmissionPolicy left resourceVersion the same, so blockApply waited 2s for a newer RV and killed the leader. Co-authored-by: Cursor --- pkg/util/blockingcacheclient/client.go | 27 ++------- pkg/util/blockingcacheclient/client_test.go | 66 +++++++++++++++++++++ 2 files changed, 72 insertions(+), 21 deletions(-) create mode 100644 pkg/util/blockingcacheclient/client_test.go diff --git a/pkg/util/blockingcacheclient/client.go b/pkg/util/blockingcacheclient/client.go index e869ef2ba9..973c739c90 100644 --- a/pkg/util/blockingcacheclient/client.go +++ b/pkg/util/blockingcacheclient/client.go @@ -215,11 +215,11 @@ func (c *CacheClient) newEmptyObjectFor(from client.Object) (client.Object, erro return created.(client.Object), nil } -// blockApply waits until the applied object appears in the cache with the expected state. +// blockApply waits until the applied object is visible in the cache. // clientObj must be non-nil (caller must have extracted it from the ApplyConfiguration). -// preApplyMeta is the object's metadata from a GET before Apply; if nil (e.g. object did not exist), -// we consider the cache updated once the object exists. Otherwise we compare until UID/Generation/ResourceVersion -// differ so the cache has observed the Apply. +// Apply() has already succeeded against the API. A no-op SSA leaves +// resourceVersion unchanged; requiring a newer RV timed out after 2s and +// crashed the vcluster leader (metrics-server VAP re-apply on every lease). func (c *CacheClient) blockApply(ctx context.Context, obj runtime.ApplyConfiguration, clientObj client.Object, preApplyMeta metav1.Object) error { nn := types.NamespacedName{Namespace: clientObj.GetNamespace(), Name: clientObj.GetName()} newObj, err := c.newEmptyObjectFor(clientObj) @@ -243,23 +243,8 @@ func (c *CacheClient) blockApply(ctx context.Context, obj runtime.ApplyConfigura return false, nil } - if preApplyMeta == nil { - // Object did not exist before Apply; it now exists in cache. - return true, nil - } - - newAccessor, err := meta.Accessor(newObj) - if err != nil { - return false, err - } - // Cache has applied state when UID/Generation/ResourceVersion changed from pre-apply. - // Condition 1: UID changed - object was deleted and recreated - // Condition 2: Generation increased - spec was updated - // Condition 3: ResourceVersion changed - any update occurred (metadata, spec, or status) - // If any of these conditions are true, the Apply operation is reflected in the cache. - return preApplyMeta.GetUID() != newAccessor.GetUID() || - newAccessor.GetGeneration() > preApplyMeta.GetGeneration() || - newAccessor.GetResourceVersion() != preApplyMeta.GetResourceVersion(), nil + // Visible in cache. That is enough for create, update, and no-op SSA. + return true, nil }) } diff --git a/pkg/util/blockingcacheclient/client_test.go b/pkg/util/blockingcacheclient/client_test.go new file mode 100644 index 0000000000..ca5c935352 --- /dev/null +++ b/pkg/util/blockingcacheclient/client_test.go @@ -0,0 +1,66 @@ +package blockingcacheclient + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +func TestBlockApplyNoopDoesNotTimeout(t *testing.T) { + obj := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "vcluster-protected-apiservices", + UID: "uid-1", + ResourceVersion: "1", + Generation: 1, + }, + } + + inner := fake.NewClientBuilder().WithObjects(obj.DeepCopy()).Build() + c := &CacheClient{Client: inner, scheme: inner.Scheme()} + + pre := obj.DeepCopy() + if err := c.blockApply(context.Background(), nil, obj, pre); err != nil { + t.Fatalf("no-op apply with unchanged resourceVersion should succeed, got %v", err) + } +} + +func TestBlockApplyWaitsUntilCreated(t *testing.T) { + obj := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + ObjectMeta: metav1.ObjectMeta{Name: "created"}, + } + + inner := fake.NewClientBuilder().Build() + gets := 0 + wrapped := interceptor.NewClient(inner, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key types.NamespacedName, out client.Object, opts ...client.GetOption) error { + gets++ + if gets >= 3 { + if err := inner.Create(ctx, obj.DeepCopy()); err != nil { + return err + } + } + return inner.Get(ctx, key, out, opts...) + }, + }) + c := &CacheClient{Client: wrapped, scheme: runtime.NewScheme()} + if err := corev1.AddToScheme(c.scheme); err != nil { + t.Fatal(err) + } + + if err := c.blockApply(context.Background(), nil, obj, nil); err != nil { + t.Fatalf("create should succeed once the object appears, got %v", err) + } + if gets < 3 { + t.Fatalf("expected polling before create, got %d gets", gets) + } +} From 5b2d1ce3aaaf67ffc28e1511d834f5adfb2e076a Mon Sep 17 00:00:00 2001 From: Jonny Date: Wed, 9 Sep 2026 15:57:32 +0100 Subject: [PATCH 2/2] fix: only accept no-op SSA apply after the cache wait times out Keep the resourceVersion catch-up wait for real applies. If the poll expires and the object is still there, Apply already succeeded. Co-authored-by: Cursor --- pkg/util/blockingcacheclient/client.go | 34 ++++++++++++++++++++------ 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/pkg/util/blockingcacheclient/client.go b/pkg/util/blockingcacheclient/client.go index 973c739c90..e85abcf81c 100644 --- a/pkg/util/blockingcacheclient/client.go +++ b/pkg/util/blockingcacheclient/client.go @@ -215,11 +215,13 @@ func (c *CacheClient) newEmptyObjectFor(from client.Object) (client.Object, erro return created.(client.Object), nil } -// blockApply waits until the applied object is visible in the cache. +// blockApply waits until the applied object appears in the cache with the expected state. // clientObj must be non-nil (caller must have extracted it from the ApplyConfiguration). -// Apply() has already succeeded against the API. A no-op SSA leaves -// resourceVersion unchanged; requiring a newer RV timed out after 2s and -// crashed the vcluster leader (metrics-server VAP re-apply on every lease). +// preApplyMeta is the object's metadata from a GET before Apply; if nil (e.g. object did not exist), +// we consider the cache updated once the object exists. Otherwise we wait until +// UID/Generation/ResourceVersion differ so the cache has observed the Apply. +// A no-op SSA leaves those unchanged, so the wait times out; if the object still +// exists, Apply() already succeeded and we treat that as synced. func (c *CacheClient) blockApply(ctx context.Context, obj runtime.ApplyConfiguration, clientObj client.Object, preApplyMeta metav1.Object) error { nn := types.NamespacedName{Namespace: clientObj.GetNamespace(), Name: clientObj.GetName()} newObj, err := c.newEmptyObjectFor(clientObj) @@ -227,7 +229,7 @@ func (c *CacheClient) blockApply(ctx context.Context, obj runtime.ApplyConfigura return err } - return wait.PollUntilContextTimeout(ctx, time.Millisecond*10, time.Second*2, true, func(context.Context) (bool, error) { + err = wait.PollUntilContextTimeout(ctx, time.Millisecond*10, time.Second*2, true, func(context.Context) (bool, error) { err := c.Client.Get(ctx, nn, newObj) if err != nil { if runtime.IsNotRegisteredError(err) { @@ -243,9 +245,27 @@ func (c *CacheClient) blockApply(ctx context.Context, obj runtime.ApplyConfigura return false, nil } - // Visible in cache. That is enough for create, update, and no-op SSA. - return true, nil + if preApplyMeta == nil { + // Object did not exist before Apply; it now exists in cache. + return true, nil + } + + newAccessor, err := meta.Accessor(newObj) + if err != nil { + return false, err + } + // Cache has applied state when UID/Generation/ResourceVersion changed from pre-apply. + return preApplyMeta.GetUID() != newAccessor.GetUID() || + newAccessor.GetGeneration() > preApplyMeta.GetGeneration() || + newAccessor.GetResourceVersion() != preApplyMeta.GetResourceVersion(), nil }) + if err == nil { + return nil + } + if getErr := c.Client.Get(ctx, nn, newObj); getErr == nil { + return nil + } + return err } // TODO: implement DeleteAllOf