Merge pull request 'Pass a container's groups to the runtime as supplementary groups and report container-groups (issue 1637, ADR 0480 clause 7)' (#127) from feat/r4d-groups into main

2026-10-21 14:45:04 +00:00
parent fd3259a3f5 d9f8bbec0c
commit 7e83137aa8
6 changed files with 168 additions and 49 deletions
@@ -3,7 +3,7 @@ module github.com/novox/mesh-host
 go 1.26.0

 require (
-	git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261021124357-863a981e014e
+	git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261021130958-85b5a2687e6a
 	github.com/nats-io/nats.go v1.54.0
 	golang.org/x/crypto v0.57.0
 	golang.org/x/sys v0.48.0
@@ -4,6 +4,8 @@ git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261021122348-6c857e1c025f h1:7J/9jOfS
 git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261021122348-6c857e1c025f/go.mod h1:ouERNpZizzwZ0HLEtAypsY9FR20FyUgUYRni6J7kVCY=
 git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261021124357-863a981e014e h1:eq29ENikw3XfEXjxL5OdnTbavv5pu1/UnQhWWuf5V5I=
 git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261021124357-863a981e014e/go.mod h1:ouERNpZizzwZ0HLEtAypsY9FR20FyUgUYRni6J7kVCY=
+git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261021130958-85b5a2687e6a h1:CVTQBD4vxbGQ+TW9MWAut/ABQN6nvAZ1VTqojNpvGUE=
+git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261021130958-85b5a2687e6a/go.mod h1:ouERNpZizzwZ0HLEtAypsY9FR20FyUgUYRni6J7kVCY=
 github.com/klauspost/compress v1.20.0 h1:a3C1ke2ohxFymNlb2HWAHjDeKCI90scRskErZkR0ezA=
 github.com/klauspost/compress v1.20.0/go.mod h1:LUdAzn7YLVvxLpc7y3V1m40wESHTgc1422pwwBSKYuI=
 github.com/nats-io/nats.go v1.54.0 h1:vsXoOxjHp/GmPUN+EcI7uOf/uB+iAP+kEsAFNQN0yzA=
@@ -2605,6 +2605,11 @@ func containerSpecReading(r *declaration.Container, declares, reads map[string]s
 	if args := identityArgs(r); len(args) > 0 {
 		b.WriteString("identity " + strings.Join(args, " ") + "\n")
 	}
+	// And the supplementary groups it joins (novox/hq ADR 0480 clause 7): fixed when the container is created, so
+	// a changed set is a container recreated. Only when there are some, so no other container has its digest moved.
+	if args := groupArgs(r); len(args) > 0 {
+		b.WriteString("groups " + strings.Join(args, " ") + "\n")
+	}
 	// **What this container reads is part of what it is.**
 	//
 	// A container takes its environment and its mounted files once, at start, and never looks
@@ -2833,54 +2838,7 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner,
 		}
 	}

-	args := []string{"run", "--detach", "--name", r.Name, "--restart", "unless-stopped"}
-	for _, file := range r.EnvFile {
-		args = append(args, "--env-file", file)
-	}
-	if r.Network != "" {
-		args = append(args, "--network", r.Network)
-	}
-	for _, c := range r.Capabilities {
-		args = append(args, "--cap-add", c)
-	}
-	if r.Logging != "" {
-		// The journal keeps the container's name on every line (CONTAINER_NAME), which is what a
-		// jail matches on (novox/hq ADR 0179); `docker logs` keeps working against the journal.
-		args = append(args, "--log-driver", r.Logging)
-	}
-	for _, d := range r.Dns {
-		args = append(args, "--dns", d)
-	}
-	if r.IP != "" {
-		args = append(args, "--ip", r.IP)
-	}
-	args = append(args,
-		"--label", specLabel+"="+want, "--label", idLabel+"="+r.ID)
-	for _, k := range sortedKeys(r.Env) {
-		args = append(args, "--env", k+"="+r.Env[k])
-	}
-	for _, p := range r.Ports {
-		args = append(args, "--publish", p)
-	}
-	for _, v := range r.Volumes {
-		args = append(args, "--volume", v)
-	}
-	args = append(args, identityArgs(r)...)
-	// **The one check the container carries is the one the module declared** (ADR 0240 rule 3): an
-	// exec command, or the image's own adopted by name, with the declared timing. Nothing else on the
-	// machine sets a container's check.
-	if r.Health.RunByRuntime() {
-		args = append(args, runtimeCheckArgs(r.Health)...)
-	}
-	for _, h := range r.Hosts {
-		// Written into the container's own hosts file by the runtime. Per container rather than
-		// by editing the machine's resolver configuration: that file belongs to something else on
-		// most machines, and a host that edited it would be fighting whatever owns it on every
-		// boot — the fault this host exists to avoid, in the place it would be hardest to see.
-		args = append(args, "--add-host", h)
-	}
-	args = append(args, r.Image)
-	args = append(args, r.Args...)
+	args := detachedRunArgs(r, want)

 	if _, err := run(ctx, cri, args...); err != nil {
 		return out, fmt.Errorf("starting container %s: %w", r.Name, err)
@@ -2935,6 +2893,60 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner,
 	return out, nil
 }

+// detachedRunArgs is the `docker run` that starts a service container and leaves it running.
+func detachedRunArgs(r *declaration.Container, want string) []string {
+	args := []string{"run", "--detach", "--name", r.Name, "--restart", "unless-stopped"}
+	for _, file := range r.EnvFile {
+		args = append(args, "--env-file", file)
+	}
+	if r.Network != "" {
+		args = append(args, "--network", r.Network)
+	}
+	for _, c := range r.Capabilities {
+		args = append(args, "--cap-add", c)
+	}
+	if r.Logging != "" {
+		// The journal keeps the container's name on every line (CONTAINER_NAME), which is what a
+		// jail matches on (novox/hq ADR 0179); `docker logs` keeps working against the journal.
+		args = append(args, "--log-driver", r.Logging)
+	}
+	for _, d := range r.Dns {
+		args = append(args, "--dns", d)
+	}
+	if r.IP != "" {
+		args = append(args, "--ip", r.IP)
+	}
+	args = append(args,
+		"--label", specLabel+"="+want, "--label", idLabel+"="+r.ID)
+	for _, k := range sortedKeys(r.Env) {
+		args = append(args, "--env", k+"="+r.Env[k])
+	}
+	for _, p := range r.Ports {
+		args = append(args, "--publish", p)
+	}
+	for _, v := range r.Volumes {
+		args = append(args, "--volume", v)
+	}
+	args = append(args, identityArgs(r)...)
+	args = append(args, groupArgs(r)...)
+	// **The one check the container carries is the one the module declared** (ADR 0240 rule 3): an
+	// exec command, or the image's own adopted by name, with the declared timing. Nothing else on the
+	// machine sets a container's check.
+	if r.Health.RunByRuntime() {
+		args = append(args, runtimeCheckArgs(r.Health)...)
+	}
+	for _, h := range r.Hosts {
+		// Written into the container's own hosts file by the runtime. Per container rather than
+		// by editing the machine's resolver configuration: that file belongs to something else on
+		// most machines, and a host that edited it would be fighting whatever owns it on every
+		// boot — the fault this host exists to avoid, in the place it would be hardest to see.
+		args = append(args, "--add-host", h)
+	}
+	args = append(args, r.Image)
+	args = append(args, r.Args...)
+	return args
+}
+
 // runtimeCheckArgs is a declared check as the runtime takes it (novox/hq ADR 0240 rule 3, to-be 48 §3):
 // its timing, and for an exec check its command, run by the container's shell. The runtime's own retries
 // are the declared failing looks, and its start period the grace — so its retries and start period do the
@@ -3025,6 +3037,7 @@ func foregroundRunArgs(r *declaration.Container, want string) []string {
 		args = append(args, "--volume", v)
 	}
 	args = append(args, identityArgs(r)...)
+	args = append(args, groupArgs(r)...)
 	for _, h := range r.Hosts {
 		args = append(args, "--add-host", h)
 	}
@@ -3033,6 +3046,16 @@ func foregroundRunArgs(r *declaration.Container, want string) []string {
 	return args
 }

+// groupArgs is the runtime's arguments that add a container's supplementary groups, one per number
+// (novox/hq ADR 0480 clause 7, issue 1637). None for a container that declares none.
+func groupArgs(r *declaration.Container) []string {
+	var args []string
+	for _, g := range r.Groups {
+		args = append(args, "--group-add", strconv.FormatInt(g, 10))
+	}
+	return args
+}
+
 // specWithoutIdentity is the spec r had before it was told who to run as.
 func specWithoutIdentity(r *declaration.Container, declares, reads map[string]string) string {
 	plain := *r
@@ -0,0 +1,54 @@
+package apply
+
+import (
+	"strings"
+	"testing"
+
+	"git.novox.be/novox/mesh-sdk/go/declaration"
+)
+
+// Issue 1637 (novox/hq ADR 0480 clause 7): a container's supplementary groups reach the runtime as --group-add.
+
+func groupsContainer(groups ...int64) *declaration.Container {
+	return &declaration.Container{ID: "n8n.app", Name: "n8n", Image: pinned, Groups: groups}
+}
+
+// 1637/AC1: both run commands carry one --group-add per group, and none without groups.
+func TestAContainerIsAddedItsSupplementaryGroups(t *testing.T) {
+	with := groupsContainer(2000, 2001)
+	for name, args := range map[string][]string{
+		"detached":   detachedRunArgs(with, "x"),
+		"foreground": foregroundRunArgs(with, "x"),
+	} {
+		got := " " + strings.Join(args, " ") + " "
+		if !strings.Contains(got, " --group-add 2000 ") || !strings.Contains(got, " --group-add 2001 ") {
+			t.Errorf("1637/AC1: the %s run does not add both groups: %s", name, got)
+		}
+	}
+	without := groupsContainer()
+	for name, args := range map[string][]string{
+		"detached":   detachedRunArgs(without, "x"),
+		"foreground": foregroundRunArgs(without, "x"),
+	} {
+		if strings.Contains(strings.Join(args, " "), "--group-add") {
+			t.Errorf("1637/AC1: the %s run adds a group none was declared", name)
+		}
+	}
+}
+
+// 1637/AC2: a changed set of groups recreates the container; no groups leaves the spec as it was.
+func TestAChangeOfGroupsRecreatesTheContainer(t *testing.T) {
+	none, a, b := groupsContainer(), groupsContainer(2000), groupsContainer(2000, 2001)
+	if containerSpec(none, inputs{}) == containerSpec(a, inputs{}) || containerSpec(a, inputs{}) == containerSpec(b, inputs{}) {
+		t.Error("1637/AC2: a change of groups does not change the container's spec")
+	}
+	same := groupsContainer(2000)
+	if containerSpec(a, inputs{}) != containerSpec(same, inputs{}) {
+		t.Error("1637/AC2: the same groups changed the spec")
+	}
+	plain := *none
+	plain.Groups = nil
+	if containerSpec(none, inputs{}) != containerSpec(&plain, inputs{}) {
+		t.Error("1637/AC2: a container with no groups has its spec moved")
+	}
+}
@@ -0,0 +1,21 @@
+package profile
+
+import (
+	"context"
+	"testing"
+	"time"
+)
+
+// 1637/AC3: the node's report carries the capability that says the node-engine reads a container's groups.
+func TestTheProfileSaysTheNodeEngineReadsContainerGroups(t *testing.T) {
+	got := Detect(context.Background(), Default(func(context.Context, string, ...string) (string, error) {
+		return "", nil
+	}), time.Second)
+	v := verdictOf(t, got, CapContainerGroups)
+	if !v.Present || v.Detail == "" || v.How == "" {
+		t.Errorf("1637/AC3: container-groups = %+v, want present with a detail and a method", v)
+	}
+	if CapContainerGroups != "container-groups" {
+		t.Errorf("1637/AC3: the capability is named %q", CapContainerGroups)
+	}
+}
@@ -35,6 +35,10 @@ const (
 	// bus's object bucket and checks it before writing (novox/hq ADR 0486 §5, issue 1695). A fact about the program, as
 	// CapContainerGroups: the controller sends references only to a node that reports it.
 	CapFileReferences = declaration.FileReferencesCapability
+	// CapContainerGroups says this node-engine reads a container's `groups` and passes each to the runtime as a
+	// supplementary group (novox/hq ADR 0480 clause 7, issue 1637). A fact about the program, not the machine: the
+	// controller sends the field only to a node that reports it, since an older node-engine refuses the key.
+	CapContainerGroups = "container-groups"

 	// The network manager this machine runs, one capability per dialect (novox/hq ADR 0161): the
 	// uplink seat's holder declares its own, so the holder for a manager the machine does not run
@@ -172,6 +176,20 @@ func (fileReferences) Detect(context.Context) Verdict {
 	}
 }

+// containerGroups reports that this node-engine reads a container's groups. It is true of the program that is
+// running, so it is always present: its absence on a node is what tells the controller the node-engine is older.
+type containerGroups struct{}
+
+func (containerGroups) Name() string { return CapContainerGroups }
+
+func (containerGroups) Detect(context.Context) Verdict {
+	return Verdict{
+		Name: CapContainerGroups, Present: true,
+		Detail: "passes a container's groups to the runtime as supplementary groups",
+		How:    "a fact about this program, not probed: the node-engine that answers reads the field",
+	}
+}
+
 // graphicalSession reports whether anything could display a window here.
 //
 // Environment rather than a probe, because a display server is reachable through a socket a
@@ -211,6 +229,7 @@ func Default(runner Runner) []Detector {
 	return []Detector{
 		privileged{},
 		fileReferences{},
+		containerGroups{},
 		graphicalSession{},
 		seat{},
 		battery{},