[MODULES-10734] - improve params detection on docker::run

lib/puppet/parser/functions/docker_params_changed.rb

@@ -0,0 +1,76 @@
+require 'open3'
+require 'json'
+
+module Puppet::Parser::Functions
+  # Checks if at least one parammeter is changed
+  newfunction(:docker_params_changed, type: :rvalue) do |args|
+    opts = args[0] || {}
+    return_value = []
+
+    if opts['sanitised_title'] && opts['osfamily']
+      stdout, stderr, status = Open3.capture3("docker inspect #{opts['sanitised_title']}")
+      if stderr.to_s == '' && status.to_s.include?('exit 0')
+        param_changed = false
+        inspect_hash = JSON.parse(stdout)[0]
+
+        # check if the image was changed
+        param_changed = true if opts['image'] && opts['image'] != inspect_hash['Config']['Image']
+
+        # check if something on volumes or mounts was changed(a new volume/mount was added or removed)
+        param_changed = true if opts['volumes'].is_a?(String) && opts['volumes'].include?(':') && opts['volumes'] != inspect_hash['Mounts'].to_a[0] && opts['osfamily'] != 'windows'
+        param_changed = true if opts['volumes'].is_a?(String) && !opts['volumes'].include?(':') && opts['volumes'] != inspect_hash['Config']['Volumes'].to_a[0] && opts['osfamily'] != 'windows'
+        param_changed = true if opts['volumes'].is_a?(String) && opts['volumes'].scan(%r{(?=:)}).count == 2 && opts['volumes'] != inspect_hash['Mounts'].to_a[0] && opts['osfamily'] == 'windows'
+        param_changed = if opts['volumes'].is_a?(String) && opts['volumes'].scan(%r{(?=:)}).count == 1 && opts['volumes'] != inspect_hash['Config']['Volumes'].to_a[0] && opts['osfamily'] == 'windows'
+                          true
+                        else
+                          param_changed
+                        end
+
+        pp_paths = opts['volumes'].reject { |item| item.include?(':') } if opts['volumes'].is_a?(Array) && opts['osfamily'] != 'windows'
+        pp_mounts = opts['volumes'].select { |item| item.include?(':') } if opts['volumes'].is_a?(Array) && opts['osfamily'] != 'windows'
+        pp_paths = opts['volumes'].select { |item| item.scan(%r{(?=:)}).count == 1 } if opts['volumes'].is_a?(Array) && opts['osfamily'] == 'windows'
+        pp_mounts = opts['volumes'].select { |item| item.scan(%r{(?=:)}).count == 2 } if opts['volumes'].is_a?(Array) && opts['osfamily'] == 'windows'
+
+        inspect_paths = if inspect_hash['Config']['Volumes']
+                          inspect_hash['Config']['Volumes'].keys
+                        else
+                          []
+                        end
+        param_changed = true if pp_paths != inspect_paths
+
+        names = inspect_hash['Mounts'].map { |item| item.values[1] } if inspect_hash['Mounts']
+        pp_names = pp_mounts.map { |item| item.split(':')[0] } if pp_mounts
+        names = names.select { |item| pp_names.include?(item) } if names && pp_names
+        destinations = inspect_hash['Mounts'].map { |item| item.values[3] } if inspect_hash['Mounts']
+        pp_destinations = pp_mounts.map { |item| item.split(':')[1] } if pp_mounts && opts['osfamily'] != 'windows'
+        pp_destinations = pp_mounts.map { |item| "#{item.split(':')[1].downcase}:#{item.split(':')[2]}" } if pp_mounts && opts['osfamily'] == 'windows'
+        destinations = destinations.select { |item| pp_destinations.include?(item) } if destinations && pp_destinations
+
+        param_changed = true if pp_names != names
+        param_changed = true if pp_destinations != destinations
+        param_changed = true if pp_mounts != [] && inspect_hash['Mounts'].nil?
+
+        # check if something on ports was changed(some ports were added or removed)
+
+        ports = inspect_hash['HostConfig']['PortBindings'].keys
+        ports = ports.map { |item| item.split('/')[0] }
+        pp_ports = opts['ports'].sort if opts['ports'].is_a?(Array)
+        pp_ports = [opts['ports']] if opts['ports'].is_a?(String)
+
+        param_changed = true if pp_ports && pp_ports != ports
+
+        return_value << if param_changed
+                          'PARAM_CHANGED'
+                        else
+                          'NO_CHANGE'
+                        end
+      else
+        return_value << 'CONTAINER_NOT_FOUND'
+      end
+    else
+      return_value << 'ARG_REQUIRED_MISSING'
+    end
+
+    return_value.flatten.join(' ')
+  end
+end

