From adf7e26fc97b3495c65085f5f2f4f3d00a6ecd02 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 06:13:24 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20fix:=20trust=20only=20Coder's=20?= =?UTF-8?q?own=20404=20in=20the=20CoderAIProvider=20sync=20and=20disable?= =?UTF-8?q?=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 404 that Coder did not send, such as an HTML page of a proxy or Coder's "Route not found." for a wrong base URL, made the sync path clear the provider ID and credential after a spec edit, so the object lost ownership for good, and made the disable path report the provider gone without disabling it. Both now wait with CoderUnavailable and change nothing. Fixes #327 Change-Id: Ifcd70ee11dcb94c41bd0c29680b22afce2248774 Signed-off-by: Thomas Kosiewski --- .../controller/coderaiprovider_disable.go | 3 +- .../coderaiprovider_disable_test.go | 28 +++++++ internal/controller/coderaiprovider_sync.go | 6 +- .../controller/coderaiprovider_sync_test.go | 76 ++++++++++++++++++- 4 files changed, 108 insertions(+), 5 deletions(-) diff --git a/internal/controller/coderaiprovider_disable.go b/internal/controller/coderaiprovider_disable.go index 1c3eb361d..acc08229d 100644 --- a/internal/controller/coderaiprovider_disable.go +++ b/internal/controller/coderaiprovider_disable.go @@ -53,7 +53,8 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent( } found, err := sdk.AIProvider(ctx, provider.Status.ProviderID) switch { - case isCoderNotFound(err): + case isCoderResourceNotFound(err): + // Only Coder's own 404 proves that nothing can use the key (#327). return stalled("Provider %s does not exist in Coder.", provider.Name), nil case err != nil: return unchecked("get provider: %s", coderErrorSummary(err)), nil diff --git a/internal/controller/coderaiprovider_disable_test.go b/internal/controller/coderaiprovider_disable_test.go index defa8ac04..12ba95172 100644 --- a/internal/controller/coderaiprovider_disable_test.go +++ b/internal/controller/coderaiprovider_disable_test.go @@ -370,3 +370,31 @@ func TestCoderAIProviderTransientSecretError(t *testing.T) { }) } } + +// #327: a 404 that Coder did not send, on the read before the disable, does +// not prove that nothing uses the key. The object waits and says that Coder +// can still send the key, and the next reconcile disables the provider. +func TestCoderAIProviderDisableNotCoder404(t *testing.T) { + t.Parallel() + for name, fault := range notCoder404s { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + editSecret(ctx, t, secret, func(s *corev1.Secret) { delete(s.Labels, credentialsecret.LabelKey) }) + writes := env.writes() + env.fake.failNext(routeAIProvider, fault) + got, result := env.reconcile(ctx, t, provider) + require.Contains(t, requireAIProviderStep(t, got, false, credentialsecret.ReasonNotOptedIn), "Coder can still send the key") + require.Equal(t, 30*time.Second, result.RequeueAfter) + require.Equal(t, writes, env.writes()) + stored, _ := env.fake.aiProvider("openai") + require.True(t, stored.enabled) + + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireAIProviderStep(t, got, true, credentialsecret.ReasonNotOptedIn), "The controller disabled provider openai in Coder") + stored, _ = env.fake.aiProvider("openai") + require.False(t, stored.enabled) + }) + } +} diff --git a/internal/controller/coderaiprovider_sync.go b/internal/controller/coderaiprovider_sync.go index ed6734f1e..c9289a565 100644 --- a/internal/controller/coderaiprovider_sync.go +++ b/internal/controller/coderaiprovider_sync.go @@ -40,7 +40,9 @@ func (r *CoderAIProviderReconciler) syncAIProvider( applied := provider.Status.AppliedGeneration == provider.Generation found, err := sdk.AIProvider(ctx, provider.Status.ProviderID) switch { - case isCoderNotFound(err): + case isCoderResourceNotFound(err): + // Only Coder's own 404 proves that the provider is gone. Any other + // error, a proxy 404 included, keeps ownership (#327). return r.providerGone(provider, applied) case err != nil: return aiProviderWait(aiProviderRetry, "CoderUnavailable", "get provider %s: %s", provider.Name, coderErrorSummary(err)), false, nil @@ -122,7 +124,7 @@ func (r *CoderAIProviderReconciler) syncAIProvider( // 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) { + } else if !isCoderResourceNotFound(readErr) { return aiProviderWait(aiProviderRetry, "CoderUnavailable", "get provider %s after a 404 update: %s", provider.Name, coderErrorSummary(readErr)), false, nil } return r.providerGone(provider, applied) diff --git a/internal/controller/coderaiprovider_sync_test.go b/internal/controller/coderaiprovider_sync_test.go index 6bd89bb99..7bebc305e 100644 --- a/internal/controller/coderaiprovider_sync_test.go +++ b/internal/controller/coderaiprovider_sync_test.go @@ -2,6 +2,7 @@ package controller_test import ( "context" + "fmt" "net/http" "testing" "time" @@ -421,9 +422,10 @@ func TestCoderAIProviderSyncAnswers(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. + // The name lookup passes. The read by ID after the create answers + // Coder's own 404. env.fake.failNext(routeAIProvider, fakeFault{Rewrite: func(answer any) any { return answer }}) - env.fake.failNext(routeAIProvider, fakeFault{Status: http.StatusNotFound}) + env.fake.failNext(routeAIProvider, fakeFault{Status: http.StatusNotFound, Message: coderResourceNotFound}) got, _ := env.reconcile(ctx, t, provider) requireAIProviderStep(t, got, false, "Pending") require.Empty(t, got.Status.ProviderID) @@ -562,3 +564,73 @@ func TestCoderAIProviderAnswerMismatchIsLogged(t *testing.T) { }) } } + +// coderResourceNotFound is the message of Coder's own 404 +// (coderd/httpapi/httpapi.go:149 in v2.37.3). +const coderResourceNotFound = "Resource not found or you do not have access to this resource" + +// notCoder404s are 404 answers that Coder itself did not send: an HTML page +// of a proxy, and Coder's answer for an unknown route (a wrong base URL). +var notCoder404s = map[string]fakeFault{ + "HTML page": {Status: http.StatusNotFound, HTML: "404 Not Found"}, + "route not found": {Status: http.StatusNotFound, Message: "Route not found."}, +} + +// #327: in sync, a 404 that Coder did not send keeps ownership. It waits +// with CoderUnavailable and changes nothing, both for an applied provider +// (no DeletedExternally) and after a spec edit (no cleared provider ID, no +// create). +func TestCoderAIProviderSyncNotCoder404(t *testing.T) { + t.Parallel() + for name, fault := range notCoder404s { + for _, edited := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/edited=%v", name, edited), func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + if edited { + provider = editSpec(ctx, t, provider, func(s *coderv1alpha1.CoderAIProviderSpec) { s.DisplayName = "OpenAI v2" }) + } + before := provider.Status.DeepCopy() + writes := env.writes() + env.fake.failNext(routeAIProvider, fault) + got, result := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Equal(t, 30*time.Second, result.RequeueAfter) + require.Equal(t, before.ProviderID, got.Status.ProviderID, "ownership is kept") + require.Equal(t, before.Credential, got.Status.Credential) + require.Equal(t, writes, env.writes(), "no Coder write") + + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "the only create is the first one") + }) + } + } +} + +// #327: the read that follows a 404 update also needs Coder's own 404 +// before the provider counts as gone. +func TestCoderAIProviderSyncUpdate404NotCoderRead(t *testing.T) { + t.Parallel() + for name, fault := range notCoder404s { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + provider = editSpec(ctx, t, provider, func(s *coderv1alpha1.CoderAIProviderSpec) { s.DisplayName = "OpenAI v2" }) + providerID := provider.Status.ProviderID + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusNotFound, Message: coderResourceNotFound}) + env.fake.failNext(routeAIProvider, fakeFault{Rewrite: func(answer any) any { return answer }}) // The sync read passes. + env.fake.failNext(routeAIProvider, fault) + got, _ := env.reconcile(ctx, t, provider) + require.Contains(t, requireAIProviderStep(t, got, false, "CoderUnavailable"), "after a 404 update") + require.Equal(t, providerID, got.Status.ProviderID, "ownership is kept") + require.NotNil(t, got.Status.Credential) + + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) + }) + } +}