Fix the Job build error when BackupReposiotry name longer than 63. (#9350)

* Fix the Job build error when BackupReposiotry name longer than 63.

Fix the Job build error.
Consider the name length limitation change in job list code.
Use hash to replace the GetValidName function.

Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>

* Use ref_name to replace ref.

Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>

---------

Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>

.github/workflows/push.yml

@@ -12,7 +12,7 @@ jobs:
   get-go-version:
     uses: ./.github/workflows/get-go-version.yaml
     with:
-      ref: ${{ github.ref }}
+      ref: ${{ github.ref_name }}
 
   build:
     name: Build

changelogs/unreleased/9350-blackpiglet

@@ -0,0 +1 @@
+Fix the Job build error when BackupReposiotry name longer than 63.

pkg/controller/backup_repository_controller.go

@@ -275,7 +275,7 @@ func (r *BackupRepoReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			log.WithError(err).Warn("Failed to get keepLatestMaintenanceJobs from ConfigMap, using CLI parameter value")
 		}
 
-		if err := maintenance.DeleteOldJobs(r.Client, req.Name, keepJobs, log); err != nil {
+		if err := maintenance.DeleteOldJobs(r.Client, *backupRepo, keepJobs, log); err != nil {
 			log.WithError(err).Warn("Failed to delete old maintenance jobs")
 		}
 	}

pkg/label/label.go

@@ -18,6 +18,7 @@ package label
 
 import (
 	"crypto/sha256"
+	"crypto/sha3"
 	"encoding/hex"
 	"fmt"
 
@@ -49,6 +50,17 @@ func GetValidName(label string) string {
 	return label[:charsFromLabel] + strSha[:6]
 }
 
+// ReturnNameOrHash returns the original name if it is within the DNS1035LabelMaxLength limit,
+// otherwise it returns the sha3 Sum224 hash(length is 56) of the name.
+func ReturnNameOrHash(name string) string {
+	if len(name) <= validation.DNS1035LabelMaxLength {
+		return name
+	}
+
+	hash := sha3.Sum224([]byte(name))
+	return hex.EncodeToString(hash[:])
+}
+
 // NewSelectorForBackup returns a Selector based on the backup name.
 // This is useful for interacting with Listers that need a Selector.
 func NewSelectorForBackup(name string) labels.Selector {

pkg/label/label_test.go

@@ -48,6 +48,32 @@ func TestGetValidLabelName(t *testing.T) {
 	}
 }
 
+func TestReturnNameOrHash(t *testing.T) {
+	tests := []struct {
+		name          string
+		label         string
+		expectedLabel string
+	}{
+		{
+			name:          "valid label name should not be modified",
+			label:         "short label value",
+			expectedLabel: "short label value",
+		},
+		{
+			name:          "label with more than 63 characters should be modified",
+			label:         "this_is_a_very_long_label_value_that_will_be_rejected_by_Kubernetes",
+			expectedLabel: "1a7399f2d00e268fc12daf431d6667319d1461e2609981070bb7e85c",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			labelVal := ReturnNameOrHash(test.label)
+			assert.Equal(t, test.expectedLabel, labelVal)
+		})
+	}
+}
+
 func TestNewSelectorForBackup(t *testing.T) {
 	selector := NewSelectorForBackup("my-backup")
 	assert.Equal(t, "velero.io/backup-name=my-backup", selector.String())

pkg/repository/maintenance/maintenance.go

@@ -32,11 +32,13 @@ import (
 	corev1api "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	velerolabel "github.com/vmware-tanzu/velero/pkg/label"
 	velerotypes "github.com/vmware-tanzu/velero/pkg/types"
 	"github.com/vmware-tanzu/velero/pkg/util"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
@@ -68,11 +70,22 @@ func GenerateJobName(repo string) string {
 }
 
 // DeleteOldJobs deletes old maintenance jobs and keeps the latest N jobs
-func DeleteOldJobs(cli client.Client, repo string, keep int, logger logrus.FieldLogger) error {
+func DeleteOldJobs(cli client.Client, repo velerov1api.BackupRepository, keep int, logger logrus.FieldLogger) error {
 	logger.Infof("Start to delete old maintenance jobs. %d jobs will be kept.", keep)
 	// Get the maintenance job list by label
 	jobList := &batchv1api.JobList{}
-	err := cli.List(context.TODO(), jobList, client.MatchingLabels(map[string]string{RepositoryNameLabel: repo}))
+	err := cli.List(
+		context.TODO(),
+		jobList,
+		&client.ListOptions{
+			Namespace: repo.Namespace,
+			LabelSelector: labels.SelectorFromSet(
+				map[string]string{
+					RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name),
+				},
+			),
+		},
+	)
 	if err != nil {
 		return err
 	}
@@ -339,10 +352,17 @@ func WaitJobComplete(cli client.Client, ctx context.Context, jobName, ns string,
 // and then return the maintenance jobs' status in the range of limit
 func WaitAllJobsComplete(ctx context.Context, cli client.Client, repo *velerov1api.BackupRepository, limit int, log logrus.FieldLogger) ([]velerov1api.BackupRepositoryMaintenanceStatus, error) {
 	jobList := &batchv1api.JobList{}
-	err := cli.List(context.TODO(), jobList, &client.ListOptions{
-		Namespace: repo.Namespace,
-	},
-		client.MatchingLabels(map[string]string{RepositoryNameLabel: repo.Name}),
+	err := cli.List(
+		context.TODO(),
+		jobList,
+		&client.ListOptions{
+			Namespace: repo.Namespace,
+			LabelSelector: labels.SelectorFromSet(
+				map[string]string{
+					RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name),
+				},
+			),
+		},
 	)
 
 	if err != nil {
@@ -558,7 +578,7 @@ func buildJob(
 	}
 
 	podLabels := map[string]string{
-		RepositoryNameLabel: repo.Name,
+		RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name),
 	}
 
 	for _, k := range util.ThirdPartyLabels {
@@ -588,7 +608,7 @@ func buildJob(
 			Name:      GenerateJobName(repo.Name),
 			Namespace: repo.Namespace,
 			Labels: map[string]string{
-				RepositoryNameLabel: repo.Name,
+				RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name),
 			},
 		},
 		Spec: batchv1api.JobSpec{

pkg/repository/maintenance/maintenance_test.go

@@ -40,6 +40,7 @@ import (
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
+	velerolabel "github.com/vmware-tanzu/velero/pkg/label"
 	"github.com/vmware-tanzu/velero/pkg/repository/provider"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
 	velerotypes "github.com/vmware-tanzu/velero/pkg/types"
@@ -48,7 +49,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/util/logging"
 )
 
-func TestGenerateJobName1(t *testing.T) {
+func TestGenerateJobName(t *testing.T) {
 	testCases := []struct {
 		repo          string
 		expectedStart string
@@ -82,59 +83,62 @@ func TestGenerateJobName1(t *testing.T) {
 }
 func TestDeleteOldJobs(t *testing.T) {
 	// Set up test repo and keep value
-	repo := "test-repo"
-	keep := 2
-
-	// Create some maintenance jobs for testing
-	var objs []client.Object
-	// Create a newer job
-	newerJob := &batchv1api.Job{
+	repo := &velerov1api.BackupRepository{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "job1",
-			Namespace: "default",
-			Labels:    map[string]string{RepositoryNameLabel: repo},
+			Name:      "label with more than 63 characters should be modified",
+			Namespace: velerov1api.DefaultNamespace,
+		},
+	}
+	keep := 1
+
+	jobArray := []client.Object{
+		&batchv1api.Job{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "job-0",
+				Namespace: velerov1api.DefaultNamespace,
+				Labels:    map[string]string{RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name)},
+			},
+			Spec: batchv1api.JobSpec{},
+		},
+		&batchv1api.Job{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "job-1",
+				Namespace: velerov1api.DefaultNamespace,
+				Labels:    map[string]string{RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name)},
+			},
+			Spec: batchv1api.JobSpec{},
+		},
+	}
+
+	newJob := &batchv1api.Job{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "job-new",
+			Namespace: velerov1api.DefaultNamespace,
+			Labels:    map[string]string{RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name)},
 		},
 		Spec: batchv1api.JobSpec{},
 	}
-	objs = append(objs, newerJob)
-	// Create older jobs
-	for i := 2; i <= 3; i++ {
-		olderJob := &batchv1api.Job{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      fmt.Sprintf("job%d", i),
-				Namespace: "default",
-				Labels:    map[string]string{RepositoryNameLabel: repo},
-				CreationTimestamp: metav1.Time{
-					Time: metav1.Now().Add(time.Duration(-24*i) * time.Hour),
-				},
-			},
-			Spec: batchv1api.JobSpec{},
-		}
-		objs = append(objs, olderJob)
-	}
-	// Create a fake Kubernetes client
+
+	// Create a fake Kubernetes client with 2 jobs.
 	scheme := runtime.NewScheme()
 	_ = batchv1api.AddToScheme(scheme)
-	cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build()
+	cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(jobArray...).Build()
+
+	// Create a new job
+	require.NoError(t, cli.Create(t.Context(), newJob))
 
 	// Call the function
-	err := DeleteOldJobs(cli, repo, keep, velerotest.NewLogger())
-	require.NoError(t, err)
+	require.NoError(t, DeleteOldJobs(cli, *repo, keep, velerotest.NewLogger()))
 
 	// Get the remaining jobs
 	jobList := &batchv1api.JobList{}
-	err = cli.List(t.Context(), jobList, client.MatchingLabels(map[string]string{RepositoryNameLabel: repo}))
-	require.NoError(t, err)
+	require.NoError(t, cli.List(t.Context(), jobList, client.MatchingLabels(map[string]string{RepositoryNameLabel: repo.Name})))
 
 	// We expect the number of jobs to be equal to 'keep'
 	assert.Len(t, jobList.Items, keep)
 
-	// We expect that the oldest jobs were deleted
-	// Job3 should not be present in the remaining list
-	assert.NotContains(t, jobList.Items, objs[2])
-
-	// Job2 should also not be present in the remaining list
-	assert.NotContains(t, jobList.Items, objs[1])
+	// Only the new created job should be left.
+	assert.Equal(t, jobList.Items[0].Name, newJob.Name)
 }
 
 func TestWaitForJobComplete(t *testing.T) {
@@ -571,7 +575,7 @@ func TestWaitAllJobsComplete(t *testing.T) {
 	repo := &velerov1api.BackupRepository{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: veleroNamespace,
-			Name:      "fake-repo",
+			Name:      "label with more than 63 characters should be modified",
 		},
 		Spec: velerov1api.BackupRepositorySpec{
 			BackupStorageLocation: "default",
@@ -595,7 +599,7 @@ func TestWaitAllJobsComplete(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:              "job1",
 			Namespace:         veleroNamespace,
-			Labels:            map[string]string{RepositoryNameLabel: "fake-repo"},
+			Labels:            map[string]string{RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name)},
 			CreationTimestamp: metav1.Time{Time: now},
 		},
 	}
@@ -604,7 +608,7 @@ func TestWaitAllJobsComplete(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:              "job1",
 			Namespace:         veleroNamespace,
-			Labels:            map[string]string{RepositoryNameLabel: "fake-repo"},
+			Labels:            map[string]string{RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name)},
 			CreationTimestamp: metav1.Time{Time: now},
 		},
 		Status: batchv1api.JobStatus{
@@ -624,7 +628,7 @@ func TestWaitAllJobsComplete(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:              "job2",
 			Namespace:         veleroNamespace,
-			Labels:            map[string]string{RepositoryNameLabel: "fake-repo"},
+			Labels:            map[string]string{RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name)},
 			CreationTimestamp: metav1.Time{Time: now.Add(time.Hour)},
 		},
 		Status: batchv1api.JobStatus{
@@ -645,7 +649,7 @@ func TestWaitAllJobsComplete(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:              "job3",
 			Namespace:         veleroNamespace,
-			Labels:            map[string]string{RepositoryNameLabel: "fake-repo"},
+			Labels:            map[string]string{RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name)},
 			CreationTimestamp: metav1.Time{Time: now.Add(time.Hour * 2)},
 		},
 		Status: batchv1api.JobStatus{
@@ -665,7 +669,7 @@ func TestWaitAllJobsComplete(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:              "job4",
 			Namespace:         veleroNamespace,
-			Labels:            map[string]string{RepositoryNameLabel: "fake-repo"},
+			Labels:            map[string]string{RepositoryNameLabel: velerolabel.ReturnNameOrHash(repo.Name)},
 			CreationTimestamp: metav1.Time{Time: now.Add(time.Hour * 3)},
 		},
 		Status: batchv1api.JobStatus{
@@ -698,7 +702,7 @@ func TestWaitAllJobsComplete(t *testing.T) {
 		{
 			name:          "list job error",
 			runtimeScheme: schemeFail,
-			expectedError: "error listing maintenance job for repo fake-repo: no kind is registered for the type v1.JobList in scheme",
+			expectedError: "error listing maintenance job for repo label with more than 63 characters should be modified: no kind is registered for the type v1.JobList in scheme",
 		},
 		{
 			name:          "job not exist",
@@ -943,6 +947,7 @@ func TestBuildJob(t *testing.T) {
 		expectedSecurityContext    *corev1api.SecurityContext
 		expectedPodSecurityContext *corev1api.PodSecurityContext
 		expectedImagePullSecrets   []corev1api.LocalObjectReference
+		backupRepository           *velerov1api.BackupRepository
 	}{
 		{
 			name: "Valid maintenance job without third party labels",
@@ -1060,6 +1065,64 @@ func TestBuildJob(t *testing.T) {
 			expectedJobName: "",
 			expectedError:   true,
 		},
+		{
+			name: "Valid maintenance job with third party labels and BackupRepository name longer than 63",
+			m: &velerotypes.JobConfigs{
+				PodResources: &kube.PodResources{
+					CPURequest:    "100m",
+					MemoryRequest: "128Mi",
+					CPULimit:      "200m",
+					MemoryLimit:   "256Mi",
+				},
+			},
+			deploy:        deploy2,
+			logLevel:      logrus.InfoLevel,
+			logFormat:     logging.NewFormatFlag(),
+			expectedError: false,
+			expectedEnv: []corev1api.EnvVar{
+				{
+					Name:  "test-name",
+					Value: "test-value",
+				},
+			},
+			expectedEnvFrom: []corev1api.EnvFromSource{
+				{
+					ConfigMapRef: &corev1api.ConfigMapEnvSource{
+						LocalObjectReference: corev1api.LocalObjectReference{
+							Name: "test-configmap",
+						},
+					},
+				},
+				{
+					SecretRef: &corev1api.SecretEnvSource{
+						LocalObjectReference: corev1api.LocalObjectReference{
+							Name: "test-secret",
+						},
+					},
+				},
+			},
+			expectedPodLabel: map[string]string{
+				RepositoryNameLabel:           velerolabel.ReturnNameOrHash("label with more than 63 characters should be modified"),
+				"azure.workload.identity/use": "fake-label-value",
+			},
+			expectedSecurityContext:    nil,
+			expectedPodSecurityContext: nil,
+			expectedImagePullSecrets: []corev1api.LocalObjectReference{
+				{
+					Name: "imagePullSecret1",
+				},
+			},
+			backupRepository: &velerov1api.BackupRepository{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "velero",
+					Name:      "label with more than 63 characters should be modified",
+				},
+				Spec: velerov1api.BackupRepositorySpec{
+					VolumeNamespace: "test-123",
+					RepositoryType:  "kopia",
+				},
+			},
+		},
 	}
 
 	param := provider.RepoParam{
@@ -1083,6 +1146,10 @@ func TestBuildJob(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			if tc.backupRepository != nil {
+				param.BackupRepo = tc.backupRepository
+			}
+
 			// Create a fake clientset with resources
 			objs := []runtime.Object{param.BackupLocation, param.BackupRepo}