Merge pull request 'Fill a module's files one by one, so a known secret's file is not held back by a template that waits (issue 1553)' (#398) from fix/1553-fill-file-by-file into main

modules/mesh-provisioner-client/README.md

@@ -43,8 +43,9 @@ and the placement values its templates name — the client:
   `${environment:posix}` is `environment:posix`, `${contribution:node-hotkeys:trigger}` is
   `contribution:node-hotkeys:trigger`. A collector naming a collection its plan does not fails. A collection with no
   item is filled empty, and filled again whenever an item of it is recorded or goes;
-- fills a module's files only once every value is there; otherwise it writes nothing and reports `waiting` with what
-  it waits for;
+- fills a file only once every value of it is there, file by file (issue 1553): a file with a value that waits stays
+  as it is, the others are filled, and it reports `waiting` with every value it waits for and the files it filled;
+  `applied` only when every file is filled (a collection's items are recorded only when nothing of the module waits);
 - has the node-engine write each changed file through its local write socket (`/run/nox-mesh-host/fill.sock`,
   mesh-host `internal/filling`), which writes it as the declaration says (an `owned` file whole, a `seed` only when
   absent, a `managed` file's own keys or block) and restarts or reloads its program as the declaration says;

modules/mesh-provisioner-client/cmd/mesh-provisioner-client/client.go

@@ -639,27 +639,32 @@ func (c *Client) reconcile(ctx context.Context, kvs map[string]jetstream.KeyValu
 		c.report(ctx, kvs, s, r)
 		return
 	}
-	if len(r.WaitsFor) > 0 {
-		// Every value or none: the files there are stay as they are.
-		r.Status = StatusWaiting
-		c.report(ctx, kvs, s, r)
-		return
-	}
-	// The items first: recorded, they fill the collectors, this module's own among them.
-	for _, it := range items {
-		if err := c.record(ctx, kvs, s, module, p, it.PlannedItem, render(it.text, values)); err != nil {
-			failed = append(failed, fmt.Sprintf("item %s: %v", it.key(), err))
-			continue
+	waiting := len(r.WaitsFor) > 0
+	// The items keep the rule of every value or none (issue 1553): a collection is recorded only once nothing of the
+	// module waits, so a collector never reads an item made while another value was missing.
+	if !waiting {
+		// The items first: recorded, they fill the collectors, this module's own among them.
+		for _, it := range items {
+			if err := c.record(ctx, kvs, s, module, p, it.PlannedItem, render(it.text, values)); err != nil {
+				failed = append(failed, fmt.Sprintf("item %s: %v", it.key(), err))
+				continue
+			}
+			r.Items = append(r.Items, it.key())
 		}
-		r.Items = append(r.Items, it.key())
 	}
-	keep := map[string]bool{}
-	for _, it := range p.Items {
-		keep[it.key()] = true
+	if !waiting {
+		keep := map[string]bool{}
+		for _, it := range p.Items {
+			keep[it.key()] = true
+		}
+		c.forgetItems(ctx, kvs, s, module, keep)
 	}
-	c.forgetItems(ctx, kvs, s, module, keep)
 	moved := false
 	for _, f := range targets {
+		// File by file (issue 1553, ADR 0349 clause 4): a target with a value that waits keeps the file there is.
+		if !allKnown(f.text, values) {
+			continue
+		}
 		content := []byte(render(byFile[f.Path], values))
 		fa := FileApplied{Path: f.Path, Takes: f.Takes}
 		a, err := c.Fill.Fill(ctx, f.Path, content)
@@ -677,9 +682,12 @@ func (c *Client) reconcile(ctx context.Context, kvs map[string]jetstream.KeyValu
 		}
 		r.Files = append(r.Files, fa)
 	}
-	if len(failed) > 0 {
+	switch {
+	case len(failed) > 0:
 		r.Status, r.Reason = StatusFailed, strings.Join(failed, "; ")
-	} else {
+	case waiting:
+		r.Status = StatusWaiting
+	default:
 		r.Status = StatusApplied
 	}
 	if moved {
@@ -689,6 +697,16 @@ func (c *Client) reconcile(ctx context.Context, kvs map[string]jetstream.KeyValu
 	c.report(ctx, kvs, s, r)
 }
 
+// allKnown says whether every value a text needs is known (issue 1553).
+func allKnown(text string, values map[Need]string) bool {
+	for _, n := range needsOf(text) {
+		if _, ok := values[n]; !ok {
+			return false
+		}
+	}
+	return true
+}
+
 // valueOf is a need's value, or what it waits for, or why it cannot be filled.
 func (c *Client) valueOf(ctx context.Context, s *state, module string, p Plan, n Need) (value, wait, fail string) {
 	answer, known := s.registry[n.Kind]

modules/mesh-provisioner-client/cmd/mesh-provisioner-client/secrets_test.go

@@ -253,9 +253,8 @@ func TestNatsASecretTheVaultDoesNotHoldYetIsWaitedFor(t *testing.T) {
 	if _, asked := e.counts(appConf); asked != 0 {
 		t.Error("a file waiting for its secret was written")
 	}
-	if _, asked := e.counts(appDB); asked != 0 {
-		t.Error("a module waiting for one value had another file written: every value or none")
-	}
+	// 1553: a file whose own values are all there is filled, though another file of the module waits.
+	eventually(t, "the secret's own file is filled while the template waits", func() bool { return e.read(t, appDB) == "pw" })
 	vault.keep("own/g14/app/app-token", "arrived", true)
 	eventually(t, "it is filled once the vault holds it", func() bool {
 		return strings.Contains(e.read(t, appConf), "token = arrived\n") && e.read(t, appDB) == "pw"
@@ -516,3 +515,50 @@ func TestAConsumersLoginIsNamedWithItsLocalName(t *testing.T) {
 		t.Errorf("a login under a local name: %s", got)
 	}
 }
+
+// 1553/AC1: a file holding one known secret is filled although a template of the module waits for a bound value;
+// the template's file is not filled, and the report says waiting, names the value, and lists the file it filled.
+func TestNatsAKnownSecretsFileIsFilledWhileATemplateWaits1553(t *testing.T) {
+	b := startBus(t)
+	heard := b.listen(t)
+	vault := startVault(t, b)
+	vault.keep("own/g14/app/app-token", "tok", false)
+	vault.keep("own/g14/app/db-password", "pw", false)
+	e := startEngine(t, map[string]string{appConf: "owned", appDB: "owned"})
+	// No config font-size: the template waits for it.
+	b.assign(t, "app", app(t))
+	aClient(t, b, e, t.TempDir())
+
+	eventually(t, "the secret's file is filled", func() bool { return e.read(t, appDB) == "pw" })
+	eventually(t, "prov.applied says waiting for the value, with the file it filled", func() bool {
+		r, _ := heard.last("app")
+		return r.Status == StatusWaiting && len(r.WaitsFor) == 1 && strings.HasPrefix(r.WaitsFor[0], "config:font-size") &&
+			len(r.Files) == 1 && r.Files[0].Path == appDB
+	})
+	if _, asked := e.counts(appConf); asked != 0 {
+		t.Error("the template waiting for a value was filled")
+	}
+}
+
+// 1553/AC3: a template one of whose values waits keeps its file as it is: Fill is never called for it, so no
+// half-filled text is written, though its other value (the secret) is known.
+func TestNatsATemplateWithAWaitingValueKeepsItsFileAsItIs1553(t *testing.T) {
+	b := startBus(t)
+	heard := b.listen(t)
+	vault := startVault(t, b)
+	vault.keep("own/g14/app/app-token", "tok", false)
+	vault.keep("own/g14/app/db-password", "pw", false)
+	e := startEngine(t, map[string]string{appConf: "owned", appDB: "owned"})
+	b.assign(t, "app", app(t))
+	aClient(t, b, e, t.TempDir())
+
+	eventually(t, "it waits", func() bool { r, _ := heard.last("app"); return r.Status == StatusWaiting })
+	eventually(t, "the other file is filled, so the pass is over", func() bool { return e.read(t, appDB) == "pw" })
+	time.Sleep(200 * time.Millisecond)
+	if _, asked := e.counts(appConf); asked != 0 {
+		t.Errorf("Fill was called %d times for a template with a waiting value", asked)
+	}
+	if got := e.read(t, appConf); got != "" {
+		t.Errorf("the template's file holds %q, want it untouched", got)
+	}
+}