From f89258e8c85b0b1f5a8c090d8f8549c209f6bd10 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 15:33:01 +0000 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=A4=96=20feat:=20create=20the=20Coder?= =?UTF-8?q?=20provider=20for=20a=20CoderAIProvider?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CoderAIProvider reconciler (still not registered) now creates the Coder provider when the name is free: it writes the create marker and the key's change token first, then POSTs the provider disabled with the nonce display name coder-k8s:. An uncertain answer keeps the marker, and the next reconcile reads Coder before it sends anything again. The nonce and the type prove ownership. Then one PATCH per generation sets the display name, icon and enabled flag. The status message for a bad baseURL no longer repeats the URL. 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: I504b0bb461fb75ecba41154e9d34e01e642af890 --- .../controller/coderaiprovider_controller.go | 69 ++-- .../coderaiprovider_controller_test.go | 39 ++- internal/controller/coderaiprovider_create.go | 197 ++++++++++++ .../controller/coderaiprovider_create_test.go | 299 ++++++++++++++++++ .../coderaiprovider_internal_test.go | 19 ++ 5 files changed, 583 insertions(+), 40 deletions(-) create mode 100644 internal/controller/coderaiprovider_create.go create mode 100644 internal/controller/coderaiprovider_create_test.go diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index 7accca5bb..3315c0b26 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -8,7 +8,6 @@ import ( "strings" "time" - "github.com/coder/coder/v2/codersdk" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" @@ -39,16 +38,19 @@ const ( // CoderAIProviderReconciler reconciles CoderAIProvider objects (#154). It is // not registered yet. It checks the spec, the credential Secret and the -// control plane, and it reports a Coder provider name that something else -// already uses. It does not write to Coder. +// control plane, creates the Coder provider when the name is free, and sets +// the spec values on the provider that it created. It never changes a +// provider that it did not create. type CoderAIProviderReconciler struct { client.Client Scheme *runtime.Scheme } -// aiProviderStep is the outcome of one reconcile. A stalled step waits for a -// change of the spec or an input. Any other step waits for progress. +// aiProviderStep is the outcome of one reconcile. A ready step reports that +// Coder matches the spec. A stalled step waits for a change of the spec or an +// input. Any other step waits for progress. type aiProviderStep struct { + ready bool stalled bool reason string message string @@ -161,7 +163,7 @@ func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err := r.patchFinalizer(ctx, provider, true); err != nil { return ctrl.Result{}, err } - step, err := r.check(ctx, provider) + step, err := r.check(ctx, provider, before) if err != nil { return ctrl.Result{}, err } @@ -173,12 +175,17 @@ func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Requ } // check validates the base URL, reads the key through the credential Secret -// contract, gets Coder access, and looks up the provider name. Every check -// before the lookup sends nothing to Coder. -func (r *CoderAIProviderReconciler) check(ctx context.Context, provider *coderv1alpha1.CoderAIProvider) (*aiProviderStep, error) { +// contract, gets Coder access, and then creates or applies the provider. The +// checks before Coder access send nothing to Coder. +func (r *CoderAIProviderReconciler) check( + ctx context.Context, provider *coderv1alpha1.CoderAIProvider, before *coderv1alpha1.CoderAIProviderStatus, +) (*aiProviderStep, error) { baseURL, err := aiProviderBaseURL(provider.Spec.BaseURL) if err != nil { - return aiProviderStalled(0, credentialsecret.ReasonMalformed, "spec.baseURL is not a URL that Coder accepts: %v", err), nil + // A fixed message: the parse error repeats the URL, which can hold + // user info. A bad spec value stalls the object, not the reconcile. + return aiProviderStalled(0, credentialsecret.ReasonMalformed, //nolint:nilerr // Reported in status. + "spec.baseURL is not a URL that Coder accepts: it must be an http or https URL with a host, without user info, surrounding whitespace, or invalid percent escapes."), nil } key, err := credentialsecret.Read(ctx, r.Client, provider.Namespace, provider.Spec.APIKeySecretRef, baseURL) if credErr := (*credentialsecret.Error)(nil); errors.As(err, &credErr) { @@ -195,31 +202,18 @@ func (r *CoderAIProviderReconciler) check(ctx context.Context, provider *coderv1 // operator token Secret that appears later. return aiProviderWait(aiProviderRetry, wait.reason, "%s", wait.message), nil } - step := checkAIProviderName(ctx, sdk, provider) - step.message = credentialsecret.Redact(step.message, key) - return step, nil -} - -// checkAIProviderName reports whether the provider name is free in Coder. It -// only reads. A provider that this object never tried to create is not ours, -// because Coder providers carry no owner (no adoption). -func checkAIProviderName(ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider) *aiProviderStep { - if provider.Status.ProviderID != "" || provider.Status.CreateAttemptTime != nil { - return aiProviderWait(aiProviderResync, "Pending", "Provider %s has a Coder ID or a create attempt. This controller version does not change it.", provider.Name) - } - found, err := sdk.AIProvider(ctx, provider.Name) - switch { - case isCoderNotFound(err): - return aiProviderWait(aiProviderResync, "Pending", "Coder has no provider named %s. This controller version does not create it.", provider.Name) - case err != nil: - return aiProviderWait(aiProviderRetry, "CoderUnavailable", "get provider %s: %s", provider.Name, coderErrorSummary(err)) + var step *aiProviderStep + if provider.Status.ProviderID == "" { + if step, err = r.createOrConfirm(ctx, sdk, provider, key, before); err != nil { + return nil, err + } } - if err := coderAnswerFor("AI provider", provider.Name, found.Name); err != nil { - return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", "%s", err.Error()) + if step == nil { + step = applyAIProvider(ctx, sdk, provider, key) } - return aiProviderStalled(aiProviderResync, "NameConflict", - "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) + // Coder messages can echo the request, so scrub the key from them. + step.message = credentialsecret.Redact(step.message, key) + return step, nil } // aiProviderBaseURL parses the exact spec value the way Coder validates @@ -264,15 +258,18 @@ func (r *CoderAIProviderReconciler) patchFinalizer(ctx context.Context, provider // to the CoderTemplateTest limit. func applyAIProviderStep(provider *coderv1alpha1.CoderAIProvider, step *aiProviderStep) { message := truncateTemplateTestMessage(step.message) - reconciling, stalled := metav1.ConditionTrue, metav1.ConditionFalse - if step.stalled { + ready, reconciling, stalled := metav1.ConditionFalse, metav1.ConditionTrue, metav1.ConditionFalse + switch { + case step.ready: + ready, reconciling = metav1.ConditionTrue, metav1.ConditionFalse + case step.stalled: reconciling, stalled = metav1.ConditionFalse, metav1.ConditionTrue } for _, c := range []struct { conditionType string status metav1.ConditionStatus }{ - {coderv1alpha1.CoderAIProviderConditionReady, metav1.ConditionFalse}, + {coderv1alpha1.CoderAIProviderConditionReady, ready}, {coderv1alpha1.CoderAIProviderConditionReconciling, reconciling}, {coderv1alpha1.CoderAIProviderConditionStalled, stalled}, } { diff --git a/internal/controller/coderaiprovider_controller_test.go b/internal/controller/coderaiprovider_controller_test.go index 9cdd8b3d5..e21f2c912 100644 --- a/internal/controller/coderaiprovider_controller_test.go +++ b/internal/controller/coderaiprovider_controller_test.go @@ -89,6 +89,21 @@ func requireAIProviderStep(t *testing.T, provider *coderv1alpha1.CoderAIProvider if stalled { want["Reconciling"], want["Stalled"] = metav1.ConditionFalse, metav1.ConditionTrue } + return requireAIProviderConditions(t, provider, want, reason) +} + +// requireAIProviderReady checks that the provider reports Ready for its +// current generation. +func requireAIProviderReady(t *testing.T, provider *coderv1alpha1.CoderAIProvider) { + t.Helper() + want := map[string]metav1.ConditionStatus{"Ready": metav1.ConditionTrue, "Reconciling": metav1.ConditionFalse, "Stalled": metav1.ConditionFalse} + requireAIProviderConditions(t, provider, want, "Applied") + require.Equal(t, provider.Generation, provider.Status.AppliedGeneration) + require.Nil(t, provider.Status.CreateAttemptTime) +} + +func requireAIProviderConditions(t *testing.T, provider *coderv1alpha1.CoderAIProvider, want map[string]metav1.ConditionStatus, reason string) string { + t.Helper() for conditionType, status := range want { condition := meta.FindStatusCondition(provider.Status.Conditions, conditionType) require.NotNil(t, condition, conditionType) @@ -115,6 +130,12 @@ func TestCoderAIProviderCredentialChecks(t *testing.T) { {name: "consented", secret: func(env aiProviderEnv) *corev1.Secret { return env.keySecret(consented, testAIProviderKey) }}, {name: "uppercase host", baseURL: "https://API.OpenAI.com/v1", secret: func(env aiProviderEnv) *corev1.Secret { return env.keySecret(consented, testAIProviderKey) }}, {name: "explicit default port", baseURL: "https://api.openai.com:443/v1", secret: func(env aiProviderEnv) *corev1.Secret { return env.keySecret(consented, testAIProviderKey) }}, + {name: "IPv6 host", baseURL: "https://[FD00::1]:8443/v1", secret: func(env aiProviderEnv) *corev1.Secret { + return env.keySecret(consentedObjectMeta("", "", "https://[fd00::1]:8443"), testAIProviderKey) + }}, + {name: "IPv6 host, other port", baseURL: "https://[fd00::1]/v1", reason: credentialsecret.ReasonDestinationNotAllowed, secret: func(env aiProviderEnv) *corev1.Secret { + return env.keySecret(consentedObjectMeta("", "", "https://[fd00::1]:8443"), testAIProviderKey) + }}, {name: "missing Secret", reason: credentialsecret.ReasonSecretNotFound}, {name: "missing key", reason: credentialsecret.ReasonKeyMissing, secret: func(env aiProviderEnv) *corev1.Secret { s := env.keySecret(consented, testAIProviderKey) @@ -178,8 +199,9 @@ func TestCoderAIProviderCredentialChecks(t *testing.T) { }) got, result := env.reconcile(ctx, t, provider) if tc.reason == "" { - requireAIProviderStep(t, got, false, "Pending") + requireAIProviderReady(t, got) require.Equal(t, 1, env.fake.requestCount(routeAIProvider), "a usable key reaches the name lookup") + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) require.Equal(t, 10*time.Minute, result.RequeueAfter) return } @@ -214,7 +236,7 @@ func TestCoderAIProviderControlPlaneNotReady(t *testing.T) { operatorToken.ObjectMeta = metav1.ObjectMeta{Name: operatorToken.Name, Namespace: env.namespace} require.NoError(t, k8sClient.Create(ctx, operatorToken)) got, _ = env.reconcile(ctx, t, provider) - requireAIProviderStep(t, got, false, "Pending") + requireAIProviderReady(t, got) } // The name lookup only reads. A provider that this object did not create is a @@ -353,7 +375,7 @@ func TestCoderAIProviderWatches(t *testing.T) { requireReason("ControlPlaneNotReady") createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) - requireReason("Pending") + requireReason("Applied") // Another control plane that names the key Secret as its operator token // Secret makes the key unusable. Removing it makes the key usable again. @@ -365,8 +387,17 @@ func TestCoderAIProviderWatches(t *testing.T) { } require.NoError(t, k8sClient.Create(ctx, other)) requireReason(credentialsecret.ReasonRefused) + // Renaming its operator token Secret frees the key Secret. The update + // event maps the old object too, so the provider is checked again. + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(other), other)) + other.Spec.OperatorAccess.GeneratedTokenSecretName = "other-operator-token" + require.NoError(t, k8sClient.Update(ctx, other)) + requireReason("Applied") + other.Spec.OperatorAccess.GeneratedTokenSecretName = secret.Name + require.NoError(t, k8sClient.Update(ctx, other)) + requireReason(credentialsecret.ReasonRefused) require.NoError(t, k8sClient.Delete(ctx, other)) - requireReason("Pending") + requireReason("Applied") require.NoError(t, k8sClient.Delete(ctx, secret)) requireReason(credentialsecret.ReasonSecretNotFound) diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go new file mode 100644 index 000000000..acaaaab22 --- /dev/null +++ b/internal/controller/coderaiprovider_create.go @@ -0,0 +1,197 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "net/http" + "slices" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +const ( + // aiProviderNoncePrefix starts the display name of a provider that the + // controller creates. With the object UID it proves that this object + // created the provider, because Coder stores no owner. + aiProviderNoncePrefix = "coder-k8s:" + // aiProviderConfirmPoll is the poll while a create result is uncertain. + aiProviderConfirmPoll = 5 * time.Second +) + +func aiProviderNonce(provider *coderv1alpha1.CoderAIProvider) string { + return aiProviderNoncePrefix + string(provider.UID) +} + +// createOrConfirm handles a provider without a Coder ID. Without a create +// marker it looks up the name and creates the provider when the name is free. +// With a marker, a create can be in flight or committed, so it reads Coder +// first and never sends the create again before that read. +func (r *CoderAIProviderReconciler) createOrConfirm( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, key credentialsecret.Value, before *coderv1alpha1.CoderAIProviderStatus, +) (*aiProviderStep, error) { + if stalled := meta.FindStatusCondition(provider.Status.Conditions, coderv1alpha1.CoderAIProviderConditionStalled); provider.Status.CreateAttemptTime == nil && + stalled != nil && stalled.Reason == "CoderRejected" && stalled.ObservedGeneration == provider.Generation { + // Coder rejected this spec. Only a spec change retries. + return aiProviderStalled(0, stalled.Reason, "%s", stalled.Message), nil + } + found, err := sdk.AIProvider(ctx, provider.Name) + switch { + case isCoderNotFound(err): + if provider.Status.CreateAttemptTime != nil { + if step := requireSiteOwner(ctx, sdk); step != nil { + return step, nil + } + } + return r.createAIProvider(ctx, sdk, provider, key, before) + case err != nil: + return aiProviderWait(aiProviderRetry, "CoderUnavailable", "get provider %s: %s", provider.Name, coderErrorSummary(err)), nil + } + return recordAIProvider(provider, found) +} + +// requireSiteOwner returns a wait unless the operator user is a site owner. +// Coder v2.37.3 answers 403 to anyone else on the provider routes +// (coderd/ai_providers.go:640-643), but a 404 proves absence only for a +// caller that can read every provider, so the controller checks it. +func requireSiteOwner(ctx context.Context, sdk *codersdk.Client) *aiProviderStep { + operator, err := sdk.User(ctx, codersdk.Me) + if err != nil { + return aiProviderWait(aiProviderRetry, "CoderUnavailable", "get operator user: %s", coderErrorSummary(err)) + } + if !slices.ContainsFunc(operator.Roles, func(role codersdk.SlimRole) bool { return role.Name == codersdk.RoleOwner && role.OrganizationID == "" }) { + return aiProviderWait(aiProviderResync, "ConfirmingCreate", "The operator user is not a site owner, so a missing provider proves nothing.") + } + return nil +} + +// recordAIProvider decides whether found is the provider that this object +// created. Only the nonce and the type prove it. Anything else is a provider +// that the controller never touches (no adoption). +func recordAIProvider(provider *coderv1alpha1.CoderAIProvider, found codersdk.AIProvider) (*aiProviderStep, error) { + if err := coderAnswerFor("AI provider", provider.Name, found.Name); err != nil { + return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", "%s", err.Error()), nil + } + if found.DisplayName != aiProviderNonce(provider) || string(found.Type) != provider.Spec.Type { + // No create of this object can commit while another provider holds + // the name, so nothing of this object can exist in Coder. + provider.Status.CreateAttemptTime, provider.Status.Credential = nil, nil + return aiProviderStalled(aiProviderResync, "NameConflict", + "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 len(found.APIKeys) != 1 || found.APIKeys[0].ID == uuid.Nil { + return aiProviderStalled(0, "KeyChangedExternally", + "Provider %s has %d keys in Coder, but this resource created it with one. The controller does not change it until the spec changes.", provider.Name, len(found.APIKeys)), nil + } + provider.Status.ProviderID, provider.Status.Credential.KeyID = found.ID.String(), found.APIKeys[0].ID.String() + provider.Status.CreateAttemptTime = nil + return nil, nil +} + +// createAIProvider writes the create marker and the change token of the key, +// then sends the only create request of this attempt. A conflict on the +// marker write ends the reconcile before any request. The provider starts +// disabled with the nonce as its display name. The apply step then sets the +// spec values. +func (r *CoderAIProviderReconciler) createAIProvider( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, key credentialsecret.Value, before *coderv1alpha1.CoderAIProviderStatus, +) (*aiProviderStep, error) { + token := key.Token + provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now()} + provider.Status.Credential = &coderv1alpha1.CoderAIProviderCredentialStatus{ + SecretName: token.SecretName, SecretKey: token.SecretKey, SecretUID: string(token.SecretUID), SecretResourceVersion: token.ResourceVersion, + } + applyAIProviderStep(provider, aiProviderWait(0, "CreatingProvider", "Creating provider %s in Coder.", provider.Name)) + if err := r.writeStatus(ctx, provider, before); err != nil { + return nil, err + } + *before = *provider.Status.DeepCopy() + + created, err := sdk.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderType(provider.Spec.Type), Name: provider.Name, DisplayName: aiProviderNonce(provider), + Enabled: false, BaseURL: provider.Spec.BaseURL, APIKeys: []string{key.Reveal()}, + }) + status := coderStatus(err) + switch { + case err == nil: + if step, err := recordAIProvider(provider, created); step != nil || err != nil { + // A wrong answer leaves the marker, so a read decides. + if step != nil && step.reason != "KeyChangedExternally" { + provider.Status.CreateAttemptTime, provider.Status.Credential = before.CreateAttemptTime, before.Credential + step = aiProviderWait(aiProviderConfirmPoll, "ConfirmingCreate", "The create answer does not match the request (%s). The controller reads Coder instead of sending it again.", step.message) + } + return step, err + } + return nil, nil + case createHadNoEffect(err): + 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 + 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 { + if provider.Status.AppliedGeneration == provider.Generation { + return aiProviderReady(provider) + } + 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.") + } + 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) + 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 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()) + } + 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.") + } + provider.Status.AppliedGeneration = provider.Generation + return aiProviderReady(provider) +} + +func sameAIProviderKey(recorded *coderv1alpha1.CoderAIProviderCredentialStatus, token credentialsecret.ChangeToken) bool { + return recorded != nil && recorded.SecretName == token.SecretName && recorded.SecretKey == token.SecretKey && + recorded.SecretUID == string(token.SecretUID) && recorded.SecretResourceVersion == token.ResourceVersion +} + +func aiProviderReady(provider *coderv1alpha1.CoderAIProvider) *aiProviderStep { + return &aiProviderStep{ready: true, reason: "Applied", message: fmt.Sprintf("Coder provider %s (ID %s) matches the spec.", provider.Name, provider.Status.ProviderID), requeue: aiProviderResync} +} diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go new file mode 100644 index 000000000..7a266f19c --- /dev/null +++ b/internal/controller/coderaiprovider_create_test.go @@ -0,0 +1,299 @@ +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/controller" +) + +// 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) { + t.Helper() + env := newAIProviderEnv(ctx, t, true) + secret := createSecret(ctx, t, env.keySecret(consentedObjectMeta("", "", testAIProviderOrigin), testAIProviderKey)) + provider := env.createProvider(ctx, t, func(s *coderv1alpha1.CoderAIProviderSpec) { + s.DisplayName, s.Icon = "OpenAI", "/icon/openai.svg" + }) + return env, secret, provider +} + +// restart gives env a new reconciler, as a controller restart does. The +// reconciler keeps no state between calls, so every step resumes from status. +func (env *aiProviderEnv) restart() { + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme} +} + +func TestCoderAIProviderCreate(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newAIProviderCreateEnv(ctx, t) + + // Stop after the create to see what it sent. + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusInternalServerError}) + got, result := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Equal(t, 30*time.Second, result.RequeueAfter) + stored, ok := env.fake.aiProvider("openai") + require.True(t, ok) + require.False(t, stored.enabled, "the provider starts disabled") + require.Equal(t, "coder-k8s:"+string(got.UID), stored.displayName, "the nonce proves who created it") + require.Equal(t, codersdk.AIProviderTypeOpenAI, stored.providerType) + require.Equal(t, "https://api.openai.com/v1", stored.baseURL) + require.Len(t, stored.keys, 1) + require.Equal(t, testAIProviderKey, stored.keys[0].plaintext) + require.Equal(t, stored.id.String(), got.Status.ProviderID) + require.Nil(t, got.Status.CreateAttemptTime) + require.Equal(t, &coderv1alpha1.CoderAIProviderCredentialStatus{ + SecretName: secret.Name, SecretKey: "api-key", SecretUID: string(secret.UID), SecretResourceVersion: secret.ResourceVersion, + KeyID: stored.keys[0].id.String(), + }, got.Status.Credential) + require.Zero(t, got.Status.AppliedGeneration) + + env.restart() + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + stored, _ = env.fake.aiProvider("openai") + 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") + + // A ready provider needs no Coder call and no status write. + before := env.fake.totalRequests() + again, _ := env.reconcile(ctx, t, got) + require.Equal(t, before, 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)) + require.Zero(t, env.fake.requestCount(routeDeleteAIProvider)) +} + +// Plan S5 and each outcome class of section 5.6: an uncertain create keeps +// the marker and is never sent again before a read, and every class ends +// with exactly one provider that this object owns. +func TestCoderAIProviderCreateOutcomes(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + fault fakeFault + firstStep string + posts int // Create requests in all. + keepMarker bool + }{ + {name: "answer lost after commit", fault: fakeFault{AfterCommit: true, Reset: true}, firstStep: "ConfirmingCreate", posts: 1, keepMarker: true}, + {name: "connection reset before commit", fault: fakeFault{Reset: true}, firstStep: "ConfirmingCreate", posts: 2, keepMarker: true}, + {name: "HTML 200 after commit", fault: fakeFault{AfterCommit: true, Status: http.StatusOK, HTML: "proxy"}, firstStep: "ConfirmingCreate", posts: 1, keepMarker: true}, + {name: "HTML 502 before commit", fault: fakeFault{Status: http.StatusBadGateway, HTML: "bad gateway"}, firstStep: "ConfirmingCreate", posts: 2, keepMarker: true}, + {name: "500 after commit", fault: fakeFault{AfterCommit: true, Status: http.StatusInternalServerError}, firstStep: "ConfirmingCreate", posts: 1, keepMarker: true}, + {name: "answer for another name", fault: fakeFault{AfterCommit: true, Rewrite: func(answer any) any { + p := answer.(codersdk.AIProvider) + p.Name = "other" + return p + }}, firstStep: "ConfirmingCreate", posts: 1, keepMarker: true}, + {name: "rate limited", fault: fakeFault{Status: http.StatusTooManyRequests}, firstStep: "CreateRetrying", posts: 2}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeCreateAIProvider, tc.fault) + + got, result := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, tc.firstStep) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) + require.Empty(t, got.Status.ProviderID) + if tc.keepMarker { + require.NotNil(t, got.Status.CreateAttemptTime, "an uncertain create keeps the marker") + require.NotNil(t, got.Status.Credential) + require.Equal(t, 5*time.Second, result.RequeueAfter) + } else { + require.Nil(t, got.Status.CreateAttemptTime, "a create without effect clears the marker") + require.Nil(t, got.Status.Credential) + } + + for range 2 { + env.restart() + got, _ = env.reconcile(ctx, t, got) + } + requireAIProviderReady(t, got) + require.Equal(t, tc.posts, env.fake.requestCount(routeCreateAIProvider)) + require.Equal(t, 1, env.fake.aiProviderCount(), "exactly one provider") + stored, _ := env.fake.aiProvider("openai") + require.Equal(t, stored.id.String(), got.Status.ProviderID) + require.Equal(t, stored.keys[0].id.String(), got.Status.Credential.KeyID) + require.Zero(t, env.fake.requestCount(routeDeleteAIProvider)) + }) + } +} + +// A create that hangs ends with the reconcile's context. The marker is +// already stored, so the next reconcile reads Coder first. +func TestCoderAIProviderCreateHangs(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + committed bool + }{{name: "before commit"}, {name: "after commit", committed: true}} { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeCreateAIProvider, fakeFault{Hang: true, AfterCommit: tc.committed}) + + short, cancel := context.WithTimeout(ctx, 2*time.Second) + 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") + + env.restart() + got, _ := env.reconcile(ctx, t, stored) + requireAIProviderReady(t, got) + posts := 2 + if tc.committed { + posts = 1 + } + require.Equal(t, posts, env.fake.requestCount(routeCreateAIProvider)) + require.Equal(t, 1, env.fake.aiProviderCount()) + }) + } +} + +// 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) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + foreign, err := env.fake.client(t, 2*time.Second).CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, Name: "openai", DisplayName: "Team key", BaseURL: "https://api.openai.com/v1", Enabled: true, + }) + require.NoError(t, err) + setupPosts := env.fake.requestCount(routeCreateAIProvider) + // The lookup does not see it yet. + env.fake.failNext(routeAIProvider, fakeFault{Status: http.StatusNotFound}) + + got, _ := env.reconcile(ctx, t, provider) + require.Contains(t, requireAIProviderStep(t, got, true, "NameConflict"), foreign.ID.String()) + require.Nil(t, got.Status.CreateAttemptTime) + require.Nil(t, got.Status.Credential) + require.Empty(t, got.Status.ProviderID) + env.restart() + got, _ = env.reconcile(ctx, t, got) + requireAIProviderStep(t, got, true, "NameConflict") + + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)-setupPosts, "one create, then never again") + require.Zero(t, env.fake.requestCount(routeUpdateAIProvider)) + require.Zero(t, env.fake.requestCount(routeDeleteAIProvider)) + stored, _ := env.fake.aiProvider("openai") + require.Equal(t, foreign.ID, stored.id) + require.Equal(t, "Team key", stored.displayName) +} + +// A rejected create is not sent again until the spec changes. +func TestCoderAIProviderCreateRejected(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeCreateAIProvider, fakeFault{Status: http.StatusBadRequest}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, true, "CoderRejected") + require.Nil(t, got.Status.CreateAttemptTime) + before := env.fake.totalRequests() + env.restart() + got, _ = env.reconcile(ctx, t, got) + requireAIProviderStep(t, got, true, "CoderRejected") + require.Equal(t, before, env.fake.totalRequests()) + + got.Spec.DisplayName = "OpenAI v2" + require.NoError(t, k8sClient.Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + require.Equal(t, 2, env.fake.requestCount(routeCreateAIProvider)) +} + +// Plan 5.6 rule 4: a missing provider proves nothing unless the operator +// user is a site owner, so the create is not sent again. +func TestCoderAIProviderConfirmNeedsSiteOwner(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeCreateAIProvider, fakeFault{Reset: true}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "ConfirmingCreate") + + env.fake.updateUser(env.fake.operatorID, func(u *codersdk.User) { u.Roles = []codersdk.SlimRole{} }) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireAIProviderStep(t, got, false, "ConfirmingCreate"), "not a site owner") + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider)) + + env.fake.updateUser(env.fake.operatorID, func(u *codersdk.User) { u.Roles = []codersdk.SlimRole{{Name: codersdk.RoleOwner}} }) + got, _ = env.reconcile(ctx, t, got) + 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("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_internal_test.go b/internal/controller/coderaiprovider_internal_test.go index d2904fe13..68846bb22 100644 --- a/internal/controller/coderaiprovider_internal_test.go +++ b/internal/controller/coderaiprovider_internal_test.go @@ -1,9 +1,13 @@ package controller import ( + "context" "testing" "github.com/stretchr/testify/require" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" ) // The controller repeats Coder's base_url check on the exact spec value. @@ -28,3 +32,18 @@ func TestAIProviderBaseURL(t *testing.T) { require.Equal(t, wantErr, err != nil, "%q: %v", raw, err) } } + +// The status message for a bad base URL is fixed: the parse error repeats +// the URL, which can hold user info. +func TestAIProviderBadBaseURLMessage(t *testing.T) { + t.Parallel() + r := &CoderAIProviderReconciler{} + provider := &coderv1alpha1.CoderAIProvider{} + provider.Spec.BaseURL = "https://user:hunter2-password@api%.example.com/v1" + step, err := r.check(context.Background(), provider, provider.Status.DeepCopy()) + require.NoError(t, err) + require.True(t, step.stalled) + require.Equal(t, credentialsecret.ReasonMalformed, step.reason) + require.NotContains(t, step.message, "hunter2") + require.NotContains(t, step.message, "api%.example.com") +} From d514eb4e0254e6278033b2a780e7310f4aa9a9ce Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 16:04:23 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=A4=96=20fix:=20keep=20CoderAIProvide?= =?UTF-8?q?r=20ownership=20across=20a=20crash=20after=20the=20apply?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Store the provider ID before the apply PATCH replaces the nonce display name, so a crash or a failed status write after the PATCH cannot turn the provider into a NameConflict. Check the applied display name, icon and enabled flag in the update answer before reporting Ready. Treat a create answer without the expected key as uncertain, so a read confirms 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: Ie28921f3eaf3a24af2a6d7b4cf47211b0652758f --- .../controller/coderaiprovider_controller.go | 8 ++++ internal/controller/coderaiprovider_create.go | 7 +++- .../controller/coderaiprovider_create_test.go | 42 +++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index 3315c0b26..8fba23047 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -207,6 +207,14 @@ func (r *CoderAIProviderReconciler) check( 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, + // 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 == nil { step = applyAIProvider(ctx, sdk, provider, key) diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index acaaaab22..11f867f15 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -1,6 +1,7 @@ package controller import ( + "cmp" "context" "errors" "fmt" @@ -127,7 +128,7 @@ func (r *CoderAIProviderReconciler) createAIProvider( case err == nil: if step, err := recordAIProvider(provider, created); step != nil || err != nil { // A wrong answer leaves the marker, so a read decides. - if step != nil && step.reason != "KeyChangedExternally" { + if step != nil { provider.Status.CreateAttemptTime, provider.Status.Credential = before.CreateAttemptTime, before.Credential step = aiProviderWait(aiProviderConfirmPoll, "ConfirmingCreate", "The create answer does not match the request (%s). The controller reads Coder instead of sending it again.", step.message) } @@ -183,6 +184,10 @@ func applyAIProvider(ctx context.Context, sdk *codersdk.Client, provider *coderv 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.") } + // 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.AppliedGeneration = provider.Generation return aiProviderReady(provider) } diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index 7a266f19c..ba3132750 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -101,6 +101,11 @@ func TestCoderAIProviderCreateOutcomes(t *testing.T) { p.Name = "other" return p }}, firstStep: "ConfirmingCreate", posts: 1, keepMarker: true}, + {name: "answer without the key", fault: fakeFault{AfterCommit: true, Rewrite: func(answer any) any { + p := answer.(codersdk.AIProvider) + p.APIKeys = nil + return p + }}, firstStep: "ConfirmingCreate", posts: 1, keepMarker: true}, {name: "rate limited", fault: fakeFault{Status: http.StatusTooManyRequests}, firstStep: "CreateRetrying", posts: 2}, } { t.Run(tc.name, func(t *testing.T) { @@ -172,6 +177,30 @@ func TestCoderAIProviderCreateHangs(t *testing.T) { } } +// The provider ID is stored before the apply replaces the nonce, so a crash +// between the apply and the next status write keeps ownership. +func TestCoderAIProviderCrashAfterApply(t *testing.T) { + t.Parallel() + 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) + 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") + 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)) + require.Equal(t, stored.id.String(), got.Status.ProviderID, "the ID was stored before the apply") + require.Nil(t, got.Status.CreateAttemptTime) + + env.restart() + got, _ = env.reconcile(ctx, t, got) + 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) { @@ -283,6 +312,19 @@ func TestCoderAIProviderApplyGuards(t *testing.T) { 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("deleted in Coder before the apply", func(t *testing.T) { t.Parallel() ctx := context.Background() From 877bfedef1788abfcf492f7066e6034b349b4b3b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 16:35:12 +0000 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=A4=96=20fix:=20check=20the=20CoderAI?= =?UTF-8?q?Provider=20key=20before=20Ready,=20and=20retry=20401=20and=2040?= =?UTF-8?q?3?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rotated key Secret does not change the generation, so check the change token before the ready shortcut. Retry a 401 or 403 on the apply PATCH, and treat a 403 on the create as a create without effect, because no Kubernetes event follows a repaired operator token or owner role. 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: I0eedef2da992751b7089adbc82b42b90572f97f4 --- internal/controller/coderaiprovider_create.go | 21 +++++++++++--- .../controller/coderaiprovider_create_test.go | 28 +++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index 11f867f15..e458b9b92 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -135,7 +135,9 @@ func (r *CoderAIProviderReconciler) createAIProvider( return step, err } return nil, nil - case createHadNoEffect(err): + 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. 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: @@ -159,12 +161,14 @@ func (r *CoderAIProviderReconciler) createAIProvider( // 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 { - if provider.Status.AppliedGeneration == provider.Generation { - return aiProviderReady(provider) - } + // 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, @@ -173,6 +177,9 @@ func applyAIProvider(ctx context.Context, sdk *codersdk.Client, provider *coderv 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: @@ -192,6 +199,12 @@ func applyAIProvider(ctx context.Context, sdk *codersdk.Client, provider *coderv return aiProviderReady(provider) } +// isCoderAuthFailure reports 401 and 403. An expired operator token or a +// removed owner role can be repaired without any Kubernetes event. +func isCoderAuthFailure(status int) bool { + return status == http.StatusUnauthorized || status == http.StatusForbidden +} + func sameAIProviderKey(recorded *coderv1alpha1.CoderAIProviderCredentialStatus, token credentialsecret.ChangeToken) bool { return recorded != nil && recorded.SecretName == token.SecretName && recorded.SecretKey == token.SecretKey && recorded.SecretUID == string(token.SecretUID) && recorded.SecretResourceVersion == token.ResourceVersion diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index ba3132750..ea14a7798 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -107,6 +107,7 @@ func TestCoderAIProviderCreateOutcomes(t *testing.T) { return p }}, firstStep: "ConfirmingCreate", posts: 1, keepMarker: true}, {name: "rate limited", fault: fakeFault{Status: http.StatusTooManyRequests}, firstStep: "CreateRetrying", posts: 2}, + {name: "forbidden", fault: fakeFault{Status: http.StatusForbidden}, firstStep: "CreateRetrying", posts: 2}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -325,6 +326,33 @@ func TestCoderAIProviderApplyGuards(t *testing.T) { 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()