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

@@ -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 {

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)
+	}
+}

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)
 	}