fix: add integration tests for NAB (migtools#73)

Signed-off-by: Mateus Oliveira <[email protected]>

Makefile

@@ -1,9 +1,9 @@
 
 # Image URL to use all building/pushing image targets
 IMG ?= quay.io/konveyor/oadp-non-admin:latest
-# Kubernetes version from OpenShift 4.15.x https://openshift-release.apps.ci.l2s4.p1.openshiftapps.com/#4-stable
+# Kubernetes version from OpenShift 4.16.x https://openshift-release.apps.ci.l2s4.p1.openshiftapps.com/#4-stable
 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.28
+ENVTEST_K8S_VERSION = 1.29
 
 # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
 ifeq (,$(shell go env GOBIN))
@@ -224,15 +224,14 @@ editorconfig: $(LOCALBIN) ## Download editorconfig locally if necessary.
 	mv $(LOCALBIN)/$${ec_binary} $(EC) ;\
 	}
 
-# TODO increase!!!
-COVERAGE_THRESHOLD=10
+COVERAGE_THRESHOLD=60
 
 .PHONY: ci
 ci: simulation-test lint docker-build hadolint check-generate check-manifests ec check-images ## Run all project continuous integration (CI) checks locally.
 
 .PHONY: simulation-test
 simulation-test: envtest ## Run unit and integration tests.
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $$(go list ./... | grep -v /e2e) -coverprofile cover.out
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $(shell go list ./... | grep -v oadp-non-admin/test) -test.coverprofile cover.out -test.v -ginkgo.vv
 	@make check-coverage
 
 .PHONY: check-coverage

cmd/main.go

@@ -98,7 +98,8 @@ func main() {
 		TLSOpts: tlsOpts,
 	})
 
