fix: recover from stale-cache conflict on ConstraintTemplatePodStatus update

The ConstraintTemplate reconciler reads the per-pod ConstraintTemplatePodStatus
through the controller-runtime informer cache and then writes it back with the
delegating client. When the same ConstraintTemplate is reconciled twice in
rapid succession (e.g. a CT update followed by the owned-CRD update event
re-enqueuing the CT), the cache can briefly serve a stale resourceVersion and
the next Update fails with a 409 Conflict, producing a noisy
"update ct pod status error" log on every occurrence.

Wrap the three PodStatus Update call sites in a retry helper that, on
Conflict, refetches the PodStatus via mgr.GetAPIReader() (uncached) to avoid
looping against the same stale cache view, re-applies the desired Status, and
retries. Mirrors the retry-on-conflict pattern already used in generateCRD
for the ConstraintTemplate annotation update.

Fixes #4595

Author: a7i

pkg/controller/constrainttemplate/constrainttemplate_controller.go

@@ -193,6 +193,7 @@ func newReconciler(mgr manager.Manager, cfClient *constraintclient.Client, wm *w
 	r := newStatsReporter()
 	reconciler := &ReconcileConstraintTemplate{
 		Client:          mgr.GetClient(),
+		apiReader:       mgr.GetAPIReader(),
 		scheme:          mgr.GetScheme(),
 		cfClient:        cfClient,
 		watcher:         w,
@@ -284,6 +285,10 @@ var _ reconcile.Reconciler = &ReconcileConstraintTemplate{}
 // ReconcileConstraintTemplate reconciles a ConstraintTemplate object.
 type ReconcileConstraintTemplate struct {
 	client.Client
+	// apiReader is an uncached reader used to refetch the latest version of a ConstraintTemplatePodStatus when an
+	// optimistic-concurrency conflict is hit on update. The default reconciler client reads through the informer cache,
+	// which can briefly return a stale resourceVersion right after a successful write and cause spurious 409 conflicts.
+	apiReader       client.Reader
 	scheme          *runtime.Scheme
 	watcher         *watch.Registrar
 	statusWatcher   *watch.Registrar
@@ -394,7 +399,7 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
 		createErr := &v1beta1.CreateCRDError{Code: ErrCreateCode, Message: err.Error()}
 		status.Status.Errors = append(status.Status.Errors, createErr)
 
-		if updateErr := r.Update(ctx, status); updateErr != nil {
+		if updateErr := r.updatePodStatusWithRetry(ctx, status); updateErr != nil {
 			logger.Error(updateErr, "update status error")
 			return reconcile.Result{Requeue: true}, nil
 		}
@@ -454,7 +459,7 @@ func (r *ReconcileConstraintTemplate) reportErrorOnCTStatus(ctx context.Context,
 		Message: fmt.Sprintf("%s: %s", message, err),
 	}
 	status.Status.Errors = append(status.Status.Errors, createErr)
-	if err2 := r.Update(ctx, status); err2 != nil {
+	if err2 := r.updatePodStatusWithRetry(ctx, status); err2 != nil {
 		return errorpkg.Wrap(err, fmt.Sprintf("Could not update status: %s", err2))
 	}
 	return err
@@ -522,7 +527,7 @@ func (r *ReconcileConstraintTemplate) handleUpdate(
 	if err != nil {
 		return reconcile.Result{}, err
 	}
-	if err := r.Update(ctx, status); err != nil {
+	if err := r.updatePodStatusWithRetry(ctx, status); err != nil {
 		logger.Error(err, "update ct pod status error")
 		return reconcile.Result{Requeue: true}, nil
 	}
@@ -587,6 +592,38 @@ func (r *ReconcileConstraintTemplate) deleteAllStatus(ctx context.Context, ctNam
 	return nil
 }
 
+// updatePodStatusWithRetry updates a ConstraintTemplatePodStatus and transparently recovers from optimistic-concurrency
+// conflicts caused by a stale informer cache. The reconciler's default client reads through the cache, so when the same
+// ConstraintTemplate is reconciled twice in rapid succession (for example: CT update followed by the owned-CRD update
+// event re-enqueuing the CT) the second reconcile can observe a PodStatus with an older resourceVersion than the one
+// already on the API server, and the subsequent Update fails with a 409 Conflict.
+//
+// On Conflict we refetch the PodStatus via the uncached APIReader, re-apply our desired Status, and attempt the Update
+// once more inline before yielding to RetryOnConflict's backoff. This collapses the common case (stale cache, no other
+// writer racing us) into a single fast retry. If that inline attempt still conflicts, the error is returned to
+// RetryOnConflict which will back off and try again. Non-conflict errors are returned as-is. The supplied status
+// pointer is updated in place with the latest object so callers can continue to mutate it after a successful return.
+func (r *ReconcileConstraintTemplate) updatePodStatusWithRetry(ctx context.Context, status *statusv1beta1.ConstraintTemplatePodStatus) error {
+	desiredStatus := status.Status
+	key := client.ObjectKeyFromObject(status)
+	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		err := r.Update(ctx, status)
+		if err == nil {
+			return nil
+		}
+		if !apierrors.IsConflict(err) {
+			return err
+		}
+		latest := &statusv1beta1.ConstraintTemplatePodStatus{}
+		if getErr := r.apiReader.Get(ctx, key, latest); getErr != nil {
+			return getErr
+		}
+		latest.Status = desiredStatus
+		*status = *latest
+		return r.Update(ctx, status)
+	})
+}
+
 func (r *ReconcileConstraintTemplate) getOrCreatePodStatus(ctx context.Context, ctName string) (*statusv1beta1.ConstraintTemplatePodStatus, error) {
 	statusObj := &statusv1beta1.ConstraintTemplatePodStatus{}
 	sName, err := statusv1beta1.KeyForConstraintTemplate(util.GetPodName(), ctName)

pkg/controller/constrainttemplate/podstatus_retry_test.go

@@ -0,0 +1,189 @@
+/*
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package constrainttemplate
+
+import (
+	"context"
+	"testing"
+
+	"github.com/open-policy-agent/frameworks/constraint/pkg/apis/templates/v1beta1"
+	statusv1beta1 "github.com/open-policy-agent/gatekeeper/v3/apis/status/v1beta1"
+	"github.com/stretchr/testify/require"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	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"
+)
+
+// staleClient wraps a controller-runtime client and serves Get requests from a snapshot taken at construction time,
+// while forwarding Update/Create/Delete/List to the real (live) client. This simulates a controller-runtime informer
+// cache that has not yet observed the latest write to the backing store, which is the underlying cause of the spurious
+// "the object has been modified" conflicts we hit in the ConstraintTemplate reconciler.
+type staleClient struct {
+	client.Client
+	stale map[client.ObjectKey]*statusv1beta1.ConstraintTemplatePodStatus
+}
+
+func (s *staleClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+	if cts, ok := obj.(*statusv1beta1.ConstraintTemplatePodStatus); ok {
+		if cached, ok := s.stale[key]; ok {
+			cached.DeepCopyInto(cts)
+			return nil
+		}
+	}
+	return s.Client.Get(ctx, key, obj, opts...)
+}
+
+func newPodStatusForTest(name string) *statusv1beta1.ConstraintTemplatePodStatus {
+	return &statusv1beta1.ConstraintTemplatePodStatus{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: statusv1beta1.GroupVersion.String(),
+			Kind:       "ConstraintTemplatePodStatus",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: "gatekeeper-system",
+		},
+	}
+}
+
+func newSchemeForTest(t *testing.T) *runtime.Scheme {
+	t.Helper()
+	scheme := runtime.NewScheme()
+	require.NoError(t, statusv1beta1.AddToScheme(scheme))
+	require.NoError(t, v1beta1.AddToScheme(scheme))
+	return scheme
+}
+
+// TestUpdatePodStatusWithRetry_StaleCacheRecovers reproduces the race that produced "update ct pod status error" in
+// production: the reconciler's cached client returns a stale resourceVersion for the PodStatus, so the first Update
+// returns a 409 Conflict; the helper must refetch via the uncached APIReader and successfully retry.
+func TestUpdatePodStatusWithRetry_StaleCacheRecovers(t *testing.T) {
+	ctx := context.Background()
+	scheme := newSchemeForTest(t)
+
+	initial := newPodStatusForTest("pod-template-status")
+	initial.Status.ID = "initial"
+
+	// Live store: the API-server-equivalent that holds the latest resourceVersion.
+	live := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(initial.DeepCopy()).
+		Build()
+
+	// Capture the resourceVersion the fake client assigned to the initial object on Build(). We can't read it off
+	// `initial` because WithObjects deep-copies its input before stamping a resourceVersion on the stored copy.
+	storedBefore := &statusv1beta1.ConstraintTemplatePodStatus{}
+	require.NoError(t, live.Get(ctx, client.ObjectKeyFromObject(initial), storedBefore))
+	staleResourceVersion := storedBefore.ResourceVersion
+	require.NotEmpty(t, staleResourceVersion, "test sanity: fake client must stamp a resourceVersion on stored objects")
+
+	// Simulate an out-of-band update advancing the resourceVersion on the API server while the controller's informer
+	// cache still serves the old version.
+	latestOnAPIServer := storedBefore.DeepCopy()
+	latestOnAPIServer.Status.ID = "out-of-band"
+	require.NoError(t, live.Update(ctx, latestOnAPIServer))
+	require.NotEqual(t, staleResourceVersion, latestOnAPIServer.ResourceVersion, "test sanity: Update must bump resourceVersion")
+
+	// The stale informer view: same spec the cache observed at storedBefore, pinned to the older resourceVersion.
+	cachedView := storedBefore.DeepCopy()
+	cachedView.ResourceVersion = staleResourceVersion
+
+	cached := &staleClient{
+		Client: live,
+		stale: map[client.ObjectKey]*statusv1beta1.ConstraintTemplatePodStatus{
+			client.ObjectKeyFromObject(initial): cachedView,
+		},
+	}
+
+	r := &ReconcileConstraintTemplate{
+		Client:    cached,
+		apiReader: live,
+		scheme:    scheme,
+	}
+
+	// The reconciler reads the PodStatus through the cached client and gets the stale version, then mutates and tries
+	// to write. The first Update must fail with Conflict (server side rejects the old resourceVersion); the helper must
+	// recover via the uncached apiReader.
+	status := &statusv1beta1.ConstraintTemplatePodStatus{}
+	require.NoError(t, cached.Get(ctx, client.ObjectKeyFromObject(initial), status))
+	require.Equal(t, staleResourceVersion, status.ResourceVersion, "test sanity: cached view is pinned to the older resourceVersion")
+	require.Equal(t, "initial", status.Status.ID, "test sanity: cached view still reflects the pre-out-of-band spec")
+	status.Status.ID = "desired-by-reconciler"
+
+	require.NoError(t, r.updatePodStatusWithRetry(ctx, status))
+
+	final := &statusv1beta1.ConstraintTemplatePodStatus{}
+	require.NoError(t, live.Get(ctx, client.ObjectKeyFromObject(initial), final))
+	require.Equal(t, "desired-by-reconciler", final.Status.ID, "desired status must be applied on top of the latest resourceVersion")
+}
+
+// TestUpdatePodStatusWithRetry_NonConflictErrorReturned ensures that errors other than 409 Conflict are returned to the
+// caller unchanged and not silently swallowed by the retry loop.
+func TestUpdatePodStatusWithRetry_NonConflictErrorReturned(t *testing.T) {
+	ctx := context.Background()
+	scheme := newSchemeForTest(t)
+
+	live := fake.NewClientBuilder().WithScheme(scheme).Build()
+
+	r := &ReconcileConstraintTemplate{
+		Client:    live,
+		apiReader: live,
+		scheme:    scheme,
+	}
+
+	// PodStatus does not exist in the live store, so Update must fail with NotFound. The helper must surface that error
+	// without retrying.
+	missing := newPodStatusForTest("missing-status")
+	missing.Status.ID = "anything"
+
+	err := r.updatePodStatusWithRetry(ctx, missing)
+	require.Error(t, err)
+	require.True(t, apierrors.IsNotFound(err), "expected NotFound, got %v", err)
+}
+
+// TestUpdatePodStatusWithRetry_HappyPath ensures that a normal update with the latest resourceVersion succeeds without
+// any retry overhead.
+func TestUpdatePodStatusWithRetry_HappyPath(t *testing.T) {
+	ctx := context.Background()
+	scheme := newSchemeForTest(t)
+
+	initial := newPodStatusForTest("happy-status")
+	initial.Status.ID = "before"
+
+	live := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(initial.DeepCopy()).
+		Build()
+
+	r := &ReconcileConstraintTemplate{
+		Client:    live,
+		apiReader: live,
+		scheme:    scheme,
+	}
+
+	current := &statusv1beta1.ConstraintTemplatePodStatus{}
+	require.NoError(t, live.Get(ctx, client.ObjectKeyFromObject(initial), current))
+	current.Status.ID = "after"
+
+	require.NoError(t, r.updatePodStatusWithRetry(ctx, current))
+
+	final := &statusv1beta1.ConstraintTemplatePodStatus{}
+	require.NoError(t, live.Get(ctx, types.NamespacedName{Name: initial.Name, Namespace: initial.Namespace}, final))
+	require.Equal(t, "after", final.Status.ID)
+}