OCPBUGS-77830: skip redirect when validating endpoint accessibility#10360
OCPBUGS-77830: skip redirect when validating endpoint accessibility#10360tthvo wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
@tthvo: This pull request explicitly references no jira issue. 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
|
[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 |
|
/cc @barbacbd |
|
For more information, we are having an internal discussion here: https://redhat-internal.slack.com/archives/C68TNFWA2/p1772550190769159 |
|
/test images |
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-4.22-amd64-nightly-aws-usgov-ipi-private-ep-fips-f7 |
|
@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/f0f1aa00-1817-11f1-9934-a73d804ad55f-0 |
|
/retitle OCPBUGS-77830: skip redirect when validating endpoint accessibility |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-bug
|
@tthvo: This pull request references Jira Issue OCPBUGS-77830, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/jira refresh |
|
@tthvo: This pull request references Jira Issue OCPBUGS-77830, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
The installer uses HTTP HEAD to validate if user-provided service endpoint URLs are reachable. However, in some cases, the request results in a redirect to AWS doc URL, which can causes install failure in disconnected environment. The users should not be required to open access to AWS docs to install. For example: $ curl --head https://sts.ap-southeast-1.amazonaws.com HTTP/1.1 302 Found Location: https://aws.amazon.com/iam
WalkthroughModified HTTP endpoint accessibility checks in AWS validation by replacing a direct Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/asset/installconfig/aws/validation.go (1)
867-877: LGTM! Redirect handling is correctly implemented.The use of
CheckRedirectreturninghttp.ErrUseLastResponseis the idiomatic Go pattern to stop following redirects while still receiving the response. Thedefer resp.Body.Close()is correctly placed after the error check, avoiding nil pointer issues.One optional enhancement: consider adding a timeout to the client to prevent indefinite hangs in slow network environments:
,
💡 Optional: Add timeout for improved reliability
client := &http.Client{ + Timeout: 30 * time.Second, CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse // Don't follow redirects }, }This would require adding
"time"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/validation.go` around lines 867 - 877, Add a timeout to the HTTP client used when checking the service endpoint to avoid indefinite hangs: set client := &http.Client{Timeout: time.Second * <reasonable_duration>, CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse }} and import "time"; update the instantiation around the existing client variable and its CheckRedirect handler in the same block so Head(endpointURL) uses the timed client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/asset/installconfig/aws/validation.go`:
- Around line 867-877: Add a timeout to the HTTP client used when checking the
service endpoint to avoid indefinite hangs: set client := &http.Client{Timeout:
time.Second * <reasonable_duration>, CheckRedirect: func(req *http.Request, via
[]*http.Request) error { return http.ErrUseLastResponse }} and import "time";
update the instantiation around the existing client variable and its
CheckRedirect handler in the same block so Head(endpointURL) uses the timed
client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a4a0338-e02b-45b3-827f-027826d33a74
📒 Files selected for processing (1)
pkg/asset/installconfig/aws/validation.go
|
For local testing, I set the environment variable platform:
aws:
region: ap-southeast-1
serviceEndpoints:
- name: sts
url: https://sts.ap-southeast-1.amazonaws.comGODEBUG=http2debug=2 ./openshift-install create manifestsFrom the debug logs, there should never be a connection to |
|
@tthvo: The following tests failed, say
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. |
2 participants
The installer uses HTTP HEAD to validate if user-provided service endpoint URLs are reachable. However, in some cases, the request results in a redirect to AWS doc URL, which can causes install failure in disconnected environment. The users should not be required to open access to AWS docs to install.
For example:
Summary by CodeRabbit