Make ResticIdentifier optional for kopia repositories (#8987)

The ResticIdentifier field in BackupRepository is only relevant for restic
repositories. For kopia repositories, this field is unused and should be
omitted. This change:

- Adds omitempty tag to ResticIdentifier field in BackupRepository CRD
- Updates controller to only populate ResticIdentifier for restic repos
- Adds tests to verify behavior for both restic and kopia repository types

This ensures backward compatibility while properly handling kopia repositories
that don't require a restic-compatible identifier.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

pkg/controller/backup_repository_controller.go

@@ -308,17 +308,21 @@ func (r *BackupRepoReconciler) getIdentifierByBSL(bsl *velerov1api.BackupStorage
 func (r *BackupRepoReconciler) initializeRepo(ctx context.Context, req *velerov1api.BackupRepository, bsl *velerov1api.BackupStorageLocation, log logrus.FieldLogger) error {
 	log.WithField("repoConfig", r.backupRepoConfig).Info("Initializing backup repository")
 
-	// confirm the repo's BackupStorageLocation is valid
-	repoIdentifier, err := r.getIdentifierByBSL(bsl, req)
-	if err != nil {
-		return r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) {
-			rr.Status.Message = err.Error()
-			rr.Status.Phase = velerov1api.BackupRepositoryPhaseNotReady
+	var repoIdentifier string
+	// Only get restic identifier for restic repositories
+	if req.Spec.RepositoryType == "" || req.Spec.RepositoryType == velerov1api.BackupRepositoryTypeRestic {
+		var err error
+		repoIdentifier, err = r.getIdentifierByBSL(bsl, req)
+		if err != nil {
+			return r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) {
+				rr.Status.Message = err.Error()
+				rr.Status.Phase = velerov1api.BackupRepositoryPhaseNotReady
 
-			if rr.Spec.MaintenanceFrequency.Duration <= 0 {
-				rr.Spec.MaintenanceFrequency = metav1.Duration{Duration: r.getRepositoryMaintenanceFrequency(req)}
-			}
-		})
+				if rr.Spec.MaintenanceFrequency.Duration <= 0 {
+					rr.Spec.MaintenanceFrequency = metav1.Duration{Duration: r.getRepositoryMaintenanceFrequency(req)}
+				}
+			})
+		}
 	}
 
 	config, err := getBackupRepositoryConfig(ctx, r, r.backupRepoConfig, r.namespace, req.Name, req.Spec.RepositoryType, log)
@@ -330,7 +334,10 @@ func (r *BackupRepoReconciler) initializeRepo(ctx context.Context, req *velerov1
 
 	// defaulting - if the patch fails, return an error so the item is returned to the queue
 	if err := r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) {
-		rr.Spec.ResticIdentifier = repoIdentifier
+		// Only set ResticIdentifier for restic repositories
+		if rr.Spec.RepositoryType == "" || rr.Spec.RepositoryType == velerov1api.BackupRepositoryTypeRestic {
+			rr.Spec.ResticIdentifier = repoIdentifier
+		}
 
 		if rr.Spec.MaintenanceFrequency.Duration <= 0 {
 			rr.Spec.MaintenanceFrequency = metav1.Duration{Duration: r.getRepositoryMaintenanceFrequency(req)}
@@ -528,16 +535,19 @@ func dueForMaintenance(req *velerov1api.BackupRepository, now time.Time) bool {
 func (r *BackupRepoReconciler) checkNotReadyRepo(ctx context.Context, req *velerov1api.BackupRepository, bsl *velerov1api.BackupStorageLocation, log logrus.FieldLogger) (bool, error) {
 	log.Info("Checking backup repository for readiness")
 
-	repoIdentifier, err := r.getIdentifierByBSL(bsl, req)
-	if err != nil {
-		return false, r.patchBackupRepository(ctx, req, repoNotReady(err.Error()))
-	}
+	// Only check and update restic identifier for restic repositories
+	if req.Spec.RepositoryType == "" || req.Spec.RepositoryType == velerov1api.BackupRepositoryTypeRestic {
+		repoIdentifier, err := r.getIdentifierByBSL(bsl, req)
+		if err != nil {
+			return false, r.patchBackupRepository(ctx, req, repoNotReady(err.Error()))
+		}
 
-	if repoIdentifier != req.Spec.ResticIdentifier {
-		if err := r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) {
-			rr.Spec.ResticIdentifier = repoIdentifier
-		}); err != nil {
-			return false, err
+		if repoIdentifier != req.Spec.ResticIdentifier {
+			if err := r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) {
+				rr.Spec.ResticIdentifier = repoIdentifier
+			}); err != nil {
+				return false, err
+			}
 		}
 	}
 
@@ -546,7 +556,7 @@ func (r *BackupRepoReconciler) checkNotReadyRepo(ctx context.Context, req *veler
 	if err := ensureRepo(req, r.repositoryManager); err != nil {
 		return false, r.patchBackupRepository(ctx, req, repoNotReady(err.Error()))
 	}
-	err = r.patchBackupRepository(ctx, req, repoReady())
+	err := r.patchBackupRepository(ctx, req, repoReady())
 	if err != nil {
 		return false, err
 	}

pkg/controller/backup_repository_controller_test.go

@@ -97,27 +97,83 @@ func TestPatchBackupRepository(t *testing.T) {
 }
 
 func TestCheckNotReadyRepo(t *testing.T) {
-	rr := mockBackupRepositoryCR()
-	rr.Spec.BackupStorageLocation = "default"
-	rr.Spec.ResticIdentifier = "fake-identifier"
-	rr.Spec.VolumeNamespace = "volume-ns-1"
-	reconciler := mockBackupRepoReconciler(t, "PrepareRepo", rr, nil)
-	err := reconciler.Client.Create(t.Context(), rr)
-	require.NoError(t, err)
-	location := velerov1api.BackupStorageLocation{
-		Spec: velerov1api.BackupStorageLocationSpec{
-			Config: map[string]string{"resticRepoPrefix": "s3:test.amazonaws.com/bucket/restic"},
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: velerov1api.DefaultNamespace,
-			Name:      rr.Spec.BackupStorageLocation,
-		},
-	}
+	// Test for restic repository
+	t.Run("restic repository", func(t *testing.T) {
+		rr := mockBackupRepositoryCR()
+		rr.Spec.BackupStorageLocation = "default"
+		rr.Spec.ResticIdentifier = "fake-identifier"
+		rr.Spec.VolumeNamespace = "volume-ns-1"
+		rr.Spec.RepositoryType = velerov1api.BackupRepositoryTypeRestic
+		reconciler := mockBackupRepoReconciler(t, "PrepareRepo", rr, nil)
+		err := reconciler.Client.Create(t.Context(), rr)
+		require.NoError(t, err)
+		location := velerov1api.BackupStorageLocation{
+			Spec: velerov1api.BackupStorageLocationSpec{
+				Config: map[string]string{"resticRepoPrefix": "s3:test.amazonaws.com/bucket/restic"},
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: velerov1api.DefaultNamespace,
+				Name:      rr.Spec.BackupStorageLocation,
+			},
+		}
 
-	_, err = reconciler.checkNotReadyRepo(t.Context(), rr, &location, reconciler.logger)
-	require.NoError(t, err)
-	assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase)
-	assert.Equal(t, "s3:test.amazonaws.com/bucket/restic/volume-ns-1", rr.Spec.ResticIdentifier)
+		_, err = reconciler.checkNotReadyRepo(t.Context(), rr, &location, reconciler.logger)
+		require.NoError(t, err)
+		assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase)
+		assert.Equal(t, "s3:test.amazonaws.com/bucket/restic/volume-ns-1", rr.Spec.ResticIdentifier)
+	})
+
+	// Test for kopia repository
+	t.Run("kopia repository", func(t *testing.T) {
+		rr := mockBackupRepositoryCR()
+		rr.Spec.BackupStorageLocation = "default"
+		rr.Spec.VolumeNamespace = "volume-ns-1"
+		rr.Spec.RepositoryType = velerov1api.BackupRepositoryTypeKopia
+		reconciler := mockBackupRepoReconciler(t, "PrepareRepo", rr, nil)
+		err := reconciler.Client.Create(t.Context(), rr)
+		require.NoError(t, err)
+		location := velerov1api.BackupStorageLocation{
+			Spec: velerov1api.BackupStorageLocationSpec{
+				Config: map[string]string{"resticRepoPrefix": "s3:test.amazonaws.com/bucket/restic"},
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: velerov1api.DefaultNamespace,
+				Name:      rr.Spec.BackupStorageLocation,
+			},
+		}
+
+		_, err = reconciler.checkNotReadyRepo(t.Context(), rr, &location, reconciler.logger)
+		require.NoError(t, err)
+		assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase)
+		// ResticIdentifier should remain empty for kopia
+		assert.Empty(t, rr.Spec.ResticIdentifier)
+	})
+
+	// Test for empty repository type (defaults to restic)
+	t.Run("empty repository type", func(t *testing.T) {
+		rr := mockBackupRepositoryCR()
+		rr.Spec.BackupStorageLocation = "default"
+		rr.Spec.ResticIdentifier = "fake-identifier"
+		rr.Spec.VolumeNamespace = "volume-ns-1"
+		// Deliberately leave RepositoryType empty
+		reconciler := mockBackupRepoReconciler(t, "PrepareRepo", rr, nil)
+		err := reconciler.Client.Create(t.Context(), rr)
+		require.NoError(t, err)
+		location := velerov1api.BackupStorageLocation{
+			Spec: velerov1api.BackupStorageLocationSpec{
+				Config: map[string]string{"resticRepoPrefix": "s3:test.amazonaws.com/bucket/restic"},
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: velerov1api.DefaultNamespace,
+				Name:      rr.Spec.BackupStorageLocation,
+			},
+		}
+
+		_, err = reconciler.checkNotReadyRepo(t.Context(), rr, &location, reconciler.logger)
+		require.NoError(t, err)
+		assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase)
+		assert.Equal(t, "s3:test.amazonaws.com/bucket/restic/volume-ns-1", rr.Spec.ResticIdentifier)
+	})
 }
 
 func startMaintenanceJobFail(client.Client, context.Context, *velerov1api.BackupRepository, string, kube.PodResources, logrus.Level, *logging.FormatFlag, logrus.FieldLogger) (string, error) {
@@ -1502,3 +1558,167 @@ func TestDeleteOldMaintenanceJob(t *testing.T) {
 		})
 	}
 }
