From 0bb1e03bf7728371e931b714a2f79b1270bcf563 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 21:38:46 +0000 Subject: [PATCH 1/3] feat: write a cleanup record before each CoderAIGatewayKey create (#353 part 2c) Change-Id: I6c7f7e46475e8077e1e05bace6651255696f5782 Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-gateway-keys.md | 10 +- internal/app/controllerapp/controllerapp.go | 2 + .../coderaigatewaykey_controller.go | 11 +- .../coderaigatewaykey_controller_test.go | 15 +- .../controller/coderaigatewaykey_delete.go | 49 +++- .../coderaigatewaykey_delete_internal_test.go | 66 +++++ .../coderaigatewaykey_delete_test.go | 2 +- .../controller/coderaigatewaykey_issue.go | 18 +- .../controller/coderaigatewaykey_record.go | 117 +++++++++ .../coderaigatewaykey_record_test.go | 229 ++++++++++++++++++ 10 files changed, 499 insertions(+), 20 deletions(-) create mode 100644 internal/controller/coderaigatewaykey_delete_internal_test.go create mode 100644 internal/controller/coderaigatewaykey_record.go create mode 100644 internal/controller/coderaigatewaykey_record_test.go diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index 62e98555f..c72343aba 100644 --- a/docs/how-to/manage-ai-gateway-keys.md +++ b/docs/how-to/manage-ai-gateway-keys.md @@ -137,7 +137,7 @@ 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. @@ -145,6 +145,14 @@ When the control plane does not wait (its namespace is being deleted, it is dele 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-` 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--` (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`. diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index 09933be6a..30222b2ca 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/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) diff --git a/internal/controller/coderaigatewaykey_controller.go b/internal/controller/coderaigatewaykey_controller.go index 0413f569b..3d638edae 100644 --- a/internal/controller/coderaigatewaykey_controller.go +++ b/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 diff --git a/internal/controller/coderaigatewaykey_controller_test.go b/internal/controller/coderaigatewaykey_controller_test.go index 2ec30dd04..6f761c5a3 100644 --- a/internal/controller/coderaigatewaykey_controller_test.go +++ b/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) diff --git a/internal/controller/coderaigatewaykey_delete.go b/internal/controller/coderaigatewaykey_delete.go index b76597c6b..9cab414fb 100644 --- a/internal/controller/coderaigatewaykey_delete.go +++ b/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. diff --git a/internal/controller/coderaigatewaykey_delete_internal_test.go b/internal/controller/coderaigatewaykey_delete_internal_test.go new file mode 100644 index 000000000..d27432b9b --- /dev/null +++ b/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") +} diff --git a/internal/controller/coderaigatewaykey_delete_test.go b/internal/controller/coderaigatewaykey_delete_test.go index aa4e838c3..94284eb25 100644 --- a/internal/controller/coderaigatewaykey_delete_test.go +++ b/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) diff --git a/internal/controller/coderaigatewaykey_issue.go b/internal/controller/coderaigatewaykey_issue.go index d8f0e2310..218c72e3f 100644 --- a/internal/controller/coderaigatewaykey_issue.go +++ b/internal/controller/coderaigatewaykey_issue.go @@ -70,15 +70,22 @@ func (r *CoderAIGatewayKeyReconciler) issueKey( return step, err } + // One instant stamps the cleanup record and the marker + // (lastCreateAttemptTime, which starts the settle window) and starts the + // write-age limit, so a slow record or marker write shortens the time left + // for Secret writes, never the window. In production the instant carries + // Go's monotonic reading. + attemptStart := r.Clock.Now() + // The record goes first, so a key that the create makes can always be + // found after the object is gone (#353 invariant 1). + if step, err := r.writeAIGatewayKeyRecord(ctx, sdk, key, prefix, &attemptStart); step != nil || err != nil { + return step, err + } + // The marker and the serial go into one optimistic status update before // the create (plan I3). A conflict stops here, before any request. serial := key.Status.NextSerial name := prefix + strconv.FormatInt(serial, 10) - // One instant stamps the marker (lastCreateAttemptTime, which starts the - // settle window) and starts the write-age limit, so a slow marker write - // shortens the time left for Secret writes, never the window. In - // production the instant carries Go's monotonic reading. - attemptStart := r.Clock.Now() now := metav1.NewTime(attemptStart) key.Status.PendingKey = &coderv1alpha1.CoderAIGatewayKeyPending{Serial: serial, Name: name, CreateAttemptTime: now} key.Status.NextSerial = serial + 1 @@ -134,6 +141,7 @@ func (r *CoderAIGatewayKeyReconciler) issueKey( return nil, err } *before = *key.Status.DeepCopy() + r.recordIssuedKeyID(ctx, key, created.ID.String()) return step, nil } diff --git a/internal/controller/coderaigatewaykey_record.go b/internal/controller/coderaigatewaykey_record.go new file mode 100644 index 000000000..9a010b1d2 --- /dev/null +++ b/internal/controller/coderaigatewaykey_record.go @@ -0,0 +1,117 @@ +package controller + +import ( + "context" + "errors" + "time" + + "github.com/coder/coder/v2/codersdk" + "sigs.k8s.io/controller-runtime/pkg/log" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// cleanupRecordKindAIGatewayKey is the record kind of a CoderAIGatewayKey. +// Its record holds the key name prefix ck8s-- as the nonce, which finds +// every key of the object, and the current key ID once it is known. +const cleanupRecordKindAIGatewayKey = "CoderAIGatewayKey" + +// writeAIGatewayKeyRecord writes the cleanup record of key with the deployment +// ID that Coder answers now. createAttempt is the time of the create that the +// caller sends next, or nil. A failed read or write returns a wait or an +// error, and the caller then sends no create (#353 invariant 1). +func (r *CoderAIGatewayKeyReconciler) writeAIGatewayKeyRecord( + ctx context.Context, sdk *codersdk.Client, key *coderv1alpha1.CoderAIGatewayKey, prefix string, createAttempt *time.Time, +) (*aiGatewayKeyStep, error) { + previous, existing, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, key.UID) + if err != nil { + return nil, err + } + deploymentID, err := coderDeploymentID(ctx, sdk) + if err != nil { + return aiGatewayKeyWait(aiGatewayKeyRetry, "CoderUnavailable", "read the Coder deployment ID for the cleanup record: %s", coderErrorSummary(err)), nil + } + if previous != nil && previous.DeploymentID != deploymentID { + // The keys of this object are in the old Coder database. The record + // is their only locator, so the controller neither overwrites it nor + // creates a key in the new database. + return aiGatewayKeyStalled("CoderDeploymentChanged", + "The keys of this object (names starting with %s) are in Coder deployment %s, but CoderControlPlane %s now reaches deployment %s. Delete the keys there, then delete ConfigMap %s/%s. Until then the operator creates no key.", + prefix, previous.DeploymentID, key.Spec.ControlPlaneRef.Name, deploymentID, r.OperatorNamespace, cleanupRecordObjectName(key.UID)), nil + } + record := cleanupRecord{ + Kind: cleanupRecordKindAIGatewayKey, UID: key.UID, Namespace: key.Namespace, Name: key.Name, + DeploymentID: deploymentID, Nonce: prefix, + } + if current := key.Status.CurrentKey; current != nil { + record.CoderID = current.ID + } + switch { + case createAttempt != nil: + record.CreateAttemptTime = *createAttempt + case key.Status.LastCreateAttemptTime != nil: + record.CreateAttemptTime = key.Status.LastCreateAttemptTime.Time + case previous != nil: + record.CreateAttemptTime = previous.CreateAttemptTime + } + return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) +} + +// recordIssuedKeyID writes the ID of the key that this reconcile created into +// the record that this reconcile wrote before the create, so it needs no new +// deployment ID read. A failure is logged, and recordAIGatewayKeyID retries. +func (r *CoderAIGatewayKeyReconciler) recordIssuedKeyID(ctx context.Context, key *coderv1alpha1.CoderAIGatewayKey, id string) { + record, existing, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, key.UID) + if err == nil && record == nil { + err = errors.New("the cleanup record is gone") + } + if err == nil && record.CoderID != id { + record.CoderID = id + err = writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, *record) + } + if err != nil { + log.FromContext(ctx).Info("Cannot record the new key ID in the cleanup record", "error", err.Error()) + } +} + +// recordAIGatewayKeyID writes the ID of a current key that Coder lists into +// the record. It never stops the reconcile: the record already holds the +// prefix, which finds the key too, so a failure is logged and retried. +func (r *CoderAIGatewayKeyReconciler) recordAIGatewayKeyID(ctx context.Context, sdk *codersdk.Client, key *coderv1alpha1.CoderAIGatewayKey, prefix string) { + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, key.UID) + if err == nil && record != nil && record.CoderID == key.Status.CurrentKey.ID { + return + } + if err == nil { + var step *aiGatewayKeyStep + if step, err = r.writeAIGatewayKeyRecord(ctx, sdk, key, prefix, nil); step != nil { + log.FromContext(ctx).Info("Cannot record the current key ID in the cleanup record", "reason", step.reason, "message", step.message) + return + } + } + if err != nil { + log.FromContext(ctx).Info("Cannot record the current key ID in the cleanup record", "error", err.Error()) + } +} + +// aiGatewayKeyRecordDeployment checks, before a delete pass, that the control +// plane still reaches the Coder deployment that the record names. It returns +// why the pass proves nothing ("" when it does), or a wait when the read +// failed. Without a record (an object from before records) the pass runs. +func (r *CoderAIGatewayKeyReconciler) aiGatewayKeyRecordDeployment(ctx context.Context, sdk *codersdk.Client, key *coderv1alpha1.CoderAIGatewayKey) (string, *aiGatewayKeyStep, error) { + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, key.UID) + if errors.Is(err, errNotACleanupRecord) { + err = nil // Not the operator's ConfigMap: it proves nothing either way. + } + if err != nil || record == nil { + return "", nil, err + } + id, err := coderDeploymentID(ctx, sdk) + if err != nil { + return "", aiGatewayKeyWait(aiGatewayKeyDeleteRetry, "CoderUnavailable", "read the Coder deployment ID: %s", coderErrorSummary(err)), nil + } + if id != record.DeploymentID { + return "CoderControlPlane now reaches Coder deployment " + id + ", but the keys are in deployment " + record.DeploymentID, nil, nil + } + return "", nil, nil +} diff --git a/internal/controller/coderaigatewaykey_record_test.go b/internal/controller/coderaigatewaykey_record_test.go new file mode 100644 index 000000000..fad0c9fc7 --- /dev/null +++ b/internal/controller/coderaigatewaykey_record_test.go @@ -0,0 +1,229 @@ +package controller_test + +import ( + "context" + "errors" + "net/http" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" +) + +func watchClient(t *testing.T) client.WithWatch { + t.Helper() + c, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + return c +} + +// aiGatewayKeyRecord returns the data of the cleanup record of uid in the +// env's operator namespace, or nil when there is none. +func (env aiGatewayKeyEnv) aiGatewayKeyRecord(ctx context.Context, t *testing.T, uid types.UID) map[string]string { + t.Helper() + configMap := &corev1.ConfigMap{} + err := k8sClient.Get(ctx, client.ObjectKey{Namespace: env.records, Name: "ck8s-cleanup-" + string(uid)}, configMap) + if apierrors.IsNotFound(err) { + return nil + } + require.NoError(t, err) + require.Equal(t, "CoderAIGatewayKey", configMap.Labels["coder.com/cleanup-record"]) + return configMap.Data +} + +// #353 part 2, key half: before each create, the controller records which +// Coder deployment holds the keys, the name prefix that finds them, and the +// time of the create. The current key ID follows once Coder answered it. The +// record holds no key. A delete that a LIST confirms removes it. +func TestCoderAIGatewayKeyCleanupRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got, _ := env.reconcile(ctx, t, env.createKey(ctx, t, "coder", testAIGatewaySecret)) + requireAIGatewayKeyReady(t, got) + want := map[string]string{ + "kind": "CoderAIGatewayKey", "uid": string(got.UID), "namespace": env.namespace, "name": got.Name, + "deploymentID": env.fake.deploymentID, "nonce": aiGatewayKeyPrefix(got), "coderID": got.Status.CurrentKey.ID, "type": "", + "createAttemptTime": got.Status.LastCreateAttemptTime.UTC().Format(time.RFC3339), + } + // reconcileErr checks that the record holds no key. + require.Equal(t, want, env.aiGatewayKeyRecord(ctx, t, got.UID)) + + // A rotation records its own create time first, and the new ID after. + env.clock.Step(time.Minute) + got.Spec.Rotation.Trigger = "rotate-1" + require.NoError(t, k8sClient.Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + require.Len(t, got.Status.RetiringKeys, 1) + want["coderID"] = got.Status.CurrentKey.ID + want["createAttemptTime"] = env.clock.Now().UTC().Format(time.RFC3339) + require.Equal(t, want, env.aiGatewayKeyRecord(ctx, t, got.UID)) + + require.NoError(t, k8sClient.Delete(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + requireAIGatewayKeyStep(t, got, false, "DeletingKeys") + require.NotNil(t, env.aiGatewayKeyRecord(ctx, t, got.UID), "the record stays while the keys exist") + env.clock.Step(5 * time.Minute) + gone, _ := env.reconcile(ctx, t, got) + require.Nil(t, gone) + require.Empty(t, env.fake.aiGatewayKeys()) + require.Nil(t, env.aiGatewayKeyRecord(ctx, t, got.UID), "a LIST confirmed the delete, so the record goes") +} + +// #353 invariant 1: without a record, the controller sends no create, and it +// writes no create marker. +func TestCoderAIGatewayKeyNoCreateWithoutRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("record write fails", func(t *testing.T) { + t.Parallel() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + key := env.createKey(ctx, t, "coder", testAIGatewaySecret) + env.client = interceptor.NewClient(watchClient(t), interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if _, ok := obj.(*corev1.ConfigMap); ok { + return errors.New("injected record write failure") + } + return c.Create(ctx, obj, opts...) + }, + }) + _, _, err := env.reconcileErr(ctx, t, key) + require.ErrorContains(t, err, "injected record write failure") + require.Zero(t, env.fake.requestCount(routeCreateAIGatewayKey), "no create without a record") + got := &coderv1alpha1.CoderAIGatewayKey{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(key), got)) + require.Zero(t, got.Status.NextSerial, "no create marker either") + }) + t.Run("deployment ID unknown", func(t *testing.T) { + t.Parallel() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + env.fake.failNext(routeBuildInfo, fakeFault{Status: http.StatusServiceUnavailable}) + got, _ := env.reconcile(ctx, t, env.createKey(ctx, t, "coder", testAIGatewaySecret)) + requireAIGatewayKeyStep(t, got, false, "CoderUnavailable") + require.Zero(t, env.fake.requestCount(routeCreateAIGatewayKey)) + require.Zero(t, got.Status.NextSerial) + require.Nil(t, env.aiGatewayKeyRecord(ctx, t, got.UID)) + }) +} + +// #353 invariants 3 and 7: when the control plane reaches another Coder +// database, the record is the only locator of the keys in the old one. The +// controller creates no key in the new one and keeps the record, and a +// delete releases the finalizer without removing it. +func TestCoderAIGatewayKeyDeploymentChanged(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got, _ := env.reconcile(ctx, t, env.createKey(ctx, t, "coder", testAIGatewaySecret)) + requireAIGatewayKeyReady(t, got) + before := env.aiGatewayKeyRecord(ctx, t, got.UID) + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() // A new database behind the same URL. + env.fake.mu.Unlock() + + got.Spec.Rotation.Trigger = "rotate-1" + require.NoError(t, k8sClient.Update(ctx, got)) + creates := env.fake.requestCount(routeCreateAIGatewayKey) + got, _ = env.reconcile(ctx, t, got) + stalled := meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderAIGatewayKeyConditionStalled) + require.Equal(t, "CoderDeploymentChanged", stalled.Reason) + require.Contains(t, stalled.Message, before["deploymentID"]) + require.Equal(t, creates, env.fake.requestCount(routeCreateAIGatewayKey), "no create in the new deployment") + require.Equal(t, before, env.aiGatewayKeyRecord(ctx, t, got.UID)) + + require.NoError(t, k8sClient.Delete(ctx, got)) + env.clock.Step(5 * time.Minute) + deletes := env.fake.requestCount(routeDeleteAIGatewayKey) + gone, _ := env.reconcile(ctx, t, got) + require.Nil(t, gone, "the finalizer goes: a LIST in the new deployment proves nothing") + require.Equal(t, deletes, env.fake.requestCount(routeDeleteAIGatewayKey)) + require.Equal(t, before, env.aiGatewayKeyRecord(ctx, t, got.UID), "the record stays for a sweep") +} + +// #353 invariant 7 in the one bounded pass while the control plane goes: the +// pass deletes what its LIST shows, but inside the settle window a create can +// still commit after that LIST, so the record stays for a sweep. After the +// window, the pass proves that no key is left, and the record goes. +func TestCoderAIGatewayKeyRecordAfterBoundedPass(t *testing.T) { + t.Parallel() + ctx := context.Background() + for name, settled := range map[string]bool{"inside the settle window": false, "after the settle window": true} { + t.Run(name, func(t *testing.T) { + t.Parallel() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got := rotatedKey(ctx, t, &env) + require.NoError(t, k8sClient.Delete(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: env.namespace}})) + holdControlPlane(ctx, t, env.namespace) + require.NoError(t, k8sClient.Delete(ctx, got)) + if settled { + env.clock.Step(5 * time.Minute) + } + r := &controller.CoderAIGatewayKeyReconciler{Client: env.client, APIReader: env.client, Scheme: scheme, Clock: env.clock, OperatorNamespace: env.records} + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) + require.NoError(t, err) + require.True(t, apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)), "the finalizer went") + require.Empty(t, env.fake.aiGatewayKeys()) + if settled { + require.Nil(t, env.aiGatewayKeyRecord(ctx, t, got.UID)) + } else { + require.NotNil(t, env.aiGatewayKeyRecord(ctx, t, got.UID)) + } + }) + } +} + +// A record whose create marker write failed: no create was sent, so the +// delete removes the record with no Coder call. +func TestCoderAIGatewayKeyRecordWithoutCreate(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + key := env.createKey(ctx, t, "coder", testAIGatewaySecret) + failing := env + failing.client = interceptor.NewClient(watchClient(t), interceptor.Funcs{ + SubResourceUpdate: func(context.Context, client.Client, string, client.Object, ...client.SubResourceUpdateOption) error { + return errors.New("injected marker write failure") + }, + }) + _, _, err := failing.reconcileErr(ctx, t, key) + require.ErrorContains(t, err, "injected marker write failure") + require.NotNil(t, env.aiGatewayKeyRecord(ctx, t, key.UID)) + require.Zero(t, env.fake.requestCount(routeCreateAIGatewayKey)) + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(key), key)) + require.Zero(t, key.Status.NextSerial) + require.NoError(t, k8sClient.Delete(ctx, key)) + requests := env.fake.totalRequests() + gone, _ := env.reconcile(ctx, t, key) + require.Nil(t, gone) + require.Equal(t, requests, env.fake.totalRequests(), "no Coder call") + require.Nil(t, env.aiGatewayKeyRecord(ctx, t, key.UID)) +} + +// An object from before records existed, or whose record lost its key ID, +// gets the record back once Coder lists its current key, with the time of +// its last create. +func TestCoderAIGatewayKeyRecordBackfill(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got, _ := env.reconcile(ctx, t, env.createKey(ctx, t, "coder", testAIGatewaySecret)) + want := env.aiGatewayKeyRecord(ctx, t, got.UID) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.records, Name: "ck8s-cleanup-" + string(got.UID)}})) + env.clock.Step(time.Minute) + got, _ = env.reconcile(ctx, t, got) + requireAIGatewayKeyReady(t, got) + require.Equal(t, want, env.aiGatewayKeyRecord(ctx, t, got.UID)) +} From 5bb248692414c910d9a0c4e5a4f88992c827eb6a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 21:47:45 +0000 Subject: [PATCH 2/3] feat: sweep the cleanup records of CoderAIGatewayKey objects that went without a delete (#353 part 2d) Change-Id: Ibf00c765664b1fa913d842a5c3e42509ea71312f Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-gateway-keys.md | 15 +- internal/controller/cleanup_sweep.go | 94 ++++++-- internal/controller/cleanup_sweep_key_test.go | 224 ++++++++++++++++++ 3 files changed, 312 insertions(+), 21 deletions(-) create mode 100644 internal/controller/cleanup_sweep_key_test.go diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index c72343aba..4bd45b8a9 100644 --- a/docs/how-to/manage-ai-gateway-keys.md +++ b/docs/how-to/manage-ai-gateway-keys.md @@ -151,10 +151,19 @@ Before each key create, the operator writes a cleanup record: a ConfigMap named 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. +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, for example in a namespace deletion, the record stays, and the operator sweeps it: + +1. It waits until the object with that UID is gone and 5 minutes have passed since the last create, so that a create still in flight commits first. +2. It looks for a ready `CoderControlPlane` in the object's namespace whose Coder answers the record's deployment ID now. It never matches by URL or name: a new Coder database behind the same URL has another deployment ID and is never swept. A control plane in another namespace never sweeps the record. +3. Through that control plane, whose operator user must be a site owner, it lists the keys and deletes every key whose name starts with the record's prefix `ck8s--`. It never deletes any other key. +4. It removes the record when a list shows no key with that prefix. A failed list or delete, or a `404` that is not Coder's own, keeps the record, and the operator retries every minute. + +So after a namespace deletion with an external database, the leftover keys go once you create the namespace again with a control plane that uses the same database. The metric `coder_k8s_cleanup_records_swept_total` (label `kind`) counts the swept records. + +When no ready control plane reaches the record's deployment, the record stays. The gauge `coder_k8s_cleanup_records_unmatched` counts these records, and the operator writes a `NoMatchingDeployment` Warning Event on the ConfigMap every 10 minutes. When that Coder deployment never comes back, delete the keys with the record's prefix in Coder by hand, then delete the ConfigMap. 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: a record names the keys that a sweep deletes. !!! 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`. + 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`. A [sweep](#cleanup-records) deletes the keys only when the namespace comes back with a control plane on the same Coder database. Until then they stay valid. Delete the `CoderAIGatewayKey` objects first and wait until they are gone, then delete the control plane or the namespace: @@ -191,7 +200,7 @@ A deletion that keeps failing never ends by itself. Leave it in one of these way ## Known limits -- **Leftover keys.** A key that commits more than 5 minutes after a lost create answer, or after the object is gone, stays in Coder. So do the keys of objects that go together with their control plane, for example in a namespace deletion. Find keys of deleted objects with `coder ai-gateway keys list`: their names start with `ck8s-` and a UID that no `CoderAIGatewayKey` has. +- **Leftover keys.** A key that commits more than 5 minutes after a lost create answer, or after the object is gone, stays in Coder. So do the keys of objects that go together with their control plane, for example in a namespace deletion, until a [sweep](#cleanup-records) deletes them. Find keys of deleted objects with `coder ai-gateway keys list`: their names start with `ck8s-` and a UID that no `CoderAIGatewayKey` has. - **A short key list.** The operator reads all keys with one list call. When a list answer names the current key but leaves out a replaced key, the operator treats the replaced key as deleted. If the key appears again, the operator deletes it as an orphan, without the rest of its grace period. Coder lists all keys in one answer, so only a proxy between the operator and Coder that changes the answer can cause this. ## Grant access diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go index aebeec4ed..accc677d3 100644 --- a/internal/controller/cleanup_sweep.go +++ b/internal/controller/cleanup_sweep.go @@ -63,8 +63,8 @@ func init() { // CleanupRecordReconciler sweeps the cleanup records of objects that went // without a confirmed delete in Coder (#353). It acts only through a ready // control plane whose current Coder deployment ID equals the record's, and -// it deletes only the recorded provider ID or the provider with the -// recorded nonce. +// it deletes only the recorded provider ID, the provider with the recorded +// nonce, or the AI Gateway keys whose names start with the recorded prefix. type CleanupRecordReconciler struct { client.Client // APIReader reads the object of a record live, so a stale cache never @@ -78,8 +78,9 @@ type CleanupRecordReconciler struct { } // SetupWithManager watches the records in the operator namespace. It wakes a -// record when its CoderAIProvider is deleted, and every record when a control -// plane gets operator access, since that control plane can match them. +// record when its CoderAIProvider or CoderAIGatewayKey is deleted, and every +// record when a control plane gets operator access, since that control plane +// can match them. func (r *CleanupRecordReconciler) SetupWithManager(mgr ctrl.Manager) error { if mgr == nil || r.Client == nil || r.APIReader == nil || r.OperatorNamespace == "" || r.Clock == nil || r.Recorder == nil { return errors.New("assertion failed: the cleanup record reconciler needs a manager, a client, an API reader, the operator namespace, a clock and an event recorder") @@ -87,11 +88,13 @@ func (r *CleanupRecordReconciler) SetupWithManager(mgr ctrl.Manager) error { isRecord := predicate.NewPredicateFuncs(func(obj client.Object) bool { return obj.GetNamespace() == r.OperatorNamespace && obj.GetLabels()[cleanupRecordLabel] != "" }) + recordOf := handler.EnqueueRequestsFromMapFunc(func(_ context.Context, obj client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: types.NamespacedName{Namespace: r.OperatorNamespace, Name: cleanupRecordObjectName(obj.GetUID())}}} + }) return ctrl.NewControllerManagedBy(mgr). For(&corev1.ConfigMap{}, builder.WithPredicates(isRecord)). - Watches(&coderv1alpha1.CoderAIProvider{}, handler.EnqueueRequestsFromMapFunc(func(_ context.Context, obj client.Object) []reconcile.Request { - return []reconcile.Request{{NamespacedName: types.NamespacedName{Namespace: r.OperatorNamespace, Name: cleanupRecordObjectName(obj.GetUID())}}} - }), builder.WithPredicates(dependentRemoved)). + Watches(&coderv1alpha1.CoderAIProvider{}, recordOf, builder.WithPredicates(dependentRemoved)). + Watches(&coderv1alpha1.CoderAIGatewayKey{}, recordOf, builder.WithPredicates(dependentRemoved)). Watches(&coderv1alpha1.CoderControlPlane{}, handler.EnqueueRequestsFromMapFunc(r.allRecords), builder.WithPredicates(operatorAccessGained)). Named("cleanuprecord"). Complete(r) @@ -149,13 +152,14 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques log.FromContext(ctx).Error(err, "Ignoring a ConfigMap that is not a valid cleanup record") return ctrl.Result{}, nil } - if record.Kind != cleanupRecordKindAIProvider { - return ctrl.Result{}, nil // Only provider records are swept so far. + kind, ok := cleanupRecordKinds[record.Kind] + if !ok { + return ctrl.Result{}, nil // A kind that this operator does not sweep. } // Act only when the object with that UID no longer exists (invariant 6). - provider := &coderv1alpha1.CoderAIProvider{} - if err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: record.Namespace, Name: record.Name}, provider); err == nil && provider.UID == record.UID { + object := kind.object() + if err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: record.Namespace, Name: record.Name}, object); err == nil && object.GetUID() == record.UID { r.setUnmatched(record.UID, record.Kind, false) return ctrl.Result{RequeueAfter: cleanupObjectRecheck}, nil } else if err != nil && !apierrors.IsNotFound(err) { @@ -181,23 +185,23 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques if len(matches) == 0 { r.setUnmatched(record.UID, record.Kind, true) r.Recorder.Eventf(configMap, nil, corev1.EventTypeWarning, "NoMatchingDeployment", "Sweep", - "No ready CoderControlPlane in namespace %s reaches Coder deployment %s, which holds the provider of deleted %s %s/%s. Delete the provider in Coder by hand, then delete this ConfigMap.", - record.Namespace, record.DeploymentID, record.Kind, record.Namespace, record.Name) + "No ready CoderControlPlane in namespace %s reaches Coder deployment %s, which holds %s of deleted %s %s/%s. Delete %s in Coder by hand, then delete this ConfigMap.", + record.Namespace, record.DeploymentID, kind.what, record.Kind, record.Namespace, record.Name, kind.what) return ctrl.Result{RequeueAfter: cleanupUnmatchedRecheck}, nil } r.setUnmatched(record.UID, record.Kind, false) - logger := log.FromContext(ctx).WithValues("record", configMap.Name, "deploymentID", record.DeploymentID, "provider", record.Namespace+"/"+record.Name) + logger := log.FromContext(ctx).WithValues("record", configMap.Name, "deploymentID", record.DeploymentID, "object", record.Kind+" "+record.Namespace+"/"+record.Name) // Each matching control plane has its own operator token, so a failure // through one tries the next. gone, why, through := false, "", logger for _, m := range matches { through = logger.WithValues("controlPlane", types.NamespacedName{Namespace: m.controlPlane.Namespace, Name: m.controlPlane.Name}) - if gone, why, err = sweepAIProvider(ctx, m.sdk, record); err != nil { + if gone, why, err = kind.sweep(ctx, m.sdk, record); err != nil { return ctrl.Result{}, err } else if gone { break } - through.Info("The cleanup of a deleted provider in Coder failed", "why", why) + through.Info("The cleanup of a deleted object in Coder failed", "why", why) } if !gone { return ctrl.Result{RequeueAfter: cleanupRetry}, nil @@ -207,10 +211,26 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, err } cleanupRecordsSwept.WithLabelValues(record.Kind).Inc() - through.Info("Swept the provider of a deleted CoderAIProvider in Coder", "result", why, "providerID", record.CoderID) + through.Info("Swept the Coder resources of a deleted object", "result", why, "coderID", record.CoderID) return ctrl.Result{}, nil } +// cleanupSweepKind is how a sweep handles the records of one object kind. +type cleanupSweepKind struct { + object func() client.Object + what string // What the object holds in Coder, for the Event. + sweep func(context.Context, *codersdk.Client, *cleanupRecord) (gone bool, why string, err error) +} + +var cleanupRecordKinds = map[string]cleanupSweepKind{ + cleanupRecordKindAIProvider: { + object: func() client.Object { return &coderv1alpha1.CoderAIProvider{} }, what: "the provider", sweep: sweepAIProvider, + }, + cleanupRecordKindAIGatewayKey: { + object: func() client.Object { return &coderv1alpha1.CoderAIGatewayKey{} }, what: "the AI Gateway keys", sweep: sweepAIGatewayKeys, + }, +} + // controlPlaneMatch is a control plane whose Coder deployment is a record's. type controlPlaneMatch struct { sdk *codersdk.Client @@ -309,6 +329,44 @@ func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupR return true, "deleted provider " + id, nil } +// sweepAIGatewayKeys deletes the keys of record in Coder: every key whose +// name is the record's prefix ck8s-- and a serial (invariant 5). The +// object is gone, so no serial bound applies: the settle window has passed, +// and no create of the object can still commit. It returns gone only when a +// LIST, read by a site owner, shows no such key. +func sweepAIGatewayKeys(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (gone bool, why string, err error) { + prefix, err := aiGatewayKeyNamePrefix(record.UID) + if err != nil { + return false, "", err + } + if record.Nonce != prefix { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has key prefix %q, not the prefix of its UID", record.UID, record.Nonce) + } + if step := requireSiteOwner(ctx, sdk); step != nil { + // Only a site owner's LIST shows every key. + return false, step.message, nil + } + deleted := 0 + for range 2 { + listed, err := sdk.ListAIGatewayKeys(ctx) + if err != nil { + return false, "list AI Gateway keys: " + coderErrorSummary(err), nil + } + own := ownAIGatewayKeys(prefix, listed) + if len(own) == 0 { + return true, fmt.Sprintf("deleted %d keys, and a list shows none left", deleted), nil + } + for _, k := range own { + // Only Coder's own 404 proves that the key is gone (#325). + if err := sdk.DeleteAIGatewayKey(ctx, k.ID); err != nil && !isCoderResourceNotFound(err) { + return false, "delete key " + k.Name + ": " + coderErrorSummary(err), nil + } + deleted++ + } + } + return false, "Coder still lists keys of the object after their delete", nil +} + // setUnmatched keeps the unmatched gauge equal to the records that no // deployment reaches. func (r *CleanupRecordReconciler) setUnmatched(uid types.UID, kind string, unmatched bool) { @@ -317,7 +375,7 @@ func (r *CleanupRecordReconciler) setUnmatched(uid types.UID, kind string, unmat } else { r.unmatched.Delete(uid) } - counts := map[string]float64{cleanupRecordKindAIProvider: 0} + counts := map[string]float64{cleanupRecordKindAIProvider: 0, cleanupRecordKindAIGatewayKey: 0} r.unmatched.Range(func(_, v any) bool { counts[v.(string)]++ return true diff --git a/internal/controller/cleanup_sweep_key_test.go b/internal/controller/cleanup_sweep_key_test.go new file mode 100644 index 000000000..f8e46add3 --- /dev/null +++ b/internal/controller/cleanup_sweep_key_test.go @@ -0,0 +1,224 @@ +package controller_test + +import ( + "context" + "net/http" + "strings" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/events" + clocktesting "k8s.io/utils/clock/testing" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/metrics" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" +) + +// keySweepEnv is a CoderAIGatewayKey that went with its control plane, as in +// a namespace deletion: Coder keeps its keys and the record stays. Coder also +// holds keys that are not the object's: a person's key, and a key of another +// object. +type keySweepEnv struct { + aiGatewayKeyEnv + key *coderv1alpha1.CoderAIGatewayKey + clock *clocktesting.FakeClock + recorder *events.FakeRecorder + sweeper *controller.CleanupRecordReconciler +} + +func newKeySweepEnv(ctx context.Context, t *testing.T) keySweepEnv { + t.Helper() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got := rotatedKey(ctx, t, &env) + sdk := env.fake.client(t, 5*time.Second) + for _, name := range []string{ + aiGatewayKeyPrefix(got) + "9", // An orphan of a create whose answer was lost. + "personal-key", + "ck8s-" + strings.ReplaceAll(uuid.NewString(), "-", "") + "-0", + } { + _, err := sdk.CreateAIGatewayKey(ctx, codersdk.CreateAIGatewayKeyRequest{Name: name}) + require.NoError(t, err) + } + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "coder"}})) + require.NoError(t, k8sClient.Delete(ctx, got)) + gone, _ := env.reconcile(ctx, t, got) + require.Nil(t, gone, "ControlPlaneGone releases the finalizer") + require.NotNil(t, env.aiGatewayKeyRecord(ctx, t, got.UID)) + require.Len(t, env.fake.aiGatewayKeys(), 5) + return newKeySweeper(t, env, got) +} + +func newKeySweeper(t *testing.T, env aiGatewayKeyEnv, key *coderv1alpha1.CoderAIGatewayKey) keySweepEnv { + t.Helper() + s := keySweepEnv{aiGatewayKeyEnv: env, key: key, clock: clocktesting.NewFakeClock(env.clock.Now()), recorder: events.NewFakeRecorder(10)} + s.sweeper = &controller.CleanupRecordReconciler{Client: watchClient(t), APIReader: k8sClient, OperatorNamespace: env.records, Clock: s.clock, Recorder: s.recorder} + return s +} + +func (s keySweepEnv) sweep(ctx context.Context, t *testing.T) ctrl.Result { + t.Helper() + result, err := s.sweeper.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKey{Namespace: s.records, Name: "ck8s-cleanup-" + string(s.key.UID)}}) + require.NoError(t, err) + return result +} + +// keysLeft returns the names of the keys that Coder holds. +func (s keySweepEnv) keysLeft() []string { + var names []string + for _, k := range s.fake.aiGatewayKeys() { + names = append(names, k.name) + } + return names +} + +// kindMetric reads a controller metric with the label kind. +func kindMetric(t *testing.T, name, kind string) float64 { + t.Helper() + families, err := metrics.Registry.Gather() + require.NoError(t, err) + for _, f := range families { + for _, m := range f.GetMetric() { + for _, l := range m.GetLabel() { + if f.GetName() == name && l.GetName() == "kind" && l.GetValue() == kind { + if m.GetGauge() != nil { + return m.GetGauge().GetValue() + } + return m.GetCounter().GetValue() + } + } + } + } + return 0 +} + +// #353, key half: after a namespace deletion, a new control plane that +// reaches the same Coder deployment deletes every key with the object's +// prefix, once the settle window has passed, and leaves every other key. A +// different deployment at the same URL never does. Not parallel: it reads +// process-wide metrics. +func TestCleanupSweepAIGatewayKeys(t *testing.T) { + ctx := context.Background() + s := newKeySweepEnv(ctx, t) + const kind = "CoderAIGatewayKey" + swept := kindMetric(t, "coder_k8s_cleanup_records_swept_total", kind) + + requests := s.fake.totalRequests() + result := s.sweep(ctx, t) + require.Equal(t, requests, s.fake.totalRequests(), "no Coder call inside the settle window") + require.Greater(t, result.RequeueAfter, 4*time.Minute) + require.LessOrEqual(t, result.RequeueAfter, 5*time.Minute, "it waits for the end of the settle window after the last create") + s.clock.Step(6 * time.Minute) + require.Equal(t, 10*time.Minute, s.sweep(ctx, t).RequeueAfter, "no control plane reaches the deployment") + event := <-s.recorder.Events + require.Contains(t, event, "NoMatchingDeployment") + require.Contains(t, event, "the AI Gateway keys") + require.Equal(t, 1.0, kindMetric(t, "coder_k8s_cleanup_records_unmatched", kind)) + + // A new control plane at the same URL, but a new Coder database answers. + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + recorded := s.fake.deploymentID + s.fake.mu.Lock() + s.fake.deploymentID = uuid.NewString() + s.fake.mu.Unlock() + require.Equal(t, 10*time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Zero(t, s.fake.requestCount(routeDeleteAIGatewayKey), "a different deployment is never swept") + require.Len(t, s.fake.aiGatewayKeys(), 5) + + // The same deployment comes back. + s.fake.mu.Lock() + s.fake.deploymentID = recorded + s.fake.mu.Unlock() + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + left := s.keysLeft() + require.Len(t, left, 2, "the person's key and the other object's key stay") + require.Contains(t, left, "personal-key") + for _, name := range left { + require.False(t, strings.HasPrefix(name, aiGatewayKeyPrefix(s.key)), "every key of the object is gone: %s", name) + } + require.Equal(t, 3, s.fake.requestCount(routeDeleteAIGatewayKey), "the current, the retiring and the orphan key") + require.Nil(t, s.aiGatewayKeyRecord(ctx, t, s.key.UID), "a LIST confirmed the delete, so the record goes") + require.Equal(t, swept+1, kindMetric(t, "coder_k8s_cleanup_records_swept_total", kind)) + require.Zero(t, kindMetric(t, "coder_k8s_cleanup_records_unmatched", kind)) +} + +// #353 invariants 5 to 7 for keys: a sweep never touches the keys of a +// living object, keeps the record until a site owner's LIST shows no key of +// the object, and decides by UID that the object is gone. +func TestCleanupSweepAIGatewayKeyCases(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("living object", func(t *testing.T) { + t.Parallel() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got, _ := env.reconcile(ctx, t, env.createKey(ctx, t, "coder", testAIGatewaySecret)) + s := newKeySweeper(t, env, got) + s.clock.Step(time.Hour) + requests := s.fake.totalRequests() + require.Equal(t, 30*time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Equal(t, requests, s.fake.totalRequests(), "no Coder call for a living object") + require.NotNil(t, s.aiGatewayKeyRecord(ctx, t, got.UID)) + }) + t.Run("same name, another object", func(t *testing.T) { + t.Parallel() + s := newKeySweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.createKey(ctx, t, "coder", testAIGatewaySecret) // The old name, a new UID. + s.clock.Step(6 * time.Minute) + require.Zero(t, s.sweep(ctx, t).RequeueAfter, "only the UID decides that the object is gone") + require.Len(t, s.keysLeft(), 2) + }) + t.Run("delete fails", func(t *testing.T) { + t.Parallel() + s := newKeySweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.failNext(routeDeleteAIGatewayKey, fakeFault{Status: http.StatusNotFound, HTML: "not found"}) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter, "only Coder's own 404 confirms a delete") + require.NotNil(t, s.aiGatewayKeyRecord(ctx, t, s.key.UID), "the record stays until Coder confirms") + require.Len(t, s.keysLeft(), 5, "the sweep stops at the failed DELETE") + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + require.Nil(t, s.aiGatewayKeyRecord(ctx, t, s.key.UID)) + require.Len(t, s.keysLeft(), 2) + }) + t.Run("LIST still shows a key", func(t *testing.T) { + t.Parallel() + s := newKeySweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + // Both LISTs of the pass show a key that each DELETE answers 404 for, + // as a Coder that lists a key it cannot delete would. + ghost := codersdk.AIGatewayKey{ID: uuid.New(), Name: aiGatewayKeyPrefix(s.key) + "1"} + for range 2 { + s.fake.failNext(routeAIGatewayKeys, fakeFault{Rewrite: func(a any) any { return append(a.([]codersdk.AIGatewayKey), ghost) }}) + } + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter, "only a LIST that shows no key of the object confirms") + require.NotNil(t, s.aiGatewayKeyRecord(ctx, t, s.key.UID)) + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + require.Nil(t, s.aiGatewayKeyRecord(ctx, t, s.key.UID)) + }) + t.Run("operator not a site owner", func(t *testing.T) { + t.Parallel() + s := newKeySweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.updateUser(s.fake.operatorID, func(u *codersdk.User) { u.Roles = []codersdk.SlimRole{} }) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Zero(t, s.fake.requestCount(routeDeleteAIGatewayKey), "only a site owner's LIST shows every key") + require.NotNil(t, s.aiGatewayKeyRecord(ctx, t, s.key.UID)) + }) + t.Run("control plane in another namespace", func(t *testing.T) { + t.Parallel() + s := newKeySweepEnv(ctx, t) + createTestControlPlane(ctx, t, createTestNamespace(ctx, t, "aigk-other"), "coder", s.fake.server.URL) + s.clock.Step(6 * time.Minute) + require.Equal(t, 10*time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Zero(t, s.fake.requestCount(routeDeleteAIGatewayKey), "only a control plane in the object's namespace sweeps") + }) +} From 56e07688a08939e60e051e442f00c45137d5d77e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 22:11:09 +0000 Subject: [PATCH 3/3] docs: the key sweep deletes the prefix plus a serial (advisor F1) Change-Id: Ie6e20c73f37d2ad40e2df7d8602fb0d0aebab4bd Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-gateway-keys.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index 4bd45b8a9..2871b84fb 100644 --- a/docs/how-to/manage-ai-gateway-keys.md +++ b/docs/how-to/manage-ai-gateway-keys.md @@ -155,8 +155,8 @@ The operator removes the record only after a list in the record's deployment sho 1. It waits until the object with that UID is gone and 5 minutes have passed since the last create, so that a create still in flight commits first. 2. It looks for a ready `CoderControlPlane` in the object's namespace whose Coder answers the record's deployment ID now. It never matches by URL or name: a new Coder database behind the same URL has another deployment ID and is never swept. A control plane in another namespace never sweeps the record. -3. Through that control plane, whose operator user must be a site owner, it lists the keys and deletes every key whose name starts with the record's prefix `ck8s--`. It never deletes any other key. -4. It removes the record when a list shows no key with that prefix. A failed list or delete, or a `404` that is not Coder's own, keeps the record, and the operator retries every minute. +3. Through that control plane, whose operator user must be a site owner, it lists the keys and deletes every key whose name is the record's prefix `ck8s--` followed by a serial number (the names that the operator gives, such as `ck8s--3`). It never deletes any other key. +4. It removes the record when a list shows no such key. A failed list or delete, or a `404` that is not Coder's own, keeps the record, and the operator retries every minute. So after a namespace deletion with an external database, the leftover keys go once you create the namespace again with a control plane that uses the same database. The metric `coder_k8s_cleanup_records_swept_total` (label `kind`) counts the swept records.