diff --git a/internal/api/domains_teardown_reserved_test.go b/internal/api/domains_teardown_reserved_test.go new file mode 100644 index 00000000..7d4fb3a3 --- /dev/null +++ b/internal/api/domains_teardown_reserved_test.go @@ -0,0 +1,30 @@ +package api + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/mctlhq/mctl-api/internal/auth" + "github.com/mctlhq/mctl-api/internal/domains" + "github.com/mctlhq/mctl-api/internal/operations" +) + +// A domain row whose team/service pair is now reserved must still be torn +// down: DeleteDomain keeps the row when teardown fails, so refusing it here +// would make the row undeletable and leave its ingress and TLS in place. +func TestTriggerRemoveCustomDomain_ReservedPairStillTearsDown(t *testing.T) { + exec := &fakeDomainExecutor{} + h := &Handlers{opts: Options{Executor: exec, Registry: operations.NewRegistry()}} + d := &domains.Domain{Team: "labs", Service: "oidc-zitadel", Domain: "x.example.com", Status: domains.StatusActive} + r := httptest.NewRequest(http.MethodDelete, "/api/v1/domains/x", nil) + r = r.WithContext(auth.WithUser(context.Background(), auth.NewGitHubUser("u1", []string{"labs"}))) + _, outcome, err := h.triggerRemoveCustomDomain(r.Context(), r, d) + if err != nil || outcome != cleanupSubmitted { + t.Fatalf("outcome %q, err %v; want the teardown submitted", outcome, err) + } + if len(exec.submitted) != 1 { + t.Fatalf("submitted %d, want 1", len(exec.submitted)) + } +} diff --git a/internal/api/handlers_ci_deploy_test.go b/internal/api/handlers_ci_deploy_test.go index 6aa509c0..881b3dc2 100644 --- a/internal/api/handlers_ci_deploy_test.go +++ b/internal/api/handlers_ci_deploy_test.go @@ -62,9 +62,12 @@ func newCIFixture(t *testing.T, repos *fakeSourceRepos) *ciFixture { opts := mctlapi.Options{ Registry: operations.NewRegistry(), // No members at all: a CI deploy must not depend on tenant roles. - GitReader: &fakeGitReader{tenants: []gitops.Tenant{{Name: "labs"}}}, - AuditLog: f.audit, - Executor: f.exec, + GitReader: &fakeGitReader{ + tenants: []gitops.Tenant{{Name: "labs"}}, + services: []gitops.Service{{Team: "labs", Name: "mctl-telegram"}}, + }, + AuditLog: f.audit, + Executor: f.exec, } if repos != nil { opts.ComponentSourceRepos = repos diff --git a/internal/api/handlers_write.go b/internal/api/handlers_write.go index 2460e9af..c352dc8e 100644 --- a/internal/api/handlers_write.go +++ b/internal/api/handlers_write.go @@ -262,6 +262,46 @@ func (h *Handlers) ExecuteOperation(w http.ResponseWriter, r *http.Request) { } } + // A new tenant name must not share an Application name space with an + // existing tenant: "labs-x" next to "labs" (or the reverse) would let one + // tenant's - spell the other's Application. This applies + // to admins too; re-running create-tenant for the same name is not a + // collision. A tenant list that cannot be read refuses. + if opName == "create-tenant" { + if status, msg := h.checkTenantNameCollision(tenantParam); status != 0 { + h.logAudit(r, audit.Entry{ + UserID: user.ID, + Operation: opName, + Parameters: auditParams, + Status: "denied", + RiskLevel: string(op.RiskLevel), + Message: msg, + }) + writeError(w, status, msg) + return + } + } + + // An operation on an existing service must name one that exists for this + // team in gitops. The workflows build the Argo CD Application name as + // - and act on it, so without this check the pair is + // only as safe as the caller's choice of it. A checkout that cannot + // answer refuses: "could not check" is not "exists". + if team, component, ok := operations.ServiceTarget(op, input); ok && requiresExistingService(opName, input) { + if status, msg := h.checkServiceExists(team, component); status != 0 { + h.logAudit(r, audit.Entry{ + UserID: user.ID, + Operation: opName, + Parameters: auditParams, + Status: "denied", + RiskLevel: string(op.RiskLevel), + Message: msg, + }) + writeError(w, status, msg) + return + } + } + // For create-tenant: also notify Backstage for immediate catalog sync. if opName == "create-tenant" && h.opts.BackstageURL != "" { go h.notifyBackstage(input) @@ -281,7 +321,7 @@ func (h *Handlers) ExecuteOperation(w http.ResponseWriter, r *http.Request) { // Executor.Submit refuses a reserved tenant name on its own. The // validation above normally gets there first; mapping it here keeps // the backstop from masquerading as a platform fault. - if errors.Is(err, operations.ErrReservedTenantName) { + if errors.Is(err, operations.ErrReservedTenantName) || errors.Is(err, operations.ErrReservedServiceApplication) { writeError(w, http.StatusBadRequest, err.Error()) return } @@ -514,3 +554,57 @@ func (h *Handlers) checkTenantIsNew(tenant string) (status int, msg string) { } return 0, "" } + +// requiresExistingService reports whether opName, which names a service, +// acts on one that must already exist. It is fail-closed: every such +// operation is gated unless listed here. deploy-service's onboard action +// creates the service; provision-database may run before a service is +// onboarded and preview-delete must still clean up after one is retired, and +// neither acts on the - Application. +func requiresExistingService(opName string, input map[string]string) bool { + switch opName { + case "provision-database", "preview-delete": + return false + case "deploy-service": + return input["action"] != "onboard" + } + return true +} + +// checkServiceExists returns 0 when team/component is a service in the +// gitops checkout, 404 when the checkout says it is not, and 503 when the +// checkout cannot tell. +func (h *Handlers) checkServiceExists(team, component string) (status int, msg string) { + if h.opts.GitReader == nil { + return http.StatusServiceUnavailable, "cannot check whether service \"" + team + "/" + component + "\" exists: gitops reader not configured" + } + exists, err := h.opts.GitReader.ServiceExists(team, component) + if err != nil { + // The error can carry checkout paths; it goes to the log, not the + // caller. + slog.Error("service existence check failed", "team", team, "component", component, "error", err) + return http.StatusServiceUnavailable, "cannot check whether service \"" + team + "/" + component + "\" exists right now; try again later" + } + if !exists { + return http.StatusNotFound, "service \"" + component + "\" not found in workspace \"" + team + "\" (a service created in the last minute may not be visible yet)" + } + return 0, "" +} + +// checkTenantNameCollision returns 0 when tenant shares no hyphen prefix with +// an existing tenant, 400 when it does, and 503 when the tenant list cannot +// be read. +func (h *Handlers) checkTenantNameCollision(tenant string) (status int, msg string) { + if h.opts.GitReader == nil { + return http.StatusServiceUnavailable, "cannot check workspace name \"" + tenant + "\": gitops reader not configured" + } + names, err := h.opts.GitReader.TenantNames() + if err != nil { + slog.Error("create-tenant name collision check failed", "tenant", tenant, "error", err) + return http.StatusServiceUnavailable, "cannot check workspace name \"" + tenant + "\" right now; try again later" + } + if other, collides := operations.TenantNameCollision(tenant, names); collides { + return http.StatusBadRequest, "tenant_name: \"" + tenant + "\" overlaps the name of existing workspace \"" + other + "\"; choose a name that neither extends nor is extended by another workspace name" + } + return 0, "" +} diff --git a/internal/api/interfaces.go b/internal/api/interfaces.go index b2750738..ff96c6f5 100644 --- a/internal/api/interfaces.go +++ b/internal/api/interfaces.go @@ -38,6 +38,13 @@ type GitReader interface { ListTenants() ([]gitops.Tenant, error) GetTenant(name string) (*gitops.Tenant, error) TenantExists(name string) (bool, error) + // TenantNames lists every tenant name in the checkout; any read failure + // is an error, never a shorter list. + TenantNames() ([]string, error) + // ServiceExists reports whether services// is a + // service in the checkout. Only a missing path is (false, nil); a read + // that failed is an error. + ServiceExists(team, component string) (bool, error) // MemberRoles returns the members[].role values under which login is a // member of namespace, as written, one list per members list naming the // login. Empty with a nil error is "read, and not a member"; anything it diff --git a/internal/api/service_existence_test.go b/internal/api/service_existence_test.go new file mode 100644 index 00000000..48dd7a0d --- /dev/null +++ b/internal/api/service_existence_test.go @@ -0,0 +1,277 @@ +package api_test + +import ( + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/mctlhq/mctl-api/internal/auth" + "github.com/mctlhq/mctl-api/internal/gitops" + mctlmcp "github.com/mctlhq/mctl-api/internal/mcp" + "github.com/mctlhq/mctl-api/internal/operations" +) + +// Operations on an existing service act on the Argo CD Application +// -. The service must exist for that team in gitops before +// anything is submitted, and a checkout that cannot answer refuses. + +// serviceOps are the operations gated on an existing service, with a body +// naming erpact's service "web". +var serviceOps = map[string]map[string]string{ + "retire-service": retireBody, + "rollback-service": rollbackBody, + "remove-custom-domain": removeDomainBody, + "add-custom-domain": {"team_name": "erpact", "service_name": "web", "domain": "app.example.com"}, + "preview-deploy": {"team_name": "erpact", "component_name": "web", "image_tag": "1.2.3"}, + "deploy-service": {"action": "deploy", "team_name": "erpact", "component_name": "web", "image_tag": "1.2.3"}, +} + +func withComponent(body map[string]string, component string) map[string]string { + out := map[string]string{} + for k, v := range body { + out[k] = v + } + if _, ok := out["service_name"]; ok { + out["service_name"] = component + } else { + out["component_name"] = component + } + return out +} + +func TestServiceOps_RefuseAServiceThatDoesNotExist(t *testing.T) { + for op, body := range serviceOps { + t.Run(op, func(t *testing.T) { + f := newRoleFixture(t) + w := postAs(t, f.router, "/api/v1/operations/"+op+"/execute", withComponent(body, "app"), member("olga-owner")) + if w.Code != http.StatusNotFound { + t.Fatalf("status %d, want 404; body %s", w.Code, w.Body.String()) + } + if len(f.exec.submitted) != 0 { + t.Fatalf("submitted %v for a service that does not exist", f.exec.submitted) + } + if a := f.lastAudit(t); a.Status != "denied" { + t.Errorf("audit status %q, want denied", a.Status) + } + }) + } +} + +func TestServiceOps_FailedReadRefuses(t *testing.T) { + for op, body := range serviceOps { + t.Run(op, func(t *testing.T) { + f := newRoleFixture(t) + f.git.serviceExistsErr = errors.New("checkout stale") + w := postAs(t, f.router, "/api/v1/operations/"+op+"/execute", body, member("olga-owner")) + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("status %d, want 503; body %s", w.Code, w.Body.String()) + } + if strings.Contains(w.Body.String(), "checkout stale") { + t.Errorf("internal error leaked to the caller: %s", w.Body.String()) + } + if len(f.exec.submitted) != 0 { + t.Fatalf("submitted %v although the service could not be checked", f.exec.submitted) + } + }) + } +} + +func TestServiceOps_ExistingServiceProceeds(t *testing.T) { + for op, body := range serviceOps { + t.Run(op, func(t *testing.T) { + f := newRoleFixture(t) + w := postAs(t, f.router, "/api/v1/operations/"+op+"/execute", body, member("olga-owner")) + if w.Code != http.StatusAccepted { + t.Fatalf("status %d, want 202; body %s", w.Code, w.Body.String()) + } + if f.git.serviceExistsCalls != 1 { + t.Errorf("ServiceExists called %d times, want 1", f.git.serviceExistsCalls) + } + }) + } +} + +// Admins are held to it too: the check is about which Application the +// workflow acts on, not about who asks. +func TestRetireService_AdminIsCheckedToo(t *testing.T) { + f := newRoleFixture(t) + admin := &auth.User{ID: "root-admin", Groups: []string{"admins"}} + w := postAs(t, f.router, "/api/v1/operations/retire-service/execute", withComponent(retireBody, "app"), admin) + if w.Code != http.StatusNotFound || len(f.exec.submitted) != 0 { + t.Fatalf("status %d, submitted %v; want 404 and nothing submitted", w.Code, f.exec.submitted) + } +} + +// Onboarding creates the service, so it is not gated on existence; a +// reserved name is still refused for it. +func TestDeployOnboard_NotGatedOnExistence(t *testing.T) { + f := newRoleFixture(t) + body := map[string]string{"action": "onboard", "team_name": "erpact", "component_name": "fresh", "image_tag": "1.0.0"} + w := postAs(t, f.router, "/api/v1/operations/deploy-service/execute", body, member("olga-owner")) + if w.Code != http.StatusAccepted { + t.Fatalf("status %d; body %s", w.Code, w.Body.String()) + } + if f.git.serviceExistsCalls != 0 { + t.Errorf("onboard consulted ServiceExists %d times", f.git.serviceExistsCalls) + } + + f = newRoleFixture(t) + body["component_name"] = "control" // erpact-control is a platform Application + w = postAs(t, f.router, "/api/v1/operations/deploy-service/execute", body, member("olga-owner")) + if w.Code != http.StatusBadRequest || len(f.exec.submitted) != 0 { + t.Fatalf("status %d, submitted %v; want 400 and nothing submitted", w.Code, f.exec.submitted) + } + if !strings.Contains(w.Body.String(), "reserved for platform use") { + t.Errorf("body %s", w.Body.String()) + } +} + +// The MCP tool goes through the same REST handler, so it is refused the same +// way. +func TestRetireService_MCPRefusesAServiceThatDoesNotExist(t *testing.T) { + f := newRoleFixture(t) + users := map[string]*auth.User{"tok-owner": member("olga-owner")} + backend := httptest.NewServer(tokenAuth(users)(f.router)) + defer backend.Close() + text, isError := callTool(t, mctlmcp.NewServer(backend.URL, ""), "tok-owner", "mctl_retire_service", + map[string]string{"team_name": "erpact", "component_name": "app", "confirm": "yes"}) + if !isError || !strings.Contains(text, "not found") { + t.Fatalf("isError=%v text=%q; want a not-found refusal", isError, text) + } + if len(f.exec.submitted) != 0 { + t.Fatalf("submitted %v", f.exec.submitted) + } +} + +// ── create-tenant names ────────────────────────────────────────────────────── + +func newCreateTenantFixture(t *testing.T) *roleFixture { + t.Helper() + f := newRoleFixture(t) + for _, n := range []string{"admins", "karabu", "labs", "mctl", "ovk"} { + f.git.tenants = append(f.git.tenants, gitops.Tenant{Name: n}) + } + return f +} + +func TestCreateTenant_RefusesPlatformApplicationNames(t *testing.T) { + users := map[string]*auth.User{"tok-new": auth.NewGitHubUser("newcomer", nil)} + for _, name := range []string{"root", "shared", "tenant", "loki", "otel", "external", "cloudnative", "grafana", "reflector", "valkey"} { + t.Run(name, func(t *testing.T) { + f := newCreateTenantFixture(t) + w := postAs(t, f.router, "/api/v1/operations/create-tenant/execute", map[string]string{"tenant_name": name}, users["tok-new"]) + if w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), "reserved") { + t.Fatalf("REST: status %d body %s; want 400 reserved", w.Code, w.Body.String()) + } + + mf := newCreateTenantFixture(t) + backend := httptest.NewServer(tokenAuth(users)(mf.router)) + defer backend.Close() + text, isError := callTool(t, mctlmcp.NewServer(backend.URL, ""), "tok-new", "mctl_create_tenant", + map[string]string{"tenant_name": name}) + if !isError || !strings.Contains(text, "reserved") { + t.Fatalf("MCP: isError=%v text=%q; want reserved", isError, text) + } + if len(f.exec.submitted)+len(mf.exec.submitted) != 0 { + t.Fatalf("submitted REST %v MCP %v", f.exec.submitted, mf.exec.submitted) + } + }) + } +} + +func TestCreateTenant_RefusesNamesOverlappingAnExistingTenant(t *testing.T) { + users := map[string]*auth.User{"tok-new": auth.NewGitHubUser("newcomer", nil)} + for _, name := range []string{"labs-mctl", "admins-mctl", "erpact-oidc", "ovk-x"} { + t.Run(name, func(t *testing.T) { + f := newCreateTenantFixture(t) + w := postAs(t, f.router, "/api/v1/operations/create-tenant/execute", map[string]string{"tenant_name": name}, users["tok-new"]) + if w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), "overlaps") { + t.Fatalf("REST: status %d body %s; want 400 overlaps", w.Code, w.Body.String()) + } + + mf := newCreateTenantFixture(t) + backend := httptest.NewServer(tokenAuth(users)(mf.router)) + defer backend.Close() + text, isError := callTool(t, mctlmcp.NewServer(backend.URL, ""), "tok-new", "mctl_create_tenant", + map[string]string{"tenant_name": name}) + if !isError || !strings.Contains(text, "overlaps") { + t.Fatalf("MCP: isError=%v text=%q; want overlaps", isError, text) + } + if len(f.exec.submitted)+len(mf.exec.submitted) != 0 { + t.Fatalf("submitted REST %v MCP %v", f.exec.submitted, mf.exec.submitted) + } + }) + } +} + +func TestCreateTenant_TenantListReadFailureRefuses(t *testing.T) { + f := newCreateTenantFixture(t) + f.git.tenantNamesErr = errors.New("checkout stale") + w := postAs(t, f.router, "/api/v1/operations/create-tenant/execute", map[string]string{"tenant_name": "acme"}, auth.NewGitHubUser("newcomer", nil)) + if w.Code != http.StatusServiceUnavailable || len(f.exec.submitted) != 0 { + t.Fatalf("status %d, submitted %v; want 503 and nothing submitted", w.Code, f.exec.submitted) + } +} + +func TestCreateTenant_OrdinaryNameStillAccepted(t *testing.T) { + f := newCreateTenantFixture(t) + w := postAs(t, f.router, "/api/v1/operations/create-tenant/execute", map[string]string{"tenant_name": "acme"}, auth.NewGitHubUser("newcomer", nil)) + if w.Code != http.StatusAccepted { + t.Fatalf("status %d; body %s", w.Code, w.Body.String()) + } +} + +// An admin re-running create-tenant for an existing tenant is neither +// reserved nor a collision with itself. +func TestCreateTenant_ExistingTenantNamesStayValidForAdmins(t *testing.T) { + admin := &auth.User{ID: "root-admin", Groups: []string{"admins"}} + for _, name := range []string{"admins", "erpact", "karabu", "labs", "mctl", "ovk"} { + t.Run(name, func(t *testing.T) { + f := newCreateTenantFixture(t) + w := postAs(t, f.router, "/api/v1/operations/create-tenant/execute", map[string]string{"tenant_name": name}, admin) + if w.Code != http.StatusAccepted { + t.Fatalf("status %d; body %s", w.Code, w.Body.String()) + } + }) + } +} + +// Every registry operation that names a service is classified here, so an +// operation added later fails this test until someone decides whether it is +// gated. The gate is fail-closed: unlisted operations are gated. +func TestServiceOps_GatingTableCoversRegistry(t *testing.T) { + notGated := map[string]bool{"provision-database": true, "preview-delete": true} + fill := map[string]string{ + "team_name": "erpact", "component_name": "missing", "service_name": "missing", "app_name": "missing", + "action": "deploy", "target_tag": "1.2.3", "image_tag": "1.2.3", "domain": "app.example.com", + "preview_id": "abc123", + } + reg := operations.NewRegistry() + seen := 0 + for _, op := range reg.List() { + if _, _, ok := operations.ServiceTarget(op, fill); !ok || op.HandlerOnly { + continue + } + seen++ + body := map[string]string{} + for _, p := range op.Parameters { + if v, ok := fill[p.Name]; ok { + body[p.Name] = v + } + } + f := newRoleFixture(t) + w := postAs(t, f.router, "/api/v1/operations/"+op.Name+"/execute", body, member("olga-owner")) + want := http.StatusNotFound + if notGated[op.Name] { + want = http.StatusAccepted + } + if w.Code != want { + t.Errorf("%s: status %d, want %d (gated=%v); body %s", op.Name, w.Code, want, !notGated[op.Name], w.Body.String()) + } + } + if seen < 8 { + t.Errorf("only %d service operations found; the table walk is broken", seen) + } +} diff --git a/internal/api/smoke_test.go b/internal/api/smoke_test.go index bdf5ae6c..b502d012 100644 --- a/internal/api/smoke_test.go +++ b/internal/api/smoke_test.go @@ -57,6 +57,9 @@ type fakeGitReader struct { humanInputFiles []gitops.HumanInputRequestFile humanInputErr error tenantExistsErr error + tenantNamesErr error + serviceExistsErr error + serviceExistsCalls int memberRolesErr error memberRolesCalls int } @@ -98,6 +101,33 @@ func (f *fakeGitReader) TenantExists(name string) (bool, error) { return false, nil } +// TenantNames answers from the fake's tenants, or fails with tenantNamesErr. +func (f *fakeGitReader) TenantNames() ([]string, error) { + if f.tenantNamesErr != nil { + return nil, f.tenantNamesErr + } + names := make([]string, 0, len(f.tenants)) + for i := range f.tenants { + names = append(names, f.tenants[i].Name) + } + return names, nil +} + +// ServiceExists answers from the fake's services, or fails with +// serviceExistsErr. +func (f *fakeGitReader) ServiceExists(team, component string) (bool, error) { + f.serviceExistsCalls++ + if f.serviceExistsErr != nil { + return false, f.serviceExistsErr + } + for _, s := range f.services { + if s.Team == team && s.Name == component { + return true, nil + } + } + return false, nil +} + func (f *fakeGitReader) ListHumanInputRequests() ([]gitops.HumanInputRequestFile, error) { return f.humanInputFiles, f.humanInputErr } diff --git a/internal/api/tenant_role_execute_test.go b/internal/api/tenant_role_execute_test.go index 3b8fbdb0..f0af2301 100644 --- a/internal/api/tenant_role_execute_test.go +++ b/internal/api/tenant_role_execute_test.go @@ -43,7 +43,10 @@ func newRoleFixture(t *testing.T) *roleFixture { {UserID: "dup", Role: "owner"}, {UserID: "dup", Role: "viewer"}, }, - }}}, + }}, services: []gitops.Service{ + {Team: "erpact", Name: "web"}, + {Team: "erpact", Name: "mctl-telegram"}, + }}, exec: &fakeExecutor{}, audit: audit.NewLogger(), } diff --git a/internal/gitops/member_roles_test.go b/internal/gitops/member_roles_test.go index 1a57b28d..f1751e3d 100644 --- a/internal/gitops/member_roles_test.go +++ b/internal/gitops/member_roles_test.go @@ -128,7 +128,7 @@ func TestMemberRoles_UnreadableIsNeverAnAnswer(t *testing.T) { t.Run("stale checkout", func(t *testing.T) { dir, r := setupTempRepo(t) writeTenantYAML(t, dir, "erpact", erpactYAML) - r.lastSync.Store(time.Now().Add(-MemberRolesMaxAge - time.Minute).UnixNano()) + r.lastSync.Store(time.Now().Add(-CheckoutMaxAge - time.Minute).UnixNano()) if got, err := r.MemberRoles("erpact", "alice"); err == nil { t.Fatalf("a stale checkout answered %q", got) } diff --git a/internal/gitops/reader.go b/internal/gitops/reader.go index 5c35ab61..40c6e6c9 100644 --- a/internal/gitops/reader.go +++ b/internal/gitops/reader.go @@ -641,10 +641,12 @@ func (r *Reader) GetTenant(name string) (*Tenant, error) { return r.readTenant(name) } -// TenantExistsMaxAge bounds how old the checkout may be when TenantExists -// answers. The checkout refreshes every minute; an older last sync could miss -// a tenant created since, so it is an error rather than "absent". -const TenantExistsMaxAge = 10 * time.Minute +// CheckoutMaxAge bounds how old the checkout may be when a reader that a +// security decision rests on answers (TenantExists, TenantNames, +// ServiceExists, MemberRoles). The checkout refreshes every minute; an older +// last sync could miss a change made since, so it is an error rather than an +// answer. +const CheckoutMaxAge = 10 * time.Minute // tenantNameRE is create-tenant's tenant_name pattern (operations registry // and wft-create-tenant): a single DNS-safe path segment. @@ -653,7 +655,7 @@ var tenantNameRE = regexp.MustCompile(`^[a-z0-9][a-z0-9-]{1,62}$`) // TenantExists reports whether platform-gitops/tenants/ exists in the // checkout, as a directory or anything else: that path is what create-tenant // would write. Only "no such file" is absent. Any other stat error, a checkout -// that never synced, or one older than TenantExistsMaxAge is an error. +// that never synced, or one older than CheckoutMaxAge is an error. func (r *Reader) TenantExists(name string) (bool, error) { if !tenantNameRE.MatchString(name) { return false, fmt.Errorf("invalid tenant name %q", name) @@ -661,12 +663,8 @@ func (r *Reader) TenantExists(name string) (bool, error) { r.mu.RLock() defer r.mu.RUnlock() - last := r.LastSync() - if last.IsZero() { - return false, errors.New("gitops checkout has never synced") - } - if age := time.Since(last); age > TenantExistsMaxAge { - return false, fmt.Errorf("gitops checkout last synced %s ago (limit %s)", age.Round(time.Second), TenantExistsMaxAge) + if err := r.checkFreshLocked(); err != nil { + return false, err } // The pattern already admits one segment only; the containment check // keeps that true for the path actually read. @@ -685,11 +683,104 @@ func (r *Reader) TenantExists(name string) (bool, error) { return false, fmt.Errorf("checking tenant %q: %w", name, err) } -// MemberRolesMaxAge bounds how old the checkout may be when MemberRoles -// answers. A role decides whether a destructive operation runs, so an -// answer from a checkout that stopped refreshing is refused rather than -// trusted: it could still show a role that has since been taken away. -const MemberRolesMaxAge = TenantExistsMaxAge +// checkFreshLocked returns an error unless the checkout has synced within +// CheckoutMaxAge. An answer from a checkout that never synced, or that +// stopped refreshing, is not one a security decision may rest on. +func (r *Reader) checkFreshLocked() error { + last := r.LastSync() + if last.IsZero() { + return errors.New("gitops checkout has never synced") + } + if age := time.Since(last); age > CheckoutMaxAge { + return fmt.Errorf("gitops checkout last synced %s ago (limit %s)", age.Round(time.Second), CheckoutMaxAge) + } + return nil +} + +// TenantNames returns the name of every entry in platform-gitops/tenants, +// whatever its kind: each is a name create-tenant would treat as taken. +// +// Unlike ListTenants it never skips what it could not read. A checkout that +// never synced or is stale, and a tenants directory that is missing or +// cannot be listed, are errors: the caller decides whether a new tenant name +// collides with an existing one, and "could not list" must not read as +// "there are none". +func (r *Reader) TenantNames() ([]string, error) { + r.mu.RLock() + defer r.mu.RUnlock() + if err := r.checkFreshLocked(); err != nil { + return nil, err + } + entries, err := os.ReadDir(filepath.Join(r.localPath, "platform-gitops", "tenants")) + if err != nil { + return nil, fmt.Errorf("listing tenants: %w", err) + } + names := make([]string, 0, len(entries)) + for _, e := range entries { + names = append(names, e.Name()) + } + return names, nil +} + +// ServiceExists reports whether team has a service named component: a plain +// directory platform-gitops/services// holding a regular +// values.yaml. That directory is what the apps ApplicationSet turns into the +// Argo CD Application -, so it is the record of which +// Applications a tenant's service operations may act on. +// +// Only a path that does not exist is (false, nil). A checkout that never +// synced or is stale, a symlink or other non-plain entry on the path, and any +// other stat error are errors, so a failed read cannot pass for "absent" and +// still less for "present". +func (r *Reader) ServiceExists(team, component string) (bool, error) { + if !serviceNameRe.MatchString(team) || !serviceNameRe.MatchString(component) { + return false, fmt.Errorf("invalid team or component name %q/%q", team, component) + } + r.mu.RLock() + defer r.mu.RUnlock() + if err := r.checkFreshLocked(); err != nil { + return false, err + } + + path, err := r.statServiceFileLocked(team, component, "values.yaml") + if err != nil { + return false, fmt.Errorf("checking service %s/%s: %w", team, component, err) + } + return path != "", nil +} + +// statServiceFileLocked walks platform-gitops/services/// +// with Lstat and returns the file's path, or "" when a path element +// below the services directory does not exist. Every directory on the path +// must be a plain directory and the file a regular file: a symlink or any +// other kind of entry is an error, as is the services directory itself +// missing (a broken checkout, not a missing service) and any other stat +// error. Names must already be validated. The caller holds r.mu. +func (r *Reader) statServiceFileLocked(team, component, file string) (string, error) { + services := filepath.Join(r.localPath, "platform-gitops", "services") + steps := []struct { + path string + dir bool + }{ + {services, true}, + {filepath.Join(services, team), true}, + {filepath.Join(services, team, component), true}, + {filepath.Join(services, team, component, file), false}, + } + for i, st := range steps { + info, err := os.Lstat(st.path) + if errors.Is(err, fs.ErrNotExist) && i > 0 { + return "", nil + } + if err != nil { + return "", err + } + if st.dir && !info.IsDir() || !st.dir && !info.Mode().IsRegular() { + return "", fmt.Errorf("%s is not a plain %s", st.path, map[bool]string{true: "directory", false: "file"}[st.dir]) + } + } + return steps[len(steps)-1].path, nil +} // MemberRoles returns the members[].role values under which login is a // member of namespace (a tenant name, or {tenant}-{team} for a multi-team @@ -699,7 +790,7 @@ const MemberRolesMaxAge = TenantExistsMaxAge // read and login is not a member. // // Unlike ListTenants, it never skips what it could not read. A checkout that -// never synced or is older than MemberRolesMaxAge, a tenants directory that +// never synced or is older than CheckoutMaxAge, a tenants directory that // cannot be listed, a values.yaml that cannot be read or parsed, and a // namespace two tenants both claim are all errors: "could not read the // role" must never come back looking like "not a member", and still less @@ -712,12 +803,8 @@ func (r *Reader) MemberRoles(namespace, login string) ([][]string, error) { r.mu.RLock() defer r.mu.RUnlock() - last := r.LastSync() - if last.IsZero() { - return nil, errors.New("gitops checkout has never synced") - } - if age := time.Since(last); age > MemberRolesMaxAge { - return nil, fmt.Errorf("gitops checkout last synced %s ago (limit %s)", age.Round(time.Second), MemberRolesMaxAge) + if err := r.checkFreshLocked(); err != nil { + return nil, err } tenantsDir := filepath.Join(r.localPath, "platform-gitops", "tenants") @@ -1061,32 +1148,15 @@ func (r *Reader) ComponentSourceRepo(team, component string) (string, error) { return "", fmt.Errorf("gitops checkout unavailable: %w", err) } - services := filepath.Join(r.localPath, "platform-gitops", "services") - steps := []struct { - path string - dir bool - }{ - {services, true}, - {filepath.Join(services, team), true}, - {filepath.Join(services, team, component), true}, - {filepath.Join(services, team, component, "catalog-info.yaml"), false}, + path, err := r.statServiceFileLocked(team, component, "catalog-info.yaml") + if err != nil { + return "", err } - for i, st := range steps { - info, err := os.Lstat(st.path) - if errors.Is(err, fs.ErrNotExist) && i > 0 { - // The services directory itself missing is a broken checkout, - // not an unregistered component. - return "", ErrSourceRepoNotRegistered - } - if err != nil { - return "", err - } - if st.dir && !info.IsDir() || !st.dir && !info.Mode().IsRegular() { - return "", fmt.Errorf("%s is not a plain %s", st.path, map[bool]string{true: "directory", false: "file"}[st.dir]) - } + if path == "" { + return "", ErrSourceRepoNotRegistered } - data, err := os.ReadFile(steps[len(steps)-1].path) //nolint:gosec // validated names, regular file inside the checkout + data, err := os.ReadFile(path) //nolint:gosec // validated names, regular file inside the checkout if err != nil { return "", err } diff --git a/internal/gitops/reader_test.go b/internal/gitops/reader_test.go index a374529d..318b9aea 100644 --- a/internal/gitops/reader_test.go +++ b/internal/gitops/reader_test.go @@ -1577,7 +1577,7 @@ func TestTenantExists(t *testing.T) { if _, err := r.TenantExists("billing"); err == nil || !strings.Contains(err.Error(), "never synced") { t.Fatalf("a checkout that never synced must not answer: %v", err) } - r.lastSync.Store(time.Now().Add(-TenantExistsMaxAge - time.Minute).UnixNano()) + r.lastSync.Store(time.Now().Add(-CheckoutMaxAge - time.Minute).UnixNano()) if _, err := r.TenantExists("billing"); err == nil { t.Fatal("a stale checkout must not answer") } @@ -1623,3 +1623,120 @@ func TestTenantExists_UnreadableIsNotAbsent(t *testing.T) { t.Fatalf("an unreadable tenants dir must be an error, got exists=%v", got) } } + +func writeServiceValues(t *testing.T, dir, team, component string) { + t.Helper() + d := filepath.Join(dir, "platform-gitops", "services", team, component) + if err := os.MkdirAll(d, 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(d, "values.yaml"), []byte("image:\n tag: 1.0.0\n"), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestServiceExists(t *testing.T) { + dir, r := setupTempRepo(t) + if _, err := r.ServiceExists("labs", "web"); err == nil || !strings.Contains(err.Error(), "never synced") { + t.Fatalf("a checkout that never synced must not answer: %v", err) + } + r.lastSync.Store(time.Now().Add(-CheckoutMaxAge - time.Minute).UnixNano()) + if _, err := r.ServiceExists("labs", "web"); err == nil { + t.Fatal("a stale checkout must not answer") + } + r.lastSync.Store(time.Now().UnixNano()) + + // No services directory at all is a broken checkout, not "absent". + if _, err := r.ServiceExists("labs", "web"); err == nil { + t.Fatal("a checkout without platform-gitops/services must be an error") + } + + writeServiceValues(t, dir, "labs", "web") + // A service directory without values.yaml is not a service. + if err := os.MkdirAll(filepath.Join(dir, "platform-gitops", "services", "labs", "half"), 0o750); err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + team, component string + want bool + }{ + {"labs", "web", true}, + {"labs", "half", false}, + {"labs", "missing", false}, + {"root", "app", false}, + {"other", "web", false}, + } { + got, err := r.ServiceExists(tc.team, tc.component) + if err != nil || got != tc.want { + t.Errorf("ServiceExists(%q, %q) = %v, %v; want %v", tc.team, tc.component, got, err, tc.want) + } + } + for _, bad := range [][2]string{{"", "web"}, {"labs", ""}, {"..", "web"}, {"labs", "../x"}, {"labs", "a/b"}, {"Labs", "web"}} { + if _, err := r.ServiceExists(bad[0], bad[1]); err == nil { + t.Errorf("ServiceExists(%q, %q) must be an error", bad[0], bad[1]) + } + } +} + +// A symlink anywhere on the path is not taken as a service, and is not +// silently "absent" either. +func TestServiceExists_SymlinkIsAnError(t *testing.T) { + dir, r := setupTempRepo(t) + r.lastSync.Store(time.Now().UnixNano()) + writeServiceValues(t, dir, "labs", "web") + if err := os.Symlink(filepath.Join(dir, "platform-gitops", "services", "labs"), + filepath.Join(dir, "platform-gitops", "services", "evil")); err != nil { + t.Fatal(err) + } + if got, err := r.ServiceExists("evil", "web"); err == nil { + t.Fatalf("a symlinked team directory must be an error, got exists=%v", got) + } +} + +func TestServiceExists_UnreadableIsNotAbsent(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores directory permissions") + } + dir, r := setupTempRepo(t) + r.lastSync.Store(time.Now().UnixNano()) + writeServiceValues(t, dir, "labs", "web") + team := filepath.Join(dir, "platform-gitops", "services", "labs") + if err := os.Chmod(team, 0); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(team, 0o750) }) //nolint:gosec // test cleanup + if got, err := r.ServiceExists("labs", "other"); err == nil { + t.Fatalf("an unreadable team dir must be an error, got exists=%v", got) + } +} + +func TestTenantNames(t *testing.T) { + dir, r := setupTempRepo(t) + if _, err := r.TenantNames(); err == nil { + t.Fatal("a checkout that never synced must not answer") + } + r.lastSync.Store(time.Now().UnixNano()) + if _, err := r.TenantNames(); err == nil { + t.Fatal("a missing tenants directory must be an error, not an empty list") + } + writeTenantYAML(t, dir, "labs", "tenant:\n name: labs\n") + // A tenant directory whose values.yaml does not parse is still a name. + writeTenantYAML(t, dir, "broken", ":\n - [\n") + names, err := r.TenantNames() + if err != nil { + t.Fatal(err) + } + if strings.Join(names, ",") != "broken,labs" { + t.Errorf("TenantNames = %v", names) + } + tenants := filepath.Join(dir, "platform-gitops", "tenants") + if os.Geteuid() != 0 { + if err := os.Chmod(tenants, 0); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(tenants, 0o750) }) //nolint:gosec // test cleanup + if _, err := r.TenantNames(); err == nil { + t.Fatal("an unreadable tenants dir must be an error") + } + } +} diff --git a/internal/operations/executor.go b/internal/operations/executor.go index 56c79c89..71b6f1d7 100644 --- a/internal/operations/executor.go +++ b/internal/operations/executor.go @@ -134,6 +134,10 @@ func (e *Executor) Submit(ctx context.Context, op Operation, params map[string]s slog.Warn("refusing to submit create-tenant", "user", userID, "error", err) return nil, err } + if err := checkServiceApplication(op, params); err != nil { + slog.Warn("refusing to submit service operation", "operation", op.Name, "user", userID, "error", err) + return nil, err + } namespace := WorkflowNamespace(op.WorkflowTemplate, team) requestID := uuid.New().String()[:8] workflowName := fmt.Sprintf("%s-%s", op.WorkflowTemplate, requestID) diff --git a/internal/operations/registry.go b/internal/operations/registry.go index e417ba1a..792103ad 100644 --- a/internal/operations/registry.go +++ b/internal/operations/registry.go @@ -267,6 +267,13 @@ func (r *Registry) ValidateInput(op Operation, input map[string]string) []string if createsTenant(op) && IsReservedTenantName(input["tenant_name"]) { errors = append(errors, reservedTenantNameMessage(input["tenant_name"])) } + // A service whose Argo CD Application name (-) is + // platform-owned is refused for every operation that addresses one, + // except teardown (see teardownOperations). + // Executor.Submit repeats this check as well. + if team, name, reserved := ReservedServiceTarget(op, input); reserved { + errors = append(errors, reservedServiceApplicationMessage(team, name)) + } return errors } @@ -511,7 +518,7 @@ var builtinOperations = []Operation{ Parameters: []ParameterDef{ {Name: "team_name", Type: "string", Required: true, Description: "Team name", Pattern: "^[a-z0-9][a-z0-9-]{0,30}$"}, {Name: "component_name", Type: "string", Required: true, Description: "Service name", Pattern: "^[a-z0-9][a-z0-9-]{0,30}$"}, - {Name: "preview_id", Type: "string", Required: true, Description: "Preview ID returned by preview-deploy"}, + {Name: "preview_id", Type: "string", Required: true, Description: "Preview ID returned by preview-deploy", Pattern: "^[a-z0-9][a-z0-9-]{0,30}$"}, }, }, { diff --git a/internal/operations/reserved_application.go b/internal/operations/reserved_application.go new file mode 100644 index 00000000..c64a50a7 --- /dev/null +++ b/internal/operations/reserved_application.go @@ -0,0 +1,166 @@ +// Copyright 2025 MCTL Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package operations + +import ( + "errors" + "fmt" + "strings" +) + +// ErrReservedServiceApplication is returned (wrapped) when a tenant service +// operation names a team and component whose Argo CD Application name, +// -, belongs to the platform. +var ErrReservedServiceApplication = errors.New("service name maps to a platform-owned application") + +// serviceComponentParams are the parameter names operations use for the +// service part of -. +var serviceComponentParams = []string{"component_name", "service_name", "app_name"} + +// reservedComponentNames are component names the platform's own +// ApplicationSets append to a tenant name (-openclaw-skills). +var reservedComponentNames = map[string]struct{}{ + "openclaw-skills": {}, +} + +// teardownOperations remove what a tenant already has, inside its own +// paths and namespaces, without acting on the - Argo CD +// Application: remove-custom-domain edits services//, and +// preview-delete uninstalls a Helm release in -preview or . The +// reserved-pair check does not apply to them, so a record that exists can +// always be cleaned up (DELETE /domains keeps a row whose teardown fails). +var teardownOperations = map[string]struct{}{ + "remove-custom-domain": {}, + "preview-delete": {}, +} + +// ChecksReservedPair reports whether op is refused for a reserved +// - pair: every operation that names a service, except the +// teardown operations above. +func ChecksReservedPair(op Operation) bool { + _, teardown := teardownOperations[op.Name] + return !teardown +} + +// ServiceTarget returns the team and component an operation acts on, when it +// takes both. ok is false for operations that do not address a service. +func ServiceTarget(op Operation, params map[string]string) (team, component string, ok bool) { + if !declares(op, "team_name") { + return "", "", false + } + for _, name := range serviceComponentParams { + if declares(op, name) { + return params["team_name"], params[name], true + } + } + return "", "", false +} + +func declares(op Operation, name string) bool { + for _, p := range op.Parameters { + if p.Name == name { + return true + } + } + return false +} + +// IsReservedServiceApplication reports whether - is the name +// of a platform-owned Argo CD Application, or a component name the platform's +// ApplicationSets generate themselves. A tenant service with that name would +// make the apps ApplicationSet produce an Application that collides with the +// platform's, and an operation on it would act on the platform's. +// +// The comparison is case-insensitive, like IsReservedTenantName. +func IsReservedServiceApplication(team, component string) bool { + t := strings.ToLower(strings.TrimSpace(team)) + c := strings.ToLower(strings.TrimSpace(component)) + if t == "" || c == "" { + return false + } + if _, ok := reservedComponentNames[c]; ok { + return true + } + name := t + "-" + c + if strings.HasPrefix(name, "tenant-") { + return true + } + for _, list := range [][]string{platformApplicationNames, tenantScopedPlatformApplications} { + for _, p := range list { + if name == p { + return true + } + } + } + return false +} + +func reservedServiceApplicationMessage(team, component string) string { + return fmt.Sprintf("%q/%q is reserved for platform use; choose a different service name", + echoTenantName(team), echoTenantName(component)) +} + +// checkServiceApplication returns a wrapped ErrReservedServiceApplication +// when op addresses a service whose Application name is platform-owned. +// Executor.Submit uses it as the backstop; Registry.ValidateInput applies the +// same predicate for the clear 400. +func checkServiceApplication(op Operation, params map[string]string) error { + if team, name, reserved := ReservedServiceTarget(op, params); reserved { + return fmt.Errorf("%w: %q", ErrReservedServiceApplication, echoTenantName(team+"-"+name)) + } + return nil +} + +// ReservedServiceTarget reports whether op, called with params, names a +// reserved -, and which name. For most operations name is the +// service. Teardown operations skip the pair itself, but preview-delete is +// still checked on the name it composes from the service and preview_id: +// the hyphen between them is not in either parameter, so neither pattern +// stops the pair from spelling a platform name. A real preview is never +// named after a platform Application, so this refuses nothing legitimate. +func ReservedServiceTarget(op Operation, params map[string]string) (team, name string, reserved bool) { + team, component, ok := ServiceTarget(op, params) + if !ok { + return "", "", false + } + if ChecksReservedPair(op) { + return team, component, IsReservedServiceApplication(team, component) + } + if id := params["preview_id"]; id != "" && declares(op, "preview_id") { + composed := component + "-" + id + return team, composed, IsReservedServiceApplication(team, composed) + } + return team, component, false +} + +// TenantNameCollision returns the existing tenant a new tenant name would +// share an Application namespace with, if any: name extends an existing +// tenant ("labs-x" next to "labs") or an existing tenant extends it ("lab" +// next to "lab-y"). Either way - of one tenant could spell the +// Application of a service of the other. Equality is not a collision: that +// is the same tenant, which create-tenant handles on its own. +func TenantNameCollision(name string, existing []string) (string, bool) { + n := strings.ToLower(strings.TrimSpace(name)) + for _, e := range existing { + e = strings.ToLower(strings.TrimSpace(e)) + if e == "" || e == n { + continue + } + if strings.HasPrefix(n, e+"-") || strings.HasPrefix(e, n+"-") { + return e, true + } + } + return "", false +} diff --git a/internal/operations/reserved_application_test.go b/internal/operations/reserved_application_test.go new file mode 100644 index 00000000..f8daa735 --- /dev/null +++ b/internal/operations/reserved_application_test.go @@ -0,0 +1,284 @@ +// Copyright 2025 MCTL Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 + +package operations + +import ( + "context" + "errors" + "strings" + "testing" +) + +// existingTenants are the tenants in platform-gitops/tenants on 2026-10-11. +var existingTenants = []string{"admins", "erpact", "karabu", "labs", "mctl", "ovk"} + +// liveApplications is `kubectl get applications -n argocd -o name` on +// 2026-10-11, the input platformApplicationNames was derived from. +var liveApplications = []string{ + "admins-mctl-agent", "admins-mctl-agents-worker", "admins-mctl-agents-worker-exec", + "admins-mctl-agents-worker-implement", "admins-mctl-api", "admins-mctl-design", + "admins-mctl-docs", "admins-mctl-portal", "admins-mctl-web", "argo-rollouts", + "argo-workflows", "argo-workflows-config", "argocd-self-managed", "cloudnative-pg", + "erpact-control", "erpact-deployer", "erpact-infra", "erpact-oidc-zitadel", + "erpact-shared", "external-secrets", "forgejo", "grafana-iac", + "karabu-draw-with-friends", "labs-agent-worker-preview", "labs-claude-remote", + "labs-coolify-mcp", "labs-genai-leader", "labs-instruments-proxy", "labs-kuptsi-app", + "labs-mctl-academy", "labs-mctl-alice", "labs-mctl-loyalty", "labs-mctl-pairdesk", + "labs-mctl-telegram", "labs-mctl-telegram-preview", "labs-oidc-zitadel", + "labs-payment-llm-pipelines", "labs-pelican-proxy", "labs-pelican-proxy-staging", + "labs-pfeifenpatenschaft-backend", "labs-portfolio", "labs-projects-mcp", + "labs-seerrsense", "loki-stack", "mctl-portal-oidc-zitadel", "minio", "monitoring", + "otel-collector", "otel-eval-tempo", "ovk-mctl-academy", "reflector", "root-app", + "shared-pg", "temporal", "temporal-web", "tenant-admins", "tenant-erpact", + "tenant-karabu", "tenant-labs", "tenant-mctl", "tenant-ovk", "valkey", "vault", + "vault-human-auth-iac", "zitadel", +} + +// tenantServices are the platform-gitops/services// +// directories on 2026-10-11: the apps ApplicationSet's Applications. +var tenantServices = map[string]bool{ + "admins-mctl-agents-worker": true, "admins-mctl-agents-worker-exec": true, + "admins-mctl-agents-worker-implement": true, "admins-mctl-design": true, + "admins-mctl-docs": true, "admins-mctl-web": true, "karabu-draw-with-friends": true, + "labs-agent-worker-preview": true, "labs-claude-remote": true, "labs-coolify-mcp": true, + "labs-genai-leader": true, "labs-instruments-proxy": true, "labs-kuptsi-app": true, + "labs-mctl-academy": true, "labs-mctl-alice": true, "labs-mctl-loyalty": true, + "labs-mctl-pairdesk": true, "labs-mctl-telegram": true, "labs-mctl-telegram-preview": true, + "labs-payment-llm-pipelines": true, "labs-pelican-proxy": true, + "labs-pelican-proxy-staging": true, "labs-pfeifenpatenschaft-backend": true, + "labs-portfolio": true, "labs-projects-mcp": true, "labs-seerrsense": true, + "ovk-mctl-academy": true, +} + +func hyphenPrefixes(name string) []string { + var out []string + for i := range name { + if name[i] == '-' { + out = append(out, name[:i]) + } + } + return append(out, name) +} + +// Every live Application that is not a tenant's own service or tenant +// Application is protected: either no new tenant can take any hyphen-prefix +// of its name, or it sits under an existing tenant and no service may be +// named so that - spells it. An Application added to the +// cluster without updating the lists fails here once added to the snapshot. +func TestLiveApplicationsAreProtected(t *testing.T) { + isTenant := map[string]bool{} + for _, tn := range existingTenants { + isTenant[tn] = true + } + for _, app := range liveApplications { + if tenantServices[app] || strings.HasPrefix(app, "tenant-") && isTenant[strings.TrimPrefix(app, "tenant-")] { + continue + } + owner := "" + for _, tn := range existingTenants { + if strings.HasPrefix(app, tn+"-") { + owner = tn + } + } + if owner != "" { + if !IsReservedServiceApplication(owner, strings.TrimPrefix(app, owner+"-")) { + t.Errorf("%s: under tenant %q but %q is not a reserved service name", app, owner, strings.TrimPrefix(app, owner+"-")) + } + continue + } + for _, p := range hyphenPrefixes(app) { + if !IsReservedTenantName(p) { + t.Errorf("%s: tenant name %q is not reserved", app, p) + } + } + } + // The generated tenant Applications: "tenant" cannot be a tenant. + if !IsReservedTenantName("tenant") { + t.Error(`"tenant" must be reserved`) + } +} + +func TestExistingTenantsRemainValid(t *testing.T) { + for _, tn := range existingTenants { + if IsReservedTenantName(tn) { + t.Errorf("existing tenant %q became reserved", tn) + } + if other, ok := TenantNameCollision(tn, existingTenants); ok { + t.Errorf("existing tenant %q collides with %q", tn, other) + } + } + // And every service they run today stays addressable. + for app := range tenantServices { + team, comp, _ := strings.Cut(app, "-") + if IsReservedServiceApplication(team, comp) { + t.Errorf("existing service %s/%s became reserved", team, comp) + } + } +} + +func TestIsReservedServiceApplication(t *testing.T) { + cases := []struct { + team, component string + want bool + }{ + {"root", "app", true}, + {"shared", "pg", true}, + {"argo", "workflows", true}, + {"vault", "human-auth-iac", true}, + {"tenant", "labs", true}, + {"tenant", "anything", true}, + {"labs", "oidc-zitadel", true}, + {"erpact", "control", true}, + {"erpact", "shared", true}, + {"admins", "mctl-api", true}, + {"mctl", "portal-oidc-zitadel", true}, + {"labs", "openclaw-skills", true}, + {"Root", "App", true}, + {"labs", "mctl-telegram", false}, + {"erpact", "web", false}, + {"admins", "mctl-web", false}, + {"labs", "oidc", false}, + {"acme", "app", false}, + {"", "app", false}, + } + for _, tc := range cases { + if got := IsReservedServiceApplication(tc.team, tc.component); got != tc.want { + t.Errorf("IsReservedServiceApplication(%q, %q) = %v, want %v", tc.team, tc.component, got, tc.want) + } + } +} + +// Every operation that addresses a - refuses a reserved +// pair in ValidateInput, whichever parameter names the service. +func TestValidateInputRejectsReservedServiceApplication(t *testing.T) { + reg := NewRegistry() + bodies := map[string]map[string]string{ + "retire-service": {"team_name": "root", "component_name": "app"}, + "rollback-service": {"team_name": "root", "component_name": "app", "target_tag": "1.0.0"}, + "deploy-service": {"action": "onboard", "team_name": "labs", "component_name": "oidc-zitadel"}, + "preview-deploy": {"team_name": "shared", "component_name": "pg", "image_tag": "1"}, + "provision-database": {"team_name": "root", "app_name": "app"}, + "add-custom-domain": {"team_name": "erpact", "service_name": "control", "domain": "a.example.com"}, + } + for name, body := range bodies { + op, ok := reg.Get(name) + if !ok { + t.Fatalf("%s not in registry", name) + } + errs := reg.ValidateInput(op, reg.ApplyDefaults(op, body)) + if !strings.Contains(strings.Join(errs, ";"), "reserved for platform use") { + t.Errorf("%s: reserved service not refused: %v", name, errs) + } + // The same operation on an ordinary service is untouched by it. + ok2 := map[string]string{} + for k, v := range body { + ok2[k] = v + } + ok2["team_name"] = "acme" + errs = reg.ValidateInput(op, reg.ApplyDefaults(op, ok2)) + if strings.Contains(strings.Join(errs, ";"), "reserved for platform use") { + t.Errorf("%s: ordinary service refused as reserved: %v", name, errs) + } + } +} + +// Submit refuses on its own, for callers that skip ValidateInput. +func TestSubmitRejectsReservedServiceApplication(t *testing.T) { + e := &Executor{} + reg := NewRegistry() + op, _ := reg.Get("retire-service") + res, err := e.Submit(context.Background(), op, map[string]string{"team_name": "root", "component_name": "app"}, "u", "root") + if !errors.Is(err, ErrReservedServiceApplication) || res != nil { + t.Fatalf("Submit = %v, %v; want ErrReservedServiceApplication", res, err) + } +} + +func TestTenantNameCollision(t *testing.T) { + cases := []struct { + name string + other string + want bool + }{ + {"labs-mctl", "labs", true}, // extends an existing tenant + {"admins-mctl", "admins", true}, + {"erpact-oidc", "erpact", true}, + {"ovk-x", "ovk", true}, + {"lab", "", false}, // "labs" does not start with "lab-" + {"labsx", "", false}, + {"labs", "", false}, // the same tenant is not a collision + {"karabu2", "", false}, + {"acme", "", false}, + } + for _, tc := range cases { + other, got := TenantNameCollision(tc.name, existingTenants) + if got != tc.want || other != tc.other { + t.Errorf("TenantNameCollision(%q) = %q, %v; want %q, %v", tc.name, other, got, tc.other, tc.want) + } + } + // The reverse direction: a new name that an existing tenant extends. + if other, got := TenantNameCollision("acme", []string{"acme-dev"}); !got || other != "acme-dev" { + t.Errorf(`"acme" next to "acme-dev" = %q, %v; want a collision`, other, got) + } +} + +// Teardown operations are exempt: a record that exists for a reserved pair +// must stay removable (DELETE /domains keeps a row whose teardown fails). +func TestTeardownOperationsSkipReservedPairCheck(t *testing.T) { + reg := NewRegistry() + for name, body := range map[string]map[string]string{ + "remove-custom-domain": {"team_name": "erpact", "service_name": "control", "domain": "a.example.com"}, + "preview-delete": {"team_name": "shared", "component_name": "pg", "preview_id": "abc123"}, + } { + op, _ := reg.Get(name) + if errs := reg.ValidateInput(op, reg.ApplyDefaults(op, body)); len(errs) != 0 { + t.Errorf("%s: teardown refused: %v", name, errs) + } + if err := checkServiceApplication(op, body); err != nil { + t.Errorf("%s: Submit backstop refused teardown: %v", name, err) + } + } +} + +func TestPreviewIDIsPatterned(t *testing.T) { + reg := NewRegistry() + op, _ := reg.Get("preview-delete") + for id, ok := range map[string]bool{"abc123": true, "x": true, "pr-42": true, "../x": false, "-x": false, "": false, "ABC": false} { + body := map[string]string{"team_name": "acme", "component_name": "web", "preview_id": id} + errs := reg.ValidateInput(op, body) + if (len(errs) == 0) != ok { + t.Errorf("preview_id %q: errs=%v, want ok=%v", id, errs, ok) + } + } +} + +// preview-delete skips the pair check but not the name it composes: the +// hyphen joining component and preview_id is in neither parameter. +func TestPreviewDeleteComposedNameIsChecked(t *testing.T) { + reg := NewRegistry() + op, _ := reg.Get("preview-delete") + for _, tc := range []struct { + component, id string + reserved bool + }{ + {"oidc", "zitadel", true}, // labs-oidc-zitadel + {"mctl", "api", false}, // labs-mctl-api is not platform-owned + {"web", "abc123", false}, + {"oidc-zitadel", "abc123", false}, // the pair itself is exempt for teardown + } { + body := map[string]string{"team_name": "labs", "component_name": tc.component, "preview_id": tc.id} + errs := reg.ValidateInput(op, body) + got := strings.Contains(strings.Join(errs, ";"), "reserved for platform use") + if got != tc.reserved { + t.Errorf("%s + %s: refused=%v, want %v (%v)", tc.component, tc.id, got, tc.reserved, errs) + } + if err := checkServiceApplication(op, body); errors.Is(err, ErrReservedServiceApplication) != tc.reserved { + t.Errorf("%s + %s: Submit backstop err=%v, want reserved=%v", tc.component, tc.id, err, tc.reserved) + } + } +} diff --git a/internal/operations/reserved_tenant.go b/internal/operations/reserved_tenant.go index e3f1e806..79984d82 100644 --- a/internal/operations/reserved_tenant.go +++ b/internal/operations/reserved_tenant.go @@ -76,10 +76,103 @@ var reservedTenantNames = map[string]struct{}{ } var ( - reservedTenantPrefixes = []string{"kube-", "argo", "platform-", "mctl-", "grafana-", "vault"} + reservedTenantPrefixes = []string{"kube-", "argo", "platform-", "mctl-", "grafana-", "vault", "tenant-", "otel-"} reservedTenantSuffixes = []string{"-system"} ) +// Tenant names that would collide with Argo CD Application names. +// +// A tenant's services become Argo CD Applications named - +// (the "apps" ApplicationSet), and the tenant itself becomes tenant- +// (the "tenants" ApplicationSet). Every Application lives in the argocd +// namespace, so the names share one space with the platform's own +// Applications. A tenant name that is a platform Application name, or a +// hyphen-prefix of one, would let that tenant's service names spell the +// platform Application's name. The rule is therefore structural over the list +// below: name N is reserved when N == P or P starts with N+"-" for some P in +// platformApplicationNames. "tenant" and the "tenant-" prefix are reserved +// outright for the tenants ApplicationSet. +// +// The list is the set of Argo CD Applications that are not tenant services +// and whose names do not start with an existing tenant's name. Derive it +// read-only from the cluster: +// +// kubectl get applications -n argocd -o name +// +// and drop the generated ones: tenant- (tenants ApplicationSet), +// - for every platform-gitops/services// +// directory (apps ApplicationSet), -openclaw-skills, and the +// platform Applications that sit under an existing tenant's prefix, which +// are listed in tenantScopedPlatformApplications instead. Also check +// platform-gitops/bootstrap/templates in mctl-gitops for Applications that +// are declared but not currently deployed. When this list changes, mirror +// the change in tenant_name_reserved in wft-create-tenant.yaml. +// +// Snapshot taken 2026-10-11 from the live cluster and bootstrap templates. +var platformApplicationNames = []string{ + "argo-rollouts", + "argo-workflows", + "argo-workflows-config", + "argocd-self-managed", + "cloudnative-pg", + "external-secrets", + "forgejo", + "grafana-iac", + "loki-stack", + "minio", + "monitoring", + "otel-collector", + "otel-eval-tempo", + "reflector", + "root-app", + "shared-pg", + "temporal", + "temporal-web", + "valkey", + "vault", + "vault-human-auth-iac", + "zitadel", +} + +// tenantScopedPlatformApplications are platform-managed Argo CD Applications +// whose names start with an existing tenant's name (admins, erpact, labs, +// mctl). They cannot feed the tenant-name rule above without reserving the +// existing tenants themselves, which would break re-running create-tenant for +// them. They are protected instead at the service level: no tenant operation +// may target - equal to one of them (see +// IsReservedServiceApplication), and the API refuses a new tenant name that +// is a hyphen-prefix extension of an existing tenant (TenantNameCollision). +var tenantScopedPlatformApplications = []string{ + "admins-mctl-agent", + "admins-mctl-api", + "admins-mctl-portal", + "erpact-control", + "erpact-deployer", + "erpact-infra", + "erpact-oidc-zitadel", + "erpact-shared", + "labs-oidc-zitadel", + "mctl-portal-oidc-zitadel", +} + +// reservedTenantExact are names reserved outright beyond the platform list: +// "tenant" would make the tenants ApplicationSet's tenant- Applications +// reachable as -. +var reservedTenantExact = map[string]struct{}{ + "tenant": {}, +} + +// collidesWithPlatformApplication reports whether n equals a platform +// Application name or is a hyphen-prefix of one. +func collidesWithPlatformApplication(n string) bool { + for _, p := range platformApplicationNames { + if n == p || strings.HasPrefix(p, n+"-") { + return true + } + } + return false +} + // ErrReservedTenantName is returned (wrapped) when create-tenant is asked for // a name that collides with a platform namespace. var ErrReservedTenantName = errors.New("tenant name is reserved for platform use") @@ -97,6 +190,12 @@ func IsReservedTenantName(name string) bool { if _, ok := reservedTenantNames[n]; ok { return true } + if _, ok := reservedTenantExact[n]; ok { + return true + } + if collidesWithPlatformApplication(n) { + return true + } for _, p := range reservedTenantPrefixes { if strings.HasPrefix(n, p) { return true diff --git a/internal/operations/reserved_tenant_test.go b/internal/operations/reserved_tenant_test.go index a6b09caa..39df62d2 100644 --- a/internal/operations/reserved_tenant_test.go +++ b/internal/operations/reserved_tenant_test.go @@ -76,6 +76,35 @@ var reservedCases = []struct { {"Kube-System", true, "case-insensitive"}, {"ARGOCD", true, "case-insensitive"}, + // Platform Argo CD Application names and their hyphen-prefixes: a tenant + // with one of these names could spell the Application as + // -. + {"root", true, "platform app root-app"}, + {"root-app", true, "platform app root-app"}, + {"shared", true, "platform app shared-pg"}, + {"shared-pg", true, "platform app shared-pg"}, + {"cloudnative", true, "platform app cloudnative-pg"}, + {"external", true, "platform app external-secrets"}, + {"grafana", true, "platform app grafana-iac"}, + {"loki", true, "platform app loki-stack"}, + {"otel", true, "platform app otel-collector"}, + {"otel-eval", true, "platform app otel-eval-tempo"}, + {"otel-anything", true, "prefix otel-"}, + {"reflector", true, "platform app reflector"}, + {"valkey", true, "platform app valkey"}, + {"temporal-web", true, "platform app temporal-web"}, + {"tenant", true, "tenants ApplicationSet"}, + {"tenant-x", true, "prefix tenant-"}, + + // The existing tenants stay valid: none of them is a hyphen-prefix of a + // platform Application in platformApplicationNames. + {"admins", false, "existing tenant"}, + {"erpact", false, "existing tenant"}, + {"karabu", false, "existing tenant"}, + {"labs", false, "existing tenant"}, + {"mctl", false, "existing tenant"}, + {"ovk", false, "existing tenant"}, + // Allowed: no pattern matches, including near-misses on the boundaries. {"billing", false, "normal"}, {"data-team", false, "normal"}, @@ -86,9 +115,7 @@ var reservedCases = []struct { {"team-argo", false, "argo is a prefix rule only"}, {"my-platform", false, "platform- is a prefix rule only"}, {"platform", false, "platform- needs the hyphen"}, - {"mctl", false, "mctl- needs the hyphen"}, {"mctlhq", false, "mctl- needs the hyphen"}, - {"grafana", false, "grafana- needs the hyphen"}, {"team-vault", false, "vault is a prefix rule only"}, {"system", false, "-system needs the hyphen"}, {"systems", false, "suffix -system must end the name"}, @@ -100,6 +127,10 @@ var reservedCases = []struct { {"my-monitoring", false, "exact match only"}, {"minio2", false, "exact match only"}, {"temporal-labs", false, "exact match only"}, + {"rooted", false, "root-app needs root followed by a hyphen"}, + {"sharedteam", false, "shared-pg needs shared followed by a hyphen"}, + {"team-root", false, "platform-app rule is a prefix rule only"}, + {"tenants", false, "tenant- needs the hyphen"}, } func TestIsReservedTenantName(t *testing.T) {