Fix default BSL setting not work

Signed-off-by: Ming Qiu <mqiu@vmware.com>

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,11 +28,13 @@ 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"
 	"github.com/vmware-tanzu/velero/pkg/cmd"
 	"github.com/vmware-tanzu/velero/pkg/cmd/util/flag"
+	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 )
 
 func NewSetCommand(f client.Factory, use string) *cobra.Command {
@@ -58,7 +60,7 @@ type SetOptions struct {
 	Name                         string
 	CACertFile                   string
 	Credential                   flag.Map
-	DefaultBackupStorageLocation bool
+	DefaultBackupStorageLocation flag.OptionalBool
 }
 
 func NewSetOptions() *SetOptions {
@@ -70,7 +72,8 @@ func NewSetOptions() *SetOptions {
 func (o *SetOptions) BindFlags(flags *pflag.FlagSet) {
 	flags.StringVar(&o.CACertFile, "cacert", o.CACertFile, "File containing a certificate bundle to use when verifying TLS connections to the object store. Optional.")
 	flags.Var(&o.Credential, "credential", "Sets the credential to be used by this location as a key-value pair, where the key is the Kubernetes Secret name, and the value is the data key name within the Secret. Optional, one value only.")
-	flags.BoolVar(&o.DefaultBackupStorageLocation, "default", o.DefaultBackupStorageLocation, "Sets this new location to be the new default backup storage location. Optional.")
+	f := flags.VarPF(&o.DefaultBackupStorageLocation, "default", "", "Sets this new location to be the new default backup storage location. Optional.")
+	f.NoOptDefVal = cmd.TRUE
 }
 
 func (o *SetOptions) Validate(c *cobra.Command, args []string, f client.Factory) error {
@@ -113,30 +116,25 @@ func (o *SetOptions) Run(c *cobra.Command, f client.Factory) error {
 		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)
-		}
-		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.
-				break
-			}
-			location.Spec.Default = false
-			if err := kbClient.Update(context.Background(), &locations.Items[i], &kbclient.UpdateOptions{}); err != nil {
+	defaultOpt := o.DefaultBackupStorageLocation.Value
+	if defaultOpt != nil {
+		if *defaultOpt { // set default backup storage location
+			// need first check if there is already a default backup storage location
+			defalutBSLs, err := storage.GetDefaultBackupStorageLocations(context.Background(), kbClient, f.Namespace())
+			if err != nil {
 				return errors.WithStack(err)
 			}
-			break
+			if len(defalutBSLs.Items) > 0 {
+				return errors.New("there is already a default backup storage location")
+			}
 		}
+	} else {
+		// user do not specify default option
+		// we should keep the original default option
+		o.DefaultBackupStorageLocation = flag.OptionalBool{Value: &location.Spec.Default}
 	}
 
-	location.Spec.Default = o.DefaultBackupStorageLocation
+	location.Spec.Default = boolptr.IsSetToTrue(o.DefaultBackupStorageLocation.Value)
 	location.Spec.StorageType.ObjectStorage.CACert = caCertData
 
 	for name, key := range o.Credential.Data() {

pkg/cmd/cli/backuplocation/set_test.go

@@ -31,6 +31,7 @@ import (
 	cmdtest "github.com/vmware-tanzu/velero/pkg/cmd/test"
 	veleroflag "github.com/vmware-tanzu/velero/pkg/cmd/util/flag"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
+	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	veleroexec "github.com/vmware-tanzu/velero/pkg/util/exec"
 )
 
@@ -73,7 +74,7 @@ func TestNewSetCommand(t *testing.T) {
 	// verify all options are set as expected
 	assert.Equal(t, backupName, o.Name)
 	assert.Equal(t, cacert, o.CACertFile)
-	assert.Equal(t, defaultBackupStorageLocation, o.DefaultBackupStorageLocation)
+	assert.Equal(t, defaultBackupStorageLocation, boolptr.IsSetToTrue(o.DefaultBackupStorageLocation.Value))
 	assert.Equal(t, true, reflect.DeepEqual(credential, o.Credential))
 
 	assert.Contains(t, e.Error(), fmt.Sprintf("%s: no such file or directory", cacert))
@@ -102,7 +103,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, "backupstoragelocations.velero.io \"bsl-1\" not 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,72 @@ 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 mostRecentCreatedBSL *velerov1api.BackupStorageLocation
+		defaultFound = true
+		for _, bsl := range defaultBSLs {
+			if mostRecentCreatedBSL == nil {
+				mostRecentCreatedBSL = bsl
+				continue
+			}
+			// For lack of a better way to compare timestamps, we use the CreationTimestamp
+			// it cloud not really find the most recent updated BSL, but it is good enough for now
+			bslTimestamp := bsl.CreationTimestamp
+			mostRecentTimestamp := mostRecentCreatedBSL.CreationTimestamp
+			if mostRecentTimestamp.Before(&bslTimestamp) {
+				mostRecentCreatedBSL = bsl
+			}
+		}
+
+		// unset all other default BSLs
+		for _, bsl := range defaultBSLs {
+			if bsl.Name != mostRecentCreatedBSL.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
+}