types: lift configValidator across LoadServerConfig sub-builders

Sub-builders called after validateServerConfig (prefixV4, prefixV6, allocation strategy, dns, oidc client secret/path, isSafeServerURL) each returned the first error. So an operator that fixed one issue saw the next one only on the next startup. Lift one *configValidator across the entire LoadServerConfig flow. validateServerConfigInto(v) populates it; each sub-builder's error is wrapped in a structured *ConfigError (with the original sentinel kept on the Cause field, so errors.Is against errOidcMutuallyExclusive, errServerURLSame/Suffix, ErrNoPrefixConfigured, and ErrInvalidAllocationStrategy keeps working). v.Err() is checked once, right before the &Config{} construction. TestReadConfig/base-domain-in-server-url-err matched the old sentinel wording; flipped to the new structured Reason. Added TestLoadServerConfig_CollectsAcrossSubBuilders to lock the wiring: four sub-builder failures from a single config produce four *ConfigErrors in the report. Updates #3227
2026-09-24 01:04:54 +09:00 · 2026-04-30 09:23:49 +00:00
parent 8a12c572ef
commit 4cad0b564e
2 changed files with 202 additions and 50 deletions
@@ -552,9 +552,21 @@ func resolveNodeExpiry() time.Duration {
 	return time.Duration(expiry)
 }

