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 permissions configuration for file output #4638

Closed
wants to merge 101 commits into from
Closed

Conversation

clery
Copy link

@clery clery commented Jul 10, 2017

Add the ability to change file permissions on file output for Metricbeat.

@karmi
Copy link

karmi commented Jul 10, 2017

Hi @plassa-b, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically on build-eu-00. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@clery
Copy link
Author

clery commented Jul 10, 2017

Hi @karmi, I added the email as you asked. Tell me if you need anything else !

@clery
Copy link
Author

clery commented Jul 13, 2017

Hey there, any news on this ? Should I make any change ? :)

@karmi
Copy link

karmi commented Jul 17, 2017

Thanks for adding the e-mail, @plassa-b, the CLA check is now green! I imagine the Beats team will get back to you shortly.

@@ -958,6 +958,7 @@ output.file:
filename: {beatname_lc}
#rotate_every_kb: 10000
#number_of_files: 7
#permissions: 0666
Copy link
Member

Choose a reason for hiding this comment

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

The commented out value should reflect the default value.

@@ -953,6 +953,9 @@ output.elasticsearch:
# default is 7 files.
#number_of_files: 7

# Permissions to use for file creation. The default is 0600.
#permissions: 0666
Copy link
Member

Choose a reason for hiding this comment

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

This file is generated based on libbeat/_meta/config.reference.yml. After updating the file in libbeat you should run make update from the beats dir to re-generate the config files.

@clery
Copy link
Author

clery commented Jul 19, 2017

Hi @andrewkroh, I just made the changes you asked for. I also merged with a more recent version of beats (i realised the config.full.yml was renamed as config.reference.yml, so I figured it would be simpler for you guys)

Also, I realised that I left a segfault when no value was given for permissions in the config file (when logging the value, I was dereferencing the value without checking it wasn't nil), so I fixed that and added the default properly where needed.

Hope it does the trick. Feel free to ask me if you need anything else.

@clery
Copy link
Author

clery commented Aug 16, 2017

Hey guys, any news about this PR ?

@ejsears
Copy link

ejsears commented Aug 21, 2017

Just checking into this as we have no other way to import data. Thanks!

@andrewkroh
Copy link
Member

LGTM. Could you add this configuration option to the documentation for the file output. https://github.com/elastic/beats/blob/master/libbeat/docs/outputconfig.asciidoc#configuration-options-4

@clery
Copy link
Author

clery commented Sep 4, 2017

Sorry for the delay.

I'm not sure I understand, if I'm correct, the documentation was already added (https://github.com/plassa-b/beats/blob/master/libbeat/docs/outputconfig.asciidoc#file-output)

Here's the diff https://github.com/elastic/beats/pull/4638/files#diff-f1a89e6c3a1ce6f296f47ad83b989ff7R961

@andrewkroh
Copy link
Member

jenkins, test it

@andrewkroh
Copy link
Member

I'm not sure I understand, if I'm correct, the documentation was already added

Sorry, I missed that in the diff.

Can you please check the tests. It looks like this change is causing a panic in a test. https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-windows/1451/beat=libbeat,label=windows/testReport/junit/(root)/logp/Test_Rotator/

exekias and others added 7 commits September 7, 2017 21:49
In some cases pod annotations are neede after the pod is deleted, for
instance when filebeat is reading the log behind the container.

This change makes sure we keep metadata after a pod is gone. By storing
access times we ensure that it's available as long as it's being used
6.0.0-beta1 snapshot does not exist as its not a snapshot anymore. Also master should be tested againts ES 7.0.0-alpha1.
* Use `beat.name` instead of `beat.hostname` in visualizations. See elastic#5276 for
  the motivation
* Add a "tip" widget that tells user how they can select another host.
* Automatically set a search pattern of the form `beat.name: "HOSTNAME"` where
  HOSTNAME is the Beat name (hostname) that uploads the dashboards.

The way the last point works is that I saved the dashbaord using this filter:
`beat.name:"CHANGEME_HOSTNAME"`. The kibana loader code does a string replacement
and replaces `CHANGEME_HOSTNAME` with the actual hostname. The disadvantage of this
approach (vs doing JSON parsing) is that we must remember to always save that
dashboard with the `CHANGEME_HOSTNAME` wildcard in place. I have added a unit test
to the system module that checks for that, so it should be relatively hard for
us to forget.

Closes elastic#5276.
@andrewkroh
Copy link
Member

Sorry for such a late response. I just realized that this never merged and it's a great enhancement.

So I'm wondering wether the test should initialize the Permissions (to avoid calling CheckIfConfigSane) or if they should be defaultly initialized somewhere else.

For simplicity I would just fix the test to initialize Permissions.

Thanks!

andrewkroh and others added 7 commits October 9, 2017 12:13
This PR adds support for enabling TLS renegotiation. The setting is `ssl.renegotiation` and the options are `never` (default), `once`, and `freely`. This exposes the three options from https://golang.org/pkg/crypto/tls/#RenegotiationSupport.

Fixes elastic#4386
The command was initially designed for the apm-server to modify the index pattern. But now a nicer solution was found and this is not needed anymore.
Already the compose timeout is set to 300. Now also setting the retries in the healtcheck to 300.
This were broken on case-sensitive file systems, because the name of the files
didn't match the name of the IDs from the dashboards.

Fixes elastic#5277.
jenkins_setup in common.bash will also be used for the generator tests.
* Move queue setting to TOC + address some review
* Remove flush_interval and spooler from outputconfig
* remove 'slot'
* add [float] tags
* Update queueconfig.asciidoc
@codecov
Copy link

codecov bot commented Oct 10, 2017

Codecov Report

Merging #4638 into master will increase coverage by 12.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4638       +/-   ##
===========================================
+ Coverage    53.9%   65.97%   +12.06%     
===========================================
  Files         555      410      -145     
  Lines       34594    30589     -4005     
===========================================
+ Hits        18647    20180     +1533     
+ Misses      14250     8746     -5504     
+ Partials     1697     1663       -34
Impacted Files Coverage Δ
metricbeat/helper/server/tcp/tcp.go 44.26% <0%> (-14.76%) ⬇️
filebeat/fileset/factory.go 65.07% <0%> (-7.94%) ⬇️
auditbeat/module/audit/kernel/audit_linux.go 43.04% <0%> (-6.1%) ⬇️
metricbeat/helper/server/http/http.go 52.11% <0%> (-4.23%) ⬇️
filebeat/config/config.go 56.66% <0%> (-2.99%) ⬇️
filebeat/registrar/registrar.go 75.59% <0%> (-2.26%) ⬇️
filebeat/crawler/crawler.go 78.31% <0%> (-1%) ⬇️
filebeat/prospector/prospector.go 89.28% <0%> (-0.25%) ⬇️
metricbeat/module/elasticsearch/node_stats/data.go 100% <0%> (ø) ⬆️
metricbeat/mb/module/runner.go 100% <0%> (ø) ⬆️
... and 397 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1060ff...4f13672. Read the comment docs.

Remove default golang version as not needed anymore. All repos / branches have now a .go-version file.
@andrewkroh
Copy link
Member

jenkins, test it

* Add Azure VM Cloud Metadata Support

* Update CHANGELOG.asciidoc

* Update processors-using.asciidoc
@andrewkroh
Copy link
Member

Could you please rebase the PR on master. We added some new scripts in master that are used by Jenkins for testing.

@clery
Copy link
Author

clery commented Oct 11, 2017

So I just actually did a git rebase from master, but giving that the PR is filled with commits that aren't from me, I'm wondering if that was actually what you wanted

@ruflin
Copy link
Contributor

ruflin commented Oct 12, 2017

@plassa-b You probably have to fetch the most recent version of based and rebase on that.

andrewkroh pushed a commit that referenced this pull request Oct 12, 2017
This PR adds a `output.file.permissions` configuration option to
control the file mode used when created the output file.
@andrewkroh
Copy link
Member

Merged in 5c51448

I used the manual instructions to rebase and squash the PR. Thanks @plassa-b

@andrewkroh andrewkroh closed this Oct 12, 2017
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.