diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index 62e98555..c72343ab 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 09933be6..30222b2c 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 0413f569..3d638eda 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 2ec30dd0..6f761c5a 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 b76597c6..9cab414f 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 00000000..d27432b9 --- /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 aa4e838c..94284eb2 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 d8f0e231..218c72e3 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 00000000..9a010b1d --- /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 00000000..fad0c9fc --- /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)) +}