OCPBUGS-99755: OTE: fix AWS endpoint resolution for non-standard partitions - #494
OCPBUGS-99755: OTE: fix AWS endpoint resolution for non-standard partitions#494tthvo wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@tthvo: This pull request references Jira Issue OCPBUGS-99755, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAWS load balancer and EC2 test clients no longer override SDK endpoint resolution with hardcoded regional hostnames. ChangesAWS endpoint selection
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@tthvo: This pull request references Jira Issue OCPBUGS-99755, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
/cc @mtulio @mfbonfigli |
|
@tthvo: This pull request references Jira Issue OCPBUGS-99755, which is valid. 3 validation(s) were run on this bug
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. |
|
/testwith openshift/installer/main/e2e-aws-eusc-techpreview openshift/installer#10672 |
|
/payload-job periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance |
|
@tthvo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/840867a0-879b-11f1-8fa2-ca425d91c641-0 |
tthvo
changed the title
|
@tthvo: This pull request references Jira Issue OCPBUGS-99755, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openshift-tests/ccm-aws-tests/e2e/aws/helper.go (1)
200-220: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd table-driven tests for every partition suffix.
This helper is shared by both ELBv2 and EC2 clients; test
cn-,eusc-, each ISO prefix,us-gov-, and the default case to prevent silent endpoint regressions.🤖 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 200 - 220, Add table-driven unit tests for dnsSuffixForRegion covering cn-, eusc-, us-iso-, us-isob-, eu-isoe-, us-isof-, us-gov-, and an unmatched region. Assert each input returns its expected partition suffix, including the default amazonaws.com case.Source: MCP tools
🤖 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.
Nitpick comments:
In `@openshift-tests/ccm-aws-tests/e2e/aws/helper.go`:
- Around line 200-220: Add table-driven unit tests for dnsSuffixForRegion
covering cn-, eusc-, us-iso-, us-isob-, eu-isoe-, us-isof-, us-gov-, and an
unmatched region. Assert each input returns its expected partition suffix,
including the default amazonaws.com case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 002f9648-ade4-4030-9da3-1446579091d8
📒 Files selected for processing (1)
openshift-tests/ccm-aws-tests/e2e/aws/helper.go
|
/payload-job periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance |
|
@tthvo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6e13f750-87a2-11f1-8221-50acf096a3f1-0 |
|
/testwith openshift/installer/main/e2e-aws-eusc-techpreview openshift/installer#10672 |
de8f63c to
f48822d
Compare
|
/payload-job periodic-ci-openshift-hypershift-release-5.0-e2e-aws-ovn-conformance-ccm-techpreview |
|
@tthvo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/84ce8b10-8a3e-11f1-960a-7f6a1726f82c-0 |
|
/payload-job periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance-techpreview |
|
@tthvo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/fb874df0-8a3e-11f1-9674-dba2d98d43cc-0 |
|
/testwith openshift/installer/main/e2e-aws-eusc-techpreview openshift/installer#10672 |
|
/testwith openshift/installer/main/e2e-aws-eusc-techpreview openshift/installer#10717 Need to use an installer without merge conflicts 😅 |
|
/test e2e-aws-ovn-upgrade |
We can rely on the default endpoint resolver of the SDK to construct the correct partition-aware public regional endpoint.
|
/payload-job periodic-ci-openshift-hypershift-release-5.0-e2e-aws-ovn-conformance-ccm-techpreview |
|
@tthvo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/72623570-8a58-11f1-9ae3-326786f070e2-0 |
|
/testwith openshift/installer/main/e2e-aws-eusc-techpreview openshift/installer#10717 |
3 participants
Description
The
BaseEndpointfor ELBv2 and EC2 clients was hardcoded to.amazonaws.com, which is incorrect for non-standard AWS partitions like EUSC (.amazonaws.eu). This caused API calls to fail on EUSC clusters because the requests were sent to the wrong domain.This PR removes the
BaseEndpointoverride to let the AWS SDK automatically resolve to an partition-aware endpoint.See also #464.
Note: Originally, I thought it was due to an outdated AWS SDK deps, but the endpoint resolution is fine (without base endpoint override) thanks to the compatible core config module. Thus, there is no longer any need to upgrade go deps.
Summary by CodeRabbit