Add warning for directory permission to in_tail.rb #4865
Conversation
lib/fluent/plugin/in_tail.rb
Outdated
| @@ -385,6 +385,13 @@ def expand_paths | |||
| $log.warn "expand_paths: stat() for #{path} failed with #{e.class.name}. Skip file." | |||
| end | |||
| } | |||
| if check_permission | |||
| (paths - excluded).map { |path| File.dirname(path) }.uniq.each { |dir| | |||
There was a problem hiding this comment.
It might be better to add select { |path| !FileTest.exist?(path) } before map.
https://github.com/fluent/fluentd/actions/runs/13865592763/job/38858881719?pr=4865#step:6:13416 👀 |
@moatom Thanks for this PR! This test failure is a known test issue, so we don't need to care about it in this PR. |
I see. In some cases, If the directory does not have the |
|
@moatom
About A, it would be possible that this feature supports only static paths that do not use wildcards, etc. About B, I feel the essential difficulty of this issue. I don't think it necessarily needs to be strictly confirmed. For example, I want to avoid false positives, such as generating warning logs for paths that just do not yet exist. |
To avoid false positives, we can consider the following direction for example.
fluentd/lib/fluent/plugin/in_tail.rb Lines 372 to 374 in e3dcebc Log message: (This supports only static paths that do not use wildcards, etc.) |
|
@daipom Thanks for the review and I'm sorry for my late response🙇
Oops. I thought the required permission was
The following part seems to expand the glob, so I believe this issue shouldn't occur (if my understanding is correct):
I am polishing my idea as follows: Experienced users are the ones who primarily use complex paths, and they are likely to manage to notice issues on their own. Therefore, in this PR, I would like to focus on adding a warning for the following simple case (if glob expansion does not occur, I want to ignore such cases): where
For this specific case, the ideal conditions for generating warnings can be summarized in the following table:
We are concerned about false positives for
I agree with this direction if the above approach is not effective! |
I might have missed cases where logs are emitted only at the debug level: |
moatom
force-pushed
the
master
branch
2 times, most recently
from
9b1c108 to
5de5a2e
Compare
Signed-off-by: Tomoaki Kobayashi <tomoaki.kobayashi.t3@alumni.tohoku.ac.jp>
lib/fluent/plugin/in_tail.rb
Outdated
| (paths - excluded).select { |path| | ||
| if check_permission && !FileTest.readable?(path) | ||
| is_bad_permission = begin | ||
| current_path = File.expand_path(path) | ||
| loop do | ||
| current_path = File.dirname(current_path) | ||
| break true unless FileTest.executable?(current_path) | ||
| break false if current_path == "/" | ||
| end | ||
| end | ||
| if is_bad_permission | ||
| $log.warn "Skip #{path} because a directory in the path lacks execute permission." | ||
| end | ||
| end | ||
| FileTest.exist?(path) | ||
| }.each { |path| |
There was a problem hiding this comment.
This should work fine unless I've overlooked something.
|
@moatom
If there are not enough permissions for the directory, |
This would be no problem!
Yes. False positives are my concern.
If it is difficult to resolve this false positive, let's go in this direction. I explain specific examples of the false positive below. CaseFor example, consider the following case. $ sudo ls -lR /tmp/2
/tmp/2:
合計 12
drwxrwx--- 2 root root 4096 3月 28 13:47 d---
drwxrwx--x 2 root root 4096 3月 28 13:47 d--x
drwxrwxr-x 2 root root 4096 3月 28 13:46 dr-x
/tmp/2/d---:
合計 8
-rw-r----- 1 root root 6 3月 28 13:47 file_---.log
-rw-r--r-- 1 root root 5 3月 28 13:47 file_r--.log
/tmp/2/d--x:
合計 8
-rw-r----- 1 root root 6 3月 28 13:47 file_---.log
-rw-r--r-- 1 root root 5 3月 28 13:47 file_r--.log
/tmp/2/dr-x:
合計 8
-rw-rw---- 1 root root 6 3月 28 13:46 file_---.log
-rw-rw-r-- 1 root root 5 3月 28 13:37 file_r--.logLet in_tail collect these 6 patterns and non-existent path (totally 7 paths). <source>
@type tail
tag test
path /tmp/2/dr-x/file_r--.log, /tmp/2/dr-x/file_---.log, /tmp/2/d--x/file_r--.log, /tmp/2/d--x/file_---.log, /tmp/2/d---/file_r--.log, /tmp/2/d---/file_---.log, /tmp/2/not_exist/test.log
read_from_head true
refresh_interval 5s
<parse>
@type none
</parse>
</source>
<match test.**>
@type stdout
</match>Result: master
Result: PR
|
Thanks for explaining the case! I'm thinking it might work if I switch the traversal direction in the code above to top-down instead. |
Signed-off-by: Tomoaki Kobayashi <tomoaki.kobayashi.t3@alumni.tohoku.ac.jp>
|
@daipom P.S. I'm not sure, but it might be a false negative when even Result: Latest PR<source>
@type tail
tag test
path /tmp/2/dr-x/file_r--.log, /tmp/2/dr-x/file_---.log, /tmp/2/d--x/file_r--.log, /tmp/2/d--x/file_---.log, /tmp/2/d---/file_r--.log, /tmp/2/d---/file_---.log, /tmp/2/not_exist/test.log
pos_file /tmp/fluentd-test.pos
read_from_head true
refresh_interval 5s
<parse>
@type none
</parse>
</source>
<match test.**>
@type stdout
</match>
$ sudo ls -lR /tmp/2
/tmp/2:
total 12
drwxrwx--- 2 root root 4096 Apr 11 14:09 d---
drwxrwx--x 2 root root 4096 Apr 11 14:09 d--x
drwxrwxr-x 2 root root 4096 Apr 11 14:09 dr-x
/tmp/2/d---:
total 8
-rw-r----- 1 root root 5 Apr 11 14:11 file_---.log
-rw-r--r-- 1 root root 5 Apr 11 14:11 file_r--.log
/tmp/2/d--x:
total 8
-rw-r----- 1 root root 5 Apr 11 14:11 file_---.log
-rw-r--r-- 1 root root 5 Apr 11 14:11 file_r--.log
/tmp/2/dr-x:
total 8
-rw-rw---- 1 root root 5 Apr 11 14:11 file_---.log
-rw-rw-r-- 1 root root 5 Apr 11 14:11 file_r--.log
$ bundle exec fluentd -c in_tail.conf
025-04-11 15:44:22 +0000 [warn]: #0 Skip /tmp/2/d---/file_r--.log because '/tmp/2/d---' lacks execute permission.
2025-04-11 15:44:22 +0000 [warn]: #0 Skip /tmp/2/d---/file_---.log because '/tmp/2/d---' lacks execute permission.
2025-04-11 15:44:22 +0000 [info]: #0 following tail of /tmp/2/dr-x/file_r--.log
2025-04-11 15:44:22.441945749 +0000 test: {"message":"test"}
2025-04-11 15:44:22 +0000 [info]: #0 following tail of /tmp/2/dr-x/file_---.log
2025-04-11 15:44:22 +0000 [warn]: #0 Permission denied @ rb_sysopen - /tmp/2/dr-x/file_---.log
2025-04-11 15:44:22 +0000 [info]: #0 following tail of /tmp/2/d--x/file_r--.log
2025-04-11 15:44:22.442540302 +0000 test: {"message":"test"}
2025-04-11 15:44:22 +0000 [info]: #0 following tail of /tmp/2/d--x/file_---.log
2025-04-11 15:44:22 +0000 [warn]: #0 Permission denied @ rb_sysopen - /tmp/2/d--x/file_---.log |
|
@moatom Thanks! Excellent! This direction looks good to me!
Sqush would be good!
Do you mean the case where we just specify |
| @@ -253,7 +253,7 @@ def start | |||
| FileUtils.mkdir_p(pos_file_dir, mode: @dir_perm) unless Dir.exist?(pos_file_dir) | |||
| @pf_file = File.open(@pos_file, File::RDWR|File::CREAT|File::BINARY, @file_perm) | |||
| @pf_file.sync = true | |||
| @pf = PositionFile.load(@pf_file, @follow_inodes, expand_paths, logger: log) | |||
| @pf = PositionFile.load(@pf_file, @follow_inodes, expand_paths(true), logger: log) | |||
There was a problem hiding this comment.
In this implementation, this check is done only when using pos_file.
This should be done independent of pos_file.
It would be good to do this check in configure by making the new method such as check_dir_permission.
Since the first half of the process is the same as expand_paths, it would be good to cut out that part into another method (expand_paths_raw?) and share it.
def configure(conf)
...
check_dir_permission
...
end
def expand_paths_raw
date = Fluent::EventTime.now
...
paths - excluded
end
def expand_paths
hash = {}
expand_paths_raw.select { |path|
File.exist?(path)
}.each { |path|
...
end
def check_dir_permission
expand_paths_raw.select ...
end| @@ -370,7 +370,17 @@ def expand_paths | |||
| # filter out non existing files, so in case pattern is without '*' we don't do unnecessary work | |||
| hash = {} | |||
| (paths - excluded).select { |path| | |||
| FileTest.exist?(path) | |||
| if check_permission && !File.readable?(path) | |||
There was a problem hiding this comment.
if check_permission && !File.readable?(path)
Wouldn't File.exist? be more appropriate?
If some parent directories don't have x permission, then that path should appear to be nonexistent.
| @@ -370,7 +370,17 @@ def expand_paths | |||
| # filter out non existing files, so in case pattern is without '*' we don't do unnecessary work | |||
| hash = {} | |||
| (paths - excluded).select { |path| | |||
| FileTest.exist?(path) | |||
| if check_permission && !File.readable?(path) | |||
| inaccessible_dir = Pathname.new(File.expand_path(path)) | |||
There was a problem hiding this comment.
inaccessible_dir = Pathname.new(File.expand_path(path))
Do we need File.expand_path?
| .to_a | ||
| .reverse |
There was a problem hiding this comment.
.to_a
.reverse
We can use .reverse_each instead.
.ascend
.reverse_each
.find| if File.exist?("#{@tmp_dir}/noaccess") | ||
| FileUtils.rm_rf("#{@tmp_dir}/noaccess") | ||
| end | ||
| end unless Fluent.windows? |
There was a problem hiding this comment.
end unless Fluent.windows?
If there is no special reason, please use omit "..." if Fluent.windows?
| }) | ||
| d = create_driver(config, false) | ||
| assert_nothing_raised do | ||
| d.run(shutdown: false) {} |
There was a problem hiding this comment.
d.run(shutdown: false) {}
There is no need to use shutdown: false.
(Maybe there are some old codes left...)
We can access the logs by Driver#logs (even after the shutdown).
Normally, we should not use $log.out.logs.
If we move the check logic to configure, maybe we don't need even Driver#run.
Example:
fluentd/test/plugin/test_in_tail.rb
Lines 3393 to 3404 in 510b891
| FileUtils.chmod(0755, "#{@tmp_dir}/noaccess") | ||
| if File.exist?("#{@tmp_dir}/noaccess") | ||
| FileUtils.rm_rf("#{@tmp_dir}/noaccess") | ||
| end |
There was a problem hiding this comment.
It would be unnecessary to clean files because it would be done in teardown automatically.
fluentd/test/plugin/test_in_tail.rb
Lines 26 to 31 in 510b891
Which issue(s) this PR fixes:
Fixes #What this PR does / why we need it:
This PR provides a warning for typical configuration mistakes that prevent log transfer from starting.
When using the tail type as a source, it is common for directories with inappropriate partitions to be mistakenly included in the source path.
In my opinion, it would be preferable to issue a warning in such cases to help users avoid these mistakes.
Steps to Reproduce:
Docs Changes:
No documentation changes required.
Release Note:
in_tail: add warning for directory permission.