diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 8e42f958..811a683e 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -161,6 +161,13 @@ rules: - patch - update - watch +- apiGroups: + - events.k8s.io + resources: + - events + verbs: + - create + - patch - apiGroups: - gateway.networking.k8s.io resources: diff --git a/dist/install.yaml b/dist/install.yaml index 6d7880a6..5e28d8f4 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -7139,6 +7139,13 @@ rules: - patch - update - watch +- apiGroups: + - events.k8s.io + resources: + - events + verbs: + - create + - patch - apiGroups: - gateway.networking.k8s.io resources: diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 129ed21c..9271550e 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -121,11 +121,11 @@ These limits remain: Deleting the object deletes the provider in Coder. With only an unconfirmed create, the operator reads Coder first and deletes only a provider that this object created. A create request can still commit for a short time, so within 2 minutes of that request the operator reads again before it trusts that the provider is absent. - 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 the control plane is gone, the operator cannot delete the provider now. The finalizer goes at once, and the operator logs the provider ID. The [cleanup record](#cleanup-records) stays, so a later sweep can delete the provider. - 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. +Delete `CoderAIProvider` objects before their namespace: a namespace deletion removes Coder's pods at once, so the control plane cannot wait for them. A sweep deletes the leftover provider only when the namespace comes back with a control plane of the same Coder deployment. Coder deletes a provider even while chat models use it. Those models stop working. ### Cleanup records @@ -133,7 +133,16 @@ Before the operator creates a provider in Coder, it writes a cleanup record: a C 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. +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, and the operator sweeps it: + +1. It waits until the object with that UID is gone and 5 minutes have passed since the last create, so that a create still in flight commits first. +2. It looks for a ready `CoderControlPlane` in the object's namespace whose Coder answers the record's deployment ID now. It never matches by URL or name: a new Coder database behind the same URL has another deployment ID and is never swept. A control plane in another namespace never sweeps the record, because Coder reports its deployment ID itself and a control plane that someone else runs could claim any ID. +3. Through that control plane it deletes the recorded provider ID. Without a recorded ID, it deletes only the provider of that name whose display name is still the record's nonce. +4. It removes the record when Coder answers the delete with success or with its own 404. Any other answer keeps the record, and the operator retries every minute. + +So after a namespace deletion with an external database, the leftover provider goes once you create the namespace again with a control plane that uses the same database. The metric `coder_k8s_cleanup_records_swept_total` (label `kind`) counts the swept records. + +When no ready control plane reaches the record's deployment, the record stays. The gauge `coder_k8s_cleanup_records_unmatched` counts these records, and the operator writes a `NoMatchingDeployment` Warning Event on the ConfigMap every 10 minutes. When that Coder deployment never comes back, delete the provider in the Coder dashboard (the Event names the object, and the record holds the provider ID), then delete the ConfigMap. Find the 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 268040e6..09933be6 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -137,6 +137,17 @@ func SetupControllers(mgr manager.Manager) error { return fmt.Errorf("unable to create AI provider controller: %w", err) } + cleanupRecordReconciler := &controller.CleanupRecordReconciler{ + Client: client, + APIReader: mgr.GetAPIReader(), + OperatorNamespace: aiProviderReconciler.OperatorNamespace, + Clock: clock.RealClock{}, + Recorder: mgr.GetEventRecorder("coder-k8s-cleanup"), + } + if err := cleanupRecordReconciler.SetupWithManager(mgr); err != nil { + return fmt.Errorf("unable to create cleanup record controller: %w", err) + } + aiGatewayKeyReconciler := &controller.CoderAIGatewayKeyReconciler{ Client: client, APIReader: mgr.GetAPIReader(), diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go new file mode 100644 index 00000000..aebeec4e --- /dev/null +++ b/internal/controller/cleanup_sweep.go @@ -0,0 +1,328 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "strings" + "sync" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/events" + "k8s.io/utils/clock" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/metrics" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +const ( + // cleanupSettleWindow is how long after the last create a sweep waits, + // so a create still in flight commits first (#353 invariant 6). + cleanupSettleWindow = 5 * time.Minute + // cleanupObjectRecheck is how often a record of a living object is + // checked again. The delete of the object also wakes the record. + cleanupObjectRecheck = 30 * time.Minute + // cleanupUnmatchedRecheck is how often a record that no control plane + // of its deployment reaches is checked again. + cleanupUnmatchedRecheck = 10 * time.Minute + // cleanupRetry is the retry after a failed Coder call. + cleanupRetry = time.Minute +) + +var ( + cleanupRecordsSwept = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "coder_k8s_cleanup_records_swept_total", + Help: "Cleanup records whose resources a sweep deleted in Coder, or found gone, after their object went without a delete.", + }, []string{"kind"}) + cleanupRecordsUnmatched = prometheus.NewGaugeVec(prometheus.GaugeOpts{ + Name: "coder_k8s_cleanup_records_unmatched", + Help: "Cleanup records of gone objects that no ready control plane of their Coder deployment reaches.", + }, []string{"kind"}) +) + +func init() { + metrics.Registry.MustRegister(cleanupRecordsSwept, cleanupRecordsUnmatched) +} + +// +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch + +// CleanupRecordReconciler sweeps the cleanup records of objects that went +// without a confirmed delete in Coder (#353). It acts only through a ready +// control plane whose current Coder deployment ID equals the record's, and +// it deletes only the recorded provider ID or the provider with the +// recorded nonce. +type CleanupRecordReconciler struct { + client.Client + // APIReader reads the object of a record live, so a stale cache never + // makes a living object look gone. + APIReader client.Reader + OperatorNamespace string + Clock clock.Clock + Recorder events.EventRecorder + + unmatched sync.Map // types.UID -> kind of a record that no deployment reaches. +} + +// SetupWithManager watches the records in the operator namespace. It wakes a +// record when its CoderAIProvider is deleted, and every record when a control +// plane gets operator access, since that control plane can match them. +func (r *CleanupRecordReconciler) SetupWithManager(mgr ctrl.Manager) error { + if mgr == nil || r.Client == nil || r.APIReader == nil || r.OperatorNamespace == "" || r.Clock == nil || r.Recorder == nil { + return errors.New("assertion failed: the cleanup record reconciler needs a manager, a client, an API reader, the operator namespace, a clock and an event recorder") + } + isRecord := predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetNamespace() == r.OperatorNamespace && obj.GetLabels()[cleanupRecordLabel] != "" + }) + return ctrl.NewControllerManagedBy(mgr). + For(&corev1.ConfigMap{}, builder.WithPredicates(isRecord)). + Watches(&coderv1alpha1.CoderAIProvider{}, handler.EnqueueRequestsFromMapFunc(func(_ context.Context, obj client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: types.NamespacedName{Namespace: r.OperatorNamespace, Name: cleanupRecordObjectName(obj.GetUID())}}} + }), builder.WithPredicates(dependentRemoved)). + Watches(&coderv1alpha1.CoderControlPlane{}, handler.EnqueueRequestsFromMapFunc(r.allRecords), builder.WithPredicates(operatorAccessGained)). + Named("cleanuprecord"). + Complete(r) +} + +// operatorAccessGained passes a control plane that has operator access, or +// whose access or URL changed. +var operatorAccessGained = predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + cp, ok := e.Object.(*coderv1alpha1.CoderControlPlane) + return ok && cp.Status.OperatorAccessReady + }, + UpdateFunc: func(e event.UpdateEvent) bool { + before, ok1 := e.ObjectOld.(*coderv1alpha1.CoderControlPlane) + after, ok2 := e.ObjectNew.(*coderv1alpha1.CoderControlPlane) + return ok1 && ok2 && after.Status.OperatorAccessReady && + (!before.Status.OperatorAccessReady || before.Status.URL != after.Status.URL) + }, + DeleteFunc: func(event.DeleteEvent) bool { return false }, + GenericFunc: func(event.GenericEvent) bool { return false }, +} + +// allRecords returns every record in the operator namespace. +func (r *CleanupRecordReconciler) allRecords(ctx context.Context, _ client.Object) []reconcile.Request { + records := &corev1.ConfigMapList{} + if err := r.List(ctx, records, client.InNamespace(r.OperatorNamespace), client.HasLabels{cleanupRecordLabel}); err != nil { + log.FromContext(ctx).Error(err, "Cannot list the cleanup records") + return nil + } + requests := make([]reconcile.Request, 0, len(records.Items)) + for _, record := range records.Items { + requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: record.Namespace, Name: record.Name}}) + } + return requests +} + +// Reconcile sweeps one record. +func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + if req.Namespace != r.OperatorNamespace { + return ctrl.Result{}, nil + } + // A live read: the cache can still hold an older create time. + configMap := &corev1.ConfigMap{} + if err := r.APIReader.Get(ctx, req.NamespacedName, configMap); apierrors.IsNotFound(err) { + if uid, ok := strings.CutPrefix(req.Name, cleanupRecordPrefix); ok { + r.setUnmatched(types.UID(uid), "", false) + } + return ctrl.Result{}, nil + } else if err != nil { + return ctrl.Result{}, err + } + record, err := parseCleanupRecord(configMap) + if err != nil { + // Never act on a record that the operator did not write. + log.FromContext(ctx).Error(err, "Ignoring a ConfigMap that is not a valid cleanup record") + return ctrl.Result{}, nil + } + if record.Kind != cleanupRecordKindAIProvider { + return ctrl.Result{}, nil // Only provider records are swept so far. + } + + // Act only when the object with that UID no longer exists (invariant 6). + provider := &coderv1alpha1.CoderAIProvider{} + if err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: record.Namespace, Name: record.Name}, provider); err == nil && provider.UID == record.UID { + r.setUnmatched(record.UID, record.Kind, false) + return ctrl.Result{RequeueAfter: cleanupObjectRecheck}, nil + } else if err != nil && !apierrors.IsNotFound(err) { + return ctrl.Result{}, err + } + settleFrom := record.CreateAttemptTime + if settleFrom.IsZero() { + settleFrom = configMap.CreationTimestamp.Time + } + if wait := settleFrom.Add(cleanupSettleWindow).Sub(r.Clock.Now()); wait > 0 { + return ctrl.Result{RequeueAfter: wait}, nil + } + + matches, unknown, err := r.matchingControlPlanes(ctx, record) + if err != nil { + return ctrl.Result{}, err + } + if len(matches) == 0 && unknown { + // A ready control plane did not answer its deployment ID, for + // example while its Coder pods start. It can still match. + return ctrl.Result{RequeueAfter: cleanupRetry}, nil + } + if len(matches) == 0 { + r.setUnmatched(record.UID, record.Kind, true) + r.Recorder.Eventf(configMap, nil, corev1.EventTypeWarning, "NoMatchingDeployment", "Sweep", + "No ready CoderControlPlane in namespace %s reaches Coder deployment %s, which holds the provider of deleted %s %s/%s. Delete the provider in Coder by hand, then delete this ConfigMap.", + record.Namespace, record.DeploymentID, record.Kind, record.Namespace, record.Name) + return ctrl.Result{RequeueAfter: cleanupUnmatchedRecheck}, nil + } + r.setUnmatched(record.UID, record.Kind, false) + logger := log.FromContext(ctx).WithValues("record", configMap.Name, "deploymentID", record.DeploymentID, "provider", record.Namespace+"/"+record.Name) + // Each matching control plane has its own operator token, so a failure + // through one tries the next. + gone, why, through := false, "", logger + for _, m := range matches { + through = logger.WithValues("controlPlane", types.NamespacedName{Namespace: m.controlPlane.Namespace, Name: m.controlPlane.Name}) + if gone, why, err = sweepAIProvider(ctx, m.sdk, record); err != nil { + return ctrl.Result{}, err + } else if gone { + break + } + through.Info("The cleanup of a deleted provider in Coder failed", "why", why) + } + if !gone { + return ctrl.Result{RequeueAfter: cleanupRetry}, nil + } + // Coder confirmed the delete in the matching deployment (invariant 7). + if err := deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, record.UID); err != nil { + return ctrl.Result{}, err + } + cleanupRecordsSwept.WithLabelValues(record.Kind).Inc() + through.Info("Swept the provider of a deleted CoderAIProvider in Coder", "result", why, "providerID", record.CoderID) + return ctrl.Result{}, nil +} + +// controlPlaneMatch is a control plane whose Coder deployment is a record's. +type controlPlaneMatch struct { + sdk *codersdk.Client + controlPlane *coderv1alpha1.CoderControlPlane +} + +// matchingControlPlanes returns the Coder clients of the ready control planes +// in the record's namespace whose current deployment ID is the record's +// (#353 invariant 4). The ID comes from Coder now, never +// from the record or a URL. unknown reports a ready control plane whose +// deployment ID read failed. +// +// Only the record's own namespace counts: Coder reports its deployment ID +// itself, so a control plane that someone else runs could claim any ID. The +// records of a namespace, and the control planes that sweep them, are then +// under the control of the same people, and a sweep can only delete what +// their own operator access could delete. +func (r *CleanupRecordReconciler) matchingControlPlanes(ctx context.Context, record *cleanupRecord) ([]controlPlaneMatch, bool, error) { + deploymentID := record.DeploymentID + controlPlanes := &coderv1alpha1.CoderControlPlaneList{} + if err := r.List(ctx, controlPlanes, client.InNamespace(record.Namespace)); err != nil { + return nil, false, fmt.Errorf("list control planes: %w", err) + } + unknown := false + var matches []controlPlaneMatch + for i := range controlPlanes.Items { + cp := &controlPlanes.Items[i] + // A live read: the cache can show the control plane ready before + // it shows its new operator token Secret. + sdk, _, wait, err := controlPlaneCoderClient(ctx, r.APIReader, cp.Namespace, cp.Name) + if err != nil || wait != nil { + // A control plane that reports operator access can still match + // once its access works. + unknown = unknown || cp.Status.OperatorAccessReady + continue + } + id, err := coderDeploymentID(ctx, sdk) + if err != nil { + log.FromContext(ctx).V(1).Info("Cannot read the deployment ID of a control plane", "controlPlane", cp.Namespace+"/"+cp.Name, "error", coderErrorSummary(err)) + unknown = true + continue + } + if id == deploymentID { + matches = append(matches, controlPlaneMatch{sdk: sdk, controlPlane: cp}) + } + } + return matches, unknown, nil +} + +// sweepAIProvider deletes the provider of record in Coder. It returns gone +// when Coder confirms that it holds no provider of the record: a delete +// answered with success or Coder's own 404, a read with Coder's own 404 for +// a site owner, or a provider of that name without the record's nonce. It +// deletes only the recorded ID or the provider whose display name is the +// recorded nonce (invariant 5). +func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (gone bool, why string, err error) { + if record.Nonce != aiProviderNoncePrefix+string(record.UID) { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) + } + if step := requireSiteOwner(ctx, sdk); step != nil { + // Only a site owner can trust Coder's 404 for a provider. + return false, step.message, nil + } + id := record.CoderID + if id != "" { + if parsed, err := uuid.Parse(id); err != nil || parsed == uuid.Nil || parsed.String() != id { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has coderID %q, which is not a canonical UUID", record.UID, id) + } + } else { + found, err := sdk.AIProvider(ctx, record.Name) + switch { + case isCoderResourceNotFound(err): + return true, "no provider of that name", nil + case err != nil: + return false, "get provider: " + coderErrorSummary(err), nil + } + if err := coderAnswerFor("AI provider", record.Name, found.Name); err != nil { + return false, err.Error(), nil //nolint:nilerr // A wrong answer retries the sweep. + } + if found.DisplayName != record.Nonce || string(found.Type) != record.Type { + // Another provider holds the name, so no create of this object + // committed after the settle window. The nonce and the type + // prove ownership, as on the controller's own paths. + return true, "the provider of that name is not this object's", nil + } + if found.ID == uuid.Nil { + return false, "", errors.New("assertion failed: Coder answered an AI provider without an ID") + } + id = found.ID.String() + } + if err := sdk.DeleteAIProvider(ctx, id); err != nil && !isCoderResourceNotFound(err) { + return false, "delete provider " + id + ": " + coderErrorSummary(err), nil + } else if err != nil { + return true, "provider " + id + " was already gone", nil //nolint:nilerr // Coder's own 404 confirms the delete. + } + return true, "deleted provider " + id, nil +} + +// setUnmatched keeps the unmatched gauge equal to the records that no +// deployment reaches. +func (r *CleanupRecordReconciler) setUnmatched(uid types.UID, kind string, unmatched bool) { + if unmatched { + r.unmatched.Store(uid, kind) + } else { + r.unmatched.Delete(uid) + } + counts := map[string]float64{cleanupRecordKindAIProvider: 0} + r.unmatched.Range(func(_, v any) bool { + counts[v.(string)]++ + return true + }) + for k, n := range counts { + cleanupRecordsUnmatched.WithLabelValues(k).Set(n) + } +} diff --git a/internal/controller/cleanup_sweep_internal_test.go b/internal/controller/cleanup_sweep_internal_test.go new file mode 100644 index 00000000..1c7cb4fb --- /dev/null +++ b/internal/controller/cleanup_sweep_internal_test.go @@ -0,0 +1,26 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/event" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// #353: a control plane wakes the cleanup records when it gets operator +// access or a new URL, since it can then match their deployment. +func TestOperatorAccessGained(t *testing.T) { + t.Parallel() + cp := func(ready bool, url string) *coderv1alpha1.CoderControlPlane { + return &coderv1alpha1.CoderControlPlane{Status: coderv1alpha1.CoderControlPlaneStatus{OperatorAccessReady: ready, URL: url}} + } + require.True(t, operatorAccessGained.Create(event.CreateEvent{Object: cp(true, "http://a")})) + require.False(t, operatorAccessGained.Create(event.CreateEvent{Object: cp(false, "http://a")})) + require.True(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(false, "http://a"), ObjectNew: cp(true, "http://a")})) + require.True(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(true, "http://a"), ObjectNew: cp(true, "http://b")})) + require.False(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(true, "http://a"), ObjectNew: cp(true, "http://a")}), "no wake on every status write") + require.False(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(true, "http://a"), ObjectNew: cp(false, "http://a")})) + require.False(t, operatorAccessGained.Delete(event.DeleteEvent{Object: cp(true, "http://a")})) +} diff --git a/internal/controller/cleanup_sweep_test.go b/internal/controller/cleanup_sweep_test.go new file mode 100644 index 00000000..9b5a8b8a --- /dev/null +++ b/internal/controller/cleanup_sweep_test.go @@ -0,0 +1,322 @@ +package controller_test + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "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/client-go/tools/events" + clocktesting "k8s.io/utils/clock/testing" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/metrics" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" +) + +// sweepEnv is a provider that went with its control plane, as in a +// namespace deletion: Coder keeps the provider and the record stays. +type sweepEnv struct { + aiProviderEnv + provider *coderv1alpha1.CoderAIProvider + clock *clocktesting.FakeClock + recorder *events.FakeRecorder + sweeper *controller.CleanupRecordReconciler +} + +func newSweepEnv(ctx context.Context, t *testing.T) sweepEnv { + t.Helper() + 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) + require.NotNil(t, cleanupRecord(ctx, t, env.namespace, provider.UID)) + return newSweeper(t, env, provider) +} + +// newSweeper returns a sweeper for the record of provider in env. +func newSweeper(t *testing.T, env aiProviderEnv, provider *coderv1alpha1.CoderAIProvider) sweepEnv { + t.Helper() + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + s := sweepEnv{aiProviderEnv: env, provider: provider, clock: clocktesting.NewFakeClock(time.Now()), recorder: events.NewFakeRecorder(10)} + s.sweeper = &controller.CleanupRecordReconciler{Client: base, APIReader: k8sClient, OperatorNamespace: env.namespace, Clock: s.clock, Recorder: s.recorder} + return s +} + +// providerMetric reads a controller metric with the label kind=CoderAIProvider. +func providerMetric(t *testing.T, name string) float64 { + t.Helper() + families, err := metrics.Registry.Gather() + require.NoError(t, err) + for _, f := range families { + for _, m := range f.GetMetric() { + for _, l := range m.GetLabel() { + if f.GetName() == name && l.GetName() == "kind" && l.GetValue() == "CoderAIProvider" { + if m.GetGauge() != nil { + return m.GetGauge().GetValue() + } + return m.GetCounter().GetValue() + } + } + } + } + return 0 +} + +func (s sweepEnv) sweep(ctx context.Context, t *testing.T) ctrl.Result { + t.Helper() + result, err := s.sweeper.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}}) + require.NoError(t, err) + return result +} + +// #353: after a namespace deletion, a new control plane that reaches the +// same Coder deployment deletes the leftover provider, once the settle +// window has passed. A different deployment at the same URL never does. +// Not parallel: it reads process-wide metrics. +func TestCleanupSweepAIProvider(t *testing.T) { + ctx := context.Background() + s := newSweepEnv(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.True(t, ok, "the provider is left in Coder") + swept := providerMetric(t, "coder_k8s_cleanup_records_swept_total") + + requests := s.fake.totalRequests() + result := s.sweep(ctx, t) + require.Equal(t, requests, s.fake.totalRequests(), "no Coder call inside the settle window") + require.Greater(t, result.RequeueAfter, 4*time.Minute, "the settle window has not passed") + require.LessOrEqual(t, result.RequeueAfter, 5*time.Minute, "it waits for the end of the settle window") + deletes := s.fake.requestCount(routeDeleteAIProvider) + s.clock.Step(6 * time.Minute) + result = s.sweep(ctx, t) + require.Equal(t, 10*time.Minute, result.RequeueAfter, "no control plane reaches the deployment") + require.Equal(t, deletes, s.fake.requestCount(routeDeleteAIProvider)) + + // A new control plane at the same URL, but another deployment answers: + // for example a new Coder database behind the old address. + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + recorded := s.fake.deploymentID + s.fake.mu.Lock() + s.fake.deploymentID = uuid.NewString() + s.fake.mu.Unlock() + result = s.sweep(ctx, t) + require.Equal(t, 10*time.Minute, result.RequeueAfter) + require.Equal(t, deletes, s.fake.requestCount(routeDeleteAIProvider), "a different deployment is never swept") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + require.Contains(t, <-s.recorder.Events, "NoMatchingDeployment") + require.Equal(t, 1.0, providerMetric(t, "coder_k8s_cleanup_records_unmatched")) + + // The same deployment comes back. + s.fake.mu.Lock() + s.fake.deploymentID = recorded + s.fake.mu.Unlock() + result = s.sweep(ctx, t) + require.Zero(t, result.RequeueAfter) + _, ok = s.fake.aiProvider("openai") + require.False(t, ok, "the sweep deleted the provider") + require.Equal(t, deletes+1, s.fake.requestCount(routeDeleteAIProvider)) + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID), "Coder confirmed the delete, so the record goes") + require.Equal(t, swept+1, providerMetric(t, "coder_k8s_cleanup_records_swept_total")) + require.Zero(t, providerMetric(t, "coder_k8s_cleanup_records_unmatched")) +} + +// #353 invariants 5 and 6: a sweep never touches the provider of a living +// object, finds a provider without a recorded ID only by its nonce, and +// leaves a provider of the same name that the object did not create. +func TestCleanupSweepAIProviderCases(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("living object", func(t *testing.T) { + t.Parallel() + env, _, provider := newReadyAIProvider(ctx, t) + s := newSweeper(t, env, provider) + s.clock.Step(time.Hour) + requests := s.fake.totalRequests() + require.Equal(t, 30*time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Equal(t, requests, s.fake.totalRequests(), "no Coder call for a living object") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, provider.UID)) + }) + t.Run("same name, another object", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.createProvider(ctx, t, nil) // A new object with the old name and a new UID. + s.clock.Step(6 * time.Minute) + s.sweep(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.False(t, ok, "only the UID decides that the object is gone") + }) + for name, nonce := range map[string]bool{"nonce matches": true, "nonce differs": false} { + t.Run(name, func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + configMap.Data["coderID"] = "" // The create committed, but its ID was never recorded. + require.NoError(t, k8sClient.Update(ctx, configMap)) + if nonce { + s.fake.mu.Lock() + for _, p := range s.fake.aiProviders { + p.displayName = "coder-k8s:" + string(s.provider.UID) + } + s.fake.mu.Unlock() + } + s.clock.Step(6 * time.Minute) + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + _, ok := s.fake.aiProvider("openai") + require.Equal(t, !nonce, ok, "only the provider with the record's nonce is deleted") + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID), "Coder holds no provider of the record") + }) + } + t.Run("delete fails", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.failNext(routeDeleteAIProvider, fakeFault{Status: http.StatusBadGateway, HTML: "bad gateway"}) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter) + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID), "the record stays until Coder confirms the delete") + s.fake.failNext(routeDeleteAIProvider, fakeFault{Status: http.StatusNotFound, HTML: "not found"}) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter, "only Coder's own 404 confirms") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("deployment ID not answered yet", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.failNext(routeBuildInfo, fakeFault{Status: http.StatusBadGateway, HTML: "starting"}) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter, "a control plane that does not answer yet can still match") + require.Empty(t, s.recorder.Events, "no NoMatchingDeployment Event for an unknown deployment") + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("control plane in another namespace", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + // Another namespace runs a control plane that answers the record's + // deployment ID, as a spoofing Coder could. + createTestControlPlane(ctx, t, createTestNamespace(ctx, t, "aip-other"), "coder", s.fake.server.URL) + s.clock.Step(6 * time.Minute) + require.Equal(t, 10*time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Zero(t, s.fake.requestCount(routeDeleteAIProvider), "only a control plane in the object's namespace sweeps") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("first matching control plane has no site owner", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + other := newFakeCoder(t) // A second Coder on the same database: same deployment, its own users. + other.mu.Lock() + other.deploymentID = s.fake.deploymentID + other.mu.Unlock() + other.updateUser(other.operatorID, func(u *codersdk.User) { u.Roles = []codersdk.SlimRole{} }) + createTestControlPlane(ctx, t, s.namespace, "coder-a", other.server.URL) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.clock.Step(6 * time.Minute) + require.Zero(t, s.sweep(ctx, t).RequeueAfter, "the next matching control plane sweeps") + _, ok := s.fake.aiProvider("openai") + require.False(t, ok) + }) + t.Run("same name and nonce, another type", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + configMap.Data["coderID"] = "" + require.NoError(t, k8sClient.Update(ctx, configMap)) + s.fake.mu.Lock() + for _, p := range s.fake.aiProviders { + p.displayName, p.providerType = "coder-k8s:"+string(s.provider.UID), codersdk.AIProviderTypeAnthropic + } + s.fake.mu.Unlock() + s.clock.Step(6 * time.Minute) + s.sweep(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.True(t, ok, "only the nonce and the recorded type prove ownership") + }) + t.Run("stale cache of the record", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + // A newer create attempt, written right before; a stale cache would + // still show the old time. + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + old := configMap.DeepCopy() + configMap.Data["createAttemptTime"] = s.clock.Now().Add(6 * time.Minute).UTC().Format(time.RFC3339) + require.NoError(t, k8sClient.Update(ctx, configMap)) + cached := s.sweeper.Client + s.sweeper.Client = interceptor.NewClient(cached.(client.WithWatch), interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if cm, ok := obj.(*corev1.ConfigMap); ok { + old.DeepCopyInto(cm) + return nil + } + return c.Get(ctx, key, obj, opts...) + }, + }) + s.clock.Step(6 * time.Minute) + require.Greater(t, s.sweep(ctx, t).RequeueAfter, time.Duration(0), "the sweep waits for the settle window after the newest create") + _, ok := s.fake.aiProvider("openai") + require.True(t, ok) + }) + t.Run("stale cache of the token Secret", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + cached := s.sweeper.Client + s.sweeper.Client = interceptor.NewClient(cached.(client.WithWatch), interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.Secret); ok { + return apierrors.NewNotFound(corev1.Resource("secrets"), key.Name) // Not in the cache yet. + } + return c.Get(ctx, key, obj, opts...) + }, + }) + s.clock.Step(6 * time.Minute) + require.Zero(t, s.sweep(ctx, t).RequeueAfter, "the sweep reads the operator access live") + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("setup needs a manager", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + require.ErrorContains(t, s.sweeper.SetupWithManager(nil), "assertion failed: the cleanup record reconciler needs a manager") + }) + t.Run("operator not a site owner", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.updateUser(s.fake.operatorID, func(u *codersdk.User) { u.Roles = []codersdk.SlimRole{} }) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Zero(t, s.fake.requestCount(routeDeleteAIProvider), "only a site owner can trust Coder's 404") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("not a record", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + configMap.Labels["coder.com/cleanup-record"] = "CoderAIGatewayKey" + require.NoError(t, k8sClient.Update(ctx, configMap)) + s.clock.Step(6 * time.Minute) + s.sweep(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.True(t, ok, "a ConfigMap that is not a valid record is never acted on") + }) +}