WIP/test/e2e: export load balancer helpers by mtulio · Pull Request #1381 · kubernetes/cloud-provider-aws

mtulio · 2026-04-14T16:30:14Z

What type of PR is this?

/kind cleanup
/kind design
/kind failing-test

What this PR does / why we need it:

This PR introduces exports for e2e library to be extended in test frameworks of layered controllers.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

k8s-ci-robot · 2026-04-14T16:30:18Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2026-04-14T16:30:18Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2026-04-14T16:30:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kmala for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2026-04-14T16:30:24Z

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mtulio · 2026-04-14T16:45:16Z

/test pull-cloud-provider-aws-test

mtulio · 2026-04-14T17:08:44Z

/test ?

mtulio · 2026-04-14T17:09:26Z

/test pull-cloud-provider-aws-e2e

mtulio · 2026-04-14T17:56:03Z

Covnerting to reguar PR just to trigger e2e

mtulio · 2026-04-14T17:56:24Z

/uncc dmis olemarkus

mtulio · 2026-04-14T17:56:55Z

/hold

mtulio · 2026-04-14T21:26:01Z

/test pull-cloud-provider-aws-e2e

mtulio · 2026-04-15T03:05:43Z

/test pull-cloud-provider-aws-e2e

mtulio · 2026-04-15T04:16:03Z

Node discovery for hairpinning traffic was failing:

/test pull-cloud-provider-aws-e2e

mtulio · 2026-04-24T02:38:59Z

/test pull-cloud-provider-aws-e2e

mtulio · 2026-04-27T15:43:46Z

Updated the worker node lookup function to get compatibility with existing logic:

/test pull-cloud-provider-aws-e2e

mtulio · 2026-04-27T19:44:48Z

/test pull-cloud-provider-aws-e2e

mtulio · 2026-04-27T22:06:32Z

local tests passing

$ ./e2e.test --ginkgo.v --ginkgo.focus="loadbalancer"
....
• [219.703 seconds]
------------------------------

Ran 5 of 8 Specs in 880.678 seconds
SUCCESS! -- 5 Passed | 0 Failed | 0 Pending | 3 Skipped

/test pull-cloud-provider-aws-e2e

mtulio · 2026-04-28T04:51:35Z

/test ?

mtulio · 2026-04-28T04:53:39Z

/test pull-cloud-provider-aws-e2e-kubetest2
/test pull-cloud-provider-aws-check
/test pull-cloud-provider-aws-test

mfbonfigli · 2026-04-28T11:27:28Z

+func (e *E2ETestHelper) cleanup(ctx context.Context) {
+	framework.Logf("Cleaning up e2e resources")
+	// TODO cleanup resources created by the test helper
+}


Do we need this or does it need to be implemented?

mfbonfigli · 2026-04-28T11:30:13Z

 				}
 				lbDNS := cfg.svc.Status.LoadBalancer.Ingress[0].Hostname
-				framework.ExpectNoError(getLBTargetCount(cfg.ctx, lbDNS, cfg.nodeCount), "AWS LB target count validation failed")
+				// TODO expected lbDNS not empty


nit: remove comment or add validation of lbDNS

mfbonfigli · 2026-04-28T11:36:33Z

 				// Use AWS SDK paginator to search through all load balancers
-				foundLB, err := getAWSLoadBalancerFromDNSName(e2e.ctx, elbClient, hostAddr)
-				framework.ExpectNoError(err, "failed to find load balancer with DNS name %s", hostAddr)
+				foundLB, err := e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(ctx, hostAddr)


GetLoadBalancerFromDNSNameWithRetry is marked as deprecated (see comment to the function), if so we should not use it in new code.

mfbonfigli · 2026-04-28T11:37:57Z

+						Capabilities: &v1.Capabilities{
+							Drop: []v1.Capability{"ALL"},
+						},
+						ReadOnlyRootFilesystem: aws.Bool(true),


nit: I don't particularly like using the AWS SDK helpers (aws.Bool in this case) outside AWS-related blocks of code, it adds unnecessary coupling.

mfbonfigli · 2026-04-28T11:38:26Z

