From 4ae1100505960058e0ab4293468c2686bfd62587 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:27:36 +0000 Subject: [PATCH 01/14] test: provider and MCP create adoption must not trust the object UID (#426) Change-Id: I09458d1f910f09b9016af22eef8325af09f82e5d Signed-off-by: Thomas Kosiewski --- .../controller/coderaiprovider_nonce_test.go | 153 ++++++++++++++++++ .../controller/codermcpserver_nonce_test.go | 120 ++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 internal/controller/coderaiprovider_nonce_test.go create mode 100644 internal/controller/codermcpserver_nonce_test.go diff --git a/internal/controller/coderaiprovider_nonce_test.go b/internal/controller/coderaiprovider_nonce_test.go new file mode 100644 index 00000000..9c2f5734 --- /dev/null +++ b/internal/controller/coderaiprovider_nonce_test.go @@ -0,0 +1,153 @@ +package controller_test + +import ( + "context" + "net/http" + "strings" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// createOldCleanupRecord writes a cleanup record as an operator version +// before #426 wrote it, with the nonce coder-k8s:. +func createOldCleanupRecord(ctx context.Context, t *testing.T, namespace, kind string, uid types.UID, data map[string]string) { + t.Helper() + data["kind"], data["uid"], data["nonce"] = kind, string(uid), "coder-k8s:"+string(uid) + require.NoError(t, k8sClient.Create(ctx, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: "ck8s-cleanup-" + string(uid), Labels: map[string]string{"coder.com/cleanup-record": kind}}, + Data: data, + })) +} + +// requireRandomNonce checks that a recorded nonce is the temporary display +// name prefix and a value that nobody can derive from the object. +func requireRandomNonce(t *testing.T, nonce string, uid types.UID) { + t.Helper() + require.True(t, strings.HasPrefix(nonce, "coder-k8s:"), nonce) + require.Greater(t, len(nonce), len("coder-k8s:")+20, nonce) + require.NotContains(t, nonce, string(uid), "the nonce is not the object UID") +} + +// #426: anyone who can read the object sees its UID, so a provider that a +// person creates with the object's name, its type and the display name +// coder-k8s: is not this object's. The controller does not adopt it, +// does not change it, and the delete of the object leaves it. +func TestCoderAIProviderUIDDisplayNameNotAdopted(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + const personsKey = "sk-person-0123456789abcdef" // #nosec G101 -- test fixture, not a credential. + _, err := env.fake.client(t, 2*time.Second).CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, Name: provider.Name, DisplayName: "coder-k8s:" + string(provider.UID), + BaseURL: provider.Spec.BaseURL, APIKeys: []string{personsKey}, + }) + require.NoError(t, err) + + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, true, "NameConflict") + require.Empty(t, got.Status.ProviderID, "the person's provider is not adopted") + stored, _ := env.fake.aiProvider(provider.Name) + require.Equal(t, "coder-k8s:"+string(provider.UID), stored.displayName, "the controller did not change it") + require.Equal(t, personsKey, stored.keys[0].plaintext) + + gone, _ := env.deleteAIProvider(ctx, t, got) + require.Nil(t, gone) + require.Zero(t, env.fake.requestCount(routeDeleteAIProvider)) + _, ok := env.fake.aiProvider(provider.Name) + require.True(t, ok, "the person's provider stays") +} + +// #426: the create sends a random nonce from the cleanup record as the +// display name, never the object UID. After a lost create answer, the next +// reconcile adopts the provider that carries that nonce. +func TestCoderAIProviderLostCreateAnswerRecoversByNonce(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeCreateAIProvider, fakeFault{AfterCommit: true, Reset: true}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "ConfirmingCreate") + stored, ok := env.fake.aiProvider(provider.Name) + require.True(t, ok, "the create committed") + nonce := cleanupRecord(ctx, t, env.namespace, got.UID)["nonce"] + require.Equal(t, nonce, stored.displayName, "the create sent the recorded nonce") + requireRandomNonce(t, nonce, got.UID) + + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, stored.id.String(), got.Status.ProviderID, "the provider with the nonce is adopted") + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) +} + +// #426: while a create can still commit, the next create sends the same +// nonce, so a late commit is still recognized. Once the operator knows the +// provider ID, Coder has shown the nonce as the display name, so the next +// create gets a new one. +func TestCoderAIProviderNonceReuse(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeCreateAIProvider, fakeFault{Status: http.StatusTooManyRequests}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CreateRetrying") + first := cleanupRecord(ctx, t, env.namespace, got.UID)["nonce"] + requireRandomNonce(t, first, got.UID) + + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusInternalServerError}) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderStep(t, got, false, "CoderUnavailable") + stored, _ := env.fake.aiProvider(provider.Name) + require.Equal(t, first, stored.displayName, "the second create sent the nonce of the first") + + require.NoError(t, env.fake.client(t, 2*time.Second).DeleteAIProvider(ctx, stored.id.String())) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, 3, env.fake.requestCount(routeCreateAIProvider)) + second := cleanupRecord(ctx, t, env.namespace, got.UID)["nonce"] + requireRandomNonce(t, second, got.UID) + require.NotEqual(t, first, second, "a create after a known provider ID gets a new nonce") +} + +// #426 migration: a record of an operator version before #426 keeps its +// nonce coder-k8s: until the next create, so a create of that version +// whose answer was lost is still adopted. The next create gets a random +// nonce. +func TestCoderAIProviderOldNonceAdoptedUntilNextCreate(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newAIProviderCreateEnv(ctx, t) + _, err := env.fake.client(t, 2*time.Second).CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, Name: provider.Name, DisplayName: "coder-k8s:" + string(provider.UID), + BaseURL: provider.Spec.BaseURL, APIKeys: []string{testAIProviderKey}, + }) + require.NoError(t, err) + attempt := time.Now().Add(-time.Minute).UTC().Truncate(time.Second) + createOldCleanupRecord(ctx, t, env.namespace, "CoderAIProvider", provider.UID, map[string]string{ + "namespace": env.namespace, "name": provider.Name, "deploymentID": env.fake.deploymentID, "coderID": "", "type": "openai", + "createAttemptTime": attempt.Format(time.RFC3339), + }) + provider.Status.CreateAttemptTime = &metav1.Time{Time: attempt} + provider.Status.Credential = &coderv1alpha1.CoderAIProviderCredentialStatus{SecretName: secret.Name, SecretKey: "api-key"} + require.NoError(t, k8sClient.Status().Update(ctx, provider)) + + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusInternalServerError}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + stored, _ := env.fake.aiProvider(provider.Name) + require.Equal(t, stored.id.String(), got.Status.ProviderID, "the create of the older version is adopted") + require.Equal(t, "coder-k8s:"+string(provider.UID), cleanupRecord(ctx, t, env.namespace, got.UID)["nonce"]) + + require.NoError(t, env.fake.client(t, 2*time.Second).DeleteAIProvider(ctx, stored.id.String())) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, 2, env.fake.requestCount(routeCreateAIProvider), "the setup create and one create of the controller") + requireRandomNonce(t, cleanupRecord(ctx, t, env.namespace, got.UID)["nonce"], got.UID) +} diff --git a/internal/controller/codermcpserver_nonce_test.go b/internal/controller/codermcpserver_nonce_test.go new file mode 100644 index 00000000..7da0dad7 --- /dev/null +++ b/internal/controller/codermcpserver_nonce_test.go @@ -0,0 +1,120 @@ +package controller_test + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// #426: anyone who can read the object sees its UID, so a config that a +// person creates with the object's slug and the display name coder-k8s: +// is not this object's. The controller does not adopt it, does not change it, +// and the delete of the object leaves it. +func TestCoderMCPServerUIDDisplayNameNotAdopted(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + req := apiKeyMCPRequest(server.Spec.Slug) + req.DisplayName = "coder-k8s:" + string(server.UID) + persons, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, req) + require.NoError(t, err) + + got, _ := env.reconcile(ctx, t, server) + requireMCPServerStep(t, got, true, "SlugConflict") + require.Empty(t, got.Status.ConfigID, "the person's config is not adopted") + require.Len(t, env.fake.mcpServers(), 1) + require.Equal(t, persons, env.fake.mcpServers()[0].view, "the controller did not change it") + + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone) + require.Zero(t, env.fake.requestCount(routeDeleteMCPServer)) + require.Len(t, env.fake.mcpServers(), 1, "the person's config stays") +} + +// #426: the create sends a random nonce from the cleanup record as the +// display name, never the object UID. After a lost create answer, the next +// reconcile adopts the config that carries that nonce. +func TestCoderMCPServerLostCreateAnswerRecoversByNonce(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(routeCreateMCPServer, fakeFault{AfterCommit: true, Reset: true}) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, false, "ConfirmingCreate") + require.Len(t, env.fake.mcpServers(), 1, "the create committed") + nonce := env.mcpServerRecord(ctx, t, got.UID)["nonce"] + require.Equal(t, nonce, env.fake.mcpServers()[0].view.DisplayName, "the create sent the recorded nonce") + requireRandomNonce(t, nonce, got.UID) + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, env.fake.mcpServers()[0].view.ID.String(), got.Status.ConfigID, "the config with the nonce is adopted") + require.Equal(t, 1, env.posts()) +} + +// #426: once the operator knows the config ID, Coder has shown the nonce as +// the display name, so the next create gets a new one. +func TestCoderMCPServerNewNonceAfterKnownID(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, old := env.applied(ctx, t, nil) + first := env.mcpServerRecord(ctx, t, got.UID)["nonce"] + requireRandomNonce(t, first, got.UID) + + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, old)) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.Equal(t, 2, env.posts()) + second := env.mcpServerRecord(ctx, t, got.UID)["nonce"] + requireRandomNonce(t, second, got.UID) + require.NotEqual(t, first, second) + require.Equal(t, second, env.requireCreated(t, got).view.DisplayName) +} + +// #426 migration: a record of an operator version before #426 keeps its +// nonce coder-k8s: until the next create, so a create of that version +// whose answer was lost is still adopted. The next create gets a random +// nonce. +func TestCoderMCPServerOldNonceAdoptedUntilNextCreate(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + req := apiKeyMCPRequest(server.Spec.Slug) + req.DisplayName = "coder-k8s:" + string(server.UID) + _, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, req) + require.NoError(t, err) + attempt := time.Now().Add(-time.Minute).UTC().Truncate(time.Second) + createOldCleanupRecord(ctx, t, env.namespace, "CoderMCPServer", server.UID, map[string]string{ + "namespace": env.namespace, "name": server.Name, "deploymentID": env.fake.deploymentID, "coderID": "", "type": "", + "organizationID": env.orgID.String(), "slug": server.Spec.Slug, "createAttemptTime": attempt.Format(time.RFC3339), + }) + server.Status.CreateAttemptTime = &metav1.Time{Time: attempt} + require.NoError(t, k8sClient.Status().Update(ctx, server)) + + got, _ := env.reconcile(ctx, t, server) + requireMCPServerApplied(t, got) + require.Equal(t, env.fake.mcpServers()[0].view.ID.String(), got.Status.ConfigID, "the create of the older version is adopted") + require.Equal(t, "coder-k8s:"+string(server.UID), env.mcpServerRecord(ctx, t, got.UID)["nonce"]) + + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, env.fake.mcpServers()[0].view.ID)) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, 2, env.posts(), "the setup create and one create of the controller") + requireRandomNonce(t, env.mcpServerRecord(ctx, t, got.UID)["nonce"], got.UID) +} From e7baaa152a7282f3fb98a47baa9947b4420b940b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:35:39 +0000 Subject: [PATCH 02/14] fix: provider and MCP creates send a secret nonce from the cleanup record (#426) The temporary display name of a create was coder-k8s:, which anyone who can read the object sees. A create now sends coder-k8s: and a random secret that only the cleanup record holds, and the create path adopts only a resource with the recorded nonce. A record of an older version keeps its UID nonce until the next create. Change-Id: I24cef38b9d832dd866e3289057c18ebd53bb629b Signed-off-by: Thomas Kosiewski --- internal/controller/cleanup_record.go | 50 ++++++++++++++++- internal/controller/cleanup_sweep.go | 8 +-- internal/controller/coderaiprovider_create.go | 51 ++++++++++------- .../controller/coderaiprovider_create_test.go | 2 +- internal/controller/coderaiprovider_record.go | 21 ++++--- .../controller/coderaiprovider_record_test.go | 2 +- .../controller/coderaiprovider_sync_test.go | 11 ++-- internal/controller/codermcpserver_apply.go | 2 +- .../controller/codermcpserver_apply_test.go | 2 +- .../controller/codermcpserver_column_test.go | 2 +- internal/controller/codermcpserver_create.go | 55 ++++++++++--------- .../controller/codermcpserver_create_test.go | 3 +- internal/controller/codermcpserver_delete.go | 16 +++--- .../controller/codermcpserver_delete_test.go | 4 +- .../controller/codermcpserver_nonce_test.go | 5 +- internal/controller/codermcpserver_record.go | 27 +++++---- .../controller/codermcpserver_record_test.go | 2 +- 17 files changed, 169 insertions(+), 94 deletions(-) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go index d8508429..5a62ddd5 100644 --- a/internal/controller/cleanup_record.go +++ b/internal/controller/cleanup_record.go @@ -2,8 +2,10 @@ package controller import ( "context" + "crypto/rand" "errors" "fmt" + "strings" "time" "github.com/coder/coder/v2/codersdk" @@ -22,8 +24,9 @@ import ( // 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. +// deployment can delete the resource. A record holds no credentials, but the +// nonce of a provider or MCP server config is a secret until its create +// commits (#426). The ConfigMap creation time is the record's created-at. const ( cleanupRecordPrefix = "ck8s-cleanup-" // cleanupRecordLabel marks a record. Its value is the object kind. @@ -53,7 +56,7 @@ type cleanupRecord struct { // 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 or of an MCP server config. + // name that the last create of a provider or MCP server config sent. Nonce string // CoderID is the Coder ID of the resource once the operator knows it. CoderID string @@ -71,6 +74,47 @@ type cleanupRecord struct { func cleanupRecordObjectName(uid types.UID) string { return cleanupRecordPrefix + string(uid) } +// The display name of a create of a provider or MCP server config is prefix +// and a random secret (#426). Only the record holds it, and namespace users +// cannot read records, so nobody can give another resource that display name +// before the create commits. Operator versions before #426 sent prefix and +// the object UID, which anyone who can read the object sees. A record of such +// a version keeps that nonce until the next create. + +// nextCreateNonce returns the nonce for a create of an object whose record +// is previous. A create of the previous nonce can still commit while the +// record has no Coder ID, and only that nonce then proves it, so it is sent +// again. Once the Coder ID is known, Coder has shown the nonce as a display +// name, so a new create gets a new one. +func nextCreateNonce(prefix string, previous *cleanupRecord) string { + if previous != nil && previous.CoderID == "" && isRandomCreateNonce(prefix, previous.Nonce) { + return previous.Nonce + } + return prefix + rand.Text() +} + +// keptCreateNonce returns the nonce for a record write without a create. An +// object without a record is from before records existed, so a create of it +// sent prefix and the object UID. +func keptCreateNonce(prefix string, uid types.UID, previous *cleanupRecord) string { + if previous != nil { + return previous.Nonce + } + return prefix + string(uid) +} + +// validCreateNonce reports a nonce that the operator can have written in the +// record of uid. +func validCreateNonce(prefix string, uid types.UID, nonce string) bool { + return nonce == prefix+string(uid) || isRandomCreateNonce(prefix, nonce) +} + +func isRandomCreateNonce(prefix, nonce string) bool { + secret, ok := strings.CutPrefix(nonce, prefix) + // rand.Text returns at least 26 characters of the base32 alphabet. + return ok && len(secret) >= 26 && strings.Trim(secret, "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567") == "" +} + // 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") diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go index c06bfec3..df9b57c0 100644 --- a/internal/controller/cleanup_sweep.go +++ b/internal/controller/cleanup_sweep.go @@ -313,8 +313,8 @@ func (r *CleanupRecordReconciler) matchingControlPlanes(ctx context.Context, rec // then returns gone when a read by a site owner finds no provider of that // name and type, and kept otherwise. func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (outcome sweepOutcome, why string, err error) { - if record.Nonce != aiProviderNoncePrefix+string(record.UID) { - return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) + if !validCreateNonce(aiProviderNoncePrefix, record.UID, record.Nonce) { + return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has a nonce that the operator does not write", record.UID) } if step := requireSiteOwner(ctx, sdk); step != nil { // Only a site owner can trust Coder's 404 for a provider. @@ -397,8 +397,8 @@ func sweepAIGatewayKeys(ctx context.Context, sdk *codersdk.Client, record *clean // it. It then returns gone when a list by a site owner shows no config with // the record's slug and none with its nonce, and kept otherwise. func sweepMCPServer(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (outcome sweepOutcome, why string, err error) { - if record.Nonce != mcpServerNoncePrefix+string(record.UID) { - return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) + if !validCreateNonce(mcpServerNoncePrefix, record.UID, record.Nonce) { + return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has a nonce that the operator does not write", record.UID) } orgID, err := uuid.Parse(record.Organization) if err != nil || orgID == uuid.Nil || orgID.String() != record.Organization || record.Slug == "" { diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index ba3c998f..be46e1a9 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -19,17 +19,13 @@ import ( const ( // aiProviderNoncePrefix starts the display name of a provider that the - // controller creates. With the object UID it proves that this object - // created the provider, because Coder stores no owner. + // controller creates. With the secret of the cleanup record it proves + // that this object created the provider, because Coder stores no owner. aiProviderNoncePrefix = "coder-k8s:" // aiProviderConfirmPoll is the poll while a create result is uncertain. aiProviderConfirmPoll = 5 * time.Second ) -func aiProviderNonce(provider *coderv1alpha1.CoderAIProvider) string { - return aiProviderNoncePrefix + string(provider.UID) -} - // createOrConfirm handles a provider without a Coder ID. Without a create // marker it looks up the name and creates the provider when the name is free. // With a marker, a create can be in flight or committed, so it reads Coder @@ -73,7 +69,15 @@ func (r *CoderAIProviderReconciler) createOrConfirm( case err != nil: return keepRejectedCreate(aiProviderWait(aiProviderRetry, "CoderUnavailable", "get provider %s: %s", provider.Name, coderErrorSummary(err)), rejected), nil } - return r.recordAIProvider(ctx, provider, found) + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) + if err != nil { + return nil, err + } + nonce := "" + if record != nil { + nonce = record.Nonce + } + return r.recordAIProvider(ctx, provider, found, nonce) } // keepRejectedCreate keeps the memory of a rejected create in the reason of a @@ -101,14 +105,18 @@ func requireSiteOwner(ctx context.Context, sdk *codersdk.Client) *aiProviderStep } // recordAIProvider decides whether found is the provider that this object -// created. Only the nonce and the type prove it. Anything else is a provider -// that the controller never touches (no adoption). -func (r *CoderAIProviderReconciler) recordAIProvider(ctx context.Context, provider *coderv1alpha1.CoderAIProvider, found codersdk.AIProvider) (*aiProviderStep, error) { +// created. Only nonce, the display name of the last create, and the type +// prove it. Without a nonce, no create of this object can be pending. +// Anything else is a provider that the controller never touches (no +// adoption). +func (r *CoderAIProviderReconciler) recordAIProvider( + ctx context.Context, provider *coderv1alpha1.CoderAIProvider, found codersdk.AIProvider, nonce string, +) (*aiProviderStep, error) { if err := coderAnswerFor("AI provider", provider.Name, found.Name); err != nil { log.FromContext(ctx).Error(err, "Coder answered something other than the controller asked for") return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", "%s", err.Error()), nil } - if found.DisplayName != aiProviderNonce(provider) || string(found.Type) != provider.Spec.Type { + if nonce == "" || found.DisplayName != nonce || string(found.Type) != provider.Spec.Type { // No create of this object can commit while another provider holds // the name, so nothing of this object can exist in Coder. provider.Status.CreateAttemptTime, provider.Status.Credential = nil, nil @@ -139,8 +147,9 @@ func (r *CoderAIProviderReconciler) recordAIProvider(ctx context.Context, provid // 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. +// reconcile before any request. The provider starts disabled with the +// recorded 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) { @@ -148,7 +157,8 @@ func (r *CoderAIProviderReconciler) createAIProvider( 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 { + nonce, step, err := r.writeAIProviderRecord(ctx, sdk, provider, &now) + if step != nil || err != nil { return step, err } provider.Status.CreateAttemptTime = &metav1.Time{Time: now} @@ -162,13 +172,13 @@ func (r *CoderAIProviderReconciler) createAIProvider( *before = *provider.Status.DeepCopy() created, err := sdk.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ - Type: codersdk.AIProviderType(provider.Spec.Type), Name: provider.Name, DisplayName: aiProviderNonce(provider), + Type: codersdk.AIProviderType(provider.Spec.Type), Name: provider.Name, DisplayName: nonce, Enabled: false, BaseURL: provider.Spec.BaseURL, APIKeys: []string{key.Reveal()}, }) status := coderStatus(err) switch { case err == nil: - if step, err := r.recordAIProvider(ctx, provider, created); step != nil || err != nil { + if step, err := r.recordAIProvider(ctx, provider, created, nonce); step != nil || err != nil { // A wrong answer leaves the marker, so a read decides. if step != nil { provider.Status.CreateAttemptTime, provider.Status.Credential = before.CreateAttemptTime, before.Credential @@ -184,7 +194,7 @@ func (r *CoderAIProviderReconciler) createAIProvider( provider.Status.CreateAttemptTime, provider.Status.Credential = nil, nil return aiProviderWait(aiProviderRetry, "CreateRetrying", "The create request had no effect: %s", coderErrorSummary(err)), nil case status >= 400 && status < 500: - return r.confirmRejectedCreate(ctx, sdk, provider, status, err) + return r.confirmRejectedCreate(ctx, sdk, provider, nonce, status, err) default: return aiProviderWait(aiProviderConfirmPoll, "ConfirmingCreate", "The create result is uncertain (%s). The controller reads Coder instead of sending it again.", coderErrorSummary(err)), nil @@ -194,15 +204,16 @@ func (r *CoderAIProviderReconciler) createAIProvider( // confirmRejectedCreate reads Coder after a 4xx answer to a create. Coder // sends its 4xx answers before the insert commits, but a proxy can put a 4xx // in place of a committed answer, so only a read proves that nothing of this -// object exists. Until then the marker stays (#253 review round 3). +// object exists. Until then the marker stays (#253 review round 3). nonce is +// the display name that the create sent. func (r *CoderAIProviderReconciler) confirmRejectedCreate( - ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, status int, createErr error, + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, nonce string, status int, createErr error, ) (*aiProviderStep, error) { found, err := sdk.AIProvider(ctx, provider.Name) switch { case err == nil: // Ours (the create committed) or someone else's (NameConflict). - return r.recordAIProvider(ctx, provider, found) + return r.recordAIProvider(ctx, provider, found, nonce) case !isCoderResourceNotFound(err): reason := "ConfirmingCreate" if !aiProviderCreateRetryable(status) { diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index 88654447..6bcee3cb 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -58,7 +58,7 @@ func TestCoderAIProviderCreate(t *testing.T) { stored, ok := env.fake.aiProvider("openai") require.True(t, ok) require.False(t, stored.enabled, "the provider starts disabled") - require.Equal(t, "coder-k8s:"+string(got.UID), stored.displayName, "the nonce proves who created it") + require.Equal(t, cleanupRecord(ctx, t, env.namespace, got.UID)["nonce"], stored.displayName, "the nonce proves who created it") require.Equal(t, codersdk.AIProviderTypeOpenAI, stored.providerType) require.Equal(t, "https://api.openai.com/v1", stored.baseURL) require.Len(t, stored.keys, 1) diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go index fbccf38a..9bc4787d 100644 --- a/internal/controller/coderaiprovider_record.go +++ b/internal/controller/coderaiprovider_record.go @@ -16,29 +16,33 @@ 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 +// time of a create that the caller sends next, or nil. It returns the +// recorded nonce, the display name of that create. 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) { +) (string, *aiProviderStep, error) { previous, existing, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) if err != nil { - return nil, err + 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 + 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 r.aiProviderDeploymentChanged(provider, previous, deploymentID), nil + return "", r.aiProviderDeploymentChanged(provider, previous, deploymentID), 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, + DeploymentID: deploymentID, Nonce: keptCreateNonce(aiProviderNoncePrefix, provider.UID, previous), CoderID: provider.Status.ProviderID, Type: provider.Spec.Type, + } + if createAttempt != nil { + record.Nonce = nextCreateNonce(aiProviderNoncePrefix, previous) } if record.CoderID == "" && createAttempt == nil && previous != nil { // A status without the ID does not prove that the provider is gone, @@ -56,7 +60,7 @@ func (r *CoderAIProviderReconciler) writeAIProviderRecord( // for the settle window after that create. record.CreateAttemptTime = previous.CreateAttemptTime } - return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) + return record.Nonce, nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) } // aiProviderDeploymentChanged is the stall of an object whose record names @@ -132,5 +136,6 @@ func (r *CoderAIProviderReconciler) ensureAIProviderRecord( if record != nil && record.CoderID != "" && record.CoderID == provider.Status.ProviderID { return nil, nil } - return r.writeAIProviderRecord(ctx, sdk, provider, nil) + _, step, err := r.writeAIProviderRecord(ctx, sdk, provider, nil) + return step, err } diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index efa530a0..3f123a6e 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -56,7 +56,7 @@ func TestCoderAIProviderCleanupRecord(t *testing.T) { delete(record, "createAttemptTime") require.Equal(t, map[string]string{ "kind": "CoderAIProvider", "uid": string(got.UID), "namespace": env.namespace, "name": "openai", - "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": stored.id.String(), "type": "openai", + "deploymentID": env.fake.deploymentID, "nonce": stored.displayName, "coderID": stored.id.String(), "type": "openai", }, record, "the provider ID is recorded before the sync replaces the nonce") got, _ = env.reconcile(ctx, t, got) diff --git a/internal/controller/coderaiprovider_sync_test.go b/internal/controller/coderaiprovider_sync_test.go index e02203dc..995803ce 100644 --- a/internal/controller/coderaiprovider_sync_test.go +++ b/internal/controller/coderaiprovider_sync_test.go @@ -493,10 +493,13 @@ func TestCoderAIProviderNonceWithoutCredential(t *testing.T) { t.Parallel() ctx := context.Background() env, _, provider := newAIProviderCreateEnv(ctx, t) - got := &coderv1alpha1.CoderAIProvider{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got)) + // A 429 that a read confirms clears the marker and the credential. + env.fake.failNext(routeCreateAIProvider, fakeFault{Status: http.StatusTooManyRequests}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CreateRetrying") + require.Nil(t, got.Status.Credential) _, err := env.fake.client(t, 2*time.Second).CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ - Type: codersdk.AIProviderTypeOpenAI, Name: "openai", DisplayName: "coder-k8s:" + string(got.UID), + Type: codersdk.AIProviderTypeOpenAI, Name: "openai", DisplayName: cleanupRecord(ctx, t, env.namespace, got.UID)["nonce"], BaseURL: "https://api.openai.com/v1", APIKeys: []string{"sk-late-commit-0123456789ab"}, }) require.NoError(t, err) @@ -505,7 +508,7 @@ func TestCoderAIProviderNonceWithoutCredential(t *testing.T) { stored, _ := env.fake.aiProvider("openai") require.Equal(t, stored.id.String(), got.Status.ProviderID) require.Equal(t, testAIProviderKey, stored.keys[0].plaintext, "the current key replaced the unknown one") - require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "only the late commit") + require.Equal(t, 2, env.fake.requestCount(routeCreateAIProvider), "the rejected create and the late commit") } // Plan 5.3 rule 5: a key PATCH that failed before it committed leaves the diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index d443b7a1..521eec39 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -448,7 +448,7 @@ func mcpServerReady(server *coderv1alpha1.CoderMCPServer) *mcpServerStep { // makes Coder clear them (plan F9). The tool lists stay unset until the spec // has them. Credentials are revealed only here, into the request body. func mcpServerUpdateRequest(server *coderv1alpha1.CoderMCPServer, values map[string]credentialsecret.Value) (codersdk.UpdateMCPServerConfigRequest, error) { - create, err := mcpServerCreateRequest(server, values) + create, err := mcpServerCreateRequest(server, values, server.Spec.DisplayName) if err != nil { return codersdk.UpdateMCPServerConfigRequest{}, err } diff --git a/internal/controller/codermcpserver_apply_test.go b/internal/controller/codermcpserver_apply_test.go index a97c8068..9e028fce 100644 --- a/internal/controller/codermcpserver_apply_test.go +++ b/internal/controller/codermcpserver_apply_test.go @@ -180,7 +180,7 @@ func TestCoderMCPServerApplyGroupNotFound(t *testing.T) { requireMCPServerStep(t, got, true, "GroupNotFound") stored := env.requireCreated(t, got) require.False(t, stored.view.Enabled) - require.Equal(t, "coder-k8s:"+string(got.UID), stored.view.DisplayName) + require.Equal(t, env.mcpServerRecord(ctx, t, got.UID)["nonce"], stored.view.DisplayName) require.Zero(t, env.fake.requestCount(routeUpdateMCPServer)) groups, _ := env.fake.mcpACL(stored.view.ID) require.Empty(t, groups, "only the remove-only ACL write: Everyone goes") diff --git a/internal/controller/codermcpserver_column_test.go b/internal/controller/codermcpserver_column_test.go index 5917f2d3..2ec00d59 100644 --- a/internal/controller/codermcpserver_column_test.go +++ b/internal/controller/codermcpserver_column_test.go @@ -81,7 +81,7 @@ func TestCoderMCPServerInterruptedUpdateKeepsObservedGeneration(t *testing.T) { require.NotEmpty(t, stored.Status.ConfigID, "the config ID was written before the PATCH") require.Equal(t, confirmedGeneration, stored.Status.ObservedGeneration, "the config ID write does not observe the edit") require.Equal(t, confirmedGeneration, meta.FindStatusCondition(stored.Status.Conditions, "Ready").ObservedGeneration) - require.Equal(t, "coder-k8s:"+string(stored.UID), env.stored(t, stored).view.DisplayName) + require.Equal(t, env.mcpServerRecord(ctx, t, stored.UID)["nonce"], env.stored(t, stored).view.DisplayName) got, _ = env.reconcile(ctx, t, stored) requireMCPServerApplied(t, got) diff --git a/internal/controller/codermcpserver_create.go b/internal/controller/codermcpserver_create.go index ac9a7e49..586e4bd6 100644 --- a/internal/controller/codermcpserver_create.go +++ b/internal/controller/codermcpserver_create.go @@ -25,13 +25,6 @@ const ( mcpServerConfirmingRejectedCreate = "ConfirmingRejectedCreate" ) -// mcpServerNonce is the display name of a config that this object created, -// until the spec's display name replaces it. Coder stores no owner, so the -// nonce is the proof (plan 5.3). -func mcpServerNonce(server *coderv1alpha1.CoderMCPServer) string { - return mcpServerNoncePrefix + string(server.UID) -} - // createOrRecord handles an object without status.configID, following the // CoderAIProvider create (#253). It reads Coder first, so a create that // committed earlier is recorded instead of sent again. Coder's unique slug @@ -61,14 +54,17 @@ func (r *CoderMCPServerReconciler) createOrRecord( if err != nil { return nil, err } - recordedID := "" + nonce, recordedID := "", "" + if record != nil { + nonce = record.Nonce + } if record != nil && record.CoderID == found.ID.String() { if step, err := r.recordedDeploymentChanged(ctx, sdk, server); step != nil || err != nil { return step, err } recordedID = record.CoderID } - if step := recordMCPServer(server, *found, recordedID); step != nil { + if step := recordMCPServer(server, *found, nonce, recordedID); step != nil { return step, nil } // Store the ID before the sync replaces the nonce (#353). @@ -114,12 +110,13 @@ func mcpServerRequireSiteOwner(ctx context.Context, sdk *codersdk.Client) *mcpSe } // recordMCPServer decides whether found, the config with the spec's slug, is -// the config that this object created. Only the nonce or recordedID, the -// config ID of the cleanup record, proves it. Anything else is a config that -// the controller never touches (no adoption). It returns nil when it recorded -// the ID. -func recordMCPServer(server *coderv1alpha1.CoderMCPServer, found codersdk.MCPServerConfig, recordedID string) *mcpServerStep { - if found.DisplayName != mcpServerNonce(server) && found.ID.String() != recordedID { +// the config that this object created. Only nonce, the display name of the +// last create, or recordedID, the config ID of the cleanup record, proves it. +// Without a nonce, no create of this object can be pending. Anything else is +// a config that the controller never touches (no adoption). It returns nil +// when it recorded the ID. +func recordMCPServer(server *coderv1alpha1.CoderMCPServer, found codersdk.MCPServerConfig, nonce, recordedID string) *mcpServerStep { + if (nonce == "" || found.DisplayName != nonce) && found.ID.String() != recordedID { // No create of this object can commit while another config holds the // slug, so nothing of this object can exist in Coder. server.Status.CreateAttemptTime, server.Status.Credentials = nil, nil @@ -134,9 +131,9 @@ func recordMCPServer(server *coderv1alpha1.CoderMCPServer, found codersdk.MCPSer // createMCPServer writes the cleanup record, then the create marker and the // change tokens, 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 config starts disabled with the nonce as its display name, -// so members never see it and chats never use it (plan F6) before the apply -// step. +// any request. The config starts disabled with the recorded nonce as its +// display name, so members never see it and chats never use it (plan F6) +// before the apply step. func (r *CoderMCPServerReconciler) createMCPServer( ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, values map[string]credentialsecret.Value, before *coderv1alpha1.CoderMCPServerStatus, @@ -144,7 +141,8 @@ func (r *CoderMCPServerReconciler) createMCPServer( now := time.Now() // Without the record, a config that a namespace deletion leaves behind // cannot be found again (#353 invariant 1). - if step, err := r.writeMCPServerRecord(ctx, sdk, server, &now); step != nil || err != nil { + nonce, step, err := r.writeMCPServerRecord(ctx, sdk, server, &now) + if step != nil || err != nil { return step, err } server.Status.CreateAttemptTime = &metav1.Time{Time: now} @@ -155,7 +153,7 @@ func (r *CoderMCPServerReconciler) createMCPServer( } *before = *server.Status.DeepCopy() - req, err := mcpServerCreateRequest(server, values) + req, err := mcpServerCreateRequest(server, values, nonce) if err != nil { return nil, err } @@ -180,7 +178,7 @@ func (r *CoderMCPServerReconciler) createMCPServer( server.Status.CreateAttemptTime, server.Status.Credentials = nil, nil return mcpServerWait(mcpServerRetry, "CreateRetrying", "The create request had no effect: %s", coderErrorSummary(err)), nil case status >= 400 && status < 500: - return confirmRejectedMCPServerCreate(ctx, sdk, server, orgID, status, err), nil + return confirmRejectedMCPServerCreate(ctx, sdk, server, orgID, nonce, status, err), nil default: return mcpServerWait(mcpServerConfirmPoll, "ConfirmingCreate", "The create result is uncertain (%s). The controller reads Coder instead of sending it again.", coderErrorSummary(err)), nil @@ -191,8 +189,9 @@ func (r *CoderMCPServerReconciler) createMCPServer( // Coder sends its 4xx answers before the insert commits, but a proxy can put a // 4xx in place of a committed answer, so only a read proves that nothing of // this object exists. Until then the marker stays (#253 review round 3). +// nonce is the display name that the create sent. func confirmRejectedMCPServerCreate( - ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, status int, createErr error, + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, nonce string, status int, createErr error, ) *mcpServerStep { retryable := mcpServerCreateRetryable(status) found, step := findMCPServerSlug(ctx, sdk, server, orgID) @@ -202,7 +201,7 @@ func confirmRejectedMCPServerCreate( case found != nil: // Ours (the create committed) or someone else's (SlugConflict). The // create dropped the recorded ID, so only the nonce can prove it. - return recordMCPServer(server, *found, "") + return recordMCPServer(server, *found, nonce, "") } if step := mcpServerRequireSiteOwner(ctx, sdk); step != nil { return keepRejectedMCPServerCreate(step, !retryable) @@ -239,15 +238,17 @@ func mcpServerCredentialStatus(auth coderv1alpha1.CoderMCPServerAuth, values map } // mcpServerCreateRequest builds the create request: every spec field, the -// credentials of the auth type, enabled false, and the nonce as the display -// name. Credentials are revealed only here, into the request body. -func mcpServerCreateRequest(server *coderv1alpha1.CoderMCPServer, values map[string]credentialsecret.Value) (codersdk.CreateMCPServerConfigRequest, error) { +// credentials of the auth type, enabled false, and displayName, the nonce of +// a create. Credentials are revealed only here, into the request body. +func mcpServerCreateRequest( + server *coderv1alpha1.CoderMCPServer, values map[string]credentialsecret.Value, displayName string, +) (codersdk.CreateMCPServerConfigRequest, error) { spec := server.Spec if spec.Transport == "" || spec.Availability == "" { return codersdk.CreateMCPServerConfigRequest{}, fmt.Errorf("assertion failed: MCP server %s/%s has no transport or availability after admission defaults", server.Namespace, server.Name) } req := codersdk.CreateMCPServerConfigRequest{ - DisplayName: mcpServerNonce(server), Slug: spec.Slug, Description: spec.Description, IconURL: spec.IconURL, + DisplayName: displayName, Slug: spec.Slug, Description: spec.Description, IconURL: spec.IconURL, Transport: spec.Transport, URL: spec.URL, AuthType: spec.Auth.Type, Availability: spec.Availability, Enabled: false, ModelIntent: spec.ModelIntent, AllowInPlanMode: spec.AllowInPlanMode, ForwardCoderHeaders: spec.ForwardCoderHeaders, } diff --git a/internal/controller/codermcpserver_create_test.go b/internal/controller/codermcpserver_create_test.go index f0b2d9db..0c95e523 100644 --- a/internal/controller/codermcpserver_create_test.go +++ b/internal/controller/codermcpserver_create_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "net/http" + "strings" "testing" "time" @@ -42,7 +43,7 @@ func (env mcpServerEnv) requireCreated(t *testing.T, server *coderv1alpha1.Coder require.NoError(t, json.Unmarshal(body, &req)) if req.Slug == server.Spec.Slug { require.False(t, req.Enabled, "created disabled") - require.Equal(t, "coder-k8s:"+string(server.UID), req.DisplayName) + require.True(t, strings.HasPrefix(req.DisplayName, "coder-k8s:"), "created with a nonce, not %q", req.DisplayName) } } return own[0] diff --git a/internal/controller/codermcpserver_delete.go b/internal/controller/codermcpserver_delete.go index b6b9db32..09600258 100644 --- a/internal/controller/codermcpserver_delete.go +++ b/internal/controller/codermcpserver_delete.go @@ -121,7 +121,7 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder return false, nil, err } } - recordedID := "" + recordedID, recordedNonce := "", "" if record != nil { // The record names the deployment that holds the config. When the // control plane now reaches another deployment, a delete there @@ -135,9 +135,9 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder case id != record.DeploymentID: return r.releaseWithoutDelete(ctx, server, fmt.Sprintf("CoderControlPlane now reaches Coder deployment %s, but the config is in deployment %s", id, record.DeploymentID)), nil, nil } - recordedID = record.CoderID + recordedID, recordedNonce = record.CoderID, record.Nonce } - unproven, step, err := deleteMCPServerInCoder(ctx, sdk, server, orgID, recordedID) + unproven, step, err := deleteMCPServerInCoder(ctx, sdk, server, orgID, recordedID, recordedNonce) if err == nil && step == nil && unproven { // A config that can be this object's stays in Coder, so the record // stays for a sweep, which reports it for manual cleanup (#353). @@ -169,9 +169,11 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder // nothing: a person can edit the slug and the display name of a config, so // the nonce does not prove that this object created it (#422). Only Coder's // own 404 proves absence (#337). unproven reports a config that it leaves -// although it can be this object's: a config with the slug, or one with the -// nonce under another slug. -func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, recordedID string) (unproven bool, step *mcpServerStep, err error) { +// although it can be this object's: a config with the slug, or one with +// recordedNonce, the nonce of the cleanup record, under another slug. +func deleteMCPServerInCoder( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, recordedID, recordedNonce string, +) (unproven bool, step *mcpServerStep, err error) { var id uuid.UUID if configID := cmp.Or(server.Status.ConfigID, recordedID); configID != "" { parsed, err := uuid.Parse(configID) @@ -206,7 +208,7 @@ func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *c "MCP server config %s is not in Coder, but its create request was sent at %s and can still commit. The controller reads again before it releases the finalizer.", server.Spec.Slug, server.Status.CreateAttemptTime.UTC().Format(time.RFC3339)), nil } - return mcpServerWithDisplayName(configs, mcpServerNonce(server)) != nil, nil, nil + return recordedNonce != "" && mcpServerWithDisplayName(configs, recordedNonce) != nil, nil, nil default: return true, nil, nil } diff --git a/internal/controller/codermcpserver_delete_test.go b/internal/controller/codermcpserver_delete_test.go index 8ffe8482..a743163e 100644 --- a/internal/controller/codermcpserver_delete_test.go +++ b/internal/controller/codermcpserver_delete_test.go @@ -297,7 +297,7 @@ func TestCoderMCPServerDeletePendingCreateRead(t *testing.T) { func createWithNonce(ctx context.Context, t *testing.T, env mcpServerEnv, server *coderv1alpha1.CoderMCPServer) { t.Helper() req := apiKeyMCPRequest(server.Spec.Slug) - req.DisplayName = "coder-k8s:" + string(server.UID) + req.DisplayName = env.mcpServerRecord(ctx, t, server.UID)["nonce"] _, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, req) require.NoError(t, err) } @@ -307,7 +307,7 @@ func createWithNonce(ctx context.Context, t *testing.T, env mcpServerEnv, server func createWithNonceUnderOtherSlug(ctx context.Context, t *testing.T, env mcpServerEnv, server *coderv1alpha1.CoderMCPServer) { t.Helper() req := apiKeyMCPRequest("renamed") - req.DisplayName = "coder-k8s:" + string(server.UID) + req.DisplayName = env.mcpServerRecord(ctx, t, server.UID)["nonce"] _, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, req) require.NoError(t, err) } diff --git a/internal/controller/codermcpserver_nonce_test.go b/internal/controller/codermcpserver_nonce_test.go index 7da0dad7..d9d6ac98 100644 --- a/internal/controller/codermcpserver_nonce_test.go +++ b/internal/controller/codermcpserver_nonce_test.go @@ -29,7 +29,10 @@ func TestCoderMCPServerUIDDisplayNameNotAdopted(t *testing.T) { requireMCPServerStep(t, got, true, "SlugConflict") require.Empty(t, got.Status.ConfigID, "the person's config is not adopted") require.Len(t, env.fake.mcpServers(), 1) - require.Equal(t, persons, env.fake.mcpServers()[0].view, "the controller did not change it") + stored := env.fake.mcpServers()[0].view + require.Equal(t, req.DisplayName, stored.DisplayName) + require.Equal(t, persons.UpdatedAt, stored.UpdatedAt, "the controller did not change it") + require.Zero(t, env.fake.requestCount(routeUpdateMCPServer)) gone, _ := env.deleteMCPServer(ctx, t, got) require.Nil(t, gone) diff --git a/internal/controller/codermcpserver_record.go b/internal/controller/codermcpserver_record.go index eb9ae177..f3ea202d 100644 --- a/internal/controller/codermcpserver_record.go +++ b/internal/controller/codermcpserver_record.go @@ -19,39 +19,43 @@ const cleanupRecordKindMCPServer = "CoderMCPServer" // writeMCPServerRecord writes the cleanup record of server with the // deployment ID that Coder answers now. 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). +// that the caller sends next, or nil. It returns the recorded nonce, the +// display name of that create. A failed read or write returns a wait or an +// error, and the caller then sends no create (#353 invariant 1). func (r *CoderMCPServerReconciler) writeMCPServerRecord( ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, createAttempt *time.Time, -) (*mcpServerStep, error) { +) (string, *mcpServerStep, error) { if orgID, err := uuid.Parse(server.Status.OrganizationID); err != nil || orgID == uuid.Nil || orgID.String() != server.Status.OrganizationID { - return nil, fmt.Errorf("assertion failed: MCP server %s/%s needs a pinned organization ID for its cleanup record, but has %q", server.Namespace, server.Name, server.Status.OrganizationID) + return "", nil, fmt.Errorf("assertion failed: MCP server %s/%s needs a pinned organization ID for its cleanup record, but has %q", server.Namespace, server.Name, server.Status.OrganizationID) } if id := server.Status.ConfigID; id != "" { // A sweep deletes the recorded ID, so only a canonical UUID goes in. if parsed, err := uuid.Parse(id); err != nil || parsed == uuid.Nil || parsed.String() != id { - return nil, fmt.Errorf("assertion failed: MCP server %s/%s has status.configID %q, which is not a canonical UUID", server.Namespace, server.Name, id) + return "", nil, fmt.Errorf("assertion failed: MCP server %s/%s has status.configID %q, which is not a canonical UUID", server.Namespace, server.Name, id) } } previous, existing, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, server.UID) if err != nil { - return nil, err + return "", nil, err } deploymentID, err := coderDeploymentID(ctx, sdk) if err != nil { - return mcpServerWait(mcpServerRetry, "CoderUnavailable", "read the Coder deployment ID for the cleanup record: %s", coderErrorSummary(err)), nil + return "", mcpServerWait(mcpServerRetry, "CoderUnavailable", "read the Coder deployment ID for the cleanup record: %s", coderErrorSummary(err)), nil } if previous != nil && previous.DeploymentID != deploymentID { // The record is the only locator of what this object made in the old // Coder database, so the controller neither overwrites it nor creates // in the new one. - return r.mcpServerDeploymentChanged(server, previous, deploymentID), nil + return "", r.mcpServerDeploymentChanged(server, previous, deploymentID), nil } record := cleanupRecord{ Kind: cleanupRecordKindMCPServer, UID: server.UID, Namespace: server.Namespace, Name: server.Name, - DeploymentID: deploymentID, Nonce: mcpServerNonce(server), CoderID: server.Status.ConfigID, + DeploymentID: deploymentID, Nonce: keptCreateNonce(mcpServerNoncePrefix, server.UID, previous), CoderID: server.Status.ConfigID, Organization: server.Status.OrganizationID, Slug: server.Spec.Slug, } + if createAttempt != nil { + record.Nonce = nextCreateNonce(mcpServerNoncePrefix, previous) + } if record.CoderID == "" && createAttempt == nil && previous != nil { // A status without the ID does not prove that the config is gone, and // after the sync only the recorded ID finds it. A new create drops the @@ -68,7 +72,7 @@ func (r *CoderMCPServerReconciler) writeMCPServerRecord( // for the settle window after that create. record.CreateAttemptTime = previous.CreateAttemptTime } - return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) + return record.Nonce, nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) } // mcpServerDeploymentChanged is the stall of an object whose record names @@ -128,5 +132,6 @@ func (r *CoderMCPServerReconciler) ensureMCPServerRecord(ctx context.Context, sd if record != nil && record.CoderID != "" && record.CoderID == server.Status.ConfigID { return nil, nil } - return r.writeMCPServerRecord(ctx, sdk, server, nil) + _, step, err := r.writeMCPServerRecord(ctx, sdk, server, nil) + return step, err } diff --git a/internal/controller/codermcpserver_record_test.go b/internal/controller/codermcpserver_record_test.go index 173e993f..2d6f0aab 100644 --- a/internal/controller/codermcpserver_record_test.go +++ b/internal/controller/codermcpserver_record_test.go @@ -55,7 +55,7 @@ func TestCoderMCPServerCleanupRecord(t *testing.T) { delete(record, "createAttemptTime") require.Equal(t, map[string]string{ "kind": "CoderMCPServer", "uid": string(got.UID), "namespace": env.namespace, "name": got.Name, - "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": env.requireCreated(t, got).view.ID.String(), + "deploymentID": env.fake.deploymentID, "nonce": env.requireCreated(t, got).view.DisplayName, "coderID": env.requireCreated(t, got).view.ID.String(), "type": "", "organizationID": env.orgID.String(), "slug": got.Spec.Slug, }, record, "the config ID is recorded before the sync replaces the nonce") From eb3d0bed91a9df948822a059cab479733eefaf1c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:36:21 +0000 Subject: [PATCH 03/14] test: a provider from before records gets one before its bounded delete (#422) Change-Id: I4deab98eed66e407e8d36895642cbdfa5b38c2eb Signed-off-by: Thomas Kosiewski --- .../controller/coderaiprovider_record_test.go | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index 3f123a6e..81444be7 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -171,6 +171,26 @@ func TestCoderAIProviderRecordBackfillOnDisable(t *testing.T) { require.Equal(t, got.Status.ProviderID, cleanupRecord(ctx, t, env.namespace, provider.UID)["coderID"]) } +// #422 item 3: a provider from before records gets one also when its object +// is already being deleted, so a release without a confirmed delete leaves a +// record for a sweep. +func TestCoderAIProviderRecordBackfillOnDelete(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)}})) + holdControlPlane(ctx, t, env.namespace) + env.fake.failNext(routeDeleteAIProvider, fakeFault{Status: http.StatusBadGateway}) + + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone, "released after one attempt") + _, ok := env.fake.aiProvider("openai") + require.True(t, ok) + record := cleanupRecord(ctx, t, env.namespace, provider.UID) + require.Equal(t, provider.Status.ProviderID, record["coderID"], "the record stays for a sweep") + require.Equal(t, env.fake.deploymentID, record["deploymentID"]) +} + // #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) { From 88603a679fd18f8dbdf50f8170f230a46b804cd7 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:37:05 +0000 Subject: [PATCH 04/14] fix: a provider from before records gets one before its bounded delete (#422) Change-Id: I17a9040ca9554da0c0d17151eb083563f5d2df58 Signed-off-by: Thomas Kosiewski --- internal/controller/coderaiprovider_delete.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 50faa503..285980bc 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -126,6 +126,13 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co ctx, cancel = context.WithTimeout(ctx, aiProviderBoundedDeleteTimeout) defer cancel() } + if provider.Status.ProviderID != "" && checkAIProviderID(provider) == nil { + // A provider from before records existed gets its record before a + // delete that can release without a confirmation (#422 item 3). + 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) + } + } // 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). From d1ff4d7e6f1e63f791aa267271f36a7f0ba7c4df Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:38:54 +0000 Subject: [PATCH 05/14] docs: the secret create nonce, record read access, and the legacy record limits (#426, #422) Change-Id: If36e326cd9f555815f7b8a5742617426c06d37fb Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-providers.md | 8 ++++++-- docs/how-to/manage-mcp-servers.md | 10 +++++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 88b10fe9..2f385052 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -134,7 +134,11 @@ Delete `CoderAIProvider` objects before their namespace: a namespace deletion re ### 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. +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 nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. + +The nonce is the display name that the create sends: `coder-k8s:` and a random secret. Only the record holds the secret. When a create answer is lost, the operator takes a provider of the object's name and type as its own only when its display name is the recorded nonce. A person cannot give a provider that display name before the create, because only the operator and cluster admins can read the record. The operator sends the same nonce again while its last create is unconfirmed, so a create that commits late is still recognized. After it knows the provider ID, it makes a new nonce for the next create: Coder showed the old one as the display name until the operator applied the spec. Records of older operator versions hold the nonce `coder-k8s:`, which anyone who can read the object sees. The operator keeps that nonce until it next creates the provider, so a lost create answer of an older version is still recognized. + +A provider that an older operator version created gets its record at the next reconcile, also while its object is being deleted. The record needs the deployment ID that Coder answers. When that read or the record write fails, and the delete fails too, in the one bounded attempt while the control plane is being deleted (for example in a namespace deletion), the finalizer goes without a record. The operator does not hold the namespace deletion longer. It logs the provider ID with `ControlPlaneGone`: delete the provider in the dashboard. 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. @@ -160,7 +164,7 @@ A record without a provider ID is left by a create that the operator could not c 3. When that provider is a leftover of the deleted object, delete it. The next sweep, within 10 minutes, then removes the record. 4. When a person owns that provider, keep it and delete the ConfigMap. -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. +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. Do not give them read access either: a record holds the secret nonce of a create. The built-in `view`, `edit` and `admin` roles include ConfigMaps, so bind them in that namespace only to cluster admins. If you install the operator in another namespace, change the namespace of both. ### Stuck finalizer diff --git a/docs/how-to/manage-mcp-servers.md b/docs/how-to/manage-mcp-servers.md index 247e92f1..bffbcb9b 100644 --- a/docs/how-to/manage-mcp-servers.md +++ b/docs/how-to/manage-mcp-servers.md @@ -95,7 +95,7 @@ The sample, with its Secret, is in `config/samples/coder_v1alpha1_codermcpserver - `controlPlaneRef`, `organization` and `slug` cannot change. The model sees the tools as `__`. To rename a slug, delete the object and create a new one. Chats then lose the old server. - The operator never takes over a config that it did not create. When the organization already has a config with the slug, the object stalls with `SlugConflict`, and the operator sends no write. -- The operator creates the config disabled, with a temporary display name. It then applies the spec. Members do not see the config before that. +- The operator creates the config disabled, with a temporary display name that holds a secret nonce (see [Cleanup records](#cleanup-records)). It then applies the spec. Members do not see the config before that. - The operator pins the organization ID at the first lookup. If the name later resolves to another ID, the object stalls with `OrganizationChanged`. Re-create the object to use the new organization. - Coder's `user_oidc` auth type is not supported. If someone sets it in the dashboard, the operator sets the auth type back at the next sync. @@ -218,13 +218,17 @@ Delete `CoderMCPServer` objects before their control plane and before their name ### Cleanup records -Before the operator creates a config in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderMCPServer` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the config. The record holds no credential: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the config's temporary display name, the pinned organization ID, the slug, and the config ID once the operator knows it. A config that an earlier operator version created gets its record on the next reconcile. A config that was left in Coder before that has no record: delete it in the dashboard. +Before the operator creates a config in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderMCPServer` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the config. The record holds no credential: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the nonce, the pinned organization ID, the slug, and the config ID once the operator knows it. A config that an earlier operator version created gets its record on the next reconcile. A config that was left in Coder before that has no record: delete it in the dashboard. + +The nonce is the temporary display name of the create: `coder-k8s:` and a random secret. When a create answer is lost, the operator takes the config with the slug as its own only when its display name is the recorded nonce or its ID is the recorded config ID. As for [AI providers](manage-ai-providers.md#cleanup-records), only the operator and cluster admins can read the record, the operator sends the same nonce again while its last create is unconfirmed, and it makes a new nonce after it knows the config ID. Records of older operator versions hold the nonce `coder-k8s:` until the next create. Do not give anyone but cluster admins read or write access to ConfigMaps in the operator's namespace. + +The record of an older config needs the deployment ID that Coder answers. When that read or the record write fails, and the delete fails too, in the one bounded attempt while the control plane is being deleted (for example in a namespace deletion), the finalizer goes without a record. The operator does not hold the namespace deletion longer. It logs the slug and the config ID with `ControlPlaneGone`: delete the config in the dashboard. When the control plane later reaches another Coder deployment (for example after its database Secret changed), the record still locates the config in the old one. The operator then stalls the object with `CoderDeploymentChanged`, keeps the record and creates nothing in the new deployment. Delete the config in the old deployment, then delete the ConfigMap. The operator removes the record when Coder confirms that the config is deleted. When the finalizer goes without a delete, the record stays, and the operator sweeps it as for [AI providers](manage-ai-providers.md#cleanup-records): once the object is gone and 5 minutes have passed since the last create, through a ready control plane in the object's namespace whose Coder answers the record's deployment ID. The sweep deletes the recorded config ID. A record that no control plane reaches stays, with the gauge `coder_k8s_cleanup_records_unmatched` and a `NoMatchingDeployment` Warning Event. When that Coder deployment never comes back, delete the config in the Coder dashboard, then delete the ConfigMap. -A record without a config ID is left by a create that the operator could not confirm. The sweep deletes nothing for such a record: a person can edit the slug and the display name of any config, so neither proves that the operator created it. When the organization has no config with the recorded slug and none with the temporary display name, the sweep removes the record. Otherwise the record stays, the gauge `coder_k8s_cleanup_records_unmatched` counts it, and the operator writes a `ManualCleanupNeeded` Warning Event on the ConfigMap that names the config. Clean up by hand: +A record without a config ID is left by a create that the operator could not confirm. The sweep deletes nothing for such a record: a person can edit the slug and the display name of any config, so neither proves that the operator created it. When the organization has no config with the recorded slug and none with the recorded nonce, the sweep removes the record. So when a person edits both the slug and the display name of the config of a lost create answer, nothing links that config to the record any more: the sweep removes the record, and the config stays in Coder. Otherwise the record stays, the gauge `coder_k8s_cleanup_records_unmatched` counts it, and the operator writes a `ManualCleanupNeeded` Warning Event on the ConfigMap that names the config. Clean up by hand: 1. Read the record and its Event: From a0003e6ae0684266311bee9552633e5b0cc6ff9f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:42:34 +0000 Subject: [PATCH 06/14] test: a nonce that Coder showed does not prove another resource (#426) Change-Id: I8d0394a03baa8bb1bd1764ce252a2c42713f8a64 Signed-off-by: Thomas Kosiewski --- .../controller/coderaiprovider_nonce_test.go | 29 +++++++++++++++++++ .../controller/codermcpserver_nonce_test.go | 26 +++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/internal/controller/coderaiprovider_nonce_test.go b/internal/controller/coderaiprovider_nonce_test.go index 9c2f5734..c2306cc5 100644 --- a/internal/controller/coderaiprovider_nonce_test.go +++ b/internal/controller/coderaiprovider_nonce_test.go @@ -151,3 +151,32 @@ func TestCoderAIProviderOldNonceAdoptedUntilNextCreate(t *testing.T) { require.Equal(t, 2, env.fake.requestCount(routeCreateAIProvider), "the setup create and one create of the controller") requireRandomNonce(t, cleanupRecord(ctx, t, env.namespace, got.UID)["nonce"], got.UID) } + +// #426: after the operator learned the provider ID, Coder has shown the nonce +// as the display name. A person who saw it can delete the provider and create +// one of the same name with that display name. It is not this object's. +func TestCoderAIProviderShownNonceNotAdopted(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusInternalServerError}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + stored, _ := env.fake.aiProvider(provider.Name) + nonce := stored.displayName + require.Equal(t, cleanupRecord(ctx, t, env.namespace, got.UID)["nonce"], nonce) + + person := env.fake.client(t, 2*time.Second) + require.NoError(t, person.DeleteAIProvider(ctx, stored.id.String())) + _, err := person.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, Name: provider.Name, DisplayName: nonce, + BaseURL: provider.Spec.BaseURL, APIKeys: []string{"sk-person-0123456789abcdef"}, + }) + require.NoError(t, err) + + got, _ = env.reconcile(ctx, t, got) + requireAIProviderStep(t, got, true, "NameConflict") + require.Empty(t, got.Status.ProviderID, "the person's provider is not adopted") + stored, _ = env.fake.aiProvider(provider.Name) + require.Equal(t, nonce, stored.displayName, "the controller did not change it") +} diff --git a/internal/controller/codermcpserver_nonce_test.go b/internal/controller/codermcpserver_nonce_test.go index d9d6ac98..2ed53abe 100644 --- a/internal/controller/codermcpserver_nonce_test.go +++ b/internal/controller/codermcpserver_nonce_test.go @@ -121,3 +121,29 @@ func TestCoderMCPServerOldNonceAdoptedUntilNextCreate(t *testing.T) { require.Equal(t, 2, env.posts(), "the setup create and one create of the controller") requireRandomNonce(t, env.mcpServerRecord(ctx, t, got.UID)["nonce"], got.UID) } + +// #426: after the operator learned the config ID, Coder has shown the nonce +// as the display name. A person who saw it can delete the config and create +// one with the slug and that display name. It is not this object's. +func TestCoderMCPServerShownNonceNotAdopted(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, false, "CoderUnavailable") + created := env.requireCreated(t, got).view + require.Equal(t, env.mcpServerRecord(ctx, t, got.UID)["nonce"], created.DisplayName) + + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, created.ID)) + req := apiKeyMCPRequest(got.Spec.Slug) + req.DisplayName = created.DisplayName + _, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, req) + require.NoError(t, err) + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "SlugConflict") + require.Empty(t, got.Status.ConfigID, "the person's config is not adopted") + require.Zero(t, env.fake.requestCount(routeUpdateMCPServer)-1, "the controller did not change it") +} From 5a5568ea2df720d3bcd3f864882b44603b95b6bb Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:43:32 +0000 Subject: [PATCH 07/14] fix: once the Coder ID is recorded, its nonce adopts no other resource (#426) Change-Id: I6cfb631af524abd36a9b552558341491f53b328e Signed-off-by: Thomas Kosiewski --- internal/controller/cleanup_record.go | 12 ++++++++++++ internal/controller/coderaiprovider_create.go | 6 +----- internal/controller/codermcpserver_create.go | 7 ++----- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go index 5a62ddd5..ae6699ef 100644 --- a/internal/controller/cleanup_record.go +++ b/internal/controller/cleanup_record.go @@ -103,6 +103,18 @@ func keptCreateNonce(prefix string, uid types.UID, previous *cleanupRecord) stri return prefix + string(uid) } +// adoptionNonce returns the nonce that proves that a resource with the Coder +// ID coderID is the object's: the recorded nonce while the record has no +// Coder ID, or for the recorded ID itself. Once the operator knows the ID, +// Coder has shown the nonce, so it proves nothing about another resource. +// Without a record it returns "". +func adoptionNonce(record *cleanupRecord, coderID string) string { + if record == nil || (record.CoderID != "" && record.CoderID != coderID) { + return "" + } + return record.Nonce +} + // validCreateNonce reports a nonce that the operator can have written in the // record of uid. func validCreateNonce(prefix string, uid types.UID, nonce string) bool { diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index be46e1a9..383519be 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -73,11 +73,7 @@ func (r *CoderAIProviderReconciler) createOrConfirm( if err != nil { return nil, err } - nonce := "" - if record != nil { - nonce = record.Nonce - } - return r.recordAIProvider(ctx, provider, found, nonce) + return r.recordAIProvider(ctx, provider, found, adoptionNonce(record, found.ID.String())) } // keepRejectedCreate keeps the memory of a rejected create in the reason of a diff --git a/internal/controller/codermcpserver_create.go b/internal/controller/codermcpserver_create.go index 586e4bd6..530c72ad 100644 --- a/internal/controller/codermcpserver_create.go +++ b/internal/controller/codermcpserver_create.go @@ -54,17 +54,14 @@ func (r *CoderMCPServerReconciler) createOrRecord( if err != nil { return nil, err } - nonce, recordedID := "", "" - if record != nil { - nonce = record.Nonce - } + recordedID := "" if record != nil && record.CoderID == found.ID.String() { if step, err := r.recordedDeploymentChanged(ctx, sdk, server); step != nil || err != nil { return step, err } recordedID = record.CoderID } - if step := recordMCPServer(server, *found, nonce, recordedID); step != nil { + if step := recordMCPServer(server, *found, adoptionNonce(record, found.ID.String()), recordedID); step != nil { return step, nil } // Store the ID before the sync replaces the nonce (#353). From 84fb3131fb838bbdfb238da533584964556b3370 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:46:31 +0000 Subject: [PATCH 08/14] test: a hanging deployment ID read during the bounded delete still releases (#422) Change-Id: I4a95915b3b9b432eaa756e284975405a14cbc111 Signed-off-by: Thomas Kosiewski --- .../controller/coderaiprovider_record_test.go | 15 +++++++++++++++ .../controller/codermcpserver_record_test.go | 17 +++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index 81444be7..b8d454c4 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -191,6 +191,21 @@ func TestCoderAIProviderRecordBackfillOnDelete(t *testing.T) { require.Equal(t, env.fake.deploymentID, record["deploymentID"]) } +// #422 item 3: a deployment ID read that hangs through the one bounded +// attempt leaves no record, but the finalizer still goes after that attempt. +func TestCoderAIProviderRecordBackfillOnDeleteHangs(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)}})) + holdControlPlane(ctx, t, env.namespace) + env.fake.failNext(routeBuildInfo, fakeFault{Hang: true}) + + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone, "released after one attempt") + require.Nil(t, cleanupRecord(ctx, t, env.namespace, provider.UID), "no deployment ID, so no record") +} + // #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) { diff --git a/internal/controller/codermcpserver_record_test.go b/internal/controller/codermcpserver_record_test.go index 2d6f0aab..c8a116e0 100644 --- a/internal/controller/codermcpserver_record_test.go +++ b/internal/controller/codermcpserver_record_test.go @@ -285,6 +285,23 @@ func TestCoderMCPServerRecordBackfillOnDelete(t *testing.T) { require.Equal(t, env.fake.deploymentID, record["deploymentID"]) } +// #422 item 3: a deployment ID read that hangs through the one bounded +// attempt leaves no record, but the finalizer still goes after that attempt. +func TestCoderMCPServerRecordBackfillOnDeleteHangs(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(got.UID)}})) + holdControlPlane(ctx, t, env.namespace) + env.fake.failNext(routeBuildInfo, fakeFault{Hang: true}) + + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone, "released after one attempt") + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID), "no deployment ID, so no record") +} + // #353: a create answer that was lost is confirmed by the next reconcile, // which records the config ID before the sync replaces the nonce. func TestCoderMCPServerRecordAfterLostCreateAnswer(t *testing.T) { From 2bbf4372820082ed3c705f6bdeef6d496c8fa3d8 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:47:22 +0000 Subject: [PATCH 09/14] fix: the legacy record backfill on delete leaves the bounded attempt able to release (#422) Change-Id: Id24ae05ac3be7f5ec092d17d3097e49c409598ae Signed-off-by: Thomas Kosiewski --- internal/controller/coderaiprovider_delete.go | 16 +++++++++------- internal/controller/codermcpserver_delete.go | 10 ++++------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 285980bc..8feadb34 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -126,18 +126,20 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co ctx, cancel = context.WithTimeout(ctx, aiProviderBoundedDeleteTimeout) defer cancel() } - if provider.Status.ProviderID != "" && checkAIProviderID(provider) == nil { - // A provider from before records existed gets its record before a - // delete that can release without a confirmation (#422 item 3). - 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) - } - } // 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 provider from before records existed gets its record before a + // delete that can release without a confirmation (#422 item 3). The + // write takes the deployment ID that Coder answers now, so no check + // follows, and nothing after it reads Kubernetes with a context that + // a hanging Coder call used up. + 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) + } } 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) diff --git a/internal/controller/codermcpserver_delete.go b/internal/controller/codermcpserver_delete.go index 09600258..aa06ef74 100644 --- a/internal/controller/codermcpserver_delete.go +++ b/internal/controller/codermcpserver_delete.go @@ -113,13 +113,11 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder } if record == nil && server.Status.ConfigID != "" { // A config from before records existed gets its record before a - // delete that can release without a confirmation. + // delete that can release without a confirmation. The write takes + // the deployment ID that Coder answers now, so no check follows, and + // nothing after it reads Kubernetes with a context that a hanging + // Coder call used up (#422 item 3). r.backfillMCPServerRecord(ctx, sdk, server) - if record, _, err = readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, server.UID); errors.Is(err, errNotACleanupRecord) { - record = nil - } else if err != nil { - return false, nil, err - } } recordedID, recordedNonce := "", "" if record != nil { From 2eb912f96e7f2a7901a151ead1bbb75650fd54dd Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:48:04 +0000 Subject: [PATCH 10/14] docs: the nonce window in Coder and the known-ID rule (#426) Change-Id: I44fd62a03699678a24c555ccb5b79d6608363ee3 Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-providers.md | 2 +- docs/how-to/manage-mcp-servers.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 2f385052..33197218 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -136,7 +136,7 @@ Delete `CoderAIProvider` objects before their namespace: a namespace deletion re Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. -The nonce is the display name that the create sends: `coder-k8s:` and a random secret. Only the record holds the secret. When a create answer is lost, the operator takes a provider of the object's name and type as its own only when its display name is the recorded nonce. A person cannot give a provider that display name before the create, because only the operator and cluster admins can read the record. The operator sends the same nonce again while its last create is unconfirmed, so a create that commits late is still recognized. After it knows the provider ID, it makes a new nonce for the next create: Coder showed the old one as the display name until the operator applied the spec. Records of older operator versions hold the nonce `coder-k8s:`, which anyone who can read the object sees. The operator keeps that nonce until it next creates the provider, so a lost create answer of an older version is still recognized. +The nonce is the display name that the create sends: `coder-k8s:` and a random secret. Only the record holds the secret. When a create answer is lost, the operator takes a provider of the object's name and type as its own only when its display name is the recorded nonce. A person cannot give a provider that display name before the create, because only the operator and cluster admins can read the record. After the create commits, Coder shows the nonce to users who can read the provider, until the operator applies the spec. Such a user could delete the provider and create another one of that name with that display name, but only within that short window, and such a user can delete the provider anyway. The operator sends the same nonce again while its last create is unconfirmed, so a create that commits late is still recognized. After it knows the provider ID, it never takes another provider by that nonce, and it makes a new nonce for the next create. Records of older operator versions hold the nonce `coder-k8s:`, which anyone who can read the object sees. The operator keeps that nonce until it next creates the provider, so a lost create answer of an older version is still recognized. A provider that an older operator version created gets its record at the next reconcile, also while its object is being deleted. The record needs the deployment ID that Coder answers. When that read or the record write fails, and the delete fails too, in the one bounded attempt while the control plane is being deleted (for example in a namespace deletion), the finalizer goes without a record. The operator does not hold the namespace deletion longer. It logs the provider ID with `ControlPlaneGone`: delete the provider in the dashboard. diff --git a/docs/how-to/manage-mcp-servers.md b/docs/how-to/manage-mcp-servers.md index bffbcb9b..4a65f8f2 100644 --- a/docs/how-to/manage-mcp-servers.md +++ b/docs/how-to/manage-mcp-servers.md @@ -220,7 +220,7 @@ Delete `CoderMCPServer` objects before their control plane and before their name Before the operator creates a config in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderMCPServer` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the config. The record holds no credential: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the nonce, the pinned organization ID, the slug, and the config ID once the operator knows it. A config that an earlier operator version created gets its record on the next reconcile. A config that was left in Coder before that has no record: delete it in the dashboard. -The nonce is the temporary display name of the create: `coder-k8s:` and a random secret. When a create answer is lost, the operator takes the config with the slug as its own only when its display name is the recorded nonce or its ID is the recorded config ID. As for [AI providers](manage-ai-providers.md#cleanup-records), only the operator and cluster admins can read the record, the operator sends the same nonce again while its last create is unconfirmed, and it makes a new nonce after it knows the config ID. Records of older operator versions hold the nonce `coder-k8s:` until the next create. Do not give anyone but cluster admins read or write access to ConfigMaps in the operator's namespace. +The nonce is the temporary display name of the create: `coder-k8s:` and a random secret. When a create answer is lost, the operator takes the config with the slug as its own only when its display name is the recorded nonce or its ID is the recorded config ID. The rules are the same as for [AI providers](manage-ai-providers.md#cleanup-records). Only the operator and cluster admins can read the record. Coder shows the nonce only until the operator applies the spec. The operator sends the same nonce again while its last create is unconfirmed. After it knows the config ID, it takes no other config by that nonce, and the next create gets a new nonce. Records of older operator versions hold the nonce `coder-k8s:` until the next create. Do not give anyone but cluster admins read or write access to ConfigMaps in the operator's namespace. The record of an older config needs the deployment ID that Coder answers. When that read or the record write fails, and the delete fails too, in the one bounded attempt while the control plane is being deleted (for example in a namespace deletion), the finalizer goes without a record. The operator does not hold the namespace deletion longer. It logs the slug and the config ID with `ControlPlaneGone`: delete the config in the dashboard. From 350d536922447ddf9e800584bb0b6a3ae2b67684 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:54:37 +0000 Subject: [PATCH 11/14] test: old UID nonces need a pending create, and the record backfill leaves time for the delete (#426, #422) Change-Id: Ia4f49570e3917aa36257d7a031f052bb6380b87c Signed-off-by: Thomas Kosiewski --- .../controller/coderaiprovider_nonce_test.go | 24 +++++++++++++++++ .../controller/coderaiprovider_record_test.go | 9 ++++--- .../controller/codermcpserver_nonce_test.go | 27 ++++++++++++++++++- .../controller/codermcpserver_record_test.go | 8 +++--- 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/internal/controller/coderaiprovider_nonce_test.go b/internal/controller/coderaiprovider_nonce_test.go index c2306cc5..f01e3471 100644 --- a/internal/controller/coderaiprovider_nonce_test.go +++ b/internal/controller/coderaiprovider_nonce_test.go @@ -180,3 +180,27 @@ func TestCoderAIProviderShownNonceNotAdopted(t *testing.T) { stored, _ = env.fake.aiProvider(provider.Name) require.Equal(t, nonce, stored.displayName, "the controller did not change it") } + +// #426 migration: the nonce coder-k8s: of an older record proves a +// create only while that create is unconfirmed. An object that an older +// version left stalled with NameConflict never creates again, so a person +// who later gives a provider of its name that display name does not get it +// adopted. +func TestCoderAIProviderOldNonceNotAdoptedWithoutPendingCreate(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + createOldCleanupRecord(ctx, t, env.namespace, "CoderAIProvider", provider.UID, map[string]string{ + "namespace": env.namespace, "name": provider.Name, "deploymentID": env.fake.deploymentID, "coderID": "", "type": "openai", + "createAttemptTime": time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), + }) + _, err := env.fake.client(t, 2*time.Second).CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, Name: provider.Name, DisplayName: "coder-k8s:" + string(provider.UID), + BaseURL: provider.Spec.BaseURL, APIKeys: []string{"sk-person-0123456789abcdef"}, + }) + require.NoError(t, err) + + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, true, "NameConflict") + require.Empty(t, got.Status.ProviderID, "the person's provider is not adopted") +} diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index b8d454c4..aa0eba7d 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -191,8 +191,8 @@ func TestCoderAIProviderRecordBackfillOnDelete(t *testing.T) { require.Equal(t, env.fake.deploymentID, record["deploymentID"]) } -// #422 item 3: a deployment ID read that hangs through the one bounded -// attempt leaves no record, but the finalizer still goes after that attempt. +// #422 item 3: a deployment ID read that hangs leaves no record, but it does +// not use up the one bounded attempt: the delete still goes out and succeeds. func TestCoderAIProviderRecordBackfillOnDeleteHangs(t *testing.T) { t.Parallel() ctx := context.Background() @@ -203,7 +203,10 @@ func TestCoderAIProviderRecordBackfillOnDeleteHangs(t *testing.T) { gone, _ := env.deleteAIProvider(ctx, t, provider) require.Nil(t, gone, "released after one attempt") - require.Nil(t, cleanupRecord(ctx, t, env.namespace, provider.UID), "no deployment ID, so no record") + require.Equal(t, 1, env.fake.requestCount(routeDeleteAIProvider)) + _, ok := env.fake.aiProvider("openai") + require.False(t, ok, "the delete had time left") + require.Nil(t, cleanupRecord(ctx, t, env.namespace, provider.UID)) } // #374 review: the controller reads records live. A cache that does not diff --git a/internal/controller/codermcpserver_nonce_test.go b/internal/controller/codermcpserver_nonce_test.go index 2ed53abe..e9031302 100644 --- a/internal/controller/codermcpserver_nonce_test.go +++ b/internal/controller/codermcpserver_nonce_test.go @@ -145,5 +145,30 @@ func TestCoderMCPServerShownNonceNotAdopted(t *testing.T) { got, _ = env.reconcile(ctx, t, got) requireMCPServerStep(t, got, true, "SlugConflict") require.Empty(t, got.Status.ConfigID, "the person's config is not adopted") - require.Zero(t, env.fake.requestCount(routeUpdateMCPServer)-1, "the controller did not change it") + require.Equal(t, 1, env.fake.requestCount(routeUpdateMCPServer), "the controller did not change it") +} + +// #426 migration: the nonce coder-k8s: of an older record proves a +// create only while that create is unconfirmed. An object that an older +// version left stalled with SlugConflict never creates again, so a person who +// later gives a config with its slug that display name does not get it +// adopted. +func TestCoderMCPServerOldNonceNotAdoptedWithoutPendingCreate(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + createOldCleanupRecord(ctx, t, env.namespace, "CoderMCPServer", server.UID, map[string]string{ + "namespace": env.namespace, "name": server.Name, "deploymentID": env.fake.deploymentID, "coderID": "", "type": "", + "organizationID": env.orgID.String(), "slug": server.Spec.Slug, "createAttemptTime": time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), + }) + req := apiKeyMCPRequest(server.Spec.Slug) + req.DisplayName = "coder-k8s:" + string(server.UID) + _, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, req) + require.NoError(t, err) + + got, _ := env.reconcile(ctx, t, server) + requireMCPServerStep(t, got, true, "SlugConflict") + require.Empty(t, got.Status.ConfigID, "the person's config is not adopted") } diff --git a/internal/controller/codermcpserver_record_test.go b/internal/controller/codermcpserver_record_test.go index c8a116e0..64ee44a9 100644 --- a/internal/controller/codermcpserver_record_test.go +++ b/internal/controller/codermcpserver_record_test.go @@ -285,8 +285,8 @@ func TestCoderMCPServerRecordBackfillOnDelete(t *testing.T) { require.Equal(t, env.fake.deploymentID, record["deploymentID"]) } -// #422 item 3: a deployment ID read that hangs through the one bounded -// attempt leaves no record, but the finalizer still goes after that attempt. +// #422 item 3: a deployment ID read that hangs leaves no record, but it does +// not use up the one bounded attempt: the delete still goes out and succeeds. func TestCoderMCPServerRecordBackfillOnDeleteHangs(t *testing.T) { t.Parallel() ctx := context.Background() @@ -299,7 +299,9 @@ func TestCoderMCPServerRecordBackfillOnDeleteHangs(t *testing.T) { gone, _ := env.deleteMCPServer(ctx, t, got) require.Nil(t, gone, "released after one attempt") - require.Nil(t, env.mcpServerRecord(ctx, t, got.UID), "no deployment ID, so no record") + require.Equal(t, 1, env.fake.requestCount(routeDeleteMCPServer)) + require.Empty(t, env.fake.mcpServers(), "the delete had time left") + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID)) } // #353: a create answer that was lost is confirmed by the next reconcile, From 3428844a9416f7304d651e1aff6ba303c8c4b143 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:55:47 +0000 Subject: [PATCH 12/14] fix: old UID nonces adopt only for a pending create, and the record backfill leaves time for the delete (#426, #422) Change-Id: I291c169de2a99b325ecdd6353c24820b3509caba Signed-off-by: Thomas Kosiewski --- internal/controller/cleanup_record.go | 8 +++++--- internal/controller/coderaiprovider_create.go | 2 +- internal/controller/coderaiprovider_delete.go | 15 +++++++++++---- internal/controller/codermcpserver_create.go | 3 ++- internal/controller/codermcpserver_delete.go | 10 ++++++---- 5 files changed, 25 insertions(+), 13 deletions(-) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go index ae6699ef..b355cb57 100644 --- a/internal/controller/cleanup_record.go +++ b/internal/controller/cleanup_record.go @@ -107,9 +107,11 @@ func keptCreateNonce(prefix string, uid types.UID, previous *cleanupRecord) stri // ID coderID is the object's: the recorded nonce while the record has no // Coder ID, or for the recorded ID itself. Once the operator knows the ID, // Coder has shown the nonce, so it proves nothing about another resource. -// Without a record it returns "". -func adoptionNonce(record *cleanupRecord, coderID string) string { - if record == nil || (record.CoderID != "" && record.CoderID != coderID) { +// The nonce prefix and object UID of an older record prove only a create that +// is still pending: anyone who can read the object knows them. Without a +// record it returns "". +func adoptionNonce(prefix string, record *cleanupRecord, coderID string, pending bool) string { + if record == nil || (record.CoderID != "" && record.CoderID != coderID) || (!pending && !isRandomCreateNonce(prefix, record.Nonce)) { return "" } return record.Nonce diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index 383519be..6624a558 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -73,7 +73,7 @@ func (r *CoderAIProviderReconciler) createOrConfirm( if err != nil { return nil, err } - return r.recordAIProvider(ctx, provider, found, adoptionNonce(record, found.ID.String())) + return r.recordAIProvider(ctx, provider, found, adoptionNonce(aiProviderNoncePrefix, record, found.ID.String(), provider.Status.CreateAttemptTime != nil)) } // keepRejectedCreate keeps the memory of a rejected create in the reason of a diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 8feadb34..d8d7e8f0 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -135,10 +135,17 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co // A provider from before records existed gets its record before a // delete that can release without a confirmation (#422 item 3). The // write takes the deployment ID that Coder answers now, so no check - // follows, and nothing after it reads Kubernetes with a context that - // a hanging Coder call used up. - 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) + // follows. It gets half of the bounded attempt, so a hanging Coder + // call leaves time for the delete. + backfillCtx, cancel := context.WithTimeout(ctx, aiProviderBoundedDeleteTimeout/2) + step, err := r.ensureAIProviderRecord(backfillCtx, sdk, provider) + cancel() + if err != nil || step != nil { + message := "" + if step != nil { + message = step.message + } + log.FromContext(ctx).Info("Cannot write the cleanup record of a provider that exists in Coder", "error", err, "step", message) } } else if record != nil && (provider.Status.ProviderID == "" || checkAIProviderID(provider) == nil) { // A forged provider ID fails its assertion below, before any request. diff --git a/internal/controller/codermcpserver_create.go b/internal/controller/codermcpserver_create.go index 530c72ad..2ccfc366 100644 --- a/internal/controller/codermcpserver_create.go +++ b/internal/controller/codermcpserver_create.go @@ -61,7 +61,8 @@ func (r *CoderMCPServerReconciler) createOrRecord( } recordedID = record.CoderID } - if step := recordMCPServer(server, *found, adoptionNonce(record, found.ID.String()), recordedID); step != nil { + nonce := adoptionNonce(mcpServerNoncePrefix, record, found.ID.String(), server.Status.CreateAttemptTime != nil) + if step := recordMCPServer(server, *found, nonce, recordedID); step != nil { return step, nil } // Store the ID before the sync replaces the nonce (#353). diff --git a/internal/controller/codermcpserver_delete.go b/internal/controller/codermcpserver_delete.go index aa06ef74..763a2502 100644 --- a/internal/controller/codermcpserver_delete.go +++ b/internal/controller/codermcpserver_delete.go @@ -114,10 +114,12 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder if record == nil && server.Status.ConfigID != "" { // A config from before records existed gets its record before a // delete that can release without a confirmation. The write takes - // the deployment ID that Coder answers now, so no check follows, and - // nothing after it reads Kubernetes with a context that a hanging - // Coder call used up (#422 item 3). - r.backfillMCPServerRecord(ctx, sdk, server) + // the deployment ID that Coder answers now, so no check follows. It + // gets half of the bounded attempt, so a hanging Coder call leaves + // time for the delete (#422 item 3). + backfillCtx, cancel := context.WithTimeout(ctx, mcpServerBoundedDeleteTimeout/2) + r.backfillMCPServerRecord(backfillCtx, sdk, server) + cancel() } recordedID, recordedNonce := "", "" if record != nil { From 3e9f6a3892e7e947f1c5fe0f51521e5caff82c97 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:56:26 +0000 Subject: [PATCH 13/14] docs: old UID nonces need a pending create, and who must not read records (#426) Change-Id: I1e511651edd2d12e5e0cc67e50a7a2d43b285028 Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-providers.md | 4 ++-- docs/how-to/manage-mcp-servers.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 33197218..33f8d0a8 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -136,7 +136,7 @@ Delete `CoderAIProvider` objects before their namespace: a namespace deletion re Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. -The nonce is the display name that the create sends: `coder-k8s:` and a random secret. Only the record holds the secret. When a create answer is lost, the operator takes a provider of the object's name and type as its own only when its display name is the recorded nonce. A person cannot give a provider that display name before the create, because only the operator and cluster admins can read the record. After the create commits, Coder shows the nonce to users who can read the provider, until the operator applies the spec. Such a user could delete the provider and create another one of that name with that display name, but only within that short window, and such a user can delete the provider anyway. The operator sends the same nonce again while its last create is unconfirmed, so a create that commits late is still recognized. After it knows the provider ID, it never takes another provider by that nonce, and it makes a new nonce for the next create. Records of older operator versions hold the nonce `coder-k8s:`, which anyone who can read the object sees. The operator keeps that nonce until it next creates the provider, so a lost create answer of an older version is still recognized. +The nonce is the display name that the create sends: `coder-k8s:` and a random secret. Only the record holds the secret. When a create answer is lost, the operator takes a provider of the object's name and type as its own only when its display name is the recorded nonce. Namespace users cannot read the record (see the access rule below), so they cannot give a provider that display name before the create. After the create commits, Coder shows the nonce to users who can read the provider, until the operator applies the spec. Until the operator learns the provider ID, normally at its next read a few seconds later, such a user could delete the provider and create another one of that name with that display name. Such a user can delete the provider anyway. Until the operator knows the provider ID, every create sends the same nonce, so a create that commits late is still recognized. After it knows the ID, it never takes another provider by that nonce, and the next create gets a new nonce. Records of older operator versions hold the nonce `coder-k8s:`, which anyone who can read the object sees. The operator accepts that nonce only while the create of the older version is unconfirmed (`status.createAttemptTime` is set), and the next create gets a random nonce. A provider that an older operator version created gets its record at the next reconcile, also while its object is being deleted. The record needs the deployment ID that Coder answers. When that read or the record write fails, and the delete fails too, in the one bounded attempt while the control plane is being deleted (for example in a namespace deletion), the finalizer goes without a record. The operator does not hold the namespace deletion longer. It logs the provider ID with `ControlPlaneGone`: delete the provider in the dashboard. @@ -164,7 +164,7 @@ A record without a provider ID is left by a create that the operator could not c 3. When that provider is a leftover of the deleted object, delete it. The next sweep, within 10 minutes, then removes the record. 4. When a person owns that provider, keep it and delete the ConfigMap. -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. Do not give them read access either: a record holds the secret nonce of a create. The built-in `view`, `edit` and `admin` roles include ConfigMaps, so bind them in that namespace only to cluster admins. If you install the operator in another namespace, change the namespace of both. +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. Do not give them read access either: a record holds the secret nonce of a create. The install keeps namespace users out, but the built-in `view`, `edit` and `admin` roles include ConfigMaps: do not bind them to other users in that namespace, and do not bind them cluster-wide with a ClusterRoleBinding. If you install the operator in another namespace, change the namespace of both. ### Stuck finalizer diff --git a/docs/how-to/manage-mcp-servers.md b/docs/how-to/manage-mcp-servers.md index 4a65f8f2..58517d25 100644 --- a/docs/how-to/manage-mcp-servers.md +++ b/docs/how-to/manage-mcp-servers.md @@ -220,7 +220,7 @@ Delete `CoderMCPServer` objects before their control plane and before their name Before the operator creates a config in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderMCPServer` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the config. The record holds no credential: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the nonce, the pinned organization ID, the slug, and the config ID once the operator knows it. A config that an earlier operator version created gets its record on the next reconcile. A config that was left in Coder before that has no record: delete it in the dashboard. -The nonce is the temporary display name of the create: `coder-k8s:` and a random secret. When a create answer is lost, the operator takes the config with the slug as its own only when its display name is the recorded nonce or its ID is the recorded config ID. The rules are the same as for [AI providers](manage-ai-providers.md#cleanup-records). Only the operator and cluster admins can read the record. Coder shows the nonce only until the operator applies the spec. The operator sends the same nonce again while its last create is unconfirmed. After it knows the config ID, it takes no other config by that nonce, and the next create gets a new nonce. Records of older operator versions hold the nonce `coder-k8s:` until the next create. Do not give anyone but cluster admins read or write access to ConfigMaps in the operator's namespace. +The nonce is the temporary display name of the create: `coder-k8s:` and a random secret. When a create answer is lost, the operator takes the config with the slug as its own only when its display name is the recorded nonce or its ID is the recorded config ID. The rules are the same as for [AI providers](manage-ai-providers.md#cleanup-records). Namespace users cannot read the record. Coder shows the nonce until the operator applies the spec. Until the operator knows the config ID, every create sends the same nonce. After it knows the ID, it takes no other config by that nonce, and the next create gets a new nonce. Records of older operator versions hold the nonce `coder-k8s:`. The operator accepts that nonce only while the create of the older version is unconfirmed (`status.createAttemptTime` is set), and the next create gets a random nonce. Do not give other users read or write access to ConfigMaps in the operator's namespace, also not through a ClusterRoleBinding of `view`, `edit` or `admin`. The record of an older config needs the deployment ID that Coder answers. When that read or the record write fails, and the delete fails too, in the one bounded attempt while the control plane is being deleted (for example in a namespace deletion), the finalizer goes without a record. The operator does not hold the namespace deletion longer. It logs the slug and the config ID with `ControlPlaneGone`: delete the config in the dashboard. From f18b14f64f948b4be773acb226f58f7fad28d4eb Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:56:52 +0000 Subject: [PATCH 14/14] chore: comment the pending rule for old UID nonces (#426) Change-Id: I735cf368639faf217a6219f073aef7e884d8900f Signed-off-by: Thomas Kosiewski --- internal/controller/cleanup_record.go | 5 +++-- internal/controller/coderaiprovider_nonce_test.go | 6 +++--- internal/controller/codermcpserver_nonce_test.go | 6 +++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go index b355cb57..1358a6e4 100644 --- a/internal/controller/cleanup_record.go +++ b/internal/controller/cleanup_record.go @@ -78,8 +78,9 @@ func cleanupRecordObjectName(uid types.UID) string { return cleanupRecordPrefix // and a random secret (#426). Only the record holds it, and namespace users // cannot read records, so nobody can give another resource that display name // before the create commits. Operator versions before #426 sent prefix and -// the object UID, which anyone who can read the object sees. A record of such -// a version keeps that nonce until the next create. +// the object UID, which anyone who can read the object sees. That nonce +// proves only a create that is still pending, and the next create gets a +// random one. // nextCreateNonce returns the nonce for a create of an object whose record // is previous. A create of the previous nonce can still commit while the diff --git a/internal/controller/coderaiprovider_nonce_test.go b/internal/controller/coderaiprovider_nonce_test.go index f01e3471..3abea46f 100644 --- a/internal/controller/coderaiprovider_nonce_test.go +++ b/internal/controller/coderaiprovider_nonce_test.go @@ -117,9 +117,9 @@ func TestCoderAIProviderNonceReuse(t *testing.T) { } // #426 migration: a record of an operator version before #426 keeps its -// nonce coder-k8s: until the next create, so a create of that version -// whose answer was lost is still adopted. The next create gets a random -// nonce. +// nonce coder-k8s: until the next create, so a pending create of that +// version whose answer was lost is still adopted. The next create gets a +// random nonce. func TestCoderAIProviderOldNonceAdoptedUntilNextCreate(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/internal/controller/codermcpserver_nonce_test.go b/internal/controller/codermcpserver_nonce_test.go index e9031302..91a35089 100644 --- a/internal/controller/codermcpserver_nonce_test.go +++ b/internal/controller/codermcpserver_nonce_test.go @@ -87,9 +87,9 @@ func TestCoderMCPServerNewNonceAfterKnownID(t *testing.T) { } // #426 migration: a record of an operator version before #426 keeps its -// nonce coder-k8s: until the next create, so a create of that version -// whose answer was lost is still adopted. The next create gets a random -// nonce. +// nonce coder-k8s: until the next create, so a pending create of that +// version whose answer was lost is still adopted. The next create gets a +// random nonce. func TestCoderMCPServerOldNonceAdoptedUntilNextCreate(t *testing.T) { t.Parallel() ctx := context.Background()