diff --git a/controllers/bsl.go b/controllers/bsl.go index e181a120a4..8caecfd4a6 100644 --- a/controllers/bsl.go +++ b/controllers/bsl.go @@ -17,9 +17,10 @@ import ( "github.com/openshift/oadp-operator/pkg/storage/aws" ) -func (r *DPAReconciler) ValidateBackupStorageLocations(dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { +func (r *DPAReconciler) ValidateBackupStorageLocations() (bool, error) { // Ensure BSL is a valid configuration // First, check for provider and then call functions based on the cloud provider for each backupstoragelocation configured + dpa := r.dpa numDefaultLocations := 0 for _, bslSpec := range dpa.Spec.BackupLocations { @@ -27,11 +28,11 @@ func (r *DPAReconciler) ValidateBackupStorageLocations(dpa oadpv1alpha1.DataProt return false, err } - if err := r.ensurePrefixWhenBackupImages(&dpa, &bslSpec); err != nil { + if err := r.ensurePrefixWhenBackupImages(&bslSpec); err != nil { return false, err } - if err := r.ensureSecretDataExists(&dpa, &bslSpec); err != nil { + if err := r.ensureSecretDataExists(&bslSpec); err != nil { return false, err } @@ -49,17 +50,17 @@ func (r *DPAReconciler) ValidateBackupStorageLocations(dpa oadpv1alpha1.DataProt // TODO: cases might need some updates for IBM/Minio/noobaa switch provider { case AWSProvider, "velero.io/aws": - err := r.validateAWSBackupStorageLocation(*bslSpec.Velero, &dpa) + err := r.validateAWSBackupStorageLocation(*bslSpec.Velero) if err != nil { return false, err } case AzureProvider, "velero.io/azure": - err := r.validateAzureBackupStorageLocation(*bslSpec.Velero, &dpa) + err := r.validateAzureBackupStorageLocation(*bslSpec.Velero) if err != nil { return false, err } case GCPProvider, "velero.io/gcp": - err := r.validateGCPBackupStorageLocation(*bslSpec.Velero, &dpa) + err := r.validateGCPBackupStorageLocation(*bslSpec.Velero) if err != nil { return false, err } @@ -97,12 +98,9 @@ func (r *DPAReconciler) ValidateBackupStorageLocations(dpa oadpv1alpha1.DataProt } func (r *DPAReconciler) ReconcileBackupStorageLocations(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - + dpa := r.dpa dpaBSLNames := []string{} + // Loop through all configured BSLs for i, bslSpec := range dpa.Spec.BackupLocations { // Create BSL as is, we can safely assume they are valid from @@ -146,7 +144,7 @@ func (r *DPAReconciler) ReconcileBackupStorageLocations(log logr.Logger) (bool, // TODO: check for BSL status condition errors and respond here if bslSpec.Velero != nil { - err := r.updateBSLFromSpec(&bsl, &dpa, *bslSpec.Velero) + err := r.updateBSLFromSpec(&bsl, *bslSpec.Velero) return err } @@ -156,7 +154,7 @@ func (r *DPAReconciler) ReconcileBackupStorageLocations(log logr.Logger) (bool, if err != nil { return err } - err = controllerutil.SetControllerReference(&dpa, &bsl, r.Scheme) + err = controllerutil.SetControllerReference(dpa, &bsl, r.Scheme) if err != nil { return err } @@ -263,9 +261,9 @@ func (r *DPAReconciler) UpdateCredentialsSecretLabels(secretName string, namespa return true, nil } -func (r *DPAReconciler) updateBSLFromSpec(bsl *velerov1.BackupStorageLocation, dpa *oadpv1alpha1.DataProtectionApplication, bslSpec velerov1.BackupStorageLocationSpec) error { +func (r *DPAReconciler) updateBSLFromSpec(bsl *velerov1.BackupStorageLocation, bslSpec velerov1.BackupStorageLocationSpec) error { // Set controller reference to Velero controller - err := controllerutil.SetControllerReference(dpa, bsl, r.Scheme) + err := controllerutil.SetControllerReference(r.dpa, bsl, r.Scheme) if err != nil { return err } @@ -313,9 +311,9 @@ func (r *DPAReconciler) updateBSLFromSpec(bsl *velerov1.BackupStorageLocation, d return nil } -func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error { +func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec) error { // validate provider plugin and secret - err := r.validateProviderPluginAndSecret(bslSpec, dpa) + err := r.validateProviderPluginAndSecret(bslSpec) if err != nil { return err } @@ -329,7 +327,7 @@ func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.Backup return fmt.Errorf("bucket name for AWS backupstoragelocation cannot be empty") } - if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() { + if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && r.dpa.BackupImages() { return fmt.Errorf("prefix for AWS backupstoragelocation object storage cannot be empty. It is required for backing up images") } @@ -347,9 +345,9 @@ func (r *DPAReconciler) validateAWSBackupStorageLocation(bslSpec velerov1.Backup return nil } -func (r *DPAReconciler) validateAzureBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error { +func (r *DPAReconciler) validateAzureBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec) error { // validate provider plugin and secret - err := r.validateProviderPluginAndSecret(bslSpec, dpa) + err := r.validateProviderPluginAndSecret(bslSpec) if err != nil { return err } @@ -371,16 +369,16 @@ func (r *DPAReconciler) validateAzureBackupStorageLocation(bslSpec velerov1.Back return fmt.Errorf("storageAccount for Azure backupstoragelocation config cannot be empty") } - if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() { + if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && r.dpa.BackupImages() { return fmt.Errorf("prefix for Azure backupstoragelocation object storage cannot be empty. it is required for backing up images") } return nil } -func (r *DPAReconciler) validateGCPBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error { +func (r *DPAReconciler) validateGCPBackupStorageLocation(bslSpec velerov1.BackupStorageLocationSpec) error { // validate provider plugin and secret - err := r.validateProviderPluginAndSecret(bslSpec, dpa) + err := r.validateProviderPluginAndSecret(bslSpec) if err != nil { return err } @@ -393,7 +391,7 @@ func (r *DPAReconciler) validateGCPBackupStorageLocation(bslSpec velerov1.Backup if len(bslSpec.ObjectStorage.Bucket) == 0 { return fmt.Errorf("bucket name for GCP backupstoragelocation cannot be empty") } - if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && dpa.BackupImages() { + if len(bslSpec.StorageType.ObjectStorage.Prefix) == 0 && r.dpa.BackupImages() { return fmt.Errorf("prefix for GCP backupstoragelocation object storage cannot be empty. it is required for backing up images") } @@ -409,12 +407,12 @@ func pluginExistsInVeleroCR(configuredPlugins []oadpv1alpha1.DefaultPlugin, expe return false } -func (r *DPAReconciler) validateProviderPluginAndSecret(bslSpec velerov1.BackupStorageLocationSpec, dpa *oadpv1alpha1.DataProtectionApplication) error { - if dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret") { +func (r *DPAReconciler) validateProviderPluginAndSecret(bslSpec velerov1.BackupStorageLocationSpec) error { + if r.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)) { + if !pluginExistsInVeleroCR(r.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)) //TODO: set warning condition on Velero CR } @@ -440,22 +438,22 @@ func (r *DPAReconciler) ensureBackupLocationHasVeleroOrCloudStorage(bsl *oadpv1a return nil } -func (r *DPAReconciler) ensurePrefixWhenBackupImages(dpa *oadpv1alpha1.DataProtectionApplication, bsl *oadpv1alpha1.BackupLocation) error { +func (r *DPAReconciler) ensurePrefixWhenBackupImages(bsl *oadpv1alpha1.BackupLocation) error { - if bsl.Velero != nil && bsl.Velero.ObjectStorage != nil && bsl.Velero.ObjectStorage.Prefix == "" && dpa.BackupImages() { + if bsl.Velero != nil && bsl.Velero.ObjectStorage != nil && bsl.Velero.ObjectStorage.Prefix == "" && r.dpa.BackupImages() { return fmt.Errorf("BackupLocation must have velero prefix when backupImages is not set to false") } - if bsl.CloudStorage != nil && bsl.CloudStorage.Prefix == "" && dpa.BackupImages() { + if bsl.CloudStorage != nil && bsl.CloudStorage.Prefix == "" && r.dpa.BackupImages() { return fmt.Errorf("BackupLocation must have cloud storage prefix when backupImages is not set to false") } return nil } -func (r *DPAReconciler) ensureSecretDataExists(dpa *oadpv1alpha1.DataProtectionApplication, bsl *oadpv1alpha1.BackupLocation) error { +func (r *DPAReconciler) ensureSecretDataExists(bsl *oadpv1alpha1.BackupLocation) error { // Check if the Velero feature flag 'no-secret' is not set - if !(dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret")) { + if !(r.dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret")) { // Check if the user specified credential under velero if bsl.Velero != nil && bsl.Velero.Credential != nil { // Check if user specified empty credential key diff --git a/controllers/bsl_test.go b/controllers/bsl_test.go index e46a5b2fb6..91c676b563 100644 --- a/controllers/bsl_test.go +++ b/controllers/bsl_test.go @@ -1503,8 +1503,9 @@ func TestDPAReconciler_ValidateBackupStorageLocations(t *testing.T) { Name: tt.dpa.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } - got, err := r.ValidateBackupStorageLocations(*tt.dpa) + got, err := r.ValidateBackupStorageLocations() if (err != nil) != tt.wantErr { t.Errorf("ValidateBackupStorageLocations() error = %v, wantErr %v", err, tt.wantErr) return @@ -1780,9 +1781,10 @@ func TestDPAReconciler_updateBSLFromSpec(t *testing.T) { } r := &DPAReconciler{ Scheme: scheme, + dpa: tt.dpa, } - err = r.updateBSLFromSpec(tt.bsl, tt.dpa, *tt.dpa.Spec.BackupLocations[0].Velero) + err = r.updateBSLFromSpec(tt.bsl, *tt.dpa.Spec.BackupLocations[0].Velero) if (err != nil) != tt.wantErr { t.Errorf("updateBSLFromSpec() error = %v, wantErr %v", err, tt.wantErr) return @@ -1912,6 +1914,7 @@ func TestDPAReconciler_ensureBackupLocationHasVeleroOrCloudStorage(t *testing.T) } r := &DPAReconciler{ Scheme: scheme, + dpa: tt.dpa, } for _, bsl := range tt.dpa.Spec.BackupLocations { if err := r.ensureBackupLocationHasVeleroOrCloudStorage(&bsl); (err != nil) != tt.wantErr { @@ -2086,9 +2089,10 @@ func TestDPAReconciler_ensurePrefixWhenBackupImages(t *testing.T) { } r := &DPAReconciler{ Scheme: scheme, + dpa: tt.dpa, } for _, bsl := range tt.dpa.Spec.BackupLocations { - err := r.ensurePrefixWhenBackupImages(tt.dpa, &bsl) + err := r.ensurePrefixWhenBackupImages(&bsl) if (err != nil) != tt.wantErr { t.Errorf("ensurePrefixWhenBackupImages() error = %v, wantErr %v", err, tt.wantErr) } @@ -2207,6 +2211,7 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { Name: tt.dpa.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } wantBSL := &velerov1.BackupStorageLocation{ ObjectMeta: metav1.ObjectMeta{ @@ -2558,7 +2563,9 @@ func TestDPAReconciler_ReconcileBackupStorageLocations(t *testing.T) { Name: tt.objects[0].GetName(), }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.objects[0].(*oadpv1alpha1.DataProtectionApplication), } + got, err := r.ReconcileBackupStorageLocations(r.Log) if (err != nil) != tt.wantErr { t.Errorf("ReconcileBackupStorageLocations() error =%v, wantErr %v", err, tt.wantErr) diff --git a/controllers/dpa_controller.go b/controllers/dpa_controller.go index d472399e6d..3c0eb588de 100644 --- a/controllers/dpa_controller.go +++ b/controllers/dpa_controller.go @@ -50,6 +50,7 @@ type DPAReconciler struct { Context context.Context NamespacedName types.NamespacedName EventRecorder record.EventRecorder + dpa *oadpv1alpha1.DataProtectionApplication } var debugMode = os.Getenv("DEBUG") == "true" @@ -79,9 +80,9 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R // Set reconciler context + name r.Context = ctx r.NamespacedName = req.NamespacedName - dpa := oadpv1alpha1.DataProtectionApplication{} + r.dpa = &oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(ctx, req.NamespacedName, &dpa); err != nil { + if err := r.Get(ctx, req.NamespacedName, r.dpa); err != nil { logger.Error(err, "unable to fetch DataProtectionApplication CR") return result, nil } @@ -107,7 +108,7 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R ) if err != nil { - apimeta.SetStatusCondition(&dpa.Status.Conditions, + apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{ Type: oadpv1alpha1.ConditionReconciled, Status: metav1.ConditionFalse, @@ -117,7 +118,7 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R ) } else { - apimeta.SetStatusCondition(&dpa.Status.Conditions, + apimeta.SetStatusCondition(&r.dpa.Status.Conditions, metav1.Condition{ Type: oadpv1alpha1.ConditionReconciled, Status: metav1.ConditionTrue, @@ -126,7 +127,7 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R }, ) } - statusErr := r.Client.Status().Update(ctx, &dpa) + statusErr := r.Client.Status().Update(ctx, r.dpa) if err == nil { // Don't mask previous error err = statusErr } @@ -213,7 +214,6 @@ type ReconcileFunc func(logr.Logger) (bool, error) // 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/monitor.go b/controllers/monitor.go index aa184ec564..a7c41b1f5b 100644 --- a/controllers/monitor.go +++ b/controllers/monitor.go @@ -8,16 +8,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - - oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" ) func (r *DPAReconciler) ReconcileVeleroMetricsSVC(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - svc := corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: "openshift-adp-velero-metrics-svc", @@ -28,7 +21,7 @@ func (r *DPAReconciler) ReconcileVeleroMetricsSVC(log logr.Logger) (bool, error) // Create SVC op, err := controllerutil.CreateOrPatch(r.Context, r.Client, &svc, func() error { // TODO: check for svc status condition errors and respond here - err := r.updateVeleroMetricsSVC(&svc, &dpa) + err := r.updateVeleroMetricsSVC(&svc) return err }) if err != nil { @@ -46,16 +39,16 @@ func (r *DPAReconciler) ReconcileVeleroMetricsSVC(log logr.Logger) (bool, error) return true, nil } -func (r *DPAReconciler) updateVeleroMetricsSVC(svc *corev1.Service, dpa *oadpv1alpha1.DataProtectionApplication) error { +func (r *DPAReconciler) updateVeleroMetricsSVC(svc *corev1.Service) error { // Setting controller owner reference on the metrics svc - err := controllerutil.SetControllerReference(dpa, svc, r.Scheme) + err := controllerutil.SetControllerReference(r.dpa, svc, r.Scheme) if err != nil { return err } // when updating the spec fields we update each field individually // to get around the immutable fields - svc.Spec.Selector = getDpaAppLabels(dpa) + svc.Spec.Selector = getDpaAppLabels(r.dpa) svc.Spec.Type = corev1.ServiceTypeClusterIP svc.Spec.Ports = []corev1.ServicePort{ @@ -69,6 +62,6 @@ func (r *DPAReconciler) updateVeleroMetricsSVC(svc *corev1.Service, dpa *oadpv1a }, } - svc.Labels = getDpaAppLabels(dpa) + svc.Labels = getDpaAppLabels(r.dpa) return nil } diff --git a/controllers/monitor_test.go b/controllers/monitor_test.go index e27230bf21..7f18f93255 100644 --- a/controllers/monitor_test.go +++ b/controllers/monitor_test.go @@ -158,9 +158,10 @@ func TestDPAReconciler_updateVeleroMetricsSVC(t *testing.T) { Name: tt.svc.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } - err = r.updateVeleroMetricsSVC(tt.svc, tt.dpa) + err = r.updateVeleroMetricsSVC(tt.svc) if !reflect.DeepEqual(tt.wantVeleroMtricsSVC.Labels, tt.svc.Labels) { t.Errorf("expected velero metrics svc labels to be %#v, got %#v", tt.wantVeleroMtricsSVC.Labels, tt.svc.Labels) } diff --git a/controllers/nodeagent.go b/controllers/nodeagent.go index 0992410535..269917bd17 100644 --- a/controllers/nodeagent.go +++ b/controllers/nodeagent.go @@ -100,12 +100,8 @@ func getNodeAgentObjectMeta(r *DPAReconciler) metav1.ObjectMeta { } func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} + dpa := r.dpa var deleteDaemonSet bool = true - - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } // Define "static" portion of daemonset ds := &appsv1.DaemonSet{ ObjectMeta: getNodeAgentObjectMeta(r), @@ -115,7 +111,7 @@ func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger) (bool, erro // V(-1) corresponds to the warn level var deprecationMsg string = "(Deprecation Warning) Use nodeAgent instead of restic, which is deprecated and will be removed in the future" log.V(-1).Info(deprecationMsg) - r.EventRecorder.Event(&dpa, corev1.EventTypeWarning, "DeprecationResticConfig", deprecationMsg) + r.EventRecorder.Event(dpa, corev1.EventTypeWarning, "DeprecationResticConfig", deprecationMsg) } if dpa.Spec.Configuration.Restic != nil && dpa.Spec.Configuration.Restic.Enable != nil && *dpa.Spec.Configuration.Restic.Enable { @@ -171,10 +167,10 @@ func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger) (bool, erro } } - if _, err := r.buildNodeAgentDaemonset(&dpa, ds); err != nil { + if _, err := r.buildNodeAgentDaemonset(ds); err != nil { return err } - if err := controllerutil.SetControllerReference(&dpa, ds, r.Scheme); err != nil { + if err := controllerutil.SetControllerReference(dpa, ds, r.Scheme); err != nil { return err } return nil @@ -215,7 +211,8 @@ func (r *DPAReconciler) ReconcileNodeAgentDaemonset(log logr.Logger) (bool, erro * ds - pointer to daemonset with objectMeta defined * returns: (pointer to daemonset, nil) if successful */ -func (r *DPAReconciler) buildNodeAgentDaemonset(dpa *oadpv1alpha1.DataProtectionApplication, ds *appsv1.DaemonSet) (*appsv1.DaemonSet, error) { +func (r *DPAReconciler) buildNodeAgentDaemonset(ds *appsv1.DaemonSet) (*appsv1.DaemonSet, error) { + dpa := r.dpa if dpa == nil { return nil, fmt.Errorf("dpa cannot be nil") } @@ -254,10 +251,11 @@ func (r *DPAReconciler) buildNodeAgentDaemonset(dpa *oadpv1alpha1.DataProtection ds.Spec = installDs.Spec ds.Name = dsName - return r.customizeNodeAgentDaemonset(dpa, ds) + return r.customizeNodeAgentDaemonset(ds) } -func (r *DPAReconciler) customizeNodeAgentDaemonset(dpa *oadpv1alpha1.DataProtectionApplication, ds *appsv1.DaemonSet) (*appsv1.DaemonSet, error) { +func (r *DPAReconciler) customizeNodeAgentDaemonset(ds *appsv1.DaemonSet) (*appsv1.DaemonSet, error) { + dpa := r.dpa if dpa.Spec.Configuration == nil || (dpa.Spec.Configuration.Restic == nil && dpa.Spec.Configuration.NodeAgent == nil) { // if restic and nodeAgent are not configured, therefore not enabled, return early. return nil, nil @@ -404,7 +402,7 @@ func (r *DPAReconciler) customizeNodeAgentDaemonset(dpa *oadpv1alpha1.DataProtec ds.Spec.Template.Spec.DNSConfig = &dpa.Spec.PodDnsConfig } - providerNeedsDefaultCreds, hasCloudStorage, err := r.noDefaultCredentials(*dpa) + providerNeedsDefaultCreds, hasCloudStorage, err := r.noDefaultCredentials() if err != nil { return nil, err } @@ -433,11 +431,6 @@ func (r *DPAReconciler) customizeNodeAgentDaemonset(dpa *oadpv1alpha1.DataProtec } func (r *DPAReconciler) ReconcileFsRestoreHelperConfig(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - fsRestoreHelperCM := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: FsRestoreHelperCM, @@ -459,7 +452,7 @@ func (r *DPAReconciler) ReconcileFsRestoreHelperConfig(log logr.Logger) (bool, e op, err := controllerutil.CreateOrPatch(r.Context, r.Client, &fsRestoreHelperCM, func() error { // update the Config Map - err := r.updateFsRestoreHelperCM(&fsRestoreHelperCM, &dpa) + err := r.updateFsRestoreHelperCM(&fsRestoreHelperCM) return err }) @@ -480,10 +473,10 @@ func (r *DPAReconciler) ReconcileFsRestoreHelperConfig(log logr.Logger) (bool, e return true, nil } -func (r *DPAReconciler) updateFsRestoreHelperCM(fsRestoreHelperCM *corev1.ConfigMap, dpa *oadpv1alpha1.DataProtectionApplication) error { +func (r *DPAReconciler) updateFsRestoreHelperCM(fsRestoreHelperCM *corev1.ConfigMap) error { // Setting controller owner reference on the FS restore helper CM - err := controllerutil.SetControllerReference(dpa, fsRestoreHelperCM, r.Scheme) + err := controllerutil.SetControllerReference(r.dpa, fsRestoreHelperCM, r.Scheme) if err != nil { return err } diff --git a/controllers/nodeagent_test.go b/controllers/nodeagent_test.go index 12648b1033..758f36bf0d 100644 --- a/controllers/nodeagent_test.go +++ b/controllers/nodeagent_test.go @@ -160,6 +160,7 @@ var _ = ginkgo.Describe("Test ReconcileNodeAgentDaemonSet function", func() { Namespace: scenario.namespace, }, EventRecorder: event, + dpa: dpa, } result, err := r.ReconcileNodeAgentDaemonset(logr.Discard()) @@ -1131,8 +1132,8 @@ func TestDPAReconciler_buildNodeAgentDaemonset(t *testing.T) { if err != nil { t.Errorf("error in creating fake client, likely programmer error") } - r := &DPAReconciler{Client: fakeClient} - if result, err := r.buildNodeAgentDaemonset(test.dpa, test.nodeAgentDaemonSet); err != nil { + r := &DPAReconciler{Client: fakeClient, dpa: test.dpa} + if result, err := r.buildNodeAgentDaemonset(test.nodeAgentDaemonSet); err != nil { if test.errorMessage != err.Error() { t.Errorf("buildNodeAgentDaemonset() error = %v, errorMessage %v", err, test.errorMessage) } @@ -1206,8 +1207,9 @@ func TestDPAReconciler_updateFsRestoreHelperCM(t *testing.T) { Name: tt.dpa.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } - if err := r.updateFsRestoreHelperCM(tt.fsRestoreHelperCM, tt.dpa); (err != nil) != tt.wantErr { + if err := r.updateFsRestoreHelperCM(tt.fsRestoreHelperCM); (err != nil) != tt.wantErr { t.Errorf("updateFsRestoreHelperCM() error = %v, wantErr %v", err, tt.wantErr) } if !reflect.DeepEqual(tt.fsRestoreHelperCM, tt.wantFsRestoreHelperCM) { @@ -1279,6 +1281,7 @@ func TestDPAReconciler_getPlatformType(t *testing.T) { Name: tt.dpa.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } got, err := r.getPlatformType() if (err != nil) != tt.wantErr { diff --git a/controllers/nonadmin_controller.go b/controllers/nonadmin_controller.go index 59b2fc0aa8..0a14b4a3a5 100644 --- a/controllers/nonadmin_controller.go +++ b/controllers/nonadmin_controller.go @@ -39,11 +39,7 @@ var ( ) func (r *DPAReconciler) ReconcileNonAdminController(log logr.Logger) (bool, error) { - // TODO https://github.com/openshift/oadp-operator/pull/1316 - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } + dpa := r.dpa nonAdminDeployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -53,7 +49,7 @@ func (r *DPAReconciler) ReconcileNonAdminController(log logr.Logger) (bool, erro } // Delete (possible) previously deployment - if !(r.checkNonAdminEnabled(&dpa) && dpa.Spec.UnsupportedOverrides[oadpv1alpha1.TechPreviewAck] == TrueVal) { + if !(r.checkNonAdminEnabled() && dpa.Spec.UnsupportedOverrides[oadpv1alpha1.TechPreviewAck] == TrueVal) { if err := r.Get( r.Context, types.NamespacedName{ @@ -95,13 +91,13 @@ func (r *DPAReconciler) ReconcileNonAdminController(log logr.Logger) (bool, erro r.Client, nonAdminDeployment, func() error { - err := r.buildNonAdminDeployment(nonAdminDeployment, &dpa) + err := r.buildNonAdminDeployment(nonAdminDeployment) if err != nil { return err } // Setting controller owner reference on the non admin controller deployment - return controllerutil.SetControllerReference(&dpa, nonAdminDeployment, r.Scheme) + return controllerutil.SetControllerReference(dpa, nonAdminDeployment, r.Scheme) }, ) if err != nil { @@ -119,9 +115,9 @@ func (r *DPAReconciler) ReconcileNonAdminController(log logr.Logger) (bool, erro return true, nil } -func (r *DPAReconciler) buildNonAdminDeployment(deploymentObject *appsv1.Deployment, dpa *oadpv1alpha1.DataProtectionApplication) error { - // TODO https://github.com/openshift/oadp-operator/pull/1316 - nonAdminImage := r.getNonAdminImage(dpa) +func (r *DPAReconciler) buildNonAdminDeployment(deploymentObject *appsv1.Deployment) error { + dpa := r.dpa + nonAdminImage := r.getNonAdminImage() imagePullPolicy, err := common.GetImagePullPolicy(dpa.Spec.ImagePullPolicy, nonAdminImage) if err != nil { r.Log.Error(err, "imagePullPolicy regex failed") @@ -193,8 +189,8 @@ func ensureRequiredSpecs(deploymentObject *appsv1.Deployment, image string, imag return nil } -func (r *DPAReconciler) checkNonAdminEnabled(dpa *oadpv1alpha1.DataProtectionApplication) bool { - // TODO https://github.com/openshift/oadp-operator/pull/1316 +func (r *DPAReconciler) checkNonAdminEnabled() bool { + dpa := r.dpa if dpa.Spec.NonAdmin != nil && dpa.Spec.NonAdmin.Enable != nil { return *dpa.Spec.NonAdmin.Enable @@ -202,8 +198,8 @@ func (r *DPAReconciler) checkNonAdminEnabled(dpa *oadpv1alpha1.DataProtectionApp return false } -func (r *DPAReconciler) getNonAdminImage(dpa *oadpv1alpha1.DataProtectionApplication) string { - // TODO https://github.com/openshift/oadp-operator/pull/1316 +func (r *DPAReconciler) getNonAdminImage() string { + dpa := r.dpa unsupportedOverride := dpa.Spec.UnsupportedOverrides[oadpv1alpha1.NonAdminControllerImageKey] if unsupportedOverride != "" { return unsupportedOverride diff --git a/controllers/nonadmin_controller_test.go b/controllers/nonadmin_controller_test.go index 28a3f8bc83..e97b04da8d 100644 --- a/controllers/nonadmin_controller_test.go +++ b/controllers/nonadmin_controller_test.go @@ -110,6 +110,7 @@ func runReconcileNonAdminControllerTest( Namespace: scenario.namespace, }, EventRecorder: event, + dpa: dpa, } result, err := r.ReconcileNonAdminController(logr.Discard()) @@ -240,10 +241,10 @@ var _ = ginkgo.Describe("Test ReconcileNonAdminController function", func() { }) func TestDPAReconcilerBuildNonAdminDeployment(t *testing.T) { - r := &DPAReconciler{} + r := &DPAReconciler{dpa: &oadpv1alpha1.DataProtectionApplication{}} t.Setenv("RELATED_IMAGE_NON_ADMIN_CONTROLLER", defaultNonAdminImage) deployment := createTestDeployment("test-build-deployment") - err := r.buildNonAdminDeployment(deployment, &oadpv1alpha1.DataProtectionApplication{}) + err := r.buildNonAdminDeployment(deployment) if err != nil { t.Errorf("buildNonAdminDeployment() errored out: %v", err) } @@ -298,7 +299,6 @@ func TestEnsureRequiredSpecs(t *testing.T) { } func TestDPAReconcilerCheckNonAdminEnabled(t *testing.T) { - r := &DPAReconciler{} tests := []struct { name string result bool @@ -354,7 +354,8 @@ func TestDPAReconcilerCheckNonAdminEnabled(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - result := r.checkNonAdminEnabled(test.dpa) + r := &DPAReconciler{dpa: test.dpa} + result := r.checkNonAdminEnabled() if result != test.result { t.Errorf("Results differ: got '%v' but expected '%v'", result, test.result) } @@ -363,7 +364,6 @@ func TestDPAReconcilerCheckNonAdminEnabled(t *testing.T) { } func TestDPAReconcilerGetNonAdminImage(t *testing.T) { - r := &DPAReconciler{} tests := []struct { name string image string @@ -401,10 +401,11 @@ func TestDPAReconcilerGetNonAdminImage(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { + r := &DPAReconciler{dpa: test.dpa} if len(test.env) > 0 { t.Setenv("RELATED_IMAGE_NON_ADMIN_CONTROLLER", test.env) } - image := r.getNonAdminImage(test.dpa) + image := r.getNonAdminImage() if image != test.image { t.Errorf("Images differ: got '%v' but expected '%v'", image, test.image) } diff --git a/controllers/registry.go b/controllers/registry.go index 3234cd53a1..87b2542721 100644 --- a/controllers/registry.go +++ b/controllers/registry.go @@ -159,11 +159,6 @@ type azureCredentials struct { } func (r *DPAReconciler) ReconcileRegistries(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - bslLabels := map[string]string{ "app.kubernetes.io/name": common.OADPOperatorVelero, "app.kubernetes.io/managed-by": common.OADPOperator, @@ -479,11 +474,6 @@ func (r *DPAReconciler) getMatchedKeyValue(key string, s string) (string, error) } func (r *DPAReconciler) ReconcileRegistrySVCs(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - // fetch the bsl instances bslList := velerov1.BackupStorageLocationList{} if err := r.List(r.Context, &bslList, &client.ListOptions{ @@ -527,11 +517,6 @@ func (r *DPAReconciler) ReconcileRegistrySVCs(log logr.Logger) (bool, error) { } func (r *DPAReconciler) ReconcileRegistryRoutes(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - // fetch the bsl instances bslList := velerov1.BackupStorageLocationList{} if err := r.List(r.Context, &bslList, &client.ListOptions{ @@ -583,7 +568,6 @@ func (r *DPAReconciler) ReconcileRegistryRoutes(log logr.Logger) (bool, error) { } func (r *DPAReconciler) ReconcileRegistryRouteConfigs(log logr.Logger) (bool, error) { - // Now for each of these bsl instances, create a registry route cm for each of them registryRouteCM := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -615,11 +599,7 @@ func (r *DPAReconciler) ReconcileRegistryRouteConfigs(log logr.Logger) (bool, er // Create secret for registry to be parsed by openshift-velero-plugin func (r *DPAReconciler) ReconcileRegistrySecrets(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - + dpa := r.dpa // fetch the bsl instances bslList := velerov1.BackupStorageLocationList{} if err := r.List(r.Context, &bslList, &client.ListOptions{ @@ -672,7 +652,7 @@ func (r *DPAReconciler) ReconcileRegistrySecrets(log logr.Logger) (bool, error) // Create Secret op, err := controllerutil.CreateOrPatch(r.Context, r.Client, &secret, func() error { // TODO: check for secret status condition errors and respond here - err := r.patchRegistrySecret(&secret, &bsl, &dpa) + err := r.patchRegistrySecret(&secret, &bsl) return err }) @@ -692,9 +672,9 @@ func (r *DPAReconciler) ReconcileRegistrySecrets(log logr.Logger) (bool, error) return true, nil } -func (r *DPAReconciler) patchRegistrySecret(secret *corev1.Secret, bsl *velerov1.BackupStorageLocation, dpa *oadpv1alpha1.DataProtectionApplication) error { +func (r *DPAReconciler) patchRegistrySecret(secret *corev1.Secret, bsl *velerov1.BackupStorageLocation) error { // Setting controller owner reference on the registry secret - err := controllerutil.SetControllerReference(dpa, secret, r.Scheme) + err := controllerutil.SetControllerReference(r.dpa, secret, r.Scheme) if err != nil { return err } diff --git a/controllers/restore_resource_version_priority.go b/controllers/restore_resource_version_priority.go index 07e65dad87..047cba3a1b 100644 --- a/controllers/restore_resource_version_priority.go +++ b/controllers/restore_resource_version_priority.go @@ -6,8 +6,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - - oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" ) const ( @@ -16,7 +14,8 @@ const ( ) // If RestoreResourcesVersionPriority is defined, configmap is created or updated and feature flag for EnableAPIGroupVersions is added to velero -func (r *DPAReconciler) ReconcileRestoreResourcesVersionPriority(dpa *oadpv1alpha1.DataProtectionApplication) (bool, error) { +func (r *DPAReconciler) ReconcileRestoreResourcesVersionPriority() (bool, error) { + dpa := r.dpa if len(dpa.Spec.Configuration.Velero.RestoreResourcesVersionPriority) == 0 { return true, nil } diff --git a/controllers/validator.go b/controllers/validator.go index 7135d99bac..5df2e57072 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -14,12 +14,10 @@ import ( // 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 { - return false, err - } + + dpa := r.dpa + if dpa.Spec.Configuration == nil || dpa.Spec.Configuration.Velero == nil { return false, errors.New("DPA CR Velero configuration cannot be nil") } @@ -41,10 +39,10 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) } } - if validBsl, err := r.ValidateBackupStorageLocations(dpa); !validBsl || err != nil { + if validBsl, err := r.ValidateBackupStorageLocations(); !validBsl || err != nil { return validBsl, err } - if validVsl, err := r.ValidateVolumeSnapshotLocations(dpa); !validVsl || err != nil { + if validVsl, err := r.ValidateVolumeSnapshotLocations(); !validVsl || err != nil { return validVsl, err } @@ -63,18 +61,19 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) // TODO refactor to call functions only once // they are called here to check error, and then after to get value - if _, err := r.getVeleroResourceReqs(&dpa); err != nil { + if _, err := r.getVeleroResourceReqs(); err != nil { return false, err } - if _, err := getResticResourceReqs(&dpa); err != nil { + + if _, err := getResticResourceReqs(dpa); err != nil { return false, err } - if _, err := getNodeAgentResourceReqs(&dpa); err != nil { + if _, err := getNodeAgentResourceReqs(dpa); err != nil { return false, err } // validate non-admin enable and tech-preview-ack - if r.checkNonAdminEnabled(&dpa) { + if r.checkNonAdminEnabled() { if !(dpa.Spec.UnsupportedOverrides[oadpv1alpha1.TechPreviewAck] == TrueVal) { return false, errors.New("in order to enable/disable the non-admin feature please set dpa.spec.unsupportedOverrides[tech-preview-ack]: 'true'") } @@ -90,12 +89,9 @@ type empty struct{} // TODO: if multiple default plugins exist, ensure we validate all of them. // Right now its sequential validation func (r *DPAReconciler) ValidateVeleroPlugins(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } + dpa := r.dpa - providerNeedsDefaultCreds, hasCloudStorage, err := r.noDefaultCredentials(dpa) + providerNeedsDefaultCreds, hasCloudStorage, err := r.noDefaultCredentials() if err != nil { return false, err } diff --git a/controllers/validator_test.go b/controllers/validator_test.go index a341816b8c..020e737baa 100644 --- a/controllers/validator_test.go +++ b/controllers/validator_test.go @@ -1409,6 +1409,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { Namespace: tt.dpa.Namespace, Name: tt.dpa.Name, }, + dpa: tt.dpa, EventRecorder: record.NewFakeRecorder(10), } t.Run(tt.name, func(t *testing.T) { diff --git a/controllers/velero.go b/controllers/velero.go index 198db0a1e4..af75b09827 100644 --- a/controllers/velero.go +++ b/controllers/velero.go @@ -71,10 +71,8 @@ var ( ) func (r *DPAReconciler) ReconcileVeleroDeployment(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } + + dpa := r.dpa veleroDeployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -90,18 +88,18 @@ func (r *DPAReconciler) ReconcileVeleroDeployment(log logr.Logger) (bool, error) // Setting Deployment selector if a new object is created as it is immutable if veleroDeployment.ObjectMeta.CreationTimestamp.IsZero() { veleroDeployment.Spec.Selector = &metav1.LabelSelector{ - MatchLabels: getDpaAppLabels(&dpa), + MatchLabels: getDpaAppLabels(dpa), } } // update the Deployment template - err := r.buildVeleroDeployment(veleroDeployment, &dpa) + err := r.buildVeleroDeployment(veleroDeployment) if err != nil { return err } // Setting controller owner reference on the velero deployment - return controllerutil.SetControllerReference(&dpa, veleroDeployment, r.Scheme) + return controllerutil.SetControllerReference(dpa, veleroDeployment, r.Scheme) }) if debugMode && op != controllerutil.OperationResultNone { // for debugging purposes fmt.Printf("DEBUG: There was a diff which resulted in an operation on Velero Deployment: %s\n", cmp.Diff(orig, veleroDeployment)) @@ -138,22 +136,22 @@ func (r *DPAReconciler) ReconcileVeleroDeployment(log logr.Logger) (bool, error) return true, nil } -func (r *DPAReconciler) veleroServiceAccount(dpa *oadpv1alpha1.DataProtectionApplication) (*corev1.ServiceAccount, error) { +func (r *DPAReconciler) veleroServiceAccount() (*corev1.ServiceAccount, error) { annotations := make(map[string]string) - sa := install.ServiceAccount(dpa.Namespace, annotations) - sa.Labels = getDpaAppLabels(dpa) + sa := install.ServiceAccount(r.dpa.Namespace, annotations) + sa.Labels = getDpaAppLabels(r.dpa) return sa, nil } -func (r *DPAReconciler) veleroClusterRoleBinding(dpa *oadpv1alpha1.DataProtectionApplication) (*rbacv1.ClusterRoleBinding, error) { - crb := install.ClusterRoleBinding(dpa.Namespace) - crb.Labels = getDpaAppLabels(dpa) +func (r *DPAReconciler) veleroClusterRoleBinding() (*rbacv1.ClusterRoleBinding, error) { + crb := install.ClusterRoleBinding(r.dpa.Namespace) + crb.Labels = getDpaAppLabels(r.dpa) return crb, nil } // Build VELERO Deployment -func (r *DPAReconciler) buildVeleroDeployment(veleroDeployment *appsv1.Deployment, dpa *oadpv1alpha1.DataProtectionApplication) error { - +func (r *DPAReconciler) buildVeleroDeployment(veleroDeployment *appsv1.Deployment) error { + dpa := r.dpa if dpa == nil { return fmt.Errorf("DPA CR cannot be nil") } @@ -163,13 +161,13 @@ func (r *DPAReconciler) buildVeleroDeployment(veleroDeployment *appsv1.Deploymen // Auto corrects DPA dpa.AutoCorrect() - _, err := r.ReconcileRestoreResourcesVersionPriority(dpa) + _, err := r.ReconcileRestoreResourcesVersionPriority() if err != nil { return fmt.Errorf("error creating configmap for restore resource version priority:" + err.Error()) } // get resource requirements for velero deployment // ignoring err here as it is checked in validator.go - veleroResourceReqs, _ := r.getVeleroResourceReqs(dpa) + veleroResourceReqs, _ := r.getVeleroResourceReqs() podAnnotations, err := common.AppendUniqueKeyTOfTMaps(dpa.Spec.PodAnnotations, veleroDeployment.Annotations) if err != nil { return fmt.Errorf("error appending pod annotations: %v", err) @@ -208,10 +206,11 @@ func (r *DPAReconciler) buildVeleroDeployment(veleroDeployment *appsv1.Deploymen veleroDeployment.Labels = labels annotations, err := common.AppendUniqueKeyTOfTMaps(veleroDeployment.Annotations, installDeployment.Annotations) veleroDeployment.Annotations = annotations - return r.customizeVeleroDeployment(dpa, veleroDeployment) + return r.customizeVeleroDeployment(veleroDeployment) } -func (r *DPAReconciler) customizeVeleroDeployment(dpa *oadpv1alpha1.DataProtectionApplication, veleroDeployment *appsv1.Deployment) error { +func (r *DPAReconciler) customizeVeleroDeployment(veleroDeployment *appsv1.Deployment) error { + dpa := r.dpa //append dpa labels var err error veleroDeployment.Labels, err = common.AppendUniqueKeyTOfTMaps(veleroDeployment.Labels, getDpaAppLabels(dpa)) @@ -320,11 +319,11 @@ func (r *DPAReconciler) customizeVeleroDeployment(dpa *oadpv1alpha1.DataProtecti break } } - if err := r.customizeVeleroContainer(dpa, veleroDeployment, veleroContainer, hasShortLivedCredentials, prometheusPort); err != nil { + if err := r.customizeVeleroContainer(veleroDeployment, veleroContainer, hasShortLivedCredentials, prometheusPort); err != nil { return err } - providerNeedsDefaultCreds, hasCloudStorage, err := r.noDefaultCredentials(*dpa) + providerNeedsDefaultCreds, hasCloudStorage, err := r.noDefaultCredentials() if err != nil { return err } @@ -389,7 +388,7 @@ func (r *DPAReconciler) customizeVeleroDeployment(dpa *oadpv1alpha1.DataProtecti if veleroDeployment.Spec.ProgressDeadlineSeconds == nil { veleroDeployment.Spec.ProgressDeadlineSeconds = ptr.To(int32(600)) } - r.appendPluginSpecificSpecs(dpa, veleroDeployment, veleroContainer, providerNeedsDefaultCreds, hasCloudStorage) + r.appendPluginSpecificSpecs(veleroDeployment, veleroContainer, providerNeedsDefaultCreds, hasCloudStorage) setPodTemplateSpecDefaults(&veleroDeployment.Spec.Template) if configMapName, ok := dpa.Annotations[common.UnsupportedVeleroServerArgsAnnotation]; ok { if configMapName != "" { @@ -407,7 +406,8 @@ func (r *DPAReconciler) customizeVeleroDeployment(dpa *oadpv1alpha1.DataProtecti } // add plugin specific specs to velero deployment -func (r *DPAReconciler) appendPluginSpecificSpecs(dpa *oadpv1alpha1.DataProtectionApplication, veleroDeployment *appsv1.Deployment, veleroContainer *corev1.Container, providerNeedsDefaultCreds map[string]bool, hasCloudStorage bool) { +func (r *DPAReconciler) appendPluginSpecificSpecs(veleroDeployment *appsv1.Deployment, veleroContainer *corev1.Container, providerNeedsDefaultCreds map[string]bool, hasCloudStorage bool) { + dpa := r.dpa init_container_resources := veleroContainer.Resources for _, plugin := range dpa.Spec.Configuration.Velero.DefaultPlugins { @@ -506,7 +506,8 @@ func (r *DPAReconciler) appendPluginSpecificSpecs(dpa *oadpv1alpha1.DataProtecti } } -func (r *DPAReconciler) customizeVeleroContainer(dpa *oadpv1alpha1.DataProtectionApplication, veleroDeployment *appsv1.Deployment, veleroContainer *corev1.Container, hasShortLivedCredentials bool, prometheusPort *int) error { +func (r *DPAReconciler) customizeVeleroContainer(veleroDeployment *appsv1.Deployment, veleroContainer *corev1.Container, hasShortLivedCredentials bool, prometheusPort *int) error { + dpa := r.dpa if veleroContainer == nil { return fmt.Errorf("could not find velero container in Deployment") } @@ -720,7 +721,8 @@ func getResourceReqs(dpa *corev1.ResourceRequirements) (corev1.ResourceRequireme } // Get Velero Resource Requirements -func (r *DPAReconciler) getVeleroResourceReqs(dpa *oadpv1alpha1.DataProtectionApplication) (corev1.ResourceRequirements, error) { +func (r *DPAReconciler) getVeleroResourceReqs() (corev1.ResourceRequirements, error) { + dpa := r.dpa if dpa.Spec.Configuration.Velero != nil && dpa.Spec.Configuration.Velero.PodConfig != nil { return getResourceReqs(&dpa.Spec.Configuration.Velero.PodConfig.ResourceAllocations) } @@ -748,7 +750,8 @@ func getNodeAgentResourceReqs(dpa *oadpv1alpha1.DataProtectionApplication) (core // noDefaultCredentials determines if a provider needs the default credentials. // This returns a map of providers found to if they need a default credential, // a boolean if Cloud Storage backup storage location was used and an error if any occured. -func (r DPAReconciler) noDefaultCredentials(dpa oadpv1alpha1.DataProtectionApplication) (map[string]bool, bool, error) { +func (r DPAReconciler) noDefaultCredentials() (map[string]bool, bool, error) { + dpa := r.dpa providerNeedsDefaultCreds := map[string]bool{} hasCloudStorage := false if dpa.Spec.Configuration.Velero.NoDefaultBackupLocation { diff --git a/controllers/velero_test.go b/controllers/velero_test.go index 4c1c51c516..489b2a7de3 100644 --- a/controllers/velero_test.go +++ b/controllers/velero_test.go @@ -233,6 +233,7 @@ var _ = ginkgo.Describe("Test ReconcileVeleroDeployment function", func() { Namespace: scenario.namespace, }, EventRecorder: event, + dpa: dpa, } result, err := r.ReconcileVeleroDeployment(logr.Discard()) @@ -1719,12 +1720,12 @@ func TestDPAReconciler_buildVeleroDeployment(t *testing.T) { if err != nil { t.Errorf("error in creating fake client, likely programmer error") } - r := DPAReconciler{Client: fakeClient} + r := DPAReconciler{Client: fakeClient, dpa: test.dpa} oadpclient.SetClient(fakeClient) if test.testProxy { t.Setenv(proxyEnvKey, proxyEnvValue) } - if err := r.buildVeleroDeployment(test.veleroDeployment, test.dpa); err != nil { + if err := r.buildVeleroDeployment(test.veleroDeployment); err != nil { if test.errorMessage != err.Error() { t.Errorf("buildVeleroDeployment() error = %v, errorMessage %v", err, test.errorMessage) } @@ -1957,6 +1958,7 @@ func Test_validateVeleroPlugins(t *testing.T) { Name: tt.dpa.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } t.Run(tt.name, func(t *testing.T) { result, err := r.ValidateVeleroPlugins(r.Log) @@ -2038,8 +2040,9 @@ func TestDPAReconciler_noDefaultCredentials(t *testing.T) { } r := DPAReconciler{ Client: fakeClient, + dpa: &tt.args.dpa, } - got, got1, err := r.noDefaultCredentials(tt.args.dpa) + got, got1, err := r.noDefaultCredentials() if (err != nil) != tt.wantErr { t.Errorf("DPAReconciler.noDefaultCredentials() error = %v, wantErr %v", err, tt.wantErr) return @@ -2110,8 +2113,9 @@ func TestDPAReconciler_VeleroDebugEnvironment(t *testing.T) { } r := DPAReconciler{ Client: fakeClient, + dpa: dpa, } - err = r.buildVeleroDeployment(deployment, dpa) + err = r.buildVeleroDeployment(deployment) if (err != nil) != tt.wantErr { t.Errorf("DPAReconciler.VeleroDebugEnvironment error = %v, wantErr %v", err, tt.wantErr) return diff --git a/controllers/vsl.go b/controllers/vsl.go index 567a4ba6e7..58862149f3 100644 --- a/controllers/vsl.go +++ b/controllers/vsl.go @@ -52,11 +52,7 @@ var validAzureKeys = map[string]bool{ } func (r *DPAReconciler) LabelVSLSecrets(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - + dpa := r.dpa for _, vsl := range dpa.Spec.SnapshotLocations { provider := strings.TrimPrefix(vsl.Velero.Provider, veleroIOPrefix) switch provider { @@ -108,7 +104,8 @@ func (r *DPAReconciler) LabelVSLSecrets(log logr.Logger) (bool, error) { return true, nil } -func (r *DPAReconciler) ValidateVolumeSnapshotLocations(dpa oadpv1alpha1.DataProtectionApplication) (bool, error) { +func (r *DPAReconciler) ValidateVolumeSnapshotLocations() (bool, error) { + dpa := r.dpa for i, vslSpec := range dpa.Spec.SnapshotLocations { if vslSpec.Velero == nil { return false, errors.New("snapshotLocation velero configuration cannot be nil") @@ -204,7 +201,7 @@ func (r *DPAReconciler) ValidateVolumeSnapshotLocations(dpa oadpv1alpha1.DataPro } } - if err := r.ensureVslSecretDataExists(&dpa, &vslSpec); err != nil { + if err := r.ensureVslSecretDataExists(&vslSpec); err != nil { return false, err } @@ -213,11 +210,7 @@ func (r *DPAReconciler) ValidateVolumeSnapshotLocations(dpa oadpv1alpha1.DataPro } func (r *DPAReconciler) ReconcileVolumeSnapshotLocations(log logr.Logger) (bool, error) { - dpa := oadpv1alpha1.DataProtectionApplication{} - if err := r.Get(r.Context, r.NamespacedName, &dpa); err != nil { - return false, err - } - + dpa := r.dpa dpaVSLNames := []string{} // Loop through all configured VSLs for i, vslSpec := range dpa.Spec.SnapshotLocations { @@ -246,7 +239,7 @@ func (r *DPAReconciler) ReconcileVolumeSnapshotLocations(log logr.Logger) (bool, // SetOwnerReference instead // Set controller reference to Velero controller - err := controllerutil.SetControllerReference(&dpa, &vsl, r.Scheme) + err := controllerutil.SetControllerReference(dpa, &vsl, r.Scheme) if err != nil { return err } @@ -316,9 +309,9 @@ func containsPlugin(d []oadpv1alpha1.DefaultPlugin, value string) bool { return false } -func (r *DPAReconciler) ensureVslSecretDataExists(dpa *oadpv1alpha1.DataProtectionApplication, vsl *oadpv1alpha1.SnapshotLocation) error { +func (r *DPAReconciler) ensureVslSecretDataExists(vsl *oadpv1alpha1.SnapshotLocation) error { // Check if the Velero feature flag 'no-secret' is not set - if !(dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret")) { + if !(r.dpa.Spec.Configuration.Velero.HasFeatureFlag("no-secret")) { // Check if the user specified credential under velero if vsl.Velero != nil && vsl.Velero.Credential != nil { // Check if user specified empty credential key diff --git a/controllers/vsl_test.go b/controllers/vsl_test.go index aa49b96a55..609dd89b23 100644 --- a/controllers/vsl_test.go +++ b/controllers/vsl_test.go @@ -525,8 +525,9 @@ func TestDPAReconciler_ValidateVolumeSnapshotLocation(t *testing.T) { Name: tt.dpa.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } - got, err := r.ValidateVolumeSnapshotLocations(*tt.dpa) + got, err := r.ValidateVolumeSnapshotLocations() if (err != nil) != tt.wantErr { t.Errorf("ValidateVolumeSnapshotLocations() error = %v, wantErr %v", err, tt.wantErr) return @@ -589,6 +590,7 @@ func TestDPAReconciler_ReconcileVolumeSnapshotLocations(t *testing.T) { Name: tt.dpa.Name, }, EventRecorder: record.NewFakeRecorder(10), + dpa: tt.dpa, } wantVSL := &velerov1.VolumeSnapshotLocation{ ObjectMeta: metav1.ObjectMeta{