Fix default BSL setting not work

Signed-off-by: Ming Qiu <[email protected]>

changelogs/unreleased/6771-qiuming-best

@@ -0,0 +1 @@
+Fix default BSL setting not work

internal/storage/storagelocation.go

@@ -18,6 +18,7 @@ package storage
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	"github.com/pkg/errors"
@@ -92,3 +93,18 @@ func ListBackupStorageLocations(ctx context.Context, kbClient client.Client, nam
 
 	return locations, nil
 }
+
+func GetDefaultBackupStorageLocations(ctx context.Context, kbClient client.Client, namespace string) (*velerov1api.BackupStorageLocationList, error) {
+	locations := new(velerov1api.BackupStorageLocationList)
+	defaultLocations := new(velerov1api.BackupStorageLocationList)
+	if err := kbClient.List(context.Background(), locations, &client.ListOptions{Namespace: namespace}); err != nil {
+		return defaultLocations, errors.Wrapf(err, fmt.Sprintf("failed to list backup storage locations in namespace %s", namespace))
+	}
+
+	for _, location := range locations.Items {
+		if location.Spec.Default {
+			defaultLocations.Items = append(defaultLocations.Items, location)
+		}
+	}
+	return defaultLocations, nil
+}

pkg/cmd/cli/backuplocation/create.go

