diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index 7fa04ba1..8dcd7a51 100644 --- a/internal/catalogue/manifest.go +++ b/internal/catalogue/manifest.go @@ -1759,6 +1759,7 @@ func ParseManifest(raw []byte) (Manifest, error) { } problems = append(problems, invokeProblems(m)...) problems = append(problems, replacesProblems(m)...) + problems = append(problems, UnitSettingProblems(m)...) problems = append(problems, endpointNameProblems(m)...) problems = append(problems, RouteProblems(m)...) for _, port := range m.Guards { diff --git a/internal/catalogue/setting_in_unit_test.go b/internal/catalogue/setting_in_unit_test.go new file mode 100644 index 00000000..77197b72 --- /dev/null +++ b/internal/catalogue/setting_in_unit_test.go @@ -0,0 +1,159 @@ +package catalogue + +import ( + "strings" + "testing" +) + +// A setting may name the unit a service resource holds: a module that stops the distribution's own timer +// for the pool the operator names cannot write the pool into its definition (novox/hq ADR 0112), and a +// unit name with ${setting:…} left in it is a unit no machine has, so the apply fails far from its cause. + +func scrubber() Manifest { + return Manifest{Module: "zfs", + Settings: map[string]SettingDeclaration{ + "scrub-cadence": {Kind: KindPreference, Default: "weekly", Why: "what the distribution's timer did"}, + }, + Resources: []map[string]any{ + {"id": "distribution-weekly", "type": "service", "unit": "zfs-scrub-weekly@${setting:scrub-pool}.timer", + "state": "stopped", "boot": "disabled"}, + {"id": "distribution-monthly", "type": "service", "unit": "zfs-scrub-monthly@${setting:scrub-pool}.timer", + "state": "stopped", "boot": "disabled"}, + {"id": "scrub-config", "type": "file", "path": "/etc/zfs-tools/scrub.conf", + "content": "pool=${setting:scrub-pool}\ncadence=${setting:scrub-cadence}\n"}, + }, + } +} + +func TestASettingNamesAServicesUnit(t *testing.T) { + m := scrubber() + layers := WithDefaults(m, []Layer{{From: "ace", Values: map[string]any{"scrub-pool": "storage", "scrub-cadence": "monthly"}}}) + for i, want := range []string{"zfs-scrub-weekly@storage.timer", "zfs-scrub-monthly@storage.timer"} { + r := map[string]any{} + for k, v := range m.Resources[i] { + r[k] = v + } + if err := settingInto(r, layers, m.Module); err != nil { + t.Fatal(err) + } + if r["unit"] != want { + t.Errorf("composed as %q, not %q", r["unit"], want) + } + } +} + +// Composed for a machine, the way a push writes it: the operator's pool and cadence reach the units' names +// and the file. +func TestAComposedMachineGetsTheUnitTheSettingNames(t *testing.T) { + r := anAdoptedAnchor() + r.Modules = append(r.Modules, scrubber()) + with := anchorRendering(false) + with.Settings["zfs"] = []Layer{{From: "anchor", Values: map[string]any{"scrub-pool": "storage", "scrub-cadence": "monthly"}}} + composed, err := r.Compose(with) + if err != nil { + t.Fatal(err) + } + if why, left := composed.LeftOut["zfs"]; left { + t.Fatalf("left out: %s", why) + } + got := byID(composed.Resources) + for id, want := range map[string]string{"zfs.distribution-weekly": "zfs-scrub-weekly@storage.timer", + "zfs.distribution-monthly": "zfs-scrub-monthly@storage.timer"} { + if got[id]["unit"] != want { + t.Errorf("%s composed as %v, not %s", id, got[id]["unit"], want) + } + t.Logf("%s: %v", id, got[id]["unit"]) + } + if c := got["zfs.scrub-config"]["content"]; c != "pool=storage\ncadence=monthly\n" { + t.Errorf("the file: %q", c) + } +} + +// A value that would not make a unit's name is refused by name, never written: a space, a slash, a +// newline that would begin a directive, or a second suffix. +func TestASettingThatMakesNoUnitNameIsRefused(t *testing.T) { + m := scrubber() + for _, bad := range []string{"", "stor age", "a/b", "storage\nExecStart=/bin/sh", "a@b", "$(x)", "*", `a\\x2d`} { + r := map[string]any{} + for k, v := range m.Resources[0] { + r[k] = v + } + err := settingInto(r, []Layer{{From: "ace", Values: map[string]any{"scrub-pool": bad}}}, m.Module) + if err == nil || !strings.Contains(err.Error(), "scrub-pool") || !strings.Contains(err.Error(), "unit") { + t.Errorf("%q: %v", bad, err) + } + if r["unit"] != m.Resources[0]["unit"] { + t.Errorf("%q: the unit was changed on refusal: %v", bad, r["unit"]) + } + } + r := map[string]any{} + for k, v := range m.Resources[0] { + r[k] = v + } + if err := settingInto(r, nil, m.Module); err == nil || !strings.Contains(err.Error(), "${setting:scrub-pool}") { + t.Errorf("nothing set: %v", err) + } +} + +// A key a unit's name asks for is a destination, so setting it is not called stray, and judging the +// settings sees it. +func TestASettingAUnitAsksForIsNotStrayAndIsJudged(t *testing.T) { + m := scrubber() + stray := strings.Join(UnusedSettings(m, []Layer{{From: "ace", Values: map[string]any{"scrub-pool": "storage"}}}), "; ") + if strings.Contains(stray, "scrub-pool") { + t.Errorf("called stray: %s", stray) + } + if err := JudgeSettings(m, nil, false); err == nil || !strings.Contains(err.Error(), "scrub-pool") { + t.Errorf("a unit's setting nothing sets is not refused when judged: %v", err) + } +} + +// Third review: a setting may name only a template's instance — right after the `@`, before the final +// suffix, and the whole instance — so a value can never make the unit another unit, nor another kind. +// Refused where the manifest is read, near its author. +func TestASettingInAUnitNameIsOnlyATemplatesInstance(t *testing.T) { + ok := []string{"zfs-scrub-weekly@${setting:scrub-pool}.timer", "getty@${setting:tty}.service"} + bad := []string{ + "${setting:unit}", + "${setting:name}.timer", + "zfs-scrub-${setting:cadence}@storage.timer", + "zfs-scrub@${setting:pool}-x.timer", + "zfs-scrub@x-${setting:pool}.timer", + "zfs-scrub@${setting:pool}.${setting:kind}", + "zfs-scrub@${setting:pool}", + "zfs-scrub@${setting:a}${setting:b}.timer", + } + for _, unit := range append(ok, bad...) { + m := Manifest{Module: "zfs", Resources: []map[string]any{{"id": "t", "type": "service", "unit": unit, "state": "stopped"}}} + err := parsed(t, m) + refused := err != nil && strings.Contains(err.Error(), "instance") + want := false + for _, b := range bad { + want = want || b == unit + } + if refused != want { + t.Errorf("%s: refused %v, want %v (%v)", unit, refused, want, err) + } + } +} + +func TestAnInstanceValueMayNotLeadWithADashNorBeLong(t *testing.T) { + m := scrubber() + for _, bad := range []string{"-storage", "--help", strings.Repeat("a", 65)} { + r := map[string]any{} + for k, v := range m.Resources[0] { + r[k] = v + } + err := settingInto(r, []Layer{{From: "ace", Values: map[string]any{"scrub-pool": bad}}}, m.Module) + if err == nil || !strings.Contains(err.Error(), "scrub-pool") { + t.Errorf("%q: %v", bad, err) + } + } + r := map[string]any{} + for k, v := range m.Resources[0] { + r[k] = v + } + if err := settingInto(r, []Layer{{From: "ace", Values: map[string]any{"scrub-pool": strings.Repeat("a", 64)}}}, m.Module); err != nil { + t.Errorf("64 characters: %v", err) + } +} diff --git a/internal/catalogue/setting_into.go b/internal/catalogue/setting_into.go index caa2a18d..9ee165b7 100644 --- a/internal/catalogue/setting_into.go +++ b/internal/catalogue/setting_into.go @@ -15,7 +15,7 @@ import ( // (novox/hq issues 122, 134). ADR 0112 names the operator as one of the four providers; this is the // operator answering. // -// `${setting:}` in a file's content is filled from the module's settings layers — the mesh's, +// `${setting:}` in a file's content, and in a service's unit name, is filled from the module's settings layers — the mesh's, // then this node's — the same layers a mergeable JSON file and a contribution already take, so // `settings set ` is the one place a person's values go. **Refused when no layer sets it**, // naming the key and the remedy: a definition that carried a default for a mail domain would be @@ -45,6 +45,9 @@ func settingsUsed(content string) []string { // the defaults under the layers with WithDefaults. A value that is not a string is written the way a program would read // it (a number without a trailing .000000, a boolean as true/false). func settingInto(resource map[string]any, layers []Layer, module string) error { + if fmt.Sprint(resource["type"]) == "service" { + return settingIntoUnit(resource, layers, module) + } if fmt.Sprint(resource["type"]) != "file" { return nil } @@ -68,6 +71,67 @@ func settingInto(resource map[string]any, layers []Layer, module string) error { return nil } +// unitPart is what a setting may put into a unit's name: the characters systemd allows in a unit name, +// less the instance's `@` and the escape's `\`, and at least one of them. Anything else — a space, a slash, +// a newline that would begin a directive in the unit file the name ends up in — is refused, never written. +var unitPart = regexp.MustCompile(`^[A-Za-z0-9:_.][A-Za-z0-9:_.-]{0,63}$`) + +// unitInstance is the one place a setting may stand in a unit's name: the whole instance of a template, +// after its `@` and before its suffix — `zfs-scrub-weekly@${setting:scrub-pool}.timer` — so a value can +// make the unit another instance of the same template and nothing else (third review of mesh-catalog #147). +var unitInstance = regexp.MustCompile(`^[A-Za-z0-9:_.-]+@\$\{setting:[a-z0-9][a-z0-9_.-]*\}\.[a-z]+$`) + +// UnitSettingProblems refuses, where a manifest is read, a service whose unit name carries a setting +// anywhere but as a template's whole instance. +func UnitSettingProblems(m Manifest) []string { + var problems []string + for _, r := range m.Resources { + if fmt.Sprint(r["type"]) != "service" { + continue + } + unit, _ := r["unit"].(string) + if len(settingsUsed(unit)) == 0 && !strings.Contains(unit, "${setting:") { + continue + } + if !unitInstance.MatchString(unit) { + problems = append(problems, fmt.Sprintf("%s: the service %v's unit %q carries a setting outside a template's "+ + "instance; a setting may stand only as the whole instance, after the @ and before the suffix "+ + "(name@${setting:key}.timer)", m.Module, r["id"], unit)) + } + } + return problems +} + +// settingIntoUnit fills ${setting:…} in a service resource's unit name: a module that holds the +// distribution's timer for the pool the operator names cannot write the pool into its definition +// (novox/hq ADR 0112). Refused, with the key and why, when nothing sets it or the value would not make a +// unit's name; the resource is left as it was. +func settingIntoUnit(resource map[string]any, layers []Layer, module string) error { + unit, ok := resource["unit"].(string) + if !ok { + return nil + } + for _, key := range settingsUsed(unit) { + value, set := settingValue(layers, key) + if !set { + return fmt.Errorf( + "%s has a service whose unit says ${setting:%s}, and nothing sets %q for it — an operator's "+ + "value is the assignment's, never the definition's (novox/hq ADR 0112): "+ + "`settings set %s ` with {%q: …}%s", + module, key, key, module, key, orNoSettings(layers)) + } + v := plainly(value) + if !unitPart.MatchString(v) { + return fmt.Errorf("%s: the setting %q is %q, which cannot be the instance of the unit %s: an instance "+ + "takes letters, digits, ':', '_', '.' and '-', does not begin with '-' (a systemctl option), and is "+ + "at most 64 characters", module, key, v, unit) + } + unit = strings.ReplaceAll(unit, "${setting:"+key+"}", v) + } + resource["unit"] = unit + return nil +} + func settingValue(layers []Layer, key string) (any, bool) { var value any set := false @@ -106,11 +170,15 @@ func settingKeysUsedBy(m Manifest) map[string]bool { } } for _, r := range m.Resources { - if fmt.Sprint(r["type"]) != "file" { - continue - } - if content, ok := r["content"].(string); ok { - note(content) + switch fmt.Sprint(r["type"]) { + case "file": + if content, ok := r["content"].(string); ok { + note(content) + } + case "service": + if unit, ok := r["unit"].(string); ok { + note(unit) + } } } inValues := func(values map[string]any) {