Skip to content

Commit 5c34c19

Browse files
kaovilaikaovilai
authored
AWS plugin config: BSL Region not required when s3ForcePathStyle is false and BackupImages is false (#517)
* OADP-153, Close #424 * Nil restic Config should delete previous restic daemonset * only check restic config if it is not nil * installCase wantError implement * Make err more verbose * commit metav1 * Changes for BackupImages considerations * fake client fix
1 parent 96bec82 commit 5c34c19

File tree

8 files changed

+312
-33
lines changed

8 files changed

+312
-33
lines changed

‎api/v1alpha1/oadp_types.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,11 @@ type DataProtectionApplicationList struct {
203203
Items []DataProtectionApplication `json:"items"`
204204
}
205205

206+
// Default BackupImages behavior when nil to true
207+
func (dpa *DataProtectionApplication) BackupImages() bool {
208+
return dpa.Spec.BackupImages == nil || *dpa.Spec.BackupImages
209+
}
210+
206211
func init() {
207212
SchemeBuilder.Register(&DataProtectionApplication{}, &DataProtectionApplicationList{}, &CloudStorage{}, &CloudStorageList{})
208213
}

‎controllers/bsl.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -190,9 +190,12 @@ func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.Backup
190190
return fmt.Errorf("bucket name for AWS backupstoragelocation cannot be empty")
191191
}
192192

193-
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 || len(bslSpec.Config[Region]) == 0 &&
194-
(dpa.Spec.BackupImages == nil || *dpa.Spec.BackupImages) {
195-
return fmt.Errorf("prefix and region for AWS backupstoragelocation object storage cannot be empty. It is required for backing up images")
193+
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() {
194+
return fmt.Errorf("prefix for AWS backupstoragelocation object storage cannot be empty. It is required for backing up images")
195+
}
196+
// BSL region is required when s3ForcePathStyle is true AND BackupImages is false
197+
if (bslSpec.Config == nil || len(bslSpec.Config[Region]) == 0 && bslSpec.Config[S3ForcePathStyle] == "true") && dpa.BackupImages() {
198+
return fmt.Errorf("region for AWS backupstoragelocation cannot be empty when s3ForcePathStyle is true or when backing up images")
196199
}
197200

198201
//TODO: Add minio, noobaa, local storage validations
@@ -224,7 +227,7 @@ func (r *DPAReconciler) validateAzureBackupStorageLocation(bslSpec velerov1.Back
224227
return fmt.Errorf("storageAccount for Azure backupstoragelocation config cannot be empty")
225228
}
226229

227-
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && (dpa.Spec.BackupImages == nil || *dpa.Spec.BackupImages) {
230+
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() {
228231
return fmt.Errorf("prefix for Azure backupstoragelocation object storage cannot be empty. it is required for backing up images")
229232
}
230233

@@ -246,8 +249,7 @@ func (r *DPAReconciler) validateGCPBackupStorageLocation(bslSpec velerov1.Backup
246249
if len(bslSpec.ObjectStorage.Bucket) == 0 {
247250
return fmt.Errorf("bucket name for GCP backupstoragelocation cannot be empty")
248251
}
249-
250-
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && (dpa.Spec.BackupImages == nil || *dpa.Spec.BackupImages) {
252+
if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() {
251253
return fmt.Errorf("prefix for GCP backupstoragelocation object storage cannot be empty. it is required for backing up images")
252254
}
253255

‎controllers/bsl_test.go

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1046,6 +1046,159 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
10461046
},
10471047
},
10481048
},
1049+
{
1050+
name: "BSL Region not set for aws provider without S3ForcePathStyle expect to fail",
1051+
dpa: &oadpv1alpha1.DataProtectionApplication{
1052+
ObjectMeta: metav1.ObjectMeta{
1053+
Name: "foo",
1054+
Namespace: "test-ns",
1055+
},
1056+
Spec: oadpv1alpha1.DataProtectionApplicationSpec{
1057+
Configuration: &oadpv1alpha1.ApplicationConfig{
1058+
Velero: &oadpv1alpha1.VeleroConfig{},
1059+
},
1060+
BackupLocations: []oadpv1alpha1.BackupLocation{
1061+
{
1062+
Velero: &velerov1.BackupStorageLocationSpec{
1063+
Provider: "aws",
1064+
StorageType: velerov1.StorageType{
1065+
ObjectStorage: &velerov1.ObjectStorageLocation{
1066+
Bucket: "test-aws-bucket",
1067+
Prefix: "test-prefix",
1068+
},
1069+
},
1070+
Credential: &corev1.SecretKeySelector{
1071+
LocalObjectReference: corev1.LocalObjectReference{
1072+
Name: "cloud-credentials",
1073+
},
1074+
},
1075+
},
1076+
},
1077+
},
1078+
},
1079+
},
1080+
want: false,
1081+
wantErr: true,
1082+
secret: &corev1.Secret{
1083+
ObjectMeta: metav1.ObjectMeta{
1084+
Name: "cloud-credentials",
1085+
Namespace: "test-ns",
1086+
},
1087+
},
1088+
},
1089+
{
1090+
name: "BSL Region not set for aws provider without S3ForcePathStyle with BackupImages false expect to succeed",
1091+
dpa: &oadpv1alpha1.DataProtectionApplication{
1092+
ObjectMeta: metav1.ObjectMeta{
1093+
Name: "foo",
1094+
Namespace: "test-ns",
1095+
},
1096+
Spec: oadpv1alpha1.DataProtectionApplicationSpec{
1097+
BackupImages: pointer.Bool(false),
1098+
Configuration: &oadpv1alpha1.ApplicationConfig{
1099+
Velero: &oadpv1alpha1.VeleroConfig{},
1100+
},
1101+
BackupLocations: []oadpv1alpha1.BackupLocation{
1102+
{
1103+
Velero: &velerov1.BackupStorageLocationSpec{
1104+
Provider: "aws",
1105+
StorageType: velerov1.StorageType{
1106+
ObjectStorage: &velerov1.ObjectStorageLocation{
1107+
Bucket: "bucket",
1108+
Prefix: "prefix",
1109+
},
1110+
},
1111+
},
1112+
},
1113+
},
1114+
},
1115+
},
1116+
secret: &corev1.Secret{
1117+
ObjectMeta: metav1.ObjectMeta{
1118+
Name: "cloud-credentials",
1119+
Namespace: "test-ns",
1120+
},
1121+
},
1122+
want: true,
1123+
wantErr: false,
1124+
},
1125+
{
1126+
name: "BSL Region set for aws provider with S3ForcePathStyle expect to succeed",
1127+
dpa: &oadpv1alpha1.DataProtectionApplication{
1128+
ObjectMeta: metav1.ObjectMeta{
1129+
Name: "foo",
1130+
Namespace: "test-ns",
1131+
},
1132+
Spec: oadpv1alpha1.DataProtectionApplicationSpec{
1133+
Configuration: &oadpv1alpha1.ApplicationConfig{
1134+
Velero: &oadpv1alpha1.VeleroConfig{},
1135+
},
1136+
BackupLocations: []oadpv1alpha1.BackupLocation{
1137+
{
1138+
Velero: &velerov1.BackupStorageLocationSpec{
1139+
Provider: "aws",
1140+
Config: map[string]string{
1141+
S3ForcePathStyle: "true",
1142+
Region: "noobaa",
1143+
},
1144+
StorageType: velerov1.StorageType{
1145+
ObjectStorage: &velerov1.ObjectStorageLocation{
1146+
Bucket: "bucket",
1147+
Prefix: "prefix",
1148+
},
1149+
},
1150+
},
1151+
},
1152+
},
1153+
},
1154+
},
1155+
secret: &corev1.Secret{
1156+
ObjectMeta: metav1.ObjectMeta{
1157+
Name: "cloud-credentials",
1158+
Namespace: "test-ns",
1159+
},
1160+
},
1161+
want: true,
1162+
wantErr: false,
1163+
},
1164+
{
1165+
name: "BSL Region not set for aws provider with S3ForcePathStyle expect to fail",
1166+
dpa: &oadpv1alpha1.DataProtectionApplication{
1167+
ObjectMeta: metav1.ObjectMeta{
1168+
Name: "foo",
1169+
Namespace: "test-ns",
1170+
},
1171+
Spec: oadpv1alpha1.DataProtectionApplicationSpec{
1172+
Configuration: &oadpv1alpha1.ApplicationConfig{
1173+
Velero: &oadpv1alpha1.VeleroConfig{},
1174+
},
1175+
BackupLocations: []oadpv1alpha1.BackupLocation{
1176+
{
1177+
Velero: &velerov1.BackupStorageLocationSpec{
1178+
Provider: "aws",
1179+
Config: map[string]string{
1180+
S3ForcePathStyle: "true",
1181+
},
1182+
StorageType: velerov1.StorageType{
1183+
ObjectStorage: &velerov1.ObjectStorageLocation{
1184+
Bucket: "bucket",
1185+
Prefix: "prefix",
1186+
},
1187+
},
1188+
},
1189+
},
1190+
},
1191+
},
1192+
},
1193+
secret: &corev1.Secret{
1194+
ObjectMeta: metav1.ObjectMeta{
1195+
Name: "cloud-credentials",
1196+
Namespace: "test-ns",
1197+
},
1198+
},
1199+
want: false,
1200+
wantErr: true,
1201+
},
10491202
}
10501203
for _, tt := range tests {
10511204
t.Run(tt.name, func(t *testing.T) {

‎controllers/registry.go

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ const (
6767
Region = "region"
6868
Profile = "profile"
6969
S3URL = "s3Url"
70+
S3ForcePathStyle = "s3ForcePathStyle"
7071
InsecureSkipTLSVerify = "insecureSkipTLSVerify"
7172
StorageAccount = "storageAccount"
7273
ResourceGroup = "resourceGroup"
@@ -192,7 +193,7 @@ func (r *DPAReconciler) ReconcileRegistries(log logr.Logger) (bool, error) {
192193
},
193194
}
194195

195-
if dpa.Spec.BackupImages != nil && !*dpa.Spec.BackupImages {
196+
if !dpa.BackupImages() {
196197
deleteContext := context.Background()
197198
if err := r.Get(deleteContext, types.NamespacedName{
198199
Name: registryDeployment.Name,
@@ -439,7 +440,12 @@ func (r *DPAReconciler) getAWSRegistryEnvVars(bsl *velerov1.BackupStorageLocatio
439440
}
440441

441442
if awsEnvVars[i].Name == RegistryStorageS3RegionEnvVarKey {
442-
awsEnvVars[i].Value = bsl.Spec.Config[Region]
443+
bslSpecRegion, regionInConfig := bsl.Spec.Config[Region]
444+
if regionInConfig {
445+
awsEnvVars[i].Value = bslSpecRegion
446+
} else {
447+
r.Log.Info("region not found in backupstoragelocation spec")
448+
}
443449
}
444450

445451
if awsEnvVars[i].Name == RegistryStorageS3SecretkeyEnvVarKey {
@@ -779,7 +785,7 @@ func (r *DPAReconciler) ReconcileRegistrySVCs(log logr.Logger) (bool, error) {
779785
},
780786
}
781787

782-
if dpa.Spec.BackupImages != nil && !*dpa.Spec.BackupImages {
788+
if !dpa.BackupImages() {
783789
deleteContext := context.Background()
784790
if err := r.Get(deleteContext, types.NamespacedName{
785791
Name: svc.Name,
@@ -888,7 +894,7 @@ func (r *DPAReconciler) ReconcileRegistryRoutes(log logr.Logger) (bool, error) {
888894
},
889895
}
890896

891-
if dpa.Spec.BackupImages != nil && !*dpa.Spec.BackupImages {
897+
if !dpa.BackupImages() {
892898
deleteContext := context.Background()
893899
if err := r.Get(deleteContext, types.NamespacedName{
894900
Name: route.Name,
@@ -980,7 +986,7 @@ func (r *DPAReconciler) ReconcileRegistryRouteConfigs(log logr.Logger) (bool, er
980986
},
981987
}
982988

983-
if dpa.Spec.BackupImages != nil && !*dpa.Spec.BackupImages {
989+
if !dpa.BackupImages() {
984990
deleteContext := context.Background()
985991
if err := r.Get(deleteContext, types.NamespacedName{
986992
Name: registryRouteCM.Name,

‎controllers/restic.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,15 +67,12 @@ func (r *DPAReconciler) ReconcileResticDaemonset(log logr.Logger) (bool, error)
6767
if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil {
6868
return false, err
6969
}
70-
if dpa.Spec.Configuration.Restic == nil {
71-
return false, nil
72-
}
7370

7471
// Define "static" portion of daemonset
7572
ds := &appsv1.DaemonSet{
7673
ObjectMeta: getResticObjectMeta(r),
7774
}
78-
if dpa.Spec.Configuration.Restic.Enable == nil || !*dpa.Spec.Configuration.Restic.Enable {
75+
if dpa.Spec.Configuration.Restic == nil || dpa.Spec.Configuration.Restic != nil && (dpa.Spec.Configuration.Restic.Enable == nil || !*dpa.Spec.Configuration.Restic.Enable) {
7976
deleteContext := context.Background()
8077
if err := r.Get(deleteContext, types.NamespacedName{
8178
Name: ds.Name,

‎tests/e2e/e2e_suite_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,10 @@ var _ = BeforeSuite(func() {
6161
Expect(err).NotTo(HaveOccurred())
6262

6363
vel = &dpaCustomResource{
64-
Namespace: namespace,
65-
Region: region,
66-
Bucket: s3Bucket,
67-
Provider: provider,
64+
Namespace: namespace,
65+
Region: region,
66+
Bucket: s3Bucket,
67+
Provider: provider,
6868
ClusterProfile: clusterProfile,
6969
}
7070
testSuiteInstanceName := "ts-" + instanceName

0 commit comments

Comments
 (0)