From 4cad0b564ef015f69b609fcba567364515688182 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 30 Apr 2026 09:23:49 +0000 Subject: [PATCH] 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 --- hscontrol/types/config.go | 184 ++++++++++++++++++++++++--------- hscontrol/types/config_test.go | 68 +++++++++++- 2 files changed, 202 insertions(+), 50 deletions(-) diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index c81a2262..52fa0ce4 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -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", ""}}, + 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", ""}, + {"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. diff --git a/hscontrol/types/config_test.go b/hscontrol/types/config_test.go index 3f34dee8..9723335e 100644 --- a/hscontrol/types/config_test.go +++ b/hscontrol/types/config_test.go @@ -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()