diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 1b24a5dd..d79020d2 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -83,7 +83,7 @@ The object has the conditions `Ready`, `Reconciling` and `Stalled`. A stalled ob ### The credential kill switch -When the key Secret fails the credential contract for a provider that the operator created, the operator disables the provider in Coder at once. This covers every credential error: the Secret is deleted, loses its label or the destinations annotation, no longer lists the origin that Coder uses, is refused, has no such key, or holds a malformed value. The update holds only `enabled: false`. Coder keeps the old key, but no longer uses it. +When the key Secret fails the credential contract for a provider that the operator created, the operator disables the provider in Coder at once. This covers every credential error: the Secret is deleted, loses its label or the destinations annotation, no longer lists the origin that Coder uses, is refused, has no such key, or holds a malformed value. The update holds only `enabled: false`. Coder keeps the old key, but no longer uses it. When a Coder call for the disable fails, including a 404 that Coder did not send (for example from a proxy), the operator retries every 30 seconds, and the condition message starts with `Coder can still send the key`. The object stalls with the credential reason. Repairing the Secret does not enable the provider again. To enable it, fix the Secret, then edit the spec (any change of `spec`, for example `displayName`). A short read error of the Secret, such as an API server timeout, is not a credential error: the operator retries and changes nothing. diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index 8e9ec5b8..0348d1f2 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -49,7 +49,9 @@ func (r *CoderAIProviderReconciler) createOrConfirm( ready.Reason == aiProviderConfirmingRejectedCreate && ready.ObservedGeneration == provider.Generation found, err := sdk.AIProvider(ctx, provider.Name) switch { - case isCoderNotFound(err): + case isCoderResourceNotFound(err): + // Only Coder's own 404 proves that the name is free. Any other + // answer keeps the marker and waits (#338). if provider.Status.CreateAttemptTime != nil { if step := requireSiteOwner(ctx, sdk); step != nil { return keepRejectedCreate(step, rejected), nil @@ -192,7 +194,7 @@ func (r *CoderAIProviderReconciler) confirmRejectedCreate( case err == nil: // Ours (the create committed) or someone else's (NameConflict). return recordAIProvider(ctx, provider, found) - case !isCoderNotFound(err): + case !isCoderResourceNotFound(err): reason := "ConfirmingCreate" if !aiProviderCreateRetryable(status) { // Remember the rejection, so a later read that confirms absence diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index b87c32f9..3c68c891 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -2,6 +2,7 @@ package controller_test import ( "context" + "fmt" "net/http" "testing" "time" @@ -10,6 +11,7 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -284,7 +286,7 @@ func TestCoderAIProviderCreateConflict(t *testing.T) { require.NoError(t, err) setupPosts := env.fake.requestCount(routeCreateAIProvider) // The lookup does not see it yet. - env.fake.failNext(routeAIProvider, fakeFault{Status: http.StatusNotFound}) + env.fake.failNext(routeAIProvider, fakeFault{Status: http.StatusNotFound, Message: coderResourceNotFound}) got, _ := env.reconcile(ctx, t, provider) require.Contains(t, requireAIProviderStep(t, got, true, "NameConflict"), foreign.ID.String()) @@ -371,3 +373,65 @@ func TestCoderAIProviderRejectedCreateOwnerCheckFails(t *testing.T) { require.Nil(t, got.Status.CreateAttemptTime) require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "the rejected create was not sent again") } + +// #338: a 404 that Coder did not send on the name lookup proves nothing. A +// pending create keeps its marker and sends no create, a new create is not +// sent, and the next reconcile with Coder's own answer decides. +func TestCoderAIProviderCreateLookupNotCoder404(t *testing.T) { + t.Parallel() + for name, fault := range notCoder404s { + for _, pending := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/pending=%v", name, pending), func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newAIProviderCreateEnv(ctx, t) + if pending { + // A create that committed, but whose answer was lost. + _, 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) + provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now()} + provider.Status.Credential = &coderv1alpha1.CoderAIProviderCredentialStatus{SecretName: secret.Name, SecretKey: "api-key"} + require.NoError(t, k8sClient.Status().Update(ctx, provider)) + } + posts := env.fake.requestCount(routeCreateAIProvider) + 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, posts, env.fake.requestCount(routeCreateAIProvider), "no create is sent") + require.Equal(t, pending, got.Status.CreateAttemptTime != nil, "a pending create keeps its marker") + + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, 1, env.fake.aiProviderCount(), "one provider in Coder") + }) + } + } +} + +// #338: after a rejected create, the read that confirms absence also needs +// Coder's own 404. Otherwise the marker stays and the create is not sent +// again. +func TestCoderAIProviderRejectedCreateNotCoder404(t *testing.T) { + t.Parallel() + for name, fault := range notCoder404s { + t.Run(name, func(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, fault) // The confirmation read. + 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, "the marker stays") + 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") + }) + } +}