From 08f463d5e50e589939fec80c3efe1bbdba377230 Mon Sep 17 00:00:00 2001 From: Salvatore Dario Minonne Date: Fri, 27 Mar 2026 14:31:11 +0100 Subject: [PATCH] fix(OCPBUGS-74960): prevent AWS resource leak on AWSEndpointService deletion The AWSEndpointService deletion path silently skipped AWS resource cleanup when getClients failed (e.g. after a controller restart with an uninitialized clientBuilder). This caused security groups, VPC endpoints, and DNS records to be orphaned because the finalizer was still removed. Changes: - Return an error instead of silently skipping cleanup when AWS clients cannot be initialized, preserving the finalizer for retry. - Attempt best-effort client initialization during deletion by listing HostedControlPlanes in the namespace, so restarts can recover when the HCP still exists. - Extract awsClientProvider interface from clientBuilder to enable unit testing the reconciler with mock AWS clients. - Add errDependencyViolation sentinel error so that DependencyViolation from security group operations triggers a controlled requeue (with RequeueAfter) instead of an error-driven exponential backoff. - Replace duplicate log.Error calls in deleteSecurityGroup with proper error wrapping using %w. - Add DependencyViolation constant to support/awsutil/errorcode.go. - Add comprehensive unit tests for deletion, restart recovery, DependencyViolation handling, and SharedVPC leak scenarios. Co-Authored-By: Claude Opus 4.6 --- .../awsprivatelink_controller.go | 81 +- .../awsprivatelink_controller_test.go | 704 ++++++++++++++++++ support/awsutil/errorcode.go | 1 + 3 files changed, 768 insertions(+), 18 deletions(-) diff --git a/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go b/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go index da704ef087f4..ed9f5f373170 100644 --- a/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go +++ b/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go @@ -184,6 +184,11 @@ func (r *PrivateServiceObserver) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, nil } +// errDependencyViolation is returned when AWS reports a DependencyViolation, +// indicating the VPC endpoint is still being deleted. The caller translates +// this into a controlled requeue rather than an error-driven requeue. +var errDependencyViolation = errors.New("security group dependency violation") + const ( finalizer = "hypershift.openshift.io/control-plane-operator-finalizer" endpointServiceDeletionRequeueDuration = 5 * time.Second @@ -196,9 +201,22 @@ const ( type AWSEndpointServiceReconciler struct { client.Client upsert.CreateOrUpdateProvider - awsClientBuilder clientBuilder + awsClientBuilder awsClientProvider +} + +// awsClientProvider abstracts AWS client creation for testability. +// +//go:generate ../../../hack/tools/bin/mockgen -source=awsprivatelink_controller.go -package=awsprivatelink -destination=awsprivatelink_controller_mock.go +type awsClientProvider interface { + getClients(ctx context.Context) (awsapi.EC2API, awsapi.ROUTE53API, error) + initializeWithHCP(log logr.Logger, hcp *hyperv1.HostedControlPlane) + getLocalHostedZoneID() string + setLocalHostedZoneID(zoneID string) } +// Verify clientBuilder implements awsClientProvider. +var _ awsClientProvider = (*clientBuilder)(nil) + type clientBuilder struct { mu sync.Mutex initialized bool @@ -316,6 +334,9 @@ func (b *clientBuilder) setFromHCP(hcp *hyperv1.HostedControlPlane) { } func (r *AWSEndpointServiceReconciler) SetupWithManager(mgr ctrl.Manager) error { + if r.awsClientBuilder == nil { + r.awsClientBuilder = &clientBuilder{} + } _, err := ctrl.NewControllerManagedBy(mgr). For(&hyperv1.AWSEndpointService{}). WithOptions(controller.Options{ @@ -391,17 +412,33 @@ func (r *AWSEndpointServiceReconciler) Reconcile(ctx context.Context, req ctrl.R 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 { - log.Error(err, "failed to get AWS client, skipping aws endpoint service cleanup") - } else { - 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 - } + 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) @@ -1028,6 +1065,10 @@ func (r *AWSEndpointServiceReconciler) delete(ctx context.Context, awsEndpointSe if awsEndpointService.Status.SecurityGroupID != "" { if err := r.deleteSecurityGroup(ctx, ec2Client, awsEndpointService.Status.SecurityGroupID); err != nil { + if errors.Is(err, errDependencyViolation) { + log.Info("security group has dependencies, will retry", "id", awsEndpointService.Status.SecurityGroupID) + return false, nil + } return false, err } log.Info("security group deleted", "id", awsEndpointService.Status.SecurityGroupID) @@ -1063,7 +1104,6 @@ func (r *AWSEndpointServiceReconciler) delete(ctx context.Context, awsEndpointSe } func (r *AWSEndpointServiceReconciler) deleteSecurityGroup(ctx context.Context, ec2Client awsapi.EC2API, sgID string) error { - log := ctrl.LoggerFrom(ctx) describeSGResult, err := ec2Client.DescribeSecurityGroups(ctx, &ec2.DescribeSecurityGroupsInput{GroupIds: []string{sgID}}) if err != nil { if supportawsutil.AWSErrorCode(err) == "InvalidGroup.NotFound" { @@ -1081,9 +1121,10 @@ func (r *AWSEndpointServiceReconciler) deleteSecurityGroup(ctx context.Context, GroupId: sg.GroupId, IpPermissions: sg.IpPermissions, }); err != nil { - log.Error(err, "failed to revoke security group ingress permissions", "SecurityGroupID", aws.ToString(sg.GroupId), "code", supportawsutil.AWSErrorCode(err)) - - return fmt.Errorf("failed to revoke security group ingress rules: %s", supportawsutil.AWSErrorCode(err)) + if supportawsutil.AWSErrorCode(err) == supportawsutil.DependencyViolation { + return fmt.Errorf("%w: %w", errDependencyViolation, err) + } + return fmt.Errorf("failed to revoke security group %s ingress rules: %w", aws.ToString(sg.GroupId), err) } } @@ -1092,16 +1133,20 @@ func (r *AWSEndpointServiceReconciler) deleteSecurityGroup(ctx context.Context, GroupId: sg.GroupId, IpPermissions: sg.IpPermissionsEgress, }); err != nil { - log.Error(err, "failed to revoke security group egress permissions", "SecurityGroupID", aws.ToString(sg.GroupId), "code", supportawsutil.AWSErrorCode(err)) - return fmt.Errorf("failed to revoke security group egress rules: %s", supportawsutil.AWSErrorCode(err)) + if supportawsutil.AWSErrorCode(err) == supportawsutil.DependencyViolation { + return fmt.Errorf("%w: %w", errDependencyViolation, err) + } + return fmt.Errorf("failed to revoke security group %s egress rules: %w", aws.ToString(sg.GroupId), err) } } if _, err = ec2Client.DeleteSecurityGroup(ctx, &ec2.DeleteSecurityGroupInput{ GroupId: sg.GroupId, }); err != nil { - log.Error(err, "failed to delete security group", "SecurityGroupID", aws.ToString(sg.GroupId), "code", supportawsutil.AWSErrorCode(err)) - return fmt.Errorf("failed to delete security group %s: %s", aws.ToString(sg.GroupId), supportawsutil.AWSErrorCode(err)) + if supportawsutil.AWSErrorCode(err) == supportawsutil.DependencyViolation { + return fmt.Errorf("%w: %w", errDependencyViolation, err) + } + return fmt.Errorf("failed to delete security group %s: %w", aws.ToString(sg.GroupId), err) } return nil diff --git a/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go b/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go index 7dde74b5c165..971655e63b20 100644 --- a/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go +++ b/control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go @@ -1,20 +1,37 @@ package awsprivatelink import ( + "context" + "errors" "fmt" "reflect" "testing" + "time" . "github.com/onsi/gomega" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/awsapi" "github.com/aws/aws-sdk-go-v2/aws" + ec2v2 "github.com/aws/aws-sdk-go-v2/service/ec2" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" + route53sdk "github.com/aws/aws-sdk-go-v2/service/route53" + route53types "github.com/aws/aws-sdk-go-v2/service/route53/types" + "github.com/aws/smithy-go" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "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/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/google/go-cmp/cmp" + "go.uber.org/mock/gomock" ) func Test_diffIDs(t *testing.T) { @@ -206,3 +223,690 @@ func TestDiffPermissions(t *testing.T) { }) } } + +func TestReconcileDeletion(t *testing.T) { + now := metav1.NewTime(time.Now()) + + ingressPermission := ec2types.IpPermission{ + FromPort: aws.Int32(6443), + ToPort: aws.Int32(6443), + IpProtocol: aws.String("tcp"), + IpRanges: []ec2types.IpRange{{CidrIp: aws.String("10.0.0.0/16")}}, + } + egressPermission := ec2types.IpPermission{ + FromPort: aws.Int32(0), + ToPort: aws.Int32(65535), + IpProtocol: aws.String("-1"), + IpRanges: []ec2types.IpRange{{CidrIp: aws.String("0.0.0.0/0")}}, + } + + testCases := []struct { + name string + awsEndpointSvc *hyperv1.AWSEndpointService + extraObjects []crclient.Object + setupMocks func(ctrl *gomock.Controller) *MockawsClientProvider + expectError bool + expectFinalizer bool + expectRequeue bool + }{ + { + name: "When all AWS resources are cleaned up successfully it should remove the finalizer", + awsEndpointSvc: &hyperv1.AWSEndpointService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + DeletionTimestamp: &now, + 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) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + // 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 exists and can be cleaned up + mockEC2.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2v2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{{ + GroupId: aws.String("sg-12345"), + IpPermissions: []ec2types.IpPermission{ingressPermission}, + IpPermissionsEgress: []ec2types.IpPermission{egressPermission}, + }}, + }, nil) + mockEC2.EXPECT().RevokeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupIngressOutput{}, nil) + mockEC2.EXPECT().RevokeSecurityGroupEgress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupEgressOutput{}, nil) + mockEC2.EXPECT().DeleteSecurityGroup(gomock.Any(), gomock.Any()).Return(&ec2v2.DeleteSecurityGroupOutput{}, nil) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + // DNS record exists and can be deleted + 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, + expectFinalizer: false, + }, + { + name: "When status has no AWS resources it should remove the finalizer", + awsEndpointSvc: &hyperv1.AWSEndpointService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + DeletionTimestamp: &now, + 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().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + return mockBuilder + }, + expectError: false, + expectFinalizer: false, + }, + { + name: "When HCP exists after restart it should initialize clients and complete deletion", + awsEndpointSvc: &hyperv1.AWSEndpointService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + DeletionTimestamp: &now, + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + EndpointID: "vpce-12345", + SecurityGroupID: "sg-12345", + DNSNames: []string{"api.example.com"}, + DNSZoneID: "Z1234567890", + }, + }, + extraObjects: []crclient.Object{ + &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-test", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + AWS: &hyperv1.AWSPlatformSpec{}, + }, + }, + }, + }, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + 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"}) + mockEC2.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2v2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{{ + GroupId: aws.String("sg-12345"), + IpPermissions: []ec2types.IpPermission{ingressPermission}, + IpPermissionsEgress: []ec2types.IpPermission{egressPermission}, + }}, + }, nil) + mockEC2.EXPECT().RevokeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupIngressOutput{}, nil) + mockEC2.EXPECT().RevokeSecurityGroupEgress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupEgressOutput{}, nil) + mockEC2.EXPECT().DeleteSecurityGroup(gomock.Any(), gomock.Any()).Return(&ec2v2.DeleteSecurityGroupOutput{}, nil) + // Best-effort initialization: HCP exists, so initializeWithHCP is called. + mockBuilder.EXPECT().initializeWithHCP(gomock.Any(), gomock.Any()) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + 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, + expectFinalizer: false, + }, + { + name: "When VPC endpoint deletion fails it should return error and preserve the finalizer", + awsEndpointSvc: &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 { + mockBuilder := NewMockawsClientProvider(mockCtrl) + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockRoute53 := awsapi.NewMockROUTE53API(mockCtrl) + mockEC2.EXPECT().DeleteVpcEndpoints(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("throttling")) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + return mockBuilder + }, + expectError: true, + expectFinalizer: true, + }, + { + name: "When security group deletion returns DependencyViolation it should requeue without error", + awsEndpointSvc: &hyperv1.AWSEndpointService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + DeletionTimestamp: &now, + 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) + mockEC2.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2v2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{{ + GroupId: aws.String("sg-12345"), + IpPermissions: []ec2types.IpPermission{ingressPermission}, + IpPermissionsEgress: []ec2types.IpPermission{egressPermission}, + }}, + }, nil) + mockEC2.EXPECT().RevokeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupIngressOutput{}, nil) + mockEC2.EXPECT().RevokeSecurityGroupEgress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupEgressOutput{}, nil) + mockEC2.EXPECT().DeleteSecurityGroup(gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{Code: "DependencyViolation", Message: "resource has a dependent object"}) + mockBuilder.EXPECT().getClients(gomock.Any()).Return(mockEC2, mockRoute53, nil) + return mockBuilder + }, + expectError: false, + expectRequeue: true, + expectFinalizer: 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) + + objects := append([]crclient.Object{tc.awsEndpointSvc}, tc.extraObjects...) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + Build() + + reconciler := &AWSEndpointServiceReconciler{ + Client: fakeClient, + awsClientBuilder: mockBuilder, + } + + ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test")) + req := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: tc.awsEndpointSvc.Name, + Namespace: tc.awsEndpointSvc.Namespace, + }, + } + + result, err := reconciler.Reconcile(ctx, req) + + 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 finalizer state on the persisted object. + // When the finalizer is removed from an object with DeletionTimestamp, + // the fake client deletes the object (simulating garbage collection). + updatedService := &hyperv1.AWSEndpointService{} + getErr := fakeClient.Get(ctx, types.NamespacedName{ + Name: tc.awsEndpointSvc.Name, + Namespace: tc.awsEndpointSvc.Namespace, + }, updatedService) + if tc.expectFinalizer { + g.Expect(getErr).ToNot(HaveOccurred(), "object should still exist when finalizer is preserved") + g.Expect(controllerutil.ContainsFinalizer(updatedService, finalizer)).To(BeTrue()) + } else { + // Object was deleted after finalizer removal — this confirms the + // finalizer was successfully removed. + g.Expect(getErr).To(HaveOccurred(), "object should be deleted after finalizer removal") + } + }) + } +} + +// TestReconcileDeletion_AfterControllerRestart verifies the fix for OCPBUGS-74960. +// +// When the controller restarts, a new clientBuilder is created in SetupWithManager +// with initialized=false. The deletion path now attempts best-effort initialization +// by listing HostedControlPlanes in the namespace. When the HCP is not found (already +// deleted), getClients returns "clients not initialized". The fix ensures the +// reconciler returns an error and preserves the finalizer so that AWS resources are +// not orphaned. +func TestReconcileDeletion_AfterControllerRestart(t *testing.T) { + g := NewGomegaWithT(t) + now := metav1.NewTime(time.Now()) + + scheme := runtime.NewScheme() + _ = hyperv1.AddToScheme(scheme) + + awsEndpointSvc := &hyperv1.AWSEndpointService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-test", + DeletionTimestamp: &now, + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + SecurityGroupID: "sg-12345", + EndpointID: "vpce-12345", + DNSNames: []string{"api.example.com"}, + DNSZoneID: "Z1234567890", + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(awsEndpointSvc). + Build() + + // Simulate a controller restart: SetupWithManager creates a fresh + // clientBuilder{} (initialized=false). The deletion path attempts best-effort + // initialization by listing HCPs, but none exist here, so getClients still + // returns "clients not initialized". + restartedReconciler := &AWSEndpointServiceReconciler{ + Client: fakeClient, + awsClientBuilder: &clientBuilder{}, // fresh, uninitialized — as created by SetupWithManager + } + + ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test")) + req := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: awsEndpointSvc.Name, + Namespace: awsEndpointSvc.Namespace, + }, + } + + _, err := restartedReconciler.Reconcile(ctx, req) + // The reconciler must return an error so controller-runtime retries. + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to get AWS clients")) + + // The finalizer must be preserved so the AWS resources (sg-12345, vpce-12345, + // api.example.com in zone Z1234567890) are not orphaned. + updatedService := &hyperv1.AWSEndpointService{} + getErr := fakeClient.Get(ctx, types.NamespacedName{ + Name: awsEndpointSvc.Name, + Namespace: awsEndpointSvc.Namespace, + }, updatedService) + g.Expect(getErr).ToNot(HaveOccurred(), "object should still exist when finalizer is preserved") + g.Expect(controllerutil.ContainsFinalizer(updatedService, finalizer)).To(BeTrue()) +} + +func TestDeleteSecurityGroup(t *testing.T) { + sgID := "sg-12345" + ingressPermission := ec2types.IpPermission{ + FromPort: aws.Int32(6443), + ToPort: aws.Int32(6443), + IpProtocol: aws.String("tcp"), + IpRanges: []ec2types.IpRange{{CidrIp: aws.String("10.0.0.0/16")}}, + } + egressPermission := ec2types.IpPermission{ + FromPort: aws.Int32(0), + ToPort: aws.Int32(65535), + IpProtocol: aws.String("-1"), + IpRanges: []ec2types.IpRange{{CidrIp: aws.String("0.0.0.0/0")}}, + } + + sgWithPermissions := &ec2v2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{{ + GroupId: aws.String(sgID), + IpPermissions: []ec2types.IpPermission{ingressPermission}, + IpPermissionsEgress: []ec2types.IpPermission{egressPermission}, + }}, + } + + testCases := []struct { + name string + setupEC2Mock func(*gomock.Controller) *awsapi.MockEC2API + expectedError bool + expectedErrorContains string + expectedSentinel error + }{ + { + name: "When security group is deleted successfully it should complete without error", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(sgWithPermissions, nil) + m.EXPECT().RevokeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupIngressOutput{}, nil) + m.EXPECT().RevokeSecurityGroupEgress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupEgressOutput{}, nil) + m.EXPECT().DeleteSecurityGroup(gomock.Any(), gomock.Any()).Return(&ec2v2.DeleteSecurityGroupOutput{}, nil) + return m + }, + expectedError: false, + }, + { + name: "When security group is not found it should return nil", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{Code: "InvalidGroup.NotFound", Message: "The security group does not exist"}) + return m + }, + expectedError: false, + }, + { + name: "When describe returns empty list it should return nil", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2v2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{}, + }, nil) + return m + }, + expectedError: false, + }, + { + name: "When revoking ingress returns DependencyViolation it should return error for retry", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(sgWithPermissions, nil) + m.EXPECT().RevokeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{Code: "DependencyViolation", Message: "resource has a dependent object"}) + return m + }, + expectedError: true, + expectedSentinel: errDependencyViolation, + }, + { + name: "When revoking egress returns DependencyViolation it should return error for retry", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(sgWithPermissions, nil) + m.EXPECT().RevokeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupIngressOutput{}, nil) + m.EXPECT().RevokeSecurityGroupEgress(gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{Code: "DependencyViolation", Message: "resource has a dependent object"}) + return m + }, + expectedError: true, + expectedSentinel: errDependencyViolation, + }, + { + name: "When deleting security group returns DependencyViolation it should return error for retry", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(sgWithPermissions, nil) + m.EXPECT().RevokeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupIngressOutput{}, nil) + m.EXPECT().RevokeSecurityGroupEgress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupEgressOutput{}, nil) + m.EXPECT().DeleteSecurityGroup(gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{Code: "DependencyViolation", Message: "resource has a dependent object"}) + return m + }, + expectedError: true, + expectedSentinel: errDependencyViolation, + }, + { + name: "When revoking ingress returns other error it should return that error", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(sgWithPermissions, nil) + m.EXPECT().RevokeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{Code: "InternalError", Message: "internal error"}) + return m + }, + expectedError: true, + expectedErrorContains: "failed to revoke security group " + sgID + " ingress rules", + }, + { + name: "When security group has no ingress rules it should skip revoke ingress", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2v2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{{ + GroupId: aws.String(sgID), + IpPermissions: []ec2types.IpPermission{}, + IpPermissionsEgress: []ec2types.IpPermission{egressPermission}, + }}, + }, nil) + m.EXPECT().RevokeSecurityGroupEgress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupEgressOutput{}, nil) + m.EXPECT().DeleteSecurityGroup(gomock.Any(), gomock.Any()).Return(&ec2v2.DeleteSecurityGroupOutput{}, nil) + return m + }, + expectedError: false, + }, + { + name: "When security group has no egress rules it should skip revoke egress", + setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API { + m := awsapi.NewMockEC2API(mockCtrl) + m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2v2.DescribeSecurityGroupsOutput{ + SecurityGroups: []ec2types.SecurityGroup{{ + GroupId: aws.String(sgID), + IpPermissions: []ec2types.IpPermission{ingressPermission}, + IpPermissionsEgress: []ec2types.IpPermission{}, + }}, + }, nil) + m.EXPECT().RevokeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(&ec2v2.RevokeSecurityGroupIngressOutput{}, nil) + m.EXPECT().DeleteSecurityGroup(gomock.Any(), gomock.Any()).Return(&ec2v2.DeleteSecurityGroupOutput{}, nil) + return m + }, + expectedError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + mockCtrl := gomock.NewController(t) + mockEC2 := tc.setupEC2Mock(mockCtrl) + + reconciler := &AWSEndpointServiceReconciler{} + ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test")) + + err := reconciler.deleteSecurityGroup(ctx, mockEC2, sgID) + + if tc.expectedError { + g.Expect(err).To(HaveOccurred()) + if tc.expectedErrorContains != "" { + g.Expect(err.Error()).To(ContainSubstring(tc.expectedErrorContains)) + } + if tc.expectedSentinel != nil { + g.Expect(errors.Is(err, tc.expectedSentinel)).To(BeTrue(), "expected error to wrap %v", tc.expectedSentinel) + } + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +// TestReconcileDeletionSharedVPC documents the remaining SharedVPC leak scenario. +// +// In SharedVPC clusters, the clientBuilder needs role ARNs from the HostedControlPlane +// (hcp.Spec.Platform.AWS.SharedVPC.RolesRef) to assume cross-account roles for EC2 +// and Route53 operations. These ARNs are only stored in-memory in the clientBuilder +// after initializeWithHCP is called. +// +// The deletion path now attempts best-effort initialization by listing HCPs in the +// namespace. However, when the operator restarts during deletion and the HCP has +// already been deleted: +// - The best-effort List finds no HCP, so initializeWithHCP is not called +// - getClients fails with "clients not initialized" +// - The fix preserves the finalizer, but retries will never succeed +// - After 10 minutes, the hypershift-operator force-removes the CPO finalizer, +// orphaning the security group, VPC endpoint, and DNS records +// +// A proper fix requires persisting the SharedVPC role ARNs in the AWSEndpointService +// status so the deletion path can authenticate independently of the HCP. +func TestReconcileDeletionSharedVPC(t *testing.T) { + now := metav1.NewTime(time.Now()) + + testCases := []struct { + name string + hasHCP bool + setupMocks func(ctrl *gomock.Controller) *MockawsClientProvider + expectError bool + expectErrorContains string + expectFinalizer bool + }{ + { + // This is the core SharedVPC leak scenario: operator restarted, HCP already + // deleted, role ARNs lost. The controller errors on every retry because it + // cannot initialize clients without the HCP. After the 10-minute grace period + // the hypershift-operator will force-remove the finalizer, leaking resources. + name: "When SharedVPC operator restarts with no HCP it should return error and preserve finalizer", + hasHCP: false, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + // No HCP found → no initializeWithHCP call. + // getClients returns "clients not initialized" to simulate uninitialized state. + mockBuilder.EXPECT().getClients(gomock.Any()).Return(nil, nil, fmt.Errorf("clients not initialized")) + return mockBuilder + }, + expectError: true, + expectErrorContains: "clients not initialized", + expectFinalizer: true, + }, + { + // This scenario shows what happens if the clientBuilder is re-initialized + // without the SharedVPC role ARNs (e.g. a naive fix that initializes without + // the HCP). getClients proceeds past the "not initialized" check but creates + // clients with default pod credentials instead of assuming the cross-account + // SharedVPC roles. In production the subsequent delete calls would fail with + // AccessDenied because the security group and VPC endpoint live in a + // different AWS account — a mocked error simulates this deterministically. + name: "When SharedVPC client is initialized without role ARNs it should fail to create AWS session", + hasHCP: false, + setupMocks: func(mockCtrl *gomock.Controller) *MockawsClientProvider { + mockBuilder := NewMockawsClientProvider(mockCtrl) + // No HCP → no initializeWithHCP call. + // getClients returns a deterministic session-creation failure, simulating + // what would happen when SharedVPC role ARNs are missing after an HCP deletion. + mockBuilder.EXPECT().getClients(gomock.Any()).Return(nil, nil, fmt.Errorf("failed to create AWS session: no region configured")) + return mockBuilder + }, + expectError: true, + expectErrorContains: "failed to", + expectFinalizer: 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) + + awsEndpointService := &hyperv1.AWSEndpointService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "private-router", + Namespace: "clusters-sharedvpc", + DeletionTimestamp: &now, + Finalizers: []string{finalizer}, + }, + Status: hyperv1.AWSEndpointServiceStatus{ + SecurityGroupID: "sg-shared-12345", + EndpointID: "vpce-shared-12345", + DNSNames: []string{"api.example.com"}, + DNSZoneID: "Z1234567890", + }, + } + + objects := []crclient.Object{awsEndpointService} + if tc.hasHCP { + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "clusters-sharedvpc", + }, + Spec: hyperv1.HostedControlPlaneSpec{ + Platform: hyperv1.PlatformSpec{ + AWS: &hyperv1.AWSPlatformSpec{ + 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 = append(objects, hcp) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + Build() + + reconciler := &AWSEndpointServiceReconciler{ + Client: fakeClient, + awsClientBuilder: mockBuilder, + } + + ctx := ctrl.LoggerInto(context.Background(), ctrl.Log.WithName("test")) + req := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "private-router", + Namespace: "clusters-sharedvpc", + }, + } + + _, err := reconciler.Reconcile(ctx, req) + + if tc.expectError { + g.Expect(err).To(HaveOccurred()) + if tc.expectErrorContains != "" { + g.Expect(err.Error()).To(ContainSubstring(tc.expectErrorContains)) + } + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + + // Verify finalizer state + updatedService := &hyperv1.AWSEndpointService{} + err = fakeClient.Get(ctx, types.NamespacedName{ + Name: "private-router", + Namespace: "clusters-sharedvpc", + }, updatedService) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(controllerutil.ContainsFinalizer(updatedService, finalizer)).To(Equal(tc.expectFinalizer)) + }) + } +} diff --git a/support/awsutil/errorcode.go b/support/awsutil/errorcode.go index 6495176e24c4..62974f5dc5f3 100644 --- a/support/awsutil/errorcode.go +++ b/support/awsutil/errorcode.go @@ -8,6 +8,7 @@ import ( const ( AuthFailure = "AuthFailure" + DependencyViolation = "DependencyViolation" UnauthorizedOperation = "UnauthorizedOperation" )