From 525b2c1a11d2890a2e1f877e9a006d07b40a7e2d Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 02:15:37 +0200 Subject: [PATCH] Sweep the definition, not the filled values, and judge each field alike at check and composition (issue 231) The first sweep ran at composition over the resource after settings, bindings and machine facts were filled, so an operator's value such as ${labels:instance} was refused as a misspelling its author never wrote, and the whole machine got nothing. Both points now sweep the resource as the manifest wrote it, against one table of which fields each pass fills, so the check and composition refuse the same things. Any ${: its pattern does not take is refused whatever its case or key. Issue 231's undeclared-setting half is not met here: refusing a key the module does not declare (ADR 0164) is not built and is handed to issue 398. --- internal/catalogue/declaration.go | 9 + internal/catalogue/environment_into.go | 14 +- internal/catalogue/unconsumed_placeholder.go | 170 +++++++++++++----- .../catalogue/unconsumed_placeholder_test.go | 65 ++++++- 4 files changed, 195 insertions(+), 63 deletions(-) diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 4b191182..ff5be865 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -911,6 +911,15 @@ func (r Resolution) compose(with Rendering, owner map[string]string, return nil, err } } + // **And every placeholder no pass fills where it stands is refused** (novox/hq issue 231): + // judged here, over the definition as written and before any pass, by the function the + // catalogue check runs — so a manifest registered by an older binary is judged too, and + // what a setting or a binding later puts into a file is its software's text, never swept. + for _, own := range m.Resources { + if problems := unconsumedPlaceholders(m.Module, own); len(problems) > 0 { + return nil, fmt.Errorf("%s", problems[0]) + } + } // Which of this module's files carry a secret, for the rule that a container may not read // one of them as its environment without saying so (ADR 0086, issue 041). diff --git a/internal/catalogue/environment_into.go b/internal/catalogue/environment_into.go index e5cb3d9b..ed2bbe28 100644 --- a/internal/catalogue/environment_into.go +++ b/internal/catalogue/environment_into.go @@ -226,9 +226,9 @@ func (m Manifest) contributionPlaceholderProblems() []string { for _, r := range m.Resources { problems = append(problems, placeholderProblems(m, r)...) problems = append(problems, seatPlaceholderProblems(m, r)...) - // And whatever is left once every pass's own placeholders are set aside: a misspelt - // namespace, or a key its namespace cannot take (novox/hq issue 231). - problems = append(problems, unconsumedPlaceholders(m.Module, r, filledByAPass, nil)...) + // And every placeholder no pass fills where it stands: a misspelt namespace, a key its + // namespace cannot take, or a field its pass does not read (novox/hq issue 231). + problems = append(problems, unconsumedPlaceholders(m.Module, r)...) } return problems } @@ -551,13 +551,7 @@ func shellCode(modules []Manifest, shell, slot string) string { // is filled before the shell's code is, and each is replaced in a single pass over what the holder // wrote, so a contributed piece is never scanned again. func contributionsInto(resource map[string]any, m Manifest, modules []Manifest, facts map[string]string, with Rendering, caps map[string]bool, unplaced *[]string) error { - // **The sweep stands here, after every other pass and before any contributed text is placed** - // (novox/hq issue 231): what is left that the mesh did not fill would reach the machine as text, - // and contributed shell code, placed below, is the shell's and is never swept (ADR 0204). The - // same function the catalogue check runs, over what the passes left. - problems := append(placeholderProblems(m, resource), seatPlaceholderProblems(m, resource)...) - problems = append(problems, unconsumedPlaceholders(m.Module, resource, nil, leftForLater)...) - if len(problems) > 0 { + if problems := append(placeholderProblems(m, resource), seatPlaceholderProblems(m, resource)...); len(problems) > 0 { return fmt.Errorf("%s", problems[0]) } content, ok := resource["content"].(string) diff --git a/internal/catalogue/unconsumed_placeholder.go b/internal/catalogue/unconsumed_placeholder.go index 556096c4..32dc95e7 100644 --- a/internal/catalogue/unconsumed_placeholder.go +++ b/internal/catalogue/unconsumed_placeholder.go @@ -8,85 +8,161 @@ import ( // A placeholder no pass consumes is refused, never written out as text (novox/hq issue 231). // -// Each namespace is filled by its own pass with its own pattern, and a word in that shape that no -// pattern matches — `${machnie:address}`, `${shel:zsh:first}`, a setting key no definition could -// declare such as `${setting:Undeclared}` — was left in the file as it was written, and reached a -// machine as a value. That is the predecessor's failure the namespaced placeholders were meant to end -// (novox/hq ADR 0164). So after every pass, what is left is swept: a `${:}` whose word is -// a lower-case token and whose key begins with a letter or a digit is the mesh's shape, and nothing -// the mesh could still fill. +// Each namespace is filled by its own pass with its own pattern, in the fields that pass reads. A +// word in that shape that no pattern matched — `${machnie:address}`, `${shel:zsh:first}`, a setting +// key no definition could declare such as `${setting:Undeclared}` — was left in the file as it was +// written and reached a machine as a value: the predecessor's failure the namespaced placeholders +// were meant to end (novox/hq ADR 0164). So the definition is swept, field by field, against the +// same table of what each pass fills where. +// +// **The definition, never the values.** Swept as the manifest wrote it, at the catalogue check and +// again at composition before any pass has run: what an operator's setting, a binding or a merged +// JSON setting puts into a file is its own software's text — `${env:HOME}` for Log4j, `${timeout:30}` +// for Spring — and refusing it there would refuse something its author never wrote, on a machine the +// check had passed (novox/hq issue 231, review of mesh-controller #211). +// +// What is refused: +// - a placeholder of a namespace the mesh knows, in a field that namespace's pass does not read: it +// would reach the machine as the same text; +// - any other `${:`, case-insensitively and whatever follows — `${Machine:address}`, +// `${machine:.address}`, `${machine: address}`, an unclosed `${machine:address` — because no +// pass's pattern takes it; +// - a namespace-shaped placeholder of a word the mesh does not know: a lower-case word, a colon, and +// a key that begins with a letter or a digit and holds no space, brace or `$`. // // **The shell's own syntax is not that shape, and passes.** `${NAME:-…}` has an upper-case word, // `${(%):-…}` and `${1:-.}` begin with no letter, and a lower-case variable with an operator after its -// colon — `${count:-}`, `${trial:+…}`, `${state:=…}` — has no key that begins with a letter or a -// digit; nor does an expansion that holds a space, a brace or a `$`. What the shape does catch is a -// zsh modifier (`${path:t}`) or a substring (`${where:0:12}`) in a resource's own text: shell code of -// that kind belongs in the module's contributed shell code, which no pass reads (novox/hq ADR 0204) -// and which this sweep never sees, because it runs before that code is placed. +// colon — `${count:-}`, `${trial:+…}`, `${state:=…}` — has no key that begins with a letter or a digit. +// What the shape does catch is a zsh modifier (`${path:t}`) or a substring (`${where:0:12}`) in a +// resource's own text: shell code of that kind belongs in the module's contributed shell code, which +// is not a resource and is never swept (novox/hq ADR 0204). + +// filler is one namespace's pass: the patterns it fills with, and the fields it reads them in, by +// resource type ("*" for any type). +type filler struct { + namespace string + patterns []*regexp.Regexp + fields map[string][]string + // why, when set, is the rule that keeps the namespace out of every other field, said with a + // refusal of one written there. + why string +} + +// fillers is the table of what each pass fills where — read from the passes themselves: settingInto, +// dirInto, accessInto, intoFile (whose ${secret:…} the node-engine fills), boundInto, portInto, +// seatInto, machineInto and contributionsInto. A pass that comes to read another field adds it here, +// or the sweep refuses the placeholder it would have filled. +var fillers = []filler{ + {namespace: "setting", patterns: []*regexp.Regexp{settingRef}, fields: map[string][]string{"file": {"content"}, "service": {"unit"}}}, + {namespace: "dir", patterns: []*regexp.Regexp{dirRef}, fields: map[string][]string{"*": {"path", "content", "volumes", "env", "env-file"}}}, + {namespace: "access", patterns: []*regexp.Regexp{accessRef}, fields: map[string][]string{"*": {"path", "content", "volumes", "env", "env-file"}}}, + {namespace: "secret", patterns: []*regexp.Regexp{placeholder}, fields: map[string][]string{"file": {"content"}}, + why: "a secret is never filled into an environment variable or any other field: put it in a file the " + + "module declares and mount that (novox/hq ADR 0086)"}, + {namespace: "bound", patterns: []*regexp.Regexp{bound}, fields: map[string][]string{"file": {"content"}}}, + {namespace: "port", patterns: []*regexp.Regexp{ofPort}, fields: map[string][]string{"file": {"content"}, "container": {"env"}, "process": {"env"}}}, + {namespace: "seat", patterns: []*regexp.Regexp{ofSeat, ofSeatReach}, fields: map[string][]string{"file": {"content"}, "container": {"env"}, "process": {"env"}}}, + {namespace: "machine", patterns: []*regexp.Regexp{ofMachine}, fields: map[string][]string{"*": {"path", "owner", "content", "name", "user", "root", "home"}}}, + // Placed in a file's content by their holders, and judged there by their own rules + // (placeholderProblems, seatPlaceholderProblems). + {namespace: "environment", patterns: []*regexp.Regexp{ofEnvironment}, fields: map[string][]string{"*": {"content"}}}, + {namespace: "shell", patterns: []*regexp.Regexp{ofShell}, fields: map[string][]string{"*": {"content"}}}, + {namespace: "contribution", patterns: []*regexp.Regexp{ofContribution}, fields: map[string][]string{"*": {"content"}}}, + // Filled in what a provider serves (consumer_into_serves.go), never in a resource. + {namespace: "consumer", patterns: []*regexp.Regexp{consumerFact}}, +} + +// reads is whether this pass fills a field of a resource of this type. +func (f filler) reads(kind, field string) bool { + return oneOf(f.fields[kind], field) || oneOf(f.fields["*"], field) +} + +// ofKnownNamespace is any `${:` and what follows it up to its brace or the end of +// its line, whatever its case: what remains of one once every pass's pattern is set aside is a +// misspelling. +var ofKnownNamespace = regexp.MustCompile(`(?i)\$\{(?:` + strings.Join(namespacesOf(fillers), "|") + `):[^}\n]*\}?`) // namespaceShaped is a placeholder in the mesh's shape: a lower-case word, a colon, and a key that // begins with a letter or a digit and holds no space, brace or `$`. var namespaceShaped = regexp.MustCompile(`\$\{[a-z][a-z0-9_-]*:[A-Za-z0-9][^\s{}$]*\}`) -// filledByAPass is every placeholder a pass over a resource consumes, each by the pattern that pass -// fills with. Judged at the catalogue check, before any of them has run. -var filledByAPass = []*regexp.Regexp{ - settingRef, dirRef, accessRef, placeholder, bound, ofPort, ofSeat, ofSeatReach, ofMachine, - ofEnvironment, ofShell, ofContribution, +func namespacesOf(fs []filler) []string { + out := make([]string, len(fs)) + for i, f := range fs { + out[i] = f.namespace + } + return out } -// leftForLater is what composition leaves in a file's content when the sweep runs, on purpose: a -// secret the node-engine fills from what it alone decrypts (ADR 0086), and the environment, the -// shell's code and the seats' contributions, which are placed after the sweep so that contributed -// text is never swept (ADR 0203, ADR 0204, ADR 0212). Each is judged by its own rules -// (placeholderProblems, seatPlaceholderProblems); anything else still standing was consumed by no -// pass, in whatever field. -var leftForLater = []*regexp.Regexp{placeholder, ofEnvironment, ofShell, ofContribution} - -// unconsumedPlaceholders is every namespace-shaped placeholder in one resource that none of the -// given patterns accounts for, named with the module, the resource and the field it stands in. -// `anywhere` are accounted for in any field; `inContent` only in a file's content. -func unconsumedPlaceholders(module string, r map[string]any, anywhere, inContent []*regexp.Regexp) []string { +// unconsumedPlaceholders is every placeholder in one resource of a definition that no pass fills +// where it stands, each named with the module, the resource, the field and the token. The same +// function at the catalogue check and at composition, over the resource as the manifest wrote it. +func unconsumedPlaceholders(module string, r map[string]any) []string { + kind := fmt.Sprint(r["type"]) var problems []string - var sweep func(field string, v any) - sweep = func(field string, v any) { + var sweep func(top, field string, v any) + sweep = func(top, field string, v any) { switch v := v.(type) { case string: rest := v - for _, p := range anywhere { - rest = p.ReplaceAllString(rest, "") - } - if field == "content" { - for _, p := range inContent { + for _, f := range fillers { + for _, p := range f.patterns { + if !f.reads(kind, top) { + for _, token := range p.FindAllString(rest, -1) { + problems = append(problems, fmt.Sprintf( + "%s's resource %v holds %s in its %s, and ${%s:…} is not filled in this field: "+ + "it would reach the machine as that text (novox/hq issue 231)%s", + module, r["id"], token, field, f.namespace, whereFilled(f))) + } + } rest = p.ReplaceAllString(rest, "") } } + for _, token := range ofKnownNamespace.FindAllString(rest, -1) { + problems = append(problems, fmt.Sprintf( + "%s's resource %v holds %s in its %s, which is no placeholder the mesh fills: a "+ + "misspelt key, or a namespace in the wrong case, would reach the machine as that "+ + "text (novox/hq issue 231)", module, r["id"], token, field)) + } + rest = ofKnownNamespace.ReplaceAllString(rest, "") for _, token := range namespaceShaped.FindAllString(rest, -1) { problems = append(problems, fmt.Sprintf( "%s's resource %v holds %s in its %s, and no pass of the mesh fills it: a misspelt "+ - "placeholder, or a key its namespace cannot take, would reach the machine as that "+ - "text (novox/hq issue 231). The mesh fills %s; the shell's own syntax belongs in "+ - "the module's shell code (ADR 0204)", - module, r["id"], token, field, strings.Join(placeholderNamespaces, ", "))) + "placeholder would reach the machine as that text (novox/hq issue 231). The mesh "+ + "fills ${%s:…}; the shell's own syntax belongs in the module's shell code (ADR 0204)", + module, r["id"], token, field, strings.Join(namespacesOf(fillers), ":…}, ${"))) } case []any: for i, e := range v { - sweep(fmt.Sprintf("%s[%d]", field, i), e) + sweep(top, fmt.Sprintf("%s[%d]", field, i), e) } case map[string]any: for _, k := range sortedKeys(v) { - sweep(field+"."+k, v[k]) + sweep(top, field+"."+k, v[k]) } } } for _, k := range sortedKeys(r) { - sweep(k, r[k]) + sweep(k, k, r[k]) } return problems } -// placeholderNamespaces is what a refusal lists as the namespaces the mesh fills in a resource. -var placeholderNamespaces = []string{ - "${access:…}", "${bound:…}", "${contribution:…}", "${dir:…}", "${environment:…}", "${machine:…}", - "${port:…}", "${seat:…}", "${secret:…}", "${setting:…}", "${shell:…}", +// whereFilled says where a namespace's pass does fill, for a refusal of one written elsewhere. +func whereFilled(f filler) string { + if f.why != "" { + return ". " + strings.ToUpper(f.why[:1]) + f.why[1:] + } + if len(f.fields) == 0 { + return "; it is filled only in what a provider serves" + } + var where []string + for _, kind := range sortedKeys(f.fields) { + of := "a " + kind + "'s" + if kind == "*" { + of = "any resource's" + } + where = append(where, of+" "+strings.Join(f.fields[kind], ", ")) + } + return "; it is filled in " + strings.Join(where, "; ") } diff --git a/internal/catalogue/unconsumed_placeholder_test.go b/internal/catalogue/unconsumed_placeholder_test.go index 4eb6ca2c..fdc34d4d 100644 --- a/internal/catalogue/unconsumed_placeholder_test.go +++ b/internal/catalogue/unconsumed_placeholder_test.go @@ -36,6 +36,22 @@ func TestAMisspelledPlaceholderIsRefusedAtTheCheck(t *testing.T) { t.Errorf("the shell's own syntax was refused: %v", err) } + // A namespace the mesh knows, misspelt in any way its own pattern does not take, and the same + // namespace in a field its pass does not read: refused at the check as at composition. + for _, c := range []struct{ resource, token, field string }{ + {`{"id":"rc","type":"file","path":"/etc/rc","content":"${Machine:address}"}`, "${Machine:address}", "content"}, + {`{"id":"rc","type":"file","path":"/etc/rc","content":"${machine:.address}"}`, "${machine:.address}", "content"}, + {`{"id":"rc","type":"file","path":"/etc/rc","content":"${machine: address}"}`, "${machine: address}", "content"}, + {`{"id":"rc","type":"file","path":"/etc/rc","content":"a=${machine:address\nb=1"}`, "${machine:address", "content"}, + {`{"id":"rc","type":"process","name":"speller","env":{"NAME":"${machine:name}"}}`, "${machine:name}", "env.NAME"}, + } { + raw := `{"module":"speller","version":"1","resources":[` + c.resource + `]}` + _, err := ParseManifest([]byte(raw)) + if err == nil || !strings.Contains(err.Error(), "speller's resource rc holds "+c.token+" in its "+c.field) { + t.Errorf("%s in its %s was accepted at the check, or not named: %v", c.token, c.field, err) + } + } + // And the shell's syntax alone, beside placeholders every pass knows, is accepted — as is the // shape a lower-case shell variable takes with an operator after its colon. raw = `{"module":"speller","version":"1","resources":[{"id":"rc","type":"file","path":"${machine:account-home}/.rc",` + @@ -58,6 +74,10 @@ func TestAMisspelledPlaceholderIsRefusedAtComposition(t *testing.T) { {"content", map[string]any{"id": "rc", "type": "file", "path": "/etc/speller.rc", "content": "b=" + undeclaredSetting + "\n"}, undeclaredSetting}, {"content", map[string]any{"id": "rc", "type": "file", "path": "/etc/speller.rc", "content": "c=" + misspelledMachine + "\n"}, misspelledMachine}, {"env.NAME", map[string]any{"id": "rc", "type": "process", "name": "speller", "env": map[string]any{"NAME": "${machine:name}"}}, "${machine:name}"}, + {"content", map[string]any{"id": "rc", "type": "file", "path": "/etc/speller.rc", "content": "${Machine:address}"}, "${Machine:address}"}, + {"content", map[string]any{"id": "rc", "type": "file", "path": "/etc/speller.rc", "content": "${machine:.address}"}, "${machine:.address}"}, + {"content", map[string]any{"id": "rc", "type": "file", "path": "/etc/speller.rc", "content": "${machine: address}"}, "${machine: address}"}, + {"content", map[string]any{"id": "rc", "type": "file", "path": "/etc/speller.rc", "content": "a=${machine:address\nb=1"}, "${machine:address"}, } { m := Manifest{Module: "speller", Version: "1", Resources: []map[string]any{c.r}} r := Resolution{Node: "workstation", Account: "op", Modules: []Manifest{m}} @@ -74,11 +94,46 @@ func TestAMisspelledPlaceholderIsRefusedAtComposition(t *testing.T) { if err != nil { t.Fatalf("the shell's own syntax was refused: %v", err) } + if got := contentOf(t, out, "speller.rc"); got != "d="+shellsOwn+"\n" { + t.Fatalf("the shell's own syntax did not pass through as written: %q", got) + } +} + +// contentOf is the content of the composed resource with this id, failing when it was not composed. +func contentOf(t *testing.T, out []map[string]any, id string) string { + t.Helper() for _, res := range out { - if res["id"] == "speller.rc" && res["content"] != "d="+shellsOwn+"\n" { - t.Fatalf("the shell's own syntax did not pass through as written: %q", res["content"]) + if res["id"] == id { + return plainly(res["content"]) } } + t.Fatalf("%s was not composed: %v", id, out) + return "" +} + +// What a value puts into a file is its software's text, not the definition's, and is never swept +// (review of mesh-controller #211): an operator's setting holding `${labels:instance}` filled through +// ${setting:…}, and a JSON setting `${level:upper}` merged into a mergeable file, reach the machine as +// set — software that templates its own configuration (Log4j's `${env:…}`, Spring's `${timeout:30}`) +// is configured exactly this way. +func TestAValueHoldingAPlaceholderShapeComposesUnchanged(t *testing.T) { + m := Manifest{Module: "templater", Version: "1", Resources: []map[string]any{ + {"id": "conf", "type": "file", "path": "/etc/templater.conf", "content": "template=${setting:template}\n"}, + {"id": "json", "type": "file", "path": "/etc/templater.json", "merge": MergeJSON, "content": `{"fmt":"plain"}`}, + }} + settings := SettingsBy{"templater": {{From: "the operator", Values: map[string]any{ + "template": "${labels:instance}", "fmt": "${level:upper}", + }}}} + out, err := Resolution{Node: "workstation", Account: "op", Modules: []Manifest{m}}.Declaration(Rendering{Settings: settings}) + if err != nil { + t.Fatalf("a value holding a placeholder's shape was refused as the definition's: %v", err) + } + if got := contentOf(t, out, "templater.conf"); got != "template=${labels:instance}\n" { + t.Errorf("the setting did not reach the file as set: %q", got) + } + if got := contentOf(t, out, "templater.json"); !strings.Contains(got, `${level:upper}`) { + t.Errorf("the merged setting did not reach the file as set: %q", got) + } } // Contributed shell code is the shell's, and no pass reads it (novox/hq ADR 0204): zsh's own @@ -91,9 +146,7 @@ func TestContributedShellCodeIsNotSwept(t *testing.T) { if err != nil { t.Fatalf("contributed shell code was swept: %v", err) } - for _, res := range out { - if res["id"] == "zsh.zshrc" && !strings.Contains(res["content"].(string), "echo ${path:t} ${shel:zsh:first}") { - t.Fatalf("the contributed code did not reach the holder's file as written: %q", res["content"]) - } + if got := contentOf(t, out, "zsh.zshrc"); !strings.Contains(got, "echo ${path:t} ${shel:zsh:first}") { + t.Fatalf("the contributed code did not reach the holder's file as written: %q", got) } }