🤖 fix: trust only Coder's own 404 in the CoderAIProvider sync and disable paths

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

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)
+		})
+	}
+}

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)

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: "<html>404 Not Found</html>"},
+	"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))
+		})
+	}
+}