+// validateServerConfig is the test-only entry point. Production code
+// goes through LoadServerConfig, which lifts the validator to span
+// every config-time sub-builder so the operator sees every problem
+// in one render.
 func validateServerConfig() error {
 	v := &configValidator{}
+	validateServerConfigInto(v)

+	return v.Err()
+}
+
+// validateServerConfigInto runs every viper-level validation rule
+// against the provided validator. Caller owns the validator and is
+// responsible for inspecting v.Err() once all sub-builders have run.
+func validateServerConfigInto(v *configValidator) {
 	depr := deprecator{seen: make(set.Set[string])}

 	// Register aliases for backward compatibility
@@ -601,7 +613,12 @@ func validateServerConfig() error {
 	if viper.GetString("oidc.issuer") != "" {
 		err := validateOIDCConfig()
 		if err != nil {
-			return err
+			v.Add(&ConfigError{
+				Reason: "OIDC configuration is invalid",
+				Detail: err.Error(),
+				Hint:   "check oidc.issuer, oidc.client_id, oidc.client_secret, and oidc.pkce.method",
+				Cause:  err,
+			})
 		}
 	}

@@ -747,32 +764,8 @@ func validateServerConfig() error {
 	validateDERPConfig(v)
 	validateDatabaseConfig(v)
 	validateMagicDNSConfig(v)
-	validatePKCEConfig(v)

 	depr.Apply(v)
-
-	return v.Err()
-}
-
-// validatePKCEConfig records a ConfigError when oidc.enabled is true
-// and oidc.pkce.method is not one of the allowed values.
-func validatePKCEConfig(v *configValidator) {
-	if !viper.GetBool("oidc.enabled") {
-		return
-	}
-
-	method := viper.GetString("oidc.pkce.method")
-
-	err := validatePKCEMethod(method)
-	if err != nil {
-		v.Add(&ConfigError{
-			Reason:  "oidc.pkce.method has an unsupported value",
-			Current: []KV{{"oidc.pkce.method", method}},
-			Allowed: []string{PKCEMethodPlain, PKCEMethodS256},
-			Hint:    "pick one of the allowed values; S256 is recommended",
-			Cause:   err,
-		})
-	}
 }

 // validateDERPConfig records ConfigErrors when the embedded DERP server
@@ -1226,30 +1219,55 @@ func LoadCLIConfig() (*Config, error) {
 // LoadServerConfig returns the full Headscale configuration to
 // host a Headscale server. This is called as part of `headscale serve`.
 func LoadServerConfig() (*Config, error) {
-	if err := validateServerConfig(); err != nil { //nolint:noinlineerr
-		return nil, err
-	}
+	v := &configValidator{}
+	validateServerConfigInto(v)

 	logConfig := logConfig()
 	zerolog.SetGlobalLevel(logConfig.Level)

 	prefix4, v4NonStandard, err := parsePrefixConfig("prefixes.v4", tsaddr.CGNATRange(), "IPv4")
 	if err != nil {
-		return nil, err
+		v.Add(&ConfigError{
+			Reason:  "prefixes.v4 is not a valid CIDR",
+			Current: []KV{{"prefixes.v4", viper.GetString("prefixes.v4")}},
+			Detail:  err.Error(),
+			Hint:    `use CIDR form, e.g. "100.64.0.0/10" (CGNAT, headscale default)`,
+			Cause:   err,
+		})
 	}

 	prefix6, v6NonStandard, err := parsePrefixConfig("prefixes.v6", tsaddr.TailscaleULARange(), "IPv6")
 	if err != nil {
-		return nil, err
+		v.Add(&ConfigError{
+			Reason:  "prefixes.v6 is not a valid CIDR",
+			Current: []KV{{"prefixes.v6", viper.GetString("prefixes.v6")}},
+			Detail:  err.Error(),
+			Hint:    `use CIDR form, e.g. "fd7a:115c:a1e0::/48" (Tailscale ULA, headscale default)`,
+			Cause:   err,
+		})
 	}

 	trusted, err := trustedProxies()
 	if err != nil {
-		return nil, err
+		v.Add(&ConfigError{
+			Reason:  "trusted_proxies contains an invalid CIDR",
+			Current: []KV{{"trusted_proxies", viper.GetStringSlice("trusted_proxies")}},
+			Detail:  err.Error(),
+			Hint:    "use specific proxy CIDRs; catch-all ranges are not allowed",
+			Cause:   err,
+		})
 	}

 	if prefix4 == nil && prefix6 == nil {
-		return nil, ErrNoPrefixConfigured
+		v.Add(&ConfigError{
+			Reason: "no IP prefix configured for the tailnet",
+			Current: []KV{
+				{"prefixes.v4", viper.GetString("prefixes.v4")},
+				{"prefixes.v6", viper.GetString("prefixes.v6")},
+			},
+			Hint:  `set at least one of prefixes.v4 (default: "100.64.0.0/10") or prefixes.v6 (default: "fd7a:115c:a1e0::/48")`,
+			Cause: ErrNoPrefixConfigured,
+		})
 	}

 	if v4NonStandard || v6NonStandard {
@@ -1283,37 +1301,61 @@ func LoadServerConfig() (*Config, error) {
 	case string(IPAllocationStrategyRandom):
 		alloc = IPAllocationStrategyRandom
 	default:
-		return nil, fmt.Errorf(
-			"%w: %q, allowed options: %s, %s",
-			ErrInvalidAllocationStrategy,
-			allocStr,
-			IPAllocationStrategySequential,
-			IPAllocationStrategyRandom,
-		)
+		v.Add(&ConfigError{
+			Reason:  "prefixes.allocation has an unsupported value",
+			Current: []KV{{"prefixes.allocation", allocStr}},
+			Allowed: []string{
+				string(IPAllocationStrategySequential),
+				string(IPAllocationStrategyRandom),
+			},
+			Hint:  "pick one of the allowed values; sequential is the default",
+			Cause: ErrInvalidAllocationStrategy,
+		})
 	}

 	dnsConfig, err := dns()
 	if err != nil {
-		return nil, err
+		v.Add(&ConfigError{
+			Reason:  "dns.extra_records cannot be parsed",
+			Current: []KV{{"dns.extra_records", "<inline records>"}},
+			Detail:  err.Error(),
+			Hint:    "check the YAML syntax; see config-example.yaml for the expected shape",
+			Cause:   err,
+		})
 	}

 	derpConfig := derpConfig()
 	logTailConfig := logtailConfig()

 	oidcClientSecret := viper.GetString("oidc.client_secret")
-
 	oidcClientSecretPath := viper.GetString("oidc.client_secret_path")
-	if oidcClientSecretPath != "" && oidcClientSecret != "" {
-		return nil, errOidcMutuallyExclusive
-	}

-	if oidcClientSecretPath != "" {
+	switch {
+	case oidcClientSecretPath != "" && oidcClientSecret != "":
+		v.Add(&ConfigError{
+			Reason: "oidc.client_secret and oidc.client_secret_path are mutually exclusive",
+			Current: []KV{
+				{"oidc.client_secret", "<redacted>"},
+				{"oidc.client_secret_path", oidcClientSecretPath},
+			},
+			Hint:  "keep one source for the secret; the path form is recommended so the secret stays out of config files",
+			See:   "https://headscale.net/stable/ref/oidc/",
+			Cause: errOidcMutuallyExclusive,
+		})
+
+	case oidcClientSecretPath != "":
 		secretBytes, err := os.ReadFile(os.ExpandEnv(oidcClientSecretPath))
 		if err != nil {
-			return nil, err
+			v.Add(&ConfigError{
+				Reason:  "oidc.client_secret_path cannot be read",
+				Current: []KV{{"oidc.client_secret_path", oidcClientSecretPath}},
+				Detail:  err.Error(),
+				Hint:    "ensure the file exists and is readable by the headscale user",
+				Cause:   err,
+			})
+		} else {
+			oidcClientSecret = strings.TrimSpace(string(secretBytes))
 		}
-
-		oidcClientSecret = strings.TrimSpace(string(secretBytes))
 	}

 	serverURL := viper.GetString("server_url")
@@ -1328,10 +1370,15 @@ func LoadServerConfig() (*Config, error) {
 	if dnsConfig.BaseDomain != "" {
 		err := isSafeServerURL(serverURL, dnsConfig.BaseDomain)
 		if err != nil {
-			return nil, err
+			addServerURLConfigError(v, serverURL, dnsConfig.BaseDomain, err)
 		}
 	}

+	err = v.Err()
+	if err != nil {
+		return nil, err
+	}
+
 	return &Config{
 		ServerURL:          serverURL,
 		Addr:               viper.GetString("listen_addr"),
@@ -1440,6 +1487,45 @@ func LoadServerConfig() (*Config, error) {
 	}, nil
 }

+// addServerURLConfigError pushes a structured ConfigError onto v for
+// each isSafeServerURL failure mode so the operator sees the issue
+// rendered consistently with the rest of the config-load report.
+func addServerURLConfigError(v *configValidator, serverURL, baseDomain string, err error) {
+	current := []KV{
+		{"server_url", serverURL},
+		{"dns.base_domain", baseDomain},
+	}
+
+	switch {
+	case errors.Is(err, errServerURLSame):
+		v.Add(&ConfigError{
+			Reason:  "server_url and dns.base_domain refer to the same hostname",
+			Current: current,
+			Hint:    "give server_url a host distinct from dns.base_domain (Tailscale takes over base_domain via MagicDNS)",
+			See:     "https://headscale.net/stable/ref/dns/",
+			Cause:   errServerURLSame,
+		})
+
+	case errors.Is(err, errServerURLSuffix):
+		v.Add(&ConfigError{
+			Reason:  "server_url is a subdomain of dns.base_domain",
+			Current: current,
+			Hint:    "host headscale on a domain outside dns.base_domain (Tailscale takes over base_domain via MagicDNS)",
+			See:     "https://headscale.net/stable/ref/dns/",
+			Cause:   errServerURLSuffix,
+		})
+
+	default:
+		v.Add(&ConfigError{
+			Reason:  "server_url cannot be parsed",
+			Current: []KV{{"server_url", serverURL}},
+			Detail:  err.Error(),
+			Hint:    "set server_url to an absolute URL, e.g. https://headscale.example.com",
+			Cause:   err,
+		})
+	}
+}
+
 // BaseDomain cannot be a suffix of the server URL.
 // This is because Tailscale takes over the domain in BaseDomain,
 // causing the headscale server and DERP to be unreachable.
@@ -165,7 +165,7 @@ func TestReadConfig(t *testing.T) {
 				return LoadServerConfig()
 			},
 			want:    nil,
-			wantErr: errServerURLSuffix.Error(),
+			wantErr: "server_url is a subdomain of dns.base_domain",
 		},
 		{
 			name:       "base-domain-not-in-server-url",
@@ -494,7 +494,10 @@ noise:
  private_key_path: noise_private.key
 server_url: http://127.0.0.1:8080
 dns:
+  magic_dns: false
  override_local_dns: false
+database:
+  type: sqlite
 oidc:` + tt.oidcBlock + "\n")

 			require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "config.yaml"), configYaml, 0o600))
@@ -565,6 +568,69 @@ dns:
 	}
 }

+func TestLoadServerConfig_CollectsAcrossSubBuilders(t *testing.T) {
+	viper.Reset()
+
+	tmpDir := t.TempDir()
+	cfg := `---
+server_url: https://example.com
+listen_addr: 0.0.0.0:8080
+prefixes:
+  v4: not-a-cidr
+  v6: also-not-a-cidr
+  allocation: bogus
+oidc:
+  client_secret: hunter2
+  client_secret_path: /nonexistent/secret
+noise:
+  private_key_path: noise_private.key
+database:
+  type: sqlite3
+dns:
+  magic_dns: false
+  override_local_dns: false
+  base_domain: example.com
+`
+	require.NoError(t, os.WriteFile(
+		filepath.Join(tmpDir, "config.yaml"), []byte(cfg), 0o600))
+	require.NoError(t, LoadConfig(tmpDir, false))
+
+	_, err := LoadServerConfig()
+	require.Error(t, err)
+
+	got := ConfigErrors(err)
+	assert.GreaterOrEqual(t, len(got), 4,
+		"expected sub-builder errors collected into one report; got %d: %v",
+		len(got), reasons(got))
+
+	// Sentinels stay reachable through ConfigError.Cause.
+	require.ErrorIs(t, err, ErrInvalidAllocationStrategy,
+		"wrapped ErrInvalidAllocationStrategy not in chain")
+	require.ErrorIs(t, err, errOidcMutuallyExclusive,
+		"wrapped errOidcMutuallyExclusive not in chain")
+
+	// Each sub-builder failure surfaces its YAML key in the rendered output.
+	rendered := err.Error()
+	for _, want := range []string{
+		"prefixes.v4",
+		"prefixes.v6",
+		"prefixes.allocation",
+		"oidc.client_secret",
+	} {
+		assert.Contains(t, rendered, want,
+			"expected rendered error to mention %q", want)
+	}
+}
+
+func reasons(errs []*ConfigError) []string {
+	out := make([]string, len(errs))
+	for i, e := range errs {
+		out[i] = e.Reason
+	}
+
+	return out
+}
+
 func TestDeprecatedKeysFlowThroughValidator(t *testing.T) {
 	viper.Reset()