Add tmpdir option to docker_compose

[canihavethisone]

This pull request adds an optional tmpdir parameter to the docker_compose resource. It is useful on systems where the default /tmp directory is mounted with noexec, or consumers wish to specify an alternate tmpdir other than the system-wide environment variable. Note that it does not create the target directory, the consumer is responsible to ensure that the directory exists and is executable.

As well as adding the type from @chelnak example, I have added the following to the provider, which includes verification that the tmpdir target exists, warns and does not set the env var if not. If there is a better way to perform this in Ruby, please comment.

  def set_tmpdir
    return unless resource[:tmpdir]
    # Check if the the tmpdir target exists
    Puppet.warning("#{resource[:tmpdir]} (defined as docker_compose tmpdir) does not exist") unless Dir.exist?(resource[:tmpdir])
    # Set TMPDIR environment variable only if defined among resources and exists
    ENV['TMPDIR'] = resource[:tmpdir] if Dir.exist?(resource[:tmpdir])
  end

The changes pass unit tests and have been verified with the following manifest

class test {

  file { '/usr/local/share/tmp_docker':
    ensure => directory
  }

  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'
    ],
    tmpdir => '/usr/local/share/tmp_docker',
    scale  => {
      compose_test => 3
    },
  }
  docker_compose {'test2':
    ensure => present,
    compose_files => [
      '/tmp/compose.yml'
    ],
  }
}

Note1: this implementation is optional, and does not affect global TMPDIR environment variable (as tested on Linux) as it only applies within the docker_compose resource. I have also updated the readme to reflect that this is optional.

Note2: if any resource has the tmpdir param set, subsequent resources will inherit it unless they specify a different tmpdir target. Therefore it only has to be specified in the first resource if you want it to apply to all (as per above example, test2 will also use the same tmpdir, however if test2 was declared first it would use the system TMPDIR variable or default to /tmp if not set and fail if that dir is noexec). If subsequent resource tmpdir is defined but does not exist, a warning is raised and the previously successful tmpdir value is used.

Apologies for the previous attempt at this that also managed directory creation and introduced a breaking change on first puppet run (though other changes in that merge did fix idempotency with scaling). This however is a more considered approach to address issue as reported at docker/compose#8041 and experienced on hardened systems with noexec on /tmp.

I have not expanded the acceptance test for these changes, and defer to maintainers if they wish to add one.

Any comment or suggestions are welcome.

[puppet-community-rangefinder]

docker_compose is a type

Breaking changes to this file WILL impact these 1 modules (exact match):

• puppetlabs-data_entitlement

Breaking changes to this file MAY impact these 1 modules (near match):

• ffquintella-dockerapp_fusio

This module is declared in 6 of 579 indexed public Puppetfiles.

---

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

[canihavethisone]

Further comment, I found that the pdk validate command presented a large number of warnings and errors in the module outside of these changes. I have not addressed these but bring them to the attention of module maintainers for their consideration @chelnak @david22swan .
Lint was fixed in #824

[chelnak]

Hey! It's taken ages to get round to this sorry!

I think that directory management should be left to the user.

We already have the tools available for this at makes sense to keep the domains isolated.

I'd be interested in getting some other opinions on this change too.

Personally I think it's a reasonable to have this as an optional part of the configuration but there may be other suggestions out there.

Thanks for putting in the effort 😀

[canihavethisone]

@chelnak yep, the pull request as it stands does not attempt directory creation and only warns if the (optional) declared tmpdir does not exist. It achieves the desired outcome without the issue from the previous merge. I'll delete my subsequent comment with the example of creating the dir to avoid confusion.

See the proposed changes to lib/puppet/provider/docker_compose/ruby.rb and reuse of your previous type in lib/puppet/type/docker_compose.rb though as newparam instead of newproperty. The readme was also updated to document this new option, and I couldn't help myself, had to correct indentation in code examples.

[smortex]

How/when is this variable reset to it's original value?

I suspect that this would have an impact with other types / provider depending on resource ordering:

foo { 'before': # ENV['TMPDIR'] #=> nil
}

docker_compose { 'bar':
  path => '/here/or/there',
}

foo { 'after': # ENV['TMPDIR'] #=>  '/here/or/there'
}

[canihavethisone]

@smortex the variable only appears to have effect within the resource, and also the resource collector and ordering in init.pp ensures that docker_compose runs last, so my tests to put ENV['TMPDIR'] during run show it as unset or reflect the system variable within prior resources. It also does not affect system-wide env whether set or unset.

    Class['docker::repos']
    -> Class['docker::install']
    -> Class['docker::config']
    -> Class['docker::service']
    -> Docker::Registry <||>
    -> Docker::Image <||>
    -> Docker::Run <||>
    -> Docker_compose <||>

[chelnak]

@canihavethisone Thanks for that explanation. Makes sense to me.

@smortex What do you think?

[chelnak]

Merging - changes are now optional and should not impact existing users.