OCPCLOUD-3215: test/ccm: add support for verifying NLB health check configuration#30952
OCPCLOUD-3215: test/ccm: add support for verifying NLB health check configuration#30952openshift-merge-bot[bot] merged 3 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (51)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughUpdated AWS SDK Go dependency and refactored ELB client to support both ELBv1 (Classic) and ELBv2 (NLB/ALB) health-check retrieval; test now selects the appropriate health-check retrieval method based on the IPv6-primary flag and compares against a precomputed expected value. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/cc @nrb @alebedev87 @sadasu |
|
The aws sdk (v1) in this repo seems quite old and requires a version bump in order to get the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/util/aws_client.go (1)
97-105:⚠️ Potential issue | 🔴 CriticalReturn explicit errors when the AWS objects are missing.
The CLB branch can still panic on an empty result or nil target, while the NLB branch returns
("", nil)and turns lookup failures into opaque assertion mismatches intest/extended/cloud_controller_manager/ccm.go. Both helpers should fail fast with an error.Suggested fix
if len(result.LoadBalancerDescriptions) == 0 { - e2e.Logf("Failed to get classic load balancers: %v", err) + return "", fmt.Errorf("classic load balancer %q not found", lbName) } healthCheck := result.LoadBalancerDescriptions[0].HealthCheck - if healthCheck == nil { - e2e.Logf("Failed to get health check: %v", err) + if healthCheck == nil || healthCheck.Target == nil { + return "", fmt.Errorf("classic load balancer %q has no health check target", lbName) } - return *healthCheck.Target, nil + return aws.StringValue(healthCheck.Target), nil @@ if len(result.LoadBalancers) == 0 { - e2e.Logf("No NLB found with name: %s", lbName) - return "", nil + return "", fmt.Errorf("network load balancer %q not found", lbName) } @@ if len(tgResult.TargetGroups) == 0 { - e2e.Logf("No target groups found for NLB: %s", lbName) - return "", nil + return "", fmt.Errorf("no target groups found for network load balancer %q", lbName) }Also applies to: 120-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/aws_client.go` around lines 97 - 105, The code currently logs failures and then dereferences result.LoadBalancerDescriptions[0].HealthCheck and healthCheck.Target which can panic; update the helper that reads result.LoadBalancerDescriptions to return explicit errors instead of calling e2e.Logf and continuing: if result.LoadBalancerDescriptions is empty return a descriptive error, if HealthCheck is nil return an error, and if healthCheck.Target is empty or nil return an error; apply the same change to the NLB branch (the code around lines 120-137) so both branches fail fast with returned errors rather than returning ("", nil) or panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/cloud_controller_manager/ccm.go`:
- Around line 156-166: Wrap the direct call to
elbClient.GetNLBHealthCheckPortPath/GetCLBHealthCheckPortPath with a bounded
poll (e.g., using o.Eventually) so the test retries until a non-empty
healthCheck is returned before asserting equality; replace the immediate calls
that set healthCheck with a closure that calls the appropriate getter based on
isIPv6Primary, returns the string (and logs or returns "" on error), and use
o.Eventually(func() string { ... }, timeout,
interval).Expect(o.Equal(expectedHealthCheck)) to assert the exact
"HTTP:10256/healthz" value once available.
---
Outside diff comments:
In `@test/extended/util/aws_client.go`:
- Around line 97-105: The code currently logs failures and then dereferences
result.LoadBalancerDescriptions[0].HealthCheck and healthCheck.Target which can
panic; update the helper that reads result.LoadBalancerDescriptions to return
explicit errors instead of calling e2e.Logf and continuing: if
result.LoadBalancerDescriptions is empty return a descriptive error, if
HealthCheck is nil return an error, and if healthCheck.Target is empty or nil
return an error; apply the same change to the NLB branch (the code around lines
120-137) so both branches fail fast with returned errors rather than returning
("", nil) or panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 670912db-49fe-432b-a97f-1adc4dcc40bd
⛔ Files ignored due to path filters (51)
go.sumis excluded by!**/*.sumvendor/github.com/aws/aws-sdk-go/aws/arn/arn.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/auth/bearer/token.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/awserr/error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/awsutil/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/client/client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/client/metadata/client_info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/corehandlers/handlers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/credentials/credentials.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/csm/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/defaults/defaults.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/ec2metadata/service.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/endpoints/defaults.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/endpoints/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/request/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/request/waiter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/session/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/session/shared_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/signer/v4/v4.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/aws/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/private/protocol/query/queryutil/queryutil.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/ec2/api.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/ec2/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/ecr/api.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/ecr/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/ecr/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/elb/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/elbv2/api.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/elbv2/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/elbv2/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/elbv2/service.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/elbv2/waiters.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/iam/api.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/iam/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/iam/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/kms/api.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/kms/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/kms/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/route53/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/s3/api.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/s3/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/s3/s3manager/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/s3/s3manager/download.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/s3/s3manager/upload_input.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/sso/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/ssooidc/api.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/ssooidc/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/ssooidc/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/aws/aws-sdk-go/service/sts/doc.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (3)
go.modtest/extended/cloud_controller_manager/ccm.gotest/extended/util/aws_client.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/util/aws_client.go (1)
77-85:⚠️ Potential issue | 🔴 CriticalMissing error handling leads to panics.
If
LoadBalancerDescriptionsis empty (line 77), the code logs but continues to access index[0]on line 81, causing an index out of bounds panic. Similarly, ifhealthCheckis nil (line 82), dereferencinghealthCheck.Targeton line 85 will panic.Additionally, the log messages reference
errwhich isnilat these points.🐛 Proposed fix to handle empty/nil cases
if len(result.LoadBalancerDescriptions) == 0 { - e2e.Logf("Failed to get classic load balancers: %v", err) + e2e.Logf("No classic load balancer found with name: %s", lbName) + return "", nil } healthCheck := result.LoadBalancerDescriptions[0].HealthCheck if healthCheck == nil { - e2e.Logf("Failed to get health check: %v", err) + e2e.Logf("No health check configured for CLB: %s", lbName) + return "", nil } return *healthCheck.Target, nilThis aligns error handling with
GetNLBHealthCheckPortPathwhich properly returns("", nil)for empty cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/aws_client.go` around lines 77 - 85, The code currently logs but continues when result.LoadBalancerDescriptions is empty and when healthCheck is nil, causing index/method panics and incorrectly referencing a nil err; update the function to check if len(result.LoadBalancerDescriptions) == 0 and immediately return "", nil (no panic) instead of accessing [0], and similarly check if healthCheck == nil and return "", nil rather than dereferencing healthCheck.Target; also remove or fix the log lines that reference the non-existent err (use a descriptive message without err or include the actual error where available) so behavior matches GetNLBHealthCheckPortPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/extended/util/aws_client.go`:
- Around line 77-85: The code currently logs but continues when
result.LoadBalancerDescriptions is empty and when healthCheck is nil, causing
index/method panics and incorrectly referencing a nil err; update the function
to check if len(result.LoadBalancerDescriptions) == 0 and immediately return "",
nil (no panic) instead of accessing [0], and similarly check if healthCheck ==
nil and return "", nil rather than dereferencing healthCheck.Target; also remove
or fix the log lines that reference the non-existent err (use a descriptive
message without err or include the actual error where available) so behavior
matches GetNLBHealthCheckPortPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d7c0a01-7d98-49ff-9f8f-ece8de303020
📒 Files selected for processing (2)
test/extended/cloud_controller_manager/ccm.gotest/extended/util/aws_client.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended/cloud_controller_manager/ccm.go
Extend the CCM load balancer test to verify health check configuration for both Classic Load Balancers (CLB) and Network Load Balancers (NLB). IPv6 dual-stack clusters use NLB, while single-stack/IPv4 primary use CLB. The test now checks that health check port and path are correctly configured to 10256/healthz for both load balancer types by querying target group configuration for NLB and load balancer description for CLB.
|
/test e2e-aws-csi |
|
/cc @neisw |
|
/lgtm |
|
/test e2e-gcp-csi |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, nrb, petr-muller, tthvo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
openshift-ci-robot
added
the
verified
|
@tthvo: This PR has been marked as verified by DetailsIn response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retitle OCPCLOUD-3215: test/ccm: add support for verifying NLB health check configuration This is a follow-up for #30938 |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@tthvo: This pull request references OCPCLOUD-3215 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/tide refresh |
|
/test e2e-gcp-ovn |
|
@tthvo: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
5 participants
Description
Extend the CCM load balancer test to verify health check configuration for both Classic Load Balancers (CLB) and Network Load Balancers (NLB). IPv6 dual-stack clusters use NLB, while single-stack/IPv4 primary use CLB.
The test now checks that health check port and path are correctly configured to 10256/healthz for both load balancer types by querying target group configuration for NLB and load balancer description for CLB.
Important
NLBs do not define health check on the Load balancer itself, but rather on its associated target group.