Skip to content

Commit f2b0978

Browse files
tesshuflowertesshuflower
committed
movers - ensure job, svc name, labels <= 63 chars
- use crc32 hash of CR name for names that are too long - new e2e tests for long names - e2e tests in customscorecard-tests - rearrange for concurrency Signed-off-by: Tesshu Flower <[email protected]>
1 parent eda52c6 commit f2b0978

24 files changed

+1393
-204
lines changed

‎CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1919
### Fixed
2020

2121
- All movers should return error if not able to EnsurePVCFromSrc
22+
- Fix for mover job/service name length too long (>63 chars) if the
23+
replicationsource or replicationdestination CR name is too long
2224

2325
### Security
2426

‎controllers/mover/rclone/mover.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ func (m *Mover) ensureJob(ctx context.Context, dataPVC *corev1.PersistentVolumeC
226226

227227
job := &batchv1.Job{
228228
ObjectMeta: metav1.ObjectMeta{
229-
Name: mover.VolSyncPrefix + "rclone-" + dir + "-" + m.owner.GetName(),
229+
Name: utils.GetJobName(mover.VolSyncPrefix+"rclone-"+dir+"-", m.owner),
230230
Namespace: m.owner.GetNamespace(),
231231
},
232232
}

‎controllers/mover/rclone/rclone_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,6 +1157,30 @@ var _ = Describe("Rclone as a source", func() {
11571157
Expect(job.Spec.Template.Spec.Containers[0].Resources).To(Equal(corev1.ResourceRequirements{}))
11581158
})
11591159

1160+
When("The ReplicationSource CR name is very long", func() {
1161+
BeforeEach(func() {
1162+
rs.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
1163+
})
1164+
1165+
It("The job name should be shortened appropriately (should handle long CR names)", func() {
1166+
j, e := mover.ensureJob(ctx, sPVC, sa, rcloneConfigSecret, nil) // Using sPVC as dataPVC (i.e. direct)
1167+
Expect(e).NotTo(HaveOccurred())
1168+
Expect(j).To(BeNil()) // hasn't completed
1169+
1170+
jobs := &batchv1.JobList{}
1171+
Expect(k8sClient.List(ctx, jobs, client.InNamespace(rs.Namespace))).To(Succeed())
1172+
Expect(len(jobs.Items)).To(Equal(1))
1173+
moverJob := jobs.Items[0]
1174+
1175+
// Reload the replicationsource to see that it got updated
1176+
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(rs), rs)).To(Succeed())
1177+
1178+
Expect(moverJob.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
1179+
// Make sure our shortened name is actually short enough
1180+
Expect(len(moverJob.GetName()) > 63).To(BeFalse())
1181+
})
1182+
})
1183+
11601184
When("moverResources (resource requirements) are provided", func() {
11611185
BeforeEach(func() {
11621186
rs.Spec.Rclone.MoverResources = &corev1.ResourceRequirements{

‎controllers/mover/restic/mover.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ func (m *Mover) ensureJob(ctx context.Context, cachePVC *corev1.PersistentVolume
290290

291291
job := &batchv1.Job{
292292
ObjectMeta: metav1.ObjectMeta{
293-
Name: mover.VolSyncPrefix + dir + "-" + m.owner.GetName(),
293+
Name: utils.GetJobName(mover.VolSyncPrefix+dir+"-", m.owner),
294294
Namespace: m.owner.GetNamespace(),
295295
},
296296
}

‎controllers/mover/restic/restic_test.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1335,6 +1335,31 @@ var _ = Describe("Restic as a source", func() {
13351335
Expect(psc.RunAsUser).To(BeNil())
13361336
Expect(psc.FSGroup).To(BeNil())
13371337
})
1338+
1339+
When("The ReplicationSource CR name is very long", func() {
1340+
BeforeEach(func() {
1341+
rs.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
1342+
})
1343+
1344+
It("The job name should be shortened appropriately (should handle long CR names)", func() {
1345+
j, e := mover.ensureJob(ctx, cache, sPVC, sa, repo, nil)
1346+
Expect(e).NotTo(HaveOccurred())
1347+
Expect(j).To(BeNil()) // hasn't completed
1348+
1349+
jobs := &batchv1.JobList{}
1350+
Expect(k8sClient.List(ctx, jobs, client.InNamespace(rs.Namespace))).To(Succeed())
1351+
Expect(len(jobs.Items)).To(Equal(1))
1352+
moverJob := jobs.Items[0]
1353+
1354+
// Reload the replicationsource to see that it got updated
1355+
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(rs), rs)).To(Succeed())
1356+
1357+
Expect(moverJob.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
1358+
// Make sure our shortened name is actually short enough
1359+
Expect(len(moverJob.GetName()) > 63).To(BeFalse())
1360+
})
1361+
})
1362+
13381363
When("A moverSecurityContext is provided", func() {
13391364
BeforeEach(func() {
13401365
rs.Spec.Restic.MoverSecurityContext = &corev1.PodSecurityContext{

‎controllers/mover/rsync/mover.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ func (m *Mover) ensureServiceAndPublishAddress(ctx context.Context) (bool, error
149149

150150
service := &corev1.Service{
151151
ObjectMeta: metav1.ObjectMeta{
152-
Name: volSyncRsyncPrefix + m.direction() + "-" + m.owner.GetName(),
152+
Name: utils.GetServiceName(volSyncRsyncPrefix+m.direction()+"-", m.owner),
153153
Namespace: m.owner.GetNamespace(),
154154
},
155155
}
@@ -279,7 +279,7 @@ func (m *Mover) direction() string {
279279

280280
func (m *Mover) serviceSelector() map[string]string {
281281
return map[string]string{
282-
"app.kubernetes.io/name": m.direction() + "-" + m.owner.GetName(),
282+
"app.kubernetes.io/name": utils.GetOwnerNameLabelValue(m.direction()+"-", m.owner),
283283
"app.kubernetes.io/component": "rsync-mover",
284284
"app.kubernetes.io/part-of": "volsync",
285285
}
@@ -355,7 +355,7 @@ func (m *Mover) ensureJob(ctx context.Context, dataPVC *corev1.PersistentVolumeC
355355
sa *corev1.ServiceAccount, rsyncSecretName string) (*batchv1.Job, error) {
356356
job := &batchv1.Job{
357357
ObjectMeta: metav1.ObjectMeta{
358-
Name: volSyncRsyncPrefix + m.direction() + "-" + m.owner.GetName(),
358+
Name: utils.GetJobName(volSyncRsyncPrefix+m.direction()+"-", m.owner),
359359
Namespace: m.owner.GetNamespace(),
360360
},
361361
}

‎controllers/mover/rsync/rsync_test.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,30 @@ var _ = Describe("Rsync as a source", func() {
726726
Expect(job.Spec.Template.Spec.ServiceAccountName).To(Equal(sa.Name))
727727
})
728728

729+
When("The ReplicationSource CR name is very long", func() {
730+
BeforeEach(func() {
731+
rs.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
732+
})
733+
734+
It("The job name should be shortened appropriately (should handle long CR names)", func() {
735+
j, e := mover.ensureJob(ctx, sPVC, sa, sshKeysSecret.GetName()) // Using sPVC as dataPVC (i.e. direct)
736+
Expect(e).NotTo(HaveOccurred())
737+
Expect(j).To(BeNil()) // hasn't completed
738+
739+
jobs := &batchv1.JobList{}
740+
Expect(k8sClient.List(ctx, jobs, client.InNamespace(rs.Namespace))).To(Succeed())
741+
Expect(len(jobs.Items)).To(Equal(1))
742+
moverJob := jobs.Items[0]
743+
744+
// Reload the replicationsource to see that it got updated
745+
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(rs), rs)).To(Succeed())
746+
747+
Expect(moverJob.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
748+
// Make sure our shortened name is actually short enough
749+
Expect(len(moverJob.GetName()) > 63).To(BeFalse())
750+
})
751+
})
752+
729753
It("Should not have container resourceRequirements set by default", func() {
730754
j, e := mover.ensureJob(ctx, sPVC, sa, sshKeysSecret.GetName()) // Using sPVC as dataPVC (i.e. direct)
731755
Expect(e).NotTo(HaveOccurred())
@@ -1283,6 +1307,37 @@ var _ = Describe("Rsync as a destination", func() {
12831307
})
12841308
})
12851309

1310+
Context("Service handled properly when replicationdestination name is very long", func() {
1311+
BeforeEach(func() {
1312+
rd.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
1313+
})
1314+
1315+
It("The service name should be shortened appropriately (should handle long CR names)", func() {
1316+
// create the svc
1317+
result, err := mover.ensureServiceAndPublishAddress(ctx)
1318+
Expect(err).To(BeNil())
1319+
1320+
if !result {
1321+
// This means the svc address wasn't populated immediately
1322+
// Keep reconciling - when service has address populated it should get updated in the rs status)
1323+
Eventually(func() bool {
1324+
gotAddr, err := mover.ensureServiceAndPublishAddress(ctx)
1325+
return err != nil && gotAddr
1326+
}, maxWait, interval).Should(BeTrue())
1327+
}
1328+
1329+
// Find the service
1330+
svcs := &corev1.ServiceList{}
1331+
Expect(k8sClient.List(ctx, svcs, client.InNamespace(rd.Namespace))).To(Succeed())
1332+
Expect(len(svcs.Items)).To(Equal(1))
1333+
rdSvc := svcs.Items[0]
1334+
1335+
Expect(rdSvc.GetName()).To(ContainSubstring(utils.GetHashedName(rd.GetName())))
1336+
// Make sure our shortened name is actually short enough
1337+
Expect(len(rdSvc.GetName()) > 63).To(BeFalse())
1338+
})
1339+
})
1340+
12861341
//nolint:dupl
12871342
Context("Service and address are handled properly", func() {
12881343
When("when no remote address is specified", func() {

‎controllers/mover/rsynctls/mover.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ func (m *Mover) ensureServiceAndPublishAddress(ctx context.Context) (bool, error
153153

154154
service := &corev1.Service{
155155
ObjectMeta: metav1.ObjectMeta{
156-
Name: volSyncRsyncTLSPrefix + m.direction() + "-" + m.owner.GetName(),
156+
Name: utils.GetServiceName(volSyncRsyncTLSPrefix+m.direction()+"-", m.owner),
157157
Namespace: m.owner.GetNamespace(),
158158
},
159159
}
@@ -285,7 +285,7 @@ func (m *Mover) direction() string {
285285

286286
func (m *Mover) serviceSelector() map[string]string {
287287
return map[string]string{
288-
"app.kubernetes.io/name": m.direction() + "-" + m.owner.GetName(),
288+
"app.kubernetes.io/name": utils.GetOwnerNameLabelValue(m.direction()+"-", m.owner),
289289
"app.kubernetes.io/component": "rsync-tls-mover",
290290
"app.kubernetes.io/part-of": "volsync",
291291
}
@@ -361,7 +361,7 @@ func (m *Mover) ensureJob(ctx context.Context, dataPVC *corev1.PersistentVolumeC
361361
sa *corev1.ServiceAccount, rsyncSecretName string) (*batchv1.Job, error) {
362362
job := &batchv1.Job{
363363
ObjectMeta: metav1.ObjectMeta{
364-
Name: volSyncRsyncTLSPrefix + m.direction() + "-" + m.owner.GetName(),
364+
Name: utils.GetJobName(volSyncRsyncTLSPrefix+m.direction()+"-", m.owner),
365365
Namespace: m.owner.GetNamespace(),
366366
},
367367
}

‎controllers/mover/rsynctls/rsync_test.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1163,6 +1163,30 @@ var _ = Describe("RsyncTLS as a source", func() {
11631163
Expect(job.Spec.Template.Spec.ServiceAccountName).To(Equal(sa.Name))
11641164
})
11651165

1166+
When("The ReplicationSource CR name is very long", func() {
1167+
BeforeEach(func() {
1168+
rs.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
1169+
})
1170+
1171+
It("The job name should be shortened appropriately (should handle long CR names)", func() {
1172+
j, e := mover.ensureJob(ctx, sPVC, sa, tlsKeySecret.GetName()) // Using sPVC as dataPVC (i.e. direct)
1173+
Expect(e).NotTo(HaveOccurred())
1174+
Expect(j).To(BeNil()) // hasn't completed
1175+
1176+
jobs := &batchv1.JobList{}
1177+
Expect(k8sClient.List(ctx, jobs, client.InNamespace(rs.Namespace))).To(Succeed())
1178+
Expect(len(jobs.Items)).To(Equal(1))
1179+
moverJob := jobs.Items[0]
1180+
1181+
// Reload the replicationsource to see that it got updated
1182+
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(rs), rs)).To(Succeed())
1183+
1184+
Expect(moverJob.GetName()).To(ContainSubstring(utils.GetHashedName(rs.GetName())))
1185+
// Make sure our shortened name is actually short enough
1186+
Expect(len(moverJob.GetName()) > 63).To(BeFalse())
1187+
})
1188+
})
1189+
11661190
getSPVC := func() *corev1.PersistentVolumeClaim {
11671191
return sPVC
11681192
}
@@ -1690,6 +1714,37 @@ var _ = Describe("Rsync as a destination", func() {
16901714
})
16911715
})
16921716

1717+
Context("Service handled properly when replicationdestination name is very long", func() {
1718+
BeforeEach(func() {
1719+
rd.Name = "very-long-name-will-cause-job-name-to-be-evenlongerthan63chars"
1720+
})
1721+
1722+
It("The service name should be shortened appropriately (should handle long CR names)", func() {
1723+
// create the svc
1724+
result, err := mover.ensureServiceAndPublishAddress(ctx)
1725+
Expect(err).To(BeNil())
1726+
1727+
if !result {
1728+
// This means the svc address wasn't populated immediately
1729+
// Keep reconciling - when service has address populated it should get updated in the rs status)
1730+
Eventually(func() bool {
1731+
gotAddr, err := mover.ensureServiceAndPublishAddress(ctx)
1732+
return err != nil && gotAddr
1733+
}, maxWait, interval).Should(BeTrue())
1734+
}
1735+
1736+
// Find the service
1737+
svcs := &corev1.ServiceList{}
1738+
Expect(k8sClient.List(ctx, svcs, client.InNamespace(rd.Namespace))).To(Succeed())
1739+
Expect(len(svcs.Items)).To(Equal(1))
1740+
rdSvc := svcs.Items[0]
1741+
1742+
Expect(rdSvc.GetName()).To(ContainSubstring(utils.GetHashedName(rd.GetName())))
1743+
// Make sure our shortened name is actually short enough
1744+
Expect(len(rdSvc.GetName()) > 63).To(BeFalse())
1745+
})
1746+
})
1747+
16931748
//nolint:dupl
16941749
Context("Service and address are handled properly", func() {
16951750
var svc *corev1.Service

‎controllers/mover/syncthing/mover.go

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ func (m *Mover) ensureDataPVC(ctx context.Context) (*corev1.PersistentVolumeClai
304304
// serviceSelector Returns a mapping of standardized Kubernetes labels.
305305
func (m *Mover) serviceSelector() map[string]string {
306306
return map[string]string{
307-
"app.kubernetes.io/name": m.owner.GetName(),
307+
"app.kubernetes.io/name": utils.GetOwnerNameLabelValue("", m.owner),
308308
"app.kubernetes.io/component": "syncthing-mover",
309309
"app.kubernetes.io/part-of": "volsync",
310310
}
@@ -386,7 +386,7 @@ func (m *Mover) ensureDeployment(ctx context.Context, dataPVC *corev1.Persistent
386386
Name: deploymentName,
387387
Namespace: m.owner.GetNamespace(),
388388
Labels: map[string]string{
389-
"app": m.owner.GetName(),
389+
"app": utils.GetOwnerNameLabelValue("", m.owner),
390390
},
391391
},
392392
}
@@ -603,7 +603,7 @@ func (m *Mover) ensureAPIService(ctx context.Context, deployment *appsv1.Deploym
603603
// ensureDataService Ensures that a service exposing the Syncthing data is present, else it will be created.
604604
// This service allows Syncthing to share data with the rest of the world.
605605
func (m *Mover) ensureDataService(ctx context.Context, deployment *appsv1.Deployment) (*corev1.Service, error) {
606-
serviceName := mover.VolSyncPrefix + m.owner.GetName() + "-data" //nolint:goconst // goconst thinks -data is used 3x
606+
serviceName := m.getDataServiceName()
607607
service := &corev1.Service{
608608
ObjectMeta: metav1.ObjectMeta{
609609
Name: serviceName,
@@ -832,6 +832,19 @@ func (m *Mover) getConnectedPeers(syncthing *api.Syncthing) []volsyncv1alpha1.Sy
832832
// getAPIServiceName Returns the name of the API service exposing the Syncthing API.
833833
func (m *Mover) getAPIServiceName() string {
834834
serviceName := mover.VolSyncPrefix + m.owner.GetName() + "-api"
835+
836+
if len(serviceName) > 63 {
837+
serviceName = mover.VolSyncPrefix + "api-" + utils.GetHashedName(m.owner.GetName())
838+
}
839+
return serviceName
840+
}
841+
842+
func (m *Mover) getDataServiceName() string {
843+
serviceName := mover.VolSyncPrefix + m.owner.GetName() + "-data" //nolint:goconst // goconst thinks -data is used 3x
844+
845+
if len(serviceName) > 63 {
846+
serviceName = mover.VolSyncPrefix + "data-" + utils.GetHashedName(m.owner.GetName())
847+
}
835848
return serviceName
836849
}
837850

0 commit comments

Comments
 (0)