🤖 feat: write a cleanup record before each CoderAIGatewayKey create (#353 part 2c)

docs/how-to/manage-ai-gateway-keys.md

@@ -137,14 +137,22 @@ The finalizer stays while Coder cannot confirm the deletion:
 | `ControlPlaneNotReady`, `OperatorAccessNotReady` | Every 60 seconds |
 | `LicenseRequired`, `CoderRejected`, `NotSupported` (`Stalled=True`) | Every 10 minutes |
 
-When the control plane is gone, nothing can delete the keys. The finalizer goes at once. The operator logs `Releasing the finalizer without deleting the keys in Coder` with reason `ControlPlaneGone` and the key IDs: delete those keys with `coder ai-gateway keys delete`.
+When the control plane is gone, nothing can delete the keys. The finalizer goes at once, and the [cleanup record](#cleanup-records) stays. The operator logs `Releasing the finalizer without deleting the keys in Coder` with reason `ControlPlaneGone` and the key IDs: delete those keys with `coder ai-gateway keys delete`.
 
 When you delete the control plane while `CoderAIGatewayKey` or `CoderAIProvider` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes: it keeps its finalizer, so Coder keeps running, and the objects delete their keys and providers as usual, with retries. The operator log shows `Waiting for the objects that use this control plane` with their names. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane (or within 30 seconds). After 10 minutes, the objects that remain get at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for their bounded pass below, and then the control plane goes anyway. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait, because Kubernetes then deletes the Coder Deployment first.
 
 When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed) and its operator access still works, the operator runs one pass of at most 10 seconds without the settle window, then removes the finalizer whatever the answer. A key create that was still in flight at that moment can commit later, and its key stays in Coder. When the pass fails, the log lists the IDs that it did not delete.
 
 Each release without a delete counts in the controller metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`), next to the log line.
 
+### Cleanup records
+
+Before each key create, the operator writes a cleanup record: a ConfigMap named `ck8s-cleanup-<object UID>` with the label `coder.com/cleanup-record=CoderAIGatewayKey` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the key. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the key name prefix `ck8s-<UID without dashes>-` (the `nonce` field), the time of the last create, and the current key ID once Coder answered it. The deployment ID lives in the Coder database, so it names the database that holds the keys.
+
+When the control plane later reaches another Coder deployment (for example after its database Secret changed), the record still locates the keys in the old one. The operator then stalls the object with `CoderDeploymentChanged` at its next key create, keeps the record and creates no key in the new deployment. A delete of the object then removes the finalizer without a delete in Coder. Delete the keys in the old deployment, then delete the ConfigMap.
+
+The operator removes the record only after a list in the record's deployment shows no key of the object: at the end of a normal delete, or after the one bounded pass when the last create was more than 5 minutes before it. In every other release without a delete, the record stays and names where the keys are. Find the records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record=CoderAIGatewayKey`. The operator gets write access to ConfigMaps only in that namespace. Do not give other users write access to ConfigMaps there.
+
 !!! warning "Delete the keys before the namespace"
     When you delete the namespace, Kubernetes deletes the control plane and the `CoderAIGatewayKey` objects at the same time. The control plane is usually gone before the operator handles the objects, so the finalizers go without a delete, and the keys stay valid in Coder. The namespace deletion finishes, and the operator logs the key IDs with reason `ControlPlaneGone`.
 

internal/app/controllerapp/controllerapp.go

@@ -153,6 +153,8 @@ func SetupControllers(mgr manager.Manager) error {
 		APIReader: mgr.GetAPIReader(),
 		Scheme:    managerScheme,
 		Clock:     clock.RealClock{},
+		// The same namespace as the provider records, so one sweep finds both.
+		OperatorNamespace: aiProviderReconciler.OperatorNamespace,
 	}
 	if err := aiGatewayKeyReconciler.SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("unable to create AI Gateway key controller: %w", err)

internal/controller/coderaigatewaykey_controller.go

@@ -83,6 +83,8 @@ type CoderAIGatewayKeyReconciler struct {
 	APIReader client.Reader
 	Scheme    *runtime.Scheme
 	Clock     clock.PassiveClock
+	// OperatorNamespace holds the cleanup records (#353).
+	OperatorNamespace string
 
 	rateLimited coderBackoff // Keyed by types.NamespacedName.
 }
@@ -112,8 +114,8 @@ func aiGatewayKeyStalled(reason, format string, args ...any) *aiGatewayKeyStep {
 // the output Secret, or of any control plane in the namespace (any of them
 // can reserve the Secret name), requeues the object at once.
 func (r *CoderAIGatewayKeyReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	if mgr == nil || r.Client == nil || r.APIReader == nil || r.Scheme == nil || r.Clock == nil {
-		return errors.New("assertion failed: AI Gateway key reconciler needs a manager, a client, an API reader, a scheme, and a clock")
+	if mgr == nil || r.Client == nil || r.APIReader == nil || r.Scheme == nil || r.Clock == nil || r.OperatorNamespace == "" {
+		return errors.New("assertion failed: AI Gateway key reconciler needs a manager, a client, an API reader, a scheme, a clock, and the operator namespace")
 	}
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&coderv1alpha1.CoderAIGatewayKey{}).
@@ -139,8 +141,8 @@ func (r *CoderAIGatewayKeyReconciler) keysInNamespace(ctx context.Context, obj c
 // Reconcile handles deletion first, stores the finalizer before any Coder
 // call, then runs the observe step and at most one action.
 func (r *CoderAIGatewayKeyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	if r.Client == nil || r.APIReader == nil || r.Scheme == nil || r.Clock == nil {
-		return ctrl.Result{}, errors.New("assertion failed: AI Gateway key reconciler needs a client, an API reader, a scheme, and a clock")
+	if r.Client == nil || r.APIReader == nil || r.Scheme == nil || r.Clock == nil || r.OperatorNamespace == "" {
+		return ctrl.Result{}, errors.New("assertion failed: AI Gateway key reconciler needs a client, an API reader, a scheme, a clock, and the operator namespace")
 	}
 	// Only a wait after HTTP 429 keeps the backoff count.
 	keepBackoff := false
@@ -273,6 +275,7 @@ func (r *CoderAIGatewayKeyReconciler) observe(
 		// Coder lists the current key, so no kill switch holds. A stale
 		// generation would re-arm a later kill switch at once.
 		key.Status.KillSwitchGeneration = 0
+		r.recordAIGatewayKeyID(ctx, sdk, key, prefix)
 	}
 	if stall != nil {
 		// The Secret is not this object's, but its live value still decides

internal/controller/coderaigatewaykey_controller_test.go

@@ -35,6 +35,7 @@ const testAIGatewaySecret = "coder-ai-gateway-key"
 // plane named coder that points at it.
 type aiGatewayKeyEnv struct {
 	namespace string
+	records   string // The operator namespace, which holds the cleanup records.
 	fake      *fakeCoder
 	client    client.Client // The reconciler's client. Tests can wrap it.
 	clock     *clocktesting.FakeClock
@@ -44,7 +45,7 @@ func newAIGatewayKeyEnv(ctx context.Context, t *testing.T, allowKeys *bool) aiGa
 	t.Helper()
 	// Status times have second precision, so the fake clock starts on a second.
 	env := aiGatewayKeyEnv{
-		namespace: createTestNamespace(ctx, t, "aigk"), fake: newFakeCoder(t), client: k8sClient,
+		namespace: createTestNamespace(ctx, t, "aigk"), records: createTestNamespace(ctx, t, "aigk-ops"), fake: newFakeCoder(t), client: k8sClient,
 		clock: clocktesting.NewFakeClock(time.Now().Truncate(time.Second)),
 	}
 	cp := createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL)
@@ -93,7 +94,7 @@ func (env aiGatewayKeyEnv) reconcile(ctx context.Context, t *testing.T, key *cod
 // metadata, the logs or the error (plan S3).
 func (env aiGatewayKeyEnv) reconcileErr(ctx context.Context, t *testing.T, key *coderv1alpha1.CoderAIGatewayKey) (*coderv1alpha1.CoderAIGatewayKey, ctrl.Result, error) {
 	t.Helper()
-	r := &controller.CoderAIGatewayKeyReconciler{Client: env.client, APIReader: env.client, Scheme: scheme, Clock: env.clock}
+	r := &controller.CoderAIGatewayKeyReconciler{Client: env.client, APIReader: env.client, Scheme: scheme, Clock: env.clock, OperatorNamespace: env.records}
 	logCtx, logs := capturingContext(ctx)
 	result, reconcileErr := r.Reconcile(logCtx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(key)})
 	got := &coderv1alpha1.CoderAIGatewayKey{}
@@ -104,6 +105,10 @@ func (env aiGatewayKeyEnv) reconcileErr(ctx context.Context, t *testing.T, key *
 		require.NoError(t, err)
 	}
 	haystacks := map[string]any{"logs": logs.String(), "error": fmt.Sprint(reconcileErr)}
+	record := &corev1.ConfigMap{}
+	if err := k8sClient.Get(ctx, client.ObjectKey{Namespace: env.records, Name: "ck8s-cleanup-" + string(key.UID)}, record); err == nil {
+		haystacks["cleanup record"] = record.Data // #353 invariant 2.
+	}
 	if got != nil {
 		haystacks["status"] = got.Status
 		secret := &corev1.Secret{}
@@ -361,7 +366,9 @@ func TestCoderAIGatewayKeyIssuesFirstKey(t *testing.T) {
 	require.Zero(t, writes)
 	require.Equal(t, got.ResourceVersion, again.ResourceVersion)
 	require.Equal(t, 2, env.fake.requestCount(routeAIGatewayKeys))
-	require.Equal(t, 3, env.fake.totalRequests())
+	// The deployment ID read before the create is for the cleanup record (#353).
+	require.Equal(t, 1, env.fake.requestCount(routeBuildInfo))
+	require.Equal(t, 4, env.fake.totalRequests())
 }
 
 func requireAIGatewayKeyReady(t *testing.T, key *coderv1alpha1.CoderAIGatewayKey) {
@@ -552,7 +559,7 @@ func TestCoderAIGatewayKeyWatches(t *testing.T) {
 	})
 	require.NoError(t, err)
 	require.NoError(t, (&controller.CoderAIGatewayKeyReconciler{
-		Client: mgr.GetClient(), APIReader: mgr.GetAPIReader(), Scheme: mgr.GetScheme(), Clock: env.clock,
+		Client: mgr.GetClient(), APIReader: mgr.GetAPIReader(), Scheme: mgr.GetScheme(), Clock: env.clock, OperatorNamespace: env.records,
 	}).SetupWithManager(mgr))
 	mgrCtx, cancel := context.WithCancel(ctx)
 	mgrDone := make(chan error, 1)

internal/controller/coderaigatewaykey_delete.go

@@ -32,7 +32,9 @@ const (
 // release when the finalizer can go. Otherwise the step says why it stays.
 func (r *CoderAIGatewayKeyReconciler) deleteStep(ctx context.Context, key *coderv1alpha1.CoderAIGatewayKey) (release bool, step *aiGatewayKeyStep, err error) {
 	if key.Status.NextSerial == 0 {
-		return true, nil, nil // No create was ever sent, so no key of this object can exist.
+		// No create was ever sent, so no key of this object can exist. A
+		// record can, when the marker write after it failed.
+		return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, key.UID)
 	}
 	ref := types.NamespacedName{Namespace: key.Namespace, Name: key.Spec.ControlPlaneRef.Name}
 	controlPlane := &coderv1alpha1.CoderControlPlane{}
@@ -53,9 +55,17 @@ func (r *CoderAIGatewayKeyReconciler) deleteStep(ctx context.Context, key *coder
 		}
 		deleting = !holds
 		if holds {
+			// A pass under the hold ends at the hold end, however slowly
+			// Coder answers, so the bounded pass after it still fits in the
+			// control plane's grace period (#384 follow-up 4).
+			var cancel context.CancelFunc
+			ctx, cancel = context.WithDeadline(ctx, controlPlaneHoldEnd(controlPlane))
+			defer cancel()
 			defer func() {
 				if step != nil {
-					step.requeue = controlPlaneHoldRequeue(controlPlane, now, step.requeue)
+					// From the end of the pass: a pass that ran until the
+					// deadline retries right after the hold end.
+					step.requeue = controlPlaneHoldRequeue(controlPlane, r.Clock.Now(), step.requeue)
 				}
 			}()
 		}
@@ -76,12 +86,25 @@ func (r *CoderAIGatewayKeyReconciler) deleteStep(ctx context.Context, key *coder
 		var cancel context.CancelFunc
 		ctx, cancel = context.WithTimeout(ctx, aiGatewayKeyBoundedDeleteTimeout)
 		defer cancel()
+		if other, wait, err := r.aiGatewayKeyRecordDeployment(ctx, sdk, key); err != nil {
+			return false, nil, err
+		} else if wait != nil || other != "" {
+			return r.releaseWithoutDelete(ctx, key, "the deployment check while CoderControlPlane is being deleted failed: "+other+aiGatewayKeyStepMessage(wait)), nil, nil
+		}
 		if _, left, failure, err := r.deleteKeys(ctx, sdk, key); err != nil {
 			return false, nil, err
 		} else if failure != nil {
 			return r.releaseWithoutDelete(ctx, key, "the one pass while CoderControlPlane is being deleted failed: "+failure.message, left...), nil, nil
 		}
-		return true, nil, nil
+		if last := key.Status.LastCreateAttemptTime; last == nil || r.Clock.Now().Before(last.Add(aiGatewayKeySettleWindow)) {
+			// A create of the last attempt can still commit after this LIST,
+			// so only a sweep after the settle window can prove that no key
+			// is left. The record stays for it.
+			log.FromContext(ctx).Info("Releasing the finalizer after one pass inside the settle window: the cleanup record stays for a sweep",
+				"record", r.OperatorNamespace+"/"+cleanupRecordObjectName(key.UID))
+			return true, nil, nil
+		}
+		return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, key.UID)
 	}
 
 	now := r.Clock.Now()
@@ -96,14 +119,22 @@ func (r *CoderAIGatewayKeyReconciler) deleteStep(ctx context.Context, key *coder
 			"The object is being deleted. The operator deletes its keys in Coder at %s, when no create of the last attempt can still commit.",
 			settleAt.UTC().Format(time.RFC3339)), nil
 	}
+	// The record names the deployment that holds the keys. When the control
+	// plane now reaches another deployment, an empty LIST there proves
+	// nothing, so the record stays for a sweep (#353 invariant 7).
+	if other, wait, err := r.aiGatewayKeyRecordDeployment(ctx, sdk, key); err != nil || wait != nil {
+		return false, wait, err
+	} else if other != "" {
+		return r.releaseWithoutDelete(ctx, key, other), nil, nil
+	}
 	// The pass repeats until a LIST shows no key of this object, so the
-	// release follows a LIST that proves it.
+	// release follows a LIST that proves it. Only then does the record go.
 	for range 2 {
 		deleted, _, failure, err := r.deleteKeys(ctx, sdk, key)
 		if err != nil || failure != nil {
 			return false, failure, err
 		} else if deleted == 0 {
-			return true, nil, nil
+			return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, key.UID)
 		}
 	}
 	return false, aiGatewayKeyWait(aiGatewayKeyDeleteRetry, reasonDeletingKeys,
@@ -153,6 +184,14 @@ func aiGatewayKeyDeleteFailure(action string, err error) *aiGatewayKeyStep {
 	return step
 }
 
+// aiGatewayKeyStepMessage returns the message of step, or "".
+func aiGatewayKeyStepMessage(step *aiGatewayKeyStep) string {
+	if step == nil {
+		return ""
+	}
+	return step.message
+}
+
 // releaseWithoutDelete logs the key IDs that can stay in Coder, because
 // nothing can delete them any more, and returns true (ControlPlaneGone). left
 // holds the IDs that a failed pass listed but did not delete.

internal/controller/coderaigatewaykey_delete_internal_test.go

@@ -0,0 +1,66 @@
+package controller
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/utils/clock"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1"
+)
+
+// #384 follow-up 4 for keys: a delete pass under the control plane hold ends
+// at the hold end, also when Coder does not answer, and its retry counts from
+// the end of the pass. Here the hold ends in 5 s and Coder hangs on the LIST,
+// whose own request timeout is 30 s.
+func TestAIGatewayKeyDeleteUnderHoldEndsAtHoldEnd(t *testing.T) {
+	t.Parallel()
+	coder := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
+		<-r.Context().Done()
+	}))
+	t.Cleanup(coder.Close)
+
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, coderv1alpha1.AddToScheme(scheme))
+	deleted := metav1.NewTime(time.Now().Add(5*time.Second - controlPlaneDependentHold))
+	controlPlane := &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{
+		Name: "coder", Namespace: "ns", DeletionTimestamp: &deleted, Finalizers: []string{workspaceRBACFinalizer},
+	}}
+	controlPlane.Status.URL = coder.URL
+	controlPlane.Status.OperatorAccessReady = true
+	controlPlane.Status.OperatorTokenSecretRef = &coderv1alpha1.SecretKeySelector{Name: "coder-operator-token"}
+	token := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Name: "coder-operator-token", Namespace: "ns"},
+		Data:       map[string][]byte{coderv1alpha1.DefaultTokenSecretKey: []byte("token")},
+	}
+	namespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns"}}
+	c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(controlPlane, token, namespace).Build()
+	r := &CoderAIGatewayKeyReconciler{Client: c, APIReader: c, Scheme: scheme, Clock: clock.RealClock{}, OperatorNamespace: "coder-system"}
+	held, err := controlPlaneHolds(context.Background(), r.Client, controlPlane, time.Now())
+	require.NoError(t, err)
+	require.True(t, held, "assertion failed: the test needs a control plane that holds")
+
+	key := &coderv1alpha1.CoderAIGatewayKey{ObjectMeta: metav1.ObjectMeta{Name: "gateway", Namespace: "ns", UID: testAIGatewayUID}}
+	key.Spec.ControlPlaneRef.Name = "coder"
+	key.Status.NextSerial = 1
+	key.Status.LastCreateAttemptTime = &metav1.Time{Time: time.Now().Add(-time.Hour)}
+	start := time.Now()
+	release, step, err := r.deleteStep(context.Background(), key)
+	require.NoError(t, err)
+	require.Less(t, time.Since(start), 15*time.Second, "the pass ends at the hold end, not at the 30 s request timeout")
+	require.False(t, release, "a pass under the hold never releases without a delete")
+	require.NotNil(t, step)
+	// The requeue counts from the end of the pass: the time left to the hold
+	// end, which is about 0 here, plus 1 s (controlPlaneHoldRequeue).
+	require.LessOrEqual(t, step.requeue, 2*time.Second, "the next pass runs right after the hold end")
+}

internal/controller/coderaigatewaykey_delete_test.go

@@ -178,7 +178,7 @@ func TestCoderAIGatewayKeyDeleteWithControlPlaneGoing(t *testing.T) {
 				require.NoError(t, err)
 				env.fake.failNext(routeDeleteAIGatewayKey, fakeFault{Status: http.StatusBadGateway})
 			}
-			r := &controller.CoderAIGatewayKeyReconciler{Client: env.client, APIReader: env.client, Scheme: scheme, Clock: env.clock}
+			r := &controller.CoderAIGatewayKeyReconciler{Client: env.client, APIReader: env.client, Scheme: scheme, Clock: env.clock, OperatorNamespace: env.records}
 			logCtx, logs := capturingContext(ctx)
 			_, err := r.Reconcile(logCtx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)})
 			require.NoError(t, err)