Improve KCP etcd client crt/key caching

Signed-off-by: Stefan Büringer [email protected]

Author: sbueringer

controlplane/kubeadm/internal/cluster.go

@@ -27,6 +27,7 @@ import (
 	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -44,7 +45,7 @@ type ManagementCluster interface {
 
 	GetMachinesForCluster(ctx context.Context, cluster *clusterv1.Cluster, filters ...collections.Func) (collections.Machines, error)
 	GetMachinePoolsForCluster(ctx context.Context, cluster *clusterv1.Cluster) (*clusterv1.MachinePoolList, error)
-	GetWorkloadCluster(ctx context.Context, clusterKey client.ObjectKey, keyEncryptionAlgorithm bootstrapv1.EncryptionAlgorithmType) (WorkloadCluster, error)
+	GetWorkloadCluster(ctx context.Context, cluster *clusterv1.Cluster, keyEncryptionAlgorithm bootstrapv1.EncryptionAlgorithmType) (WorkloadCluster, error)
 }
 
 // Management holds operations on the management cluster.
@@ -61,13 +62,14 @@ type Management struct {
 // ClientCertEntry is an Entry for the Cache that stores the client cert.
 type ClientCertEntry struct {
 	Cluster             client.ObjectKey
+	ClusterUID          types.UID
 	ClientCert          *tls.Certificate
 	EncryptionAlgorithm bootstrapv1.EncryptionAlgorithmType
 }
 
 // Key returns the cache key of a ClientCertEntry.
 func (r ClientCertEntry) Key() string {
-	return fmt.Sprintf("%s/%s", r.Cluster.String(), r.EncryptionAlgorithm)
+	return fmt.Sprintf("%s/%s/%s", r.Cluster.String(), r.ClusterUID, r.EncryptionAlgorithm)
 }
 
 // RemoteClusterConnectionError represents a failure to connect to a remote cluster.
@@ -113,7 +115,9 @@ func (m *Management) GetMachinePoolsForCluster(ctx context.Context, cluster *clu
 
 // GetWorkloadCluster builds a cluster object.
 // The cluster comes with an etcd client generator to connect to any etcd pod living on a managed machine.
-func (m *Management) GetWorkloadCluster(ctx context.Context, clusterKey client.ObjectKey, keyEncryptionAlgorithm bootstrapv1.EncryptionAlgorithmType) (WorkloadCluster, error) {
+func (m *Management) GetWorkloadCluster(ctx context.Context, cluster *clusterv1.Cluster, keyEncryptionAlgorithm bootstrapv1.EncryptionAlgorithmType) (WorkloadCluster, error) {
+	clusterKey := client.ObjectKeyFromObject(cluster)
+
 	// TODO(chuckha): Inject this dependency.
 	// TODO(chuckha): memoize this function. The workload client only exists as long as a reconciliation loop.
 	restConfig, err := m.ClusterCache.GetRESTConfig(ctx, clusterKey)
@@ -142,15 +146,16 @@ func (m *Management) GetWorkloadCluster(ctx context.Context, clusterKey client.O
 	var clientCert tls.Certificate
 	if keyData != nil {
 		// Get client cert from cache if possible, otherwise generate it and add it to the cache.
-		if entry, ok := m.ClientCertCache.Has(ClientCertEntry{Cluster: clusterKey, EncryptionAlgorithm: keyEncryptionAlgorithm}.Key()); ok {
+		// Note: The caching assumes that the etcd CA is not rotated during the lifetime of a Cluster.
+		if entry, ok := m.ClientCertCache.Has(ClientCertEntry{Cluster: clusterKey, ClusterUID: cluster.UID, EncryptionAlgorithm: keyEncryptionAlgorithm}.Key()); ok {
 			clientCert = *entry.ClientCert
 		} else {
 			// The client cert expires after 10 years, but that's okay as the cache has a TTL of 1 day.
 			clientCert, err = generateClientCert(crtData, keyData, keyEncryptionAlgorithm)
 			if err != nil {
 				return nil, err
 			}
-			m.ClientCertCache.Add(ClientCertEntry{Cluster: clusterKey, ClientCert: &clientCert, EncryptionAlgorithm: keyEncryptionAlgorithm})
+			m.ClientCertCache.Add(ClientCertEntry{Cluster: clusterKey, ClusterUID: cluster.UID, ClientCert: &clientCert, EncryptionAlgorithm: keyEncryptionAlgorithm})
 		}
 	} else {
 		clientCert, err = m.getAPIServerEtcdClientCert(ctx, clusterKey)

controlplane/kubeadm/internal/cluster_test.go

@@ -161,10 +161,6 @@ func TestGetWorkloadCluster(t *testing.T) {
 			secret.KubeconfigDataName: testEnvKubeconfig,
 		},
 	}
-	clusterKey := client.ObjectKey{
-		Name:      "my-cluster",
-		Namespace: ns.Name,
-	}
 
 	tests := []struct {
 		name       string
@@ -173,40 +169,34 @@ func TestGetWorkloadCluster(t *testing.T) {
 		expectErr  bool
 	}{
 		{
-			name:       "returns a workload cluster",
-			clusterKey: clusterKey,
-			objs:       []client.Object{etcdSecret.DeepCopy(), kubeconfigSecret.DeepCopy()},
-			expectErr:  false,
+			name:      "returns a workload cluster",
+			objs:      []client.Object{etcdSecret.DeepCopy(), kubeconfigSecret.DeepCopy()},
+			expectErr: false,
 		},
 		{
-			name:       "returns error if cannot get rest.Config from kubeconfigSecret",
-			clusterKey: clusterKey,
-			objs:       []client.Object{etcdSecret.DeepCopy()},
-			expectErr:  true,
+			name:      "returns error if cannot get rest.Config from kubeconfigSecret",
+			objs:      []client.Object{etcdSecret.DeepCopy()},
+			expectErr: true,
 		},
 		{
-			name:       "returns error if unable to find the etcd secret",
-			clusterKey: clusterKey,
-			objs:       []client.Object{kubeconfigSecret.DeepCopy()},
-			expectErr:  true,
+			name:      "returns error if unable to find the etcd secret",
+			objs:      []client.Object{kubeconfigSecret.DeepCopy()},
+			expectErr: true,
 		},
 		{
-			name:       "returns error if unable to find the certificate in the etcd secret",
-			clusterKey: clusterKey,
-			objs:       []client.Object{emptyCrtEtcdSecret.DeepCopy(), kubeconfigSecret.DeepCopy()},
-			expectErr:  true,
+			name:      "returns error if unable to find the certificate in the etcd secret",
+			objs:      []client.Object{emptyCrtEtcdSecret.DeepCopy(), kubeconfigSecret.DeepCopy()},
+			expectErr: true,
 		},
 		{
-			name:       "returns error if unable to find the key in the etcd secret",
-			clusterKey: clusterKey,
-			objs:       []client.Object{emptyKeyEtcdSecret.DeepCopy(), kubeconfigSecret.DeepCopy()},
-			expectErr:  true,
+			name:      "returns error if unable to find the key in the etcd secret",
+			objs:      []client.Object{emptyKeyEtcdSecret.DeepCopy(), kubeconfigSecret.DeepCopy()},
+			expectErr: true,
 		},
 		{
-			name:       "returns error if unable to generate client cert",
-			clusterKey: clusterKey,
-			objs:       []client.Object{badCrtEtcdSecret.DeepCopy(), kubeconfigSecret.DeepCopy()},
-			expectErr:  true,
+			name:      "returns error if unable to generate client cert",
+			objs:      []client.Object{badCrtEtcdSecret.DeepCopy(), kubeconfigSecret.DeepCopy()},
+			expectErr: true,
 		},
 	}
 
@@ -250,7 +240,7 @@ func TestGetWorkloadCluster(t *testing.T) {
 			})
 			g.Expect(err).ToNot(HaveOccurred())
 
-			workloadCluster, err := m.GetWorkloadCluster(ctx, tt.clusterKey, bootstrapv1.EncryptionAlgorithmRSA2048)
+			workloadCluster, err := m.GetWorkloadCluster(ctx, cluster, bootstrapv1.EncryptionAlgorithmRSA2048)
 			if tt.expectErr {
 				g.Expect(err).To(HaveOccurred())
 				g.Expect(workloadCluster).To(BeNil())

controlplane/kubeadm/internal/control_plane.go

@@ -394,7 +394,7 @@ func (c *ControlPlane) GetWorkloadCluster(ctx context.Context) (WorkloadCluster,
 		return c.workloadCluster, nil
 	}
 
-	workloadCluster, err := c.managementCluster.GetWorkloadCluster(ctx, client.ObjectKeyFromObject(c.Cluster), c.GetKeyEncryptionAlgorithm())
+	workloadCluster, err := c.managementCluster.GetWorkloadCluster(ctx, c.Cluster, c.GetKeyEncryptionAlgorithm())
 	if err != nil {
 		return nil, err
 	}

controlplane/kubeadm/internal/controllers/fakes_test.go

@@ -49,7 +49,7 @@ func (f *fakeManagementCluster) List(ctx context.Context, list client.ObjectList
 	return f.Reader.List(ctx, list, opts...)
 }
 
-func (f *fakeManagementCluster) GetWorkloadCluster(_ context.Context, _ client.ObjectKey, _ bootstrapv1.EncryptionAlgorithmType) (internal.WorkloadCluster, error) {
+func (f *fakeManagementCluster) GetWorkloadCluster(_ context.Context, _ *clusterv1.Cluster, _ bootstrapv1.EncryptionAlgorithmType) (internal.WorkloadCluster, error) {
 	return f.Workload, f.WorkloadErr
 }