Fix permission denied issue introduced in v4.2.0 #820
Conversation
chelnak
force-pushed
the
GH-819-permission_denied_from_mkdir
branch
3 times, most recently
from
bf47696 to
038c575
Compare
| @@ -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') | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This seems to be true for other command definitions. It's a temporary override but I do see your point.
There was a problem hiding this comment.
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 /).
There was a problem hiding this comment.
Hey @bjvrielink, I think you may have misinterpreted the proposed change here.
This PR removes the directory creation.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This commit reverts the modification of the TMPDIR environment variable from the previous release. This change was made to fix a bug in docker compose where some processes would fail if the noexec bit had been set on /tmp. However this change caused unexpected failures in certain environments.
chelnak
force-pushed
the
GH-819-permission_denied_from_mkdir
branch
from
038c575 to
a34a1e2
Compare
Context
This PR fixes #819
What has changed?
This PR reverts the modification of the TMPDIR environment variable from the previous release. This change was made to fix a bug in docker compose where some processes would fail if the noexec bit had been set on /tmp. However this change caused unexpected failures in certain environments.