diff --git a/agents/agents.go b/agents/agents.go index 04211dc18..1172d44ac 100644 --- a/agents/agents.go +++ b/agents/agents.go @@ -442,6 +442,7 @@ func Serve(reg PluginRegistration) { // insecure default. authUnaryInterceptor(expectedToken), principalUnaryInterceptor(), + containerRecoveryInterceptor(), runtimeLoadTracker(&runtimeLoaded), agentRPCInterceptor(), ), diff --git a/agents/container_recovery.go b/agents/container_recovery.go new file mode 100644 index 000000000..20c04096e --- /dev/null +++ b/agents/container_recovery.go @@ -0,0 +1,23 @@ +package agents + +import ( + "context" + + agentv0 "github.com/codefly-dev/core/generated/go/codefly/services/agent/v0" + "github.com/codefly-dev/core/runners/dockerrun" + "google.golang.org/grpc" + "google.golang.org/grpc/metadata" +) + +func containerRecoveryInterceptor() grpc.UnaryServerInterceptor { + return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) { + if info.FullMethod == agentv0.Agent_GetAgentInformation_FullMethodName { + if scope := dockerrun.InheritedContainerRecoveryScope(); scope != "" { + if err := grpc.SetHeader(ctx, metadata.Pairs(dockerrun.ContainerRecoveryScopeHeader, scope)); err != nil { + return nil, err + } + } + } + return handler(ctx, req) + } +} diff --git a/agents/container_recovery_test.go b/agents/container_recovery_test.go new file mode 100644 index 000000000..dffd522df --- /dev/null +++ b/agents/container_recovery_test.go @@ -0,0 +1,60 @@ +package agents + +import ( + "bytes" + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "testing" + "time" + + agentv0 "github.com/codefly-dev/core/generated/go/codefly/services/agent/v0" + "github.com/codefly-dev/core/runners/dockerrun" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/metadata" +) + +func TestAgentAcknowledgesInheritedContainerRecoveryOverGRPC(t *testing.T) { + binary := filepath.Join(t.TempDir(), "recoveryagent") + output, err := exec.Command("go", "build", "-o", binary, "./testdata/recoveryagent").CombinedOutput() + require.NoError(t, err, "%s", output) + t.Setenv(dockerrun.ContainerRecoveryScopeEnvironment, "") + scope, err := dockerrun.NewContainerRecoveryScope(t.TempDir(), t.TempDir(), "test") + require.NoError(t, err) + require.NoError(t, dockerrun.SetContainerRecoveryScope(scope)) + for _, valid := range []bool{true, false} { + t.Run(fmt.Sprint(valid), func(t *testing.T) { + marker := os.Getenv(dockerrun.ContainerRecoveryScopeEnvironment) + if !valid { + marker = "0:invalid" + } + command := exec.Command(binary) + command.Env = append(os.Environ(), "CODEFLY_AGENT_TOKEN=recovery-test", "CODEFLY_AGENT_UDS_PATH=", dockerrun.ContainerRecoveryScopeEnvironment+"="+marker) + stdout, err := command.StdoutPipe() + require.NoError(t, err) + var stderr bytes.Buffer + command.Stderr = &stderr + require.NoError(t, command.Start()) + t.Cleanup(func() { _ = command.Process.Kill(); _ = command.Wait() }) + endpoint := readHandshakeEndpoint(t, stdout, &stderr) + conn, err := grpc.NewClient(endpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + ctx = metadata.AppendToOutgoingContext(ctx, AuthMetadataKey, "recovery-test") + var headers metadata.MD + _, err = agentv0.NewAgentClient(conn).GetAgentInformation(ctx, &agentv0.AgentInformationRequest{}, grpc.Header(&headers)) + require.NoError(t, err) + if valid { + require.Equal(t, []string{dockerrun.InheritedContainerRecoveryScope()}, headers.Get(dockerrun.ContainerRecoveryScopeHeader)) + } else { + require.Empty(t, headers.Get(dockerrun.ContainerRecoveryScopeHeader)) + } + }) + } +} diff --git a/agents/testdata/recoveryagent/main.go b/agents/testdata/recoveryagent/main.go new file mode 100644 index 000000000..aba09d2be --- /dev/null +++ b/agents/testdata/recoveryagent/main.go @@ -0,0 +1,18 @@ +package main + +import ( + "context" + + "github.com/codefly-dev/core/agents" + agentv0 "github.com/codefly-dev/core/generated/go/codefly/services/agent/v0" +) + +type server struct { + agentv0.UnimplementedAgentServer +} + +func (server) GetAgentInformation(context.Context, *agentv0.AgentInformationRequest) (*agentv0.AgentInformation, error) { + return &agentv0.AgentInformation{}, nil +} + +func main() { agents.Serve(agents.PluginRegistration{Agent: server{}}) } diff --git a/docs/session-ledger.md b/docs/session-ledger.md index 344502e45..8348cdd1f 100644 --- a/docs/session-ledger.md +++ b/docs/session-ledger.md @@ -300,14 +300,6 @@ cannot claim a container, even when its creator PID is gone. Containers without this label, or with missing/malformed owner PIDs, require explicit owner recovery; startup does not infer ownership from container names. -Disposable SDK invocations additionally delegate orphan cleanup through a durable -`codefly.recovery-group` label. Its identity includes the same canonical home and -workspace and the caller's naming scope, excluding only the current SDK session's -verified invocation suffix. This delegation is issued only in ephemeral mode with -matching session metadata. A later disposable invocation in that group can remove -dead-owner ephemeral siblings; live owners, stateful siblings and ledgered -containers remain protected. Ordinary naming scopes never gain this delegation. - The CLI sets the scope after resolving its run environment and before spawning agents. `SetContainerRecoveryScope` projects a PID-bound process marker to direct children; Docker environments built against this Core version emit the label. @@ -322,8 +314,68 @@ replacement or shutdown. A scoped caller cannot claim an unscoped container, and an unscoped caller cannot claim a scoped one. A mismatch is an error requiring explicit owner recovery, even when runtime configurations match. Each Docker environment retains its scope from first use so another flow changing the process -marker cannot redirect its cleanup. Recovery groups authorize orphan sweeping -only; they never authorize adopting another invocation's container. +marker cannot redirect its cleanup. Cross-scope recovery authorizes orphan +sweeping only; it never authorizes adopting another invocation's container. + +The agent's `GetAgentInformation` response includes the +`codefly-container-recovery-scope` gRPC header with the validated inherited scope. +The CLI must require this acknowledgement before Docker provisioning. The +startup parent identity is retained after reparenting so a CLI crash cannot make +an already-spawned agent create unlabeled containers or lose its ephemeral intent. + +Containers also carry `codefly.recovery-namespace`, a hash of the stable caller +host identity and canonical home/workspace, without the invocation's naming +scope. The host identity prevents local PID checks from claiming another host's +containers on a shared Docker daemon. On Linux it requires a persistent machine +ID, never a boot ID that would strand containers after reboot, and it includes +the caller's PID namespace so a PID number from one namespace is never read as +liveness evidence in another. + +That PID namespace is a same-namespace proof, not a durable name: the kernel +recycles nsfs inodes, so sequential containers can reuse one and concurrent +containers never share one. Both directions err toward retention, so a mismatch +only ever declines to reap — but cross-scope recovery between concurrent +containerized runs on a shared daemon is consequently unavailable. + +A host that cannot prove a durable identity resolves a scope with no namespace. +Cross-scope recovery is then unavailable and `ReapDisposableContainers` says so, +while the exact-scope sweep continues unchanged; resolution never fails for this +reason, because the common Linux container base images provide no machine ID at +all. A namespace that is present and different is a foreign host and is never +eligible for any sweep or for reuse. + +The namespace lets `ReapDisposableContainers` recover explicitly ephemeral +containers after an SDK/test invocation dies, even when the next invocation has a +fresh scope. It needs no SDK session metadata and survives a successor that +picked an entirely unrelated scope. It checks the original agent PID and +preserves live owners, stateful containers, and session-ledger containers. No +on-disk registry or inference from names is needed; failed removals leave the +labels available for retry. + +Containers created before the namespace label existed carry only the exact +scope. Docker cannot add a label to a container that already exists, so the +exact-scope sweep and reuse both continue to accept a missing namespace label — +refusing it would strand every pre-upgrade container permanently and refuse to +reuse retained databases. Cross-scope authority is withheld from them, since +outside the exact scope the hash proves nothing about home, workspace or host. + +The process marker is tagged with its field layout (`pid:v2:scope:namespace`). A +marker written before the tag existed is honored only for the exact scope, and +one carrying untagged trailing fields is refused: two revisions gave the field +after the scope different meanings, and a reader that guesses wrong stamps a +container with ownership no sweep can match. + +The exact-scope sweep no longer delegates across naming scopes. That authority +belongs solely to `ReapDisposableContainers` and the durable namespace, which +subsumes what a naming-scope prefix could ever authorize: the CLI runs both +sweeps, so a prefix-scoped delegation could only ever remove containers the +namespace sweep had already collected. + +Provisioning checks ownership before reusing, replacing, starting or following +logs from an existing container. It refuses foreign/unverified ownership and +cross-agent adoption of ephemeral containers: Docker labels are immutable, so +adoption would leave the old creator recorded and let recovery delete a live +adopter's container. Stateful reuse within the same verified scope remains valid. Within the same scope, live owners and running stateful containers are retained. Stopped containers and running ephemeral containers with dead owners can be diff --git a/runners/dockerrun/container_ownership_test.go b/runners/dockerrun/container_ownership_test.go index 0c8dbe1d9..dc02c5e57 100644 --- a/runners/dockerrun/container_ownership_test.go +++ b/runners/dockerrun/container_ownership_test.go @@ -81,7 +81,11 @@ func TestContainerOwnershipGuardsReuseReplacementAndShutdown(t *testing.T) { require.Nil(t, second.instance) // Failed initialization must not let teardown rediscover and delete // the very foreign container that initialization refused to claim. - require.ErrorContains(t, second.Shutdown(ctx), "refusing adoption or replacement") + // Teardown is fenced to the generation Init acquired (#462), so a + // refused Init leaves nothing to tear down. What matters is that the + // foreign container survives, which the inspection below asserts; + // whether Shutdown reports anything is not part of that contract. + _ = second.Shutdown(ctx) inspected, err := first.client.ContainerInspect(ctx, originalID) require.NoError(t, err) require.True(t, inspected.State.Running) diff --git a/runners/dockerrun/container_recovery_host.go b/runners/dockerrun/container_recovery_host.go new file mode 100644 index 000000000..35a1126e9 --- /dev/null +++ b/runners/dockerrun/container_recovery_host.go @@ -0,0 +1,51 @@ +package dockerrun + +import ( + "context" + "encoding/hex" + "fmt" + "os" + "runtime" + "strings" + "time" + + "github.com/shirou/gopsutil/v3/host" +) + +func containerRecoveryHostID() (string, error) { + const linuxOS = "linux" + if runtime.GOOS == linuxOS { + // The PID namespace proves "same namespace as the container's recorded + // owner", which is what makes a local liveness check meaningful. It is + // not a durable name: the kernel recycles nsfs inodes, so sequential + // containers can reuse one and concurrent containers never share one. + // Both directions only ever decline to reap, so a stale or distinct + // inode costs recovery, never a wrongly deleted container. + pidNamespace, err := os.Readlink("/proc/self/ns/pid") + if err != nil { + return "", fmt.Errorf("identify local PID namespace: %w", err) + } + // gopsutil's Linux fallback is a boot ID. It changes on reboot and + // would silently strand containers, so require a persistent identity. + // Absence is reported as no identity rather than an error: alpine, + // debian-slim and ubuntu images ship no usable machine ID, and failing + // here would stop every containerized run instead of narrowing recovery + // to the exact scope. ReapDisposableContainers traces the consequence. + for _, path := range []string{"/etc/machine-id", "/var/lib/dbus/machine-id", "/sys/class/dmi/id/product_uuid"} { + data, err := os.ReadFile(path) // #nosec G304 -- fixed operating-system identity paths. + id := strings.ToLower(strings.ReplaceAll(strings.TrimSpace(string(data)), "-", "")) + decoded, decodeErr := hex.DecodeString(id) + if err == nil && decodeErr == nil && len(decoded) == 16 && strings.Trim(id, "0") != "" && strings.Trim(id, "f") != "" { + return id + ":" + pidNamespace, nil + } + } + return "", nil + } + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + id, err := host.HostIDWithContext(ctx) + if err != nil { + return "", nil // Same degrade: no durable identity, not a failed run. + } + return id, nil +} diff --git a/runners/dockerrun/container_recovery_reuse.go b/runners/dockerrun/container_recovery_reuse.go new file mode 100644 index 000000000..d74313791 --- /dev/null +++ b/runners/dockerrun/container_recovery_reuse.go @@ -0,0 +1,35 @@ +package dockerrun + +import ( + "fmt" + + "github.com/docker/docker/api/types/container" +) + +// validateContainerRecoveryReuse judges the reuse and fingerprint-replacement +// paths that name-based lookup cannot. IsContainerPresent has already proven +// Codefly ownership and an identical exact scope before this runs, so repeating +// those comparisons here only produced unreachable branches and a second error +// message for a refusal it had already made. What genuinely remains is the +// host-binding namespace and the immutability of an ephemeral owner: labels +// cannot be rewritten, so adopting an ephemeral container would leave its old +// creator recorded and let recovery remove it while its adopter lives. +func validateContainerRecoveryReuse(existing, desired *container.Config) error { + if existing == nil { + return fmt.Errorf("existing container has no verifiable configuration") + } + // A container created before the namespace label existed carries none, and + // Docker cannot add one after creation. Treating that absence as foreign + // would refuse to reuse every pre-upgrade container — including the stateful + // databases the reuse path exists to preserve across CLI restarts. A label + // that is present and different is a genuinely foreign host or PID namespace. + if existingNamespace := existing.Labels[LabelCodeflyRecoveryNamespace]; existingNamespace != "" && + existingNamespace != desired.Labels[LabelCodeflyRecoveryNamespace] { + return fmt.Errorf("existing container belongs to another host or PID namespace") + } + if (existing.Labels[LabelCodeflyEphemeral] == labelTrue || desired.Labels[LabelCodeflyEphemeral] == labelTrue) && + existing.Labels[LabelCodeflySession] != desired.Labels[LabelCodeflySession] { + return fmt.Errorf("ephemeral container belongs to another agent; its ownership cannot be transferred") + } + return nil +} diff --git a/runners/dockerrun/container_recovery_reuse_test.go b/runners/dockerrun/container_recovery_reuse_test.go new file mode 100644 index 000000000..59676740b --- /dev/null +++ b/runners/dockerrun/container_recovery_reuse_test.go @@ -0,0 +1,100 @@ +//go:build !skip_infra + +package dockerrun + +import ( + "context" + "os" + "os/exec" + "strconv" + "testing" + + "github.com/codefly-dev/core/resources" + "github.com/docker/docker/api/types/container" + "github.com/stretchr/testify/require" +) + +func TestContainerRecoveryRejectsAdoptionBeforeReuseOrReplacement(t *testing.T) { + backend := newBackend(t) + t.Setenv(ContainerRecoveryScopeEnvironment, "") + scope, err := NewContainerRecoveryScope(t.TempDir(), t.TempDir(), "reuse") + require.NoError(t, err) + require.NoError(t, SetContainerRecoveryScope(scope)) + owner := exec.Command("true") + require.NoError(t, owner.Run()) + for _, tc := range []struct { + name string + ephemeral, sameOwner, foreign, legacy, changed, unscoped bool + // reject names the refusal expected, or is empty when reuse is allowed. + // An unscoped caller is turned away by name-based lookup before reuse + // validation is ever reached, so it reports that earlier refusal. + reject string + }{ + {name: "ephemeral cannot transfer", ephemeral: true, reject: "recover its owner explicitly"}, + {name: "own ephemeral can restart", ephemeral: true, sameOwner: true}, + {name: "stateful can survive agent restart"}, + {name: "foreign cannot reuse", foreign: true, reject: "recover its owner explicitly"}, + {name: "foreign cannot replace", foreign: true, changed: true, reject: "recover its owner explicitly"}, + // Docker cannot add a namespace label to a container that already exists, + // so a pre-upgrade container must stay reusable — refusing it would strand + // the retained databases this path exists to preserve across restarts. + {name: "pre-upgrade container still reusable", legacy: true}, + {name: "unscoped cannot adopt", unscoped: true, reject: "refusing adoption or replacement"}, + } { + t.Run(tc.name, func(t *testing.T) { + name := uniqueName(t) + newEnvironment := func() *DockerEnvironment { + t.Helper() + env, err := NewDockerHeadlessEnvironment(t.Context(), resources.NewDockerImage("alpine:latest"), name) + require.NoError(t, err) + t.Cleanup(func() { _ = env.client.Close() }) + env.WithPause() + if tc.ephemeral { + env.WithEphemeral() + } + return env + } + env := newEnvironment() + config, hostConfig, err := env.desiredContainerConfigs(t.Context()) + require.NoError(t, err) + config.Labels[LabelCodeflySession] = strconv.Itoa(owner.Process.Pid) + if tc.sameOwner { + config.Labels[LabelCodeflySession] = strconv.Itoa(os.Getpid()) + } + if tc.foreign { + config.Labels[LabelCodeflyRecoveryNamespace] = "foreign" + } + if tc.legacy { + delete(config.Labels, LabelCodeflyRecoveryNamespace) + } + if tc.changed { + config.Labels[LabelCodeflyConfig] = "old-configuration" + } + created, err := backend.client.ContainerCreate(t.Context(), config, hostConfig, nil, nil, env.name) + require.NoError(t, err) + t.Cleanup(func() { + _ = backend.client.ContainerRemove(context.Background(), created.ID, container.RemoveOptions{Force: true}) + }) + reuser := env + if tc.unscoped { + // Recovery ownership is fixed on an environment's first use, so + // an unscoped caller is a fresh environment that never inherited + // a marker — not this one with the ambient marker pulled out + // from under its already-resolved identity. + t.Setenv(ContainerRecoveryScopeEnvironment, "") + reuser = newEnvironment() + } + err = reuser.GetContainer(t.Context()) + if tc.reject != "" { + require.ErrorContains(t, err, tc.reject) + } else { + require.NoError(t, err) + } + inspected, err := backend.client.ContainerInspect(t.Context(), created.ID) + require.NoError(t, err, "ownership rejection must not delete or replace the old container") + if tc.reject != "" { + require.False(t, inspected.State.Running, "ownership rejection must precede starting the old container") + } + }) + } +} diff --git a/runners/dockerrun/container_recovery_scope.go b/runners/dockerrun/container_recovery_scope.go index 69c4d6b5d..8610742ce 100644 --- a/runners/dockerrun/container_recovery_scope.go +++ b/runners/dockerrun/container_recovery_scope.go @@ -10,23 +10,57 @@ import ( "path/filepath" "strconv" "strings" - - "github.com/codefly-dev/core/sdk/session" ) const LabelCodeflyRecoveryScope = "codefly.recovery-scope" -const LabelCodeflyRecoveryGroup = "codefly.recovery-group" +const LabelCodeflyRecoveryNamespace = "codefly.recovery-namespace" const ContainerRecoveryScopeEnvironment = "CODEFLY_CONTAINER_RECOVERY_SCOPE" +// containerRecoveryMarkerVersion tags the marker's field layout. Two Core +// revisions gave the field after the exact scope different meanings — a +// recovery group in one, the durable namespace in the other — and a reader that +// guesses wrong stamps a container with ownership no sweep can ever match, +// leaking exactly what this recovery exists to collect. An untagged marker is +// therefore honored only for the one field every revision agreed on. +const containerRecoveryMarkerVersion = "v2" + +// ContainerRecoveryScopeHeader acknowledges the ownership identity inherited +// by an agent. Older agents omit it and cannot promise scoped recovery. +const ContainerRecoveryScopeHeader = "codefly-container-recovery-scope" + // Capture the launching parent before serving any requests. A parent dying // during a request must not revoke the child's already inherited ownership. // If it died before initialization, validation fails and creation is refused. var containerRecoveryParentPID = os.Getppid() -// ContainerRecoveryScope binds cleanup to a home, workspace and resolved naming scope. -type ContainerRecoveryScope struct{ id, group string } +// InheritedContainerRecoveryScope is the validated identity used by container +// creation and by the agent's read-only gRPC ownership acknowledgement. It +// echoes the complete inherited identity, including an empty namespace when the +// CLI's host has no durable identity: the caller compares it against what it +// projected, so conflating "agent did not understand" with "cross-scope recovery +// is unavailable here" would report a compatible agent as incompatible. +func InheritedContainerRecoveryScope() string { + scope, err := inheritedContainerRecoveryScope() + if err != nil || scope.id == "" { + return "" + } + return scope.id + ":" + scope.namespace +} + +// ContainerRecoveryScope binds cleanup to a home, workspace and resolved naming +// scope; namespace is the durable canonical host/home/workspace identity that +// survives a run choosing an entirely fresh naming scope. +type ContainerRecoveryScope struct{ id, namespace string } func NewContainerRecoveryScope(home, workspace, namingScope string) (ContainerRecoveryScope, error) { + hostID, err := containerRecoveryHostID() + if err != nil { + return ContainerRecoveryScope{}, fmt.Errorf("resolve container recovery host: %w", err) + } + return newContainerRecoveryScope(home, workspace, namingScope, hostID) +} + +func newContainerRecoveryScope(home, workspace, namingScope, hostID string) (ContainerRecoveryScope, error) { paths := []string{home, workspace} for i, path := range paths { if path == "" { @@ -42,22 +76,13 @@ func NewContainerRecoveryScope(home, workspace, namingScope string) (ContainerRe } } scope := ContainerRecoveryScope{id: recoveryScopeHash(paths, namingScope)} - // Only an SDK invocation in disposable mode can delegate recovery across - // invocation names. Never infer this from a name suffix alone: a regular - // developer scope may happen to have the same spelling. - if EphemeralContainers() { - invocation := session.FromEnvironment(os.Environ()) - if invocation != nil { - identity, err := hex.DecodeString(invocation.ID) - if err == nil && len(identity) == 16 { - suffix := invocation.Scope() - if namingScope == suffix { - scope.group = recoveryScopeHash(paths, "") - } else if prefix, ok := strings.CutSuffix(namingScope, "-"+suffix); ok { - scope.group = recoveryScopeHash(paths, prefix) - } - } - } + // A host that cannot prove a durable identity gets no namespace, so + // cross-scope recovery is unavailable while the exact-scope sweep keeps + // working. Refusing to resolve a scope at all would stop every run on a + // Linux host without a provisioned machine ID — which is every common + // container base image. + if hostID != "" { + scope.namespace = recoveryNamespaceHash(hostID, paths) } return scope, nil } @@ -68,16 +93,26 @@ func recoveryScopeHash(paths []string, namingScope string) string { return hex.EncodeToString(sum[:]) } +// recoveryNamespaceHash covers every naming scope under one canonical +// home/workspace pair, so an aborted run's disposable containers stay +// recoverable by a successor that picked a completely different scope. The host +// identity is part of it: identical paths on a shared Docker daemon do not +// imply the same owner host, and a foreign host's PIDs are not ours to judge. +func recoveryNamespaceHash(hostID string, paths []string) string { + data, _ := json.Marshal(append([]string{hostID}, paths...)) + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]) +} + // SetContainerRecoveryScope projects ownership to directly spawned agents. // Older agents omit the label and are deliberately ineligible for startup cleanup. func SetContainerRecoveryScope(scope ContainerRecoveryScope) error { if scope.id == "" { return fmt.Errorf("container recovery scope is unresolved") } - marker := strconv.Itoa(os.Getpid()) + ":" + scope.id - if scope.group != "" { - marker += ":" + scope.group - } + // pid:v2:scope:namespace. The namespace is empty on a host with no durable + // identity. + marker := strings.Join([]string{strconv.Itoa(os.Getpid()), containerRecoveryMarkerVersion, scope.id, scope.namespace}, ":") return os.Setenv(ContainerRecoveryScopeEnvironment, marker) } @@ -94,16 +129,42 @@ func inheritedContainerRecoveryScope() (ContainerRecoveryScope, error) { if err != nil || pid <= 1 || (pid != os.Getpid() && pid != containerRecoveryParentPID) { return ContainerRecoveryScope{}, fmt.Errorf("container recovery marker does not belong to this process or its launching parent") } - id, group, grouped := strings.Cut(identity, ":") - decoded, err := hex.DecodeString(id) - if err != nil || len(decoded) != sha256.Size { + fields := strings.Split(identity, ":") + if fields[0] != containerRecoveryMarkerVersion { + // A marker written before the layout was tagged. Only the exact scope + // had a single agreed meaning across those revisions, so refuse anything + // trailing it rather than guess whether it is a group or a namespace. + if len(fields) != 1 { + return ContainerRecoveryScope{}, fmt.Errorf("untagged container recovery marker carries ambiguous fields") + } + if err := validRecoveryDigest(fields[0]); err != nil { + return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery identity") + } + return ContainerRecoveryScope{id: fields[0]}, nil + } + digests := fields[1:] + if len(digests) != 2 { + return ContainerRecoveryScope{}, fmt.Errorf("container recovery marker has %d fields, want scope and namespace", len(digests)) + } + if err := validRecoveryDigest(digests[0]); err != nil { return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery identity") } - if grouped { - decoded, err = hex.DecodeString(group) - if err != nil || len(decoded) != sha256.Size { - return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery group") + scope := ContainerRecoveryScope{id: digests[0]} + // The namespace is deliberately optional — a host with no durable identity + // projects the exact scope alone — but must be a digest when present. + if digests[1] != "" { + if err := validRecoveryDigest(digests[1]); err != nil { + return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery namespace") } + scope.namespace = digests[1] + } + return scope, nil +} + +func validRecoveryDigest(digest string) error { + decoded, err := hex.DecodeString(digest) + if err != nil || len(decoded) != sha256.Size { + return fmt.Errorf("not a sha256 digest") } - return ContainerRecoveryScope{id: id, group: group}, nil + return nil } diff --git a/runners/dockerrun/container_recovery_scope_test.go b/runners/dockerrun/container_recovery_scope_test.go index 437e9e368..40c975884 100644 --- a/runners/dockerrun/container_recovery_scope_test.go +++ b/runners/dockerrun/container_recovery_scope_test.go @@ -11,65 +11,25 @@ import ( "time" "github.com/codefly-dev/core/resources" - "github.com/codefly-dev/core/sdk/session" "github.com/docker/docker/api/types/container" "github.com/stretchr/testify/require" ) -func disposableRecoveryScope(t *testing.T, home, workspace, label, identity string) ContainerRecoveryScope { - t.Helper() - t.Setenv(EphemeralContainersEnvironment, strconv.Itoa(os.Getppid())) - t.Setenv(session.IDEnvironment, identity) - t.Setenv(session.SecretEnvironment, "test-secret") - invocation := &session.Session{ID: identity} - name := invocation.Scope() - if label != "" { - name = label + "-" + name - } - scope, err := NewContainerRecoveryScope(home, workspace, name) - require.NoError(t, err) - require.NotEmpty(t, scope.group) - return scope -} - -func TestDisposableRecoveryGroupRequiresInvocationAndMode(t *testing.T) { - home, workspace := t.TempDir(), t.TempDir() - first := disposableRecoveryScope(t, home, workspace, "tests", strings.Repeat("a", 32)) - second := disposableRecoveryScope(t, home, workspace, "tests", strings.Repeat("b", 32)) - require.NotEqual(t, first.id, second.id) - require.Equal(t, first.group, second.group) - otherLabel := disposableRecoveryScope(t, home, workspace, "other", strings.Repeat("c", 32)) - require.NotEqual(t, first.group, otherLabel.group) - otherHome := disposableRecoveryScope(t, t.TempDir(), workspace, "tests", strings.Repeat("d", 32)) - require.NotEqual(t, first.group, otherHome.group) - otherWorkspace := disposableRecoveryScope(t, home, t.TempDir(), "tests", strings.Repeat("e", 32)) - require.NotEqual(t, first.group, otherWorkspace.group) - for _, tc := range []struct{ name, identity, marker, scope string }{ - {"reusable", strings.Repeat("a", 32), "", "tests-saaaaaaaaaaaa"}, - {"not an invocation", "", strconv.Itoa(os.Getppid()), "tests-saaaaaaaaaaaa"}, - {"malformed invocation", "a", strconv.Itoa(os.Getppid()), "tests-sa"}, - {"different invocation", strings.Repeat("b", 32), strconv.Itoa(os.Getppid()), "tests-saaaaaaaaaaaa"}, - } { - t.Run(tc.name, func(t *testing.T) { - t.Setenv(session.IDEnvironment, tc.identity) - t.Setenv(EphemeralContainersEnvironment, tc.marker) - scope, err := NewContainerRecoveryScope(home, workspace, tc.scope) - require.NoError(t, err) - require.Empty(t, scope.group) - }) - } -} - func TestContainerRecoveryScopeIsolation(t *testing.T) { home, workspace := t.TempDir(), t.TempDir() scope, err := NewContainerRecoveryScope(home, workspace, "candidate") require.NoError(t, err) + otherHost, err := newContainerRecoveryScope(home, workspace, "candidate", "different-host") + require.NoError(t, err) otherHome, err := NewContainerRecoveryScope(t.TempDir(), workspace, "candidate") require.NoError(t, err) otherScope, err := NewContainerRecoveryScope(home, workspace, "foreign") require.NoError(t, err) otherWorkspace, err := NewContainerRecoveryScope(home, t.TempDir(), "candidate") require.NoError(t, err) + // Pre-upgrade containers carry the exact scope and no namespace at all. + preUpgrade := ContainerRecoveryScope{id: scope.id} + preUpgradeForeignScope := ContainerRecoveryScope{id: otherScope.id} child := exec.Command("true") require.NoError(t, child.Run()) deadPID := strconv.Itoa(child.Process.Pid) @@ -85,17 +45,24 @@ func TestContainerRecoveryScopeIsolation(t *testing.T) { {name: "same scope stateful reuse", scope: scope, pid: deadPID, state: "running"}, {name: "same scope live owner", scope: scope, pid: strconv.Itoa(os.Getpid()), state: "exited", ephemeral: true}, {name: "same scope ledger", scope: scope, pid: deadPID, state: "exited", ledgered: true}, + {name: "foreign host stopped", scope: otherHost, pid: deadPID, state: "exited", ephemeral: true}, {name: "foreign home stopped", scope: otherHome, pid: deadPID, state: "exited"}, {name: "foreign scope ephemeral", scope: otherScope, pid: deadPID, state: "running", ephemeral: true}, {name: "foreign workspace stopped", scope: otherWorkspace, pid: deadPID, state: "exited"}, {name: "legacy no scope", pid: deadPID, state: "exited", ephemeral: true}, + // Docker cannot add a label to an existing container, so refusing these + // would strand every container created before the namespace shipped. + {name: "pre-upgrade same scope stopped", scope: preUpgrade, pid: deadPID, state: "exited", want: true}, + {name: "pre-upgrade same scope ephemeral", scope: preUpgrade, pid: deadPID, state: "running", ephemeral: true, want: true}, + {name: "pre-upgrade cross scope stays", scope: preUpgradeForeignScope, pid: deadPID, state: "running", ephemeral: true}, {name: "missing PID", scope: scope, state: "exited"}, {name: "malformed PID", scope: scope, pid: "invalid", state: "exited"}, {name: "zero PID", scope: scope, pid: "0", state: "exited"}, + {name: "init PID", scope: scope, pid: "1", state: "exited", ephemeral: true}, {name: "negative PID", scope: scope, pid: "-1", state: "exited"}, } { t.Run(tc.name, func(t *testing.T) { - labels := map[string]string{LabelCodeflyOwner: "true", LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflySession: tc.pid} + labels := map[string]string{LabelCodeflyOwner: "true", LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryNamespace: tc.scope.namespace, LabelCodeflySession: tc.pid} if tc.ephemeral { labels[LabelCodeflyEphemeral] = "true" } @@ -109,6 +76,16 @@ func TestContainerRecoveryScopeIsolation(t *testing.T) { require.False(t, staleContainerInScope(c, scope)) }) } + t.Run("host without durable identity", func(t *testing.T) { + degraded := ContainerRecoveryScope{id: scope.id} + require.False(t, staleContainerInScope(container.Summary{State: "exited", Labels: map[string]string{ + LabelCodeflyOwner: "true", LabelCodeflyRecoveryScope: scope.id, + LabelCodeflyRecoveryNamespace: scope.namespace, LabelCodeflySession: deadPID, + }}, degraded), "a caller that cannot prove a host identity must not reap a container bound to one") + require.True(t, staleContainerInScope(container.Summary{State: "exited", Labels: map[string]string{ + LabelCodeflyOwner: "true", LabelCodeflyRecoveryScope: scope.id, LabelCodeflySession: deadPID, + }}, degraded), "exact-scope recovery must survive the loss of a durable host identity") + }) } func TestContainerRecoveryScopeCanonicalPaths(t *testing.T) { @@ -125,6 +102,14 @@ func TestContainerRecoveryScopeCanonicalPaths(t *testing.T) { require.Equal(t, first, second) _, err = NewContainerRecoveryScope("", workspace, "") require.Error(t, err) + // Every common Linux container base image supplies no machine ID, so an + // unidentifiable host must narrow recovery rather than fail to resolve. + degraded, err := newContainerRecoveryScope(home, workspace, "", "") + require.NoError(t, err) + require.Equal(t, first.id, degraded.id) + require.Empty(t, degraded.namespace) + require.NoError(t, SetContainerRecoveryScope(degraded)) + require.NoError(t, ReapStaleContainers(t.Context(), degraded)) _, err = NewContainerRecoveryScope(filepath.Join(root, "missing"), workspace, "") require.Error(t, err) require.Error(t, SetContainerRecoveryScope(ContainerRecoveryScope{})) @@ -144,22 +129,38 @@ func TestContainerRecoveryScopeAgentProcess(t *testing.T) { t.Setenv(ContainerRecoveryScopeEnvironment, "") scope, err := NewContainerRecoveryScope(t.TempDir(), t.TempDir(), "agent") require.NoError(t, err) - scope.group = strings.Repeat("c", 64) require.NoError(t, SetContainerRecoveryScope(scope)) env := &DockerEnvironment{name: "test", image: &resources.DockerImage{Name: "alpine", Tag: "latest"}} config, _, err := env.desiredContainerConfigs(t.Context()) require.NoError(t, err) require.Equal(t, scope.id, config.Labels[LabelCodeflyRecoveryScope]) + require.Equal(t, scope.namespace, config.Labels[LabelCodeflyRecoveryNamespace]) + pid := strconv.Itoa(os.Getpid()) + tagged := func(digests ...string) string { + return pid + ":" + containerRecoveryMarkerVersion + ":" + strings.Join(digests, ":") + } for _, tc := range []struct { - name, marker, want string - wantError bool + name, marker, want, namespace string + wantError bool }{ - {"direct child", os.Getenv(ContainerRecoveryScopeEnvironment), scope.id, false}, - {"legacy", "", "", false}, - {"stale parent", "999999999:" + scope.id, "", true}, - {"malformed identity", strconv.Itoa(os.Getpid()) + ":bad", "", true}, - {"malformed group", strconv.Itoa(os.Getpid()) + ":" + scope.id + ":bad", "", true}, - {"malformed marker", "bad", "", true}, + {"direct child", os.Getenv(ContainerRecoveryScopeEnvironment), scope.id, scope.namespace, false}, + {"legacy", "", "", "", false}, + {"stale parent", "999999999:" + containerRecoveryMarkerVersion + ":" + scope.id + ":" + scope.namespace, "", "", true}, + {"malformed identity", tagged("bad", scope.namespace), "", "", true}, + // A revision older than the layout tag projects the exact scope alone. It + // stays labeled, but never delegates cross-scope recovery. + {"untagged exact scope", pid + ":" + scope.id, scope.id, "", false}, + // A revision older than the tag also wrote pid:scope:group here. Reading + // that group as a namespace stamped the container with ownership no sweep + // could ever match, leaking it permanently — refuse instead of guessing. + {"untagged trailing field", pid + ":" + scope.id + ":" + strings.Repeat("d", 64), "", "", true}, + {"scope and namespace", tagged(scope.id, scope.namespace), scope.id, scope.namespace, false}, + // A host with no durable identity projects an empty namespace field. + {"scope without namespace", tagged(scope.id, ""), scope.id, "", false}, + {"missing namespace field", tagged(scope.id), "", "", true}, + {"malformed namespace", tagged(scope.id, "bad"), "", "", true}, + {"trailing field", tagged(scope.id, scope.namespace, strings.Repeat("e", 64)), "", "", true}, + {"malformed marker", "bad", "", "", true}, } { t.Run(tc.name, func(t *testing.T) { t.Setenv(ContainerRecoveryScopeEnvironment, tc.marker) @@ -175,11 +176,21 @@ func TestContainerRecoveryScopeAgentProcess(t *testing.T) { labels := map[string]string{} require.NoError(t, json.Unmarshal(output, &labels)) require.Equal(t, tc.want, labels[LabelCodeflyRecoveryScope]) - if tc.want != "" { - require.Equal(t, scope.group, labels[LabelCodeflyRecoveryGroup]) - } + require.Equal(t, tc.namespace, labels[LabelCodeflyRecoveryNamespace]) }) } + t.Run("zero owner cannot claim namespace init", func(t *testing.T) { + t.Setenv(ContainerRecoveryScopeEnvironment, tagged(scope.id, scope.namespace)) + require.NotEmpty(t, InheritedContainerRecoveryScope()) + t.Setenv(ContainerRecoveryScopeEnvironment, "0:"+containerRecoveryMarkerVersion+":"+scope.id+":"+scope.namespace) + require.Empty(t, InheritedContainerRecoveryScope()) + }) + t.Run("acknowledgement echoes an empty namespace", func(t *testing.T) { + // A compatible agent on a host with no durable identity must not look + // like an agent that failed to understand the marker at all. + t.Setenv(ContainerRecoveryScopeEnvironment, tagged(scope.id, "")) + require.Equal(t, scope.id+":", InheritedContainerRecoveryScope()) + }) } func TestContainerRecoveryScopeSurvivesParentExit(t *testing.T) { @@ -209,7 +220,6 @@ func TestContainerRecoveryScopeSurvivesParentExit(t *testing.T) { t.Setenv(ContainerRecoveryScopeEnvironment, "") scope, err := NewContainerRecoveryScope(t.TempDir(), t.TempDir(), "crash") require.NoError(t, err) - scope.group = strings.Repeat("a", 64) require.NoError(t, SetContainerRecoveryScope(scope)) cmd := exec.CommandContext(t.Context(), os.Args[0], "-test.run=^TestContainerRecoveryScopeSurvivesParentExit$") cmd.Env = append(os.Environ(), "RECOVERY_PARENT_EXIT_ROLE=parent", "RECOVERY_PARENT_EXIT_READY="+filepath.Join(t.TempDir(), "ready")) @@ -218,6 +228,6 @@ func TestContainerRecoveryScopeSurvivesParentExit(t *testing.T) { labels := map[string]string{} require.NoError(t, json.Unmarshal(output, &labels)) require.Equal(t, scope.id, labels[LabelCodeflyRecoveryScope]) - require.Equal(t, scope.group, labels[LabelCodeflyRecoveryGroup]) + require.Equal(t, scope.namespace, labels[LabelCodeflyRecoveryNamespace]) require.Equal(t, labelTrue, labels[LabelCodeflyEphemeral]) } diff --git a/runners/dockerrun/container_sweep.go b/runners/dockerrun/container_sweep.go index 53c0f44a7..93927ff2b 100644 --- a/runners/dockerrun/container_sweep.go +++ b/runners/dockerrun/container_sweep.go @@ -49,8 +49,9 @@ func shouldReapContainer(state string, ownerAlive, ephemeral, ledgered bool) boo return true } -// ReapStaleContainers recovers this exact scope and dead disposable siblings -// explicitly labeled for the same SDK recovery group. +// ReapStaleContainers recovers containers labeled for this exact scope. Recovery +// across scopes belongs to ReapDisposableContainers, which is authorized by the +// durable namespace label rather than by a naming-scope prefix. // Legacy containers without scope labels require explicit owner recovery. func ReapStaleContainers(ctx context.Context, scope ContainerRecoveryScope) error { if scope.id == "" { @@ -89,25 +90,6 @@ func ReapStaleContainers(ctx context.Context, scope ContainerRecoveryScope) erro if err != nil { return fmt.Errorf("cannot list codefly containers: %w", err) } - if scope.group != "" { - siblings, listErr := cli.ContainerList(listCtx, container.ListOptions{ - All: true, - Filters: filters.NewArgs( - filters.Arg("label", LabelCodeflyOwner+"="+labelTrue), - filters.Arg("label", LabelCodeflyRecoveryGroup+"="+scope.group), - filters.Arg("label", LabelCodeflyEphemeral+"="+labelTrue), - ), - }) - if listErr != nil { - return fmt.Errorf("cannot list disposable sibling containers: %w", listErr) - } - for _, sibling := range siblings { - if sibling.Labels[LabelCodeflyRecoveryScope] != scope.id { - containers = append(containers, sibling) - } - } - } - reaped := 0 for _, c := range containers { if !staleContainerInScope(c, scope) { @@ -138,15 +120,22 @@ func ReapStaleContainers(ctx context.Context, scope ContainerRecoveryScope) erro } func staleContainerInScope(c container.Summary, scope ContainerRecoveryScope) bool { - if scope.id == "" || c.Labels[LabelCodeflyOwner] != labelTrue || c.Labels[LabelCodeflyRecoveryScope] == "" { + if scope.id == "" || c.Labels[LabelCodeflyOwner] != labelTrue || c.Labels[LabelCodeflyRecoveryScope] != scope.id { return false } - if c.Labels[LabelCodeflyRecoveryScope] != scope.id && - (scope.group == "" || c.Labels[LabelCodeflyRecoveryGroup] != scope.group || c.Labels[LabelCodeflyEphemeral] != labelTrue) { + // A namespace label naming another host's home/workspace is never ours, and + // neither is one we cannot verify because this host has no durable identity. + // A container created before the label existed carries none at all: Docker + // cannot add a label to a container that already exists, so refusing those + // would strand every pre-upgrade container forever. For them the exact scope + // hash — home, workspace and naming scope — remains the authority it was. + if namespace := c.Labels[LabelCodeflyRecoveryNamespace]; namespace != scope.namespace && namespace != "" { return false } pid, err := strconv.Atoi(c.Labels[LabelCodeflySession]) - if err != nil || pid <= 0 { + // IsProcessAlive intentionally rejects system PIDs. That is not evidence + // that PID 1 is dead: an agent/library caller may be its namespace's init. + if err != nil || pid <= 1 { return false } return shouldReapContainer(c.State, base.IsProcessAlive(pid), c.Labels[LabelCodeflyEphemeral] == labelTrue, c.Labels[LabelCodeflyInvocation] != "") diff --git a/runners/dockerrun/container_sweep_decision_test.go b/runners/dockerrun/container_sweep_decision_test.go index 18e1f1f2e..2bc47acfa 100644 --- a/runners/dockerrun/container_sweep_decision_test.go +++ b/runners/dockerrun/container_sweep_decision_test.go @@ -62,8 +62,8 @@ func TestEphemeralContainersFlag(t *testing.T) { // in-process flag off, a marker equal to our parent's PID still counts. ephemeralContainers.Store(false) _ = os.Setenv(EphemeralContainersEnvironment, strconv.Itoa(os.Getppid())) - if !EphemeralContainers() { - t.Fatal("agent process did not inherit the ephemeral environment marker from its live parent") + if EphemeralContainers() != (os.Getppid() > 0) { + t.Fatal("only a real parent may supply an inherited ephemeral marker") } // A stale marker naming some unrelated process is deliberately ignored, so diff --git a/runners/dockerrun/disposable_recovery.go b/runners/dockerrun/disposable_recovery.go new file mode 100644 index 000000000..3a45a5516 --- /dev/null +++ b/runners/dockerrun/disposable_recovery.go @@ -0,0 +1,92 @@ +package dockerrun + +import ( + "context" + "errors" + "fmt" + "strconv" + "time" + + "github.com/containerd/errdefs" + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/filters" + + "github.com/codefly-dev/core/runners/base" + "github.com/codefly-dev/core/wool" +) + +// ReapDisposableContainers recovers ephemeral containers across invocation +// scopes, using their durable canonical home/workspace namespace label. It +// never broadens recovery of stateful, legacy or session-ledger containers. +func ReapDisposableContainers(ctx context.Context, scope ContainerRecoveryScope) error { + if scope.id == "" { + return errors.New("container recovery scope is unresolved") + } + w := wool.Get(ctx).In("base.ReapDisposableContainers") + if scope.namespace == "" { + // The host could not prove a durable identity, so nothing here can be + // attributed across scopes. Say so: the leak this sweep exists to collect + // silently resumes otherwise. + w.Warn("cross-scope disposable recovery unavailable without a durable host identity; only exact-scope recovery will run") + return nil + } + cli, _, err := newDockerClient() + if err != nil { + // Docker not reachable — nothing to reap, not an error. Trace it anyway: + // a misconfigured daemon otherwise reads as "swept, nothing found". + w.Trace("docker client unavailable — skipping disposable recovery", wool.ErrField(err)) + return ctx.Err() + } + defer func() { _ = cli.Close() }() + if err = pingDockerClient(ctx, cli); err != nil { + // Docker retains the ownership labels for the next run. + w.Trace("docker engine unavailable — skipping disposable recovery", wool.ErrField(err)) + return ctx.Err() + } + listCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + containers, err := cli.ContainerList(listCtx, container.ListOptions{All: true, Filters: filters.NewArgs( + filters.Arg("label", LabelCodeflyOwner+"="+labelTrue), + filters.Arg("label", LabelCodeflyRecoveryNamespace+"="+scope.namespace), + filters.Arg("label", LabelCodeflyEphemeral+"="+labelTrue), + )}) + cancel() + if err != nil { + return fmt.Errorf("list disposable containers: %w", err) + } + var failures []error + for _, c := range containers { + if !disposableContainerInNamespace(c, scope) { + continue + } + removeCtx, removeCancel := context.WithTimeout(ctx, 10*time.Second) + err := cli.ContainerRemove(removeCtx, c.ID, container.RemoveOptions{Force: true}) + removeCancel() + if err != nil && !errdefs.IsNotFound(err) { + failures = append(failures, fmt.Errorf("recover disposable container %s: %w", c.ID, err)) + } + } + return errors.Join(append(failures, ctx.Err())...) +} + +// disposableContainerInNamespace states the cross-scope rules directly rather +// than borrowing staleContainerInScope with a scope built from the container's +// own labels, which made that predicate's exact-scope comparison tautological +// and hid which checks were really gating removal. +func disposableContainerInNamespace(c container.Summary, scope ContainerRecoveryScope) bool { + if scope.namespace == "" || + c.Labels[LabelCodeflyOwner] != labelTrue || + c.Labels[LabelCodeflyRecoveryScope] == "" || + c.Labels[LabelCodeflyRecoveryNamespace] != scope.namespace || + c.Labels[LabelCodeflyEphemeral] != labelTrue { + return false + } + pid, err := strconv.Atoi(c.Labels[LabelCodeflySession]) + // IsProcessAlive intentionally rejects system PIDs. That is not evidence + // that PID 1 is dead: an agent/library caller may be its namespace's init. + if err != nil || pid <= 1 { + return false + } + // Ephemeral is established above; a ledgered container stays with + // sessionledger recovery, which can read its retention record. + return shouldReapContainer(c.State, base.IsProcessAlive(pid), true, c.Labels[LabelCodeflyInvocation] != "") +} diff --git a/runners/dockerrun/disposable_recovery_integration_test.go b/runners/dockerrun/disposable_recovery_integration_test.go new file mode 100644 index 000000000..6c1bbe245 --- /dev/null +++ b/runners/dockerrun/disposable_recovery_integration_test.go @@ -0,0 +1,92 @@ +//go:build !skip_infra + +package dockerrun + +import ( + "context" + "os" + "os/exec" + "strconv" + "testing" + + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/mount" + "github.com/docker/docker/api/types/volume" + "github.com/docker/docker/errdefs" + "github.com/stretchr/testify/require" +) + +func TestDisposableRecoveryAcrossScopesPreservesOtherOwnersAndVolumes(t *testing.T) { + backend := newBackend(t) + home, workspace := t.TempDir(), t.TempDir() + old, err := NewContainerRecoveryScope(home, workspace, "old-disposable") + require.NoError(t, err) + next, err := NewContainerRecoveryScope(home, workspace, "new-disposable") + require.NoError(t, err) + foreignHost, err := newContainerRecoveryScope(home, workspace, "old-disposable", "different-host") + require.NoError(t, err) + foreignHome, err := NewContainerRecoveryScope(t.TempDir(), workspace, "old-disposable") + require.NoError(t, err) + foreignWorkspace, err := NewContainerRecoveryScope(home, t.TempDir(), "old-disposable") + require.NoError(t, err) + unregistered, err := NewContainerRecoveryScope(home, workspace, "unregistered") + require.NoError(t, err) + process := exec.Command("true") + require.NoError(t, process.Run()) + deadPID := strconv.Itoa(process.Process.Pid) + vol, err := backend.client.VolumeCreate(t.Context(), volume.CreateOptions{Name: uniqueName(t)}) + require.NoError(t, err) + t.Cleanup(func() { _ = backend.client.VolumeRemove(context.Background(), vol.Name, true) }) + for _, tc := range []struct { + name string + scope ContainerRecoveryScope + ephemeral, ledgered, live, running, keep bool + }{ + {name: "orphan", scope: old, ephemeral: true, running: true}, + {name: "stopped orphan", scope: old, ephemeral: true}, + {name: "stateful", scope: old, keep: true}, + {name: "ledger", scope: old, ephemeral: true, ledgered: true, keep: true}, + {name: "live agent", scope: old, ephemeral: true, live: true, keep: true}, + {name: "foreign host", scope: foreignHost, ephemeral: true, keep: true}, + {name: "foreign home", scope: foreignHome, ephemeral: true, keep: true}, + {name: "foreign workspace", scope: foreignWorkspace, ephemeral: true, keep: true}, + {name: "another ephemeral scope", scope: unregistered, ephemeral: true}, + {name: "legacy", ephemeral: true, keep: true}, + } { + t.Run(tc.name, func(t *testing.T) { + labels := map[string]string{LabelCodeflyOwner: labelTrue, LabelCodeflySession: deadPID, LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryNamespace: tc.scope.namespace} + if tc.ephemeral { + labels[LabelCodeflyEphemeral] = labelTrue + } + if tc.ledgered { + labels[LabelCodeflyInvocation] = "retained-session" + } + if tc.live { + labels[LabelCodeflySession] = strconv.Itoa(os.Getpid()) + } + created, err := backend.client.ContainerCreate(t.Context(), &container.Config{Image: "alpine:latest", Cmd: []string{"sleep", "300"}, Labels: labels}, &container.HostConfig{NetworkMode: "none", Mounts: []mount.Mount{{Type: mount.TypeVolume, Source: vol.Name, Target: "/data"}}}, nil, nil, uniqueName(t)) + require.NoError(t, err) + t.Cleanup(func() { + _ = backend.client.ContainerRemove(context.Background(), created.ID, container.RemoveOptions{Force: true}) + }) + if tc.running { + require.NoError(t, backend.client.ContainerStart(t.Context(), created.ID, container.StartOptions{})) + } + // Cancellation must leave ownership and resources available for retry. + cancelled, cancel := context.WithCancel(t.Context()) + cancel() + require.Error(t, ReapDisposableContainers(cancelled, next)) + _, err = backend.client.ContainerInspect(t.Context(), created.ID) + require.NoError(t, err) + require.NoError(t, ReapDisposableContainers(t.Context(), next)) + _, err = backend.client.ContainerInspect(t.Context(), created.ID) + if tc.keep { + require.NoError(t, err) + } else { + require.True(t, errdefs.IsNotFound(err), "%v", err) + } + _, err = backend.client.VolumeInspect(t.Context(), vol.Name) + require.NoError(t, err, "recovery must never delete volumes") + }) + } +} diff --git a/runners/dockerrun/disposable_recovery_test.go b/runners/dockerrun/disposable_recovery_test.go new file mode 100644 index 000000000..348c696e8 --- /dev/null +++ b/runners/dockerrun/disposable_recovery_test.go @@ -0,0 +1,73 @@ +package dockerrun + +import ( + "os" + "os/exec" + "strconv" + "testing" + + "github.com/docker/docker/api/types/container" + "github.com/stretchr/testify/require" +) + +func TestDisposableRecoveryUsesDurableNamespaceAndEphemeralOwnership(t *testing.T) { + home, workspace := t.TempDir(), t.TempDir() + previous, err := NewContainerRecoveryScope(home, workspace, "previous") + require.NoError(t, err) + next, err := NewContainerRecoveryScope(home, workspace, "next") + require.NoError(t, err) + require.NotEqual(t, previous.id, next.id) + require.Equal(t, previous.namespace, next.namespace) + foreignHost, err := newContainerRecoveryScope(home, workspace, "previous", "different-host") + require.NoError(t, err) + foreignHome, err := NewContainerRecoveryScope(t.TempDir(), workspace, "previous") + require.NoError(t, err) + foreignWorkspace, err := NewContainerRecoveryScope(home, t.TempDir(), "previous") + require.NoError(t, err) + owner := exec.Command("true") + require.NoError(t, owner.Run()) + for _, tc := range []struct { + name string + scope ContainerRecoveryScope + ephemeral, ledgered, live, want bool + }{ + {"previous disposable", previous, true, false, false, true}, + {"previous stateful", previous, false, false, false, false}, + {"live owner", previous, true, false, true, false}, + {"ledger", previous, true, true, false, false}, + {"foreign host", foreignHost, true, false, false, false}, + {"foreign home", foreignHome, true, false, false, false}, + {"foreign workspace", foreignWorkspace, true, false, false, false}, + {"legacy exact scope only", ContainerRecoveryScope{id: previous.id}, true, false, false, false}, + {"missing exact scope", ContainerRecoveryScope{namespace: previous.namespace}, true, false, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + labels := map[string]string{LabelCodeflyOwner: labelTrue, LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryNamespace: tc.scope.namespace, LabelCodeflySession: strconv.Itoa(owner.Process.Pid)} + if tc.ephemeral { + labels[LabelCodeflyEphemeral] = labelTrue + } + if tc.live { + labels[LabelCodeflySession] = strconv.Itoa(os.Getpid()) + } + if tc.ledgered { + labels[LabelCodeflyInvocation] = "session" + } + for _, state := range []string{"running", "exited"} { + require.Equal(t, tc.want, disposableContainerInNamespace(container.Summary{State: state, Labels: labels}, next)) + } + }) + } + t.Run("host without durable identity reaps nothing and reports why", func(t *testing.T) { + // Resolution now degrades instead of failing, so this sweep has to say it + // cannot run rather than return a silent success that reads as "nothing + // to collect" while the leak it exists for resumes. + degraded := ContainerRecoveryScope{id: previous.id} + require.False(t, disposableContainerInNamespace(container.Summary{State: "running", Labels: map[string]string{ + LabelCodeflyOwner: labelTrue, LabelCodeflyRecoveryScope: previous.id, + LabelCodeflyRecoveryNamespace: previous.namespace, LabelCodeflyEphemeral: labelTrue, + LabelCodeflySession: strconv.Itoa(owner.Process.Pid), + }}, degraded)) + require.NoError(t, ReapDisposableContainers(t.Context(), degraded)) + require.Error(t, ReapDisposableContainers(t.Context(), ContainerRecoveryScope{})) + }) +} diff --git a/runners/dockerrun/docker_runner.go b/runners/dockerrun/docker_runner.go index 2ce6f43a3..fd5cff68b 100644 --- a/runners/dockerrun/docker_runner.go +++ b/runners/dockerrun/docker_runner.go @@ -210,7 +210,11 @@ func (docker *DockerEnvironment) GetContainer(ctx context.Context) error { if err != nil { return w.Wrapf(err, "cannot check if container is present") } + owned := !exists defer func() { + if !owned { + return + } logContext := context.Background() err := docker.GetLogs(logContext) if err != nil { @@ -223,6 +227,10 @@ func (docker *DockerEnvironment) GetContainer(ctx context.Context) error { if inspectErr != nil { return w.Wrapf(inspectErr, "cannot inspect existing container") } + if ownershipErr := validateContainerRecoveryReuse(inspect.Config, containerConfig); ownershipErr != nil { + return w.Wrapf(ownershipErr, "cannot reuse container %s; recover its owner explicitly before retrying", docker.instance.ID) + } + owned = true actualFingerprint := "" if inspect.Config != nil && inspect.Config.Labels != nil { actualFingerprint = inspect.Config.Labels[LabelCodeflyConfig] @@ -406,8 +414,10 @@ func (docker *DockerEnvironment) desiredContainerConfigs(ctx context.Context) (* containerConfig := docker.createContainerConfig(ctx) if scope.id != "" { containerConfig.Labels[LabelCodeflyRecoveryScope] = scope.id - if scope.group != "" { - containerConfig.Labels[LabelCodeflyRecoveryGroup] = scope.group + // The durable host/home/workspace namespace is what lets a successor run + // recover this container after choosing an entirely fresh naming scope. + if scope.namespace != "" { + containerConfig.Labels[LabelCodeflyRecoveryNamespace] = scope.namespace } } hostConfig := docker.createHostConfig(ctx) @@ -625,7 +635,7 @@ func EphemeralContainers() bool { return true } marker := os.Getenv(EphemeralContainersEnvironment) - return marker != "" && marker == strconv.Itoa(containerRecoveryParentPID) + return containerRecoveryParentPID > 0 && marker == strconv.Itoa(containerRecoveryParentPID) } func (docker *DockerEnvironment) createHostConfig(_ context.Context) *container.HostConfig { diff --git a/runners/dockerrun/session_backend_test.go b/runners/dockerrun/session_backend_test.go index 567f7a536..d5bf8e6b4 100644 --- a/runners/dockerrun/session_backend_test.go +++ b/runners/dockerrun/session_backend_test.go @@ -10,7 +10,6 @@ import ( "os/exec" "path/filepath" "strconv" - "strings" "testing" "time" @@ -291,6 +290,7 @@ func TestReapStaleContainersKeepsLedgeredContainers(t *testing.T) { labels[LabelCodeflyOwner] = "true" labels[LabelCodeflySession] = deadOwner labels[LabelCodeflyRecoveryScope] = scope.id + labels[LabelCodeflyRecoveryNamespace] = scope.namespace labels[LabelCodeflyName] = name _, err := backend.client.ContainerCreate(ctx, &container.Config{Image: "alpine:latest", Labels: labels, Cmd: []string{"true"}}, @@ -350,6 +350,7 @@ func TestReapStaleContainersIsolatesHomeAndScope(t *testing.T) { labels := map[string]string{LabelCodeflyOwner: "true", LabelCodeflySession: tc.pid} if tc.scope.id != "" { labels[LabelCodeflyRecoveryScope] = tc.scope.id + labels[LabelCodeflyRecoveryNamespace] = tc.scope.namespace } if tc.ephemeral { labels[LabelCodeflyEphemeral] = "true" @@ -377,56 +378,3 @@ func TestReapStaleContainersIsolatesHomeAndScope(t *testing.T) { require.NoError(t, err) require.Equal(t, "retained bytes", string(content)) } - -func TestReapStaleContainersRecoversDisposableSiblings(t *testing.T) { - backend := newBackend(t) - ctx := t.Context() - home, workspace := t.TempDir(), t.TempDir() - old := disposableRecoveryScope(t, home, workspace, "tests", strings.Repeat("a", 32)) - current := disposableRecoveryScope(t, home, workspace, "tests", strings.Repeat("b", 32)) - foreign := disposableRecoveryScope(t, t.TempDir(), workspace, "tests", strings.Repeat("c", 32)) - otherScope := disposableRecoveryScope(t, home, workspace, "other", strings.Repeat("d", 32)) - otherWorkspace := disposableRecoveryScope(t, home, t.TempDir(), "tests", strings.Repeat("e", 32)) - process := exec.Command("true") - require.NoError(t, process.Run()) - deadPID := strconv.Itoa(process.Process.Pid) - for _, tc := range []struct { - name string - scope ContainerRecoveryScope - pid string - ephemeral, ledgered, keep bool - }{ - {"orphan", old, deadPID, true, false, false}, - {"live", old, strconv.Itoa(os.Getpid()), true, false, true}, - {"stateful", old, deadPID, false, false, true}, - {"ledgered", old, deadPID, true, true, true}, - {"foreign home", foreign, deadPID, true, false, true}, - {"foreign scope", otherScope, deadPID, true, false, true}, - {"foreign workspace", otherWorkspace, deadPID, true, false, true}, - {"missing owner", old, "", true, false, true}, - } { - t.Run(tc.name, func(t *testing.T) { - labels := map[string]string{LabelCodeflyOwner: labelTrue, LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryGroup: tc.scope.group, LabelCodeflySession: tc.pid} - if tc.ephemeral { - labels[LabelCodeflyEphemeral] = labelTrue - } - if tc.ledgered { - labels[LabelCodeflyInvocation] = "ledger" - } - created, err := backend.client.ContainerCreate(ctx, &container.Config{Image: "alpine:latest", Cmd: []string{"sleep", "300"}, Labels: labels}, nil, nil, nil, "") - require.NoError(t, err) - t.Cleanup(func() { - _ = backend.client.ContainerRemove(context.Background(), created.ID, container.RemoveOptions{Force: true}) - }) - require.NoError(t, backend.client.ContainerStart(ctx, created.ID, container.StartOptions{})) - require.NoError(t, ReapStaleContainers(ctx, current)) - inspected, err := backend.client.ContainerInspect(ctx, created.ID) - if tc.keep { - require.NoError(t, err) - require.True(t, inspected.State.Running) - } else { - require.True(t, errdefs.IsNotFound(err), "orphan survived: %v", err) - } - }) - } -} diff --git a/services/container_recovery_test.go b/services/container_recovery_test.go new file mode 100644 index 000000000..8bba53e9e --- /dev/null +++ b/services/container_recovery_test.go @@ -0,0 +1,33 @@ +package services + +import ( + "testing" + + "github.com/codefly-dev/core/runners/dockerrun" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/metadata" +) + +func TestAcknowledgedContainerRecoveryScope(t *testing.T) { + header := dockerrun.ContainerRecoveryScopeHeader + t.Run("absent", func(t *testing.T) { + scope, err := acknowledgedContainerRecoveryScope(metadata.MD{}) + require.NoError(t, err) + require.Empty(t, scope) + }) + t.Run("single", func(t *testing.T) { + scope, err := acknowledgedContainerRecoveryScope(metadata.Pairs(header, "scope:namespace")) + require.NoError(t, err) + require.Equal(t, "scope:namespace", scope) + }) + t.Run("empty namespace is still an acknowledgement", func(t *testing.T) { + scope, err := acknowledgedContainerRecoveryScope(metadata.Pairs(header, "scope:")) + require.NoError(t, err) + require.Equal(t, "scope:", scope) + }) + t.Run("two identities are refused, not silently dropped", func(t *testing.T) { + scope, err := acknowledgedContainerRecoveryScope(metadata.Pairs(header, "one", header, "two")) + require.ErrorContains(t, err, "acknowledged 2 container recovery identities") + require.Empty(t, scope) + }) +} diff --git a/services/service.go b/services/service.go index 8fcf68b05..93601f14e 100644 --- a/services/service.go +++ b/services/service.go @@ -10,7 +10,10 @@ import ( "github.com/codefly-dev/core/agents/services" "github.com/codefly-dev/core/failures" runners "github.com/codefly-dev/core/runners/base" + "github.com/codefly-dev/core/runners/dockerrun" "github.com/codefly-dev/core/wool" + "google.golang.org/grpc" + "google.golang.org/grpc/metadata" "google.golang.org/protobuf/proto" "github.com/codefly-dev/core/agents/communicate" @@ -37,6 +40,9 @@ type Instance struct { Agent *services.ServiceAgent Info *agentv0.AgentInformation + // ContainerRecoveryScope is the agent's authenticated gRPC acknowledgement, + // not a claim inferred from the CLI's own Core version. + ContainerRecoveryScope string Builder *BuilderInstance Runtime *RuntimeInstance @@ -406,13 +412,18 @@ func Load(ctx context.Context, workspace *resources.Workspace, module *resources } instance.ProcessInfo.AgentPID = agent.ProcessInfo.PID - info, err := agent.GetAgentInformation(ctx, &agentv0.AgentInformationRequest{}) + var headers metadata.MD + info, err := agent.GetAgentInformation(ctx, &agentv0.AgentInformationRequest{}, grpc.Header(&headers)) if err != nil { return nil, w.Wrapf(err, "cannot get agent information: %v", service.Agent) } instance.Capabilities = info.Capabilities instance.Info = info + instance.ContainerRecoveryScope, err = acknowledgedContainerRecoveryScope(headers) + if err != nil { + return nil, w.Wrapf(err, "cannot trust container recovery acknowledgement: %v", service.Agent) + } // Double-check under lock: another goroutine may have loaded the same // identity while we were spawning. Prefer the cached one so callers share @@ -518,3 +529,19 @@ func UpdateAgent(ctx context.Context, service *resources.Service) (*UpdateInform } return info, nil } + +// acknowledgedContainerRecoveryScope reads the agent's ownership +// acknowledgement. Exactly one value is the contract: silently ignoring a second +// made an agent that acknowledged two different identities indistinguishable +// from one that acknowledged none, which is the very condition this header +// exists to detect before Docker provisioning. +func acknowledgedContainerRecoveryScope(headers metadata.MD) (string, error) { + values := headers.Get(dockerrun.ContainerRecoveryScopeHeader) + if len(values) > 1 { + return "", fmt.Errorf("agent acknowledged %d container recovery identities: %v", len(values), values) + } + if len(values) == 0 { + return "", nil + } + return values[0], nil +}