Filebeat: Makes registry_file_permission configurable #6455

dolftax · 2018-02-23T11:26:37Z

This supports use-cases where the registry file is read
by some other external service/component.

Signed-off-by: Jaipradeesh jaipradeesh@gmail.com

elasticmachine · 2018-02-23T11:26:38Z

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

houndci-bot · 2018-02-23T11:26:48Z

filebeat/registrar/registrar.go

@@ -38,9 +39,10 @@ var (
 	registryWrites = monitoring.NewInt(nil, "registrar.writes")
 )

-func New(registryFile string, flushTimeout time.Duration, out successLogger) (*Registrar, error) {
+func New(registryFile string, registryFilePermission os.FileMode, flushTimeout time.Duration, out successLogger) (*Registrar, error) {


exported function New should have comment or be unexported

dolftax · 2018-02-23T11:30:56Z

FYI, yet to update docs, configuration reference, etc,..

ruflin · 2018-02-27T05:44:34Z

filebeat/registrar/registrar.go

@@ -85,6 +87,12 @@ func (r *Registrar) Init() error {
 		return fmt.Errorf("Registry file path is not a regular file: %s", r.registryFile)
 	}

+	// Update registry file permissions
+	err = os.Chmod(r.registryFile, r.registryFilePermission)


Not sure if we really should update the file permission here. I would rather suggest to change in on line 262 so we already write it with the correct permission.

dolftax · 2018-02-27T09:06:51Z

Grep for registry_file in filebeat dir returns the following. make update isn't updating all the below files. Should it be done manually?

./filebeat.reference.yml:557:#filebeat.registry_file: ${path.data}/registry
./filebeat.reference.yml:560:#filebeat.registry_file_permission: 0600
./_meta/common.reference.p2.yml:262:#filebeat.registry_file: ${path.data}/registry
./_meta/common.reference.p2.yml:265:#filebeat.registry_file_permission: 0600
./_meta/beat.reference.yml:557:#filebeat.registry_file: ${path.data}/registry
./_meta/beat.reference.yml:560:#filebeat.registry_file_permission: 0600
./docs/filebeat-general-options.asciidoc:20:==== `registry_file`
./docs/filebeat-general-options.asciidoc:27:filebeat.registry_file: registry
./docs/filebeat-general-options.asciidoc:42:options, such as `registry_file`, are ignored.
./docs/faq.asciidoc:56:Filebeat keeps the state of each file and persists the state to disk in the `registry_file`. The file state is used to continue file reading at a previous position when Filebeat is restarted. If a large number of new files are produced every day, the registry file might grow to be too large. To reduce the size of the registry file, there are two configuration options available: <<clean-removed,`clean_removed`>> and <<clean-inactive,`clean_inactive`>>.
./tests/load/filebeat.yml:17:  registry_file: registry
./tests/load/filebeat.yml:18:  registry_file_permission: 0600
./tests/system/test_registrar.py:141:    def test_custom_registry_file_location(self):
./tests/system/test_registrar.py:918:        registry_file = self.working_dir + "/registry"
./tests/system/test_registrar.py:924:        registry_file_path = self.working_dir + "/registry"
./tests/system/test_registrar.py:925:        with open(registry_file_path, 'w') as registry_file:
./tests/system/test_registrar.py:927:            registry_file.write("Hello World")
./tests/system/config/filebeat_modules.yml.j2:1:filebeat.registry_file: {{ beat.working_dir + '/' }}{{ registryFile|default("registry")}}
./tests/system/config/filebeat.yml.j2:94:filebeat.registry_file: {{ beat.working_dir + '/' }}{{ registryFile|default("registry")}}
./tests/system/config/filebeat_inputs.yml.j2:8:filebeat.registry_file: {{ beat.working_dir + '/' }}{{ registryFile|default("registry")}}
./config/config.go:26:	RegistryFile           string               `config:"registry_file"`
./config/config.go:27:	RegistryFilePermission os.FileMode          `config:"registry_file_permission"`

@ruflin

ruflin · 2018-03-02T01:27:03Z

@jaipradeesh Not sure what part you want to update in all these files? I think it's important to the permission settings in the reference file and the docs.

dolftax · 2018-03-02T04:57:34Z

No problem @ruflin. I've updated the docs. Please do a review :-) . I've also added some verification steps below.

Permission config test

➜  filebeat git:(filebeat-configurable-registry-file-perm) cat filebeat.yml | grep "registry_file_permission"

➜  filebeat git:(filebeat-configurable-registry-file-perm) ls -la data | grep "registry"                     
-rw-------.  1 dolftax dolftax    3 Mar  2 09:00 registry

➜  filebeat git:(filebeat-configurable-registry-file-perm) ./filebeat -c filebeat.yml
^C%                      

➜ filebeat git:(filebeat-configurable-registry-file-perm) ls -la data | grep "registry"                     
-rw-------.  1 dolftax dolftax    3 Mar  2 09:27 registry

➜  filebeat git:(filebeat-configurable-registry-file-perm) cat filebeat.yml | grep "registry_file_permission"
filebeat.registry_file_permission: 0644

➜  filebeat git:(filebeat-configurable-registry-file-perm) ./filebeat -c filebeat.yml                        
^C%                                 

➜  filebeat git:(filebeat-configurable-registry-file-perm) ls -la data | grep "registry"                     
-rw-r--r--.  1 dolftax dolftax    3 Mar  2 09:27 registry

ruflin · 2018-03-06T05:23:00Z

filebeat/tests/load/filebeat.yml

@@ -15,7 +15,7 @@ filebeat:
  spool_size: 4096
  idle_timeout: 5s
  registry_file: registry
-


This just triggered an idea on my end. It would be really great to have a system test that checks if the permission of the registry file is actually changed in case this is set differently. Can you add one?

Will add tests for 0644, 0600 and 0664. IMO, that should suffice.

Great to hear. Please ping us when you pushed a new commit. This will also show if it causes any problems on windows.

ruflin · 2018-03-06T05:23:31Z

@urso @andrewkroh Do you see any issue adding this feature?

andrewkroh · 2018-03-07T05:59:45Z

One issue is that we use a plural (permissions) in naming the option for other areas. I think the name should be changed.

wk8 · 2018-03-12T22:30:09Z

Could this please be merged and shipped with the next release? Much needed here too :) Thanks!

dolftax · 2018-03-13T09:43:17Z

Test that filebeat registry permission is set as per configuration ... ok (0.3507s)
Test that filebeat default registry permission is set ... ok (0.3478s)
Test that filebeat registry permission is updated along with configuration ... ok (1.5567s)

ruflin · 2018-03-13T09:51:09Z

jenkins, test it

ruflin · 2018-03-13T09:52:49Z

filebeat/tests/system/test_registrar.py

+           max_timeout=1)
+
+       # Wait a momemt to make sure registry is completely written
+       time.sleep(1)


Normally this can become flaky on slow tests machines. But we can still fix it if the build starts to break.

ruflin · 2018-03-13T09:54:40Z

@jaipradeesh Thanks, LGTM. Can you revert the Makefile change, just to be sure. Afterwards we should be good to go in case Jenkins agrees :-)

dolftax · 2018-03-13T10:25:58Z

Makefile, reverted.
System test log - https://dpaste.de/qGTb/raw

ruflin · 2018-03-13T10:35:33Z

jenkins, test it

dolftax · 2018-03-13T11:37:18Z

Weirdly, update config test is passing locally (Fedora 27) - https://dpaste.de/qGTb/#L127 but failing in travis (both OS X and Ubuntu Trusty) - https://travis-ci.org/elastic/beats/jobs/352789249#L352

Checking why. Any pointers appreciated.

ruflin · 2018-03-13T12:21:48Z

Could it be a timing issue related to the 1s? Travis is pretty slow.

ruflin · 2018-03-13T12:22:23Z

@jaipradeesh I assume you deleted the comment about pep8 and discovered make fmt.

dolftax · 2018-03-13T12:50:17Z

@ruflin Used https://pypi.python.org/pypi/flake8 // make fmt helps. Yeah, deleted the comment as I didn't want to bother you.

ruflin · 2018-03-14T09:48:52Z

filebeat/tests/system/test_registrar.py

@@ -1293,7 +1440,8 @@ def test_ignore_older_state_clean_inactive(self):

        # Make sure state is written
        self.wait_until(
-            lambda: self.log_contains("Registry file updated. 0 states written."),
+            lambda: self.log_contains(


Seems like you used a different pep setting then what we have in make fmt for the line length. It's not a big deal but in general would be nice to only have the changes related to the PR in.

Noted. From next PR.

Modified PEP8 config as per beats' requirements.

ruflin · 2018-03-14T09:49:39Z

jenkins, test it

ruflin · 2018-03-14T09:50:08Z

PR LGTM. Now waiting on Jenkins to go green.

ruflin · 2018-03-14T09:51:22Z

filebeat/tests/system/test_registrar.py

+            max_timeout=1)
+
+        # Wait a momemt to make sure registry is completely written
+        time.sleep(5)


It would be nice if you could do a follow up PR that does not use time.Sleep but for example watches the registry file for the file permission changes. Otherwise this increases our CI time by at least 5 seconds. If we have this in many tests this is not nice.

I should be able to work on another filebeat bug over the weekend. Will push it along with that.

Great, but please make it 2 different PR's to sepearate concerns.

dolftax · 2018-03-14T10:17:08Z

Build fails on windows. Looks like Windows ACL have to be modified. Two ways to it.

This option isn't supported on Windows
Implement something like https://github.com/hectane/go-acl

ruflin · 2018-03-14T11:19:40Z

For the first version I suggest we not support it on Windows but states this very clearly in the docs. Can you update the docs and tests accordingly, please also add a note to the reference config file. There are some other system tests which are skipped on windows, you can copy the checks from there.

@andrewkroh WDYT?

andrewkroh · 2018-03-14T15:43:13Z

Currently we don't do any Windows ACL management anywhere in the project. So I would just say this option isn't supported on Windows.

This supports use-cases where the registry file is read by some other external service/component. Signed-off-by: Jaipradeesh <jaipradeesh@gmail.com>

Signed-off-by: Jaipradeesh <jaipradeesh@gmail.com>

ruflin · 2018-03-15T12:37:18Z

jenkins, test it

ruflin · 2018-03-15T13:38:53Z

@jaipradeesh Merged. Thanks a lot for following through with this.

houndci-bot reviewed Feb 23, 2018

View reviewed changes

dolftax mentioned this pull request Feb 23, 2018

[Request] Any way to set the permissions for the registry file. #6293

Closed

dolftax force-pushed the filebeat-configurable-registry-file-perm branch from f5122c6 to 4662bc2 Compare February 23, 2018 13:13

ruflin added in progress Pull request is currently in progress. review Filebeat Filebeat labels Feb 26, 2018

ruflin requested a review from andrewkroh February 26, 2018 02:39

ruflin reviewed Feb 27, 2018

View reviewed changes

dolftax force-pushed the filebeat-configurable-registry-file-perm branch from 4662bc2 to 797748a Compare February 27, 2018 08:43

dolftax force-pushed the filebeat-configurable-registry-file-perm branch 2 times, most recently from 325cf09 to 8d97d0f Compare February 28, 2018 17:14

dolftax force-pushed the filebeat-configurable-registry-file-perm branch 2 times, most recently from f628649 to 7e1e969 Compare March 2, 2018 04:57

dolftax force-pushed the filebeat-configurable-registry-file-perm branch 2 times, most recently from 3ea5697 to 74e8e0e Compare March 2, 2018 05:43

ruflin reviewed Mar 6, 2018

View reviewed changes

dolftax force-pushed the filebeat-configurable-registry-file-perm branch from 74e8e0e to 2cc740e Compare March 9, 2018 06:27

urso approved these changes Mar 12, 2018

View reviewed changes

dolftax force-pushed the filebeat-configurable-registry-file-perm branch 4 times, most recently from 66bd66a to 168ac88 Compare March 13, 2018 07:50

ruflin reviewed Mar 13, 2018

View reviewed changes

dolftax force-pushed the filebeat-configurable-registry-file-perm branch from 9ad0fc8 to cf7895c Compare March 13, 2018 10:20

dolftax force-pushed the filebeat-configurable-registry-file-perm branch from cf7895c to 63a3ee8 Compare March 13, 2018 11:19

dolftax force-pushed the filebeat-configurable-registry-file-perm branch 2 times, most recently from 57d4474 to 492a5d3 Compare March 13, 2018 15:10

ruflin reviewed Mar 14, 2018

View reviewed changes

dolftax force-pushed the filebeat-configurable-registry-file-perm branch from 492a5d3 to 81106de Compare March 14, 2018 18:08

dolftax added 2 commits March 14, 2018 23:43

Filebeat: Makes registry_file_permission configurable

ad600a6

This supports use-cases where the registry file is read by some other external service/component. Signed-off-by: Jaipradeesh <jaipradeesh@gmail.com>

Filebeat: Adds system test to check registry_file_permission

fd14e3a

Signed-off-by: Jaipradeesh <jaipradeesh@gmail.com>

dolftax force-pushed the filebeat-configurable-registry-file-perm branch from 81106de to fd14e3a Compare March 14, 2018 18:14

ruflin merged commit 0de34ab into elastic:master Mar 15, 2018

dolftax deleted the filebeat-configurable-registry-file-perm branch March 15, 2018 13:55

kalilou mentioned this pull request Apr 28, 2020

Datadog Filebeat integration: Registry denied for datadog to read DataDog/integrations-extras#588

Closed

Filebeat: Makes registry_file_permission configurable #6455

Filebeat: Makes registry_file_permission configurable #6455

Conversation

dolftax commented Feb 23, 2018 • edited Loading

elasticmachine commented Feb 23, 2018

Choose a reason for hiding this comment

dolftax commented Feb 23, 2018

Choose a reason for hiding this comment

dolftax commented Feb 27, 2018 • edited Loading

ruflin commented Mar 2, 2018

dolftax commented Mar 2, 2018

Permission config test

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

ruflin commented Mar 6, 2018

andrewkroh commented Mar 7, 2018

wk8 commented Mar 12, 2018

dolftax commented Mar 13, 2018

ruflin commented Mar 13, 2018

Choose a reason for hiding this comment

ruflin commented Mar 13, 2018

dolftax commented Mar 13, 2018

ruflin commented Mar 13, 2018

dolftax commented Mar 13, 2018 • edited Loading

ruflin commented Mar 13, 2018

ruflin commented Mar 13, 2018

dolftax commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Mar 14, 2018

ruflin commented Mar 14, 2018

Choose a reason for hiding this comment

dolftax Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dolftax commented Mar 14, 2018 • edited Loading

ruflin commented Mar 14, 2018

andrewkroh commented Mar 14, 2018

ruflin commented Mar 15, 2018

ruflin commented Mar 15, 2018

dolftax commented Feb 23, 2018 •

edited

Loading

dolftax commented Feb 27, 2018 •

edited

Loading

dolftax commented Mar 13, 2018 •

edited

Loading

dolftax Mar 14, 2018 •

edited

Loading

dolftax commented Mar 14, 2018 •

edited

Loading