From 3ed9194ca4e8c9ffa3b28c9e1550d22e1d0fe893 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 2 May 2022 10:47:23 -0400 Subject: [PATCH 1/8] Initial commit of fix for OADP-460. --- controllers/validator.go | 10 ++++- controllers/velero.go | 67 +++++++++++++++++++--------------- pkg/credentials/credentials.go | 3 ++ 3 files changed, 48 insertions(+), 32 deletions(-) diff --git a/controllers/validator.go b/controllers/validator.go index 7630b68fca0..7a2c284b4a6 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -18,8 +18,14 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) return false, errors.New("DPA CR Velero configuration cannot be nil") } - if len(dpa.Spec.BackupLocations) == 0 && !dpa.Spec.Configuration.Velero.NoDefaultBackupLocation { - return false, errors.New("no backupstoragelocations configured, ensure a backupstoragelocation has been configured or use the noDefaultLocationBackupLocation flag") + if dpa.Spec.Configuration.Velero.NoDefaultBackupLocation { + if len(dpa.Spec.BackupLocations) != 0 { + return false, errors.New("DPA CR Velero configuration cannot have backup locations if NoDefaultBackupLocation is set") + } + } else { + if len(dpa.Spec.BackupLocations) == 0 { + return false, errors.New("no backupstoragelocations configured, ensure a backupstoragelocation has been configured or use the noDefaultLocationBackupLocation flag") + } } if dpa.Spec.Configuration.Velero.NoDefaultBackupLocation && dpa.BackupImages() { diff --git a/controllers/velero.go b/controllers/velero.go index d5d64dd8ea9..3269b43b50e 100644 --- a/controllers/velero.go +++ b/controllers/velero.go @@ -676,41 +676,48 @@ func (r *DPAReconciler) getResticResourceReqs(dpa *oadpv1alpha1.DataProtectionAp func (r DPAReconciler) noDefaultCredentials(dpa oadpv1alpha1.DataProtectionApplication) (map[string]bool, bool, error) { providerNeedsDefaultCreds := map[string]bool{} hasCloudStorage := false - - for _, bsl := range dpa.Spec.BackupLocations { - if bsl.Velero != nil && bsl.Velero.Credential == nil { - bslProvider := strings.TrimPrefix(bsl.Velero.Provider, "velero.io/") - providerNeedsDefaultCreds[bslProvider] = true - } - if bsl.Velero != nil && bsl.Velero.Credential != nil { - bslProvider := strings.TrimPrefix(bsl.Velero.Provider, "velero.io/") - if found := providerNeedsDefaultCreds[bslProvider]; !found { - providerNeedsDefaultCreds[bslProvider] = false + if dpa.Spec.Configuration.Velero.NoDefaultBackupLocation { + // go through cloudprovider plugins and mark providerNeedsDefaultCreds to false + for _, provider := range dpa.Spec.Configuration.Velero.DefaultPlugins { + if psf, ok := credentials.PluginSpecificFields[provider]; ok && psf.IsCloudProvider { + providerNeedsDefaultCreds[psf.PluginName] = false } } - if bsl.CloudStorage != nil { - if bsl.CloudStorage.Credential == nil { - cloudStorage := oadpv1alpha1.CloudStorage{} - err := r.Get(r.Context, types.NamespacedName{Name: bsl.CloudStorage.CloudStorageRef.Name, Namespace: dpa.Namespace}, &cloudStorage) - if err != nil { - return nil, false, err + } else { + for _, bsl := range dpa.Spec.BackupLocations { + if bsl.Velero != nil && bsl.Velero.Credential == nil { + bslProvider := strings.TrimPrefix(bsl.Velero.Provider, "velero.io/") + providerNeedsDefaultCreds[bslProvider] = true + } + if bsl.Velero != nil && bsl.Velero.Credential != nil { + bslProvider := strings.TrimPrefix(bsl.Velero.Provider, "velero.io/") + if found := providerNeedsDefaultCreds[bslProvider]; !found { + providerNeedsDefaultCreds[bslProvider] = false + } + } + if bsl.CloudStorage != nil { + if bsl.CloudStorage.Credential == nil { + cloudStorage := oadpv1alpha1.CloudStorage{} + err := r.Get(r.Context, types.NamespacedName{Name: bsl.CloudStorage.CloudStorageRef.Name, Namespace: dpa.Namespace}, &cloudStorage) + if err != nil { + return nil, false, err + } + providerNeedsDefaultCreds[string(cloudStorage.Spec.Provider)] = true + } else { + hasCloudStorage = true } - providerNeedsDefaultCreds[string(cloudStorage.Spec.Provider)] = true - } else { - hasCloudStorage = true } } - } - - for _, vsl := range dpa.Spec.SnapshotLocations { - if vsl.Velero != nil { - // To handle the case where we want to manually hand the credentials for a cloud storage created - // Bucket credentials via configuration. Only AWS is supported - provider := strings.TrimPrefix(vsl.Velero.Provider, "velero.io") - if provider == string(oadpv1alpha1.AWSBucketProvider) && hasCloudStorage { - providerNeedsDefaultCreds[provider] = false - } else { - providerNeedsDefaultCreds[provider] = true + for _, vsl := range dpa.Spec.SnapshotLocations { + if vsl.Velero != nil { + // To handle the case where we want to manually hand the credentials for a cloud storage created + // Bucket credentials via configuration. Only AWS is supported + provider := strings.TrimPrefix(vsl.Velero.Provider, "velero.io") + if provider == string(oadpv1alpha1.AWSBucketProvider) && hasCloudStorage { + providerNeedsDefaultCreds[provider] = false + } else { + providerNeedsDefaultCreds[provider] = true + } } } } diff --git a/pkg/credentials/credentials.go b/pkg/credentials/credentials.go index 0ef4664d958..9bae0b2e424 100644 --- a/pkg/credentials/credentials.go +++ b/pkg/credentials/credentials.go @@ -273,6 +273,9 @@ func AppendPluginSpecificSpecs(dpa *oadpv1alpha1.DataProtectionApplication, vele if !pluginSpecificMap.IsCloudProvider || !pluginNeedsCheck { continue } + if dpa.Spec.Configuration.Velero.NoDefaultBackupLocation && pluginSpecificMap.IsCloudProvider { + continue + } // set default secret name to use secretName := pluginSpecificMap.SecretName // append plugin specific volume mounts From 30ac88dd38307cc3c144405ad15cb84960e16d09 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 2 May 2022 10:50:10 -0400 Subject: [PATCH 2/8] error message change --- controllers/validator.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/controllers/validator.go b/controllers/validator.go index 7a2c284b4a6..b620933a68a 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -20,16 +20,16 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) if dpa.Spec.Configuration.Velero.NoDefaultBackupLocation { if len(dpa.Spec.BackupLocations) != 0 { - return false, errors.New("DPA CR Velero configuration cannot have backup locations if NoDefaultBackupLocation is set") + return false, errors.New("DPA CR Velero configuration cannot have backup locations if noDefaultBackupLocation is set") } } else { if len(dpa.Spec.BackupLocations) == 0 { - return false, errors.New("no backupstoragelocations configured, ensure a backupstoragelocation has been configured or use the noDefaultLocationBackupLocation flag") + return false, errors.New("no backupstoragelocations configured, ensure a backupstoragelocation has been configured or use the noDefaultBackupLocation flag") } } if dpa.Spec.Configuration.Velero.NoDefaultBackupLocation && dpa.BackupImages() { - return false, errors.New("backupImages needs to be set to false when noDefaultLocationBackupLocation is set") + return false, errors.New("backupImages needs to be set to false when noDefaultBackupLocation is set") } if len(dpa.Spec.BackupLocations) > 0 { From 2b26e487bae7008e9614198a08dcf0e2f06fa4a7 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 3 May 2022 10:37:45 -0400 Subject: [PATCH 3/8] if vsl specified we still need default creds --- controllers/velero.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/controllers/velero.go b/controllers/velero.go index 3269b43b50e..744fa9fa270 100644 --- a/controllers/velero.go +++ b/controllers/velero.go @@ -708,16 +708,16 @@ func (r DPAReconciler) noDefaultCredentials(dpa oadpv1alpha1.DataProtectionAppli } } } - for _, vsl := range dpa.Spec.SnapshotLocations { - if vsl.Velero != nil { - // To handle the case where we want to manually hand the credentials for a cloud storage created - // Bucket credentials via configuration. Only AWS is supported - provider := strings.TrimPrefix(vsl.Velero.Provider, "velero.io") - if provider == string(oadpv1alpha1.AWSBucketProvider) && hasCloudStorage { - providerNeedsDefaultCreds[provider] = false - } else { - providerNeedsDefaultCreds[provider] = true - } + } + for _, vsl := range dpa.Spec.SnapshotLocations { + if vsl.Velero != nil { + // To handle the case where we want to manually hand the credentials for a cloud storage created + // Bucket credentials via configuration. Only AWS is supported + provider := strings.TrimPrefix(vsl.Velero.Provider, "velero.io") + if provider == string(oadpv1alpha1.AWSBucketProvider) && hasCloudStorage { + providerNeedsDefaultCreds[provider] = false + } else { + providerNeedsDefaultCreds[provider] = true } } } From d14b22eee26a9e69627f7bdc40d812141fa177f7 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 3 May 2022 10:40:42 -0400 Subject: [PATCH 4/8] add usage doc --- docs/credentials.md | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/docs/credentials.md b/docs/credentials.md index 5e1979b1f8a..d89f71a547d 100644 --- a/docs/credentials.md +++ b/docs/credentials.md @@ -171,7 +171,25 @@ spec:
-3. #### No `BackupStorageLocation` or `VolumeSnapshotLocation` specified, but the plugin for the provider exists: +1. #### No `BackupStorageLocation` specified, but the plugin for the provider exists: +Specify .spec.configuration.noDefaultBackupLocation like so: +```yaml +apiVersion: oadp.openshift.io/v1alpha1 +kind: DataProtectionApplication +metadata: + name: velero-sample +spec: + configuration: + velero: + defaultPlugins: + - openshift + - aws + restic: + enable: true + noDefaultBackupLocation: true +``` +If you don't need volumesnapshotlocation, you will not need to create a VSL credentials. + +If you need `VolumeSnapshotLocation`, regardless of the `noDefaultBackupLocation` setting, you will need a to create VSL credentials. - - The default secret name still needs to exist in order for the Velero installation - to be successful, but the secret can be empty. \ No newline at end of file +VSL credentials **must** be the [default secret name](#defaultsecrets) From c70f85de53c0f4a817222d7ee2865fef73dd77b6 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 3 May 2022 10:51:49 -0400 Subject: [PATCH 5/8] spec indent update --- docs/credentials.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/credentials.md b/docs/credentials.md index d89f71a547d..853b519da44 100644 --- a/docs/credentials.md +++ b/docs/credentials.md @@ -184,9 +184,9 @@ spec: defaultPlugins: - openshift - aws + noDefaultBackupLocation: true restic: enable: true - noDefaultBackupLocation: true ``` If you don't need volumesnapshotlocation, you will not need to create a VSL credentials. From 7b49d25a9b7a370d85736f87c1c474d749c9d577 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 3 May 2022 23:58:48 -0400 Subject: [PATCH 6/8] restic only mount secretVolume if NoDefaultBackupLocation is found. --- pkg/credentials/credentials.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/credentials/credentials.go b/pkg/credentials/credentials.go index 9bae0b2e424..c745b4cdc26 100644 --- a/pkg/credentials/credentials.go +++ b/pkg/credentials/credentials.go @@ -175,14 +175,12 @@ func AppendCloudProviderVolumes(dpa *oadpv1alpha1.DataProtectionApplication, ds // ok is boolean that will be true if `plugin` is a valid key in `PluginSpecificFields` map // pattern from https://golang.org/doc/effective_go#maps // this replaces the need to iterate through the `pluginSpecificFields` O(n) -> O(1) - if cloudProviderMap, ok := PluginSpecificFields[plugin]; ok { - if !cloudProviderMap.IsCloudProvider { - continue - } + if cloudProviderMap, ok := PluginSpecificFields[plugin]; ok && + cloudProviderMap.IsCloudProvider && + !dpa.Spec.Configuration.Velero.NoDefaultBackupLocation { - pluginNeedsCheck, foundInBSLorVSL := providerNeedsDefaultCreds[string(plugin)] - - if !foundInBSLorVSL && !hasCloudStorage { + pluginNeedsCheck, foundProviderPlugin := providerNeedsDefaultCreds[string(plugin)] + if !foundProviderPlugin && !hasCloudStorage { pluginNeedsCheck = true } From e17575733b9a0828357fdbc6126c8855444c144f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 3 May 2022 23:58:59 -0400 Subject: [PATCH 7/8] go fmt --- controllers/velero.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/velero.go b/controllers/velero.go index 744fa9fa270..4ca68c26ba3 100644 --- a/controllers/velero.go +++ b/controllers/velero.go @@ -683,7 +683,7 @@ func (r DPAReconciler) noDefaultCredentials(dpa oadpv1alpha1.DataProtectionAppli providerNeedsDefaultCreds[psf.PluginName] = false } } - } else { + } else { for _, bsl := range dpa.Spec.BackupLocations { if bsl.Velero != nil && bsl.Velero.Credential == nil { bslProvider := strings.TrimPrefix(bsl.Velero.Provider, "velero.io/") From 91fbcd689bc56208f6277878bfab481e03f90138 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Wed, 11 May 2022 10:34:18 -0400 Subject: [PATCH 8/8] added TestDPAReconciler_noDefaultCredentials --- controllers/velero_test.go | 93 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/controllers/velero_test.go b/controllers/velero_test.go index cccd829569c..adf8e5d38ef 100644 --- a/controllers/velero_test.go +++ b/controllers/velero_test.go @@ -1992,3 +1992,96 @@ func Test_validateVeleroPlugins(t *testing.T) { }) } } + +var allDefaultPluginsList = []oadpv1alpha1.DefaultPlugin{ + oadpv1alpha1.DefaultPluginAWS, + oadpv1alpha1.DefaultPluginGCP, + oadpv1alpha1.DefaultPluginMicrosoftAzure, + oadpv1alpha1.DefaultPluginKubeVirt, + oadpv1alpha1.DefaultPluginOpenShift, + oadpv1alpha1.DefaultPluginCSI, +} + +func TestDPAReconciler_noDefaultCredentials(t *testing.T) { + type args struct { + dpa oadpv1alpha1.DataProtectionApplication + } + tests := []struct { + name string + args args + want map[string]bool + wantHasCloudStorage bool + wantErr bool + }{ + { + name: "dpa with all plugins but with noDefualtBackupLocation should not require default credentials", + args: args{ + dpa: oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-Velero-CR", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: allDefaultPluginsList, + NoDefaultBackupLocation: true, + }, + }, + }, + }, + }, + want: map[string]bool{ + "velero-plugin-for-aws": false, + "velero-plugin-for-gcp": false, + "velero-plugin-for-microsoft-azure": false, + }, + wantHasCloudStorage: false, + wantErr: false, + }, + { + name: "dpa no default cloudprovider plugins should not require default credentials", + args: args{ + dpa: oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-Velero-CR", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{oadpv1alpha1.DefaultPluginOpenShift}, + NoDefaultBackupLocation: true, + }, + }, + }, + }, + }, + want: map[string]bool{}, + wantHasCloudStorage: false, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeClient, err := getFakeClientFromObjects(&tt.args.dpa) + if err != nil { + t.Errorf("error in creating fake client, likely programmer error") + } + r := DPAReconciler{ + Client: fakeClient, + } + got, got1, err := r.noDefaultCredentials(tt.args.dpa) + if (err != nil) != tt.wantErr { + t.Errorf("DPAReconciler.noDefaultCredentials() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("DPAReconciler.noDefaultCredentials() got = \n%v, \nwant \n%v", got, tt.want) + } + if got1 != tt.wantHasCloudStorage { + t.Errorf("DPAReconciler.noDefaultCredentials() got1 = %v, want %v", got1, tt.wantHasCloudStorage) + } + }) + } +}