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.

Sign up for GitHub

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

Fix permission denied issue introduced in v4.2.0 #820

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Fix permission denied issue introduced in v4.2.0 #820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b686a3
(GH-819) Revert TMPDIR changes
chelnak Apr 12, 2022
a34a1e2
(GH-819) Fix acceptance and unit tests
chelnak Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions lib/puppet/provider/docker_compose/ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
end

has_command(:docker_compose, command(:dockercompose)) do
Dir.mkdir('/tmp_docker') unless Dir.exist?('/tmp_docker')
ENV.store('TMPDIR', '/tmp_docker')
environment(HOME: '/root')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overriding $HOME unconditionally is probably a bad idea. This is still a draft so probably needed for some tests, but flag it so that we don't forget about it by mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be true for other command definitions. It's a temporary override but I do see your point.

Copy link

Choose a reason for hiding this comment

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

Please please please do not create new directories in /.
Please pick some other place (for example under /usr/local/share, /opt, /var/cache, or anywhere else but /).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @bjvrielink, I think you may have misinterpreted the proposed change here.

This PR removes the directory creation.

Copy link

Choose a reason for hiding this comment

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

Yeah, now I noticed. I've spent a little too much time right now trying to figure out what was/is broken in the current release.

Copy link
Contributor

@canihavethisone canihavethisone Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I did some runtime tests with the new changes, and line 27 of lib/puppet/provider/docker_compose/ruby.rb was failing as it wasn't getting the new tmpdir param.

I got it to work by changing line 27 of lib/puppet/provider/docker_compose/ruby.rb to

compose_output = YAML.safe_load(exec_dockercompose(args))

however, it is not idempotent as the TMPDIR var gets set every time the resource runs. I can't find a way to silence it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot!

Interesting regarding the idempotency. I wouldn't have expected it to have an impact.

It's also highly possible that my suggested implementation is not the right fit here.

Maybe we shouldn't be overridden TMPDIR at all given that it it isn't actually a docker setting.

Could this boil down to a simple documentation update where we advise that impacted users may want to override TMPDIR with another puppet resource?

@smortex @kenyon what are your thoughts?

Copy link
Contributor

@canihavethisone canihavethisone Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I tried setting the TMPDIR env var previously using another puppet resource. However without a reboot the docker_compose resource wasn't seeing/respecting it. I like your draft approach as it takes care of it close to the compose resource, it just needs to occur silently if the param is declared - interestingly the existing (breaking) approach did that, however my testing with a hybrid method wasn't reading the resource[:tmpdir] if set earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed so the resource hash is not available inside the has_command block so we can't use the environment method inside it.

That's why I ended up wrapping the command exec. Maybe Env.store isn't the right way to go about it 🧐.

The environment method did not seem to be available outside of that scope either (at least from inside a pry session).. could be wrong about that though.

I'm not by a computer right now so can't do any testing until later on.

Copy link
Contributor

@canihavethisone canihavethisone Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

An alternative approach may be to precede the docker-compose command with 'TMPDIR=resource[:tmpdir]' so that it occurs as part of the command if the tmpdir param is declared. That way ruby isn't doing it, the shell is. Just a thought.

Setting the env var within the resource (and it subsequently being ephemeral) also avoids setting it globally on the system.

end

def exists?
Expand Down
6 changes: 0 additions & 6 deletions spec/classes/init_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,12 +196,6 @@
}
else
it {
# Stub /tmp_docker dir to prevent shelling out during spec test
allow(Dir).to receive(:exist?).and_wrap_original do |original_method, a|
original_method.call(a)
end
allow(Dir).to receive(:exist?).with('/tmp_docker').and_return(true)

is_expected.to contain_class('docker::repos').that_comes_before('Class[docker::install]')
is_expected.to contain_class('docker::install').that_comes_before('Class[docker::config]')
is_expected.to contain_class('docker::config').that_comes_before('Class[docker::service]')
Expand Down
9 changes: 0 additions & 9 deletions spec/defines/registry_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,6 @@

require 'spec_helper'

# Stub /tmp_docker dir to prevent shelling out during spec test
class Dir
class << self
def exist?(var)
return true if var == '/tmp_docker'
end
end
end

tests = {
'with ensure => absent' => {
'ensure' => 'absent',
Expand Down
9 changes: 0 additions & 9 deletions spec/unit/lib/puppet/type/docker_compose_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,6 @@

require 'spec_helper'

# Stub /tmp_docker dir to prevent shelling out during spec test
class Dir
class << self
def exist?(var)
return true if var == '/tmp_docker'
end
end
end

compose = Puppet::Type.type(:docker_compose)

describe compose do
Expand Down