fix: reset certificate SANs on update

This affects both API server and Talos API cert SANs. Before the fix, SANs accumulated changes over time, so even if the hostname changes, old hostname is still kept in SANs. Even though it shouldn't be a problem in general, it is confusing as after reboot list will be reset back to expected value. Signed-off-by: Andrey Smirnov <andrey.smirnov@talos-systems.com>
siderolabs · May 12, 2022 · afb6795 · afb6795
1 parent c87432f
commit afb6795
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 1 deletion.
diff --git a/internal/app/machined/pkg/controllers/secrets/api_cert_sans.go b/internal/app/machined/pkg/controllers/secrets/api_cert_sans.go
@@ -115,6 +115,8 @@ func (ctrl *APICertSANsController) Run(ctx context.Context, r controller.Runtime
 		if err = r.Modify(ctx, secrets.NewCertSAN(secrets.NamespaceName, secrets.CertSANAPIID), func(r resource.Resource) error {
 			spec := r.(*secrets.CertSAN).TypedSpec()
 
+			spec.Reset()
+
 			spec.AppendIPs(apiRoot.CertSANIPs...)
 			spec.AppendIPs(nodeAddresses.IPs()...)
 

diff --git a/internal/app/machined/pkg/controllers/secrets/api_cert_sans_test.go b/internal/app/machined/pkg/controllers/secrets/api_cert_sans_test.go
@@ -9,6 +9,7 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"reflect"
 	"sync"
 	"testing"
 	"time"
@@ -118,6 +119,42 @@ func (suite *APICertSANsSuite) TestReconcileControlPlane() {
 			},
 		),
 	)
+
+	_, err := suite.state.UpdateWithConflicts(suite.ctx, rootSecrets.Metadata(), func(r resource.Resource) error {
+		r.(*secrets.OSRoot).TypedSpec().CertSANDNSNames = []string{"other.org"}
+
+		return nil
+	})
+	suite.Require().NoError(err)
+
+	suite.Assert().NoError(
+		retry.Constant(10*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
+			func() error {
+				certSANs, err := suite.state.Get(
+					suite.ctx,
+					resource.NewMetadata(
+						secrets.NamespaceName,
+						secrets.CertSANType,
+						secrets.CertSANAPIID,
+						resource.VersionUndefined,
+					),
+				)
+				if err != nil {
+					return err
+				}
+
+				spec := certSANs.(*secrets.CertSAN).TypedSpec()
+
+				expectedDNSNames := []string{"bar", "bar.some.org", "other.org"}
+
+				if !reflect.DeepEqual(expectedDNSNames, spec.DNSNames) {
+					return retry.ExpectedErrorf("expected %v, got %v", expectedDNSNames, spec.DNSNames)
+				}
+
+				return nil
+			},
+		),
+	)
 }
 
 func (suite *APICertSANsSuite) TearDownTest() {

diff --git a/internal/app/machined/pkg/controllers/secrets/kubernetes_cert_sans.go b/internal/app/machined/pkg/controllers/secrets/kubernetes_cert_sans.go
@@ -115,6 +115,8 @@ func (ctrl *KubernetesCertSANsController) Run(ctx context.Context, r controller.
 		if err = r.Modify(ctx, secrets.NewCertSAN(secrets.NamespaceName, secrets.CertSANKubernetesID), func(r resource.Resource) error {
 			spec := r.(*secrets.CertSAN).TypedSpec()
 
+			spec.Reset()
+
 			spec.Append(k8sRoot.Endpoint.Hostname())
 			spec.Append(k8sRoot.CertSANs...)
 

diff --git a/internal/app/machined/pkg/controllers/secrets/kubernetes_cert_sans_test.go b/internal/app/machined/pkg/controllers/secrets/kubernetes_cert_sans_test.go
@@ -11,6 +11,7 @@ import (
 	"log"
 	"net"
 	"net/url"
+	"reflect"
 	"sync"
 	"testing"
 	"time"
@@ -99,7 +100,9 @@ func (suite *KubernetesCertSANsSuite) TestReconcile() {
 	suite.Assert().NoError(
 		retry.Constant(10*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
 			func() error {
-				certSANs, err := suite.state.Get(
+				var certSANs resource.Resource
+
+				certSANs, err = suite.state.Get(
 					suite.ctx,
 					resource.NewMetadata(
 						secrets.NamespaceName,
@@ -137,6 +140,54 @@ func (suite *KubernetesCertSANsSuite) TestReconcile() {
 			},
 		),
 	)
+
+	_, err = suite.state.UpdateWithConflicts(suite.ctx, rootSecrets.Metadata(), func(r resource.Resource) error {
+		r.(*secrets.KubernetesRoot).TypedSpec().Endpoint, err = url.Parse("https://some.other.url:6443/")
+
+		return err
+	})
+	suite.Require().NoError(err)
+
+	suite.Assert().NoError(
+		retry.Constant(10*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
+			func() error {
+				var certSANs resource.Resource
+
+				certSANs, err = suite.state.Get(
+					suite.ctx,
+					resource.NewMetadata(
+						secrets.NamespaceName,
+						secrets.CertSANType,
+						secrets.CertSANKubernetesID,
+						resource.VersionUndefined,
+					),
+				)
+				if err != nil {
+					return err
+				}
+
+				spec := certSANs.(*secrets.CertSAN).TypedSpec()
+
+				expectedDNSNames := []string{
+					"example.com",
+					"foo",
+					"foo.example.com",
+					"kubernetes",
+					"kubernetes.default",
+					"kubernetes.default.svc",
+					"kubernetes.default.svc.cluster.remote",
+					"localhost",
+					"some.other.url",
+				}
+
+				if !reflect.DeepEqual(spec.DNSNames, expectedDNSNames) {
+					return retry.ExpectedErrorf("expected %v, got %v", expectedDNSNames, spec.DNSNames)
+				}
+
+				return nil
+			},
+		),
+	)
 }
 
 func (suite *KubernetesCertSANsSuite) TearDownTest() {

diff --git a/pkg/machinery/resources/secrets/cert_sans.go b/pkg/machinery/resources/secrets/cert_sans.go
@@ -63,6 +63,13 @@ func (spec CertSANSpec) DeepCopy() CertSANSpec {
 	}
 }
 
+// Reset the list of SANs.
+func (spec *CertSANSpec) Reset() {
+	spec.DNSNames = nil
+	spec.IPs = nil
+	spec.FQDN = ""
+}
+
 // Append list of SANs splitting into IPs/DNS names.
 func (spec *CertSANSpec) Append(sans ...string) {
 	for _, san := range sans {