From 1af5ea3ceac75dc017acb02230fe060c4b398754 Mon Sep 17 00:00:00 2001 From: Madhav Bhargava Date: Tue, 12 Nov 2024 15:46:03 +0530 Subject: [PATCH] Fix to allow etcd-druid to patch STS when there is only a change of mount-path even if all pods are not up and running (#915) --- internal/common/constants.go | 6 + .../component/clientservice/clientservice.go | 4 +- .../clientservice/clientservice_test.go | 3 +- internal/component/peerservice/peerservice.go | 4 +- .../component/peerservice/peerservice_test.go | 4 +- internal/component/statefulset/builder.go | 26 ++-- internal/component/statefulset/statefulset.go | 13 +- internal/utils/statefulset.go | 38 +++++- internal/utils/statefulset_test.go | 128 ++++++++++++++++++ 9 files changed, 207 insertions(+), 19 deletions(-) diff --git a/internal/common/constants.go b/internal/common/constants.go index d6d1524c3..ea46afef4 100644 --- a/internal/common/constants.go +++ b/internal/common/constants.go @@ -121,8 +121,14 @@ const ( VolumeNameEtcdClientTLS = "etcd-client-tls" // VolumeNameEtcdPeerCA is the name of the volume that contains the CA certificate bundle and CA certificate key used to sign certificates for peer communication. VolumeNameEtcdPeerCA = "etcd-peer-ca" + // OldVolumeNameEtcdPeerCA is the old name of the volume that contains the CA certificate bundle and CA certificate key used to sign certificates for peer communication. + // TODO: (i062009) remove this when all clusters have started to use the new volume names. + OldVolumeNameEtcdPeerCA = "peer-url-ca-etcd" // VolumeNameEtcdPeerServerTLS is the name of the volume that contains the server certificate-key pair used to set up the peer server. VolumeNameEtcdPeerServerTLS = "etcd-peer-server-tls" + // OldVolumeNameEtcdPeerServerTLS is the old name of the volume that contains the server certificate-key pair used to set up the peer server. + // TODO: (i062009) remove this when all clusters have started to use the new volume names. + OldVolumeNameEtcdPeerServerTLS = "peer-url-etcd-server-tls" // VolumeNameBackupRestoreCA is the name of the volume that contains the CA certificate bundle and CA certificate key used to sign certificates for backup-restore communication. VolumeNameBackupRestoreCA = "backup-restore-ca" // VolumeNameBackupRestoreServerTLS is the name of the volume that contains the server certificate-key pair used to set up the backup-restore server. diff --git a/internal/component/clientservice/clientservice.go b/internal/component/clientservice/clientservice.go index 8e740bda5..ccb95bdad 100644 --- a/internal/component/clientservice/clientservice.go +++ b/internal/component/clientservice/clientservice.go @@ -111,7 +111,9 @@ func buildResource(etcd *druidv1alpha1.Etcd, svc *corev1.Service) { svc.OwnerReferences = []metav1.OwnerReference{druidv1alpha1.GetAsOwnerReference(etcd.ObjectMeta)} svc.Spec.Type = corev1.ServiceTypeClusterIP svc.Spec.SessionAffinity = corev1.ServiceAffinityNone - svc.Spec.Selector = druidv1alpha1.GetDefaultLabels(etcd.ObjectMeta) + // Client service should only target StatefulSet pods. Default labels are going to be present on anything that is managed by etcd-druid and started for an etcd cluster. + // Therefore, only using default labels as label selector can cause issues as we have already seen in https://github.com/gardener/etcd-druid/issues/914 + svc.Spec.Selector = utils.MergeMaps(druidv1alpha1.GetDefaultLabels(etcd.ObjectMeta), map[string]string{druidv1alpha1.LabelComponentKey: common.ComponentNameStatefulSet}) svc.Spec.Ports = getPorts(etcd) } diff --git a/internal/component/clientservice/clientservice_test.go b/internal/component/clientservice/clientservice_test.go index 001fcdec1..ed05d1522 100644 --- a/internal/component/clientservice/clientservice_test.go +++ b/internal/component/clientservice/clientservice_test.go @@ -272,6 +272,7 @@ func matchClientService(g *WithT, etcd *druidv1alpha1.Etcd, actualSvc corev1.Ser expectedAnnotations = etcd.Spec.Etcd.ClientService.Annotations expectedLabels = utils.MergeMaps(etcd.Spec.Etcd.ClientService.Labels, druidv1alpha1.GetDefaultLabels(etcdObjMeta)) } + expectedLabelSelector := utils.MergeMaps(druidv1alpha1.GetDefaultLabels(etcdObjMeta), map[string]string{druidv1alpha1.LabelComponentKey: common.ComponentNameStatefulSet}) g.Expect(actualSvc).To(MatchFields(IgnoreExtras, Fields{ "ObjectMeta": MatchFields(IgnoreExtras, Fields{ @@ -284,7 +285,7 @@ func matchClientService(g *WithT, etcd *druidv1alpha1.Etcd, actualSvc corev1.Ser "Spec": MatchFields(IgnoreExtras, Fields{ "Type": Equal(corev1.ServiceTypeClusterIP), "SessionAffinity": Equal(corev1.ServiceAffinityNone), - "Selector": Equal(druidv1alpha1.GetDefaultLabels(etcdObjMeta)), + "Selector": Equal(expectedLabelSelector), "Ports": ConsistOf( Equal(corev1.ServicePort{ Name: "client", diff --git a/internal/component/peerservice/peerservice.go b/internal/component/peerservice/peerservice.go index 966145e9c..49b2dbd30 100644 --- a/internal/component/peerservice/peerservice.go +++ b/internal/component/peerservice/peerservice.go @@ -111,7 +111,9 @@ func buildResource(etcd *druidv1alpha1.Etcd, svc *corev1.Service) { svc.Spec.Type = corev1.ServiceTypeClusterIP svc.Spec.ClusterIP = corev1.ClusterIPNone svc.Spec.SessionAffinity = corev1.ServiceAffinityNone - svc.Spec.Selector = druidv1alpha1.GetDefaultLabels(etcd.ObjectMeta) + // Peer service should only target StatefulSet pods. Default labels are going to be present on anything that is managed by etcd-druid and started for an etcd cluster. + // Therefore, only using default labels as label selector can cause issues as we have already seen in https://github.com/gardener/etcd-druid/issues/914 + svc.Spec.Selector = utils.MergeMaps(druidv1alpha1.GetDefaultLabels(etcd.ObjectMeta), map[string]string{druidv1alpha1.LabelComponentKey: common.ComponentNameStatefulSet}) svc.Spec.PublishNotReadyAddresses = true svc.Spec.Ports = getPorts(etcd) } diff --git a/internal/component/peerservice/peerservice_test.go b/internal/component/peerservice/peerservice_test.go index fee6cf88b..2b79b20ca 100644 --- a/internal/component/peerservice/peerservice_test.go +++ b/internal/component/peerservice/peerservice_test.go @@ -13,6 +13,7 @@ import ( "github.com/gardener/etcd-druid/internal/common" "github.com/gardener/etcd-druid/internal/component" druiderr "github.com/gardener/etcd-druid/internal/errors" + "github.com/gardener/etcd-druid/internal/utils" testutils "github.com/gardener/etcd-druid/test/utils" "github.com/go-logr/logr" @@ -246,6 +247,7 @@ func newPeerService(etcd *druidv1alpha1.Etcd) *corev1.Service { func matchPeerService(g *WithT, etcd *druidv1alpha1.Etcd, actualSvc corev1.Service) { peerPort := ptr.Deref(etcd.Spec.Etcd.ServerPort, common.DefaultPortEtcdPeer) etcdObjMeta := etcd.ObjectMeta + expectedLabelSelector := utils.MergeMaps(druidv1alpha1.GetDefaultLabels(etcdObjMeta), map[string]string{druidv1alpha1.LabelComponentKey: common.ComponentNameStatefulSet}) g.Expect(actualSvc).To(MatchFields(IgnoreExtras, Fields{ "ObjectMeta": MatchFields(IgnoreExtras, Fields{ "Name": Equal(druidv1alpha1.GetPeerServiceName(etcdObjMeta)), @@ -257,7 +259,7 @@ func matchPeerService(g *WithT, etcd *druidv1alpha1.Etcd, actualSvc corev1.Servi "Type": Equal(corev1.ServiceTypeClusterIP), "ClusterIP": Equal(corev1.ClusterIPNone), "SessionAffinity": Equal(corev1.ServiceAffinityNone), - "Selector": Equal(druidv1alpha1.GetDefaultLabels(etcdObjMeta)), + "Selector": Equal(expectedLabelSelector), "Ports": ConsistOf( Equal(corev1.ServicePort{ Name: "peer", diff --git a/internal/component/statefulset/builder.go b/internal/component/statefulset/builder.go index 743cdaf23..f2c921def 100644 --- a/internal/component/statefulset/builder.go +++ b/internal/component/statefulset/builder.go @@ -708,8 +708,22 @@ func getEtcdContainerSecretVolumeMounts(etcd *druidv1alpha1.Etcd) []corev1.Volum }, ) } - if etcd.Spec.Etcd.PeerUrlTLS != nil { + secretVolumeMounts = append(secretVolumeMounts, getEtcdContainerPeerVolumeMounts(etcd)...) + if etcd.Spec.Backup.TLS != nil { secretVolumeMounts = append(secretVolumeMounts, + corev1.VolumeMount{ + Name: common.VolumeNameBackupRestoreCA, + MountPath: common.VolumeMountPathBackupRestoreCA, + }, + ) + } + return secretVolumeMounts +} + +func getEtcdContainerPeerVolumeMounts(etcd *druidv1alpha1.Etcd) []corev1.VolumeMount { + peerTLSVolMounts := make([]corev1.VolumeMount, 0, 2) + if etcd.Spec.Etcd.PeerUrlTLS != nil { + peerTLSVolMounts = append(peerTLSVolMounts, corev1.VolumeMount{ Name: common.VolumeNameEtcdPeerCA, MountPath: common.VolumeMountPathEtcdPeerCA, @@ -720,15 +734,7 @@ func getEtcdContainerSecretVolumeMounts(etcd *druidv1alpha1.Etcd) []corev1.Volum }, ) } - if etcd.Spec.Backup.TLS != nil { - secretVolumeMounts = append(secretVolumeMounts, - corev1.VolumeMount{ - Name: common.VolumeNameBackupRestoreCA, - MountPath: common.VolumeMountPathBackupRestoreCA, - }, - ) - } - return secretVolumeMounts + return peerTLSVolMounts } // getPodVolumes gets volumes that needs to be mounted onto the etcd StatefulSet pods diff --git a/internal/component/statefulset/statefulset.go b/internal/component/statefulset/statefulset.go index fc575ff98..e21948215 100644 --- a/internal/component/statefulset/statefulset.go +++ b/internal/component/statefulset/statefulset.go @@ -266,7 +266,7 @@ func (r _resource) handleTLSChanges(ctx component.OperatorContext, etcd *druidv1 // and the cluster will be stuck in a bad state. Updating peer URL is a cluster wide operation as all members will need to know that a peer TLS has changed. // If not all members are ready then rolling-update of StatefulSet can potentially cause a healthy node to be restarted causing loss of quorum from which // there will not be an automatic recovery. - if shouldRequeueForMultiNodeEtcdIfPodsNotReady(existingSts) { + if !r.hasTLSEnablementForPeerURLReflectedOnSTS(etcd, existingSts) && shouldRequeueForMultiNodeEtcdIfPodsNotReady(existingSts) { return druiderr.New( druiderr.ErrRequeueAfter, component.OperationSync, @@ -309,10 +309,17 @@ func shouldRequeueForMultiNodeEtcdIfPodsNotReady(sts *appsv1.StatefulSet) bool { sts.Status.ReadyReplicas < *sts.Spec.Replicas } -func isStatefulSetTLSConfigInSync(etcd *druidv1alpha1.Etcd, sts *appsv1.StatefulSet) bool { +func (r _resource) hasTLSEnablementForPeerURLReflectedOnSTS(etcd *druidv1alpha1.Etcd, existingSts *appsv1.StatefulSet) bool { + newEtcdWrapperPeerTLSVolMounts := getEtcdContainerPeerVolumeMounts(etcd) + containerPeerTLSVolMounts := utils.GetEtcdContainerPeerTLSVolumeMounts(existingSts) + r.logger.Info("Checking if peer URL TLS enablement is reflected on StatefulSet", "len(newEtcdWrapperPeerTLSVolMounts)", len(newEtcdWrapperPeerTLSVolMounts), "len(containerPeerTLSVolMounts)", len(containerPeerTLSVolMounts)) + return len(containerPeerTLSVolMounts) == len(newEtcdWrapperPeerTLSVolMounts) +} + +func isStatefulSetTLSConfigInSync(etcd *druidv1alpha1.Etcd, existingSts *appsv1.StatefulSet) bool { newEtcdbrTLSVolMounts := getBackupRestoreContainerSecretVolumeMounts(etcd) newEtcdWrapperTLSVolMounts := getEtcdContainerSecretVolumeMounts(etcd) - containerTLSVolMounts := utils.GetStatefulSetContainerTLSVolumeMounts(sts) + containerTLSVolMounts := utils.GetStatefulSetContainerTLSVolumeMounts(existingSts) return !hasTLSVolumeMountsChanged(containerTLSVolMounts[common.ContainerNameEtcd], newEtcdWrapperTLSVolMounts) && !hasTLSVolumeMountsChanged(containerTLSVolMounts[common.ContainerNameEtcdBackupRestore], newEtcdbrTLSVolMounts) } diff --git a/internal/utils/statefulset.go b/internal/utils/statefulset.go index b87f3234f..316a6e160 100644 --- a/internal/utils/statefulset.go +++ b/internal/utils/statefulset.go @@ -92,13 +92,47 @@ func FetchPVCWarningMessagesForStatefulSet(ctx context.Context, cl client.Client } var ( - etcdTLSVolumeMountNames = sets.New[string](common.VolumeNameEtcdCA, common.VolumeNameEtcdServerTLS, common.VolumeNameEtcdClientTLS, common.VolumeNameEtcdPeerCA, common.VolumeNameEtcdPeerServerTLS, common.VolumeNameBackupRestoreCA) - etcdbrTLSVolumeMountNames = sets.New[string](common.VolumeNameBackupRestoreServerTLS, common.VolumeNameEtcdCA, common.VolumeNameEtcdClientTLS) + etcdTLSVolumeMountNames = sets.New[string](common.VolumeNameEtcdCA, + common.VolumeNameEtcdServerTLS, + common.VolumeNameEtcdClientTLS, + common.VolumeNameEtcdPeerCA, + common.VolumeNameEtcdPeerServerTLS, + common.VolumeNameBackupRestoreCA) + possiblePeerTLSVolumeMountNames = sets.New[string](common.VolumeNameEtcdPeerCA, + common.OldVolumeNameEtcdPeerCA, + common.VolumeNameEtcdPeerServerTLS, + common.OldVolumeNameEtcdPeerServerTLS) + etcdbrTLSVolumeMountNames = sets.New[string](common.VolumeNameBackupRestoreServerTLS, + common.VolumeNameEtcdCA, + common.VolumeNameEtcdClientTLS) ) +// GetEtcdContainerPeerTLSVolumeMounts returns the volume mounts for the etcd container that are related to peer TLS. +// It will look at both older names (present in version <= v0.22) and new names (present in version >= v0.23) to create the slice. +func GetEtcdContainerPeerTLSVolumeMounts(sts *appsv1.StatefulSet) []corev1.VolumeMount { + volumeMounts := make([]corev1.VolumeMount, 0, 2) + if sts == nil { + return volumeMounts + } + for _, container := range sts.Spec.Template.Spec.Containers { + if container.Name == common.ContainerNameEtcd { + for _, volMount := range container.VolumeMounts { + if possiblePeerTLSVolumeMountNames.Has(volMount.Name) { + volumeMounts = append(volumeMounts, volMount) + } + } + break + } + } + return volumeMounts +} + // GetStatefulSetContainerTLSVolumeMounts returns a map of container name to TLS volume mounts for the given StatefulSet. func GetStatefulSetContainerTLSVolumeMounts(sts *appsv1.StatefulSet) map[string][]corev1.VolumeMount { containerVolMounts := make(map[string][]corev1.VolumeMount, 2) // each pod is a 2 container pod. Init containers are not counted as containers. + if sts == nil { + return containerVolMounts + } for _, container := range sts.Spec.Template.Spec.Containers { if _, ok := containerVolMounts[container.Name]; !ok { // Assuming 6 volume mounts per container. If there are more in future then this map's capacity will be increased by the golang runtime. diff --git a/internal/utils/statefulset_test.go b/internal/utils/statefulset_test.go index 38079a23f..9d4568f13 100644 --- a/internal/utils/statefulset_test.go +++ b/internal/utils/statefulset_test.go @@ -12,6 +12,7 @@ import ( "testing" "github.com/gardener/etcd-druid/internal/client/kubernetes" + "github.com/gardener/etcd-druid/internal/common" testutils "github.com/gardener/etcd-druid/test/utils" appsv1 "k8s.io/api/apps/v1" @@ -270,3 +271,130 @@ func TestFetchPVCWarningMessagesForStatefulSet(t *testing.T) { }) } } + +func TestGetEtcdContainerPeerTLSVolumeMounts(t *testing.T) { + testCases := []struct { + name string + isSTSNil bool + oldVolMountNames bool + expectedVolumeMounts []corev1.VolumeMount + }{ + { + name: "sts is nil", + isSTSNil: true, + expectedVolumeMounts: []corev1.VolumeMount{}, + }, + { + name: "sts with old volume mount names", + oldVolMountNames: true, + expectedVolumeMounts: []corev1.VolumeMount{ + {Name: common.OldVolumeNameEtcdPeerCA, MountPath: common.VolumeMountPathEtcdPeerCA}, + {Name: common.OldVolumeNameEtcdPeerServerTLS, MountPath: common.VolumeMountPathEtcdPeerServerTLS}, + }, + }, + { + name: "sts with new volume mount names", + expectedVolumeMounts: []corev1.VolumeMount{ + {Name: common.VolumeNameEtcdPeerCA, MountPath: common.VolumeMountPathEtcdPeerCA}, + {Name: common.VolumeNameEtcdPeerServerTLS, MountPath: common.VolumeMountPathEtcdPeerServerTLS}, + }, + }, + } + g := NewWithT(t) + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var sts *appsv1.StatefulSet + if tc.isSTSNil { + g.Expect(GetEtcdContainerPeerTLSVolumeMounts(sts)).To(HaveLen(0)) + } else { + sts = testutils.CreateStatefulSet("test-sts", "test-ns", uuid.NewUUID(), 3) + sts.Spec.Template.Spec.Containers = append(sts.Spec.Template.Spec.Containers, corev1.Container{Name: common.ContainerNameEtcd}) + if tc.oldVolMountNames { + sts.Spec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ + {Name: common.OldVolumeNameEtcdPeerCA, MountPath: common.VolumeMountPathEtcdPeerCA}, + {Name: common.OldVolumeNameEtcdPeerServerTLS, MountPath: common.VolumeMountPathEtcdPeerServerTLS}, + } + } else { + sts.Spec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ + {Name: common.VolumeNameEtcdPeerCA, MountPath: common.VolumeMountPathEtcdPeerCA}, + {Name: common.VolumeNameEtcdPeerServerTLS, MountPath: common.VolumeMountPathEtcdPeerServerTLS}, + } + } + g.Expect(GetEtcdContainerPeerTLSVolumeMounts(sts)).To(Equal(tc.expectedVolumeMounts)) + } + }) + } +} + +func TestGetStatefulSetContainerTLSVolumeMounts(t *testing.T) { + testCases := []struct { + name string + isSTSNil bool + peerTLSEnabled bool + expectedVolumeMounts map[string][]corev1.VolumeMount + }{ + { + name: "sts is nil", + isSTSNil: true, + expectedVolumeMounts: map[string][]corev1.VolumeMount{}, + }, + { + name: "sts with peer TLS enabled", + peerTLSEnabled: true, + expectedVolumeMounts: map[string][]corev1.VolumeMount{ + common.ContainerNameEtcd: { + {Name: common.VolumeNameEtcdPeerCA, MountPath: common.VolumeMountPathEtcdPeerCA}, + {Name: common.VolumeNameEtcdPeerServerTLS, MountPath: common.VolumeMountPathEtcdPeerServerTLS}, + }, + common.ContainerNameEtcdBackupRestore: { + {Name: common.VolumeNameBackupRestoreServerTLS, MountPath: common.VolumeMountPathBackupRestoreServerTLS}, + {Name: common.VolumeNameEtcdCA, MountPath: common.VolumeMountPathEtcdCA}, + {Name: common.VolumeNameEtcdClientTLS, MountPath: common.VolumeMountPathEtcdClientTLS}, + }, + }, + }, + { + name: "sts with peer TLS disabled", + peerTLSEnabled: false, + expectedVolumeMounts: map[string][]corev1.VolumeMount{ + common.ContainerNameEtcd: {}, + common.ContainerNameEtcdBackupRestore: { + {Name: common.VolumeNameBackupRestoreServerTLS, MountPath: common.VolumeMountPathBackupRestoreServerTLS}, + {Name: common.VolumeNameEtcdCA, MountPath: common.VolumeMountPathEtcdCA}, + {Name: common.VolumeNameEtcdClientTLS, MountPath: common.VolumeMountPathEtcdClientTLS}, + }, + }, + }, + } + g := NewWithT(t) + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var sts *appsv1.StatefulSet + if tc.isSTSNil { + g.Expect(GetStatefulSetContainerTLSVolumeMounts(sts)).To(HaveLen(0)) + } else { + sts = testutils.CreateStatefulSet("test-sts", "test-ns", uuid.NewUUID(), 3) + sts.Spec.Template.Spec.Containers = []corev1.Container{ + {Name: common.ContainerNameEtcd}, + {Name: common.ContainerNameEtcdBackupRestore}, + } + if tc.peerTLSEnabled { + sts.Spec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ + {Name: common.VolumeNameEtcdPeerCA, MountPath: common.VolumeMountPathEtcdPeerCA}, + {Name: common.VolumeNameEtcdPeerServerTLS, MountPath: common.VolumeMountPathEtcdPeerServerTLS}, + } + } + sts.Spec.Template.Spec.Containers[1].VolumeMounts = []corev1.VolumeMount{ + {Name: common.VolumeNameBackupRestoreServerTLS, MountPath: common.VolumeMountPathBackupRestoreServerTLS}, + {Name: common.VolumeNameEtcdCA, MountPath: common.VolumeMountPathEtcdCA}, + {Name: common.VolumeNameEtcdClientTLS, MountPath: common.VolumeMountPathEtcdClientTLS}, + } + g.Expect(GetStatefulSetContainerTLSVolumeMounts(sts)).To(Equal(tc.expectedVolumeMounts)) + } + }) + } +}