diff --git a/.cspell.json b/.cspell.json index 0d0ebbb3..96b92d91 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 14926786..5a1860ea 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -95,6 +95,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_*' - '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 26972f97..f6923d88 100644 --- a/config/default/kustomization.yaml +++ b/config/default/kustomization.yaml @@ -10,6 +10,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 00000000..5572237f --- /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 c6aa1f0e..8e42f958 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -198,3 +198,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 6cfcce5f..6d7880a6 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6960,6 +6960,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 @@ -7160,6 +7178,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 52533e09..dd511f2e 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`, `CoderAIGatewayKey`, and `CoderMCPServer` | -| `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/admission/` | Opt-in admission policies that the install bundle does not apply, for example the operator-only status write policy for `CoderMCPServer` | | `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`) | diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index b9427ee6..62e98555 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 e1879d0f..129ed21c 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -122,11 +122,19 @@ 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. + +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 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 defca81c..268040e6 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -129,6 +129,9 @@ func SetupControllers(mgr manager.Manager) error { aiProviderReconciler := &controller.CoderAIProviderReconciler{ Client: client, Scheme: managerScheme, + // The operator's own namespace holds the cleanup records (#353). + OperatorNamespace: detectOperatorNamespace(), + APIReader: mgr.GetAPIReader(), } if err := aiProviderReconciler.SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create AI provider controller: %w", err) @@ -211,6 +214,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 00000000..9f1b1377 --- /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 new file mode 100644 index 00000000..7465269a --- /dev/null +++ b/internal/controller/cleanup_record.go @@ -0,0 +1,179 @@ +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" + "sigs.k8s.io/controller-runtime/pkg/log" +) + +// 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" + cleanupRecordType = "type" + 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 + // 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 +} + +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) { + 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: %w", namespace, configMap.Name, uid, errNotACleanupRecord) + } + 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], Type: d[cleanupRecordType], + } + 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: %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): %w", configMap.Namespace, configMap.Name, cleanupRecordCreateSentAt, err, errNotACleanupRecord) + } + 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, cleanupRecordType: record.Type, + } + 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. 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 + } + 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 b65d3808..c8f96fa1 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,13 @@ 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 + // 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. } @@ -77,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 }, @@ -139,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 { - 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 == "" || 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 { @@ -194,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 @@ -223,6 +233,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 +258,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 515046e0..8ef2c028 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, 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()}).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.go b/internal/controller/coderaiprovider_create.go index 0348d1f2..c946a211 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 3c68c891..e7053d14 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, APIReader: k8sClient} } func TestCoderAIProviderCreate(t *testing.T) { diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 527b0d30..7a67c086 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -59,8 +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. - return true, nil, nil + // 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{} @@ -107,7 +116,31 @@ 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 { + // 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 + // 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.APIReader, 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") @@ -126,10 +159,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_record.go b/internal/controller/coderaiprovider_record.go new file mode 100644 index 00000000..cf4a9603 --- /dev/null +++ b/internal/controller/coderaiprovider_record.go @@ -0,0 +1,100 @@ +package controller + +import ( + "context" + "time" + + "github.com/coder/coder/v2/codersdk" + "sigs.k8s.io/controller-runtime/pkg/log" + + 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.APIReader, 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 + } + 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, Type: provider.Spec.Type, + } + 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) +} + +// 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. +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.APIReader, 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 00000000..b30c3a76 --- /dev/null +++ b/internal/controller/coderaiprovider_record_test.go @@ -0,0 +1,290 @@ +package controller_test + +import ( + "context" + "errors" + "net/http" + "strings" + "testing" + "time" + + "github.com/google/uuid" + "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" + 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, +// 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(), "type": "openai", + }, 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") +} + +// #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)) +} + +// #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) + } +} + +// #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") + + // #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") +} diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go index 2fb44bcf..2ac02db0 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 3c53d444..01824e4a 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"))) })