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 HTTP output plugin #2488

Merged
merged 8 commits into from
Jul 30, 2019
Merged

Add HTTP output plugin #2488

merged 8 commits into from
Jul 30, 2019

Conversation

repeatedly
Copy link
Member

Signed-off-by: Masahiro Nakagawa repeatedly@gmail.com

Which issue(s) this PR fixes:
None

What this PR does / why we need it:
Add HTTP output plugin for v1.7. I got several requests from different users in kubecon. Users have the endpoint to accept logs via HTTP/HTTPs. So generic HTTP output plugin is useful for typical usecases.

Discussion point:

  • Support in_http format. fluentd has forward protocol so out_http -> in_http usecase is very rare. I think no need for now
  • Support [{}, {},..., {}] like json/msgpack. Current implementation uses ndjson. I'm not sure one large array payload is useful or not.

Docs Changes:
Need to add article to output section. I will write it after ready to merge.

Release Note:
Add http output plugin

@repeatedly repeatedly added the feature request *Deprecated Label* Use enhancement label in general label Jul 9, 2019
@repeatedly repeatedly self-assigned this Jul 9, 2019
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
@repeatedly repeatedly marked this pull request as ready for review July 24, 2019 23:14
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

There is a failure testcase for Ruby 2.1 and 2.2 tasks in Travis CI.
Otherwise, looks reasonable for me at most patch.

test/plugin/test_out_http.rb Outdated Show resolved Hide resolved
end
if @tls_private_key_path
raise Fluent::ConfigError, "tls_private_key_path is wrong: #{@tls_private_key_path}" unless File.file?(@tls_private_key_path)
opt[:key] = OpenSSL::PKey::read(File.read(@tls_private_key_path), @tls_private_key_passphrase)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

end

def parse_endpoint(chunk)
endpoint = extract_placeholders(@endpoint, chunk)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

assert_not_empty result.headers
end

def test_write_with_single_value_format
Copy link
Contributor

Choose a reason for hiding this comment

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

test_write_with_single_value_format is failing in Ruby 2.1 and 2.2 tasks.
Could you check them?
https://travis-ci.org/fluent/fluentd/jobs/563321723#L1909

lib/fluent/plugin/out_http.rb Show resolved Hide resolved
test/plugin/test_out_http.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/out_http.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/out_http.rb Show resolved Hide resolved
end

def setup_http_option
use_ssl = URI.parse(@endpoint).scheme == 'https'
Copy link
Member

Choose a reason for hiding this comment

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

if a user sets URI which includes a placeholder (e.g. http://127.0.0.1/${path}) to endpoint directive, does this line raise URI::InvalidURIError?

require 'net/http'
require 'uri'
 
URI.parse('http://127.0.0.1/${path}') #=> raise 'bad URI(is not URI?): "http://127.0.0.1/${path}" (URI::InvalidURIError)'

end

def server_port
19880
Copy link
Member

Choose a reason for hiding this comment

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

unused_port is better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure which is the better. This is fixed value, not dynamic value like helper's unused_port.
So this naming is also ok for me.

test/plugin/test_out_http.rb Outdated Show resolved Hide resolved
test/plugin/test_out_http.rb Show resolved Hide resolved
d.instance_shutdown
end

# basic auth test includes "error_response_as_unrecoverable true" case
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is needed because I don't add explicit standalone test for true version

Copy link
Member

Choose a reason for hiding this comment

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

I think this is needed because I don't add explicit standalone test for true version

Yes. but

basic auth test includes "error_response_as_unrecoverable true" case

what does this comment mean?
This test is not for basic auth, right? and basic auth test looks not having error_response_as_unrecoverable true case. why do we need this comment?

lib/fluent/plugin/out_http.rb Show resolved Hide resolved
@cosmo0920 cosmo0920 self-requested a review July 25, 2019 04:23
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

Looks reasonable for me.
BTW, this plugin seems to be the successor of https://github.com/fluent-plugins-nursery/fluent-plugin-out-http.

Should we announce the predecessor plugin deprecation in its repository?

Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Copy link
Member

@ganmacs ganmacs left a comment

Choose a reason for hiding this comment

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

Rest of my comments are nits. LGTM

@repeatedly
Copy link
Member Author

repeatedly commented Jul 26, 2019

out_http doesn't cover all cases of fluent-plugin-out-http so the successor is not the correct.
For example, this plugin doesn't support non-buffer mode and form format.
But for buffering case, I think this plugin is better than fluent-plugin-out-http so users can choose the plugin for their usecases.

@cosmo0920
Copy link
Contributor

out_http doesn't cover all cases of fluent-plugin-out-http so the successor is not the correct.
For example, this plugin doesn't support non-buffer mode and form format.
But for buffering case, I think this plugin is better than fluent-plugin-out-http so users can choose the plugin for their usecases.

I see. Thanks for the information. 😃

Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
@repeatedly repeatedly merged commit 3d96b15 into master Jul 30, 2019
@repeatedly repeatedly deleted the out_http branch July 30, 2019 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request *Deprecated Label* Use enhancement label in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants