Make send_file aware of user option
#277
Conversation
|
I think good, but I need to think other corner case for subclass of Please wait a few days. 🙏 |
|
Thank you for your reviewing!
No problem. I'm itamae beginner, so there's a possibility that I overlooked somethings. |
|
Dear all, Regarding to the test, I think unit-test on Itamae::Resource::File is enough to cover this kind of issue so that I write the test below as spec/unit/lib/itamae/resource/file_spec.rb which works well as I expect(it fails on v1.10.2, but it passes on this PR). Unfortunately, this requires 'SUDO' 😢. When PASSWD is set in sudoers config, sudo prompt appears every test execution... so that I didn't PR and just show the test here to discuss any better approach (although I set NOPASSWD in sudoers config so that I do not feel any stress on this, actually). require 'itamae'
describe Itamae::Resource::File do
describe 'when user attribute is different from execution user' do
file_resource_path = '/tmp/itamae_test_a'
file_resource_content = 'a-content'
subject(:file_resource) do
runner = Itamae::Runner.run([], :local, {log_level: 'info'})
recipe = Itamae::Recipe.new(runner, '/tmp/dummy_recipe.rb')
described_class.new(recipe, 'a') do
user 'nobody'
path file_resource_path
content file_resource_content
end
end
describe '@resource_name' do
it { expect(subject.resource_name).to eq 'a' }
it { expect(subject.attributes.content).to eq file_resource_content }
end
describe '#run' do
FileUtils.rm_f(file_resource_path)
it 'create' do
subject.run
expect(File).to exist(file_resource_path)
expect(File.read(file_resource_path)).to eq file_resource_content
end
end
end
end
Agreed, but Itamae::Resource::File is a base class of RemoteFile and Template so that it could be good enough to workaround on this |
|
@fuminori-ido Thank you for your comment, and the test case!
Personaly it is not acceptable to me (NOT to itamae-kitchen team). It is stressful, and I think it is storange that test case requires sudo permission. If a test requires sudo permission, I will stop the test case by security reason. I tried to execute the integration test on docker with |
|
@fuminori-ido Thank you for test code. But this depends on the internal implementation of Itamae.
I think too. But I doesn't write and run test code yet. I believe production code that passes the test code than my imagination. |
|
I'm working on the integration test with |
pocke
force-pushed
the
user-option-file
branch
from
72bb9a1
to
0b6c17a
Compare
|
I added an integration test for ordinary user. It tests the following.
I think it is better if the NoteI found a regression, but personally I think it is not a bug. file '/tmp/a' do
user 'itamae'
source "hello.txt"
endlog:
It is a permission problem. itamae/lib/itamae/resource/file.rb Lines 172 to 192 in 0b6c17a However, personally I think it is acceptable. Because we can avoid this error with options. The following code works well. # Use owner and group options instead of user option
file '/tmp/a' do
owner 'itamae'
group 'itamae'
source "hello.txt"
endI think specifying However, maybe there are some recipes that will be broken by this pull request unfortunately. What do you think about this regression? If you do not accept this regression, I'll rethink the solution to avoid this problem. |
|
Sorry, I found another regression. I'll describe and fix the regression soon. |
|
I've fixed the metioned both regressions. And add specs for the regressions. ProblemThe latter regression, which it haven't been described by me yet, is similar the former one. But it happens only if an ordinary user executes itamae. file '/tmp/a' do
user 'another_ordinary_user'
owner 'another_ordinary_user'
group 'another_ordinary_user'
endIf an ordinary user other than We can avoid the regression with specifing And I guess this regression will be an actual problem. For example, I can easily imagine the following case.
The case does not work if the regression exists. SolutionSee 2f426bc. I choose shell pipe to receive content of the tempfile between the users. |
| ###### | ||
|
|
||
| # Docker backend raises an error with `user` option, so it tests only on `itamae local`. | ||
| # After fix this error, please move this code and the spec to `default.rb`. |
There was a problem hiding this comment.
I tried to add the following code to recipes/default.rb, but it does not work.
Because docker backend raises an error with this code.
Docker backend's send_file sends a file with the root user, and currently, it is not configurable, unfortunately.
I guess we can move the code to recipes/default.rb by fixing docker backend, but this pull request does not focus this problem. I guess this problem already exists before this pull request because I do not touch docker backend in this pull request.
|
I'll release later |
|
Thank you for your reviewing! |
Problem
useroption does not work withfile,remote_fileand so on resources.Probably other resouces have similar problems, but I've not confirm them yet. I describe the similar problems in the Cause section.
background
I'd like to execute itamae process with an ordinary user, but I'd like to put files that are owned by the root user.
When itamae needs the root user, I want to write
user 'root'explicitly, and I want to usesudocommand only if itamae needs the root user.For instance, I'd like to use Itamae to provision a desktop computer for development. In this case, I'd like to put some files that is owned by an ordinary user (e.g.
~/.vimrc), and owned by the root user (e.g./etc/hosts) too.Unfortunately I cannot execute Itamae on the background.
I investigated this problem, and I describe this problem in this pull request.
Related issues
Fileresource fails when specifyuserattribute different from execution user. #214The first three issues mention the same problem, so I think we can close these issues by this pull request.
The last issue is a pull request for this problem, but I think the patch is not appropriate because the same reason as the sorah's comment. So we need another solution for this problem.
Reproduce
test.rbSorah says "Why not using owner attribute instead of user?" in this comment ( #221 (comment) ), but the suggestion raises another error in this case.
test.rbItamae changes the owner of the file to specified user by
owneroption, which is the root user, but executed user is an ordinary user so the user does not have the permission to chown.I think I need the following code for expected behaviour. But it also raises the same error as the first code.
Cause
In short, because
send_fileis not aware ofuseroption.The error is occurred in
lib/itamae/backend.rb:109insend_filemethod.itamae/lib/itamae/backend.rb
Line 109 in 452e105
It calls
Specinfra::Backend::XXX#send_file. TheXXXisExec,SSH,Docker, and so on. When it isExec, which Itamae::BackendisLocal,send_filemethod callsFileUtils.cp` method.https://github.com/mizzy/specinfra/blob/d3c00a0076c7ef86d28e22ac545f69db7c031d70/lib/specinfra/backend/exec.rb#L24-L26
FileUtils.cpis a Ruby method, not an external command. So it is not aware ofuseroption. It's simply called in an itamae process, so itamae does not change user.By the way, I wrote " I describe the similar problems in the Cause section." in the first section of this pull request.
Itamae has similar code, so I guess there are similar problems.
For example,
Backend#send_directorymedhod also callsFileUtilsmethod.itamae/lib/itamae/backend.rb
Lines 112 to 121 in 425b9f7
https://github.com/mizzy/specinfra/blob/d3c00a0076c7ef86d28e22ac545f69db7c031d70/lib/specinfra/backend/exec.rb#L28-L30
I also find
receive_filemethod that is monkeypatched by Itamae.itamae/lib/itamae/ext/specinfra.rb
Lines 11 to 19 in 425b9f7
Maybe we need to fix them, but this pull request does not change them. It only focuses
send_filemethod.Because I do not use other than
send_filemethod, so I do not have motivation to fix these method.Solution
Use an external command for
send_filemethod. See the diff!Update 2019-01-16: This description is incomplete. Please read this comment also. #277 (comment)
Note
This patch does not add any test cases. I'm not sure how to test this patch. I think we should add an integration test case for this change, but the integration test works only with docker. So I cannot add an integration test case for
itamae localcommand.If you have a good solution for test, please tell me it. I'd like to write test for this problem.
I guess we can execute the integration test in the docker image, but with
itamae localcommand also. For examle,docker run ubuntu:trusty -v $(pwd):/itamae /itamae/bin/itamae local ...(this command is simplified).note: I've confirmed the patch works well in my environment.