diff --git a/cmd/doctor_workspace.go b/cmd/doctor_workspace.go index f88ace6a..fb051bff 100644 --- a/cmd/doctor_workspace.go +++ b/cmd/doctor_workspace.go @@ -1380,7 +1380,10 @@ func checkServiceConfigurationSources(ctx context.Context, env *environments.Env // module's, is reported and returns nil. Duplicate keys within one // configuration are flagged as for a service directory. func loadWorkspaceConfigurations(ctx context.Context, ws *resources.Workspace, env *resources.Environment, relCfgDir string, report *workspaceReadinessReport) *configurations.WorkspaceConfigurations { - provided, err := configurations.ReadWorkspaceConfigurations(ctx, ws, env) + // The same acquisition a run supplies, so the doctor reports what a run + // would deliver for a bound module artifact rather than core's + // "requires a resolver" refusal. + provided, err := configurations.ReadWorkspaceConfigurations(ctx, ws, env, orchestration.ModuleArtifactResolverFor(ws)) if err != nil { code := codeConfigurationInvalid if errors.Is(err, configurations.ErrConfigurationConflict) { diff --git a/cmd/environment/import.go b/cmd/environment/import.go index de5b855b..f94fb096 100644 --- a/cmd/environment/import.go +++ b/cmd/environment/import.go @@ -12,6 +12,7 @@ import ( "github.com/codefly-dev/cli/cmd/common" "github.com/codefly-dev/cli/pkg/cli/yamledit" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/resources" "github.com/codefly-dev/core/shared" "github.com/pmezard/go-difflib/difflib" @@ -281,7 +282,7 @@ func runImport(ctx context.Context, opts *importOptions) error { if err := environments.ValidateEnvironmentExclusions(ctx, workspace, selected); err != nil { return fmt.Errorf("product intent: %w", err) } - if err := environments.ValidateSecretPolicy(ctx, workspace, selected, intent.ServiceSecrets); err != nil { + if err := environments.ValidateSecretPolicy(ctx, workspace, selected, intent.ServiceSecrets, orchestration.ModuleArtifactResolverFor(workspace)); err != nil { return fmt.Errorf("product intent: %w", err) } } diff --git a/cmd/generate/runnable_bindings.go b/cmd/generate/runnable_bindings.go index 5daaf89f..a833ccd3 100644 --- a/cmd/generate/runnable_bindings.go +++ b/cmd/generate/runnable_bindings.go @@ -15,6 +15,7 @@ import ( "github.com/codefly-dev/cli/cmd/common" "github.com/codefly-dev/cli/pkg/cli" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/cli/pkg/remotenetwork" runnablespkg "github.com/codefly-dev/cli/pkg/runnables" "github.com/codefly-dev/core/composition" @@ -655,6 +656,10 @@ func newEndpointResolver(ctx context.Context, workspace *resources.Workspace, en if err != nil { return nil, err } + // The same acquisition every other delivery and check load supplies: a + // bound module artifact reaching this manager must be resolved from the + // selected module, not refused as unresolvable (module_artifact.go). + reader.WithModuleArtifactResolver(orchestration.ModuleArtifactResolverFor(workspace)) manager.WithLoader(reader) if resolver.remote, err = remotenetwork.NewRemoteManager(ctx, manager); err != nil { return nil, err diff --git a/cmd/generate/runnables.go b/cmd/generate/runnables.go index 4ba41af2..7e029269 100644 --- a/cmd/generate/runnables.go +++ b/cmd/generate/runnables.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "io/fs" "os" "path/filepath" "slices" @@ -159,6 +160,9 @@ type derivation struct { files map[string][]byte publication *modulecontract.Contract publicationErr error + // publicationAbsent means the module publishes no contract AT ALL, as + // distinct from publishing one that could not be read. + publicationAbsent bool // release is the module's own release version, empty for a module that // declares no package manifest. release string @@ -271,6 +275,13 @@ func deriveRunnablesIn(ctx context.Context, workspaceName, workspaceDir string, // Load the selected owner's declaration once. A module with no operations // still produces an empty index; an operation cannot publish without it. derived.publication, derived.publicationErr = modulecontract.Load(module.Dir()) + // Whether the module HAS a declaration is a different question from + // whether it could be read, and only an existence observation answers it. + // Core's loader reads through os.Root, which reports a dangling in-root + // symlink as fs.ErrNotExist — so the error alone cannot tell "this module + // publishes no contract" from "its contract is a broken link". Core keeps + // owning the content; this is only the entry. + derived.publicationAbsent = derived.publicationErr != nil && !publicationEntryExists(module.Dir()) var endpoints []composition.APIContractEndpoint if catalog != nil { @@ -546,16 +557,60 @@ func (d *derivation) audience() string { // publication principal is independent of the producer checkout's module name; // an explicitly declared operation must agree, never be silently rewritten. func (d *derivation) record(endpoint *composition.APIContractEndpoint, name string, pkg *basev0.RunnablePackage, spec *corerunnable.OperationSpec, origin derivedOperation) error { - if d.publicationErr != nil { - return fmt.Errorf("runnable %s/%s/%s needs its owner's module contract: %w", endpoint.Service, endpoint.Endpoint, name, d.publicationErr) + // An unreadable or malformed contract is never absence. A module that + // authored one and got it wrong must be told so, whatever the operation — + // turning a schema skew, a bad field or a dangling link into "no module + // publication applies" would admit exactly the declarations the strict + // reader exists to refuse. + if d.publicationErr != nil && !d.publicationAbsent { + return fmt.Errorf("runnable %s/%s/%s cannot read its owner's module contract: %w", endpoint.Service, endpoint.Endpoint, name, d.publicationErr) } - if d.publication == nil || !slices.ContainsFunc(d.publication.Destinations, func(destination modulecontract.Destination) bool { - return destination.Service == endpoint.Service && destination.Endpoint == endpoint.Endpoint - }) { - return fmt.Errorf("module contract does not publish runnable destination %s/%s", endpoint.Service, endpoint.Endpoint) - } - if spec.Audience != d.publication.Principal { - return fmt.Errorf("runnable %s/%s/%s audience %q differs from module contract principal %q", endpoint.Service, endpoint.Endpoint, name, spec.Audience, d.publication.Principal) + switch { + case d.publicationErr == nil: + // A real contract exists, so the module's publication applies to every + // operation it serves — including an explicit one. Its destinations are + // the endpoints it exposes, and its principal is the one audience its + // credentials are issued to: an explicit audience that differs from it + // is a conflict in the module's own declarations, not host permission, + // and is refused rather than reinterpreted. + if !slices.ContainsFunc(d.publication.Destinations, func(destination modulecontract.Destination) bool { + return destination.Service == endpoint.Service && destination.Endpoint == endpoint.Endpoint + }) { + return fmt.Errorf("module contract does not publish runnable destination %s/%s", endpoint.Service, endpoint.Endpoint) + } + if spec.Audience != d.publication.Principal { + return fmt.Errorf("runnable %s/%s/%s audience %q differs from module contract principal %q", endpoint.Service, endpoint.Endpoint, name, spec.Audience, d.publication.Principal) + } + case !origin.explicit: + // Absent, and this operation's audience was DERIVED. + // No contract at all, and this operation's audience was DERIVED — core + // defaults it from ServiceOwner.Audience, which this generation fills + // from the module publication. With no publication there is no + // principal to derive from, so the audience would be empty: refuse, + // naming what is missing. + return fmt.Errorf("runnable %s/%s/%s derives its audience by default and so needs its owner's module contract: %w", endpoint.Service, endpoint.Endpoint, name, d.publicationErr) + default: + // No contract, and the method carries the operation option — which is + // what core's OperationSpec.Declared marks: the OPTION's presence, not + // the presence of an audience in it. So the audience is the author's + // own, read from that option rather than derived from any module + // principal: a host surface a module reaches without being installed as + // one. Core does not require it to equal a principal, and requiring a + // contract here demanded a module identity the module does not have. + // + // Core's OperationFromMethod validates the audience before it sets + // Declared, so an annotated method with an empty audience does not + // reach here; the check is kept anyway, because this branch is the one + // place an audience is accepted without a principal to compare it + // against, and it must not be the one that lets an empty one through. + // + // The served-target, scope and lookup checks upstream are unchanged, so + // an unserved method or a missing paired Lookup still refuses, and the + // explicit audience and scopes are carried through exactly as core + // derived them. + if spec.Audience == "" { + return fmt.Errorf("runnable %s/%s/%s carries the operation option but declares no audience, and its owner publishes no module contract", endpoint.Service, endpoint.Endpoint, name) + } } operation := pkg.GetServiceOperations()[0] packageBytes, err := runnablespkg.CanonicalJSON(pkg) @@ -708,3 +763,24 @@ func kebab(name string) string { func methodName(fullMethod string) string { return fullMethod[strings.LastIndex(fullMethod, "/")+1:] } + +// publicationEntryExists reports whether the module directory holds a +// module-contract entry, whatever the state of what it points at. +// +// Lstat, not Stat: a dangling symlink IS a declaration the module authored, +// and following it would report the same "not found" that Core's loader +// already reports — which is exactly the conflation this exists to break. +// Confined through os.Root like every other read of a composed module's tree, +// and any failure to observe is read as "present", so an unobservable entry +// refuses rather than passing as absence. +func publicationEntryExists(moduleDir string) bool { + root, err := os.OpenRoot(moduleDir) + if err != nil { + return true + } + defer func() { _ = root.Close() }() + if _, err := root.Lstat(modulecontract.FileName); err != nil { + return !errors.Is(err, fs.ErrNotExist) + } + return true +} diff --git a/cmd/generate/runnables_audience_test.go b/cmd/generate/runnables_audience_test.go index 7270322a..075c69b7 100644 --- a/cmd/generate/runnables_audience_test.go +++ b/cmd/generate/runnables_audience_test.go @@ -35,12 +35,12 @@ func TestRunnableAudienceRefusalsLeaveOutputUnchanged(t *testing.T) { edit func(*testing.T, string) want string }{ - {"missing", func(t *testing.T, dir string) { - require.NoError(t, os.Remove(filepath.Join(dir, modulecontract.FileName))) - }, "needs its owner's module contract"}, + // A contract that exists and cannot be read is never absence: a module + // that authored one and got it wrong is told so, whatever kind of + // operation it serves. {"malformed", func(t *testing.T, dir string) { writeFixtureFile(t, filepath.Join(dir, modulecontract.FileName), []byte("schema: codefly/module-contract/v1\nprincipal: documents\nunknown: true\n")) - }, "needs its owner's module contract"}, + }, "cannot read its owner's module contract"}, {"uncovered destination", func(t *testing.T, dir string) { saveRunnablePublication(t, dir, "documents.ingestion", modulecontract.Destination{ID: "other", Service: "runtime-worker", Endpoint: "other", Kind: "module"}) }, "does not publish runnable destination runtime-worker/grpc"}, @@ -54,7 +54,7 @@ func TestRunnableAudienceRefusalsLeaveOutputUnchanged(t *testing.T) { writeFixtureFile(t, outside, data) require.NoError(t, os.Remove(filepath.Join(dir, modulecontract.FileName))) require.NoError(t, os.Symlink(outside, filepath.Join(dir, modulecontract.FileName))) - }, "needs its owner's module contract"}, + }, "cannot read its owner's module contract"}, } { t.Run(test.name, func(t *testing.T) { ctx := context.Background() @@ -80,3 +80,106 @@ func TestRunnableAudienceChecksOpenAPIPublication(t *testing.T) { require.ErrorContains(t, RunnablesCmd.RunE(RunnablesCmd, []string{"releases"}), "differs from module contract principal") require.Equal(t, before, snapshotTree(t, moduleDir)) } + +// TestRunnableExplicitAudienceNeedsNoModuleContract is the SaaS case, and the +// behaviour that replaces a universal gate. +// +// The author annotated the method's audience explicitly (core sets +// OperationSpec.Declared from the method option), so the audience is theirs — +// a host surface the module reaches without being installed as one. Core does +// not require it to equal any module principal, and demanding a module +// contract here demanded an identity the module does not have. Generation +// proceeds and carries the explicit audience, its scopes and its lookup +// through unchanged. +func TestRunnableExplicitAudienceNeedsNoModuleContract(t *testing.T) { + ctx := context.Background() + root, moduleDir := saveRunnableFixture(t, ctx, descriptorSet(t, ingestionFile(conformingOperation())), "0.1.0") + t.Chdir(root) + resetRunnablesFlags(t) + require.NoError(t, os.Remove(filepath.Join(moduleDir, modulecontract.FileName))) + + require.NoError(t, RunnablesCmd.RunE(RunnablesCmd, []string{"documents"})) + + data, err := os.ReadFile(filepath.Join(moduleDir, filepath.FromSlash(runnablespkg.DerivedDir), + "runtime-worker", "grpc", "ApplyText", runnablespkg.OperationFileName)) + require.NoError(t, err) + var operation map[string]any + require.NoError(t, json.Unmarshal(data, &operation)) + require.Equal(t, "documents.ingestion", operation["audience"], "the explicit audience is preserved, not derived") + require.Equal(t, "/documents.ingest.v1.IngestionService/LookupText", operation["lookup_method"], "and its lookup") + require.NotEmpty(t, operation["invoke_scopes"], "and its scopes") + require.NotEmpty(t, operation["lookup_scopes"]) +} + +// TestRunnableDefaultAudienceStillNeedsTheModuleContract keeps the waiver +// narrow. A method with no explicit audience takes core's default from +// ServiceOwner.Audience, which this generation fills from the module +// publication — so with no contract there is no principal to derive from, and +// it refuses by name rather than emitting an empty audience. +func TestRunnableDefaultAudienceStillNeedsTheModuleContract(t *testing.T) { + ctx := context.Background() + file := ingestionFile(conformingOperation(), hopPolicyMethod("CountText", "documents", "read")) + root, moduleDir := saveRunnableFixture(t, ctx, withHopPolicy(t, descriptorSet(t, file)), "0.1.0") + t.Chdir(root) + resetRunnablesFlags(t) + require.NoError(t, RunnablesCmd.RunE(RunnablesCmd, []string{"documents"})) + before := snapshotTree(t, moduleDir) + + require.NoError(t, os.Remove(filepath.Join(moduleDir, modulecontract.FileName))) + err := RunnablesCmd.RunE(RunnablesCmd, []string{"documents"}) + require.ErrorContains(t, err, "derives its audience by default and so needs its owner's module contract") + require.Equal(t, before, snapshotTree(t, moduleDir), "a refusal leaves the output unchanged") +} + +// TestRunnableDanglingModuleContractIsNotAbsence closes the gap between "no +// declaration" and "a declaration that cannot be read". +// +// Core's loader reads through os.Root, and a dangling symlink INSIDE the module +// reports fs.ErrNotExist — the same error as a module that published nothing. +// Treating that as absence would let a broken declaration take the explicit- +// audience path, which is the one path that accepts an audience with no +// principal to check it against. The entry exists, so generation refuses and +// writes nothing. +func TestRunnableDanglingModuleContractIsNotAbsence(t *testing.T) { + ctx := context.Background() + root, moduleDir := saveRunnableFixture(t, ctx, descriptorSet(t, ingestionFile(conformingOperation())), "0.1.0") + t.Chdir(root) + resetRunnablesFlags(t) + require.NoError(t, RunnablesCmd.RunE(RunnablesCmd, []string{"documents"})) + before := snapshotTree(t, moduleDir) + + // A link to a sibling that does not exist: in-root, so os.Root resolves it + // rather than refusing the escape, and finds nothing. + contract := filepath.Join(moduleDir, modulecontract.FileName) + require.NoError(t, os.Remove(contract)) + require.NoError(t, os.Symlink("absent.module.contract.codefly.yaml", contract)) + entry, err := os.Lstat(contract) + require.NoError(t, err, "the declaration entry exists") + require.NotZero(t, entry.Mode()&os.ModeSymlink) + _, statErr := os.Stat(contract) + require.True(t, os.IsNotExist(statErr), "and what it points at does not") + + err = RunnablesCmd.RunE(RunnablesCmd, []string{"documents"}) + require.ErrorContains(t, err, "cannot read its owner's module contract") + require.Equal(t, before, snapshotTree(t, moduleDir), "a refusal leaves the output unchanged") +} + +// TestRunnableAnnotatedMethodWithNoAudienceRefuses pins the floor under the +// explicit branch. Core's OperationFromMethod validates the audience before it +// sets Declared, so this should be unreachable through core — which is the +// point: the branch that accepts an audience without a principal must not be +// the one that accepts an empty one, whatever reaches it. +func TestRunnableAnnotatedMethodWithNoAudienceRefuses(t *testing.T) { + ctx := context.Background() + operation := conformingOperation() + operation.Audience = "" + root, moduleDir := saveRunnableFixture(t, ctx, descriptorSet(t, ingestionFile(operation)), "0.1.0") + t.Chdir(root) + resetRunnablesFlags(t) + require.NoError(t, os.Remove(filepath.Join(moduleDir, modulecontract.FileName))) + before := snapshotTree(t, moduleDir) + + require.Error(t, RunnablesCmd.RunE(RunnablesCmd, []string{"documents"}), + "an annotated method with no audience cannot publish without a principal") + require.Equal(t, before, snapshotTree(t, moduleDir), "and writes nothing") +} diff --git a/docs/commands.md b/docs/commands.md index 543eccbc..15350f9e 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -152,6 +152,41 @@ run selected (`--env`), the excluded root's file included. A writer handed no environment refuses by name rather than writing a document no reader could find. +**What `--output-env` can represent, and what it refuses.** The file is raw +`KEY=value` lines, and the SDK reader that imports it +(`LoadRuntimeEnvironmentFile`) scans lines, strips one trailing CR, cuts at the +first `=`, and refuses the **whole** artifact if any name is not `CODEFLY__` +plus at least one character of `A`–`Z`, `0`–`9` or `_`. So before its first +write the run holds the complete output batch — own, workspace, dependency and +runtime configuration, `--set` and derived overrides, identity, fixture and +endpoint records alike — to exactly that grammar, and refuses any record it +cannot represent: + +- a name outside the reader's grammar (no `CODEFLY__` prefix, no suffix, + lowercase, hyphen, whitespace, non-ASCII, or carrying `=`, CR, LF or NUL); +- a **value** containing LF, CR or NUL, whichever record it belongs to. A value + carrying a line break is the reason the rule covers every record and not just + the interesting ones: the reader would read its tail as a further `KEY=value` + line, so an unrelated note could substitute a value the run never offered. + +Empty values, and values holding one or many `=`, remain representable — the +reader splits on the first separator only. A refused batch leaves an absent +file absent and an existing one byte-identical; a malformed record already in +an existing file is refused rather than rewritten under this run's name. +Direct process delivery is unaffected and still carries a value's exact bytes, +line breaks included: this is a limit of the raw file format, not of +configuration. + +Two consequences worth stating plainly. An override whose key is not a +`CODEFLY__` carrier (`--set FOO=bar`) now refuses the export rather than +writing a file the reader would reject wholesale. And a **structured +configuration document** cannot be exported this way at all: its carrier is a +prefix plus a lowercase hex SHA256, which that grammar does not admit, so the +reader has never been able to load such a file. The CLI now says so before +writing instead of reporting success. Making it exportable needs an uppercase +document carrier in core, or a reader in the SDK that accepts this one; neither +is the CLI's to decide. + **The delivery host's federation surface.** In a host environment the native run hands the service `host.delivery` names, when it declares the `federation` workspace configuration, the three keys that turn its delivery surface on, diff --git a/docs/configuration-contract.md b/docs/configuration-contract.md index d0bb05fc..d37a17dd 100644 --- a/docs/configuration-contract.md +++ b/docs/configuration-contract.md @@ -115,6 +115,71 @@ identity on a managed service uses service's own identity uses `["service-identity"]`. Proxy containers, image choices and loopback routing are not part of this contract. +## Public module artifacts + +A workspace configuration value may be **bound to a file in a composed +module** instead of being written out as a literal, so a public artifact a +module already ships — an installation document, for example — reaches services +as configuration without being copied into the composition and kept in step by +hand. + +The binding is declared in the selected profile, in +`configurations//module-artifacts.codefly.json`, and names only a +destination and a source: + +```json +{ + "schemaVersion": 1, + "bindings": [ + {"configuration": "docs", "key": "README", + "source": {"module": "wiki", "path": "docs/install.md"}} + ] +} +``` + +**Acquired once per ordinary load, and nothing is frozen across loads.** Every +ordinary configuration load — a run's delivery, a `codefly doctor` check — +acquires the declared file afresh and carries its exact bytes, with the SHA256 +and byte count core computes *from those bytes*, on the value that load +delivers. A standalone check therefore describes what *it* observed and proves +nothing about a later delivery; an already running service keeps the +configuration it was delivered until its ordinary lifecycle replaces it. A run +whose planning read saw one version and whose delivery load saw another +delivers the second, and never presents the first as its evidence. + +**What the CLI refuses.** The declared path is opened under the selected +module's directory with confined, non-blocking, regular-file access, so an +absolute path, a `..` element, a symlink leaving the module, a FIFO, a +directory and a missing file all refuse; so does a file that changed while it +was being read. A composed module this machine has not materialized refuses +rather than being fetched behind the read, and a path the CLI materialized +needs a resolution receipt that answers the request the workspace makes now. An +explicit `path:`, an overlay path or a worktree legitimately has no receipt: +its bytes are local development evidence, not release authenticity. Core then +bounds the bytes at the inline carrier threshold (32 KiB, inclusive), requires +valid UTF-8 with no NUL, and refuses text that would need interpolation. + +**No value can quietly take a bound destination.** A literal, secret, template +or structured document offered for the same destination is a conflict rather +than a silent winner, so moving an existing literal onto a binding is an +explicit source edit: remove the literal and add the binding. At delivery the +same rule holds against *additional* offers — a configuration the agent returns +from Init, a dependency's configuration at Start, a `--set` or derived runtime +override — including one that offers the identical bytes. Equality buys no +replacement right, and an empty override map does not skip the check. Removing +a binding restores ordinary ownership on the next load; there is no generated +state to clean up. + +**An explicit binding is the author's assertion that these bytes are public.** +Confinement and a digest do not establish publicness, and no scanner or +filename rule can: a binding declaring private or mixed-secret content as +public exposes both the content and a testable digest. That judgement belongs +to whoever reviews the composition. + +Such a value is delivered inline to the process. It cannot be exported through +`--output-env` when its bytes contain a line break, which that raw format +cannot represent (see `docs/commands.md`). + ## Module-owned keys A few keys on an environment belong to a **module agent**, not to the CLI. The diff --git a/docs/runnable.md b/docs/runnable.md index 57816824..731d7d84 100644 --- a/docs/runnable.md +++ b/docs/runnable.md @@ -43,13 +43,39 @@ owns the option, the descriptor-to-bounded-schema projection and published contracts, the layout on disk and the drift gate. Nothing about the contract is authored twice — it is the projection of the method's own messages. -The selected owner's `module.contract.codefly.yaml` supplies the audience through -Core's strict module-contract reader. Its principal is independent of the -producer checkout's module name; generation keeps the package's module identity -unchanged. Every generated destination must be published by that contract, and -an explicit operation audience must match its principal. Missing, unreadable or -contradictory publication refuses generation before output changes. A module -with no operations still writes an empty index. +Where an operation's audience comes from decides what the module must publish. + +A method that **annotates its audience explicitly** (Core sets +`OperationSpec.Declared` from the method option) carries the author's own +audience. That may be a host surface the module reaches without being installed +as one — a source-operation audience, say — so Core does not require it to equal +any module principal, and neither does generation: such an operation needs no +`module.contract.codefly.yaml` at all. Its served target, scopes and lookup are +still checked as always, so an unserved method or a missing paired Lookup +refuses. + +A method that **carries no operation option** takes Core's default from +`ServiceOwner.Audience`, which generation fills from the selected owner's +`module.contract.codefly.yaml` through Core's strict module-contract reader. +There the contract is required: with no publication there is no principal to +derive from, and generation refuses rather than emit an empty audience. + +The distinction is the option's presence, not an audience's: `Declared` marks +that the method carries the option. Core validates the audience inside it +before setting `Declared`, and generation refuses an annotated method whose +audience is empty rather than treating it as either case — it has no principal +to check against and nothing of its own to carry. + +When that contract **does exist**, the module's publication applies to every +operation it serves, explicit ones included: each generated destination must be +published by it, and an audience differing from its principal is a conflict in +the module's own declarations rather than host permission. Its principal is +independent of the producer checkout's module name; generation keeps the +package's module identity unchanged. + +An unreadable, malformed or escaped contract is never read as absence — a module +that authored one and got it wrong is refused by name. Every refusal happens +before output changes. A module with no operations still writes an empty index. Derivation is endpoint-specific: `catalog.codefly.json`'s `endpoints[].services[].procedures` declares the served surface; descriptor bytes diff --git a/go.mod b/go.mod index 62420b36..0a8df19b 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,7 @@ require ( github.com/codefly-dev/authority/libraries/delivery/go v0.0.0-20261026023425-000d44495d93 github.com/codefly-dev/authority/libraries/fixture-composition/go v0.0.0-20261027171336-5178f01696e3 github.com/codefly-dev/authority/libraries/hop/go v0.0.0-20261026175541-3afad7224855 - github.com/codefly-dev/core v0.19.10 + github.com/codefly-dev/core v0.19.11-0.20261028153419-de19eba80550 github.com/codefly-dev/golor v0.1.3 github.com/codefly-dev/llm v0.1.7 github.com/codefly-dev/sdk-go/workcontext v0.1.1-0.20261018052022-70dc7a546458 diff --git a/go.sum b/go.sum index f118e5da..31175fa7 100644 --- a/go.sum +++ b/go.sum @@ -119,6 +119,8 @@ github.com/codefly-dev/authority/libraries/resources/go v0.0.0-20261026023425-00 github.com/codefly-dev/authority/libraries/resources/go v0.0.0-20261026023425-000d44495d93/go.mod h1:1rmjrVyr0h7zOuwOHS9i+BoeS9dr3XNvtupLZFwY6As= github.com/codefly-dev/core v0.19.10 h1:rTeh1vph0xGtpiCCt2Dl1AH2Rz9hzIYt1YGcbv6bLYE= github.com/codefly-dev/core v0.19.10/go.mod h1:aTJY3Rw0OSVkbWRThgDzSkqieoF9mW4MiPRXM8Vzxww= +github.com/codefly-dev/core v0.19.11-0.20261028153419-de19eba80550 h1:IBur1pFM7LelM+9ecMgl2/Mxgd+04rgesSVFm3R1k0E= +github.com/codefly-dev/core v0.19.11-0.20261028153419-de19eba80550/go.mod h1:aTJY3Rw0OSVkbWRThgDzSkqieoF9mW4MiPRXM8Vzxww= github.com/codefly-dev/golor v0.1.3 h1:xmo+ceyJFRYZdvpWE2fNd0jeaadp/Ibm1BnganiGKOc= github.com/codefly-dev/golor v0.1.3/go.mod h1:sl/u/K1l7J0Pr3xyVZp8fOJYQItKKst1No9JqgzLLoY= github.com/codefly-dev/gortk v0.2.0 h1:7bOlS5valYz2zil+fZctQNcPCYBcPj86abcw9N8h1hQ= diff --git a/pkg/cli/output_sink.go b/pkg/cli/output_sink.go index 4d8080e6..cdfb3e65 100644 --- a/pkg/cli/output_sink.go +++ b/pkg/cli/output_sink.go @@ -1,29 +1,34 @@ package cli import ( - "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/wool" ) -// flowOutputSink adapts the package-level CLI narration functions to +// FlowOutputSink adapts the package-level CLI narration functions to // orchestration.OutputSink, so cobra commands can keep their existing // stdout narration now that pkg/orchestration no longer imports pkg/cli // directly. -type flowOutputSink struct{} +// +// It satisfies that interface structurally and does not name it, which is +// what keeps pkg/cli BELOW pkg/orchestration in the import graph: naming it +// put pkg/cli above orchestration, and since pkg/composition narrates through +// pkg/cli, orchestration could then not read a composed module's tree +// (module_artifact.go) without a cycle. +type FlowOutputSink struct{} // NewOutputSink returns the pkg/cli-backed orchestration.OutputSink. -func NewOutputSink() orchestration.OutputSink { - return flowOutputSink{} +func NewOutputSink() FlowOutputSink { + return FlowOutputSink{} } -func (flowOutputSink) Process(log *wool.Log) { GetLogger().Process(log) } +func (FlowOutputSink) Process(log *wool.Log) { GetLogger().Process(log) } -func (flowOutputSink) ProcessWithSource(source *wool.Identifier, log *wool.Log) { +func (FlowOutputSink) ProcessWithSource(source *wool.Identifier, log *wool.Log) { GetLogger().ProcessWithSource(source, log) } -func (flowOutputSink) Info(format string, args ...any) { Info(format, args...) } +func (FlowOutputSink) Info(format string, args ...any) { Info(format, args...) } -func (flowOutputSink) Error(format string, args ...any) { Error(format, args...) } +func (FlowOutputSink) Error(format string, args ...any) { Error(format, args...) } -func (flowOutputSink) RegisterLoggingResource(unique string) { RegisterLoggingResource(unique) } +func (FlowOutputSink) RegisterLoggingResource(unique string) { RegisterLoggingResource(unique) } diff --git a/pkg/composition/fixtures.go b/pkg/composition/fixtures.go index 51ba8271..64e96fd2 100644 --- a/pkg/composition/fixtures.go +++ b/pkg/composition/fixtures.go @@ -12,7 +12,6 @@ import ( "github.com/codefly-dev/cli/pkg/cli" "github.com/codefly-dev/cli/pkg/environments" - "github.com/codefly-dev/cli/pkg/orchestration" corecomposition "github.com/codefly-dev/core/composition" "github.com/codefly-dev/core/resources" ) @@ -197,7 +196,7 @@ func ValidateFixtureSelection(ctx context.Context, workspace *resources.Workspac // hand at each call site, so only the one that happened to be covered was ever // exercised by a test. func ResolveFixtureSelection(ctx context.Context, workspace *resources.Workspace, env *environments.Environment, override string) (string, error) { - selected := orchestration.SelectedFixture(env, override) + selected := environments.SelectedFixture(env, override) if err := ValidateFixtureSelection(ctx, workspace, selected); err != nil { return "", err } diff --git a/pkg/composition/module_selection.go b/pkg/composition/module_selection.go new file mode 100644 index 00000000..f414f343 --- /dev/null +++ b/pkg/composition/module_selection.go @@ -0,0 +1,174 @@ +package composition + +import ( + "context" + "fmt" + "path/filepath" + + "github.com/codefly-dev/core/resources" +) + +// SelectedModule is one composed module's selection as this invocation holds +// it: where it resolves, how it was selected, and the request that selection +// answers. +// +// It is comparable with == on purpose, so a caller holding a selection across a +// read compares the WHOLE thing rather than picking a field. Its fields are the +// existing owner evidence — the resolution, the mode the materializer's own +// predicate derives, the machine-local directive as it stands on disk, the +// materialization receipt, and the reference the workspace makes — carried as +// scalars, because a pointer comparison answers "the same object" when the +// question is "the same selection". +type SelectedModule struct { + // Dir is the directory the module resolves to. + Dir string + // Verified is true only for a CLI-materialized selection whose mode is + // signature- or digest-checked. An explicit path or worktree is false: its + // bytes are local development evidence, not release authenticity. + Verified bool + // Mode is how the module is materialized, as a receipt records it. + Mode ResolutionMode + + // OverlayDir is the directory whose overlay and resolution record this + // selection was read from — NearestOverlayDir's answer, or the workspace + // when nothing is nearer. + // + // It is part of the selection because a directive path is relative to the + // overlay file that declares it. A nearer overlay appearing with the SAME + // path text re-points the module without changing one character of the + // directive, so the raw path cannot tell the two selections apart and this + // is what does. + OverlayDir string + + // The machine-local directive, re-read from disk on every ask. This is what + // moves when a developer re-points a module between two reads — and it can + // move while the RESOLVED path does not, because ResolveModule answers from + // the overlay snapshot the workspace was loaded with. + DirectivePath string + DirectiveWorktree string + DirectiveGit bool + DirectivePinned bool + + // The materialization receipt: the request it answered and what that + // produced. Empty throughout for a selection the CLI did not materialize. + ReceiptSource string + ReceiptModule string + ReceiptRequested string + ReceiptVersion string + ReceiptPath string + ReceiptDigest string + ReceiptCommit string + + // The composed reference this selection answers. + RefSource string + RefModule string + RefVersion string +} + +// SelectModule is the selection of a composed module right now, +// judged under the owner rules the materializer itself uses, and nothing else. +// +// It never pulls, materializes or reloads: a caller that reads bytes out of a +// composed module reads what this invocation already selected, so a reference +// the workspace cannot load as a local checkout is refused rather than fetched +// behind the read (ResolveComposedModuleDir is the materializing half, for +// callers whose job is to make the tree exist). +// +// Provenance decides how hard that refusal is. A path the CLI materialized — +// pinnedManaged, the same predicate the materializer gates its own writes on — +// must have a receipt that answers the request this workspace makes now, or +// the bytes under it are from some other resolution and the selection is +// stale. An explicit `path:`, an overlay path or a worktree legitimately has +// no receipt at all: it is the developer's own checkout, selected by them, and +// its bytes are local development evidence rather than release authenticity. +// Returning verified=false for those is the whole point — a caller that needs +// to say where bytes came from must not call a worktree a release. +func SelectModule(ctx context.Context, workspace *resources.Workspace, module string) (*SelectedModule, error) { + var ref *resources.ModuleReference + for _, candidate := range workspace.Modules { + if candidate.Name == module { + ref = candidate + break + } + } + if ref == nil { + return nil, fmt.Errorf("module %s is not composed in workspace %s", module, workspace.Name) + } + state, err := loadMaterializationState(ctx, workspace) + if err != nil { + return nil, err + } + var directive *resources.ModuleResolveDirective + if state.overlay != nil { + directive = state.overlay.Resolve[module] + } + receipt := state.receipts[module] + mode := ResolutionModeFor(ref, directive, receipt, state.declared[module]) + resolution, err := workspace.ResolveModule(ctx, ref) + if err != nil { + return nil, err + } + if resolution.Kind == resources.ResolutionPinned { + return nil, fmt.Errorf("module %s is not materialized on this machine; resolve the composition before reading its tree", module) + } + selected := &SelectedModule{ + Dir: resolution.Dir, + Mode: mode, + OverlayDir: state.recordDir, + RefSource: ref.Source, + RefModule: ref.Module, + RefVersion: ref.Version, + } + if directive != nil { + selected.DirectivePath = directive.Path + selected.DirectiveWorktree = directive.Worktree + selected.DirectiveGit = directive.Git + selected.DirectivePinned = directive.Pinned + } + if receipt != nil { + selected.ReceiptSource = receipt.Source + selected.ReceiptModule = receipt.Module + selected.ReceiptRequested = receipt.Requested + selected.ReceiptVersion = receipt.Version + selected.ReceiptPath = receipt.Path + selected.ReceiptDigest = receipt.Digest + selected.ReceiptCommit = receipt.Commit + } + if pinnedManaged(ref, directive, receipt.ResolvedPath(), state.roots.owned...) { + if !moduleReceiptAnswers(ref, directive, receipt, mode) { + return nil, fmt.Errorf("module %s materialization is unresolved or stale; resolve the composition before reading its tree", module) + } + // Two sources had to agree to get here, and they are read at different + // moments: the overlay and the receipts come off disk NOW, while + // workspace.ResolveModule answers from the overlay snapshot the + // workspace was LOADED with. A selection changed in between — a + // re-materialization, an edited overlay — makes the receipt describe a + // path the resolution no longer names, and the bytes would then be read + // from one selection while attributed to another's receipt. Require the + // resolved directory to be the receipt's path, or under it where the + // reference names a module subpath, and refuse the divergence rather + // than pick a side. + if !sameSelectedDir(selected.Dir, receipt.ResolvedPath()) { + return nil, fmt.Errorf("module %s resolves to a different directory than its materialization receipt records; resolve the composition again before reading its tree", module) + } + selected.Verified = !mode.Unverified() + } + return selected, nil +} + +// sameSelectedDir compares the resolved directory with the receipt's, and +// requires them to be the SAME directory — not one under the other. +// +// The materializer records the module directory itself, with ref.Module's +// subpath already joined on (updateReceipt stores the resolved module dir, not +// the checkout root), so the receipt and the resolution are two spellings of +// one directory and nothing legitimately nests. A prefix test would pass a +// stale selection that happens to sit beneath the current one — exactly the +// case the reviewer's A-nested-under-B repro covers — so only normalized +// equality can tell "the same selection" from "a different one inside it". +func sameSelectedDir(dir, selected string) bool { + if dir == "" || selected == "" { + return false + } + return filepath.Clean(dir) == filepath.Clean(selected) +} diff --git a/pkg/conformance/matrix.json b/pkg/conformance/matrix.json index 2b3cd329..b3e44868 100644 --- a/pkg/conformance/matrix.json +++ b/pkg/conformance/matrix.json @@ -1,7 +1,7 @@ { "schema_version": 1, "cli": "source", - "core": "v0.19.10", + "core": "v0.19.11", "rows": [ { "id": "linux-amd64-source", diff --git a/pkg/environments/environment.go b/pkg/environments/environment.go index b9c81b0e..18fefcc4 100644 --- a/pkg/environments/environment.go +++ b/pkg/environments/environment.go @@ -1932,3 +1932,23 @@ func (s *EnvironmentServiceSecrets) validateGenerators() error { } return nil } + +// SelectedFixture resolves the fixture a workspace-bound flow runs with. An +// environment declares the fixture its runtime should use, so an ordinary +// `codefly run service` / `codefly test service` needs no --fixture at all. +// An explicit override always wins, and an environment that deliberately +// omits a fixture — because it loads a real provider — resolves to none. +// +// It lives here, beside the declaration, rather than in the orchestration +// package that is its main caller, because the composition package resolves a +// fixture selection too — and orchestration now reads composed module trees +// (module_artifact.go), so the two cannot both depend on each other. +func SelectedFixture(environment *Environment, override string) string { + if override != "" { + return override + } + if environment == nil { + return "" + } + return environment.Fixture +} diff --git a/pkg/environments/product_secrets.go b/pkg/environments/product_secrets.go index 84c0fdee..e401e7f2 100644 --- a/pkg/environments/product_secrets.go +++ b/pkg/environments/product_secrets.go @@ -64,7 +64,12 @@ func (policy *ProductSecretPolicy) Validate() error { // secret, and keys that group declares; a may-be-empty entry names a secret key // some declared group produces. Anything else is refused by name: it would // generate, or excuse, a value no rendered service reads. -func ValidateSecretPolicy(ctx context.Context, workspace *resources.Workspace, env *Environment, policy *ProductSecretPolicy) error { +// The resolver is a parameter rather than something this package builds: a +// module artifact's bytes are acquired from a composed module under the +// composition's own provenance rules, which live above pkg/environments. A +// caller that omits it reads a composition with a bound artifact as a refusal +// from Core — never as an absent value. +func ValidateSecretPolicy(ctx context.Context, workspace *resources.Workspace, env *Environment, policy *ProductSecretPolicy, resolvers ...configurations.ModuleArtifactResolver) error { if policy.Empty() { return nil } @@ -81,6 +86,9 @@ func ValidateSecretPolicy(ctx context.Context, workspace *resources.Workspace, e composed[identity.Unique()] = true } reader, err := configurations.NewConfigurationLocalReader(ctx, workspace) + for _, resolve := range resolvers { + reader = reader.WithModuleArtifactResolver(resolve) + } if err != nil { return err } diff --git a/pkg/gitops/authority.go b/pkg/gitops/authority.go index ce659408..a7f68283 100644 --- a/pkg/gitops/authority.go +++ b/pkg/gitops/authority.go @@ -12,6 +12,7 @@ import ( compositionv1 "github.com/codefly-dev/authority/libraries/authority-client/go/gen/host/composition/v1" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/configurations" modulecontract "github.com/codefly-dev/core/contracts/module" "github.com/codefly-dev/core/resources" @@ -88,7 +89,7 @@ func moduleAuthority( if request.contract.Principal != module.Name { return nil, fmt.Errorf("module %s publishes a contract whose principal is %q; the principal every credential of a module is issued to is the module's own name, so the contract declares principal: %s", module.Name, request.contract.Principal, module.Name) } - provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime()) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime(), orchestration.ModuleArtifactResolverFor(workspace)) if err != nil { return nil, fmt.Errorf("read the workspace configurations the contract of %s resolves against: %w", module.Name, err) } diff --git a/pkg/gitops/configuration_groups.go b/pkg/gitops/configuration_groups.go index 2073cb57..78e364d1 100644 --- a/pkg/gitops/configuration_groups.go +++ b/pkg/gitops/configuration_groups.go @@ -14,6 +14,7 @@ import ( "strings" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/configurations" modulecontract "github.com/codefly-dev/core/contracts/module" "github.com/codefly-dev/core/resources" @@ -79,7 +80,7 @@ func workspaceConfigurationDigests(ctx context.Context, workspace *resources.Wor // them now; a group the environment does not provide is absent from the // result. func digestProvidedGroups(ctx context.Context, workspace *resources.Workspace, env *environments.Environment, consumed map[string]struct{}) (map[string]string, error) { - provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime()) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime(), orchestration.ModuleArtifactResolverFor(workspace)) if err != nil { return nil, fmt.Errorf("read the workspace configurations this render bakes in: %w", err) } diff --git a/pkg/gitops/module_authority.go b/pkg/gitops/module_authority.go index eadea06c..bad79d99 100644 --- a/pkg/gitops/module_authority.go +++ b/pkg/gitops/module_authority.go @@ -7,6 +7,7 @@ import ( "slices" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/configurations" basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" "github.com/codefly-dev/core/resources" @@ -17,7 +18,7 @@ import ( // Read through Core so composed modules, imports and profile selection follow // the same rules as every other workspace configuration. func refuseAuthoredRenderConfigurations(ctx context.Context, workspace *resources.Workspace, env *resources.Environment) error { - provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env, orchestration.ModuleArtifactResolverFor(workspace)) if err != nil { return err } diff --git a/pkg/gitops/runnable_grants.go b/pkg/gitops/runnable_grants.go index 71527c84..6c3a3516 100644 --- a/pkg/gitops/runnable_grants.go +++ b/pkg/gitops/runnable_grants.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/codefly-dev/cli/pkg/environments" + "github.com/codefly-dev/cli/pkg/orchestration" runnablespkg "github.com/codefly-dev/cli/pkg/runnables" "github.com/codefly-dev/core/configurations" modulecontract "github.com/codefly-dev/core/contracts/module" @@ -239,7 +240,7 @@ func WithPreparedBindingGrants(ctx context.Context, workspace *resources.Workspa if len(installers) == 0 { return principals, nil } - provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime()) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, env.Runtime(), orchestration.ModuleArtifactResolverFor(workspace)) if err != nil { return nil, fmt.Errorf("read the prepared bindings the envelope lists: %w", err) } diff --git a/pkg/orchestration/disposable_engine_test.go b/pkg/orchestration/disposable_engine_test.go index 95c667db..c895a3f3 100644 --- a/pkg/orchestration/disposable_engine_test.go +++ b/pkg/orchestration/disposable_engine_test.go @@ -377,10 +377,18 @@ func disposableBuiltRef(call disposableEngineCall) string { func disposableOrdinaryRuntimeRef(t *testing.T, b *Builder) string { t.Helper() ordinary := *b - world := *b.world - world.disposableImageScope = "" - world.RebuildImages = true // identity/name only; do not query a cache for reuse - ordinary.world = &world + // By pointer, never a copy: World carries a sync.Once (the memoized + // workspace producer lookup), so `world := *b.world` duplicates a lock — + // vet's copylocks, and a real bug rather than noise, because the copy's + // Once inherits the original's fired state. The two fields the derivation + // needs are overridden on the scoped builder's own world and restored + // before returning, which is sound here because this helper is called from + // a sequential test and executes no Build and no cache write. + scope, rebuild := b.world.disposableImageScope, b.world.RebuildImages + b.world.disposableImageScope = "" + b.world.RebuildImages = true // identity/name only; do not query a cache for reuse + defer func() { b.world.disposableImageScope, b.world.RebuildImages = scope, rebuild }() + ordinary.world = b.world ordinary.localRuntime = true plan := b.planned.response.GetResult().GetDockerBuildPlan() recipe, err := ordinary.recipeAsBuilt(plan.Recipes[0], false) diff --git a/pkg/orchestration/environment.go b/pkg/orchestration/environment.go index fc888a64..cad6da7b 100644 --- a/pkg/orchestration/environment.go +++ b/pkg/orchestration/environment.go @@ -22,17 +22,9 @@ func SelectEnvironment(workspace *resources.Workspace, name string) (*environmen return environments.Select(workspace, name) } -// SelectedFixture resolves the fixture a workspace-bound flow runs with. An -// environment declares the fixture its runtime should use, so an ordinary -// `codefly run service` / `codefly test service` needs no --fixture at all. -// An explicit override always wins, and an environment that deliberately -// omits a fixture — because it loads a real provider — resolves to none. +// SelectedFixture resolves the fixture a workspace-bound flow runs with, +// where the environment declares it (environments.SelectedFixture, which the +// composition package shares). func SelectedFixture(environment *environments.Environment, override string) string { - if override != "" { - return override - } - if environment == nil { - return "" - } - return environment.Fixture + return environments.SelectedFixture(environment, override) } diff --git a/pkg/orchestration/environment_remote_manager.go b/pkg/orchestration/environment_remote_manager.go index 95e52959..ad1f6032 100644 --- a/pkg/orchestration/environment_remote_manager.go +++ b/pkg/orchestration/environment_remote_manager.go @@ -21,6 +21,10 @@ func NewEnvironmentRemoteManager(ctx context.Context, workspace *resources.Works if err != nil { return nil, err } + // The same acquisition every other delivery and check load supplies: a + // bound module artifact reaching this manager must be resolved from the + // selected module, not refused as unresolvable (module_artifact.go). + localReader.WithModuleArtifactResolver(ModuleArtifactResolverFor(workspace)) manager.WithLoader(localReader) return remotenetwork.NewRemoteManager(ctx, manager) } diff --git a/pkg/orchestration/excluded_root_output_env.go b/pkg/orchestration/excluded_root_output_env.go index d6777ed0..5828d09f 100644 --- a/pkg/orchestration/excluded_root_output_env.go +++ b/pkg/orchestration/excluded_root_output_env.go @@ -73,19 +73,6 @@ func (flow *Flow) exportExcludedOriginEnvironment(ctx context.Context) error { if err != nil { return fmt.Errorf("name the run environment for the excluded root's output environment: %w", err) } - if err := AppendServiceProcessConfigurationsToFile( - ctx, - flow.outputEnvPath, - environment, - runtimeContext, - serviceConfiguration, - workspaceConfigurations, - dependencyConfigurations, - nil, - ); err != nil { - return fmt.Errorf("write excluded root configurations: %w", err) - } - protoIdentity := &basev0.ServiceIdentity{ Workspace: identity.Workspace, Module: identity.Module, @@ -102,16 +89,30 @@ func (flow *Flow) exportExcludedOriginEnvironment(ctx context.Context) error { if flow.world.Env != nil && flow.world.Env.NamingScope != "" { overrides[resources.NamingScopePrefix] = flow.world.Env.NamingScope } - if err := AppendRuntimeEnvironmentToFile( - ctx, - flow.outputEnvPath, - protoIdentity, - runtimeContext, - flow.fixture, - overrides, - endpointMappings, - ); err != nil { - return fmt.Errorf("write excluded root runtime environment: %w", err) + // The selected payload is checked against itself and against this export's + // override map — duplicates inside it are still collisions — and the + // complete record batch is converted, before the first append. No agent + // Init runs for an excluded root, so there is no returned contribution and + // no retained delivery to pretend there is one. + selected := deliveryBaseline(serviceConfiguration, workspaceConfigurations, dependencyConfigurations) + if deliveryErr := resources.ValidateModuleArtifactDelivery(selected, nil, overrides); deliveryErr != nil { + return fmt.Errorf("export excluded root environment: %w", deliveryErr) + } + configurationVariables, err := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, + serviceConfiguration, workspaceConfigurations, dependencyConfigurations, nil) + if err != nil { + return fmt.Errorf("convert excluded root configurations: %w", err) + } + runtimeVariables, err := runtimeEnvironmentVariables(ctx, protoIdentity, runtimeContext, flow.fixture, overrides, endpointMappings) + if err != nil { + return fmt.Errorf("convert excluded root runtime environment: %w", err) + } + records, err := outputRecords(append(configurationVariables, runtimeVariables...)) + if err != nil { + return fmt.Errorf("export excluded root environment: %w", err) + } + if err := writeOutputRecords(ctx, flow.outputEnvPath, records); err != nil { + return fmt.Errorf("write excluded root environment: %w", err) } return nil } diff --git a/pkg/orchestration/hop_configurations.go b/pkg/orchestration/hop_configurations.go index 02806d74..14270be0 100644 --- a/pkg/orchestration/hop_configurations.go +++ b/pkg/orchestration/hop_configurations.go @@ -43,6 +43,12 @@ func NewWorkspaceConfigurationReader(ctx context.Context, workspace *resources.W if err != nil { return nil, err } + // Bound here, so run delivery and every check that asks what this + // invocation resolves acquire module artifacts the same way: a bound + // destination core cannot resolve refuses rather than falling back to a + // literal, and a standalone check observes its own acquisition rather than + // inheriting one (module_artifact.go). + reader.WithModuleArtifactResolver(ModuleArtifactResolverFor(workspace)) return &WorkspaceConfigurationReader{ConfigurationInformationLocalReader: reader, workspace: workspace}, nil } @@ -77,14 +83,34 @@ func (r *WorkspaceConfigurationReader) Load(ctx context.Context, env *resources. if fixture != nil { // Replace any module default with the complete merge. Core may have // resolved a single fragment (or identical fragments) without ambiguity. + // + // What is replaced must not be something Core was asked to deliver. A + // module artifact bound to this group has already been acquired, had + // its bytes copied and its identity computed, and is carried on the + // value Core resolved — deleting that info here would substitute this + // generated document for the artifact the composition declared, with + // nothing said. The whole-group replacement is exactly the lossy + // precedence the delivery contract requires to be checked BEFORE the + // offer is discarded, so it refuses instead. for i, config := range r.combined { if config.GetOrigin() != resources.ConfigurationWorkspace { continue } replacement := proto.CloneOf(config) + var bound bool replacement.Infos = slices.DeleteFunc(replacement.Infos, func(info *basev0.ConfigurationInformation) bool { - return info.GetName() == fixturefragments.Group + if info.GetName() != fixturefragments.Group { + return false + } + bound = bound || slices.ContainsFunc(info.GetConfigurationValues(), func(value *basev0.ConfigurationValue) bool { + return value.GetResolvedModuleArtifact() != nil || value.GetModuleArtifact() != nil + }) + return true }) + if bound { + return fmt.Errorf("%s is bound to a module artifact and is also the merged fixture composition: the generated document would replace the declared artifact, so remove the binding or the fragments (%w)", + fixturefragments.Group, configurations.ErrConfigurationConflict) + } r.combined[i] = replacement } r.combined = append(r.combined, fixture) diff --git a/pkg/orchestration/local_runtime_image.go b/pkg/orchestration/local_runtime_image.go index b3d1d4e9..cd5fcefd 100644 --- a/pkg/orchestration/local_runtime_image.go +++ b/pkg/orchestration/local_runtime_image.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "path/filepath" + "slices" "strings" "github.com/distribution/reference" @@ -39,6 +40,29 @@ func (runner *Runner) localRuntimeImage(ctx context.Context) (string, error) { if advertised, supported := ValidationOperationSupport(runner.instance.Info, ValidationArtifactBuild); advertised && !supported { return "", nil } + // Whether this run relies on the handoff is decided by the agent + // advertising the optional capability — not by whether its builder happens + // to emit exactly one recipe. + // + // A single recipe does not make a runtime image. A database agent emits one + // for its migration or bootstrap image, and reading the runtime role off + // that sole recipe both built the wrong image and then failed the run for a + // capability the agent never claimed. An agent that does not advertise + // keeps its own image selection, exactly as a stock-image one does, and its + // build stays out of this path entirely. + // + // The test is deliberately the capability list and not contract.Check: + // Check reports a protocol-version mismatch with the same ErrIncompatible + // as a missing capability, so treating any error as "not advertised" would + // silently accept an agent this host cannot speak to. Absence of an + // optional capability is the only thing waived here; the strict guard in + // buildLocalRuntimeImage still runs the full Check once the handoff is + // selected, so an advertising agent with an incompatible protocol is + // refused rather than skipped. The disposable image policy above runs + // before this skip and is never waived by it. + if !slices.Contains(runner.instance.Info.GetContract().GetCapabilities(), contract.RuntimeInitImage) { + return "", nil + } if runner.instance.Builder == nil { if err := runner.instance.LoadBuilder(ctx); err != nil { return "", err @@ -118,6 +142,10 @@ func (b *Builder) buildLocalRuntimeImage(ctx context.Context) (string, error) { if len(plan.GetRecipes()) != 1 { return "", fmt.Errorf("local runtime for %s requires exactly one image recipe, got %d", b.instance.Unique(), len(plan.GetRecipes())) } + // The strict guard, retained: localRuntimeImage only waives the ABSENCE of + // the optional capability, so the full contract check still runs here for + // an agent that did advertise it — a protocol-version mismatch, or any + // other incompatibility, refuses the handoff rather than being skipped. if err := b.instance.RequireAgentCapabilities(contract.RuntimeInitImage); err != nil { return "", fmt.Errorf("cannot supply a local runtime image to %s: %w", b.instance.Unique(), err) } diff --git a/pkg/orchestration/local_runtime_image_test.go b/pkg/orchestration/local_runtime_image_test.go index aee7410c..b75a17e8 100644 --- a/pkg/orchestration/local_runtime_image_test.go +++ b/pkg/orchestration/local_runtime_image_test.go @@ -7,6 +7,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -47,17 +48,40 @@ type runtimeImageBuilderPeer struct { imageCachePeer stock bool malformed bool + // recipeName renames the emitted recipe. core's docker.proto makes name + // the recipe's logical identity ("such as app or migration"), so a sole + // recipe named "bootstrap" is a migration runner, not a runtime image. + recipeName string + // loads/inits/builds count the builder RPCs the CLI actually made. An + // agent that does not advertise the handoff must reach NONE of them: a + // zero docker-build count alone would still pass while the CLI loaded, + // initialized and planned a builder whose result it will never use. + loads atomic.Int32 + inits atomic.Int32 + builds atomic.Int32 } -func (*runtimeImageBuilderPeer) Load(context.Context, *builderv0.LoadRequest) (*builderv0.LoadResponse, error) { +func (peer *runtimeImageBuilderPeer) Load(context.Context, *builderv0.LoadRequest) (*builderv0.LoadResponse, error) { + peer.loads.Add(1) return &builderv0.LoadResponse{State: &builderv0.LoadStatus{State: builderv0.LoadStatus_READY}}, nil } -func (*runtimeImageBuilderPeer) Init(context.Context, *builderv0.InitRequest) (*builderv0.InitResponse, error) { +func (peer *runtimeImageBuilderPeer) Init(context.Context, *builderv0.InitRequest) (*builderv0.InitResponse, error) { + peer.inits.Add(1) return &builderv0.InitResponse{State: &builderv0.InitStatus{State: builderv0.InitStatus_SUCCESS}}, nil } +// requireNoBuilderTraffic is the zero-call assertion: no Load, no Init, no +// Build (the plan request) reached the agent's builder at all. +func (peer *runtimeImageBuilderPeer) requireNoBuilderTraffic(t *testing.T) { + t.Helper() + require.Zero(t, peer.loads.Load(), "the builder must not be loaded for an agent that will not use the handoff") + require.Zero(t, peer.inits.Load(), "nor initialized") + require.Zero(t, peer.builds.Load(), "nor asked for a build plan") +} + func (peer *runtimeImageBuilderPeer) Build(ctx context.Context, req *builderv0.BuildRequest) (*builderv0.BuildResponse, error) { + peer.builds.Add(1) if peer.stock { return &builderv0.BuildResponse{ State: &builderv0.BuildStatus{State: builderv0.BuildStatus_SUCCESS}, @@ -65,6 +89,11 @@ func (peer *runtimeImageBuilderPeer) Build(ctx context.Context, req *builderv0.B }, nil } resp, err := peer.imageCachePeer.Build(ctx, req) + if err == nil && peer.recipeName != "" { + for _, recipe := range resp.GetResult().GetDockerBuildPlan().GetRecipes() { + recipe.Name = peer.recipeName + } + } if err == nil && peer.malformed { err = os.WriteFile(filepath.Join(req.GetOutputDirectory(), "Dockerfile"), []byte("FROM tampered\n"), 0o644) } @@ -72,9 +101,14 @@ func (peer *runtimeImageBuilderPeer) Build(ctx context.Context, req *builderv0.B } func runtimeImageRunner(t *testing.T, peer *runtimeImageBuilderPeer) (*Runner, *dockerStub) { + t.Helper() + return runtimeImageRunnerWithAgent(t, peer, realAgentInputs) +} + +func runtimeImageRunnerWithAgent(t *testing.T, peer *runtimeImageBuilderPeer, mode string) (*Runner, *dockerStub) { t.Helper() stub := installDockerStub(t, true) - runner, world := gatewayRunner(t, startRealAgent(t, realAgentInputs)) + runner, world := gatewayRunner(t, startRealAgent(t, mode)) root := t.TempDir() world.Workspace.WithDir(root) serviceDir := filepath.Join(root, "modules/web/services/gateway") @@ -145,11 +179,80 @@ esac } } -func TestRunnerRefusesImageHandoffWithoutLiveCapability(t *testing.T) { - runner, stub := runtimeImageRunner(t, &runtimeImageBuilderPeer{}) +// TestRunnerLeavesTheImageToANonAdvertisingAgent is the Postgres shape, and +// the behaviour that replaces an earlier refusal. +// +// The agent has a builder and its builder emits a recipe — but that recipe is +// not its runtime image (a database's is its migration or bootstrap image), and +// it never advertised runtime-init-image/v1. So the run must leave the image to +// the agent: no build, no handoff, no refusal. Reading the runtime role off the +// sole recipe instead built the wrong image and then failed the run for a +// capability the agent never claimed, which is what blocked local startup. +func TestRunnerLeavesTheImageToANonAdvertisingAgent(t *testing.T) { + peer := &runtimeImageBuilderPeer{} + runner, stub := runtimeImageRunner(t, peer) + runner.instance.Info.Contract = contract.Current() + require.NotContains(t, runner.instance.Info.GetContract().GetCapabilities(), contract.RuntimeInitImage) + + output, err := runner.Init(t.Context()) + require.NoError(t, err, "a non-advertising agent owns its image; the run must not refuse") + require.NotNil(t, output, "and Init must complete normally, not short-circuit") + require.Zero(t, stub.calls(t, "build"), "its recipe must not be built as a runtime image") + require.NotContains(t, exposedValues(t.Context(), t, runner.world), "runtime-image") + // The decision is taken before the builder is touched at all. + peer.requireNoBuilderTraffic(t) +} + +// TestRunnerPropagatesTheAgentsOwnInitFailure keeps the early return from +// becoming a swallow-all. Skipping the handoff must not also skip — or absorb — +// the runtime agent's own Init outcome: a non-advertising agent whose Init +// fails still fails the run, with its own diagnostic. +func TestRunnerPropagatesTheAgentsOwnInitFailure(t *testing.T) { + peer := &runtimeImageBuilderPeer{} + runner, _ := runtimeImageRunnerWithAgent(t, peer, realAgentRefuses) runner.instance.Info.Contract = contract.Current() + + _, err := runner.Init(t.Context()) + require.Error(t, err, "the agent's own Init failure must reach the caller") + require.ErrorContains(t, err, realAgentRefusalMessage) + require.NotContains(t, err.Error(), contract.RuntimeInitImage, + "and it must not be reported as a handoff-capability problem") + peer.requireNoBuilderTraffic(t) +} + +// TestRunnerLeavesTheImageToAnAgentPinningItsOwnRuntime is the third class the +// earlier model had no case for, and the one service-postgres is: a stock or +// pinned runtime image PLUS a build recipe for a DIFFERENT image (its +// migration/bootstrap runner). core's docker.proto says a recipe's name is its +// logical identity and that a service may emit several, so cardinality cannot +// say which recipe — if any — is the runtime image. Here the sole recipe is +// named "bootstrap", and it must not be built or handed to Runtime.Init. +func TestRunnerLeavesTheImageToAnAgentPinningItsOwnRuntime(t *testing.T) { + peer := &runtimeImageBuilderPeer{recipeName: "bootstrap"} + runner, stub := runtimeImageRunner(t, peer) + runner.instance.Info.Contract = contract.Current() + + _, err := runner.Init(t.Context()) + require.NoError(t, err, "a migration recipe is not a declaration of the runtime image") + require.Zero(t, stub.calls(t, "build")) + require.NotContains(t, exposedValues(t.Context(), t, runner.world), "runtime-image") + peer.requireNoBuilderTraffic(t) +} + +// TestRunnerRefusesAnIncompatibleAdvertisingAgent keeps the waiver specific. +// contract.Check reports a protocol-version mismatch with the same +// ErrIncompatible as a missing capability, so an agent that DID advertise the +// handoff and cannot speak this host's protocol must still be refused — not +// quietly treated as one that owns its own image. +func TestRunnerRefusesAnIncompatibleAdvertisingAgent(t *testing.T) { + runner, stub := runtimeImageRunner(t, &runtimeImageBuilderPeer{}) + runner.instance.Info.GetContract().ProtocolVersion = contract.Current().GetProtocolVersion() + 1 + require.Contains(t, runner.instance.Info.GetContract().GetCapabilities(), contract.RuntimeInitImage) + _, err := runner.Init(t.Context()) - require.ErrorContains(t, err, contract.RuntimeInitImage) + require.ErrorIs(t, err, contract.ErrIncompatible, + "an incompatible protocol must refuse, not be waived as a missing optional capability") + require.ErrorContains(t, err, "CLI-agent protocol version") require.Zero(t, stub.calls(t, "build")) } @@ -160,9 +263,15 @@ func TestRunnerRefusesMalformedRuntimeRecipe(t *testing.T) { require.Zero(t, stub.calls(t, "build")) } -func TestRunnerStockImageNeedsNoHandoffCapability(t *testing.T) { +// TestRunnerAdvertisingAgentWithNoRecipeKeepsItsOwnImage is the other half: +// the agent advertises the handoff, so the capability gate lets it through, +// and its builder emits no recipe at all (a stock-image service). That is not +// a refusal either — the agent keeps its own selection. It deliberately +// advertises, so the assertion exercises the no-recipe path rather than +// returning early at the capability gate. +func TestRunnerAdvertisingAgentWithNoRecipeKeepsItsOwnImage(t *testing.T) { runner, stub := runtimeImageRunner(t, &runtimeImageBuilderPeer{stock: true}) - runner.instance.Info.Contract = contract.Current() + require.Contains(t, runner.instance.Info.GetContract().GetCapabilities(), contract.RuntimeInitImage) _, err := runner.Init(t.Context()) require.NoError(t, err) require.NotContains(t, exposedValues(t.Context(), t, runner.world), "runtime-image") diff --git a/pkg/orchestration/module_artifact.go b/pkg/orchestration/module_artifact.go new file mode 100644 index 00000000..ed6c60cc --- /dev/null +++ b/pkg/orchestration/module_artifact.go @@ -0,0 +1,141 @@ +package orchestration + +import ( + "context" + "fmt" + "io" + "os" + "syscall" + + "github.com/codefly-dev/cli/pkg/composition" + "github.com/codefly-dev/core/configurations" + basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" + "github.com/codefly-dev/core/resources" + "google.golang.org/protobuf/proto" +) + +// moduleArtifactResolverFor is the acquisition half of core's per-load module +// artifact seam: core owns the declaration, the profile precedence, the +// collision checks and the typed identity it computes from the bytes; the +// selected module, the confined read and the provenance of what was read are +// the CLI's, because only the CLI knows how this invocation resolved that +// module. +// +// Each ordinary Load acquires afresh — core shares one acquisition for the +// repeated uses WITHIN a load and starts clean on the next one — so this +// resolver reads the workspace as the load sees it and never caches across +// loads. A later load may legitimately see changed bytes; nothing here freezes +// them, and nothing rereads the file at emission to prove they were stable. +func ModuleArtifactResolverFor(workspace *resources.Workspace) configurations.ModuleArtifactResolver { + return func(ctx context.Context, ref *basev0.ConfigurationModuleArtifactReference) ([]byte, error) { + return acquireModuleArtifact(ctx, workspace, ref) + } +} + +// acquireModuleArtifact reads the declared path out of the already selected +// module, under that module's directory and nowhere else. +// +// The confinement is os.Root, so a symlink, a `..` element or an absolute path +// cannot leave the selection even if it survived core's reference validation. +// os.Root alone is not enough, though: it still happily opens a FIFO and then +// blocks forever on the read, and it says nothing about what was opened. So the +// open is non-blocking, the mode is checked before a byte is read, and the same +// open file is stat'ed again afterwards — a file replaced or rewritten under +// the read refuses rather than delivering a mixture of two versions. That is a +// same-file observation, not a snapshot of the tree against arbitrary +// concurrent mutation. +func acquireModuleArtifact(ctx context.Context, workspace *resources.Workspace, ref *basev0.ConfigurationModuleArtifactReference) ([]byte, error) { + if workspace == nil { + return nil, fmt.Errorf("module artifacts require a selected workspace") + } + if err := resources.ValidateModuleArtifactReference(ref); err != nil { + return nil, err + } + selected, err := composition.SelectModule(ctx, workspace, ref.Module) + if err != nil { + return nil, err + } + dir := selected.Dir + root, err := os.OpenRoot(dir) + if err != nil { + return nil, fmt.Errorf("cannot confine module artifact reads to module %s", ref.Module) + } + defer root.Close() + file, err := root.OpenFile(ref.Path, os.O_RDONLY|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, fmt.Errorf("cannot open module artifact %q in module %s", ref.Path, ref.Module) + } + defer file.Close() + before, err := file.Stat() + if err != nil { + return nil, fmt.Errorf("cannot inspect module artifact %q in module %s", ref.Path, ref.Module) + } + if !before.Mode().IsRegular() { + return nil, fmt.Errorf("module artifact %q in module %s must be a regular file", ref.Path, ref.Module) + } + // One byte over core's inclusive inline bound is read deliberately: core + // refuses the oversize value by its own threshold, and reading no further + // keeps an enormous file from being pulled into memory to be rejected. + content, err := io.ReadAll(io.LimitReader(file, resources.FileCarrierThreshold+1)) + if err != nil { + return nil, fmt.Errorf("cannot read module artifact %q in module %s", ref.Path, ref.Module) + } + after, err := file.Stat() + if err != nil { + return nil, fmt.Errorf("cannot re-inspect module artifact %q in module %s", ref.Path, ref.Module) + } + if !os.SameFile(before, after) || after.Size() != before.Size() || !after.ModTime().Equal(before.ModTime()) || int64(len(content)) != before.Size() { + return nil, fmt.Errorf("module artifact %q in module %s changed while it was being read", ref.Path, ref.Module) + } + // And the SELECTION, re-asked around the read — compared WHOLE, not by + // path. The same-open-file checks above only say the file did not move + // under us; they say nothing about whether this is still the selection the + // composition makes. + // + // The path alone cannot answer that. ResolveModule answers from the overlay + // snapshot the workspace was loaded with, so a non-managed selection — an + // explicit path or a worktree, which carries no receipt — resolves to the + // same directory on both asks even after the overlay on disk has been + // re-pointed; and a selection can change mode or the request it answers + // while keeping its directory. Comparing the descriptor, which carries the + // disk-read directive, the receipt's request and the derived mode beside + // the directory, is what makes those visible. It is cheap: no pull, no + // reload, and the refusal is the same one as before the read. + reselected, err := composition.SelectModule(ctx, workspace, ref.Module) + if err != nil { + return nil, err + } + if *reselected != *selected { + return nil, fmt.Errorf("module %s was reselected while artifact %q was being read; resolve the composition again", ref.Module, ref.Path) + } + return content, nil +} + +// deliveryBaseline is a phase's configuration payload as core must see it: +// every offer, in the order it was gathered, with nothing merged, deduplicated +// or reduced to a map first. A duplicate coordinate, an alias and a repeated +// pointer all stay visible, because the offer that would be LOST to precedence +// is exactly the one core has to refuse. +func deliveryBaseline(own *basev0.Configuration, workspace, dependencies []*basev0.Configuration) []*basev0.Configuration { + baseline := make([]*basev0.Configuration, 0, len(workspace)+len(dependencies)+1) + if own != nil { + baseline = append(baseline, own) + } + baseline = append(baseline, workspace...) + return append(baseline, dependencies...) +} + +// detachConfigurations is the retained copy of a delivered payload: typed +// clones, so the generated artifact evidence travels with each value and a +// later load, filter or clone of the original slices cannot quietly rewrite +// what this service was delivered. +func detachConfigurations(confs []*basev0.Configuration) []*basev0.Configuration { + detached := make([]*basev0.Configuration, 0, len(confs)) + for _, conf := range confs { + if conf == nil { + continue + } + detached = append(detached, proto.CloneOf(conf)) + } + return detached +} diff --git a/pkg/orchestration/module_artifact_delivery_test.go b/pkg/orchestration/module_artifact_delivery_test.go new file mode 100644 index 00000000..189e8241 --- /dev/null +++ b/pkg/orchestration/module_artifact_delivery_test.go @@ -0,0 +1,220 @@ +package orchestration + +import ( + "context" + "os" + "path/filepath" + "testing" + + basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" + "github.com/codefly-dev/core/resources" + "github.com/stretchr/testify/require" +) + +// boundWorkspace is a composition whose selected profile binds one public +// module artifact to a workspace destination: the shape Product would author +// for the reviewed Documents installation. The artifact lives in the composed +// module, selected by an explicit committed path. +func boundWorkspace(t *testing.T, body string, extra map[string]string) *resources.Workspace { + t.Helper() + files := map[string]string{ + "workspace.codefly.yaml": `name: artifact-delivery +layout: modules +modules: + - name: wiki + path: modules/wiki +`, + "modules/wiki/module.codefly.yaml": excludedRootExportModule, + "modules/wiki/services/backend/service.codefly.yaml": excludedRootExportService + "workspace-configuration-dependencies:\n - docs\n", + "modules/wiki/docs/install.md": body, + "configurations/local/module-artifacts.codefly.json": `{ + "schemaVersion": 1, + "bindings": [ + {"configuration": "docs", "key": "README", "source": {"module": "wiki", "path": "docs/install.md"}} + ] +}`, + } + for name, content := range extra { + files[name] = content + } + return writeTempWorkspace(t, files) +} + +// loadBound reads the workspace configurations the way a run and a check both +// do — through the CLI's own reader, which is where the resolver is bound. +func loadBound(t *testing.T, workspace *resources.Workspace) []*basev0.Configuration { + t.Helper() + reader, err := NewWorkspaceConfigurationReader(context.Background(), workspace) + require.NoError(t, err) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + require.NoError(t, reader.Load(context.Background(), env.Runtime())) + return reader.Configurations() +} + +// boundValue finds the delivered value for the bound destination. +func boundValue(confs []*basev0.Configuration) *basev0.ConfigurationValue { + for _, conf := range confs { + for _, info := range conf.GetInfos() { + if info.GetName() != "docs" { + continue + } + for _, value := range info.GetConfigurationValues() { + if value.GetKey() == "README" { + return value + } + } + } + } + return nil +} + +// TestAnOrdinaryLoadDeliversTheBoundArtifactWithItsEvidence is the seam end to +// end: the CLI's reader supplies the resolver, core reads the declaration +// during profile loading, acquires the bytes once and attaches the identity it +// computed FROM those bytes. +func TestAnOrdinaryLoadDeliversTheBoundArtifactWithItsEvidence(t *testing.T) { + body := "# Install\n\nRun `codefly run`.\n" + workspace := boundWorkspace(t, body, nil) + + value := boundValue(loadBound(t, workspace)) + require.NotNil(t, value, "the bound destination was delivered") + require.Equal(t, body, value.GetValue(), "exact bytes, newlines included") + require.Nil(t, value.GetModuleArtifact(), "the declaration is resolved, not delivered unresolved") + evidence := value.GetResolvedModuleArtifact() + require.NotNil(t, evidence) + require.Equal(t, "wiki", evidence.GetReference().GetModule()) + require.Equal(t, "docs/install.md", evidence.GetReference().GetPath()) + require.Equal(t, int64(len(body)), evidence.GetArtifactIdentity().GetSizeBytes()) + require.NoError(t, resources.ValidateModuleArtifactValue(value)) +} + +// TestANextLoadDeliversChangedSource is the per-load contract, and the reason +// there is no flow-spanning freeze: an earlier observation describes itself, +// and the next supported load delivers what the source says then. +func TestANextLoadDeliversChangedSource(t *testing.T) { + workspace := boundWorkspace(t, "A\n", nil) + require.Equal(t, "A\n", boundValue(loadBound(t, workspace)).GetValue()) + + require.NoError(t, os.WriteFile(filepath.Join(workspace.Dir(), "modules/wiki/docs/install.md"), []byte("B\n"), 0o600)) + later := boundValue(loadBound(t, workspace)) + require.Equal(t, "B\n", later.GetValue(), "the next load acquires the then-selected source") + require.NoError(t, resources.ValidateModuleArtifactValue(later)) +} + +// TestALiteralCannotSilentlyTakeTheBoundDestination is the transition rule: +// moving from a literal to a binding is an explicit reviewed source edit, so +// the two declarations together refuse rather than one quietly winning — +// whichever way precedence happens to fall. +func TestALiteralCannotSilentlyTakeTheBoundDestination(t *testing.T) { + workspace := boundWorkspace(t, "# Install\n", map[string]string{ + "configurations/local/docs.env": "README=a literal nobody removed\n", + }) + reader, err := NewWorkspaceConfigurationReader(context.Background(), workspace) + require.NoError(t, err) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + require.Error(t, reader.Load(context.Background(), env.Runtime())) +} + +// TestAMissingArtifactRefusesTheLoad keeps the failure closed: there is no +// cached literal, per-key fallback or empty success for a declared binding +// whose source is gone. +func TestAMissingArtifactRefusesTheLoad(t *testing.T) { + workspace := boundWorkspace(t, "# Install\n", nil) + require.NoError(t, os.Remove(filepath.Join(workspace.Dir(), "modules/wiki/docs/install.md"))) + + reader, err := NewWorkspaceConfigurationReader(context.Background(), workspace) + require.NoError(t, err) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + err = reader.Load(context.Background(), env.Runtime()) + require.Error(t, err) + require.Contains(t, err.Error(), "wiki", "the refusal names the declared module") + require.Contains(t, err.Error(), "docs/install.md", "and its declared path") +} + +// TestACheckLoadAcquiresItsOwnObservation is the standalone-check rule: it +// uses the same loader and resolver as a run, and records what IT saw — it +// neither inherits a run's acquisition nor proves a later delivery. +func TestACheckLoadAcquiresItsOwnObservation(t *testing.T) { + workspace := boundWorkspace(t, "A\n", nil) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + + checked, _, err := WorkspaceConfigurationsForChecking(context.Background(), workspace, env) + require.NoError(t, err) + value := boundValue([]*basev0.Configuration{{Origin: resources.ConfigurationWorkspace, Infos: checked.Infos}}) + require.NotNil(t, value) + require.Equal(t, "A\n", value.GetValue()) + require.NotNil(t, value.GetResolvedModuleArtifact()) +} + +// TestAnAdditionalOfferCannotTakeABoundDestination is the delivery check at +// the phase seams, exercised through the call the runner and the excluded-root +// export both make. Equality gives no replacement right, an empty override map +// does not skip the configuration-to-configuration comparison, and an +// unrelated offer is untouched. +func TestAnAdditionalOfferCannotTakeABoundDestination(t *testing.T) { + workspace := boundWorkspace(t, "# Install\n", nil) + delivered := detachConfigurations(loadBound(t, workspace)) + require.NotNil(t, boundValue(delivered)) + + offer := func(key, text string) []*basev0.Configuration { + return []*basev0.Configuration{{Origin: resources.ConfigurationWorkspace, Infos: []*basev0.ConfigurationInformation{{ + Name: "docs", ConfigurationValues: []*basev0.ConfigurationValue{{Key: key, Value: text}}, + }}}} + } + + require.NoError(t, resources.ValidateModuleArtifactDelivery(delivered, nil, nil), + "the single legitimate bound value, with no contribution, passes") + require.Error(t, resources.ValidateModuleArtifactDelivery(delivered, offer("README", "something else"), nil), + "a returned or dependency offer for the bound destination refuses with an empty override map") + require.Error(t, resources.ValidateModuleArtifactDelivery(delivered, offer("README", "# Install\n"), nil), + "and copying the bytes buys no replacement authority") + require.NoError(t, resources.ValidateModuleArtifactDelivery(delivered, offer("CHANGELOG", "unrelated"), nil), + "an unrelated key keeps its ordinary behaviour") + + // The override map is compared by carrier, not by value, including the + // hyphen and secret-namespace spellings the SDK accepts. + key := resources.ConfigurationValueEnvironmentKey( + &basev0.Configuration{Origin: resources.ConfigurationWorkspace}, + "docs", &basev0.ConfigurationValue{Key: "README"}) + require.Error(t, resources.ValidateModuleArtifactDelivery(delivered, nil, map[string]string{key: "via --set"}), + "a runtime override for the bound carrier refuses") + require.NoError(t, resources.ValidateModuleArtifactDelivery(delivered, nil, map[string]string{"CODEFLY__UNRELATED": "fine"})) +} + +// TestTheExcludedRootExportChecksItsOwnPayload closes the one export that +// runs no agent Init: there is no returned contribution and no retained +// delivery, so the selected payload is checked against itself and against the +// export's own override map before the first append. +func TestTheExcludedRootExportChecksItsOwnPayload(t *testing.T) { + ctx := context.Background() + workspace := boundWorkspace(t, "# Install\n", nil) + module, err := workspace.LoadModuleFromName(ctx, "wiki") + require.NoError(t, err) + service, err := module.LoadServiceFromName(ctx, "backend") + require.NoError(t, err) + env, err := SelectEnvironment(workspace, LocalEnvironmentName) + require.NoError(t, err) + + flow, err := NewFlow(ctx, workspace, module, service, env, RunMode) + require.NoError(t, err) + output := filepath.Join(t.TempDir(), "runtime.env") + flow.WithRuntimeContext(resources.RuntimeContextNative) + flow.WithExcludeRoot(true) + flow.WithOutputEnv(output) + // An override aimed at the bound carrier: the export must refuse before it + // writes anything at all. + flow.WithOverrides(map[string]map[string]string{ + resources.WithUnique(service).Unique(): { + resources.ConfigurationValueEnvironmentKey( + &basev0.Configuration{Origin: resources.ConfigurationWorkspace}, + "docs", &basev0.ConfigurationValue{Key: "README"}): "substituted", + }, + }) + require.Error(t, flow.exportExcludedOriginEnvironment(ctx)) + _, statErr := os.Stat(output) + require.True(t, os.IsNotExist(statErr), "an absent output stays absent") +} diff --git a/pkg/orchestration/module_artifact_test.go b/pkg/orchestration/module_artifact_test.go new file mode 100644 index 00000000..dcf9505f --- /dev/null +++ b/pkg/orchestration/module_artifact_test.go @@ -0,0 +1,273 @@ +package orchestration + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + + "github.com/codefly-dev/cli/pkg/composition" + basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" + "github.com/codefly-dev/core/resources" + "github.com/stretchr/testify/require" +) + +// artifactWorkspace is a one-module composition selected by an explicit +// committed path: a developer checkout, which legitimately has no +// materialization receipt, so acquisition succeeds and its provenance is local +// development evidence rather than release authenticity. +func artifactWorkspace(t *testing.T) *resources.Workspace { + t.Helper() + return writeTempWorkspace(t, map[string]string{ + "workspace.codefly.yaml": `name: artifact-export +layout: modules +modules: + - name: wiki + path: modules/wiki +`, + "modules/wiki/module.codefly.yaml": excludedRootExportModule, + "modules/wiki/services/backend/service.codefly.yaml": excludedRootExportService, + }) +} + +func artifactReference(module, path string) *basev0.ConfigurationModuleArtifactReference { + return &basev0.ConfigurationModuleArtifactReference{Module: module, Path: path} +} + +// TestTheResolverAcquiresTheSelectedModulesExactBytes is the delivery truth: +// whitespace and newlines included, byte for byte, with no reserialization. +func TestTheResolverAcquiresTheSelectedModulesExactBytes(t *testing.T) { + ctx := context.Background() + workspace := artifactWorkspace(t) + body := "# Install\n\n indented\ttab\n\nlast line without newline" + require.NoError(t, os.MkdirAll(filepath.Join(workspace.Dir(), "modules/wiki/docs"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(workspace.Dir(), "modules/wiki/docs/install.md"), []byte(body), 0o600)) + + content, err := ModuleArtifactResolverFor(workspace)(ctx, artifactReference("wiki", "docs/install.md")) + require.NoError(t, err) + require.Equal(t, body, string(content)) +} + +// TestTheResolverRefusesWhatItCannotConfineOrBound covers the acquisition +// obligations core hands to the CLI. Every one of these is a real file or a +// real path, not a stub: a string prefix check followed by an unrestricted +// open passes several of them. +func TestTheResolverRefusesWhatItCannotConfineOrBound(t *testing.T) { + ctx := context.Background() + workspace := artifactWorkspace(t) + module := filepath.Join(workspace.Dir(), "modules/wiki") + require.NoError(t, os.MkdirAll(filepath.Join(module, "docs"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(module, "docs/install.md"), []byte("ok\n"), 0o600)) + + // A secret parked beside the composition, which a traversal or a symlink + // out of the module would otherwise deliver as public configuration. + require.NoError(t, os.WriteFile(filepath.Join(workspace.Dir(), "outside.txt"), []byte("not-public\n"), 0o600)) + require.NoError(t, os.Symlink(filepath.Join(workspace.Dir(), "outside.txt"), filepath.Join(module, "docs/escape.md"))) + require.NoError(t, os.Symlink("../../../outside.txt", filepath.Join(module, "docs/relative-escape.md"))) + require.NoError(t, syscall.Mkfifo(filepath.Join(module, "docs/pipe"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(module, "docs/oversize.md"), + []byte(strings.Repeat("x", resources.FileCarrierThreshold+1)), 0o600)) + + resolve := ModuleArtifactResolverFor(workspace) + for _, probe := range []struct { + name string + ref *basev0.ConfigurationModuleArtifactReference + }{ + {"an absolute path", artifactReference("wiki", filepath.Join(workspace.Dir(), "outside.txt"))}, + {"a traversing path", artifactReference("wiki", "../../outside.txt")}, + {"an absolute symlink out of the module", artifactReference("wiki", "docs/escape.md")}, + {"a relative symlink out of the module", artifactReference("wiki", "docs/relative-escape.md")}, + {"a FIFO, which would block the read", artifactReference("wiki", "docs/pipe")}, + {"a directory", artifactReference("wiki", "docs")}, + {"a missing file", artifactReference("wiki", "docs/absent.md")}, + {"an empty path", artifactReference("wiki", "")}, + {"a module the composition does not name", artifactReference("ghost", "docs/install.md")}, + } { + t.Run(probe.name, func(t *testing.T) { + content, err := resolve(ctx, probe.ref) + require.Error(t, err) + require.Empty(t, content) + require.NotContains(t, err.Error(), "not-public", "the refusal never carries the content it refused") + }) + } + + // One byte over core's inclusive bound is acquired, then refused by core's + // own byte admission — the CLI reads the extra byte precisely so the + // refusal can be made rather than silently truncating. + oversize, err := resolve(ctx, artifactReference("wiki", "docs/oversize.md")) + require.NoError(t, err) + require.Len(t, oversize, resources.FileCarrierThreshold+1) + require.Error(t, resources.ValidateModuleArtifactBytes(string(oversize))) + + // And the inclusive threshold itself is admitted. + require.NoError(t, os.WriteFile(filepath.Join(module, "docs/exact.md"), + []byte(strings.Repeat("x", resources.FileCarrierThreshold)), 0o600)) + exact, err := resolve(ctx, artifactReference("wiki", "docs/exact.md")) + require.NoError(t, err) + require.NoError(t, resources.ValidateModuleArtifactBytes(string(exact))) +} + +// TestTheResolverRefusesAnUnmaterializedModule is the receipt half seen from +// the consuming side: a composed module this machine has not materialized is +// refused rather than fetched behind the read, and no literal stands in. +func TestTheResolverRefusesAnUnmaterializedModule(t *testing.T) { + ctx := context.Background() + workspace := writeTempWorkspace(t, map[string]string{ + "workspace.codefly.yaml": `name: artifact-pinned +layout: modules +modules: + - name: wiki + source: github.com/codefly-dev/absent + version: 0.0.1 +`, + }) + content, err := ModuleArtifactResolverFor(workspace)(ctx, artifactReference("wiki", "docs/install.md")) + require.Error(t, err) + require.Empty(t, content) +} + +// TestTheDeliveryBaselinePreservesEveryOffer is what makes core's collision +// check possible: the offer precedence would discard is exactly the one that +// must still be visible, so nothing is merged, deduplicated or keyed first. +func TestTheDeliveryBaselinePreservesEveryOffer(t *testing.T) { + value := func(key, text string) *basev0.ConfigurationValue { + return &basev0.ConfigurationValue{Key: key, Value: text} + } + own := &basev0.Configuration{Origin: "wiki/backend", Infos: []*basev0.ConfigurationInformation{{ + Name: "docs", ConfigurationValues: []*basev0.ConfigurationValue{value("README", "a")}, + }}} + first := &basev0.Configuration{Origin: resources.ConfigurationWorkspace, Infos: []*basev0.ConfigurationInformation{{ + Name: "docs", ConfigurationValues: []*basev0.ConfigurationValue{value("README", "b")}, + }}} + // The same pointer offered twice, and a duplicate coordinate: both survive. + baseline := deliveryBaseline(own, []*basev0.Configuration{first, first}, []*basev0.Configuration{first}) + require.Len(t, baseline, 4) + require.Same(t, own, baseline[0]) + for _, conf := range baseline[1:] { + require.Same(t, first, conf) + } + require.Empty(t, deliveryBaseline(nil, nil, nil)) +} + +// TestTheRetainedDeliveryIsDetached locks what Start compares against. The +// retained payload must not move when the slices it was gathered from are +// mutated by a later load, and the generated artifact evidence must travel +// with each value — a copy that dropped it would protect nothing. +func TestTheRetainedDeliveryIsDetached(t *testing.T) { + body := "# Install\n" + digest := sha256.Sum256([]byte(body)) + evidence := &basev0.ResolvedConfigurationModuleArtifact{ + Reference: artifactReference("wiki", "docs/install.md"), + ArtifactIdentity: &basev0.SourceIdentity{ + Algorithm: basev0.SourceIdentityAlgorithm_SOURCE_IDENTITY_ALGORITHM_SHA256, + Digest: hex.EncodeToString(digest[:]), + SizeBytes: int64(len(body)), + }, + } + source := []*basev0.Configuration{nil, {Origin: resources.ConfigurationWorkspace, Infos: []*basev0.ConfigurationInformation{{ + Name: "docs", ConfigurationValues: []*basev0.ConfigurationValue{ + {Key: "README", Value: body, ResolvedModuleArtifact: evidence}, + }, + }}}} + + detached := detachConfigurations(source) + require.Len(t, detached, 1, "a nil offer carries nothing to retain") + retained := detached[0].Infos[0].ConfigurationValues[0] + require.Equal(t, body, retained.Value) + require.NotNil(t, retained.ResolvedModuleArtifact, "the evidence travels with the copy") + require.NotSame(t, evidence, retained.ResolvedModuleArtifact, "and it is a clone, not the same message") + require.NoError(t, resources.ValidateModuleArtifactValue(retained)) + + source[1].Infos[0].ConfigurationValues[0].Value = "rewritten" + require.Equal(t, body, detached[0].Infos[0].ConfigurationValues[0].Value, + "the retained delivery is what the service received, whatever happens to the slices it came from") +} + +// TestTheSelectionIsComparedWholeNotByPath is the case a path-only recheck +// cannot see. +// +// A non-managed selection — an explicit overlay path, carrying no receipt — +// resolves through the overlay snapshot the workspace was LOADED with, so it +// answers the same directory on every ask even after the overlay on disk has +// been re-pointed. Comparing directories across the read therefore passes +// while the selection has actually moved. The descriptor carries the disk-read +// directive beside the resolved directory, so the move is visible. +func TestTheSelectionIsComparedWholeNotByPath(t *testing.T) { + ctx := context.Background() + workspace := writeTempWorkspace(t, map[string]string{ + "workspace.codefly.yaml": `name: artifact-reselect +layout: modules +modules: + - name: wiki + path: modules/wiki +`, + "modules/wiki/module.codefly.yaml": excludedRootExportModule, + "modules/wiki/services/backend/service.codefly.yaml": excludedRootExportService, + "modules/wiki/docs/install.md": "A\n", + "codefly.local.yaml": "resolve:\n wiki:\n path: modules/wiki\n", + }) + + first, err := composition.SelectModule(ctx, workspace, "wiki") + require.NoError(t, err) + require.False(t, first.Verified, "an explicit overlay path is local development evidence") + require.Equal(t, "modules/wiki", first.DirectivePath) + + // Re-point the module on disk, to a directory that resolves the same way + // through the loaded snapshot. + require.NoError(t, os.MkdirAll(filepath.Join(workspace.Dir(), "elsewhere/wiki/docs"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(workspace.Dir(), "codefly.local.yaml"), + []byte("resolve:\n wiki:\n path: elsewhere/wiki\n"), 0o600)) + + second, err := composition.SelectModule(ctx, workspace, "wiki") + require.NoError(t, err) + require.NotEqual(t, *first, *second, "the selection moved, and the descriptor says so") + require.Equal(t, "elsewhere/wiki", second.DirectivePath, "which the disk-read directive is what carries") + require.Equal(t, first.Dir, second.Dir, + "while the RESOLVED directory is unchanged — which is exactly why a path-only recheck passed") +} + +// TestANearerOverlayChangesTheSelectionWithoutChangingItsPath is the last way +// a selection can move while every other field of it holds still. +// +// A directive path is relative to the overlay file that declares it. A parent +// overlay saying `path: modules/wiki` and a nearer workspace overlay saying the +// very same thing select DIFFERENT directories, so comparing the raw path text +// cannot tell them apart — and the resolved directory cannot either, because +// ResolveModule answers from the snapshot the workspace was loaded with. The +// overlay the selection was read from is what distinguishes them. +func TestANearerOverlayChangesTheSelectionWithoutChangingItsPath(t *testing.T) { + ctx := context.Background() + parent := t.TempDir() + child := filepath.Join(parent, "composition") + write := func(name, body string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(name), 0o755)) + require.NoError(t, os.WriteFile(name, []byte(body), 0o600)) + } + const overlay = "resolve:\n wiki:\n path: modules/wiki\n" + write(filepath.Join(parent, "codefly.local.yaml"), overlay) + write(filepath.Join(child, "workspace.codefly.yaml"), + "name: origin-change\nlayout: modules\nmodules:\n - name: wiki\n source: github.com/example/wiki\n version: 1.0.0\n") + write(filepath.Join(parent, "modules/wiki/docs/install.md"), "A\n") + write(filepath.Join(child, "modules/wiki/docs/install.md"), "B\n") + + workspace, err := resources.LoadWorkspaceFromDir(ctx, child) + require.NoError(t, err) + before, err := composition.SelectModule(ctx, workspace, "wiki") + require.NoError(t, err) + require.Equal(t, parent, before.OverlayDir, "the parent overlay is the nearest one") + + // The same text, declared nearer. Nothing about the directive changes. + write(filepath.Join(child, "codefly.local.yaml"), overlay) + after, err := composition.SelectModule(ctx, workspace, "wiki") + require.NoError(t, err) + + require.Equal(t, before.DirectivePath, after.DirectivePath, "the path text is identical") + require.Equal(t, before.Dir, after.Dir, "and so is the resolved directory, from the loaded snapshot") + require.NotEqual(t, *before, *after, "yet the selection moved, and the overlay origin says so") + require.Equal(t, child, after.OverlayDir) +} diff --git a/pkg/orchestration/output_records.go b/pkg/orchestration/output_records.go new file mode 100644 index 00000000..ff504a0c --- /dev/null +++ b/pkg/orchestration/output_records.go @@ -0,0 +1,109 @@ +package orchestration + +import ( + "fmt" + "strings" + + "github.com/codefly-dev/core/resources" +) + +// outputRecord is one line of the `--output-env` artifact, already serialized. +// +// The string is converted ONCE, here, and carried through validation to the +// write: a value is an `any`, so converting it again at write time could +// produce a different string from the one that was checked — which is the +// whole hole a format check is supposed to close. +type outputRecord struct { + key string + line string +} + +// outputRecords converts an emitter's variables into serialized records and +// holds the complete batch to the format the supported reader actually parses. +// +// The reader (sdk-go LoadRuntimeEnvironmentFile) scans lines, strips one +// trailing CR, cuts at the FIRST `=`, and refuses the whole file if any name +// is not `CODEFLY__` plus at least one character of [A-Z0-9_]. So this is +// transport grammar, not a second semantic carrier matcher, and it applies to +// EVERY record in the batch — bound artifact, ordinary value, override, +// identity, fixture, endpoint alike. A value carrying LF is the reason: the +// reader would read its tail as another record, and `NOTE=note\nCODEFLY__…=B` +// replaces a value no key matching could have protected. CR is refused +// because the reader silently eats a trailing one; NUL because a process +// environment cannot carry it. +// +// Diagnostics name a record's index and a generic reason, or a key that is +// already known to be well formed. A malformed key and every value stay out of +// the message: they are the attacker-controlled halves. +func outputRecords(environments []*resources.EnvironmentVariable) ([]outputRecord, error) { + records := make([]outputRecord, 0, len(environments)) + for index, env := range environments { + if env == nil { + return nil, fmt.Errorf("output environment record %d is empty", index) + } + if err := validOutputKey(env.Key); err != nil { + return nil, fmt.Errorf("output environment record %d: %w", index, err) + } + // resources.EnvironmentVariable.String is this same conversion; it is + // spelled out so the retained line and the checked line are one value. + line := fmt.Sprintf("%s=%v", env.Key, env.Value) + if err := validOutputValue(line[len(env.Key)+1:]); err != nil { + return nil, fmt.Errorf("output environment carrier %s: %w", env.Key, err) + } + records = append(records, outputRecord{key: env.Key, line: line}) + } + return records, nil +} + +// validOutputKey is the reader's own name grammar, byte for byte. +func validOutputKey(key string) error { + const prefix = "CODEFLY__" + if !strings.HasPrefix(key, prefix) || len(key) == len(prefix) { + return fmt.Errorf("its name is not a CODEFLY__ carrier, so the supported reader refuses the whole artifact") + } + for index := 0; index < len(key); index++ { + character := key[index] + if (character < 'A' || character > 'Z') && (character < '0' || character > '9') && character != '_' { + return fmt.Errorf("its name is not uppercase ASCII, digits and underscores, so the supported reader refuses the whole artifact") + } + } + return nil +} + +// validOutputValue refuses the delimiters this format cannot represent. An +// empty value and a value holding one or many `=` are representable: the +// reader cuts at the first separator and keeps the rest verbatim. +func validOutputValue(value string) error { + if strings.ContainsAny(value, "\n\r\x00") { + return fmt.Errorf("its value contains a line break or NUL, which this raw output format cannot represent; direct process delivery carries it unchanged") + } + return nil +} + +// validRetainedOutputRecords refuses an existing artifact this writer would +// otherwise rewrite line by line. A retained line is not authenticated source +// evidence — syntax says nothing about where a well formed old record came +// from — but rewriting a malformed one would launder it into output this run +// is answerable for. +func validRetainedOutputRecords(existing string) error { + for number, line := range strings.Split(existing, "\n") { + if line == "" { + continue + } + key, value, found := strings.Cut(line, "=") + if !found { + return fmt.Errorf("the existing output environment holds a record at line %d with no separator", number+1) + } + if err := validOutputKey(key); err != nil { + return fmt.Errorf("the existing output environment holds a record at line %d whose name is not a supported carrier", number+1) + } + // The value too, not just the name: the rewrite reproduces this line + // verbatim, and a retained value carrying CR or NUL is exactly as + // unreadable as a new one. Splitting on LF cannot leave an embedded LF + // here, but CR and NUL survive it. + if err := validOutputValue(value); err != nil { + return fmt.Errorf("the existing output environment holds a record at line %d whose value this format cannot represent", number+1) + } + } + return nil +} diff --git a/pkg/orchestration/output_records_test.go b/pkg/orchestration/output_records_test.go new file mode 100644 index 00000000..1cb3dbe0 --- /dev/null +++ b/pkg/orchestration/output_records_test.go @@ -0,0 +1,234 @@ +package orchestration + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" + "github.com/codefly-dev/core/resources" +) + +// variable is one record offered to the writer. +func variable(key string, value any) *resources.EnvironmentVariable { + return &resources.EnvironmentVariable{Key: key, Value: value} +} + +// TestTheOutputWriterRefusesRecordsTheReaderCannotParse is the counterexample +// the whole-record rule exists for: an unrelated, UNBOUND value whose text +// carries an LF and a second `KEY=value` line. Semantic carrier matching +// cannot see it — the record's own key is nothing special — but the reader +// scans lines, so that tail lands in the process environment as a value for a +// key the run never offered. Refusing every unrepresentable record, bound or +// not, is what closes it. +func TestTheOutputWriterRefusesRecordsTheReaderCannotParse(t *testing.T) { + ctx := context.Background() + for _, probe := range []struct { + name string + records []*resources.EnvironmentVariable + }{ + {"a line break smuggles a second record", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE", "note\nCODEFLY__WORKSPACE_CONFIGURATION__DOCS__README=substituted"), + }}, + {"a carriage return the reader would silently eat", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE", "note\r"), + }}, + {"a NUL a process environment cannot carry", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE", "note\x00tail"), + }}, + {"a key outside the reader's prefix", []*resources.EnvironmentVariable{ + variable("NOTE", "plain"), + }}, + {"a key with no suffix at all", []*resources.EnvironmentVariable{ + variable("CODEFLY__", "plain"), + }}, + {"a lowercase key", []*resources.EnvironmentVariable{ + variable("CODEFLY__note", "plain"), + }}, + {"a hyphenated key", []*resources.EnvironmentVariable{ + variable("CODEFLY__READ-WRITE", "plain"), + }}, + {"a key carrying the separator", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE=X", "plain"), + }}, + {"a key carrying a line break", []*resources.EnvironmentVariable{ + variable("CODEFLY__NOTE\nCODEFLY__OTHER", "plain"), + }}, + // The offending record is LAST, which is the point: a check that ran + // per append would already have written the valid one. + {"an invalid record after a valid one", []*resources.EnvironmentVariable{ + variable("CODEFLY__WORKSPACE_CONFIGURATION__DOCS__README", "fine"), + variable("CODEFLY__NOTE", "note\nCODEFLY__WORKSPACE_CONFIGURATION__DOCS__README=substituted"), + }}, + } { + t.Run(probe.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "runtime.env") + require.Error(t, appendEnvironmentVariablesToFile(ctx, path, probe.records)) + _, err := os.Stat(path) + require.True(t, os.IsNotExist(err), "an absent output stays absent") + + // And an existing artifact keeps its exact bytes. + existing := filepath.Join(t.TempDir(), "runtime.env") + require.NoError(t, appendEnvironmentVariablesToFile(ctx, existing, []*resources.EnvironmentVariable{ + variable("CODEFLY__SENTINEL", "untouched"), + })) + before, err := os.ReadFile(existing) + require.NoError(t, err) + require.Error(t, appendEnvironmentVariablesToFile(ctx, existing, probe.records)) + after, err := os.ReadFile(existing) + require.NoError(t, err) + require.Equal(t, before, after, "a refused batch leaves the existing artifact byte-identical") + }) + } +} + +// TestRefusalDiagnosticsCarryNoValuesOrMalformedKeys keeps the refusal from +// becoming the leak: a malformed key and every value are the halves an +// attacker controls, so the message names a record index or an already valid +// carrier and a generic reason. +func TestRefusalDiagnosticsCarryNoValuesOrMalformedKeys(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "runtime.env") + + err := appendEnvironmentVariablesToFile(ctx, path, []*resources.EnvironmentVariable{ + variable("CODEFLY__FINE", "ok"), + variable("totally-made-up-key", "secret-value"), + }) + require.Error(t, err) + require.Contains(t, err.Error(), "record 1") + require.NotContains(t, err.Error(), "totally-made-up-key") + require.NotContains(t, err.Error(), "secret-value") + + err = appendEnvironmentVariablesToFile(ctx, path, []*resources.EnvironmentVariable{ + variable("CODEFLY__TOKEN", "line\nsecret-value"), + }) + require.Error(t, err) + require.Contains(t, err.Error(), "CODEFLY__TOKEN") + require.NotContains(t, err.Error(), "secret-value") +} + +// readerGrammar is sdk-go v0.4.0's validRuntimeEnvironmentName, transcribed +// for this test ONLY. +// +// The CLI does not depend on root sdk-go and this must not make it: the +// production check lives in validOutputKey, and the agreement between the two +// is itself asserted below, so a divergence shows up here rather than in a +// run. A roundtrip through the real reader is a separate, out-of-process +// check against a built binary. +func readerGrammar(name string) bool { + if !strings.HasPrefix(name, "CODEFLY__") || len(name) == len("CODEFLY__") { + return false + } + for _, character := range name { + switch { + case character >= 'A' && character <= 'Z': + case character >= '0' && character <= '9': + case character == '_': + default: + return false + } + } + return true +} + +// TestTheWriterAgreesWithTheReadersGrammar is what keeps the production rule +// honest: every name the writer admits the reader must admit, and the other +// way round. +func TestTheWriterAgreesWithTheReadersGrammar(t *testing.T) { + for _, name := range []string{ + "CODEFLY__WORKSPACE_CONFIGURATION__DOCS__README", "CODEFLY__A", "CODEFLY__9", "CODEFLY___", + "CODEFLY__", "CODEFLY", "codefly__A", "CODEFLY__a", "CODEFLY__A-B", "CODEFLY__A B", + "CODEFLY__A=B", "CODEFLY__A\nB", "CODEFLY__Ä", "", "NOTE", + resources.ConfigurationDocumentKey(resources.ConfigurationWorkspace, "fixture-composition", "local", false), + } { + require.Equal(t, readerGrammar(name), validOutputKey(name) == nil, + "the writer and the supported reader disagree about a carrier name") + } +} + +// TestRepresentableRecordsRoundTrip is the positive half: an empty value and +// a value holding several `=` survive unchanged, because the reader cuts at +// the FIRST separator only. +func TestRepresentableRecordsRoundTrip(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "runtime.env") + require.NoError(t, appendEnvironmentVariablesToFile(ctx, path, []*resources.EnvironmentVariable{ + variable("CODEFLY__WORKSPACE_CONFIGURATION__DOCS__README", "k=v=w"), + variable("CODEFLY__EMPTY", ""), + variable("CODEFLY__DIGITS_9", "9"), + })) + body, err := os.ReadFile(path) + require.NoError(t, err) + values := map[string]string{} + for _, line := range strings.Split(strings.TrimSuffix(string(body), "\n"), "\n") { + name, value, found := strings.Cut(strings.TrimSuffix(line, "\r"), "=") + require.True(t, found) + require.True(t, readerGrammar(name)) + values[name] = value + } + require.Equal(t, map[string]string{ + "CODEFLY__WORKSPACE_CONFIGURATION__DOCS__README": "k=v=w", + "CODEFLY__EMPTY": "", + "CODEFLY__DIGITS_9": "9", + }, values) +} + +// TestTheWriterRefusesMalformedRetainedRecords covers the rewrite direction. +// The writer rereads the file and rewrites it whole, so a malformed line +// already there would be reproduced under this run's name. Syntax says +// nothing about where a well formed old record came from — this is integrity, +// not provenance — but laundering a record the reader refuses is still the +// writer's doing. +func TestTheWriterRefusesMalformedRetainedRecords(t *testing.T) { + ctx := context.Background() + for _, retained := range []string{ + "NOTE=plain\n", + "CODEFLY__lower=plain\n", + "no-separator-at-all\n", + } { + path := filepath.Join(t.TempDir(), "runtime.env") + require.NoError(t, os.WriteFile(path, []byte(retained), 0o600)) + require.Error(t, appendEnvironmentVariablesToFile(ctx, path, []*resources.EnvironmentVariable{ + variable("CODEFLY__FINE", "ok"), + })) + body, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, retained, string(body), "the refused rewrite left the file alone") + } +} + +// TestAStructuredConfigurationDocumentCannotBeExportedRaw records a defect +// this format rule surfaced rather than created, and pins the CLI's honest +// half of it. +// +// resources.ConfigurationDocumentKey is a prefix plus a LOWERCASE hex SHA256, +// and sdk-go's validRuntimeEnvironmentName accepts only A–Z, 0–9 and `_`. So +// the reader refuses the WHOLE artifact whenever a run exports a structured +// configuration document — platform-core's merged fixture-composition is +// exactly that shape. Before this check the CLI wrote such a file and +// reported success; nothing could ever load it. Refusing before the first +// write is the only answer available here: making it exportable needs either +// an uppercase document carrier in core or a reader in sdk-go that accepts +// this one, and neither belongs to the CLI. +func TestAStructuredConfigurationDocumentCannotBeExportedRaw(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "runtime.env") + native := resources.NewRuntimeContextNative() + document := &basev0.Configuration{Origin: resources.ConfigurationWorkspace, RuntimeContext: native, Infos: []*basev0.ConfigurationInformation{{ + Name: "fixture-composition", + Data: &basev0.ConfigurationData{Kind: "yaml", Content: []byte("schema: codefly.dev/authority/fixture-composition/v1\nroles: []\n")}, + }}} + key := resources.ConfigurationDocumentKey(resources.ConfigurationWorkspace, "fixture-composition", "local", false) + require.True(t, strings.HasPrefix(key, "CODEFLY__"), "the prefix is fine; the digest is not") + require.Error(t, validOutputKey(key), "the reader's own grammar refuses a lowercase hex digest") + + err := AppendServiceProcessConfigurationsToFile(ctx, path, localRunEnvironment(), native, nil, + []*basev0.Configuration{document}, nil, nil) + require.Error(t, err) + _, statErr := os.Stat(path) + require.True(t, os.IsNotExist(statErr), "nothing was written") +} diff --git a/pkg/orchestration/runner.go b/pkg/orchestration/runner.go index 65b81118..8fd5d2ad 100644 --- a/pkg/orchestration/runner.go +++ b/pkg/orchestration/runner.go @@ -71,6 +71,16 @@ type Runner struct { // state says it does (addressesMoved). startedMappings string + // delivered is the configuration payload this service's last SUCCESSFUL + // Init actually received, detached from the slices it was gathered from. + // Start compares its own offers against it (module_artifact.go): a + // destination a module artifact was bound to at Init cannot be taken over + // later by a dependency's or an override's value for the same carrier. + // Nil until an Init succeeds — a failed one leaves no baseline rather than + // an empty one that would protect nothing — and replaced whole by the next + // successful Init, which is the ordinary way a reload adopts new bytes. + delivered []*basev0.Configuration + restartMu sync.Mutex pendingRestart ActionType restartInFlight bool @@ -294,13 +304,6 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { if err = runner.checkInitialPortAvailability(ctx, inputs.networkMappings); err != nil { return nil, w.Wrapf(err, "cannot initialize %s", runner.instance.Unique()) } - // A container-only service with a build recipe gets the image this machine - // just built, as a canonical digest reference; everything else keeps the - // agent's own image selection and hands Init an empty field. - runtimeImage, err := runner.localRuntimeImage(ctx) - if err != nil { - return nil, w.Wrapf(err, "cannot prepare local runtime image for %s", runner.instance.Unique()) - } dependenciesEndpoints := inputs.dependenciesEndpoints dependenciesNetworkMappings := inputs.dependenciesNetworkMappings conf := inputs.conf @@ -309,6 +312,47 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { dependenciesConfigurations := inputs.dependenciesConfigurations networkMappings := inputs.networkMappings + // The admitted payload, unmerged, and the process override map, computed + // ONCE: the same map rides the request and is what core compares against, + // so a late callback cannot make the request and the check disagree. Core + // owns the comparison (carriers, aliases, origin/group/key); it refuses an + // override, or a second offer inside this payload, that would take over a + // destination a module artifact is bound to. + overrides := runner.runtimeOverridesFor(networkMappings) + baseline := deliveryBaseline(conf, workspaceConfigurations, dependenciesConfigurations) + if err = resources.ValidateModuleArtifactDelivery(baseline, nil, overrides); err != nil { + return nil, w.Wrapf(err, "cannot initialize %s", runner.instance.Unique()) + } + // Known-invalid output is refused before the agent is called at all, where + // the inputs already exist. This is early refusal, not a substitute for + // checking the response: what the agent returns is unknown until it + // answers, and a refusal then cannot pretend Init had no effects. + if runner.outputEnv != "" { + environment, envErr := runner.world.Env.Proto() + if envErr != nil { + return nil, w.Wrapf(envErr, "cannot name the run environment for the output environment") + } + known, knownErr := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, conf, workspaceConfigurations, nil, nil) + if knownErr != nil { + return nil, knownErr + } + if _, knownErr = outputRecords(known); knownErr != nil { + return nil, w.Wrapf(knownErr, "cannot export the output environment for %s", runner.instance.Unique()) + } + } + + // Only now the image work, which builds and loads: the delivery check above + // has no side effects and must settle first, so a payload this run is going + // to refuse does not leave a built image behind as its only trace. + // + // A container-only service with a build recipe gets the image this machine + // just built, as a canonical digest reference; everything else keeps the + // agent's own image selection and hands Init an empty field. + runtimeImage, err := runner.localRuntimeImage(ctx) + if err != nil { + return nil, w.Wrapf(err, "cannot prepare local runtime image for %s", runner.instance.Unique()) + } + // Init is the only lifecycle call guaranteed to reach a service under test: // a test policy replaces the origin's Start with a barrier or skips it // altogether, so a fixture or an override delivered only at Start reaches @@ -324,7 +368,7 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { WorkspaceConfigurations: workspaceConfigurations, DependenciesConfigurations: dependenciesConfigurations, Fixture: runner.fixture, - Overrides: runner.runtimeOverridesFor(networkMappings), + Overrides: overrides, } err = resources.Validate(req) if err != nil { @@ -368,6 +412,21 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { runner.networkMappings = accepted runner.pinAcceptedPorts(accepted) + // Only a successful Init establishes a delivered payload, and the retained + // copy is detached: the configurations this service actually received stay + // what they were whatever later loads, filters or clones do to the slices + // they came from. A failed or missing Init leaves no baseline rather than + // an empty one. + runner.delivered = detachConfigurations(baseline) + // The returned configurations are an additional offer, checked BEFORE they + // are exposed or written: an agent that answers with its own value for a + // destination a module artifact is bound to does not get to replace it by + // returning the same bytes, and an empty override map does not skip the + // configuration-to-configuration comparison. + if err = resources.ValidateModuleArtifactDelivery(runner.delivered, resp.RuntimeConfigurations, overrides); err != nil { + return nil, w.Wrapf(err, "cannot accept the configurations %s returned from its initialization", runner.instance.Unique()) + } + err = runner.world.ConfigurationManager.ExposeConfiguration(ctx, runner.instance.Identity, resp.RuntimeConfigurations...) if err != nil { return nil, w.Wrapf(err, "cannot record shared configuration infos") @@ -384,17 +443,20 @@ func (runner *Runner) Init(ctx context.Context) (*OutputProperty, error) { if envErr != nil { return nil, w.Wrapf(envErr, "cannot name the run environment for the output environment") } - err = AppendServiceProcessConfigurationsToFile( - ctx, - runner.outputEnv, - environment, - runtimeContext, - conf, - workspaceConfigurations, - nil, - resp.RuntimeConfigurations, - ) - if err != nil { + // One chunk, so the exported slice and the validated batch are the same + // projection: the admitted own/workspace values this run already + // checked, plus what the agent returned. The full admitted payload + // remains the collision baseline above — including bound inputs this + // export omits — rather than being re-offered here as a contribution. + variables, variablesErr := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, conf, workspaceConfigurations, nil, resp.RuntimeConfigurations) + if variablesErr != nil { + return nil, variablesErr + } + records, recordsErr := outputRecords(variables) + if recordsErr != nil { + return nil, w.Wrapf(recordsErr, "cannot export the output environment for %s", runner.instance.Unique()) + } + if err = writeOutputRecords(ctx, runner.outputEnv, records); err != nil { return nil, w.Wrapf(err, "cannot write environment variables to file") } } @@ -917,6 +979,88 @@ func (runner *Runner) InitRemote(ctx context.Context) (*OutputProperty, error) { return runner.outputPropertyForInit.Process(ctx) } +// pendingStartOutput converts everything this Start would export, in the +// order the file has always carried it, and returns it together with the +// configuration offers that are new at this phase — without writing anything. +// Gathering it up front is what lets a refusal leave the output untouched: an +// invalid record in the second chunk must not be found after the first has +// reached disk. It answers (nil, nil, nil) when no output was requested. +func (runner *Runner) pendingStartOutput( + ctx context.Context, + dependenciesNetworkMappings []*basev0.NetworkMapping, + overrides map[string]string, +) ([]outputRecord, []*basev0.Configuration, error) { + if runner.outputEnv == "" { + return nil, nil, nil + } + w := wool.Get(ctx).In("service.NewRunner", wool.ThisField(runner.instance)) + runtimeContext, runtimeErr := resources.NewRuntimeContext(runner.runtimeContext) + if runtimeErr != nil { + return nil, nil, w.Wrapf(runtimeErr, "cannot create output environment runtime context") + } + // Environment export needs the same identity fields the agent receives, + // but it must also work for infrastructure agents whose optional path + // fields are empty. ServiceIdentity.Proto validates those deployment + // fields and is therefore intentionally not the conversion boundary. + identity := &basev0.ServiceIdentity{ + Workspace: runner.instance.Identity.Workspace, + Module: runner.instance.Identity.Module, + Name: runner.instance.Identity.Name, + Version: runner.instance.Identity.Version, + WorkspacePath: runner.instance.Identity.WorkspacePath, + RelativeToWorkspace: runner.instance.Identity.RelativeToWorkspace, + } + // Core guards this same dereference before making the identical call + // (services.RuntimeInstance.Start) and simply forwards the request + // unnarrowed when the module is absent. This carrier cannot do that: + // unnarrowed here means writing dependency addresses whose visibility + // nothing can check. Without a consumer module there is no permitted + // set to compute, so refuse to write rather than write unchecked. + if runner.instance.Module == nil { + return nil, nil, w.NewError("cannot resolve dependency network mappings for output environment: %s has no module", runner.instance.Unique()) + } + endpointMappings, mappingErr := outputEnvNetworkMappings( + runner.world.Workspace, + runner.instance.Module.Name, + runner.instance.Service.ServiceDependencies, + runner.networkMappings, + dependenciesNetworkMappings, + ) + if mappingErr != nil { + return nil, nil, w.Wrapf(mappingErr, "cannot resolve dependency network mappings for output environment") + } + // Dependency agents publish connection configurations during their Init. + // Start is the single point that writes them to the output environment, + // after the dependency barrier: the target's own Init may have observed + // the shared configuration state before a concurrently initialized + // dependency exposed its runtime values, so only here are they complete. + dependencyConfigurations, dependencyErr := runner.world.SharedState.GetDependentConfigurationsFor(ctx, runner.instance.Identity) + if dependencyErr != nil { + return nil, nil, w.Wrapf(dependencyErr, "cannot load dependency configurations for output environment") + } + environment, envErr := runner.world.Env.Proto() + if envErr != nil { + return nil, nil, w.Wrapf(envErr, "cannot name the run environment for the output environment") + } + // Both chunks are converted before either is written, in the order the + // file has always carried them: the dependency configurations, then the + // runtime identity, overrides and endpoints. An invalid record in the + // second one must not be discovered after the first has reached disk. + configurationVariables, variablesErr := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, nil, nil, dependencyConfigurations, nil) + if variablesErr != nil { + return nil, nil, variablesErr + } + runtimeVariables, runtimeVariablesErr := runtimeEnvironmentVariables(ctx, identity, runtimeContext, runner.fixture, overrides, endpointMappings) + if runtimeVariablesErr != nil { + return nil, nil, runtimeVariablesErr + } + batch, recordsErr := outputRecords(append(configurationVariables, runtimeVariables...)) + if recordsErr != nil { + return nil, nil, w.Wrapf(recordsErr, "cannot export the output environment for %s", runner.instance.Unique()) + } + return batch, dependencyConfigurations, nil +} + func (runner *Runner) Start(ctx context.Context) (*OutputProperty, error) { w := wool.Get(ctx).In("service.NewRunner", wool.ThisField(runner.instance)) w.Debug("start") @@ -938,95 +1082,64 @@ func (runner *Runner) Start(ctx context.Context) (*OutputProperty, error) { } } - err := runner.StopIfNeeded(ctx) + // Build the request. Everything this phase will deliver is gathered and + // checked BEFORE the running instance is stopped and before either output + // append: a refusal then leaves an absent output absent and an existing + // one byte-identical, where a refusal discovered between the two appends + // could not. These reads are of shared dependency state, which stopping + // this service does not change. + dependenciesNetworkMappings, err := runner.world.SharedState.GetDependenciesNetworkMappings(ctx, runner.instance.Service) if err != nil { - return nil, w.Wrapf(err, "cannot stopAfter service instance if needed") + return nil, w.Wrapf(err, "cannot load service instance") + } + // Computed once and reused for the delivery check, both appends and the + // StartRequest: the override callbacks derive addresses from shared state + // as it stands now, so asking twice can legitimately give two maps. + overrides := runner.runtimeOverrides() + pending, contributions, err := runner.pendingStartOutput(ctx, dependenciesNetworkMappings, overrides) + if err != nil { + return nil, err } - // Build the request - dependenciesNetworkMappings, err := runner.world.SharedState.GetDependenciesNetworkMappings(ctx, runner.instance.Service) + // A nil baseline is not "nothing to protect": Core reads it as an empty set + // of bound destinations and passes, so a Start that never had a successful + // Init would check this phase's offers against nothing. The missing + // evidence is an orchestration error. + // + // It sits immediately before the validation it protects, not ahead of the + // gathering above: the gathering only reads and converts, writing nothing, + // and its own preconditions — a consumer module to narrow dependency + // visibility against, most of all — must keep reporting themselves rather + // than being masked by this one. + if runner.delivered == nil { + return nil, w.NewError("cannot start %s: no successful initialization retained its configuration delivery, so this phase's configuration offers and overrides cannot be checked against what the service was admitted with", runner.instance.Unique()) + } + + // The retained Init payload is the comparison baseline, not a re-emission: + // Start sends no fresh configuration in its request and exports only the + // dependency batch, so a dependency offer that aliases a destination a + // module artifact was bound to at Init refuses here — byte-equal or not. + if err = resources.ValidateModuleArtifactDelivery(runner.delivered, contributions, overrides); err != nil { + return nil, w.Wrapf(err, "cannot start %s", runner.instance.Unique()) + } + + err = runner.StopIfNeeded(ctx) if err != nil { - return nil, w.Wrapf(err, "cannot load service instance") + return nil, w.Wrapf(err, "cannot stopAfter service instance if needed") } - if runner.outputEnv != "" { - runtimeContext, runtimeErr := resources.NewRuntimeContext(runner.runtimeContext) - if runtimeErr != nil { - return nil, w.Wrapf(runtimeErr, "cannot create output environment runtime context") - } - // Environment export needs the same identity fields the agent receives, - // but it must also work for infrastructure agents whose optional path - // fields are empty. ServiceIdentity.Proto validates those deployment - // fields and is therefore intentionally not the conversion boundary. - identity := &basev0.ServiceIdentity{ - Workspace: runner.instance.Identity.Workspace, - Module: runner.instance.Identity.Module, - Name: runner.instance.Identity.Name, - Version: runner.instance.Identity.Version, - WorkspacePath: runner.instance.Identity.WorkspacePath, - RelativeToWorkspace: runner.instance.Identity.RelativeToWorkspace, - } - // Core guards this same dereference before making the identical call - // (services.RuntimeInstance.Start) and simply forwards the request - // unnarrowed when the module is absent. This carrier cannot do that: - // unnarrowed here means writing dependency addresses whose visibility - // nothing can check. Without a consumer module there is no permitted - // set to compute, so refuse to write rather than write unchecked. - if runner.instance.Module == nil { - return nil, w.NewError("cannot resolve dependency network mappings for output environment: %s has no module", runner.instance.Unique()) - } - endpointMappings, mappingErr := outputEnvNetworkMappings( - runner.world.Workspace, - runner.instance.Module.Name, - runner.instance.Service.ServiceDependencies, - runner.networkMappings, - dependenciesNetworkMappings, - ) - if mappingErr != nil { - return nil, w.Wrapf(mappingErr, "cannot resolve dependency network mappings for output environment") - } - // Dependency agents publish connection configurations during their Init. - // Start is the single point that writes them to the output environment, - // after the dependency barrier: the target's own Init may have observed - // the shared configuration state before a concurrently initialized - // dependency exposed its runtime values, so only here are they complete. - dependencyConfigurations, dependencyErr := runner.world.SharedState.GetDependentConfigurationsFor(ctx, runner.instance.Identity) - if dependencyErr != nil { - return nil, w.Wrapf(dependencyErr, "cannot load dependency configurations for output environment") - } - environment, envErr := runner.world.Env.Proto() - if envErr != nil { - return nil, w.Wrapf(envErr, "cannot name the run environment for the output environment") - } - err = AppendServiceProcessConfigurationsToFile( - ctx, - runner.outputEnv, - environment, - runtimeContext, - nil, - nil, - dependencyConfigurations, - nil, - ) - if err != nil { - return nil, w.Wrapf(err, "cannot write dependency configurations to output environment") - } - if appendErr := AppendRuntimeEnvironmentToFile( - ctx, - runner.outputEnv, - identity, - runtimeContext, - runner.fixture, - runner.runtimeOverrides(), - endpointMappings, - ); appendErr != nil { - return nil, w.Wrapf(appendErr, "cannot write runtime environment variables to file") + + if len(pending) > 0 { + // The same strings that were checked, written through the one writer + // in a single pass rather than rebuilt as two appends. + if err = writeOutputRecords(ctx, runner.outputEnv, pending); err != nil { + return nil, w.Wrapf(err, "cannot write the output environment for %s", runner.instance.Unique()) } } req := &runtimev0.StartRequest{ DependenciesNetworkMappings: dependenciesNetworkMappings, Fixture: runner.fixture, - Overrides: runner.runtimeOverrides(), + Overrides: overrides, } err = resources.Validate(req) if err != nil { @@ -1849,6 +1962,14 @@ func (runner *Runner) WithRemote(environment *environments.Environment) { func AppendEnvironmentVariablesToFile(ctx context.Context, filePath string, environment *basev0.Environment, confs []*basev0.Configuration) error { w := wool.Get(ctx).In("resources.AppendToFile", wool.Field("filePath", filePath)) + // On the UNFILTERED payload, before the filter and before the manager's + // lossy merge: filtering drops offers, and a dropped offer is one the + // collision check can no longer see — while the merge lets an ordinary + // value for a bound destination overwrite it. Same floor as + // serviceProcessConfigurationVariables, for the other exported entrypoint. + if err := resources.ValidateModuleArtifactDelivery(confs, nil, nil); err != nil { + return w.Wrap(err) + } // filter out for native filtered := resources.FilterConfigurations(confs, resources.NewRuntimeContextNative()) m := resources.NewEnvironmentVariableManager() @@ -1893,29 +2014,69 @@ func AppendServiceProcessConfigurationsToFile( dependencyConfigurations []*basev0.Configuration, runtimeConfigurations []*basev0.Configuration, ) error { - w := wool.Get(ctx).In("resources.AppendServiceProcessConfigurationsToFile", wool.Field("filePath", filePath)) + environments, err := serviceProcessConfigurationVariables(ctx, environment, runtimeContext, serviceConfiguration, workspaceConfigurations, dependencyConfigurations, runtimeConfigurations) + if err != nil { + return err + } + return appendEnvironmentVariablesToFile(ctx, filePath, environments) +} + +// serviceProcessConfigurationVariables is that export's conversion, without +// its write: a phase that appends more than one chunk must hold the complete +// batch before the first of them reaches the file, so the conversion and the +// write are separable (output_records.go says why the batch is the unit). +func serviceProcessConfigurationVariables( + ctx context.Context, + environment *basev0.Environment, + runtimeContext *basev0.RuntimeContext, + serviceConfiguration *basev0.Configuration, + workspaceConfigurations []*basev0.Configuration, + dependencyConfigurations []*basev0.Configuration, + runtimeConfigurations []*basev0.Configuration, +) ([]*resources.EnvironmentVariable, error) { + w := wool.Get(ctx).In("resources.AppendServiceProcessConfigurationsToFile") if environment == nil || environment.GetName() == "" { - return w.NewError("the service process environment names no run environment, so a configuration document could not be carried under the key its reader derives") + return nil, w.NewError("the service process environment names no run environment, so a configuration document could not be carried under the key its reader derives") + } + // Before anything is filtered, merged or converted: this is the exported + // entrypoint, so a direct caller reaches it without the phase checks in + // Init and Start, and the manager below is a lossy merge — it appends + // configurations and emits one record per carrier, so an ordinary value + // for a destination a module artifact is bound to simply overwrites it and + // the file ends up with the wrong bytes under the right key. + // + // The complete payload is passed as the comparison baseline with no + // contributions: that checks every offer against every other, which is the + // collision this seam can actually see, without inventing a second offer by + // passing the same values twice. The phases still make their own + // baseline/contribution call with the retained delivery and the phase + // override map; this one is the floor under them, and the record grammar + // check in the writer stays where it is. + if err := resources.ValidateModuleArtifactDelivery( + deliveryBaseline(serviceConfiguration, workspaceConfigurations, append(slices.Clone(dependencyConfigurations), runtimeConfigurations...)), + nil, nil, + ); err != nil { + return nil, w.Wrap(err) } manager := resources.NewEnvironmentVariableManager() manager.SetEnvironment(environment) if err := manager.AddConfigurations(ctx, serviceConfiguration); err != nil { - return w.Wrapf(err, "cannot add service configuration") + return nil, w.Wrapf(err, "cannot add service configuration") } if err := manager.AddConfigurations(ctx, workspaceConfigurations...); err != nil { - return w.Wrapf(err, "cannot add workspace configurations") + return nil, w.Wrapf(err, "cannot add workspace configurations") } if err := manager.AddConfigurations(ctx, resources.FilterConfigurations(dependencyConfigurations, runtimeContext)...); err != nil { - return w.Wrapf(err, "cannot add dependency configurations") + return nil, w.Wrapf(err, "cannot add dependency configurations") } if err := manager.AddConfigurations(ctx, resources.FilterConfigurations(runtimeConfigurations, runtimeContext)...); err != nil { - return w.Wrapf(err, "cannot add runtime configurations") + return nil, w.Wrapf(err, "cannot add runtime configurations") } environments, err := manager.All() if err != nil { - return w.Wrapf(err, "cannot get service process environment variables") + return nil, w.Wrapf(err, "cannot get service process environment variables") } - return appendEnvironmentVariablesToFile(ctx, filePath, environments) + return environments, nil } // outputEnvNetworkMappings is the endpoint set the output environment carries: @@ -2002,9 +2163,28 @@ func AppendRuntimeEnvironmentToFile( overrides map[string]string, endpointMappings []*basev0.NetworkMapping, ) error { - w := wool.Get(ctx).In("resources.AppendRuntimeEnvironmentToFile", wool.Field("filePath", filePath)) + environments, err := runtimeEnvironmentVariables(ctx, identity, runtimeContext, fixture, overrides, endpointMappings) + if err != nil { + return err + } + return appendEnvironmentVariablesToFile(ctx, filePath, environments) +} + +// runtimeEnvironmentVariables is that export's conversion, without its write, +// for the same reason as serviceProcessConfigurationVariables: Start writes +// both chunks, and an invalid record in this one must not be discovered after +// the other has already reached the file. +func runtimeEnvironmentVariables( + ctx context.Context, + identity *basev0.ServiceIdentity, + runtimeContext *basev0.RuntimeContext, + fixture string, + overrides map[string]string, + endpointMappings []*basev0.NetworkMapping, +) ([]*resources.EnvironmentVariable, error) { + w := wool.Get(ctx).In("resources.AppendRuntimeEnvironmentToFile") if identity == nil { - return w.NewError("service identity is required") + return nil, w.NewError("service identity is required") } manager := resources.NewEnvironmentVariableManager() manager.SetIdentity(identity) @@ -2016,21 +2196,49 @@ func AppendRuntimeEnvironmentToFile( endpointMappings, resources.NetworkAccessFromRuntimeContext(runtimeContext), ); err != nil { - return w.Wrapf(err, "cannot add runtime endpoints") + return nil, w.Wrapf(err, "cannot add runtime endpoints") } environments, err := manager.All() if err != nil { - return w.Wrapf(err, "cannot get runtime environment variables") + return nil, w.Wrapf(err, "cannot get runtime environment variables") } - return appendEnvironmentVariablesToFile(ctx, filePath, environments) + return environments, nil } func appendEnvironmentVariablesToFile( ctx context.Context, filePath string, environments []*resources.EnvironmentVariable, +) error { + records, err := outputRecords(environments) + if err != nil { + return err + } + return writeOutputRecords(ctx, filePath, records) +} + +// writeOutputRecords is the one seam every `--output-env` write passes +// through, and therefore the one that enforces record integrity: a phase that +// preflights its complete batch and a caller that appends a single chunk reach +// the same check, so a direct internal caller cannot write a record the +// supported reader would refuse. It is a backstop, not the whole-batch +// guarantee — a caller that has already written a preceding chunk cannot be +// given no-write refusal by a check that runs this late, which is why the +// multi-chunk phases gather and validate everything first. +func writeOutputRecords( + ctx context.Context, + filePath string, + records []outputRecord, ) error { w := wool.Get(ctx).In("resources.AppendToFile", wool.Field("filePath", filePath)) + for index, record := range records { + if err := validOutputKey(record.key); err != nil { + return w.Wrapf(err, "cannot write output environment record %d", index) + } + if err := validOutputValue(record.line[len(record.key)+1:]); err != nil { + return w.Wrapf(err, "cannot write output environment carrier %s", record.key) + } + } filePath = strings.TrimSpace(filePath) if filePath == "" { return w.NewError("output environment file path is required") @@ -2052,6 +2260,9 @@ func appendEnvironmentVariablesToFile( if err != nil && !os.IsNotExist(err) { return w.Wrapf(err, "cannot read environment variables file") } + if err = validRetainedOutputRecords(string(existing)); err != nil { + return w.Wrap(err) + } var order []string values := map[string]string{} for _, line := range strings.Split(string(existing), "\n") { @@ -2064,11 +2275,13 @@ func appendEnvironmentVariablesToFile( } values[key] = line } - for _, env := range environments { - if _, seen := values[env.Key]; !seen { - order = append(order, env.Key) + for _, record := range records { + if _, seen := values[record.key]; !seen { + order = append(order, record.key) } - values[env.Key] = fmt.Sprintf("%s=%v", env.Key, env.Value) + // The line serialized when the batch was checked, never converted a + // second time here. + values[record.key] = record.line } var content strings.Builder for _, key := range order { diff --git a/pkg/orchestration/runner_accepted_network_mappings_test.go b/pkg/orchestration/runner_accepted_network_mappings_test.go index fcd88460..47fa6671 100644 --- a/pkg/orchestration/runner_accepted_network_mappings_test.go +++ b/pkg/orchestration/runner_accepted_network_mappings_test.go @@ -50,8 +50,13 @@ const ( realAgentUnknown = "unknown" // invents an endpoint that was never proposed realAgentDuplicate = "duplicate" // returns two native views for one endpoint realAgentInputs = "inputs" // echoes back the invocation inputs Init carried + realAgentRefuses = "refuses" // answers Init with ERROR and its own diagnostic ) +// realAgentRefusalMessage is the agent's own Init diagnostic, which the run +// must surface unchanged rather than replacing with a host-side explanation. +const realAgentRefusalMessage = "postmaster refused the data directory" + // exposedConfigurationName marks the runtime configuration the agent exports on // every Init. A rejected Init must not publish it. const exposedConfigurationName = "runtime" @@ -131,6 +136,11 @@ func proposedPorts(proposed []*basev0.NetworkMapping) map[uint16]bool { } func (agent *realAgent) Init(_ context.Context, req *runtimev0.InitRequest) (*runtimev0.InitResponse, error) { + if agent.mode == realAgentRefuses { + return &runtimev0.InitResponse{Status: &runtimev0.InitStatus{ + State: runtimev0.InitStatus_ERROR, Message: realAgentRefusalMessage, + }}, nil + } if image := req.GetRuntimeImage(); image != "" { parsed, err := reference.ParseNamed(image) if err != nil { diff --git a/pkg/orchestration/runner_output_env_test.go b/pkg/orchestration/runner_output_env_test.go index 01ef85d2..e42cc34f 100644 --- a/pkg/orchestration/runner_output_env_test.go +++ b/pkg/orchestration/runner_output_env_test.go @@ -130,17 +130,9 @@ func TestAppendServiceProcessConfigurationsToFileIncludesSecretsAndFiltersDepend ) runtime := configuration("mind/mind", "runtime", "session_id", "session-123", false, native) - // The composition root's own structured document, handed to every service - // of the run as platform-core's merged fixture-composition is: it is - // carried under a key that hashes origin, name AND environment, so a - // writer that names no environment cannot write it at all. - document := &basev0.Configuration{Origin: resources.ConfigurationWorkspace, RuntimeContext: native, Infos: []*basev0.ConfigurationInformation{{ - Name: "fixture-composition", - Data: &basev0.ConfigurationData{Kind: "yaml", Content: []byte("schema: codefly.dev/authority/fixture-composition/v1\nroles: []\n")}, - }}} if err := AppendServiceProcessConfigurationsToFile( context.Background(), path, localRunEnvironment(), native, service, - []*basev0.Configuration{workspace, document}, + []*basev0.Configuration{workspace}, []*basev0.Configuration{postgres, containerOnly}, []*basev0.Configuration{runtime}, ); err != nil { @@ -156,21 +148,11 @@ func TestAppendServiceProcessConfigurationsToFileIncludesSecretsAndFiltersDepend "CODEFLY__WORKSPACE_CONFIGURATION__DOGFOOD__CAMPAIGN=headless\n", "CODEFLY__SERVICE_SECRET_CONFIGURATION__INFRA__POSTGRES__POSTGRES__READ_WRITE_CONNECTION=postgres://runtime\n", "CODEFLY__SERVICE_CONFIGURATION__MIND__MIND__RUNTIME__SESSION_ID=session-123\n", - // The document, under the key its reader derives for this environment. - resources.ConfigurationDocumentKey(resources.ConfigurationWorkspace, "fixture-composition", "local", false) + "=", } { if !strings.Contains(text, expected) { t.Fatalf("service process environment is missing %q; keys=%v", expected, environmentKeys(text)) } } - carrier := carrierValue(t, text, resources.ConfigurationDocumentKey(resources.ConfigurationWorkspace, "fixture-composition", "local", false)) - content, err := resources.DecodeConfigurationDocument(carrier, resources.ConfigurationWorkspace, "fixture-composition", "local", false) - if err != nil { - t.Fatalf("the written document does not read back for environment local: %v", err) - } - if !strings.Contains(string(content), "codefly.dev/authority/fixture-composition/v1") { - t.Fatalf("the document lost its content: %s", content) - } if strings.Contains(text, "REDIS") || strings.Contains(text, "redis://container") { t.Fatalf("container-only dependency leaked into native environment; keys=%v", environmentKeys(text)) } diff --git a/pkg/orchestration/runner_reload_addresses_test.go b/pkg/orchestration/runner_reload_addresses_test.go index 0ff761a6..5964a603 100644 --- a/pkg/orchestration/runner_reload_addresses_test.go +++ b/pkg/orchestration/runner_reload_addresses_test.go @@ -73,11 +73,14 @@ func TestTheOutputEnvironmentFileKeepsOneValuePerKey(t *testing.T) { } require.NoError(t, appendEnvironmentVariablesToFile(ctx, path, environments)) } - write("CODEFLY__ENDPOINT__APP__API__REST__REST", "http://127.0.0.1:55634", "KEEP", "one") - write("CODEFLY__ENDPOINT__APP__API__REST__REST", "http://127.0.0.1:50695", "ADDED", "two") + // Every key is a real carrier spelling: the writer now holds the whole + // batch to the grammar the supported reader parses, so a placeholder like + // "KEEP" is refused rather than written (output_records.go). + write("CODEFLY__ENDPOINT__APP__API__REST__REST", "http://127.0.0.1:55634", "CODEFLY__KEPT", "one") + write("CODEFLY__ENDPOINT__APP__API__REST__REST", "http://127.0.0.1:50695", "CODEFLY__ADDED", "two") content, err := os.ReadFile(path) require.NoError(t, err) - require.Equal(t, "CODEFLY__ENDPOINT__APP__API__REST__REST=http://127.0.0.1:50695\nKEEP=one\nADDED=two\n", string(content), "one line per key, in first-seen order, with the latest value") + require.Equal(t, "CODEFLY__ENDPOINT__APP__API__REST__REST=http://127.0.0.1:50695\nCODEFLY__KEPT=one\nCODEFLY__ADDED=two\n", string(content), "one line per key, in first-seen order, with the latest value") info, err := os.Stat(path) require.NoError(t, err) require.Equal(t, os.FileMode(0o600), info.Mode().Perm())