Revert #476 "SPLAT-2713: Update AWS CCM e2e test module version" - #491
Conversation
|
@redhat-chai-bot: This pull request references SPLAT-2713 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 "5.0.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. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughAWS e2e tests now resolve regions from environment variables or the cluster Infrastructure resource, with local partition-aware validation. The BareMetal controller test uses a direct ConfigMap assertion instead of polling, and the AWS e2e dependency is updated. ChangesAWS region resolution
BareMetal ConfigMap assertion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TestEnvironment
participant getRegionFromEnv
participant loadAWSConfig
participant getRegionFromInfrastructure
participant InfrastructureAPI
TestEnvironment->>getRegionFromEnv: Read region environment variables
getRegionFromEnv->>loadAWSConfig: Set AWS_REGION from first non-empty value
loadAWSConfig->>getRegionFromInfrastructure: Request cluster AWS region
getRegionFromInfrastructure->>InfrastructureAPI: Fetch Infrastructure resource
InfrastructureAPI-->>getRegionFromInfrastructure: Return AWS region
getRegionFromInfrastructure-->>loadAWSConfig: Return region or error
loadAWSConfig->>loadAWSConfig: Validate fallback region
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift-tests/ccm-aws-tests/e2e/aws/helper.go`:
- Around line 45-50: Update the getRegionFromInfrastructure fallback handling so
a failure is not silently ignored when cfg.Region already matches
awsRegionPattern. Log the retrieval error before continuing with the valid
configured region, while preserving the existing invalid-region error return and
successful region assignment behavior.
In `@pkg/controllers/cloud_config_sync_controller_test.go`:
- Around line 607-609: Update the ConfigMap listing assertion in the test to use
a child context with an explicit timeout instead of the unbounded suite-level
context. Create the timeout context immediately before cl.List, pass it to that
call, and ensure the context is released after the operation.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4bc38063-d675-4715-b21d-dbec075b02bc
⛔ Files ignored due to path filters (4)
openshift-tests/ccm-aws-tests/go.sumis excluded by!**/*.sumopenshift-tests/ccm-aws-tests/vendor/k8s.io/cloud-provider-aws/tests/e2e/cloudconfig.gois excluded by!**/vendor/**openshift-tests/ccm-aws-tests/vendor/k8s.io/cloud-provider-aws/tests/e2e/loadbalancer.gois excluded by!**/vendor/**openshift-tests/ccm-aws-tests/vendor/modules.txtis excluded by!**/vendor/**
📒 Files selected for processing (5)
openshift-tests/ccm-aws-tests/e2e/aws/helper.goopenshift-tests/ccm-aws-tests/e2e/common/helper.goopenshift-tests/ccm-aws-tests/go.modopenshift-tests/ccm-aws-tests/main.gopkg/controllers/cloud_config_sync_controller_test.go
💤 Files with no reviewable changes (1)
- openshift-tests/ccm-aws-tests/e2e/common/helper.go
| region, err := getRegionFromInfrastructure(ctx) | ||
| if err == nil { | ||
| cfg.Region = region | ||
| } else if !common.AWSRegionPattern.MatchString(cfg.Region) { | ||
| } else if !awsRegionPattern.MatchString(cfg.Region) { | ||
| return aws.Config{}, fmt.Errorf("AWS region %q is not valid and failed to get region from Infrastructure: %w", cfg.Region, err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Do not ignore the error when falling back to a valid region.
If getRegionFromInfrastructure fails, but the fallback cfg.Region is valid according to the regex, the error returned from getRegionFromInfrastructure is silently swallowed. As per path instructions, "Never ignore error returns".
If the error is acceptable in this fallback scenario, it should at least be logged so that underlying API or RBAC failures are not completely hidden.
💻 Proposed fix
region, err := getRegionFromInfrastructure(ctx)
if err == nil {
cfg.Region = region
- } else if !awsRegionPattern.MatchString(cfg.Region) {
- return aws.Config{}, fmt.Errorf("AWS region %q is not valid and failed to get region from Infrastructure: %w", cfg.Region, err)
+ } else {
+ if !awsRegionPattern.MatchString(cfg.Region) {
+ return aws.Config{}, fmt.Errorf("AWS region %q is not valid and failed to get region from Infrastructure: %w", cfg.Region, err)
+ }
+ framework.Logf("Failed to get region from Infrastructure (falling back to %q): %v", cfg.Region, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| region, err := getRegionFromInfrastructure(ctx) | |
| if err == nil { | |
| cfg.Region = region | |
| } else if !common.AWSRegionPattern.MatchString(cfg.Region) { | |
| } else if !awsRegionPattern.MatchString(cfg.Region) { | |
| return aws.Config{}, fmt.Errorf("AWS region %q is not valid and failed to get region from Infrastructure: %w", cfg.Region, err) | |
| } | |
| region, err := getRegionFromInfrastructure(ctx) | |
| if err == nil { | |
| cfg.Region = region | |
| } else { | |
| if !awsRegionPattern.MatchString(cfg.Region) { | |
| return aws.Config{}, fmt.Errorf("AWS region %q is not valid and failed to get region from Infrastructure: %w", cfg.Region, err) | |
| } | |
| framework.Logf("Failed to get region from Infrastructure (falling back to %q): %v", cfg.Region, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/ccm-aws-tests/e2e/aws/helper.go` around lines 45 - 50, Update
the getRegionFromInfrastructure fallback handling so a failure is not silently
ignored when cfg.Region already matches awsRegionPattern. Log the retrieval
error before continuing with the valid configured region, while preserving the
existing invalid-region error return and successful region assignment behavior.
Source: Path instructions
| allCMs := &corev1.ConfigMapList{} | ||
| Expect(cl.List(ctx, allCMs, &client.ListOptions{Namespace: targetNamespaceName})).To(Succeed()) | ||
| Expect(len(allCMs.Items)).To(BeZero()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the ConfigMap list call.
cl.List uses the suite-level context.Background() without a deadline, so the test can hang indefinitely if the API server is unavailable. Create a child context with an explicit timeout for this cluster operation.
As per path instructions, operations interacting with clusters must include timeouts.
Proposed fix
allCMs := &corev1.ConfigMapList{}
- Expect(cl.List(ctx, allCMs, &client.ListOptions{Namespace: targetNamespaceName})).To(BeNil())
+ listCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+ Expect(cl.List(listCtx, allCMs, &client.ListOptions{Namespace: targetNamespaceName})).To(Succeed())
Expect(len(allCMs.Items)).To(BeZero())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| allCMs := &corev1.ConfigMapList{} | |
| Expect(cl.List(ctx, allCMs, &client.ListOptions{Namespace: targetNamespaceName})).To(Succeed()) | |
| Expect(len(allCMs.Items)).To(BeZero()) | |
| allCMs := &corev1.ConfigMapList{} | |
| listCtx, cancel := context.WithTimeout(ctx, 30*time.Second) | |
| defer cancel() | |
| Expect(cl.List(listCtx, allCMs, &client.ListOptions{Namespace: targetNamespaceName})).To(Succeed()) | |
| Expect(len(allCMs.Items)).To(BeZero()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controllers/cloud_config_sync_controller_test.go` around lines 607 - 609,
Update the ConfigMap listing assertion in the test to use a child context with
an explicit timeout instead of the unbounded suite-level context. Create the
timeout context immediately before cl.List, pass it to that call, and ensure the
context is released after the operation.
Source: Path instructions
|
/override ci/prow/e2e-aws-ovn |
|
@neisw: Overrode contexts on behalf of neisw: ci/prow/e2e-aws-ovn, ci/prow/e2e-aws-ovn-upgrade, ci/prow/e2e-azure-ovn-upgrade, ci/prow/level0-clusterinfra-azure-ipi-proxy-tests 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 kubernetes-sigs/prow repository. |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
openshift-ci-robot
added
the
verified
|
@neisw: 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. |
|
@redhat-chai-bot: 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. |
4 participants
This reverts merge commit d9a0c00 (PR #476).
Reason
PR #476 bumped the
k8s.io/cloud-provider-aws/tests/e2emodule, introducing two NLB security group transition tests that consistently time out (0/9 pass rate), causing 5.0 CI payload 5.0.0-0.ci-2026-07-15-075942 to be rejected.Failing tests
[cloud-provider-aws-e2e] loadbalancer NLB should transition from BYO SG to Managed Security Group— context deadline exceeded (loadbalancer.go:625)[cloud-provider-aws-e2e] loadbalancer NLB should transition from Managed SG to BYO Security Group— Timed out after 120.001s (loadbalancer.go:523)The tests expect AWS CCM to reconcile NLB security groups within 120s, but reconciliation does not complete in time. The failure appears related to the upgrade path from 4.22 → 5.0, where installer-created IAM permissions from 4.22 do not include the new permissions required by the 5.0 CCM feature.
References
@neisw requested in Slack thread
Summary by CodeRabbit
Bug Fixes
Tests
Chores