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.

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

Permission denied from mkdir /tmp_docker in v4.2.0 #819

Closed
kenyon opened this issue Apr 11, 2022 · 23 comments · Fixed by #820
Closed

Permission denied from mkdir /tmp_docker in v4.2.0 #819

kenyon opened this issue Apr 11, 2022 · 23 comments · Fixed by #820
Assignees

Comments

@kenyon
Copy link
Contributor

kenyon commented Apr 11, 2022

With v4.2.0 of this module, I get Permission denied - /tmp_docker from a puppet agent run.

Caused by #805: https://github.com/puppetlabs/puppetlabs-docker/pull/805/files#r847521695

@r-catania
Copy link

We are seeing the same behavior with 4.2.0

error during compilation: Evaluation Error: Error while evaluating a Virtual Query, Could not autoload puppet/type/docker_compose: Could not autoload puppet/provider/docker_compose/ruby: Permission denied @ dir_s_mkdir - /tmp_docker

@canihavethisone
Copy link
Contributor

I have commented on the pull request. Requested investigation.

@canihavethisone
Copy link
Contributor

canihavethisone commented Apr 12, 2022

I've done some testing with this and confirmed that the pull request has an unexpected outcome in some environments on the first puppet run only. Observations are:

  1. /tmp_docker dir creation neither attempts or errors if docker classes are not included in ENC
  2. /tmp_docker dir creation error only occurs on first puppet run when any docker classes are included in ENC, does not error on subsequent runs. This could be a similar issue to docker_volume not created on first puppet run #782, Error: Could not find a suitable provider for docker_compose #742 and docker_network is not applied correctly on first run #703 where providers are not realized on the first puppet run?
  3. /tmp_docker dir creation only actually occurs if the docker_compose is declared (this is the intended outcome)
  4. observation 2 above occurs regardless of where the directory for TMPDIR env var is created

As per comments in the pull request, the intention of the dir and env var are to overcome known issue as also reported at docker/compose#8041.

I will investigate a way to overcome observation 2 above, and have also requested maintainer input on this. In the interim, the error does not appear to occur beyond the first puppet run.

@chelnak
Copy link
Contributor

chelnak commented Apr 12, 2022

Hey all! Thanks for raising this. It's certainly strange that this has popped up. It's not something that we observed initially.

We will spin up a lab today and attempt to reproduce the issue.

Thanks!!

@chelnak
Copy link
Contributor

chelnak commented Apr 12, 2022

OK I'm looking at this now 👀

Now I'm in the codebase, I'm starting to wonder if we should create a new property on the type. e.g tmpdir.

We can make our intention clear in the description and say that this can be used to override the TMPDIR setting for docker compose and should only be used on systems that are impacted by the referenced bug.

We should also probably avoid creating new directories in this situation and instead recommend something like /var/tmp as an alternative to /tmp.. It might also be worth noting that unlike /tmp, the contents of /var/tmp will persist between reboots.

With the above implementation it is the responsibility of the module consumer to ensure that thier custom tmpdir exists before docker compose attempts to use it.

What do you think?

@chelnak
Copy link
Contributor

chelnak commented Apr 12, 2022

@canihavethisone @kenyon

Please could you share an example manifest that I can use to reproduce the error? I want to make sure that we are doing things in the same way.

@canihavethisone
Copy link
Contributor

Sounds like a good approach. I'll check a CIS hardened system in a few mins with regard to the exec bit on the suggested /var/tmp. I also observed that the temporary files created by docker-compose are cleaned up after it runs.

@canihavethisone
Copy link
Contributor

canihavethisone commented Apr 12, 2022

i tested with a slim wrapper such as:

class test (
  $composefiles = lookup( 'test::composefiles',    Optional[Hash],  'deep', {} ),
  $compose_defs = lookup( 'test::compose_defs',    Optional[Hash],  'deep', {} ),
){

  # Include the main docker class
  class { 'docker':
    log_driver => 'journald',
  }

  # Include classes as listed in hiera
  hiera_include('classes')

  # Create docker-compose files
  $composefiles.each  |$key, $value| {
    file { $key:
      * => $value
    }
  }

  # Process docker-compose resources
  $compose_defs.each  |$key, $value| {
    docker_compose { $key:
      * => $value
    }
  }

  # Ensure that iptables is installed before docker
  Package <| title == 'iptables' |> -> Package <| title =='docker' |>

  # Ensure that images are built, then containers run, then docker compose
  Docker::Image <| |> -> Docker::Run <| |> -> Docker_compose <| |> 

}

with hiera such as:

---
classes:
  - docker::compose

test::composefiles:
  /tmp/composefile1.yaml:
    source: 'http://192.168.1.60/composefile1.yaml'
  /tmp/composefile2.yaml:
    source: 'http://192.168.1.60/composefile2.yaml'

test::compose_defs:
  test:
    compose_files: ['/tmp/composefile1.yaml']
    subscribe:     'File[/tmp/composefile1.yaml]'
    ensure:        present
    scale:
      db:        1
      wordpress: 1      
  test2:
    compose_files: ['/tmp/composefile2.yaml']
    subscribe:     'File[/tmp/composefile2.yaml]'
    ensure:        present
    scale:
      my_test:    1
      my_test_2:  2
      redis:      3
      postgres:   4

