types: route deprecation fatals through configValidator

deprecator.Log() called log.Fatal directly, so a single deprecated
key killed the process before any other validation rule could
report. Operators fixing one issue at a time.

Replace Log() with Apply(*configValidator). Warns still go to
log.Warn; fatals are pushed onto the validator as *ConfigError so
they merge with the rest of the config-load report. The free-form
strings collected via set.Set are gone; deprecator now stores
typed deprecation{OldKey, NewKey} records and Apply renders them
into the structured ConfigError shape.

Move the early-return PKCE check into a new validatePKCEConfig
modular validator for the same reason.

Updates #3227

hscontrol/types/config.go

@@ -553,10 +553,9 @@ func resolveNodeExpiry() time.Duration {
 }
 
 func validateServerConfig() error {
-	depr := deprecator{
+	v := &configValidator{}
-		warns:  make(set.Set[string]),
+
-		fatals: make(set.Set[string]),
+	depr := deprecator{seen: make(set.Set[string])}
-	}
 
 	// Register aliases for backward compatibility
 	// Has to be called _after_ viper.ReadInConfig()
@@ -606,10 +605,6 @@ func validateServerConfig() error {
 		}
 	}
 
-	depr.Log()
-
-	v := &configValidator{}
-
 	if viper.IsSet("dns.extra_records") && viper.IsSet("dns.extra_records_path") {
 		v.Add(&ConfigError{
 			Reason: "dns.extra_records and dns.extra_records_path are mutually exclusive",
@@ -752,10 +747,34 @@ 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
 // is enabled without the addresses or paths it needs.
 func validateDERPConfig(v *configValidator) {
@@ -1445,20 +1464,61 @@ func isSafeServerURL(serverURL, baseDomain string) error {
 	return nil
 }
 
-type deprecator struct {
+// deprecation describes a configuration key that is no longer
-	warns  set.Set[string]
+// supported. NewKey is the replacement (when one exists) or empty
-	fatals set.Set[string]
+// when the key has been removed without a replacement.
+type deprecation struct {
+	OldKey string
+	NewKey string
+	Hint   string
 }
 
-// fatal deprecates and adds an entry to the fatal list of options if the oldKey is set.
+// deprecator collects deprecated configuration keys observed in the
+// current viper config. Warns are non-blocking and surface via
+// log.Warn; fatals are pushed onto a configValidator as ConfigError
+// entries so they merge with the rest of the validation report
+// instead of pre-empting it via log.Fatal.
+type deprecator struct {
+	seen   set.Set[string]
+	warns  []deprecation
+	fatals []deprecation
+}
+
+func (d *deprecator) addWarn(dep deprecation) {
+	if d.seen.Contains(dep.OldKey) {
+		return
+	}
+
+	d.seen.Add(dep.OldKey)
+	d.warns = append(d.warns, dep)
+}
+
+func (d *deprecator) addFatal(dep deprecation) {
+	if d.seen.Contains(dep.OldKey) {
+		return
+	}
+
+	d.seen.Add(dep.OldKey)
+	d.fatals = append(d.fatals, dep)
+}
+
+// warnWithAlias registers an alias from newKey to oldKey and records
+// a non-blocking warning when oldKey is set.
+//
+//nolint:unused
+func (d *deprecator) warnWithAlias(newKey, oldKey string) {
+	// NOTE: RegisterAlias is called with NEW KEY -> OLD KEY
+	viper.RegisterAlias(newKey, oldKey)
+
+	if viper.IsSet(oldKey) {
+		d.addWarn(deprecation{OldKey: oldKey, NewKey: newKey})
+	}
+}
+
+// fatal records a removed key (no replacement) as a config error.
 func (d *deprecator) fatal(oldKey string) {
 	if viper.IsSet(oldKey) {
-		d.fatals.Add(
+		d.addFatal(deprecation{OldKey: oldKey})
-			fmt.Sprintf(
-				"The %q configuration key has been removed. Please see the changelog for more details.",
-				oldKey,
-			),
-		)
 	}
 }
 
@@ -1468,83 +1528,92 @@ func (d *deprecator) fatal(oldKey string) {
 // policy side.
 func (d *deprecator) fatalWithHint(oldKey, hint string) {
 	if viper.IsSet(oldKey) {
-		d.fatals.Add(
+		d.addFatal(deprecation{OldKey: oldKey, Hint: hint})
-			fmt.Sprintf(
-				"The %q configuration key has been removed. %s",
-				oldKey,
-				hint,
-			),
-		)
 	}
 }
 
-// fatalIfNewKeyIsNotUsed deprecates and adds an entry to the fatal list of options if the oldKey is set and the new key is _not_ set.
+// fatalIfNewKeyIsNotUsed records a config error when oldKey is set
-// If the new key is set, a warning is emitted instead.
+// without newKey, or a warning when both are set (oldKey ignored,
+// newKey takes precedence).
 func (d *deprecator) fatalIfNewKeyIsNotUsed(newKey, oldKey string) {
-	if viper.IsSet(oldKey) && !viper.IsSet(newKey) {
+	if !viper.IsSet(oldKey) {
-		d.fatals.Add(
+		return
-			fmt.Sprintf(
-				"The %q configuration key is deprecated. Please use %q instead. %q has been removed.",
-				oldKey,
-				newKey,
-				oldKey,
-			),
-		)
-	} else if viper.IsSet(oldKey) {
-		d.warns.Add(fmt.Sprintf("The %q configuration key is deprecated. Please use %q instead. %q has been removed.", oldKey, newKey, oldKey))
 	}
+
+	if !viper.IsSet(newKey) {
+		d.addFatal(deprecation{OldKey: oldKey, NewKey: newKey})
+
+		return
+	}
+
+	d.addWarn(deprecation{OldKey: oldKey, NewKey: newKey})
 }
 
-// fatalIfSet fatals if the oldKey is set at all, regardless of whether
+// fatalIfSet records a config error any time oldKey is set, naming
-// the newKey is set. Use this when the old key has been fully removed
+// newKey as the replacement.
-// and any use of it should be a hard error.
 func (d *deprecator) fatalIfSet(oldKey, newKey string) {
 	if viper.IsSet(oldKey) {
-		d.fatals.Add(
+		d.addFatal(deprecation{OldKey: oldKey, NewKey: newKey})
-			fmt.Sprintf(
-				"The %q configuration key has been removed. Please use %q instead.",
-				oldKey,
-				newKey,
-			),
-		)
 	}
 }
 
-// warn deprecates and adds an option to log a warning if the oldKey is set.
+// warnNoAlias records a non-blocking warning when oldKey is set,
+// pointing operators at newKey.
 //
 //nolint:unused
 func (d *deprecator) warnNoAlias(newKey, oldKey string) {
 	if viper.IsSet(oldKey) {
-		d.warns.Add(
+		d.addWarn(deprecation{OldKey: oldKey, NewKey: newKey})
-			fmt.Sprintf(
-				"The %q configuration key is deprecated. Please use %q instead. %q has been removed.",
-				oldKey,
-				newKey,
-				oldKey,
-			),
-		)
 	}
 }
 
-func (d *deprecator) String() string {
+// warn records a non-blocking warning when oldKey is set, with no
-	var b strings.Builder
+// replacement to point at.
-
+//
-	for _, w := range d.warns.Slice() {
+//nolint:unused
-		fmt.Fprintf(&b, "WARN: %s\n", w)
+func (d *deprecator) warn(oldKey string) {
+	if viper.IsSet(oldKey) {
+		d.addWarn(deprecation{OldKey: oldKey})
 	}
-
-	for _, f := range d.fatals.Slice() {
-		fmt.Fprintf(&b, "FATAL: %s\n", f)
-	}
-
-	return b.String()
 }
 
-func (d *deprecator) Log() {
+// Apply emits warns via log.Warn and pushes one *ConfigError per
-	if len(d.fatals) > 0 {
+// fatal onto v. Run after the rest of validation so deprecated-key
-		log.Fatal().Msg("\n" + d.String())
+// failures surface alongside the other config errors in a single
-	} else if len(d.warns) > 0 {
+// pass.
-		log.Warn().Msg("\n" + d.String())
+func (d *deprecator) Apply(v *configValidator) {
+	for _, w := range d.warns {
+		if w.NewKey != "" {
+			log.Warn().Msgf(
+				"configuration key %q is deprecated; use %q instead",
+				w.OldKey, w.NewKey)
+
+			continue
+		}
+
+		log.Warn().Msgf("configuration key %q is deprecated", w.OldKey)
+	}
+
+	for _, f := range d.fatals {
+		ce := &ConfigError{
+			Current: []KV{{f.OldKey, viper.Get(f.OldKey)}},
+		}
+
+		if f.NewKey != "" {
+			ce.Reason = fmt.Sprintf(
+				"configuration key %s has been removed; use %s instead",
+				f.OldKey, f.NewKey)
+			ce.Hint = fmt.Sprintf("remove %s and set %s", f.OldKey, f.NewKey)
+		} else {
+			ce.Reason = fmt.Sprintf("configuration key %s has been removed", f.OldKey)
+			if f.Hint != "" {
+				ce.Hint = f.Hint
+			} else {
+				ce.Hint = fmt.Sprintf("remove %s; see the CHANGELOG for context", f.OldKey)
+			}
+		}
+
+		v.Add(ce)
 	}
 }
 

hscontrol/types/config_test.go

@@ -6,6 +6,7 @@ import (
 	"net/netip"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -564,6 +565,56 @@ dns:
 	}
 }
 
+func TestDeprecatedKeysFlowThroughValidator(t *testing.T) {
+	viper.Reset()
+
+	tmpDir := t.TempDir()
+	cfg := `---
+server_url: https://example.com
+listen_addr: 0.0.0.0:8080
+oidc:
+  expiry: 1h
+dns_config:
+  use_username_in_magic_dns: true
+noise:
+  private_key_path: noise_private.key
+database:
+  type: sqlite3
+dns:
+  magic_dns: false
+  override_local_dns: false
+`
+	require.NoError(t, os.WriteFile(
+		filepath.Join(tmpDir, "config.yaml"), []byte(cfg), 0o600))
+	require.NoError(t, LoadConfig(tmpDir, false))
+
+	err := validateServerConfig()
+	require.Error(t, err)
+
+	got := ConfigErrors(err)
+	require.GreaterOrEqual(t, len(got), 2,
+		"expected fatal deprecated keys to surface as ConfigErrors; got %d", len(got))
+
+	reasons := make([]string, 0, len(got))
+	for _, ce := range got {
+		reasons = append(reasons, ce.Reason)
+	}
+
+	wantSubstrs := []string{"oidc.expiry", "dns_config.use_username_in_magic_dns"}
+	for _, want := range wantSubstrs {
+		var found bool
+
+		for _, r := range reasons {
+			if strings.Contains(r, want) {
+				found = true
+				break
+			}
+		}
+
+		assert.True(t, found, "expected ConfigError mentioning %q in reasons %v", want, reasons)
+	}
+}
+
 // OK
 // server_url: headscale.com, base: clients.headscale.com
 // server_url: headscale.com, base: headscale.net