Skip to content
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

add lax_redirect setting #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsvd
Copy link
Contributor

@jsvd jsvd commented Oct 28, 2021

The httpclient library used is very conservative when it comes to status code handling. According to RFC7231, only HEAD and GET requests should be automatically redirected. Others like POST should required user interaction (e.g. a browser dialog window).

However it is possible to customize redirect strategy to override this behavior and there's even a more Lax strategy..aptly named LaxRedirectStrategy. This strategy will only redirect HEAD/GET/DELETE/POST, but not PUT.

This PR adds an option to enable LaxRedirectStrategy. Logstash plugins that rely on this mixin will have the ability to set lax_redirects => true when using POST/DELETE methods.

To test this, instruct a local logstash to use this a local copy of this repo by editing the logstash's Gemfile:

gem "logstash-mixin-http_client", :path => "/tmp/logstash-plugins/logstash-mixin-http_client"

With an endpoint that has a 307 redirect:

require 'sinatra'

post "/foo/bar" do
  redirect "/test", 307
end

post "/test" do
  [200, {}, {}]
end

Then use a pipeline that does a POST:

bin/logstash -e "output { http { url => 'http://127.0.0.1:4567/foo/bar' lax_redirects => true http_method => post } }"

The sinatra app will show either 1 or 2 requests from Logstash depending on lax_redirects being false or true, respectively.

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This appears to work.
  2. I'm a little bummed that this mixin is so light on tests, and would prefer automated validation.
  3. Should we also support for the Expect: 100-continue handshake users can avoid sending the whole payload twice when they are redirected (and dealing with well-behaved recipients)?
    3b. is our http-input well-behaved when it receives the Expect: 100-continue header in a request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants