OCPBUGS-93739: skip LB Service deletion in HCCO for AWS and actively clean up NLBs during e2e teardown

control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go

@@ -2851,19 +2851,27 @@ func (r *reconciler) ensureCloudResourcesDestroyed(ctx context.Context, hcp *hyp
 		}
 	}
 
-	log.Info("Ensuring load balancers are removed")
-	removed, err := r.ensureServiceLoadBalancersRemoved(ctx)
-	if err != nil {
-		if isConnectionError(err) {
-			hasConnectionError = true
-			log.Info("Connection error while removing load balancers", "error", err.Error())
-		}
-		errs = append(errs, err)
-	}
-	if !removed {
-		remaining.Insert("loadbalancers")
+	// On AWS, NLBs/ELBs are cleaned up directly via the AWS API in the
+	// infrastructure destroy flow rather than waiting for the cloud controller
+	// to process Service deletions, which is async and non-deterministic.
+	var removed bool
+	if hcp.Spec.Platform.Type == hyperv1.AWSPlatform {
+		log.Info("Skipping load balancer Service deletion for AWS; NLBs are cleaned up directly by the infrastructure destroy flow")
 	} else {
-		log.Info("Load balancers are removed")
+		log.Info("Ensuring load balancers are removed")
+		removed, err = r.ensureServiceLoadBalancersRemoved(ctx)
+		if err != nil {
+			if isConnectionError(err) {
+				hasConnectionError = true
+				log.Info("Connection error while removing load balancers", "error", err.Error())
+			}
+			errs = append(errs, err)
+		}
+		if !removed {
+			remaining.Insert("loadbalancers")
+		} else {
+			log.Info("Load balancers are removed")
+		}
 	}
 
 	log.Info("Ensuring persistent volumes are removed")

control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go

@@ -1042,6 +1042,68 @@ func TestDestroyCloudResources(t *testing.T) {
 	}
 }
 
