Skip to content

Commit 780e2f5

Browse files
author
Shawn Hurley
committed
Fixing and adding unit tests to validate validator logic
1 parent 943b354 commit 780e2f5

10 files changed

+316
-76
lines changed

‎api/v1alpha1/oadp_types.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ type ApplicationConfig struct {
108108
Restic *ResticConfig `json:"restic,omitempty"`
109109
}
110110

111-
type CloudStoreageLocation struct {
111+
type CloudStorageLocation struct {
112112
CloudStorageRef corev1.LocalObjectReference `json:"cloudStorageRef"`
113113

114114
// Config is for provider-specific configuration fields.
@@ -135,7 +135,7 @@ type BackupLocation struct {
135135
// +optional
136136
Velero *velero.BackupStorageLocationSpec `json:"velero,omitempty"`
137137
// +optional
138-
CloudStorage *CloudStoreageLocation `json:"bucket,omitempty"`
138+
CloudStorage *CloudStorageLocation `json:"bucket,omitempty"`
139139
}
140140

141141
// SnapshotLocation defines the configuration for the DPA snapshot store

‎api/v1alpha1/zz_generated.deepcopy.go

Lines changed: 34 additions & 34 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎bundle/manifests/oadp-operator.clusterserviceversion.yaml

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -448,10 +448,6 @@ spec:
448448
- --leader-elect
449449
command:
450450
- /manager
451-
volumeMounts:
452-
- mountPath: /var/run/secrets/openshift/serviceaccount
453-
name: bound-sa-token
454-
readOnly: true
455451
env:
456452
- name: WATCH_NAMESPACE
457453
valueFrom:
@@ -527,6 +523,10 @@ spec:
527523
path: /healthz
528524
port: 8081
529525
periodSeconds: 10
526+
volumeMounts:
527+
- mountPath: /var/run/secrets/openshift/serviceaccount
528+
name: bound-sa-token
529+
readOnly: true
530530
securityContext:
531531
runAsNonRoot: true
532532
serviceAccountName: openshift-adp-controller-manager
@@ -536,9 +536,9 @@ spec:
536536
projected:
537537
sources:
538538
- serviceAccountToken:
539-
path: token
540-
expirationSeconds: 3600
541539
audience: openshift
540+
expirationSeconds: 3600
541+
path: token
542542
permissions:
543543
- rules:
544544
- apiGroups:

‎bundle/manifests/oadp.openshift.io_cloudstorages.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ spec:
5959
type: string
6060
provider:
6161
enum:
62-
- AWS
62+
- aws
6363
type: string
6464
region:
6565
description: Region for the bucket to be in, will be us-east-1 if

‎bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ spec:
5656
0 disables sync.
5757
nullable: true
5858
type: string
59-
bucketRef:
59+
cloudStorageRef:
6060
description: LocalObjectReference contains enough information
6161
to let you locate the referenced object inside the same
6262
namespace.
@@ -96,7 +96,7 @@ spec:
9696
backup storage location.
9797
type: boolean
9898
required:
99-
- bucketRef
99+
- cloudStorageRef
100100
type: object
101101
velero:
102102
description: 'TODO: Add name/annotations/labels support'

‎config/manager/manager.yaml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ spec:
2727
containers:
2828
- command:
2929
- /manager
30+
volumeMounts:
31+
- mountPath: /var/run/secrets/openshift/serviceaccount
32+
name: bound-sa-token
33+
readOnly: true
3034
env:
3135
- name: WATCH_NAMESPACE
3236
valueFrom:
@@ -105,4 +109,12 @@ spec:
105109
cpu: 500m
106110
memory: 128Mi
107111
serviceAccountName: controller-manager
112+
volumes:
113+
- name: bound-sa-token
114+
projected:
115+
sources:
116+
- serviceAccountToken:
117+
path: token
118+
expirationSeconds: 3600
119+
audience: openshift
108120
terminationGracePeriodSeconds: 10

‎controllers/bsl_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -838,7 +838,7 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
838838
},
839839
},
840840
},
841-
CloudStorage: &oadpv1alpha1.CloudStoreageLocation{
841+
CloudStorage: &oadpv1alpha1.CloudStorageLocation{
842842
CloudStorageRef: corev1.LocalObjectReference{},
843843
Config: map[string]string{},
844844
Credential: &corev1.SecretKeySelector{},
@@ -868,7 +868,7 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
868868
Spec: oadpv1alpha1.DataProtectionApplicationSpec{
869869
BackupLocations: []oadpv1alpha1.BackupLocation{
870870
{
871-
CloudStorage: &oadpv1alpha1.CloudStoreageLocation{
871+
CloudStorage: &oadpv1alpha1.CloudStorageLocation{
872872
CloudStorageRef: corev1.LocalObjectReference{},
873873
Config: map[string]string{},
874874
Credential: &corev1.SecretKeySelector{},
@@ -898,7 +898,7 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
898898
Spec: oadpv1alpha1.DataProtectionApplicationSpec{
899899
BackupLocations: []oadpv1alpha1.BackupLocation{
900900
{
901-
CloudStorage: &oadpv1alpha1.CloudStoreageLocation{
901+
CloudStorage: &oadpv1alpha1.CloudStorageLocation{
902902
CloudStorageRef: corev1.LocalObjectReference{
903903
Name: "testing",
904904
},
@@ -930,7 +930,7 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
930930
Spec: oadpv1alpha1.DataProtectionApplicationSpec{
931931
BackupLocations: []oadpv1alpha1.BackupLocation{
932932
{
933-
CloudStorage: &oadpv1alpha1.CloudStoreageLocation{
933+
CloudStorage: &oadpv1alpha1.CloudStorageLocation{
934934
CloudStorageRef: corev1.LocalObjectReference{
935935
Name: "testing",
936936
},
@@ -1020,7 +1020,7 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) {
10201020
},
10211021
},
10221022
{
1023-
CloudStorage: &oadpv1alpha1.CloudStoreageLocation{
1023+
CloudStorage: &oadpv1alpha1.CloudStorageLocation{
10241024
CloudStorageRef: corev1.LocalObjectReference{
10251025
Name: "testing",
10261026
},

‎controllers/validator.go

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -58,32 +58,47 @@ func (r *DPAReconciler) ValidateVeleroPlugins(log logr.Logger) (bool, error) {
5858
}
5959

6060
providerNeedsDefaultCreds := map[string]bool{}
61+
hasCloudStorage := false
6162

6263
for _, bsl := range dpa.Spec.BackupLocations {
6364
if bsl.Velero != nil && bsl.Velero.Credential == nil {
6465
providerNeedsDefaultCreds[strings.TrimPrefix(bsl.Velero.Provider, "velero.io/")] = true
6566
}
66-
if bsl.CloudStorage != nil && bsl.CloudStorage.Credential == nil {
67-
cloudStroage := oadpv1alpha1.CloudStorage{}
68-
err := r.Get(r.Context, types.NamespacedName{Name: bsl.CloudStorage.CloudStorageRef.Name, Namespace: dpa.Namespace}, &cloudStroage)
69-
if err != nil {
70-
return false, err
67+
if bsl.CloudStorage != nil {
68+
hasCloudStorage = true
69+
if bsl.CloudStorage.Credential == nil {
70+
cloudStroage := oadpv1alpha1.CloudStorage{}
71+
err := r.Get(r.Context, types.NamespacedName{Name: bsl.CloudStorage.CloudStorageRef.Name, Namespace: dpa.Namespace}, &cloudStroage)
72+
if err != nil {
73+
return false, err
74+
}
75+
providerNeedsDefaultCreds[string(cloudStroage.Spec.Provider)] = true
7176
}
72-
providerNeedsDefaultCreds[string(cloudStroage.Spec.Provider)] = true
7377
}
7478
}
7579

7680
for _, vsl := range dpa.Spec.SnapshotLocations {
7781
if vsl.Velero != nil {
78-
providerNeedsDefaultCreds[vsl.Velero.Provider] = true
82+
// To handle the case where we want to manually hand the credentials for a cloud storage created
83+
// Bucket credententials via configuration. Only AWS is supported
84+
provider := strings.TrimPrefix(vsl.Velero.Provider, "velero.io")
85+
if provider != string(oadpv1alpha1.AWSBucketProvider) {
86+
providerNeedsDefaultCreds[provider] = true
87+
}
7988
}
8089
}
8190

8291
var defaultPlugin oadpv1alpha1.DefaultPlugin
8392
for _, plugin := range dpa.Spec.Configuration.Velero.DefaultPlugins {
8493

8594
pluginSpecificMap, ok := credentials.PluginSpecificFields[plugin]
86-
if ok && pluginSpecificMap.IsCloudProvider && providerNeedsDefaultCreds[string(plugin)] {
95+
pluginNeedsCheck, foundInBSLorVSL := providerNeedsDefaultCreds[string(plugin)]
96+
97+
if !foundInBSLorVSL && !hasCloudStorage {
98+
pluginNeedsCheck = true
99+
}
100+
101+
if ok && pluginSpecificMap.IsCloudProvider && pluginNeedsCheck {
87102
secretName := pluginSpecificMap.SecretName
88103
_, err := r.getProviderSecret(secretName)
89104
if err != nil {

0 commit comments

Comments
 (0)