Use per hosts SANs for controllers in ConfigureK0S phase

phase/configure_k0s.go

@@ -95,50 +95,41 @@ func (p *ConfigureK0s) Prepare(config *v1beta1.Cluster) error {
 		// do nothing - base k0s config does not contain any existing SANs
 	}
 
-	// populate SANs with all controller addresses
 	for i, c := range p.Config.Spec.Hosts.Controllers() {
 		if c.Reset {
 			continue
 		}
-		if !slices.Contains(sans, c.Address()) {
-			sans = append(sans, c.Address())
-			log.Debugf("added controller %d address %s to spec.api.sans", i+1, c.Address())
-		}
-		if c.PrivateAddress != "" && !slices.Contains(sans, c.PrivateAddress) {
-			sans = append(sans, c.PrivateAddress)
-			log.Debugf("added controller %d private address %s to spec.api.sans", i+1, c.PrivateAddress)
-		}
-	}
 
-	// assign populated sans to the base config
-	p.newBaseConfig.DigMapping("spec", "api")["sans"] = sans
+		// keep dedicated SANs list per controller,
+		// to avoid k0s config regeneration and k0scontroller restarts
+		// when adding new CP node to the cluster
+		log.Debugf("building spec.api.sans for controller %d (%s)", i+1, c.Address())
 
