From e4019f26c1ab215c4dbe0e2fa5eb8b3101d298bf Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Tue, 9 Nov 2021 20:04:58 -0500 Subject: [PATCH] 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 --- changelogs/unreleased/4322-zubron | 1 + pkg/cmd/cli/backuplocation/create.go | 61 ++++++++++-------- pkg/cmd/cli/backuplocation/create_test.go | 78 +++++++++++++++++++++++ 3 files changed, 112 insertions(+), 28 deletions(-) create mode 100644 changelogs/unreleased/4322-zubron create mode 100644 pkg/cmd/cli/backuplocation/create_test.go diff --git a/changelogs/unreleased/4322-zubron b/changelogs/unreleased/4322-zubron new file mode 100644 index 000000000..fed1addc8 --- /dev/null +++ b/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. diff --git a/pkg/cmd/cli/backuplocation/create.go b/pkg/cmd/cli/backuplocation/create.go index a541d7589..d8403b842 100644 --- a/pkg/cmd/cli/backuplocation/create.go +++ b/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 } diff --git a/pkg/cmd/cli/backuplocation/create_test.go b/pkg/cmd/cli/backuplocation/create_test.go new file mode 100644 index 000000000..57977e7ea --- /dev/null +++ b/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) +}