From d8a793fa9763348231b6cdfc0f382d706aa57cfd Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 19:26:17 +0000 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=A4=96=20feat:=20sync=20CoderAIProvid?= =?UTF-8?q?er=20drift,=20key=20rotation=20and=20kill=20switches?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CoderAIProvider reconciler (still not registered) now reads the Coder provider on every reconcile and sends one sparse PATCH with every field that differs from the spec. The base URL and the key travel in the same PATCH, so Coder never enables a provider at a stale URL. A changed key Secret value is resent behind a resend marker, and an in-memory SHA-256 skips resends for metadata-only Secret changes. After a full apply, a dashboard delete, disable or key change stalls the object until the spec changes. A 4xx create answer is now confirmed by a read before the create marker is cleared, and a nonce match without a recorded credential is recorded instead of failing an assertion. Refs #154 Signed-off-by: Thomas Kosiewski --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ Change-Id: Id83a2bc2b262042bfce91b4e712d60f1ec7094df --- .../controller/coderaiprovider_controller.go | 19 +- .../coderaiprovider_controller_test.go | 4 +- internal/controller/coderaiprovider_create.go | 86 ++-- .../controller/coderaiprovider_create_test.go | 134 ++---- internal/controller/coderaiprovider_sync.go | 178 ++++++++ .../controller/coderaiprovider_sync_test.go | 380 ++++++++++++++++++ internal/controller/fakecoder_test.go | 5 + 7 files changed, 648 insertions(+), 158 deletions(-) create mode 100644 internal/controller/coderaiprovider_sync.go create mode 100644 internal/controller/coderaiprovider_sync_test.go diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index 995418a8a..48e86cf72 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -6,6 +6,7 @@ import ( "fmt" "net/url" "strings" + "sync" "time" corev1 "k8s.io/api/core/v1" @@ -44,6 +45,8 @@ const ( type CoderAIProviderReconciler struct { client.Client Scheme *runtime.Scheme + + sentKeys sync.Map // types.UID -> SHA-256 of the key that Coder holds. Memory only. } // aiProviderStep is the outcome of one reconcile. A ready step reports that @@ -201,22 +204,28 @@ func (r *CoderAIProviderReconciler) check( return aiProviderWait(aiProviderRetry, wait.reason, "%s", wait.message), nil } var step *aiProviderStep - if provider.Status.ProviderID == "" { + recreate := provider.Status.ProviderID == "" + if !recreate { + if step, recreate, err = r.syncAIProvider(ctx, sdk, provider, key, before); err != nil { + return nil, err + } + } + if recreate { if step, err = r.createOrConfirm(ctx, sdk, provider, key, before); err != nil { return nil, err } if step == nil { - // Store the ID before the apply replaces the nonce: after that, + // Store the ID before the sync replaces the nonce: after that, // only the stored ID proves that this object created the provider. if err := r.writeStatus(ctx, provider, before); err != nil { return nil, err } *before = *provider.Status.DeepCopy() + if step, _, err = r.syncAIProvider(ctx, sdk, provider, key, before); err != nil { + return nil, err + } } } - if step == nil { - step = applyAIProvider(ctx, sdk, provider, key) - } // Coder messages can echo the request, so scrub the key from them. step.message = credentialsecret.Redact(step.message, key) return step, nil diff --git a/internal/controller/coderaiprovider_controller_test.go b/internal/controller/coderaiprovider_controller_test.go index ecc8000fa..4923879bb 100644 --- a/internal/controller/coderaiprovider_controller_test.go +++ b/internal/controller/coderaiprovider_controller_test.go @@ -200,7 +200,7 @@ func TestCoderAIProviderCredentialChecks(t *testing.T) { got, result := env.reconcile(ctx, t, provider) if tc.reason == "" { requireAIProviderReady(t, got) - require.Equal(t, 1, env.fake.requestCount(routeAIProvider), "a usable key reaches the name lookup") + require.Equal(t, 2, env.fake.requestCount(routeAIProvider), "a usable key reaches the name lookup, then the read by ID") require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) require.Equal(t, 10*time.Minute, result.RequeueAfter) return @@ -361,7 +361,7 @@ func TestCoderAIProviderWatches(t *testing.T) { require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got)) condition := meta.FindStatusCondition(got.Status.Conditions, "Ready") return condition != nil && condition.Reason == reason - }, 30*time.Second, 100*time.Millisecond, reason) + }, 60*time.Second, 100*time.Millisecond, reason) } requireReason("ControlPlaneNotReady") diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index e458b9b92..6cd71b660 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -1,9 +1,7 @@ package controller import ( - "cmp" "context" - "errors" "fmt" "net/http" "slices" @@ -88,8 +86,14 @@ func recordAIProvider(provider *coderv1alpha1.CoderAIProvider, found codersdk.AI "Coder already has a provider named %s (ID %s) that this resource did not create. The controller never changes it. Choose another name, or delete that provider in Coder.", provider.Name, found.ID), nil } - if found.ID == uuid.Nil || provider.Status.Credential == nil { - return nil, fmt.Errorf("assertion failed: AI provider %s/%s has a create marker without a credential, or Coder answered without an ID", provider.Namespace, provider.Name) + if found.ID == uuid.Nil { + return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", "Coder answered provider %s without an ID.", provider.Name), nil + } + if provider.Status.Credential == nil { + // A create of this object committed after its marker was cleared, so + // the key it sent is not known. An empty change token makes the sync + // step send the current key. + provider.Status.Credential = &coderv1alpha1.CoderAIProviderCredentialStatus{} } if len(found.APIKeys) != 1 || found.APIKeys[0].ID == uuid.Nil { return aiProviderStalled(0, "KeyChangedExternally", @@ -134,69 +138,45 @@ func (r *CoderAIProviderReconciler) createAIProvider( } return step, err } + r.rememberSentKey(provider, key) return nil, nil - case createHadNoEffect(err) || status == http.StatusForbidden: - // Coder answers 403 from its authorization check before the insert - // commits (coderd/ai_providers.go:233-237), so it had no effect. + case status == 0 && createHadNoEffect(err): + // A failed dial or TLS handshake never sent the request. provider.Status.CreateAttemptTime, provider.Status.Credential = nil, nil return aiProviderWait(aiProviderRetry, "CreateRetrying", "The create request had no effect: %s", coderErrorSummary(err)), nil - case status == http.StatusConflict: - // Someone created the name after the lookup. Read it: the nonce - // proves whether an earlier create of this object committed. - found, err := sdk.AIProvider(ctx, provider.Name) - if err != nil { - return aiProviderWait(aiProviderConfirmPoll, "ConfirmingCreate", "get provider %s after a conflict: %s", provider.Name, coderErrorSummary(err)), nil - } - return recordAIProvider(provider, found) case status >= 400 && status < 500: - provider.Status.CreateAttemptTime, provider.Status.Credential = nil, nil - return aiProviderStalled(0, "CoderRejected", "Coder rejected the create request: %s", coderErrorSummary(err)), nil + return r.confirmRejectedCreate(ctx, sdk, provider, 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 } } -// applyAIProvider sets the display name, icon and enabled flag of the -// provider that this object created, once per generation. A changed base URL -// or key needs a later controller version, so it does not count as applied. -func applyAIProvider(ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, key credentialsecret.Value) *aiProviderStep { - // A Secret change does not change the generation, so check the key - // before the ready shortcut. - if !sameAIProviderKey(provider.Status.Credential, key.Token) { - return aiProviderWait(aiProviderResync, "Pending", "The key Secret changed after the create. This controller version does not send a changed key.") - } - if provider.Status.AppliedGeneration == provider.Generation { - return aiProviderReady(provider) - } - enabled := provider.Spec.Enabled == nil || *provider.Spec.Enabled - updated, err := sdk.UpdateAIProvider(ctx, provider.Status.ProviderID, codersdk.UpdateAIProviderRequest{ - DisplayName: &provider.Spec.DisplayName, Icon: &provider.Spec.Icon, Enabled: &enabled, - }) - status := coderStatus(err) +// 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). +func (r *CoderAIProviderReconciler) confirmRejectedCreate( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, status int, createErr error, +) (*aiProviderStep, error) { + found, err := sdk.AIProvider(ctx, provider.Name) switch { - case status == http.StatusNotFound: - return aiProviderStalled(0, "DeletedExternally", "Provider %s (ID %s) is gone from Coder. The controller does not create it again until the spec changes.", provider.Name, provider.Status.ProviderID) - case isCoderAuthFailure(status): - // No Kubernetes event follows a repaired token or role, so retry. - return aiProviderWait(aiProviderRetry, "CoderUnavailable", "update provider %s: %s", provider.Name, coderErrorSummary(err)) - case status >= 400 && status < 500 && status != http.StatusTooManyRequests: - return aiProviderStalled(0, "CoderRejected", "Coder rejected the update: %s", coderErrorSummary(err)) - case err != nil: - return aiProviderWait(aiProviderRetry, "CoderUnavailable", "update provider %s: %s", provider.Name, coderErrorSummary(err)) - } - if err := errors.Join(coderAnswerFor("AI provider", provider.Status.ProviderID, updated.ID.String()), coderAnswerFor("AI provider", provider.Name, updated.Name)); err != nil { - return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", "%s", err.Error()) + case err == nil: + // Ours (the create committed) or someone else's (NameConflict). + return recordAIProvider(provider, found) + case !isCoderNotFound(err): + return aiProviderWait(aiProviderRetry, "ConfirmingCreate", "get provider %s after a %d answer: %s", provider.Name, status, coderErrorSummary(err)), nil } - if updated.BaseURL != provider.Spec.BaseURL || string(updated.Type) != provider.Spec.Type { - return aiProviderWait(aiProviderResync, "Pending", "The base URL changed after the create. This controller version does not send a changed base URL.") + if step := requireSiteOwner(ctx, sdk); step != nil { + return step, nil } - // Coder shows the name for an empty display name (db2sdk.go:52-55). - if updated.DisplayName != cmp.Or(provider.Spec.DisplayName, provider.Name) || updated.Icon != provider.Spec.Icon || updated.Enabled != enabled { - return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", "Coder answered the update of provider %s without the requested display name, icon or enabled flag.", provider.Name) + provider.Status.CreateAttemptTime, provider.Status.Credential = nil, nil + switch status { + case http.StatusUnauthorized, http.StatusForbidden, http.StatusConflict, http.StatusTooManyRequests: + return aiProviderWait(aiProviderRetry, "CreateRetrying", "The create request had no effect: %s", coderErrorSummary(createErr)), nil + default: + return aiProviderStalled(0, "CoderRejected", "Coder rejected the create request: %s", coderErrorSummary(createErr)), nil } - provider.Status.AppliedGeneration = provider.Generation - return aiProviderReady(provider) } // isCoderAuthFailure reports 401 and 403. An expired operator token or a diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index ea14a7798..b7cefe271 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -14,8 +14,13 @@ import ( coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/controller" + "github.com/coder/coder-k8s/internal/credentialsecret/credentialsecrettest" ) +// hangTimeout ends a reconcile whose Coder request hangs. It leaves room for +// the requests before the hang on a loaded test host. +const hangTimeout = 10 * time.Second + // newAIProviderCreateEnv returns a namespace with a ready control plane, a // consented key Secret and a provider object named openai. func newAIProviderCreateEnv(ctx context.Context, t *testing.T) (aiProviderEnv, *corev1.Secret, *coderv1alpha1.CoderAIProvider) { @@ -67,12 +72,13 @@ func TestCoderAIProviderCreate(t *testing.T) { require.True(t, stored.enabled) require.Equal(t, "OpenAI", stored.displayName) require.Equal(t, "/icon/openai.svg", stored.icon) - require.Equal(t, testAIProviderKey, stored.keys[0].plaintext, "the apply step does not touch the key") + require.Equal(t, testAIProviderKey, stored.keys[0].plaintext, "the sync step does not resend an unchanged key") + require.Equal(t, got.Status.Credential.KeyID, stored.keys[0].id.String()) - // A ready provider needs no Coder call and no status write. + // A ready provider costs one read and no write or status update. before := env.fake.totalRequests() again, _ := env.reconcile(ctx, t, got) - require.Equal(t, before, env.fake.totalRequests()) + require.Equal(t, before+1, env.fake.totalRequests()) require.Equal(t, got.ResourceVersion, again.ResourceVersion) require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) require.Equal(t, 2, env.fake.requestCount(routeUpdateAIProvider)) @@ -157,13 +163,14 @@ func TestCoderAIProviderCreateHangs(t *testing.T) { env, _, provider := newAIProviderCreateEnv(ctx, t) env.fake.failNext(routeCreateAIProvider, fakeFault{Hang: true, AfterCommit: tc.committed}) - short, cancel := context.WithTimeout(ctx, 2*time.Second) + short, cancel := context.WithTimeout(ctx, hangTimeout) defer cancel() _, err := env.r.Reconcile(short, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) require.Error(t, err, "the status write after the hang fails with the expired context") stored := &coderv1alpha1.CoderAIProvider{} require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), stored)) require.NotNil(t, stored.Status.CreateAttemptTime, "the marker was written before the request") + credentialsecrettest.RequireNoLeak(t, []string{testAIProviderKey}, map[string]any{"error": err, "status": stored.Status}) env.restart() got, _ := env.reconcile(ctx, t, stored) @@ -185,11 +192,13 @@ func TestCoderAIProviderCrashAfterApply(t *testing.T) { ctx := context.Background() env, _, provider := newAIProviderCreateEnv(ctx, t) env.fake.failNext(routeUpdateAIProvider, fakeFault{Hang: true, AfterCommit: true}) - short, cancel := context.WithTimeout(ctx, 2*time.Second) + short, cancel := context.WithTimeout(ctx, hangTimeout) defer cancel() _, err := env.r.Reconcile(short, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) require.Error(t, err, "the status write after the hang fails with the expired context") - stored, _ := env.fake.aiProvider("openai") + credentialsecrettest.RequireNoLeak(t, []string{testAIProviderKey}, map[string]any{"error": err}) + stored, ok := env.fake.aiProvider("openai") + require.True(t, ok, "the create committed") require.Equal(t, "OpenAI", stored.displayName, "the apply committed and replaced the nonce") got := &coderv1alpha1.CoderAIProvider{} require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got)) @@ -202,6 +211,27 @@ func TestCoderAIProviderCrashAfterApply(t *testing.T) { require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) } +// #253 review round 3: a 4xx that replaces a committed create answer, which +// Coder itself never sends after a commit, is confirmed by a read. The +// object then owns the provider instead of failing an assertion. +func TestCoderAIProviderCreateRejectedAfterCommit(t *testing.T) { + t.Parallel() + for _, status := range []int{http.StatusNotAcceptable, http.StatusBadRequest} { + t.Run(http.StatusText(status), func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeCreateAIProvider, fakeFault{AfterCommit: true, Status: status, Detail: "echo " + testAIProviderKey}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) + require.Equal(t, 1, env.fake.aiProviderCount()) + stored, _ := env.fake.aiProvider("openai") + require.Equal(t, stored.id.String(), got.Status.ProviderID) + }) + } +} + // Plan S4: a provider that someone else creates between the lookup and the // create gives NameConflict, and the controller never changes it. func TestCoderAIProviderCreateConflict(t *testing.T) { @@ -275,95 +305,3 @@ func TestCoderAIProviderConfirmNeedsSiteOwner(t *testing.T) { requireAIProviderReady(t, got) require.Equal(t, 2, env.fake.requestCount(routeCreateAIProvider)) } - -// The apply step only sets display name, icon and enabled. A changed key or -// a provider deleted in Coder does not count as applied. -func TestCoderAIProviderApplyGuards(t *testing.T) { - t.Parallel() - t.Run("key Secret changed after the create", func(t *testing.T) { - t.Parallel() - ctx := context.Background() - env, secret, provider := newAIProviderCreateEnv(ctx, t) - env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusInternalServerError}) - got, _ := env.reconcile(ctx, t, provider) - require.NotEmpty(t, got.Status.ProviderID) - - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) - secret.Data["api-key"] = []byte(testAIProviderKey + "-rotated") - require.NoError(t, k8sClient.Update(ctx, secret)) - got, _ = env.reconcile(ctx, t, got) - require.Contains(t, requireAIProviderStep(t, got, false, "Pending"), "key Secret changed") - require.Equal(t, 1, env.fake.requestCount(routeUpdateAIProvider), "no update with a changed key") - stored, _ := env.fake.aiProvider("openai") - require.Equal(t, testAIProviderKey, stored.keys[0].plaintext) - }) - t.Run("base URL changed after the create", func(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) - require.NotEmpty(t, got.Status.ProviderID) - - got.Spec.BaseURL = "https://api.openai.com/v2" // Same origin, so the key is still allowed. - require.NoError(t, k8sClient.Update(ctx, got)) - got, _ = env.reconcile(ctx, t, got) - require.Contains(t, requireAIProviderStep(t, got, false, "Pending"), "base URL changed") - require.Zero(t, got.Status.AppliedGeneration) - stored, _ := env.fake.aiProvider("openai") - require.Equal(t, "https://api.openai.com/v1", stored.baseURL) - }) - t.Run("update answer without the requested values", func(t *testing.T) { - t.Parallel() - ctx := context.Background() - env, _, provider := newAIProviderCreateEnv(ctx, t) - env.fake.failNext(routeUpdateAIProvider, fakeFault{Rewrite: func(answer any) any { - p := answer.(codersdk.AIProvider) - p.Enabled = false - return p - }}) - got, _ := env.reconcile(ctx, t, provider) - requireAIProviderStep(t, got, true, "CoderAnswerMismatch") - require.Zero(t, got.Status.AppliedGeneration) - }) - t.Run("key Secret rotated after Ready", func(t *testing.T) { - t.Parallel() - ctx := context.Background() - env, secret, provider := newAIProviderCreateEnv(ctx, t) - got, _ := env.reconcile(ctx, t, provider) - requireAIProviderReady(t, got) - - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) - secret.Data["api-key"] = []byte(testAIProviderKey + "-rotated") - require.NoError(t, k8sClient.Update(ctx, secret)) - got, _ = env.reconcile(ctx, t, got) - require.Contains(t, requireAIProviderStep(t, got, false, "Pending"), "key Secret changed") - require.Equal(t, got.Generation, got.Status.AppliedGeneration, "the spec is still applied") - }) - for _, status := range []int{http.StatusUnauthorized, http.StatusForbidden} { - t.Run(http.StatusText(status)+" on the apply is retried", func(t *testing.T) { - t.Parallel() - ctx := context.Background() - env, _, provider := newAIProviderCreateEnv(ctx, t) - env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: status}) - got, result := env.reconcile(ctx, t, provider) - requireAIProviderStep(t, got, false, "CoderUnavailable") - require.Equal(t, 30*time.Second, result.RequeueAfter) - got, _ = env.reconcile(ctx, t, got) - requireAIProviderReady(t, got) - }) - } - t.Run("deleted in Coder before the apply", func(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) - require.NoError(t, env.fake.client(t, 2*time.Second).DeleteAIProvider(ctx, got.Status.ProviderID)) - - got, _ = env.reconcile(ctx, t, got) - requireAIProviderStep(t, got, true, "DeletedExternally") - require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "no new create") - require.Zero(t, got.Status.AppliedGeneration) - }) -} diff --git a/internal/controller/coderaiprovider_sync.go b/internal/controller/coderaiprovider_sync.go new file mode 100644 index 000000000..c989ecd86 --- /dev/null +++ b/internal/controller/coderaiprovider_sync.go @@ -0,0 +1,178 @@ +package controller + +import ( + "cmp" + "context" + "crypto/sha256" + "errors" + "fmt" + "net/http" + "time" + + "github.com/coder/coder/v2/codersdk" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +// syncAIProvider reads the provider that this object created and sends one +// sparse PATCH with every field that differs from the spec. The base URL and +// the key always travel in the same PATCH, so Coder never pairs a key with a +// URL that its Secret does not allow, and never enables a provider at a stale +// URL. After a full apply, a dashboard delete, disable or key change is a +// kill switch: the controller reports it and waits for a spec change (D11). +// +// recreate is true when the provider is gone before a full apply of this +// generation: the caller then creates it again, if the name is free. +func (r *CoderAIProviderReconciler) syncAIProvider( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, key credentialsecret.Value, before *coderv1alpha1.CoderAIProviderStatus, +) (step *aiProviderStep, recreate bool, err error) { + cred := provider.Status.Credential + if cred == nil { + return nil, false, fmt.Errorf("assertion failed: AI provider %s/%s has a provider ID without a credential", provider.Namespace, provider.Name) + } + applied := provider.Status.AppliedGeneration == provider.Generation + found, err := sdk.AIProvider(ctx, provider.Status.ProviderID) + switch { + case isCoderNotFound(err): + return r.providerGone(provider, applied) + case err != nil: + return aiProviderWait(aiProviderRetry, "CoderUnavailable", "get provider %s: %s", provider.Name, coderErrorSummary(err)), false, nil + } + if step := checkAIProviderAnswer(provider, found); step != nil { + return step, false, nil + } + + enabled := provider.Spec.Enabled == nil || *provider.Spec.Enabled + keys := found.APIKeys + ownKey := len(keys) == 1 && keys[0].ID.String() == cred.KeyID + resending := cred.ResendAttemptTime != nil + if applied && !resending { + switch { + case enabled && !found.Enabled: + return aiProviderStalled(aiProviderResync, "DisabledExternally", + "Provider %s was disabled in Coder. The controller does not enable it again until the spec changes.", provider.Name), false, nil + case !ownKey: + return aiProviderStalled(aiProviderResync, "KeyChangedExternally", + "The key of provider %s was changed in Coder. The controller does not send its key again until the spec changes.", provider.Name), false, nil + } + } + + // A spec edit re-arms the object, so it also replaces a key that Coder + // holds but this object did not send. + sendKey := resending || !ownKey + if !sendKey && !sameAIProviderKey(cred, key.Token) { + if r.sentKeyMatches(provider, key) { + // Only the Secret's metadata changed: Coder holds this value. + setAIProviderToken(cred, key.Token) + } else { + sendKey = true + } + } + req := codersdk.UpdateAIProviderRequest{} + if found.DisplayName != cmp.Or(provider.Spec.DisplayName, provider.Name) { + req.DisplayName = &provider.Spec.DisplayName + } + if found.Icon != provider.Spec.Icon { + req.Icon = &provider.Spec.Icon + } + if found.Enabled != enabled { + req.Enabled = &enabled + } + if found.BaseURL != provider.Spec.BaseURL { + req.BaseURL = &provider.Spec.BaseURL + } + if sendKey { + value := key.Reveal() + req.APIKeys = &[]codersdk.AIProviderKeyMutation{{APIKey: &value}} + } + if req.IsEmpty() { + provider.Status.AppliedGeneration = provider.Generation + return aiProviderReady(provider), false, nil + } + if sendKey && !resending { + // The marker goes first. A crash after the PATCH leaves it set, and + // the next reconcile sends the key once more (plan 5.3). + cred.ResendAttemptTime = &metav1.Time{Time: time.Now()} + if err := r.writeStatus(ctx, provider, before); err != nil { + return nil, false, err + } + *before = *provider.Status.DeepCopy() + // The status update decodes the stored object into provider. + cred = provider.Status.Credential + } + + updated, err := sdk.UpdateAIProvider(ctx, provider.Status.ProviderID, req) + status := coderStatus(err) + switch { + case status == http.StatusNotFound: + return r.providerGone(provider, applied) + case isCoderAuthFailure(status) || status == http.StatusTooManyRequests: + // No Kubernetes event follows a repaired token or role, so retry. + return aiProviderWait(aiProviderRetry, "CoderUnavailable", "update provider %s: %s", provider.Name, coderErrorSummary(err)), false, nil + case status >= 400 && status < 500: + return aiProviderStalled(0, "CoderRejected", "Coder rejected the update: %s", coderErrorSummary(err)), false, nil + case err != nil: + return aiProviderWait(aiProviderRetry, "CoderUnavailable", "update provider %s: %s", provider.Name, coderErrorSummary(err)), false, nil + } + if step := checkAIProviderAnswer(provider, updated); step != nil { + return step, false, nil + } + if updated.DisplayName != cmp.Or(provider.Spec.DisplayName, provider.Name) || updated.Icon != provider.Spec.Icon || + updated.Enabled != enabled || updated.BaseURL != provider.Spec.BaseURL { + return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", + "Coder answered the update of provider %s without the requested display name, icon, enabled flag or base URL.", provider.Name), false, nil + } + if sendKey { + if len(updated.APIKeys) != 1 || updated.APIKeys[0].ID.String() == cred.KeyID { + // The marker stays, so the next reconcile sends the key again. + return aiProviderWait(aiProviderRetry, "CoderAnswerMismatch", + "Coder answered the key update of provider %s without exactly one new key. The controller sends the key again.", provider.Name), false, nil + } + cred.KeyID, cred.ResendAttemptTime = updated.APIKeys[0].ID.String(), nil + setAIProviderToken(cred, key.Token) + r.rememberSentKey(provider, key) + } + provider.Status.AppliedGeneration = provider.Generation + return aiProviderReady(provider), false, nil +} + +// providerGone handles a provider that is missing in Coder. After a full +// apply it is a kill switch. Before one, the object creates it again. +func (r *CoderAIProviderReconciler) providerGone(provider *coderv1alpha1.CoderAIProvider, applied bool) (*aiProviderStep, bool, error) { + if applied { + return aiProviderStalled(aiProviderResync, "DeletedExternally", + "Provider %s (ID %s) was deleted in Coder. The controller does not create it again until the spec changes.", provider.Name, provider.Status.ProviderID), false, nil + } + provider.Status.ProviderID, provider.Status.Credential = "", nil + r.sentKeys.Delete(provider.UID) + return nil, true, nil +} + +// checkAIProviderAnswer checks that Coder answered for the provider that +// this object created, with the type it cannot change. +func checkAIProviderAnswer(provider *coderv1alpha1.CoderAIProvider, answer codersdk.AIProvider) *aiProviderStep { + if err := errors.Join(coderAnswerFor("AI provider", provider.Status.ProviderID, answer.ID.String()), + coderAnswerFor("AI provider", provider.Name, answer.Name), + coderAnswerFor("AI provider type", provider.Spec.Type, string(answer.Type))); err != nil { + return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", "%s", err.Error()) + } + return nil +} + +func setAIProviderToken(cred *coderv1alpha1.CoderAIProviderCredentialStatus, token credentialsecret.ChangeToken) { + cred.SecretName, cred.SecretKey, cred.SecretUID, cred.SecretResourceVersion = token.SecretName, token.SecretKey, string(token.SecretUID), token.ResourceVersion +} + +// rememberSentKey keeps a SHA-256 of the value that Coder now holds, in +// process memory only (plan 5.3, D1). It lets a metadata-only Secret change +// skip a resend. After a restart the map is empty, so one resend can happen. +func (r *CoderAIProviderReconciler) rememberSentKey(provider *coderv1alpha1.CoderAIProvider, key credentialsecret.Value) { + r.sentKeys.Store(provider.UID, sha256.Sum256([]byte(key.Reveal()))) +} + +func (r *CoderAIProviderReconciler) sentKeyMatches(provider *coderv1alpha1.CoderAIProvider, key credentialsecret.Value) bool { + sum, ok := r.sentKeys.Load(provider.UID) + return ok && sum.([sha256.Size]byte) == sha256.Sum256([]byte(key.Reveal())) +} diff --git a/internal/controller/coderaiprovider_sync_test.go b/internal/controller/coderaiprovider_sync_test.go new file mode 100644 index 000000000..15b56cab4 --- /dev/null +++ b/internal/controller/coderaiprovider_sync_test.go @@ -0,0 +1,380 @@ +package controller_test + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret/credentialsecrettest" +) + +const testAIProviderKeyRotated = "sk-ai-provider-rotated-4c8e2b6f" // #nosec G101 -- test fixture, not a credential. + +// newReadyAIProvider returns a provider that reached Ready. +func newReadyAIProvider(ctx context.Context, t *testing.T) (aiProviderEnv, *corev1.Secret, *coderv1alpha1.CoderAIProvider) { + t.Helper() + env, secret, provider := newAIProviderCreateEnv(ctx, t) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + return env, secret, got +} + +func (env aiProviderEnv) writes() int { + return env.fake.requestCount(routeCreateAIProvider) + env.fake.requestCount(routeUpdateAIProvider) + env.fake.requestCount(routeDeleteAIProvider) +} + +func setSecretValue(ctx context.Context, t *testing.T, secret *corev1.Secret, value string) { + t.Helper() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Data["api-key"] = []byte(value) + require.NoError(t, k8sClient.Update(ctx, secret)) +} + +func editSpec(ctx context.Context, t *testing.T, provider *coderv1alpha1.CoderAIProvider, edit func(*coderv1alpha1.CoderAIProviderSpec)) *coderv1alpha1.CoderAIProvider { + t.Helper() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), provider)) + edit(&provider.Spec) + require.NoError(t, k8sClient.Update(ctx, provider)) + return provider +} + +// Plan S13: a reconcile without a spec, Secret or Coder difference sends no +// write, also for an empty display name, which Coder shows as the name. +func TestCoderAIProviderSyncNoDifference(t *testing.T) { + t.Parallel() + for _, displayName := range []string{"OpenAI", ""} { + t.Run("display name "+displayName, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + provider = editSpec(ctx, t, provider, func(s *coderv1alpha1.CoderAIProviderSpec) { s.DisplayName = displayName }) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + writes := env.writes() + for range 3 { + env.restart() + again, _ := env.reconcile(ctx, t, got) + require.Equal(t, got.ResourceVersion, again.ResourceVersion) + } + require.Equal(t, writes, env.writes()) + }) + } +} + +// Dashboard edits of the display name and icon are drift, not kill switches: +// one sparse PATCH restores them and keeps the key. +func TestCoderAIProviderSyncDrift(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + _, err := env.fake.client(t, 2*time.Second).UpdateAIProvider(ctx, provider.Status.ProviderID, codersdk.UpdateAIProviderRequest{ + DisplayName: ptrTo("Renamed"), Icon: ptrTo("/icon/other.svg"), + }) + require.NoError(t, err) + writes := env.writes() + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + require.Equal(t, writes+1, env.writes()) + stored, _ := env.fake.aiProvider("openai") + require.Equal(t, "OpenAI", stored.displayName) + require.Equal(t, "/icon/openai.svg", stored.icon) + require.Equal(t, provider.Status.Credential.KeyID, stored.keys[0].id.String(), "the key was not sent again") +} + +// A Secret value change gives exactly one PATCH and a new change token. A +// metadata-only change gives none, until a restart empties the in-memory +// hash. +func TestCoderAIProviderKeyRotation(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + + setSecretValue(ctx, t, secret, testAIProviderKeyRotated) + writes := env.writes() + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + require.Equal(t, writes+1, env.writes()) + stored, _ := env.fake.aiProvider("openai") + require.Len(t, stored.keys, 1) + require.Equal(t, testAIProviderKeyRotated, stored.keys[0].plaintext) + require.Equal(t, stored.keys[0].id.String(), got.Status.Credential.KeyID) + require.NotEqual(t, provider.Status.Credential.KeyID, got.Status.Credential.KeyID) + require.Equal(t, secret.ResourceVersion, got.Status.Credential.SecretResourceVersion) + require.Nil(t, got.Status.Credential.ResendAttemptTime) + credentialsecrettest.RequireNoLeak(t, []string{testAIProviderKey, testAIProviderKeyRotated}, map[string]any{"status": got.Status}) + + // A label change only: the token moves, Coder is not written. + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Labels["team"] = "ml" + require.NoError(t, k8sClient.Update(ctx, secret)) + writes = env.writes() + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, writes, env.writes()) + require.Equal(t, secret.ResourceVersion, got.Status.Credential.SecretResourceVersion) + + // After a restart the hash is gone, so one metadata-only change resends. + env.restart() + secret.Labels["team"] = "research" + require.NoError(t, k8sClient.Update(ctx, secret)) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, writes+1, env.writes()) + got, _ = env.reconcile(ctx, t, got) + require.Equal(t, writes+1, env.writes(), "the resend refills the hash") + + // Another key in the same Secret changes neither UID nor resourceVersion + // of the reference, but it is another value. + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Data["api-key-v2"] = []byte(testAIProviderKey + "-v2") + require.NoError(t, k8sClient.Update(ctx, secret)) + got = editSpec(ctx, t, got, func(s *coderv1alpha1.CoderAIProviderSpec) { s.APIKeySecretRef.Key = "api-key-v2" }) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + stored, _ = env.fake.aiProvider("openai") + require.Equal(t, testAIProviderKey+"-v2", stored.keys[0].plaintext) + require.Equal(t, "api-key-v2", got.Status.Credential.SecretKey) +} + +// #253 review round 3: a provider whose first apply failed is never enabled +// at its old base URL. URL and enabled flag travel in one PATCH. +func TestCoderAIProviderBaseURLChange(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") + + got = editSpec(ctx, t, got, func(s *coderv1alpha1.CoderAIProviderSpec) { s.BaseURL = "https://api.openai.com/v2" }) + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusInternalServerError}) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderStep(t, got, false, "CoderUnavailable") + stored, _ := env.fake.aiProvider("openai") + require.False(t, stored.enabled, "a failed PATCH leaves the provider disabled") + require.Equal(t, "https://api.openai.com/v1", stored.baseURL) + + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + stored, _ = env.fake.aiProvider("openai") + require.True(t, stored.enabled) + require.Equal(t, "https://api.openai.com/v2", stored.baseURL) + + // A later URL change keeps the key. + keyID := got.Status.Credential.KeyID + got = editSpec(ctx, t, got, func(s *coderv1alpha1.CoderAIProviderSpec) { s.BaseURL = "https://api.openai.com/v3" }) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + stored, _ = env.fake.aiProvider("openai") + require.Equal(t, "https://api.openai.com/v3", stored.baseURL) + require.Equal(t, keyID, stored.keys[0].id.String()) +} + +// Plan S12, combined edit: a new key Secret and a new origin go in one PATCH, +// so Coder never pairs the old key with the new URL. +func TestCoderAIProviderKeyAndURLTogether(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + other := env.keySecret(consentedObjectMeta("", "", "https://llm.example.com"), testAIProviderKeyRotated) + other.Name = "llm-api-key" + createSecret(ctx, t, other) + got := editSpec(ctx, t, provider, func(s *coderv1alpha1.CoderAIProviderSpec) { + s.BaseURL, s.APIKeySecretRef = "https://llm.example.com/v1", coderv1alpha1.SecretKeySelector{Name: "llm-api-key", Key: "api-key"} + }) + writes := env.writes() + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, writes+1, env.writes(), "one PATCH") + stored, _ := env.fake.aiProvider("openai") + require.Equal(t, "https://llm.example.com/v1", stored.baseURL) + require.Equal(t, testAIProviderKeyRotated, stored.keys[0].plaintext) + require.Equal(t, "llm-api-key", got.Status.Credential.SecretName) +} + +// Plan 5.3 rule 5: a crash after the key PATCH leaves the resend marker, and +// the next reconcile sends the key once more. Coder ends with that key only. +func TestCoderAIProviderResendCrash(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + setSecretValue(ctx, t, secret, testAIProviderKeyRotated) + updates := env.fake.requestCount(routeUpdateAIProvider) + env.fake.failNext(routeUpdateAIProvider, fakeFault{Hang: true, AfterCommit: true}) + short, cancel := context.WithTimeout(ctx, hangTimeout) + defer cancel() + _, err := env.r.Reconcile(short, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.Error(t, err) + stored := &coderv1alpha1.CoderAIProvider{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), stored)) + require.NotNil(t, stored.Status.Credential.ResendAttemptTime, "the marker was written before the PATCH") + credentialsecrettest.RequireNoLeak(t, []string{testAIProviderKey, testAIProviderKeyRotated}, map[string]any{"error": err, "status": stored.Status}) + + env.restart() + got, _ := env.reconcile(ctx, t, stored) + requireAIProviderReady(t, got) + require.Nil(t, got.Status.Credential.ResendAttemptTime) + require.Equal(t, updates+2, env.fake.requestCount(routeUpdateAIProvider), "the key was sent twice") + coder, _ := env.fake.aiProvider("openai") + require.Len(t, coder.keys, 1) + require.Equal(t, testAIProviderKeyRotated, coder.keys[0].plaintext) + require.Equal(t, coder.keys[0].id.String(), got.Status.Credential.KeyID) +} + +// Plan S14: a dashboard delete, disable or key change after a full apply is a +// kill switch: Stalled, and zero writes until a spec edit re-arms the object. +func TestCoderAIProviderKillSwitches(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + reason string + act func(ctx context.Context, c *codersdk.Client, id string) error + check func(t *testing.T, before coderv1alpha1.CoderAIProviderStatus, after fakeAIProvider) + }{ + {reason: "DeletedExternally", act: func(ctx context.Context, c *codersdk.Client, id string) error { + return c.DeleteAIProvider(ctx, id) + }, check: func(t *testing.T, before coderv1alpha1.CoderAIProviderStatus, after fakeAIProvider) { + require.NotEqual(t, before.ProviderID, after.id.String(), "a spec edit creates the provider again") + }}, + {reason: "DisabledExternally", act: func(ctx context.Context, c *codersdk.Client, id string) error { + _, err := c.UpdateAIProvider(ctx, id, codersdk.UpdateAIProviderRequest{Enabled: ptrTo(false)}) + return err + }, check: func(t *testing.T, _ coderv1alpha1.CoderAIProviderStatus, after fakeAIProvider) { + require.True(t, after.enabled) + }}, + {reason: "KeyChangedExternally", act: func(ctx context.Context, c *codersdk.Client, id string) error { + _, err := c.UpdateAIProvider(ctx, id, codersdk.UpdateAIProviderRequest{ + APIKeys: &[]codersdk.AIProviderKeyMutation{{APIKey: ptrTo("sk-dashboard-0123456789abcdef")}}, + }) + return err + }, check: func(t *testing.T, _ coderv1alpha1.CoderAIProviderStatus, after fakeAIProvider) { + require.Len(t, after.keys, 1) + require.Equal(t, testAIProviderKey, after.keys[0].plaintext, "a spec edit sends the key again") + }}, + } { + t.Run(tc.reason, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + require.NoError(t, tc.act(ctx, env.fake.client(t, 2*time.Second), provider.Status.ProviderID)) + writes := env.writes() + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, true, tc.reason) + for range 2 { + env.restart() + got, _ = env.reconcile(ctx, t, got) + requireAIProviderStep(t, got, true, tc.reason) + } + require.Equal(t, writes, env.writes(), "a kill switch sends nothing") + + // A spec edit whose apply fails does not count as applied, so the + // next reconcile applies again instead of reporting a kill switch. + got = editSpec(ctx, t, got, func(s *coderv1alpha1.CoderAIProviderSpec) { s.DisplayName = "OpenAI v2" }) + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusBadGateway}) + env.fake.failNext(routeCreateAIProvider, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + require.NotEqual(t, got.Generation, got.Status.AppliedGeneration) + for range 3 { + if got, _ = env.reconcile(ctx, t, got); got.Status.AppliedGeneration == got.Generation { + break + } + } + requireAIProviderReady(t, got) + stored, _ := env.fake.aiProvider("openai") + require.Equal(t, "OpenAI v2", stored.displayName) + tc.check(t, provider.Status, stored) + }) + } +} + +// Plan S3 end to end: Coder echoes the key in Message, Detail and +// validations of failed writes. The key appears in no status, log or error. +func TestCoderAIProviderErrorsHoldNoKey(t *testing.T) { + t.Parallel() + echo := fakeFault{Status: http.StatusBadRequest, Message: "bad key " + testAIProviderKeyRotated, Detail: "key " + testAIProviderKeyRotated} + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + setSecretValue(ctx, t, secret, testAIProviderKeyRotated) + env.fake.failNext(routeUpdateAIProvider, echo) + got, _ := env.reconcile(ctx, t, provider) + require.Contains(t, requireAIProviderStep(t, got, true, "CoderRejected"), "[REDACTED]") + + other, _, fresh := newAIProviderCreateEnv(ctx, t) + echo.Message, echo.Detail = "bad key "+testAIProviderKey, "key "+testAIProviderKey + other.fake.failNext(routeCreateAIProvider, echo) + got, _ = other.reconcile(ctx, t, fresh) + require.Contains(t, requireAIProviderStep(t, got, true, "CoderRejected"), "[REDACTED]") +} + +// The 401, 403 and answer checks of the sync PATCH. +func TestCoderAIProviderSyncAnswers(t *testing.T) { + t.Parallel() + for _, status := range []int{http.StatusUnauthorized, http.StatusForbidden} { + t.Run(http.StatusText(status)+" is retried", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: status}) + got, result := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Equal(t, 30*time.Second, result.RequeueAfter) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + }) + } + t.Run("answer without the requested values", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeUpdateAIProvider, fakeFault{Rewrite: func(answer any) any { + p := answer.(codersdk.AIProvider) + p.Enabled = false + return p + }}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, true, "CoderAnswerMismatch") + require.Zero(t, got.Status.AppliedGeneration) + }) + t.Run("deleted before a full apply is created again", func(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) + oldID := got.Status.ProviderID + require.NoError(t, env.fake.client(t, 2*time.Second).DeleteAIProvider(ctx, oldID)) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.NotEqual(t, oldID, got.Status.ProviderID) + require.Equal(t, 2, env.fake.requestCount(routeCreateAIProvider)) + }) +} + +// A nonce provider found without a recorded credential (a create that +// committed after its marker was cleared) is ours: the controller records it +// and sends the current key, instead of failing an assertion. +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)) + _, err := env.fake.client(t, 2*time.Second).CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, Name: "openai", DisplayName: "coder-k8s:" + string(got.UID), + BaseURL: "https://api.openai.com/v1", APIKeys: []string{"sk-late-commit-0123456789ab"}, + }) + require.NoError(t, err) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + 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") +} diff --git a/internal/controller/fakecoder_test.go b/internal/controller/fakecoder_test.go index ca0c20520..04a1dcb30 100644 --- a/internal/controller/fakecoder_test.go +++ b/internal/controller/fakecoder_test.go @@ -51,6 +51,8 @@ type fakeFault struct { Rewrite func(answer any) any // Detail goes into the error answer's detail and a validation error. Detail string + // Message replaces the error answer's message. + Message string // HTML answers Status with this text/html body instead of a JSON error, // like a proxy or SSO page. With a 2xx Status, codersdk v2.37 returns a // *codersdk.Error with that status. @@ -201,6 +203,9 @@ func (f *fakeCoder) route(mux *http.ServeMux, pattern, route string, h func(*htt _, _ = w.Write([]byte(fault.HTML)) default: resp := codersdk.Response{Message: fmt.Sprintf("fake fault %d", fault.Status), Detail: fault.Detail} + if fault.Message != "" { + resp.Message = fault.Message + } if fault.Detail != "" { resp.Validations = []codersdk.ValidationError{{Field: "name", Detail: fault.Detail}} } From f70321cd8768eefed1a6ed24714891c0f334c932 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 20:05:49 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=A4=96=20fix:=20handle=20a=20CoderAIP?= =?UTF-8?q?rovider=20gone=20right=20after=20the=20create,=20and=20zero=20k?= =?UTF-8?q?ey=20IDs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A provider deleted between the create and the first sync no longer dereferences a nil step: the next reconcile looks up the name again. A key update answer with a zero key ID is a mismatch, and the resend marker stays. A dashboard disable during a pending key resend is still a kill switch, so a Secret rotation cannot undo it. Refs #154 Signed-off-by: Thomas Kosiewski --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ Change-Id: I3c31c5d8dcada80daf2f2b4a4dcbded1d576e0b9 --- .../controller/coderaiprovider_controller.go | 8 ++- internal/controller/coderaiprovider_sync.go | 22 ++++--- .../controller/coderaiprovider_sync_test.go | 66 +++++++++++++++++++ 3 files changed, 85 insertions(+), 11 deletions(-) diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index 48e86cf72..7754d0b58 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -221,9 +221,15 @@ func (r *CoderAIProviderReconciler) check( return nil, err } *before = *provider.Status.DeepCopy() - if step, _, err = r.syncAIProvider(ctx, sdk, provider, key, before); err != nil { + var gone bool + if step, gone, err = r.syncAIProvider(ctx, sdk, provider, key, before); err != nil { return nil, err } + if gone { + // Deleted right after the create. The next reconcile looks + // up the name again and creates the provider if it is free. + step = aiProviderWait(aiProviderConfirmPoll, "Pending", "Provider %s disappeared from Coder right after the create. The controller checks the name again.", provider.Name) + } } } // Coder messages can echo the request, so scrub the key from them. diff --git a/internal/controller/coderaiprovider_sync.go b/internal/controller/coderaiprovider_sync.go index c989ecd86..d7d9845af 100644 --- a/internal/controller/coderaiprovider_sync.go +++ b/internal/controller/coderaiprovider_sync.go @@ -10,6 +10,7 @@ import ( "time" "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" @@ -48,15 +49,16 @@ func (r *CoderAIProviderReconciler) syncAIProvider( keys := found.APIKeys ownKey := len(keys) == 1 && keys[0].ID.String() == cred.KeyID resending := cred.ResendAttemptTime != nil - if applied && !resending { - switch { - case enabled && !found.Enabled: - return aiProviderStalled(aiProviderResync, "DisabledExternally", - "Provider %s was disabled in Coder. The controller does not enable it again until the spec changes.", provider.Name), false, nil - case !ownKey: - return aiProviderStalled(aiProviderResync, "KeyChangedExternally", - "The key of provider %s was changed in Coder. The controller does not send its key again until the spec changes.", provider.Name), false, nil - } + // A delete or disable in Coder is a kill switch also while a key resend + // is pending: a Secret rotation must not undo a revocation. Only the key + // check waits for the resend, which changes the key itself. + switch { + case applied && enabled && !found.Enabled: + return aiProviderStalled(aiProviderResync, "DisabledExternally", + "Provider %s was disabled in Coder. The controller does not enable it again until the spec changes.", provider.Name), false, nil + case applied && !resending && !ownKey: + return aiProviderStalled(aiProviderResync, "KeyChangedExternally", + "The key of provider %s was changed in Coder. The controller does not send its key again until the spec changes.", provider.Name), false, nil } // A spec edit re-arms the object, so it also replaces a key that Coder @@ -125,7 +127,7 @@ func (r *CoderAIProviderReconciler) syncAIProvider( "Coder answered the update of provider %s without the requested display name, icon, enabled flag or base URL.", provider.Name), false, nil } if sendKey { - if len(updated.APIKeys) != 1 || updated.APIKeys[0].ID.String() == cred.KeyID { + if len(updated.APIKeys) != 1 || updated.APIKeys[0].ID == uuid.Nil || updated.APIKeys[0].ID.String() == cred.KeyID { // The marker stays, so the next reconcile sends the key again. return aiProviderWait(aiProviderRetry, "CoderAnswerMismatch", "Coder answered the key update of provider %s without exactly one new key. The controller sends the key again.", provider.Name), false, nil diff --git a/internal/controller/coderaiprovider_sync_test.go b/internal/controller/coderaiprovider_sync_test.go index 15b56cab4..ea50b4822 100644 --- a/internal/controller/coderaiprovider_sync_test.go +++ b/internal/controller/coderaiprovider_sync_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" ctrl "sigs.k8s.io/controller-runtime" @@ -229,6 +230,40 @@ func TestCoderAIProviderResendCrash(t *testing.T) { require.Equal(t, coder.keys[0].id.String(), got.Status.Credential.KeyID) } +// A delete or disable in Coder while a key resend is pending is still a kill +// switch: the Secret rotation does not undo it. +func TestCoderAIProviderKillSwitchDuringResend(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + reason string + act func(ctx context.Context, c *codersdk.Client, id string) error + }{ + {reason: "DisabledExternally", act: func(ctx context.Context, c *codersdk.Client, id string) error { + _, err := c.UpdateAIProvider(ctx, id, codersdk.UpdateAIProviderRequest{Enabled: ptrTo(false)}) + return err + }}, + {reason: "DeletedExternally", act: func(ctx context.Context, c *codersdk.Client, id string) error { + return c.DeleteAIProvider(ctx, id) + }}, + } { + t.Run(tc.reason, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + setSecretValue(ctx, t, secret, testAIProviderKeyRotated) + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusBadGateway}) + got, _ := env.reconcile(ctx, t, provider) + require.NotNil(t, got.Status.Credential.ResendAttemptTime, "a resend is pending") + + require.NoError(t, tc.act(ctx, env.fake.client(t, 2*time.Second), got.Status.ProviderID)) + writes := env.writes() + got, _ = env.reconcile(ctx, t, got) + requireAIProviderStep(t, got, true, tc.reason) + require.Equal(t, writes, env.writes()) + }) + } +} + // Plan S14: a dashboard delete, disable or key change after a full apply is a // kill switch: Stalled, and zero writes until a spec edit re-arms the object. func TestCoderAIProviderKillSwitches(t *testing.T) { @@ -342,6 +377,37 @@ func TestCoderAIProviderSyncAnswers(t *testing.T) { requireAIProviderStep(t, got, true, "CoderAnswerMismatch") require.Zero(t, got.Status.AppliedGeneration) }) + t.Run("key answer with a zero key ID", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + setSecretValue(ctx, t, secret, testAIProviderKeyRotated) + env.fake.failNext(routeUpdateAIProvider, fakeFault{Rewrite: func(answer any) any { + p := answer.(codersdk.AIProvider) + p.APIKeys[0].ID = uuid.Nil + return p + }}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderAnswerMismatch") + require.NotNil(t, got.Status.Credential.ResendAttemptTime, "the marker stays") + require.NotEqual(t, uuid.Nil.String(), got.Status.Credential.KeyID) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + }) + t.Run("deleted right after the create", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + // The name lookup passes. The read by ID after the create answers 404. + env.fake.failNext(routeAIProvider, fakeFault{Rewrite: func(answer any) any { return answer }}) + env.fake.failNext(routeAIProvider, fakeFault{Status: http.StatusNotFound}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "Pending") + require.Empty(t, got.Status.ProviderID) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "the provider was still there, with the nonce") + }) t.Run("deleted before a full apply is created again", func(t *testing.T) { t.Parallel() ctx := context.Background() From 6a7811ea0dc86e5b5a075316de6abe3d060a4ac0 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 20:32:35 +0000 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=A4=96=20fix:=20confirm=20a=20CoderAI?= =?UTF-8?q?Provider=20404=20update,=20and=20keep=20rejected=20creates=20st?= =?UTF-8?q?alled?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 404 to a sync PATCH is confirmed by a read before the controller drops ownership, because a proxy can put a 404 in place of a committed PATCH that already replaced the nonce. A rejected create whose confirmation read fails is remembered, so a later read that confirms absence stalls instead of sending the create again. A create answer without a provider ID is an assertion failure again. Refs #154 Signed-off-by: Thomas Kosiewski --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ Change-Id: I20e41722d73b684b02a240971521de86ecee71ed --- internal/controller/coderaiprovider_create.go | 35 +++++++++++++--- .../controller/coderaiprovider_create_test.go | 40 +++++++++++++++++++ internal/controller/coderaiprovider_sync.go | 8 ++++ .../controller/coderaiprovider_sync_test.go | 14 +++++++ 4 files changed, 92 insertions(+), 5 deletions(-) diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index 6cd71b660..f4800d58e 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -48,6 +48,13 @@ func (r *CoderAIProviderReconciler) createOrConfirm( if step := requireSiteOwner(ctx, sdk); step != nil { return step, nil } + if ready := meta.FindStatusCondition(provider.Status.Conditions, coderv1alpha1.CoderAIProviderConditionReady); ready != nil && + ready.Reason == aiProviderConfirmingRejectedCreate && ready.ObservedGeneration == provider.Generation { + // The last create of this spec was rejected, and now a read + // confirms that it created nothing. Do not send it again. + provider.Status.CreateAttemptTime, provider.Status.Credential = nil, nil + return aiProviderStalled(0, "CoderRejected", "Coder rejected the create request of provider %s, and a read confirmed that nothing was created.", provider.Name), nil + } } return r.createAIProvider(ctx, sdk, provider, key, before) case err != nil: @@ -87,7 +94,7 @@ func recordAIProvider(provider *coderv1alpha1.CoderAIProvider, found codersdk.AI provider.Name, found.ID), nil } if found.ID == uuid.Nil { - return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", "Coder answered provider %s without an ID.", provider.Name), nil + return nil, fmt.Errorf("assertion failed: Coder answered AI provider %s without an ID", provider.Name) } if provider.Status.Credential == nil { // A create of this object committed after its marker was cleared, so @@ -165,18 +172,36 @@ func (r *CoderAIProviderReconciler) confirmRejectedCreate( // Ours (the create committed) or someone else's (NameConflict). return recordAIProvider(provider, found) case !isCoderNotFound(err): - return aiProviderWait(aiProviderRetry, "ConfirmingCreate", "get provider %s after a %d answer: %s", provider.Name, status, coderErrorSummary(err)), nil + reason := "ConfirmingCreate" + if !aiProviderCreateRetryable(status) { + // Remember the rejection, so a later read that confirms absence + // stalls instead of sending the rejected create again. + reason = aiProviderConfirmingRejectedCreate + } + return aiProviderWait(aiProviderRetry, reason, "get provider %s after a %d answer: %s", provider.Name, status, coderErrorSummary(err)), nil } if step := requireSiteOwner(ctx, sdk); step != nil { return step, nil } provider.Status.CreateAttemptTime, provider.Status.Credential = nil, nil + if aiProviderCreateRetryable(status) { + return aiProviderWait(aiProviderRetry, "CreateRetrying", "The create request had no effect: %s", coderErrorSummary(createErr)), nil + } + return aiProviderStalled(0, "CoderRejected", "Coder rejected the create request: %s", coderErrorSummary(createErr)), nil +} + +// aiProviderConfirmingRejectedCreate is the wait reason after a create that +// Coder rejected, while the read that confirms absence fails. +const aiProviderConfirmingRejectedCreate = "ConfirmingRejectedCreate" + +// aiProviderCreateRetryable reports 4xx create answers that a retry can fix +// without a spec change. +func aiProviderCreateRetryable(status int) bool { switch status { case http.StatusUnauthorized, http.StatusForbidden, http.StatusConflict, http.StatusTooManyRequests: - return aiProviderWait(aiProviderRetry, "CreateRetrying", "The create request had no effect: %s", coderErrorSummary(createErr)), nil - default: - return aiProviderStalled(0, "CoderRejected", "Coder rejected the create request: %s", coderErrorSummary(createErr)), nil + return true } + return false } // isCoderAuthFailure reports 401 and 403. An expired operator token or a diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index b7cefe271..248c7e014 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" ctrl "sigs.k8s.io/controller-runtime" @@ -232,6 +233,45 @@ func TestCoderAIProviderCreateRejectedAfterCommit(t *testing.T) { } } +// A rejected create is not sent again when the read that confirms absence +// fails once. +func TestCoderAIProviderRejectedCreateConfirmFails(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeAIProvider, fakeFault{Rewrite: func(answer any) any { return answer }}) // The name lookup passes. + env.fake.failNext(routeAIProvider, fakeFault{Status: http.StatusBadGateway}) // The confirmation read fails. + env.fake.failNext(routeCreateAIProvider, fakeFault{Status: http.StatusBadRequest}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "ConfirmingRejectedCreate") + require.NotNil(t, got.Status.CreateAttemptTime) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderStep(t, got, true, "CoderRejected") + require.Nil(t, got.Status.CreateAttemptTime) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "the rejected create was not sent again") +} + +// A create answer without a provider ID is impossible, so it fails fast. +// The marker stays, and a read decides on the next reconcile. +func TestCoderAIProviderCreateAnswerWithoutID(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeCreateAIProvider, fakeFault{Rewrite: func(answer any) any { + p := answer.(codersdk.AIProvider) + p.ID = uuid.Nil + return p + }}) + _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.ErrorContains(t, err, "assertion failed: Coder answered AI provider openai without an ID") + stored := &coderv1alpha1.CoderAIProvider{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), stored)) + require.NotNil(t, stored.Status.CreateAttemptTime) + got, _ := env.reconcile(ctx, t, stored) + requireAIProviderReady(t, got) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) +} + // Plan S4: a provider that someone else creates between the lookup and the // create gives NameConflict, and the controller never changes it. func TestCoderAIProviderCreateConflict(t *testing.T) { diff --git a/internal/controller/coderaiprovider_sync.go b/internal/controller/coderaiprovider_sync.go index d7d9845af..0736a2429 100644 --- a/internal/controller/coderaiprovider_sync.go +++ b/internal/controller/coderaiprovider_sync.go @@ -109,6 +109,14 @@ func (r *CoderAIProviderReconciler) syncAIProvider( status := coderStatus(err) switch { case status == http.StatusNotFound: + // A proxy can put a 404 in place of a committed PATCH, which can + // have replaced the nonce. Only a read proves that the provider is + // gone, so ownership is kept until then. + if _, readErr := sdk.AIProvider(ctx, provider.Status.ProviderID); readErr == nil { + return aiProviderWait(aiProviderRetry, "CoderAnswerMismatch", "Coder answered 404 to the update of provider %s, but the provider exists.", provider.Name), false, nil + } else if !isCoderNotFound(readErr) { + return aiProviderWait(aiProviderRetry, "CoderUnavailable", "get provider %s after a 404 update: %s", provider.Name, coderErrorSummary(readErr)), false, nil + } return r.providerGone(provider, applied) case isCoderAuthFailure(status) || status == http.StatusTooManyRequests: // No Kubernetes event follows a repaired token or role, so retry. diff --git a/internal/controller/coderaiprovider_sync_test.go b/internal/controller/coderaiprovider_sync_test.go index ea50b4822..181c77e16 100644 --- a/internal/controller/coderaiprovider_sync_test.go +++ b/internal/controller/coderaiprovider_sync_test.go @@ -408,6 +408,20 @@ func TestCoderAIProviderSyncAnswers(t *testing.T) { requireAIProviderReady(t, got) require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "the provider was still there, with the nonce") }) + t.Run("404 in place of a committed update", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeUpdateAIProvider, fakeFault{AfterCommit: true, Status: http.StatusNotFound}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderAnswerMismatch") + stored, _ := env.fake.aiProvider("openai") + require.Equal(t, stored.id.String(), got.Status.ProviderID, "ownership is kept") + require.Equal(t, "OpenAI", stored.displayName, "the update replaced the nonce") + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) + }) t.Run("deleted before a full apply is created again", func(t *testing.T) { t.Parallel() ctx := context.Background()