Only set BSL credential field if provided (#4322)

Previously, the BSL credential field would always be set when using the
`create` command, even if no credential details were provided. This
would result in an empty `SecretKeySelector` in the BSL which would
cause operations using this BSL to fail as Velero would attempt to fetch
a `Secret` with an empty name from the K8s API server.

With this change, the `Credential` field is only set if credential
details have been specified. This change also includes some refactoring
to allow the change to be tested.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

changelogs/unreleased/4322-zubron

@@ -0,0 +1 @@
+Fixed an issue with the `backup-location create` command where the BSL Credential field would be set to an invalid empty SecretKeySelector when no credential details were provided.

pkg/cmd/cli/backuplocation/create.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2020 the Velero contributors.
+Copyright the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -133,39 +133,22 @@ func (o *CreateOptions) Complete(args []string, f client.Factory) error {
 	return nil
 }
 
-func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error {
-	var backupSyncPeriod, validationFrequency *metav1.Duration
-
+func (o *CreateOptions) BuildBackupStorageLocation(namespace string, setBackupSyncPeriod, setValidationFrequency bool) (*velerov1api.BackupStorageLocation, error) {
 	var caCertData []byte
 	if o.CACertFile != "" {
 		realPath, err := filepath.Abs(o.CACertFile)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		caCertData, err = ioutil.ReadFile(realPath)
 		if err != nil {
-			return err
+			return nil, err
 		}
 	}
 
-	if c.Flags().Changed("backup-sync-period") {
-		backupSyncPeriod = &metav1.Duration{Duration: o.BackupSyncPeriod}
-	}
-
-	if c.Flags().Changed("validation-frequency") {
-		validationFrequency = &metav1.Duration{Duration: o.ValidationFrequency}
-	}
-
-	var secretName, secretKey string
-	for k, v := range o.Credential.Data() {
-		secretName = k
-		secretKey = v
-		break
-	}
-
 	backupStorageLocation := &velerov1api.BackupStorageLocation{
 		ObjectMeta: metav1.ObjectMeta{
-			Namespace: f.Namespace(),
+			Namespace: namespace,
 			Name:      o.Name,
 			Labels:    o.Labels.Data(),
 		},
@@ -178,15 +161,37 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error {
 					CACert: caCertData,
 				},
 			},
-			Config:              o.Config.Data(),
-			Credential:          builder.ForSecretKeySelector(secretName, secretKey).Result(),
-			Default:             o.DefaultBackupStorageLocation,
-			AccessMode:          velerov1api.BackupStorageLocationAccessMode(o.AccessMode.String()),
-			BackupSyncPeriod:    backupSyncPeriod,
-			ValidationFrequency: validationFrequency,
+			Config:     o.Config.Data(),
+			Default:    o.DefaultBackupStorageLocation,
+			AccessMode: velerov1api.BackupStorageLocationAccessMode(o.AccessMode.String()),
 		},
 	}
 
+	if setBackupSyncPeriod {
+		backupStorageLocation.Spec.BackupSyncPeriod = &metav1.Duration{Duration: o.BackupSyncPeriod}
+	}
+
+	if setValidationFrequency {
+		backupStorageLocation.Spec.ValidationFrequency = &metav1.Duration{Duration: o.ValidationFrequency}
+	}
+
+	for secretName, secretKey := range o.Credential.Data() {
+		backupStorageLocation.Spec.Credential = builder.ForSecretKeySelector(secretName, secretKey).Result()
+		break
+	}
+
+	return backupStorageLocation, nil
+}
+
+func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error {
+	setBackupSyncPeriod := c.Flags().Changed("backup-sync-period")
+	setValidationFrequency := c.Flags().Changed("validation-frequency")
+
+	backupStorageLocation, err := o.BuildBackupStorageLocation(f.Namespace(), setBackupSyncPeriod, setValidationFrequency)
+	if err != nil {
+		return err
+	}
+
 	if printed, err := output.PrintWithFormat(c, backupStorageLocation); printed || err != nil {
 		return err
 	}

pkg/cmd/cli/backuplocation/create_test.go

@@ -0,0 +1,78 @@
+/*
+Copyright the Velero Contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package backuplocation
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestBuildBackupStorageLocationSetsNamespace(t *testing.T) {
+	o := NewCreateOptions()
+
+	bsl, err := o.BuildBackupStorageLocation("velero-test-ns", false, false)
+	assert.NoError(t, err)
+	assert.Equal(t, "velero-test-ns", bsl.Namespace)
+}
+
+func TestBuildBackupStorageLocationSetsSyncPeriod(t *testing.T) {
+	o := NewCreateOptions()
+	o.BackupSyncPeriod = 2 * time.Minute
+
+	bsl, err := o.BuildBackupStorageLocation("velero-test-ns", false, false)
+	assert.NoError(t, err)
+	assert.Nil(t, bsl.Spec.BackupSyncPeriod)
+
+	bsl, err = o.BuildBackupStorageLocation("velero-test-ns", true, false)
+	assert.NoError(t, err)
+	assert.Equal(t, &metav1.Duration{Duration: 2 * time.Minute}, bsl.Spec.BackupSyncPeriod)
+}
+
+func TestBuildBackupStorageLocationSetsValidationFrequency(t *testing.T) {
+	o := NewCreateOptions()
+	o.ValidationFrequency = 2 * time.Minute
+
+	bsl, err := o.BuildBackupStorageLocation("velero-test-ns", false, false)
+	assert.NoError(t, err)
+	assert.Nil(t, bsl.Spec.ValidationFrequency)
+
+	bsl, err = o.BuildBackupStorageLocation("velero-test-ns", false, true)
+	assert.NoError(t, err)
+	assert.Equal(t, &metav1.Duration{Duration: 2 * time.Minute}, bsl.Spec.ValidationFrequency)
+}
+
+func TestBuildBackupStorageLocationSetsCredential(t *testing.T) {
+	o := NewCreateOptions()
+
+	bsl, err := o.BuildBackupStorageLocation("velero-test-ns", false, false)
+	assert.NoError(t, err)
+	assert.Nil(t, bsl.Spec.Credential)
+
+	setErr := o.Credential.Set("my-secret=key-from-secret")
+	assert.NoError(t, setErr)
+
+	bsl, err = o.BuildBackupStorageLocation("velero-test-ns", false, true)
+	assert.NoError(t, err)
+	assert.Equal(t, &v1.SecretKeySelector{
+		LocalObjectReference: v1.LocalObjectReference{Name: "my-secret"},
+		Key:                  "key-from-secret",
+	}, bsl.Spec.Credential)
+}