From 27cccabbd2e845da21ef49489e7506f10c30696d Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 10 Aug 2023 21:11:11 -0400 Subject: [PATCH 1/9] validator and type updates --- api/v1alpha1/cloud_storage_types.go | 8 +- api/v1alpha1/oadp_types.go | 11 +++ ...enshift.io_dataprotectionapplications.yaml | 8 ++ ...enshift.io_dataprotectionapplications.yaml | 8 ++ controllers/validator.go | 24 +++--- controllers/validator_test.go | 77 +++++++++++++++++++ 6 files changed, 123 insertions(+), 13 deletions(-) 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/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/validator.go b/controllers/validator.go index 186412aff6..48a91b88f8 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -37,20 +37,24 @@ 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 set to true") + } + if location.CloudStorage != nil { + if location.CloudStorage.Prefix == "" && dpa.BackupImages() { + return false, errors.New("BackupLocation must have cloud storage prefix when backupImages is set to true") } } } - 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") } } diff --git a/controllers/validator_test.go b/controllers/validator_test.go index b3431930f7..b8352d291e 100644 --- a/controllers/validator_test.go +++ b/controllers/validator_test.go @@ -226,6 +226,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -276,6 +277,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -335,6 +337,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -385,6 +388,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{}, @@ -429,6 +433,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{}, @@ -466,6 +471,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -513,6 +519,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, }, }, + BackupImages: pointer.Bool(false), }, }, objects: []client.Object{ @@ -968,6 +975,76 @@ 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", + }, + }, + }, + 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) From 99b86dd529dd4e5d48f2ab5b9b0c6d42bbb7b05b Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 10 Aug 2023 21:34:21 -0400 Subject: [PATCH 2/9] validator and type updates Signed-off-by: Tiger Kaovilai --- api/v1alpha1/zz_generated.deepcopy.go | 5 +++++ controllers/bsl.go | 6 +----- controllers/bsl_test.go | 2 +- controllers/dpa_controller.go | 4 ++-- controllers/validator.go | 6 +++++- controllers/validator_test.go | 9 +++------ controllers/vsl.go | 6 +----- controllers/vsl_test.go | 2 +- 8 files changed, 19 insertions(+), 21 deletions(-) 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/controllers/bsl.go b/controllers/bsl.go index 84be85b74b..d5dfb982b3 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") } diff --git a/controllers/bsl_test.go b/controllers/bsl_test.go index a91567f973..b219dde3ae 100644 --- a/controllers/bsl_test.go +++ b/controllers/bsl_test.go @@ -1218,7 +1218,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 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 48a91b88f8..287c589a96 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 { @@ -99,7 +102,8 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) if _, err := getResticResourceReqs(&dpa); err != nil { return false, err } - + r.ValidateBackupStorageLocations(dpa) + r.ValidateVolumeSnapshotLocations(dpa) return true, nil } diff --git a/controllers/validator_test.go b/controllers/validator_test.go index b8352d291e..0b3e43da36 100644 --- a/controllers/validator_test.go +++ b/controllers/validator_test.go @@ -995,15 +995,13 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, Configuration: &oadpv1alpha1.ApplicationConfig{ Velero: &oadpv1alpha1.VeleroConfig{ - DefaultPlugins: []oadpv1alpha1.DefaultPlugin{ - }, + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{}, }, }, BackupImages: pointer.Bool(true), }, }, - objects: []client.Object{ - }, + objects: []client.Object{}, wantErr: true, want: false, }, @@ -1027,8 +1025,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { }, Configuration: &oadpv1alpha1.ApplicationConfig{ Velero: &oadpv1alpha1.VeleroConfig{ - DefaultPlugins: []oadpv1alpha1.DefaultPlugin{ - }, + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{}, }, }, BackupImages: pointer.Bool(true), 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 From f7370f9f48a90a5928efc390ecc1154469860038 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 10 Aug 2023 22:12:23 -0400 Subject: [PATCH 3/9] CloudStorageLocation prefix is added to BSL --- controllers/bsl.go | 2 + controllers/bsl_test.go | 315 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 313 insertions(+), 4 deletions(-) diff --git a/controllers/bsl.go b/controllers/bsl.go index d5dfb982b3..75bd48d4f1 100644 --- a/controllers/bsl.go +++ b/controllers/bsl.go @@ -142,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: diff --git a/controllers/bsl_test.go b/controllers/bsl_test.go index b219dde3ae..58404763f4 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" @@ -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,309 @@ 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) + } + }) + } } From d29c55235fad24e527c012f2ca37ef2de393c63f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 10 Aug 2023 22:50:47 -0400 Subject: [PATCH 4/9] clarify backupImages error Signed-off-by: Tiger Kaovilai --- controllers/bsl.go | 2 +- controllers/bsl_test.go | 3 +-- controllers/validator.go | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/controllers/bsl.go b/controllers/bsl.go index 75bd48d4f1..0a70206769 100644 --- a/controllers/bsl.go +++ b/controllers/bsl.go @@ -254,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") } diff --git a/controllers/bsl_test.go b/controllers/bsl_test.go index 58404763f4..7c4a2df14c 100644 --- a/controllers/bsl_test.go +++ b/controllers/bsl_test.go @@ -1804,7 +1804,7 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { Key: "credentials", }, Region: "test-region", - Name: "test-bucket", + Name: "test-bucket", }, }, }, @@ -1830,7 +1830,6 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { }, Key: "credentials", }, - }, }, }, diff --git a/controllers/validator.go b/controllers/validator.go index 287c589a96..defa3d9c33 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -50,7 +50,7 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) } if location.CloudStorage != nil { if location.CloudStorage.Prefix == "" && dpa.BackupImages() { - return false, errors.New("BackupLocation must have cloud storage prefix when backupImages is set to true") + return false, errors.New("BackupLocation must have cloud storage prefix when backupImages is not set to false") } } } From 5cc57502a0579c56b3248e97ee2c2acf3f9aad5c Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Fri, 11 Aug 2023 02:25:02 -0400 Subject: [PATCH 5/9] validator refactor returns err from validating funcs Signed-off-by: Tiger Kaovilai --- controllers/validator.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/controllers/validator.go b/controllers/validator.go index defa3d9c33..baf442d199 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -102,8 +102,12 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) if _, err := getResticResourceReqs(&dpa); err != nil { return false, err } - r.ValidateBackupStorageLocations(dpa) - r.ValidateVolumeSnapshotLocations(dpa) + 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 } From 7349db52bfeacc52a3fa457a7a96702200b614ea Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Fri, 11 Aug 2023 03:36:29 -0400 Subject: [PATCH 6/9] unit tests fix --- controllers/bsl.go | 3 + controllers/validator.go | 2 +- controllers/validator_test.go | 127 ++++++++++++++++++++++++++++++++-- 3 files changed, 127 insertions(+), 5 deletions(-) diff --git a/controllers/bsl.go b/controllers/bsl.go index 0a70206769..06fdab629b 100644 --- a/controllers/bsl.go +++ b/controllers/bsl.go @@ -327,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/validator.go b/controllers/validator.go index baf442d199..22fc2ed86b 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -102,7 +102,7 @@ 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 { + if validBsl, err := r.ValidateBackupStorageLocations(dpa); !validBsl || err != nil { return validBsl, err } if validVsl, err := r.ValidateVolumeSnapshotLocations(dpa); !validVsl || err != nil { diff --git a/controllers/validator_test.go b/controllers/validator_test.go index 0b3e43da36..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" @@ -378,6 +377,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { { Velero: &v1.VolumeSnapshotLocationSpec{ Provider: "aws", + Config: map[string]string{ + AWSRegion: "us-east-1", + }, }, }, }, @@ -422,6 +424,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { { Velero: &v1.VolumeSnapshotLocationSpec{ Provider: "aws", + Config: map[string]string{ + AWSRegion: "us-east-1", + }, }, }, }, @@ -461,6 +466,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { { Velero: &v1.VolumeSnapshotLocationSpec{ Provider: "aws", + Config: map[string]string{ + AWSRegion: "us-east-1", + }, }, }, }, @@ -502,6 +510,11 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { CloudStorageRef: corev1.LocalObjectReference{ Name: "testing", }, + Credential: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cloud-credentials", + }, + }, }, }, }, @@ -509,6 +522,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { { Velero: &v1.VolumeSnapshotLocationSpec{ Provider: "aws", + Config: map[string]string{ + AWSRegion: "us-east-1", + }, }, }, }, @@ -553,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", + }, + }, }, }, }, @@ -588,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", }, }, @@ -618,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", }, }, @@ -674,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{ @@ -682,6 +728,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "Creds", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -694,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, }, @@ -709,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{ @@ -717,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", }, }, @@ -734,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, }, @@ -749,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{ @@ -757,6 +841,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "cloud", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -798,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{ @@ -806,6 +899,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "cloud", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -868,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{ @@ -876,6 +978,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "cloud", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -924,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{ @@ -932,6 +1043,9 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Key: "cloud", Optional: new(bool), }, + Config: map[string]string{ + "region": "us-east-1", + }, }, }, }, @@ -1020,6 +1134,11 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Name: "testing", }, Prefix: "some-prefix", + Credential: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cloud-credentials", + }, + }, }, }, }, From 3c0365c9413eea12d04c8551d06602883ee3fda9 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Fri, 11 Aug 2023 03:43:12 -0400 Subject: [PATCH 7/9] simplify expression Signed-off-by: Tiger Kaovilai --- controllers/validator.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/controllers/validator.go b/controllers/validator.go index 22fc2ed86b..bf9b8696b9 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -48,10 +48,8 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) 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 set to true") } - if location.CloudStorage != nil { - if location.CloudStorage.Prefix == "" && dpa.BackupImages() { - return false, errors.New("BackupLocation must have cloud storage 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") } } From 9451f3fad219d2b4e9024e15859d36b7438b7529 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Sat, 12 Aug 2023 02:33:19 -0400 Subject: [PATCH 8/9] Add ignorable restore error logs and unit test Signed-off-by: Tiger Kaovilai --- tests/e2e/lib/velero_helpers.go | 32 ++++++------------ tests/e2e/lib/velero_helpers_test.go | 49 ++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 22 deletions(-) create mode 100644 tests/e2e/lib/velero_helpers_test.go 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) + } + }) + } +} From cac034aff80aa073cab1640826f971d30cee9c06 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 15 Aug 2023 23:30:27 -0400 Subject: [PATCH 9/9] consistent error string across bl.velero and bl.cloudstorage --- controllers/validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/validator.go b/controllers/validator.go index bf9b8696b9..dcca3fe0c2 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -46,7 +46,7 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) 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 set to true") + 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")