@@ -31,6 +31,7 @@ import (
 
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/vmware-tanzu/velero/internal/storage"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
 	"github.com/vmware-tanzu/velero/pkg/client"
@@ -204,19 +205,13 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error {
 
 	if o.DefaultBackupStorageLocation {
 		// There is one and only one default backup storage location.
-		// Disable any existing default backup storage location.
-		locations := new(velerov1api.BackupStorageLocationList)
-		if err := kbClient.List(context.Background(), locations, &kbclient.ListOptions{Namespace: f.Namespace()}); err != nil {
+		// Disable any existing default backup storage location first.
+		defalutBSLs, err := storage.GetDefaultBackupStorageLocations(context.Background(), kbClient, f.Namespace())
+		if err != nil {
 			return errors.WithStack(err)
 		}
-		for i, location := range locations.Items {
-			if location.Spec.Default {
-				location.Spec.Default = false
-				if err := kbClient.Update(context.Background(), &locations.Items[i], &kbclient.UpdateOptions{}); err != nil {
-					return errors.WithStack(err)
-				}
-				break
-			}
+		if len(defalutBSLs.Items) > 0 {
+			return errors.New("there is already exist default backup storage location, please unset it first or do not set --default flag")
 		}
 	}
 

pkg/cmd/cli/backuplocation/set.go

@@ -28,6 +28,7 @@ import (
 
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/vmware-tanzu/velero/internal/storage"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
 	"github.com/vmware-tanzu/velero/pkg/client"
@@ -104,36 +105,40 @@ func (o *SetOptions) Run(c *cobra.Command, f client.Factory) error {
 		}
 	}
 
-	location := &velerov1api.BackupStorageLocation{}
-	err = kbClient.Get(context.Background(), kbclient.ObjectKey{
-		Namespace: f.Namespace(),
-		Name:      o.Name,
-	}, location)
+	defalutBSLs, err := storage.GetDefaultBackupStorageLocations(context.Background(), kbClient, f.Namespace())
 	if err != nil {
 		return errors.WithStack(err)
 	}
 
-	if o.DefaultBackupStorageLocation {
-		// There is one and only one default backup storage location.
-		// Disable the origin default backup storage location.
-		locations := new(velerov1api.BackupStorageLocationList)
-		if err := kbClient.List(context.Background(), locations, &kbclient.ListOptions{Namespace: f.Namespace()}); err != nil {
-			return errors.WithStack(err)
+	if !o.DefaultBackupStorageLocation { // unset default backup storage location
+		if len(defalutBSLs.Items) == 0 {
+			return errors.New("no default backup storage location found for removing default")
 		}
-		for i, location := range locations.Items {
-			if !location.Spec.Default {
-				continue
-			}
-			if location.Name == o.Name {
-				// Do not update if the origin default BSL is the current one.
+		// check whether it is already unset default
+		foundDefault := false
+		for _, bsl := range defalutBSLs.Items {
+			if bsl.Name == o.Name {
+				foundDefault = true
 				break
 			}
-			location.Spec.Default = false
-			if err := kbClient.Update(context.Background(), &locations.Items[i], &kbclient.UpdateOptions{}); err != nil {
-				return errors.WithStack(err)
-			}
-			break
 		}
+		if !foundDefault {
+			return errors.New("it is already one non-default backup storage location")
+		}
+	} else { // set default backup storage location
+		// need first check if there is already a default backup storage location
+		if len(defalutBSLs.Items) > 0 {
+			return errors.New("there is already a default backup storage location")
+		}
+	}
+
+	location := &velerov1api.BackupStorageLocation{}
+	err = kbClient.Get(context.Background(), kbclient.ObjectKey{
+		Namespace: f.Namespace(),
+		Name:      o.Name,
+	}, location)
+	if err != nil {
+		return errors.WithStack(err)
 	}
 
 	location.Spec.Default = o.DefaultBackupStorageLocation

pkg/cmd/cli/backuplocation/set_test.go

@@ -102,7 +102,7 @@ func TestSetCommand_Execute(t *testing.T) {
 	_, stderr, err := veleroexec.RunCommand(cmd)
 
 	if err != nil {
-		assert.Contains(t, stderr, fmt.Sprintf("backupstoragelocations.velero.io \"%s\" not found", bsl))
+		assert.Contains(t, stderr, "no default backup storage location found")
 		return
 	}
 	t.Fatalf("process ran with err %v, want backup delete successfully", err)

pkg/controller/backup_storage_location_controller.go

@@ -96,11 +96,8 @@ func (r *backupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr
 	pluginManager := r.newPluginManager(log)
 	defer pluginManager.CleanupClients()
 
-	var defaultFound bool
+	// find the BSL that matches the request
 	for _, bsl := range locationList.Items {
-		if bsl.Spec.Default {
-			defaultFound = true
-		}
 		if bsl.Name == req.Name && bsl.Namespace == req.Namespace {
 			location = bsl
 		}
@@ -111,15 +108,11 @@ func (r *backupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr
 		return ctrl.Result{}, nil
 	}
 
-	isDefault := location.Spec.Default
-
-	// TODO(2.0) remove this check since the server default will be deprecated
-	if !defaultFound && location.Name == r.defaultBackupLocationInfo.StorageLocation {
-		// For backward-compatible, to configure the backup storage location as the default if
-		// none of the BSLs be marked as the default and the BSL name matches against the
-		// "velero server --default-backup-storage-location".
-		isDefault = true
-		defaultFound = true
+	// decide the default BSL
+	defaultFound, err := r.ensureSingleDefaultBSL(locationList)
+	if err != nil {
+		log.WithError(err).Error("failed to ensure single default bsl")
+		return ctrl.Result{}, nil
 	}
 
 	func() {
@@ -155,9 +148,6 @@ func (r *backupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr
 			log.WithError(err).Error("fail to validate backup store")
 			return
 		}
-
-		// updates the default backup location
-		location.Spec.Default = isDefault
 	}()
 
 	r.logReconciledPhase(defaultFound, locationList, unavailableErrors)
@@ -220,3 +210,68 @@ func (r *backupStorageLocationReconciler) SetupWithManager(mgr ctrl.Manager) err
 		Watches(g, nil, builder.WithPredicates(gp)).
 		Complete(r)
 }
+
+// ensureSingleDefaultBSL ensures that there is only one default BSL in the namespace.
+// the default BSL priority is as follows:
+// 1. follow the user's setting (the most recent validation BSL is the default BSL)
+// 2. follow the server's setting ("velero server --default-backup-storage-location")
+func (r *backupStorageLocationReconciler) ensureSingleDefaultBSL(locationList velerov1api.BackupStorageLocationList) (bool, error) {
+	// get all default BSLs
+	var defaultBSLs []*velerov1api.BackupStorageLocation
+	var defaultFound bool
+	for i, location := range locationList.Items {
+		if location.Spec.Default {
+			defaultBSLs = append(defaultBSLs, &locationList.Items[i])
+		}
+	}
+
+	if len(defaultBSLs) > 1 { // more than 1 default BSL
+		// find the most recent updated default BSL
+		var mostRecentUpdatedBSL *velerov1api.BackupStorageLocation
+		defaultFound = true
+		for _, bsl := range defaultBSLs {
+			if mostRecentUpdatedBSL == nil {
+				mostRecentUpdatedBSL = bsl
+				continue
+			}
+			if mostRecentUpdatedBSL.Status.LastValidationTime.Before(bsl.Status.LastValidationTime) {
+				mostRecentUpdatedBSL = bsl
+			}
+		}
+
+		// unset all other default BSLs
+		for _, bsl := range defaultBSLs {
+			if bsl.Name != mostRecentUpdatedBSL.Name {
+				bsl.Spec.Default = false
+				if err := r.client.Update(r.ctx, bsl); err != nil {
+					return defaultFound, errors.Wrapf(err, "failed to unset default backup storage location %q", bsl.Name)
+				}
+				r.log.Debugf("update default backup storage location %q to false", bsl.Name)
+			}
+		}
+	} else if len(defaultBSLs) == 0 { // no default BSL
+		// find the BSL that matches the "velero server --default-backup-storage-location"
+		var defaultBSL *velerov1api.BackupStorageLocation
+		for i, location := range locationList.Items {
+			if location.Name == r.defaultBackupLocationInfo.StorageLocation {
+				defaultBSL = &locationList.Items[i]
+				break
+			}
+		}
+
+		// set the default BSL
+		if defaultBSL != nil {
+			defaultBSL.Spec.Default = true
+			defaultFound = true
+			if err := r.client.Update(r.ctx, defaultBSL); err != nil {
+				return defaultFound, errors.Wrapf(err, "failed to set default backup storage location %q", defaultBSL.Name)
+			}
+			r.log.Debugf("update default backup storage location %q to true", defaultBSL.Name)
+		} else {
+			defaultFound = false
+		}
+	} else { // only 1 default BSL
+		defaultFound = true
+	}
+	return defaultFound, nil
+}

pkg/controller/backup_storage_location_controller_test.go

@@ -17,12 +17,15 @@ limitations under the License.
 package controller
 
 import (
+	"context"
+	"testing"
 	"time"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -172,3 +175,125 @@ var _ = Describe("Backup Storage Location Reconciler", func() {
 		}
 	})
 })
+
+func TestEnsureSingleDefaultBSL(t *testing.T) {
+	tests := []struct {
+		name               string
+		locations          velerov1api.BackupStorageLocationList
+		defaultBackupInfo  storage.DefaultBackupLocationInfo
+		expectedDefaultSet bool
+		expectedError      error
+	}{
+		{
+			name: "MultipleDefaults",
+			locations: func() velerov1api.BackupStorageLocationList {
+				var locations velerov1api.BackupStorageLocationList
+				locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-1").LastValidationTime(time.Now()).Default(true).Result())
+				locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").LastValidationTime(time.Now().Add(-1 * time.Hour)).Default(true).Result())
+				return locations
+			}(),
+			expectedDefaultSet: true,
+			expectedError:      nil,
+		},
+		{
+			name: "NoDefault with exist default bsl in defaultBackupInfo",
+			locations: func() velerov1api.BackupStorageLocationList {
+				var locations velerov1api.BackupStorageLocationList
+				locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-1").Default(false).Result())
+				locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").Default(false).Result())
+				return locations
+			}(),
+			defaultBackupInfo: storage.DefaultBackupLocationInfo{
+				StorageLocation: "location-2",
+			},
+			expectedDefaultSet: true,
+			expectedError:      nil,
+		},
+		{
+			name: "NoDefault with non-exist default bsl in defaultBackupInfo",
+			locations: func() velerov1api.BackupStorageLocationList {
+				var locations velerov1api.BackupStorageLocationList
+				locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-1").Default(false).Result())
+				locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").Default(false).Result())
+				return locations
+			}(),
+			defaultBackupInfo: storage.DefaultBackupLocationInfo{
+				StorageLocation: "location-3",
+			},
+			expectedDefaultSet: false,
+			expectedError:      nil,
+		},
+		{
+			name: "SingleDefault",
+			locations: func() velerov1api.BackupStorageLocationList {
+				var locations velerov1api.BackupStorageLocationList
+				locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-1").Default(true).Result())
+				locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").Default(false).Result())
+				return locations
+			}(),
+			expectedDefaultSet: true,
+			expectedError:      nil,
+		},
+	}
+
+	for _, test := range tests {
+		// Setup reconciler
+		assert.Nil(t, velerov1api.AddToScheme(scheme.Scheme))
+		t.Run(test.name, func(t *testing.T) {
+			r := &backupStorageLocationReconciler{
+				ctx:                       context.Background(),
+				client:                    fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(&test.locations).Build(),
+				defaultBackupLocationInfo: test.defaultBackupInfo,
+				log:                       velerotest.NewLogger(),
+			}
+			defaultFound, err := r.ensureSingleDefaultBSL(test.locations)
+
+			assert.Equal(t, test.expectedDefaultSet, defaultFound)
+			assert.Equal(t, test.expectedError, err)
+		})
+	}
+}
+
+func TestBSLReconcile(t *testing.T) {
+	tests := []struct {
+		name          string
+		locationList  velerov1api.BackupStorageLocationList
+		defaultFound  bool
+		expectedError error
+	}{
+		{
+			name:          "NoBSL",
+			locationList:  velerov1api.BackupStorageLocationList{},
+			defaultFound:  false,
+			expectedError: nil,
+		},
+		{
+			name: "BSLNotFound",
+			locationList: func() velerov1api.BackupStorageLocationList {
+				var locations velerov1api.BackupStorageLocationList
+				locations.Items = append(locations.Items, *builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "location-2").Result())
+				return locations
+			}(),
+			defaultFound:  false,
+			expectedError: nil,
+		},
+	}
+	pluginManager := &pluginmocks.Manager{}
+	pluginManager.On("CleanupClients").Return(nil)
+	for _, test := range tests {
+		// Setup reconciler
+		assert.Nil(t, velerov1api.AddToScheme(scheme.Scheme))
+		t.Run(test.name, func(t *testing.T) {
+			r := &backupStorageLocationReconciler{
+				ctx:              context.Background(),
+				client:           fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(&test.locationList).Build(),
+				newPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
+				log:              velerotest.NewLogger(),
+			}
+
+			result, err := r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: velerov1api.DefaultNamespace, Name: "location-1"}})
+			assert.Equal(t, test.expectedError, err)
+			assert.Equal(t, ctrl.Result{}, result)
+		})
+	}
+}