diff --git a/api/v1alpha1/codermcpserver_types.go b/api/v1alpha1/codermcpserver_types.go index c76871b36..7ccbd6cdd 100644 --- a/api/v1alpha1/codermcpserver_types.go +++ b/api/v1alpha1/codermcpserver_types.go @@ -133,9 +133,12 @@ type CoderMCPServerSpec struct { // Auth is how Coder authenticates to the server. Auth CoderMCPServerAuth `json:"auth"` // Access, when set, makes the controller own the server's access list in - // Coder: exactly the listed groups can read the server. When omitted, the - // controller leaves the access list alone, and Coder's default lets every - // organization member read the server. + // Coder: exactly the listed groups can read the server. Every sync + // therefore also removes each user entry, for example a user that an admin + // added in the Coder dashboard. Coder does not show the entry of a deleted + // group, so the sync cannot remove it, but it grants nothing. + // When omitted, the controller leaves the access list alone, and Coder's + // default lets every organization member read the server. // +optional Access *CoderMCPServerAccess `json:"access,omitempty"` } diff --git a/config/crd/dormant/coder.com_codermcpservers.yaml b/config/crd/dormant/coder.com_codermcpservers.yaml index 03333c966..334fcfd38 100644 --- a/config/crd/dormant/coder.com_codermcpservers.yaml +++ b/config/crd/dormant/coder.com_codermcpservers.yaml @@ -65,9 +65,12 @@ spec: access: description: |- Access, when set, makes the controller own the server's access list in - Coder: exactly the listed groups can read the server. When omitted, the - controller leaves the access list alone, and Coder's default lets every - organization member read the server. + Coder: exactly the listed groups can read the server. Every sync + therefore also removes each user entry, for example a user that an admin + added in the Coder dashboard. Coder does not show the entry of a deleted + group, so the sync cannot remove it, but it grants nothing. + When omitted, the controller leaves the access list alone, and Coder's + default lets every organization member read the server. properties: groups: description: |- diff --git a/internal/controller/codermcpserver_acl.go b/internal/controller/codermcpserver_acl.go index 37f317d5f..01c9e1be5 100644 --- a/internal/controller/codermcpserver_acl.go +++ b/internal/controller/codermcpserver_acl.go @@ -34,6 +34,23 @@ var mcpServerGroupNamePattern = regexp.MustCompile(`^[a-zA-Z0-9]+(?:-[a-zA-Z0-9] // It returns nil when the ACL matches. func syncMCPServerACL( ctx context.Context, sdk *codersdk.Client, orgID, configID uuid.UUID, access *coderv1alpha1.CoderMCPServerAccess, +) (*mcpServerStep, error) { + return syncMCPServerACLPass(ctx, sdk, orgID, configID, access, false) +} + +// narrowMCPServerACL removes every ACL entry that access does not list, and +// adds nothing. A caller that changes the config runs it before the change +// and syncMCPServerACL after it. Then no principal outside both the old and +// the new access list can reach the config at any point, also when the +// change fails. It returns nil when nothing is left to remove. +func narrowMCPServerACL( + ctx context.Context, sdk *codersdk.Client, orgID, configID uuid.UUID, access *coderv1alpha1.CoderMCPServerAccess, +) (*mcpServerStep, error) { + return syncMCPServerACLPass(ctx, sdk, orgID, configID, access, true) +} + +func syncMCPServerACLPass( + ctx context.Context, sdk *codersdk.Client, orgID, configID uuid.UUID, access *coderv1alpha1.CoderMCPServerAccess, removeOnly bool, ) (*mcpServerStep, error) { if sdk == nil || orgID == uuid.Nil || configID == uuid.Nil { return nil, fmt.Errorf("assertion failed: MCP server ACL sync needs a Coder client, an organization ID, and a config ID") @@ -62,8 +79,10 @@ func syncMCPServerACL( } delete(want, g.ID) } - for id := range want { - patch.GroupRoles[id.String()] = codersdk.MCPServerConfigRoleRead + if !removeOnly { + for id := range want { + patch.GroupRoles[id.String()] = codersdk.MCPServerConfigRoleRead + } } // access lists groups only, so every user entry goes. for _, u := range acl.Users { diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go new file mode 100644 index 000000000..d023c0db7 --- /dev/null +++ b/internal/controller/codermcpserver_apply.go @@ -0,0 +1,181 @@ +package controller + +import ( + "context" + "fmt" + "net/http" + "strings" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/credentialsecret" +) + +// applyMCPServer sets the spec on the config that this object created (plan +// 4.5 create steps 5 and 6). It reads the config, removes the ACL entries +// that spec.access does not list, sends one PATCH with every spec field and +// every credential of the auth type, and only then adds the missing groups. +// So while the PATCH changes or enables the config, the ACL holds only groups +// that both the old and the new access list allow (plan S20), and a failed +// PATCH leaves the ACL narrower, never broader. Every PATCH sets url, so it +// always carries the credentials (plan 4.6 item 5). It returns a ready step +// and sets status.appliedGeneration on success. +func applyMCPServer( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, values map[string]credentialsecret.Value, +) (*mcpServerStep, error) { + id, err := uuid.Parse(server.Status.ConfigID) + if err != nil || id == uuid.Nil { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s has status.configID %q, which is not a UUID", server.Namespace, server.Name, server.Status.ConfigID) + } + current, err := sdk.MCPServerConfigByID(ctx, orgID, id) + switch { + case isCoderNotFound(err): + // Re-creating it after a spec change comes with the update path. + return mcpServerStalled(0, "DeletedExternally", "MCP server config %s no longer exists in Coder. This controller version does not create it again.", id), nil + case err != nil: + return mcpServerWait(mcpServerRetry, "CoderUnavailable", "get MCP server config %s: %s", id, coderErrorSummary(err)), nil + } + if current.ID != id || current.OrganizationID != orgID || current.Slug != server.Spec.Slug { + return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", + "assertion failed: Coder answered MCP server config %s with slug %s in organization %s when asked for %s", current.ID, current.Slug, current.OrganizationID, id), nil + } + if step, err := narrowMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { + return step, err + } + req, err := mcpServerUpdateRequest(server, values) + if err != nil { + return nil, err + } + updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, req) + switch status := coderStatus(err); { + case status == http.StatusBadRequest: + return mcpServerStalled(0, "CoderRejected", "Coder rejected the update: %s", coderErrorSummary(err)), nil + case err != nil: + // The PATCH sets absolute values, so sending it again is safe. This + // includes a 404: a proxy can put one in place of a committed answer, + // 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)), nil + } + if field := mcpServerUpdateMismatch(id, current.AuthType, req, updated); field != "" { + return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered the update of MCP server config %s with another %s", id, field), nil + } + if step, err := syncMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { + return step, err + } + server.Status.Credentials = mcpServerCredentialStatus(server.Spec.Auth, values) + server.Status.AppliedGeneration = server.Generation + return mcpServerReady(server), nil +} + +// mcpServerUpdateMismatch returns the first field of the update answer that +// differs from the request, or "". Coder returns every field the request +// sets, except the credential values, for which it returns a has_* flag. A +// changed auth type makes Coder clear the credentials of the other types +// (plan F9), so their flags must then be false. +func mcpServerUpdateMismatch(id uuid.UUID, oldAuthType string, req codersdk.UpdateMCPServerConfigRequest, got codersdk.MCPServerConfig) string { + if got.ID != id { + return "ID" + } + strs := []struct { + name string + want *string + got string + }{ + {"display_name", req.DisplayName, got.DisplayName}, + {"description", req.Description, got.Description}, + {"icon_url", req.IconURL, got.IconURL}, + {"transport", req.Transport, got.Transport}, + {"url", req.URL, got.URL}, + {"auth_type", req.AuthType, got.AuthType}, + {"availability", req.Availability, got.Availability}, + {"api_key_header", req.APIKeyHeader, got.APIKeyHeader}, + {"oauth2_client_id", req.OAuth2ClientID, got.OAuth2ClientID}, + {"oauth2_auth_url", req.OAuth2AuthURL, got.OAuth2AuthURL}, + {"oauth2_token_url", req.OAuth2TokenURL, got.OAuth2TokenURL}, + {"oauth2_revocation_url", req.OAuth2RevocationURL, got.OAuth2RevocationURL}, + {"oauth2_scopes", req.OAuth2Scopes, got.OAuth2Scopes}, + } + for _, f := range strs { + if f.want != nil && *f.want != f.got { + return f.name + } + } + bools := []struct { + name string + want *bool + got bool + }{ + {"enabled", req.Enabled, got.Enabled}, + {"model_intent", req.ModelIntent, got.ModelIntent}, + {"allow_in_plan_mode", req.AllowInPlanMode, got.AllowInPlanMode}, + {"forward_coder_headers", req.ForwardCoderHeaders, got.ForwardCoderHeaders}, + } + for _, f := range bools { + if f.want != nil && *f.want != f.got { + return f.name + } + } + switch { + case req.APIKeyValue != nil && !got.HasAPIKey: + return "has_api_key" + case req.CustomHeaders != nil && !got.HasCustomHeaders: + return "has_custom_headers" + case req.OAuth2ClientSecret != nil && !got.HasOAuth2Secret: + return "has_oauth2_secret" + } + if req.AuthType != nil && *req.AuthType != oldAuthType { + switch { + case req.APIKeyValue == nil && got.HasAPIKey: + return "has_api_key" + case req.CustomHeaders == nil && got.HasCustomHeaders: + return "has_custom_headers" + case req.OAuth2ClientSecret == nil && got.HasOAuth2Secret: + return "has_oauth2_secret" + } + } + return "" +} + +// mcpServerReady reports a config that matched the spec when this generation +// was applied. This version reads Coder only to apply a new generation. +func mcpServerReady(server *coderv1alpha1.CoderMCPServer) *mcpServerStep { + return &mcpServerStep{ + ready: true, reason: "Applied", requeue: mcpServerResync, + message: fmt.Sprintf("MCP server config %s matches generation %d of the spec.", server.Status.ConfigID, server.Status.AppliedGeneration), + } +} + +// mcpServerUpdateRequest sets every spec field and the credentials of the +// auth type. Fields of the other auth types stay unset: a changed auth type +// makes Coder clear them (plan F9). The tool lists stay unset until the spec +// has them. Credentials are revealed only here, into the request body. +func mcpServerUpdateRequest(server *coderv1alpha1.CoderMCPServer, values map[string]credentialsecret.Value) (codersdk.UpdateMCPServerConfigRequest, error) { + create, err := mcpServerCreateRequest(server, values) + if err != nil { + return codersdk.UpdateMCPServerConfigRequest{}, err + } + spec := server.Spec + enabled := spec.Enabled == nil || *spec.Enabled + req := codersdk.UpdateMCPServerConfigRequest{ + DisplayName: &spec.DisplayName, Description: &create.Description, IconURL: &create.IconURL, + Transport: &create.Transport, URL: &create.URL, AuthType: &create.AuthType, Availability: &create.Availability, + Enabled: &enabled, ModelIntent: &create.ModelIntent, AllowInPlanMode: &create.AllowInPlanMode, ForwardCoderHeaders: &create.ForwardCoderHeaders, + } + switch spec.Auth.Type { + case "api_key": + req.APIKeyHeader, req.APIKeyValue = &create.APIKeyHeader, &create.APIKeyValue + case "custom_headers": + req.CustomHeaders = &create.CustomHeaders + case "oauth2": + req.OAuth2ClientID, req.OAuth2ClientSecret = &create.OAuth2ClientID, &create.OAuth2ClientSecret + req.OAuth2AuthURL, req.OAuth2TokenURL = &create.OAuth2AuthURL, &create.OAuth2TokenURL + // "" clears a revocation URL that an earlier generation set. + req.OAuth2RevocationURL, req.OAuth2Scopes = &create.OAuth2RevocationURL, &create.OAuth2Scopes + } + if strings.TrimSpace(*req.DisplayName) == "" { + return req, fmt.Errorf("assertion failed: MCP server %s/%s has an empty display name after admission", server.Namespace, server.Name) + } + return req, nil +} diff --git a/internal/controller/codermcpserver_apply_test.go b/internal/controller/codermcpserver_apply_test.go new file mode 100644 index 000000000..16b35f3e7 --- /dev/null +++ b/internal/controller/codermcpserver_apply_test.go @@ -0,0 +1,382 @@ +package controller_test + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "testing" + "time" + + "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" +) + +// writeRoutes returns the routes of the fake's recorded write requests, in +// request order. +func (env mcpServerEnv) writeRoutes() []string { + env.fake.mu.Lock() + defer env.fake.mu.Unlock() + var out []string + for _, b := range env.fake.mcp.bodies { + out = append(out, b.route) + } + return out +} + +func (env mcpServerEnv) lastUpdate(t *testing.T) codersdk.UpdateMCPServerConfigRequest { + t.Helper() + bodies := env.fake.mcpRequestBodies(routeUpdateMCPServer) + require.NotEmpty(t, bodies) + var req codersdk.UpdateMCPServerConfigRequest + require.NoError(t, json.Unmarshal(bodies[len(bodies)-1], &req)) + return req +} + +// Plan S20 and 4.5 steps 5 and 6: the config is created disabled with +// Coder's default Everyone entry. The entries that spec.access does not list +// go first, then one PATCH enables the config with the spec values, and only +// then are the missing groups added. So the enabled config never has a group +// that the spec does not list. The PATCH sets url, so it carries the +// credential too. +func TestCoderMCPServerApplyOrder(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + data := env.fake.addGroup(env.orgID, "data", "") + got, result := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"data"}} + })) + requireMCPServerApplied(t, got) + require.Equal(t, 10*time.Minute, result.RequeueAfter) + require.Equal(t, []string{routeCreateMCPServer, routeUpdateMCPServerACL, routeUpdateMCPServer, routeUpdateMCPServerACL}, env.writeRoutes(), + "create, remove Everyone, enable, then add data") + acls := env.fake.mcpRequestBodies(routeUpdateMCPServerACL) + require.JSONEq(t, `{"group_roles":{"`+env.orgID.String()+`":""}}`, string(acls[0]), "the first ACL write only removes") + require.JSONEq(t, `{"group_roles":{"`+data.String()+`":"read"}}`, string(acls[1])) + stored := env.requireCreated(t, got) + require.True(t, stored.view.Enabled) + require.Equal(t, "GitHub", stored.view.DisplayName) + require.Equal(t, testMCPKey, stored.apiKeyValue) + groups, users := env.fake.mcpACL(stored.view.ID) + require.Equal(t, []uuid.UUID{data}, groups) + require.Empty(t, users) + req := env.lastUpdate(t) + require.Equal(t, testMCPKey, *req.APIKeyValue, "a PATCH that sets url carries the credential") + require.Equal(t, "https://mcp.example.com/github/mcp", *req.URL) + require.Len(t, got.Status.Credentials, 1) + + before := env.mcpRequests() + again, _ := env.reconcile(ctx, t, got) + require.Equal(t, before, env.mcpRequests(), "an applied generation sends nothing") + require.Equal(t, got.ResourceVersion, again.ResourceVersion) +} + +// A new generation that moves an enabled config to other groups and also +// disables or changes it: when the PATCH fails, the ACL holds only groups +// that both generations allow, so no newly listed group can use the old +// configuration. +func TestCoderMCPServerApplyNeverBroadensBeforeTheChange(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + edit func(*coderv1alpha1.CoderMCPServerSpec) + fault fakeFault + }{ + {name: "disable rejected", edit: func(s *coderv1alpha1.CoderMCPServerSpec) { s.Enabled = ptrTo(false) }, fault: fakeFault{Status: http.StatusBadRequest}}, + {name: "URL change fails", edit: func(s *coderv1alpha1.CoderMCPServerSpec) { s.URL = "https://mcp.example.com/v2/mcp" }, fault: fakeFault{Status: http.StatusBadGateway}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + data := env.fake.addGroup(env.orgID, "data", "") + env.fake.addGroup(env.orgID, "ops", "") + got, _ := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"Everyone", "data"}} + })) + requireMCPServerApplied(t, got) + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Access.Groups = []string{"data", "ops"} + tc.edit(&got.Spec) + require.NoError(t, k8sClient.Update(ctx, got)) + env.fake.failNext(routeUpdateMCPServer, tc.fault) + got, _ = env.reconcile(ctx, t, got) + require.NotEqual(t, got.Generation, got.Status.AppliedGeneration) + groups, _ := env.fake.mcpACL(env.requireCreated(t, got).view.ID) + require.Equal(t, []uuid.UUID{data}, groups, "Everyone is gone, ops is not added yet") + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + }) + } +} + +// Plan S15: access with no groups and enabled false reach Coder as an empty +// ACL on a disabled config. Without access, the controller never touches the +// ACL, so Coder's default Everyone entry stays. +func TestCoderMCPServerApplyAccess(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + access *coderv1alpha1.CoderMCPServerAccess + enabled bool + acl bool // The ACL routes are called. + }{ + {name: "no groups, disabled", access: &coderv1alpha1.CoderMCPServerAccess{Groups: []string{}}, acl: true}, + {name: "access omitted", enabled: 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, _ := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access, s.Enabled = tc.access, ptrTo(tc.enabled) + })) + requireMCPServerApplied(t, got) + stored := env.requireCreated(t, got) + require.Equal(t, tc.enabled, stored.view.Enabled) + groups, _ := env.fake.mcpACL(stored.view.ID) + aclCalls := env.fake.requestCount(routeMCPServerACL) + env.fake.requestCount(routeUpdateMCPServerACL) + if tc.acl { + require.Empty(t, groups, "no Everyone entry") + require.NotZero(t, aclCalls) + } else { + require.Equal(t, []uuid.UUID{env.orgID}, groups) + require.Zero(t, aclCalls) + } + }) + } +} + +// A group that does not resolve keeps the new config disabled with the nonce, +// and sends no PATCH. Once the group exists, the next reconcile applies. +func TestCoderMCPServerApplyGroupNotFound(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"data"}} + })) + requireMCPServerStep(t, got, true, "GroupNotFound") + stored := env.requireCreated(t, got) + require.False(t, stored.view.Enabled) + require.Equal(t, "coder-k8s:"+string(got.UID), stored.view.DisplayName) + require.Zero(t, env.fake.requestCount(routeUpdateMCPServer)+env.fake.requestCount(routeUpdateMCPServerACL)) + require.Zero(t, got.Status.AppliedGeneration) + + env.fake.addGroup(env.orgID, "data", "") + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.True(t, env.requireCreated(t, got).view.Enabled) +} + +// rewriteUpdate changes the update answer after Coder applied the update. +func rewriteUpdate(edit func(*codersdk.MCPServerConfig)) fakeFault { + return fakeFault{AfterCommit: true, Rewrite: func(answer any) any { + c := answer.(codersdk.MCPServerConfig) + edit(&c) + return c + }} +} + +// Each failure of the apply leaves the generation unapplied, sends no other +// write, and never puts a credential into status or logs. +func TestCoderMCPServerApplyFailures(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + route string + fault fakeFault + reason string + stalled bool + retry bool // The next reconcile applies the spec. + }{ + {name: "config gone", route: routeMCPServer, fault: fakeFault{Status: http.StatusNotFound}, reason: "DeletedExternally", stalled: true}, + {name: "read fails", route: routeMCPServer, fault: fakeFault{Status: http.StatusServiceUnavailable}, reason: "CoderUnavailable", retry: true}, + {name: "read answers another slug", route: routeMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: fakeFault{Rewrite: func(answer any) any { + c := answer.(codersdk.MCPServerConfig) + c.Slug = "other" + return c + }}}, + {name: "update fails", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusInternalServerError, Detail: "upstream " + testMCPKey}, reason: "CoderUnavailable", retry: true}, + {name: "update rejected", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusBadRequest, Detail: "rejected " + testMCPKey}, reason: "CoderRejected", stalled: true}, + {name: "update answers 404", route: routeUpdateMCPServer, fault: fakeFault{Status: http.StatusNotFound}, reason: "CoderUnavailable", retry: true}, + {name: "update answers another enabled", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.Enabled = false })}, + {name: "update answers another description", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.Description = "x" })}, + {name: "update answers another availability", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.Availability = "force_on" })}, + {name: "update answers another flag", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.ForwardCoderHeaders = true })}, + {name: "update answers another header", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.APIKeyHeader = "X-Other" })}, + {name: "update answers no key", route: routeUpdateMCPServer, reason: "CoderAnswerMismatch", stalled: true, fault: rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.HasAPIKey = false })}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(tc.route, tc.fault) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, tc.stalled, tc.reason) + require.Zero(t, got.Status.AppliedGeneration) + require.NotEmpty(t, got.Status.ConfigID, "the ID is stored before the apply") + require.Equal(t, 1, env.posts()) + if tc.retry { + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.True(t, env.requireCreated(t, got).view.Enabled) + } + }) + } +} + +// A spec change after the apply sends one PATCH with the new values. It sets +// url, so it carries the current credential (plan 4.6 item 5), and status +// records that credential's change token. +func TestCoderMCPServerApplySpecChange(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + secret := env.keySecret(ctx, t, testMCPKey) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerApplied(t, got) + + rotated := testMCPKey + "-rotated" + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Data["token"] = []byte(rotated) + require.NoError(t, k8sClient.Update(ctx, secret)) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.URL, got.Spec.Description = "https://mcp.example.com/v2/mcp", "Issues" + require.NoError(t, k8sClient.Update(ctx, got)) + before := env.fake.requestCount(routeUpdateMCPServer) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, 1, env.fake.requestCount(routeUpdateMCPServer)-before) + require.Equal(t, 1, env.posts()) + stored := env.requireCreated(t, got) + require.Equal(t, "https://mcp.example.com/v2/mcp", stored.view.URL) + require.Equal(t, "Issues", stored.view.Description) + require.Equal(t, rotated, *env.lastUpdate(t).APIKeyValue) + require.Equal(t, rotated, stored.apiKeyValue) + require.Len(t, got.Status.Credentials, 1) + require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) +} + +// appliedStatusFailClient fails the status write that records an applied +// generation, like a crash after the PATCH. +type appliedStatusFailClient struct{ client.Client } + +func (c appliedStatusFailClient) Status() client.SubResourceWriter { + return appliedStatusFailWriter{c.Client.Status()} +} + +type appliedStatusFailWriter struct{ client.SubResourceWriter } + +func (w appliedStatusFailWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if s, ok := obj.(*coderv1alpha1.CoderMCPServer); ok && s.Status.AppliedGeneration != 0 { + return errors.New("lost status write") + } + return w.SubResourceWriter.Update(ctx, obj, opts...) +} + +// The PATCH replaces the nonce, so the config ID is stored before it. When +// the status write after the PATCH is lost, the next reconcile still knows +// the config and applies again, instead of reporting its own config as a +// slug conflict. +func TestCoderMCPServerApplyLostStatusWrite(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + r := &controller.CoderMCPServerReconciler{Client: appliedStatusFailClient{k8sClient}, Scheme: scheme} + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(server)}) + require.ErrorContains(t, err, "lost status write") + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(server), server)) + require.NotEmpty(t, server.Status.ConfigID) + require.Zero(t, server.Status.AppliedGeneration) + require.Equal(t, "GitHub", env.requireCreated(t, server).view.DisplayName, "the PATCH replaced the nonce") + + got, _ := env.reconcile(ctx, t, server) + requireMCPServerApplied(t, got) + require.Equal(t, 1, env.posts()) + require.Equal(t, 2, env.fake.requestCount(routeUpdateMCPServer)) +} + +// A Secret change moves its change token but not the generation. The next +// reconcile sends the new value in one PATCH and records the new token, and +// the one after sends nothing. +func TestCoderMCPServerApplySecretRotation(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + secret := env.keySecret(ctx, t, testMCPKey) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerApplied(t, got) + + rotated := testMCPKey + "-rotated" + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), secret)) + secret.Data["token"] = []byte(rotated) + require.NoError(t, k8sClient.Update(ctx, secret)) + before := env.fake.requestCount(routeUpdateMCPServer) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + require.Equal(t, 1, env.fake.requestCount(routeUpdateMCPServer)-before) + require.Equal(t, rotated, env.requireCreated(t, got).apiKeyValue) + require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) + + before = env.mcpRequests() + again, _ := env.reconcile(ctx, t, got) + require.Equal(t, before, env.mcpRequests(), "a recorded token sends nothing") + require.Equal(t, got.ResourceVersion, again.ResourceVersion) +} + +// A spec change of the auth type relies on Coder clearing the old type's +// credential. An answer that still holds it is not accepted. +func TestCoderMCPServerApplyAuthTypeChange(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + fault *fakeFault + }{ + {name: "old key cleared"}, + {name: "old key kept", fault: ptrTo(rewriteUpdate(func(c *codersdk.MCPServerConfig) { c.HasAPIKey = 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, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerApplied(t, got) + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Auth = coderv1alpha1.CoderMCPServerAuth{Type: "none"} + require.NoError(t, k8sClient.Update(ctx, got)) + if tc.fault != nil { + env.fake.failNext(routeUpdateMCPServer, *tc.fault) + } + got, _ = env.reconcile(ctx, t, got) + stored := env.requireCreated(t, got) + require.Equal(t, "none", stored.view.AuthType) + if tc.fault == nil { + requireMCPServerApplied(t, got) + require.Empty(t, stored.apiKeyValue, "Coder cleared the key") + require.Empty(t, got.Status.Credentials) + return + } + require.Contains(t, requireMCPServerStep(t, got, true, "CoderAnswerMismatch"), "has_api_key") + require.NotEqual(t, got.Generation, got.Status.AppliedGeneration) + }) + } +} diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 72375da88..74cfdea3a 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/url" + "slices" "strings" "time" "unicode/utf8" @@ -184,9 +185,10 @@ func (r *CoderMCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Reque } // check gets Coder access, resolves and pins the organization, applies the -// control plane's opt-in, reads the credentials, and then creates the config -// or records the one that this object created. Every Coder call before the -// slug lookup is an organization lookup. +// control plane's opt-in, reads the credentials, creates the config or +// records the one that this object created, and applies a new generation of +// the spec. Every Coder call before the slug lookup is an organization +// lookup. func (r *CoderMCPServerReconciler) check( ctx context.Context, server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus, ) (*mcpServerStep, error) { @@ -228,9 +230,16 @@ func (r *CoderMCPServerReconciler) check( *before = *server.Status.DeepCopy() } } - if step == nil { - step = mcpServerWait(mcpServerResync, "Pending", - "MCP server config %s exists in Coder, disabled. This controller version does not apply the spec yet.", server.Status.ConfigID) + switch { + case step != nil: + case server.Status.AppliedGeneration == server.Generation && + slices.Equal(server.Status.Credentials, mcpServerCredentialStatus(server.Spec.Auth, values)): + // A Secret change moves its change token without a new generation. + step = mcpServerReady(server) + default: + if step, err = applyMCPServer(ctx, sdk, server, org.ID, values); err != nil { + return nil, err + } } // Coder messages can echo the request, so scrub the credentials from them. all := make([]credentialsecret.Value, 0, len(values)) diff --git a/internal/controller/codermcpserver_controller_test.go b/internal/controller/codermcpserver_controller_test.go index 134c08170..ea5b18262 100644 --- a/internal/controller/codermcpserver_controller_test.go +++ b/internal/controller/codermcpserver_controller_test.go @@ -122,12 +122,17 @@ func (env mcpServerEnv) mcpRequests() int { return env.fake.totalRequests() - env.fake.requestCount(routeOrganization) } -// requireMCPServerStep checks the kstatus conditions of a wait or a stall and -// returns the message. +// requireMCPServerStep checks the kstatus conditions of a wait, a stall, or +// the ready reason Applied, and returns the message. func requireMCPServerStep(t *testing.T, server *coderv1alpha1.CoderMCPServer, stalled bool, reason string) string { t.Helper() want := map[string]metav1.ConditionStatus{"Ready": metav1.ConditionFalse, "Reconciling": metav1.ConditionTrue, "Stalled": metav1.ConditionFalse} - if stalled { + switch { + case reason == "Applied": + require.False(t, stalled) + want["Ready"], want["Reconciling"] = metav1.ConditionTrue, metav1.ConditionFalse + require.Equal(t, server.Generation, server.Status.AppliedGeneration) + case stalled: want["Reconciling"], want["Stalled"] = metav1.ConditionFalse, metav1.ConditionTrue } for conditionType, status := range want { @@ -251,10 +256,11 @@ func TestCoderMCPServerCredentialChecks(t *testing.T) { } got, result := env.reconcile(ctx, t, env.create(ctx, t, tc.spec)) if tc.reason == "" { - require.Contains(t, requireMCPServerStep(t, got, false, "Pending"), "exists in Coder, disabled.") + requireMCPServerStep(t, got, false, "Applied") require.Equal(t, 1, env.fake.requestCount(routeMCPServers), "usable credentials reach the slug lookup") require.Equal(t, 1, env.fake.requestCount(routeCreateMCPServer)) - require.Equal(t, 2, env.mcpRequests()) + require.Equal(t, 1, env.fake.requestCount(routeUpdateMCPServer)) + require.Equal(t, 4, env.mcpRequests(), "list, create, get and update") require.Equal(t, 10*time.Minute, result.RequeueAfter) return } @@ -322,15 +328,15 @@ func TestCoderMCPServerControlPlaneAndOptIn(t *testing.T) { }{ {optIn: nil, reason: "MCPServersNotAllowed", lookups: 1}, {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"other", "missing"}}, reason: "MCPServersNotAllowed", lookups: 3}, - {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"CODER"}}, reason: "Pending", lookups: 1}, - {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"other", "coder"}}, reason: "Pending", lookups: 2}, + {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"CODER"}}, reason: "Applied", lookups: 1}, + {optIn: &coderv1alpha1.MCPServersSpec{Organizations: []string{"other", "coder"}}, reason: "Applied", lookups: 2}, } { env.setOptIn(ctx, t, tc.optIn) orgLookups, mcp := env.fake.requestCount(routeOrganization), env.mcpRequests() got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, tc.reason != "Pending", tc.reason) + requireMCPServerStep(t, got, tc.reason != "Applied", tc.reason) require.Equal(t, tc.lookups, env.fake.requestCount(routeOrganization)-orgLookups, tc.reason) - if tc.reason != "Pending" { + if tc.reason != "Applied" { require.Equal(t, mcp, env.mcpRequests(), "only organization lookups happen") } } @@ -352,7 +358,7 @@ func TestCoderMCPServerControlPlaneAndOptIn(t *testing.T) { require.Equal(t, mcp, env.mcpRequests()) env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}, AllowForceOn: true}) got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, false, "Pending") + requireMCPServerStep(t, got, false, "Applied") } // The organization ID is pinned at the first resolution. A later resolution @@ -371,7 +377,7 @@ func TestCoderMCPServerOrganizationPinning(t *testing.T) { first := env.fake.addOrganization("data") got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, false, "Pending") + requireMCPServerStep(t, got, false, "Applied") require.Equal(t, first.String(), got.Status.OrganizationID) // The organization is deleted and another one takes its name. @@ -504,7 +510,7 @@ func TestCoderMCPServerSecretChangesDuringReconcile(t *testing.T) { require.Zero(t, env.mcpRequests()) got, _ = env.reconcile(ctx, t, server) - requireMCPServerStep(t, got, false, "Pending") + requireMCPServerStep(t, got, false, "Applied") } // Plan S15: access with no groups and enabled false survive the finalizer @@ -579,7 +585,7 @@ func TestCoderMCPServerWatches(t *testing.T) { return condition != nil && condition.Reason == reason }, 30*time.Second, 100*time.Millisecond, reason) } - requireReason("Pending") + requireReason("Applied") require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(tenant), tenant)) tenant.Labels = nil @@ -587,19 +593,19 @@ func TestCoderMCPServerWatches(t *testing.T) { requireReason(credentialsecret.ReasonNotOptedIn) tenant.Labels = map[string]string{credentialsecret.LabelKey: credentialsecret.LabelValue} require.NoError(t, k8sClient.Update(ctx, tenant)) - requireReason("Pending") + requireReason("Applied") env.setOptIn(ctx, t, nil) requireReason("MCPServersNotAllowed") env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) - requireReason("Pending") + requireReason("Applied") // Another control plane that uses a credential Secret for its own access // makes it unusable. Deleting that control plane makes it usable again. other := createControlPlaneUsing(ctx, t, env.namespace, "license", tenant.Name) requireReason(credentialsecret.ReasonRefused) require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Name: other, Namespace: env.namespace}})) - requireReason("Pending") + requireReason("Applied") require.NoError(t, k8sClient.Delete(ctx, key)) requireReason(credentialsecret.ReasonSecretNotFound) diff --git a/internal/controller/codermcpserver_create_test.go b/internal/controller/codermcpserver_create_test.go index 0d5b50b2c..0832f63ab 100644 --- a/internal/controller/codermcpserver_create_test.go +++ b/internal/controller/codermcpserver_create_test.go @@ -48,11 +48,10 @@ func (env mcpServerEnv) requireCreated(t *testing.T, server *coderv1alpha1.Coder return own[0] } -// requireMCPServerCreatedStep checks the reason after a create in this -// version, which does not apply the spec yet. -func requireMCPServerCreatedStep(t *testing.T, server *coderv1alpha1.CoderMCPServer) { +// requireMCPServerApplied checks that the spec was applied after the create. +func requireMCPServerApplied(t *testing.T, server *coderv1alpha1.CoderMCPServer) { t.Helper() - require.Contains(t, requireMCPServerStep(t, server, false, "Pending"), "exists in Coder, disabled.") + require.Contains(t, requireMCPServerStep(t, server, false, "Applied"), "matches generation") } // Plan 4.5 create steps 1 to 3 for every auth type: one create, disabled with @@ -125,7 +124,7 @@ func TestCoderMCPServerCreate(t *testing.T) { base := env.posts() // Creates of the test setup. got, result := env.reconcile(ctx, t, env.create(ctx, t, tc.spec)) stored := env.requireCreated(t, got) - requireMCPServerCreatedStep(t, got) + requireMCPServerApplied(t, got) require.Equal(t, 1, env.posts()-base) require.Equal(t, 10*time.Minute, result.RequeueAfter) require.Equal(t, "github", stored.view.Slug) @@ -145,7 +144,7 @@ func TestCoderMCPServerCreate(t *testing.T) { before := env.mcpRequests() again, _ := env.reconcile(ctx, t, got) - require.Equal(t, before, env.mcpRequests(), "a recorded config needs no MCP request in this version") + require.Equal(t, before, env.mcpRequests(), "an applied generation needs no MCP request in this version") require.Equal(t, got.ResourceVersion, again.ResourceVersion) }) } @@ -175,7 +174,7 @@ func TestCoderMCPServerCreateAnswers(t *testing.T) { }}, reason: "ConfirmingCreate", requeue: 5 * time.Second, marker: true, posts: 1}, {name: "429", fault: fakeFault{Status: http.StatusTooManyRequests}, reason: "CreateRetrying", requeue: 30 * time.Second, posts: 2}, {name: "401", fault: fakeFault{Status: http.StatusUnauthorized}, reason: "CreateRetrying", requeue: 30 * time.Second, posts: 2}, - {name: "400 after commit", fault: fakeFault{Status: http.StatusBadRequest, AfterCommit: true}, reason: "Pending", requeue: 10 * time.Minute, posts: 1}, + {name: "400 after commit", fault: fakeFault{Status: http.StatusBadRequest, AfterCommit: true}, reason: "Applied", requeue: 10 * time.Minute, posts: 1}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -189,12 +188,12 @@ func TestCoderMCPServerCreateAnswers(t *testing.T) { require.Equal(t, tc.requeue, result.RequeueAfter) require.Equal(t, 1, env.posts()) require.Equal(t, tc.marker, got.Status.CreateAttemptTime != nil, "create marker") - if tc.reason != "Pending" { + if tc.reason != "Applied" { require.Empty(t, got.Status.ConfigID) got, _ = env.reconcile(ctx, t, got) } env.requireCreated(t, got) - requireMCPServerCreatedStep(t, got) + requireMCPServerApplied(t, got) require.Equal(t, tc.posts, env.posts()) }) } @@ -233,7 +232,7 @@ func TestCoderMCPServerCreateRejected(t *testing.T) { require.NoError(t, k8sClient.Update(ctx, got)) got, _ = env.reconcile(ctx, t, got) env.requireCreated(t, got) - requireMCPServerCreatedStep(t, got) + requireMCPServerApplied(t, got) require.Equal(t, 2, env.posts()) } @@ -269,8 +268,8 @@ func TestCoderMCPServerCreateRecovery(t *testing.T) { reason string posts int }{ - {name: "create committed", existing: true, reason: "Pending"}, - {name: "nothing committed", reason: "Pending", posts: 1}, + {name: "create committed", existing: true, reason: "Applied"}, + {name: "nothing committed", reason: "Applied", posts: 1}, {name: "nothing visible to a non-owner", notOwner: true, reason: "ConfirmingCreate"}, } { t.Run(tc.name, func(t *testing.T) { @@ -296,9 +295,9 @@ func TestCoderMCPServerCreateRecovery(t *testing.T) { env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme} base := env.posts() // Creates of the test setup. got, _ := env.reconcile(ctx, t, server) - if tc.reason == "Pending" { + if tc.reason == "Applied" { env.requireCreated(t, got) - requireMCPServerCreatedStep(t, got) + requireMCPServerApplied(t, got) } else { requireMCPServerStep(t, got, false, tc.reason) require.NotNil(t, got.Status.CreateAttemptTime)