-	if len(constant.OadpNamespace) == 0 {
+	oadpNamespace := os.Getenv(constant.NamespaceEnvVar)
+	if len(oadpNamespace) == 0 {
 		setupLog.Error(fmt.Errorf("%v environment variable is empty", constant.NamespaceEnvVar), "environment variable must be set")
 		os.Exit(1)
 	}
@@ -132,8 +133,9 @@ func main() {
 	}
 
 	if err = (&controller.NonAdminBackupReconciler{
-		Client: mgr.GetClient(),
-		Scheme: mgr.GetScheme(),
+		Client:        mgr.GetClient(),
+		Scheme:        mgr.GetScheme(),
+		OADPNamespace: oadpNamespace,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "NonAdminBackup")
 		os.Exit(1)

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/onsi/gomega v1.30.0
 	github.com/stretchr/testify v1.8.4
 	github.com/vmware-tanzu/velero v1.12.0
+	k8s.io/api v0.29.0
 	k8s.io/apimachinery v0.29.0
 	k8s.io/client-go v0.29.0
 	sigs.k8s.io/controller-runtime v0.17.0
@@ -65,7 +66,6 @@ require (
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
-	k8s.io/api v0.29.0 // indirect
 	k8s.io/apiextensions-apiserver v0.29.0 // indirect
 	k8s.io/component-base v0.29.0 // indirect
 	k8s.io/klog/v2 v2.110.1 // indirect

internal/common/constant/constant.go

@@ -17,8 +17,6 @@ limitations under the License.
 // Package constant contains all common constants used in the project
 package constant
 
-import "os"
-
 // Common labels for objects manipulated by the Non Admin Controller
 // Labels should be used to identify the NAC object
 // Annotations on the other hand should be used to define ownership
@@ -37,15 +35,9 @@ const (
 	NamespaceEnvVar = "WATCH_NAMESPACE"
 )
 
-// OadpNamespace is the namespace OADP operator is installed
-var OadpNamespace = os.Getenv(NamespaceEnvVar)
-
 // EmptyString defines a constant for the empty string
 const EmptyString = ""
 
-// NameSpaceString k8s Namespace string
-const NameSpaceString = "Namespace"
-
 // MaxKubernetesNameLength represents maximum length of the name in k8s
 const MaxKubernetesNameLength = 253
 

internal/common/function/function.go

@@ -21,14 +21,9 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
-	"errors"
 	"fmt"
-	"reflect"
 
-	"github.com/go-logr/logr"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
-	apimeta "k8s.io/apimachinery/pkg/api/meta"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -89,10 +84,8 @@ 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) {
-	if nonAdminBackup == nil {
-		return nil, fmt.Errorf("nonAdminBackup is nil")
-	}
-
+	// TODO https://github.com/migtools/oadp-non-admin/issues/60
+	// this should be Kubernetes API validation
 	if nonAdminBackup.Spec.BackupSpec == nil {
 		return nil, fmt.Errorf("BackupSpec is not defined")
 	}
@@ -105,7 +98,7 @@ func GetBackupSpecFromNonAdminBackup(nonAdminBackup *nacv1alpha1.NonAdminBackup)
 		veleroBackupSpec.IncludedNamespaces = []string{nonAdminBackup.Namespace}
 	} else {
 		if !containsOnlyNamespace(veleroBackupSpec.IncludedNamespaces, nonAdminBackup.Namespace) {
-			return nil, fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other then: %s", nonAdminBackup.Namespace)
+			return nil, fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other than: %s", nonAdminBackup.Namespace)
 		}
 	}
 
@@ -144,138 +137,9 @@ func GenerateVeleroBackupName(namespace, nabName string) string {
 	return veleroBackupName
 }
 
-// UpdateNonAdminPhase updates the phase of a NonAdminBackup object with the provided phase.
-func UpdateNonAdminPhase(ctx context.Context, r client.Client, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup, phase nacv1alpha1.NonAdminBackupPhase) (bool, error) {
-	if nab == nil {
-		return false, errors.New("NonAdminBackup object is nil")
-	}
-
-	// Ensure phase is valid
-	if phase == constant.EmptyString {
-		return false, errors.New("NonAdminBackupPhase cannot be empty")
-	}
-
-	if nab.Status.Phase == phase {
-		// No change, no need to update
-		logger.V(1).Info("NonAdminBackup Phase is already up to date")
-		return false, nil
-	}
-
-	// Update NAB status
-	nab.Status.Phase = phase
-	if err := r.Status().Update(ctx, nab); err != nil {
-		logger.Error(err, "Failed to update NonAdminBackup Phase")
-		return false, err
-	}
-
-	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Phase set to: %s", phase))
-
-	return true, nil
-}
-
-// UpdateNonAdminBackupCondition updates the condition of a NonAdminBackup object
-// based on the provided parameters. It validates the input parameters and ensures
-// that the condition is set to the desired status only if it differs from the current status.
-// If the condition is already set to the desired status, no update is performed.
-func UpdateNonAdminBackupCondition(ctx context.Context, r client.Client, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup, condition nacv1alpha1.NonAdminCondition, conditionStatus metav1.ConditionStatus, reason string, message string) (bool, error) {
-	if nab == nil {
-		return false, errors.New("NonAdminBackup object is nil")
-	}
-
-	// Ensure phase and condition are valid
-	if condition == constant.EmptyString {
-		return false, errors.New("NonAdminBackup Condition cannot be empty")
-	}
-
-	if conditionStatus == constant.EmptyString {
-		return false, errors.New("NonAdminBackup Condition Status cannot be empty")
-	} else if conditionStatus != metav1.ConditionTrue && conditionStatus != metav1.ConditionFalse && conditionStatus != metav1.ConditionUnknown {
-		return false, errors.New("NonAdminBackup Condition Status must be valid metav1.ConditionStatus")
-	}
-
-	if reason == constant.EmptyString {
-		return false, errors.New("NonAdminBackup Condition Reason cannot be empty")
-	}
-
-	if message == constant.EmptyString {
-		return false, errors.New("NonAdminBackup Condition Message cannot be empty")
-	}
-
-	// Check if the condition is already set to the desired status
-	currentCondition := apimeta.FindStatusCondition(nab.Status.Conditions, string(condition))
-	if currentCondition != nil && currentCondition.Status == conditionStatus && currentCondition.Reason == reason && currentCondition.Message == message {
-		// Condition is already set to the desired status, no need to update
-		logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition is already set to: %s", condition))
-		return false, nil
-	}
-
-	// Update NAB status condition
-	apimeta.SetStatusCondition(&nab.Status.Conditions,
-		metav1.Condition{
-			Type:    string(condition),
-			Status:  conditionStatus,
-			Reason:  reason,
-			Message: message,
-		},
-	)
-
-	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition to: %s", condition))
-	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition Reason to: %s", reason))
-	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition Message to: %s", message))
-
-	// Update NAB status
-	if err := r.Status().Update(ctx, nab); err != nil {
-		logger.Error(err, "NonAdminBackup Condition - Failed to update")
-		return false, err
-	}
-
-	return true, nil
-}
-
-// UpdateNonAdminBackupFromVeleroBackup update, if necessary, NonAdminBackup object fields related to referenced Velero Backup object, if no error occurs
-func UpdateNonAdminBackupFromVeleroBackup(ctx context.Context, r client.Client, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup, veleroBackup *velerov1api.Backup) (bool, error) {
-	logger.V(1).Info("NonAdminBackup BackupSpec and VeleroBackupStatus - request to update")
-
-	if reflect.DeepEqual(nab.Status.VeleroBackupStatus, &veleroBackup.Status) && reflect.DeepEqual(nab.Spec.BackupSpec, &veleroBackup.Spec) {
-		// No change, no need to update
-		logger.V(1).Info("NonAdminBackup BackupSpec and BackupStatus - nothing to update")
-		return false, nil
-	}
-
-	// Check if BackupStatus needs to be updated
-	if !reflect.DeepEqual(nab.Status.VeleroBackupStatus, &veleroBackup.Status) || nab.Status.VeleroBackupName != veleroBackup.Name || nab.Status.VeleroBackupNamespace != veleroBackup.Namespace {
-		nab.Status.VeleroBackupStatus = veleroBackup.Status.DeepCopy()
-		nab.Status.VeleroBackupName = veleroBackup.Name
-		nab.Status.VeleroBackupNamespace = veleroBackup.Namespace
-		if err := r.Status().Update(ctx, nab); err != nil {
-			logger.Error(err, "NonAdminBackup BackupStatus - Failed to update")
-			return false, err
-		}
-		logger.V(1).Info("NonAdminBackup BackupStatus - updated")
-	} else {
-		logger.V(1).Info("NonAdminBackup BackupStatus - up to date")
-	}
-
-	// Check if BackupSpec needs to be updated
-	if !reflect.DeepEqual(nab.Spec.BackupSpec, &veleroBackup.Spec) {
-		nab.Spec.BackupSpec = veleroBackup.Spec.DeepCopy()
-		if err := r.Update(ctx, nab); err != nil {
-			logger.Error(err, "NonAdminBackup BackupSpec - Failed to update")
-			return false, err
-		}
-		logger.V(1).Info("NonAdminBackup BackupSpec - updated")
-	} else {
-		logger.V(1).Info("NonAdminBackup BackupSpec - up to date")
-	}
-
-	// If either BackupStatus or BackupSpec was updated, return true
-	return true, nil
-}
-
 // CheckVeleroBackupLabels return true if Velero Backup object has required Non Admin labels, false otherwise
-func CheckVeleroBackupLabels(backup *velerov1api.Backup) bool {
+func CheckVeleroBackupLabels(labels map[string]string) bool {
 	// TODO also need to check for constant.OadpLabel label?
-	labels := backup.GetLabels()
 	value, exists := labels[constant.ManagedByLabel]
 	return exists && value == constant.ManagedByLabelValue
 }

internal/common/function/function_test.go

@@ -22,6 +22,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/onsi/ginkgo/v2"
 	"github.com/stretchr/testify/assert"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -35,6 +36,8 @@ import (
 	"github.com/migtools/oadp-non-admin/internal/common/constant"
 )
 
+var _ = ginkgo.Describe("PLACEHOLDER", func() {})
+
 func TestMergeMaps(t *testing.T) {
 	const (
 		d     = "d"
@@ -161,20 +164,13 @@ func TestAddNonAdminBackupAnnotations(t *testing.T) {
 }
 
 func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
-	// Test case: nonAdminBackup is nil
-	nonAdminBackup := (*nacv1alpha1.NonAdminBackup)(nil)
-	backupSpec, err := GetBackupSpecFromNonAdminBackup(nonAdminBackup)
-	assert.Error(t, err)
-	assert.Nil(t, backupSpec)
-	assert.Equal(t, "nonAdminBackup is nil", err.Error())
-
 	// Test case: BackupSpec is nil
-	nonAdminBackup = &nacv1alpha1.NonAdminBackup{
+	nonAdminBackup := &nacv1alpha1.NonAdminBackup{
 		Spec: nacv1alpha1.NonAdminBackupSpec{
 			BackupSpec: nil,
 		},
 	}
-	backupSpec, err = GetBackupSpecFromNonAdminBackup(nonAdminBackup)
+	backupSpec, err := GetBackupSpecFromNonAdminBackup(nonAdminBackup)
 	assert.Error(t, err)
 	assert.Nil(t, backupSpec)
 	assert.Equal(t, "BackupSpec is not defined", err.Error())
@@ -219,7 +215,7 @@ func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
 
 	assert.Error(t, err)
 	assert.Nil(t, backupSpec)
-	assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other then: namespace2", err.Error())
+	assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other than: namespace2", err.Error())
 
 	backupSpecInput = &velerov1api.BackupSpec{
 		IncludedNamespaces: []string{"namespace3"},
@@ -237,7 +233,7 @@ func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
 
 	assert.Error(t, err)
 	assert.Nil(t, backupSpec)
-	assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other then: namespace4", err.Error())
+	assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other than: namespace4", err.Error())
 }
 
 func TestGenerateVeleroBackupName(t *testing.T) {
@@ -330,15 +326,15 @@ func TestCheckVeleroBackupLabels(t *testing.T) {
 			},
 		},
 	}
-	assert.True(t, CheckVeleroBackupLabels(backupWithLabel), "Expected backup to have required label")
+	assert.True(t, CheckVeleroBackupLabels(backupWithLabel.GetLabels()), "Expected backup to have required label")
 
 	// Backup does not have the required label
 	backupWithoutLabel := &velerov1api.Backup{
 		ObjectMeta: metav1.ObjectMeta{
 			Labels: map[string]string{},
 		},
 	}
-	assert.False(t, CheckVeleroBackupLabels(backupWithoutLabel), "Expected backup to not have required label")
+	assert.False(t, CheckVeleroBackupLabels(backupWithoutLabel.GetLabels()), "Expected backup to not have required label")
 
 	// Backup has the required label with incorrect value
 	backupWithIncorrectValue := &velerov1api.Backup{
@@ -348,5 +344,5 @@ func TestCheckVeleroBackupLabels(t *testing.T) {
 			},
 		},
 	}
-	assert.False(t, CheckVeleroBackupLabels(backupWithIncorrectValue), "Expected backup to not have required label")
+	assert.False(t, CheckVeleroBackupLabels(backupWithIncorrectValue.GetLabels()), "Expected backup to not have required label")
 }