Fix for #87 - Don't update NAB Spec from Velero Backup Spec (#88)

Signed-off-by: Michal Pryc <mpryc@redhat.com>

.golangci.yml

@@ -47,6 +47,17 @@ linters-settings:
       limitations under the License.
   govet:
     enable-all: true
+  importas:
+    no-unaliased: true
+    alias:
+      - pkg: github.com/vmware-tanzu/velero/pkg/apis/velero/v1
+        alias: velerov1
+      - pkg: k8s.io/apimachinery/pkg/apis/meta/v1
+        alias: metav1
+      - pkg: k8s.io/api/core/v1
+        alias: corev1
+      - pkg: github.com/migtools/oadp-non-admin/api/v1alpha1
+        alias: nacv1alpha1
   misspell:
     locale: US
   nakedret:
@@ -95,6 +106,7 @@ linters:
     - gosec
     - gosimple
     - govet
+    - importas
     - ineffassign
     - misspell
     - nakedret

api/v1alpha1/nonadminbackup_types.go

@@ -17,7 +17,7 @@ limitations under the License.
 package v1alpha1
 
 import (
-	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -37,7 +37,7 @@ const (
 // NonAdminBackupSpec defines the desired state of NonAdminBackup
 type NonAdminBackupSpec struct {
 	// BackupSpec defines the specification for a Velero backup.
-	BackupSpec *velerov1api.BackupSpec `json:"backupSpec,omitempty"`
+	BackupSpec *velerov1.BackupSpec `json:"backupSpec,omitempty"`
 
 	// NonAdminBackup log level (use debug for the most logging, leave unset for default)
 	// +optional
@@ -58,7 +58,7 @@ type NonAdminBackupStatus struct {
 
 	// VeleroBackupStatus captures the current status of a Velero backup.
 	// +optional
-	VeleroBackupStatus *velerov1api.BackupStatus `json:"veleroBackupStatus,omitempty"`
+	VeleroBackupStatus *velerov1.BackupStatus `json:"veleroBackupStatus,omitempty"`
 
 	Phase      NonAdminBackupPhase `json:"phase,omitempty"`
 	Conditions []metav1.Condition  `json:"conditions,omitempty"`

cmd/main.go

@@ -23,7 +23,7 @@ import (
 	"fmt"
 	"os"
 
-	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
@@ -51,7 +51,7 @@ func init() {
 
 	utilruntime.Must(nacv1alpha1.AddToScheme(scheme))
 
-	utilruntime.Must(velerov1api.AddToScheme(scheme))
+	utilruntime.Must(velerov1.AddToScheme(scheme))
 	// +kubebuilder:scaffold:scheme
 }
 

internal/common/function/function.go

@@ -23,7 +23,7 @@ import (
 	"encoding/hex"
 	"fmt"
 
-	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -83,7 +83,7 @@ func containsOnlyNamespace(namespaces []string, namespace string) bool {
 }
 
 // GetBackupSpecFromNonAdminBackup return BackupSpec object from NonAdminBackup spec, if no error occurs
-func GetBackupSpecFromNonAdminBackup(nonAdminBackup *nacv1alpha1.NonAdminBackup) (*velerov1api.BackupSpec, error) {
+func GetBackupSpecFromNonAdminBackup(nonAdminBackup *nacv1alpha1.NonAdminBackup) (*velerov1.BackupSpec, error) {
 	// TODO https://github.com/migtools/oadp-non-admin/issues/60
 	// this should be Kubernetes API validation
 	if nonAdminBackup.Spec.BackupSpec == nil {
@@ -147,7 +147,7 @@ func CheckVeleroBackupLabels(labels map[string]string) bool {
 // TODO not used
 
 // GetNonAdminBackupFromVeleroBackup return referenced NonAdminBackup object from Velero Backup object, if no error occurs
-func GetNonAdminBackupFromVeleroBackup(ctx context.Context, clientInstance client.Client, backup *velerov1api.Backup) (*nacv1alpha1.NonAdminBackup, error) {
+func GetNonAdminBackupFromVeleroBackup(ctx context.Context, clientInstance client.Client, backup *velerov1.Backup) (*nacv1alpha1.NonAdminBackup, error) {
 	// Check if the backup has the required annotations to identify the associated NonAdminBackup object
 	logger := log.FromContext(ctx)
 

internal/common/function/function_test.go

@@ -24,7 +24,7 @@ import (
 
 	"github.com/onsi/ginkgo/v2"
 	"github.com/stretchr/testify/assert"
-	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
@@ -176,7 +176,7 @@ func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
 	assert.Equal(t, "BackupSpec is not defined", err.Error())
 
 	// Test case: NonAdminBackup with valid BackupSpec
-	backupSpecInput := &velerov1api.BackupSpec{
+	backupSpecInput := &velerov1.BackupSpec{
 		IncludedNamespaces:      []string{"namespace1"},
 		StorageLocation:         "s3://bucket-name/path/to/backup",
 		VolumeSnapshotLocations: []string{"volume-snapshot-location"},
@@ -199,7 +199,7 @@ func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
 	assert.Equal(t, backupSpecInput.StorageLocation, backupSpec.StorageLocation)
 	assert.Equal(t, backupSpecInput.VolumeSnapshotLocations, backupSpec.VolumeSnapshotLocations)
 
-	backupSpecInput = &velerov1api.BackupSpec{
+	backupSpecInput = &velerov1.BackupSpec{
 		IncludedNamespaces: []string{"namespace2", "namespace3"},
 	}
 
@@ -217,7 +217,7 @@ func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
 	assert.Nil(t, backupSpec)
 	assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other than: namespace2", err.Error())
 
-	backupSpecInput = &velerov1api.BackupSpec{
+	backupSpecInput = &velerov1.BackupSpec{
 		IncludedNamespaces: []string{"namespace3"},
 	}
 
@@ -289,7 +289,7 @@ func TestGetNonAdminBackupFromVeleroBackup(t *testing.T) {
 		t.Fatalf("Failed to register NonAdminBackup type: %v", err)
 	}
 
-	backup := &velerov1api.Backup{
+	backup := &velerov1.Backup{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "test-namespace",
 			Name:      "test-backup",
@@ -319,7 +319,7 @@ func TestGetNonAdminBackupFromVeleroBackup(t *testing.T) {
 
 func TestCheckVeleroBackupLabels(t *testing.T) {
 	// Backup has the required label
-	backupWithLabel := &velerov1api.Backup{
+	backupWithLabel := &velerov1.Backup{
 		ObjectMeta: metav1.ObjectMeta{
 			Labels: map[string]string{
 				constant.ManagedByLabel: constant.ManagedByLabelValue,
@@ -329,15 +329,15 @@ func TestCheckVeleroBackupLabels(t *testing.T) {
 	assert.True(t, CheckVeleroBackupLabels(backupWithLabel.GetLabels()), "Expected backup to have required label")
 
 	// Backup does not have the required label
-	backupWithoutLabel := &velerov1api.Backup{
+	backupWithoutLabel := &velerov1.Backup{
 		ObjectMeta: metav1.ObjectMeta{
 			Labels: map[string]string{},
 		},
 	}
 	assert.False(t, CheckVeleroBackupLabels(backupWithoutLabel.GetLabels()), "Expected backup to not have required label")
 
 	// Backup has the required label with incorrect value
-	backupWithIncorrectValue := &velerov1api.Backup{
+	backupWithIncorrectValue := &velerov1.Backup{
 		ObjectMeta: metav1.ObjectMeta{
 			Labels: map[string]string{
 				constant.ManagedByLabel: "incorrect-value",

internal/controller/nonadminbackup_controller.go

@@ -23,7 +23,7 @@ import (
 	"reflect"
 
 	"github.com/go-logr/logr"
-	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -98,7 +98,7 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return ctrl.Result{}, nil
 	}
 
-	reconcileExit, reconcileRequeue, reconcileErr = r.UpdateSpecStatus(ctx, logger, &nab)
+	reconcileExit, reconcileRequeue, reconcileErr = r.SyncVeleroBackupWithNonAdminBackup(ctx, logger, &nab)
 	if reconcileRequeue {
 		return ctrl.Result{Requeue: true}, reconcileErr
 	} else if reconcileExit && reconcileErr != nil {
@@ -213,27 +213,25 @@ func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger
 	return false, false, nil
 }
 
-// UpdateSpecStatus updates the Spec and Status from the NonAdminBackup.
+// SyncVeleroBackupWithNonAdminBackup ensures the VeleroBackup associated with the given NonAdminBackup resource
+// is created, if it does not exist.
+// The function also updates the status and conditions of the NonAdminBackup resource to reflect the state
+// of the VeleroBackup.
 //
 // Parameters:
 //
 //	ctx: Context for the request.
-//	log: Logger instance for logging messages.
+//	logrLogger: Logger instance for logging messages.
 //	nab: Pointer to the NonAdminBackup object.
-//
-// The function generates the name for the Velero Backup object based on the provided namespace and name.
-// It then checks if a Velero Backup object with that name already exists. If it does not exist, it creates a new one
-// and updates NonAdminBackup Status. Otherwise, updates NonAdminBackup VeleroBackup Spec and Status based on Velero Backup object Spec and Status.
-// The function returns boolean values indicating whether the reconciliation loop should exit or requeue
-func (r *NonAdminBackupReconciler) UpdateSpecStatus(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) {
+func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) {
 	logger := logrLogger
 
 	veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name)
 	if veleroBackupName == constant.EmptyString {
 		return true, false, errors.New("unable to generate Velero Backup name")
 	}
 
-	veleroBackup := velerov1api.Backup{}
+	veleroBackup := velerov1.Backup{}
 	veleroBackupLogger := logger.WithValues("VeleroBackup", types.NamespacedName{Name: veleroBackupName, Namespace: r.OADPNamespace})
 	err := r.Get(ctx, client.ObjectKey{Namespace: r.OADPNamespace, Name: veleroBackupName}, &veleroBackup)
 	if err != nil {
@@ -252,7 +250,7 @@ func (r *NonAdminBackupReconciler) UpdateSpecStatus(ctx context.Context, logrLog
 			return true, false, errBackup
 		}
 
-		veleroBackup = velerov1api.Backup{
+		veleroBackup = velerov1.Backup{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      veleroBackupName,
 				Namespace: r.OADPNamespace,
@@ -308,41 +306,28 @@ func (r *NonAdminBackupReconciler) UpdateSpecStatus(ctx context.Context, logrLog
 		return false, true, nil
 	}
 
-	// We should not update already created VeleroBackup object.
-	// The VeleroBackup within NonAdminBackup will
-	// be reverted back to the previous state - the state which created VeleroBackup
-	// in a first place, so they will be in sync.
-	veleroBackupLogger.Info("VeleroBackup already exists, checking if NonAdminBackup VeleroBackupSpec and VeleroBackupStatus needs update")
+	// Ensure that the NonAdminBackup's NonAdminBackupStatus is in sync
+	// with the VeleroBackup. Any required updates to the NonAdminBackup
+	// Status will be applied based on the current state of the VeleroBackup.
+	veleroBackupLogger.Info("VeleroBackup already exists, verifying if NonAdminBackup Status requires update")
 	updated = updateNonAdminBackupVeleroBackupStatus(&nab.Status, &veleroBackup)
 	if updated {
 		if err := r.Status().Update(ctx, nab); err != nil {
-			veleroBackupLogger.Error(err, "NonAdminBackup BackupStatus - Failed to update")
-			return true, false, err
-		}
-
-		logger.V(1).Info("NonAdminBackup - Requeue after Status Update")
-		return false, true, nil
-	}
-	updated = updateNonAdminBackupVeleroBackupSpec(&nab.Spec, &veleroBackup)
-	if updated {
-		if err := r.Update(ctx, nab); err != nil {
-			veleroBackupLogger.Error(err, "NonAdminBackup BackupSpec - Failed to update")
+			veleroBackupLogger.Error(err, "Failed to update NonAdminBackup Status after VeleroBackup reconciliation")
 			return true, false, err
 		}
 
-		logger.V(1).Info("NonAdminBackup - Requeue after Spec Update")
-		return false, true, nil
+		logger.V(1).Info("NonAdminBackup Status updated successfully")
 	}
 
-	logger.V(1).Info("NonAdminBackup VeleroBackupSpec and VeleroBackupStatus already up to date")
 	return false, false, nil
 }
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *NonAdminBackupReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&nacv1alpha1.NonAdminBackup{}).
-		Watches(&velerov1api.Backup{}, &handler.VeleroBackupHandler{}).
+		Watches(&velerov1.Backup{}, &handler.VeleroBackupHandler{}).
 		WithEventFilter(predicate.CompositePredicate{
 			NonAdminBackupPredicate: predicate.NonAdminBackupPredicate{},
 			VeleroBackupPredicate: predicate.VeleroBackupPredicate{
@@ -370,7 +355,7 @@ func updateNonAdminPhase(phase *nacv1alpha1.NonAdminBackupPhase, newPhase nacv1a
 
 // updateNonAdminBackupVeleroBackupStatus sets the VeleroBackup fields in NonAdminBackup object status and returns true
 // if the VeleroBackup fields are changed by this call.
-func updateNonAdminBackupVeleroBackupStatus(status *nacv1alpha1.NonAdminBackupStatus, veleroBackup *velerov1api.Backup) bool {
+func updateNonAdminBackupVeleroBackupStatus(status *nacv1alpha1.NonAdminBackupStatus, veleroBackup *velerov1.Backup) bool {
 	if !reflect.DeepEqual(status.VeleroBackupStatus, &veleroBackup.Status) || status.VeleroBackupName != veleroBackup.Name || status.VeleroBackupNamespace != veleroBackup.Namespace {
 		status.VeleroBackupStatus = veleroBackup.Status.DeepCopy()
 		status.VeleroBackupName = veleroBackup.Name
@@ -379,13 +364,3 @@ func updateNonAdminBackupVeleroBackupStatus(status *nacv1alpha1.NonAdminBackupSt
 	}
 	return false
 }
-
-// updateNonAdminBackupVeleroBackupSpec sets the BackupSpec in NonAdminBackup object spec and returns true
-// if the BackupSpec is changed by this call.
-func updateNonAdminBackupVeleroBackupSpec(spec *nacv1alpha1.NonAdminBackupSpec, veleroBackup *velerov1api.Backup) bool {
-	if !reflect.DeepEqual(spec.BackupSpec, &veleroBackup.Spec) {
-		spec.BackupSpec = veleroBackup.Spec.DeepCopy()
-		return true
-	}
-	return false
-}