Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 0 additions & 57 deletions cmd/cloud-controller-manager-aws-tests-ext/e2e/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,61 +5,13 @@ import (
"fmt"
"strings"

"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/config"
ec2 "github.com/aws/aws-sdk-go-v2/service/ec2"
ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
elbv2 "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2"
elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types"
configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/kubernetes/test/e2e/framework"
)

// AWS helpers

// createAWSClientLoadBalancer creates an AWS ELBv2 client using default credentials configured in the environment.
func createAWSClientLoadBalancer(ctx context.Context) (*elbv2.Client, error) {
cfg, err := config.LoadDefaultConfig(ctx)
if err != nil {
return nil, fmt.Errorf("unable to load AWS config: %v", err)
}
return elbv2.NewFromConfig(cfg), nil
}

// getAWSLoadBalancerFromDNSName finds a load balancer by DNS name using the AWS ELBv2 client.
func getAWSLoadBalancerFromDNSName(ctx context.Context, elbClient *elbv2.Client, lbDNSName string) (*elbv2types.LoadBalancer, error) {
var foundLB *elbv2types.LoadBalancer
framework.Logf("describing load balancers with DNS %s", lbDNSName)

paginator := elbv2.NewDescribeLoadBalancersPaginator(elbClient, &elbv2.DescribeLoadBalancersInput{})
for paginator.HasMorePages() {
page, err := paginator.NextPage(ctx)
if err != nil {
return nil, fmt.Errorf("failed to describe load balancers: %v", err)
}

framework.Logf("found %d load balancers in page", len(page.LoadBalancers))
// Search for the load balancer with matching DNS name in this page
for i := range page.LoadBalancers {
if aws.ToString(page.LoadBalancers[i].DNSName) == lbDNSName {
foundLB = &page.LoadBalancers[i]
framework.Logf("found load balancer with DNS %s", aws.ToString(foundLB.DNSName))
break
}
}
if foundLB != nil {
break
}
}

if foundLB == nil {
return nil, fmt.Errorf("no load balancer found with DNS name: %s", lbDNSName)
}

return foundLB, nil
}

// isFeatureEnabled checks if an OpenShift feature gate is enabled by querying the
// FeatureGate resource named "cluster" using the typed OpenShift config API.
//
Expand Down Expand Up @@ -120,15 +72,6 @@ func isFeatureEnabled(ctx context.Context, featureName string) (bool, error) {
return false, nil
}

// getAWSClientEC2 creates an AWS EC2 client using default credentials configured in the environment.
func createAWSClientEC2(ctx context.Context) (*ec2.Client, error) {
cfg, err := config.LoadDefaultConfig(ctx)
if err != nil {
return nil, fmt.Errorf("unable to load AWS config: %v", err)
}
return ec2.NewFromConfig(cfg), nil
}