The content of the compose files is very basic
composefile1.yaml:

version: "3.9"
    
services:
  db:
    image: mysql:5.7
    volumes:
      - db_data:/var/lib/mysql
    restart: always
    environment:
      MYSQL_ROOT_PASSWORD: somewordpress
      MYSQL_DATABASE: wordpress
      MYSQL_USER: wordpress
      MYSQL_PASSWORD: wordpress
    
  wordpress:
    depends_on:
      - db
    image: wordpress:latest
    volumes:
      - wordpress_data:/var/www/html
    ports:
      - "8000:80"
    restart: always
    environment:
      WORDPRESS_DB_HOST: db
      WORDPRESS_DB_USER: wordpress
      WORDPRESS_DB_PASSWORD: wordpress
      WORDPRESS_DB_NAME: wordpress
volumes:
  db_data: {}
  wordpress_data: {}

composefile2.yaml:

version: '3'
services:
  my_test:
    build:
      context: .
      dockerfile: ./test.dockerfile
    command: >
      sh -c "echo hello from my_test_$$(hostname) > /etc/motd &&
             while true; do echo hello from my_test_$$(hostname); sleep 5; done"
    restart: unless-stopped
  my_test_2:
    image: my_test:35
    command: >
      sh -c "echo hello from my_test_2_$$(hostname) > /etc/motd &&
             while true; do echo hello from my_test_2_$$(hostname); sleep 5; done"
    restart: unless-stopped
  redis:
    image: redis:latest
    ports:
      - "6379-6400:639"
  postgres:
    image: postgres:latest
    restart: always
    entrypoint: ["/bin/sh","-c"]
    command: 
    - | 
       echo hello from postgres_$$(hostname) > /etc/motd
       while true; do echo hello from postgres_$$(hostname); sleep 5; done
    ports:
      - "6500-6510:5432"

@canihavethisone
Copy link
Contributor

on CIS hardened systems, /var/tmp is also noexec. Perhaps another folder such as /usr/local/share/tmp_docker?

@canihavethisone
Copy link
Contributor

canihavethisone commented Apr 12, 2022

if users prefer to just allow exec on /tmp and it doesn't conflict with puppet managed hardening, the following works but is yuck

  if $compose_defs {
    exec { 'allow exec on /tmp':
      command => 'mount /tmp -o remount,exec',
      unless  => 'mount | grep "on /tmp" | grep -v noexec',
      path    => ['/usr/bin', 'bin']
    }
  }

@chelnak
Copy link
Contributor

chelnak commented Apr 12, 2022

I feel like we should allow the user to decide on the directory that is used to override the /tmp directory. We should just give the option to pass in a directory and not prescribe a certain behaviour.

Maybe in the documentation we can indicate that the directory used should not have the noexec bit.

@maltewhiite
Copy link

maltewhiite commented Apr 12, 2022

Experiencing the same thing. Happens on every puppet run.

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Resource Statement, Evaluation Error: Error while evaluating a Virtual Query, Could not autoload puppet/type/docker_compose: Could not autoload puppet/provider/docker_compose/ruby: Permission denied - /tmp_docker (file: /etc/puppetlabs/code/environments/test/modules/docker/manifests/init.pp, line: 663, column: 8) on node 

What version of this puppet-module should I roll back to?

EDIT: rolling back to 4.1.2 from september seems to be working.

@canihavethisone
Copy link
Contributor

canihavethisone commented Apr 12, 2022

v4.1.2 doesn't have this issue. Alternatively it doesn't occur on the second puppet run with the latest v4.2.0, and the /tmp_docker folder is only created if docker_compose resources are declared

@maltewhiite
Copy link

maltewhiite commented Apr 12, 2022

I only declare "class { 'docker':" and docker::registry and docker::image. No docker::compose at all. Still experienced the error. And it happens on every run, not only the first run.

  $docker_packages = ['docker-ce', 'docker-ce-cli', 'containerd.io', ]
  package { $docker_packages:
    ensure => 'latest',
    before => Class['docker'],
  }

  class { 'docker':
    manage_package  => false,
    selinux_enabled => true,
    version         => 'latest',
  }

  docker::registry { $docker_registry:
    username => $docker_registry_username,
    password => $docker_registry_password,
    require  => Class['docker'],
  }

  docker::image { 'gitlab-runner':
    ensure    => 'latest',
    image     => 'gitlab/gitlab-runner',
    image_tag => 'latest',
    require   => Docker::Registry[$docker_registry],
  }

@canihavethisone
Copy link
Contributor

canihavethisone commented Apr 12, 2022

yep, as noted in the observations above the error appears on first puppet run regardless, however not on subsequent runs and the actual dir creation only occurs if docker_compose is referenced. I think it is linked to resources not being realized correctly on the first run, but regardless, @chelnak has proposed a more suitable approach moving ahead.

