From e7ba69ad40df870e3f65b44c3bf7dd048707a0d7 Mon Sep 17 00:00:00 2001 From: Juan Manuel Parrilla Madrid Date: Mon, 6 Apr 2026 12:27:44 +0200 Subject: [PATCH 1/3] feat(CNTRLPLANE-2678): add HCPEtcdBackup controller to HyperShift Operator Implements the HCPEtcdBackup reconciler that orchestrates etcd snapshot and upload Jobs. The controller watches HCPEtcdBackup CRs, validates etcd health, manages temporary RBAC and NetworkPolicy resources for cross-namespace access, creates 3-container backup Jobs (fetch-certs, snapshot, upload), tracks Job status via pod termination messages, and enforces count-based retention of completed backups. Key design decisions: - findJobForBackup runs before findActiveJob serial guard to prevent the controller from rejecting its own Job on re-reconcile - BackupRejected is a terminal state to prevent backup accumulation when concurrent backups are attempted - BackoffLimit=0 to avoid race conditions where Kubernetes retries a pod after the controller has already cleaned up RBAC - Cleanup errors are propagated (not swallowed) so controller-runtime retries on transient failures, preventing leaked resources - --etcd-backup-max-count is validated to be at least 1 Adds EtcdBackupSucceeded condition type for HCP-to-HC status propagation. Controller is registered behind the HCPEtcdBackup feature gate. Co-Authored-By: Claude Opus 4.6 Signed-off-by: Juan Manuel Parrilla Madrid --- api/hypershift/v1beta1/etcdbackup_types.go | 9 +- .../v1beta1/hostedcluster_conditions.go | 5 + docs/content/reference/aggregated-docs.md | 5 + docs/content/reference/api.md | 5 + .../controllers/etcdbackup/reconciler.go | 936 ++++++++++++ .../controllers/etcdbackup/reconciler_test.go | 1259 +++++++++++++++++ .../hostedcluster/hostedcluster_controller.go | 1 + hypershift-operator/main.go | 20 + support/config/constants.go | 1 + .../hypershift/v1beta1/etcdbackup_types.go | 9 +- .../v1beta1/hostedcluster_conditions.go | 5 + 11 files changed, 2247 insertions(+), 8 deletions(-) create mode 100644 hypershift-operator/controllers/etcdbackup/reconciler.go create mode 100644 hypershift-operator/controllers/etcdbackup/reconciler_test.go diff --git a/api/hypershift/v1beta1/etcdbackup_types.go b/api/hypershift/v1beta1/etcdbackup_types.go index d667e2619515..d59ef4752a95 100644 --- a/api/hypershift/v1beta1/etcdbackup_types.go +++ b/api/hypershift/v1beta1/etcdbackup_types.go @@ -20,10 +20,11 @@ const ( // BackupCompleted indicates whether the etcd backup has completed. BackupCompleted ConditionType = "BackupCompleted" - BackupSucceededReason string = "BackupSucceeded" - BackupFailedReason string = "BackupFailed" - BackupAlreadyInProgressReason string = "BackupAlreadyInProgress" - EtcdUnhealthyReason string = "EtcdUnhealthy" + BackupSucceededReason string = "BackupSucceeded" + BackupFailedReason string = "BackupFailed" + BackupInProgressReason string = "BackupInProgress" + BackupRejectedReason string = "BackupRejected" + EtcdUnhealthyReason string = "EtcdUnhealthy" ) // HCPEtcdBackupStorageType is the type of storage for etcd backups. diff --git a/api/hypershift/v1beta1/hostedcluster_conditions.go b/api/hypershift/v1beta1/hostedcluster_conditions.go index c43bf81992f5..59e13f4cba67 100644 --- a/api/hypershift/v1beta1/hostedcluster_conditions.go +++ b/api/hypershift/v1beta1/hostedcluster_conditions.go @@ -195,6 +195,11 @@ const ( // recovery job was triggered. EtcdRecoveryActive ConditionType = "EtcdRecoveryActive" + // EtcdBackupSucceeded bubbles up from HCP. It indicates the result of the + // most recent etcd backup. True means the last backup completed successfully; + // False means a backup is in progress or the last backup failed. + EtcdBackupSucceeded ConditionType = "EtcdBackupSucceeded" + // ClusterSizeComputed indicates that a t-shirt size was computed for this HostedCluster. // The last transition time for this condition is used to manage how quickly transitions occur. ClusterSizeComputed = "ClusterSizeComputed" diff --git a/docs/content/reference/aggregated-docs.md b/docs/content/reference/aggregated-docs.md index 9dd25f9270a7..2f80e6cf3286 100644 --- a/docs/content/reference/aggregated-docs.md +++ b/docs/content/reference/aggregated-docs.md @@ -35407,6 +35407,11 @@ components like the konnectivity-agent workload.

EtcdAvailable bubbles up the same condition from HCP. It signals if etcd is available. A failure here often means a software bug or a non-stable cluster.

+

"EtcdBackupSucceeded"

+

EtcdBackupSucceeded bubbles up from HCP. It indicates the result of the +most recent etcd backup. True means the last backup completed successfully; +False means a backup is in progress or the last backup failed.

+

"EtcdRecoveryActive"

EtcdRecoveryActive indicates that the Etcd cluster is failing and the recovery job was triggered.

diff --git a/docs/content/reference/api.md b/docs/content/reference/api.md index 8d90e4566757..0b4345101814 100644 --- a/docs/content/reference/api.md +++ b/docs/content/reference/api.md @@ -5832,6 +5832,11 @@ components like the konnectivity-agent workload.

EtcdAvailable bubbles up the same condition from HCP. It signals if etcd is available. A failure here often means a software bug or a non-stable cluster.

+

"EtcdBackupSucceeded"

+

EtcdBackupSucceeded bubbles up from HCP. It indicates the result of the +most recent etcd backup. True means the last backup completed successfully; +False means a backup is in progress or the last backup failed.

+

"EtcdRecoveryActive"

EtcdRecoveryActive indicates that the Etcd cluster is failing and the recovery job was triggered.

diff --git a/hypershift-operator/controllers/etcdbackup/reconciler.go b/hypershift-operator/controllers/etcdbackup/reconciler.go new file mode 100644 index 000000000000..e30581830ebc --- /dev/null +++ b/hypershift-operator/controllers/etcdbackup/reconciler.go @@ -0,0 +1,936 @@ +package etcdbackup + +import ( + "context" + "fmt" + "sort" + "strings" + "time" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/hypershift-operator/featuregate" + supportconfig "github.com/openshift/hypershift/support/config" + "github.com/openshift/hypershift/support/releaseinfo" + hyperutil "github.com/openshift/hypershift/support/util" + + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/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/types" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/util/workqueue" + "k8s.io/utils/ptr" + + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +const ( + ControllerName = "hcpetcdbackup" + + // Labels used on backup Jobs. + LabelApp = "app" + LabelName = "etcd-backup" + labelHCP = "hypershift.openshift.io/hcp" + + // LabelBackupName is the label key for the backup CR name on Jobs. + LabelBackupName = "hypershift.openshift.io/backup-name" + // LabelHCPNamespace is the label key for the HCP namespace on Jobs. + LabelHCPNamespace = "hypershift.openshift.io/hcp-namespace" + + // pullSecretName is the name of the pull secret copied to HCP namespaces. + pullSecretName = "pull-secret" + + // RBACName is the name of the Role and RoleBinding created in HCP namespaces. + RBACName = "etcd-backup-job" + // NetworkPolicyName is the name of the NetworkPolicy created in HCP namespaces. + NetworkPolicyName = "allow-etcd-backup" + + // ServiceAccount name for backup Jobs in the HO namespace. + jobServiceAccountName = "etcd-backup-job" + + // Volume names. + volumeEtcdCerts = "etcd-certs" + volumeEtcdBackup = "etcd-backup" + volumeCredentials = "backup-credentials" + + // Mount paths. + mountPathEtcdCerts = "/etc/etcd-certs" + mountPathEtcdBackup = "/etc/etcd-backup" + mountPathCredentials = "/etc/etcd-backup-creds" + + requeueInterval = 10 * time.Second +) + +// HCPEtcdBackupReconciler reconciles HCPEtcdBackup resources by orchestrating +// etcd snapshot and upload Jobs in the HyperShift Operator namespace. +type HCPEtcdBackupReconciler struct { + client.Client + OperatorNamespace string + ReleaseProvider releaseinfo.ProviderWithOpenShiftImageRegistryOverrides + HypershiftOperatorImage string + MaxBackupCount int +} + +func (r *HCPEtcdBackupReconciler) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + Named(ControllerName). + For(&hyperv1.HCPEtcdBackup{}). + Watches(&batchv1.Job{}, handler.EnqueueRequestsFromMapFunc( + func(ctx context.Context, obj client.Object) []reconcile.Request { + backupName := obj.GetLabels()[LabelBackupName] + hcpNamespace := obj.GetLabels()[LabelHCPNamespace] + if backupName == "" || hcpNamespace == "" { + return nil + } + return []reconcile.Request{{ + NamespacedName: types.NamespacedName{ + Name: backupName, + Namespace: hcpNamespace, + }, + }} + }, + )). + WithOptions(controller.Options{ + RateLimiter: workqueue.NewTypedItemExponentialFailureRateLimiter[reconcile.Request](1*time.Second, 30*time.Second), + }). + Complete(r) +} + +func (r *HCPEtcdBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + // Feature gate check + if !featuregate.Gate().Enabled(featuregate.HCPEtcdBackup) { + return ctrl.Result{}, nil + } + + // Fetch the HCPEtcdBackup CR + backup := &hyperv1.HCPEtcdBackup{} + if err := r.Get(ctx, req.NamespacedName, backup); err != nil { + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to get HCPEtcdBackup: %w", err) + } + + // If backup is in a terminal state, ensure cleanup and run retention. + // Return errors so controller-runtime retries cleanup on transient failures, + // preventing leaked RBAC or NetworkPolicy resources. + if isTerminal(backup) { + if err := r.cleanupResources(ctx, backup); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to cleanup resources for completed backup: %w", err) + } + if err := r.enforceRetention(ctx, backup.Namespace); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to enforce retention: %w", err) + } + return ctrl.Result{}, nil + } + + // Look up the HostedControlPlane in the same namespace + hcp, err := r.getHostedControlPlane(ctx, backup.Namespace) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to look up HostedControlPlane: %w", err) + } + if hcp == nil { + r.setCondition(backup, metav1.Condition{ + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupFailedReason, + Message: "HostedControlPlane not found in namespace " + backup.Namespace, + }) + if err := r.Status().Update(ctx, backup); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) + } + return ctrl.Result{}, nil + } + + // Phase 1 health check: etcd StatefulSet readiness + healthy, msg, err := r.checkEtcdHealth(ctx, backup.Namespace) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to check etcd health: %w", err) + } + if !healthy { + r.setCondition(backup, metav1.Condition{ + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.EtcdUnhealthyReason, + Message: msg, + }) + if err := r.Status().Update(ctx, backup); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) + } + return ctrl.Result{RequeueAfter: requeueInterval}, nil + } + + // Check if we already created a Job for this backup + existingJob, err := r.findJobForBackup(ctx, backup) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to find job for backup: %w", err) + } + + if existingJob != nil { + // Monitor existing Job status + return r.handleJobStatus(ctx, backup, existingJob, hcp) + } + + // Serial execution guard: reject if another backup's Job is already active. + // This runs after findJobForBackup so we don't reject our own Job. + activeJob, err := r.findActiveJob(ctx, backup.Namespace) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to check for active jobs: %w", err) + } + if activeJob != nil { + logger.Info("rejecting backup: another backup Job is already active", "activeJob", activeJob.Name) + r.setCondition(backup, metav1.Condition{ + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupRejectedReason, + Message: fmt.Sprintf("rejected: backup Job %q is already running for this HCP; delete this CR and retry after the active backup completes", activeJob.Name), + }) + if err := r.Status().Update(ctx, backup); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) + } + return ctrl.Result{}, nil + } + + // Validate prerequisites before creating any resources. + // Check credential Secret early so we don't create RBAC/NetworkPolicy unnecessarily. + credentialSecretName, err := r.getCredentialSecretName(backup) + if err != nil { + r.setCondition(backup, metav1.Condition{ + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupFailedReason, + Message: err.Error(), + }) + if statusErr := r.Status().Update(ctx, backup); statusErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", statusErr) + } + return ctrl.Result{}, nil + } + + credSecret := &corev1.Secret{} + if err := r.Get(ctx, types.NamespacedName{Name: credentialSecretName, Namespace: r.OperatorNamespace}, credSecret); err != nil { + if apierrors.IsNotFound(err) { + r.setCondition(backup, metav1.Condition{ + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupFailedReason, + Message: fmt.Sprintf("credential Secret %q not found in namespace %q", credentialSecretName, r.OperatorNamespace), + }) + if statusErr := r.Status().Update(ctx, backup); statusErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", statusErr) + } + return ctrl.Result{}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to get credential Secret: %w", err) + } + + // Create resources and Job + logger.Info("creating backup resources", "backup", backup.Name, "namespace", backup.Namespace) + + if err := r.ensureServiceAccount(ctx); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to ensure ServiceAccount: %w", err) + } + + if err := r.ensureRBAC(ctx, backup); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to ensure RBAC: %w", err) + } + + if err := r.ensureNetworkPolicy(ctx, backup); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to ensure NetworkPolicy: %w", err) + } + + if err := r.createBackupJob(ctx, backup, hcp); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to create backup Job: %w", err) + } + + // Set status to indicate backup is in progress + r.setCondition(backup, metav1.Condition{ + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupInProgressReason, + Message: "Backup Job created, waiting for completion", + }) + if err := r.Status().Update(ctx, backup); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) + } + + // Bubble up to HCP + if err := r.updateHCPBackupCondition(ctx, hcp, metav1.Condition{ + Type: string(hyperv1.EtcdBackupSucceeded), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupInProgressReason, + Message: fmt.Sprintf("Backup %q is in progress", backup.Name), + }); err != nil { + logger.Error(err, "failed to update HCP backup condition") + } + + return ctrl.Result{RequeueAfter: requeueInterval}, nil +} + +// isTerminal returns true if the backup is in a terminal state. +func isTerminal(backup *hyperv1.HCPEtcdBackup) bool { + cond := meta.FindStatusCondition(backup.Status.Conditions, string(hyperv1.BackupCompleted)) + if cond == nil { + return false + } + return cond.Status == metav1.ConditionTrue || + cond.Reason == hyperv1.BackupFailedReason || + cond.Reason == hyperv1.BackupRejectedReason +} + +// setCondition sets or updates a condition on the backup status. +func (r *HCPEtcdBackupReconciler) setCondition(backup *hyperv1.HCPEtcdBackup, condition metav1.Condition) { + condition.ObservedGeneration = backup.Generation + meta.SetStatusCondition(&backup.Status.Conditions, condition) +} + +// updateHCPBackupCondition sets a condition on the HostedControlPlane to bubble +// up the etcd backup status. The HC controller propagates this to the HostedCluster. +func (r *HCPEtcdBackupReconciler) updateHCPBackupCondition(ctx context.Context, hcp *hyperv1.HostedControlPlane, condition metav1.Condition) error { + condition.ObservedGeneration = hcp.Generation + meta.SetStatusCondition(&hcp.Status.Conditions, condition) + return r.Status().Update(ctx, hcp) +} + +// getHostedControlPlane finds the HostedControlPlane in the given namespace. +// Returns nil if none found. +func (r *HCPEtcdBackupReconciler) getHostedControlPlane(ctx context.Context, namespace string) (*hyperv1.HostedControlPlane, error) { + hcpList := &hyperv1.HostedControlPlaneList{} + if err := r.List(ctx, hcpList, client.InNamespace(namespace)); err != nil { + return nil, err + } + if len(hcpList.Items) == 0 { + return nil, nil + } + return &hcpList.Items[0], nil +} + +// checkEtcdHealth verifies the etcd StatefulSet has all replicas ready. +func (r *HCPEtcdBackupReconciler) checkEtcdHealth(ctx context.Context, namespace string) (bool, string, error) { + sts := &appsv1.StatefulSet{} + if err := r.Get(ctx, types.NamespacedName{Name: "etcd", Namespace: namespace}, sts); err != nil { + if apierrors.IsNotFound(err) { + return false, "etcd StatefulSet not found", nil + } + return false, "", err + } + + desired := ptr.Deref(sts.Spec.Replicas, 1) + if sts.Status.ReadyReplicas < desired { + return false, fmt.Sprintf("etcd StatefulSet not fully ready: %d/%d replicas ready", + sts.Status.ReadyReplicas, desired), nil + } + return true, "", nil +} + +// findActiveJob checks if any backup Job is currently active for the given HCP namespace. +// Callers must check for their own backup's Job first (via findJobForBackup) to avoid +// self-rejection when re-reconciling after Job creation. +func (r *HCPEtcdBackupReconciler) findActiveJob(ctx context.Context, hcpNamespace string) (*batchv1.Job, error) { + jobList := &batchv1.JobList{} + if err := r.List(ctx, jobList, + client.InNamespace(r.OperatorNamespace), + client.MatchingLabels{ + LabelApp: LabelName, + LabelHCPNamespace: hcpNamespace, + }, + ); err != nil { + return nil, err + } + + for i := range jobList.Items { + job := &jobList.Items[i] + if job.Status.Active > 0 { + return job, nil + } + } + return nil, nil +} + +// findJobForBackup finds the Job created for this specific backup. +func (r *HCPEtcdBackupReconciler) findJobForBackup(ctx context.Context, backup *hyperv1.HCPEtcdBackup) (*batchv1.Job, error) { + jobList := &batchv1.JobList{} + if err := r.List(ctx, jobList, + client.InNamespace(r.OperatorNamespace), + client.MatchingLabels{ + LabelBackupName: backup.Name, + LabelHCPNamespace: backup.Namespace, + }, + ); err != nil { + return nil, err + } + if len(jobList.Items) == 0 { + return nil, nil + } + return &jobList.Items[0], nil +} + +// handleJobStatus monitors Job status and updates HCPEtcdBackup conditions. +func (r *HCPEtcdBackupReconciler) handleJobStatus(ctx context.Context, backup *hyperv1.HCPEtcdBackup, job *batchv1.Job, hcp *hyperv1.HostedControlPlane) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + for _, cond := range job.Status.Conditions { + if cond.Type == batchv1.JobComplete && cond.Status == corev1.ConditionTrue { + logger.Info("backup Job completed successfully", "job", job.Name) + + // Extract snapshotURL from the upload container's termination message. + // The etcd-upload command writes the URL to /dev/termination-log. + if url, err := r.getSnapshotURLFromPod(ctx, job); err != nil { + logger.Error(err, "failed to read snapshot URL from pod termination message") + } else if url != "" { + backup.Status.SnapshotURL = url + } + + // Propagate encryption metadata based on storage config + r.setEncryptionMetadata(backup) + + r.setCondition(backup, metav1.Condition{ + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionTrue, + Reason: hyperv1.BackupSucceededReason, + Message: "Backup completed successfully", + }) + + if err := r.Status().Update(ctx, backup); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) + } + + // Bubble up success to HCP + if err := r.updateHCPBackupCondition(ctx, hcp, metav1.Condition{ + Type: string(hyperv1.EtcdBackupSucceeded), + Status: metav1.ConditionTrue, + Reason: hyperv1.BackupSucceededReason, + Message: fmt.Sprintf("Backup %q completed successfully", backup.Name), + }); err != nil { + logger.Error(err, "failed to update HCP backup condition") + } + + if err := r.cleanupResources(ctx, backup); err != nil { + logger.Error(err, "failed to cleanup resources after successful backup") + } + return ctrl.Result{}, nil + } + + if cond.Type == batchv1.JobFailed && cond.Status == corev1.ConditionTrue { + logger.Info("backup Job failed", "job", job.Name, "reason", cond.Message) + + r.setCondition(backup, metav1.Condition{ + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupFailedReason, + Message: fmt.Sprintf("Backup Job failed: %s", cond.Message), + }) + + if err := r.Status().Update(ctx, backup); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) + } + + // Bubble up failure to HCP + if err := r.updateHCPBackupCondition(ctx, hcp, metav1.Condition{ + Type: string(hyperv1.EtcdBackupSucceeded), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupFailedReason, + Message: fmt.Sprintf("Backup %q failed: %s", backup.Name, cond.Message), + }); err != nil { + logger.Error(err, "failed to update HCP backup condition") + } + + if err := r.cleanupResources(ctx, backup); err != nil { + logger.Error(err, "failed to cleanup resources after failed backup") + } + return ctrl.Result{}, nil + } + } + + // Job still running + return ctrl.Result{RequeueAfter: requeueInterval}, nil +} + +// getSnapshotURLFromPod reads the snapshot URL from the upload container's +// termination message in the Pod controlled by the given Job. +func (r *HCPEtcdBackupReconciler) getSnapshotURLFromPod(ctx context.Context, job *batchv1.Job) (string, error) { + podList := &corev1.PodList{} + if err := r.List(ctx, podList, + client.InNamespace(job.Namespace), + client.MatchingLabels{"batch.kubernetes.io/job-name": job.Name}, + ); err != nil { + return "", fmt.Errorf("failed to list pods for job %q: %w", job.Name, err) + } + + for i := range podList.Items { + pod := &podList.Items[i] + for _, cs := range pod.Status.ContainerStatuses { + if cs.Name == "upload" && cs.State.Terminated != nil && cs.State.Terminated.Message != "" { + return strings.TrimSpace(cs.State.Terminated.Message), nil + } + } + } + return "", nil +} + +// setEncryptionMetadata populates encryption metadata on the backup status +// based on the storage configuration. +func (r *HCPEtcdBackupReconciler) setEncryptionMetadata(backup *hyperv1.HCPEtcdBackup) { + switch backup.Spec.Storage.StorageType { + case hyperv1.S3BackupStorage: + if backup.Spec.Storage.S3.KMSKeyARN != "" { + backup.Status.EncryptionMetadata = hyperv1.HCPEtcdBackupEncryptionMetadata{ + AWS: hyperv1.HCPEtcdBackupEncryptionMetadataAWS{ + KMSKeyARN: backup.Spec.Storage.S3.KMSKeyARN, + }, + } + } + case hyperv1.AzureBlobBackupStorage: + if backup.Spec.Storage.AzureBlob.EncryptionKeyURL != "" { + backup.Status.EncryptionMetadata = hyperv1.HCPEtcdBackupEncryptionMetadata{ + Azure: hyperv1.HCPEtcdBackupEncryptionMetadataAzure{ + EncryptionKeyURL: backup.Spec.Storage.AzureBlob.EncryptionKeyURL, + }, + } + } + } +} + +// ensureServiceAccount creates the ServiceAccount for backup Jobs in the HO namespace. +func (r *HCPEtcdBackupReconciler) ensureServiceAccount(ctx context.Context) error { + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: jobServiceAccountName, + Namespace: r.OperatorNamespace, + }, + } + _, err := controllerutil.CreateOrUpdate(ctx, r.Client, sa, func() error { + return nil + }) + return err +} + +// ensureRBAC creates the Role and RoleBinding in the HCP namespace for the backup Job SA. +func (r *HCPEtcdBackupReconciler) ensureRBAC(ctx context.Context, backup *hyperv1.HCPEtcdBackup) error { + // Role in HCP namespace granting read access to etcd TLS resources + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: backup.Namespace, + }, + } + _, err := controllerutil.CreateOrUpdate(ctx, r.Client, role, func() error { + role.Rules = []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"secrets"}, + ResourceNames: []string{"etcd-client-tls"}, + Verbs: []string{"get"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"configmaps"}, + ResourceNames: []string{"etcd-ca"}, + Verbs: []string{"get"}, + }, + } + return nil + }) + if err != nil { + return fmt.Errorf("failed to ensure Role: %w", err) + } + + // RoleBinding binding the HO namespace SA to the HCP namespace Role + rb := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: backup.Namespace, + }, + } + _, err = controllerutil.CreateOrUpdate(ctx, r.Client, rb, func() error { + rb.RoleRef = rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "Role", + Name: RBACName, + } + rb.Subjects = []rbacv1.Subject{ + { + Kind: rbacv1.ServiceAccountKind, + Name: jobServiceAccountName, + Namespace: r.OperatorNamespace, + }, + } + return nil + }) + if err != nil { + return fmt.Errorf("failed to ensure RoleBinding: %w", err) + } + + return nil +} + +// ensureNetworkPolicy creates the temporary NetworkPolicy in the HCP namespace +// allowing ingress from the HO namespace to etcd on port 2379. +func (r *HCPEtcdBackupReconciler) ensureNetworkPolicy(ctx context.Context, backup *hyperv1.HCPEtcdBackup) error { + np := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: NetworkPolicyName, + Namespace: backup.Namespace, + }, + } + _, err := controllerutil.CreateOrUpdate(ctx, r.Client, np, func() error { + etcdPort := intstr.FromInt32(supportconfig.EtcdClientPort) + tcpProtocol := corev1.ProtocolTCP + np.Spec = networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "etcd", + }, + }, + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + { + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "kubernetes.io/metadata.name": r.OperatorNamespace, + }, + }, + }, + }, + Ports: []networkingv1.NetworkPolicyPort{ + { + Protocol: &tcpProtocol, + Port: &etcdPort, + }, + }, + }, + }, + PolicyTypes: []networkingv1.PolicyType{ + networkingv1.PolicyTypeIngress, + }, + } + return nil + }) + return err +} + +// cleanupResources removes temporary NetworkPolicy and RBAC from the HCP namespace. +func (r *HCPEtcdBackupReconciler) cleanupResources(ctx context.Context, backup *hyperv1.HCPEtcdBackup) error { + logger := log.FromContext(ctx) + var firstErr error + + // Delete NetworkPolicy + np := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: NetworkPolicyName, + Namespace: backup.Namespace, + }, + } + if err := r.Delete(ctx, np); err != nil && !apierrors.IsNotFound(err) { + logger.Error(err, "failed to delete NetworkPolicy", "name", NetworkPolicyName) + firstErr = err + } + + // Delete RoleBinding + rb := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: backup.Namespace, + }, + } + if err := r.Delete(ctx, rb); err != nil && !apierrors.IsNotFound(err) { + logger.Error(err, "failed to delete RoleBinding", "name", RBACName) + if firstErr == nil { + firstErr = err + } + } + + // Delete Role + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: backup.Namespace, + }, + } + if err := r.Delete(ctx, role); err != nil && !apierrors.IsNotFound(err) { + logger.Error(err, "failed to delete Role", "name", RBACName) + if firstErr == nil { + firstErr = err + } + } + + return firstErr +} + +// createBackupJob creates the backup Job in the HO namespace with the 3-container +// PodSpec: fetch-etcd-certs (init), etcdctl snapshot save (init), etcd-upload (main). +func (r *HCPEtcdBackupReconciler) createBackupJob(ctx context.Context, backup *hyperv1.HCPEtcdBackup, hcp *hyperv1.HostedControlPlane) error { + // Resolve images + pullSecret := &corev1.Secret{} + if err := r.Get(ctx, types.NamespacedName{Name: pullSecretName, Namespace: backup.Namespace}, pullSecret); err != nil { + // Preserve error type (including IsNotFound) so caller can detect permanent failures + return fmt.Errorf("pull secret %q in namespace %q: %w", pullSecretName, backup.Namespace, err) + } + pullSecretBytes := pullSecret.Data[corev1.DockerConfigJsonKey] + + releaseImage := hyperutil.HCPControlPlaneReleaseImage(hcp) + + cpoImage, err := r.resolveControlPlaneOperatorImage(ctx, hcp, releaseImage, pullSecretBytes) + if err != nil { + return fmt.Errorf("failed to resolve CPO image: %w", err) + } + + etcdImage, err := hyperutil.GetPayloadImageFromRelease(ctx, r.ReleaseProvider, releaseImage, "etcd", pullSecretBytes) + if err != nil { + return fmt.Errorf("failed to resolve etcd image: %w", err) + } + + // Build upload args based on storage type + uploadArgs, credentialSecretName, err := r.buildUploadArgs(backup) + if err != nil { + return fmt.Errorf("failed to build upload args: %w", err) + } + + jobLabels := map[string]string{ + LabelApp: LabelName, + labelHCP: hcp.Name, + LabelBackupName: backup.Name, + LabelHCPNamespace: backup.Namespace, + } + + job := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: fmt.Sprintf("etcd-backup-%s-", backup.Name), + Namespace: r.OperatorNamespace, + Labels: jobLabels, + }, + Spec: batchv1.JobSpec{ + TTLSecondsAfterFinished: ptr.To[int32](600), + ActiveDeadlineSeconds: ptr.To[int64](900), + BackoffLimit: ptr.To[int32](0), + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: jobLabels, + }, + Spec: corev1.PodSpec{ + ServiceAccountName: jobServiceAccountName, + RestartPolicy: corev1.RestartPolicyNever, + Volumes: []corev1.Volume{ + { + Name: volumeEtcdCerts, + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + { + Name: volumeEtcdBackup, + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + { + Name: volumeCredentials, + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: credentialSecretName, + }, + }, + }, + }, + InitContainers: []corev1.Container{ + { + Name: "fetch-certs", + Image: cpoImage, + Command: []string{ + "control-plane-operator", "fetch-etcd-certs", + "--hcp-namespace", backup.Namespace, + "--output-dir", mountPathEtcdCerts, + }, + VolumeMounts: []corev1.VolumeMount{ + { + Name: volumeEtcdCerts, + MountPath: mountPathEtcdCerts, + }, + }, + }, + { + Name: "snapshot", + Image: etcdImage, + Env: []corev1.EnvVar{ + {Name: "ETCDCTL_API", Value: "3"}, + }, + Command: []string{ + "/usr/bin/etcdctl", + "--endpoints", fmt.Sprintf("https://etcd-client.%s.svc:%d", backup.Namespace, supportconfig.EtcdClientPort), + "--cacert", mountPathEtcdCerts + "/ca.crt", + "--cert", mountPathEtcdCerts + "/etcd-client.crt", + "--key", mountPathEtcdCerts + "/etcd-client.key", + "snapshot", "save", + mountPathEtcdBackup + "/snapshot.db", + }, + VolumeMounts: []corev1.VolumeMount{ + { + Name: volumeEtcdCerts, + MountPath: mountPathEtcdCerts, + ReadOnly: true, + }, + { + Name: volumeEtcdBackup, + MountPath: mountPathEtcdBackup, + }, + }, + }, + }, + Containers: []corev1.Container{ + { + Name: "upload", + Image: cpoImage, + Command: uploadArgs, + VolumeMounts: []corev1.VolumeMount{ + { + Name: volumeEtcdBackup, + MountPath: mountPathEtcdBackup, + ReadOnly: true, + }, + { + Name: volumeCredentials, + MountPath: mountPathCredentials, + ReadOnly: true, + }, + }, + }, + }, + }, + }, + }, + } + + return r.Create(ctx, job) +} + +// resolveControlPlaneOperatorImage resolves the CPO image for the given HCP, +// handling annotation overrides and disconnected environments. +func (r *HCPEtcdBackupReconciler) resolveControlPlaneOperatorImage(ctx context.Context, hcp *hyperv1.HostedControlPlane, releaseImage string, pullSecret []byte) (string, error) { + // Check for annotation override on HCP (propagated from HostedCluster) + if val, ok := hcp.Annotations[hyperv1.ControlPlaneOperatorImageAnnotation]; ok { + return val, nil + } + + // Resolve from release payload — the "hypershift" component is the CPO image + releaseInfo, err := r.ReleaseProvider.Lookup(ctx, releaseImage, pullSecret) + if err != nil { + return "", fmt.Errorf("failed to lookup release image: %w", err) + } + + if hypershiftImage, exists := releaseInfo.ComponentImages()["hypershift"]; exists { + return hypershiftImage, nil + } + + // Fallback to HO's own image + return r.HypershiftOperatorImage, nil +} + +// getCredentialSecretName returns the name of the credential Secret referenced +// in the backup's storage configuration. This is used for early validation +// before creating RBAC/NetworkPolicy resources. +func (r *HCPEtcdBackupReconciler) getCredentialSecretName(backup *hyperv1.HCPEtcdBackup) (string, error) { + switch backup.Spec.Storage.StorageType { + case hyperv1.S3BackupStorage: + return backup.Spec.Storage.S3.Credentials.Name, nil + case hyperv1.AzureBlobBackupStorage: + return backup.Spec.Storage.AzureBlob.Credentials.Name, nil + } + return "", fmt.Errorf("unsupported storage type: %s", backup.Spec.Storage.StorageType) +} + +// buildUploadArgs constructs the command args for the etcd-upload container +// and returns the credential Secret name. +func (r *HCPEtcdBackupReconciler) buildUploadArgs(backup *hyperv1.HCPEtcdBackup) ([]string, string, error) { + args := []string{ + "control-plane-operator", "etcd-upload", + "--snapshot-path", mountPathEtcdBackup + "/snapshot.db", + } + + switch backup.Spec.Storage.StorageType { + case hyperv1.S3BackupStorage: + s3 := backup.Spec.Storage.S3 + args = append(args, + "--storage-type", "S3", + "--aws-bucket", s3.Bucket, + "--aws-region", s3.Region, + "--key-prefix", s3.KeyPrefix, + "--credentials-file", mountPathCredentials+"/credentials", + ) + if s3.KMSKeyARN != "" { + args = append(args, "--aws-kms-key-arn", s3.KMSKeyARN) + } + return args, s3.Credentials.Name, nil + + case hyperv1.AzureBlobBackupStorage: + azure := backup.Spec.Storage.AzureBlob + args = append(args, + "--storage-type", "AzureBlob", + "--azure-container", azure.Container, + "--azure-storage-account", azure.StorageAccount, + "--key-prefix", azure.KeyPrefix, + "--credentials-file", mountPathCredentials+"/credentials", + ) + if azure.EncryptionKeyURL != "" { + args = append(args, "--azure-encryption-scope", azure.EncryptionKeyURL) + } + return args, azure.Credentials.Name, nil + } + + return nil, "", fmt.Errorf("unsupported storage type: %s", backup.Spec.Storage.StorageType) +} + +// enforceRetention deletes the oldest completed HCPEtcdBackup CRs if the count +// exceeds MaxBackupCount. +func (r *HCPEtcdBackupReconciler) enforceRetention(ctx context.Context, namespace string) error { + if r.MaxBackupCount <= 0 { + return nil + } + + backupList := &hyperv1.HCPEtcdBackupList{} + if err := r.List(ctx, backupList, client.InNamespace(namespace)); err != nil { + return fmt.Errorf("failed to list HCPEtcdBackup CRs: %w", err) + } + + // Filter completed backups only + var completed []hyperv1.HCPEtcdBackup + for _, b := range backupList.Items { + cond := meta.FindStatusCondition(b.Status.Conditions, string(hyperv1.BackupCompleted)) + if cond != nil && cond.Status == metav1.ConditionTrue { + completed = append(completed, b) + } + } + + if len(completed) <= r.MaxBackupCount { + return nil + } + + // Sort by creation timestamp (oldest first) + sort.SliceStable(completed, func(i, j int) bool { + return completed[i].CreationTimestamp.Before(&completed[j].CreationTimestamp) + }) + + // Delete excess + toDelete := len(completed) - r.MaxBackupCount + for i := range toDelete { + if err := r.Delete(ctx, &completed[i]); err != nil { + if !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to delete old HCPEtcdBackup %s: %w", completed[i].Name, err) + } + } + } + return nil +} diff --git a/hypershift-operator/controllers/etcdbackup/reconciler_test.go b/hypershift-operator/controllers/etcdbackup/reconciler_test.go new file mode 100644 index 000000000000..7bc5ccd719ab --- /dev/null +++ b/hypershift-operator/controllers/etcdbackup/reconciler_test.go @@ -0,0 +1,1259 @@ +package etcdbackup + +import ( + "context" + "fmt" + "os" + "testing" + "time" + + . "github.com/onsi/gomega" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/hypershift-operator/featuregate" + "github.com/openshift/hypershift/support/releaseinfo" + + configv1 "github.com/openshift/api/config/v1" + imageapi "github.com/openshift/api/image/v1" + + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestMain(m *testing.M) { + featuregate.ConfigureFeatureSet(string(configv1.TechPreviewNoUpgrade)) + os.Exit(m.Run()) +} + +const ( + testHCPNamespace = "clusters-test" + testHONamespace = "hypershift" + testBackupName = "backup-1" + testHCPName = "test-hcp" + testReleaseImage = "quay.io/openshift-release-dev/ocp-release:4.16.0-x86_64" +) + +func newScheme() *runtime.Scheme { + s := runtime.NewScheme() + _ = hyperv1.AddToScheme(s) + _ = batchv1.AddToScheme(s) + _ = corev1.AddToScheme(s) + _ = appsv1.AddToScheme(s) + _ = networkingv1.AddToScheme(s) + _ = rbacv1.AddToScheme(s) + return s +} + +func newReconciler(objs ...client.Object) *HCPEtcdBackupReconciler { + scheme := newScheme() + clientObjs := make([]client.Object, len(objs)) + copy(clientObjs, objs) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(clientObjs...). + WithStatusSubresource(&hyperv1.HCPEtcdBackup{}, &hyperv1.HostedControlPlane{}). + Build() + + return &HCPEtcdBackupReconciler{ + Client: fakeClient, + OperatorNamespace: testHONamespace, + ReleaseProvider: &fakeReleaseProvider{}, + HypershiftOperatorImage: "quay.io/hypershift/hypershift:latest", + MaxBackupCount: 5, + } +} + +func newHCPEtcdBackup() *hyperv1.HCPEtcdBackup { + return &hyperv1.HCPEtcdBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + Spec: hyperv1.HCPEtcdBackupSpec{ + Storage: hyperv1.HCPEtcdBackupStorage{ + StorageType: hyperv1.S3BackupStorage, + S3: hyperv1.HCPEtcdBackupS3{ + Bucket: "my-bucket", + Region: "us-east-1", + KeyPrefix: "backups/test", + Credentials: hyperv1.SecretReference{ + Name: "aws-creds", + }, + }, + }, + }, + } +} + +func newHostedControlPlane() *hyperv1.HostedControlPlane { + return &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: testHCPName, + Namespace: testHCPNamespace, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + ReleaseImage: testReleaseImage, + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + }, + }, + } +} + +func newEtcdStatefulSet(ready int32, replicas int32) *appsv1.StatefulSet { + return &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd", + Namespace: testHCPNamespace, + }, + Spec: appsv1.StatefulSetSpec{ + Replicas: ptr.To(replicas), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "etcd"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "etcd"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "etcd", Image: "etcd:latest"}}, + }, + }, + }, + Status: appsv1.StatefulSetStatus{ + ReadyReplicas: ready, + }, + } +} + +const ( + testCPOImage = "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:cpo-fake" + testEtcdImage = "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:etcd-fake" +) + +// fakeReleaseProvider implements releaseinfo.ProviderWithOpenShiftImageRegistryOverrides. +type fakeReleaseProvider struct{} + +func (f *fakeReleaseProvider) Lookup(ctx context.Context, image string, pullSecret []byte) (*releaseinfo.ReleaseImage, error) { + return &releaseinfo.ReleaseImage{ + ImageStream: &imageapi.ImageStream{ + Spec: imageapi.ImageStreamSpec{ + Tags: []imageapi.TagReference{ + {Name: "hypershift", From: &corev1.ObjectReference{Name: testCPOImage}}, + {Name: "etcd", From: &corev1.ObjectReference{Name: testEtcdImage}}, + }, + }, + }, + }, nil +} + +func (f *fakeReleaseProvider) GetRegistryOverrides() map[string]string { + return nil +} + +func (f *fakeReleaseProvider) GetOpenShiftImageRegistryOverrides() map[string][]string { + return nil +} + +func (f *fakeReleaseProvider) GetMirroredReleaseImage() string { + return "" +} + +func TestReconcile(t *testing.T) { + t.Run("When HCPEtcdBackup is not found it should not requeue", func(t *testing.T) { + g := NewGomegaWithT(t) + r := newReconciler() + + result, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: "nonexistent", + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(ctrl.Result{})) + }) + + t.Run("When HostedControlPlane is not found it should set BackupFailed", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + r := newReconciler(backup) + + result, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(ctrl.Result{})) + + // Verify status + updated := &hyperv1.HCPEtcdBackup{} + g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + g.Expect(updated.Status.Conditions).To(HaveLen(1)) + g.Expect(updated.Status.Conditions[0].Type).To(Equal(string(hyperv1.BackupCompleted))) + g.Expect(updated.Status.Conditions[0].Status).To(Equal(metav1.ConditionFalse)) + g.Expect(updated.Status.Conditions[0].Reason).To(Equal(hyperv1.BackupFailedReason)) + }) + + t.Run("When etcd StatefulSet is not ready it should set EtcdUnhealthy", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + sts := newEtcdStatefulSet(1, 3) // Only 1 of 3 ready + r := newReconciler(backup, hcp, sts) + + result, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result.RequeueAfter).To(Equal(requeueInterval)) + + updated := &hyperv1.HCPEtcdBackup{} + g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + g.Expect(updated.Status.Conditions).To(HaveLen(1)) + g.Expect(updated.Status.Conditions[0].Reason).To(Equal(hyperv1.EtcdUnhealthyReason)) + }) + + t.Run("When etcd StatefulSet is not found it should set EtcdUnhealthy", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + r := newReconciler(backup, hcp) // No StatefulSet + + result, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result.RequeueAfter).To(Equal(requeueInterval)) + + updated := &hyperv1.HCPEtcdBackup{} + g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + g.Expect(updated.Status.Conditions[0].Reason).To(Equal(hyperv1.EtcdUnhealthyReason)) + g.Expect(updated.Status.Conditions[0].Message).To(ContainSubstring("not found")) + }) + + t.Run("When another backup is already active it should reject and not requeue", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + sts := newEtcdStatefulSet(3, 3) + + // Active Job for this HCP namespace + activeJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-backup-other", + Namespace: testHONamespace, + Labels: map[string]string{ + LabelApp: LabelName, + LabelHCPNamespace: testHCPNamespace, + LabelBackupName: "other-backup", + }, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "test", Image: "test:latest"}}, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + Status: batchv1.JobStatus{ + Active: 1, + }, + } + r := newReconciler(backup, hcp, sts, activeJob) + + result, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(ctrl.Result{})) + + updated := &hyperv1.HCPEtcdBackup{} + g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + g.Expect(updated.Status.Conditions[0].Reason).To(Equal(hyperv1.BackupRejectedReason)) + g.Expect(updated.Status.Conditions[0].Message).To(ContainSubstring("rejected")) + }) + + t.Run("When the backup's own Job is active it should monitor it instead of rejecting", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + sts := newEtcdStatefulSet(3, 3) + + // Active Job belonging to THIS backup (same LabelBackupName) + ownJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-backup-" + testBackupName, + Namespace: testHONamespace, + Labels: map[string]string{ + LabelApp: LabelName, + LabelHCPNamespace: testHCPNamespace, + LabelBackupName: testBackupName, + }, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "test", Image: "test:latest"}}, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + Status: batchv1.JobStatus{ + Active: 1, + }, + } + r := newReconciler(backup, hcp, sts, ownJob) + + result, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + // Should requeue to monitor the Job, NOT reject + g.Expect(result.RequeueAfter).To(Equal(requeueInterval)) + + updated := &hyperv1.HCPEtcdBackup{} + g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + // Should NOT be BackupRejected + for _, c := range updated.Status.Conditions { + g.Expect(c.Reason).ToNot(Equal(hyperv1.BackupRejectedReason), + "backup should not reject its own active Job") + } + }) + + t.Run("When backup is in terminal state it should cleanup and not requeue", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + backup.Status.Conditions = []metav1.Condition{ + { + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionTrue, + Reason: hyperv1.BackupSucceededReason, + }, + } + r := newReconciler(backup) + + result, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(ctrl.Result{})) + }) + + t.Run("When credential Secret does not exist it should set BackupFailed", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + sts := newEtcdStatefulSet(3, 3) + pullSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: pullSecretName, + Namespace: testHCPNamespace, + }, + Data: map[string][]byte{ + corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), + }, + } + // No credential secret — should trigger BackupFailed + r := newReconciler(backup, hcp, sts, pullSecret) + + result, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(ctrl.Result{})) + + updated := &hyperv1.HCPEtcdBackup{} + g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + g.Expect(updated.Status.Conditions).To(HaveLen(1)) + g.Expect(updated.Status.Conditions[0].Type).To(Equal(string(hyperv1.BackupCompleted))) + g.Expect(updated.Status.Conditions[0].Status).To(Equal(metav1.ConditionFalse)) + g.Expect(updated.Status.Conditions[0].Reason).To(Equal(hyperv1.BackupFailedReason)) + g.Expect(updated.Status.Conditions[0].Message).To(ContainSubstring("credential Secret")) + }) + + t.Run("When backup failed it should be terminal", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + backup.Status.Conditions = []metav1.Condition{ + { + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupFailedReason, + }, + } + r := newReconciler(backup) + + result, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(ctrl.Result{})) + }) +} + +func TestIsTerminal(t *testing.T) { + t.Run("When no conditions exist it should not be terminal", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := &hyperv1.HCPEtcdBackup{} + g.Expect(isTerminal(backup)).To(BeFalse()) + }) + + t.Run("When BackupCompleted is True it should be terminal", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := &hyperv1.HCPEtcdBackup{ + Status: hyperv1.HCPEtcdBackupStatus{ + Conditions: []metav1.Condition{ + { + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionTrue, + Reason: hyperv1.BackupSucceededReason, + }, + }, + }, + } + g.Expect(isTerminal(backup)).To(BeTrue()) + }) + + t.Run("When BackupCompleted reason is BackupFailed it should be terminal", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := &hyperv1.HCPEtcdBackup{ + Status: hyperv1.HCPEtcdBackupStatus{ + Conditions: []metav1.Condition{ + { + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupFailedReason, + }, + }, + }, + } + g.Expect(isTerminal(backup)).To(BeTrue()) + }) + + t.Run("When BackupCompleted reason is BackupRejected it should be terminal", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := &hyperv1.HCPEtcdBackup{ + Status: hyperv1.HCPEtcdBackupStatus{ + Conditions: []metav1.Condition{ + { + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupRejectedReason, + }, + }, + }, + } + g.Expect(isTerminal(backup)).To(BeTrue()) + }) + + t.Run("When BackupCompleted is False with non-terminal reason it should not be terminal", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := &hyperv1.HCPEtcdBackup{ + Status: hyperv1.HCPEtcdBackupStatus{ + Conditions: []metav1.Condition{ + { + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.EtcdUnhealthyReason, + }, + }, + }, + } + g.Expect(isTerminal(backup)).To(BeFalse()) + }) +} + +func TestEnsureRBAC(t *testing.T) { + t.Run("When ensureRBAC is called it should create Role and RoleBinding in HCP namespace", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + r := newReconciler(backup) + ctx := context.Background() + + err := r.ensureRBAC(ctx, backup) + g.Expect(err).ToNot(HaveOccurred()) + + // Verify Role + role := &rbacv1.Role{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, role)).To(Succeed()) + g.Expect(role.Rules).To(HaveLen(2)) + g.Expect(role.Rules[0].Resources).To(ContainElement("secrets")) + g.Expect(role.Rules[0].ResourceNames).To(ContainElement("etcd-client-tls")) + g.Expect(role.Rules[1].Resources).To(ContainElement("configmaps")) + g.Expect(role.Rules[1].ResourceNames).To(ContainElement("etcd-ca")) + + // Verify RoleBinding + rb := &rbacv1.RoleBinding{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, rb)).To(Succeed()) + g.Expect(rb.RoleRef.Name).To(Equal(RBACName)) + g.Expect(rb.Subjects).To(HaveLen(1)) + g.Expect(rb.Subjects[0].Name).To(Equal(jobServiceAccountName)) + g.Expect(rb.Subjects[0].Namespace).To(Equal(testHONamespace)) + }) +} + +func TestEnsureNetworkPolicy(t *testing.T) { + t.Run("When ensureNetworkPolicy is called it should create NetworkPolicy in HCP namespace", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + r := newReconciler(backup) + ctx := context.Background() + + err := r.ensureNetworkPolicy(ctx, backup) + g.Expect(err).ToNot(HaveOccurred()) + + np := &networkingv1.NetworkPolicy{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: NetworkPolicyName, Namespace: testHCPNamespace}, np)).To(Succeed()) + g.Expect(np.Spec.PodSelector.MatchLabels).To(HaveKeyWithValue("app", "etcd")) + g.Expect(np.Spec.Ingress).To(HaveLen(1)) + g.Expect(np.Spec.Ingress[0].From[0].NamespaceSelector.MatchLabels).To(HaveKeyWithValue("kubernetes.io/metadata.name", testHONamespace)) + g.Expect(np.Spec.Ingress[0].Ports[0].Port.IntValue()).To(Equal(2379)) + g.Expect(np.Spec.PolicyTypes).To(ContainElement(networkingv1.PolicyTypeIngress)) + }) +} + +func TestCleanupResources(t *testing.T) { + t.Run("When cleanup is called it should delete NetworkPolicy, Role, and RoleBinding", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + + np := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: NetworkPolicyName, + Namespace: testHCPNamespace, + }, + } + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: testHCPNamespace, + }, + } + rb := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: testHCPNamespace, + }, + } + r := newReconciler(backup, np, role, rb) + ctx := context.Background() + + err := r.cleanupResources(ctx, backup) + g.Expect(err).ToNot(HaveOccurred()) + + // All resources should be deleted + g.Expect(r.Get(ctx, types.NamespacedName{Name: NetworkPolicyName, Namespace: testHCPNamespace}, &networkingv1.NetworkPolicy{})).ToNot(Succeed()) + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, &rbacv1.Role{})).ToNot(Succeed()) + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, &rbacv1.RoleBinding{})).ToNot(Succeed()) + }) + + t.Run("When resources do not exist cleanup should succeed", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + r := newReconciler(backup) + ctx := context.Background() + + err := r.cleanupResources(ctx, backup) + g.Expect(err).ToNot(HaveOccurred()) + }) +} + +func newTestJob(status batchv1.JobStatus) *batchv1.Job { + return &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-backup-test", + Namespace: testHONamespace, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "test", Image: "test:latest"}}, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + Status: status, + } +} + +func TestHandleJobStatus(t *testing.T) { + t.Run("When Job succeeds it should set BackupCompleted True and read snapshotURL from termination message", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + job := newTestJob(batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{ + { + Type: batchv1.JobComplete, + Status: corev1.ConditionTrue, + }, + }, + }) + // Pod with termination message from the upload container + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-backup-test-pod", + Namespace: testHONamespace, + Labels: map[string]string{ + "batch.kubernetes.io/job-name": "etcd-backup-test", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "upload", Image: "test:latest"}}, + RestartPolicy: corev1.RestartPolicyNever, + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "upload", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + ExitCode: 0, + Message: "s3://my-bucket/backups/test/snapshot.db", + }, + }, + }, + }, + }, + } + r := newReconciler(backup, job, hcp, pod) + + result, err := r.handleJobStatus(context.Background(), backup, job, hcp) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(ctrl.Result{})) + + updated := &hyperv1.HCPEtcdBackup{} + g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + g.Expect(updated.Status.Conditions[0].Status).To(Equal(metav1.ConditionTrue)) + g.Expect(updated.Status.Conditions[0].Reason).To(Equal(hyperv1.BackupSucceededReason)) + g.Expect(updated.Status.SnapshotURL).To(Equal("s3://my-bucket/backups/test/snapshot.db")) + + // Verify HCP condition was set + updatedHCP := &hyperv1.HostedControlPlane{} + g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testHCPName, Namespace: testHCPNamespace}, updatedHCP)).To(Succeed()) + hcpCond := meta.FindStatusCondition(updatedHCP.Status.Conditions, string(hyperv1.EtcdBackupSucceeded)) + g.Expect(hcpCond).ToNot(BeNil()) + g.Expect(hcpCond.Status).To(Equal(metav1.ConditionTrue)) + g.Expect(hcpCond.Reason).To(Equal(hyperv1.BackupSucceededReason)) + }) + + t.Run("When Job fails it should set BackupFailed", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + job := newTestJob(batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{ + { + Type: batchv1.JobFailed, + Status: corev1.ConditionTrue, + Message: "BackoffLimitExceeded", + }, + }, + }) + r := newReconciler(backup, job, hcp) + + result, err := r.handleJobStatus(context.Background(), backup, job, hcp) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(ctrl.Result{})) + + updated := &hyperv1.HCPEtcdBackup{} + g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + g.Expect(updated.Status.Conditions[0].Status).To(Equal(metav1.ConditionFalse)) + g.Expect(updated.Status.Conditions[0].Reason).To(Equal(hyperv1.BackupFailedReason)) + g.Expect(updated.Status.Conditions[0].Message).To(ContainSubstring("BackoffLimitExceeded")) + }) + + t.Run("When Job is still running it should requeue", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + job := newTestJob(batchv1.JobStatus{Active: 1}) + r := newReconciler(backup, job, hcp) + + result, err := r.handleJobStatus(context.Background(), backup, job, hcp) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result.RequeueAfter).To(Equal(requeueInterval)) + }) +} + +func TestSetEncryptionMetadata(t *testing.T) { + t.Run("When S3 with KMS key it should set AWS encryption metadata", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + backup.Spec.Storage.S3.KMSKeyARN = "arn:aws:kms:us-east-1:123456789012:key/test-key" + r := newReconciler(backup) + + r.setEncryptionMetadata(backup) + g.Expect(backup.Status.EncryptionMetadata.AWS.KMSKeyARN).To(Equal("arn:aws:kms:us-east-1:123456789012:key/test-key")) + }) + + t.Run("When S3 without KMS key it should not set encryption metadata", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + r := newReconciler(backup) + + r.setEncryptionMetadata(backup) + g.Expect(backup.Status.EncryptionMetadata.AWS.KMSKeyARN).To(BeEmpty()) + }) + + t.Run("When AzureBlob with encryption key it should set Azure encryption metadata", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := &hyperv1.HCPEtcdBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + Spec: hyperv1.HCPEtcdBackupSpec{ + Storage: hyperv1.HCPEtcdBackupStorage{ + StorageType: hyperv1.AzureBlobBackupStorage, + AzureBlob: hyperv1.HCPEtcdBackupAzureBlob{ + Container: "my-container", + StorageAccount: "mystorageaccount", + KeyPrefix: "backups/test", + Credentials: hyperv1.SecretReference{Name: "azure-creds"}, + EncryptionKeyURL: "https://myvault.vault.azure.net/keys/mykey", + }, + }, + }, + } + r := newReconciler(backup) + + r.setEncryptionMetadata(backup) + g.Expect(backup.Status.EncryptionMetadata.Azure.EncryptionKeyURL).To(Equal("https://myvault.vault.azure.net/keys/mykey")) + }) +} + +func TestBuildUploadArgs(t *testing.T) { + t.Run("When storage type is S3 it should build S3 upload args", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + r := newReconciler(backup) + + args, credSecret, err := r.buildUploadArgs(backup) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(credSecret).To(Equal("aws-creds")) + g.Expect(args).To(ContainElements( + "--storage-type", "S3", + "--aws-bucket", "my-bucket", + "--aws-region", "us-east-1", + "--key-prefix", "backups/test", + )) + }) + + t.Run("When S3 has KMS key it should include kms-key-arn flag", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + backup.Spec.Storage.S3.KMSKeyARN = "arn:aws:kms:us-east-1:123456789012:key/test" + r := newReconciler(backup) + + args, _, err := r.buildUploadArgs(backup) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(args).To(ContainElements("--aws-kms-key-arn", "arn:aws:kms:us-east-1:123456789012:key/test")) + }) + + t.Run("When storage type is AzureBlob it should build Azure upload args", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := &hyperv1.HCPEtcdBackup{ + Spec: hyperv1.HCPEtcdBackupSpec{ + Storage: hyperv1.HCPEtcdBackupStorage{ + StorageType: hyperv1.AzureBlobBackupStorage, + AzureBlob: hyperv1.HCPEtcdBackupAzureBlob{ + Container: "my-container", + StorageAccount: "mystorageaccount", + KeyPrefix: "backups", + Credentials: hyperv1.SecretReference{Name: "azure-creds"}, + }, + }, + }, + } + r := newReconciler() + + args, credSecret, err := r.buildUploadArgs(backup) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(credSecret).To(Equal("azure-creds")) + g.Expect(args).To(ContainElements( + "--storage-type", "AzureBlob", + "--azure-container", "my-container", + "--azure-storage-account", "mystorageaccount", + )) + }) +} + +func TestEnforceRetention(t *testing.T) { + t.Run("When completed backup count exceeds max it should delete oldest", func(t *testing.T) { + g := NewGomegaWithT(t) + + baseTime := time.Now().Add(-7 * time.Hour) + var backups []client.Object + for i := range 7 { + b := &hyperv1.HCPEtcdBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("backup-%d", i), + Namespace: testHCPNamespace, + CreationTimestamp: metav1.NewTime(baseTime.Add(time.Duration(i) * time.Hour)), + }, + Status: hyperv1.HCPEtcdBackupStatus{ + Conditions: []metav1.Condition{ + { + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionTrue, + Reason: hyperv1.BackupSucceededReason, + }, + }, + }, + } + backups = append(backups, b) + } + + r := newReconciler(backups...) + r.MaxBackupCount = 5 + + err := r.enforceRetention(context.Background(), testHCPNamespace) + g.Expect(err).ToNot(HaveOccurred()) + + // Should have deleted the 2 oldest (backup-0, backup-1) + remaining := &hyperv1.HCPEtcdBackupList{} + g.Expect(r.List(context.Background(), remaining, client.InNamespace(testHCPNamespace))).To(Succeed()) + g.Expect(remaining.Items).To(HaveLen(5)) + + remainingNames := make([]string, len(remaining.Items)) + for i, b := range remaining.Items { + remainingNames[i] = b.Name + } + g.Expect(remainingNames).To(ContainElements("backup-2", "backup-3", "backup-4", "backup-5", "backup-6")) + g.Expect(remainingNames).ToNot(ContainElement("backup-0")) + g.Expect(remainingNames).ToNot(ContainElement("backup-1")) + }) + + t.Run("When MaxBackupCount is 0 it should not enforce retention", func(t *testing.T) { + g := NewGomegaWithT(t) + r := newReconciler() + r.MaxBackupCount = 0 + + err := r.enforceRetention(context.Background(), testHCPNamespace) + g.Expect(err).ToNot(HaveOccurred()) + }) + + t.Run("When completed backup count is within max it should not delete", func(t *testing.T) { + g := NewGomegaWithT(t) + + var backups []client.Object + for i := range 3 { + b := &hyperv1.HCPEtcdBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "backup-" + string(rune('a'+i)), + Namespace: testHCPNamespace, + }, + Status: hyperv1.HCPEtcdBackupStatus{ + Conditions: []metav1.Condition{ + { + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionTrue, + Reason: hyperv1.BackupSucceededReason, + }, + }, + }, + } + backups = append(backups, b) + } + + r := newReconciler(backups...) + r.MaxBackupCount = 5 + + err := r.enforceRetention(context.Background(), testHCPNamespace) + g.Expect(err).ToNot(HaveOccurred()) + + remaining := &hyperv1.HCPEtcdBackupList{} + g.Expect(r.List(context.Background(), remaining, client.InNamespace(testHCPNamespace))).To(Succeed()) + g.Expect(remaining.Items).To(HaveLen(3)) + }) +} + +func TestCheckEtcdHealth(t *testing.T) { + t.Run("When etcd StatefulSet is fully ready it should return healthy", func(t *testing.T) { + g := NewGomegaWithT(t) + sts := newEtcdStatefulSet(3, 3) + r := newReconciler(sts) + + healthy, msg, err := r.checkEtcdHealth(context.Background(), testHCPNamespace) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(healthy).To(BeTrue()) + g.Expect(msg).To(BeEmpty()) + }) + + t.Run("When etcd StatefulSet has fewer ready replicas it should return unhealthy", func(t *testing.T) { + g := NewGomegaWithT(t) + sts := newEtcdStatefulSet(2, 3) + r := newReconciler(sts) + + healthy, msg, err := r.checkEtcdHealth(context.Background(), testHCPNamespace) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(healthy).To(BeFalse()) + g.Expect(msg).To(ContainSubstring("2/3")) + }) +} + +func TestFindActiveJob(t *testing.T) { + t.Run("When no active jobs exist it should return nil", func(t *testing.T) { + g := NewGomegaWithT(t) + r := newReconciler() + + job, err := r.findActiveJob(context.Background(), testHCPNamespace) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(job).To(BeNil()) + }) + + t.Run("When active job exists it should return it", func(t *testing.T) { + g := NewGomegaWithT(t) + activeJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-backup-active", + Namespace: testHONamespace, + Labels: map[string]string{ + LabelApp: LabelName, + LabelHCPNamespace: testHCPNamespace, + }, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "test", Image: "test:latest"}}, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + Status: batchv1.JobStatus{ + Active: 1, + }, + } + r := newReconciler(activeJob) + + job, err := r.findActiveJob(context.Background(), testHCPNamespace) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(job).ToNot(BeNil()) + g.Expect(job.Name).To(Equal("etcd-backup-active")) + }) +} + +func TestCreateBackupJob(t *testing.T) { + t.Run("When creating a backup Job it should build correct 3-container PodSpec", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + pullSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: pullSecretName, + Namespace: testHCPNamespace, + }, + Data: map[string][]byte{ + corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), + }, + } + credSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aws-creds", + Namespace: testHONamespace, + }, + Data: map[string][]byte{ + "credentials": []byte("fake-creds"), + }, + } + r := newReconciler(backup, hcp, pullSecret, credSecret) + ctx := context.Background() + + err := r.createBackupJob(ctx, backup, hcp) + g.Expect(err).ToNot(HaveOccurred()) + + // Find the created Job + jobList := &batchv1.JobList{} + g.Expect(r.List(ctx, jobList, client.InNamespace(testHONamespace))).To(Succeed()) + g.Expect(jobList.Items).To(HaveLen(1)) + + job := &jobList.Items[0] + + // Verify labels + g.Expect(job.Labels[LabelApp]).To(Equal(LabelName)) + g.Expect(job.Labels[labelHCP]).To(Equal(testHCPName)) + g.Expect(job.Labels[LabelBackupName]).To(Equal(testBackupName)) + g.Expect(job.Labels[LabelHCPNamespace]).To(Equal(testHCPNamespace)) + + // Verify Job spec + g.Expect(*job.Spec.TTLSecondsAfterFinished).To(Equal(int32(600))) + g.Expect(*job.Spec.ActiveDeadlineSeconds).To(Equal(int64(900))) + g.Expect(*job.Spec.BackoffLimit).To(Equal(int32(0))) + + podSpec := job.Spec.Template.Spec + + // Verify ServiceAccount + g.Expect(podSpec.ServiceAccountName).To(Equal(jobServiceAccountName)) + g.Expect(podSpec.RestartPolicy).To(Equal(corev1.RestartPolicyNever)) + + // Verify volumes + g.Expect(podSpec.Volumes).To(HaveLen(3)) + g.Expect(podSpec.Volumes[0].Name).To(Equal(volumeEtcdCerts)) + g.Expect(podSpec.Volumes[0].EmptyDir).ToNot(BeNil()) + g.Expect(podSpec.Volumes[1].Name).To(Equal(volumeEtcdBackup)) + g.Expect(podSpec.Volumes[1].EmptyDir).ToNot(BeNil()) + g.Expect(podSpec.Volumes[2].Name).To(Equal(volumeCredentials)) + g.Expect(podSpec.Volumes[2].Secret.SecretName).To(Equal("aws-creds")) + + // Verify init containers + g.Expect(podSpec.InitContainers).To(HaveLen(2)) + + fetchCerts := podSpec.InitContainers[0] + g.Expect(fetchCerts.Name).To(Equal("fetch-certs")) + g.Expect(fetchCerts.Image).To(Equal(testCPOImage)) + g.Expect(fetchCerts.Command).To(ContainElements("fetch-etcd-certs", "--hcp-namespace", testHCPNamespace)) + + snapshot := podSpec.InitContainers[1] + g.Expect(snapshot.Name).To(Equal("snapshot")) + g.Expect(snapshot.Image).To(Equal(testEtcdImage)) + g.Expect(snapshot.Command).To(ContainElement("/usr/bin/etcdctl")) + g.Expect(snapshot.Command).To(ContainElement("snapshot")) + g.Expect(snapshot.Env).To(ContainElement(corev1.EnvVar{Name: "ETCDCTL_API", Value: "3"})) + + // Verify main container + g.Expect(podSpec.Containers).To(HaveLen(1)) + upload := podSpec.Containers[0] + g.Expect(upload.Name).To(Equal("upload")) + g.Expect(upload.Image).To(Equal(testCPOImage)) + g.Expect(upload.Command).To(ContainElements("etcd-upload", "--storage-type", "S3")) + g.Expect(upload.Command).To(ContainElements("--aws-bucket", "my-bucket")) + g.Expect(upload.Command).To(ContainElements("--aws-region", "us-east-1")) + }) + + t.Run("When credential Secret does not exist it should return an error", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + pullSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: pullSecretName, + Namespace: testHCPNamespace, + }, + Data: map[string][]byte{ + corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), + }, + } + // No credential secret created + r := newReconciler(backup, hcp, pullSecret) + ctx := context.Background() + + err := r.createBackupJob(ctx, backup, hcp) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("credential Secret")) + g.Expect(err.Error()).To(ContainSubstring("aws-creds")) + }) + + t.Run("When storage type is AzureBlob it should build correct Azure upload args", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := &hyperv1.HCPEtcdBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + Spec: hyperv1.HCPEtcdBackupSpec{ + Storage: hyperv1.HCPEtcdBackupStorage{ + StorageType: hyperv1.AzureBlobBackupStorage, + AzureBlob: hyperv1.HCPEtcdBackupAzureBlob{ + Container: "my-container", + StorageAccount: "mystorageaccount", + KeyPrefix: "backups/test", + Credentials: hyperv1.SecretReference{Name: "azure-creds"}, + }, + }, + }, + } + hcp := newHostedControlPlane() + pullSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: pullSecretName, + Namespace: testHCPNamespace, + }, + Data: map[string][]byte{ + corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), + }, + } + credSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "azure-creds", + Namespace: testHONamespace, + }, + } + r := newReconciler(backup, hcp, pullSecret, credSecret) + ctx := context.Background() + + err := r.createBackupJob(ctx, backup, hcp) + g.Expect(err).ToNot(HaveOccurred()) + + jobList := &batchv1.JobList{} + g.Expect(r.List(ctx, jobList, client.InNamespace(testHONamespace))).To(Succeed()) + g.Expect(jobList.Items).To(HaveLen(1)) + + upload := jobList.Items[0].Spec.Template.Spec.Containers[0] + g.Expect(upload.Command).To(ContainElements("etcd-upload", "--storage-type", "AzureBlob")) + g.Expect(upload.Command).To(ContainElements("--azure-container", "my-container")) + g.Expect(upload.Command).To(ContainElements("--azure-storage-account", "mystorageaccount")) + + // Verify credential volume uses Azure secret + credVolume := jobList.Items[0].Spec.Template.Spec.Volumes[2] + g.Expect(credVolume.Secret.SecretName).To(Equal("azure-creds")) + }) + + t.Run("When pull secret does not exist it should return an error", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + // No pull secret created + r := newReconciler(backup, hcp) + ctx := context.Background() + + err := r.createBackupJob(ctx, backup, hcp) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("pull secret")) + }) + + t.Run("When KMS key is set it should include kms-key-arn in upload args", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + backup.Spec.Storage.S3.KMSKeyARN = "arn:aws:kms:us-east-1:123456789012:key/test-key" + hcp := newHostedControlPlane() + pullSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: pullSecretName, + Namespace: testHCPNamespace, + }, + Data: map[string][]byte{ + corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), + }, + } + credSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aws-creds", + Namespace: testHONamespace, + }, + } + r := newReconciler(backup, hcp, pullSecret, credSecret) + ctx := context.Background() + + err := r.createBackupJob(ctx, backup, hcp) + g.Expect(err).ToNot(HaveOccurred()) + + jobList := &batchv1.JobList{} + g.Expect(r.List(ctx, jobList, client.InNamespace(testHONamespace))).To(Succeed()) + g.Expect(jobList.Items).To(HaveLen(1)) + + upload := jobList.Items[0].Spec.Template.Spec.Containers[0] + g.Expect(upload.Command).To(ContainElements("--aws-kms-key-arn", "arn:aws:kms:us-east-1:123456789012:key/test-key")) + }) +} + +func TestReconcileHappyPath(t *testing.T) { + t.Run("When etcd is healthy and no active Job exists it should create backup resources and Job", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + hcp := newHostedControlPlane() + sts := newEtcdStatefulSet(3, 3) + pullSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: pullSecretName, + Namespace: testHCPNamespace, + }, + Data: map[string][]byte{ + corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), + }, + } + credSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aws-creds", + Namespace: testHONamespace, + }, + Data: map[string][]byte{ + "credentials": []byte("fake-creds"), + }, + } + r := newReconciler(backup, hcp, sts, pullSecret, credSecret) + ctx := context.Background() + + result, err := r.Reconcile(ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: testBackupName, + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result.RequeueAfter).To(Equal(requeueInterval)) + + // Verify Job was created + jobList := &batchv1.JobList{} + g.Expect(r.List(ctx, jobList, client.InNamespace(testHONamespace))).To(Succeed()) + g.Expect(jobList.Items).To(HaveLen(1)) + g.Expect(jobList.Items[0].Labels[LabelBackupName]).To(Equal(testBackupName)) + + // Verify ServiceAccount was created + sa := &corev1.ServiceAccount{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: jobServiceAccountName, Namespace: testHONamespace}, sa)).To(Succeed()) + + // Verify RBAC was created in HCP namespace + role := &rbacv1.Role{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, role)).To(Succeed()) + rb := &rbacv1.RoleBinding{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, rb)).To(Succeed()) + + // Verify NetworkPolicy was created in HCP namespace + np := &networkingv1.NetworkPolicy{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: NetworkPolicyName, Namespace: testHCPNamespace}, np)).To(Succeed()) + + // Verify backup status set to BackupInProgress + updated := &hyperv1.HCPEtcdBackup{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + g.Expect(updated.Status.Conditions).To(HaveLen(1)) + g.Expect(updated.Status.Conditions[0].Reason).To(Equal(hyperv1.BackupInProgressReason)) + g.Expect(updated.Status.Conditions[0].Status).To(Equal(metav1.ConditionFalse)) + + // Verify HCP condition was set + updatedHCP := &hyperv1.HostedControlPlane{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: testHCPName, Namespace: testHCPNamespace}, updatedHCP)).To(Succeed()) + hcpCond := meta.FindStatusCondition(updatedHCP.Status.Conditions, string(hyperv1.EtcdBackupSucceeded)) + g.Expect(hcpCond).ToNot(BeNil()) + g.Expect(hcpCond.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(hcpCond.Reason).To(Equal(hyperv1.BackupInProgressReason)) + }) +} diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go index c1dd9a31d31a..2b2c9750eb25 100644 --- a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go +++ b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go @@ -830,6 +830,7 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques hyperv1.HostedClusterRestoredFromBackup, hyperv1.DataPlaneConnectionAvailable, hyperv1.ControlPlaneConnectionAvailable, + hyperv1.EtcdBackupSucceeded, } for _, conditionType := range hcpConditions { diff --git a/hypershift-operator/main.go b/hypershift-operator/main.go index c1a85a3040a0..e8f0196bd177 100644 --- a/hypershift-operator/main.go +++ b/hypershift-operator/main.go @@ -27,6 +27,7 @@ import ( pkiconfig "github.com/openshift/hypershift/control-plane-pki-operator/config" etcdrecovery "github.com/openshift/hypershift/etcd-recovery" "github.com/openshift/hypershift/hypershift-operator/controllers/auditlogpersistence" + "github.com/openshift/hypershift/hypershift-operator/controllers/etcdbackup" "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster" hcmetrics "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/metrics" "github.com/openshift/hypershift/hypershift-operator/controllers/hostedclustersizing" @@ -138,6 +139,7 @@ type StartOptions struct { EnableDedicatedRequestServingIsolation bool ScaleFromZeroProvider string ScaleFromZeroCreds string + EtcdBackupMaxCount int } func NewStartCommand() *cobra.Command { @@ -176,6 +178,7 @@ func NewStartCommand() *cobra.Command { cmd.Flags().BoolVar(&opts.EnableDedicatedRequestServingIsolation, "enable-dedicated-request-serving-isolation", true, "If true, enables scheduling of request serving components to dedicated nodes") cmd.Flags().StringVar(&opts.ScaleFromZeroProvider, "scale-from-zero-provider", opts.ScaleFromZeroProvider, "Platform type for scale-from-zero autoscaling (aws)") cmd.Flags().StringVar(&opts.ScaleFromZeroCreds, "scale-from-zero-creds", opts.ScaleFromZeroCreds, "Path to credentials file for scale-from-zero instance type queries") + cmd.Flags().IntVar(&opts.EtcdBackupMaxCount, "etcd-backup-max-count", 5, "Maximum number of completed HCPEtcdBackup CRs to retain per HostedControlPlane") // Attempt to determine featureset prior to adding featuregate flags. // It is safe to get the empty string from this as the empty string is the default featureset. @@ -212,6 +215,10 @@ func NewStartCommand() *cobra.Command { func run(ctx context.Context, opts *StartOptions, log logr.Logger) error { log.Info("Starting hypershift-operator-manager", "version", supportedversion.String()) + if opts.EtcdBackupMaxCount < 1 { + return fmt.Errorf("--etcd-backup-max-count must be at least 1, got %d", opts.EtcdBackupMaxCount) + } + // Validate scale-from-zero configuration early supportedProviders := set.New("aws") if opts.ScaleFromZeroCreds != "" { @@ -632,6 +639,19 @@ func run(ctx context.Context, opts *StartOptions, log logr.Logger) error { log.Info("UWM telemetry remote write controller disabled") } + if featuregate.Gate().Enabled(featuregate.HCPEtcdBackup) { + etcdBackupReconciler := &etcdbackup.HCPEtcdBackupReconciler{ + Client: mgr.GetClient(), + OperatorNamespace: opts.Namespace, + ReleaseProvider: registryProvider.ReleaseProvider, + HypershiftOperatorImage: operatorImage, + MaxBackupCount: opts.EtcdBackupMaxCount, + } + if err := etcdBackupReconciler.SetupWithManager(mgr); err != nil { + return fmt.Errorf("unable to create etcd backup controller: %w", err) + } + } + if sharedingress.UseSharedIngress() { sharedIngress := sharedingress.SharedIngressReconciler{ Namespace: opts.Namespace, diff --git a/support/config/constants.go b/support/config/constants.go index 8c8eeb45d1e8..f1cb399706d6 100644 --- a/support/config/constants.go +++ b/support/config/constants.go @@ -28,6 +28,7 @@ const ( DefaultAdvertiseIPv4Address = "172.20.0.1" DefaultAdvertiseIPv6Address = "fd00::1" DefaultEtcdURL = "https://etcd-client:2379" + EtcdClientPort = 2379 // KASSVCLBAzurePort is needed because for Azure we currently hardcode 7443 for the SVC LB as 6443 collides with public LB rule for the management cluster. // https://bugzilla.redhat.com/show_bug.cgi?id=2060650 // TODO(alberto): explore exposing multiple Azure frontend IPs on the load balancer. diff --git a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/etcdbackup_types.go b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/etcdbackup_types.go index d667e2619515..d59ef4752a95 100644 --- a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/etcdbackup_types.go +++ b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/etcdbackup_types.go @@ -20,10 +20,11 @@ const ( // BackupCompleted indicates whether the etcd backup has completed. BackupCompleted ConditionType = "BackupCompleted" - BackupSucceededReason string = "BackupSucceeded" - BackupFailedReason string = "BackupFailed" - BackupAlreadyInProgressReason string = "BackupAlreadyInProgress" - EtcdUnhealthyReason string = "EtcdUnhealthy" + BackupSucceededReason string = "BackupSucceeded" + BackupFailedReason string = "BackupFailed" + BackupInProgressReason string = "BackupInProgress" + BackupRejectedReason string = "BackupRejected" + EtcdUnhealthyReason string = "EtcdUnhealthy" ) // HCPEtcdBackupStorageType is the type of storage for etcd backups. diff --git a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go index c43bf81992f5..59e13f4cba67 100644 --- a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go +++ b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go @@ -195,6 +195,11 @@ const ( // recovery job was triggered. EtcdRecoveryActive ConditionType = "EtcdRecoveryActive" + // EtcdBackupSucceeded bubbles up from HCP. It indicates the result of the + // most recent etcd backup. True means the last backup completed successfully; + // False means a backup is in progress or the last backup failed. + EtcdBackupSucceeded ConditionType = "EtcdBackupSucceeded" + // ClusterSizeComputed indicates that a t-shirt size was computed for this HostedCluster. // The last transition time for this condition is used to manage how quickly transitions occur. ClusterSizeComputed = "ClusterSizeComputed" From a8c34f77b0309519af1c03915abd8cf8d2b42065 Mon Sep 17 00:00:00 2001 From: Juan Manuel Parrilla Madrid Date: Mon, 6 Apr 2026 12:28:12 +0200 Subject: [PATCH 2/3] test(CNTRLPLANE-2678): add integration tests for HCPEtcdBackup controller Add integration tests that validate the HCPEtcdBackup controller end-to-end against a live management cluster with S3 and Azure Blob storage backends. Tests: - S3 happy path: backup completes with snapshotURL, RBAC/NetworkPolicy cleaned up, EtcdBackupSucceeded condition propagated to HCP - Azure Blob happy path: same validations for Azure storage - Invalid credentials: controller sets BackupFailed as terminal condition when credential Secret is missing waitForBackupCondition monitors both the CR condition and Job status to detect pod-level failures early (e.g. init container errors) instead of waiting for the full timeout. run.sh gains a `controller` subcommand that creates/destroys all cloud resources (S3 buckets, Azure RG/storage/SP, K8s Secrets) automatically via setup/teardown functions with EXIT trap. AWS credentials fallback now extracts only the [default] profile instead of copying the entire credentials file. Co-Authored-By: Claude Opus 4.6 Signed-off-by: Juan Manuel Parrilla Madrid --- .../controllers/etcdbackup/reconciler_test.go | 41 +- .../oadp/controller/controller_test.go | 567 ++++++++++++++++++ test/integration/oadp/run.sh | 357 ++++++++++- 3 files changed, 939 insertions(+), 26 deletions(-) create mode 100644 test/integration/oadp/controller/controller_test.go diff --git a/hypershift-operator/controllers/etcdbackup/reconciler_test.go b/hypershift-operator/controllers/etcdbackup/reconciler_test.go index 7bc5ccd719ab..92e1b0b3ae8e 100644 --- a/hypershift-operator/controllers/etcdbackup/reconciler_test.go +++ b/hypershift-operator/controllers/etcdbackup/reconciler_test.go @@ -370,24 +370,16 @@ func TestReconcile(t *testing.T) { g.Expect(result).To(Equal(ctrl.Result{})) }) - t.Run("When credential Secret does not exist it should set BackupFailed", func(t *testing.T) { + t.Run("When credential Secret does not exist it should set BackupFailed without creating RBAC or NetworkPolicy", func(t *testing.T) { g := NewGomegaWithT(t) backup := newHCPEtcdBackup() hcp := newHostedControlPlane() sts := newEtcdStatefulSet(3, 3) - pullSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: pullSecretName, - Namespace: testHCPNamespace, - }, - Data: map[string][]byte{ - corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), - }, - } - // No credential secret — should trigger BackupFailed - r := newReconciler(backup, hcp, sts, pullSecret) + // No credential secret — should trigger BackupFailed before creating any resources + r := newReconciler(backup, hcp, sts) + ctx := context.Background() - result, err := r.Reconcile(context.Background(), ctrl.Request{ + result, err := r.Reconcile(ctx, ctrl.Request{ NamespacedName: types.NamespacedName{ Name: testBackupName, Namespace: testHCPNamespace, @@ -397,12 +389,17 @@ func TestReconcile(t *testing.T) { g.Expect(result).To(Equal(ctrl.Result{})) updated := &hyperv1.HCPEtcdBackup{} - g.Expect(r.Get(context.Background(), types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) + g.Expect(r.Get(ctx, types.NamespacedName{Name: testBackupName, Namespace: testHCPNamespace}, updated)).To(Succeed()) g.Expect(updated.Status.Conditions).To(HaveLen(1)) g.Expect(updated.Status.Conditions[0].Type).To(Equal(string(hyperv1.BackupCompleted))) g.Expect(updated.Status.Conditions[0].Status).To(Equal(metav1.ConditionFalse)) g.Expect(updated.Status.Conditions[0].Reason).To(Equal(hyperv1.BackupFailedReason)) g.Expect(updated.Status.Conditions[0].Message).To(ContainSubstring("credential Secret")) + + // Verify no RBAC or NetworkPolicy was created (early validation prevents resource waste) + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, &rbacv1.Role{})).ToNot(Succeed()) + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, &rbacv1.RoleBinding{})).ToNot(Succeed()) + g.Expect(r.Get(ctx, types.NamespacedName{Name: NetworkPolicyName, Namespace: testHCPNamespace}, &networkingv1.NetworkPolicy{})).ToNot(Succeed()) }) t.Run("When backup failed it should be terminal", func(t *testing.T) { @@ -1059,7 +1056,7 @@ func TestCreateBackupJob(t *testing.T) { g.Expect(upload.Command).To(ContainElements("--aws-region", "us-east-1")) }) - t.Run("When credential Secret does not exist it should return an error", func(t *testing.T) { + t.Run("When credential Secret does not exist the Job should still be created", func(t *testing.T) { g := NewGomegaWithT(t) backup := newHCPEtcdBackup() hcp := newHostedControlPlane() @@ -1072,14 +1069,20 @@ func TestCreateBackupJob(t *testing.T) { corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`), }, } - // No credential secret created + // No credential secret — createBackupJob does not validate it; + // validation is done earlier in Reconcile via getCredentialSecretName + Get. r := newReconciler(backup, hcp, pullSecret) ctx := context.Background() err := r.createBackupJob(ctx, backup, hcp) - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring("credential Secret")) - g.Expect(err.Error()).To(ContainSubstring("aws-creds")) + g.Expect(err).ToNot(HaveOccurred()) + + jobList := &batchv1.JobList{} + g.Expect(r.List(ctx, jobList, client.InNamespace(testHONamespace))).To(Succeed()) + g.Expect(jobList.Items).To(HaveLen(1)) + // The Job references the credential Secret in its volume — Kubernetes will + // fail the Pod at runtime if it doesn't exist, but that's caught by Reconcile. + g.Expect(jobList.Items[0].Spec.Template.Spec.Volumes[2].Secret.SecretName).To(Equal("aws-creds")) }) t.Run("When storage type is AzureBlob it should build correct Azure upload args", func(t *testing.T) { diff --git a/test/integration/oadp/controller/controller_test.go b/test/integration/oadp/controller/controller_test.go new file mode 100644 index 000000000000..8b39c2e1eaa6 --- /dev/null +++ b/test/integration/oadp/controller/controller_test.go @@ -0,0 +1,567 @@ +//go:build integration +// +build integration + +// Package controller contains integration tests for the HCPEtcdBackup controller. +// +// These tests validate the end-to-end controller flow against a live +// management cluster hosting a HostedCluster. The controller creates +// RBAC, NetworkPolicy, and a backup Job, then reports status through +// the HCPEtcdBackup CR conditions. +// +// Required environment variables: +// - KUBECONFIG: path to the management cluster kubeconfig +// - ETCD_BACKUP_TEST_HCP_NAMESPACE: the HCP namespace (e.g. clusters-my-hcp) +// +// S3 test environment variables: +// - ETCD_BACKUP_TEST_S3_BUCKET: S3 bucket for backup storage +// - ETCD_BACKUP_TEST_S3_REGION: AWS region of the S3 bucket +// - ETCD_BACKUP_TEST_S3_KEY_PREFIX: S3 key prefix for backup files +// - ETCD_BACKUP_TEST_S3_CREDENTIALS_SECRET: name of the Secret containing AWS credentials +// +// Azure test environment variables: +// - ETCD_BACKUP_TEST_AZURE_CONTAINER: Azure Blob container name +// - ETCD_BACKUP_TEST_AZURE_STORAGE_ACCOUNT: Azure Storage Account name +// - ETCD_BACKUP_TEST_AZURE_KEY_PREFIX: blob name prefix for backup files +// - ETCD_BACKUP_TEST_AZURE_CREDENTIALS_SECRET: name of the Secret containing Azure credentials +// +// Optional environment variables: +// - ETCD_BACKUP_TEST_HO_NAMESPACE: the HO namespace (defaults to "hypershift") +// - ETCD_BACKUP_TEST_TIMEOUT: polling timeout in seconds (defaults to 300) +// - ETCD_BACKUP_TEST_POLL_INTERVAL: polling interval in seconds (defaults to 5) +// +// Run S3 tests: +// +// KUBECONFIG=/path/to/kubeconfig \ +// ETCD_BACKUP_TEST_HCP_NAMESPACE=clusters-my-hcp \ +// ETCD_BACKUP_TEST_S3_BUCKET=my-bucket \ +// ETCD_BACKUP_TEST_S3_REGION=us-east-2 \ +// ETCD_BACKUP_TEST_S3_KEY_PREFIX=etcd-backups/my-hcp \ +// ETCD_BACKUP_TEST_S3_CREDENTIALS_SECRET=hypershift-operator-aws-credentials \ +// go test -tags integration -v -timeout 10m ./test/integration/oadp/controller/... +// +// Run Azure tests: +// +// KUBECONFIG=/path/to/kubeconfig \ +// ETCD_BACKUP_TEST_HCP_NAMESPACE=clusters-my-hcp \ +// ETCD_BACKUP_TEST_AZURE_CONTAINER=oadp \ +// ETCD_BACKUP_TEST_AZURE_STORAGE_ACCOUNT=jparrill \ +// ETCD_BACKUP_TEST_AZURE_KEY_PREFIX=etcd-backups/my-hcp \ +// ETCD_BACKUP_TEST_AZURE_CREDENTIALS_SECRET=azure-backup-credentials \ +// go test -tags integration -v -timeout 10m ./test/integration/oadp/controller/... +package controller + +import ( + "context" + "fmt" + "net/url" + "os" + "os/exec" + "strconv" + "strings" + "testing" + "time" + + . "github.com/onsi/gomega" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/hypershift-operator/controllers/etcdbackup" + + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/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/util/wait" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + + ctrl "sigs.k8s.io/controller-runtime" + crclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/config" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + + "go.uber.org/zap/zapcore" +) + +func TestMain(m *testing.M) { + ctrl.SetLogger(zap.New(zap.UseDevMode(true), zap.JSONEncoder(func(o *zapcore.EncoderConfig) { + o.EncodeTime = zapcore.RFC3339TimeEncoder + }))) + os.Exit(m.Run()) +} + +const ( + defaultHONamespace = "hypershift" + defaultTimeout = 300 + defaultPollInterval = 5 + + // cleanupCmdTimeout is the timeout for cloud CLI cleanup commands. + cleanupCmdTimeout = 30 * time.Second +) + +type baseConfig struct { + HCPNamespace string + HONamespace string + Timeout time.Duration + PollInterval time.Duration +} + +type s3Config struct { + Bucket string + Region string + KeyPrefix string + CredentialsSecret string +} + +type azureConfig struct { + Container string + StorageAccount string + KeyPrefix string + CredentialsSecret string +} + +func loadBaseConfig(t *testing.T) baseConfig { + t.Helper() + + hcpNS := os.Getenv("ETCD_BACKUP_TEST_HCP_NAMESPACE") + if hcpNS == "" { + t.Skip("ETCD_BACKUP_TEST_HCP_NAMESPACE not set") + } + + hoNS := os.Getenv("ETCD_BACKUP_TEST_HO_NAMESPACE") + if hoNS == "" { + hoNS = defaultHONamespace + } + + timeout := defaultTimeout + if v := os.Getenv("ETCD_BACKUP_TEST_TIMEOUT"); v != "" { + if parsed, err := strconv.Atoi(v); err == nil { + timeout = parsed + } + } + + pollInterval := defaultPollInterval + if v := os.Getenv("ETCD_BACKUP_TEST_POLL_INTERVAL"); v != "" { + if parsed, err := strconv.Atoi(v); err == nil { + pollInterval = parsed + } + } + + return baseConfig{ + HCPNamespace: hcpNS, + HONamespace: hoNS, + Timeout: time.Duration(timeout) * time.Second, + PollInterval: time.Duration(pollInterval) * time.Second, + } +} + +func loadS3Config(t *testing.T) *s3Config { + t.Helper() + bucket := os.Getenv("ETCD_BACKUP_TEST_S3_BUCKET") + region := os.Getenv("ETCD_BACKUP_TEST_S3_REGION") + keyPrefix := os.Getenv("ETCD_BACKUP_TEST_S3_KEY_PREFIX") + credSecret := os.Getenv("ETCD_BACKUP_TEST_S3_CREDENTIALS_SECRET") + + if bucket == "" || region == "" || keyPrefix == "" || credSecret == "" { + return nil + } + return &s3Config{ + Bucket: bucket, + Region: region, + KeyPrefix: keyPrefix, + CredentialsSecret: credSecret, + } +} + +func loadAzureConfig(t *testing.T) *azureConfig { + t.Helper() + container := os.Getenv("ETCD_BACKUP_TEST_AZURE_CONTAINER") + storageAccount := os.Getenv("ETCD_BACKUP_TEST_AZURE_STORAGE_ACCOUNT") + keyPrefix := os.Getenv("ETCD_BACKUP_TEST_AZURE_KEY_PREFIX") + credSecret := os.Getenv("ETCD_BACKUP_TEST_AZURE_CREDENTIALS_SECRET") + + if container == "" || storageAccount == "" || keyPrefix == "" || credSecret == "" { + return nil + } + return &azureConfig{ + Container: container, + StorageAccount: storageAccount, + KeyPrefix: keyPrefix, + CredentialsSecret: credSecret, + } +} + +func newClient(t *testing.T) crclient.Client { + t.Helper() + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add client-go scheme: %v", err) + } + if err := hyperv1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add hypershift scheme: %v", err) + } + + restConfig, err := config.GetConfig() + if err != nil { + t.Fatalf("failed to get kubeconfig: %v", err) + } + + k8sClient, err := crclient.New(restConfig, crclient.Options{Scheme: scheme}) + if err != nil { + t.Fatalf("failed to create k8s client: %v", err) + } + return k8sClient +} + +// cleanupBackup deletes the HCPEtcdBackup CR, ignoring not-found errors. +func cleanupBackup(ctx context.Context, t *testing.T, k8sClient crclient.Client, backup *hyperv1.HCPEtcdBackup) { + t.Helper() + if err := k8sClient.Delete(ctx, backup); crclient.IgnoreNotFound(err) != nil { + t.Logf("warning: failed to delete HCPEtcdBackup %s/%s: %v", backup.Namespace, backup.Name, err) + } +} + +// cleanupSnapshot deletes the uploaded snapshot from cloud storage using the CLI. +// Supported URL formats: +// - S3: s3:/// +// - Azure: https://.blob.core.windows.net// +func cleanupSnapshot(t *testing.T, snapshotURL string) { + t.Helper() + if snapshotURL == "" { + return + } + + ctx, cancel := context.WithTimeout(context.Background(), cleanupCmdTimeout) + defer cancel() + + if strings.HasPrefix(snapshotURL, "s3://") { + t.Logf("Deleting S3 snapshot: %s", snapshotURL) + out, err := exec.CommandContext(ctx, "aws", "s3", "rm", snapshotURL).CombinedOutput() + if err != nil { + t.Logf("warning: failed to delete S3 snapshot %s: %v\n%s", snapshotURL, err, string(out)) + } + return + } + + if strings.Contains(snapshotURL, ".blob.core.windows.net/") { + parsed, err := url.Parse(snapshotURL) + if err != nil { + t.Logf("warning: failed to parse Azure snapshot URL %s: %v", snapshotURL, err) + return + } + host := parsed.Hostname() + account := strings.SplitN(host, ".", 2)[0] + // path is // + parts := strings.SplitN(strings.TrimPrefix(parsed.Path, "/"), "/", 2) + if len(parts) != 2 { + t.Logf("warning: unexpected Azure blob URL path: %s", parsed.Path) + return + } + container, blobName := parts[0], parts[1] + + t.Logf("Deleting Azure blob: account=%s container=%s blob=%s", account, container, blobName) + // Use account key for deletion to avoid RBAC issues with the current user. + keyOut, keyErr := exec.CommandContext(ctx, "az", "storage", "account", "keys", "list", + "--account-name", account, + "--query", "[0].value", + "-o", "tsv", + ).Output() + if keyErr != nil { + t.Logf("warning: failed to get storage account key for %s: %v", account, keyErr) + return + } + accountKey := strings.TrimSpace(string(keyOut)) + out, err := exec.CommandContext(ctx, "az", "storage", "blob", "delete", + "--account-name", account, + "--account-key", accountKey, + "--container-name", container, + "--name", blobName, + ).CombinedOutput() + if err != nil { + t.Logf("warning: failed to delete Azure blob %s: %v\n%s", snapshotURL, err, string(out)) + } + return + } + + t.Logf("warning: unknown snapshot URL scheme, skipping cleanup: %s", snapshotURL) +} + +// waitForBackupCondition polls the HCPEtcdBackup CR until the BackupCompleted +// condition reaches a terminal state (True, BackupFailed, or BackupRejected). +// It also monitors the backup Job to detect pod-level failures early (e.g. +// init container errors) instead of waiting for the full timeout. +func waitForBackupCondition(ctx context.Context, k8sClient crclient.Client, backup *hyperv1.HCPEtcdBackup, cfg baseConfig) (*metav1.Condition, error) { + var lastCond *metav1.Condition + err := wait.PollUntilContextTimeout(ctx, cfg.PollInterval, cfg.Timeout, true, func(ctx context.Context) (bool, error) { + if err := k8sClient.Get(ctx, crclient.ObjectKeyFromObject(backup), backup); err != nil { + return false, err + } + + cond := meta.FindStatusCondition(backup.Status.Conditions, string(hyperv1.BackupCompleted)) + if cond != nil { + lastCond = cond + if cond.Status == metav1.ConditionTrue { + return true, nil + } + if cond.Reason == hyperv1.BackupFailedReason || cond.Reason == hyperv1.BackupRejectedReason { + return true, nil + } + } + + // Check Job/Pod status to detect early failures (e.g. init container errors) + // before the controller has had time to update the CR condition. + jobList := &batchv1.JobList{} + if err := k8sClient.List(ctx, jobList, + crclient.InNamespace(cfg.HONamespace), + crclient.MatchingLabels{ + etcdbackup.LabelBackupName: backup.Name, + etcdbackup.LabelHCPNamespace: backup.Namespace, + }, + ); err == nil && len(jobList.Items) > 0 { + job := &jobList.Items[0] + for _, c := range job.Status.Conditions { + if c.Type == batchv1.JobFailed && c.Status == corev1.ConditionTrue { + return true, fmt.Errorf("backup Job %q failed: %s", job.Name, c.Message) + } + } + } + + return false, nil + }) + return lastCond, err +} + +// verifyBackupSuccess asserts that the backup completed successfully and +// that the controller cleaned up temporary resources (NetworkPolicy, RBAC). +// It also verifies the EtcdBackupSucceeded condition was propagated to the HCP. +func verifyBackupSuccess(ctx context.Context, t *testing.T, g Gomega, k8sClient crclient.Client, backup *hyperv1.HCPEtcdBackup, cfg baseConfig) { + t.Helper() + + cond, err := waitForBackupCondition(ctx, k8sClient, backup, cfg) + g.Expect(err).ToNot(HaveOccurred(), "timed out waiting for BackupCompleted condition") + g.Expect(cond).ToNot(BeNil(), "BackupCompleted condition not found") + + g.Expect(cond.Status).To(Equal(metav1.ConditionTrue), + "expected BackupCompleted=True, got reason=%s message=%s", cond.Reason, cond.Message) + g.Expect(cond.Reason).To(Equal(hyperv1.BackupSucceededReason)) + t.Logf("Backup completed successfully: reason=%s message=%s", cond.Reason, cond.Message) + + g.Expect(backup.Status.SnapshotURL).ToNot(BeEmpty(), "snapshotURL should be populated after successful backup") + t.Logf("Snapshot URL: %s", backup.Status.SnapshotURL) + + // Verify NetworkPolicy cleaned up + np := &networkingv1.NetworkPolicy{} + err = k8sClient.Get(ctx, crclient.ObjectKey{Name: etcdbackup.NetworkPolicyName, Namespace: cfg.HCPNamespace}, np) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), + "NetworkPolicy %s should be cleaned up after backup completion", etcdbackup.NetworkPolicyName) + t.Log("Verified: NetworkPolicy cleaned up") + + // Verify RBAC cleaned up + role := &rbacv1.Role{} + err = k8sClient.Get(ctx, crclient.ObjectKey{Name: etcdbackup.RBACName, Namespace: cfg.HCPNamespace}, role) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), + "Role %s should be cleaned up after backup completion", etcdbackup.RBACName) + + rb := &rbacv1.RoleBinding{} + err = k8sClient.Get(ctx, crclient.ObjectKey{Name: etcdbackup.RBACName, Namespace: cfg.HCPNamespace}, rb) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), + "RoleBinding %s should be cleaned up after backup completion", etcdbackup.RBACName) + t.Log("Verified: RBAC cleaned up") + + // Verify backup Job exists in HO namespace + jobList := &batchv1.JobList{} + g.Expect(k8sClient.List(ctx, jobList, + crclient.InNamespace(cfg.HONamespace), + crclient.MatchingLabels{ + etcdbackup.LabelBackupName: backup.Name, + etcdbackup.LabelHCPNamespace: cfg.HCPNamespace, + }, + )).To(Succeed()) + g.Expect(jobList.Items).To(HaveLen(1), "expected exactly one backup Job") + t.Logf("Backup Job: %s (succeeded=%d)", jobList.Items[0].Name, jobList.Items[0].Status.Succeeded) + + // Verify EtcdBackupSucceeded condition propagated to HCP + hcpList := &hyperv1.HostedControlPlaneList{} + g.Expect(k8sClient.List(ctx, hcpList, crclient.InNamespace(cfg.HCPNamespace))).To(Succeed()) + g.Expect(hcpList.Items).ToNot(BeEmpty(), "HostedControlPlane not found in namespace %s", cfg.HCPNamespace) + hcpCond := meta.FindStatusCondition(hcpList.Items[0].Status.Conditions, "EtcdBackupSucceeded") + g.Expect(hcpCond).ToNot(BeNil(), "EtcdBackupSucceeded condition should be set on HCP") + g.Expect(hcpCond.Status).To(Equal(metav1.ConditionTrue), + "expected EtcdBackupSucceeded=True on HCP, got reason=%s", hcpCond.Reason) + t.Logf("Verified: HCP EtcdBackupSucceeded condition: status=%s reason=%s", hcpCond.Status, hcpCond.Reason) +} + +// TestWhenS3BackupCRIsCreated_ControllerShouldCompleteBackupSuccessfully +// validates the full HCPEtcdBackup controller flow with S3 storage: +// 1. Create an HCPEtcdBackup CR with S3 storage config +// 2. Wait for the controller to reconcile and complete the backup +// 3. Verify BackupCompleted=True with snapshotURL populated +// 4. Verify controller cleaned up NetworkPolicy and RBAC after completion +// 5. Verify EtcdBackupSucceeded condition on HCP +func TestWhenS3BackupCRIsCreated_ControllerShouldCompleteBackupSuccessfully(t *testing.T) { + cfg := loadBaseConfig(t) + s3 := loadS3Config(t) + if s3 == nil { + t.Skip("S3 env vars not set (ETCD_BACKUP_TEST_S3_BUCKET, ETCD_BACKUP_TEST_S3_REGION, ETCD_BACKUP_TEST_S3_KEY_PREFIX, ETCD_BACKUP_TEST_S3_CREDENTIALS_SECRET)") + } + + ctx, cancel := context.WithTimeout(context.Background(), cfg.Timeout+1*time.Minute) + defer cancel() + + k8sClient := newClient(t) + g := NewWithT(t) + + backupName := fmt.Sprintf("etcd-backup-s3-%d", time.Now().Unix()) + backup := &hyperv1.HCPEtcdBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: backupName, + Namespace: cfg.HCPNamespace, + }, + Spec: hyperv1.HCPEtcdBackupSpec{ + Storage: hyperv1.HCPEtcdBackupStorage{ + StorageType: hyperv1.S3BackupStorage, + S3: hyperv1.HCPEtcdBackupS3{ + Bucket: s3.Bucket, + Region: s3.Region, + KeyPrefix: s3.KeyPrefix, + Credentials: hyperv1.SecretReference{ + Name: s3.CredentialsSecret, + }, + }, + }, + }, + } + + t.Cleanup(func() { + cleanupSnapshot(t, backup.Status.SnapshotURL) + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cleanupCancel() + t.Log("Cleaning up S3 HCPEtcdBackup CR...") + cleanupBackup(cleanupCtx, t, k8sClient, backup) + }) + + t.Logf("Creating HCPEtcdBackup %s in %s (S3: %s/%s)", backupName, cfg.HCPNamespace, s3.Bucket, s3.KeyPrefix) + g.Expect(k8sClient.Create(ctx, backup)).To(Succeed()) + + t.Log("Waiting for S3 backup to complete...") + verifyBackupSuccess(ctx, t, g, k8sClient, backup, cfg) +} + +// TestWhenAzureBlobBackupCRIsCreated_ControllerShouldCompleteBackupSuccessfully +// validates the full HCPEtcdBackup controller flow with Azure Blob storage: +// 1. Create an HCPEtcdBackup CR with AzureBlob storage config +// 2. Wait for the controller to reconcile and complete the backup +// 3. Verify BackupCompleted=True with snapshotURL populated +// 4. Verify controller cleaned up NetworkPolicy and RBAC after completion +// 5. Verify EtcdBackupSucceeded condition on HCP +func TestWhenAzureBlobBackupCRIsCreated_ControllerShouldCompleteBackupSuccessfully(t *testing.T) { + cfg := loadBaseConfig(t) + az := loadAzureConfig(t) + if az == nil { + t.Skip("Azure env vars not set (ETCD_BACKUP_TEST_AZURE_CONTAINER, ETCD_BACKUP_TEST_AZURE_STORAGE_ACCOUNT, ETCD_BACKUP_TEST_AZURE_KEY_PREFIX, ETCD_BACKUP_TEST_AZURE_CREDENTIALS_SECRET)") + } + + ctx, cancel := context.WithTimeout(context.Background(), cfg.Timeout+1*time.Minute) + defer cancel() + + k8sClient := newClient(t) + g := NewWithT(t) + + backupName := fmt.Sprintf("etcd-backup-azure-%d", time.Now().Unix()) + backup := &hyperv1.HCPEtcdBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: backupName, + Namespace: cfg.HCPNamespace, + }, + Spec: hyperv1.HCPEtcdBackupSpec{ + Storage: hyperv1.HCPEtcdBackupStorage{ + StorageType: hyperv1.AzureBlobBackupStorage, + AzureBlob: hyperv1.HCPEtcdBackupAzureBlob{ + Container: az.Container, + StorageAccount: az.StorageAccount, + KeyPrefix: az.KeyPrefix, + Credentials: hyperv1.SecretReference{ + Name: az.CredentialsSecret, + }, + }, + }, + }, + } + + t.Cleanup(func() { + cleanupSnapshot(t, backup.Status.SnapshotURL) + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cleanupCancel() + t.Log("Cleaning up Azure HCPEtcdBackup CR...") + cleanupBackup(cleanupCtx, t, k8sClient, backup) + }) + + t.Logf("Creating HCPEtcdBackup %s in %s (Azure: %s/%s/%s)", backupName, cfg.HCPNamespace, az.StorageAccount, az.Container, az.KeyPrefix) + g.Expect(k8sClient.Create(ctx, backup)).To(Succeed()) + + t.Log("Waiting for Azure backup to complete...") + verifyBackupSuccess(ctx, t, g, k8sClient, backup, cfg) +} + +// TestWhenBackupCRHasInvalidCredentials_ControllerShouldSetBackupFailed +// validates that the controller correctly reports failure when the +// credentials Secret does not exist. +func TestWhenBackupCRHasInvalidCredentials_ControllerShouldSetBackupFailed(t *testing.T) { + cfg := loadBaseConfig(t) + ctx, cancel := context.WithTimeout(context.Background(), cfg.Timeout+1*time.Minute) + defer cancel() + + k8sClient := newClient(t) + g := NewWithT(t) + + backupName := fmt.Sprintf("etcd-backup-fail-%d", time.Now().Unix()) + backup := &hyperv1.HCPEtcdBackup{ + ObjectMeta: metav1.ObjectMeta{ + Name: backupName, + Namespace: cfg.HCPNamespace, + }, + Spec: hyperv1.HCPEtcdBackupSpec{ + Storage: hyperv1.HCPEtcdBackupStorage{ + StorageType: hyperv1.S3BackupStorage, + S3: hyperv1.HCPEtcdBackupS3{ + Bucket: "nonexistent-bucket", + Region: "us-east-1", + KeyPrefix: "test", + Credentials: hyperv1.SecretReference{ + Name: "nonexistent-credentials-secret", + }, + }, + }, + }, + } + + t.Cleanup(func() { + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cleanupCancel() + t.Log("Cleaning up failed HCPEtcdBackup CR...") + cleanupBackup(cleanupCtx, t, k8sClient, backup) + }) + + t.Logf("Creating HCPEtcdBackup %s with invalid credentials", backupName) + g.Expect(k8sClient.Create(ctx, backup)).To(Succeed()) + + t.Log("Waiting for backup to reach terminal state...") + cond, err := waitForBackupCondition(ctx, k8sClient, backup, cfg) + g.Expect(err).ToNot(HaveOccurred(), "timed out waiting for terminal condition") + g.Expect(cond).ToNot(BeNil(), "BackupCompleted condition not found") + + g.Expect(cond.Status).To(Equal(metav1.ConditionFalse), + "expected BackupCompleted=False for invalid credentials") + g.Expect(cond.Reason).To(Equal(hyperv1.BackupFailedReason), + "expected reason BackupFailed, got %s: %s", cond.Reason, cond.Message) + t.Logf("Backup failed as expected: reason=%s message=%s", cond.Reason, cond.Message) + + // Verify controller cleaned up resources even on failure + np := &networkingv1.NetworkPolicy{} + err = k8sClient.Get(ctx, crclient.ObjectKey{Name: etcdbackup.NetworkPolicyName, Namespace: cfg.HCPNamespace}, np) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), + "NetworkPolicy should be cleaned up after backup failure") + t.Log("Verified: NetworkPolicy cleaned up after failure") +} diff --git a/test/integration/oadp/run.sh b/test/integration/oadp/run.sh index ced310d5ff56..bcb8a545cef7 100755 --- a/test/integration/oadp/run.sh +++ b/test/integration/oadp/run.sh @@ -6,11 +6,17 @@ # ./test/integration/oadp/run.sh upload - Run etcd-upload integration tests (S3 + Azure) # ./test/integration/oadp/run.sh upload aws - Run only S3 upload tests # ./test/integration/oadp/run.sh upload azure - Run only Azure Blob upload tests +# ./test/integration/oadp/run.sh controller - Run controller tests for all configured providers +# ./test/integration/oadp/run.sh controller aws - Run controller tests with S3 only +# ./test/integration/oadp/run.sh controller azure - Run controller tests with Azure Blob only # # Prerequisites: # - Build the CPO binary: make control-plane-operator # - For AWS tests: aws cli authenticated (aws sts get-caller-identity must succeed) # - For Azure tests: az cli authenticated (az account show must succeed) +# - For controller tests: KUBECONFIG pointing to management cluster with HCPEtcdBackup +# controller running, plus ETCD_BACKUP_TEST_HCP_NAMESPACE set. Cloud resources +# (S3 buckets, Azure storage, K8s Secrets) are created/destroyed automatically. set -o errexit set -o nounset @@ -89,10 +95,14 @@ function setup_aws() { session_token="${session_token:-${AWS_SESSION_TOKEN:-}}" if [[ -z "${access_key}" || -z "${secret_key}" ]]; then - # Fall back to default credentials file if it exists + # Fall back to credentials file: extract only the [default] profile if [[ -f "${HOME}/.aws/credentials" ]]; then - CREATED_AWS_CREDS_FILE="${HOME}/.aws/credentials" - log "Using existing credentials file: ${CREATED_AWS_CREDS_FILE}" + awk '/^\[default\]/{found=1; next} /^\[/{found=0} found && /^[^#]/' "${HOME}/.aws/credentials" > "${CREATED_AWS_CREDS_FILE}" + # Prepend the [default] header + sed -i '' '1i\ +[default] +' "${CREATED_AWS_CREDS_FILE}" 2>/dev/null || sed -i '1i[default]' "${CREATED_AWS_CREDS_FILE}" + log "Extracted [default] profile from credentials file: ${CREATED_AWS_CREDS_FILE}" else err "Cannot determine AWS credentials. Ensure aws cli is configured." fi @@ -373,21 +383,354 @@ function run_upload() { return ${test_result} } +# ── Controller Tests: AWS Setup/Teardown ──────────────────────────────────── + +CTRL_AWS_BUCKET="" +CTRL_AWS_REGION="" +CTRL_AWS_CREDS_SECRET="" + +function setup_controller_aws() { + require_command aws + require_command oc + + if ! aws sts get-caller-identity &>/dev/null; then + err "AWS CLI is not authenticated. Run 'aws configure' or set AWS credentials." + fi + + local ho_namespace="${ETCD_BACKUP_TEST_HO_NAMESPACE}" + + CTRL_AWS_REGION=$(aws configure get region 2>/dev/null || echo "us-east-1") + log "Using AWS region: ${CTRL_AWS_REGION}" + + # Create temporary S3 bucket + CTRL_AWS_BUCKET="etcd-ctrl-test-${SUFFIX}" + log "Creating temporary S3 bucket: ${CTRL_AWS_BUCKET}..." + if [[ "${CTRL_AWS_REGION}" == "us-east-1" ]]; then + aws s3api create-bucket \ + --bucket "${CTRL_AWS_BUCKET}" \ + --region "${CTRL_AWS_REGION}" \ + --output json >/dev/null 2>&1 + else + aws s3api create-bucket \ + --bucket "${CTRL_AWS_BUCKET}" \ + --region "${CTRL_AWS_REGION}" \ + --create-bucket-configuration LocationConstraint="${CTRL_AWS_REGION}" \ + --output json >/dev/null 2>&1 + fi + + # Build AWS credentials file content + local access_key secret_key session_token + access_key=$(aws configure get aws_access_key_id 2>/dev/null || echo "") + secret_key=$(aws configure get aws_secret_access_key 2>/dev/null || echo "") + session_token=$(aws configure get aws_session_token 2>/dev/null || echo "") + access_key="${access_key:-${AWS_ACCESS_KEY_ID:-}}" + secret_key="${secret_key:-${AWS_SECRET_ACCESS_KEY:-}}" + session_token="${session_token:-${AWS_SESSION_TOKEN:-}}" + + local creds_content="" + if [[ -n "${access_key}" && -n "${secret_key}" ]]; then + creds_content="[default] +aws_access_key_id = ${access_key} +aws_secret_access_key = ${secret_key}" + if [[ -n "${session_token}" ]]; then + creds_content="${creds_content} +aws_session_token = ${session_token}" + fi + elif [[ -f "${HOME}/.aws/credentials" ]]; then + # Extract only the [default] profile to avoid leaking other profiles + creds_content=$(awk '/^\[default\]/{found=1; print; next} /^\[/{found=0} found && /^[^#]/' "${HOME}/.aws/credentials") + else + err "Cannot determine AWS credentials." + fi + + # Create K8s Secret in HO namespace + CTRL_AWS_CREDS_SECRET="etcd-backup-aws-test-${SUFFIX}" + log "Creating K8s Secret ${CTRL_AWS_CREDS_SECRET} in ${ho_namespace}..." + local tmp_creds + tmp_creds=$(mktemp /tmp/etcd-ctrl-aws-creds.XXXXXX) + echo "${creds_content}" > "${tmp_creds}" + oc create secret generic "${CTRL_AWS_CREDS_SECRET}" \ + --from-file=credentials="${tmp_creds}" \ + -n "${ho_namespace}" + rm -f "${tmp_creds}" + + # Export env vars for Go tests + export ETCD_BACKUP_TEST_S3_BUCKET="${CTRL_AWS_BUCKET}" + export ETCD_BACKUP_TEST_S3_REGION="${CTRL_AWS_REGION}" + export ETCD_BACKUP_TEST_S3_KEY_PREFIX="etcd-backups/controller-test-${SUFFIX}" + export ETCD_BACKUP_TEST_S3_CREDENTIALS_SECRET="${CTRL_AWS_CREDS_SECRET}" +} + +function teardown_controller_aws() { + local ho_namespace="${ETCD_BACKUP_TEST_HO_NAMESPACE:-hypershift}" + + if [[ -n "${CTRL_AWS_CREDS_SECRET}" ]]; then + log "Deleting K8s Secret ${CTRL_AWS_CREDS_SECRET}..." + oc delete secret "${CTRL_AWS_CREDS_SECRET}" -n "${ho_namespace}" 2>/dev/null || true + fi + + if [[ -n "${CTRL_AWS_BUCKET}" ]]; then + log "Deleting S3 bucket ${CTRL_AWS_BUCKET} and its contents..." + aws s3 rb "s3://${CTRL_AWS_BUCKET}" \ + --force \ + --region "${CTRL_AWS_REGION}" 2>/dev/null || true + fi +} + +# ── Controller Tests: Azure Setup/Teardown ────────────────────────────────── + +CTRL_AZURE_RG="" +CTRL_AZURE_STORAGE_ACCOUNT="" +CTRL_AZURE_CONTAINER="" +CTRL_AZURE_SP_ID="" +CTRL_AZURE_CREDS_SECRET="" + +function setup_controller_azure() { + require_command az + require_command jq + require_command oc + + if ! az account show &>/dev/null; then + err "Azure CLI is not authenticated. Run 'az login'." + fi + + local ho_namespace="${ETCD_BACKUP_TEST_HO_NAMESPACE}" + local azure_sub_id azure_location="eastus" + azure_sub_id=$(az account show --query id -o tsv) + log "Using Azure subscription: ${azure_sub_id}" + + # Create temporary resource group + CTRL_AZURE_RG="etcd-ctrl-test-${SUFFIX}" + log "Creating temporary resource group: ${CTRL_AZURE_RG}..." + az group create \ + --name "${CTRL_AZURE_RG}" \ + --location "${azure_location}" \ + -o none + + # Create temporary storage account + CTRL_AZURE_STORAGE_ACCOUNT="etcdctrl${SUFFIX}" + log "Creating temporary storage account: ${CTRL_AZURE_STORAGE_ACCOUNT}..." + az storage account create \ + --name "${CTRL_AZURE_STORAGE_ACCOUNT}" \ + --resource-group "${CTRL_AZURE_RG}" \ + --location "${azure_location}" \ + --sku Standard_LRS \ + --kind StorageV2 \ + -o none + + # Create temporary container + CTRL_AZURE_CONTAINER="etcd-backup-test" + log "Creating temporary container: ${CTRL_AZURE_CONTAINER}..." + az storage container create \ + --name "${CTRL_AZURE_CONTAINER}" \ + --account-name "${CTRL_AZURE_STORAGE_ACCOUNT}" \ + --auth-mode login \ + -o none + + # Create temporary service principal with Storage Blob Data Contributor + local scope="/subscriptions/${azure_sub_id}/resourceGroups/${CTRL_AZURE_RG}/providers/Microsoft.Storage/storageAccounts/${CTRL_AZURE_STORAGE_ACCOUNT}" + log "Creating temporary service principal..." + local sp_json + sp_json=$(az ad sp create-for-rbac \ + --name "etcd-ctrl-test-${SUFFIX}" \ + --role "Storage Blob Data Contributor" \ + --scopes "${scope}" \ + -o json 2>/dev/null) + + local client_id tenant_id client_secret + client_id=$(echo "${sp_json}" | jq -r '.appId') + tenant_id=$(echo "${sp_json}" | jq -r '.tenant') + client_secret=$(echo "${sp_json}" | jq -r '.password') + + if [[ -z "${client_id}" || "${client_id}" == "null" ]]; then + err "Failed to create service principal: ${sp_json}" + fi + CTRL_AZURE_SP_ID="${client_id}" + + # Create K8s Secret in HO namespace with Azure credentials + CTRL_AZURE_CREDS_SECRET="etcd-backup-azure-test-${SUFFIX}" + log "Creating K8s Secret ${CTRL_AZURE_CREDS_SECRET} in ${ho_namespace}..." + local tmp_creds + tmp_creds=$(mktemp /tmp/etcd-ctrl-azure-creds.XXXXXX.json) + cat > "${tmp_creds}" </dev/null && \ + AZURE_CONFIG_DIR="${sp_config_dir}" az storage blob list \ + --account-name "${CTRL_AZURE_STORAGE_ACCOUNT}" \ + --container-name "${CTRL_AZURE_CONTAINER}" \ + --auth-mode login \ + -o none 2>/dev/null; then + log "RBAC propagation confirmed (attempt ${i}/${max_retries})" + break + fi + if [[ ${i} -eq ${max_retries} ]]; then + rm -rf "${sp_config_dir}" + err "Azure RBAC propagation timed out after $((max_retries * retry_interval))s" + fi + log "RBAC not ready yet, retrying in ${retry_interval}s (attempt ${i}/${max_retries})..." + sleep ${retry_interval} + done + rm -rf "${sp_config_dir}" + + # Export env vars for Go tests + export ETCD_BACKUP_TEST_AZURE_CONTAINER="${CTRL_AZURE_CONTAINER}" + export ETCD_BACKUP_TEST_AZURE_STORAGE_ACCOUNT="${CTRL_AZURE_STORAGE_ACCOUNT}" + export ETCD_BACKUP_TEST_AZURE_KEY_PREFIX="etcd-backups/controller-test-${SUFFIX}" + export ETCD_BACKUP_TEST_AZURE_CREDENTIALS_SECRET="${CTRL_AZURE_CREDS_SECRET}" +} + +function teardown_controller_azure() { + local ho_namespace="${ETCD_BACKUP_TEST_HO_NAMESPACE:-hypershift}" + + if [[ -n "${CTRL_AZURE_CREDS_SECRET}" ]]; then + log "Deleting K8s Secret ${CTRL_AZURE_CREDS_SECRET}..." + oc delete secret "${CTRL_AZURE_CREDS_SECRET}" -n "${ho_namespace}" 2>/dev/null || true + fi + + if [[ -n "${CTRL_AZURE_SP_ID}" ]]; then + log "Deleting service principal ${CTRL_AZURE_SP_ID}..." + az ad sp delete --id "${CTRL_AZURE_SP_ID}" 2>/dev/null || true + az ad app delete --id "${CTRL_AZURE_SP_ID}" 2>/dev/null || true + fi + + # Safety net: clean up leftover SPs + local leftover_sps + leftover_sps=$(az ad sp list --display-name "etcd-ctrl-test-${SUFFIX}" --query "[].appId" -o tsv 2>/dev/null || echo "") + for sp_id in ${leftover_sps}; do + log "Cleaning up leftover SP ${sp_id}..." + az ad sp delete --id "${sp_id}" 2>/dev/null || true + az ad app delete --id "${sp_id}" 2>/dev/null || true + done + + if [[ -n "${CTRL_AZURE_RG}" ]]; then + log "Deleting resource group ${CTRL_AZURE_RG} (cascades to all resources)..." + az group delete \ + --name "${CTRL_AZURE_RG}" \ + --yes \ + --no-wait \ + -o none 2>/dev/null || true + fi +} + +# ── Controller Tests: Run ─────────────────────────────────────────────────── + +function teardown_controller() { + log "Cleaning up controller test resources..." + teardown_controller_aws 2>/dev/null || true + teardown_controller_azure 2>/dev/null || true + log "Controller cleanup complete." +} + +function run_controller() { + local provider="${1:-all}" + local aws_ok=false azure_ok=false + local test_result=0 + + log "Running HCPEtcdBackup controller integration tests (provider: ${provider})..." + + # Required env vars + : "${KUBECONFIG:?KUBECONFIG must be set}" + : "${ETCD_BACKUP_TEST_HCP_NAMESPACE:?ETCD_BACKUP_TEST_HCP_NAMESPACE must be set}" + + require_command oc + + export KUBECONFIG + export ETCD_BACKUP_TEST_HCP_NAMESPACE + export ETCD_BACKUP_TEST_HO_NAMESPACE="${ETCD_BACKUP_TEST_HO_NAMESPACE:-hypershift}" + export ETCD_BACKUP_TEST_TIMEOUT="${ETCD_BACKUP_TEST_TIMEOUT:-300}" + export ETCD_BACKUP_TEST_POLL_INTERVAL="${ETCD_BACKUP_TEST_POLL_INTERVAL:-5}" + + log "HCP namespace: ${ETCD_BACKUP_TEST_HCP_NAMESPACE}" + log "HO namespace: ${ETCD_BACKUP_TEST_HO_NAMESPACE}" + log "Timeout: ${ETCD_BACKUP_TEST_TIMEOUT}s" + + # Detect available providers + case "${provider}" in + aws) + aws_ok=true + ;; + azure) + azure_ok=true + ;; + all) + if command -v aws &>/dev/null && aws sts get-caller-identity &>/dev/null 2>&1; then + aws_ok=true + else + log "AWS CLI not authenticated, skipping S3 controller tests" + fi + if command -v az &>/dev/null && az account show &>/dev/null 2>&1; then + azure_ok=true + else + log "Azure CLI not authenticated, skipping Azure controller tests" + fi + ;; + esac + + if [[ "${aws_ok}" == "false" && "${azure_ok}" == "false" ]]; then + err "No cloud provider authenticated. Run 'aws configure' and/or 'az login' first." + fi + + # Ensure cleanup runs on exit + trap 'teardown_controller' EXIT + + # Setup + if [[ "${aws_ok}" == "true" ]]; then + setup_controller_aws + fi + if [[ "${azure_ok}" == "true" ]]; then + setup_controller_azure + fi + + # Run tests + log "Running controller integration tests..." + # Go test timeout must be larger than the polling timeout to avoid panics. + # The test adds 1min buffer to the context; we add 2min to the go test timeout. + local go_test_timeout=$((ETCD_BACKUP_TEST_TIMEOUT + 120)) + go test -tags integration -v -timeout "${go_test_timeout}s" \ + ./test/integration/oadp/controller/... || test_result=$? + + return ${test_result} +} + # ── Main ───────────────────────────────────────────────────────────────────── if [[ $# -eq 0 ]]; then - echo "Usage: $0 {cli|upload [aws|azure]}" + echo "Usage: $0 {cli|upload [aws|azure]|controller [aws|azure]}" exit 1 fi command="${1}" shift case "${command}" in - "cli") run_cli ;; - "upload") run_upload "${1:-all}" ;; + "cli") run_cli ;; + "upload") run_upload "${1:-all}" ;; + "controller") run_controller "${1:-all}" ;; *) echo "Unknown command: ${command}" - echo "Usage: $0 {cli|upload [aws|azure]}" + echo "Usage: $0 {cli|upload [aws|azure]|controller [aws|azure]}" exit 1 ;; esac From b920edf05e71a61ae18d736ee9aafe29660325cb Mon Sep 17 00:00:00 2001 From: Juan Manuel Parrilla Madrid Date: Wed, 8 Apr 2026 09:43:29 +0200 Subject: [PATCH 3/3] fix(CNTRLPLANE-2678): prevent rejected backup from deleting shared resources When two HCPEtcdBackup CRs are created simultaneously, the second is rejected by the serial guard. However, the rejected backup entering a terminal state triggered cleanupResources, which deleted the NetworkPolicy and RBAC that the first (active) backup's Job still needs, causing it to timeout. Add an active Job guard to cleanupResources: before deleting shared resources, check if any backup Job is still running in the same HCP namespace. If so, skip the cleanup and log a message. Co-Authored-By: Claude Opus 4.6 Signed-off-by: Juan Manuel Parrilla Madrid --- .../controllers/etcdbackup/reconciler.go | 25 ++++ .../controllers/etcdbackup/reconciler_test.go | 136 ++++++++++++++++++ 2 files changed, 161 insertions(+) diff --git a/hypershift-operator/controllers/etcdbackup/reconciler.go b/hypershift-operator/controllers/etcdbackup/reconciler.go index e30581830ebc..09299d5f5155 100644 --- a/hypershift-operator/controllers/etcdbackup/reconciler.go +++ b/hypershift-operator/controllers/etcdbackup/reconciler.go @@ -253,6 +253,18 @@ func (r *HCPEtcdBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reques } if err := r.createBackupJob(ctx, backup, hcp); err != nil { + if apierrors.IsNotFound(err) { + r.setCondition(backup, metav1.Condition{ + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupFailedReason, + Message: err.Error(), + }) + if statusErr := r.Status().Update(ctx, backup); statusErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", statusErr) + } + return ctrl.Result{}, nil + } return ctrl.Result{}, fmt.Errorf("failed to create backup Job: %w", err) } @@ -624,8 +636,21 @@ func (r *HCPEtcdBackupReconciler) ensureNetworkPolicy(ctx context.Context, backu } // cleanupResources removes temporary NetworkPolicy and RBAC from the HCP namespace. +// It skips deletion if another backup Job is still active in the same HCP namespace, +// because the shared resources (NetworkPolicy, RBAC) are needed by that Job. func (r *HCPEtcdBackupReconciler) cleanupResources(ctx context.Context, backup *hyperv1.HCPEtcdBackup) error { logger := log.FromContext(ctx) + + // Guard: don't delete shared resources while another backup Job is active. + activeJob, err := r.findActiveJob(ctx, backup.Namespace) + if err != nil { + return fmt.Errorf("failed to check for active jobs before cleanup: %w", err) + } + if activeJob != nil { + logger.Info("skipping cleanup: another backup Job is still active", "activeJob", activeJob.Name) + return nil + } + var firstErr error // Delete NetworkPolicy diff --git a/hypershift-operator/controllers/etcdbackup/reconciler_test.go b/hypershift-operator/controllers/etcdbackup/reconciler_test.go index 92e1b0b3ae8e..ce4175145804 100644 --- a/hypershift-operator/controllers/etcdbackup/reconciler_test.go +++ b/hypershift-operator/controllers/etcdbackup/reconciler_test.go @@ -402,6 +402,82 @@ func TestReconcile(t *testing.T) { g.Expect(r.Get(ctx, types.NamespacedName{Name: NetworkPolicyName, Namespace: testHCPNamespace}, &networkingv1.NetworkPolicy{})).ToNot(Succeed()) }) + t.Run("When rejected backup reconciles with active Job from another backup it should not delete shared resources", func(t *testing.T) { + g := NewGomegaWithT(t) + + // Rejected backup (tc4b in the QE scenario) + rejectedBackup := newHCPEtcdBackup() + rejectedBackup.Name = "pr8139-tc4b" + rejectedBackup.Status.Conditions = []metav1.Condition{ + { + Type: string(hyperv1.BackupCompleted), + Status: metav1.ConditionFalse, + Reason: hyperv1.BackupRejectedReason, + Message: "rejected: another backup Job is already running", + LastTransitionTime: metav1.Now(), + }, + } + + // Active Job from the first backup (tc4a) + activeJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-backup-pr8139-tc4a-xyz", + Namespace: testHONamespace, + Labels: map[string]string{ + LabelApp: LabelName, + LabelBackupName: "pr8139-tc4a", + LabelHCPNamespace: testHCPNamespace, + }, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "test", Image: "test:latest"}}, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + Status: batchv1.JobStatus{Active: 1}, + } + + // Shared resources created by tc4a + np := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: NetworkPolicyName, + Namespace: testHCPNamespace, + }, + } + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: testHCPNamespace, + }, + } + rb := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: testHCPNamespace, + }, + } + + r := newReconciler(rejectedBackup, activeJob, np, role, rb) + ctx := context.Background() + + result, err := r.Reconcile(ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{ + Name: "pr8139-tc4b", + Namespace: testHCPNamespace, + }, + }) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(result).To(Equal(ctrl.Result{})) + + // Shared resources must still exist — the active Job needs them + g.Expect(r.Get(ctx, types.NamespacedName{Name: NetworkPolicyName, Namespace: testHCPNamespace}, &networkingv1.NetworkPolicy{})).To(Succeed()) + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, &rbacv1.Role{})).To(Succeed()) + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, &rbacv1.RoleBinding{})).To(Succeed()) + }) + t.Run("When backup failed it should be terminal", func(t *testing.T) { g := NewGomegaWithT(t) backup := newHCPEtcdBackup() @@ -590,6 +666,66 @@ func TestCleanupResources(t *testing.T) { err := r.cleanupResources(ctx, backup) g.Expect(err).ToNot(HaveOccurred()) }) + + t.Run("When another backup Job is active it should skip cleanup", func(t *testing.T) { + g := NewGomegaWithT(t) + backup := newHCPEtcdBackup() + + // Resources that should NOT be deleted + np := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: NetworkPolicyName, + Namespace: testHCPNamespace, + }, + } + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: testHCPNamespace, + }, + } + rb := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: RBACName, + Namespace: testHCPNamespace, + }, + } + + // Active Job from another backup in the same HCP namespace + activeJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-backup-other-backup", + Namespace: testHONamespace, + Labels: map[string]string{ + LabelApp: LabelName, + LabelBackupName: "other-backup", + LabelHCPNamespace: testHCPNamespace, + }, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "test", Image: "test:latest"}}, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + Status: batchv1.JobStatus{ + Active: 1, + }, + } + + r := newReconciler(backup, np, role, rb, activeJob) + ctx := context.Background() + + err := r.cleanupResources(ctx, backup) + g.Expect(err).ToNot(HaveOccurred()) + + // All resources should still exist + g.Expect(r.Get(ctx, types.NamespacedName{Name: NetworkPolicyName, Namespace: testHCPNamespace}, &networkingv1.NetworkPolicy{})).To(Succeed()) + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, &rbacv1.Role{})).To(Succeed()) + g.Expect(r.Get(ctx, types.NamespacedName{Name: RBACName, Namespace: testHCPNamespace}, &rbacv1.RoleBinding{})).To(Succeed()) + }) } func newTestJob(status batchv1.JobStatus) *batchv1.Job {