From f50ab815c4bd0b85ad285f37071e243f913901d2 Mon Sep 17 00:00:00 2001 From: Mateus Oliveira Date: Tue, 24 Sep 2024 11:06:28 -0300 Subject: [PATCH] fix: add integration tests for NAB (#73) Signed-off-by: Mateus Oliveira --- Makefile | 9 +- cmd/main.go | 8 +- go.mod | 2 +- hack/extra-crds/velero.io_backups.yaml | 607 ++++++++++++++++ internal/common/constant/constant.go | 8 - internal/common/function/function.go | 144 +--- internal/common/function/function_test.go | 24 +- .../controller/nonadminbackup_controller.go | 356 ++++++---- .../nonadminbackup_controller_test.go | 672 ++++++++++++++++-- internal/controller/suite_test.go | 26 +- internal/handler/velerobackup_handler.go | 8 +- .../predicate/nonadminbackup_predicate.go | 19 +- internal/predicate/velerobackup_predicate.go | 26 +- 13 files changed, 1508 insertions(+), 401 deletions(-) create mode 100644 hack/extra-crds/velero.io_backups.yaml diff --git a/Makefile b/Makefile index 443fa00..19f0dfd 100644 --- a/Makefile +++ b/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 diff --git a/cmd/main.go b/cmd/main.go index 8d6be27..a363de5 100644 --- a/cmd/main.go +++ b/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) diff --git a/go.mod b/go.mod index e0f489d..32d49c7 100644 --- a/go.mod +++ b/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 diff --git a/hack/extra-crds/velero.io_backups.yaml b/hack/extra-crds/velero.io_backups.yaml new file mode 100644 index 0000000..dd51184 --- /dev/null +++ b/hack/extra-crds/velero.io_backups.yaml @@ -0,0 +1,607 @@ +# from https://github.com/openshift/velero/blob/d8101a298016/config/crd/v1/bases/velero.io_backups.yaml +# from go.mod replace github.com/vmware-tanzu/velero => github.com/openshift/velero v0.10.2-0.20231024175012-d8101a298016 +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.12.0 + name: backups.velero.io +spec: + group: velero.io + names: + kind: Backup + listKind: BackupList + plural: backups + singular: backup + scope: Namespaced + versions: + - name: v1 + schema: + openAPIV3Schema: + description: Backup is a Velero resource that represents the capture of Kubernetes + cluster state at a point in time (API objects and associated volume state). + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: BackupSpec defines the specification for a Velero backup. + properties: + csiSnapshotTimeout: + description: CSISnapshotTimeout specifies the time used to wait for + CSI VolumeSnapshot status turns to ReadyToUse during creation, before + returning error as timeout. The default value is 10 minute. + type: string + datamover: + description: DataMover specifies the data mover to be used by the + backup. If DataMover is "" or "velero", the built-in data mover + will be used. + type: string + defaultVolumesToFsBackup: + description: DefaultVolumesToFsBackup specifies whether pod volume + file system backup should be used for all volumes by default. + nullable: true + type: boolean + defaultVolumesToRestic: + description: "DefaultVolumesToRestic specifies whether restic should + be used to take a backup of all pod volumes by default. \n Deprecated: + this field is no longer used and will be removed entirely in future. + Use DefaultVolumesToFsBackup instead." + nullable: true + type: boolean + excludedClusterScopedResources: + description: ExcludedClusterScopedResources is a slice of cluster-scoped + resource type names to exclude from the backup. If set to "*", all + cluster-scoped resource types are excluded. The default value is + empty. + items: + type: string + nullable: true + type: array + excludedNamespaceScopedResources: + description: ExcludedNamespaceScopedResources is a slice of namespace-scoped + resource type names to exclude from the backup. If set to "*", all + namespace-scoped resource types are excluded. The default value + is empty. + items: + type: string + nullable: true + type: array + excludedNamespaces: + description: ExcludedNamespaces contains a list of namespaces that + are not included in the backup. + items: + type: string + nullable: true + type: array + excludedResources: + description: ExcludedResources is a slice of resource names that are + not included in the backup. + items: + type: string + nullable: true + type: array + hooks: + description: Hooks represent custom behaviors that should be executed + at different phases of the backup. + properties: + resources: + description: Resources are hooks that should be executed when + backing up individual instances of a resource. + items: + description: BackupResourceHookSpec defines one or more BackupResourceHooks + that should be executed based on the rules defined for namespaces, + resources, and label selector. + properties: + excludedNamespaces: + description: ExcludedNamespaces specifies the namespaces + to which this hook spec does not apply. + items: + type: string + nullable: true + type: array + excludedResources: + description: ExcludedResources specifies the resources to + which this hook spec does not apply. + items: + type: string + nullable: true + type: array + includedNamespaces: + description: IncludedNamespaces specifies the namespaces + to which this hook spec applies. If empty, it applies + to all namespaces. + items: + type: string + nullable: true + type: array + includedResources: + description: IncludedResources specifies the resources to + which this hook spec applies. If empty, it applies to + all resources. + items: + type: string + nullable: true + type: array + labelSelector: + description: LabelSelector, if specified, filters the resources + to which this hook spec applies. + nullable: true + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector + that contains values, a key, and an operator that + relates the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: operator represents a key's relationship + to a set of values. Valid operators are In, + NotIn, Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. + If the operator is In or NotIn, the values array + must be non-empty. If the operator is Exists + or DoesNotExist, the values array must be empty. + This array is replaced during a strategic merge + patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. + A single {key,value} in the matchLabels map is equivalent + to an element of matchExpressions, whose key field + is "key", the operator is "In", and the values array + contains only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic + name: + description: Name is the name of this hook. + type: string + post: + description: PostHooks is a list of BackupResourceHooks + to execute after storing the item in the backup. These + are executed after all "additional items" from item actions + are processed. + items: + description: BackupResourceHook defines a hook for a resource. + properties: + exec: + description: Exec defines an exec hook. + properties: + command: + description: Command is the command and arguments + to execute. + items: + type: string + minItems: 1 + type: array + container: + description: Container is the container in the + pod where the command should be executed. If + not specified, the pod's first container is + used. + type: string + onError: + description: OnError specifies how Velero should + behave if it encounters an error executing this + hook. + enum: + - Continue + - Fail + type: string + timeout: + description: Timeout defines the maximum amount + of time Velero should wait for the hook to complete + before considering the execution a failure. + type: string + required: + - command + type: object + required: + - exec + type: object + type: array + pre: + description: PreHooks is a list of BackupResourceHooks to + execute prior to storing the item in the backup. These + are executed before any "additional items" from item actions + are processed. + items: + description: BackupResourceHook defines a hook for a resource. + properties: + exec: + description: Exec defines an exec hook. + properties: + command: + description: Command is the command and arguments + to execute. + items: + type: string + minItems: 1 + type: array + container: + description: Container is the container in the + pod where the command should be executed. If + not specified, the pod's first container is + used. + type: string + onError: + description: OnError specifies how Velero should + behave if it encounters an error executing this + hook. + enum: + - Continue + - Fail + type: string + timeout: + description: Timeout defines the maximum amount + of time Velero should wait for the hook to complete + before considering the execution a failure. + type: string + required: + - command + type: object + required: + - exec + type: object + type: array + required: + - name + type: object + nullable: true + type: array + type: object + includeClusterResources: + description: IncludeClusterResources specifies whether cluster-scoped + resources should be included for consideration in the backup. + nullable: true + type: boolean + includedClusterScopedResources: + description: IncludedClusterScopedResources is a slice of cluster-scoped + resource type names to include in the backup. If set to "*", all + cluster-scoped resource types are included. The default value is + empty, which means only related cluster-scoped resources are included. + items: + type: string + nullable: true + type: array + includedNamespaceScopedResources: + description: IncludedNamespaceScopedResources is a slice of namespace-scoped + resource type names to include in the backup. The default value + is "*". + items: + type: string + nullable: true + type: array + includedNamespaces: + description: IncludedNamespaces is a slice of namespace names to include + objects from. If empty, all namespaces are included. + items: + type: string + nullable: true + type: array + includedResources: + description: IncludedResources is a slice of resource names to include + in the backup. If empty, all resources are included. + items: + type: string + nullable: true + type: array + itemOperationTimeout: + description: ItemOperationTimeout specifies the time used to wait + for asynchronous BackupItemAction operations The default value is + 1 hour. + type: string + labelSelector: + description: LabelSelector is a metav1.LabelSelector to filter with + when adding individual objects to the backup. If empty or nil, all + objects are included. Optional. + nullable: true + properties: + matchExpressions: + description: matchExpressions is a list of label selector requirements. + The requirements are ANDed. + items: + description: A label selector requirement is a selector that + contains values, a key, and an operator that relates the key + and values. + properties: + key: + description: key is the label key that the selector applies + to. + type: string + operator: + description: operator represents a key's relationship to + a set of values. Valid operators are In, NotIn, Exists + and DoesNotExist. + type: string + values: + description: values is an array of string values. If the + operator is In or NotIn, the values array must be non-empty. + If the operator is Exists or DoesNotExist, the values + array must be empty. This array is replaced during a strategic + merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. A single + {key,value} in the matchLabels map is equivalent to an element + of matchExpressions, whose key field is "key", the operator + is "In", and the values array contains only "value". The requirements + are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic + metadata: + properties: + labels: + additionalProperties: + type: string + type: object + type: object + orLabelSelectors: + description: OrLabelSelectors is list of metav1.LabelSelector to filter + with when adding individual objects to the backup. If multiple provided + they will be joined by the OR operator. LabelSelector as well as + OrLabelSelectors cannot co-exist in backup request, only one of + them can be used. + items: + description: A label selector is a label query over a set of resources. + The result of matchLabels and matchExpressions are ANDed. An empty + label selector matches all objects. A null label selector matches + no objects. + properties: + matchExpressions: + description: matchExpressions is a list of label selector requirements. + The requirements are ANDed. + items: + description: A label selector requirement is a selector that + contains values, a key, and an operator that relates the + key and values. + properties: + key: + description: key is the label key that the selector applies + to. + type: string + operator: + description: operator represents a key's relationship + to a set of values. Valid operators are In, NotIn, Exists + and DoesNotExist. + type: string + values: + description: values is an array of string values. If the + operator is In or NotIn, the values array must be non-empty. + If the operator is Exists or DoesNotExist, the values + array must be empty. This array is replaced during a + strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. A single + {key,value} in the matchLabels map is equivalent to an element + of matchExpressions, whose key field is "key", the operator + is "In", and the values array contains only "value". The requirements + are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic + nullable: true + type: array + orderedResources: + additionalProperties: + type: string + description: OrderedResources specifies the backup order of resources + of specific Kind. The map key is the resource name and value is + a list of object names separated by commas. Each resource name has + format "namespace/objectname". For cluster resources, simply use + "objectname". + nullable: true + type: object + resourcePolicy: + description: ResourcePolicy specifies the referenced resource policies + that backup should follow + properties: + apiGroup: + description: APIGroup is the group for the resource being referenced. + If APIGroup is not specified, the specified Kind must be in + the core API group. For any other third-party types, APIGroup + is required. + type: string + kind: + description: Kind is the type of resource being referenced + type: string + name: + description: Name is the name of resource being referenced + type: string + required: + - kind + - name + type: object + x-kubernetes-map-type: atomic + snapshotMoveData: + description: SnapshotMoveData specifies whether snapshot data should + be moved + nullable: true + type: boolean + snapshotVolumes: + description: SnapshotVolumes specifies whether to take snapshots of + any PV's referenced in the set of objects included in the Backup. + nullable: true + type: boolean + storageLocation: + description: StorageLocation is a string containing the name of a + BackupStorageLocation where the backup should be stored. + type: string + ttl: + description: TTL is a time.Duration-parseable string describing how + long the Backup should be retained for. + type: string + volumeSnapshotLocations: + description: VolumeSnapshotLocations is a list containing names of + VolumeSnapshotLocations associated with this backup. + items: + type: string + type: array + type: object + status: + description: BackupStatus captures the current status of a Velero backup. + properties: + backupItemOperationsAttempted: + description: BackupItemOperationsAttempted is the total number of + attempted async BackupItemAction operations for this backup. + type: integer + backupItemOperationsCompleted: + description: BackupItemOperationsCompleted is the total number of + successfully completed async BackupItemAction operations for this + backup. + type: integer + backupItemOperationsFailed: + description: BackupItemOperationsFailed is the total number of async + BackupItemAction operations for this backup which ended with an + error. + type: integer + completionTimestamp: + description: CompletionTimestamp records the time a backup was completed. + Completion time is recorded even on failed backups. Completion time + is recorded before uploading the backup object. The server's time + is used for CompletionTimestamps + format: date-time + nullable: true + type: string + csiVolumeSnapshotsAttempted: + description: CSIVolumeSnapshotsAttempted is the total number of attempted + CSI VolumeSnapshots for this backup. + type: integer + csiVolumeSnapshotsCompleted: + description: CSIVolumeSnapshotsCompleted is the total number of successfully + completed CSI VolumeSnapshots for this backup. + type: integer + errors: + description: Errors is a count of all error messages that were generated + during execution of the backup. The actual errors are in the backup's + log file in object storage. + type: integer + expiration: + description: Expiration is when this Backup is eligible for garbage-collection. + format: date-time + nullable: true + type: string + failureReason: + description: FailureReason is an error that caused the entire backup + to fail. + type: string + formatVersion: + description: FormatVersion is the backup format version, including + major, minor, and patch version. + type: string + phase: + description: Phase is the current state of the Backup. + enum: + - New + - FailedValidation + - InProgress + - WaitingForPluginOperations + - WaitingForPluginOperationsPartiallyFailed + - Finalizing + - FinalizingPartiallyFailed + - Completed + - PartiallyFailed + - Failed + - Deleting + type: string + progress: + description: Progress contains information about the backup's execution + progress. Note that this information is best-effort only -- if Velero + fails to update it during a backup for any reason, it may be inaccurate/stale. + nullable: true + properties: + itemsBackedUp: + description: ItemsBackedUp is the number of items that have actually + been written to the backup tarball so far. + type: integer + totalItems: + description: TotalItems is the total number of items to be backed + up. This number may change throughout the execution of the backup + due to plugins that return additional related items to back + up, the velero.io/exclude-from-backup label, and various other + filters that happen as items are processed. + type: integer + type: object + startTimestamp: + description: StartTimestamp records the time a backup was started. + Separate from CreationTimestamp, since that value changes on restores. + The server's time is used for StartTimestamps + format: date-time + nullable: true + type: string + validationErrors: + description: ValidationErrors is a slice of all validation errors + (if applicable). + items: + type: string + nullable: true + type: array + version: + description: 'Version is the backup format major version. Deprecated: + Please see FormatVersion' + type: integer + volumeSnapshotsAttempted: + description: VolumeSnapshotsAttempted is the total number of attempted + volume snapshots for this backup. + type: integer + volumeSnapshotsCompleted: + description: VolumeSnapshotsCompleted is the total number of successfully + completed volume snapshots for this backup. + type: integer + warnings: + description: Warnings is a count of all warning messages that were + generated during execution of the backup. The actual warnings are + in the backup's log file in object storage. + type: integer + type: object + type: object + served: true + storage: true diff --git a/internal/common/constant/constant.go b/internal/common/constant/constant.go index c3748d7..b5a6dec 100644 --- a/internal/common/constant/constant.go +++ b/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 diff --git a/internal/common/function/function.go b/internal/common/function/function.go index 250e58b..040fdb9 100644 --- a/internal/common/function/function.go +++ b/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 } diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index dcec107..2907201 100644 --- a/internal/common/function/function_test.go +++ b/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,7 +326,7 @@ 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{ @@ -338,7 +334,7 @@ func TestCheckVeleroBackupLabels(t *testing.T) { 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") } diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 85456c2..e20d0c2 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -20,16 +20,17 @@ package controller import ( "context" "errors" - "time" + "reflect" "github.com/go-logr/logr" velerov1api "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" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -43,13 +44,15 @@ import ( // NonAdminBackupReconciler reconciles a NonAdminBackup object type NonAdminBackupReconciler struct { client.Client - Scheme *runtime.Scheme - Context context.Context + Scheme *runtime.Scheme + OADPNamespace string } const ( - nameField = "Name" - requeueTimeSeconds = 10 + phaseUpdateRequeue = "NonAdminBackup - Requeue after Phase Update" + conditionUpdateRequeue = "NonAdminBackup - Requeue after Condition Update" + phaseUpdateError = "Failed to update NonAdminBackup Phase" + conditionUpdateError = "Failed to update NonAdminBackup Condition" ) // +kubebuilder:rbac:groups=nac.oadp.openshift.io,resources=nonadminbackups,verbs=get;list;watch;create;update;patch;delete @@ -59,66 +62,56 @@ const ( // +kubebuilder:rbac:groups=velero.io,resources=backups,verbs=get;list;watch;create;update;patch // Reconcile is part of the main kubernetes reconciliation loop which aims to -// move the current state of the cluster closer to the desired state. -// TODO(user): Modify the Reconcile function to compare the state specified by -// the NonAdminBackup object against the actual cluster state, and then -// perform operations to make the cluster state reflect the state specified by -// the user. -// -// For more details, check Reconcile and its Result here: -// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.0/pkg/reconcile +// move the current state of the cluster closer to the desired state, +// defined in NonAdminBackup object Spec. func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - rLog := log.FromContext(ctx) - logger := rLog.WithValues("NonAdminBackup", req.NamespacedName) - logger.V(1).Info(">>> Reconcile NonAdminBackup - loop start") + logger := log.FromContext(ctx) + logger.V(1).Info("NonAdminBackup Reconcile start") // Get the NonAdminBackup object nab := nacv1alpha1.NonAdminBackup{} err := r.Get(ctx, req.NamespacedName, &nab) - - // Bail out when the Non Admin Backup reconcile was triggered, when the NAB got deleted - // Reconcile loop was triggered when Velero Backup object got updated and NAB isn't there if err != nil { if apierrors.IsNotFound(err) { - logger.V(1).Info("Non existing NonAdminBackup CR", nameField, req.Name, constant.NameSpaceString, req.Namespace) + logger.V(1).Info(err.Error()) return ctrl.Result{}, nil } - logger.Error(err, "Unable to fetch NonAdminBackup CR", nameField, req.Name, constant.NameSpaceString, req.Namespace) + logger.Error(err, "Unable to fetch NonAdminBackup") return ctrl.Result{}, err } - reconcileExit, reconcileRequeue, reconcileErr := r.InitNonAdminBackup(ctx, rLog, &nab) + reconcileExit, reconcileRequeue, reconcileErr := r.Init(ctx, logger, &nab) if reconcileRequeue { - return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + return ctrl.Result{Requeue: true}, reconcileErr } else if reconcileExit && reconcileErr != nil { - return ctrl.Result{}, reconcile.TerminalError(reconcileErr) + return ctrl.Result{}, reconcileErr } else if reconcileExit { return ctrl.Result{}, nil } - reconcileExit, reconcileRequeue, reconcileErr = r.ValidateVeleroBackupSpec(ctx, rLog, &nab) + reconcileExit, reconcileRequeue, reconcileErr = r.ValidateSpec(ctx, logger, &nab) if reconcileRequeue { - return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + return ctrl.Result{Requeue: true}, reconcileErr } else if reconcileExit && reconcileErr != nil { - return ctrl.Result{}, reconcile.TerminalError(reconcileErr) + return ctrl.Result{}, reconcileErr } else if reconcileExit { return ctrl.Result{}, nil } - reconcileExit, reconcileRequeue, reconcileErr = r.CreateVeleroBackupSpec(ctx, rLog, &nab) + reconcileExit, reconcileRequeue, reconcileErr = r.UpdateSpecStatus(ctx, logger, &nab) if reconcileRequeue { - return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + return ctrl.Result{Requeue: true}, reconcileErr } else if reconcileExit && reconcileErr != nil { - return ctrl.Result{}, reconcile.TerminalError(reconcileErr) + return ctrl.Result{}, reconcileErr } else if reconcileExit { return ctrl.Result{}, nil } - logger.V(1).Info(">>> Reconcile NonAdminBackup - loop end") + logger.V(1).Info("NonAdminBackup Reconcile exit") return ctrl.Result{}, nil } -// InitNonAdminBackup sets the New Phase on a NonAdminBackup object if it is not already set. +// Init initializes the Status.Phase from the NonAdminBackup. // // Parameters: // @@ -128,30 +121,29 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque // // The function checks if the Phase of the NonAdminBackup object is empty. // If it is empty, it sets the Phase to "New". -// It then returns boolean values indicating whether the reconciliation loop should requeue -// and whether the status was updated. -func (r *NonAdminBackupReconciler) InitNonAdminBackup(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := logrLogger.WithValues("InitNonAdminBackup", nab.Namespace) - // Set initial Phase - if nab.Status.Phase == constant.EmptyString { - // Phase: New - updatedStatus, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseNew) +// It then returns boolean values indicating whether the reconciliation loop should requeue or exit +// and error value whether the status was updated successfully. +func (r *NonAdminBackupReconciler) Init(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + logger := logrLogger - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: New", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdate - } - - if updatedStatus { - logger.V(1).Info("NonAdminBackup CR - Requeue after Phase Update") + if nab.Status.Phase == constant.EmptyString { + updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseNew) + if updated { + if err := r.Status().Update(ctx, nab); err != nil { + logger.Error(err, phaseUpdateError) + return true, false, err + } + + logger.V(1).Info(phaseUpdateRequeue) return false, true, nil } } + logger.V(1).Info("NonAdminBackup Phase already initialized") return false, false, nil } -// ValidateVeleroBackupSpec validates the VeleroBackup Spec from the NonAdminBackup. +// ValidateSpec validates the Spec from the NonAdminBackup. // // Parameters: // @@ -159,62 +151,69 @@ func (r *NonAdminBackupReconciler) InitNonAdminBackup(ctx context.Context, logrL // logrLogger: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. // -// The function attempts to get the BackupSpec from the NonAdminBackup object. -// If an error occurs during this process, the function sets the NonAdminBackup status to "BackingOff" -// and updates the corresponding condition accordingly. -// If the BackupSpec is invalid, the function sets the NonAdminBackup condition to "InvalidBackupSpec". -// If the BackupSpec is valid, the function sets the NonAdminBackup condition to "BackupAccepted". -func (r *NonAdminBackupReconciler) ValidateVeleroBackupSpec(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := logrLogger.WithValues("ValidateVeleroBackupSpec", nab.Namespace) +// The function validates the Spec from the NonAdminBackup object. +// If the BackupSpec is invalid, the function sets the NonAdminBackup phase to "BackingOff". +// If the BackupSpec is invalid, the function sets the NonAdminBackup condition Accepted to "False". +// If the BackupSpec is valid, the function sets the NonAdminBackup condition Accepted to "True". +func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { + logger := logrLogger // Main Validation point for the VeleroBackup included in NonAdminBackup spec _, err := function.GetBackupSpecFromNonAdminBackup(nab) - if err != nil { - // Use errMsg if errMsgFromErr is not available, otherwise use errMsgFromErr - errMsg := "NonAdminBackup CR does not contain valid BackupSpec" - if errMsgFromErr := err.Error(); errMsgFromErr != "" { - errMsg = errMsgFromErr - } - logger.Error(err, errMsg) - - updatedStatus, errUpdateStatus := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseBackingOff) - if errUpdateStatus != nil { - logger.Error(errUpdateStatus, "Unable to set NonAdminBackup Phase: BackingOff", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdateStatus - } else if updatedStatus { - // We do not requeue - the State was set to BackingOff - return true, false, nil + updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseBackingOff) + if updated { + if updateErr := r.Status().Update(ctx, nab); updateErr != nil { + logger.Error(updateErr, phaseUpdateError) + return true, false, updateErr + } + + logger.V(1).Info(phaseUpdateRequeue) + return false, true, nil } - // Continue. VeleroBackup looks fine, setting Accepted condition - updatedCondition, errUpdateCondition := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionFalse, "InvalidBackupSpec", errMsg) - - if errUpdateCondition != nil { - logger.Error(errUpdateCondition, "Unable to set BackupAccepted Condition: False", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdateCondition - } else if updatedCondition { - return true, false, nil + updated = meta.SetStatusCondition(&nab.Status.Conditions, + metav1.Condition{ + Type: string(nacv1alpha1.NonAdminConditionAccepted), + Status: metav1.ConditionFalse, + Reason: "InvalidBackupSpec", + Message: err.Error(), + }, + ) + if updated { + if updateErr := r.Status().Update(ctx, nab); updateErr != nil { + logger.Error(updateErr, conditionUpdateError) + return true, false, updateErr + } } - // We do not requeue - this was an error from getting Spec from NAB - return true, false, err + logger.Error(err, "NonAdminBackup Spec is not valid") + return true, false, reconcile.TerminalError(err) } - updatedStatus, errUpdateStatus := function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "BackupAccepted", "backup accepted") - if errUpdateStatus != nil { - logger.Error(errUpdateStatus, "Unable to set BackupAccepted Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdateStatus - } else if updatedStatus { - // We do requeue - The VeleroBackup got accepted and next reconcile loop will continue - // with further work on the VeleroBackup such as creating it + updated := meta.SetStatusCondition(&nab.Status.Conditions, + metav1.Condition{ + Type: string(nacv1alpha1.NonAdminConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + }, + ) + if updated { + if err := r.Status().Update(ctx, nab); err != nil { + logger.Error(err, conditionUpdateError) + return true, false, err + } + + logger.V(1).Info(conditionUpdateRequeue) return false, true, nil } + logger.V(1).Info("NonAdminBackup Spec already validated") return false, false, nil } -// CreateVeleroBackupSpec creates or updates a Velero Backup object based on the provided NonAdminBackup object. +// UpdateSpecStatus updates the Spec and Status from the NonAdminBackup. // // Parameters: // @@ -222,31 +221,32 @@ func (r *NonAdminBackupReconciler) ValidateVeleroBackupSpec(ctx context.Context, // log: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. // -// The function generates a 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. +// 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) CreateVeleroBackupSpec(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := logrLogger.WithValues("CreateVeleroBackupSpec", nab.Namespace) +func (r *NonAdminBackupReconciler) UpdateSpecStatus(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{} - err := r.Get(ctx, client.ObjectKey{Namespace: constant.OadpNamespace, Name: veleroBackupName}, &veleroBackup) - - if err != nil && apierrors.IsNotFound(err) { + 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 { + if !apierrors.IsNotFound(err) { + veleroBackupLogger.Error(err, "Unable to fetch VeleroBackup") + return true, false, err + } // Create VeleroBackup - // Don't update phase nor conditions yet. - // Those will be updated when then Reconcile loop is triggered by the VeleroBackup object - logger.Info("No backup found", nameField, veleroBackupName) + veleroBackupLogger.Info("VeleroBackup not found") // We don't validate error here. // This was already validated in the ValidateVeleroBackupSpec backupSpec, errBackup := function.GetBackupSpecFromNonAdminBackup(nab) - if errBackup != nil { // Should never happen as it was already checked return true, false, errBackup @@ -255,65 +255,86 @@ func (r *NonAdminBackupReconciler) CreateVeleroBackupSpec(ctx context.Context, l veleroBackup = velerov1api.Backup{ ObjectMeta: metav1.ObjectMeta{ Name: veleroBackupName, - Namespace: constant.OadpNamespace, + Namespace: r.OADPNamespace, }, Spec: *backupSpec, } - } else if err != nil { - logger.Error(err, "Unable to fetch VeleroBackup") - return true, false, err - } else { - // 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. - logger.Info("Backup already exists, updating NonAdminBackup status", nameField, veleroBackupName) - updatedNab, errBackupUpdate := function.UpdateNonAdminBackupFromVeleroBackup(ctx, r.Client, logger, nab, &veleroBackup) - // Regardless if the status was updated or not, we should not - // requeue here as it was only status update. - if errBackupUpdate != nil { - return true, false, errBackupUpdate - } else if updatedNab { - logger.V(1).Info("NonAdminBackup CR - Rqueue after Status Update") - return false, true, nil + + // Ensure labels are set for the Backup object + existingLabels := veleroBackup.Labels + naManagedLabels := function.AddNonAdminLabels(existingLabels) + veleroBackup.Labels = naManagedLabels + + // Ensure annotations are set for the Backup object + existingAnnotations := veleroBackup.Annotations + ownerUUID := string(nab.ObjectMeta.UID) + nabManagedAnnotations := function.AddNonAdminBackupAnnotations(nab.Namespace, nab.Name, ownerUUID, existingAnnotations) + veleroBackup.Annotations = nabManagedAnnotations + + err = r.Create(ctx, &veleroBackup) + if err != nil { + veleroBackupLogger.Error(err, "Failed to create VeleroBackup") + return true, false, err } - return true, false, nil + veleroBackupLogger.Info("VeleroBackup successfully created") } - // Ensure labels are set for the Backup object - existingLabels := veleroBackup.Labels - naManagedLabels := function.AddNonAdminLabels(existingLabels) - veleroBackup.Labels = naManagedLabels - - // Ensure annotations are set for the Backup object - existingAnnotations := veleroBackup.Annotations - ownerUUID := string(nab.ObjectMeta.UID) - nabManagedAnnotations := function.AddNonAdminBackupAnnotations(nab.Namespace, nab.Name, ownerUUID, existingAnnotations) - veleroBackup.Annotations = nabManagedAnnotations + updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseCreated) + if updated { + if err := r.Status().Update(ctx, nab); err != nil { + logger.Error(err, phaseUpdateError) + return true, false, err + } - _, err = controllerutil.CreateOrPatch(ctx, r.Client, &veleroBackup, nil) - if err != nil { - logger.Error(err, "Failed to create backup", nameField, veleroBackupName) - return true, false, err + logger.V(1).Info(phaseUpdateRequeue) + return false, true, nil } - logger.Info("VeleroBackup successfully created", nameField, veleroBackupName) - _, errUpdate := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseCreated) - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set NonAdminBackup Phase: Created", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdate + updated = meta.SetStatusCondition(&nab.Status.Conditions, + metav1.Condition{ + Type: string(nacv1alpha1.NonAdminConditionQueued), + Status: metav1.ConditionTrue, + Reason: "BackupScheduled", + Message: "Created Velero Backup object", + }, + ) + if updated { + if err := r.Status().Update(ctx, nab); err != nil { + logger.Error(err, conditionUpdateError) + return true, false, err + } + + logger.V(1).Info(conditionUpdateRequeue) + return false, true, nil } - _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionAccepted, metav1.ConditionTrue, "Validated", "Valid Backup config") - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set BackupAccepted Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdate + + // 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") + 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 } - _, errUpdate = function.UpdateNonAdminBackupCondition(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminConditionQueued, metav1.ConditionTrue, "BackupScheduled", "Created Velero Backup object") - if errUpdate != nil { - logger.Error(errUpdate, "Unable to set BackupQueued Condition: True", nameField, nab.Name, constant.NameSpaceString, nab.Namespace) - return true, false, errUpdate + updated = updateNonAdminBackupVeleroBackupSpec(&nab.Spec, &veleroBackup) + if updated { + if err := r.Update(ctx, nab); err != nil { + veleroBackupLogger.Error(err, "NonAdminBackup BackupSpec - Failed to update") + return true, false, err + } + + logger.V(1).Info("NonAdminBackup - Requeue after Spec Update") + return false, true, nil } + logger.V(1).Info("NonAdminBackup VeleroBackupSpec and VeleroBackupStatus already up to date") return false, false, nil } @@ -325,9 +346,46 @@ func (r *NonAdminBackupReconciler) SetupWithManager(mgr ctrl.Manager) error { WithEventFilter(predicate.CompositePredicate{ NonAdminBackupPredicate: predicate.NonAdminBackupPredicate{}, VeleroBackupPredicate: predicate.VeleroBackupPredicate{ - OadpVeleroNamespace: constant.OadpNamespace, + OadpVeleroNamespace: r.OADPNamespace, }, - Context: r.Context, }). Complete(r) } + +// updateNonAdminPhase sets the phase in NonAdminBackup object status and returns true +// if the phase is changed by this call. +func updateNonAdminPhase(phase *nacv1alpha1.NonAdminBackupPhase, newPhase nacv1alpha1.NonAdminBackupPhase) bool { + // Ensure phase is valid + if newPhase == constant.EmptyString { + return false + } + + if *phase == newPhase { + return false + } + + *phase = newPhase + return true +} + +// 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 { + if !reflect.DeepEqual(status.VeleroBackupStatus, &veleroBackup.Status) || status.VeleroBackupName != veleroBackup.Name || status.VeleroBackupNamespace != veleroBackup.Namespace { + status.VeleroBackupStatus = veleroBackup.Status.DeepCopy() + status.VeleroBackupName = veleroBackup.Name + status.VeleroBackupNamespace = veleroBackup.Namespace + return true + } + 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 +} diff --git a/internal/controller/nonadminbackup_controller_test.go b/internal/controller/nonadminbackup_controller_test.go index 551e3e4..5cfeabf 100644 --- a/internal/controller/nonadminbackup_controller_test.go +++ b/internal/controller/nonadminbackup_controller_test.go @@ -18,66 +18,654 @@ package controller import ( "context" + "fmt" + "reflect" + "strings" + "time" - ginkgov2 "github.com/onsi/ginkgo/v2" + "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/api/errors" + "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/reconcile" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/migtools/oadp-non-admin/internal/common/function" ) -var _ = ginkgov2.Describe("NonAdminBackup Controller", func() { - ginkgov2.Context("When reconciling a resource", func() { - const resourceName = "test-resource" +const ( + testNonAdminBackupName = "test-non-admin-backup" + placeholder = "PLACEHOLDER" +) + +type nonAdminBackupSingleReconcileScenario struct { + resultError error + priorStatus *nacv1alpha1.NonAdminBackupStatus + spec nacv1alpha1.NonAdminBackupSpec + ExpectedStatus nacv1alpha1.NonAdminBackupStatus + result reconcile.Result + createVeleroBackup bool +} + +type nonAdminBackupFullReconcileScenario struct { + spec nacv1alpha1.NonAdminBackupSpec + status nacv1alpha1.NonAdminBackupStatus +} + +func buildTestNonAdminBackup(namespace string, spec nacv1alpha1.NonAdminBackupSpec) *nacv1alpha1.NonAdminBackup { + return &nacv1alpha1.NonAdminBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: testNonAdminBackupName, + Namespace: namespace, + }, + Spec: spec, + } +} + +func checkTestNonAdminBackupStatus(nonAdminBackup *nacv1alpha1.NonAdminBackup, expectedStatus nacv1alpha1.NonAdminBackupStatus, nonAdminNamespaceName string, oadpNamespaceName string) error { + if nonAdminBackup.Status.Phase != expectedStatus.Phase { + return fmt.Errorf("NonAdminBackup Status Phase %v is not equal to expected %v", nonAdminBackup.Status.Phase, expectedStatus.Phase) + } + veleroBackupName := expectedStatus.VeleroBackupName + if expectedStatus.VeleroBackupName == placeholder { + veleroBackupName = function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName) + } + if nonAdminBackup.Status.VeleroBackupName != veleroBackupName { + return fmt.Errorf("NonAdminBackup Status VeleroBackupName %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackupName, veleroBackupName) + } + veleroBackupNamespace := expectedStatus.VeleroBackupNamespace + if expectedStatus.VeleroBackupNamespace == placeholder { + veleroBackupNamespace = oadpNamespaceName + } + if nonAdminBackup.Status.VeleroBackupNamespace != veleroBackupNamespace { + return fmt.Errorf("NonAdminBackup Status VeleroBackupNamespace %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackupNamespace, veleroBackupNamespace) + } + if !reflect.DeepEqual(nonAdminBackup.Status.VeleroBackupStatus, expectedStatus.VeleroBackupStatus) { + return fmt.Errorf("NonAdminBackup Status VeleroBackupStatus %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackupStatus, expectedStatus.VeleroBackupStatus) + } + + if len(nonAdminBackup.Status.Conditions) != len(expectedStatus.Conditions) { + return fmt.Errorf("NonAdminBackup Status has %v Condition(s), expected to have %v", len(nonAdminBackup.Status.Conditions), len(expectedStatus.Conditions)) + } + for index := range nonAdminBackup.Status.Conditions { + if nonAdminBackup.Status.Conditions[index].Type != expectedStatus.Conditions[index].Type { + return fmt.Errorf("NonAdminBackup Status Conditions [%v] Type %v is not equal to expected %v", index, nonAdminBackup.Status.Conditions[index].Type, expectedStatus.Conditions[index].Type) + } + if nonAdminBackup.Status.Conditions[index].Status != expectedStatus.Conditions[index].Status { + return fmt.Errorf("NonAdminBackup Status Conditions [%v] Status %v is not equal to expected %v", index, nonAdminBackup.Status.Conditions[index].Status, expectedStatus.Conditions[index].Status) + } + if nonAdminBackup.Status.Conditions[index].Reason != expectedStatus.Conditions[index].Reason { + return fmt.Errorf("NonAdminBackup Status Conditions [%v] Reason %v is not equal to expected %v", index, nonAdminBackup.Status.Conditions[index].Reason, expectedStatus.Conditions[index].Reason) + } + if !strings.Contains(nonAdminBackup.Status.Conditions[index].Message, expectedStatus.Conditions[index].Message) { + return fmt.Errorf("NonAdminBackup Status Conditions [%v] Message %v does not contain expected message %v", index, nonAdminBackup.Status.Conditions[index].Message, expectedStatus.Conditions[index].Message) + } + } + return nil +} - ctx := context.Background() +func createTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oadpNamespaceName string) error { + nonAdminNamespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: nonAdminNamespaceName, + }, + } + err := k8sClient.Create(ctx, nonAdminNamespace) + if err != nil { + return err + } - typeNamespacedName := types.NamespacedName{ - Name: resourceName, - Namespace: "default", // TODO(user):Modify as needed + oadpNamespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: oadpNamespaceName, + }, + } + return k8sClient.Create(ctx, oadpNamespace) +} + +func deleteTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oadpNamespaceName string) error { + oadpNamespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: oadpNamespaceName, + }, + } + err := k8sClient.Delete(ctx, oadpNamespace) + if err != nil { + return err + } + + nonAdminNamespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: nonAdminNamespaceName, + }, + } + return k8sClient.Delete(ctx, nonAdminNamespace) +} + +var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile function", func() { + var ( + ctx = context.Background() + nonAdminNamespaceName = "" + oadpNamespaceName = "" + counter = 0 + updateTestScenario = func() { + counter++ + nonAdminNamespaceName = fmt.Sprintf("test-nonadminbackup-reconcile-%v", counter) + oadpNamespaceName = nonAdminNamespaceName + "-oadp" + } + ) + + ginkgo.AfterEach(func() { + nonAdminBackup := &nacv1alpha1.NonAdminBackup{} + if k8sClient.Get( + ctx, + types.NamespacedName{ + Name: testNonAdminBackupName, + Namespace: nonAdminNamespaceName, + }, + nonAdminBackup, + ) == nil { + gomega.Expect(k8sClient.Delete(ctx, nonAdminBackup)).To(gomega.Succeed()) } - nonadminbackup := &nacv1alpha1.NonAdminBackup{} - ginkgov2.BeforeEach(func() { - ginkgov2.By("creating the custom resource for the Kind NonAdminBackup") - err := k8sClient.Get(ctx, typeNamespacedName, nonadminbackup) - if err != nil && errors.IsNotFound(err) { - resource := &nacv1alpha1.NonAdminBackup{ + gomega.Expect(deleteTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + }) + + ginkgo.DescribeTable("Reconcile triggered by NonAdminBackup Delete event", + func(scenario nonAdminBackupSingleReconcileScenario) { + updateTestScenario() + + gomega.Expect(createTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + + result, err := (&NonAdminBackupReconciler{ + Client: k8sClient, + Scheme: testEnv.Scheme, + }).Reconcile( + context.Background(), + reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: nonAdminNamespaceName, + Name: testNonAdminBackupName, + }}, + ) + + gomega.Expect(result).To(gomega.Equal(scenario.result)) + gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) + }, + ginkgo.Entry("Should exit", nonAdminBackupSingleReconcileScenario{ + result: reconcile.Result{}, + }), + ) + + ginkgo.DescribeTable("Reconcile triggered by NonAdminBackup Create/Update events and by Requeue", + func(scenario nonAdminBackupSingleReconcileScenario) { + updateTestScenario() + + gomega.Expect(createTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + + nonAdminBackup := buildTestNonAdminBackup(nonAdminNamespaceName, scenario.spec) + gomega.Expect(k8sClient.Create(ctx, nonAdminBackup)).To(gomega.Succeed()) + + if scenario.createVeleroBackup { + veleroBackup := &v1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: resourceName, - Namespace: "default", + Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName), + Namespace: oadpNamespaceName, + }, + Spec: v1.BackupSpec{ + IncludedNamespaces: []string{nonAdminNamespaceName}, }, - // TODO(user): Specify other spec details if needed. } - gomega.Expect(k8sClient.Create(ctx, resource)).To(gomega.Succeed()) + gomega.Expect(k8sClient.Create(ctx, veleroBackup)).To(gomega.Succeed()) } - }) - - ginkgov2.AfterEach(func() { - // TODO(user): Cleanup logic after each test, like removing the resource instance. - resource := &nacv1alpha1.NonAdminBackup{} - err := k8sClient.Get(ctx, typeNamespacedName, resource) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - - ginkgov2.By("Cleanup the specific resource instance NonAdminBackup") - gomega.Expect(k8sClient.Delete(ctx, resource)).To(gomega.Succeed()) - }) - ginkgov2.It("should successfully reconcile the resource", func() { - ginkgov2.By("Reconciling the created resource") - controllerReconciler := &NonAdminBackupReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), + + if scenario.priorStatus != nil { + nonAdminBackup.Status = *scenario.priorStatus + if nonAdminBackup.Status.VeleroBackupName == placeholder { + nonAdminBackup.Status.VeleroBackupName = function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName) + } + if nonAdminBackup.Status.VeleroBackupNamespace == placeholder { + nonAdminBackup.Status.VeleroBackupNamespace = oadpNamespaceName + } + gomega.Expect(k8sClient.Status().Update(ctx, nonAdminBackup)).To(gomega.Succeed()) } + // easy hack to test that only one update call happens per reconcile + // priorResourceVersion, err := strconv.Atoi(nonAdminBackup.ResourceVersion) + // gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) - _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - // TODO(user): Add more specific assertions depending on your controller's reconciliation logic. - // Example: If you expect a certain status condition after reconciliation, verify it here. - }) + result, err := (&NonAdminBackupReconciler{ + Client: k8sClient, + Scheme: testEnv.Scheme, + OADPNamespace: oadpNamespaceName, + }).Reconcile( + context.Background(), + reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: nonAdminNamespaceName, + Name: testNonAdminBackupName, + }}, + ) + gomega.Expect(result).To(gomega.Equal(scenario.result)) + if scenario.resultError == nil { + gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) + } else { + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(err.Error()).To(gomega.ContainSubstring(scenario.resultError.Error())) + } + + gomega.Expect(k8sClient.Get( + ctx, + types.NamespacedName{ + Name: testNonAdminBackupName, + Namespace: nonAdminNamespaceName, + }, + nonAdminBackup, + )).To(gomega.Succeed()) + + gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.ExpectedStatus, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + if scenario.priorStatus != nil { + if len(scenario.priorStatus.VeleroBackupName) > 0 { + gomega.Expect(reflect.DeepEqual( + nonAdminBackup.Spec.BackupSpec, + &v1.BackupSpec{ + IncludedNamespaces: []string{ + nonAdminNamespaceName, + }, + }, + )).To(gomega.BeTrue()) + } + } + + // easy hack to test that only one update call happens per reconcile + // currentResourceVersion, err := strconv.Atoi(nonAdminBackup.ResourceVersion) + // gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) + // gomega.Expect(currentResourceVersion - priorResourceVersion).To(gomega.Equal(1)) + }, + ginkgo.Entry("When triggered by NonAdminBackup Create event, should update NonAdminBackup phase to new and Requeue", nonAdminBackupSingleReconcileScenario{ + ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseNew, + }, + result: reconcile.Result{Requeue: true}, + }), + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new), should update NonAdminBackup Condition to Accepted True and Requeue", nonAdminBackupSingleReconcileScenario{ + spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &v1.BackupSpec{}, + }, + priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseNew, + }, + ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseNew, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + }, + }, + }, + result: reconcile.Result{Requeue: true}, + }), + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True), should update NonAdminBackup phase to created and Requeue", nonAdminBackupSingleReconcileScenario{ + spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &v1.BackupSpec{}, + }, + priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseNew, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + LastTransitionTime: metav1.NewTime(time.Now()), + }, + }, + }, + ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + // TODO should not have VeleroBackupName and VeleroBackupNamespace? + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + }, + }, + }, + result: reconcile.Result{Requeue: true}, + }), + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase created; Conditions Accepted True), should update NonAdminBackup Condition to Queued True and Requeue", nonAdminBackupSingleReconcileScenario{ + createVeleroBackup: true, + spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &v1.BackupSpec{}, + }, + priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + LastTransitionTime: metav1.NewTime(time.Now()), + }, + }, + }, + ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + // TODO should not have VeleroBackupName and VeleroBackupNamespace? + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + }, + { + Type: "Queued", + Status: metav1.ConditionTrue, + Reason: "BackupScheduled", + Message: "Created Velero Backup object", + }, + }, + }, + result: reconcile.Result{Requeue: true}, + }), + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase created; Conditions Accepted True, Queued True), should update NonAdminBackup VeleroBackupStatus and Requeue", nonAdminBackupSingleReconcileScenario{ + createVeleroBackup: true, + spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &v1.BackupSpec{}, + }, + priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + LastTransitionTime: metav1.NewTime(time.Now()), + }, + { + Type: "Queued", + Status: metav1.ConditionTrue, + Reason: "BackupScheduled", + Message: "Created Velero Backup object", + LastTransitionTime: metav1.NewTime(time.Now()), + }, + }, + }, + ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + VeleroBackupName: placeholder, + VeleroBackupNamespace: placeholder, + VeleroBackupStatus: &v1.BackupStatus{}, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + }, + { + Type: "Queued", + Status: metav1.ConditionTrue, + Reason: "BackupScheduled", + Message: "Created Velero Backup object", + }, + }, + }, + result: reconcile.Result{Requeue: true}, + }), + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase created; Conditions Accepted True, Queued True; VeleroBackupStatus), should update NonAdminBackup spec BackupSpec and Requeue", nonAdminBackupSingleReconcileScenario{ + createVeleroBackup: true, + spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &v1.BackupSpec{}, + }, + priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + VeleroBackupName: placeholder, + VeleroBackupNamespace: placeholder, + VeleroBackupStatus: &v1.BackupStatus{}, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + LastTransitionTime: metav1.NewTime(time.Now()), + }, + { + Type: "Queued", + Status: metav1.ConditionTrue, + Reason: "BackupScheduled", + Message: "Created Velero Backup object", + LastTransitionTime: metav1.NewTime(time.Now()), + }, + }, + }, + ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + VeleroBackupName: placeholder, + VeleroBackupNamespace: placeholder, + VeleroBackupStatus: &v1.BackupStatus{}, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + }, + { + Type: "Queued", + Status: metav1.ConditionTrue, + Reason: "BackupScheduled", + Message: "Created Velero Backup object", + }, + }, + }, + // TODO should not exit? + result: reconcile.Result{Requeue: true}, + }), + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new) [invalid spec], should update NonAdminBackup phase to BackingOff and Requeue", nonAdminBackupSingleReconcileScenario{ + spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &v1.BackupSpec{ + IncludedNamespaces: []string{"not-valid"}, + }, + }, + priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseNew, + }, + ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseBackingOff, + }, + result: reconcile.Result{Requeue: true}, + }), + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase BackingOff), should update NonAdminBackup Condition to Accepted False and stop with terminal error", nonAdminBackupSingleReconcileScenario{ + // TODO this validates spec again... + spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &v1.BackupSpec{ + IncludedNamespaces: []string{"not-valid"}, + }, + }, + priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseBackingOff, + }, + ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseBackingOff, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionFalse, + Reason: "InvalidBackupSpec", + Message: "spec.backupSpec.IncludedNamespaces can not contain namespaces other than:", + }, + }, + }, + resultError: reconcile.TerminalError(fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other than: ")), + }), + ) +}) + +var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", func() { + var ( + ctx context.Context + cancel context.CancelFunc + nonAdminNamespaceName = "" + oadpNamespaceName = "" + counter = 0 + updateTestScenario = func() { + ctx, cancel = context.WithCancel(context.Background()) + counter++ + nonAdminNamespaceName = fmt.Sprintf("test-nonadminbackup-reconcile-full-%v", counter) + oadpNamespaceName = nonAdminNamespaceName + "-oadp" + } + ) + + ginkgo.AfterEach(func() { + gomega.Expect(deleteTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + + cancel() + // wait cancel + time.Sleep(1 * time.Second) }) + + ginkgo.DescribeTable("full reconcile loop", + func(scenario nonAdminBackupFullReconcileScenario) { + updateTestScenario() + + gomega.Expect(createTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + + k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: k8sClient.Scheme(), + }) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + err = (&NonAdminBackupReconciler{ + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + OADPNamespace: oadpNamespaceName, + }).SetupWithManager(k8sManager) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + go func() { + defer ginkgo.GinkgoRecover() + err = k8sManager.Start(ctx) + gomega.Expect(err).ToNot(gomega.HaveOccurred(), "failed to run manager") + }() + // wait manager start + time.Sleep(1 * time.Second) + + ginkgo.By("Waiting Reconcile of create event") + nonAdminBackup := buildTestNonAdminBackup(nonAdminNamespaceName, scenario.spec) + gomega.Expect(k8sClient.Create(ctx, nonAdminBackup)).To(gomega.Succeed()) + // wait NAB reconcile + time.Sleep(1 * time.Second) + + ginkgo.By("Fetching NonAdminBackup after Reconcile") + gomega.Expect(k8sClient.Get( + ctx, + types.NamespacedName{ + Name: testNonAdminBackupName, + Namespace: nonAdminNamespaceName, + }, + nonAdminBackup, + )).To(gomega.Succeed()) + + ginkgo.By("Validating NonAdminBackup Status") + gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.status, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + + if len(scenario.status.VeleroBackupName) > 0 { + ginkgo.By("Validating NonAdminBackup Spec") + gomega.Expect(reflect.DeepEqual( + nonAdminBackup.Spec.BackupSpec, + &v1.BackupSpec{ + IncludedNamespaces: []string{ + nonAdminNamespaceName, + }, + }, + )).To(gomega.BeTrue()) + + ginkgo.By("Simulating VeleroBackup update to finished state") + veleroBackup := &v1.Backup{} + gomega.Expect(k8sClient.Get( + ctx, + types.NamespacedName{ + Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName), + Namespace: oadpNamespaceName, + }, + veleroBackup, + )).To(gomega.Succeed()) + veleroBackup.Status.Phase = v1.BackupPhaseCompleted + // TODO can not call .Status().Update() for veleroBackup object: backups.velero.io "name..." not found error + gomega.Expect(k8sClient.Update(ctx, veleroBackup)).To(gomega.Succeed()) + + gomega.Eventually(func() (bool, error) { + err := k8sClient.Get( + ctx, + types.NamespacedName{ + Name: testNonAdminBackupName, + Namespace: nonAdminNamespaceName, + }, + nonAdminBackup, + ) + if err != nil { + return false, err + } + return nonAdminBackup.Status.VeleroBackupStatus.Phase == v1.BackupPhaseCompleted, nil + }, 5*time.Second, 1*time.Second).Should(gomega.BeTrue()) + } + + ginkgo.By("Waiting Reconcile of delete event") + gomega.Expect(k8sClient.Delete(ctx, nonAdminBackup)).To(gomega.Succeed()) + time.Sleep(1 * time.Second) + }, + ginkgo.Entry("Should update NonAdminBackup until VeleroBackup completes and then delete it", nonAdminBackupFullReconcileScenario{ + spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &v1.BackupSpec{}, + }, + status: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + VeleroBackupName: placeholder, + VeleroBackupNamespace: placeholder, + VeleroBackupStatus: &v1.BackupStatus{}, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + }, + { + Type: "Queued", + Status: metav1.ConditionTrue, + Reason: "BackupScheduled", + Message: "Created Velero Backup object", + }, + }, + }, + }), + ginkgo.Entry("Should update NonAdminBackup until it invalidates and then delete it", nonAdminBackupFullReconcileScenario{ + spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &v1.BackupSpec{ + IncludedNamespaces: []string{"not-valid"}, + }, + }, + status: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseBackingOff, + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionFalse, + Reason: "InvalidBackupSpec", + Message: "spec.backupSpec.IncludedNamespaces can not contain namespaces other than:", + }, + }, + }, + }), + ) }) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index e1510ba..4f99475 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -22,8 +22,9 @@ import ( "runtime" "testing" - ginkgov2 "github.com/onsi/ginkgo/v2" + "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -42,17 +43,20 @@ var k8sClient client.Client var testEnv *envtest.Environment func TestControllers(t *testing.T) { - gomega.RegisterFailHandler(ginkgov2.Fail) + gomega.RegisterFailHandler(ginkgo.Fail) - ginkgov2.RunSpecs(t, "Controller Suite") + ginkgo.RunSpecs(t, "Controller Suite") } -var _ = ginkgov2.BeforeSuite(func() { - logf.SetLogger(zap.New(zap.WriteTo(ginkgov2.GinkgoWriter), zap.UseDevMode(true))) +var _ = ginkgo.BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(ginkgo.GinkgoWriter), zap.UseDevMode(true))) - ginkgov2.By("bootstrapping test environment") + ginkgo.By("bootstrapping test environment") testEnv = &envtest.Environment{ - CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, + CRDDirectoryPaths: []string{ + filepath.Join("..", "..", "config", "crd", "bases"), + filepath.Join("..", "..", "hack", "extra-crds"), + }, ErrorIfCRDPathMissing: true, // The BinaryAssetsDirectory is only required if you want to run the tests directly @@ -61,7 +65,7 @@ var _ = ginkgov2.BeforeSuite(func() { // Note that you must have the required binaries setup under the bin directory to perform // the tests directly. When we run make test it will be setup and used automatically. BinaryAssetsDirectory: filepath.Join("..", "..", "bin", "k8s", - fmt.Sprintf("1.29.0-%s-%s", runtime.GOOS, runtime.GOARCH)), + fmt.Sprintf("1.29.3-%s-%s", runtime.GOOS, runtime.GOARCH)), } var err error @@ -72,6 +76,8 @@ var _ = ginkgov2.BeforeSuite(func() { err = nacv1alpha1.AddToScheme(scheme.Scheme) gomega.Expect(err).NotTo(gomega.HaveOccurred()) + err = velerov1.AddToScheme(scheme.Scheme) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) // +kubebuilder:scaffold:scheme @@ -80,8 +86,8 @@ var _ = ginkgov2.BeforeSuite(func() { gomega.Expect(k8sClient).NotTo(gomega.BeNil()) }) -var _ = ginkgov2.AfterSuite(func() { - ginkgov2.By("tearing down the test environment") +var _ = ginkgo.AfterSuite(func() { + ginkgo.By("tearing down the test environment") err := testEnv.Stop() gomega.Expect(err).NotTo(gomega.HaveOccurred()) }) diff --git a/internal/handler/velerobackup_handler.go b/internal/handler/velerobackup_handler.go index d1fe4c1..ce484b9 100644 --- a/internal/handler/velerobackup_handler.go +++ b/internal/handler/velerobackup_handler.go @@ -41,17 +41,17 @@ func getVeleroBackupHandlerLogger(ctx context.Context, name, namespace string) l // Create event handler func (*VeleroBackupHandler) Create(ctx context.Context, evt event.CreateEvent, _ workqueue.RateLimitingInterface) { - nameSpace := evt.Object.GetNamespace() + namespace := evt.Object.GetNamespace() name := evt.Object.GetName() - logger := getVeleroBackupHandlerLogger(ctx, name, nameSpace) + logger := getVeleroBackupHandlerLogger(ctx, name, namespace) logger.V(1).Info("Received Create VeleroBackupHandler") } // Update event handler func (*VeleroBackupHandler) Update(ctx context.Context, evt event.UpdateEvent, q workqueue.RateLimitingInterface) { - nameSpace := evt.ObjectNew.GetNamespace() + namespace := evt.ObjectNew.GetNamespace() name := evt.ObjectNew.GetName() - logger := getVeleroBackupHandlerLogger(ctx, name, nameSpace) + logger := getVeleroBackupHandlerLogger(ctx, name, namespace) logger.V(1).Info("Received Update VeleroBackupHandler") annotations := evt.ObjectNew.GetAnnotations() diff --git a/internal/predicate/nonadminbackup_predicate.go b/internal/predicate/nonadminbackup_predicate.go index d3309a3..6597de5 100644 --- a/internal/predicate/nonadminbackup_predicate.go +++ b/internal/predicate/nonadminbackup_predicate.go @@ -39,9 +39,9 @@ func getNonAdminBackupPredicateLogger(ctx context.Context, name, namespace strin // Create event filter func (NonAdminBackupPredicate) Create(ctx context.Context, evt event.CreateEvent) bool { - nameSpace := evt.Object.GetNamespace() + namespace := evt.Object.GetNamespace() name := evt.Object.GetName() - logger := getNonAdminBackupPredicateLogger(ctx, name, nameSpace) + logger := getNonAdminBackupPredicateLogger(ctx, name, namespace) logger.V(1).Info("NonAdminBackupPredicate: Received Create event") if nonAdminBackup, ok := evt.Object.(*nacv1alpha1.NonAdminBackup); ok { if nonAdminBackup.Status.Phase == constant.EmptyString || nonAdminBackup.Status.Phase == nacv1alpha1.NonAdminBackupPhaseNew { @@ -55,10 +55,9 @@ func (NonAdminBackupPredicate) Create(ctx context.Context, evt event.CreateEvent // Update event filter func (NonAdminBackupPredicate) Update(ctx context.Context, evt event.UpdateEvent) bool { - // Do not reconcile on Status update - nameSpace := evt.ObjectNew.GetNamespace() + namespace := evt.ObjectNew.GetNamespace() name := evt.ObjectNew.GetName() - logger := getNonAdminBackupPredicateLogger(ctx, name, nameSpace) + logger := getNonAdminBackupPredicateLogger(ctx, name, namespace) logger.V(1).Info("NonAdminBackupPredicate: Received Update event") if evt.ObjectNew.GetGeneration() != evt.ObjectOld.GetGeneration() { @@ -73,7 +72,7 @@ func (NonAdminBackupPredicate) Update(ctx context.Context, evt event.UpdateEvent // New phase set, reconcile if oldPhase == constant.EmptyString && newPhase != constant.EmptyString { - logger.V(1).Info("NonAdminBsackupPredicate: Accepted Update event - phase change") + logger.V(1).Info("NonAdminBackupPredicate: Accepted Update event - phase change") return true } else if oldPhase == nacv1alpha1.NonAdminBackupPhaseNew && newPhase == nacv1alpha1.NonAdminBackupPhaseCreated { logger.V(1).Info("NonAdminBackupPredicate: Accepted Update event - phase created") @@ -88,18 +87,18 @@ func (NonAdminBackupPredicate) Update(ctx context.Context, evt event.UpdateEvent // Delete event filter func (NonAdminBackupPredicate) Delete(ctx context.Context, evt event.DeleteEvent) bool { - nameSpace := evt.Object.GetNamespace() + namespace := evt.Object.GetNamespace() name := evt.Object.GetName() - logger := getNonAdminBackupPredicateLogger(ctx, name, nameSpace) + logger := getNonAdminBackupPredicateLogger(ctx, name, namespace) logger.V(1).Info("NonAdminBackupPredicate: Accepted Delete event") return true } // Generic event filter func (NonAdminBackupPredicate) Generic(ctx context.Context, evt event.GenericEvent) bool { - nameSpace := evt.Object.GetNamespace() + namespace := evt.Object.GetNamespace() name := evt.Object.GetName() - logger := getNonAdminBackupPredicateLogger(ctx, name, nameSpace) + logger := getNonAdminBackupPredicateLogger(ctx, name, namespace) logger.V(1).Info("NonAdminBackupPredicate: Accepted Generic event") return true } diff --git a/internal/predicate/velerobackup_predicate.go b/internal/predicate/velerobackup_predicate.go index 6f436bd..22da1d3 100644 --- a/internal/predicate/velerobackup_predicate.go +++ b/internal/predicate/velerobackup_predicate.go @@ -20,7 +20,6 @@ import ( "context" "github.com/go-logr/logr" - velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/log" @@ -43,28 +42,25 @@ func getBackupPredicateLogger(ctx context.Context, name, namespace string) logr. // Create event filter func (veleroBackupPredicate VeleroBackupPredicate) Create(ctx context.Context, evt event.CreateEvent) bool { - if backup, ok := evt.Object.(*velerov1api.Backup); ok { - nameSpace := evt.Object.GetNamespace() - if nameSpace != veleroBackupPredicate.OadpVeleroNamespace { - return false - } + namespace := evt.Object.GetNamespace() + if namespace != veleroBackupPredicate.OadpVeleroNamespace { + return false + } - name := evt.Object.GetName() - logger := getBackupPredicateLogger(ctx, name, nameSpace) - logger.V(1).Info("VeleroBackupPredicate: Received Create event") + name := evt.Object.GetName() + logger := getBackupPredicateLogger(ctx, name, namespace) + logger.V(1).Info("VeleroBackupPredicate: Received Create event") - return function.CheckVeleroBackupLabels(backup) - } - return false + return function.CheckVeleroBackupLabels(evt.Object.GetLabels()) } // Update event filter func (veleroBackupPredicate VeleroBackupPredicate) Update(ctx context.Context, evt event.UpdateEvent) bool { - nameSpace := evt.ObjectNew.GetNamespace() + namespace := evt.ObjectNew.GetNamespace() name := evt.ObjectNew.GetName() - logger := getBackupPredicateLogger(ctx, name, nameSpace) + logger := getBackupPredicateLogger(ctx, name, namespace) logger.V(1).Info("VeleroBackupPredicate: Received Update event") - return nameSpace == veleroBackupPredicate.OadpVeleroNamespace + return namespace == veleroBackupPredicate.OadpVeleroNamespace } // Delete event filter