// getAWSSecurityGroup retrieves a security group by ID using the AWS EC2 client.
func getAWSSecurityGroup(ctx context.Context, ec2Client *ec2.Client, sgID string) (*ec2types.SecurityGroup, error) {
framework.Logf("describing security group %s", sgID)
Expand Down
144 changes: 71 additions & 73 deletions cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"strings"
"time"

elbv2 "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2"
elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand All @@ -18,6 +17,8 @@ import (
"k8s.io/kubernetes/test/e2e/framework"
e2eservice "k8s.io/kubernetes/test/e2e/framework/service"
admissionapi "k8s.io/pod-security-admission/api"

ccme2e "k8s.io/cloud-provider-aws/tests/e2e"
)

const (
Expand Down Expand Up @@ -118,21 +119,21 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
It("should create NLB service with security group attached", func(ctx context.Context) {
isNLBFeatureEnabled(ctx)

By("creatomg required AWS clients")
elbClient, err := createAWSClientLoadBalancer(ctx)
framework.ExpectNoError(err, "failed to create AWS ELB client")
By("creating E2E test helper")
e2e, err := ccme2e.NewE2ETestHelper(ctx, cs)
framework.ExpectNoError(err, "failed to create E2E test helper")

By("creating test service and deployment configuration")
serviceName := "nlb-sg-crt"
_, jig, err := createServiceNLB(ctx, cs, ns, serviceName, map[int32]int32{80: 8080})
framework.ExpectNoError(err, "failed to create NLB service load balancer")

foundLB, lbDNS, err := getNLBMetaFromName(ctx, cs, ns, serviceName, elbClient)
foundLB, lbDNS, err := getNLBMetaFromName(ctx, cs, ns, serviceName, e2e)
framework.ExpectNoError(err, "failed to get NLB metadata")
Expect(foundLB).NotTo(BeNil(), "found load balancer is nil")

DeferCleanup(func(cleanupCtx context.Context) {
err := deleteServiceAndWaitForLoadBalancerDeletion(cleanupCtx, jig, lbDNS)
DeferCleanup(func() {
err := deleteServiceAndWaitForLoadBalancerDeletion(e2e, jig, lbDNS)
framework.ExpectNoError(err, "failed to delete service and wait for load balancer deletion")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
})

Expand Down Expand Up @@ -165,9 +166,9 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
It("should have security groups attached to default ingress controller NLB", func(ctx context.Context) {
isNLBFeatureEnabled(ctx)

By("creatomg required AWS clients")
elbClient, err := createAWSClientLoadBalancer(ctx)
framework.ExpectNoError(err, "failed to create AWS ELB client")
By("creating E2E test helper")
e2e, err := ccme2e.NewE2ETestHelper(ctx, cs)
framework.ExpectNoError(err, "failed to create E2E test helper")

By("getting default ingress controller service")
ingressNamespace := "openshift-ingress"
Expand Down Expand Up @@ -204,26 +205,23 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
}
framework.Logf("Default ingress controller is using NLB type")

// Eventually ensures that the load balancer is found and is in the Active state.
var foundLB *elbv2types.LoadBalancer
err = wait.PollUntilContextTimeout(ctx, 10*time.Second, 3*time.Minute, true, func(pollCtx context.Context) (bool, error) {
lb, err := getAWSLoadBalancerFromDNSName(pollCtx, elbClient, lbDNS)
Eventually(ctx, func(ctx context.Context) error {
lb, err := e2e.GetAWSHelper().GetLoadBalancerFromDNSName(lbDNS)
if err != nil {
framework.Logf("Failed to find load balancer with DNS %s: %v", lbDNS, err)
return false, nil
}
if lb != nil && lb.State != nil && lb.State.Code == elbv2types.LoadBalancerStateEnumActive {
foundLB = lb
return true, nil
return err
}
if lb == nil {
framework.Logf("Load balancer %s not returned yet", lbDNS)
return false, nil
return fmt.Errorf("load balancer not found")
}
framework.Logf("Load balancer not yet active, current state: %v", lb.State)
return false, nil
})
framework.ExpectNoError(err, "failed to find active load balancer")
Expect(foundLB).NotTo(BeNil(), "found load balancer is nil")
if lb.State == nil || lb.State.Code != elbv2types.LoadBalancerStateEnumActive {
return fmt.Errorf("load balancer state is not Active: %v", lb.State)
}
foundLB = lb
return nil
}).WithTimeout(20*time.Minute).Should(Succeed(),
"load balancer should be found and in the Active state")

By("verifying security groups are attached to the default ingress NLB")
Expect(len(foundLB.SecurityGroups)).To(BeNumerically(">", 0),
Expand Down Expand Up @@ -253,24 +251,21 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
It("should update security group rules when service is updated", func(ctx context.Context) {
isNLBFeatureEnabled(ctx)

By("creatomg required AWS clients")
ec2Client, err := createAWSClientEC2(ctx)
framework.ExpectNoError(err, "failed to create AWS EC2 client")

elbClient, err := createAWSClientLoadBalancer(ctx)
framework.ExpectNoError(err, "failed to create AWS ELB client")
By("creating E2E test helper")
e2e, err := ccme2e.NewE2ETestHelper(ctx, cs)
framework.ExpectNoError(err, "failed to create E2E test helper")

By("creating test service and deployment configuration")
serviceName := "nlb-sg-upd"
_, jig, err := createServiceNLB(ctx, cs, ns, serviceName, map[int32]int32{80: 8080})
framework.ExpectNoError(err, "failed to create NLB service load balancer")

foundLB, lbDNS, err := getNLBMetaFromName(ctx, cs, ns, serviceName, elbClient)
foundLB, lbDNS, err := getNLBMetaFromName(ctx, cs, ns, serviceName, e2e)
framework.ExpectNoError(err, "failed to get NLB metadata")
Expect(foundLB).NotTo(BeNil(), "found load balancer is nil")

DeferCleanup(func(cleanupCtx context.Context) {
err := deleteServiceAndWaitForLoadBalancerDeletion(cleanupCtx, jig, lbDNS)
DeferCleanup(func() {
err := deleteServiceAndWaitForLoadBalancerDeletion(e2e, jig, lbDNS)
framework.ExpectNoError(err, "failed to delete service and wait for load balancer deletion")
})
Comment on lines -272 to 270
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will have an issue here if we pass the existing e2e because the e2e will use the original test context which might be invalid/canceled eg by the test framework. We should either:

  1. create a new e2e object using the cleanupCtx and pass that down

or, better

  1. pass the cleanup context to deleteServiceAndWaitForLoadBalancerDeletion and change the upstream so that the context is passed at every operation and not stored in e2e.

From K8s blog on testing https://www.kubernetes.dev/blog/2023/04/12/e2e-testing-best-practices-reloaded/:

Don’t use the ctx passed into ginkgo.It in a ginkgo.DeferCleanup callback because the context will be canceled when the cleanup code runs.
[...]
Instead, register a function which accepts a new context

Copy link
Copy Markdown
Contributor Author

@mtulio mtulio Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion Federico, that's totally make sense as we are refactoring the e2e library to standardize the utilization of the retries preventing duplicated function retries.

The last update I've reviewed, still validating the overall context as well checking if the cleanup will succeed


Expand All @@ -280,7 +275,7 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
framework.Logf("Load balancer has %d security group(s) attached before update", len(foundLB.SecurityGroups))

By("getting security group rules")
originalSGRules, err := getAWSSecurityGroupRules(ctx, ec2Client, foundLB.SecurityGroups)
originalSGRules, err := getAWSSecurityGroupRules(ctx, e2e.GetAWSHelper().GetEC2Client(), foundLB.SecurityGroups)
framework.ExpectNoError(err, "failed to get security group rules")
Comment on lines +278 to 279
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't require the security-group rule count to increase.

The behavioral contract here is “ports 80 and 443 are present after the update.” AWS/CCM can replace or consolidate rules without increasing the total rule count, so len(originalSGRules) >= len(currentSGRules) can fail on a correct reconcile and create a false-negative e2e failure.

Also applies to: 297-320

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/cloud-controller-manager-aws-tests-ext/e2e/loadbalancer.go` around lines
281 - 282, The test currently asserts the security-group rule count increased by
comparing originalSGRules and currentSGRules; change this to assert that rules
allowing ports 80 and 443 (HTTP/HTTPS) exist in the current security group rules
regardless of count: after calling getAWSSecurityGroupRules(...) for the updated
load balancer (foundLB.SecurityGroups), scan currentSGRules for entries matching
port/protocol pairs 80/TCP and 443/TCP and fail only if either is missing;
update the same logic used around lines 297-320 to stop relying on
len(originalSGRules) vs len(currentSGRules) and instead check presence of
required ports in currentSGRules (use the getAWSSecurityGroupRules,
originalSGRules, currentSGRules, and foundLB symbols to locate the code).


By("updating service: adding a new port")
Expand All @@ -297,7 +292,7 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala

By("waiting for security group rules to include the new port 443")
Eventually(ctx, func(ctx context.Context) ([]int32, error) {
foundLB, err = getAWSLoadBalancerFromDNSName(ctx, elbClient, lbDNS)
foundLB, err = e2e.GetAWSHelper().GetLoadBalancerFromDNSName(lbDNS)
if err != nil {
framework.Logf("Error finding load balancer: %v", err)
return nil, err
Expand All @@ -311,7 +306,7 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
return nil, fmt.Errorf("load balancer has no security groups attached")
}

currentSGRules, err := getAWSSecurityGroupRules(ctx, ec2Client, foundLB.SecurityGroups)
currentSGRules, err := getAWSSecurityGroupRules(ctx, e2e.GetAWSHelper().GetEC2Client(), foundLB.SecurityGroups)
if err != nil {
framework.Logf("failed to get security group rules to calculate the diff: %v", err)
return nil, err
Expand Down Expand Up @@ -364,20 +359,17 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
It("should cleanup security groups when service is deleted", func(ctx context.Context) {
isNLBFeatureEnabled(ctx)

By("creatomg required AWS clients")
ec2Client, err := createAWSClientEC2(ctx)
framework.ExpectNoError(err, "failed to create AWS EC2 client")

elbClient, err := createAWSClientLoadBalancer(ctx)
framework.ExpectNoError(err, "failed to create AWS ELB client")
By("creating E2E test helper")
e2e, err := ccme2e.NewE2ETestHelper(ctx, cs)
framework.ExpectNoError(err, "failed to create E2E test helper")

By("creating test service and deployment configuration")
serviceName := "nlb-sg-cleanup-test"

_, jig, err := createServiceNLB(ctx, cs, ns, serviceName, map[int32]int32{80: 8080})
framework.ExpectNoError(err, "failed to create NLB service load balancer")

foundLB, lbDNS, err := getNLBMetaFromName(ctx, cs, ns, serviceName, elbClient)
foundLB, lbDNS, err := getNLBMetaFromName(ctx, cs, ns, serviceName, e2e)
framework.ExpectNoError(err, "failed to get NLB metadata")
Expect(foundLB).NotTo(BeNil(), "found load balancer is nil")

Expand All @@ -393,20 +385,20 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala

By("verifying security groups exist before deletion")
for _, sgID := range securityGroupIDs {
exists, err := securityGroupExists(ctx, ec2Client, sgID)
exists, err := securityGroupExists(ctx, e2e.GetAWSHelper().GetEC2Client(), sgID)
framework.ExpectNoError(err, "failed to check security group %s", sgID)
Expect(exists).To(BeTrue(), "security group %s should exist before deletion", sgID)
}

err = deleteServiceAndWaitForLoadBalancerDeletion(ctx, jig, lbDNS)
err = deleteServiceAndWaitForLoadBalancerDeletion(e2e, jig, lbDNS)
framework.ExpectNoError(err, "failed to delete service and wait for load balancer deletion")

By("verifying managed security groups are cleaned up")
// Poll for security group cleanup with timeout (AWS cleanup can take time)
err = wait.PollUntilContextTimeout(ctx, 10*time.Second, 3*time.Minute, true, func(ctx context.Context) (bool, error) {
allDeleted := true
for _, sgID := range securityGroupIDs {
exists, err := securityGroupExists(ctx, ec2Client, sgID)
exists, err := securityGroupExists(ctx, e2e.GetAWSHelper().GetEC2Client(), sgID)
if err != nil {
framework.Logf("Error checking security group %s: %v", sgID, err)
return false, err
Expand Down Expand Up @@ -447,12 +439,9 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
It("should have correct security group rules for service ports", func(ctx context.Context) {
isNLBFeatureEnabled(ctx)

By("creatomg required AWS clients")
ec2Client, err := createAWSClientEC2(ctx)
framework.ExpectNoError(err, "failed to create AWS EC2 client")

elbClient, err := createAWSClientLoadBalancer(ctx)
framework.ExpectNoError(err, "failed to create AWS ELB client")
By("creating E2E test helper")
e2e, err := ccme2e.NewE2ETestHelper(ctx, cs)
framework.ExpectNoError(err, "failed to create E2E test helper")

By("creating test service and deployment configuration")
serviceName := "nlb-sg-rules-test"
Expand All @@ -465,12 +454,12 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
lbDNS := svc.Status.LoadBalancer.Ingress[0].Hostname
framework.Logf("Load balancer DNS: %s", lbDNS)

foundLB, lbDNS, err := getNLBMetaFromName(ctx, cs, ns, serviceName, elbClient)
foundLB, lbDNS, err := getNLBMetaFromName(ctx, cs, ns, serviceName, e2e)
framework.ExpectNoError(err, "failed to get NLB metadata")
Expect(foundLB).NotTo(BeNil(), "found load balancer is nil")

DeferCleanup(func(cleanupCtx context.Context) {
err := deleteServiceAndWaitForLoadBalancerDeletion(cleanupCtx, jig, lbDNS)
DeferCleanup(func() {
err := deleteServiceAndWaitForLoadBalancerDeletion(e2e, jig, lbDNS)
framework.ExpectNoError(err, "failed to delete service and wait for load balancer deletion")
})

Expand All @@ -480,7 +469,7 @@ var _ = Describe(fmt.Sprintf("%s NLB [OCPFeatureGate:%s]", e2eTestPrefixLoadBala
framework.Logf("Load balancer has %d security group(s) attached", len(foundLB.SecurityGroups))

By("inspecting security group rules")
currentSGRules, err := getAWSSecurityGroupRules(ctx, ec2Client, foundLB.SecurityGroups)
currentSGRules, err := getAWSSecurityGroupRules(ctx, e2e.GetAWSHelper().GetEC2Client(), foundLB.SecurityGroups)
framework.ExpectNoError(err, "failed to get security group rules to calculate the diff")
Expect(len(currentSGRules)).To(BeNumerically(">", 0), "security group rules should not be empty")

Expand Down Expand Up @@ -543,42 +532,51 @@ func createServiceNLB(ctx context.Context, cs clientset.Interface, ns *v1.Namesp
}

// getNLBMetaFromName gets the NLB metadata (AWS API object) from the service/loadbalancer name.
func getNLBMetaFromName(ctx context.Context, cs clientset.Interface, ns *v1.Namespace, serviceName string, elbc *elbv2.Client) (*elbv2types.LoadBalancer, string, error) {
func getNLBMetaFromName(ctx context.Context, cs clientset.Interface, ns *v1.Namespace, serviceName string, e2e *ccme2e.E2ETestHelper) (*elbv2types.LoadBalancer, string, error) {
By("getting service to extract load balancer DNS name")
svc, err := cs.CoreV1().Services(ns.Name).Get(ctx, serviceName, metav1.GetOptions{})
framework.ExpectNoError(err, "failed to get service %s", serviceName)
if err != nil {
return nil, "", fmt.Errorf("failed to get service %s: %v", serviceName, err)
}

By("extracting load balancer DNS name")
Expect(len(svc.Status.LoadBalancer.Ingress)).To(BeNumerically(">", 0),
"no ingress entry found in LoadBalancer status")

if len(svc.Status.LoadBalancer.Ingress) == 0 {
return nil, "", fmt.Errorf("no ingress entry found in LoadBalancer status")
}
lbDNS := svc.Status.LoadBalancer.Ingress[0].Hostname
Expect(lbDNS).NotTo(BeEmpty(), "Ingress Hostname must not be empty")
if lbDNS == "" {
return nil, "", fmt.Errorf("Ingress Hostname must not be empty")
}
framework.Logf("Load balancer DNS: %s", lbDNS)

foundLB, err := getAWSLoadBalancerFromDNSName(ctx, elbc, lbDNS)
framework.ExpectNoError(err, "failed to find load balancer with DNS name %s", lbDNS)
Expect(foundLB).NotTo(BeNil(), "found load balancer is nil")
// Lookup the Load Balancer from the DNS name with default retries.
foundLB, err := e2e.GetAWSHelper().GetLoadBalancerFromDNSNameWithRetry(lbDNS)
if err != nil {
e2e.GatherServiceInfo()
return nil, "", fmt.Errorf("failed to find load balancer with DNS name %s: %v", lbDNS, err)
}
if foundLB == nil {
return nil, "", fmt.Errorf("found load balancer is nil")
}

return foundLB, lbDNS, nil
}

// deleteServiceAndWaitForLoadBalancerDeletion deletes the service and waits for the load balancer to be deleted.
func deleteServiceAndWaitForLoadBalancerDeletion(ctx context.Context, jig *e2eservice.TestJig, lbDNS string) error {
func deleteServiceAndWaitForLoadBalancerDeletion(e2e *ccme2e.E2ETestHelper, jig *e2eservice.TestJig, lbDNS string) error {
By("deleting the service")
// using deletion context to avoid leaking resources
ctx := context.TODO()
err := jig.Client.CoreV1().Services(jig.Namespace).Delete(ctx, jig.Name, metav1.DeleteOptions{})
framework.ExpectNoError(err, "failed to delete service")

By("waiting for load balancer to be destroyed")
loadBalancerCreateTimeout := e2eservice.GetServiceLoadBalancerCreationTimeout(ctx, jig.Client)

// Get ELB client once before polling
elbClient, err := createAWSClientLoadBalancer(ctx)
framework.ExpectNoError(err, "failed to create AWS ELB client")

// Poll for load balancer deletion
// Get the load balancer from the DNS name with retry toleranting
// errors when not found as we expect the load balancer to be deleted.
err = wait.PollUntilContextTimeout(ctx, 5*time.Second, loadBalancerCreateTimeout, true, func(ctx context.Context) (bool, error) {
foundLB, err := getAWSLoadBalancerFromDNSName(ctx, elbClient, lbDNS)
foundLB, err := e2e.GetAWSHelper().GetLoadBalancerFromDNSName(lbDNS)
if err != nil {
// Check if the error indicates the load balancer was not found (i.e., successfully deleted)
if strings.Contains(err.Error(), "no load balancer found with DNS name") {
Expand Down
Loading