+
+func TestInitializeRepoWithRepositoryTypes(t *testing.T) {
+	scheme := runtime.NewScheme()
+	corev1api.AddToScheme(scheme)
+	velerov1api.AddToScheme(scheme)
+
+	// Test for restic repository
+	t.Run("restic repository", func(t *testing.T) {
+		rr := mockBackupRepositoryCR()
+		rr.Spec.BackupStorageLocation = "default"
+		rr.Spec.VolumeNamespace = "volume-ns-1"
+		rr.Spec.RepositoryType = velerov1api.BackupRepositoryTypeRestic
+
+		location := &velerov1api.BackupStorageLocation{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: velerov1api.DefaultNamespace,
+				Name:      "default",
+			},
+			Spec: velerov1api.BackupStorageLocationSpec{
+				Provider: "aws",
+				StorageType: velerov1api.StorageType{
+					ObjectStorage: &velerov1api.ObjectStorageLocation{
+						Bucket: "test-bucket",
+						Prefix: "test-prefix",
+					},
+				},
+				Config: map[string]string{
+					"region": "us-east-1",
+				},
+			},
+		}
+
+		fakeClient := clientFake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(rr, location).Build()
+		mgr := &repomokes.Manager{}
+		mgr.On("PrepareRepo", rr).Return(nil)
+
+		reconciler := NewBackupRepoReconciler(
+			velerov1api.DefaultNamespace,
+			velerotest.NewLogger(),
+			fakeClient,
+			mgr,
+			testMaintenanceFrequency,
+			"",
+			3,
+			"",
+			kube.PodResources{},
+			logrus.InfoLevel,
+			nil,
+		)
+
+		err := reconciler.initializeRepo(t.Context(), rr, location, reconciler.logger)
+		require.NoError(t, err)
+
+		// Verify ResticIdentifier is set for restic
+		assert.NotEmpty(t, rr.Spec.ResticIdentifier)
+		assert.Contains(t, rr.Spec.ResticIdentifier, "volume-ns-1")
+		assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase)
+	})
+
+	// Test for kopia repository
+	t.Run("kopia repository", func(t *testing.T) {
+		rr := mockBackupRepositoryCR()
+		rr.Spec.BackupStorageLocation = "default"
+		rr.Spec.VolumeNamespace = "volume-ns-1"
+		rr.Spec.RepositoryType = velerov1api.BackupRepositoryTypeKopia
+
+		location := &velerov1api.BackupStorageLocation{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: velerov1api.DefaultNamespace,
+				Name:      "default",
+			},
+			Spec: velerov1api.BackupStorageLocationSpec{
+				Provider: "aws",
+				StorageType: velerov1api.StorageType{
+					ObjectStorage: &velerov1api.ObjectStorageLocation{
+						Bucket: "test-bucket",
+						Prefix: "test-prefix",
+					},
+				},
+				Config: map[string]string{
+					"region": "us-east-1",
+				},
+			},
+		}
+
+		fakeClient := clientFake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(rr, location).Build()
+		mgr := &repomokes.Manager{}
+		mgr.On("PrepareRepo", rr).Return(nil)
+
+		reconciler := NewBackupRepoReconciler(
+			velerov1api.DefaultNamespace,
+			velerotest.NewLogger(),
+			fakeClient,
+			mgr,
+			testMaintenanceFrequency,
+			"",
+			3,
+			"",
+			kube.PodResources{},
+			logrus.InfoLevel,
+			nil,
+		)
+
+		err := reconciler.initializeRepo(t.Context(), rr, location, reconciler.logger)
+		require.NoError(t, err)
+
+		// Verify ResticIdentifier is NOT set for kopia
+		assert.Empty(t, rr.Spec.ResticIdentifier)
+		assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase)
+	})
+
+	// Test for empty repository type (defaults to restic)
+	t.Run("empty repository type", func(t *testing.T) {
+		rr := mockBackupRepositoryCR()
+		rr.Spec.BackupStorageLocation = "default"
+		rr.Spec.VolumeNamespace = "volume-ns-1"
+		// Leave RepositoryType empty
+
+		location := &velerov1api.BackupStorageLocation{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: velerov1api.DefaultNamespace,
+				Name:      "default",
+			},
+			Spec: velerov1api.BackupStorageLocationSpec{
+				Provider: "aws",
+				StorageType: velerov1api.StorageType{
+					ObjectStorage: &velerov1api.ObjectStorageLocation{
+						Bucket: "test-bucket",
+						Prefix: "test-prefix",
+					},
+				},
+				Config: map[string]string{
+					"region": "us-east-1",
+				},
+			},
+		}
+
+		fakeClient := clientFake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(rr, location).Build()
+		mgr := &repomokes.Manager{}
+		mgr.On("PrepareRepo", rr).Return(nil)
+
+		reconciler := NewBackupRepoReconciler(
+			velerov1api.DefaultNamespace,
+			velerotest.NewLogger(),
+			fakeClient,
+			mgr,
+			testMaintenanceFrequency,
+			"",
+			3,
+			"",
+			kube.PodResources{},
+			logrus.InfoLevel,
+			nil,
+		)
+
+		err := reconciler.initializeRepo(t.Context(), rr, location, reconciler.logger)
+		require.NoError(t, err)
+
+		// Verify ResticIdentifier is set when type is empty (defaults to restic)
+		assert.NotEmpty(t, rr.Spec.ResticIdentifier)
+		assert.Contains(t, rr.Spec.ResticIdentifier, "volume-ns-1")
+		assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase)
+	})
+}