🐛 BSLs with validation disabled should be validated at least once (#3084)

* 🐛 BSLs with validation disabled should be validated at least once

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

* review comments

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

changelogs/unreleased/3084-ashish-amarnath

@@ -0,0 +1 @@
+🐛 BSLs with validation disabled should be validated at least once

internal/storage/storagelocation.go

@@ -51,24 +51,28 @@ func IsReadyToValidate(bslValidationFrequency *metav1.Duration, lastValidationTi
 		validationFrequency = bslValidationFrequency.Duration
 	}
 
-	if validationFrequency == 0 {
-		log.Debug("Validation period for this backup location is set to 0, skipping validation")
-		return false
-	}
-
 	if validationFrequency < 0 {
 		log.Debugf("Validation period must be non-negative, changing from %d to %d", validationFrequency, defaultLocationInfo.StoreValidationFrequency)
 		validationFrequency = defaultLocationInfo.StoreValidationFrequency
 	}
 
 	lastValidation := lastValidationTime
-	if lastValidation != nil { // always ready to validate the first time around, so only even do this check if this has happened before
-		nextValidation := lastValidation.Add(validationFrequency) // next validation time: last validation time + validation frequency
-		if time.Now().UTC().Before(nextValidation) {              // ready only when NOW is equal to or after the next validation time
-			return false
-		}
+	if lastValidation == nil {
+		// Regardless of validation frequency, we want to validate all BSLs at least once.
+		return true
 	}
 
+	if validationFrequency == 0 {
+		// Validation was disabled so return false.
+		log.Debug("Validation period for this backup location is set to 0, skipping validation")
+		return false
+	}
+
+	// We want to validate BSL only if the set validation frequency/ interval has elapsed.
+	nextValidation := lastValidation.Add(validationFrequency) // next validation time: last validation time + validation frequency
+	if time.Now().UTC().Before(nextValidation) {              // ready only when NOW is equal to or after the next validation time
+		return false
+	}
 	return true
 }
 

internal/storage/storagelocation_test.go

@@ -36,17 +36,23 @@ func TestIsReadyToValidate(t *testing.T) {
 		bslValidationFrequency *metav1.Duration
 		lastValidationTime     *metav1.Time
 		defaultLocationInfo    DefaultBackupLocationInfo
-
-		// serverDefaultValidationFrequency time.Duration
-		// backupLocation                   *velerov1api.BackupStorageLocation
-		ready bool
+		ready                  bool
 	}{
 		{
-			name:                   "don't validate, since frequency is set to zero",
+			name:                   "validate when true when validation frequency is zero and lastValidationTime is nil",
 			bslValidationFrequency: &metav1.Duration{Duration: 0},
 			defaultLocationInfo: DefaultBackupLocationInfo{
 				StoreValidationFrequency: 0,
 			},
+			ready: true,
+		},
+		{
+			name:                   "don't validate when false when validation is disabled and lastValidationTime is not nil",
+			bslValidationFrequency: &metav1.Duration{Duration: 0},
+			lastValidationTime:     &metav1.Time{Time: time.Now()},
+			defaultLocationInfo: DefaultBackupLocationInfo{
+				StoreValidationFrequency: 0,
+			},
 			ready: false,
 		},
 		{
@@ -63,7 +69,8 @@ func TestIsReadyToValidate(t *testing.T) {
 			defaultLocationInfo: DefaultBackupLocationInfo{
 				StoreValidationFrequency: 1,
 			},
-			ready: false,
+			lastValidationTime: &metav1.Time{Time: time.Now()},
+			ready:              false,
 		},
 		{
 			name: "validate as per default setting when location setting is not set",
@@ -77,7 +84,8 @@ func TestIsReadyToValidate(t *testing.T) {
 			defaultLocationInfo: DefaultBackupLocationInfo{
 				StoreValidationFrequency: 0,
 			},
-			ready: false,
+			lastValidationTime: &metav1.Time{Time: time.Now()},
+			ready:              false,
 		},
 		{
 			name:                   "don't validate when now is before the NEXT validation time (validation frequency + last validation time)",
@@ -112,8 +120,8 @@ func TestIsReadyToValidate(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 			log := velerotest.NewLogger()
-
-			g.Expect(IsReadyToValidate(tt.bslValidationFrequency, tt.lastValidationTime, tt.defaultLocationInfo, log)).To(BeIdenticalTo(tt.ready))
+			actual := IsReadyToValidate(tt.bslValidationFrequency, tt.lastValidationTime, tt.defaultLocationInfo, log)
+			g.Expect(actual).To(BeIdenticalTo(tt.ready))
 		})
 	}
 }

pkg/controller/backup_storage_location_controller_test.go

@@ -120,13 +120,13 @@ var _ = Describe("Backup Storage Location Reconciler", func() {
 			wantErr        bool
 		}{
 			{
-				backupLocation: builder.ForBackupStorageLocation("ns-1", "location-1").ValidationFrequency(0).Result(),
+				backupLocation: builder.ForBackupStorageLocation("ns-1", "location-1").ValidationFrequency(0).LastValidationTime(time.Now()).Result(),
 				isValidError:   nil,
 				expectedPhase:  "",
 				wantErr:        false,
 			},
 			{
-				backupLocation: builder.ForBackupStorageLocation("ns-1", "location-2").ValidationFrequency(0).Result(),
+				backupLocation: builder.ForBackupStorageLocation("ns-1", "location-2").ValidationFrequency(0).LastValidationTime(time.Now()).Result(),
 				isValidError:   nil,
 				expectedPhase:  "",
 				wantErr:        false,