Merge pull request #1535 from tesshuflower/cherry-pick-release-0.12-1…

…519-resourcenamelimit-63chars

[release-0.12] Merge pull request #1519 from tesshuflower/resourcenamelimit_63chars

CHANGELOG.md

@@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - All movers should return error if not able to EnsurePVCFromSrc
+- Fix for mover job/service name length too long (>63 chars) if the
+  replicationsource or replicationdestination CR name is too long
 
 ### Security
 

controllers/mover/rclone/mover.go

@@ -226,7 +226,7 @@ func (m *Mover) ensureJob(ctx context.Context, dataPVC *corev1.PersistentVolumeC
 
 	job := &batchv1.Job{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      mover.VolSyncPrefix + "rclone-" + dir + "-" + m.owner.GetName(),
+			Name:      utils.GetJobName(mover.VolSyncPrefix+"rclone-"+dir+"-", m.owner),
 			Namespace: m.owner.GetNamespace(),
 		},
 	}

controllers/mover/rclone/rclone_test.go

@@ -1157,6 +1157,30 @@ var _ = Describe("Rclone as a source", func() {
 					Expect(job.Spec.Template.Spec.Containers[0].Resources).To(Equal(corev1.ResourceRequirements{}))
 				})
 
+				When("The ReplicationSource CR name is very long", func() {
+					BeforeEach(func() {
+						rs.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
+					})
+
+					It("The job name should be shortened appropriately (should handle long CR names)", func() {
+						j, e := mover.ensureJob(ctx, sPVC, sa, rcloneConfigSecret, nil) // Using sPVC as dataPVC (i.e. direct)
+						Expect(e).NotTo(HaveOccurred())
+						Expect(j).To(BeNil()) // hasn't completed
+
+						jobs := &batchv1.JobList{}
+						Expect(k8sClient.List(ctx, jobs, client.InNamespace(rs.Namespace))).To(Succeed())
+						Expect(len(jobs.Items)).To(Equal(1))
+						moverJob := jobs.Items[0]
+
+						// Reload the replicationsource to see that it got updated
+						Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(rs), rs)).To(Succeed())
+
+						Expect(moverJob.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
+						// Make sure our shortened name is actually short enough
+						Expect(len(moverJob.GetName()) > 63).To(BeFalse())
+					})
+				})
+
 				When("moverResources (resource requirements) are provided", func() {
 					BeforeEach(func() {
 						rs.Spec.Rclone.MoverResources = &corev1.ResourceRequirements{

controllers/mover/restic/mover.go

@@ -290,7 +290,7 @@ func (m *Mover) ensureJob(ctx context.Context, cachePVC *corev1.PersistentVolume
 
 	job := &batchv1.Job{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      mover.VolSyncPrefix + dir + "-" + m.owner.GetName(),
+			Name:      utils.GetJobName(mover.VolSyncPrefix+dir+"-", m.owner),
 			Namespace: m.owner.GetNamespace(),
 		},
 	}

controllers/mover/restic/restic_test.go

@@ -1335,6 +1335,31 @@ var _ = Describe("Restic as a source", func() {
 					Expect(psc.RunAsUser).To(BeNil())
 					Expect(psc.FSGroup).To(BeNil())
 				})
+
+				When("The ReplicationSource CR name is very long", func() {
+					BeforeEach(func() {
+						rs.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
+					})
+
+					It("The job name should be shortened appropriately (should handle long CR names)", func() {
+						j, e := mover.ensureJob(ctx, cache, sPVC, sa, repo, nil)
+						Expect(e).NotTo(HaveOccurred())
+						Expect(j).To(BeNil()) // hasn't completed
+
+						jobs := &batchv1.JobList{}
+						Expect(k8sClient.List(ctx, jobs, client.InNamespace(rs.Namespace))).To(Succeed())
+						Expect(len(jobs.Items)).To(Equal(1))
+						moverJob := jobs.Items[0]
+
+						// Reload the replicationsource to see that it got updated
+						Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(rs), rs)).To(Succeed())
+
+						Expect(moverJob.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
+						// Make sure our shortened name is actually short enough
+						Expect(len(moverJob.GetName()) > 63).To(BeFalse())
+					})
+				})
+
 				When("A moverSecurityContext is provided", func() {
 					BeforeEach(func() {
 						rs.Spec.Restic.MoverSecurityContext = &corev1.PodSecurityContext{

controllers/mover/rsync/mover.go

@@ -149,7 +149,7 @@ func (m *Mover) ensureServiceAndPublishAddress(ctx context.Context) (bool, error
 
 	service := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      volSyncRsyncPrefix + m.direction() + "-" + m.owner.GetName(),
+			Name:      utils.GetServiceName(volSyncRsyncPrefix+m.direction()+"-", m.owner),
 			Namespace: m.owner.GetNamespace(),
 		},
 	}
@@ -279,7 +279,7 @@ func (m *Mover) direction() string {
 
 func (m *Mover) serviceSelector() map[string]string {
 	return map[string]string{
-		"app.kubernetes.io/name":      m.direction() + "-" + m.owner.GetName(),
+		"app.kubernetes.io/name":      utils.GetOwnerNameLabelValue(m.direction()+"-", m.owner),
 		"app.kubernetes.io/component": "rsync-mover",
 		"app.kubernetes.io/part-of":   "volsync",
 	}
@@ -355,7 +355,7 @@ func (m *Mover) ensureJob(ctx context.Context, dataPVC *corev1.PersistentVolumeC
 	sa *corev1.ServiceAccount, rsyncSecretName string) (*batchv1.Job, error) {
 	job := &batchv1.Job{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      volSyncRsyncPrefix + m.direction() + "-" + m.owner.GetName(),
+			Name:      utils.GetJobName(volSyncRsyncPrefix+m.direction()+"-", m.owner),
 			Namespace: m.owner.GetNamespace(),
 		},
 	}

controllers/mover/rsync/rsync_test.go

@@ -726,6 +726,30 @@ var _ = Describe("Rsync as a source", func() {
 					Expect(job.Spec.Template.Spec.ServiceAccountName).To(Equal(sa.Name))
 				})
 
+				When("The ReplicationSource CR name is very long", func() {
+					BeforeEach(func() {
+						rs.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
+					})
+
+					It("The job name should be shortened appropriately (should handle long CR names)", func() {
+						j, e := mover.ensureJob(ctx, sPVC, sa, sshKeysSecret.GetName()) // Using sPVC as dataPVC (i.e. direct)
+						Expect(e).NotTo(HaveOccurred())
+						Expect(j).To(BeNil()) // hasn't completed
+
+						jobs := &batchv1.JobList{}
+						Expect(k8sClient.List(ctx, jobs, client.InNamespace(rs.Namespace))).To(Succeed())
+						Expect(len(jobs.Items)).To(Equal(1))
+						moverJob := jobs.Items[0]
+
+						// Reload the replicationsource to see that it got updated
+						Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(rs), rs)).To(Succeed())
+
+						Expect(moverJob.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
+						// Make sure our shortened name is actually short enough
+						Expect(len(moverJob.GetName()) > 63).To(BeFalse())
+					})
+				})
+
 				It("Should not have container resourceRequirements set by default", func() {
 					j, e := mover.ensureJob(ctx, sPVC, sa, sshKeysSecret.GetName()) // Using sPVC as dataPVC (i.e. direct)
 					Expect(e).NotTo(HaveOccurred())
@@ -1283,6 +1307,37 @@ var _ = Describe("Rsync as a destination", func() {
 			})
 		})
 
+		Context("Service handled properly when replicationdestination name is very long", func() {
+			BeforeEach(func() {
+				rd.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
+			})
+
+			It("The service name should be shortened appropriately (should handle long CR names)", func() {
+				// create the svc
+				result, err := mover.ensureServiceAndPublishAddress(ctx)
+				Expect(err).To(BeNil())
+
+				if !result {
+					// This means the svc address wasn't populated immediately
+					// Keep reconciling - when service has address populated it should get updated in the rs status)
+					Eventually(func() bool {
+						gotAddr, err := mover.ensureServiceAndPublishAddress(ctx)
+						return err != nil && gotAddr
+					}, maxWait, interval).Should(BeTrue())
+				}
+
+				// Find the service
+				svcs := &corev1.ServiceList{}
+				Expect(k8sClient.List(ctx, svcs, client.InNamespace(rd.Namespace))).To(Succeed())
+				Expect(len(svcs.Items)).To(Equal(1))
+				rdSvc := svcs.Items[0]
+
+				Expect(rdSvc.GetName()).To(ContainSubstring(utils.GetHashedName(rd.GetName())))
+				// Make sure our shortened name is actually short enough
+				Expect(len(rdSvc.GetName()) > 63).To(BeFalse())
+			})
+		})
+
 		//nolint:dupl
 		Context("Service and address are handled properly", func() {
 			When("when no remote address is specified", func() {

controllers/mover/rsynctls/mover.go

@@ -153,7 +153,7 @@ func (m *Mover) ensureServiceAndPublishAddress(ctx context.Context) (bool, error
 
 	service := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      volSyncRsyncTLSPrefix + m.direction() + "-" + m.owner.GetName(),
+			Name:      utils.GetServiceName(volSyncRsyncTLSPrefix+m.direction()+"-", m.owner),
 			Namespace: m.owner.GetNamespace(),
 		},
 	}
@@ -285,7 +285,7 @@ func (m *Mover) direction() string {
 
 func (m *Mover) serviceSelector() map[string]string {
 	return map[string]string{
-		"app.kubernetes.io/name":      m.direction() + "-" + m.owner.GetName(),
+		"app.kubernetes.io/name":      utils.GetOwnerNameLabelValue(m.direction()+"-", m.owner),
 		"app.kubernetes.io/component": "rsync-tls-mover",
 		"app.kubernetes.io/part-of":   "volsync",
 	}
@@ -361,7 +361,7 @@ func (m *Mover) ensureJob(ctx context.Context, dataPVC *corev1.PersistentVolumeC
 	sa *corev1.ServiceAccount, rsyncSecretName string) (*batchv1.Job, error) {
 	job := &batchv1.Job{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      volSyncRsyncTLSPrefix + m.direction() + "-" + m.owner.GetName(),
+			Name:      utils.GetJobName(volSyncRsyncTLSPrefix+m.direction()+"-", m.owner),
 			Namespace: m.owner.GetNamespace(),
 		},
 	}

controllers/mover/rsynctls/rsync_test.go

@@ -1163,6 +1163,30 @@ var _ = Describe("RsyncTLS as a source", func() {
 					Expect(job.Spec.Template.Spec.ServiceAccountName).To(Equal(sa.Name))
 				})
 
+				When("The ReplicationSource CR name is very long", func() {
+					BeforeEach(func() {
+						rs.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
+					})
+
+					It("The job name should be shortened appropriately (should handle long CR names)", func() {
+						j, e := mover.ensureJob(ctx, sPVC, sa, tlsKeySecret.GetName()) // Using sPVC as dataPVC (i.e. direct)
+						Expect(e).NotTo(HaveOccurred())
+						Expect(j).To(BeNil()) // hasn't completed
+
+						jobs := &batchv1.JobList{}
+						Expect(k8sClient.List(ctx, jobs, client.InNamespace(rs.Namespace))).To(Succeed())
+						Expect(len(jobs.Items)).To(Equal(1))
+						moverJob := jobs.Items[0]
+
+						// Reload the replicationsource to see that it got updated
+						Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(rs), rs)).To(Succeed())
+
+						Expect(moverJob.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
+						// Make sure our shortened name is actually short enough
+						Expect(len(moverJob.GetName()) > 63).To(BeFalse())
+					})
+				})
+
 				getSPVC := func() *corev1.PersistentVolumeClaim {
 					return sPVC
 				}
@@ -1690,6 +1714,37 @@ var _ = Describe("Rsync as a destination", func() {
 			})
 		})
 
+		Context("Service handled properly when replicationdestination name is very long", func() {
+			BeforeEach(func() {
+				rd.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
+			})
+
+			It("The service name should be shortened appropriately (should handle long CR names)", func() {
+				// create the svc
+				result, err := mover.ensureServiceAndPublishAddress(ctx)
+				Expect(err).To(BeNil())
+
+				if !result {
+					// This means the svc address wasn't populated immediately
+					// Keep reconciling - when service has address populated it should get updated in the rs status)
+					Eventually(func() bool {
+						gotAddr, err := mover.ensureServiceAndPublishAddress(ctx)
+						return err != nil && gotAddr
+					}, maxWait, interval).Should(BeTrue())
+				}
+
+				// Find the service
+				svcs := &corev1.ServiceList{}
+				Expect(k8sClient.List(ctx, svcs, client.InNamespace(rd.Namespace))).To(Succeed())
+				Expect(len(svcs.Items)).To(Equal(1))
+				rdSvc := svcs.Items[0]
+
+				Expect(rdSvc.GetName()).To(ContainSubstring(utils.GetHashedName(rd.GetName())))
+				// Make sure our shortened name is actually short enough
+				Expect(len(rdSvc.GetName()) > 63).To(BeFalse())
+			})
+		})
+
 		//nolint:dupl
 		Context("Service and address are handled properly", func() {
 			var svc *corev1.Service

controllers/mover/syncthing/mover.go

@@ -304,7 +304,7 @@ func (m *Mover) ensureDataPVC(ctx context.Context) (*corev1.PersistentVolumeClai
 // serviceSelector Returns a mapping of standardized Kubernetes labels.
 func (m *Mover) serviceSelector() map[string]string {
 	return map[string]string{
-		"app.kubernetes.io/name":      m.owner.GetName(),
+		"app.kubernetes.io/name":      utils.GetOwnerNameLabelValue("", m.owner),
 		"app.kubernetes.io/component": "syncthing-mover",
 		"app.kubernetes.io/part-of":   "volsync",
 	}
@@ -386,7 +386,7 @@ func (m *Mover) ensureDeployment(ctx context.Context, dataPVC *corev1.Persistent
 			Name:      deploymentName,
 			Namespace: m.owner.GetNamespace(),
 			Labels: map[string]string{
-				"app": m.owner.GetName(),
+				"app": utils.GetOwnerNameLabelValue("", m.owner),
 			},
 		},
 	}
@@ -603,7 +603,7 @@ func (m *Mover) ensureAPIService(ctx context.Context, deployment *appsv1.Deploym
 // ensureDataService Ensures that a service exposing the Syncthing data is present, else it will be created.
 // This service allows Syncthing to share data with the rest of the world.
 func (m *Mover) ensureDataService(ctx context.Context, deployment *appsv1.Deployment) (*corev1.Service, error) {
-	serviceName := mover.VolSyncPrefix + m.owner.GetName() + "-data" //nolint:goconst // goconst thinks -data is used 3x
+	serviceName := m.getDataServiceName()
 	service := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      serviceName,
@@ -832,6 +832,19 @@ func (m *Mover) getConnectedPeers(syncthing *api.Syncthing) []volsyncv1alpha1.Sy
 // getAPIServiceName Returns the name of the API service exposing the Syncthing API.
 func (m *Mover) getAPIServiceName() string {
 	serviceName := mover.VolSyncPrefix + m.owner.GetName() + "-api"
+
+	if len(serviceName) > 63 {
+		serviceName = mover.VolSyncPrefix + "api-" + utils.GetHashedName(m.owner.GetName())
+	}
+	return serviceName
+}
+
+func (m *Mover) getDataServiceName() string {
+	serviceName := mover.VolSyncPrefix + m.owner.GetName() + "-data" //nolint:goconst // goconst thinks -data is used 3x
+
+	if len(serviceName) > 63 {
+		serviceName = mover.VolSyncPrefix + "data-" + utils.GetHashedName(m.owner.GetName())
+	}
 	return serviceName
 }
 

controllers/mover/syncthing/syncthing_test.go

@@ -30,6 +30,7 @@ import (
 	volsyncv1alpha1 "github.com/backube/volsync/api/v1alpha1"
 	cMover "github.com/backube/volsync/controllers/mover"
 	"github.com/backube/volsync/controllers/mover/syncthing/api"
+	"github.com/backube/volsync/controllers/utils"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/syncthing/syncthing/lib/config"
@@ -287,13 +288,51 @@ var _ = Describe("When an RS specifies Syncthing", func() {
 					Expect(err).NotTo(HaveOccurred())
 					Expect(deployment).NotTo(BeNil())
 
+					appLabel, ok := deployment.GetLabels()["app"]
+					Expect(ok).To(BeTrue())
+					Expect(appLabel).To(Equal(rs.GetName()))
+
 					// make sure the service is created
 					svc, err := mover.ensureDataService(ctx, deployment)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(svc).NotTo(BeNil())
 					Expect(svc.Spec.Type).To(Equal(corev1.ServiceTypeClusterIP))
 				})
 
+				When("replicationsource CR name is very long", func() {
+					JustBeforeEach(func() {
+						rs.Name = "super-very-extra-long-name-that-will-definitely-need-to-be-truncated"
+					})
+
+					It("Should be able to create a deployment and api and data service", func() {
+						// get a deployment
+						deployment, err := mover.ensureDeployment(ctx, srcPVC, configPVC, sa, apiSecret)
+						Expect(err).NotTo(HaveOccurred())
+						Expect(deployment).NotTo(BeNil())
+
+						appLabel, ok := deployment.GetLabels()["app"]
+						Expect(ok).To(BeTrue())
+						Expect(appLabel).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
+						Expect(len(appLabel) > 63).To(BeFalse())
+
+						// make sure the api service is created
+						apiSvc, err := mover.ensureAPIService(ctx, deployment)
+						Expect(err).NotTo(HaveOccurred())
+						Expect(apiSvc).NotTo(BeNil())
+						Expect(apiSvc.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
+						// Make sure our shortened name is actually short enough
+						Expect(len(apiSvc.GetName()) > 63).To(BeFalse())
+
+						// make sure the data service is created
+						dataSvc, err := mover.ensureDataService(ctx, deployment)
+						Expect(err).NotTo(HaveOccurred())
+						Expect(dataSvc).NotTo(BeNil())
+						Expect(dataSvc.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
+						// Make sure our shortened name is actually short enough
+						Expect(len(dataSvc.GetName()) > 63).To(BeFalse())
+					})
+				})
+
 				It("Can get DataServiceAddress", func() {
 					// test data
 					const staticIP string = "1.2.3.4"