fix(relay-server): stop the report from answering a question it was not asked

Three findings from review, all cases where the report was confident about
something it had not actually checked.

`--env-file` was documented as showing what a deployment will run, but it
resolves the file against the relay binary's defaults. Compose supplies its
own first: a file carrying only PORTAL_URL and UDP_ENABLED=true reported
`udp-transport blocked` while `docker compose up` would give it
MIN_PORT=40000 and enable it. Reimplementing Compose's defaults here would
build a second configuration engine, so the claim is narrowed instead. The
accurate check needs no new code — running the subcommand inside the
container lets Compose build the environment first:

    docker compose run --rm -T portal config

That is now what .env.example, the configuration page and the command's own
usage recommend, and a file-scoped report says in its header which of the
two questions it answered.

`discoveryFeature` inspected only the parsed hostname, so
PORTAL_URL=http://relay.example.com reported enabled and portal.NewServer
rejected the same value seconds later — the exact divergence this feature
exists to remove. It now calls utils.NormalizeRelayURL and
utils.NormalizeRelayURLs, the same normalization the server applies, rather
than holding a second opinion about it. Bootstraps are counted after
normalization for the same reason.

loadEnvFile silently skipped any line without an `=`. `DISCOVERY true`
vanished and discovery reported its default with nothing to explain why,
which is the silent misconfiguration this command was written to expose.
Malformed lines now fail with file:line.

Also keys dnsProviderCredential by acme's exported Type* constants rather
than repeating the provider names, so acme stays the one place that decides
what is supported and this map only adds the credential each one needs.

Author: rabbitson87

.env.example

@@ -3,14 +3,20 @@
 # Copy to .env and edit. Every key here is read either by the relay binary or by
 # Docker Compose; nothing else reads this file.
 #
-# Check a configured file before starting anything. The report gives the
+# Check the configuration before starting anything. The report gives the
 # effective value and source of every key, names any key that nothing reads, and
 # says which features are off and what is missing:
 #
-#   docker compose run --rm -T portal config --env-file /dev/stdin < .env
+#   docker compose run --rm -T portal config
 #
-# -T because the file arrives on stdin. Without it Compose asks for a TTY and
-# older versions fail with "the input device is not a TTY".
+# No --env-file: Compose has already built the container's environment from this
+# file plus its own defaults, so the report describes what `docker compose up`
+# will actually run. Passing --env-file instead reads the file against the relay
+# binary's defaults, which differ -- MIN_PORT is 0 there and 40000 under Compose
+# -- and would report features as blocked that the deployment enables.
+#
+# -T because `docker compose run` asks for a TTY otherwise, and older versions
+# fail with "the input device is not a TTY".
 #
 # API_PORT and SNI_PORT are deliberately absent. The bundled topology fixes them
 # at 4017 and 443 because the relay reaches its own API listener through its SNI

cmd/relay-server/config.go

@@ -100,6 +100,15 @@ func discoveryFeature(cfg relayServerConfig) feature {
 		f.State, f.By = stateDisabled, "DISCOVERY=false"
 		return f
 	}
+	// The same normalization portal.NewServer applies, not a second opinion
+	// about it. Checking only the parsed hostname would report PORTAL_URL=http://…
+	// as enabled and then have the server reject it seconds later, which is
+	// exactly the divergence this report exists to remove.
+	if _, err := utils.NormalizeRelayURL(cfg.PortalURL); err != nil {
+		f.State, f.By = stateBlocked, "DISCOVERY=true"
+		f.Missing = fmt.Sprintf("PORTAL_URL is not usable as a relay URL: %v", err)
+		return f
+	}
 	host := portalURLHost(cfg.PortalURL)
 	if host == "" || utils.IsLocalRelayHost(host) {
 		f.State, f.By = stateBlocked, "DISCOVERY=true"
@@ -108,9 +117,15 @@ func discoveryFeature(cfg relayServerConfig) feature {
 			host)
 		return f
 	}
+	bootstraps, err := utils.NormalizeRelayURLs(utils.SplitCSV(cfg.Bootstraps)...)
+	if err != nil {
+		f.State, f.By = stateBlocked, "DISCOVERY=true"
+		f.Missing = fmt.Sprintf("BOOTSTRAPS is not usable: %v", err)
+		return f
+	}
 	f.State, f.By = stateEnabled, "DISCOVERY=true"
 	f.Detail = fmt.Sprintf("host=%s bootstraps=%d wireguard_port=%d",
-		host, len(utils.SplitCSV(cfg.Bootstraps)), cfg.WireGuardPort)
+		host, len(bootstraps), cfg.WireGuardPort)
 	return f
 }
 
