From ad7151fd183d17789968456ef4072eb48400a17b Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 22 Aug 2023 13:49:23 -0400 Subject: [PATCH] OADP-1057: CloudStorageLocation Prefix/CACert support (#1126) * validator and type updates * validator and type updates Signed-off-by: Tiger Kaovilai * CloudStorageLocation prefix is added to BSL * clarify backupImages error Signed-off-by: Tiger Kaovilai * validator refactor returns err from validating funcs Signed-off-by: Tiger Kaovilai * unit tests fix * simplify expression Signed-off-by: Tiger Kaovilai * Add ignorable restore error logs and unit test Signed-off-by: Tiger Kaovilai * consistent error string across bl.velero and bl.cloudstorage --------- Signed-off-by: Tiger Kaovilai --- api/v1alpha1/cloud_storage_types.go | 8 +- api/v1alpha1/oadp_types.go | 11 + api/v1alpha1/zz_generated.deepcopy.go | 5 + ...enshift.io_dataprotectionapplications.yaml | 8 + ...enshift.io_dataprotectionapplications.yaml | 8 + controllers/bsl.go | 13 +- controllers/bsl_test.go | 316 +++++++++++++++++- controllers/dpa_controller.go | 4 +- controllers/validator.go | 34 +- controllers/validator_test.go | 201 ++++++++++- controllers/vsl.go | 6 +- controllers/vsl_test.go | 2 +- tests/e2e/lib/velero_helpers.go | 32 +- tests/e2e/lib/velero_helpers_test.go | 49 +++ 14 files changed, 637 insertions(+), 60 deletions(-) create mode 100644 tests/e2e/lib/velero_helpers_test.go diff --git a/api/v1alpha1/cloud_storage_types.go b/api/v1alpha1/cloud_storage_types.go index 83c0f5a4ca..a0d2d6d07c 100644 --- a/api/v1alpha1/cloud_storage_types.go +++ b/api/v1alpha1/cloud_storage_types.go @@ -5,6 +5,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// CloudStorage types are APIs for automatic bucket creation at cloud providers if defined name do not exists. + //+kubebuilder:object:root=true //+kubebuilder:subresource:status @@ -19,9 +21,9 @@ type CloudStorage struct { type CloudStorageProvider string const ( - AWSBucketProvider CloudStorageProvider = "aws" - AzureBucketProvider CloudStorageProvider = "azure" - GCPBucketProvider CloudStorageProvider = "gcp" + AWSBucketProvider CloudStorageProvider = CloudStorageProvider(DefaultPluginAWS) + AzureBucketProvider CloudStorageProvider = CloudStorageProvider(DefaultPluginMicrosoftAzure) + GCPBucketProvider CloudStorageProvider = CloudStorageProvider(DefaultPluginGCP) ) type CloudStorageSpec struct { diff --git a/api/v1alpha1/oadp_types.go b/api/v1alpha1/oadp_types.go index 810867de05..0c320bf923 100644 --- a/api/v1alpha1/oadp_types.go +++ b/api/v1alpha1/oadp_types.go @@ -152,6 +152,7 @@ type ApplicationConfig struct { Restic *ResticConfig `json:"restic,omitempty"` } +// CloudStorageLocation defines BackupStorageLocation using bucket referenced by CloudStorage CR. type CloudStorageLocation struct { CloudStorageRef corev1.LocalObjectReference `json:"cloudStorageRef"` @@ -171,6 +172,16 @@ type CloudStorageLocation struct { // +optional // +nullable BackupSyncPeriod *metav1.Duration `json:"backupSyncPeriod,omitempty"` + + // Prefix and CACert are copied from velero/pkg/apis/v1/backupstoragelocation_types.go under ObjectStorageLocation + + // Prefix is the path inside a bucket to use for Velero storage. Optional. + // +optional + Prefix string `json:"prefix,omitempty"` + + // CACert defines a CA bundle to use when verifying TLS connections to the provider. + // +optional + CACert []byte `json:"caCert,omitempty"` } // BackupLocation defines the configuration for the DPA backup storage diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 7b9a212076..fa4c3632c7 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -159,6 +159,11 @@ func (in *CloudStorageLocation) DeepCopyInto(out *CloudStorageLocation) { *out = new(metav1.Duration) **out = **in } + if in.CACert != nil { + in, out := &in.CACert, &out.CACert + *out = make([]byte, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CloudStorageLocation. diff --git a/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml b/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml index f78d13d1ca..8a2552f2a3 100644 --- a/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml +++ b/bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml @@ -41,11 +41,16 @@ spec: description: BackupLocation defines the configuration for the DPA backup storage properties: bucket: + description: CloudStorageLocation defines BackupStorageLocation using bucket referenced by CloudStorage CR. properties: backupSyncPeriod: description: backupSyncPeriod defines how frequently to sync backup API objects from object storage. A value of 0 disables sync. nullable: true type: string + caCert: + description: CACert defines a CA bundle to use when verifying TLS connections to the provider. + format: byte + type: string cloudStorageRef: description: LocalObjectReference contains enough information to let you locate the referenced object inside the same namespace. properties: @@ -76,6 +81,9 @@ spec: default: description: default indicates this location is the default backup storage location. type: boolean + prefix: + description: Prefix is the path inside a bucket to use for Velero storage. Optional. + type: string required: - cloudStorageRef type: object diff --git a/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml b/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml index f78d13d1ca..8a2552f2a3 100644 --- a/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml +++ b/config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml @@ -41,11 +41,16 @@ spec: description: BackupLocation defines the configuration for the DPA backup storage properties: bucket: + description: CloudStorageLocation defines BackupStorageLocation using bucket referenced by CloudStorage CR. properties: backupSyncPeriod: description: backupSyncPeriod defines how frequently to sync backup API objects from object storage. A value of 0 disables sync. nullable: true type: string + caCert: + description: CACert defines a CA bundle to use when verifying TLS connections to the provider. + format: byte + type: string cloudStorageRef: description: LocalObjectReference contains enough information to let you locate the referenced object inside the same namespace. properties: @@ -76,6 +81,9 @@ spec: default: description: default indicates this location is the default backup storage location. type: boolean + prefix: + description: Prefix is the path inside a bucket to use for Velero storage. Optional. + type: string required: - cloudStorageRef type: object diff --git a/controllers/bsl.go b/controllers/bsl.go index 84be85b74b..06fdab629b 100644 --- a/controllers/bsl.go +++ b/controllers/bsl.go @@ -14,11 +14,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -func (r *DPAReconciler) ValidateBackupStorageLocations(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } +func (r *DPAReconciler) ValidateBackupStorageLocations(dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { if dpa.Spec.Configuration == nil || dpa.Spec.Configuration.Velero == nil { return false, errors.New("DPA CR Velero configuration cannot be nil") } @@ -146,6 +142,8 @@ func (r *DPAReconciler) ReconcileBackupStorageLocations(log logr.Logger) (bool, bsl.Spec.Default = bslSpec.CloudStorage.Default bsl.Spec.ObjectStorage = &velerov1.ObjectStorageLocation{ Bucket: bucket.Spec.Name, + Prefix: bslSpec.CloudStorage.Prefix, + CACert: bslSpec.CloudStorage.CACert, } switch bucket.Spec.Provider { case oadpv1alpha1.AWSBucketProvider: @@ -256,7 +254,7 @@ func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.Backup if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() { return fmt.Errorf("prefix for AWS backupstoragelocation object storage cannot be empty. It is required for backing up images") } - // BSL region is required when s3ForcePathStyle is true AND BackupImages is false + // BSL region is required when s3ForcePathStyle is true AND BackupImages is true if (bslSpec.Config == nil || len(bslSpec.Config[Region]) == 0 && bslSpec.Config[S3ForcePathStyle] == "true") && dpa.BackupImages() { return fmt.Errorf("region for AWS backupstoragelocation cannot be empty when s3ForcePathStyle is true or when backing up images") } @@ -329,6 +327,9 @@ func pluginExistsInVeleroCR(configuredPlugins []oadpv1alpha1.DefaultPlugin, expe } func (r *DPAReconciler) validateProviderPluginAndSecret(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error { + if dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret") { + return nil + } // check for existence of provider plugin and warn if the plugin is absent if !pluginExistsInVeleroCR(dpa.Spec.Configuration.Velero.DefaultPlugins, oadpv1alpha1.DefaultPlugin(bslSpec.Provider)) { r.Log.Info(fmt.Sprintf("%s backupstoragelocation is configured but velero plugin for %s is not present", bslSpec.Provider, bslSpec.Provider)) diff --git a/controllers/bsl_test.go b/controllers/bsl_test.go index a91567f973..7c4a2df14c 100644 --- a/controllers/bsl_test.go +++ b/controllers/bsl_test.go @@ -2,6 +2,7 @@ package controllers import ( "context" + "fmt" "reflect" "testing" @@ -9,6 +10,7 @@ import ( v1 "k8s.io/api/core/v1" "github.com/go-logr/logr" + "github.com/google/go-cmp/cmp" oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -1218,7 +1220,7 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) { }, EventRecorder: record.NewFakeRecorder(10), } - got, err := r.ValidateBackupStorageLocations(r.Log) + got, err := r.ValidateBackupStorageLocations(*tt.dpa) if (err != nil) != tt.wantErr { t.Errorf("ValidateBackupStorageLocations() error = %v, wantErr %v", err, tt.wantErr) return @@ -1450,7 +1452,7 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { }, } - tests := []struct { + ownerReferenceTests := []struct { name string dpa *oadpv1alpha1.DataProtectionApplication secret *corev1.Secret @@ -1511,7 +1513,7 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { wantErr: false, }, } - for _, tt := range tests { + for _, tt := range ownerReferenceTests { t.Run(tt.name, func(t *testing.T) { fakeClient, err := getFakeClientFromObjects(tt.dpa, tt.secret, tt.cs) if err != nil { @@ -1537,8 +1539,8 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { Kind: "DataProtectionApplication", Name: tt.dpa.Name, UID: tt.dpa.UID, - Controller: pointer.BoolPtr(true), - BlockOwnerDeletion: pointer.BoolPtr(true), + Controller: pointer.Bool(true), + BlockOwnerDeletion: pointer.Bool(true), }}, }, } @@ -1565,4 +1567,308 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { } }) } + bslPrefixCATests := []struct { + name string + objects []client.Object + want bool + wantErr bool + wantBSL velerov1.BackupStorageLocation + }{ + { + name: "dpa.spec.backupLocation.Velero has Prefix set", + objects: []client.Object{ + &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dpa", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + BackupLocations: []oadpv1alpha1.BackupLocation{ + { + Velero: &velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Prefix: "test-prefix", + }, + }, + }, + }, + }, + }, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cloud-credentials", + Namespace: "test-ns", + }, + }, + }, + want: true, + wantErr: false, + wantBSL: velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dpa-1", + Namespace: "test-ns", + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Prefix: "test-prefix", + }, + }, + }, + }, + }, + { + name: "dpa.spec.backupLocation.CloudStorage has Prefix set", + objects: []client.Object{ + &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dpa", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + BackupLocations: []oadpv1alpha1.BackupLocation{ + { + CloudStorage: &oadpv1alpha1.CloudStorageLocation{ + CloudStorageRef: v1.LocalObjectReference{ + Name: "test-cs", + }, + Credential: &v1.SecretKeySelector{ + LocalObjectReference: v1.LocalObjectReference{ + Name: "cloud-credentials", + }, + Key: "credentials", + }, + Prefix: "test-prefix", + }, + }, + }, + }, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cloud-credentials", + Namespace: "test-ns", + }, + }, + &oadpv1alpha1.CloudStorage{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cs", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.CloudStorageSpec{ + Provider: "aws", + CreationSecret: v1.SecretKeySelector{ + LocalObjectReference: v1.LocalObjectReference{ + Name: "cloud-credentials", + }, + Key: "credentials", + }, + }, + }, + }, + want: true, + wantErr: false, + wantBSL: velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dpa-1", + Namespace: "test-ns", + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Prefix: "test-prefix", + }, + }, + Credential: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cloud-credentials", + }, + Key: "credentials", + }, + }, + }, + }, + { + name: "dpa.spec.backupLocation.Velero has Prefix set and CA set", + objects: []client.Object{ + &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dpa", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + BackupLocations: []oadpv1alpha1.BackupLocation{ + { + Velero: &velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + CACert: []byte("test-ca"), + }, + }, + Credential: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cloud-credentials", + }, + Key: "credentials", + }, + }, + }, + }, + }, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cloud-credentials", + Namespace: "test-ns", + }, + }, + }, + want: true, + wantErr: false, + wantBSL: velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dpa-1", + Namespace: "test-ns", + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Prefix: "test-prefix", + Bucket: "test-bucket", + CACert: []byte("test-ca"), + }, + }, + Credential: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cloud-credentials", + }, + Key: "credentials", + }, + }, + }, + }, + { + name: "dpa.spec.backupLocation.CloudStorage has Prefix set and CA set", + objects: []client.Object{ + &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dpa", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + BackupLocations: []oadpv1alpha1.BackupLocation{ + { + CloudStorage: &oadpv1alpha1.CloudStorageLocation{ + CloudStorageRef: v1.LocalObjectReference{ + Name: "test-cs", + }, + Credential: &v1.SecretKeySelector{ + LocalObjectReference: v1.LocalObjectReference{ + Name: "cloud-credentials", + }, + Key: "credentials", + }, + Prefix: "test-prefix", + CACert: []byte("test-ca"), + }, + }, + }, + }, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cloud-credentials", + Namespace: "test-ns", + }, + }, + &oadpv1alpha1.CloudStorage{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cs", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.CloudStorageSpec{ + Provider: "aws", + CreationSecret: v1.SecretKeySelector{ + LocalObjectReference: v1.LocalObjectReference{ + Name: "cloud-credentials", + }, + Key: "credentials", + }, + Region: "test-region", + Name: "test-bucket", + }, + }, + }, + want: true, + wantErr: false, + wantBSL: velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dpa-1", + Namespace: "test-ns", + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + CACert: []byte("test-ca"), + }, + }, + Credential: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cloud-credentials", + }, + Key: "credentials", + }, + }, + }, + }, + } + for _, tt := range bslPrefixCATests { + t.Run(tt.name, func(t *testing.T) { + fakeClient, err := getFakeClientFromObjects(tt.objects...) + if err != nil { + t.Errorf("error in creating fake client, likely programmer error") + } + r := &DPAReconciler{ + Client: fakeClient, + Scheme: fakeClient.Scheme(), + Log: logr.Discard(), + Context: newContextForTest(tt.name), + NamespacedName: types.NamespacedName{ + Namespace: tt.objects[0].GetNamespace(), + Name: tt.objects[0].GetName(), + }, + EventRecorder: record.NewFakeRecorder(10), + } + got, err := r.ReconcileBackupStorageLocations(r.Log) + if (err != nil) != tt.wantErr { + t.Errorf("ReconcileBackupStorageLocations() error =%v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("ReconcileBackupStorageLocations() got = %v, want %v", got, tt.want) + } + bsl := &velerov1.BackupStorageLocation{} + err = r.Get(r.Context, client.ObjectKey{Namespace: tt.objects[0].GetNamespace(), Name: "test-dpa-1"}, bsl) + if (err != nil) != tt.wantErr { + t.Errorf("ReconcileBackupStorageLocations() error =%v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(bsl.Spec, tt.wantBSL.Spec) { + fmt.Println(cmp.Diff(bsl.Spec, tt.wantBSL.Spec)) + t.Errorf("ReconcileBackupStorageLocations() expected BSL spec to be %#v, got %#v", tt.wantBSL.Spec, bsl.Spec) + } + }) + } } diff --git a/controllers/dpa_controller.go b/controllers/dpa_controller.go index a214c6add1..e315eaf904 100644 --- a/controllers/dpa_controller.go +++ b/controllers/dpa_controller.go @@ -90,14 +90,12 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R _, err := ReconcileBatch(r.Log, r.ValidateDataProtectionCR, r.ReconcileResticRestoreHelperConfig, - r.ValidateBackupStorageLocations, r.ReconcileBackupStorageLocations, r.ReconcileRegistrySecrets, r.ReconcileRegistries, r.ReconcileRegistrySVCs, r.ReconcileRegistryRoutes, r.ReconcileRegistryRouteConfigs, - r.ValidateVolumeSnapshotLocations, r.LabelVSLSecrets, r.ReconcileVolumeSnapshotLocations, r.ReconcileVeleroDeployment, @@ -214,6 +212,8 @@ type ReconcileFunc func(logr.Logger) (bool, error) // reconcileBatch steps through a list of reconcile functions until one returns // false or an error. func ReconcileBatch(l logr.Logger, reconcileFuncs ...ReconcileFunc) (bool, error) { + // TODO: #1127 DPAReconciler already have a logger, use it instead of passing to each reconcile functions + // TODO: #1128 Right now each reconcile functions call get for DPA, we can do it once and pass it to each function for _, f := range reconcileFuncs { if cont, err := f(l); !cont || err != nil { return cont, err diff --git a/controllers/validator.go b/controllers/validator.go index 186412aff6..dcca3fe0c2 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -14,6 +14,9 @@ import ( "github.com/openshift/oadp-operator/pkg/credentials" ) +// ValidateDataProtectionCR function validates the DPA CR, returns true if valid, false otherwise +// it calls other validation functions to validate the DPA CR +// TODO: #1129 Clean up duplicate logic for validating backupstoragelocations and volumesnapshotlocations in dpa func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) { dpa := oadpv1alpha1.DataProtectionApplication{} if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { @@ -37,20 +40,22 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) return false, errors.New("backupImages needs to be set to false when noDefaultBackupLocation is set") } - if len(dpa.Spec.BackupLocations) > 0 { - for _, location := range dpa.Spec.BackupLocations { - // check for velero BSL config or cloud storage config - if location.Velero == nil && location.CloudStorage == nil { - return false, errors.New("BackupLocation must have velero or bucket configuration") - } + for _, location := range dpa.Spec.BackupLocations { + // check for velero BSL config or cloud storage config + if location.Velero == nil && location.CloudStorage == nil { + return false, errors.New("BackupLocation must have velero or bucket configuration") + } + if location.Velero != nil && location.Velero.ObjectStorage != nil && location.Velero.ObjectStorage.Prefix == "" && dpa.BackupImages() { + return false, errors.New("BackupLocation must have velero prefix when backupImages is not set to false") + } + if location.CloudStorage != nil && location.CloudStorage.Prefix == "" && dpa.BackupImages() { + return false, errors.New("BackupLocation must have cloud storage prefix when backupImages is not set to false") } } - if len(dpa.Spec.SnapshotLocations) > 0 { - for _, location := range dpa.Spec.SnapshotLocations { - if location.Velero == nil { - return false, errors.New("snapshotLocation velero configuration cannot be nil") - } + for _, location := range dpa.Spec.SnapshotLocations { + if location.Velero == nil { + return false, errors.New("snapshotLocation velero configuration cannot be nil") } } @@ -95,7 +100,12 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) if _, err := getResticResourceReqs(&dpa); err != nil { return false, err } - + if validBsl, err := r.ValidateBackupStorageLocations(dpa); !validBsl || err != nil { + return validBsl, err + } + if validVsl, err := r.ValidateVolumeSnapshotLocations(dpa); !validVsl || err != nil { + return validVsl, err + } return true, nil } diff --git a/controllers/validator_test.go b/controllers/validator_test.go index b3431930f7..7a1a976a99 100644 --- a/controllers/validator_test.go +++ b/controllers/validator_test.go @@ -3,12 +3,11 @@ package controllers import ( "testing" - "k8s.io/apimachinery/pkg/api/resource" - "github.com/go-logr/logr" oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" v1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -226,6 +225,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -276,6 +276,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -335,6 +336,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -375,6 +377,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { { Velero: &v1.VolumeSnapshotLocationSpec{ Provider: "aws", + Config: map[string]string{ + AWSRegion: "us-east-1", + }, }, }, }, @@ -385,6 +390,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{}, @@ -418,6 +424,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { { Velero: &v1.VolumeSnapshotLocationSpec{ Provider: "aws", + Config: map[string]string{ + AWSRegion: "us-east-1", + }, }, }, }, @@ -429,6 +438,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{}, @@ -456,6 +466,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { { Velero: &v1.VolumeSnapshotLocationSpec{ Provider: "aws", + Config: map[string]string{ + AWSRegion: "us-east-1", + }, }, }, }, @@ -466,6 +479,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -496,6 +510,11 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { CloudStorageRef: corev1.LocalObjectReference{ Name: "testing", }, + Credential: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cloud-credentials", + }, + }, }, }, }, @@ -503,6 +522,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { { Velero: &v1.VolumeSnapshotLocationSpec{ Provider: "aws", + Config: map[string]string{ + AWSRegion: "us-east-1", + }, }, }, }, @@ -513,6 +535,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -546,7 +569,19 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupLocations: []oadpv1alpha1.BackupLocation{ { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/gcp", + Credential: &corev1.SecretKeySelector{ + Key: "credentials", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cloud-credentials-gcp", + }, + }, }, }, }, @@ -581,6 +616,12 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupLocations: []oadpv1alpha1.BackupLocation{ { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/gcp", }, }, @@ -611,6 +652,12 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupLocations: []oadpv1alpha1.BackupLocation{ { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/gcp", }, }, @@ -667,6 +714,12 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupLocations: []oadpv1alpha1.BackupLocation{ { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/aws", Credential: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ @@ -675,6 +728,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "Creds", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -687,7 +743,14 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, - objects: []client.Object{}, + objects: []client.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Test", + Namespace: "test-ns", + }, + }, + }, wantErr: false, want: true, }, @@ -702,6 +765,12 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupLocations: []oadpv1alpha1.BackupLocation{ { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/aws", Credential: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ @@ -710,10 +779,19 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "Creds", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/aws", }, }, @@ -727,7 +805,14 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, - objects: []client.Object{}, + objects: []client.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Test", + Namespace: "test-ns", + }, + }, + }, wantErr: true, want: false, }, @@ -742,6 +827,12 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupLocations: []oadpv1alpha1.BackupLocation{ { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/aws", Credential: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ @@ -750,6 +841,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "cloud", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -791,6 +885,12 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupLocations: []oadpv1alpha1.BackupLocation{ { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/aws", Credential: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ @@ -799,6 +899,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "cloud", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -861,6 +964,12 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupLocations: []oadpv1alpha1.BackupLocation{ { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/aws", Credential: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ @@ -869,6 +978,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "cloud", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -917,6 +1029,12 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupLocations: []oadpv1alpha1.BackupLocation{ { Velero: &v1.BackupStorageLocationSpec{ + StorageType: v1.StorageType{ + ObjectStorage: &v1.ObjectStorageLocation{ + Bucket: "test-bucket", + Prefix: "test-prefix", + }, + }, Provider: "velero.io/aws", Credential: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ @@ -925,6 +1043,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "cloud", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -968,6 +1089,78 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { wantErr: false, want: true, }, + { + name: "If DPA CR has CloudStorageLocation without Prefix defined with backupImages enabled, error case", + dpa: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-DPA-CR", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + BackupLocations: []oadpv1alpha1.BackupLocation{ + { + CloudStorage: &oadpv1alpha1.CloudStorageLocation{ + CloudStorageRef: corev1.LocalObjectReference{ + Name: "testing", + }, + Prefix: "", + }, + }, + }, + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{}, + }, + }, + BackupImages: pointer.Bool(true), + }, + }, + objects: []client.Object{}, + wantErr: true, + want: false, + }, + { + name: "If DPA CR has CloudStorageLocation with Prefix defined with backupImages enabled, no error case", + dpa: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-DPA-CR", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + BackupLocations: []oadpv1alpha1.BackupLocation{ + { + CloudStorage: &oadpv1alpha1.CloudStorageLocation{ + CloudStorageRef: corev1.LocalObjectReference{ + Name: "testing", + }, + Prefix: "some-prefix", + Credential: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cloud-credentials", + }, + }, + }, + }, + }, + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{}, + }, + }, + BackupImages: pointer.Bool(true), + }, + }, + objects: []client.Object{ + &oadpv1alpha1.CloudStorage{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testing", + Namespace: "test-ns", + }, + }, + }, + wantErr: false, + want: true, + }, } for _, tt := range tests { tt.objects = append(tt.objects, tt.dpa) diff --git a/controllers/vsl.go b/controllers/vsl.go index 11d4b97daf..5d49ec32aa 100644 --- a/controllers/vsl.go +++ b/controllers/vsl.go @@ -104,11 +104,7 @@ func (r *DPAReconciler) LabelVSLSecrets(log logr.Logger) (bool, error) { return true, nil } -func (r *DPAReconciler) ValidateVolumeSnapshotLocations(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } +func (r *DPAReconciler) ValidateVolumeSnapshotLocations(dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { if dpa.Spec.Configuration == nil { return false, errors.New("application configuration not found") } diff --git a/controllers/vsl_test.go b/controllers/vsl_test.go index 8ac1e86992..af51d77828 100644 --- a/controllers/vsl_test.go +++ b/controllers/vsl_test.go @@ -510,7 +510,7 @@ func TestDPAReconciler_ValidateVolumeSnapshotLocation(t *testing.T) { }, EventRecorder: record.NewFakeRecorder(10), } - got, err := r.ValidateVolumeSnapshotLocations(r.Log) + got, err := r.ValidateVolumeSnapshotLocations(*tt.dpa) if (err != nil) != tt.wantErr { t.Errorf("ValidateVolumeSnapshotLocations() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/tests/e2e/lib/velero_helpers.go b/tests/e2e/lib/velero_helpers.go index 7062d70c27..1c911a99bb 100644 --- a/tests/e2e/lib/velero_helpers.go +++ b/tests/e2e/lib/velero_helpers.go @@ -164,6 +164,10 @@ var errorIgnorePatterns = []string{ "blob unknown", "num errors=0", "level=debug", // debug logs may contain the text error about recoverable errors so ignore them + + // Ignore managed fields errors per https://github.com/vmware-tanzu/velero/pull/6110 and avoid e2e failure. + // https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_oadp-operator/1126/pull-ci-openshift-oadp-operator-master-4.10-operator-e2e-aws/1690109468546699264#1:build-log.txt%3A686 + "level=error msg=\"error patch for managed fields ", } func recoverFromPanicLogs(veleroNamespace string, panicReason interface{}, panicFrom string) string { @@ -178,37 +182,21 @@ func recoverFromPanicLogs(veleroNamespace string, panicReason interface{}, panic func BackupErrorLogs(ocClient client.Client, backup velero.Backup) []string { bl := BackupLogs(ocClient, backup) - errorRegex, err := regexp.Compile("error|Error") - if err != nil { - return []string{"could not compile regex: ", err.Error()} - } - logLines := []string{} - for _, line := range strings.Split(bl, "\n") { - if errorRegex.MatchString(line) { - // ignore some expected errors - ignoreLine := false - for _, ignore := range errorIgnorePatterns { - ignoreLine, _ = regexp.MatchString(ignore, line) - if ignoreLine { - break - } - } - if !ignoreLine { - logLines = append(logLines, line) - } - } - } - return logLines + return errorLogsExcludingIgnored(bl) } func RestoreErrorLogs(ocClient client.Client, restore velero.Restore) []string { rl := RestoreLogs(ocClient, restore) + return errorLogsExcludingIgnored(rl) +} + +func errorLogsExcludingIgnored(logs string) []string { errorRegex, err := regexp.Compile("error|Error") if err != nil { return []string{"could not compile regex: ", err.Error()} } logLines := []string{} - for _, line := range strings.Split(rl, "\n") { + for _, line := range strings.Split(logs, "\n") { if errorRegex.MatchString(line) { // ignore some expected errors ignoreLine := false diff --git a/tests/e2e/lib/velero_helpers_test.go b/tests/e2e/lib/velero_helpers_test.go new file mode 100644 index 0000000000..d1d18359ed --- /dev/null +++ b/tests/e2e/lib/velero_helpers_test.go @@ -0,0 +1,49 @@ +package lib + +import ( + "reflect" + "testing" +) + +func Test_errorLogsExcludingIgnored(t *testing.T) { + type args struct { + logs string + } + tests := []struct { + name string + args args + want []string + }{ + { + // sample from https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_oadp-operator/1126/pull-ci-openshift-oadp-operator-master-4.10-operator-e2e-aws/1690109468546699264#1:build-log.txt%3A686 + name: "error patch for managed fields are ignored", + args: args{ + logs: `time="2023-08-11T22:02:39Z" level=debug msg="status field for endpointslices.discovery.k8s.io: exists: false, should restore: false" logSource="pkg/restore/restore.go:1487" restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d +time="2023-08-11T22:02:39Z" level=error msg="error patch for managed fields mysql-persistent/mysql-6ztv6: endpointslices.discovery.k8s.io \"mysql-6ztv6\" not found" logSource="pkg/restore/restore.go:1516" restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d +time="2023-08-11T22:02:39Z" level=info msg="Restored 40 items out of an estimated total of 50 (estimate will change throughout the restore)" logSource="pkg/restore/restore.go:669" name=mysql-6ztv6 namespace=mysql-persistent progress= resource=endpointslices.discovery.k8s.io restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d +time="2023-08-11T22:02:39Z" level=info msg="restore status includes excludes: " logSource="pkg/restore/restore.go:1189" restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d +time="2023-08-11T22:02:39Z" level=debug msg="Skipping action because it does not apply to this resource" logSource="pkg/plugin/framework/action_resolver.go:61" restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d`, + }, + want: []string{}, + }, + { + // sample from https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_oadp-operator/1126/pull-ci-openshift-oadp-operator-master-4.10-operator-e2e-aws/1690109468546699264#1:build-log.txt%3A686 + name: "error NOT for patch managed fields are NOT ignored", + args: args{ + logs: `time="2023-08-11T22:02:39Z" level=debug msg="status field for endpointslices.discovery.k8s.io: exists: false, should restore: false" logSource="pkg/restore/restore.go:1487" restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d +time="2023-08-11T22:02:39Z" level=error msg="error patch NOT for managed fields mysql-persistent/mysql-6ztv6: endpointslices.discovery.k8s.io \"mysql-6ztv6\" not found" logSource="pkg/restore/restore.go:1516" restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d +time="2023-08-11T22:02:39Z" level=info msg="Restored 40 items out of an estimated total of 50 (estimate will change throughout the restore)" logSource="pkg/restore/restore.go:669" name=mysql-6ztv6 namespace=mysql-persistent progress= resource=endpointslices.discovery.k8s.io restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d +time="2023-08-11T22:02:39Z" level=info msg="restore status includes excludes: " logSource="pkg/restore/restore.go:1189" restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d +time="2023-08-11T22:02:39Z" level=debug msg="Skipping action because it does not apply to this resource" logSource="pkg/plugin/framework/action_resolver.go:61" restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d`, + }, + want: []string{`time="2023-08-11T22:02:39Z" level=error msg="error patch NOT for managed fields mysql-persistent/mysql-6ztv6: endpointslices.discovery.k8s.io \"mysql-6ztv6\" not found" logSource="pkg/restore/restore.go:1516" restore=openshift-adp/mysql-twovol-csi-e2e-76673cb9-3892-11ee-b9ab-0a580a83082d`}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := errorLogsExcludingIgnored(tt.args.logs); !reflect.DeepEqual(got, tt.want) { + t.Errorf("errorLogsExcludingIgnored() = %v, want %v", got, tt.want) + } + }) + } +}