diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index 7accca5bb..8fba23047 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,26 @@ 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 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 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 +266,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..e458b9b92 --- /dev/null +++ b/internal/controller/coderaiprovider_create.go @@ -0,0 +1,215 @@ +package controller + +import ( + "cmp" + "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 { + 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) || 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: + // 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 { + // 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) + 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()) + } + 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) +} + +// 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 +} + +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..ea14a7798 --- /dev/null +++ b/internal/controller/coderaiprovider_create_test.go @@ -0,0 +1,369 @@ +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: "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}, + {name: "forbidden", fault: fakeFault{Status: http.StatusForbidden}, 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()) + }) + } +} + +// 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) { + 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("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_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") +}