@@ -321,19 +336,25 @@ func loadEnvFile(path string) ([]envFileEntry, error) {
 
 	var entries []envFileEntry
 	scanner := bufio.NewScanner(file)
+	lineNo := 0
 	for scanner.Scan() {
+		lineNo++
 		line := strings.TrimSpace(scanner.Text())
 		if line == "" || strings.HasPrefix(line, "#") {
 			continue
 		}
 		line = strings.TrimPrefix(line, "export ")
+		// A line that is neither blank, a comment, nor an assignment is a
+		// mistake, and skipping it would reproduce the silent misconfiguration
+		// this command exists to expose: `DISCOVERY true` would simply vanish
+		// and the feature would report its default with nothing to explain why.
 		name, value, found := strings.Cut(line, "=")
 		if !found {
-			continue
+			return nil, fmt.Errorf("%s:%d: not an assignment: %q", path, lineNo, line)
 		}
 		name = strings.TrimSpace(name)
 		if name == "" {
-			continue
+			return nil, fmt.Errorf("%s:%d: assignment has no name: %q", path, lineNo, line)
 		}
 		// Compose does not expand values read from an env file, so neither do we.
 		value = strings.TrimSpace(value)
@@ -410,7 +431,18 @@ func displayValue(name, value string) string {
 func writeConfigReport(w io.Writer, cfg relayServerConfig, entries []envFileEntry, source string) {
 	relay := knownEnvNames()
 
-	fmt.Fprintf(w, "Portal relay configuration (%s)\n\n", source)
+	fmt.Fprintf(w, "Portal relay configuration (%s)\n", source)
+	// Say what was inspected, because the two modes answer different questions
+	// and only one of them describes a Compose deployment. Reading a file in
+	// isolation applies relay defaults to every key the file omits, while
+	// Compose supplies its own first: a file with only PORTAL_URL reports
+	// MIN_PORT=0 here, and `docker compose up` would run it with 40000.
+	if len(entries) > 0 {
+		fmt.Fprint(w, "Keys absent from this file take relay defaults. A Compose deployment\n"+
+			"supplies its own first; for that environment run the command inside the\n"+
+			"container instead: docker compose run --rm -T portal config\n")
+	}
+	fmt.Fprintln(w)
 
 	supplied := make(map[string]bool, len(entries))
 	for _, entry := range entries {
@@ -723,7 +755,11 @@ func runConfigCommand(args []string) error {
 		format      string
 	)
 	fs := utils.NewFlagSet("relay-server config", printConfigUsage)
-	utils.StringFlag(fs, &envFilePath, "env-file", "", "env file to inspect instead of the process environment")
+	utils.StringFlag(fs, &envFilePath, "env-file", "",
+		"read this file in place of the process environment, against relay defaults. "+
+			"Compose supplies its own defaults on top of a file, so to see what a Compose "+
+			"deployment will actually run, omit this flag and let Compose build the environment: "+
+			"docker compose run --rm -T portal config")
 	utils.StringFlag(fs, &format, "format", "text", "output format: text, env or names")
 
 	if err := utils.ParseFlagSet(fs, args, printConfigUsage); err != nil {
@@ -780,8 +816,9 @@ func printConfigUsage(w io.Writer) {
 			"relay-server config [--env-file PATH] [--format text|env]",
 		},
 		[]string{
-			"relay-server config",
-			"relay-server config --env-file .env",
+			"docker compose run --rm -T portal config    # what Compose will run",
+			"relay-server config                         # this process environment",
+			"relay-server config --env-file .env         # one file, against relay defaults",
 			"relay-server config --format env > env.reference",
 		},
 	)

cmd/relay-server/config_test.go

@@ -163,3 +163,82 @@ func TestACMEFeatureBlockedWithoutCredential(t *testing.T) {
 		t.Fatalf("acme missing = %q, want it to name the credential", f.Missing)
 	}
 }
+
+// A line that is neither blank, a comment, nor an assignment is a typo, and
+// dropping it would recreate the silent misconfiguration this command exists to
+// expose: the feature would report its default with nothing to explain why.
+func TestLoadEnvFileRejectsMalformedLines(t *testing.T) {
+	for name, line := range map[string]string{
+		"missing separator": "DISCOVERY true",
+		"empty name":        "=true",
+	} {
+		t.Run(name, func(t *testing.T) {
+			path := writeEnvFile(t, "PORTAL_URL=https://relay.example.com", line)
+
+			_, err := loadEnvFile(path)
+			if err == nil {
+				t.Fatalf("%q was accepted", line)
+			}
+			if !strings.Contains(err.Error(), ":2:") {
+				t.Fatalf("error does not point at the line: %v", err)
+			}
+		})
+	}
+}
+
+func TestLoadEnvFileKeepsCommentsAndBlanks(t *testing.T) {
+	path := writeEnvFile(t, "# a comment", "", "  ", "export PORTAL_URL=https://relay.example.com")
+
+	entries, err := loadEnvFile(path)
+	if err != nil {
+		t.Fatalf("load env file: %v", err)
+	}
+	if len(entries) != 1 || entries[0].Name != "PORTAL_URL" {
+		t.Fatalf("entries = %v, want only PORTAL_URL", entries)
+	}
+}
+
+// The report must not claim a feature works when the server will reject the
+// same value moments later. portal.NewServer normalizes PORTAL_URL through
+// utils.NormalizeRelayURL, which requires https.
+func TestDiscoveryBlockedForNonHTTPSPortalURL(t *testing.T) {
+	path := writeEnvFile(t, "DISCOVERY=true", "PORTAL_URL=http://relay.example.com")
+	cfg := resolveWithEnvFile(t, path)
+
+	f := discoveryFeature(cfg)
+	if f.State != stateBlocked {
+		t.Fatalf("discovery state = %q, want blocked for a non-https PORTAL_URL", f.State)
+	}
+	if !strings.Contains(f.Missing, "https") {
+		t.Fatalf("missing = %q, want it to name the https requirement", f.Missing)
+	}
+}
+
+func TestDiscoveryBlockedForUnusableBootstraps(t *testing.T) {
+	path := writeEnvFile(t,
+		"DISCOVERY=true",
+		"PORTAL_URL=https://relay.example.com",
+		"BOOTSTRAPS=http://peer.example.com")
+	cfg := resolveWithEnvFile(t, path)
+
+	f := discoveryFeature(cfg)
+	if f.State != stateBlocked {
+		t.Fatalf("discovery state = %q, want blocked for an unusable BOOTSTRAPS", f.State)
+	}
+}
+
+func TestDiscoveryEnabledCountsNormalizedBootstraps(t *testing.T) {
+	path := writeEnvFile(t,
+		"DISCOVERY=true",
+		"PORTAL_URL=https://relay.example.com",
+		"BOOTSTRAPS=https://a.example.com,https://b.example.com")
+	cfg := resolveWithEnvFile(t, path)
+
+	f := discoveryFeature(cfg)
+	if f.State != stateEnabled {
+		t.Fatalf("discovery state = %q, want enabled", f.State)
+	}
+	if !strings.Contains(f.Detail, "bootstraps=2") {
+		t.Fatalf("detail = %q, want bootstraps=2", f.Detail)
+	}
+}

cmd/relay-server/envcatalog.go

@@ -1,5 +1,7 @@
 package main
 
+import "github.com/gosuda/portal-tunnel/v2/portal/acme"
+
 // The deployment .env is shared by the relay and by Docker Compose itself.
 // Checking a key against the relay's own flags alone would report the
 // Compose-level ones as unknown, so the keys owned elsewhere are catalogued
@@ -64,11 +66,16 @@ var pinnedByTopology = map[string]struct {
 // credential it requires. Providers whose credentials come from an ambient
 // chain (an instance role, application default credentials) map to an empty
 // list because there is nothing to require.
+//
+// The keys are acme's own exported constants rather than repeated strings, so
+// a provider added there cannot silently go unreported here: acme.NewDNSProvider
+// decides what is supported, and this map only adds the credential each one
+// needs, which is knowledge the report owns.
 var dnsProviderCredential = map[string][]string{
-	"cloudflare": {"CLOUDFLARE_TOKEN"},
-	"hetzner":    {"HETZNER_API_TOKEN"},
-	"njalla":     {"NJALLA_TOKEN"},
-	"vultr":      {"VULTR_API_KEY"},
-	"route53":    nil,
-	"gcloud":     nil,
+	acme.TypeCloudflare: {"CLOUDFLARE_TOKEN"},
+	acme.TypeHetzner:    {"HETZNER_API_TOKEN"},
+	acme.TypeNjalla:     {"NJALLA_TOKEN"},
+	acme.TypeVultr:      {"VULTR_API_KEY"},
+	acme.TypeRoute53:    nil,
+	acme.TypeGCloud:     nil,
 }

docs/src/routes/configuration/+page.md

@@ -13,13 +13,29 @@ This page describes what each variable means. To see what a specific deployment
 is actually doing, ask the binary rather than reading a table:
 
 ```bash
-relay-server config --env-file .env
+docker compose run --rm -T portal config
 ```
 
 It prints every key with its effective value and where that value came from,
 names any key nothing reads, and reports which features are off and what is
-missing. `relay-server config --format env` regenerates the full list from the
-flag definitions, and `make check-env-example` fails when this page or
+missing.
+
+Run it **inside the container, without `--env-file`**. Compose has already
+combined `.env` with the defaults declared in `docker-compose.yml`, so the
+report then describes the environment `docker compose up` will actually
+provide. `--env-file` reads a file on its own, against the relay binary's
+defaults — `MIN_PORT` is `0` there and `40000` under Compose — so a file that
+sets only `PORTAL_URL` and `UDP_ENABLED=true` is reported as
+`udp-transport blocked` although the deployment would enable it. Use it to
+inspect a file in isolation, not to predict a deployment:
+
+```bash
+relay-server config                    # this process environment
+relay-server config --env-file .env    # one file, against relay defaults
+```
+
+`relay-server config --format env` regenerates the full list from the flag
+definitions, and `make check-env-example` fails when this page or
 `.env.example` stops mentioning a key.
 
 ## Relay Server Environment Variables