diff --git a/api/v1alpha1/coderaiprovider_types.go b/api/v1alpha1/coderaiprovider_types.go index f3f08cf37..453299b58 100644 --- a/api/v1alpha1/coderaiprovider_types.go +++ b/api/v1alpha1/coderaiprovider_types.go @@ -119,9 +119,11 @@ type CoderAIProviderStatus struct { // +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status` // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" // +kubebuilder:validation:XValidation:rule="size(self.metadata.name) <= 63 && self.metadata.name.matches('^[a-z0-9]+(-[a-z0-9]+)*$')",message="metadata.name is the Coder provider name: at most 63 lowercase letters, digits, and single hyphens between them" +// +kubebuilder:validation:XValidation:rule="!self.metadata.name.matches('^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$') && !self.metadata.name.matches('^[0-9a-f]{32}$')",message="metadata.name must not be a UUID: Coder reads a UUID-shaped provider name as a provider ID" // CoderAIProvider manages one AI provider in Coder. The object name is the -// Coder provider name. +// Coder provider name. It cannot be a UUID, because Coder looks up a +// UUID-shaped name as a provider ID. type CoderAIProvider struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` diff --git a/config/crd/dormant/coder.com_coderaiproviders.yaml b/config/crd/dormant/coder.com_coderaiproviders.yaml index d188c534e..6b8a6187e 100644 --- a/config/crd/dormant/coder.com_coderaiproviders.yaml +++ b/config/crd/dormant/coder.com_coderaiproviders.yaml @@ -32,7 +32,8 @@ spec: openAPIV3Schema: description: |- CoderAIProvider manages one AI provider in Coder. The object name is the - Coder provider name. + Coder provider name. It cannot be a UUID, because Coder looks up a + UUID-shaped name as a provider ID. properties: apiVersion: description: |- @@ -260,6 +261,10 @@ spec: - message: 'metadata.name is the Coder provider name: at most 63 lowercase letters, digits, and single hyphens between them' rule: size(self.metadata.name) <= 63 && self.metadata.name.matches('^[a-z0-9]+(-[a-z0-9]+)*$') + - message: 'metadata.name must not be a UUID: Coder reads a UUID-shaped provider + name as a provider ID' + rule: '!self.metadata.name.matches(''^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$'') + && !self.metadata.name.matches(''^[0-9a-f]{32}$'')' served: true storage: true subresources: diff --git a/internal/controller/coderaiprovider_api_test.go b/internal/controller/coderaiprovider_api_test.go index 0f7a3628c..cda57d0bd 100644 --- a/internal/controller/coderaiprovider_api_test.go +++ b/internal/controller/coderaiprovider_api_test.go @@ -43,6 +43,9 @@ func TestCoderAIProviderAPIValidation(t *testing.T) { {name: "name with 64 characters", objectName: strings.Repeat("a", 64), wantErr: "metadata.name is the Coder provider name"}, {name: "name with a dot", objectName: "open.ai", wantErr: "metadata.name is the Coder provider name"}, {name: "name with two hyphens", objectName: "open--ai", wantErr: "metadata.name is the Coder provider name"}, + {name: "name shaped like a UUID", objectName: "0b6f6c2e-3a4d-4c55-9d2f-3f1b1f0c9a11", wantErr: "metadata.name must not be a UUID"}, + {name: "name shaped like a UUID without hyphens", objectName: "0b6f6c2e3a4d4c559d2f3f1b1f0c9a11", wantErr: "metadata.name must not be a UUID"}, + {name: "name with 32 characters that is not hex", objectName: "0b6f6c2e3a4d4c559d2f3f1b1f0c9a1z", wantEnabled: true}, {name: "unknown type", wantErr: "spec.type", mutate: func(s *coderv1alpha1.CoderAIProviderSpec) { s.Type = "bedrock" }}, diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go new file mode 100644 index 000000000..7accca5bb --- /dev/null +++ b/internal/controller/coderaiprovider_controller.go @@ -0,0 +1,295 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "net/url" + "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" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +const ( + aiProviderControlPlaneRefIndex = ".spec.controlPlaneRef.name" + // aiProviderResync re-reads Coder for objects that wait on Coder state, + // for example a name conflict that an admin can remove in the dashboard. + aiProviderResync = 10 * time.Minute + // aiProviderRetry is the retry for waits that no watch ends: a failed + // Coder call, or an operator token Secret that is not readable yet. + aiProviderRetry = 30 * time.Second +) + +// 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. +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. +type aiProviderStep struct { + stalled bool + reason string + message string + requeue time.Duration +} + +func aiProviderWait(requeue time.Duration, reason, format string, args ...any) *aiProviderStep { + return &aiProviderStep{reason: reason, message: fmt.Sprintf(format, args...), requeue: requeue} +} + +func aiProviderStalled(requeue time.Duration, reason, format string, args ...any) *aiProviderStep { + return &aiProviderStep{stalled: true, reason: reason, message: fmt.Sprintf(format, args...), requeue: requeue} +} + +// SetupWithManager wires the reconciler into controller-runtime. Nothing +// calls it until activation. A change of the referenced control plane or of +// the key Secret requeues the provider. +func (r *CoderAIProviderReconciler) SetupWithManager(mgr ctrl.Manager) error { + if mgr == nil || r.Client == nil || r.Scheme == nil { + return fmt.Errorf("assertion failed: AI provider reconciler needs a manager, a client, and a scheme") + } + indexes := map[string]func(*coderv1alpha1.CoderAIProvider) string{ + aiProviderControlPlaneRefIndex: func(p *coderv1alpha1.CoderAIProvider) string { return p.Spec.ControlPlaneRef.Name }, + credentialSecretNamesFieldIndex: func(p *coderv1alpha1.CoderAIProvider) string { return p.Spec.APIKeySecretRef.Name }, + } + for name, value := range indexes { + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &coderv1alpha1.CoderAIProvider{}, name, func(obj client.Object) []string { + if p, ok := obj.(*coderv1alpha1.CoderAIProvider); ok && value(p) != "" { + return []string{value(p)} + } + return nil + }); err != nil { + return fmt.Errorf("index AI providers by %s: %w", name, err) + } + } + return ctrl.NewControllerManagedBy(mgr). + For(&coderv1alpha1.CoderAIProvider{}). + Watches(&coderv1alpha1.CoderControlPlane{}, handler.EnqueueRequestsFromMapFunc(r.providersForControlPlane)). + Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request { + return r.providersWith(ctx, obj.GetNamespace(), credentialSecretNamesFieldIndex, obj.GetName()) + }), + builder.WithPredicates(credentialsecret.WatchPredicate())). + Named("coderaiprovider"). + Complete(r) +} + +// providersForControlPlane maps a control plane to the providers that use it, +// and to the providers whose key Secret it names as its operator token +// Secret, because credentialsecret.Read refuses those. +func (r *CoderAIProviderReconciler) providersForControlPlane(ctx context.Context, obj client.Object) []reconcile.Request { + requests := r.providersWith(ctx, obj.GetNamespace(), aiProviderControlPlaneRefIndex, obj.GetName()) + controlPlane, ok := obj.(*coderv1alpha1.CoderControlPlane) + if !ok { + return requests + } + tokenSecrets := []string{credentialsecret.OperatorTokenSecretName(controlPlane)} + if ref := controlPlane.Status.OperatorTokenSecretRef; ref != nil { + tokenSecrets = append(tokenSecrets, strings.TrimSpace(ref.Name)) + } + for _, name := range sortedUnique(tokenSecrets) { + requests = append(requests, r.providersWith(ctx, obj.GetNamespace(), credentialSecretNamesFieldIndex, name)...) + } + return requests +} + +// providersWith returns requests for the providers in namespace whose index +// holds value. +func (r *CoderAIProviderReconciler) providersWith(ctx context.Context, namespace, index, value string) []reconcile.Request { + var providers coderv1alpha1.CoderAIProviderList + if err := r.List(ctx, &providers, client.InNamespace(namespace), client.MatchingFields{index: value}); err != nil { + log.FromContext(ctx).Error(err, "list AI providers", "index", index, "value", value) + return nil + } + requests := make([]reconcile.Request, 0, len(providers.Items)) + for _, p := range providers.Items { + requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: p.Namespace, Name: p.Name}}) + } + return requests +} + +// Reconcile runs the checks in order and reports the first one that fails. +func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + if r.Client == nil || r.Scheme == nil { + return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client and a scheme") + } + provider := &coderv1alpha1.CoderAIProvider{} + if err := r.Get(ctx, req.NamespacedName, provider); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) + } + if provider.Name != req.Name || provider.Namespace != req.Namespace { + return ctrl.Result{}, fmt.Errorf("assertion failed: fetched AI provider %s/%s does not match %s", provider.Namespace, provider.Name, req.NamespacedName) + } + before := provider.Status.DeepCopy() + + if !provider.DeletionTimestamp.IsZero() { + if !controllerutil.ContainsFinalizer(provider, coderv1alpha1.CoderAIProviderCleanupFinalizer) { + return ctrl.Result{}, nil + } + if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { + // Nothing of this provider can exist in Coder. + return ctrl.Result{}, r.patchFinalizer(ctx, provider, false) + } + // Keep the finalizer: this controller version cannot prove what Coder + // holds, so it neither deletes nor forgets the provider. + applyAIProviderStep(provider, aiProviderWait(0, "DeletePending", + "Provider %s can exist in Coder. This controller version keeps the finalizer and deletes nothing.", provider.Name)) + return ctrl.Result{}, r.writeStatus(ctx, provider, before) + } + + if err := r.patchFinalizer(ctx, provider, true); err != nil { + return ctrl.Result{}, err + } + step, err := r.check(ctx, provider) + if err != nil { + return ctrl.Result{}, err + } + applyAIProviderStep(provider, step) + if err := r.writeStatus(ctx, provider, before); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: step.requeue}, nil +} + +// 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) { + 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 + } + key, err := credentialsecret.Read(ctx, r.Client, provider.Namespace, provider.Spec.APIKeySecretRef, baseURL) + if credErr := (*credentialsecret.Error)(nil); errors.As(err, &credErr) { + return aiProviderStalled(0, credErr.Reason, "%s", credErr.Message), nil + } else if err != nil { + return nil, err + } + sdk, _, wait, err := controlPlaneCoderClient(ctx, r.Client, provider.Namespace, provider.Spec.ControlPlaneRef.Name) + if err != nil { + return nil, err + } + if wait != nil { + // The control plane watch ends most waits. A retry also covers an + // 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)) + } + if err := coderAnswerFor("AI provider", provider.Name, found.Name); err != nil { + return aiProviderStalled(aiProviderResync, "CoderAnswerMismatch", "%s", err.Error()) + } + 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) +} + +// aiProviderBaseURL parses the exact spec value the way Coder validates +// base_url (codersdk/aiproviders.go:459-476). It also requires a host name and +// refuses user info, as admission does, so the check holds without admission. +func aiProviderBaseURL(raw string) (*url.URL, error) { + if strings.TrimSpace(raw) != raw { + return nil, errors.New("leading or trailing whitespace") + } + parsed, err := url.Parse(raw) + if err != nil { + return nil, err + } + if parsed.User != nil { + return nil, errors.New("user info is not allowed") + } + if _, err := credentialsecret.Origin(parsed); err != nil { + return nil, err + } + return parsed, nil +} + +// patchFinalizer adds or removes the cleanup finalizer with a merge patch. The +// optimistic lock turns a concurrent finalizer change into a conflict, not a +// lost update. +func (r *CoderAIProviderReconciler) patchFinalizer(ctx context.Context, provider *coderv1alpha1.CoderAIProvider, add bool) error { + base := provider.DeepCopy() + changed := controllerutil.RemoveFinalizer(provider, coderv1alpha1.CoderAIProviderCleanupFinalizer) + if add { + changed = controllerutil.AddFinalizer(provider, coderv1alpha1.CoderAIProviderCleanupFinalizer) + } + if !changed { + return nil + } + if err := r.Patch(ctx, provider, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})); err != nil { + return fmt.Errorf("patch finalizer of AI provider %s/%s: %w", provider.Namespace, provider.Name, err) + } + return nil +} + +// applyAIProviderStep sets the kstatus conditions for step. Messages are cut +// to the CoderTemplateTest limit. +func applyAIProviderStep(provider *coderv1alpha1.CoderAIProvider, step *aiProviderStep) { + message := truncateTemplateTestMessage(step.message) + reconciling, stalled := metav1.ConditionTrue, metav1.ConditionFalse + if step.stalled { + reconciling, stalled = metav1.ConditionFalse, metav1.ConditionTrue + } + for _, c := range []struct { + conditionType string + status metav1.ConditionStatus + }{ + {coderv1alpha1.CoderAIProviderConditionReady, metav1.ConditionFalse}, + {coderv1alpha1.CoderAIProviderConditionReconciling, reconciling}, + {coderv1alpha1.CoderAIProviderConditionStalled, stalled}, + } { + meta.SetStatusCondition(&provider.Status.Conditions, metav1.Condition{ + Type: c.conditionType, Status: c.status, ObservedGeneration: provider.Generation, Reason: step.reason, Message: message, + }) + } +} + +// writeStatus writes the status only when it changed. +func (r *CoderAIProviderReconciler) writeStatus(ctx context.Context, provider *coderv1alpha1.CoderAIProvider, before *coderv1alpha1.CoderAIProviderStatus) error { + provider.Status.ObservedGeneration = provider.Generation + if equality.Semantic.DeepEqual(before, &provider.Status) { + return nil + } + if err := r.Status().Update(ctx, provider); err != nil { + return fmt.Errorf("update status of AI provider %s/%s: %w", provider.Namespace, provider.Name, err) + } + return nil +} diff --git a/internal/controller/coderaiprovider_controller_test.go b/internal/controller/coderaiprovider_controller_test.go new file mode 100644 index 000000000..9cdd8b3d5 --- /dev/null +++ b/internal/controller/coderaiprovider_controller_test.go @@ -0,0 +1,373 @@ +package controller_test + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/config" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" + "github.com/coder/coder-k8s/internal/credentialsecret" + "github.com/coder/coder-k8s/internal/credentialsecret/credentialsecrettest" +) + +const ( + testAIProviderKey = "sk-ai-provider-7f3c9a1e5b2d" // #nosec G101 -- test fixture, not a credential. + testAIProviderOrigin = "https://api.openai.com" +) + +// aiProviderEnv is one namespace with a fake Coder and, optionally, a ready +// control plane named coder that points at it. +type aiProviderEnv struct { + namespace string + fake *fakeCoder + r *controller.CoderAIProviderReconciler +} + +func newAIProviderEnv(ctx context.Context, t *testing.T, withControlPlane bool) aiProviderEnv { + t.Helper() + env := aiProviderEnv{namespace: createTestNamespace(ctx, t, "aip"), fake: newFakeCoder(t)} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme} + if withControlPlane { + createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) + } + return env +} + +func (env aiProviderEnv) keySecret(meta metav1.ObjectMeta, value string) *corev1.Secret { + meta.Name, meta.Namespace = "openai-api-key", env.namespace + return tokenSecret(meta, "api-key", value) +} + +func (env aiProviderEnv) createProvider(ctx context.Context, t *testing.T, mutate func(*coderv1alpha1.CoderAIProviderSpec)) *coderv1alpha1.CoderAIProvider { + t.Helper() + spec := validAIProviderSpec() + if mutate != nil { + mutate(&spec) + } + provider := &coderv1alpha1.CoderAIProvider{ObjectMeta: metav1.ObjectMeta{Name: "openai", Namespace: env.namespace}, Spec: spec} + require.NoError(t, k8sClient.Create(ctx, provider)) + t.Cleanup(func() { + _ = k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), provider) + provider.Finalizers = nil + _ = k8sClient.Update(context.Background(), provider) + _ = k8sClient.Delete(context.Background(), provider) + }) + return provider +} + +// reconcile runs one reconcile with captured logs, checks that neither the +// status, the logs, nor the error holds the key, and returns the stored object. +func (env aiProviderEnv) reconcile(ctx context.Context, t *testing.T, provider *coderv1alpha1.CoderAIProvider) (*coderv1alpha1.CoderAIProvider, ctrl.Result) { + t.Helper() + logCtx, logs := capturingContext(ctx) + result, err := env.r.Reconcile(logCtx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.NoError(t, err) + got := &coderv1alpha1.CoderAIProvider{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got)) + credentialsecrettest.RequireNoLeak(t, []string{testAIProviderKey}, map[string]any{"status": got.Status, "annotations": got.Annotations, "logs": logs.String()}) + return got, result +} + +// requireAIProviderStep checks the kstatus conditions of a wait or a stall. +func requireAIProviderStep(t *testing.T, provider *coderv1alpha1.CoderAIProvider, stalled bool, reason string) string { + t.Helper() + want := map[string]metav1.ConditionStatus{"Ready": metav1.ConditionFalse, "Reconciling": metav1.ConditionTrue, "Stalled": metav1.ConditionFalse} + if stalled { + want["Reconciling"], want["Stalled"] = metav1.ConditionFalse, metav1.ConditionTrue + } + for conditionType, status := range want { + condition := meta.FindStatusCondition(provider.Status.Conditions, conditionType) + require.NotNil(t, condition, conditionType) + require.Equal(t, status, condition.Status, conditionType) + require.Equal(t, reason, condition.Reason, conditionType) + require.Equal(t, provider.Generation, condition.ObservedGeneration) + } + require.Equal(t, provider.Generation, provider.Status.ObservedGeneration) + return meta.FindStatusCondition(provider.Status.Conditions, "Ready").Message +} + +// Plan tests S1, S2, S10 and S12 at reconciler level: every credential +// failure stalls with zero Coder requests, and a usable key reaches Coder. +func TestCoderAIProviderCredentialChecks(t *testing.T) { + t.Parallel() + consented := consentedObjectMeta("", "", testAIProviderOrigin) + for _, tc := range []struct { + name string + baseURL string // Default: https://api.openai.com/v1. + secret func(env aiProviderEnv) *corev1.Secret + setup func(ctx context.Context, t *testing.T, env aiProviderEnv) + reason string // Empty: the key is usable and the name lookup runs. + }{ + {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: "missing Secret", reason: credentialsecret.ReasonSecretNotFound}, + {name: "missing key", reason: credentialsecret.ReasonKeyMissing, secret: func(env aiProviderEnv) *corev1.Secret { + s := env.keySecret(consented, testAIProviderKey) + s.Data = map[string][]byte{"other": []byte(testAIProviderKey)} + return s + }}, + {name: "no label", reason: credentialsecret.ReasonNotOptedIn, secret: func(env aiProviderEnv) *corev1.Secret { + m := consentedObjectMeta("", "", testAIProviderOrigin) + m.Labels = nil + return env.keySecret(m, testAIProviderKey) + }}, + {name: "no destinations", reason: credentialsecret.ReasonNotOptedIn, secret: func(env aiProviderEnv) *corev1.Secret { + m := consentedObjectMeta("", "", testAIProviderOrigin) + m.Annotations = nil + return env.keySecret(m, testAIProviderKey) + }}, + {name: "other port", baseURL: "https://api.openai.com:8443/v1", reason: credentialsecret.ReasonDestinationNotAllowed, secret: func(env aiProviderEnv) *corev1.Secret { return env.keySecret(consented, testAIProviderKey) }}, + {name: "other scheme", baseURL: "http://api.openai.com/v1", reason: credentialsecret.ReasonDestinationNotAllowed, secret: func(env aiProviderEnv) *corev1.Secret { return env.keySecret(consented, testAIProviderKey) }}, + {name: "subdomain", baseURL: "https://eu.api.openai.com/v1", reason: credentialsecret.ReasonDestinationNotAllowed, secret: func(env aiProviderEnv) *corev1.Secret { return env.keySecret(consented, testAIProviderKey) }}, + {name: "trailing dot", baseURL: "https://api.openai.com./v1", reason: credentialsecret.ReasonDestinationNotAllowed, secret: func(env aiProviderEnv) *corev1.Secret { return env.keySecret(consented, testAIProviderKey) }}, + {name: "not Opaque", reason: credentialsecret.ReasonRefused, secret: func(env aiProviderEnv) *corev1.Secret { + s := env.keySecret(consented, testAIProviderKey) + s.Type = "example.com/api-key" + return s + }}, + {name: "owned by a coder.com object", reason: credentialsecret.ReasonRefused, secret: func(env aiProviderEnv) *corev1.Secret { + m := consentedObjectMeta("", "", testAIProviderOrigin) + m.OwnerReferences = []metav1.OwnerReference{{APIVersion: "coder.com/v1alpha1", Kind: "CoderControlPlane", Name: "coder", UID: "0b6f6c2e-3a4d-4c55-9d2f-3f1b1f0c9a11"}} + return env.keySecret(m, testAIProviderKey) + }}, + {name: "empty value", reason: credentialsecret.ReasonMalformed, secret: func(env aiProviderEnv) *corev1.Secret { return env.keySecret(consented, "") }}, + {name: "whitespace", reason: credentialsecret.ReasonMalformed, secret: func(env aiProviderEnv) *corev1.Secret { return env.keySecret(consented, testAIProviderKey+"\n") }}, + {name: "oversized", reason: credentialsecret.ReasonMalformed, secret: func(env aiProviderEnv) *corev1.Secret { + return env.keySecret(consented, testAIProviderKey+strings.Repeat("k", credentialsecret.MaxValueBytes)) + }}, + {name: "operator token Secret, labeled", reason: credentialsecret.ReasonRefused, setup: func(ctx context.Context, t *testing.T, env aiProviderEnv) { + operatorToken := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "coder-operator-token"}, operatorToken)) + operatorToken.Labels = map[string]string{credentialsecret.LabelKey: credentialsecret.LabelValue} + operatorToken.Annotations = map[string]string{credentialsecret.DestinationsAnnotation: testAIProviderOrigin} + require.NoError(t, k8sClient.Update(ctx, operatorToken)) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIProviderEnv(ctx, t, true) + if tc.secret != nil { + createSecret(ctx, t, tc.secret(env)) + } + if tc.setup != nil { + tc.setup(ctx, t, env) + } + provider := env.createProvider(ctx, t, func(s *coderv1alpha1.CoderAIProviderSpec) { + if tc.baseURL != "" { + s.BaseURL = tc.baseURL + } + if tc.setup != nil { + s.APIKeySecretRef = coderv1alpha1.SecretKeySelector{Name: "coder-operator-token", Key: coderv1alpha1.DefaultTokenSecretKey} + } + }) + got, result := env.reconcile(ctx, t, provider) + if tc.reason == "" { + requireAIProviderStep(t, got, false, "Pending") + require.Equal(t, 1, env.fake.requestCount(routeAIProvider), "a usable key reaches the name lookup") + require.Equal(t, 10*time.Minute, result.RequeueAfter) + return + } + requireAIProviderStep(t, got, true, tc.reason) + require.Zero(t, env.fake.totalRequests(), "a credential failure sends nothing to Coder") + require.Zero(t, result.RequeueAfter, "the Secret watch requeues the provider") + }) + } +} + +func TestCoderAIProviderControlPlaneNotReady(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIProviderEnv(ctx, t, false) + createSecret(ctx, t, env.keySecret(consentedObjectMeta("", "", testAIProviderOrigin), testAIProviderKey)) + provider := env.createProvider(ctx, t, nil) + got, result := env.reconcile(ctx, t, provider) + require.Equal(t, "CoderControlPlane coder does not exist.", requireAIProviderStep(t, got, false, "ControlPlaneNotReady")) + require.Equal(t, 30*time.Second, result.RequeueAfter) + + // A ready control plane whose operator token Secret is missing: no watch + // sees the Secret appear, so the wait retries on a timer. + createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) + operatorToken := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "coder-operator-token"}, operatorToken)) + require.NoError(t, k8sClient.Delete(ctx, operatorToken)) + got, result = env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "OperatorAccessNotReady") + require.Equal(t, 30*time.Second, result.RequeueAfter) + require.Zero(t, env.fake.totalRequests()) + + 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") +} + +// The name lookup only reads. A provider that this object did not create is a +// conflict (plan S4, read-only part), and Coder errors never put Detail or the +// key into status or logs (the redaction half of S3). +func TestCoderAIProviderNameLookup(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + prepare func(ctx context.Context, t *testing.T, f *fakeCoder) + stalled bool + reason string + message string + requeue time.Duration + }{ + { + name: "name taken", stalled: true, reason: "NameConflict", requeue: 10 * time.Minute, message: "Coder already has a provider named openai (ID ", + prepare: func(ctx context.Context, t *testing.T, f *fakeCoder) { + _, err := f.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) + }, + }, + { + name: "Coder error with the key in its detail", reason: "CoderUnavailable", requeue: 30 * time.Second, message: "get provider openai: Coder answered 500: fake fault 500", + prepare: func(_ context.Context, _ *testing.T, f *fakeCoder) { + for range 2 { // One fault per reconcile. + f.failNext(routeAIProvider, fakeFault{Status: 500, Detail: "upstream rejected " + testAIProviderKey}) + } + }, + }, + { + name: "answer for another provider", stalled: true, reason: "CoderAnswerMismatch", requeue: 10 * time.Minute, message: `assertion failed: Coder answered AI provider "other" when asked for "openai"`, + prepare: func(ctx context.Context, t *testing.T, f *fakeCoder) { + _, err := f.client(t, 2*time.Second).CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{Type: codersdk.AIProviderTypeOpenAI, Name: "openai", BaseURL: "https://api.openai.com/v1"}) + require.NoError(t, err) + for range 2 { // One fault per reconcile. + f.failNext(routeAIProvider, fakeFault{Rewrite: func(answer any) any { + p := answer.(codersdk.AIProvider) + p.Name = "other" + return p + }}) + } + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIProviderEnv(ctx, t, true) + createSecret(ctx, t, env.keySecret(consentedObjectMeta("", "", testAIProviderOrigin), testAIProviderKey)) + tc.prepare(ctx, t, env.fake) + before := env.fake.totalRequests() + got, result := env.reconcile(ctx, t, env.createProvider(ctx, t, nil)) + require.Contains(t, requireAIProviderStep(t, got, tc.stalled, tc.reason), tc.message) + require.Equal(t, tc.requeue, result.RequeueAfter) + require.Equal(t, 1, env.fake.totalRequests()-before, "one GET and no write") + + // A reconcile without a change sends no Coder write and leaves the + // stored status unchanged (no condition time flaps). + again, _ := env.reconcile(ctx, t, got) + require.Equal(t, got.ResourceVersion, again.ResourceVersion) + }) + } +} + +func TestCoderAIProviderFinalizer(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIProviderEnv(ctx, t, false) + provider := env.createProvider(ctx, t, func(s *coderv1alpha1.CoderAIProviderSpec) { s.Enabled = ptrTo(false) }) + got, _ := env.reconcile(ctx, t, provider) + require.Equal(t, []string{coderv1alpha1.CoderAIProviderCleanupFinalizer}, got.Finalizers) + require.Equal(t, ptrTo(false), got.Spec.Enabled, "the finalizer patch keeps enabled: false") + + // With nothing in Coder, deletion releases the finalizer. + require.NoError(t, k8sClient.Delete(ctx, got)) + _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) + require.NoError(t, err) + require.True(t, apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got))) + + // A provider with a Coder ID keeps its finalizer: this version deletes nothing. + kept := env.createProvider(ctx, t, nil) + kept, _ = env.reconcile(ctx, t, kept) + kept.Status.ProviderID = "5d0c6c2e-3a4d-4c55-9d2f-3f1b1f0c9a11" + require.NoError(t, k8sClient.Status().Update(ctx, kept)) + require.NoError(t, k8sClient.Delete(ctx, kept)) + kept, _ = env.reconcile(ctx, t, kept) + require.Equal(t, []string{coderv1alpha1.CoderAIProviderCleanupFinalizer}, kept.Finalizers) + requireAIProviderStep(t, kept, false, "DeletePending") + require.Zero(t, env.fake.totalRequests()) +} + +// Plan S2 through the manager: a Secret update that removes the label, a +// Secret delete, and control plane changes each requeue the provider at once. +func TestCoderAIProviderWatches(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIProviderEnv(ctx, t, false) + mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme, Metrics: metricsserver.Options{BindAddress: "0"}, HealthProbeBindAddress: "0", + Cache: cache.Options{DefaultNamespaces: map[string]cache.Config{env.namespace: {}}}, + Controller: config.Controller{SkipNameValidation: ptrTo(true)}, + }) + require.NoError(t, err) + require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme()}).SetupWithManager(mgr)) + mgrCtx, cancel := context.WithCancel(ctx) + mgrDone := make(chan error, 1) + go func() { mgrDone <- mgr.Start(mgrCtx) }() + t.Cleanup(func() { + cancel() + require.NoError(t, <-mgrDone) + }) + + secret := createSecret(ctx, t, env.keySecret(consentedObjectMeta("", "", testAIProviderOrigin), testAIProviderKey)) + provider := env.createProvider(ctx, t, nil) + requireReason := func(reason string) { + t.Helper() + require.Eventually(t, func() bool { + got := &coderv1alpha1.CoderAIProvider{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got)) + condition := meta.FindStatusCondition(got.Status.Conditions, "Ready") + return condition != nil && condition.Reason == reason + }, 30*time.Second, 100*time.Millisecond, reason) + } + requireReason("ControlPlaneNotReady") + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Labels = nil + require.NoError(t, k8sClient.Update(ctx, secret)) + requireReason(credentialsecret.ReasonNotOptedIn) + + secret.Labels = map[string]string{credentialsecret.LabelKey: credentialsecret.LabelValue} + require.NoError(t, k8sClient.Update(ctx, secret)) + requireReason("ControlPlaneNotReady") + + createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) + requireReason("Pending") + + // Another control plane that names the key Secret as its operator token + // Secret makes the key unusable. Removing it makes the key usable again. + other := &coderv1alpha1.CoderControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: env.namespace}, + Spec: coderv1alpha1.CoderControlPlaneSpec{ + Image: "coder-control-plane:test", OperatorAccess: coderv1alpha1.OperatorAccessSpec{GeneratedTokenSecretName: secret.Name}, + }, + } + require.NoError(t, k8sClient.Create(ctx, other)) + requireReason(credentialsecret.ReasonRefused) + require.NoError(t, k8sClient.Delete(ctx, other)) + requireReason("Pending") + + require.NoError(t, k8sClient.Delete(ctx, secret)) + requireReason(credentialsecret.ReasonSecretNotFound) +} diff --git a/internal/controller/coderaiprovider_internal_test.go b/internal/controller/coderaiprovider_internal_test.go new file mode 100644 index 000000000..d2904fe13 --- /dev/null +++ b/internal/controller/coderaiprovider_internal_test.go @@ -0,0 +1,30 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// The controller repeats Coder's base_url check on the exact spec value. +// Admission also rejects most of these, so they are tested here. +func TestAIProviderBaseURL(t *testing.T) { + t.Parallel() + for raw, wantErr := range map[string]bool{ + "https://api.openai.com/v1": false, + "http://[fd00::1]:8080/v1#models": false, + "https://api.example.com/v1?x=%zz": false, // Coder's url.Parse accepts a bad query escape too. + "https://api.example.com/v1#%zz": true, // Bad escape in the fragment. + "https://api.example.com/v1#a%2": true, + "https://api%.example.com": true, + "https://api.example.com/%zz": true, + "https://api.openai.com/v1 ": true, // The controller never trims. + "http://:8080/v1": true, // No host name. + "ftp://api.openai.com": true, + "api.openai.com": true, + "https://user:pass@api.openai.com/v": true, // Coder accepts user info. Admission and the controller refuse it. + } { + _, err := aiProviderBaseURL(raw) + require.Equal(t, wantErr, err != nil, "%q: %v", raw, err) + } +}