lib/puppet/parser/functions/docker_run_flags.rb

@@ -23,7 +23,7 @@ module Puppet::Parser::Functions
     end
 
     if opts['net'].is_a? String
-      flags << "--net #{opts['net']}"
+      flags << "--net #{opts['net'].shellescape}"
     end
 
     if opts['memory_limit']

manifests/run.pp

@@ -400,18 +400,74 @@
       $inspect = [ "${docker_command} inspect ${sanitised_title}", ]
 
       if $custom_unless {
-        $exec_unless = concat($custom_unless, $inspect)
+        $exec_unless = $custom_unless
       } else {
         $exec_unless = $inspect
       }
 
-      exec { "run ${title} with docker":
-        command     => join($run_with_docker_command, ' '),
-        unless      => $exec_unless,
-        environment => $exec_environment,
-        path        => $exec_path,
-        provider    => $exec_provider,
-        timeout     => $exec_timeout,
+      $detect_changes = docker_params_changed(
+        {
+          sanitised_title => $sanitised_title,
+          osfamily        => $facts['os']['family'],
+          image           => $image,
+          volumes         => $volumes,
+          ports           => $ports,
+        }
+      )
+
+      case $detect_changes {
+        'CONTAINER_NOT_FOUND': {
+          exec { "run ${title} with docker":
+            command     => join($run_with_docker_command, ' '),
+            unless      => $exec_unless,
+            environment => $exec_environment,
+            path        => $exec_path,
+            provider    => $exec_provider,
+            timeout     => $exec_timeout,
+          }
+        }
+        'PARAM_CHANGED': {
+          exec { "stop ${title} with docker":
+            command     => "${docker_command} stop --time=${stop_wait_time} ${sanitised_title}",
+            onlyif      => "${docker_command} inspect ${sanitised_title}",
+            environment => $exec_environment,
+            path        => $exec_path,
+            provider    => $exec_provider,
+            timeout     => $exec_timeout,
+          }
+
+          exec { "remove ${title} with docker":
+            command     => "${docker_command} rm -v ${sanitised_title}",
+            onlyif      => "${docker_command} inspect ${sanitised_title}",
+            environment => $exec_environment,
+            path        => $exec_path,
+            provider    => $exec_provider,
+            timeout     => $exec_timeout,
+          }
+
+          file { $cidfile:
+            ensure => absent,
+          }
+
+          exec { "run ${title} with docker":
+            command     => join($run_with_docker_command, ' '),
+            unless      => $exec_unless,
+            environment => $exec_environment,
+            path        => $exec_path,
+            provider    => $exec_provider,
+            timeout     => $exec_timeout,
+          }
+        }
+        'ARG_REQUIRED_MISSING': {
+          fail(translate('sanitised_title and osfamily are required for docker_params_changed function'))
+        }
+        default: {
+          # Use ONLY on debugging - DO NOT UNCOMMENT OTHERWISE
+          # notify { 'this case should not be executed':
+          #   message  => "detect_changes: ${detect_changes}",
+          #   withpath => true,
+          # }
+        }
       }
 
       if $running == false {

spec/acceptance/docker_params_changed_spec.rb

@@ -0,0 +1,145 @@
+require 'spec_helper_acceptance'
+
+if os[:family] == 'windows'
+  os_name = run_shell('systeminfo | findstr /R /C:"OS Name"')
+  raise 'Could not retrieve systeminfo for Windows box' if os_name.exit_code != 0
+  os_name = os_name.stdout.split(%r{\s}).include?('2016') ? 'win-2016' : 'win-2019'
+  docker_args = 'docker_ee => true'
+  docker_network = 'nat'
+  volume_location = 'C:\\'
+  docker_image = if os_name == 'win-2016'
+                   'stefanscherer/nanoserver:sac2016'
+                 else
+                   'stefanscherer/nanoserver:10.0.17763.1040'
+                 end
+else
+  docker_args = if os[:family] == 'redhat'
+                  "repo_opt => '--enablerepo=localmirror-extras'"
+                else
+                  ''
+                end
+  docker_network = 'bridge'
+  volume_location = '/opt'
+  docker_image = 'hello-world:linux'
+end
+
+describe 'docker trigger parammeters change' do
+  before(:all) do
+    if os[:family] != 'windows'
+      install_pp = "class { 'docker': #{docker_args}}"
+      apply_manifest(install_pp)
+    end
+    run_shell("mkdir #{volume_location}/volume_1")
+    run_shell("mkdir #{volume_location}/volume_2")
+  end
+
+  context 'when image is changed' do
+    image_changed = if os[:family] == 'windows'
+                      if os_name == 'win-2016'
+                        'stefanscherer/nanoserver:10.0.14393.2551'
+                      else
+                        'stefanscherer/nanoserver:1809'
+                      end
+                    else
+                      'hello-world:latest'
+                    end
+    let(:pp1) do
+      "
+          class {'docker': #{docker_args}}
+          docker::run {'servercore': image => '#{docker_image}', restart => 'always', net => '#{docker_network}' }
+      "
+    end
+
+    let(:pp2) do
+      "
+          class {'docker': #{docker_args}}
+          docker::run {'servercore': image => '#{image_changed}', restart => 'always', net => '#{docker_network}' }
+      "
+    end
+
+    it 'creates servercore with first image' do
+      idempotent_apply(pp1)
+    end
+
+    it 'detect image change and apply the change' do
+      apply_manifest(pp2, catch_failures: true)
+      run_shell('docker inspect --format="{{ .Config.Image }}" servercore') do |r|
+        expect(r.stdout).to match(%r{#{image_changed}})
+      end
+    end
+  end
+
+  context 'when volumes parameter is changed' do
+    if os[:family] == 'windows'
+      volumes1 = "volumes => ['volume-1:C:\\volume_1']"
+      volumes2 = "volumes => ['volume-1:C:\\volume_1', 'volume-2:C:\\volume_2']"
+    else
+      volumes1 = "volumes => ['volume-1:#{volume_location}/volume_1']"
+      volumes2 = "volumes => ['volume-1:#{volume_location}/volume_1', 'volume-2:#{volume_location}/volume_2']"
+    end
+
+    let(:pp1) do
+      "
+          class {'docker': #{docker_args}}
+          docker::run {'servercore': image => '#{docker_image}', restart => 'always', net => '#{docker_network}', #{volumes1}}
+      "
+    end
+
+    let(:pp2) do
+      "
+          class {'docker': #{docker_args}}
+          docker::run {'servercore': image => '#{docker_image}', restart => 'always', net => '#{docker_network}', #{volumes2}}
+      "
+    end
+
+    it "creates servercore with #{volumes1}" do
+      idempotent_apply(pp1)
+    end
+
+    it "creates servercore with #{volumes2}" do
+      apply_manifest(pp2, catch_failures: true)
+      run_shell('docker inspect servercore --format="{{ json .Mounts }}"') do |r|
+        inspect_result = JSON.parse(r.stdout)
+        inspect_result = inspect_result.map { |item| item['Name'] }.sort
+        expect(inspect_result).to eq(['volume-1', 'volume-2'])
+      end
+    end
+  end
+
+  context 'when ports parameter is changed' do
+    ports1 = "ports => ['4444']"
+    ports2 = "ports => ['4444', '4445']"
+
+    let(:pp1) do
+      "
+          class {'docker': #{docker_args}}
+          docker::run {'servercore': image => '#{docker_image}', restart => 'always', net => '#{docker_network}', #{ports1}}
+      "
+    end
+
+    let(:pp2) do
+      "
+          class {'docker': #{docker_args}}
+          docker::run {'servercore': image => '#{docker_image}', restart => 'always', net => '#{docker_network}', #{ports2}}
+      "
+    end
+
+    it 'creates servercore with ports => ["4444"]' do
+      idempotent_apply(pp1)
+    end
+
+    it 'creates servercore with ports => ["4444", "4445"]' do
+      apply_manifest(pp2, catch_failures: true)
+      run_shell('docker inspect servercore --format="{{ json .HostConfig.PortBindings }}"') do |r|
+        inspect_result = JSON.parse(r.stdout)
+        inspect_result = inspect_result.keys.map { |item| item.split('/')[0] }.sort
+        expect(inspect_result).to eq(['4444', '4445'])
+      end
+    end
+  end
+
+  after(:all) do
+    run_shell("rm -r #{volume_location}/volume_1")
+    run_shell("rm -r #{volume_location}/volume_2")
+  end
+end