WIP: Fix BoostRepoConnect fallback to use PrepareRepo instead of ConnectToRepo (#8279)

pkg/controller/backup_repository_controller_test.go

@@ -252,6 +252,294 @@ func TestGetRepositoryMaintenanceFrequency(t *testing.T) {
 	}
 }
 
+func TestInvalidateBackupReposForBSL(t *testing.T) {
+	tests := []struct {
+		name          string
+		bsl           *velerov1api.BackupStorageLocation
+		repos         []*velerov1api.BackupRepository
+		expectNotReady int
+	}{
+		{
+			name: "invalidates repos matching BSL label",
+			bsl: &velerov1api.BackupStorageLocation{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: velerov1api.DefaultNamespace,
+					Name:      "bsl-1",
+				},
+			},
+			repos: []*velerov1api.BackupRepository{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: velerov1api.DefaultNamespace,
+						Name:      "repo-1",
+						Labels: map[string]string{
+							velerov1api.StorageLocationLabel: "bsl-1",
+						},
+					},
+					Spec: velerov1api.BackupRepositorySpec{
+						MaintenanceFrequency: metav1.Duration{Duration: testMaintenanceFrequency},
+						BackupStorageLocation: "bsl-1",
+					},
+					Status: velerov1api.BackupRepositoryStatus{
+						Phase: velerov1api.BackupRepositoryPhaseReady,
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: velerov1api.DefaultNamespace,
+						Name:      "repo-2",
+						Labels: map[string]string{
+							velerov1api.StorageLocationLabel: "bsl-1",
+						},
+					},
+					Spec: velerov1api.BackupRepositorySpec{
+						MaintenanceFrequency: metav1.Duration{Duration: testMaintenanceFrequency},
+						BackupStorageLocation: "bsl-1",
+					},
+					Status: velerov1api.BackupRepositoryStatus{
+						Phase: velerov1api.BackupRepositoryPhaseReady,
+					},
+				},
+			},
+			expectNotReady: 2,
+		},
+		{
+			name: "does not invalidate repos for different BSL",
+			bsl: &velerov1api.BackupStorageLocation{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: velerov1api.DefaultNamespace,
+					Name:      "bsl-1",
+				},
+			},
+			repos: []*velerov1api.BackupRepository{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: velerov1api.DefaultNamespace,
+						Name:      "repo-other",
+						Labels: map[string]string{
+							velerov1api.StorageLocationLabel: "bsl-other",
+						},
+					},
+					Spec: velerov1api.BackupRepositorySpec{
+						MaintenanceFrequency: metav1.Duration{Duration: testMaintenanceFrequency},
+						BackupStorageLocation: "bsl-other",
+					},
+					Status: velerov1api.BackupRepositoryStatus{
+						Phase: velerov1api.BackupRepositoryPhaseReady,
+					},
+				},
+			},
+			expectNotReady: 0,
+		},
+		{
+			name: "no repos to invalidate",
+			bsl: &velerov1api.BackupStorageLocation{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: velerov1api.DefaultNamespace,
+					Name:      "bsl-1",
+				},
+			},
+			repos:          nil,
+			expectNotReady: 0,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			reconciler := NewBackupRepoReconciler(
+				velerov1api.DefaultNamespace,
+				velerotest.NewLogger(),
+				velerotest.NewFakeControllerRuntimeClient(t),
+				time.Duration(0), nil)
+
+			for _, repo := range test.repos {
+				assert.NoError(t, reconciler.Client.Create(context.TODO(), repo))
+			}
+
+			requests := reconciler.invalidateBackupReposForBSL(context.TODO(), test.bsl)
+			assert.NotNil(t, requests)
+
+			notReadyCount := 0
+			for _, repo := range test.repos {
+				updated := &velerov1api.BackupRepository{}
+				err := reconciler.Client.Get(context.TODO(), types.NamespacedName{
+					Namespace: repo.Namespace,
+					Name:      repo.Name,
+				}, updated)
+				assert.NoError(t, err)
+				if updated.Status.Phase == velerov1api.BackupRepositoryPhaseNotReady {
+					assert.Equal(t, "re-establish on BSL change or create", updated.Status.Message)
+					notReadyCount++
+				}
+			}
+			assert.Equal(t, test.expectNotReady, notReadyCount)
+		})
+	}
+}
+
+// TestCheckNotReadyRepoUpdatesIdentifierOnPrefixChange verifies that when BSL prefix
+// changes from "velero" to "velero-new" and BackupRepository is NotReady,
+// checkNotReadyRepo updates the ResticIdentifier to reflect the new prefix.
+func TestCheckNotReadyRepoUpdatesIdentifierOnPrefixChange(t *testing.T) {
+	rr := mockBackupRepositoryCR()
+	rr.Spec.BackupStorageLocation = "default"
+	rr.Spec.ResticIdentifier = "s3:test.amazonaws.com/bucket/velero/restic/volume-ns-1"
+	rr.Spec.VolumeNamespace = "volume-ns-1"
+	rr.Status.Phase = velerov1api.BackupRepositoryPhaseNotReady
+	reconciler := mockBackupRepoReconciler(t, rr, "PrepareRepo", rr, nil)
+	err := reconciler.Client.Create(context.TODO(), rr)
+	assert.NoError(t, err)
+
+	// BSL now has a different prefix than what the repo was initialized with
+	bsl := &velerov1api.BackupStorageLocation{
+		Spec: velerov1api.BackupStorageLocationSpec{
+			Config: map[string]string{"resticRepoPrefix": "s3:test.amazonaws.com/bucket/velero-new/restic"},
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: velerov1api.DefaultNamespace,
+			Name:      "default",
+		},
+	}
+	err = reconciler.Client.Create(context.TODO(), bsl)
+	assert.NoError(t, err)
+
+	ready, err := reconciler.checkNotReadyRepo(context.TODO(), rr, reconciler.logger)
+	assert.NoError(t, err)
+	assert.True(t, ready)
+	assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase)
+	assert.Equal(t, "s3:test.amazonaws.com/bucket/velero-new/restic/volume-ns-1", rr.Spec.ResticIdentifier)
+}
+
+// TestCheckNotReadyRepoKopiaUpdatesIdentifierOnPrefixChange verifies that kopia
+// repositories also get their ResticIdentifier updated when BSL prefix changes.
+// On oadp-1.4, checkNotReadyRepo does not have a restic-only guard, so this should work.
+func TestCheckNotReadyRepoKopiaUpdatesIdentifierOnPrefixChange(t *testing.T) {
+	rr := mockBackupRepositoryCR()
+	rr.Spec.BackupStorageLocation = "default"
+	rr.Spec.RepositoryType = "kopia"
+	rr.Spec.ResticIdentifier = "s3:test.amazonaws.com/bucket/velero/restic/volume-ns-1"
+	rr.Spec.VolumeNamespace = "volume-ns-1"
+	rr.Status.Phase = velerov1api.BackupRepositoryPhaseNotReady
+	reconciler := mockBackupRepoReconciler(t, rr, "PrepareRepo", rr, nil)
+	err := reconciler.Client.Create(context.TODO(), rr)
+	assert.NoError(t, err)
+
+	bsl := &velerov1api.BackupStorageLocation{
+		Spec: velerov1api.BackupStorageLocationSpec{
+			Config: map[string]string{"resticRepoPrefix": "s3:test.amazonaws.com/bucket/velero-new/restic"},
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: velerov1api.DefaultNamespace,
+			Name:      "default",
+		},
+	}
+	err = reconciler.Client.Create(context.TODO(), bsl)
+	assert.NoError(t, err)
+
+	ready, err := reconciler.checkNotReadyRepo(context.TODO(), rr, reconciler.logger)
+	assert.NoError(t, err)
+	assert.True(t, ready)
+	assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase)
+	assert.Equal(t, "s3:test.amazonaws.com/bucket/velero-new/restic/volume-ns-1", rr.Spec.ResticIdentifier)
+}
+
+// TestBSLDeleteRecreateWithNewPrefix simulates the full BSL delete/recreate scenario (#8279):
+// 1. BSL exists with prefix "velero", BackupRepository is Ready
+// 2. BSL is deleted -> checkNotReadyRepo patches repo to NotReady (BSL not found)
+// 3. BSL is recreated with prefix "velero-new" -> invalidateBackupReposForBSL fires
+// 4. Reconcile runs again -> checkNotReadyRepo updates ResticIdentifier
+func TestBSLDeleteRecreateWithNewPrefix(t *testing.T) {
+	bslName := "ts-dpa-1"
+	rr := &velerov1api.BackupRepository{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: velerov1api.DefaultNamespace,
+			Name:      "test-prefix-repo",
+			Labels: map[string]string{
+				velerov1api.StorageLocationLabel: bslName,
+			},
+		},
+		Spec: velerov1api.BackupRepositorySpec{
+			MaintenanceFrequency:  metav1.Duration{Duration: testMaintenanceFrequency},
+			BackupStorageLocation: bslName,
+			ResticIdentifier:      "s3:test.amazonaws.com/bucket/velero/restic/volume-ns-1",
+			VolumeNamespace:       "volume-ns-1",
+		},
+		Status: velerov1api.BackupRepositoryStatus{
+			Phase: velerov1api.BackupRepositoryPhaseReady,
+		},
+	}
+	mgr := &repomokes.Manager{}
+	mgr.On("PrepareRepo", mock.Anything).Return(nil)
+	reconciler := NewBackupRepoReconciler(
+		velerov1api.DefaultNamespace,
+		velerotest.NewLogger(),
+		velerotest.NewFakeControllerRuntimeClient(t),
+		testMaintenanceFrequency,
+		mgr,
+	)
+	err := reconciler.Client.Create(context.TODO(), rr)
+	assert.NoError(t, err)
+
+	// Step 1: BSL exists with old prefix
+	oldBSL := &velerov1api.BackupStorageLocation{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: velerov1api.DefaultNamespace,
+			Name:      bslName,
+		},
+		Spec: velerov1api.BackupStorageLocationSpec{
+			Config: map[string]string{"resticRepoPrefix": "s3:test.amazonaws.com/bucket/velero/restic"},
+		},
+	}
+	err = reconciler.Client.Create(context.TODO(), oldBSL)
+	assert.NoError(t, err)
+
+	// Step 2: BSL is deleted (simulates DPA deletion)
+	err = reconciler.Client.Delete(context.TODO(), oldBSL)
+	assert.NoError(t, err)
+
+	// Repo reconciliation while BSL is gone - getIdentiferByBSL fails,
+	// checkNotReadyRepo patches repo to NotReady and returns (false, nil)
+	ready, err := reconciler.checkNotReadyRepo(context.TODO(), rr, reconciler.logger)
+	assert.NoError(t, err)
+	assert.False(t, ready)
+	assert.Equal(t, velerov1api.BackupRepositoryPhaseNotReady, rr.Status.Phase)
+	// The stale ResticIdentifier is unchanged since BSL was not found
+	assert.Equal(t, "s3:test.amazonaws.com/bucket/velero/restic/volume-ns-1", rr.Spec.ResticIdentifier)
+
+	// Step 3: BSL recreated with new prefix (simulates DPA recreation)
+	newBSL := &velerov1api.BackupStorageLocation{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: velerov1api.DefaultNamespace,
+			Name:      bslName,
+		},
+		Spec: velerov1api.BackupStorageLocationSpec{
+			Config: map[string]string{"resticRepoPrefix": "s3:test.amazonaws.com/bucket/velero-new/restic"},
+		},
+	}
+	err = reconciler.Client.Create(context.TODO(), newBSL)
+	assert.NoError(t, err)
+
+	// BSL create event triggers invalidateBackupReposForBSL (PR #7380)
+	requests := reconciler.invalidateBackupReposForBSL(context.TODO(), newBSL)
+	assert.NotNil(t, requests)
+
+	// Verify repo was marked NotReady with BSL change message
+	updated := &velerov1api.BackupRepository{}
+	err = reconciler.Client.Get(context.TODO(), types.NamespacedName{
+		Namespace: rr.Namespace,
+		Name:      rr.Name,
+	}, updated)
+	assert.NoError(t, err)
+	assert.Equal(t, velerov1api.BackupRepositoryPhaseNotReady, updated.Status.Phase)
+	assert.Equal(t, "re-establish on BSL change or create", updated.Status.Message)
+
+	// Step 4: Reconcile runs - checkNotReadyRepo should update identifier to new prefix
+	ready, err = reconciler.checkNotReadyRepo(context.TODO(), updated, reconciler.logger)
+	assert.NoError(t, err)
+	assert.True(t, ready)
+	assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, updated.Status.Phase)
+	assert.Equal(t, "s3:test.amazonaws.com/bucket/velero-new/restic/volume-ns-1", updated.Spec.ResticIdentifier)
+}
+
 func TestNeedInvalidBackupRepo(t *testing.T) {
 	tests := []struct {
 		name   string

pkg/repository/provider/unified_repo.go

@@ -235,7 +235,7 @@ func (urp *unifiedRepoProvider) BoostRepoConnect(ctx context.Context, param Repo
 		return nil
 	}
 
-	return urp.ConnectToRepo(ctx, param)
+	return urp.PrepareRepo(ctx, param)
 }
 
 func (urp *unifiedRepoProvider) PruneRepo(ctx context.Context, param RepoParam) error {

pkg/repository/provider/unified_repo_test.go

@@ -1232,7 +1232,7 @@ func TestBoostRepoConnect(t *testing.T) {
 			retFuncInit: func(context.Context, udmrepo.RepoOptions, bool) error {
 				return errors.New("fake-error-2")
 			},
-			expectedErr: "error to connect backup repo: fake-error-2",
+			expectedErr: "error to connect to backup repo: fake-error-2",
 		},
 		{
 			name:            "repo not opened and connect succeed",
@@ -1332,6 +1332,65 @@ func TestBoostRepoConnect(t *testing.T) {
 	}
 }
 
+// TestBoostRepoConnectInitializesWhenRepoNotFound tests that when the kopia repository
+// doesn't exist at the target storage location (e.g., after BSL prefix change),
+// BoostRepoConnect initializes a new repo via PrepareRepo instead of failing with
+// "error to connect backup repo". See https://github.com/vmware-tanzu/velero/issues/8279
+func TestBoostRepoConnectInitializesWhenRepoNotFound(t *testing.T) {
+	var backupRepo *reposervicenmocks.BackupRepo
+
+	getter := new(credmock.SecretStore)
+	getter.On("Get", mock.Anything, mock.Anything).Return("fake-password", nil)
+
+	repoService := new(reposervicenmocks.BackupRepoService)
+
+	repoService.On("Open", mock.Anything, mock.Anything).Return(
+		func(context.Context, udmrepo.RepoOptions) udmrepo.BackupRepo {
+			return backupRepo
+		},
+		func(context.Context, udmrepo.RepoOptions) error {
+			return errors.New("config file not found")
+		},
+	)
+
+	// Init with createNew=false returns ErrRepositoryNotInitialized (repo doesn't exist at new prefix)
+	// Init with createNew=true should succeed (initialize new repo)
+	repoService.On("Init", mock.Anything, mock.Anything, mock.Anything).Return(
+		func(ctx context.Context, repoOption udmrepo.RepoOptions, createNew bool) error {
+			if !createNew {
+				return repo.ErrRepositoryNotInitialized
+			}
+			return nil
+		},
+	)
+
+	funcTable = localFuncTable{
+		getStorageVariables: func(*velerov1api.BackupStorageLocation, string, string) (map[string]string, error) {
+			return map[string]string{}, nil
+		},
+		getStorageCredentials: func(*velerov1api.BackupStorageLocation, velerocredentials.FileStore) (map[string]string, error) {
+			return map[string]string{}, nil
+		},
+	}
+
+	urp := unifiedRepoProvider{
+		credentialGetter: velerocredentials.CredentialGetter{
+			FromSecret: getter,
+		},
+		repoService: repoService,
+		log:         velerotest.NewLogger(),
+	}
+
+	err := urp.BoostRepoConnect(context.Background(), RepoParam{
+		BackupLocation: &velerov1api.BackupStorageLocation{},
+		BackupRepo:     &velerov1api.BackupRepository{},
+	})
+
+	// With the fix, BoostRepoConnect should initialize the repo when ConnectToRepo
+	// gets ErrRepositoryNotInitialized, just like PrepareRepo does.
+	assert.NoError(t, err)
+}
+
 func TestPruneRepo(t *testing.T) {
 	testCases := []struct {
 		name            string