+func TestDestroyCloudResources_WhenPlatformIsAWS_ItShouldSkipLoadBalancerServiceDeletion(t *testing.T) {
+	t.Parallel()
+	g := NewGomegaWithT(t)
+
+	fakeHCP := &hyperv1.HostedControlPlane{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-hcp",
+			Namespace: "test-namespace",
+		},
+		Spec: hyperv1.HostedControlPlaneSpec{
+			Platform: hyperv1.PlatformSpec{
+				Type: hyperv1.AWSPlatform,
+			},
+		},
+		Status: hyperv1.HostedControlPlaneStatus{
+			Conditions: []metav1.Condition{
+				{
+					Type:   string(hyperv1.CloudResourcesDestroyed),
+					Status: metav1.ConditionFalse,
+				},
+			},
+		},
+	}
+
+	lbService := &corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-loadbalancer",
+			Namespace: "default",
+		},
+		Spec: corev1.ServiceSpec{
+			Type: corev1.ServiceTypeLoadBalancer,
+		},
+	}
+
+	guestClient := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(lbService).Build()
+	kasDeployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "kube-apiserver",
+			Namespace: fakeHCP.Namespace,
+		},
+	}
+	cpClient := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(fakeHCP, kasDeployment).WithStatusSubresource(&hyperv1.HostedControlPlane{}).Build()
+
+	r := &reconciler{
+		client:                 guestClient,
+		uncachedClient:         fake.NewClientBuilder().WithScheme(api.Scheme).Build(),
+		cpClient:               cpClient,
+		CreateOrUpdateProvider: &simpleCreateOrUpdater{},
+		cleanupTracker:         supportutil.NewCleanupTracker(),
+	}
+
+	remaining, _, err := r.ensureCloudResourcesDestroyed(t.Context(), fakeHCP)
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(remaining.Has("loadbalancers")).To(BeFalse(), "AWS should not track loadbalancers in remaining set")
+
+	// Verify the LoadBalancer Service was NOT deleted
+	svc := &corev1.Service{}
+	err = guestClient.Get(t.Context(), client.ObjectKeyFromObject(lbService), svc)
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(svc.Spec.Type).To(Equal(corev1.ServiceTypeLoadBalancer))
+}
+
 func TestDestroyCloudResourcesWithKASUnavailable(t *testing.T) {
 	t.Parallel()
 	fakeHCP := &hyperv1.HostedControlPlane{

test/e2e/util/fixture.go

@@ -2,6 +2,7 @@ package util
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -23,6 +24,8 @@ import (
 
 	awssdk "github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/aws/arn"
+	elbv2 "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2"
+	elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types"
 	"github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi"
 	resourcegroupstaggingapitypes "github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/types"
 
@@ -296,7 +299,8 @@ func destroyCluster(ctx context.Context, t *testing.T, hc *hyperv1.HostedCluster
 	}
 }
 
-// validateAWSGuestResourcesDeletedFunc waits for 15min or until the guest cluster resources are gone.
+// validateAWSGuestResourcesDeletedFunc actively deletes tagged NLBs/ELBs and
+// then polls until all remaining guest cluster resources are gone.
 func validateAWSGuestResourcesDeletedFunc(ctx context.Context, t *testing.T, infraID, awsCreds, awsRegion string) func() {
 	if IsLessThan(Version415) {
 		return func() {
@@ -310,11 +314,15 @@ func validateAWSGuestResourcesDeletedFunc(ctx context.Context, t *testing.T, inf
 		taggingClient := resourcegroupstaggingapi.NewFromConfig(*awsSession, func(o *resourcegroupstaggingapi.Options) {
 			o.Retryer = awsConfig()
 		})
+		elbv2Client := elbv2.NewFromConfig(*awsSession, func(o *elbv2.Options) {
+			o.Retryer = awsConfig()
+		})
 		var lastOutput *resourcegroupstaggingapi.GetResourcesOutput
 
-		// Find load balancers, persistent volumes, or s3 buckets belonging to the guest cluster
+		// Find load balancers, persistent volumes, or s3 buckets belonging to the guest cluster.
+		// Any tagged NLBs/ELBs are actively deleted rather than waiting for the
+		// cloud controller to clean them up asynchronously.
 		err := wait.PollUntilContextTimeout(ctx, 20*time.Second, 15*time.Minute, false, func(ctx context.Context) (bool, error) {
-			// Filter get cluster resources.
 			output, err := taggingClient.GetResources(ctx, &resourcegroupstaggingapi.GetResourcesInput{
 				ResourceTypeFilters: []string{
 					"elasticloadbalancing:loadbalancer",
@@ -336,6 +344,11 @@ func validateAWSGuestResourcesDeletedFunc(ctx context.Context, t *testing.T, inf
 				return false, nil
 			}
 			lastOutput = output
+
+			if err := deleteTaggedLoadBalancers(ctx, t, elbv2Client, lastOutput.ResourceTagMappingList); err != nil {
+				return false, err
+			}
+
 			if hasGuestResources(t, lastOutput.ResourceTagMappingList) {
 				return false, nil
 			}
@@ -398,6 +411,40 @@ func clusterTag(infraID string) string {
 	return fmt.Sprintf("kubernetes.io/cluster/%s", infraID)
 }
 
+// loadBalancerDeleter abstracts the ELBv2 DeleteLoadBalancer operation for testing.
+type loadBalancerDeleter interface {
+	DeleteLoadBalancer(ctx context.Context, params *elbv2.DeleteLoadBalancerInput, optFns ...func(*elbv2.Options)) (*elbv2.DeleteLoadBalancerOutput, error)
+}
+
+// deleteTaggedLoadBalancers actively deletes any ELBs/NLBs found in the
+// resource tag mapping list rather than waiting for the cloud controller.
+// Terminal AWS errors (OperationNotPermitted, ResourceInUse) are returned
+// immediately; transient errors are logged for retry by the caller's poll loop.
+func deleteTaggedLoadBalancers(ctx context.Context, t *testing.T, client loadBalancerDeleter, mappings []resourcegroupstaggingapitypes.ResourceTagMapping) error {
+	for _, mapping := range mappings {
+		resourceARN, err := arn.Parse(awssdk.ToString(mapping.ResourceARN))
+		if err != nil {
+			continue
+		}
+		if resourceARN.Service != "elasticloadbalancing" {
+			continue
+		}
+		lbARN := awssdk.ToString(mapping.ResourceARN)
+		t.Logf("Actively deleting load balancer: %s", lbARN)
+		if _, err := client.DeleteLoadBalancer(ctx, &elbv2.DeleteLoadBalancerInput{
+			LoadBalancerArn: mapping.ResourceARN,
+		}); err != nil {
+			var opNotPermitted *elbv2types.OperationNotPermittedException
+			var resourceInUse *elbv2types.ResourceInUseException
+			if errors.As(err, &opNotPermitted) || errors.As(err, &resourceInUse) {
+				return fmt.Errorf("terminal error deleting load balancer %s: %w", lbARN, err)
+			}
+			t.Logf("Failed to delete load balancer %s (will retry): %v", lbARN, err)
+		}
+	}
+	return nil
+}
+
 // newClusterDumper returns a function that dumps important diagnostic data for
 // a cluster based on the cluster's platform. The output directory will be named
 // according to the test name. So, the returned dump function should be called

test/e2e/util/fixture_test.go

@@ -0,0 +1,133 @@
+package util
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	. "github.com/onsi/gomega"
+
+	awssdk "github.com/aws/aws-sdk-go-v2/aws"
+	elbv2 "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2"
+	elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types"
+	resourcegroupstaggingapitypes "github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/types"
+)
+
+type fakeLoadBalancerDeleter struct {
+	attempted []string
+	deleted   []string
+	errs      []error
+}
+
+func (f *fakeLoadBalancerDeleter) DeleteLoadBalancer(_ context.Context, input *elbv2.DeleteLoadBalancerInput, _ ...func(*elbv2.Options)) (*elbv2.DeleteLoadBalancerOutput, error) {
+	lbARN := awssdk.ToString(input.LoadBalancerArn)
+	f.attempted = append(f.attempted, lbARN)
+	if len(f.errs) > 0 {
+		err := f.errs[0]
+		f.errs = f.errs[1:]
+		if err != nil {
+			return nil, err
+		}
+	}
+	f.deleted = append(f.deleted, lbARN)
+	return &elbv2.DeleteLoadBalancerOutput{}, nil
+}
+
+func TestDeleteTaggedLoadBalancers(t *testing.T) {
+	t.Parallel()
+	validNLBArn := "arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/net/my-nlb/abc123"
+	validALBArn := "arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/app/my-alb/def456"
+	ec2Arn := "arn:aws:ec2:us-east-1:123456789012:volume/vol-abc123"
+
+	tests := []struct {
+		name           string
+		mappings       []resourcegroupstaggingapitypes.ResourceTagMapping
+		errs           []error
+		wantErr        bool
+		wantErrContain string
+		wantAttempted  []string
+		wantDeleted    []string
+	}{
+		{
+			name: "When mappings contain NLB and ALB ARNs it should delete both",
+			mappings: []resourcegroupstaggingapitypes.ResourceTagMapping{
+				{ResourceARN: awssdk.String(validNLBArn)},
+				{ResourceARN: awssdk.String(validALBArn)},
+			},
+			wantAttempted: []string{validNLBArn, validALBArn},
+			wantDeleted:   []string{validNLBArn, validALBArn},
+		},
+		{
+			name: "When mappings contain non-LB ARNs it should skip them",
+			mappings: []resourcegroupstaggingapitypes.ResourceTagMapping{
+				{ResourceARN: awssdk.String(ec2Arn)},
+				{ResourceARN: awssdk.String(validNLBArn)},
+			},
+			wantAttempted: []string{validNLBArn},
+			wantDeleted:   []string{validNLBArn},
+		},
+		{
+			name: "When mappings contain malformed ARNs it should skip them",
+			mappings: []resourcegroupstaggingapitypes.ResourceTagMapping{
+				{ResourceARN: awssdk.String("not-an-arn")},
+				{ResourceARN: awssdk.String(validNLBArn)},
+			},
+			wantAttempted: []string{validNLBArn},
+			wantDeleted:   []string{validNLBArn},
+		},
+		{
+			name:     "When mappings are empty it should do nothing",
+			mappings: []resourcegroupstaggingapitypes.ResourceTagMapping{},
+		},
+		{
+			name: "When transient error on first LB it should continue to second LB",
+			mappings: []resourcegroupstaggingapitypes.ResourceTagMapping{
+				{ResourceARN: awssdk.String(validNLBArn)},
+				{ResourceARN: awssdk.String(validALBArn)},
+			},
+			errs:          []error{fmt.Errorf("RequestLimitExceeded")},
+			wantAttempted: []string{validNLBArn, validALBArn},
+			wantDeleted:   []string{validALBArn},
+		},
+		{
+			name: "When OperationNotPermitted on first LB it should stop immediately",
+			mappings: []resourcegroupstaggingapitypes.ResourceTagMapping{
+				{ResourceARN: awssdk.String(validNLBArn)},
+				{ResourceARN: awssdk.String(validALBArn)},
+			},
+			errs:           []error{&elbv2types.OperationNotPermittedException{Message: awssdk.String("deletion protection enabled")}},
+			wantErr:        true,
+			wantErrContain: "terminal error",
+			wantAttempted:  []string{validNLBArn},
+		},
+		{
+			name: "When ResourceInUse on first LB it should stop immediately",
+			mappings: []resourcegroupstaggingapitypes.ResourceTagMapping{
+				{ResourceARN: awssdk.String(validNLBArn)},
+				{ResourceARN: awssdk.String(validALBArn)},
+			},
+			errs:           []error{&elbv2types.ResourceInUseException{Message: awssdk.String("associated with VPC endpoint")}},
+			wantErr:        true,
+			wantErrContain: "terminal error",
+			wantAttempted:  []string{validNLBArn},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			fake := &fakeLoadBalancerDeleter{errs: tc.errs}
+			err := deleteTaggedLoadBalancers(context.Background(), t, fake, tc.mappings)
+
+			if tc.wantErr {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(ContainSubstring(tc.wantErrContain))
+				g.Expect(fake.attempted).To(Equal(tc.wantAttempted))
+				return
+			}
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(fake.attempted).To(Equal(tc.wantAttempted))
+			g.Expect(fake.deleted).To(Equal(tc.wantDeleted))
+		})
+	}
+}