-
Notifications
You must be signed in to change notification settings - Fork 147
add support for force_path_style option #141
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
Conversation
lib/logstash/inputs/s3.rb
Outdated
| s3 = Aws::S3::Resource.new(aws_options_hash) | ||
| options = aws_options_hash || {} | ||
| if @force_path_style == true | ||
| options.merge!(:force_path_style => @force_path_style) |
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.
is this an AWS::S3 flag ? I can't find it documented .... do you have a link ?
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.
The option can be found here: https://docs.aws.amazon.com/sdkforruby/api/Aws/S3/Client.html
Btw, the constructor validates that the settings exist, so force_path_style has to be valid (we'd get an exception otherwise).
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.
@jsvd - thanks, sometimes i think i am blind.
There are alot of good settings in there... should we open it up the configuration to general purpose hash of options to pass in (kinda like we do with jdbc and sequel ) ?
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.
It's a good question, we could change that but would be a breaking change, maybe we can open an issue to do it until logstash 7.0 GAs
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.
is it a breaking change ?
config :additional_settings, :validate => :hash, :default => {}
...
options.merge!(@additional_settings)
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.
I'm thinking that probably we'd like to remove some of the current config settings as they'd be done through the additional_settings hash.
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.
we could just swap the merge to be result = @additional_settings.merge(options) calling it additional implies that other things in there take precedence.
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.
ok. I have no strong feelings here, will redo as generic additional_settings
|
LGTM |
|
thanks! @jakelandis |
|
Just a quick question, how would you connect to an custom endpoint. Which variable should the url go in? Also can SSL options like certificates be passed in the |
|
@SalmaanP Custom endpoints are supported at the top level since v3.3.0, see doc: Remember that you must include the protocol, not just the domain name. See the full list of S3 endpoints. Note also that this does not guarantee you'll be able to connect to "compatible" endpoints from S3 competitors. This plugin uses the Ruby AWS SDK, which doesn't guarantee this use case. This setting can be useful for IPV6 (example above) or for govcloud, however. |
|
@webmat I see endpoint is there in the documentation but I don't see it defined anywhere here In my tests with a custom on-prem s3 bucket, using the latest version of the plugin and endpoint at the top level did not work, so did putting it in additional options. However, using it by changing the plugin as done here did work. I was also able to add SSL options here in the same way and it started to work. |
|
This functionality is provided by the logstash-mixin-aws, in version 4.3.0 https://github.com/logstash-plugins/logstash-mixin-aws/blob/master/CHANGELOG.md#430 Make sure you have upgraded both s3 and this mixing. To see plugins and versions installed you can do |
|
@jsvd I see, thanks for the information! |
this is useful when using custom endpoints in s3
replaces #139
a similar PR should be made against https://github.com/logstash-plugins/logstash-output-s3