docker: make log rotation a preference, 100m x 10 by default, judged by a step before daemon.json takes it (novox/hq issue 452)

The controller pastes a setting into daemon.json as text and the runtime is its
only reader, so a bad size would surface as a runtime that does not start and a
quote could add any key. A step refuses the value first and gates the module.
The defaults are the one hand-set machine's, so it does not change; nothing
restarts the runtime, which takes the keys at its next start.

modules/docker/cmd/docker-tools/logrotation.go

@@ -0,0 +1,80 @@
+package main
+
+// The container runtime's log rotation, checked before it reaches daemon.json (novox/hq issue 452).
+//
+// **Why a step and not the file alone.** daemon.json's log-driver and log-opts are filled by the controller
+// from this module's settings log-max-size and log-max-file, pasted as text (novox/hq issue 346): the
+// controller judges nothing but that a value is one line. The runtime is the only reader of daemon.json and
+// reads log-opts only when it starts, so a bad value would be found at the runtime's next start, as a runtime
+// that does not start, and a value with a quote in it could add any key to the file. So the module's
+// manifest writes the same two values into log-rotation.conf first, and runs this as a step before daemon.json:
+// a value refused here fails the step, and the host skips the rest of the module (novox/hq ADR 0136), so
+// daemon.json keeps what it had, and the refusal is in the apply's report.
+
+import (
+	"fmt"
+	"os"
+	"regexp"
+	"strings"
+)
+
+// logRotation is what log-rotation.conf says, both values as the runtime takes them: text.
+type logRotation struct{ Size, Files string }
+
+var (
+	// A size as the json-file driver reads it, with the unit required and in lower case, as decided on issue
+	// 452: a whole number from 1 with k, m or g, at most nine digits, so no value overflows the runtime's count.
+	logSize = regexp.MustCompile(`^[1-9][0-9]{0,8}[kmg]$`)
+	// A count of files from 1 to 99, without a leading zero.
+	logFiles = regexp.MustCompile(`^[1-9][0-9]?$`)
+)
+
+// parseLogRotation reads `size=` and `files=` once each; a comment or an empty line is skipped, and any other
+// line is refused, so a value that brought a line of its own is never read past.
+func parseLogRotation(text string) (logRotation, error) {
+	var got logRotation
+	seen := map[string]bool{}
+	for _, line := range strings.Split(text, "\n") {
+		if line == "" || strings.HasPrefix(line, "#") {
+			continue
+		}
+		key, value, ok := strings.Cut(line, "=")
+		if !ok || (key != "size" && key != "files") {
+			return logRotation{}, fmt.Errorf("log-rotation.conf has a line that is not size= or files=: %q", line)
+		}
+		if seen[key] {
+			return logRotation{}, fmt.Errorf("log-rotation.conf says %s twice", key)
+		}
+		seen[key] = true
+		switch key {
+		case "size":
+			if !logSize.MatchString(value) {
+				return logRotation{}, fmt.Errorf("log-max-size %q is not a size: a whole number from 1 with k, m or g "+
+					"after it, such as 100m; daemon.json was left as it is", value)
+			}
+			got.Size = value
+		case "files":
+			if !logFiles.MatchString(value) {
+				return logRotation{}, fmt.Errorf("log-max-file %q is not a count from 1 to 99; daemon.json was left "+
+					"as it is", value)
+			}
+			got.Files = value
+		}
+	}
+	for _, key := range []string{"size", "files"} {
+		if !seen[key] {
+			return logRotation{}, fmt.Errorf("log-rotation.conf does not say %s", key)
+		}
+	}
+	return got, nil
+}
+
+// checkLogRotationFile is the step: the file the manifest wrote, judged.
+func checkLogRotationFile(path string) error {
+	raw, err := os.ReadFile(path)
+	if err != nil {
+		return err
+	}
+	_, err = parseLogRotation(string(raw))
+	return err
+}

modules/docker/cmd/docker-tools/logrotation_test.go

