Merge pull request 'fix(boot-test): exactly what a new server gets, and pinpoint settings it can't start without' (#20) from feat/builder-pinpoint-empty-settings into main

AUTHORING.md

@@ -34,6 +34,8 @@ Know this before writing anything:
   **Never put a placeholder or default password in a template.** Shared defaults are readable by anyone
   from the public game list.
 - Join passwords stay `""`, with `"x-savedValue": {"key": "GAME_JOIN_PASSWORD", "label": "Game join password", "secret": true}`.
+  If the server can't start without one, give it `"x-generate": {"type": "password"}` as well, so every new server
+  gets its own. Boot tests run with exactly what a new server gets: empty settings stay empty.
 - No real credentials, tokens or IDs anywhere in this repo. A value a server can't start without (a
   player ID, a license token) goes in `test.yaml` under `secrets`, mapped to a CI secret name.
 

cmd/gbrgames/boottest.go

@@ -32,8 +32,9 @@ func runBootTestPayload(args []string) error {
 	fs := flag.NewFlagSet("boottest-payload", flag.ExitOnError)
 	gamesDir := fs.String("games", "games", "directory holding one folder per game")
 	out := fs.String("out", "", "file to write (default stdout)")
-	asDeployed := fs.Bool("as-deployed", false, "settings exactly as a new server gets them: only x-generate fields filled, "+
-		"no throwaway passwords (the AI builder boots drafts this way)")
+	fillBlank := fs.Bool("fill-blank-passwords", false, "also give blank password settings a throwaway value. Never for a "+
+		"boot test: a real server doesn't get one. Only the AI builder uses it, to find out whether an empty setting is "+
+		"why a draft fails")
 	fs.Parse(args)
 	if fs.NArg() != 1 {
 		return errors.New("boottest-payload needs a game id")
@@ -73,12 +74,12 @@ func runBootTestPayload(args []string) error {
 			config[key] = value
 		}
 	}
-	// Like the panel fills blank x-generate fields on deploy; other blank passwords (which an owner
-	// would type in, and some games refuse to start without) get a throwaway value too, except
-	// -as-deployed: a draft must start with exactly what a new server gets
+	// Exactly what a new server gets: the panel fills blank x-generate fields on deploy and nothing else, so
+	// the boot test does too. Filling other blank passwords hid templates whose server won't start without
+	// one: they passed CI and failed on the first real deploy.
 	for key, p := range schemaProperties(g) {
 		prop, _ := p.(map[string]any)
-		if prop["x-generate"] == nil && (*asDeployed || !passwordKey.MatchString(key)) {
+		if prop["x-generate"] == nil && (!*fillBlank || !passwordKey.MatchString(key)) {
 			continue
 		}
 		// Only missing or empty text: ports, booleans and other values are never touched

cmd/gbrgames/boottest_test.go

@@ -8,7 +8,7 @@ import (
 	"testing"
 )
 
-func TestBootTestPayloadOnlyFillsBlankPasswords(t *testing.T) {
+func TestBootTestPayloadIsWhatANewServerGets(t *testing.T) {
 	dir := t.TempDir()
 	game := filepath.Join(dir, "games", "demo")
 	if err := os.MkdirAll(game, 0o755); err != nil {
@@ -52,10 +52,12 @@ configSchema:
 	if c["ENABLE_RCON"] != true {
 		t.Errorf("ENABLE_RCON changed to %v", c["ENABLE_RCON"])
 	}
-	for _, k := range []string{"RCON_PASSWORD", "ServerPassword"} {
-		if s, _ := c[k].(string); !strings.HasPrefix(s, "boot-") {
-			t.Errorf("%s not filled: %v", k, c[k])
+	// Exactly what a new server gets: the x-generate password is made, a blank join password stays blank
+	if s, _ := c["RCON_PASSWORD"].(string); !strings.HasPrefix(s, "boot-") {
+		t.Errorf("x-generate RCON_PASSWORD not filled: %v", c["RCON_PASSWORD"])
 	}
+	if c["ServerPassword"] != "" {
+		t.Errorf("ServerPassword must stay empty, as for a new server: %v", c["ServerPassword"])
 	}
 	if c["AdminPassword"] != "kept-by-owner" {
 		t.Errorf("AdminPassword overwritten: %v", c["AdminPassword"])
@@ -64,8 +66,8 @@ configSchema:
 		t.Errorf("placeholders not resolved: %v", c["MOTD"])
 	}
 
-	// As a new server gets it (what the AI builder boots): only x-generate is filled, a blank join password stays blank
-	if err := runBootTestPayload([]string{"-as-deployed", "-games", filepath.Join(dir, "games"), "-out", out, "demo"}); err != nil {
+	// Only for the builder's diagnosis: blank passwords filled too
+	if err := runBootTestPayload([]string{"-fill-blank-passwords", "-games", filepath.Join(dir, "games"), "-out", out, "demo"}); err != nil {
 		t.Fatal(err)
 	}
 	raw, _ = os.ReadFile(out)
@@ -73,10 +75,7 @@ configSchema:
 	if err := json.Unmarshal(raw, &payload); err != nil {
 		t.Fatal(err)
 	}
-	if s, _ := payload.GameConfig["RCON_PASSWORD"].(string); !strings.HasPrefix(s, "boot-") {
-		t.Errorf("as deployed: x-generate RCON_PASSWORD not filled: %v", payload.GameConfig["RCON_PASSWORD"])
-	}
-	if payload.GameConfig["ServerPassword"] != "" {
-		t.Errorf("as deployed: ServerPassword should stay empty, got %v", payload.GameConfig["ServerPassword"])
+	if s, _ := payload.GameConfig["ServerPassword"].(string); !strings.HasPrefix(s, "boot-") {
+		t.Errorf("-fill-blank-passwords: ServerPassword not filled: %v", payload.GameConfig["ServerPassword"])
 	}
 }

cmd/gbrgames/build.go

@@ -9,6 +9,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"sort"
 	"strings"
 )
 
@@ -232,46 +233,26 @@ func firstBoot(name, image, steamApp string, steam bool, startup string) (*bootR
 // bootCheck deploys the template with the node-agent's BootTest, the same check CI runs, and when the
 // game wrote files outside its volume on the first boot, boots it once more to see that it no longer does.
 func bootCheck(gamesDir, id, agentImage string, wantOutside bool) (problems, verified []string, err error) {
-	self, err := os.Executable()
-	if err != nil {
-		return nil, nil, err
-	}
-	// A boot test that was killed (worker restarted mid-build) leaves its container and volume behind,
-	// and the next deploy under the same name would trip over them
-	runCmd("docker", "rm", "-f", "boottest-"+id)
-	runCmd("docker", "volume", "rm", "-f", "boottest-"+id+"-data")
-	payloadCmd := exec.Command(self, "boottest-payload", "-as-deployed", "-games", gamesDir, id)
-	var stderr bytes.Buffer
-	payloadCmd.Stderr = &stderr
-	payload, err := payloadCmd.Output()
-	if err != nil {
-		var exitErr *exec.ExitError
-		if errors.As(err, &exitErr) && exitErr.ExitCode() == exitMissingSecret {
-			// The builder has no CI secrets: that's the template's problem to fix, not a reason to stop
-			return []string{"the boot test can't run: " + strings.TrimSpace(stderr.String()) + ". Remove 'secrets' from " +
-				"test.yaml; boot tests fill password settings with a throwaway value"}, nil, nil
-		}
-		return nil, nil, fmt.Errorf("boottest-payload: %v: %s", err, strings.TrimSpace(stderr.String()))
-	}
 	if out, err := runCmd("docker", "pull", "-q", agentImage); err != nil {
 		return nil, nil, fmt.Errorf("docker pull %s: %v: %s", agentImage, err, out)
 	}
-	cmd := exec.Command("docker", "run", "--rm", "-i", "-v", "/var/run/docker.sock:/var/run/docker.sock",
-		"--entrypoint", "java", agentImage, "-Dloader.main=com.gbreed.gbr.nodeagent.BootTest", "-cp", "app.jar",
-		"org.springframework.boot.loader.launch.PropertiesLauncher")
-	cmd.Stdin = bytes.NewReader(payload)
-	var output bytes.Buffer
-	cmd.Stdout = &teeWriter{&output}
-	cmd.Stderr = cmd.Stdout
-	runErr := cmd.Run()
-	if runErr != nil {
-		var exitErr *exec.ExitError
-		if !errors.As(runErr, &exitErr) {
-			return nil, nil, runErr
+	output, passed, problems, err := agentBoot(gamesDir, id, agentImage, false)
+	if err != nil || problems != nil {
+		return problems, nil, err
 	}
-		return []string{bootFailure(output.String())}, nil, nil
+	if !passed {
+		// The server's own output rarely says it stopped because a setting was empty. Booting once more
+		// with the blank ones filled tells for certain whether they are the cause.
+		if blank := blankRequiredLooking(gamesDir, id); len(blank) > 0 {
+			fmt.Printf("[build] it failed with the defaults; booting again with %s filled to see whether empty settings are the cause\n",
+				strings.Join(blank, ", "))
+			if _, filledPassed, _, err := agentBoot(gamesDir, id, agentImage, true); err == nil && filledPassed {
+				return []string{emptySettingsProblem(blank, bootVerdict(output))}, nil, nil
 			}
-	verified = append(verified, "boot test with the node-agent: "+bootVerdict(output.String()))
+		}
+		return []string{bootFailure(output)}, nil, nil
+	}
+	verified = append(verified, "boot test with the node-agent: "+bootVerdict(output))
 
 	if wantOutside {
 		fmt.Println("[build] first boot wrote saves outside the volume; booting the draft to see that it no longer does")
@@ -290,6 +271,82 @@ func bootCheck(gamesDir, id, agentImage string, wantOutside bool) (problems, ver
 	return nil, verified, nil
 }
 
+// agentBoot runs one boot test with the node-agent's BootTest, with the settings a new server gets, like
+// CI. fillBlank also fills blank password-like settings, only to find out whether they are why a draft
+// fails. problems is set when the template can't be boot-tested at all.
+func agentBoot(gamesDir, id, agentImage string, fillBlank bool) (output string, passed bool, problems []string, err error) {
+	self, err := os.Executable()
+	if err != nil {
+		return "", false, nil, err
+	}
+	// A boot test that was killed (worker restarted mid-build) leaves its container and volume behind,
+	// and the next deploy under the same name would trip over them
+	runCmd("docker", "rm", "-f", "boottest-"+id)
+	runCmd("docker", "volume", "rm", "-f", "boottest-"+id+"-data")
+	args := []string{"boottest-payload", "-games", gamesDir, id}
+	if fillBlank {
+		args = []string{"boottest-payload", "-fill-blank-passwords", "-games", gamesDir, id}
+	}
+	payloadCmd := exec.Command(self, args...)
+	var stderr bytes.Buffer
+	payloadCmd.Stderr = &stderr
+	payload, err := payloadCmd.Output()
+	if err != nil {
+		var exitErr *exec.ExitError
+		if errors.As(err, &exitErr) && exitErr.ExitCode() == exitMissingSecret {
+			// The builder has no CI secrets: that's the template's problem to fix, not a reason to stop
+			return "", false, []string{"the boot test can't run: " + strings.TrimSpace(stderr.String()) + ". Remove 'secrets' from " +
+				"test.yaml; boot tests run with exactly what a new server gets"}, nil
+		}
+		return "", false, nil, fmt.Errorf("boottest-payload: %v: %s", err, strings.TrimSpace(stderr.String()))
+	}
+	cmd := exec.Command("docker", "run", "--rm", "-i", "-v", "/var/run/docker.sock:/var/run/docker.sock",
+		"--entrypoint", "java", agentImage, "-Dloader.main=com.gbreed.gbr.nodeagent.BootTest", "-cp", "app.jar",
+		"org.springframework.boot.loader.launch.PropertiesLauncher")
+	cmd.Stdin = bytes.NewReader(payload)
+	var out bytes.Buffer
+	cmd.Stdout = &teeWriter{&out}
+	cmd.Stderr = cmd.Stdout
+	if runErr := cmd.Run(); runErr != nil {
+		var exitErr *exec.ExitError
+		if !errors.As(runErr, &exitErr) {
+			return "", false, nil, runErr
+		}
+		return out.String(), false, nil, nil
+	}
+	return out.String(), true, nil, nil
+}
+
+// blankRequiredLooking lists the settings a new server gets empty that look like a server might need a
+// value: password-like keys with an empty default and no x-generate.
+func blankRequiredLooking(gamesDir, id string) []string {
+	g, err := loadMap(filepath.Join(gamesDir, id, "game.yaml"))
+	if err != nil {
+		return nil
+	}
+	defaults, _ := g["defaultConfig"].(map[string]any)
+	var keys []string
+	for key, p := range schemaProperties(g) {
+		prop, _ := p.(map[string]any)
+		if prop["x-generate"] != nil || !passwordKey.MatchString(key) {
+			continue
+		}
+		if v, ok := defaults[key]; !ok || v == nil || v == "" {
+			keys = append(keys, key)
+		}
+	}
+	sort.Strings(keys)
+	return keys
+}
+
+func emptySettingsProblem(keys []string, verdict string) string {
+	return fmt.Sprintf("the server stops when %s is empty, as it is for every new server: booted with your defaults it "+
+		"failed (%s), and booted again with a value in %s it ran. A new server must start without the owner typing "+
+		"anything: give %s \"x-generate\": {\"type\": \"password\"} in configSchema (the panel then makes a password for "+
+		"each server; keep the default \"\"), or a default value that works. Keep the setting so owners can change it",
+		strings.Join(keys, ", "), verdict, strings.Join(keys, ", "), strings.Join(keys, ", "))
+}
+
 func firstBootGame(gamesDir, id string) (*bootReport, error) {
 	outDir := filepath.Join("firstboot-out", id+"-draft")
 	if err := runFirstBoot([]string{"-games", gamesDir, "-game", id, "-out", outDir}); err != nil {

cmd/gbrgames/build_test.go

@@ -1,6 +1,8 @@
 package main
 
 import (
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 )
@@ -48,3 +50,30 @@ func TestVariantNotes(t *testing.T) {
 		}
 	}
 }
+
+func TestBlankRequiredLookingSettings(t *testing.T) {
+	dir := t.TempDir()
+	game := filepath.Join(dir, "example")
+	os.MkdirAll(game, 0o755)
+	os.WriteFile(filepath.Join(game, "game.yaml"), []byte(`id: example
+defaultConfig:
+  JoinPassword: ""
+  AdminPassword: ""
+  ServerPassword: set-by-default
+  ServerName: x
+configSchema:
+  properties:
+    JoinPassword: {type: string}
+    AdminPassword: {type: string, x-generate: {type: password}}
+    ServerPassword: {type: string}
+    ServerName: {type: string}
+`), 0o644)
+	got := blankRequiredLooking(dir, "example")
+	if len(got) != 1 || got[0] != "JoinPassword" {
+		t.Errorf("got %v, want only JoinPassword (AdminPassword is generated, ServerPassword has a default)", got)
+	}
+	msg := emptySettingsProblem(got, "FAILED: server crashed")
+	if !strings.Contains(msg, "stops when JoinPassword is empty") || !strings.Contains(msg, `"x-generate": {"type": "password"}`) {
+		t.Errorf("message: %s", msg)
+	}
+}

cmd/gbrgames/draft.go

@@ -276,7 +276,7 @@ func draftCompleteness(game, test map[string]any) []string {
 	// Secrets only exist as CI settings a person adds; a draft with one can't be boot-tested anywhere
 	if secrets, _ := test["secrets"].(map[string]any); len(secrets) > 0 {
 		problems = append(problems, "test.yaml has 'secrets' ("+strings.Join(sortedKeys(secrets), ", ")+"), which no boot test of a "+
-			"new game has: remove them. Password settings get a throwaway value in boot tests automatically")
+			"new game has: remove them. A setting the server needs a password in gets \"x-generate\" instead")
 	}
 	if _, ok := game["readyPattern"]; !ok {
 		if _, ok := test["settleSec"]; !ok {

cmd/gbrgames/validate.go

@@ -168,8 +168,9 @@ func validateGame(dir, folder string, gameSchema, testSchema *jsonschema.Schema)
 			add("test.yaml config.%s is not a key of this game", key)
 		}
 		if sensitiveKey.MatchString(key) {
-			add("test.yaml config.%s looks like a credential; remove it (boot tests fill password settings with a throwaway "+
-				"value), or, only if the server can't start without a real one, map it under 'secrets' to a CI secret", key)
+			add("test.yaml config.%s looks like a credential; remove it. A new server starts without it, and the boot test "+
+				"does too: if the server can't, give the setting \"x-generate\" (a password per server). Map it under "+
+				"'secrets' to a CI secret only for a value that must be real (a license or account ID)", key)
 		}
 	}
 	for _, key := range sortedKeys(secrets) {