@@ -0,0 +1,883 @@
+/*
+Copyright 2025 The Kubernetes Authors.


Should be 2026

mfbonfigli · 2026-04-28T12:14:10Z

+func (h *E2ETestHelperAWS) GetLBTargets(ctx context.Context, lbDNSName string, listenerPort, targetPort int32) ([]string, error) {
+	foundLB, err := h.GetLoadBalancerFromDNSNameDefaultTimeout(ctx, lbDNSName)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get load balancer from DNS name: %v", err)


nit (was already present before): it is good practice to error wrap with %w instead of %v, consider fixing.

mfbonfigli · 2026-04-28T12:14:17Z

+		LoadBalancerArn: aws.String(lbARN),
+	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to describe listeners: %v", err)


nit (was already present before): it is good practice to error wrap with %w instead of %v, consider fixing.

mfbonfigli · 2026-04-28T12:39:55Z

+//	    MaxAttempts: 5,
+//	    MaxBackoff: 10 * time.Second,
+//	})
+func NewAWSHelperWithOptions(ctx context.Context, opts E2ETestHelperAWSOptions) (*E2ETestHelperAWS, error) {


nit, just an alternative approach to E2ETestHelperAWSOptions:

It is quite common in go to use the "Functional Options" pattern to pass optional configurations, and it might work well here as well. For example instead of having a NewAWSHelperWithOptions and a E2ETestHelperAWS struct, we could have:

type E2ETestHelperAWSOpt func(*E2ETestHelperAWS) func NewAWSHelper(ctx context.Context, opts ...E2ETestHelperAWSOpt) (*E2ETestHelperAWS, error) { h := &E2ETestHelperAWS {} for _, optFn := range opts { optFn(h) } // rest of code down here } func WithMaxAttempts(attempts int) func(*E2ETestHelperAWS) { return func(h *E2ETestHelperAWS) { h.MaxAttempts = attempts } } func WithMaxBackoff(maxBackoff time.Duration) func(*E2ETestHelperAWS) { return func(h *E2ETestHelperAWS) { h.MaxBackoff = maxBackoff } }

With this pattern you have only 1 constructor and the user can pass only the configurations he wants to edit.
For example to get the default one would do:

h, err := NewAWSHelper(ctx)

And instead to customize only one of the settings would do:

h, err := NewAWSHelper(ctx, WithMaxAttempts(5))

I think this approach is less code and more flexible.

mfbonfigli · 2026-04-28T13:11:34Z

+			framework.Logf("error getting LB targets (will retry): %v", err)
+			lastErr = err


we could consider isolating and treating differently retryable vs non-retryable errors here too.

mfbonfigli · 2026-04-28T13:18:49Z

+	opts.ApplyDefaults()
+
+	cfg, err := config.LoadDefaultConfig(ctx)
+	framework.ExpectNoError(err, "unable to load AWS config")


nit: Should we do test assertions inside the helpers (here and in other parts of the PR)? Not a strong opinion, just generally I'd do assertions inside the test function.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 14, 2026

k8s-ci-robot requested review from cheftako and yue9944882 April 14, 2026 16:30

k8s-ci-robot added the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Apr 14, 2026

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 14, 2026

mtulio mentioned this pull request Apr 14, 2026

DNM: OTE/CCM-AWS: update e2e fetching upstream exports openshift/cluster-cloud-controller-manager-operator#447

Draft

mtulio changed the title ~~test/e2e: export load balancer helpers~~ WIP/test/e2e: export load balancer helpers Apr 14, 2026

mtulio marked this pull request as ready for review April 14, 2026 17:56

k8s-ci-robot requested review from dims and olemarkus April 14, 2026 17:56

k8s-ci-robot removed the request for review from olemarkus April 14, 2026 17:56

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2026

mtulio force-pushed the e2e-lb-export branch from fc9c0a2 to b7a710f Compare April 14, 2026 21:10

mtulio marked this pull request as draft April 14, 2026 21:25

mtulio force-pushed the e2e-lb-export branch from b7a710f to 4e08365 Compare April 15, 2026 03:05

mfbonfigli reviewed Apr 16, 2026

View reviewed changes

mtulio mentioned this pull request Apr 21, 2026

Fix leak managed/owned security group on Service update with BYO SG on CLB #1209

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2026

mtulio force-pushed the e2e-lb-export branch from 5763be2 to 9b0ee8b Compare April 24, 2026 01:19

mtulio added 7 commits April 23, 2026 22:55

e2e/go.mod: update AWS GO SDK

f8533f0

e2e: create helper and export common functions

b78eaa1

review sample node discovery

0205e67

e2e: increase curl timeout

1e8dd14

e2e/fix: variable shadowing on node list

5907530

e2e/nit: decrease vebosity

0948cef

e2e: export both GetLoadBalancerFromDNSNameWithRetry and WithTImeout

d2e817a

mtulio force-pushed the e2e-lb-export branch from 9b0ee8b to d2e817a Compare April 24, 2026 02:03

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2026

fix/e2e: resolve discoverClusterWorkerNode lookup

9761c7e

mtulio added 2 commits April 27, 2026 14:58

fix e2e curl success validation pattern

f97fccb

e2e/docs: update devel helper documentation

49b2f8e

e2e/fix: update aws node/retry resilience

234b7ed

mtulio force-pushed the e2e-lb-export branch from c9ed1f6 to 234b7ed Compare April 27, 2026 22:05

mfbonfigli reviewed Apr 28, 2026

View reviewed changes

		framework.Logf("error getting LB targets (will retry): %v", err)
		lastErr = err

Conversation

mtulio commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 14, 2026

Uh oh!

k8s-ci-robot commented Apr 14, 2026

Uh oh!

k8s-ci-robot commented Apr 14, 2026

Uh oh!

k8s-ci-robot commented Apr 14, 2026

Uh oh!

mtulio commented Apr 14, 2026

Uh oh!

mtulio commented Apr 14, 2026

Uh oh!

mtulio commented Apr 14, 2026

Uh oh!

mtulio commented Apr 14, 2026

Uh oh!

mtulio commented Apr 14, 2026

Uh oh!

mtulio commented Apr 14, 2026

Uh oh!

mtulio commented Apr 14, 2026

Uh oh!

mtulio commented Apr 15, 2026

Uh oh!

mtulio commented Apr 15, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtulio commented Apr 24, 2026

Uh oh!

mtulio commented Apr 27, 2026

Uh oh!

mtulio commented Apr 27, 2026

Uh oh!

mtulio commented Apr 27, 2026

Uh oh!

mtulio commented Apr 28, 2026

Uh oh!

mtulio commented Apr 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtulio commented Apr 14, 2026 •

edited

Loading