diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index 0614079b3..e4bcb1980 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -3,7 +3,9 @@ package controller import ( "cmp" "context" + "crypto/sha256" "fmt" + "maps" "net/http" "slices" "strings" @@ -27,12 +29,14 @@ import ( // it, so the ACL never holds a group that neither generation allows (plan // S20). // -// A dashboard delete after a full apply of this generation is a kill switch: -// the controller reports it and writes nothing until the spec changes (plan -// 4.7 item 2). The other kill switches come in the next change. recreate is -// true when the config is gone before a full apply of this generation: the -// caller then creates it again, if the slug is free. -func syncMCPServer( +// After a full apply of this generation, a dashboard delete, disable or +// credential clear is a kill switch: the controller reports it and writes +// nothing until the spec changes (plan 4.7 item 2). An auth type user_oidc +// is always set back, with only the auth fields and url, so a disable stays. +// +// recreate is true when the config is gone before a full apply of this +// generation: the caller then creates it again, if the slug is free. +func (r *CoderMCPServerReconciler) syncMCPServer( ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, values map[string]credentialsecret.Value, ) (step *mcpServerStep, recreate bool, err error) { id, err := uuid.Parse(server.Status.ConfigID) @@ -47,6 +51,7 @@ func syncMCPServer( "MCP server config %s was deleted in Coder. The controller does not create it again until the spec changes.", id), false, nil case isCoderNotFound(err): server.Status.ConfigID, server.Status.Credentials = "", nil + r.sentCredentials.Delete(server.UID) return nil, true, nil case err != nil: return mcpServerWait(mcpServerRetry, "CoderUnavailable", "get MCP server config %s: %s", id, coderErrorSummary(err)), false, nil @@ -56,14 +61,46 @@ func syncMCPServer( "assertion failed: Coder answered MCP server config %s with slug %s in organization %s when asked for %s", found.ID, found.Slug, found.OrganizationID, id), false, nil } + // user_oidc sends each chat user's login token to url (plan F14), so it is + // set back even when a kill switch holds every other write. + oidc := found.AuthType == "user_oidc" + enabled := server.Spec.Enabled == nil || *server.Spec.Enabled + var kill *mcpServerStep + switch active := mcpServerActiveCredentials(server.Spec.Auth); { + case applied && enabled && !found.Enabled: + kill = mcpServerStalled(mcpServerResync, "DisabledExternally", + "MCP server config %s was disabled in Coder. The controller does not enable it again until the spec changes.", id) + case applied && !oidc && (active.apiKey && !found.HasAPIKey || active.headers && !found.HasCustomHeaders || active.secret && !found.HasOAuth2Secret): + // A dashboard change of the auth type lands here too: Coder clears + // the credentials of the old type (plan F9). + kill = mcpServerStalled(mcpServerResync, "CredentialClearedExternally", + "A credential of MCP server config %s was cleared in Coder (auth type %s). The controller does not send it again until the spec changes.", id, found.AuthType) + } + if kill != nil && !oidc { + return kill, false, nil + } + full, err := mcpServerUpdateRequest(server, values) if err != nil { return nil, false, err } - if req := mcpServerSparseRequest(server, full, found, values); req != (codersdk.UpdateMCPServerConfigRequest{}) { - if step, err := narrowMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { - return step, false, err + // A hash is only for a field of the spec's auth type: a change of the + // auth type cleared the others in Coder, also when its answer was lost. + r.pruneSentCredentials(server) + req, sent := r.mcpServerSparseRequest(server, full, found, values) + if kill != nil { + req = mcpServerAuthOnly(req) + } + if req != (codersdk.UpdateMCPServerConfigRequest{}) { + if kill == nil { + if step, err := narrowMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { + return step, false, err + } } + // Coder can commit the PATCH and the answer still get lost. Until a + // good answer, the controller does not know which value Coder holds, + // so a retry must send these credentials again. + r.forgetSentCredentials(server, sent) updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, req) switch status := coderStatus(err); { case status == http.StatusBadRequest: @@ -74,12 +111,21 @@ func syncMCPServer( // so the GET of the next reconcile decides whether the config is gone. return mcpServerWait(mcpServerRetry, "CoderUnavailable", "update MCP server config %s: %s", id, coderErrorSummary(err)), false, nil } - if field := mcpServerMismatch(server.Spec.Auth, full, updated); field != "" || updated.ID != id { + check := full + if kill != nil { + check = mcpServerAuthOnly(full) + } + if field := mcpServerMismatch(server.Spec.Auth, check, updated); field != "" || updated.ID != id { return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered the update of MCP server config %s with another %s", id, cmp.Or(field, "ID")), false, nil } + r.rememberSentCredentials(server, sent, values) } else if field := mcpServerMismatch(server.Spec.Auth, full, found); field != "" { return nil, false, fmt.Errorf("assertion failed: MCP server %s/%s differs in %s, but the sync request is empty", server.Namespace, server.Name, field) } + if kill != nil { + kill.message = "The controller set the auth type back from user_oidc. " + kill.message + return kill, false, nil + } if step, err := syncMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { return step, false, err } @@ -97,11 +143,12 @@ func mcpServerActiveCredentials(auth coderv1alpha1.CoderMCPServerAuth) mcpServer // mcpServerSparseRequest drops every field of full that found already holds. // It keeps a credential when a URL or the auth type changes, when its change -// token changed, or when Coder lost it. It clears credentials of the other -// auth types that Coder holds (#314). -func mcpServerSparseRequest( +// token changed and Coder does not hold the value already, or when Coder lost +// it. It clears credentials of the other auth types that Coder holds (#314). +// It returns the credential fields that the request sends. +func (r *CoderMCPServerReconciler) mcpServerSparseRequest( server *coderv1alpha1.CoderMCPServer, full codersdk.UpdateMCPServerConfigRequest, found codersdk.MCPServerConfig, values map[string]credentialsecret.Value, -) codersdk.UpdateMCPServerConfigRequest { +) (codersdk.UpdateMCPServerConfigRequest, []string) { req := full authChanged := found.AuthType != *full.AuthType for _, f := range mcpServerStringFields(&req, found) { @@ -125,7 +172,11 @@ func mcpServerSparseRequest( } token := values[field].Token c, ok := recorded[field] - return !ok || c.SecretName != token.SecretName || c.SecretKey != token.SecretKey || c.SecretUID != string(token.SecretUID) || c.SecretResourceVersion != token.ResourceVersion + if ok && c.SecretName == token.SecretName && c.SecretKey == token.SecretKey && c.SecretUID == string(token.SecretUID) && c.SecretResourceVersion == token.ResourceVersion { + return false + } + // Only the Secret's metadata changed when Coder holds this value. + return !r.sentCredentialMatches(server, field, values[field]) } var sent []string for _, c := range mcpServerCredentialRefs(server.Spec.Auth) { @@ -138,8 +189,13 @@ func mcpServerSparseRequest( sends := func(prefix string) bool { return slices.ContainsFunc(sent, func(f string) bool { return strings.HasPrefix(f, prefix) }) } - // Custom headers are one Coder field, so they all go together. - if !active.headers || !sends("header:") { + if active.headers && sends("header:") { + // Custom headers are one Coder field, so they all go together. + sent = slices.DeleteFunc(sent, func(f string) bool { return strings.HasPrefix(f, "header:") }) + for _, h := range server.Spec.Auth.CustomHeaders { + sent = append(sent, "header:"+h.Name) + } + } else { req.CustomHeaders = nil } if !active.apiKey || !sends("apiKey") { @@ -159,7 +215,19 @@ func mcpServerSparseRequest( if !active.secret && found.HasOAuth2Secret { req.OAuth2ClientSecret = &empty } - return req + return req, sent +} + +// mcpServerAuthOnly keeps only the auth fields of req and url. The auth +// fields can carry credentials, so url goes with them: Coder must never +// store a credential next to a url that the Secret does not allow (plan 4.6 +// item 5). +func mcpServerAuthOnly(req codersdk.UpdateMCPServerConfigRequest) codersdk.UpdateMCPServerConfigRequest { + return codersdk.UpdateMCPServerConfigRequest{ + URL: req.URL, AuthType: req.AuthType, APIKeyHeader: req.APIKeyHeader, APIKeyValue: req.APIKeyValue, CustomHeaders: req.CustomHeaders, + OAuth2ClientID: req.OAuth2ClientID, OAuth2ClientSecret: req.OAuth2ClientSecret, OAuth2AuthURL: req.OAuth2AuthURL, + OAuth2TokenURL: req.OAuth2TokenURL, OAuth2RevocationURL: req.OAuth2RevocationURL, OAuth2Scopes: req.OAuth2Scopes, + } } type mcpServerStringField struct { @@ -244,6 +312,66 @@ func mcpServerMismatch(auth coderv1alpha1.CoderMCPServerAuth, want codersdk.Upda return "" } +// rememberSentCredentials keeps a SHA-256 of each credential value that Coder +// now holds, in process memory only (plan 4.6 item 4), so a metadata-only +// Secret change sends nothing. After a restart the map is empty, so one +// resend can happen. It drops the hashes of fields that the spec's auth type +// does not use: the PATCH cleared those credentials, so a later round trip +// of the auth type must send them again. +func (r *CoderMCPServerReconciler) rememberSentCredentials(server *coderv1alpha1.CoderMCPServer, sent []string, values map[string]credentialsecret.Value) { + sums := map[string][sha256.Size]byte{} + if old, ok := r.sentCredentials.Load(server.UID); ok { + for _, c := range mcpServerCredentialRefs(server.Spec.Auth) { + if sum, ok := old.(map[string][sha256.Size]byte)[c.field]; ok { + sums[c.field] = sum + } + } + } + for _, field := range sent { + sums[field] = sha256.Sum256([]byte(values[field].Reveal())) + } + r.sentCredentials.Store(server.UID, sums) +} + +// pruneSentCredentials drops the hashes of fields that the spec's auth type +// does not use. +func (r *CoderMCPServerReconciler) pruneSentCredentials(server *coderv1alpha1.CoderMCPServer) { + old, ok := r.sentCredentials.Load(server.UID) + if !ok { + return + } + sums := map[string][sha256.Size]byte{} + for _, c := range mcpServerCredentialRefs(server.Spec.Auth) { + if sum, ok := old.(map[string][sha256.Size]byte)[c.field]; ok { + sums[c.field] = sum + } + } + r.sentCredentials.Store(server.UID, sums) +} + +// forgetSentCredentials drops the hashes of fields before a write that sends +// them. +func (r *CoderMCPServerReconciler) forgetSentCredentials(server *coderv1alpha1.CoderMCPServer, fields []string) { + old, ok := r.sentCredentials.Load(server.UID) + if !ok || len(fields) == 0 { + return + } + sums := maps.Clone(old.(map[string][sha256.Size]byte)) + for _, field := range fields { + delete(sums, field) + } + r.sentCredentials.Store(server.UID, sums) +} + +func (r *CoderMCPServerReconciler) sentCredentialMatches(server *coderv1alpha1.CoderMCPServer, field string, value credentialsecret.Value) bool { + sums, ok := r.sentCredentials.Load(server.UID) + if !ok { + return false + } + sum, ok := sums.(map[string][sha256.Size]byte)[field] + return ok && sum == sha256.Sum256([]byte(value.Reveal())) +} + // mcpServerReady reports a config that matches the spec. func mcpServerReady(server *coderv1alpha1.CoderMCPServer) *mcpServerStep { return &mcpServerStep{ diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index cffc297a7..cb4a4c942 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -6,6 +6,7 @@ import ( "fmt" "net/url" "strings" + "sync" "time" "unicode/utf8" @@ -52,6 +53,10 @@ const ( type CoderMCPServerReconciler struct { client.Client Scheme *runtime.Scheme + + // sentCredentials maps types.UID -> credential field -> SHA-256 of the + // value that Coder holds. Memory only (plan 4.6 item 4). + sentCredentials sync.Map } // mcpServerStep is the outcome of one reconcile. A ready step reports that @@ -230,7 +235,7 @@ func (r *CoderMCPServerReconciler) check( *before = *server.Status.DeepCopy() } var recreate bool - if step, recreate, err = syncMCPServer(ctx, sdk, server, org.ID, values); err != nil || !recreate { + if step, recreate, err = r.syncMCPServer(ctx, sdk, server, org.ID, values); err != nil || !recreate { break } } diff --git a/internal/controller/codermcpserver_create.go b/internal/controller/codermcpserver_create.go index de5fd6bc8..04f1d49ff 100644 --- a/internal/controller/codermcpserver_create.go +++ b/internal/controller/codermcpserver_create.go @@ -141,6 +141,11 @@ func (r *CoderMCPServerReconciler) createMCPServer( "The create answer does not match the request. The controller reads Coder instead of sending it again."), nil } server.Status.ConfigID, server.Status.CreateAttemptTime = created.ID.String(), nil + var sent []string + for _, c := range mcpServerCredentialRefs(server.Spec.Auth) { + sent = append(sent, c.field) + } + r.rememberSentCredentials(server, sent, values) return nil, nil case status == 0 && createHadNoEffect(err): // A failed dial or TLS handshake never sent the request. diff --git a/internal/controller/codermcpserver_sync_test.go b/internal/controller/codermcpserver_sync_test.go index 217b3e4ae..e260da3dc 100644 --- a/internal/controller/codermcpserver_sync_test.go +++ b/internal/controller/codermcpserver_sync_test.go @@ -2,6 +2,8 @@ package controller_test import ( "context" + "encoding/json" + "net/http" "slices" "testing" "time" @@ -9,9 +11,11 @@ import ( "github.com/coder/coder/v2/codersdk" "github.com/google/uuid" "github.com/stretchr/testify/require" + 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" ) // dashboard is a Coder client for the edits that an admin makes in the @@ -47,6 +51,10 @@ func (env mcpServerEnv) stored(t *testing.T, server *coderv1alpha1.CoderMCPServe return s } +func headerMCPSecret(ctx context.Context, t *testing.T, env mcpServerEnv) { + env.secret(ctx, t, "tracker-mcp", map[string]string{"x-api-key": testMCPKey}, testMCPOrigin) +} + func oauth2MCPSecret(ctx context.Context, t *testing.T, env mcpServerEnv) { env.secret(ctx, t, "tracker-oauth", map[string]string{"client-secret": testMCPClientSecret}, testMCPOrigin, testMCPAuthOrigin) } @@ -174,3 +182,240 @@ func TestCoderMCPServerSyncDeletedConfig(t *testing.T) { require.True(t, stored.view.Enabled) require.Equal(t, testMCPKey, stored.apiKeyValue) } + +// Plan S14: after a full apply, a dashboard disable, credential clear or +// auth type change is a kill switch, like the delete of +// TestCoderMCPServerSyncDeletedConfig. It gives Stalled=True and no write +// until the spec changes. A spec edit re-arms the object, and an apply that +// fails keeps the old applied generation. +func TestCoderMCPServerSyncKillSwitches(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name, reason string + edit codersdk.UpdateMCPServerConfigRequest + }{ + {name: "disable", reason: "DisabledExternally", edit: codersdk.UpdateMCPServerConfigRequest{Enabled: ptrTo(false)}}, + {name: "credential clear", reason: "CredentialClearedExternally", edit: codersdk.UpdateMCPServerConfigRequest{APIKeyValue: ptrTo("")}}, + {name: "auth type change", reason: "CredentialClearedExternally", edit: codersdk.UpdateMCPServerConfigRequest{AuthType: ptrTo("none")}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, tc.edit) + require.NoError(t, err) + mark := len(env.writeRoutes()) + for range 2 { + var result ctrl.Result + got, result = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, tc.reason) + require.Equal(t, 10*time.Minute, result.RequeueAfter, "no Kubernetes event follows a dashboard edit") + } + require.Empty(t, env.writesSince(mark), "a kill switch holds every write") + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Description = "re-armed" + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.Equal(t, got.Generation-1, got.Status.AppliedGeneration) + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + stored := env.stored(t, got) + require.True(t, stored.view.Enabled) + require.Equal(t, "api_key", stored.view.AuthType) + require.Equal(t, testMCPKey, stored.apiKeyValue) + require.Equal(t, "re-armed", stored.view.Description) + }) + } +} + +// Plan 4.7 item 4 and S19: a dashboard switch to user_oidc is always set +// back, also when it cleared the credentials. The one PATCH carries only +// auth fields, so a config that is also disabled stays disabled. +func TestCoderMCPServerSyncUserOIDC(t *testing.T) { + t.Parallel() + authFields := []string{ + "url", "auth_type", "api_key_header", "api_key_value", "custom_headers", "oauth2_client_id", "oauth2_client_secret", + "oauth2_auth_url", "oauth2_token_url", "oauth2_revocation_url", "oauth2_scopes", + } + for _, tc := range []struct { + name string + spec func(*coderv1alpha1.CoderMCPServerSpec) + secrets func(context.Context, *testing.T, mcpServerEnv) + disabled bool + movedURL bool // The dashboard also moves url to another origin. + }{ + {name: "none", spec: func(s *coderv1alpha1.CoderMCPServerSpec) { s.Auth = coderv1alpha1.CoderMCPServerAuth{Type: "none"} }}, + {name: "api key"}, + {name: "api key, also disabled", disabled: true}, + {name: "api key, also disabled and moved", disabled: true, movedURL: true}, + {name: "custom headers", secrets: headerMCPSecret, spec: func(s *coderv1alpha1.CoderMCPServerSpec) { useCustomHeaders(s, "X-Api-Key") }}, + {name: "oauth2", secrets: oauth2MCPSecret, spec: func(s *coderv1alpha1.CoderMCPServerSpec) { + useOAuth2(s, func(o *coderv1alpha1.CoderMCPServerOAuth2Auth) { o.Scopes = []string{"read"} }) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + if tc.secrets == nil { + tc.secrets = func(ctx context.Context, t *testing.T, env mcpServerEnv) { env.keySecret(ctx, t, testMCPKey) } + } + tc.secrets(ctx, t, env) + got, id := env.applied(ctx, t, tc.spec) + want := env.stored(t, got) + + edit := codersdk.UpdateMCPServerConfigRequest{AuthType: ptrTo("user_oidc")} + if tc.disabled { + edit.Enabled, want.view.Enabled = ptrTo(false), false + } + if tc.movedURL { + edit.URL = ptrTo("https://evil.example.net/mcp") + } + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, edit) + require.NoError(t, err) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.Equal(t, routeUpdateMCPServer, writes[0].route) + var body map[string]json.RawMessage + require.NoError(t, json.Unmarshal(writes[0].body, &body)) + for field := range body { + require.Contains(t, authFields, field, "the PATCH carries only auth fields") + } + require.JSONEq(t, `"`+got.Spec.Auth.Type+`"`, string(body["auth_type"])) + if tc.movedURL { + require.JSONEq(t, `"`+got.Spec.URL+`"`, string(body["url"]), "the credential goes only with the allowed url") + } + require.Equal(t, want, env.stored(t, got)) + if tc.disabled { + require.Contains(t, requireMCPServerStep(t, got, true, "DisabledExternally"), "set the auth type back from user_oidc") + } else { + requireMCPServerApplied(t, got) + } + + mark = len(env.writeRoutes()) + env.reconcile(ctx, t, got) + require.Empty(t, env.writesSince(mark)) + }) + } +} + +// Plan 4.6 item 4 and S13: a metadata-only Secret change moves the change +// token. Status records the new token and nothing is sent. A restart empties +// the in-memory hashes, so the next such change resends the value once. +func TestCoderMCPServerSyncSecretMetadataChange(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + secret := env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + touch := func(note string) { + t.Helper() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Annotations["example.com/note"] = note + require.NoError(t, k8sClient.Update(ctx, secret)) + } + + touch("one") + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Empty(t, env.writesSince(mark)) + require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) + + env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme} + touch("two") + mark = len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.JSONEq(t, `{"api_key_value":"`+testMCPKey+`"}`, string(writes[0].body)) + require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) +} + +// A change of the auth type clears the old credential in Coder, so its +// hash goes too, also when the answer of that PATCH was lost. Otherwise, +// after an auth type round trip, the hash would claim that Coder holds the +// Secret's value while Coder holds a key set in the dashboard. +func TestCoderMCPServerSyncAuthRoundTripResends(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + lost bool // The PATCH to none commits, but its answer is lost. + }{{name: "answer received"}, {name: "answer lost", lost: true}} { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + setAuth := func(auth coderv1alpha1.CoderMCPServerAuth) { + t.Helper() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Auth = auth + require.NoError(t, k8sClient.Update(ctx, got)) + } + original := got.Spec.Auth + setAuth(coderv1alpha1.CoderMCPServerAuth{Type: "none"}) + if tc.lost { + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway, AfterCommit: true}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + } + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, id, codersdk.UpdateMCPServerConfigRequest{ + AuthType: ptrTo("api_key"), APIKeyHeader: ptrTo("Authorization"), APIKeyValue: ptrTo("dashboard-key"), + }) + require.NoError(t, err) + setAuth(original) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, testMCPKey, env.stored(t, got).apiKeyValue) + }) + } +} + +// A credential PATCH can commit in Coder while its answer is lost. The +// controller then does not know which value Coder holds, so the retry sends +// the credential even when its value matches an earlier hash: here the +// Secret goes from A to B, the PATCH with B commits without an answer, and +// the Secret goes back to A. +func TestCoderMCPServerSyncLostAnswerResends(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + secret := env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + setKey := func(value string) { + t.Helper() + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Data["token"] = []byte(value) + require.NoError(t, k8sClient.Update(ctx, secret)) + } + + rotated := testMCPKey + "-rotated" + setKey(rotated) + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway, AfterCommit: true}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.Equal(t, rotated, env.stored(t, got).apiKeyValue, "the PATCH committed") + + setKey(testMCPKey) + mark := len(env.writeRoutes()) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + writes := env.writesSince(mark) + require.Len(t, writes, 1) + require.JSONEq(t, `{"api_key_value":"`+testMCPKey+`"}`, string(writes[0].body)) + require.Equal(t, testMCPKey, env.stored(t, got).apiKeyValue) +}