Fix BackupRepositories becoming stale when BSL config changes while Velero is not running

- Add BSLConfigHashAnnotation to track BSL configuration changes
- Implement calculateBSLConfigHash() for deterministic hash generation
- Add validateBackupRepositoriesOnStartup() to check repositories on startup
- Trigger validation on first reconciliation instead of using time.Sleep
- Use existing repoReady/repoNotReady helper functions consistently
- Add comprehensive test coverage for new functionality

Fixes #8279

Signed-off-by: Tapio Kaovila <tkaovila@redhat.com>

Author: kaovilai

pkg/apis/velero/v1/labels_annotations.go

@@ -54,6 +54,10 @@ const (
 	// RepositoryTypeLabel is the label key used to identify the type of a repository
 	RepositoryTypeLabel = "velero.io/repository-type"
 
+	// BSLConfigHashAnnotation is the annotation key for storing BSL config hash on BackupRepository
+	// to detect configuration changes that occurred while Velero was not running
+	BSLConfigHashAnnotation = "velero.io/bsl-config-hash"
+
 	// DataUploadLabel is the label key used to identify the dataupload for snapshot backup pod
 	DataUploadLabel = "velero.io/data-upload"
 

pkg/controller/backup_repository_controller.go

@@ -19,10 +19,14 @@ package controller
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"reflect"
 	"slices"
+	"sort"
+	"sync"
 	"time"
 
 	"github.com/petar/GoLLRB/llrb"
@@ -66,6 +70,10 @@ type BackupRepoReconciler struct {
 	repoMaintenanceConfig string
 	logLevel              logrus.Level
 	logFormat             *logging.FormatFlag
+
+	// Startup validation fields
+	startupValidationDone bool
+	startupMutex          sync.Mutex
 }
 
 func NewBackupRepoReconciler(
@@ -80,16 +88,18 @@ func NewBackupRepoReconciler(
 	logFormat *logging.FormatFlag,
 ) *BackupRepoReconciler {
 	c := &BackupRepoReconciler{
-		client,
-		namespace,
-		logger,
-		clocks.RealClock{},
-		maintenanceFrequency,
-		backupRepoConfig,
-		repositoryManager,
-		repoMaintenanceConfig,
-		logLevel,
-		logFormat,
+		Client:                client,
+		namespace:             namespace,
+		logger:                logger,
+		clock:                 clocks.RealClock{},
+		maintenanceFrequency:  maintenanceFrequency,
+		backupRepoConfig:      backupRepoConfig,
+		repositoryManager:     repositoryManager,
+		repoMaintenanceConfig: repoMaintenanceConfig,
+		logLevel:              logLevel,
+		logFormat:             logFormat,
+		startupValidationDone: false,
+		startupMutex:          sync.Mutex{},
 	}
 
 	return c
@@ -149,7 +159,13 @@ func (r *BackupRepoReconciler) invalidateBackupReposForBSL(ctx context.Context,
 	requests := []reconcile.Request{}
 	for i := range list.Items {
 		r.logger.WithField("BSL", bsl.Name).Infof("Invalidating Backup Repository %s", list.Items[i].Name)
-		if err := r.patchBackupRepository(context.Background(), &list.Items[i], repoNotReady("re-establish on BSL change, create or delete")); err != nil {
+		if err := r.patchBackupRepository(context.Background(), &list.Items[i], func(rr *velerov1api.BackupRepository) {
+			repoNotReady("re-establish on BSL change, create or delete")(rr)
+			// Clear the hash annotation so it will be recalculated
+			if rr.Annotations != nil {
+				delete(rr.Annotations, velerov1api.BSLConfigHashAnnotation)
+			}
+		}); err != nil {
 			r.logger.WithField("BSL", bsl.Name).WithError(err).Errorf("fail to patch BackupRepository %s", list.Items[i].Name)
 			continue
 		}
@@ -159,6 +175,128 @@ func (r *BackupRepoReconciler) invalidateBackupReposForBSL(ctx context.Context,
 	return requests
 }
 
+// calculateBSLConfigHash generates a deterministic hash of BSL configuration fields
+// that affect repository connectivity
+func calculateBSLConfigHash(bsl *velerov1api.BackupStorageLocation) string {
+	if bsl == nil {
+		return ""
+	}
+
+	// Create a struct with only the fields that affect repository connectivity
+	configData := struct {
+		Bucket string
+		Prefix string
+		CACert []byte
+		Config map[string]string
+	}{}
+
+	if bsl.Spec.StorageType.ObjectStorage != nil {
+		configData.Bucket = bsl.Spec.StorageType.ObjectStorage.Bucket
+		configData.Prefix = bsl.Spec.StorageType.ObjectStorage.Prefix
+		configData.CACert = bsl.Spec.StorageType.ObjectStorage.CACert
+	}
+
+	// Sort config keys for deterministic hash
+	if bsl.Spec.Config != nil {
+		configData.Config = make(map[string]string)
+		keys := make([]string, 0, len(bsl.Spec.Config))
+		for k := range bsl.Spec.Config {
+			keys = append(keys, k)
+		}
+		sort.Strings(keys)
+		for _, k := range keys {
+			configData.Config[k] = bsl.Spec.Config[k]
+		}
+	}
+
+	// Calculate hash
+	data, err := json.Marshal(configData)
+	if err != nil {
+		// This should never happen with our simple struct, but handle it anyway
+		// Return empty string which will trigger repository re-validation
+		return ""
+	}
+	hash := sha256.Sum256(data)
+	return hex.EncodeToString(hash[:])
+}
+
+// validateBackupRepositoriesOnStartup checks all BackupRepositories on controller startup
+// and invalidates any that have stale BSL configuration
+func (r *BackupRepoReconciler) validateBackupRepositoriesOnStartup(ctx context.Context) error {
+	r.logger.Info("Validating backup repositories on startup")
+
+	// List all BackupRepositories
+	repoList := &velerov1api.BackupRepositoryList{}
+	if err := r.List(ctx, repoList); err != nil {
+		return errors.Wrap(err, "failed to list backup repositories")
+	}
+
+	// List all BSLs for hash calculation
+	bslList := &velerov1api.BackupStorageLocationList{}
+	if err := r.List(ctx, bslList); err != nil {
+		return errors.Wrap(err, "failed to list backup storage locations")
+	}
+
+	// Create map of BSL name to BSL for quick lookup
+	bslMap := make(map[string]*velerov1api.BackupStorageLocation)
+	for i := range bslList.Items {
+		bslMap[bslList.Items[i].Name] = &bslList.Items[i]
+	}
+
+	invalidatedCount := 0
+	for i := range repoList.Items {
+		repo := &repoList.Items[i]
+
+		// Skip if repository is new (not yet initialized)
+		if repo.Status.Phase == "" || repo.Status.Phase == velerov1api.BackupRepositoryPhaseNew {
+			continue
+		}
+
+		// Get the associated BSL
+		bsl, exists := bslMap[repo.Spec.BackupStorageLocation]
+		if !exists {
+			r.logger.WithField("repo", repo.Name).Warn("BSL not found for repository, skipping validation")
+			continue
+		}
+
+		// Calculate current BSL config hash
+		currentHash := calculateBSLConfigHash(bsl)
+
+		// Get stored hash from annotation
+		storedHash := ""
+		if repo.Annotations != nil {
+			storedHash = repo.Annotations[velerov1api.BSLConfigHashAnnotation]
+		}
+
+		// If hashes don't match or annotation is missing, invalidate the repository
+		if storedHash == "" || storedHash != currentHash {
+			r.logger.WithFields(logrus.Fields{
+				"repo":        repo.Name,
+				"bsl":         bsl.Name,
+				"storedHash":  storedHash,
+				"currentHash": currentHash,
+			}).Info("Invalidating backup repository due to BSL configuration change detected on startup")
+
+			if err := r.patchBackupRepository(ctx, repo, func(rr *velerov1api.BackupRepository) {
+				repoNotReady("BSL configuration changed while Velero was not running, re-establishing connection")(rr)
+
+				// Update the hash annotation
+				if rr.Annotations == nil {
+					rr.Annotations = make(map[string]string)
+				}
+				rr.Annotations[velerov1api.BSLConfigHashAnnotation] = currentHash
+			}); err != nil {
+				r.logger.WithField("repo", repo.Name).WithError(err).Error("Failed to invalidate backup repository")
+				continue
+			}
+			invalidatedCount++
+		}
+	}
+
+	r.logger.WithField("invalidatedCount", invalidatedCount).Info("Completed backup repository validation on startup")
+	return nil
+}
+
 // needInvalidBackupRepo returns true if the BSL's storage type, bucket, prefix, CACert, or config has changed
 func (r *BackupRepoReconciler) needInvalidBackupRepo(oldObj client.Object, newObj client.Object) bool {
 	oldBSL := oldObj.(*velerov1api.BackupStorageLocation)
@@ -212,6 +350,23 @@ func (r *BackupRepoReconciler) needInvalidBackupRepo(oldObj client.Object, newOb
 }
 
 func (r *BackupRepoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	// Perform one-time startup validation
+	r.startupMutex.Lock()
+	if !r.startupValidationDone {
+		r.startupValidationDone = true
+		r.startupMutex.Unlock()
+
+		// Run validation asynchronously to avoid blocking this reconciliation
+		go func() {
+			r.logger.Info("Running startup validation for backup repositories")
+			if err := r.validateBackupRepositoriesOnStartup(context.Background()); err != nil {
+				r.logger.WithError(err).Error("Failed to validate backup repositories on startup")
+			}
+		}()
+	} else {
+		r.startupMutex.Unlock()
+	}
+
 	log := r.logger.WithField("backupRepo", req.String())
 	backupRepo := &velerov1api.BackupRepository{}
 	if err := r.Get(ctx, req.NamespacedName, backupRepo); err != nil {
@@ -315,8 +470,7 @@ func (r *BackupRepoReconciler) initializeRepo(ctx context.Context, req *velerov1
 		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
+				repoNotReady(err.Error())(rr)
 
 				if rr.Spec.MaintenanceFrequency.Duration <= 0 {
 					rr.Spec.MaintenanceFrequency = metav1.Duration{Duration: r.getRepositoryMaintenanceFrequency(req)}
@@ -332,6 +486,9 @@ func (r *BackupRepoReconciler) initializeRepo(ctx context.Context, req *velerov1
 		log.Infof("Init repo with config %v", config)
 	}
 
+	// Calculate BSL config hash for tracking changes
+	bslConfigHash := calculateBSLConfigHash(bsl)
+
 	// 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) {
 		// Only set ResticIdentifier for restic repositories
@@ -344,6 +501,12 @@ func (r *BackupRepoReconciler) initializeRepo(ctx context.Context, req *velerov1
 		}
 
 		rr.Spec.RepositoryConfig = config
+
+		// Add BSL config hash annotation
+		if rr.Annotations == nil {
+			rr.Annotations = make(map[string]string)
+		}
+		rr.Annotations[velerov1api.BSLConfigHashAnnotation] = bslConfigHash
 	}); err != nil {
 		return err
 	}
@@ -353,7 +516,7 @@ func (r *BackupRepoReconciler) initializeRepo(ctx context.Context, req *velerov1
 	}
 
 	return r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) {
-		rr.Status.Phase = velerov1api.BackupRepositoryPhaseReady
+		repoReady()(rr)
 		rr.Status.LastMaintenanceTime = &metav1.Time{Time: time.Now()}
 	})
 }
@@ -539,6 +702,9 @@ 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")
 
+	// Calculate current BSL config hash
+	bslConfigHash := calculateBSLConfigHash(bsl)
+
 	// Only check and update restic identifier for restic repositories
 	if req.Spec.RepositoryType == "" || req.Spec.RepositoryType == velerov1api.BackupRepositoryTypeRestic {
 		repoIdentifier, err := r.getIdentifierByBSL(bsl, req)
@@ -560,7 +726,17 @@ 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())
+
+	// Update status to ready and update BSL config hash
+	err := r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) {
+		repoReady()(rr)
+
+		// Update BSL config hash annotation
+		if rr.Annotations == nil {
+			rr.Annotations = make(map[string]string)
+		}
+		rr.Annotations[velerov1api.BSLConfigHashAnnotation] = bslConfigHash
+	})
 	if err != nil {
 		return false, err
 	}