From 7355f0e4c557fcd145867c2579a9813424d03ffe Mon Sep 17 00:00:00 2001 From: OpenShift CI Bot Date: Wed, 24 Jun 2026 14:14:40 +0000 Subject: [PATCH] fix(control-plane-operator): add HCP finalizer to AWSEndpointService reconciler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a finalizer on the HostedControlPlane to block HCP deletion until all AWS PrivateLink resources (VPC endpoints, security groups, DNS records) are cleaned up by the AWSEndpointService reconciler. - Add hcpAWSPrivateLinkFinalizerName finalizer, placed after client initialization succeeds on the normal reconciliation path - Add reconcileHCPDeletion to clean up AWS resources for each AWSEndpointService CR before removing the HCP finalizer - Replace handler.Funcs{UpdateFunc: ...} HCP watch with EnqueueRequestsFromMapFunc so Create/Delete events also trigger reconciliation (critical for CPO restarts during HCP deletion) - Use convergent multi-CR coordination: each reconciler cleans its own CR, only the last one to finish removes the HCP finalizer - Move HCP deletion check before CR finalizer addition to prevent re-addition loop during HCP deletion cleanup cycles - Move AWS client initialization inside CR finalizer check in reconcileHCPDeletion so already-cleaned-up CRs proceed to the pending-CRs check without needing AWS API calls - Add comprehensive unit tests covering finalizer lifecycle, HCP deletion cleanup, CR deletion cleanup, concurrent reconciler coordination, and SharedVPC scenarios Previously, if the HCP was deleted before AWSEndpointService cleanup, the controller could not construct valid AWS clients — particularly for SharedVPC clusters where cross-account role ARNs are sourced from the HCP spec — orphaning AWS resources. Signed-off-by: OpenShift CI Bot Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 --- .../awsprivatelink_controller.go | 345 ++++-- .../awsprivatelink_controller_test.go | 987 ++++++++++++++++++ 2 files changed, 1248 insertions(+), 84 deletions(-) diff --git a/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go b/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go index 135663feb30c..191d3ce50f7c 100644 --- a/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go +++ b/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go @@ -46,8 +46,8 @@ import ( "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/event" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" @@ -219,6 +219,16 @@ const ( endpointServiceDeletionRequeueDuration = 5 * time.Second hypershiftLocalZone = "hypershift.local" routerDomain = "apps" + + // hcpAWSPrivateLinkFinalizerName is a finalizer placed on the HostedControlPlane by + // the AWSEndpointService reconciler. It blocks HCP deletion until all AWS PrivateLink + // resources (VPC endpoints, security groups, DNS records) are cleaned up. + // + // Without this finalizer, the AWSEndpointService CR's own finalizer runs during CR + // deletion, but the controller may not be able to construct valid AWS clients if the + // HCP has already been deleted — particularly for SharedVPC clusters where cross-account + // role ARNs are sourced from the HCP spec. This would orphan AWS resources. + hcpAWSPrivateLinkFinalizerName = "hypershift.openshift.io/aws-private-link-endpoint-cleanup" ) // AWSEndpointServiceReconciler watches AWSEndpointService resources and reconciles @@ -358,6 +368,7 @@ func (b *clientBuilder) setFromHCP(hcp *hyperv1.HostedControlPlane) { } else { b.assumeSharedVPCEndpointRoleARN = "" b.assumeSharedVPCRoute53RoleARN = "" + b.localZoneID = "" } } @@ -371,7 +382,9 @@ func (r *AWSEndpointServiceReconciler) SetupWithManager(mgr ctrl.Manager) error RateLimiter: workqueue.NewTypedItemExponentialFailureRateLimiter[reconcile.Request](3*time.Second, 30*time.Second), MaxConcurrentReconciles: 10, }). - Watches(&hyperv1.HostedControlPlane{}, handler.Funcs{UpdateFunc: r.enqueueOnAccessChange(mgr)}). + Watches(&hyperv1.HostedControlPlane{}, handler.EnqueueRequestsFromMapFunc( + r.mapHCPToAWSEndpointService(), + )). Build(r) if err != nil { return fmt.Errorf("failed setting up with a controller manager: %w", err) @@ -381,30 +394,42 @@ func (r *AWSEndpointServiceReconciler) SetupWithManager(mgr ctrl.Manager) error return nil } -func (r *AWSEndpointServiceReconciler) enqueueOnAccessChange(mgr ctrl.Manager) func(context.Context, event.UpdateEvent, workqueue.TypedRateLimitingInterface[reconcile.Request]) { - return func(ctx context.Context, e event.UpdateEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) { - logger := mgr.GetLogger() - newHCP, isOk := e.ObjectNew.(*hyperv1.HostedControlPlane) - if !isOk { - logger.Info("WARNING: enqueueOnAccessChange: new resource is not of type HostedControlPlane") - return +// mapHCPToAWSEndpointService maps HostedControlPlane events to the AWSEndpointService CRs +// in the same namespace. This uses EnqueueRequestsFromMapFunc (instead of handler.Funcs) +// so that all event types — Create, Update, Delete — trigger the mapping. This is critical +// for controller restarts: if the CPO restarts while an HCP is being deleted (DeletionTimestamp +// already set), the informer cache sync generates a Create event, not an Update. Using +// handler.Funcs{UpdateFunc: ...} would miss this, defeating the purpose of the HCP finalizer. +// +// The function only enqueues CRs when the HCP has our finalizer, avoiding unnecessary +// reconciliations for unrelated HCPs. EndpointAccess changes (previously detected via +// old/new comparison) are picked up by the reconciler's periodic 5-minute requeue. +func (r *AWSEndpointServiceReconciler) mapHCPToAWSEndpointService() handler.MapFunc { + return func(ctx context.Context, obj client.Object) []reconcile.Request { + hcp, ok := obj.(*hyperv1.HostedControlPlane) + if !ok { + return nil } - oldHCP, isOk := e.ObjectOld.(*hyperv1.HostedControlPlane) - if !isOk { - logger.Info("WARNING: enqueueOnAccessChange: old resource is not of type HostedControlPlane") - return + + // Only trigger reconciliation when the HCP has our finalizer; this avoids + // unnecessary reconciliations for HCPs that are not related to AWS PrivateLink. + if !controllerutil.ContainsFinalizer(hcp, hcpAWSPrivateLinkFinalizerName) { + return nil } - // Only enqueue awsendpointservices when there is a change in the endpointaccess value, otherwise ignore changes - if newHCP.Spec.Platform.AWS != nil && oldHCP.Spec.Platform.AWS != nil && newHCP.Spec.Platform.AWS.EndpointAccess != oldHCP.Spec.Platform.AWS.EndpointAccess { - awsEndpointServiceList := &hyperv1.AWSEndpointServiceList{} - if err := r.List(context.Background(), awsEndpointServiceList, client.InNamespace(newHCP.Namespace)); err != nil { - logger.Error(err, "enqueueOnAccessChange: cannot list awsendpointservices") - return - } - for i := range awsEndpointServiceList.Items { - q.Add(reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&awsEndpointServiceList.Items[i])}) - } + + awsEndpointServiceList := &hyperv1.AWSEndpointServiceList{} + if err := r.List(ctx, awsEndpointServiceList, client.InNamespace(hcp.Namespace)); err != nil { + log.FromContext(ctx).Error(err, "mapHCPToAWSEndpointService: cannot list AWSEndpointService resources", "namespace", hcp.Namespace) + return nil + } + + var requests []reconcile.Request + for i := range awsEndpointServiceList.Items { + requests = append(requests, reconcile.Request{ + NamespacedName: client.ObjectKeyFromObject(&awsEndpointServiceList.Items[i]), + }) } + return requests } } @@ -433,57 +458,7 @@ func (r *AWSEndpointServiceReconciler) Reconcile(ctx context.Context, req ctrl.R // Return early if deleted if !awsEndpointService.DeletionTimestamp.IsZero() { - if !controllerutil.ContainsFinalizer(awsEndpointService, finalizer) { - // If we previously removed our finalizer, don't delete again and return early - return ctrl.Result{}, nil - } - - // Best-effort initialization for deletion reconciles: after a controller restart - // the clientBuilder is uninitialized because initializeWithHCP is only called in - // the non-deletion path. If the HCP still exists, initialize from it so that - // getClients can succeed and deletion can proceed. - // - // Known issue (SharedVPC): when the HCP is already deleted, the SharedVPC role - // ARNs (needed for cross-account EC2/Route53 access) are lost. Initialization - // cannot happen, getClients will fail, and the finalizer will be preserved until - // the hypershift-operator force-removes it after the grace period — orphaning - // AWS resources in the shared VPC account. A proper fix requires persisting the - // SharedVPC role ARNs in the AWSEndpointService status. See - // TestReconcileDeletionSharedVPC for details. - hcpList := &hyperv1.HostedControlPlaneList{} - if err := r.List(ctx, hcpList, &client.ListOptions{Namespace: req.Namespace}); err == nil && len(hcpList.Items) == 1 { - r.awsClientBuilder.initializeWithHCP(log, &hcpList.Items[0]) - } - - ec2Client, route53Client, err := r.awsClientBuilder.getClients(ctx) - if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to get AWS clients for endpoint service cleanup: %w", err) - } - completed, err := r.delete(ctx, awsEndpointService, ec2Client, route53Client) - if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to delete resource: %w", err) - } - if !completed { - return ctrl.Result{RequeueAfter: endpointServiceDeletionRequeueDuration}, nil - } - if controllerutil.ContainsFinalizer(awsEndpointService, finalizer) { - controllerutil.RemoveFinalizer(awsEndpointService, finalizer) - if err := r.Update(ctx, awsEndpointService); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to remove finalizer: %w", err) - } - } - return ctrl.Result{}, nil - } - - // Ensure the awsEndpointService has a finalizer for cleanup - if !controllerutil.ContainsFinalizer(awsEndpointService, finalizer) { - controllerutil.AddFinalizer(awsEndpointService, finalizer) - if err := r.Update(ctx, awsEndpointService); err != nil { - if apierrors.IsConflict(err) { - return ctrl.Result{Requeue: true}, nil - } - return ctrl.Result{}, fmt.Errorf("failed to add finalizer: %w", err) - } + return r.reconcileCRDeletion(ctx, awsEndpointService, req.Namespace, log) } serviceName := awsEndpointService.Status.EndpointServiceName @@ -493,19 +468,35 @@ func (r *AWSEndpointServiceReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, nil } - // Fetch the HostedControlPlane - hcpList := &hyperv1.HostedControlPlaneList{} - if err := r.List(ctx, hcpList, &client.ListOptions{Namespace: req.Namespace}); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to get resource: %w", err) + hcp, err := r.getHostedControlPlane(ctx, req.Namespace) + if err != nil { + return ctrl.Result{}, err } - if len(hcpList.Items) == 0 { - // Return early if HostedControlPlane is deleted + if hcp == nil { return ctrl.Result{}, nil } - if len(hcpList.Items) > 1 { - return ctrl.Result{}, fmt.Errorf("unexpected number of HostedControlPlanes in namespace, expected: 1, actual: %d", len(hcpList.Items)) + + // Handle HCP deletion: clean up AWS resources while HCP credentials are still valid. + // This check is before the CR finalizer addition so that during HCP deletion, + // reconcileHCPDeletion does not re-add the CR finalizer after removing it. + if !hcp.DeletionTimestamp.IsZero() { + return r.reconcileHCPDeletion(ctx, awsEndpointService, hcp, log) + } + + // Ensure the awsEndpointService has a finalizer for cleanup. + // This is placed after the serviceName check so that CRs not yet populated + // by hypershift-operator don't get a finalizer that would block HCP deletion, + // and after the HCP deletion check to avoid re-adding the finalizer during + // HCP deletion cleanup cycles. + if !controllerutil.ContainsFinalizer(awsEndpointService, finalizer) { + controllerutil.AddFinalizer(awsEndpointService, finalizer) + if err := r.Update(ctx, awsEndpointService); err != nil { + if apierrors.IsConflict(err) { + return ctrl.Result{RequeueAfter: time.Second}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to add finalizer: %w", err) + } } - hcp := &hcpList.Items[0] if isPaused, duration := util.IsReconciliationPaused(log, hcp.Spec.PausedUntil); isPaused { log.Info("Reconciliation paused", "pausedUntil", *hcp.Spec.PausedUntil) @@ -515,7 +506,14 @@ func (r *AWSEndpointServiceReconciler) Reconcile(ctx context.Context, req ctrl.R r.awsClientBuilder.initializeWithHCP(log, hcp) ec2Client, route53Client, err := r.awsClientBuilder.getClients(ctx) if err != nil { - return ctrl.Result{}, err + return ctrl.Result{}, fmt.Errorf("failed to get AWS clients for endpoint reconciliation: %w", err) + } + + // Add HCP finalizer to block HCP deletion until AWS PrivateLink cleanup is done. + // This is done after initializing clients, ensuring the reconciler is operational + // and there are actually resources to protect. + if result, err := r.ensureHCPFinalizer(ctx, hcp, log); err != nil || !result.IsZero() { + return result, err } // Reconcile the AWSEndpointService @@ -565,6 +563,185 @@ func isAWSThrottleError(err error) bool { return false } +// reconcileCRDeletion handles the AWSEndpointService CR deletion path. +// It initializes AWS clients (best-effort from HCP if available), cleans up +// AWS resources, and removes the CR finalizer. +func (r *AWSEndpointServiceReconciler) reconcileCRDeletion(ctx context.Context, awsEndpointService *hyperv1.AWSEndpointService, namespace string, log logr.Logger) (ctrl.Result, error) { + if !controllerutil.ContainsFinalizer(awsEndpointService, finalizer) { + return ctrl.Result{}, nil + } + + // Best-effort initialization for deletion reconciles: after a controller restart + // the clientBuilder is uninitialized because initializeWithHCP is only called in + // the non-deletion path. If the HCP still exists, initialize from it so that + // getClients can succeed and deletion can proceed. + // + // The HCP finalizer (hcpAWSPrivateLinkFinalizerName), when present, blocks HCP + // deletion so the HCP remains available during this cleanup. Note: the finalizer + // is only added after a successful normal reconciliation. If the controller has + // not yet reconciled (e.g., controller was down since cluster creation), the HCP + // may be deleted before the finalizer is placed, and this best-effort initialization + // is the only protection. See reconcileHCPDeletion for the HCP-deletion cleanup path. + hcpList := &hyperv1.HostedControlPlaneList{} + if err := r.List(ctx, hcpList, &client.ListOptions{Namespace: namespace}); err == nil && len(hcpList.Items) == 1 { + r.awsClientBuilder.initializeWithHCP(log, &hcpList.Items[0]) + } + + ec2Client, route53Client, err := r.awsClientBuilder.getClients(ctx) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to get AWS clients for endpoint service cleanup: %w", err) + } + completed, err := r.delete(ctx, awsEndpointService, ec2Client, route53Client) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to delete resource: %w", err) + } + if !completed { + return ctrl.Result{RequeueAfter: endpointServiceDeletionRequeueDuration}, nil + } + if controllerutil.ContainsFinalizer(awsEndpointService, finalizer) { + originalAES := awsEndpointService.DeepCopy() + controllerutil.RemoveFinalizer(awsEndpointService, finalizer) + if err := r.Patch(ctx, awsEndpointService, client.MergeFromWithOptions(originalAES, client.MergeFromWithOptimisticLock{})); err != nil { + if apierrors.IsConflict(err) { + return ctrl.Result{RequeueAfter: time.Second}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to remove finalizer: %w", err) + } + } + return ctrl.Result{}, nil +} + +// ensureHCPFinalizer adds the HCP finalizer if not already present. +// Uses optimistic locking via MergeFromWithOptimisticLock to safely update the HCP. +func (r *AWSEndpointServiceReconciler) ensureHCPFinalizer(ctx context.Context, hcp *hyperv1.HostedControlPlane, log logr.Logger) (ctrl.Result, error) { + if controllerutil.ContainsFinalizer(hcp, hcpAWSPrivateLinkFinalizerName) { + return ctrl.Result{}, nil + } + + log.Info("Adding HCP finalizer for AWS PrivateLink cleanup") + originalHCP := hcp.DeepCopy() + controllerutil.AddFinalizer(hcp, hcpAWSPrivateLinkFinalizerName) + if err := r.Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { + if apierrors.IsConflict(err) { + // Use RequeueAfter to avoid a tight retry loop when multiple AWSEndpointService + // reconcilers concurrently try to patch the same HCP. + return ctrl.Result{RequeueAfter: time.Second}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to add HCP finalizer: %w", err) + } + return ctrl.Result{}, nil +} + +func (r *AWSEndpointServiceReconciler) getHostedControlPlane(ctx context.Context, namespace string) (*hyperv1.HostedControlPlane, error) { + hcpList := &hyperv1.HostedControlPlaneList{} + if err := r.List(ctx, hcpList, &client.ListOptions{Namespace: namespace}); err != nil { + return nil, fmt.Errorf("failed to get resource: %w", err) + } + if len(hcpList.Items) == 0 { + return nil, nil + } + if len(hcpList.Items) > 1 { + return nil, fmt.Errorf("unexpected number of HostedControlPlanes in namespace, expected: 1, actual: %d", len(hcpList.Items)) + } + return &hcpList.Items[0], nil +} + +// reconcileHCPDeletion handles HCP deletion by cleaning up AWS PrivateLink resources +// and removing the HCP finalizer. This ensures AWS credentials remain valid during +// the cleanup process, which is critical for SharedVPC clusters where the cross-account +// role ARNs are sourced from the HCP spec. +// +// Convergent multi-CR coordination: +// With MaxConcurrentReconciles: 10 and the HCP watch enqueuing ALL CRs on HCP deletion, +// multiple reconcilers run this function concurrently — one per AWSEndpointService CR. +// Each reconciler cleans up its own CR's AWS resources, then checks whether all CRs in +// the namespace are done. Only the last reconciler to finish (the one that finds no pending +// CRs) removes the HCP finalizer. Earlier reconcilers see pending CRs and return +// RequeueAfter to retry; when they re-enter, they find the HCP finalizer already removed +// and return early at the top of this function. This convergent pattern produces a small +// number of no-op requeues but is correct and self-healing. +func (r *AWSEndpointServiceReconciler) reconcileHCPDeletion(ctx context.Context, awsEndpointService *hyperv1.AWSEndpointService, hcp *hyperv1.HostedControlPlane, log logr.Logger) (ctrl.Result, error) { + if hcp.DeletionTimestamp.IsZero() { + return ctrl.Result{}, nil + } + + if !controllerutil.ContainsFinalizer(hcp, hcpAWSPrivateLinkFinalizerName) { + return ctrl.Result{}, nil + } + + // If the AWSEndpointService itself is being deleted, let the existing CR deletion + // path (which returns early at the top of Reconcile) handle cleanup. This prevents + // the two deletion paths from racing under MaxConcurrentReconciles > 1, where the + // CR deletion path and HCP deletion path could both try to clean up the same CR. + if !awsEndpointService.DeletionTimestamp.IsZero() { + return ctrl.Result{}, nil + } + + log.Info("HCP is being deleted, cleaning up AWS PrivateLink resources before removing HCP finalizer") + + // Clean up this AWSEndpointService's AWS resources. + // Client initialization is inside the finalizer check so that already-cleaned-up + // CRs (no finalizer) proceed straight to the pending-CRs check without needing + // AWS API calls. This prevents transient STS/credential issues from blocking + // HCP finalizer removal when all CRs are already cleaned up. + if controllerutil.ContainsFinalizer(awsEndpointService, finalizer) { + r.awsClientBuilder.initializeWithHCP(log, hcp) + ec2Client, route53Client, err := r.awsClientBuilder.getClients(ctx) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to get AWS clients during HCP deletion cleanup: %w", err) + } + + completed, err := r.delete(ctx, awsEndpointService, ec2Client, route53Client) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to clean up AWS resources during HCP deletion: %w", err) + } + if !completed { + return ctrl.Result{RequeueAfter: endpointServiceDeletionRequeueDuration}, nil + } + + originalAES := awsEndpointService.DeepCopy() + controllerutil.RemoveFinalizer(awsEndpointService, finalizer) + if err := r.Patch(ctx, awsEndpointService, client.MergeFromWithOptions(originalAES, client.MergeFromWithOptimisticLock{})); err != nil { + if apierrors.IsConflict(err) { + return ctrl.Result{RequeueAfter: time.Second}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to remove AWSEndpointService finalizer during HCP deletion: %w", err) + } + log.Info("Removed AWSEndpointService finalizer after AWS resource cleanup") + } + + // Check if all AWSEndpointService CRs in this namespace have been cleaned up. + // Only remove the HCP finalizer once all CRs no longer have our CR finalizer, + // meaning all AWS resources have been cleaned up. + awsEndpointServiceList := &hyperv1.AWSEndpointServiceList{} + if err := r.List(ctx, awsEndpointServiceList, client.InNamespace(hcp.Namespace)); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to list AWSEndpointService resources: %w", err) + } + for i := range awsEndpointServiceList.Items { + if controllerutil.ContainsFinalizer(&awsEndpointServiceList.Items[i], finalizer) { + log.Info("Other AWSEndpointService resources still have finalizers, keeping HCP finalizer", + "pendingResource", awsEndpointServiceList.Items[i].Name) + return ctrl.Result{RequeueAfter: endpointServiceDeletionRequeueDuration}, nil + } + } + + // All AWSEndpointService resources are cleaned up — remove the HCP finalizer + // to unblock HCP deletion. + log.Info("All AWS PrivateLink resources cleaned up, removing HCP finalizer") + originalHCP := hcp.DeepCopy() + controllerutil.RemoveFinalizer(hcp, hcpAWSPrivateLinkFinalizerName) + if err := r.Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { + if apierrors.IsConflict(err) { + // Use RequeueAfter to avoid a tight retry loop when multiple AWSEndpointService + // reconcilers concurrently try to patch the same HCP. + return ctrl.Result{RequeueAfter: time.Second}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to remove HCP finalizer: %w", err) + } + + return ctrl.Result{}, nil +} + func hasAWSConfig(platform *hyperv1.PlatformSpec) bool { return platform.Type == hyperv1.AWSPlatform && platform.AWS != nil && platform.AWS.CloudProviderConfig != nil && platform.AWS.CloudProviderConfig.Subnet != nil && platform.AWS.CloudProviderConfig.Subnet.ID != nil diff --git a/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go b/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go index 8e8f59cfd029..852fd6ee667c 100644 --- a/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go +++ b/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go @@ -21,13 +21,16 @@ import ( "github.com/aws/smithy-go" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" crclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -1736,6 +1739,990 @@ func TestReconcileDeletionSharedVPC(t *testing.T) { } } +func TestReconcileCRDeletion(t *testing.T) { + testCases := []struct { + name string + crFinalizers []string + setupMocks func(ctrl *gomock.Controller) *MockawsClientProvider + patchInterceptor func(ctx context.Context, c crclient.WithWatch, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error + expectError bool + expectRequeue bool + expectCRDeleted bool + expectCRFinalizer bool + }{ + { + name: "When CR does not have the finalizer it should return early", + crFinalizers: nil, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + return NewMockawsClientProvider(mockCtrl) + }, + expectCRFinalizer: false, + }, + { + name: "When cleanup succeeds it should remove the CR finalizer", + crFinalizers: []string{finalizer}, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + + return mockBuilder + }, + expectCRDeleted: true, + expectCRFinalizer: false, + }, + { + name: "When AWS client initialization fails it should return error", + crFinalizers: []string{finalizer}, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(nil, nil, fmt.Errorf("failed to create AWS clients")) + return mockBuilder + }, + expectError: true, + expectCRFinalizer: true, + }, + { + name: "When removing CR finalizer returns conflict error it should requeue", + crFinalizers: []string{finalizer}, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + + return mockBuilder + }, + patchInterceptor: func(ctx context.Context, c crclient.WithWatch, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error { + if _, ok := obj.(*hyperv1.AWSEndpointService); ok { + return apierrors.NewConflict(schema.GroupResource{Resource: "awsendpointservices"}, "private-router", fmt.Errorf("conflict")) + } + return c.Patch(ctx, obj, patch, opts...) + }, + expectRequeue: true, + expectCRFinalizer: true, + }, + { + name: "When removing CR finalizer returns non-conflict error it should return error", + crFinalizers: []string{finalizer}, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + + return mockBuilder + }, + patchInterceptor: func(ctx context.Context, c crclient.WithWatch, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error { + if _, ok := obj.(*hyperv1.AWSEndpointService); ok { + return fmt.Errorf("internal server error") + } + return c.Patch(ctx, obj, patch, opts...) + }, + expectError: true, + expectCRFinalizer: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + mockCtrl := gomock.NewController(t) + mockBuilder := tc.setupMocks(mockCtrl) + + scheme := runtime.NewScheme() + _ = hyperv1.AddToScheme(scheme) + + aes := &hyperv1.AWSEndpointService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: tc.crFinalizers, + }, + Status: hyperv1.AWSEndpointServiceStatus{}, + } + if len(tc.crFinalizers) > 0 { + now := metav1.NewTime(time.Now()) + aes.DeletionTimestamp = &now + } + + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-test", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{}, + }, + }, + } + + clientBuilder := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(aes, hcp) + if tc.patchInterceptor != nil { + clientBuilder = clientBuilder.WithInterceptorFuncs(interceptor.Funcs{ + Patch: tc.patchInterceptor, + }) + } + fakeClient := clientBuilder.Build() + + reconciler := &AWSEndpointServiceReconciler{ + Client: fakeClient, + awsClientBuilder: mockBuilder, + } + + ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test")) + + fetchedAES := &hyperv1.AWSEndpointService{} + err := fakeClient.Get(ctx, types.NamespacedName{Name: aes.Name, Namespace: aes.Namespace}, fetchedAES) + g.Expect(err).ToNot(HaveOccurred()) + + result, err := reconciler.reconcileCRDeletion(ctx, fetchedAES, "clusters-test", ctrl.Log.WithName("test")) + + if tc.expectError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + if tc.expectRequeue { + g.Expect(result.RequeueAfter).To(Equal(time.Second)) + } + + updatedAES := &hyperv1.AWSEndpointService{} + getErr := fakeClient.Get(ctx, types.NamespacedName{Name: aes.Name, Namespace: aes.Namespace}, updatedAES) + if tc.expectCRDeleted { + g.Expect(apierrors.IsNotFound(getErr)).To(BeTrue(), "expected AES to be garbage-collected after finalizer removal") + } else { + g.Expect(getErr).ToNot(HaveOccurred()) + g.Expect(controllerutil.ContainsFinalizer(updatedAES, finalizer)).To(Equal(tc.expectCRFinalizer)) + } + }) + } +} + +func TestEnsureHCPFinalizer(t *testing.T) { + testCases := []struct { + name string + hcpFinalizers []string + patchInterceptor func(ctx context.Context, c crclient.WithWatch, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error + expectFinalizerAdded bool + expectRequeue bool + expectError bool + }{ + { + name: "When HCP does not have the finalizer it should add it", + hcpFinalizers: nil, + expectFinalizerAdded: true, + expectRequeue: false, + }, + { + name: "When HCP already has the finalizer it should be a no-op", + hcpFinalizers: []string{hcpAWSPrivateLinkFinalizerName}, + expectFinalizerAdded: true, + expectRequeue: false, + }, + { + name: "When patching HCP returns conflict error it should requeue", + hcpFinalizers: nil, + patchInterceptor: func(ctx context.Context, c crclient.WithWatch, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error { + return apierrors.NewConflict(schema.GroupResource{Resource: "hostedcontrolplanes"}, "test-hcp", fmt.Errorf("conflict")) + }, + expectFinalizerAdded: false, + expectRequeue: true, + }, + { + name: "When patching HCP returns non-conflict error it should return the error", + hcpFinalizers: nil, + patchInterceptor: func(ctx context.Context, c crclient.WithWatch, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error { + return fmt.Errorf("internal server error") + }, + expectFinalizerAdded: false, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + scheme := runtime.NewScheme() + _ = hyperv1.AddToScheme(scheme) + + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-test", + Finalizers: tc.hcpFinalizers, + }, + } + + clientBuilder := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(hcp) + if tc.patchInterceptor != nil { + clientBuilder = clientBuilder.WithInterceptorFuncs(interceptor.Funcs{ + Patch: tc.patchInterceptor, + }) + } + fakeClient := clientBuilder.Build() + + reconciler := &AWSEndpointServiceReconciler{ + Client: fakeClient, + } + + ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test")) + result, err := reconciler.ensureHCPFinalizer(ctx, hcp, ctrl.Log.WithName("test")) + + if tc.expectError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + if tc.expectRequeue { + g.Expect(result.RequeueAfter).To(Equal(time.Second)) + } else { + g.Expect(result.IsZero()).To(BeTrue()) + } + + updatedHCP := &hyperv1.HostedControlPlane{} + err = fakeClient.Get(ctx, types.NamespacedName{ + Name: "test-hcp", + Namespace: "clusters-test", + }, updatedHCP) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(controllerutil.ContainsFinalizer(updatedHCP, hcpAWSPrivateLinkFinalizerName)).To(Equal(tc.expectFinalizerAdded)) + }) + } +} + +func TestReconcileHCPDeletion(t *testing.T) { + now := metav1.NewTime(time.Now()) + + testCases := []struct { + name string + hcpDeletionTimestamp *metav1.Time + hcpFinalizers []string + hcpHasSharedVPC bool + awsEndpointSvcs []*hyperv1.AWSEndpointService + setupMocks func(ctrl *gomock.Controller) *MockawsClientProvider + expectError bool + expectHCPDeleted bool + expectHCPFinalizer bool + expectCRFinalizer bool + expectRequeue bool + }{ + { + name: "When HCP is not being deleted it should return early", + hcpFinalizers: []string{hcpAWSPrivateLinkFinalizerName}, + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + EndpointID: "vpce-12345", + }, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + return NewMockawsClientProvider(mockCtrl) + }, + expectError: false, + expectHCPDeleted: false, + expectHCPFinalizer: true, + expectCRFinalizer: true, + expectRequeue: false, + }, + { + name: "When HCP is being deleted and has our finalizer it should clean up and remove both finalizers", + hcpDeletionTimestamp: &now, + hcpFinalizers: []string{hcpAWSPrivateLinkFinalizerName}, + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + EndpointID: "vpce-12345", + SecurityGroupID: "sg-12345", + DNSNames: []string{"api.example.com"}, + DNSZoneID: "Z1234567890", + }, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + + // VPC endpoint: delete succeeds, describe returns not found + mockEC2.EXPECT().DeleteVpcEndpoints(gomock.Any(), gomock.Any()).Return(&ec2v2.DeleteVpcEndpointsOutput{}, nil) + mockEC2.EXPECT().DescribeVpcEndpoints(gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{Code: "InvalidVpcEndpointId.NotFound", Message: "not found"}) + // Security group cleanup + mockEC2.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2v2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{{ + GroupId: aws.String("sg-12345"), + IpPermissions: []ec2types.IpPermission{}, + IpPermissionsEgress: []ec2types.IpPermission{}, + }}, + }, nil) + mockEC2.EXPECT().DeleteSecurityGroup(gomock.Any(), gomock.Any()).Return(&ec2v2.DeleteSecurityGroupOutput{}, nil) + // DNS record cleanup + mockRoute53.EXPECT().ListResourceRecordSets(gomock.Any(), gomock.Any()).Return( + &route53sdk.ListResourceRecordSetsOutput{ + ResourceRecordSets: []route53types.ResourceRecordSet{{ + Name: aws.String("api.example.com."), + Type: route53types.RRTypeCname, + TTL: aws.Int64(300), + ResourceRecords: []route53types.ResourceRecord{ + {Value: aws.String("vpce-12345.vpce-svc.us-east-1.vpce.amazonaws.com")}, + }, + }}, + }, nil) + mockRoute53.EXPECT().ChangeResourceRecordSets(gomock.Any(), gomock.Any()).Return( + &route53sdk.ChangeResourceRecordSetsOutput{}, nil) + + return mockBuilder + }, + expectError: false, + expectHCPDeleted: true, + expectHCPFinalizer: false, + expectCRFinalizer: false, + expectRequeue: false, + }, + { + name: "When HCP does not have our finalizer it should return early", + hcpDeletionTimestamp: &now, + hcpFinalizers: []string{"hypershift.openshift.io/finalizer"}, + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + EndpointID: "vpce-12345", + }, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + return NewMockawsClientProvider(mockCtrl) + }, + expectError: false, + expectHCPDeleted: false, + expectHCPFinalizer: false, + expectCRFinalizer: true, + expectRequeue: false, + }, + { + name: "When HCP has SharedVPC and is being deleted it should initialize clients with role ARNs and clean up", + hcpDeletionTimestamp: &now, + hcpFinalizers: []string{hcpAWSPrivateLinkFinalizerName}, + hcpHasSharedVPC: true, + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{}, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + + return mockBuilder + }, + expectError: false, + expectHCPDeleted: true, + expectHCPFinalizer: false, + expectCRFinalizer: false, + expectRequeue: false, + }, + { + name: "When AWS client initialization fails during HCP deletion it should return error", + hcpDeletionTimestamp: &now, + hcpFinalizers: []string{hcpAWSPrivateLinkFinalizerName}, + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + EndpointID: "vpce-12345", + }, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(nil, nil, fmt.Errorf("failed to create AWS clients")) + return mockBuilder + }, + expectError: true, + expectHCPDeleted: false, + expectHCPFinalizer: true, + expectCRFinalizer: true, + expectRequeue: false, + }, + { + name: "When AWS resource cleanup fails during HCP deletion it should return error", + hcpDeletionTimestamp: &now, + hcpFinalizers: []string{hcpAWSPrivateLinkFinalizerName}, + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + EndpointID: "vpce-12345", + }, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + mockEC2.EXPECT().DeleteVpcEndpoints(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("throttling")) + return mockBuilder + }, + expectError: true, + expectHCPDeleted: false, + expectHCPFinalizer: true, + expectCRFinalizer: true, + expectRequeue: false, + }, + { + name: "When AWS resource cleanup is incomplete during HCP deletion it should requeue", + hcpDeletionTimestamp: &now, + hcpFinalizers: []string{hcpAWSPrivateLinkFinalizerName}, + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + SecurityGroupID: "sg-12345", + }, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + mockEC2.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2v2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{{ + GroupId: aws.String("sg-12345"), + IpPermissions: []ec2types.IpPermission{}, + IpPermissionsEgress: []ec2types.IpPermission{}, + }}, + }, nil) + mockEC2.EXPECT().DeleteSecurityGroup(gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{Code: "DependencyViolation", Message: "resource has a dependent object"}) + return mockBuilder + }, + expectError: false, + expectHCPDeleted: false, + expectHCPFinalizer: true, + expectCRFinalizer: true, + expectRequeue: true, + }, + { + name: "When AWSEndpointService is being deleted it should return early and let the CR deletion path handle cleanup", + hcpDeletionTimestamp: &now, + hcpFinalizers: []string{hcpAWSPrivateLinkFinalizerName}, + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + DeletionTimestamp: &now, + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + EndpointID: "vpce-12345", + }, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + return NewMockawsClientProvider(mockCtrl) + }, + expectError: false, + expectHCPDeleted: false, + expectHCPFinalizer: true, + expectCRFinalizer: true, + expectRequeue: false, + }, + { + name: "When multiple AWSEndpointService CRs exist and one still has finalizer it should keep HCP finalizer", + hcpDeletionTimestamp: &now, + hcpFinalizers: []string{hcpAWSPrivateLinkFinalizerName}, + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "kube-apiserver-private", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{}, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{}, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + + return mockBuilder + }, + expectError: false, + expectHCPDeleted: false, + expectHCPFinalizer: true, + expectCRFinalizer: false, + expectRequeue: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + mockCtrl := gomock.NewController(t) + mockBuilder := tc.setupMocks(mockCtrl) + + scheme := runtime.NewScheme() + _ = hyperv1.AddToScheme(scheme) + + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-test", + DeletionTimestamp: tc.hcpDeletionTimestamp, + Finalizers: tc.hcpFinalizers, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{}, + }, + }, + } + if tc.hcpHasSharedVPC { + hcp.Spec.Platform.AWS.SharedVPC = &hyperv1.AWSSharedVPC{ + RolesRef: hyperv1.AWSSharedVPCRolesRef{ + ControlPlaneARN: "arn:aws:iam::123456789012:role/shared-vpc-endpoint-role", + IngressARN: "arn:aws:iam::123456789012:role/shared-vpc-route53-role", + }, + } + } + + objects := []crclient.Object{hcp} + for _, svc := range tc.awsEndpointSvcs { + objects = append(objects, svc) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + Build() + + reconciler := &AWSEndpointServiceReconciler{ + Client: fakeClient, + awsClientBuilder: mockBuilder, + } + + ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test")) + + // Re-fetch objects from the fake client so they have the assigned ResourceVersion + // (required for Patch with optimistic locking). + fetchedHCP := &hyperv1.HostedControlPlane{} + err := fakeClient.Get(ctx, types.NamespacedName{Name: hcp.Name, Namespace: hcp.Namespace}, fetchedHCP) + g.Expect(err).ToNot(HaveOccurred()) + + targetSvcDef := tc.awsEndpointSvcs[0] + fetchedSvc := &hyperv1.AWSEndpointService{} + err = fakeClient.Get(ctx, types.NamespacedName{Name: targetSvcDef.Name, Namespace: targetSvcDef.Namespace}, fetchedSvc) + g.Expect(err).ToNot(HaveOccurred()) + + result, err := reconciler.reconcileHCPDeletion(ctx, fetchedSvc, fetchedHCP, ctrl.Log.WithName("test")) + + if tc.expectError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + + if tc.expectRequeue { + g.Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + } + + // Verify HCP finalizer state. + // When the HCP has DeletionTimestamp set and all finalizers are removed, + // the fake client automatically garbage-collects the object. + updatedHCP := &hyperv1.HostedControlPlane{} + getErr := fakeClient.Get(ctx, types.NamespacedName{ + Name: "test-hcp", + Namespace: "clusters-test", + }, updatedHCP) + if tc.expectHCPDeleted { + g.Expect(apierrors.IsNotFound(getErr)).To(BeTrue(), "expected HCP to be garbage-collected after finalizer removal") + } else { + g.Expect(getErr).ToNot(HaveOccurred()) + g.Expect(controllerutil.ContainsFinalizer(updatedHCP, hcpAWSPrivateLinkFinalizerName)).To(Equal(tc.expectHCPFinalizer)) + } + + // Verify CR finalizer state of the target service + updatedSvc := &hyperv1.AWSEndpointService{} + err = fakeClient.Get(ctx, types.NamespacedName{ + Name: targetSvcDef.Name, + Namespace: targetSvcDef.Namespace, + }, updatedSvc) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(controllerutil.ContainsFinalizer(updatedSvc, finalizer)).To(Equal(tc.expectCRFinalizer)) + }) + } +} + +func TestReconcileHCPDeletionClientErrors(t *testing.T) { + now := metav1.NewTime(time.Now()) + + testCases := []struct { + name string + awsEndpointSvcs []*hyperv1.AWSEndpointService + setupMocks func(ctrl *gomock.Controller) *MockawsClientProvider + clientInterceptors interceptor.Funcs + expectError bool + expectRequeue bool + }{ + { + name: "When removing AWSEndpointService finalizer fails during HCP deletion it should return error", + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{}, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + return mockBuilder + }, + clientInterceptors: interceptor.Funcs{ + Patch: func(ctx context.Context, c crclient.WithWatch, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error { + if _, ok := obj.(*hyperv1.AWSEndpointService); ok { + return fmt.Errorf("patch failed") + } + return c.Patch(ctx, obj, patch, opts...) + }, + }, + expectError: true, + }, + { + name: "When listing AWSEndpointService resources fails during HCP deletion it should return error", + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + }, + Status: hyperv1.AWSEndpointServiceStatus{}, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + return NewMockawsClientProvider(mockCtrl) + }, + clientInterceptors: interceptor.Funcs{ + List: func(ctx context.Context, c crclient.WithWatch, list crclient.ObjectList, opts ...crclient.ListOption) error { + if _, ok := list.(*hyperv1.AWSEndpointServiceList); ok { + return fmt.Errorf("list failed") + } + return c.List(ctx, list, opts...) + }, + }, + expectError: true, + }, + { + name: "When removing AWSEndpointService finalizer returns conflict error it should requeue", + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{}, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + return mockBuilder + }, + clientInterceptors: interceptor.Funcs{ + Patch: func(ctx context.Context, c crclient.WithWatch, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error { + if _, ok := obj.(*hyperv1.AWSEndpointService); ok { + return apierrors.NewConflict(schema.GroupResource{Resource: "awsendpointservices"}, "private-router", fmt.Errorf("conflict")) + } + return c.Patch(ctx, obj, patch, opts...) + }, + }, + expectRequeue: true, + }, + { + name: "When removing HCP finalizer returns conflict error it should requeue", + awsEndpointSvcs: []*hyperv1.AWSEndpointService{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + }, + Status: hyperv1.AWSEndpointServiceStatus{}, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + return NewMockawsClientProvider(mockCtrl) + }, + clientInterceptors: interceptor.Funcs{ + Patch: func(ctx context.Context, c crclient.WithWatch, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error { + if _, ok := obj.(*hyperv1.HostedControlPlane); ok { + return apierrors.NewConflict(schema.GroupResource{Resource: "hostedcontrolplanes"}, "test-hcp", fmt.Errorf("conflict")) + } + return c.Patch(ctx, obj, patch, opts...) + }, + }, + expectRequeue: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + mockCtrl := gomock.NewController(t) + mockBuilder := tc.setupMocks(mockCtrl) + + scheme := runtime.NewScheme() + _ = hyperv1.AddToScheme(scheme) + + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-test", + DeletionTimestamp: &now, + Finalizers: []string{hcpAWSPrivateLinkFinalizerName}, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{}, + }, + }, + } + + objects := []crclient.Object{hcp} + for _, svc := range tc.awsEndpointSvcs { + objects = append(objects, svc) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + WithInterceptorFuncs(tc.clientInterceptors). + Build() + + reconciler := &AWSEndpointServiceReconciler{ + Client: fakeClient, + awsClientBuilder: mockBuilder, + } + + ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test")) + + fetchedHCP := &hyperv1.HostedControlPlane{} + err := fakeClient.Get(ctx, types.NamespacedName{Name: hcp.Name, Namespace: hcp.Namespace}, fetchedHCP) + g.Expect(err).ToNot(HaveOccurred()) + + fetchedSvc := &hyperv1.AWSEndpointService{} + err = fakeClient.Get(ctx, types.NamespacedName{Name: tc.awsEndpointSvcs[0].Name, Namespace: tc.awsEndpointSvcs[0].Namespace}, fetchedSvc) + g.Expect(err).ToNot(HaveOccurred()) + + result, err := reconciler.reconcileHCPDeletion(ctx, fetchedSvc, fetchedHCP, ctrl.Log.WithName("test")) + + if tc.expectError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + if tc.expectRequeue { + g.Expect(result.RequeueAfter).To(Equal(time.Second)) + } + }) + } +} + +func TestMapHCPToAWSEndpointService(t *testing.T) { + now := metav1.NewTime(time.Now()) + + testCases := []struct { + name string + hcp *hyperv1.HostedControlPlane + existingCRs []crclient.Object + expectEnqueued int + }{ + { + name: "When HCP has our finalizer it should enqueue all CRs", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-test", + Finalizers: []string{hcpAWSPrivateLinkFinalizerName}, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + AWS: &hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Private}, + }, + }, + }, + existingCRs: []crclient.Object{ + &hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "kube-apiserver-private", Namespace: "clusters-test"}}, + &hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "private-router", Namespace: "clusters-test"}}, + }, + expectEnqueued: 2, + }, + { + name: "When HCP is being deleted with our finalizer it should enqueue all CRs", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-test", + DeletionTimestamp: &now, + Finalizers: []string{hcpAWSPrivateLinkFinalizerName}, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + AWS: &hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Private}, + }, + }, + }, + existingCRs: []crclient.Object{ + &hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "kube-apiserver-private", Namespace: "clusters-test"}}, + &hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "private-router", Namespace: "clusters-test"}}, + }, + expectEnqueued: 2, + }, + { + name: "When HCP does not have our finalizer it should not enqueue", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-test", + DeletionTimestamp: &now, + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + AWS: &hyperv1.AWSPlatformSpec{EndpointAccess: hyperv1.Private}, + }, + }, + }, + existingCRs: []crclient.Object{ + &hyperv1.AWSEndpointService{ObjectMeta: metav1.ObjectMeta{Name: "kube-apiserver-private", Namespace: "clusters-test"}}, + }, + expectEnqueued: 0, + }, + { + name: "When HCP has our finalizer but no CRs exist it should return empty", + hcp: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-test", + Finalizers: []string{hcpAWSPrivateLinkFinalizerName}, + }, + }, + existingCRs: nil, + expectEnqueued: 0, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + scheme := runtime.NewScheme() + _ = hyperv1.AddToScheme(scheme) + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(tc.existingCRs...). + Build() + + reconciler := &AWSEndpointServiceReconciler{ + Client: fakeClient, + } + + mapFunc := reconciler.mapHCPToAWSEndpointService() + + ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test")) + requests := mapFunc(ctx, tc.hcp) + + g.Expect(len(requests)).To(Equal(tc.expectEnqueued)) + }) + } +} + func TestExtractNLBName(t *testing.T) { testCases := []struct { name string