chelnak added a commit that referenced this issue Apr 12, 2022
This commit introduces a new tmpdir property for the docker_compose
resource that allows the module consumer to decide on the value of
TMPDIR (The location that docker compose uses for temp files).

With the new implementation:

If the tmpdir property is not set, the docker compose will use it's
default value of /tmp.

If the tmpdir property is set, the docker
compose will use the value of the tmpdir property.

It is the responsibility of the consumer to ensure that the directory
passed to the tmpdir is available and writable by the user that is
running puppet.
@chelnak
Copy link
Contributor

chelnak commented Apr 12, 2022

Hey team! I've got a draft PR up with a suggested fix: #820

It's early days but feedback is welcome.

@chelnak chelnak self-assigned this Apr 12, 2022
@chelnak chelnak added the bugfix label Apr 12, 2022
chelnak added a commit that referenced this issue Apr 12, 2022
This commit introduces a new tmpdir property for the docker_compose
resource that allows the module consumer to decide on the value of
TMPDIR (The location that docker compose uses for temp files).

With the new implementation:

If the tmpdir property is not set, the docker compose will use it's
default value of /tmp.

If the tmpdir property is set, the docker
compose will use the value of the tmpdir property.

It is the responsibility of the consumer to ensure that the directory
passed to the tmpdir is available and writable by the user that is
running puppet.
chelnak added a commit that referenced this issue Apr 12, 2022
This commit introduces a new tmpdir property for the docker_compose
resource that allows the module consumer to decide on the value of
TMPDIR (The location that docker compose uses for temp files).

With the new implementation:

If the tmpdir property is not set, the docker compose will use it's
default value of /tmp.

If the tmpdir property is set, the docker
compose will use the value of the tmpdir property.

It is the responsibility of the consumer to ensure that the directory
passed to the tmpdir is available and writable by the user that is
running puppet.
@baron1405
Copy link

baron1405 commented Apr 13, 2022

This is a blocker for my org puppet infra. Please push out a new release ASAP. Thanks!

@chelnak
Copy link
Contributor

chelnak commented Apr 14, 2022

Hey @baron1405,

We are working on a fix. For now thought, please pin to the previous version if possible.

chelnak added a commit that referenced this issue Apr 14, 2022
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
Copy link
Contributor

chelnak commented Apr 14, 2022

@canihavethisone We are going to back out the additional changes made to modify the TMPDIR environment variable. In this case, I think the priority is to restore the module to a working state for those that don't/can't pin to a specific versions or for new users of the module.

This is an issue we could still attempt to fix and if you would like to continue it would be great if you could open a new issue for it so we can start discussing the way forward.

Thanks for all of your hard work!

@baron1405
Copy link

Thanks @chelnak for the quick reply and everyone's work on this!

@canihavethisone
Copy link
Contributor

canihavethisone commented Apr 14, 2022

thanks @chelnak . Good thing is that the idempotency fix for scaling (which was the main intent) remains intact.

david22swan added a commit that referenced this issue Apr 14, 2022
…_mkdir

Fix permission denied issue introduced in v4.2.0
@chelnak
Copy link
Contributor

chelnak commented Apr 14, 2022

The fix is now in master. I've tested with the following manifest:

class { 'docker':
  log_driver => 'journald',
}

$compose = "compose_test:
  image: ubuntu:14.04
  command: /bin/sh -c 'while true; do echo hello world; sleep 1; done'
"

file { '/tmp/compose.yml':
  content => $compose
}

class { 'docker::compose':
  ensure => present,
}

docker_compose {'test':
  ensure => present,
  compose_files => [
    '/tmp/compose.yml'
  ],
}

snippet of logs

Info: Checking for compose project test
Debug: Executing: '/usr/local/bin/docker-compose -f /tmp/compose.yml -p test config'
Debug: Executing: '/usr/bin/docker ps --format {{.Label "com.docker.compose.service"}}-{{.Image}} --filter label=com.docker.compose.project=test'
Info: Running compose project test
Debug: Executing: '/usr/local/bin/docker-compose -f /tmp/compose.yml -p test up -d --remove-orphans'
Notice: /Stage[main]/Main/Docker_compose[test]/ensure: created
Debug: /Stage[main]/Main/Docker_compose[test]: The container Class[Main] will propagate my refresh event
Debug: Class[Main]: The container Stage[main] will propagate my refresh event
Debug: Finishing transaction 22400
Debug: Storing state
Debug: Pruned old state cache entries in 0.00 seconds
Debug: Stored state in 0.00 seconds
Notice: Applied catalog in 21.65 seconds
Debug: Applying settings catalog for sections reporting, metrics
Debug: Finishing transaction 28920
Debug: Received report to process from ubuntu2004.localdomain
Debug: Processing report from ubuntu2004.localdomain with processor Puppet::Reports::Store

and everything appears to be ok.

We will cut 4.2.1 from this fix.

@chelnak
Copy link
Contributor

chelnak commented Apr 14, 2022

Hey everyone, version 4.2.1 is now available 👍

This release removes the additional changes that modified the TMPDIR environment variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants