From eb5f9d2f531bc8414d24aff80a422bf1492915fd Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 01:51:37 +0200 Subject: [PATCH] Allow a setting in a unit's name only as a template's whole instance, never leading with a dash, at most 64 characters Third review of mesh-catalog #147. A setting anywhere else in a unit's name could make the unit another unit or another kind; it is now refused where the manifest is read. A value beginning with '-' would be read by systemctl as an option, and a long one is no pool's name. --- internal/catalogue/manifest.go | 1 + internal/catalogue/setting_in_unit_test.go | 50 ++++++++++++++++++++++ internal/catalogue/setting_into.go | 33 ++++++++++++-- 3 files changed, 81 insertions(+), 3 deletions(-) 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 index bac0d0ad..77197b72 100644 --- a/internal/catalogue/setting_in_unit_test.go +++ b/internal/catalogue/setting_in_unit_test.go @@ -107,3 +107,53 @@ func TestASettingAUnitAsksForIsNotStrayAndIsJudged(t *testing.T) { 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 05ea8872..9ee165b7 100644 --- a/internal/catalogue/setting_into.go +++ b/internal/catalogue/setting_into.go @@ -74,7 +74,33 @@ func settingInto(resource map[string]any, layers []Layer, module string) error { // 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:_.-]+$`) +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 @@ -96,8 +122,9 @@ func settingIntoUnit(resource map[string]any, layers []Layer, module string) err } v := plainly(value) if !unitPart.MatchString(v) { - return fmt.Errorf("%s: the setting %q is %q, which cannot be part of the unit %s: a unit's name takes "+ - "letters, digits, ':', '_', '.' and '-' only", module, key, v, unit) + 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) }