-	for _, h := range p.Config.Spec.Hosts.Controllers() {
-		if h.Reset {
-			continue
-		}
+		// store sans to config
+		controllerBaseConfig := p.newBaseConfig.Dup()
+		controllerBaseConfig.DigMapping("spec", "api")["sans"] = hostsSans(sans, c)
 
-		cfgNew, err := p.configFor(h)
+		cfgNew, err := p.configFor(c, controllerBaseConfig)
 		if err != nil {
-			return fmt.Errorf("failed to build k0s config for %s: %w", h, err)
+			return fmt.Errorf("failed to build k0s config for %s: %w", c, err)
 		}
-		tempConfigPath, err := h.FS().CreateTemp("", "")
+		tempConfigPath, err := c.FS().CreateTemp("", "")
 		if err != nil {
 			return fmt.Errorf("failed to create temporary file for config: %w", err)
 		}
 		defer func() {
-			if err := h.Sudo().FS().Remove(tempConfigPath); err != nil {
-				log.Warnf("%s: failed to delete temporary file %s: %s", h, tempConfigPath, err)
+			if err := c.Sudo().FS().Remove(tempConfigPath); err != nil {
+				log.Warnf("%s: failed to delete temporary file %s: %s", c, tempConfigPath, err)
 			}
 		}()
 
-		if err := h.Sudo().FS().WriteFile(tempConfigPath, []byte(cfgNew), 0o600); err != nil {
+		if err := c.Sudo().FS().WriteFile(tempConfigPath, []byte(cfgNew), 0o600); err != nil {
 			return err
 		}
 
 		// Prepare has no ctx; the Prepare interface takes only *v1beta1.Cluster.
 		// TODO: thread a real ctx if the Prepare interface ever takes a context.
-		if err := p.validateConfig(context.Background(), h, tempConfigPath); err != nil {
+		if err := p.validateConfig(context.Background(), c, tempConfigPath); err != nil {
 			return err
 		}
 
@@ -147,7 +138,7 @@ func (p *ConfigureK0s) Prepare(config *v1beta1.Cluster) error {
 		if err := yaml.Unmarshal([]byte(cfgNew), &cfgA); err != nil {
 			return fmt.Errorf("failed to unmarshal new config: %w", err)
 		}
-		if err := yaml.Unmarshal([]byte(h.Metadata.K0sExistingConfig), &cfgB); err != nil {
+		if err := yaml.Unmarshal([]byte(c.Metadata.K0sExistingConfig), &cfgB); err != nil {
 			return fmt.Errorf("failed to unmarshal existing config: %w", err)
 		}
 		cfgAString, err := yaml.Marshal(cfgA)
@@ -160,13 +151,13 @@ func (p *ConfigureK0s) Prepare(config *v1beta1.Cluster) error {
 		}
 
 		if bytes.Equal(cfgAString, cfgBString) {
-			log.Debugf("%s: configuration will not change", h)
+			log.Debugf("%s: configuration will not change", c)
 			continue
 		}
 
-		log.Debugf("%s: configuration will change", h)
-		h.Metadata.K0sNewConfig = cfgNew
-		p.hosts = append(p.hosts, h)
+		log.Debugf("%s: configuration will change", c)
+		c.Metadata.K0sNewConfig = cfgNew
+		p.hosts = append(p.hosts, c)
 	}
 
 	return nil
@@ -228,6 +219,20 @@ func (p *ConfigureK0s) Run(ctx context.Context) error {
 	return p.parallelDo(ctx, controllers, p.configureK0s)
 }
 
+// hostsSans returns a dedicated copy of the base sans extended with the
+// host's own addresses.
+func hostsSans(sans []string, host *cluster.Host) []string {
+	out := make([]string, len(sans))
+	copy(out, sans)
+	if !slices.Contains(out, host.Address()) {
+		out = append(out, host.Address())
+	}
+	if host.PrivateAddress != "" && !slices.Contains(out, host.PrivateAddress) {
+		out = append(out, host.PrivateAddress)
+	}
+	return out
+}
+
 func requiresIPv6NodeLocalAPIAddress(cfg dig.Mapping) bool {
 	if cfg == nil {
 		return false
@@ -335,19 +340,19 @@ func (p *ConfigureK0s) configureK0s(ctx context.Context, h *cluster.Host) error
 	return nil
 }
 
-func (p *ConfigureK0s) configFor(h *cluster.Host) (string, error) {
+func (p *ConfigureK0s) configFor(h *cluster.Host, hostBaseConfig dig.Mapping) (string, error) {
 	var cfg dig.Mapping
 
 	if p.Config.Spec.K0s.DynamicConfig {
 		if h == p.leader && h.Metadata.K0sRunningVersion == nil {
 			log.Debugf("%s: leader will get a full config on initialize ", h)
-			cfg = p.newBaseConfig.Dup()
+			cfg = hostBaseConfig
 		} else {
 			log.Debugf("%s: using a stripped down config for dynamic config", h)
 			cfg = p.Config.Spec.K0s.NodeConfig()
 		}
 	} else {
-		cfg = p.newBaseConfig.Dup()
+		cfg = hostBaseConfig
 	}
 
 	var addr string

phase/configure_k0s_test.go

@@ -8,11 +8,21 @@ import (
 	"github.com/k0sproject/k0sctl/configurer/linux"
 	"github.com/k0sproject/k0sctl/pkg/apis/k0sctl.k0sproject.io/v1beta1"
 	"github.com/k0sproject/k0sctl/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster"
+	"github.com/k0sproject/rig/v2"
+	"github.com/k0sproject/rig/v2/protocol/ssh"
 	"github.com/k0sproject/version"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/yaml.v2"
 )
 
+func testController(address, privateAddress string) *cluster.Host {
+	return &cluster.Host{
+		Role:            "controller",
+		PrivateAddress:  privateAddress,
+		CompositeConfig: rig.CompositeConfig{SSH: &ssh.Config{Address: address}},
+	}
+}
+
 func TestBuildConfigValidateCommandAddsFeatureGates(t *testing.T) {
 	cfg := &v1beta1.Cluster{
 		Spec: &cluster.Spec{
@@ -45,7 +55,7 @@ func TestConfigForSetsAPIAddressWhenIPv6NodeLocalLBEnabled(t *testing.T) {
 	p := &ConfigureK0s{GenericPhase: GenericPhase{Config: clusterConfig}, newBaseConfig: base}
 	h := &cluster.Host{PrivateAddress: "fc00::101"}
 
-	config, err := p.configFor(h)
+	config, err := p.configFor(h, base.Dup())
 	require.NoError(t, err)
 	require.Equal(t, "fc00::101", apiAddressFromConfig(t, config))
 }
@@ -65,24 +75,101 @@ func TestConfigForLeavesAPIAddressWhenIPv6NodeLocalLBDisabled(t *testing.T) {
 	p := &ConfigureK0s{GenericPhase: GenericPhase{Config: clusterConfig}, newBaseConfig: base}
 	h := &cluster.Host{PrivateAddress: "fc00::102"}
 
-	config, err := p.configFor(h)
+	config, err := p.configFor(h, base.Dup())
 	require.NoError(t, err)
 	require.Empty(t, apiAddressFromConfig(t, config))
 }
 
+func TestControllerSansAppendsOwnAddresses(t *testing.T) {
+	base := []string{"lb.example.com"}
+
+	sans := hostsSans(base, testController("10.0.0.1", "172.16.0.1"))
+	require.Equal(t, []string{"lb.example.com", "10.0.0.1", "172.16.0.1"}, sans)
+
+	sans = hostsSans(base, testController("10.0.0.2", ""))
+	require.Equal(t, []string{"lb.example.com", "10.0.0.2"}, sans)
+}
+
+func TestControllerSansDoesNotMutateBase(t *testing.T) {
+	// build base via append so the slice has spare capacity, which would
+	// expose backing array sharing between the per-controller copies
+	var base []string
+	base = append(base, "lb.example.com")
+
+	sansA := hostsSans(base, testController("10.0.0.1", "172.16.0.1"))
+	sansB := hostsSans(base, testController("10.0.0.2", "172.16.0.2"))
+
+	require.Equal(t, []string{"lb.example.com"}, base)
+	require.Equal(t, []string{"lb.example.com", "10.0.0.1", "172.16.0.1"}, sansA)
+	require.Equal(t, []string{"lb.example.com", "10.0.0.2", "172.16.0.2"}, sansB)
+}
+
+func TestControllerSansDedup(t *testing.T) {
+	base := []string{"10.0.0.1", "172.16.0.1"}
+	sans := hostsSans(base, testController("10.0.0.1", "172.16.0.1"))
+	require.Equal(t, base, sans)
+
+	sans = hostsSans(nil, testController("10.0.0.1", "10.0.0.1"))
+	require.Equal(t, []string{"10.0.0.1"}, sans)
+}
+
+func TestConfigForUsesPerHostSans(t *testing.T) {
+	newBaseConfig := dig.Mapping{
+		"spec": dig.Mapping{
+			"api": dig.Mapping{
+				"sans": []string{"lb.example.com"},
+			},
+		},
+	}
+
+	clusterConfig := &v1beta1.Cluster{Spec: &cluster.Spec{K0s: &cluster.K0s{}}}
+	p := &ConfigureK0s{GenericPhase: GenericPhase{Config: clusterConfig}, newBaseConfig: newBaseConfig}
+
+	hosts := []*cluster.Host{
+		testController("10.0.0.1", "172.16.0.1"),
+		testController("10.0.0.2", "172.16.0.2"),
+	}
+
+	base := []string{"lb.example.com"}
+	for i, h := range hosts {
+		hostBaseConfig := p.newBaseConfig.Dup()
+		hostBaseConfig.DigMapping("spec", "api")["sans"] = hostsSans(base, h)
+
+		config, err := p.configFor(h, hostBaseConfig)
+		require.NoError(t, err)
+
+		expected := []string{"lb.example.com", h.Address(), h.PrivateAddress}
+		require.Equal(t, expected, sansFromConfig(t, config), "host %d", i+1)
+	}
+
+	// shared base config must remain untouched
+	require.Equal(t, []string{"lb.example.com"}, newBaseConfig.Dig("spec", "api", "sans"))
+}
+
 type apiSpec struct {
 	Spec struct {
 		API struct {
-			Address string `yaml:"address"`
+			Address string   `yaml:"address"`
+			Sans    []string `yaml:"sans"`
 		} `yaml:"api"`
 	} `yaml:"spec"`
 }
 
-func apiAddressFromConfig(t *testing.T, cfg string) string {
+func parseAPISpec(t *testing.T, cfg string) apiSpec {
 	t.Helper()
 	parts := strings.SplitN(cfg, "\n", 2)
 	require.Len(t, parts, 2)
 	var parsed apiSpec
 	require.NoError(t, yaml.Unmarshal([]byte(parts[1]), &parsed))
-	return parsed.Spec.API.Address
+	return parsed
+}
+
+func apiAddressFromConfig(t *testing.T, cfg string) string {
+	t.Helper()
+	return parseAPISpec(t, cfg).Spec.API.Address
+}
+
+func sansFromConfig(t *testing.T, cfg string) []string {
+	t.Helper()
+	return parseAPISpec(t, cfg).Spec.API.Sans
 }