e2e/fix: update aws node/retry resilience

Author: mtulio

tests/e2e/helper_aws.go

@@ -18,6 +18,8 @@ package e2e
 import (
 	"context"
 	"fmt"
+	"os"
+	"strconv"
 	"time"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -34,28 +36,89 @@ import (
 )
 
 const (
+	// DefaultRetryTimeoutMinutes is the default timeout for load balancer DNS resolution with retries.
+	// This can be overridden via the E2E_LB_TIMEOUT_MINUTES environment variable.
 	DefaultRetryTimeoutMinutes = 20
+
+	// DefaultRetryMaxAttempts is the default maximum number of retry attempts for AWS API calls.
+	// This can be overridden via E2ETestHelperAWSOptions.
+	DefaultRetryMaxAttempts = 10
+
+	// DefaultRetryMaxBackoff is the default maximum backoff duration between retries.
+	// This can be overridden via E2ETestHelperAWSOptions.
+	DefaultRetryMaxBackoff = 30 * time.Second
+
+	// DefaultTargetPollInterval is the default polling interval for target health checks.
+	// Target registration is eventual consistency, so we use fixed-interval polling.
+	DefaultTargetPollInterval = 5 * time.Second
+
+	// DefaultTargetWaitTimeout is the default timeout for waiting for targets to register.
+	// AWS target health checks typically complete within 30-90 seconds.
+	DefaultTargetWaitTimeout = 3 * time.Minute
+
+	// EnvLBTimeoutMinutes is the environment variable name for configuring LB timeout.
+	EnvLBTimeoutMinutes = "E2E_LB_TIMEOUT_MINUTES"
 )
 
+// E2ETestHelperAWSOptions configures the behavior of E2ETestHelperAWS.
+type E2ETestHelperAWSOptions struct {
+	// MaxAttempts configures the maximum number of retry attempts for AWS API calls.
+	// Default: DefaultRetryMaxAttempts (10)
+	MaxAttempts int
+
+	// MaxBackoff configures the maximum backoff duration between retries.
+	// Default: DefaultRetryMaxBackoff (30 seconds)
+	MaxBackoff time.Duration
+}
+
+// ApplyDefaults fills in default values for unset options.
+func (o *E2ETestHelperAWSOptions) ApplyDefaults() {
+	if o.MaxAttempts == 0 {
+		o.MaxAttempts = DefaultRetryMaxAttempts
+	}
+	if o.MaxBackoff == 0 {
+		o.MaxBackoff = DefaultRetryMaxBackoff
+	}
+}
+
 // E2ETestHelperAWS provides AWS API operations for e2e tests.
+// AWS SDK v2 clients are safe for concurrent use and do not require explicit cleanup.
+// Context cancellation will terminate ongoing API calls.
 type E2ETestHelperAWS struct {
 	retryer     *retry.Standard
 	ec2Client   *ec2.Client
 	elbClient   *elb.Client
 	elbv2Client *elbv2.Client
 }
 
-// NewAWSHelper creates a new AWS helper with configured clients
+// NewAWSHelper creates a new AWS helper with configured clients using default options.
 func NewAWSHelper(ctx context.Context) (*E2ETestHelperAWS, error) {
+	return NewAWSHelperWithOptions(ctx, E2ETestHelperAWSOptions{})
+}
+
+// NewAWSHelperWithOptions creates a new AWS helper with custom retry configuration.
+// Configure custom retryer to handle transient AWS API errors and credential failures.
+// AWS API limits for ELB are generous (400 TPS for DescribeLoadBalancers),
+// so aggressive retries are safe and necessary for CI stability.
+//
+// For rate-limited environments or shared test accounts, adjust MaxAttempts and MaxBackoff:
+//
+//	helper, err := NewAWSHelperWithOptions(ctx, E2ETestHelperAWSOptions{
+//	    MaxAttempts: 5,
+//	    MaxBackoff: 10 * time.Second,
+//	})
+func NewAWSHelperWithOptions(ctx context.Context, opts E2ETestHelperAWSOptions) (*E2ETestHelperAWS, error) {
+	opts.ApplyDefaults()
+
 	cfg, err := config.LoadDefaultConfig(ctx)
 	framework.ExpectNoError(err, "unable to load AWS config")
+	if err != nil {
+		return nil, fmt.Errorf("unable to load AWS config: %w", err)
+	}
 
-	// Configure custom retryer to handle transient AWS API errors and credential failures.
-	// AWS API limits for ELB are generous (400 TPS for DescribeLoadBalancers),
-	// so aggressive retries are safe and necessary for CI stability.
 	customRetryer := retry.NewStandard(func(o *retry.StandardOptions) {
-		o.MaxAttempts = 10              // Handle IMDS timeouts and transient errors
-		o.MaxBackoff = 30 * time.Second // Cap backoff to avoid excessive wait
+		o.MaxAttempts = opts.MaxAttempts
+		o.MaxBackoff = opts.MaxBackoff
 	})
 
 	// Create AWS clients with custom retryer
@@ -80,8 +143,10 @@ func (h *E2ETestHelperAWS) GetELBV2Client() *elbv2.Client {
 }
 
 // GetLBTargets returns the targets for a given LB DNS name, listener port, and target port.
+// This performs a single check without retry. For waiting until targets are registered,
+// use WaitForLBTargets instead.
 func (h *E2ETestHelperAWS) GetLBTargets(ctx context.Context, lbDNSName string, listenerPort, targetPort int32) ([]string, error) {
-	foundLB, err := h.GetLoadBalancerFromDNSNameWithRetry(ctx, lbDNSName)
+	foundLB, err := h.GetLoadBalancerFromDNSNameDefaultTimeout(ctx, lbDNSName)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get load balancer from DNS name: %v", err)
 	}
@@ -94,6 +159,7 @@ func (h *E2ETestHelperAWS) GetLBTargets(ctx context.Context, lbDNSName string, l
 		return nil, fmt.Errorf("failed to describe listeners: %v", err)
 	}
 
+	framework.Logf("Found %d listeners for load balancer, checking for listener port %d", len(listenersOut.Listeners), int32(listenerPort))
 	targetGroupARNs := map[string]struct{}{}
 	for _, listener := range listenersOut.Listeners {
 		if aws.ToInt32(listener.Port) == int32(listenerPort) {
@@ -105,28 +171,79 @@ func (h *E2ETestHelperAWS) GetLBTargets(ctx context.Context, lbDNSName string, l
 			}
 		}
 	}
+	framework.Logf("Found %d target groups for listener", len(targetGroupARNs))
 	if len(targetGroupARNs) == 0 {
 		return nil, fmt.Errorf("no target groups found for LB: %s", lbARN)
 	}
 
 	targets := []string{}
 	for tgARN := range targetGroupARNs {
+		framework.Logf("Describing target group %s", tgARN)
 		tgHealth, err := h.elbv2Client.DescribeTargetHealth(ctx, &elbv2.DescribeTargetHealthInput{
 			TargetGroupArn: aws.String(tgARN),
 		})
 		if err != nil {
 			return nil, fmt.Errorf("failed to describe target health for TG %s: %v", tgARN, err)
 		}
+		framework.Logf("Found %d targets", len(tgHealth.TargetHealthDescriptions))
 		for _, target := range tgHealth.TargetHealthDescriptions {
-			if aws.ToInt32(target.Target.Port) == int32(targetPort) {
-				targets = append(targets, aws.ToString(target.Target.Id))
-			}
+			targets = append(targets, aws.ToString(target.Target.Id))
 		}
 	}
 	return targets, nil
 }
 
-// GetLoadBalancerFromDNSName describes a load balancers filtered by DNS name.
+// WaitForLBTargets polls until the specified load balancer has at least the expected number of targets.
+// This uses fixed-interval polling (not exponential backoff) because target registration is
+// eventual consistency, not an API failure scenario.
+//
+// AWS target registration typically takes 30-90 seconds for initial health checks.
+// Uses 5-second polling interval as a balance between responsiveness and API call volume.
+//
+// Example:
+//
+//	// Wait up to 3 minutes for at least 3 targets to be registered
+//	targets, err := helper.WaitForLBTargets(ctx, dnsName, 80, 8080, 3, 3*time.Minute)
+func (h *E2ETestHelperAWS) WaitForLBTargets(ctx context.Context, lbDNSName string, listenerPort, targetPort int32, minTargets int, timeout time.Duration) ([]string, error) {
+	var targets []string
+	var lastErr error
+
+	framework.Logf("Waiting for LB %s to have at least %d targets (timeout: %v) on port %d", lbDNSName, minTargets, timeout, listenerPort)
+
+	// Use fixed-interval polling for state convergence (Kubernetes standard pattern)
+	// Poll immediately first (true), then every 5 seconds
+	err := wait.PollUntilContextTimeout(ctx, DefaultTargetPollInterval, timeout, true, func(ctx context.Context) (bool, error) {
+		var err error
+		targets, err = h.GetLBTargets(ctx, lbDNSName, listenerPort, targetPort)
+		if err != nil {
+			// Log but continue polling - target groups might not be ready yet
+			framework.Logf("error getting LB targets (will retry): %v", err)
+			lastErr = err
+			return false, nil
+		}
+
+		framework.Logf("LB target status: found %d targets, waiting for at least %d", len(targets), minTargets)
+
+		if len(targets) >= minTargets {
+			framework.Logf("Target count satisfied: %d >= %d", len(targets), minTargets)
+			return true, nil // Success
+		}
+
+		return false, nil // Keep polling
+	})
+
+	if err != nil {
+		if lastErr != nil {
+			return targets, fmt.Errorf("timed out waiting for %d targets (last error: %v): %w", minTargets, lastErr, err)
+		}
+		return targets, fmt.Errorf("timed out waiting for %d targets (got %d): %w", minTargets, len(targets), err)
+	}
+
+	return targets, nil
+}
+
+// GetLoadBalancerFromDNSName performs a single attempt to describe load balancers filtered by DNS name.
+// For retry logic with exponential backoff, use GetLoadBalancerFromDNSNameWithBackoff.
 func (h *E2ETestHelperAWS) GetLoadBalancerFromDNSName(ctx context.Context, lbDNSName string) (*elbv2types.LoadBalancer, error) {
 	framework.Logf("describing load balancers with DNS %s", lbDNSName)
 
@@ -149,10 +266,10 @@ func (h *E2ETestHelperAWS) GetLoadBalancerFromDNSName(ctx context.Context, lbDNS
 	return nil, fmt.Errorf("no load balancer found with DNS name: %s", lbDNSName)
 }
 
-// GetLoadBalancerFromDNSNameWithTimeout describes a load balancers filtered by DNS name with retry using
-// exponential backoff.
-// AWS API
-func (h *E2ETestHelperAWS) GetLoadBalancerFromDNSNameWithTimeout(ctx context.Context, lbDNSName string, timeout time.Duration) (*elbv2types.LoadBalancer, error) {
+// GetLoadBalancerFromDNSNameWithBackoff describes a load balancer filtered by DNS name with retry using
+// exponential backoff and a custom timeout.
+// Use this when you need control over the timeout duration for specific test scenarios.
+func (h *E2ETestHelperAWS) GetLoadBalancerFromDNSNameWithBackoff(ctx context.Context, lbDNSName string, timeout time.Duration) (*elbv2types.LoadBalancer, error) {
 	var foundLB *elbv2types.LoadBalancer
 
 	ctx, cancel := context.WithTimeout(ctx, timeout)
@@ -187,10 +304,33 @@ func (h *E2ETestHelperAWS) GetLoadBalancerFromDNSNameWithTimeout(ctx context.Con
 	return foundLB, nil
 }
 
-// GetLoadBalancerFromDNSNameWithRetry describes a load balancers filtered by DNS name with
-// default retry values.
-// The default timeout is 20 minutes based on the AWS API limits and different regions
-// where DNS propagation.
+// GetLoadBalancerFromDNSNameDefaultTimeout describes a load balancer filtered by DNS name with
+// default retry configuration.
+// The default timeout is 20 minutes (configurable via E2E_LB_TIMEOUT_MINUTES env var),
+// based on AWS API limits and DNS propagation delays across different regions.
+//
+// Example: Override default timeout via environment variable:
+//
+//	export E2E_LB_TIMEOUT_MINUTES=30  # Use 30 minutes for slow environments
+func (h *E2ETestHelperAWS) GetLoadBalancerFromDNSNameDefaultTimeout(ctx context.Context, lbDNSName string) (*elbv2types.LoadBalancer, error) {
+	timeout := getDefaultLBTimeout()
+	return h.GetLoadBalancerFromDNSNameWithBackoff(ctx, lbDNSName, timeout)
+}
+
+// getDefaultLBTimeout returns the default load balancer timeout.
+// Checks E2E_LB_TIMEOUT_MINUTES environment variable first, falls back to DefaultRetryTimeoutMinutes.
+func getDefaultLBTimeout() time.Duration {
+	if timeoutStr := os.Getenv(EnvLBTimeoutMinutes); timeoutStr != "" {
+		if timeoutMinutes, err := strconv.Atoi(timeoutStr); err == nil && timeoutMinutes > 0 {
+			return time.Duration(timeoutMinutes) * time.Minute
+		}
+	}
+	return DefaultRetryTimeoutMinutes * time.Minute
+}
+
+// Deprecated: GetLoadBalancerFromDNSNameWithRetry is deprecated.
+// Use GetLoadBalancerFromDNSNameDefaultTimeout instead for clarity.
+// This function will be removed in a future version.
 func (h *E2ETestHelperAWS) GetLoadBalancerFromDNSNameWithRetry(ctx context.Context, lbDNSName string) (*elbv2types.LoadBalancer, error) {
-	return h.GetLoadBalancerFromDNSNameWithTimeout(ctx, lbDNSName, DefaultRetryTimeoutMinutes*time.Minute)
+	return h.GetLoadBalancerFromDNSNameDefaultTimeout(ctx, lbDNSName)
 }

tests/e2e/loadbalancer.go

@@ -119,9 +119,9 @@ var _ = Describe("[cloud-provider-aws-e2e] loadbalancer", func() {
 				}
 				lbDNS := cfg.svc.Status.LoadBalancer.Ingress[0].Hostname
 				// TODO expected lbDNS not empty
-				// TODO expect awshelper not nil
-				targets, err := cfg.GetAWSHelper().GetLBTargets(ctx, lbDNS, 80, cfg.GetServicePodPort())
-				framework.ExpectNoError(err, "failed to get LB target count")
+
+				targets, err := cfg.GetAWSHelper().WaitForLBTargets(ctx, lbDNS, 80, cfg.GetServicePodPort(), cfg.nodeCount, DefaultTargetWaitTimeout)
+				framework.ExpectNoError(err, "failed waiting for LB targets to register")
 				framework.Logf("LB targets: %v", targets)
 				gomega.Expect(len(targets)).To(gomega.Equal(cfg.nodeCount), "AWS LB target count validation failed")
 			},