Add a permissions option to logging.files for all beats #4428

atoulme · 2017-05-31T16:36:53Z

This is a submission for a fix to issue #4295.

elasticmachine · 2017-05-31T16:36:55Z

Can one of the admins verify this patch?

elasticmachine · 2017-05-31T16:37:02Z

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?

dedemorton · 2017-05-31T16:41:43Z

libbeat/docs/loggingconfig.asciidoc

+===== files.permissions
+
+The permission mask to apply when rotating log files. The default value is 0600. The 
+`permissions` option must be a valid file permissions mask.


Can we add a link to a resource that describes what constitutes a valid file permissions mask? An example is always good, too.

Would a link to this do? https://golang.org/pkg/os/#FileMode
Or a link to https://en.wikipedia.org/wiki/File_system_permissions#Numeric_notation ?

Hmm...I wouldn't link to wikipedia even though it's more descriptive. Linking to the golang doc might actually confuse people.

So maybe instead of a link, you could be a bit more descriptive. Maybe say something like, "The permissions options must be a valid Unix-style file permissions mask expressed in octal notation (for example, 0754).

Or you could link to the following website, which is super useful, but I hesitate to point to it as a resource because I don't know who maintains it or how long it will be around: https://www.filepermissions.com/

You got it. I have amended my commit according to your comments. Any further feedback is very welcome! Thanks!

atoulme · 2017-06-01T06:31:24Z

Current status:

travis build is green now.
the appveyor build fails on one test case. I unpacked the system test output and looked for error reports.
I found this error:
testing.go:610: race detected during execution of test
This seems unrelated to the changes as well.
the documentation was reviewed by @dedemorton. I am waiting on her feedback to add a link to the doc as I'm not familiar with the format used here.
Waiting for further review by @dedemorton and others.
Changed to use os.ModePerm according to @andrewkroh's review
Please let me know if I can help push this any further.

tsg · 2017-06-02T12:16:14Z

@atoulme Thanks for the PR! There's now a failure on Travis that looks related to this change: https://travis-ci.org/elastic/beats/jobs/238535618#L423

tsg · 2017-06-02T13:43:39Z

libbeat/docs/loggingconfig.asciidoc

@@ -27,6 +27,7 @@ logging.files:
  path: /var/log/mybeat
  name: mybeat.log
  keepfiles: 7
+  permissions: 0644


I worry slightly about how YAML interprets this number, is it sure to be octal?

From the point of view of go, it's supposed to be a uint32. So it should be ok?
https://golang.org/pkg/os/#FileMode
Note I added validation to make sure the number is less than 7777, but now I realize we could do better.

The users will be required to start the value with a leading 0 in order for it to be interpreted as octal in YAML.

@andrewkroh I have added some information in the doc to that effect.

andrewkroh · 2017-06-02T16:22:53Z

libbeat/logp/file_rotator.go

@@ -56,6 +57,10 @@ func (rotator *FileRotator) CheckIfConfigSane() error {
 	if *rotator.KeepFiles < 2 || *rotator.KeepFiles >= RotatorMaxFiles {
 		return fmt.Errorf("The number of files to keep should be between 2 and %d", RotatorMaxFiles-1)
 	}
+
+	if rotator.Permissions != nil && (*rotator.Permissions > 7777) {


I think 7777 should be os.ModePerm instead. Only the permission bits should be settable in the mode.

andrewkroh · 2017-06-02T16:24:45Z

libbeat/docs/loggingconfig.asciidoc

@@ -27,6 +27,7 @@ logging.files:
  path: /var/log/mybeat
  name: mybeat.log
  keepfiles: 7
+  permissions: 0644


The users will be required to start the value with a leading 0 in order for it to be interpreted as octal in YAML.

andrewkroh · 2017-06-02T16:25:46Z

libbeat/logp/file_rotator.go

@@ -56,6 +57,10 @@ func (rotator *FileRotator) CheckIfConfigSane() error {
 	if *rotator.KeepFiles < 2 || *rotator.KeepFiles >= RotatorMaxFiles {
 		return fmt.Errorf("The number of files to keep should be between 2 and %d", RotatorMaxFiles-1)
 	}
+
+	if rotator.Permissions != nil && (*rotator.Permissions > 7777) {
+		return fmt.Errorf("The permissions mask %d is invalid", *rotator.Permissions)


Error strings should not be capitalized. https://github.com/golang/go/wiki/CodeReviewComments#error-strings

WDYT about providing a hint here about starting the configured value with a leading 0?

andrewkroh · 2017-06-02T22:02:42Z

libbeat/logp/file_rotator.go

+		return fmt.Errorf("the number of files to keep should be between 2 and %d", RotatorMaxFiles-1)
+	}
+
+	if rotator.Permissions != nil && (*rotator.Permissions > 07777) {


Can you replace the 07777 with the constant os.ModePerm (which is 0777). That should be the max value because users should be setting mode type.

tsg · 2017-06-06T11:52:09Z

jenkins, test it

(cherry picked from commit fecbdf4)

dedemorton reviewed May 31, 2017

View reviewed changes

atoulme force-pushed the issue_4295 branch 4 times, most recently from e96d54e to 726b983 Compare June 1, 2017 03:06

atoulme force-pushed the issue_4295 branch from 726b983 to a2b4ccc Compare June 1, 2017 22:27

tsg added libbeat review labels Jun 2, 2017

tsg reviewed Jun 2, 2017

View reviewed changes

atoulme force-pushed the issue_4295 branch from a2b4ccc to 2ae1d50 Compare June 2, 2017 16:00

andrewkroh reviewed Jun 2, 2017

View reviewed changes

atoulme force-pushed the issue_4295 branch 4 times, most recently from 032c54f to eb29a28 Compare June 2, 2017 19:12

andrewkroh requested changes Jun 2, 2017

View reviewed changes

atoulme force-pushed the issue_4295 branch from eb29a28 to af9c2f0 Compare June 2, 2017 22:57

Add a permissions option to logging.files for all beats

fecf3de

atoulme force-pushed the issue_4295 branch from af9c2f0 to fecf3de Compare June 2, 2017 23:26

andrewkroh approved these changes Jun 6, 2017

View reviewed changes

andrewkroh merged commit fecbdf4 into elastic:master Jun 6, 2017

andrewkroh pushed a commit to andrewkroh/beats that referenced this pull request Oct 13, 2017

Add a permissions option to logging.files for all beats (elastic#4428)

dd882fc

(cherry picked from commit fecbdf4)

andrewkroh mentioned this pull request Oct 13, 2017

Cherry-pick #4428 to 5.6: Add a permissions option to logging.files for all beats #5392

Merged

exekias pushed a commit that referenced this pull request Oct 23, 2017

Add a permissions option to logging.files for all beats (#4428) (#5392)

5bb0386

(cherry picked from commit fecbdf4)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a permissions option to logging.files for all beats #4428

Add a permissions option to logging.files for all beats #4428

atoulme commented May 31, 2017

elasticmachine commented May 31, 2017

elasticmachine commented May 31, 2017

dedemorton May 31, 2017

atoulme May 31, 2017

dedemorton Jun 1, 2017

atoulme Jun 1, 2017

atoulme commented Jun 1, 2017 •

edited

Loading

tsg commented Jun 2, 2017

tsg Jun 2, 2017

atoulme Jun 2, 2017

andrewkroh Jun 2, 2017

atoulme Jun 2, 2017

andrewkroh Jun 2, 2017

andrewkroh Jun 2, 2017

andrewkroh Jun 2, 2017

andrewkroh Jun 2, 2017

andrewkroh Jun 2, 2017

tsg commented Jun 6, 2017

Add a permissions option to logging.files for all beats #4428

Add a permissions option to logging.files for all beats #4428

Conversation

atoulme commented May 31, 2017

elasticmachine commented May 31, 2017

elasticmachine commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atoulme commented Jun 1, 2017 • edited Loading

tsg commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsg commented Jun 6, 2017

atoulme commented Jun 1, 2017 •

edited

Loading