From 553cad7604a9cecc8469e816dd54635cece789b8 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 15:59:09 +0000 Subject: [PATCH 1/5] feat: write a cleanup record before each CoderAIProvider create (#353) Change-Id: Ice66d8e736e7372a3afd95d8c96a79955aca6b04 Signed-off-by: Thomas Kosiewski --- .github/workflows/ci.yaml | 1 + config/default/kustomization.yaml | 1 + config/rbac/cleanup-records-rolebinding.yaml | 16 ++ config/rbac/role.yaml | 18 ++ dist/install.yaml | 32 ++++ docs/explanation/architecture.md | 2 +- docs/how-to/manage-ai-gateway-keys.md | 2 +- docs/how-to/manage-ai-providers.md | 8 +- internal/app/controllerapp/controllerapp.go | 2 + internal/controller/cleanup_record.go | 165 ++++++++++++++++++ .../controller/coderaiprovider_controller.go | 17 +- .../coderaiprovider_controller_test.go | 4 +- internal/controller/coderaiprovider_create.go | 18 +- .../controller/coderaiprovider_create_test.go | 2 +- internal/controller/coderaiprovider_delete.go | 14 +- internal/controller/coderaiprovider_record.go | 71 ++++++++ .../controller/coderaiprovider_record_test.go | 154 ++++++++++++++++ internal/controller/controlplane_hold_test.go | 1 + internal/controller/fakecoder_test.go | 25 ++- 19 files changed, 527 insertions(+), 26 deletions(-) create mode 100644 config/rbac/cleanup-records-rolebinding.yaml create mode 100644 internal/controller/cleanup_record.go create mode 100644 internal/controller/coderaiprovider_record.go create mode 100644 internal/controller/coderaiprovider_record_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index d568619ed..a759b8b11 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -94,6 +94,7 @@ jobs: # Shared Coder access that CoderTemplateTest and later pool controllers call. - 'internal/controller/controlplane_access.go' - 'internal/controller/controlplane_hold.go' + - 'internal/controller/cleanup_record*' - 'internal/controller/coder_access.go' - 'internal/controller/coder_workspace_steps.go' # The URL that the aggregated API server and the controllers use for Coder API calls. diff --git a/config/default/kustomization.yaml b/config/default/kustomization.yaml index d3f9b0e3a..c4379d6d9 100644 --- a/config/default/kustomization.yaml +++ b/config/default/kustomization.yaml @@ -9,6 +9,7 @@ resources: - ../crd/bases/coder.com_coderprovisioners.yaml - ../crd/bases/coder.com_codertemplatetests.yaml - ../crd/bases/coder.com_coderworkspaceproxies.yaml + - ../rbac/cleanup-records-rolebinding.yaml - ../rbac/clusterrolebinding.yaml - ../rbac/role.yaml - ../rbac/serviceaccount.yaml diff --git a/config/rbac/cleanup-records-rolebinding.yaml b/config/rbac/cleanup-records-rolebinding.yaml new file mode 100644 index 000000000..5572237fb --- /dev/null +++ b/config/rbac/cleanup-records-rolebinding.yaml @@ -0,0 +1,16 @@ +# Lets the operator write its cleanup records (#353): ConfigMaps named ck8s-cleanup- in its own +# namespace. The namespaced Role manager-role comes from the kubebuilder RBAC markers in role.yaml. No other +# subject gets write access to these ConfigMaps. +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: coder-k8s-cleanup-records + namespace: coder-system +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: manager-role +subjects: + - kind: ServiceAccount + name: coder-k8s + namespace: coder-system diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 561f059eb..0b240a3cb 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -201,3 +201,21 @@ rules: - patch - update - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: manager-role + namespace: coder-system +rules: +- apiGroups: + - "" + resources: + - configmaps + verbs: + - create + - delete + - get + - list + - update + - watch diff --git a/dist/install.yaml b/dist/install.yaml index 23b1e3b72..eb15dae61 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6410,6 +6410,24 @@ metadata: namespace: coder-system --- apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: manager-role + namespace: coder-system +rules: +- apiGroups: + - "" + resources: + - configmaps + verbs: + - create + - delete + - get + - list + - update + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: name: manager-role @@ -6613,6 +6631,20 @@ rules: - watch --- apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: coder-k8s-cleanup-records + namespace: coder-system +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: manager-role +subjects: +- kind: ServiceAccount + name: coder-k8s + namespace: coder-system +--- +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: name: coder-k8s diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index c6a02ff4f..987146852 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -76,7 +76,7 @@ graph TD | Path | Contents | | --- | --- | | `config/crd/bases/` | The generated CRDs for `CoderControlPlane`, `CoderProvisioner`, `CoderWorkspaceProxy`, `CoderTemplateTest`, `CoderAIProvider`, and `CoderAIGatewayKey` | -| `config/rbac/` | The `coder-k8s` ServiceAccount, the `manager-role` ClusterRole, and the bindings (including auth-delegator) | +| `config/rbac/` | The `coder-k8s` ServiceAccount, the `manager-role` ClusterRole, the `manager-role` Role in `coder-system` (write access to the cleanup-record ConfigMaps there), and the bindings (including auth-delegator) | | `config/apiserver-standalone/` | For standalone `--app=aggregated-apiserver`: the `coder-k8s-apiserver` ServiceAccount, its RBAC, and the placeholder Secret for the serving CA | | `deploy/deployment.yaml` | The `coder-k8s` Deployment (default `--app=all`) | | `deploy/apiserver-service.yaml`, `deploy/apiserver-apiservice.yaml` | The Service and APIService that make the aggregated API available | diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index b9427ee63..62e98555f 100644 --- a/docs/how-to/manage-ai-gateway-keys.md +++ b/docs/how-to/manage-ai-gateway-keys.md @@ -139,7 +139,7 @@ The finalizer stays while Coder cannot confirm the deletion: 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 you delete the control plane while `CoderAIGatewayKey` or `CoderAIProvider` objects that use it are being deleted, the control plane waits for them 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 30 more seconds plus 10 seconds per object of one kind (at most 5 minutes) for their bounded pass below, and then the control plane goes anyway. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait, because Kubernetes then deletes the Coder Deployment first. +When you delete the control plane while `CoderAIGatewayKey` or `CoderAIProvider` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes: it keeps its finalizer, so Coder keeps running, and the objects delete their keys and providers as usual, with retries. The operator log shows `Waiting for the objects that use this control plane` with their names. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane (or within 30 seconds). After 10 minutes, the objects that remain get at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for their bounded pass below, and then the control plane goes anyway. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait, because Kubernetes then deletes the Coder Deployment first. When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed) and its operator access still works, the operator runs one pass of at most 10 seconds without the settle window, then removes the finalizer whatever the answer. A key create that was still in flight at that moment can commit later, and its key stays in Coder. When the pass fails, the log lists the IDs that it did not delete. diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index e1879d0f7..133479721 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -122,11 +122,17 @@ Deleting the object deletes the provider in Coder. With only an unconfirmed crea - When Coder is unreachable, or the control plane has no operator access, the finalizer `coder.com/ai-provider-cleanup` stays and the operator retries every 60 seconds. See [Stuck finalizer](#stuck-finalizer). - When the control plane is gone, nothing can delete the provider. The finalizer goes at once, and the operator logs the provider ID. Delete the provider in the Coder dashboard. -- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus 30 seconds and 10 seconds per object of one kind (at most 5 minutes) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. 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. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. +- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` 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, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. 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. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. - When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed) and its operator access still works, the operator sends one delete with a 10-second limit, then removes the finalizer. Without working access, the finalizer goes without a delete. Either way a namespace deletion finishes. When the delete did not succeed, the operator logs the provider ID: delete the provider in the Coder dashboard. The metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`) counts these releases. Delete `CoderAIProvider` objects before their namespace: a namespace deletion removes Coder's pods at once, so the control plane cannot wait for them. Coder deletes a provider even while chat models use it. Those models stop working. +### Cleanup records + +Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the provider's display-name nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. + +The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays. Find these records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. + ### Stuck finalizer A deleted `CoderAIProvider` stays `Terminating` while the operator cannot prove that Coder no longer holds its provider. Two cases never end by themselves: diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index d0ca9b118..1bbf7a88f 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -129,6 +129,8 @@ func SetupControllers(mgr manager.Manager) error { aiProviderReconciler := &controller.CoderAIProviderReconciler{ Client: client, Scheme: managerScheme, + // The operator's own namespace holds the cleanup records (#353). + OperatorNamespace: detectLeaderElectionNamespace(), } if err := aiProviderReconciler.SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create AI provider controller: %w", err) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go new file mode 100644 index 000000000..44eff4a80 --- /dev/null +++ b/internal/controller/cleanup_record.go @@ -0,0 +1,165 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Cleanup records (#353 part 2, option B′). Before the operator creates +// something in Coder for an object, it writes a record: one ConfigMap named +// ck8s-cleanup- in the operator's own namespace. The record says +// which Coder deployment holds the resource and how to find it. It stays when +// the object goes without a confirmed delete in Coder (for example in a +// namespace deletion), so a later sweep through a control plane of the same +// deployment can delete the resource. A record holds no secrets. The +// ConfigMap creation time is the record's created-at. +const ( + cleanupRecordPrefix = "ck8s-cleanup-" + // cleanupRecordLabel marks a record. Its value is the object kind. + cleanupRecordLabel = "coder.com/cleanup-record" + + cleanupRecordKind = "kind" + cleanupRecordUID = "uid" + cleanupRecordNamespace = "namespace" + cleanupRecordName = "name" + cleanupRecordDeploymentID = "deploymentID" + cleanupRecordNonce = "nonce" + cleanupRecordCoderID = "coderID" + cleanupRecordCreateSentAt = "createAttemptTime" +) + +// cleanupRecord is the content of a record. +type cleanupRecord struct { + Kind string + UID types.UID + Namespace string + Name string + // DeploymentID is Coder's deployment ID (/api/v2/buildinfo), read from + // Coder in the reconcile that sends the create. It lives in the Coder + // database, so it names the database that holds the resource. + DeploymentID string + // Nonce finds a resource whose Coder ID is not known yet: the display + // name of a provider. + Nonce string + // CoderID is the Coder ID of the resource once the operator knows it. + CoderID string + // CreateAttemptTime is when the operator last sent a create. A sweep + // waits for the settle window after it. + CreateAttemptTime time.Time +} + +func cleanupRecordObjectName(uid types.UID) string { return cleanupRecordPrefix + string(uid) } + +// coderDeploymentID reads Coder's deployment ID. Only a canonical UUID is +// accepted: the ID decides which resources a sweep may delete. +func coderDeploymentID(ctx context.Context, sdk *codersdk.Client) (string, error) { + info, err := sdk.BuildInfo(ctx) + if err != nil { + return "", err + } + if id, err := uuid.Parse(info.DeploymentID); err != nil || id == uuid.Nil || id.String() != info.DeploymentID { + return "", fmt.Errorf("coder answered a deployment ID %q that is not a canonical UUID", info.DeploymentID) + } + return info.DeploymentID, nil +} + +// readCleanupRecord returns the record of uid, or nil when there is none. A +// ConfigMap of that name that is not a record of uid is an error: the +// operator never writes over it. +func readCleanupRecord(ctx context.Context, reader client.Reader, namespace string, uid types.UID) (*cleanupRecord, *corev1.ConfigMap, error) { + if namespace == "" || uid == "" { + return nil, nil, errors.New("assertion failed: a cleanup record needs the operator namespace and an object UID") + } + configMap := &corev1.ConfigMap{} + if err := reader.Get(ctx, types.NamespacedName{Namespace: namespace, Name: cleanupRecordObjectName(uid)}, configMap); apierrors.IsNotFound(err) { + return nil, nil, nil + } else if err != nil { + return nil, nil, fmt.Errorf("get cleanup record %s/%s: %w", namespace, cleanupRecordObjectName(uid), err) + } + record, err := parseCleanupRecord(configMap) + if err != nil { + return nil, nil, err + } + if record.UID != uid { + return nil, nil, fmt.Errorf("ConfigMap %s/%s is not the cleanup record of object %s", namespace, configMap.Name, uid) + } + return record, configMap, nil +} + +// parseCleanupRecord reads a record from its ConfigMap. +func parseCleanupRecord(configMap *corev1.ConfigMap) (*cleanupRecord, error) { + d := configMap.Data + record := &cleanupRecord{ + Kind: d[cleanupRecordKind], UID: types.UID(d[cleanupRecordUID]), Namespace: d[cleanupRecordNamespace], Name: d[cleanupRecordName], + DeploymentID: d[cleanupRecordDeploymentID], Nonce: d[cleanupRecordNonce], CoderID: d[cleanupRecordCoderID], + } + if configMap.Labels[cleanupRecordLabel] == "" || configMap.Labels[cleanupRecordLabel] != record.Kind || record.UID == "" || + configMap.Name != cleanupRecordObjectName(record.UID) || record.DeploymentID == "" { + return nil, fmt.Errorf("ConfigMap %s/%s is not a valid cleanup record", configMap.Namespace, configMap.Name) + } + if t := d[cleanupRecordCreateSentAt]; t != "" { + parsed, err := time.Parse(time.RFC3339, t) + if err != nil { + return nil, fmt.Errorf("cleanup record %s/%s has an invalid %s: %w", configMap.Namespace, configMap.Name, cleanupRecordCreateSentAt, err) + } + record.CreateAttemptTime = parsed + } + return record, nil +} + +// writeCleanupRecord creates or replaces the record in namespace. existing +// is the ConfigMap that readCleanupRecord returned, or nil. +func writeCleanupRecord(ctx context.Context, c client.Client, namespace string, existing *corev1.ConfigMap, record cleanupRecord) error { + if namespace == "" || record.Kind == "" || record.UID == "" || record.DeploymentID == "" { + return errors.New("assertion failed: a cleanup record needs the operator namespace, a kind, an object UID and a deployment ID") + } + data := map[string]string{ + cleanupRecordKind: record.Kind, cleanupRecordUID: string(record.UID), cleanupRecordNamespace: record.Namespace, cleanupRecordName: record.Name, + cleanupRecordDeploymentID: record.DeploymentID, cleanupRecordNonce: record.Nonce, cleanupRecordCoderID: record.CoderID, + } + if !record.CreateAttemptTime.IsZero() { + data[cleanupRecordCreateSentAt] = record.CreateAttemptTime.UTC().Format(time.RFC3339) + } + if existing != nil { + updated := existing.DeepCopy() + updated.Data = data + if err := c.Update(ctx, updated); err != nil { + return fmt.Errorf("update cleanup record %s/%s: %w", namespace, updated.Name, err) + } + return nil + } + created := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: cleanupRecordObjectName(record.UID), Namespace: namespace, + Labels: map[string]string{cleanupRecordLabel: record.Kind, "app.kubernetes.io/managed-by": "coder-k8s"}, + }, + Data: data, + } + if err := c.Create(ctx, created); err != nil { + return fmt.Errorf("create cleanup record %s/%s: %w", namespace, created.Name, err) + } + return nil +} + +// deleteCleanupRecord removes the record of uid. Call it only after Coder +// confirmed that it holds no resource of the object. +func deleteCleanupRecord(ctx context.Context, c client.Client, namespace string, uid types.UID) error { + if namespace == "" || uid == "" { + return errors.New("assertion failed: a cleanup record needs the operator namespace and an object UID") + } + configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: cleanupRecordObjectName(uid)}} + if err := c.Delete(ctx, configMap); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("delete cleanup record %s/%s: %w", namespace, configMap.Name, err) + } + return nil +} diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index b65d38081..76bd7332b 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -42,6 +42,7 @@ const ( // +kubebuilder:rbac:groups=coder.com,resources=coderaiproviders,verbs=get;list;watch;patch // +kubebuilder:rbac:groups=coder.com,resources=coderaiproviders/status,verbs=get;update // +kubebuilder:rbac:groups=coder.com,resources=coderaiproviders/finalizers,verbs=update +// +kubebuilder:rbac:groups="",namespace=coder-system,resources=configmaps,verbs=get;list;watch;create;update;delete // CoderAIProviderReconciler reconciles CoderAIProvider objects (#154). It // checks the spec, the credential Secret and the @@ -52,6 +53,10 @@ type CoderAIProviderReconciler struct { client.Client Scheme *runtime.Scheme + // OperatorNamespace holds the cleanup records (#353): the operator's own + // namespace, which only the operator can write ConfigMaps in. + OperatorNamespace string + sentKeys sync.Map // types.UID -> SHA-256 of the key that Coder holds. Memory only. } @@ -139,8 +144,8 @@ func (r *CoderAIProviderReconciler) providersWith(ctx context.Context, namespace // Reconcile runs the checks in order and reports the first one that fails. func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - if r.Client == nil || r.Scheme == nil { - return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client and a scheme") + if r.Client == nil || r.Scheme == nil || r.OperatorNamespace == "" { + return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client, a scheme and the operator namespace") } provider := &coderv1alpha1.CoderAIProvider{} if err := r.Get(ctx, req.NamespacedName, provider); err != nil { @@ -223,6 +228,11 @@ func (r *CoderAIProviderReconciler) check( // operator token Secret that appears later. return aiProviderWait(aiProviderRetry, wait.reason, "%s", wait.message), nil } + // The record carries the provider ID before the sync replaces the + // nonce, so a later sweep can still find the provider (#353). + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); step != nil || err != nil { + return step, err + } var step *aiProviderStep recreate := provider.Status.ProviderID == "" if !recreate { @@ -243,6 +253,9 @@ func (r *CoderAIProviderReconciler) check( if step == nil { // Store the ID before the sync replaces the nonce: after that, // only the stored ID proves that this object created the provider. + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); step != nil || err != nil { + return step, err + } if err := r.writeStatus(ctx, provider, before); err != nil { return nil, err } diff --git a/internal/controller/coderaiprovider_controller_test.go b/internal/controller/coderaiprovider_controller_test.go index 515046e0a..053347393 100644 --- a/internal/controller/coderaiprovider_controller_test.go +++ b/internal/controller/coderaiprovider_controller_test.go @@ -42,7 +42,7 @@ type aiProviderEnv struct { func newAIProviderEnv(ctx context.Context, t *testing.T, withControlPlane bool) aiProviderEnv { t.Helper() env := aiProviderEnv{namespace: createTestNamespace(ctx, t, "aip"), fake: newFakeCoder(t)} - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} if withControlPlane { createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) } @@ -384,7 +384,7 @@ func TestCoderAIProviderWatches(t *testing.T) { Controller: config.Controller{SkipNameValidation: ptrTo(true)}, }) require.NoError(t, err) - require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme()}).SetupWithManager(mgr)) + require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme(), OperatorNamespace: env.namespace}).SetupWithManager(mgr)) mgrCtx, cancel := context.WithCancel(ctx) mgrDone := make(chan error, 1) go func() { mgrDone <- mgr.Start(mgrCtx) }() diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index 0348d1f24..c946a211a 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -134,16 +134,22 @@ func recordAIProvider(ctx context.Context, provider *coderv1alpha1.CoderAIProvid return nil, nil } -// createAIProvider writes the create marker and the change token of the key, -// then sends the only create request of this attempt. A conflict on the -// marker write ends the reconcile before any request. The provider starts -// disabled with the nonce as its display name. The apply step then sets the -// spec values. +// createAIProvider writes the cleanup record, then the create marker and the +// change token of the key, then sends the only create request of this +// attempt. A failed record write or a conflict on the marker write ends the +// reconcile before any request. The provider starts disabled with the nonce +// as its display name. The apply step then sets the spec values. func (r *CoderAIProviderReconciler) createAIProvider( ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, key credentialsecret.Value, before *coderv1alpha1.CoderAIProviderStatus, ) (*aiProviderStep, error) { token := key.Token - provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now()} + now := time.Now() + // The record goes first: without it, a provider that a namespace + // deletion leaves behind cannot be found again (#353 invariant 1). + if step, err := r.writeAIProviderRecord(ctx, sdk, provider, &now); step != nil || err != nil { + return step, err + } + provider.Status.CreateAttemptTime = &metav1.Time{Time: now} provider.Status.Credential = &coderv1alpha1.CoderAIProviderCredentialStatus{ SecretName: token.SecretName, SecretKey: token.SecretKey, SecretUID: string(token.SecretUID), SecretResourceVersion: token.ResourceVersion, } diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index 3c68c8914..7b98baaea 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -39,7 +39,7 @@ func newAIProviderCreateEnv(ctx context.Context, t *testing.T) (aiProviderEnv, * // restart gives env a new reconciler, as a controller restart does. The // reconciler keeps no state between calls, so every step resumes from status. func (env *aiProviderEnv) restart() { - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} } func TestCoderAIProviderCreate(t *testing.T) { diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 527b0d301..b0866b7fd 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -59,8 +59,9 @@ func checkAIProviderID(provider *coderv1alpha1.CoderAIProvider) error { // the finalizer stays. func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *coderv1alpha1.CoderAIProvider) (release bool, step *aiProviderStep, err error) { if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { - // Nothing of this object can exist in Coder. - return true, nil, nil + // Nothing of this object can exist in Coder: earlier Coder answers + // cleared the create marker, so the record goes too. + return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) } ref := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.ControlPlaneRef.Name} controlPlane := &coderv1alpha1.CoderControlPlane{} @@ -107,7 +108,14 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co ctx, cancel = context.WithTimeout(ctx, aiProviderBoundedDeleteTimeout) defer cancel() } - if step, err = r.deleteInCoder(ctx, sdk, provider); err != nil && deleting { + step, err = r.deleteInCoder(ctx, sdk, provider) + if err == nil && step == nil { + // Coder confirmed that it holds no provider of this object. Only + // then does the record go (#353 invariant 7). A failed record + // delete retries, and the retry gets Coder's 404 for the provider. + return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + } + if err != nil && deleting { // The attempt ended with an impossible answer. Log it, and still // release: the one attempt is over. log.FromContext(ctx).Error(err, "The one delete attempt while CoderControlPlane is being deleted got an impossible answer") diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go new file mode 100644 index 000000000..fe28d0b16 --- /dev/null +++ b/internal/controller/coderaiprovider_record.go @@ -0,0 +1,71 @@ +package controller + +import ( + "context" + "time" + + "github.com/coder/coder/v2/codersdk" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// cleanupRecordKindAIProvider is the record kind of a CoderAIProvider. +const cleanupRecordKindAIProvider = "CoderAIProvider" + +// writeAIProviderRecord writes the cleanup record of provider with the +// deployment ID that Coder answers now, so the record names the database that +// the next create or the learned provider ID belongs to. createAttempt is the +// time of a 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 *CoderAIProviderReconciler) writeAIProviderRecord( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, createAttempt *time.Time, +) (*aiProviderStep, error) { + previous, existing, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + if err != nil { + return nil, err + } + deploymentID, err := coderDeploymentID(ctx, sdk) + if err != nil { + return aiProviderWait(aiProviderRetry, "CoderUnavailable", "read the Coder deployment ID for the cleanup record: %s", coderErrorSummary(err)), nil + } + record := cleanupRecord{ + Kind: cleanupRecordKindAIProvider, UID: provider.UID, Namespace: provider.Namespace, Name: provider.Name, + DeploymentID: deploymentID, Nonce: aiProviderNonce(provider), CoderID: provider.Status.ProviderID, + } + if createAttempt != nil { + record.CreateAttemptTime = *createAttempt + } else if provider.Status.CreateAttemptTime != nil { + record.CreateAttemptTime = provider.Status.CreateAttemptTime.Time + } else if previous != nil { + // The marker went after a confirmed create. The sweep still waits + // for the settle window after that create. + record.CreateAttemptTime = previous.CreateAttemptTime + } + return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) +} + +// ensureAIProviderRecord makes sure that a provider which can exist in Coder +// has a record with its current provider ID. Objects from before records +// existed get one here too. +func (r *CoderAIProviderReconciler) ensureAIProviderRecord( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, +) (*aiProviderStep, error) { + if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { + return nil, nil // Nothing of this object can exist in Coder yet. The create writes the record. + } + if provider.Status.ProviderID != "" { + // A sweep deletes the recorded ID, so only a canonical UUID goes in. + if err := checkAIProviderID(provider); err != nil { + return nil, err + } + } + record, _, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + if err != nil { + return nil, err + } + if record != nil && record.CoderID == provider.Status.ProviderID { + return nil, nil + } + return r.writeAIProviderRecord(ctx, sdk, provider, nil) +} diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go new file mode 100644 index 000000000..9cef7acec --- /dev/null +++ b/internal/controller/coderaiprovider_record_test.go @@ -0,0 +1,154 @@ +package controller_test + +import ( + "context" + "errors" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + 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" +) + +// cleanupRecord returns the data of the cleanup record of uid in namespace, +// or nil when there is none. +func cleanupRecord(ctx context.Context, t *testing.T, namespace string, uid types.UID) map[string]string { + t.Helper() + configMap := &corev1.ConfigMap{} + err := k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "ck8s-cleanup-" + string(uid)}, configMap) + if apierrors.IsNotFound(err) { + return nil + } + require.NoError(t, err) + require.Equal(t, "CoderAIProvider", configMap.Labels["coder.com/cleanup-record"]) + return configMap.Data +} + +// #353 part 2: before its create, the controller records which Coder +// deployment holds the provider, and the provider ID before the sync replaces +// the nonce. The record holds no key. A confirmed delete in Coder removes it. +func TestCoderAIProviderCleanupRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + + // Stop after the create, before the sync replaces the nonce. + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusInternalServerError}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + stored, ok := env.fake.aiProvider("openai") + require.True(t, ok) + record := cleanupRecord(ctx, t, env.namespace, got.UID) + require.NotEmpty(t, record["createAttemptTime"]) + delete(record, "createAttemptTime") + require.Equal(t, map[string]string{ + "kind": "CoderAIProvider", "uid": string(got.UID), "namespace": env.namespace, "name": "openai", + "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": stored.id.String(), + }, record, "the provider ID is recorded before the sync replaces the nonce") + + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + for _, v := range cleanupRecord(ctx, t, env.namespace, got.UID) { + require.NotContains(t, v, testAIProviderKey, "a record holds no key") + } + + gone, _ := env.deleteAIProvider(ctx, t, got) + require.Nil(t, gone) + _, ok = env.fake.aiProvider("openai") + require.False(t, ok) + require.Nil(t, cleanupRecord(ctx, t, env.namespace, got.UID), "Coder confirmed the delete, so the record goes") +} + +// #353 invariant 1: without a record, the controller sends no create. +func TestCoderAIProviderNoCreateWithoutRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("record write fails", func(t *testing.T) { + t.Parallel() + env, _, provider := newAIProviderCreateEnv(ctx, t) + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + env.r.Client = interceptor.NewClient(base, 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.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.ErrorContains(t, err, "injected record write failure") + require.Zero(t, env.fake.requestCount(routeCreateAIProvider), "no create without a record") + got := &coderv1alpha1.CoderAIProvider{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got)) + require.Nil(t, got.Status.CreateAttemptTime, "no create marker either") + }) + t.Run("deployment ID unknown", func(t *testing.T) { + t.Parallel() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeBuildInfo, fakeFault{Status: http.StatusServiceUnavailable}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Zero(t, env.fake.requestCount(routeCreateAIProvider)) + require.Nil(t, cleanupRecord(ctx, t, env.namespace, got.UID)) + }) + t.Run("deployment ID not a UUID", func(t *testing.T) { + t.Parallel() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.mu.Lock() + env.fake.deploymentID = "https://coder.example.com" + env.fake.mu.Unlock() + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Zero(t, env.fake.requestCount(routeCreateAIProvider)) + }) +} + +// #353: a release without a delete in Coder keeps the record, so a later +// sweep can delete the provider. +func TestCoderAIProviderRecordKeptWithoutDelete(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "coder"}})) + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone, "ControlPlaneGone releases the finalizer") + _, ok := env.fake.aiProvider("openai") + require.True(t, ok, "the provider stays in Coder") + record := cleanupRecord(ctx, t, env.namespace, provider.UID) + require.Equal(t, provider.Status.ProviderID, record["coderID"], "the record stays for a sweep") +} + +// #353: a provider from before records existed gets one, with the deployment +// ID that Coder answers now. +func TestCoderAIProviderRecordBackfill(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}})) + builds := env.fake.requestCount(routeBuildInfo) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + record := cleanupRecord(ctx, t, env.namespace, provider.UID) + require.Equal(t, provider.Status.ProviderID, record["coderID"]) + require.Equal(t, env.fake.deploymentID, record["deploymentID"]) + require.Equal(t, builds+1, env.fake.requestCount(routeBuildInfo)) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "no new create") + + // A ConfigMap of that name that is not a record is never written over. + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}, configMap)) + configMap.Data["uid"] = strings.ToUpper(string(provider.UID)) + require.NoError(t, k8sClient.Update(ctx, configMap)) + _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.ErrorContains(t, err, "is not a valid cleanup record") +} diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go index 2fb44bcf2..2ac02db02 100644 --- a/internal/controller/controlplane_hold_test.go +++ b/internal/controller/controlplane_hold_test.go @@ -353,6 +353,7 @@ func TestReleasedWithoutDeleteMetricRecovered(t *testing.T) { // control plane revokes the managed workspace RBAC before it waits. func TestControlPlaneHoldRevokesWorkspaceRBAC(t *testing.T) { t.Parallel() + ensureGatewaySchemeRegistered(t) // The RBAC cleanup also reads HTTPRoutes. ctx := context.Background() namespace := createTestNamespace(ctx, t, "hold-rbac") workspaceNamespace := createTestNamespace(ctx, t, "hold-rbac-ws") diff --git a/internal/controller/fakecoder_test.go b/internal/controller/fakecoder_test.go index 3c53d4441..01824e4aa 100644 --- a/internal/controller/fakecoder_test.go +++ b/internal/controller/fakecoder_test.go @@ -28,6 +28,7 @@ const fakeCoderToken = "operator-session-token" // Route names for fakeCoder.failNext and fakeCoder.requestCount. const ( routeUser = "user" + routeBuildInfo = "buildInfo" routeOrganization = "organization" routeOrgMember = "organizationMember" routeTemplateByName = "templateByName" @@ -84,15 +85,17 @@ type fakeCoder struct { mu sync.Mutex now func() time.Time operatorID uuid.UUID - users map[uuid.UUID]codersdk.User - orgs map[uuid.UUID]codersdk.Organization - orgRoles map[uuid.UUID]map[uuid.UUID][]codersdk.SlimRole // Organization -> user -> roles. - templates map[uuid.UUID]codersdk.Template - versions map[uuid.UUID]codersdk.TemplateVersion - workspaces map[uuid.UUID]*fakeWorkspace - pool fakePoolState // Workspace pool routes, see fakecoder_pool_test.go. - faults map[string][]fakeFault - requests map[string]int // Route -> requests received. + // deploymentID is the deployment ID that /api/v2/buildinfo answers. + deploymentID string + users map[uuid.UUID]codersdk.User + orgs map[uuid.UUID]codersdk.Organization + orgRoles map[uuid.UUID]map[uuid.UUID][]codersdk.SlimRole // Organization -> user -> roles. + templates map[uuid.UUID]codersdk.Template + versions map[uuid.UUID]codersdk.TemplateVersion + workspaces map[uuid.UUID]*fakeWorkspace + pool fakePoolState // Workspace pool routes, see fakecoder_pool_test.go. + faults map[string][]fakeFault + requests map[string]int // Route -> requests received. aiProviders map[uuid.UUID]*fakeAIProvider // See fakecoder_ai_test.go. aiGateway fakeAIGatewayState // AI Gateway key routes, see fakecoder_aigateway_test.go. @@ -109,8 +112,12 @@ func newFakeCoder(t *testing.T) *fakeCoder { faults: map[string][]fakeFault{}, requests: map[string]int{}, aiProviders: map[uuid.UUID]*fakeAIProvider{}, } f.operatorID = f.addUser("operator", codersdk.LoginTypePassword, codersdk.RoleOwner) + f.deploymentID = uuid.NewString() mux := http.NewServeMux() + f.route(mux, "GET /api/v2/buildinfo", routeBuildInfo, func(*http.Request) (int, any) { + return http.StatusOK, codersdk.BuildInfoResponse{Version: "v2.37.3", DeploymentID: f.deploymentID} + }) f.route(mux, "GET /api/v2/users/{user}", routeUser, func(r *http.Request) (int, any) { return found(f.userByIdent(r.PathValue("user"))) }) From 07a012629f70d651414b9210a9b63b3fd238397b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 16:50:57 +0000 Subject: [PATCH 2/5] fix: read cleanup records live, backfill them on the disable path, delete only a valid record, and use the recorded provider ID on delete Change-Id: I43013d332be2b96542173c7871a73bdf8efe4957 Signed-off-by: Thomas Kosiewski --- .cspell.json | 1 + .github/workflows/ci.yaml | 2 +- internal/app/controllerapp/controllerapp.go | 1 + internal/controller/cleanup_record.go | 28 +++++--- .../controller/coderaiprovider_controller.go | 7 +- .../coderaiprovider_controller_test.go | 4 +- .../controller/coderaiprovider_create_test.go | 2 +- internal/controller/coderaiprovider_delete.go | 24 +++++-- .../controller/coderaiprovider_disable.go | 6 ++ internal/controller/coderaiprovider_record.go | 4 +- .../controller/coderaiprovider_record_test.go | 72 +++++++++++++++++++ 11 files changed, 130 insertions(+), 21 deletions(-) diff --git a/.cspell.json b/.cspell.json index df441da00..60da9e667 100644 --- a/.cspell.json +++ b/.cspell.json @@ -2,6 +2,7 @@ "version": "0.2", "language": "en", "words": [ + "buildinfo", "allapp", "automount", "prebuild", diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a759b8b11..626ed3807 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -94,7 +94,7 @@ jobs: # Shared Coder access that CoderTemplateTest and later pool controllers call. - 'internal/controller/controlplane_access.go' - 'internal/controller/controlplane_hold.go' - - 'internal/controller/cleanup_record*' + - 'internal/controller/cleanup_*' - 'internal/controller/coder_access.go' - 'internal/controller/coder_workspace_steps.go' # The URL that the aggregated API server and the controllers use for Coder API calls. diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index 1bbf7a88f..ef8b63f14 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -131,6 +131,7 @@ func SetupControllers(mgr manager.Manager) error { Scheme: managerScheme, // The operator's own namespace holds the cleanup records (#353). OperatorNamespace: detectLeaderElectionNamespace(), + APIReader: mgr.GetAPIReader(), } if err := aiProviderReconciler.SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create AI provider controller: %w", err) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go index 44eff4a80..b98c52d73 100644 --- a/internal/controller/cleanup_record.go +++ b/internal/controller/cleanup_record.go @@ -13,6 +13,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" ) // Cleanup records (#353 part 2, option B′). Before the operator creates @@ -60,6 +61,10 @@ type cleanupRecord struct { func cleanupRecordObjectName(uid types.UID) string { return cleanupRecordPrefix + string(uid) } +// errNotACleanupRecord marks a ConfigMap of a record's name that is not a +// valid record of that object. The operator never writes over it or deletes it. +var errNotACleanupRecord = errors.New("not a cleanup record of this object") + // coderDeploymentID reads Coder's deployment ID. Only a canonical UUID is // accepted: the ID decides which resources a sweep may delete. func coderDeploymentID(ctx context.Context, sdk *codersdk.Client) (string, error) { @@ -91,7 +96,7 @@ func readCleanupRecord(ctx context.Context, reader client.Reader, namespace stri return nil, nil, err } if record.UID != uid { - return nil, nil, fmt.Errorf("ConfigMap %s/%s is not the cleanup record of object %s", namespace, configMap.Name, uid) + return nil, nil, fmt.Errorf("ConfigMap %s/%s is not the cleanup record of object %s: %w", namespace, configMap.Name, uid, errNotACleanupRecord) } return record, configMap, nil } @@ -105,12 +110,12 @@ func parseCleanupRecord(configMap *corev1.ConfigMap) (*cleanupRecord, error) { } if configMap.Labels[cleanupRecordLabel] == "" || configMap.Labels[cleanupRecordLabel] != record.Kind || record.UID == "" || configMap.Name != cleanupRecordObjectName(record.UID) || record.DeploymentID == "" { - return nil, fmt.Errorf("ConfigMap %s/%s is not a valid cleanup record", configMap.Namespace, configMap.Name) + return nil, fmt.Errorf("ConfigMap %s/%s is not a valid cleanup record: %w", configMap.Namespace, configMap.Name, errNotACleanupRecord) } if t := d[cleanupRecordCreateSentAt]; t != "" { parsed, err := time.Parse(time.RFC3339, t) if err != nil { - return nil, fmt.Errorf("cleanup record %s/%s has an invalid %s: %w", configMap.Namespace, configMap.Name, cleanupRecordCreateSentAt, err) + return nil, fmt.Errorf("cleanup record %s/%s has an invalid %s (%w): %w", configMap.Namespace, configMap.Name, cleanupRecordCreateSentAt, err, errNotACleanupRecord) } record.CreateAttemptTime = parsed } @@ -152,13 +157,18 @@ func writeCleanupRecord(ctx context.Context, c client.Client, namespace string, } // deleteCleanupRecord removes the record of uid. Call it only after Coder -// confirmed that it holds no resource of the object. -func deleteCleanupRecord(ctx context.Context, c client.Client, namespace string, uid types.UID) error { - if namespace == "" || uid == "" { - return errors.New("assertion failed: a cleanup record needs the operator namespace and an object UID") +// confirmed that it holds no resource of the object. It reads the record +// through reader (live) and deletes only that ConfigMap (UID precondition). +// A ConfigMap of that name that is not a record of uid stays. +func deleteCleanupRecord(ctx context.Context, c client.Client, reader client.Reader, namespace string, uid types.UID) error { + record, configMap, err := readCleanupRecord(ctx, reader, namespace, uid) + if errors.Is(err, errNotACleanupRecord) { + log.FromContext(ctx).Error(err, "Leaving a ConfigMap that is not the cleanup record of this object") + return nil + } else if err != nil || record == nil { + return err } - configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: cleanupRecordObjectName(uid)}} - if err := c.Delete(ctx, configMap); err != nil && !apierrors.IsNotFound(err) { + if err := c.Delete(ctx, configMap, client.Preconditions{UID: &configMap.UID}); err != nil && !apierrors.IsNotFound(err) { return fmt.Errorf("delete cleanup record %s/%s: %w", namespace, configMap.Name, err) } return nil diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index 76bd7332b..ceb41cc24 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -56,6 +56,9 @@ type CoderAIProviderReconciler struct { // OperatorNamespace holds the cleanup records (#353): the operator's own // namespace, which only the operator can write ConfigMaps in. OperatorNamespace string + // APIReader reads the cleanup records live: a record written moments ago + // can be missing from the cache. + APIReader client.Reader sentKeys sync.Map // types.UID -> SHA-256 of the key that Coder holds. Memory only. } @@ -144,8 +147,8 @@ func (r *CoderAIProviderReconciler) providersWith(ctx context.Context, namespace // Reconcile runs the checks in order and reports the first one that fails. func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - if r.Client == nil || r.Scheme == nil || r.OperatorNamespace == "" { - return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client, a scheme and the operator namespace") + if r.Client == nil || r.Scheme == nil || r.OperatorNamespace == "" || r.APIReader == nil { + return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client, a scheme, the operator namespace and an API reader") } provider := &coderv1alpha1.CoderAIProvider{} if err := r.Get(ctx, req.NamespacedName, provider); err != nil { diff --git a/internal/controller/coderaiprovider_controller_test.go b/internal/controller/coderaiprovider_controller_test.go index 053347393..8ef2c0286 100644 --- a/internal/controller/coderaiprovider_controller_test.go +++ b/internal/controller/coderaiprovider_controller_test.go @@ -42,7 +42,7 @@ type aiProviderEnv struct { func newAIProviderEnv(ctx context.Context, t *testing.T, withControlPlane bool) aiProviderEnv { t.Helper() env := aiProviderEnv{namespace: createTestNamespace(ctx, t, "aip"), fake: newFakeCoder(t)} - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace, APIReader: k8sClient} if withControlPlane { createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) } @@ -384,7 +384,7 @@ func TestCoderAIProviderWatches(t *testing.T) { Controller: config.Controller{SkipNameValidation: ptrTo(true)}, }) require.NoError(t, err) - require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme(), OperatorNamespace: env.namespace}).SetupWithManager(mgr)) + require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme(), OperatorNamespace: env.namespace, APIReader: mgr.GetAPIReader()}).SetupWithManager(mgr)) mgrCtx, cancel := context.WithCancel(ctx) mgrDone := make(chan error, 1) go func() { mgrDone <- mgr.Start(mgrCtx) }() diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index 7b98baaea..e7053d147 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -39,7 +39,7 @@ func newAIProviderCreateEnv(ctx context.Context, t *testing.T) (aiProviderEnv, * // restart gives env a new reconciler, as a controller restart does. The // reconciler keeps no state between calls, so every step resumes from status. func (env *aiProviderEnv) restart() { - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace, APIReader: k8sClient} } func TestCoderAIProviderCreate(t *testing.T) { diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index b0866b7fd..b8361bf58 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -61,7 +61,7 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { // Nothing of this object can exist in Coder: earlier Coder answers // cleared the create marker, so the record goes too. - return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, provider.UID) } ref := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.ControlPlaneRef.Name} controlPlane := &coderv1alpha1.CoderControlPlane{} @@ -113,7 +113,7 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co // Coder confirmed that it holds no provider of this object. Only // then does the record go (#353 invariant 7). A failed record // delete retries, and the retry gets Coder's 404 for the provider. - return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, provider.UID) } if err != nil && deleting { // The attempt ended with an impossible answer. Log it, and still @@ -134,10 +134,26 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co // only the nonce and the type prove that the provider is this object's. func (r *CoderAIProviderReconciler) deleteInCoder(ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider) (*aiProviderStep, error) { id := provider.Status.ProviderID - if id != "" { - if err := checkAIProviderID(provider); err != nil { + if id == "" { + // A failed status write after the create can leave the provider ID + // only in the record. It proves ownership after the nonce is gone. + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) + if err != nil { return nil, err } + if record != nil && record.CoderID != "" { + id = record.CoderID + if parsed, err := uuid.Parse(id); err != nil || parsed == uuid.Nil || parsed.String() != id { + return nil, fmt.Errorf("assertion failed: the cleanup record of AI provider %s/%s has coderID %q, which is not a canonical UUID", provider.Namespace, provider.Name, id) + } + } + } + if id != "" { + if provider.Status.ProviderID != "" { + if err := checkAIProviderID(provider); err != nil { + return nil, err + } + } } else { found, err := sdk.AIProvider(ctx, provider.Name) switch { diff --git a/internal/controller/coderaiprovider_disable.go b/internal/controller/coderaiprovider_disable.go index acc08229d..d5b3f516d 100644 --- a/internal/controller/coderaiprovider_disable.go +++ b/internal/controller/coderaiprovider_disable.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/coder/coder/v2/codersdk" + "sigs.k8s.io/controller-runtime/pkg/log" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/credentialsecret" @@ -62,6 +63,11 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent( if step := checkAIProviderAnswer(ctx, provider, found); step != nil { return step, nil } + // A provider from before cleanup records gets its record here too. A + // failed record never delays the disable: the next reconcile retries it. + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); err != nil || step != nil { + log.FromContext(ctx).Info("Cannot write the cleanup record of a provider with a failing key Secret", "error", err, "step", step) + } if !found.Enabled { return stalled("Provider %s is disabled in Coder.", provider.Name), nil } diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go index fe28d0b16..81e36e662 100644 --- a/internal/controller/coderaiprovider_record.go +++ b/internal/controller/coderaiprovider_record.go @@ -21,7 +21,7 @@ const cleanupRecordKindAIProvider = "CoderAIProvider" func (r *CoderAIProviderReconciler) writeAIProviderRecord( ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, createAttempt *time.Time, ) (*aiProviderStep, error) { - previous, existing, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + previous, existing, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) if err != nil { return nil, err } @@ -60,7 +60,7 @@ func (r *CoderAIProviderReconciler) ensureAIProviderRecord( return nil, err } } - record, _, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) if err != nil { return nil, err } diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index 9cef7acec..9d62cb097 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -6,6 +6,7 @@ import ( "net/http" "strings" "testing" + "time" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -152,3 +153,74 @@ func TestCoderAIProviderRecordBackfill(t *testing.T) { _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) require.ErrorContains(t, err, "is not a valid cleanup record") } + +// #374 review: a provider from before records gets one also while its key +// Secret fails the credential contract and the controller disables it. +func TestCoderAIProviderRecordBackfillOnDisable(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}})) + editSecret(ctx, t, secret, func(s *corev1.Secret) { delete(s.Labels, "coder.com/credential") }) + got, _ := env.reconcile(ctx, t, provider) + stored, _ := env.fake.aiProvider("openai") + require.False(t, stored.enabled, "the disable still runs") + require.Equal(t, got.Status.ProviderID, cleanupRecord(ctx, t, env.namespace, provider.UID)["coderID"]) +} + +// #374 review: the controller reads records live. A cache that does not +// show the record yet must not make it create the record twice. +func TestCoderAIProviderRecordReadLive(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + env.r.Client = interceptor.NewClient(base, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.ConfigMap); ok { + return apierrors.NewNotFound(corev1.Resource("configmaps"), key.Name) // A stale cache. + } + return c.Get(ctx, key, obj, opts...) + }, + }) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + require.Equal(t, got.Status.ProviderID, cleanupRecord(ctx, t, env.namespace, provider.UID)["coderID"]) +} + +// #374 review: a ConfigMap of the record's name that is not this object's +// record is never deleted. +func TestCoderAIProviderDeleteKeepsForeignConfigMap(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + provider.Finalizers = []string{coderv1alpha1.CoderAIProviderCleanupFinalizer} + require.NoError(t, k8sClient.Update(ctx, provider)) + foreign := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}, Data: map[string]string{"owner": "someone else"}} + require.NoError(t, k8sClient.Create(ctx, foreign)) + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone, "nothing of the object exists in Coder, so the finalizer goes") + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(foreign), foreign), "the foreign ConfigMap stays") +} + +// #374 review: when the status write after a create failed, the record holds +// the only provider ID. A delete uses it even after the nonce is gone. +func TestCoderAIProviderDeleteUsesRecordedID(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + provider.Status.ProviderID = "" + provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now().Add(-time.Hour)} + require.NoError(t, k8sClient.Status().Update(ctx, provider)) + env.fake.mu.Lock() + for _, p := range env.fake.aiProviders { + p.displayName = "Renamed in the dashboard" + } + env.fake.mu.Unlock() + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone) + _, ok := env.fake.aiProvider("openai") + require.False(t, ok, "the recorded ID proves ownership") + require.Nil(t, cleanupRecord(ctx, t, env.namespace, provider.UID)) +} From 6e1d1bf90ff860c1c7825bbbfbbf36f875eddc2b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 17:13:28 +0000 Subject: [PATCH 3/5] fix: backfill records before every check, keep the record when the control plane reaches another deployment, and guard the setup Change-Id: Iee4add631b870c6070066c275f89f84ed86762fa Signed-off-by: Thomas Kosiewski --- .../controller/coderaiprovider_controller.go | 8 +++-- internal/controller/coderaiprovider_delete.go | 17 ++++++++++ .../controller/coderaiprovider_disable.go | 6 ---- internal/controller/coderaiprovider_record.go | 21 ++++++++++++ .../controller/coderaiprovider_record_test.go | 32 +++++++++++++++++++ 5 files changed, 75 insertions(+), 9 deletions(-) diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index ceb41cc24..c8f96fa1c 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -85,8 +85,8 @@ func aiProviderStalled(requeue time.Duration, reason, format string, args ...any // SetupWithManager wires the reconciler into controller-runtime. A change of // the referenced control plane or of the key Secret requeues the provider. func (r *CoderAIProviderReconciler) SetupWithManager(mgr ctrl.Manager) error { - if mgr == nil || r.Client == nil || r.Scheme == nil { - return fmt.Errorf("assertion failed: AI provider reconciler needs a manager, a client, and a scheme") + if mgr == nil || r.Client == nil || r.Scheme == nil || r.OperatorNamespace == "" || r.APIReader == nil { + return fmt.Errorf("assertion failed: AI provider reconciler needs a manager, a client, a scheme, the operator namespace and an API reader") } indexes := map[string]func(*coderv1alpha1.CoderAIProvider) string{ aiProviderControlPlaneRefIndex: func(p *coderv1alpha1.CoderAIProvider) string { return p.Spec.ControlPlaneRef.Name }, @@ -202,10 +202,12 @@ func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Requ // check validates the base URL, reads the key through the credential Secret // contract, gets Coder access, and then creates or applies the provider. The // checks before Coder access send nothing to Coder, except the disable of a -// provider whose key Secret fails the credential contract (#267, #290). +// provider whose key Secret fails the credential contract (#267, #290), and +// the deployment ID read of a record backfill. func (r *CoderAIProviderReconciler) check( ctx context.Context, provider *coderv1alpha1.CoderAIProvider, before *coderv1alpha1.CoderAIProviderStatus, ) (*aiProviderStep, error) { + r.backfillAIProviderRecord(ctx, provider) baseURL, err := aiProviderBaseURL(provider.Spec.BaseURL) if err != nil { // A fixed message: the parse error repeats the URL, which can hold diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index b8361bf58..353f6b0a2 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -108,6 +108,23 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co ctx, cancel = context.WithTimeout(ctx, aiProviderBoundedDeleteTimeout) defer cancel() } + // The record names the deployment that holds the provider. When the + // control plane now reaches another deployment, a delete there proves + // nothing, so the record stays for a sweep (#353 invariant 7). + if record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID); err != nil && !errors.Is(err, errNotACleanupRecord) { + return false, nil, err + } else if record != nil && (provider.Status.ProviderID == "" || checkAIProviderID(provider) == nil) { + // A forged provider ID fails its assertion below, before any request. + id, err := coderDeploymentID(ctx, sdk) + switch { + case err != nil && deleting: + return r.releaseWithoutDelete(ctx, provider, "the deployment ID read while CoderControlPlane is being deleted failed: "+coderErrorSummary(err)), nil, nil + case err != nil: + return false, aiProviderWait(aiProviderDeletePoll, "CoderUnavailable", "read the Coder deployment ID: %s", coderErrorSummary(err)), nil + case id != record.DeploymentID: + return r.releaseWithoutDelete(ctx, provider, fmt.Sprintf("CoderControlPlane now reaches Coder deployment %s, but the provider is in deployment %s", id, record.DeploymentID)), nil, nil + } + } step, err = r.deleteInCoder(ctx, sdk, provider) if err == nil && step == nil { // Coder confirmed that it holds no provider of this object. Only diff --git a/internal/controller/coderaiprovider_disable.go b/internal/controller/coderaiprovider_disable.go index d5b3f516d..acc08229d 100644 --- a/internal/controller/coderaiprovider_disable.go +++ b/internal/controller/coderaiprovider_disable.go @@ -6,7 +6,6 @@ import ( "fmt" "github.com/coder/coder/v2/codersdk" - "sigs.k8s.io/controller-runtime/pkg/log" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/credentialsecret" @@ -63,11 +62,6 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent( if step := checkAIProviderAnswer(ctx, provider, found); step != nil { return step, nil } - // A provider from before cleanup records gets its record here too. A - // failed record never delays the disable: the next reconcile retries it. - if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); err != nil || step != nil { - log.FromContext(ctx).Info("Cannot write the cleanup record of a provider with a failing key Secret", "error", err, "step", step) - } if !found.Enabled { return stalled("Provider %s is disabled in Coder.", provider.Name), nil } diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go index 81e36e662..24c9f40a2 100644 --- a/internal/controller/coderaiprovider_record.go +++ b/internal/controller/coderaiprovider_record.go @@ -5,6 +5,7 @@ import ( "time" "github.com/coder/coder/v2/codersdk" + "sigs.k8s.io/controller-runtime/pkg/log" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" ) @@ -45,6 +46,26 @@ func (r *CoderAIProviderReconciler) writeAIProviderRecord( return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) } +// backfillAIProviderRecord writes the missing record of a provider that +// exists in Coder from before records existed. It runs before every other +// check, so a stalled object (a bad base URL or key Secret) gets its record +// too. It never stops the reconcile: a failure is logged and retried. +func (r *CoderAIProviderReconciler) backfillAIProviderRecord(ctx context.Context, provider *coderv1alpha1.CoderAIProvider) { + if provider.Status.ProviderID == "" || checkAIProviderID(provider) != nil { + return // The create path writes the record. A forged ID fails later checks. + } + if record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID); err == nil && record != nil && record.CoderID == provider.Status.ProviderID { + return + } + sdk, _, wait, err := controlPlaneCoderClient(ctx, r.Client, provider.Namespace, provider.Spec.ControlPlaneRef.Name) + if err != nil || wait != nil { + return // The control plane cannot be reached now. A later reconcile retries. + } + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); err != nil || step != nil { + log.FromContext(ctx).Info("Cannot write the cleanup record of a provider that exists in Coder", "error", err, "step", step) + } +} + // ensureAIProviderRecord makes sure that a provider which can exist in Coder // has a record with its current provider ID. Objects from before records // existed get one here too. diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index 9d62cb097..50d8263ab 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -16,8 +17,10 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" ) // cleanupRecord returns the data of the cleanup record of uid in namespace, @@ -224,3 +227,32 @@ func TestCoderAIProviderDeleteUsesRecordedID(t *testing.T) { require.False(t, ok, "the recorded ID proves ownership") require.Nil(t, cleanupRecord(ctx, t, env.namespace, provider.UID)) } + +// #374 review round 2: when the control plane now reaches another Coder +// deployment (a new database), a delete there proves nothing. The finalizer +// goes without a delete, and the record stays for a sweep. +func TestCoderAIProviderDeleteAfterDeploymentSwitch(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() + env.fake.mu.Unlock() + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone) + require.Zero(t, env.fake.requestCount(routeDeleteAIProvider), "no delete in another deployment") + require.Equal(t, provider.Status.ProviderID, cleanupRecord(ctx, t, env.namespace, provider.UID)["coderID"], "the record stays") +} + +// The reconciler refuses to start without its cleanup record dependencies. +func TestCoderAIProviderSetupNeedsRecordDependencies(t *testing.T) { + t.Parallel() + for name, r := range map[string]*controller.CoderAIProviderReconciler{ + "no operator namespace": {Client: k8sClient, Scheme: scheme, APIReader: k8sClient}, + "no API reader": {Client: k8sClient, Scheme: scheme, OperatorNamespace: "coder-system"}, + } { + mgr, err := ctrl.NewManager(cfg, ctrl.Options{Scheme: scheme, Metrics: metricsserver.Options{BindAddress: "0"}, HealthProbeBindAddress: "0"}) + require.NoError(t, err) + require.ErrorContains(t, r.SetupWithManager(mgr), "assertion failed: AI provider reconciler needs", name) + } +} From 887b0f10dbe0f62ef7dc712ebc669fb210dafa64 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 17:53:17 +0000 Subject: [PATCH 4/5] fix: keep the record when the deployment changes under a live object, record the provider type, and default the record namespace to coder-system Change-Id: I7b699d65437effb2e94cefb175ee758d163b2c83 Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-providers.md | 2 ++ internal/app/controllerapp/controllerapp.go | 22 +++++++++++++++- .../controllerapp_internal_test.go | 22 ++++++++++++++++ internal/controller/cleanup_record.go | 8 ++++-- internal/controller/coderaiprovider_record.go | 10 ++++++- .../controller/coderaiprovider_record_test.go | 26 ++++++++++++++++++- 6 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 internal/app/controllerapp/controllerapp_internal_test.go diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 133479721..129ed21c0 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -131,6 +131,8 @@ Delete `CoderAIProvider` objects before their namespace: a namespace deletion re Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the provider's display-name nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. +When the control plane later reaches another Coder deployment (for example after its database Secret changed), the record still locates the provider in the old one. The operator then stalls the object with `CoderDeploymentChanged`, keeps the record and creates nothing in the new deployment. Delete the provider in the old deployment, then delete the ConfigMap. + The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays. Find these records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. ### Stuck finalizer diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index ef8b63f14..7f5030f4e 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -130,7 +130,7 @@ func SetupControllers(mgr manager.Manager) error { Client: client, Scheme: managerScheme, // The operator's own namespace holds the cleanup records (#353). - OperatorNamespace: detectLeaderElectionNamespace(), + OperatorNamespace: detectOperatorNamespace(), APIReader: mgr.GetAPIReader(), } if err := aiProviderReconciler.SetupWithManager(mgr); err != nil { @@ -206,6 +206,26 @@ func Run(ctx context.Context) error { return nil } +// defaultOperatorNamespace is the namespace of the installed operator, where +// the Role for the cleanup records lives (config/rbac). +const defaultOperatorNamespace = "coder-system" + +// detectOperatorNamespace returns the namespace that holds the cleanup +// records: POD_NAMESPACE, else the in-cluster namespace file, else +// coder-system, which the shipped Role names. Only that fallback differs +// from detectLeaderElectionNamespace. +func detectOperatorNamespace() string { + if ns := strings.TrimSpace(os.Getenv("POD_NAMESPACE")); ns != "" { + return ns + } + if data, err := os.ReadFile(inClusterNamespacePath); err == nil { + if ns := strings.TrimSpace(string(data)); ns != "" { + return ns + } + } + return defaultOperatorNamespace +} + // detectLeaderElectionNamespace returns the namespace to use for leader-election // lease objects. Resolution order: // 1. POD_NAMESPACE env var (allows explicit override for any environment). diff --git a/internal/app/controllerapp/controllerapp_internal_test.go b/internal/app/controllerapp/controllerapp_internal_test.go new file mode 100644 index 000000000..9f1b1377e --- /dev/null +++ b/internal/app/controllerapp/controllerapp_internal_test.go @@ -0,0 +1,22 @@ +package controllerapp + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +// #353: the cleanup records live in the namespace that the shipped Role +// names when neither POD_NAMESPACE nor the in-cluster namespace file says +// otherwise, also for a controller that runs outside the cluster. +func TestDetectOperatorNamespace(t *testing.T) { + if _, err := os.Stat(inClusterNamespacePath); err == nil { + t.Skip("this host has an in-cluster namespace file") + } + t.Setenv("POD_NAMESPACE", "") + require.Equal(t, "coder-system", detectOperatorNamespace()) + require.Equal(t, "kube-system", detectLeaderElectionNamespace(), "the leader-election fallback stays") + t.Setenv("POD_NAMESPACE", "ops") + require.Equal(t, "ops", detectOperatorNamespace()) +} diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go index b98c52d73..7465269a6 100644 --- a/internal/controller/cleanup_record.go +++ b/internal/controller/cleanup_record.go @@ -36,6 +36,7 @@ const ( cleanupRecordDeploymentID = "deploymentID" cleanupRecordNonce = "nonce" cleanupRecordCoderID = "coderID" + cleanupRecordType = "type" cleanupRecordCreateSentAt = "createAttemptTime" ) @@ -54,6 +55,9 @@ type cleanupRecord struct { Nonce string // CoderID is the Coder ID of the resource once the operator knows it. CoderID string + // Type is the provider type that the create sent. With the nonce it + // proves that a provider found by name is the object's. + Type string // CreateAttemptTime is when the operator last sent a create. A sweep // waits for the settle window after it. CreateAttemptTime time.Time @@ -106,7 +110,7 @@ func parseCleanupRecord(configMap *corev1.ConfigMap) (*cleanupRecord, error) { d := configMap.Data record := &cleanupRecord{ Kind: d[cleanupRecordKind], UID: types.UID(d[cleanupRecordUID]), Namespace: d[cleanupRecordNamespace], Name: d[cleanupRecordName], - DeploymentID: d[cleanupRecordDeploymentID], Nonce: d[cleanupRecordNonce], CoderID: d[cleanupRecordCoderID], + DeploymentID: d[cleanupRecordDeploymentID], Nonce: d[cleanupRecordNonce], CoderID: d[cleanupRecordCoderID], Type: d[cleanupRecordType], } if configMap.Labels[cleanupRecordLabel] == "" || configMap.Labels[cleanupRecordLabel] != record.Kind || record.UID == "" || configMap.Name != cleanupRecordObjectName(record.UID) || record.DeploymentID == "" { @@ -130,7 +134,7 @@ func writeCleanupRecord(ctx context.Context, c client.Client, namespace string, } data := map[string]string{ cleanupRecordKind: record.Kind, cleanupRecordUID: string(record.UID), cleanupRecordNamespace: record.Namespace, cleanupRecordName: record.Name, - cleanupRecordDeploymentID: record.DeploymentID, cleanupRecordNonce: record.Nonce, cleanupRecordCoderID: record.CoderID, + cleanupRecordDeploymentID: record.DeploymentID, cleanupRecordNonce: record.Nonce, cleanupRecordCoderID: record.CoderID, cleanupRecordType: record.Type, } if !record.CreateAttemptTime.IsZero() { data[cleanupRecordCreateSentAt] = record.CreateAttemptTime.UTC().Format(time.RFC3339) diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go index 24c9f40a2..cf4a96038 100644 --- a/internal/controller/coderaiprovider_record.go +++ b/internal/controller/coderaiprovider_record.go @@ -30,9 +30,17 @@ func (r *CoderAIProviderReconciler) writeAIProviderRecord( if err != nil { return aiProviderWait(aiProviderRetry, "CoderUnavailable", "read the Coder deployment ID for the cleanup record: %s", coderErrorSummary(err)), nil } + if previous != nil && previous.DeploymentID != deploymentID { + // The control plane now reaches another Coder database. The record + // is the only locator of what this object made in the old one, so + // the controller neither overwrites it nor creates in the new one. + return aiProviderStalled(aiProviderResync, "CoderDeploymentChanged", + "Provider %s (ID %q) is in Coder deployment %s, but CoderControlPlane %s now reaches deployment %s. Delete the provider there, then delete ConfigMap %s/%s. Until then the controller creates nothing.", + provider.Name, previous.CoderID, previous.DeploymentID, provider.Spec.ControlPlaneRef.Name, deploymentID, r.OperatorNamespace, cleanupRecordObjectName(provider.UID)), nil + } record := cleanupRecord{ Kind: cleanupRecordKindAIProvider, UID: provider.UID, Namespace: provider.Namespace, Name: provider.Name, - DeploymentID: deploymentID, Nonce: aiProviderNonce(provider), CoderID: provider.Status.ProviderID, + DeploymentID: deploymentID, Nonce: aiProviderNonce(provider), CoderID: provider.Status.ProviderID, Type: provider.Spec.Type, } if createAttempt != nil { record.CreateAttemptTime = *createAttempt diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index 50d8263ab..e40671e3b 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -56,7 +56,7 @@ func TestCoderAIProviderCleanupRecord(t *testing.T) { delete(record, "createAttemptTime") require.Equal(t, map[string]string{ "kind": "CoderAIProvider", "uid": string(got.UID), "namespace": env.namespace, "name": "openai", - "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": stored.id.String(), + "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": stored.id.String(), "type": "openai", }, record, "the provider ID is recorded before the sync replaces the nonce") got, _ = env.reconcile(ctx, t, got) @@ -256,3 +256,27 @@ func TestCoderAIProviderSetupNeedsRecordDependencies(t *testing.T) { require.ErrorContains(t, r.SetupWithManager(mgr), "assertion failed: AI provider reconciler needs", name) } } + +// #374 review round 3: when the control plane reaches another Coder +// deployment while the object lives, the provider looks gone, but the +// record still locates it in the old deployment. The controller keeps the +// record and creates nothing in the new deployment. +func TestCoderAIProviderDeploymentSwitchKeepsRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + before := cleanupRecord(ctx, t, env.namespace, provider.UID) + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() // A new database behind the same control plane. + for id := range env.fake.aiProviders { + delete(env.fake.aiProviders, id) + } + env.fake.mu.Unlock() + provider.Spec.DisplayName = "OpenAI (edited)" + require.NoError(t, k8sClient.Update(ctx, provider)) + got, _ := env.reconcile(ctx, t, provider) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireAIProviderStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"]) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "no create in the new deployment") + require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record keeps the old locator") +} From a7cd76a006d448e1f3f055df29d588dd61b0a68e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 18:23:28 +0000 Subject: [PATCH 5/5] fix: keep a record with a provider ID when the object is deleted with an empty status Change-Id: Ib7fa8097dbaf483d8f50c72676c746ceceabc1c1 Signed-off-by: Thomas Kosiewski --- internal/controller/coderaiprovider_delete.go | 14 +++++++++++--- internal/controller/coderaiprovider_record_test.go | 8 ++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 353f6b0a2..7a67c0864 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -59,9 +59,17 @@ func checkAIProviderID(provider *coderv1alpha1.CoderAIProvider) error { // the finalizer stays. func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *coderv1alpha1.CoderAIProvider) (release bool, step *aiProviderStep, err error) { if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { - // Nothing of this object can exist in Coder: earlier Coder answers - // cleared the create marker, so the record goes too. - return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, provider.UID) + // Earlier Coder answers cleared the create marker. A record with a + // provider ID still locates a provider, for example in a deployment + // that the control plane no longer reaches (CoderDeploymentChanged), + // so only a record without one proves that nothing exists. + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) + if err != nil && !errors.Is(err, errNotACleanupRecord) { + return false, nil, err + } + if record == nil || record.CoderID == "" { + return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, provider.UID) + } } ref := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.ControlPlaneRef.Name} controlPlane := &coderv1alpha1.CoderControlPlane{} diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index e40671e3b..b30c3a761 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -279,4 +279,12 @@ func TestCoderAIProviderDeploymentSwitchKeepsRecord(t *testing.T) { require.Contains(t, requireAIProviderStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"]) require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "no create in the new deployment") require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record keeps the old locator") + + // #374 review round 4: deleting the object in this state keeps the + // record too, so a sweep in the old deployment can still find it. + require.Empty(t, got.Status.ProviderID) + gone, _ := env.deleteAIProvider(ctx, t, got) + require.Nil(t, gone, "the finalizer goes without a delete") + require.Zero(t, env.fake.requestCount(routeDeleteAIProvider), "no delete in the new deployment") + require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record stays for a sweep") }