From 45114e824a7aef5351a1a6c0dbdd232576e20618 Mon Sep 17 00:00:00 2001 From: Ben Magistro Date: Tue, 13 Dec 2022 21:01:12 +0000 Subject: [PATCH 1/2] Fixes #881 - docker mount comparison This resolves issues identified around mount/volume comparisons for non Windows based OSs. I assume the Windows variant has all the issues found for 881 too but cannot verify/fix. To resolve the issues, the mounts/volumes are turned into hashes and the whole hash is compared versus trying to compare lists of keys. This allows related parameters (source + dest) to be compared at the same time. Signed-off-by: Ben Magistro --- lib/puppet/functions/docker_params_changed.rb | 113 +++++++++++++----- 1 file changed, 81 insertions(+), 32 deletions(-) diff --git a/lib/puppet/functions/docker_params_changed.rb b/lib/puppet/functions/docker_params_changed.rb index bcd4b12a..baf79c84 100644 --- a/lib/puppet/functions/docker_params_changed.rb +++ b/lib/puppet/functions/docker_params_changed.rb @@ -86,38 +86,87 @@ def detect_changes(opts) 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? + # we need to split the mounts from the manifest based on types (binds and volumes) + # we then do a similar split on inspect[hostconfig][mounts] + # TODO make volumes an array vs mixed type support; this feels like it was partially attempted with any2array() + # TODO handle checking options better + pp_binds = [] + pp_volumes = [] + if opts['osfamily'] != 'windows' + vols = opts['volumes'].is_a?(String) ? [opts['volumes']] : opts['volumes'] + vols.each do |vol| + vol_params = vol.split(':') + # we are going to assume that if length is 1 or the first char of the first element is not one of [./~] it is a volume + if vol_params.length == 1 + t_vol = { 'destination' => vol_params[0], 'rw' => true} + # not pushing this to the array because i'm not sure how to deal with the compare... + #pp_volumes.append(t_vol) + elsif not ['.', '~', '/'].include?(vol_params[0][0]) + # this next line is fragile and likely to break because it assumes some ordering op the options + # which may not be true + t_rw = vol_params[2] ? vol_params[2].split(',')[0].downcase == 'rw' ? true : false : true + t_vol = { 'name' => vol_params[0], 'destination' => vol_params[1], 'rw' => t_rw } + pp_volumes.append(t_vol) + else + # this next line is fragile and likely to break because it assumes some ordering op the options + # which may not be true + t_rw = vol_params[2] ? vol_params[2].split(',')[0].downcase == 'rw' ? true : false : true + t_bind = { 'source' => vol_params[0], 'destination' => vol_params[1], 'rw' => t_rw } + pp_binds.append(t_bind) + end + end + + binds = [] + volumes = [] + inspect_hash['Mounts'].map do |mount| + if mount.fetch('Type').downcase == 'bind' + binds.append({ 'source' => mount.fetch('Source'), 'destination' => mount.fetch('Destination'), 'rw' => mount.fetch('RW') }) + elsif mount.fetch('Type').downcase == 'volume' + volumes.append({ 'name' => mount.fetch('Name'), 'destination' => mount.fetch('Destination'), 'rw' => mount.fetch('RW') }) + end + end + + param_changed = true if (pp_binds.length != binds.length && Set.new(pp_binds) != Set.new(binds)) + param_changed = true if (pp_volumes.length > volumes.length && !volumes.subset(pp_volumes)) + elsif opts['osfamily'] == 'windows' + # going to consider this broken for windows since I cant test this... + # what we can do is keep the existing logic and hope for the best... + 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'].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 + + # for the mounts from insepct, if it is a volume, use the volume name + # otherwise use the source path as the value saved for comparison with the manifest values + names = inspect_hash['Mounts'].map { |item| item.fetch('Type').downcase == 'volume' ? item.fetch('Name') : item.fetch('Source') } if inspect_hash['Mounts'] + # for the mounts from the manifest, split the entry and use + pp_names = pp_mounts.map { |item| item.split(':')[0] } if pp_mounts + # sort both lists for the comparison since we don't know which order they will be in + param_changed = true if pp_names.sort != names.sort + + + destinations = inspect_hash['Mounts'].map { |item| item.fetch('Destination') } if inspect_hash['Mounts'] + # remove inspect_paths volumes that are mounted + inspect_paths = inspect_paths.reject { |item| destinations.include?(item) } + 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_destinations.sort != destinations.sort + param_changed = true if pp_paths != inspect_paths + param_changed = true if pp_mounts != [] && inspect_hash['Mounts'].nil? + end + # check if something on ports was changed(some ports were added or removed) From 3101b6f4bb20fc86fb35a0e73e1e5e5506b9a871 Mon Sep 17 00:00:00 2001 From: Ben Magistro Date: Tue, 21 Mar 2023 03:12:16 +0000 Subject: [PATCH 2/2] Fixes #781 - docker port comparisons This resolves issues identified around port comparison by mapping all related fields into a hash and comparing the whole hash versus individual keys. Signed-off-by: Ben Magistro --- lib/puppet/functions/docker_params_changed.rb | 55 +++++++++++++++++-- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/lib/puppet/functions/docker_params_changed.rb b/lib/puppet/functions/docker_params_changed.rb index baf79c84..cc5ae088 100644 --- a/lib/puppet/functions/docker_params_changed.rb +++ b/lib/puppet/functions/docker_params_changed.rb @@ -169,13 +169,58 @@ def detect_changes(opts) # check if something on ports was changed(some ports were added or removed) + # for the ports from inspect, transform this into a hash so we can compare as a set + # with one entry per address family + ports = [] + inspect_hash['NetworkSettings']['Ports'].each do |key, value| + next if not value + cp = key.split('/') + value.each do |host_info| + entry = { + 'protocol' => cp[1], + 'host_ip' => host_info.fetch('HostIp'), + 'host_port' => host_info.fetch('HostPort'), + 'container_port' => cp[0], + } + ports.append(entry) + end + end - 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) + # TODO the manifest should probably also validate with this regex pattern (or a similar one) + # for the ports from the manifest, transform them into a similar hash so we can compare as a set + # with one entry per address family + pp_ports = [] + if opts['ports'].is_a?(String) + if opts['ports'] =~ /(?:(.+):)?([0-9]+):([0-9]+)(?:\/(.+))?/ + addrs = $1 ? [$1] : ['0.0.0.0', '::'] + addrs.each do |addr| + entry = { + 'protocol' => $4 || 'tcp', + 'host_ip' => addr, + 'host_port' => $2, + 'container_port' => $3, + } + pp_ports.append(entry) + end + end + elsif opts['ports'].is_a?(Array) + opts['ports'].each do |port| + if port =~ /(?:(.+):)?([0-9]+):([0-9]+)(?:\/(.+))?/ + addrs = $1 ? [$1] : ['0.0.0.0', '::'] + addrs.each do |addr| + entry = { + 'protocol' => $4 || 'tcp', + 'host_ip' => addr, + 'host_port' => $2, + 'container_port' => $3, + } + pp_ports.append(entry) + end + end + end + end - param_changed = true if pp_ports && pp_ports != ports + param_changed = true if (pp_ports.length != ports.length && Set.new(pp_ports) != Set.new(ports)) if param_changed remove_container(opts['sanitised_title'], opts['osfamily'], opts['stop_wait_time'], opts['cidfile'])