@@ -0,0 +1,214 @@
+package main
+
+import (
+	"encoding/json"
+	"fmt"
+	"os"
+	"path/filepath"
+	"reflect"
+	"regexp"
+	"strings"
+	"testing"
+)
+
+// The container runtime's log rotation is a preference (novox/hq issue 452).
+
+type manifestForLogs struct {
+	Settings map[string]struct {
+		Kind    string `json:"kind"`
+		Default any    `json:"default"`
+		Why     string `json:"why"`
+	} `json:"settings"`
+	Resources []map[string]any `json:"resources"`
+}
+
+func readManifestForLogs(t *testing.T) manifestForLogs {
+	t.Helper()
+	raw, err := os.ReadFile("../../module.json")
+	if err != nil {
+		t.Fatal(err)
+	}
+	var m manifestForLogs
+	if err := json.Unmarshal(raw, &m); err != nil {
+		t.Fatal(err)
+	}
+	return m
+}
+
+func resourceByID(m manifestForLogs, id string) (map[string]any, int) {
+	for i, r := range m.Resources {
+		if r["id"] == id {
+			return r, i
+		}
+	}
+	return nil, -1
+}
+
+// filled is a file's content as the controller fills it from the settings' defaults alone: each
+// ${setting:<key>} replaced by the default as text, the way the controller writes a value into a file.
+func filled(t *testing.T, m manifestForLogs, content string) string {
+	t.Helper()
+	ref := regexp.MustCompile(`\$\{setting:([a-z0-9][a-z0-9_.-]*)\}`)
+	var missing []string
+	out := ref.ReplaceAllStringFunc(content, func(s string) string {
+		key := ref.FindStringSubmatch(s)[1]
+		d, ok := m.Settings[key]
+		if !ok || d.Kind != "preference" {
+			missing = append(missing, key)
+			return s
+		}
+		switch v := d.Default.(type) {
+		case float64:
+			return fmt.Sprintf("%d", int64(v))
+		default:
+			return fmt.Sprint(v)
+		}
+	})
+	if len(missing) > 0 {
+		t.Fatalf("the content asks for %v, which the manifest gives no preference default", missing)
+	}
+	return strings.ReplaceAll(out, "${seat:mesh-artifact-store:reach}", "registry.example:5100")
+}
+
+func TestTheDefaultLogRotationIsTheOneTheHandSetMachineHas(t *testing.T) {
+	m := readManifestForLogs(t)
+	daemon, _ := resourceByID(m, "daemon")
+	if daemon == nil {
+		t.Fatal("no daemon resource")
+	}
+	var keys map[string]any
+	if err := json.Unmarshal([]byte(filled(t, m, daemon["content"].(string))), &keys); err != nil {
+		t.Fatalf("daemon.json's keys with the defaults are not JSON: %v", err)
+	}
+	if keys["log-driver"] != "json-file" {
+		t.Errorf("log-driver is %v, want json-file (the driver every machine runs today)", keys["log-driver"])
+	}
+	want := map[string]any{"max-size": "100m", "max-file": "10"}
+	if !reflect.DeepEqual(keys["log-opts"], want) {
+		t.Errorf("log-opts is %v, want %v", keys["log-opts"], want)
+	}
+	if keys["live-restore"] != true {
+		t.Errorf("live-restore is %v", keys["live-restore"])
+	}
+	// The values a person set are the terminal's only (novox/hq issue 346): the file says so.
+	if daemon["trusted"] != true {
+		t.Errorf("daemon asks for settings and says trusted %v, want true", daemon["trusted"])
+	}
+}
+
+func TestTheLogRotationIsCheckedBeforeDaemonJSONIsWritten(t *testing.T) {
+	m := readManifestForLogs(t)
+	conf, confAt := resourceByID(m, "log-rotation")
+	check, checkAt := resourceByID(m, "log-rotation-check")
+	_, daemonAt := resourceByID(m, "daemon")
+	if conf == nil || check == nil {
+		t.Fatal("no log-rotation file or log-rotation-check step")
+	}
+	// The host applies a module's resources in declared order, and a step that fails skips the rest
+	// of its module (novox/hq ADR 0136): declared before daemon, a refused value never reaches it.
+	if !(confAt < checkAt && checkAt < daemonAt) {
+		t.Errorf("order: log-rotation %d, log-rotation-check %d, daemon %d", confAt, checkAt, daemonAt)
+	}
+	if check["type"] != "process" || check["run-once"] != true || check["artifact"] != "tools" {
+		t.Errorf("the check is not a step of the tools bundle: %v", check)
+	}
+	if !reflect.DeepEqual(check["run"], []any{"./docker-tools", "check-log-rotation"}) {
+		t.Errorf("run %v", check["run"])
+	}
+	if !reflect.DeepEqual(check["restart-on"], []any{"log-rotation"}) {
+		t.Errorf("restart-on %v", check["restart-on"])
+	}
+	if _, asRoot := check["user"]; asRoot {
+		t.Errorf("the check reads a 0600 file and must run as root")
+	}
+	if conf["trusted"] != true {
+		t.Errorf("log-rotation says trusted %v, want true", conf["trusted"])
+	}
+	// The step reads the very values daemon.json takes: the same keys, filled the same way.
+	values, err := parseLogRotation(filled(t, m, conf["content"].(string)))
+	if err != nil {
+		t.Fatalf("the defaults are refused: %v", err)
+	}
+	if values != (logRotation{Size: "100m", Files: "10"}) {
+		t.Errorf("defaults read as %+v", values)
+	}
+	daemon, _ := resourceByID(m, "daemon")
+	for _, key := range []string{"${setting:log-max-size}", "${setting:log-max-file}"} {
+		if !strings.Contains(daemon["content"].(string), key) || !strings.Contains(conf["content"].(string), key) {
+			t.Errorf("%s is not in both daemon and log-rotation", key)
+		}
+	}
+}
+
+func TestTheRuntimeIsNeverRestartedForTheLogRotation(t *testing.T) {
+	m := readManifestForLogs(t)
+	runtime, _ := resourceByID(m, "runtime")
+	if _, restarts := runtime["restart-on"]; restarts {
+		t.Errorf("docker.service restarts on %v: a restart stops every container", runtime["restart-on"])
+	}
+	if !reflect.DeepEqual(runtime["reload-on"], []any{"daemon"}) {
+		t.Errorf("reload-on %v", runtime["reload-on"])
+	}
+}
+
+func TestABadLogRotationIsRefused(t *testing.T) {
+	good := map[string]string{"size": "100m", "files": "10"}
+	conf := func(over map[string]string) string {
+		lines := []string{"# a comment"}
+		for _, k := range []string{"size", "files"} {
+			v := good[k]
+			if o, ok := over[k]; ok {
+				v = o
+			}
+			lines = append(lines, k+"="+v)
+		}
+		return strings.Join(lines, "\n") + "\n"
+	}
+	for _, bad := range []map[string]string{
+		{"size": "100"}, {"size": "10x"}, {"size": "0m"}, {"size": "-1"}, {"size": ""}, {"size": "m"},
+		{"size": "1.5m"}, {"size": "10M"}, {"size": "100m\""}, {"size": `100m", "data-root": "/tmp`},
+		{"size": "100m\nfiles=3"}, {"size": " 100m"}, {"size": "99999999999m"},
+		{"files": "0"}, {"files": "100"}, {"files": "-1"}, {"files": ""}, {"files": "01"}, {"files": "1\""},
+		{"files": "1\nfiles=2"}, {"files": "3.0"}, {"files": "ten"},
+	} {
+		if v, err := parseLogRotation(conf(bad)); err == nil {
+			t.Errorf("%q accepted as %+v", bad, v)
+		}
+	}
+	for _, broken := range []string{"size=100m\n", "files=10\n", "size=100m\nfiles=10\nfiles=3\n",
+		"size=100m\nfiles=10\nmode=x\n", "size=100m\nfiles\n"} {
+		if v, err := parseLogRotation(broken); err == nil {
+			t.Errorf("%q accepted as %+v", broken, v)
+		}
+	}
+}
+
+func TestAGoodLogRotationIsAccepted(t *testing.T) {
+	for _, c := range []struct{ size, files string }{{"100m", "10"}, {"512k", "1"}, {"2g", "99"}, {"50m", "3"}} {
+		got, err := parseLogRotation("# comment\nsize=" + c.size + "\nfiles=" + c.files + "\n")
+		if err != nil {
+			t.Errorf("%v refused: %v", c, err)
+			continue
+		}
+		if got != (logRotation{Size: c.size, Files: c.files}) {
+			t.Errorf("%v read as %+v", c, got)
+		}
+	}
+	// And from the file the step is handed.
+	path := filepath.Join(t.TempDir(), "log-rotation.conf")
+	if err := os.WriteFile(path, []byte("size=50m\nfiles=3\n"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := checkLogRotationFile(path); err != nil {
+		t.Errorf("a good file refused: %v", err)
+	}
+	if err := os.WriteFile(path, []byte("size=50\nfiles=3\n"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := checkLogRotationFile(path); err == nil || !strings.Contains(err.Error(), "log-max-size") {
+		t.Errorf("a bad size was not refused by its setting's name: %v", err)
+	}
+	if err := checkLogRotationFile(filepath.Join(t.TempDir(), "absent")); err == nil {
+		t.Error("an absent file was accepted")
+	}
+}

modules/docker/cmd/docker-tools/main.go

@@ -16,6 +16,20 @@ import (
 )
 
 func main() {
+	// Started as `docker-tools check-log-rotation` it is the module's step that judges the log rotation before
+	// daemon.json takes it (logrotation.go, novox/hq issue 452): run as root by the host, reading the file
+	// DOCKER_LOG_ROTATION names. With no argument, the runtime's tools bundle.
+	if len(os.Args) > 1 {
+		if os.Args[1] != "check-log-rotation" || len(os.Args) != 2 {
+			fmt.Fprintln(os.Stderr, "usage: docker-tools [check-log-rotation]")
+			os.Exit(2)
+		}
+		if err := checkLogRotationFile(os.Getenv("DOCKER_LOG_ROTATION")); err != nil {
+			fmt.Fprintln(os.Stderr, err)
+			os.Exit(1)
+		}
+		return
+	}
 	// An empty name serves as the module the runtime names (MESH_SERVED_MODULE): docker.
 	if err := stdio.Serve("", tools(NewClient())); err != nil {
 		fmt.Fprintln(os.Stderr, err)