diff --git a/docs/commands.md b/docs/commands.md index 554ae783..0435f396 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -121,6 +121,40 @@ never `--stand-alone` or `--exclude-root`; `--exclude-dependency` and out. The selected environment's `exclude-services` also removes services from the composition roots; a module withdrawn whole contributes no root, including its service-entry. CLI and run-profile exclusions still apply on top. + +**An operation is held to the environment it selected, never refused because a +DIFFERENT environment's `exclude-services` is inconsistent with the workspace +graph.** Run, build, deploy and `deploy gitops render` each check the one +environment they were given against the graph, and a refusal names that +environment in the message itself rather than only under `--debug`. + +This is scoped to the graph-consistency check, which is where the +cross-environment refusal came from. It is not a general rule that other +environments are ignored. The check itself decodes and validates EVERY +environment's declarations before it compares the selected one against the +graph, so a **malformed** `exclude-services` anywhere — an unqualified name, a +duplicate, a service withdrawn from itself — still refuses, whatever was +selected. (Loading the workspace does not: a workspace with such a declaration +still loads, and a clean environment still selects. It is the validation that +refuses, not the load.) Module namespaces, host composition, +service-scoped names (`service-config`, `service-identity`, `service-mesh`) +and `managed-services` keys are likewise still held across every environment, +so a bad declaration in one of those blocks in an unselected environment can +still refuse a correct operation elsewhere. +This matters because the exclusion graph is the workspace's **current** +composition: an environment imported before a module overlay added a service +carries no `withdrawn-from` for it, so holding every environment to that graph +refused a correct render of a different environment — and the operator could +not satisfy it, since re-importing the offending environment requires the +product intent to declare it. + +Every CLI path that checks exclusions against the graph selects an +environment: run, build, test and deploy through the flow, `deploy gitops +render` and the solution and dev renders through the workspace validation, and +`codefly doctor workspace` through its own scope check — doctor selects and +calls the selected check directly, rather than relying on the fallback. The +whole-workspace form, which holds every environment's graph at once, remains +the behaviour for a caller that has selected none. The deterministic port hash folds every endpoint into a few thousand buckets, so a `--naming-scope` can hash two endpoints to one port and abort the run; `--temporary-ports` sidesteps the hash, under the ordering its diff --git a/docs/configuration-contract.md b/docs/configuration-contract.md index d37a17dd..b90b8194 100644 --- a/docs/configuration-contract.md +++ b/docs/configuration-contract.md @@ -243,7 +243,19 @@ environments: Nothing may still reach what is withdrawn: a service that declares a dependency on it is refused unless `withdrawn-from` names it, and so are a solution consuming one of its endpoints, an environment declaration naming it -and a configuration reference to one of its endpoints. A service the module's +and a configuration reference to one of its endpoints. **Those refusals are +judged for the environment the operation selected, and the message names that +environment**: an operation is not refused because a different environment's +`exclude-services` is inconsistent with the workspace graph. A **malformed** +exclusion is a separate matter — the check validates every environment's +declarations for shape before comparing the selected one against the graph, so +one of those refuses whatever was selected. It is the validation that refuses, +not the load: such a workspace still loads, and a clean environment still +selects. That distinction is load-bearing, because the graph a +declaration is held to is the workspace's current composition — an environment +imported before a module overlay added a service carries no `withdrawn-from` +for it, and holding every environment to that graph refuses operations on +environments whose own declarations are complete. A service the module's `interface:` exports, or names its `service-entry`, is withdrawn only with the whole module: an environment that names every service of a module leaves the module out, and its exports leave with it. Withdrawing some of its services is diff --git a/pkg/environments/excluded_services.go b/pkg/environments/excluded_services.go index 87c0a744..6ae0ccb1 100644 --- a/pkg/environments/excluded_services.go +++ b/pkg/environments/excluded_services.go @@ -161,10 +161,12 @@ func ValidateEnvironmentExclusions(ctx context.Context, workspace *resources.Wor if err != nil { return err } - if err := env.validateExclusionsAgainst(graph); err != nil { - return fmt.Errorf("environment %q: %w", env.Name, err) - } - return nil + // No "environment %q:" prefix here: validateExclusionsAgainst names the + // environment inside each message, because a prefix is a separate link in + // the error chain and the plain renderer shows only the leaf — which is how + // a `--env blue` refusal printed no environment at all and sent its reader + // to re-declare something that was already declared. + return env.validateExclusionsAgainst(graph) } // ValidateExcludedServices holds every environment's exclude-services to the @@ -188,7 +190,7 @@ func ValidateExcludedServices(ctx context.Context, workspace *resources.Workspac } } if err := env.validateExclusionsAgainst(graph); err != nil { - return fmt.Errorf("environment %q: %w", env.Name, err) + return err } } return nil @@ -253,21 +255,21 @@ func (env *Environment) validateExclusionsAgainst(graph *exclusionGraph) error { } for _, entry := range env.ExcludeServices { if !composed[entry.Service] { - return fmt.Errorf("exclude-services names %q, which the workspace does not compose", entry.Service) + return fmt.Errorf("environment %q excludes %q, which the workspace does not compose", env.Name, entry.Service) } for _, consumer := range consumers[entry.Service] { if excluded[consumer] || slices.Contains(entry.WithdrawnFrom, consumer) { continue } - return fmt.Errorf("exclude-services withdraws %q, but %s declares a dependency on it: name %s under its withdrawn-from to withdraw that edge with it, or stop excluding %s", - entry.Service, consumer, consumer, entry.Service) + return fmt.Errorf("environment %q withdraws %q, but %s declares a dependency on it: name %s under that environment's withdrawn-from to withdraw the edge with it, or stop excluding %s there", + env.Name, entry.Service, consumer, consumer, entry.Service) } for _, consumer := range entry.WithdrawnFrom { if !slices.Contains(consumers[entry.Service], consumer) { - return fmt.Errorf("exclude-services %q withdrawn-from names %s, which declares no dependency on it", entry.Service, consumer) + return fmt.Errorf("environment %q excludes %q with a withdrawn-from naming %s, which declares no dependency on it", env.Name, entry.Service, consumer) } if excluded[consumer] { - return fmt.Errorf("exclude-services %q withdrawn-from names %s, which this environment excludes too", entry.Service, consumer) + return fmt.Errorf("environment %q excludes %q with a withdrawn-from naming %s, which it excludes too", env.Name, entry.Service, consumer) } } } @@ -277,7 +279,7 @@ func (env *Environment) validateExclusionsAgainst(graph *exclusionGraph) error { for index := range graph.consumed { consumption := &graph.consumed[index] if excluded[resources.ServiceUnique(consumption.Module, consumption.Service)] { - return fmt.Errorf("solution %s consumes %s/%s/%s (api.consumes), which exclude-services withdraws from this environment", consumption.Solution, consumption.Module, consumption.Service, consumption.Endpoint) + return fmt.Errorf("solution %s consumes %s/%s/%s (api.consumes), which environment %q withdraws", consumption.Solution, consumption.Module, consumption.Service, consumption.Endpoint, env.Name) } } for _, named := range env.serviceDeclarations() { @@ -290,7 +292,7 @@ func (env *Environment) validateExclusionsAgainst(graph *exclusionGraph) error { } } if excluded[target] { - return fmt.Errorf("%s names %s, which exclude-services withdraws from this environment: drop the declaration, or stop excluding %s", named.block, named.name, target) + return fmt.Errorf("%s names %s, which environment %q withdraws: drop the declaration, or stop excluding %s there", named.block, named.name, env.Name, target) } } return nil diff --git a/pkg/environments/excluded_services_test.go b/pkg/environments/excluded_services_test.go index 48bf62ea..4ecd1fa2 100644 --- a/pkg/environments/excluded_services_test.go +++ b/pkg/environments/excluded_services_test.go @@ -116,7 +116,7 @@ func TestExcludeServicesRefusesWhatStillConsumesIt(t *testing.T) { "a withdrawal from a non-consumer": {` exclude-services: - service: shop/vault withdrawn-from: [shop/api, shop/cache] -`, "withdrawn-from names shop/cache, which declares no dependency on it"}, +`, `environment "staging" excludes "shop/vault" with a withdrawn-from naming shop/cache, which declares no dependency on it`}, "a service the workspace does not compose": {` exclude-services: - service: shop/vaults `, `"shop/vaults", which the workspace does not compose`}, @@ -221,7 +221,7 @@ service-dependencies: reloaded, err := resources.LoadWorkspaceFromDir(context.Background(), root) require.NoError(t, err) err = environments.ValidateWorkspace(context.Background(), reloaded) - require.ErrorContains(t, err, `exclude-services withdraws "shop/cache", but portal/web declares a dependency on it`) + require.ErrorContains(t, err, `environment "staging" withdraws "shop/cache", but portal/web declares a dependency on it`) } // A module naming the excluded service its entry routes the host to a service @@ -313,3 +313,86 @@ func TestCoordinateContractCarriesExcludedServices(t *testing.T) { env.ExcludeServices[0].WithdrawnFrom[0] = "changed/consumer" require.Equal(t, "saas/accounts", contract.Environment.ExcludeServices[0].WithdrawnFrom[0]) } + +// TestValidateWorkspaceHoldsOnlyTheSelectedEnvironment exercises the changed +// path directly: environments.ValidateWorkspace, which the gitops render +// callers reach. An earlier regression for this went through NewFlow, which +// already used ValidateEnvironmentExclusions at flow.go:484 — so it passed +// against unfixed main and proved nothing about ValidateWorkspace. +// +// The fixture gives `staging` an exclusion the graph refuses (shop/api still +// consumes shop/vault) and leaves `local` clean. A render of `local` must not +// inherit staging's refusal; a render of `staging` must still get it; and a +// caller that selects nothing must still hold the whole workspace. +// +// (`doctor workspace` is NOT such a caller: it selects an environment and +// calls ValidateEnvironmentExclusions directly. The no-selection fallback is +// for a caller whose subject really is the whole workspace.) +func TestValidateWorkspaceHoldsOnlyTheSelectedEnvironment(t *testing.T) { + ctx := context.Background() + workspace := writeExclusionWorkspace(t, ` exclude-services: + - service: shop/vault +`) + declared, err := environments.FromWorkspace(workspace) + require.NoError(t, err) + byName := map[string]*environments.Environment{} + for _, env := range declared { + if env != nil { + byName[env.Name] = env + } + } + require.Contains(t, byName, "staging") + require.Contains(t, byName, "local") + require.Empty(t, byName["local"].ExcludeServices, "local is the clean environment") + + // The selected environment is the inconsistent one: still refused, so this + // test cannot pass by the check having been disabled. + err = environments.ValidateWorkspace(ctx, workspace, byName["staging"]) + require.Error(t, err) + require.ErrorContains(t, err, `environment "staging" withdraws "shop/vault"`) + + // The selected environment is clean: must NOT be refused over staging's + // declaration. This is the assertion that fails before the fix. + require.NoError(t, environments.ValidateWorkspace(ctx, workspace, byName["local"]), + "a selected environment must not be refused over another environment's exclusion") + + // Nothing selected: the whole workspace is still held, preserving the + // default for a caller whose subject is the workspace itself. + err = environments.ValidateWorkspace(ctx, workspace) + require.Error(t, err, "with no selection every environment is still held") + require.ErrorContains(t, err, `environment "staging" withdraws "shop/vault"`) + + // A nil selection is the same as none, not a silent skip. + err = environments.ValidateWorkspace(ctx, workspace, nil) + require.Error(t, err, "a nil selection must not skip the check") +} + +// TestSelectionNarrowsTheGraphCheckAndNothingElse pins the limit of the +// change, so the prose describing it cannot drift past the code. +// +// Selecting an environment narrows the GRAPH-consistency check and only that. +// ValidateWorkspace decodes and validates every environment's declarations +// before it compares the selected one against the graph, so a MALFORMED +// exclusion — an unqualified name, here, in an environment nobody selected — +// still refuses. Note what the assertions below show about where that +// happens: the workspace LOADS and the clean environment SELECTS; it is the +// validation that refuses, not the load. That is deliberate — a declaration +// that cannot be parsed is not a disagreement between an environment and the +// graph, it is a workspace that does not say what it means. +func TestSelectionNarrowsTheGraphCheckAndNothingElse(t *testing.T) { + ctx := context.Background() + workspace := writeExclusionWorkspace(t, ` exclude-services: + - service: vault +`) + // The clean environment selects fine — Select decodes only the one it is + // asked for, so the malformed declaration elsewhere does not block it. + local, err := environments.Select(workspace, "local") + require.NoError(t, err) + + // But validating with that selection still refuses, because the first + // thing the check does is decode EVERY environment. This is the boundary: + // the selection narrows the graph comparison, not the parsing. + err = environments.ValidateWorkspace(ctx, workspace, local) + require.Error(t, err, "a malformed exclusion elsewhere still refuses a selected, clean environment") + require.ErrorContains(t, err, "exclude-services service") +} diff --git a/pkg/environments/workspace.go b/pkg/environments/workspace.go index 8e5807bb..ff5bfaa1 100644 --- a/pkg/environments/workspace.go +++ b/pkg/environments/workspace.go @@ -191,7 +191,36 @@ func validateResourcePathComponent(kind, name string) error { // it earlier. // Loading the graph is why this is a pass separate from postLoad, mirroring // ValidateServiceDependencies. -func ValidateWorkspace(ctx context.Context, workspace *resources.Workspace) error { +// A selected environment narrows the exclusion check to that environment. The +// exclusion graph is built from the workspace's CURRENT composition +// (loadExclusionGraph: LoadServices, LoadModules, ConsumedEndpoints), so +// holding EVERY environment to it refuses a flow over a declaration that is +// not its own: an environment imported before a module overlay added a service +// has no withdrawal for that service, and a render of a different, correct +// environment then fails over it. Blue hit exactly that — `render --env blue` +// refused over `staging`, whose composition predates robin/sandbox. +// +// So a caller that has selected an environment passes it, and gets +// ValidateEnvironmentExclusions' contract: "a flow is held to its own +// environment's GRAPH, never refused over another's". A caller with no +// selection passes none and keeps holding every environment, which is right +// for a check whose subject IS the workspace. `doctor workspace` is not that +// caller: it selects, and calls ValidateEnvironmentExclusions directly +// (cmd/doctor_workspace.go) rather than relying on this fallback. +// +// What the selection narrows is EXACTLY the graph-consistency check, and +// nothing else in this function: +// +// - FromWorkspace above decodes and validates every environment, including +// the SHAPE of its exclude-services (qualified names, duplicates, +// self-withdrawal). A malformed exclusion anywhere still refuses here. +// - module namespaces, host composition, service-scoped names and +// managed-service keys are all still held across every environment. +// +// Those remain whole-workspace deliberately — they are invariants of the +// workspace, not of a selection — but they are the same cross-environment +// shape, so narrowing them is a separate decision, not an oversight here. +func ValidateWorkspace(ctx context.Context, workspace *resources.Workspace, selected ...*Environment) error { environments, err := FromWorkspace(workspace) if err != nil { return err @@ -208,7 +237,7 @@ func ValidateWorkspace(ctx context.Context, workspace *resources.Workspace) erro return w.Wrap(hostErr) } } - if excludedErr := ValidateExcludedServices(ctx, workspace); excludedErr != nil { + if excludedErr := validateSelectedExclusions(ctx, workspace, selected); excludedErr != nil { return w.Wrap(excludedErr) } needsGraph := false @@ -380,3 +409,17 @@ func validateModuleNamespaces(workspace *resources.Workspace, environments []*En } return nil } + +// validateSelectedExclusions holds exclude-services to the workspace graph for +// the one selected environment, or for every environment when nothing was +// selected. A nil selection is the same as none: a caller that has no +// environment must not silently skip the check. +func validateSelectedExclusions(ctx context.Context, workspace *resources.Workspace, selected []*Environment) error { + for _, env := range selected { + if env == nil { + continue + } + return ValidateEnvironmentExclusions(ctx, workspace, env) + } + return ValidateExcludedServices(ctx, workspace) +} diff --git a/pkg/gitops/dev.go b/pkg/gitops/dev.go index 1d0416a0..d9dc6402 100644 --- a/pkg/gitops/dev.go +++ b/pkg/gitops/dev.go @@ -134,7 +134,7 @@ func buildRenderedServiceImages( rebuild bool, sink orchestration.OutputSink, ) ([]string, error) { - if err := environments.ValidateWorkspace(ctx, workspace); err != nil { + if err := environments.ValidateWorkspace(ctx, workspace, env); err != nil { return nil, err } if err := prepareSnapshotRegistry(ctx, env); err != nil { diff --git a/pkg/gitops/excluded_services_test.go b/pkg/gitops/excluded_services_test.go index f0faec6e..5d66d006 100644 --- a/pkg/gitops/excluded_services_test.go +++ b/pkg/gitops/excluded_services_test.go @@ -62,6 +62,60 @@ func TestRenderModuleLeavesAnExcludedServiceOutOfTheTreeAndTheInventory(t *testi `rendered unit "store" is a service environment "staging" excludes`) } +// TestRenderModuleIsNotRefusedOverAnotherEnvironmentsExclusion is the render +// caller's half of the Blue defect, and it is the one a direct test of +// ValidateWorkspace cannot give: it fails if RenderModule stops passing the +// environment it was given, however correct the validator underneath is. +// +// It covers that one entry. The other three callers that pass a selection — +// orchestrate.go's second call, dev.go and solution.go — are reviewed source, +// not covered here. +// +// `stale` excludes shop/store while shop/api still consumes it and withdraws +// nothing, so that environment genuinely does not hold up against the +// workspace graph. Rendering `staging`, whose own declaration is clean, must +// still succeed — the exclusion graph is built from the workspace's CURRENT +// composition, so an environment imported before a module overlay added a +// service carries no withdrawal for it, and an operator cannot repair an +// environment the product intent does not declare. +func TestRenderModuleIsNotRefusedOverAnotherEnvironmentsExclusion(t *testing.T) { + installFakeAgents(t) + root := t.TempDir() + require.NoError(t, copyTree(filepath.Join("testdata", "container-ports"), root)) + path := filepath.Join(root, resources.WorkspaceConfigurationName) + data, err := os.ReadFile(path) + require.NoError(t, err) + data = append(data, []byte(` - name: stale + namespace: acme + registry: + url: registry.example.com/acme + exclude-services: + - service: shop/store +`)...) + require.NoError(t, os.WriteFile(path, data, 0o644)) + ctx := context.Background() + workspace, err := resources.LoadWorkspaceFromDir(ctx, root) + require.NoError(t, err) + module, err := workspace.LoadModuleFromName(ctx, "shop") + require.NoError(t, err) + + // The other environment really is inconsistent with the graph: selecting + // it is refused. Without this the next assertion would pass vacuously. + stale, err := orchestration.SelectEnvironment(workspace, "stale") + require.NoError(t, err) + _, err = RenderModule(ctx, workspace, module, stale, "acme-stale", nil) + require.ErrorContains(t, err, "shop/store", + "the inconsistent environment must still be refused when it is the selected one") + + // Selecting the clean environment must not inherit that refusal. + staging, err := orchestration.SelectEnvironment(workspace, "staging") + require.NoError(t, err) + result, err := RenderModule(ctx, workspace, module, staging, "acme-staging", nil) + require.NoError(t, err, "a render is held to its own environment, never refused over another's") + require.DirExists(t, filepath.Join(result.Path, "services", "store"), + "and the service the OTHER environment excludes is rendered here") +} + // A module the environment withdraws whole renders nothing there: the render // refuses it by name instead of producing an empty tree. func TestRenderModuleRefusesAModuleTheEnvironmentWithdrawsWhole(t *testing.T) { diff --git a/pkg/gitops/orchestrate.go b/pkg/gitops/orchestrate.go index 45ae2375..1b99a542 100644 --- a/pkg/gitops/orchestrate.go +++ b/pkg/gitops/orchestrate.go @@ -46,7 +46,7 @@ func renderModuleTree( if err := selectionguard.RejectUnboundExecution(workspace.Dir(), module.Dir()); err != nil { return RenderResult{}, err } - if err := environments.ValidateWorkspace(ctx, workspace); err != nil { + if err := environments.ValidateWorkspace(ctx, workspace, env); err != nil { return RenderResult{}, err } if env.WithdrawsModule(module) { @@ -618,7 +618,7 @@ func RenderService(ctx context.Context, workspace *resources.Workspace, module * } func renderService(ctx context.Context, workspace *resources.Workspace, module *resources.Module, service *resources.Service, env *environments.Environment, project string, build renderBuild, sink orchestration.OutputSink) (RenderResult, error) { - if err := environments.ValidateWorkspace(ctx, workspace); err != nil { + if err := environments.ValidateWorkspace(ctx, workspace, env); err != nil { return RenderResult{}, err } if err := CheckCompositionPermissions(ctx, workspace, env, true, module.Name); err != nil { diff --git a/pkg/gitops/solution.go b/pkg/gitops/solution.go index ad7b3e72..ab82414b 100644 --- a/pkg/gitops/solution.go +++ b/pkg/gitops/solution.go @@ -59,7 +59,7 @@ func RenderSolution(ctx context.Context, req *SolutionRenderRequest) (RenderResu if err := selectionguard.RejectUnboundExecution(req.Workspace.Dir(), req.Source); err != nil { return RenderResult{}, err } - if err := environments.ValidateWorkspace(ctx, req.Workspace); err != nil { + if err := environments.ValidateWorkspace(ctx, req.Workspace, req.Environment); err != nil { return RenderResult{}, err } env := req.Environment diff --git a/pkg/orchestration/excluded_services_test.go b/pkg/orchestration/excluded_services_test.go index 70ad63af..59bfc74e 100644 --- a/pkg/orchestration/excluded_services_test.go +++ b/pkg/orchestration/excluded_services_test.go @@ -122,3 +122,54 @@ func TestPlanRefusesARootGroupReferenceToAnExcludedService(t *testing.T) { require.ErrorContains(t, err, "from this environment (exclude-services") require.ErrorContains(t, err, "platform/authority") } + +// TestFlowIsNotRefusedOverAnotherEnvironmentsExclusion is the Blue render +// defect: a flow selecting one environment was refused over a DIFFERENT +// environment's exclude-services. +// +// The exclusion graph is built from the workspace's current composition, so an +// environment imported before a module overlay added a service carries no +// withdrawal for it. Holding every environment to that graph meant a correct +// `--env blue` render failed over `staging`, whose composition predates the +// service — and the operator could not fix it from the declaration side, +// because the product intent declares no `staging` to re-import. +// +// Here `stale` excludes shop/vault while shop/api still consumes it, so +// `stale` genuinely does not hold up. Selecting `staging`, whose own +// declaration is clean, must still succeed. +func TestFlowIsNotRefusedOverAnotherEnvironmentsExclusion(t *testing.T) { + ctx := context.Background() + files := exclusionWorkspaceFiles("") + files["workspace.codefly.yaml"] = `name: acme +layout: modules +modules: + - name: shop +environments: + - name: staging + namespace: acme + - name: stale + namespace: acme + exclude-services: + - service: shop/vault +` + workspace := writeTempWorkspace(t, files) + module, err := workspace.LoadModuleFromName(ctx, "shop") + require.NoError(t, err) + api, err := module.LoadServiceFromName(ctx, "api") + require.NoError(t, err) + + // The other environment really is inconsistent with the graph: selecting it + // refuses, which is what makes the next assertion meaningful rather than + // vacuous. + stale, err := SelectEnvironment(workspace, "stale") + require.NoError(t, err) + _, err = NewFlow(ctx, workspace, module, api, stale, RunMode) + require.ErrorContains(t, err, "declares a dependency on it", + "the inconsistent environment must still refuse when it is the selected one") + + // Selecting the clean environment must not inherit that refusal. + staging, err := SelectEnvironment(workspace, "staging") + require.NoError(t, err) + _, err = NewFlow(ctx, workspace, module, api, staging, RunMode) + require.NoError(t, err, "a flow is held to its own environment, never refused over another's") +}