-
-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add --max-redirects
option
#248
Conversation
That's precisely the issue, you got 200 when you should have gotten a 301. So redirects were followed by default (which is still the existing case, no change there) and that was hidden from the clients using it. In my use-case, I'm following the redirects and extracting the redirect path in the client connecting to mubeng. Moreover, before this PR, for redirects paths with >10 redirects it would return an error rather than the last redirect (see #230) |
Unless you provide an instance of the host like that to reproduce the issue, it would be better. The URL of the issue you're referring to is just fine by me. |
Yes, by default there is no change in behavior (otherwise it's a breaking change). But if you set max-redirects to 0, you let the client app control the redirect. Default settings:
by setting max-redirects to 0
Both cases made exactly 3 calls (301, 302, 200). Another example, you might not want to follow the redirects at all in the client :
max-redirects=0
|
I get it. But I need a URL that has >10 redirects in order to reproduce the issue and see what behavior I get by default. |
for http://www.soulmatereading.com (note, it's http and not https): in my PR
Head
same happens with http://www.bunch.ca/about (http not https) |
I didn't notice that, lol. Now it makes sense to me. |
Unresolved questions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lesgo! Thank you, @yudelevi.
--max-redirects
option
IMPORTANT: Please do not create a Pull Request without creating an issue first!
(Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of the pull request).
Summary
I've run into a few issues with redirects (#247, #230) that were caused by the fact that the HTTP library in go silently follows 10 redirects and fails without returning the last call.
Clients using the proxy would fail without any insight and without receiving a proper failure code. Moreover, in some use cases, the 3xx code and destination is a result by itself.
Proposed of changes
This PR adds a --max-retries option that defaults to the standard 10 redirects the HTTP library follows.
In addition to that, the last result is returned in a case of 3xx redirect.
How has this been tested?
Proof:
fixing error above will break compatibility with <1.20
Testing below for 301 and following redirects via curl with -L flag:
curl -k -v -L -x "http://127.0.0.1:9000" https://httpbin.org/status/301
andcurl -k -v -x "http://127.0.0.1:9000" https://httpbin.org/status/301
Closing issues
Fixes #247 , #230
Checklist: