From 0a1180f9dbcb5679c75dd9cf2dfd8b8cdb67fb5b Mon Sep 17 00:00:00 2001 From: Andrew Date: Sun, 10 Jul 2022 16:20:25 +1000 Subject: [PATCH 1/8] fix diffparam_volume and add diffparam_mount and diffparam_tmpfs This is a fix for bug #355. This compares the arguments podman recieved for the currently existing container with the (effective) arguments to come. This approach was taken over parsing Mounts from inspect because: 1. This line from https://github.com/containers/podman/blob/e084f0ee1e1ded564110e84eefca9f18b5669adf/libpod/container_inspect.go#L224 regarding inspect's Mount value: "Only includes user-specified mounts. Only includes bind mounts and named volumes, not tmpfs volumes." Thus an incomplete solution would result. 2. The code required to parse so that it could be compared with the stuff to come was getting silly-level complex. 3. Anonymous volumes were impossible to decipher as both Name and Source are filled in with the podman-generated values. Thus we compare the arguments podman create was run with to make the existing container with the closest values to those arguments in the new config. This resulted in simpler code that takes care of the issue of anonymous volumes. The downside is that if someone moves, say, a tmpfs from mount to tmpfs (or vice versa) that would reult in exactly the same result this will be considered a different config. This can (possibly) be fixed if and when it becomes an actual issue. Signed-off-by: Andrew --- .../podman/podman_container_lib.py | 66 ++++++++++--------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/plugins/module_utils/podman/podman_container_lib.py b/plugins/module_utils/podman/podman_container_lib.py index ff4c1862..5ba56b39 100644 --- a/plugins/module_utils/podman/podman_container_lib.py +++ b/plugins/module_utils/podman/podman_container_lib.py @@ -1167,6 +1167,19 @@ def diffparam_mac_address(self): after = before return self._diff_update_and_compare('mac_address', before, after) + def diffparam_mount(self): + before = [] + if self.info['config'].get('createcommand'): + cr_com = self.info['config']['createcommand'] + for i, v in enumerate(cr_com): + if v == '--mount': + before.append(cr_com[i + 1]) + after = self.params.get('mount') + if not after: + after = [] + before, after = sorted(list(set(before))), sorted(list(set(after))) + return self._diff_update_and_compare('mount', before, after) + def diffparam_network(self): net_mode_before = self.info['hostconfig']['networkmode'] net_mode_after = '' @@ -1307,6 +1320,21 @@ def diffparam_timezone(self): after = self.params['timezone'] return self._diff_update_and_compare('timezone', before, after) + def diffparam_tmpfs(self): + before = [] + if self.info['config'].get('createcommand'): + cr_com = self.info['config']['createcommand'] + for i, v in enumerate(cr_com): + if v == '--tmpfs': + before.append(cr_com[i + 1]) + after = [] + tmpfs = self.params.get('tmpfs') + if tmpfs: + for k, v in tmpfs.items(): + after.append('{}:{}'.format(k, v)) + before, after = sorted(list(set(before))), sorted(list(set(after))) + return self._diff_update_and_compare('tmpfs', before, after) + def diffparam_tty(self): before = self.info['config']['tty'] after = self.params['tty'] @@ -1348,37 +1376,15 @@ def diffparam_uts(self): return self._diff_update_and_compare('uts', before, after) def diffparam_volume(self): - def clean_volume(x): - '''Remove trailing and double slashes from volumes.''' - if not x.rstrip("/"): - return "/" - return x.replace("//", "/").rstrip("/") - - before = self.info['mounts'] - before_local_vols = [] - if before: - volumes = [] - local_vols = [] - for m in before: - if m['type'] != 'volume': - volumes.append( - [ - clean_volume(m['source']), - clean_volume(m['destination']) - ]) - elif m['type'] == 'volume': - local_vols.append( - [m['name'], clean_volume(m['destination'])]) - before = [":".join(v) for v in volumes] - before_local_vols = [":".join(v) for v in local_vols] - if self.params['volume'] is not None: - after = [":".join( - [clean_volume(i) for i in v.split(":")[:2]] - ) for v in self.params['volume']] - else: + before = [] + if self.info['config'].get('createcommand'): + cr_com = self.info['config']['createcommand'] + for i, v in enumerate(cr_com): + if v == '--volume': + before.append(cr_com[i + 1]) + after = self.params.get('volume') + if not after: after = [] - if before_local_vols: - after = list(set(after).difference(before_local_vols)) before, after = sorted(list(set(before))), sorted(list(set(after))) return self._diff_update_and_compare('volume', before, after) From a94f2284008304d394e5de15e03ddd8341d9cd98 Mon Sep 17 00:00:00 2001 From: Andrew Date: Sun, 10 Jul 2022 17:12:53 +1000 Subject: [PATCH 2/8] clean paths in diff checks The old code removed unnecessary slashes via the _clean_volume() function. I accidentally got rid of it and have re-introduced it here as self._clean_path(). The rename is because it is used outside of mere check of volumes but the internal code is as before. Signed-off-by: Andrew --- .../podman/podman_container_lib.py | 35 +++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/plugins/module_utils/podman/podman_container_lib.py b/plugins/module_utils/podman/podman_container_lib.py index 5ba56b39..1f680282 100644 --- a/plugins/module_utils/podman/podman_container_lib.py +++ b/plugins/module_utils/podman/podman_container_lib.py @@ -809,6 +809,28 @@ def _diff_update_and_compare(self, param_name, before, after): return True return False + def _clean_path(self, x): + '''Remove trailing and double slashes from path.''' + if not x.rstrip("/"): + return "/" + return x.replace("//", "/").rstrip("/") + + def _clean_path_in_mount_str(self, mounts): + mfin = [] + for mstr in mounts: + for mitem in mstr.split(','): + nv = mitem.split('=', maxsplit=1) + miname = nv[0] + mival = nv[1] if len(nv) > 1 else None + if miname in ['src', 'source', 'dst', 'dest', 'destination', 'target']: + if mival: + mival = self._clean_path(mival) + if mival is None: + mfin.append(miname) + else: + mfin.append("{0}={1}".format(miname, mival)) + return mfin + def diffparam_annotation(self): before = self.info['config']['annotations'] or {} after = before.copy() @@ -1173,10 +1195,12 @@ def diffparam_mount(self): cr_com = self.info['config']['createcommand'] for i, v in enumerate(cr_com): if v == '--mount': - before.append(cr_com[i + 1]) + before = self._clean_path_in_mount_str(cr_com[i + 1]) after = self.params.get('mount') if not after: after = [] + else: + after = self._clean_path_in_mount_str(after) before, after = sorted(list(set(before))), sorted(list(set(after))) return self._diff_update_and_compare('mount', before, after) @@ -1331,7 +1355,10 @@ def diffparam_tmpfs(self): tmpfs = self.params.get('tmpfs') if tmpfs: for k, v in tmpfs.items(): - after.append('{}:{}'.format(k, v)) + if v: + after.append('{0}:{1}'.format(self._clean_path(k), self._clean_path(v))) + else: + after.append(self._clean_path(k)) before, after = sorted(list(set(before))), sorted(list(set(after))) return self._diff_update_and_compare('tmpfs', before, after) @@ -1381,10 +1408,12 @@ def diffparam_volume(self): cr_com = self.info['config']['createcommand'] for i, v in enumerate(cr_com): if v == '--volume': - before.append(cr_com[i + 1]) + before = self._clean_path_in_mount_str(cr_com[i + 1]) after = self.params.get('volume') if not after: after = [] + else: + after = self._clean_path_in_mount_str(after) before, after = sorted(list(set(before))), sorted(list(set(after))) return self._diff_update_and_compare('volume', before, after) From f213347682a85490e293862381b999e1b042d15e Mon Sep 17 00:00:00 2001 From: Andrew Date: Mon, 11 Jul 2022 11:49:30 +1000 Subject: [PATCH 3/8] correct string in list context errors and sort mount string Began sorting in self._clean_path_in_mount_str() as an early defense against ordering of mount arguments changing. This steels against false comparison errors later. Fixed lots of embarrassing string in list context errors. Oops! :( Signed-off-by: Andrew --- .../podman/podman_container_lib.py | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/plugins/module_utils/podman/podman_container_lib.py b/plugins/module_utils/podman/podman_container_lib.py index 1f680282..84a5253b 100644 --- a/plugins/module_utils/podman/podman_container_lib.py +++ b/plugins/module_utils/podman/podman_container_lib.py @@ -816,9 +816,13 @@ def _clean_path(self, x): return x.replace("//", "/").rstrip("/") def _clean_path_in_mount_str(self, mounts): + '''Parse a mount string, using _clean_path() on any known path + values. Sort the resulting mount string as a defense against + order changing as the return is likely used for comparisons.''' mfin = [] - for mstr in mounts: - for mitem in mstr.split(','): + for mstr in [mounts] if type(mounts) is str else mounts: + msfin = [] + for mitem in sorted(mstr.split(',')): nv = mitem.split('=', maxsplit=1) miname = nv[0] mival = nv[1] if len(nv) > 1 else None @@ -826,10 +830,11 @@ def _clean_path_in_mount_str(self, mounts): if mival: mival = self._clean_path(mival) if mival is None: - mfin.append(miname) + msfin.append(miname) else: - mfin.append("{0}={1}".format(miname, mival)) - return mfin + msfin.append("{0}={1}".format(miname, mival)) + mfin.append(','.join(msfin)) + return mfin[0] if type(mounts) is str else mfin def diffparam_annotation(self): before = self.info['config']['annotations'] or {} @@ -1195,7 +1200,7 @@ def diffparam_mount(self): cr_com = self.info['config']['createcommand'] for i, v in enumerate(cr_com): if v == '--mount': - before = self._clean_path_in_mount_str(cr_com[i + 1]) + before.append(self._clean_path_in_mount_str(cr_com[i + 1])) after = self.params.get('mount') if not after: after = [] @@ -1350,13 +1355,14 @@ def diffparam_tmpfs(self): cr_com = self.info['config']['createcommand'] for i, v in enumerate(cr_com): if v == '--tmpfs': - before.append(cr_com[i + 1]) + tp, tm = cr_com[i + 1].split(':') + before.append('{0}:{1}'.format(self._clean_path(tp), self._clean_path_in_mount_str(tm))) after = [] tmpfs = self.params.get('tmpfs') if tmpfs: for k, v in tmpfs.items(): if v: - after.append('{0}:{1}'.format(self._clean_path(k), self._clean_path(v))) + after.append('{0}:{1}'.format(self._clean_path(k), self._clean_path_in_mount_str(v))) else: after.append(self._clean_path(k)) before, after = sorted(list(set(before))), sorted(list(set(after))) @@ -1408,7 +1414,7 @@ def diffparam_volume(self): cr_com = self.info['config']['createcommand'] for i, v in enumerate(cr_com): if v == '--volume': - before = self._clean_path_in_mount_str(cr_com[i + 1]) + before.append(self._clean_path_in_mount_str(cr_com[i + 1])) after = self.params.get('volume') if not after: after = [] From 31c6830cbdc412460e784ace6d94b2b39b5b91b0 Mon Sep 17 00:00:00 2001 From: Andrew Date: Tue, 12 Jul 2022 10:06:16 +1000 Subject: [PATCH 4/8] fix slash squashing in diffparam_volume and _clean_path Made _clean_path() squash multiple consecutive slashes into one as 3+ slashes are just as meaningful as 1. Slash handling is now done properly diffparam_volume(). Signed-off-by: Andrew --- .../podman/podman_container_lib.py | 35 +++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/plugins/module_utils/podman/podman_container_lib.py b/plugins/module_utils/podman/podman_container_lib.py index 84a5253b..c0bb8ee5 100644 --- a/plugins/module_utils/podman/podman_container_lib.py +++ b/plugins/module_utils/podman/podman_container_lib.py @@ -810,10 +810,16 @@ def _diff_update_and_compare(self, param_name, before, after): return False def _clean_path(self, x): - '''Remove trailing and double slashes from path.''' + '''Remove trailing and multiple consecutive slashes from path.''' if not x.rstrip("/"): return "/" - return x.replace("//", "/").rstrip("/") + b = x.rstrip("/") + while True: + a = b.replace("//", "/") + if a == b: + break + b = a + return a def _clean_path_in_mount_str(self, mounts): '''Parse a mount string, using _clean_path() on any known path @@ -1409,17 +1415,34 @@ def diffparam_uts(self): return self._diff_update_and_compare('uts', before, after) def diffparam_volume(self): + def clean_volume_def(vdef): + # va = [:|:][:] + va = vdef.split(':', maxsplit=2) + if len(va) == 2: + if len(va[1]) > 0 and va[1][0] == '/': + # va = [:|:] + return '{0}:{1}'.format(self._clean_path(va[0]), self._clean_path(va[1])) + # va = [:] + return '{0}:{1}'.format(self._clean_path(va[0]), self._clean_path_in_mount_str(va[1])) + elif len(va) == 3: + # va = [:|:][:] + return '{0}:{1}:{2}'.format(self._clean_path(va[0]), self._clean_path(va[1]), self._clean_path_in_mount_str(va[2])) + # va = + return '{0}'.format(self._clean_path(va[0])) + before = [] if self.info['config'].get('createcommand'): cr_com = self.info['config']['createcommand'] for i, v in enumerate(cr_com): if v == '--volume': - before.append(self._clean_path_in_mount_str(cr_com[i + 1])) - after = self.params.get('volume') - if not after: + before.append(clean_volume_def(cr_com[i + 1])) + after = [] + params = self.params.get('volume') + if not params: after = [] else: - after = self._clean_path_in_mount_str(after) + for p in params: + after.append(clean_volume_def(p)) before, after = sorted(list(set(before))), sorted(list(set(after))) return self._diff_update_and_compare('volume', before, after) From 7508c5ae8f3ab4d3a9464e330118c2a709770915 Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 12 Oct 2022 15:39:24 +1300 Subject: [PATCH 5/8] check --mount, --tmpfs, --volume and VOLUME for changes Combine diffparam_mount(), diffparam_tmpfs() and diffparam_volume() and add handling of VOLUME into one function that allows for movement of definitions between the various ways of specifying mount points as long as the options are equivalent. This necessitated translations of the various ways of defining mount points into one single "language" so that the definitions may be compared and checked for similarity. prep_mount_args_for_comp() does this work and includes within it the default options for the various CLI arguments and VOLUME. The consequence of this is that the diff returned by the function no longer literally represents the real-life arguments passed but, rather, the translations of those arguments. Signed-off-by: Andrew --- .../podman/podman_container_lib.py | 300 ++++++++++++------ 1 file changed, 208 insertions(+), 92 deletions(-) diff --git a/plugins/module_utils/podman/podman_container_lib.py b/plugins/module_utils/podman/podman_container_lib.py index c0bb8ee5..cd5dec1c 100644 --- a/plugins/module_utils/podman/podman_container_lib.py +++ b/plugins/module_utils/podman/podman_container_lib.py @@ -809,39 +809,6 @@ def _diff_update_and_compare(self, param_name, before, after): return True return False - def _clean_path(self, x): - '''Remove trailing and multiple consecutive slashes from path.''' - if not x.rstrip("/"): - return "/" - b = x.rstrip("/") - while True: - a = b.replace("//", "/") - if a == b: - break - b = a - return a - - def _clean_path_in_mount_str(self, mounts): - '''Parse a mount string, using _clean_path() on any known path - values. Sort the resulting mount string as a defense against - order changing as the return is likely used for comparisons.''' - mfin = [] - for mstr in [mounts] if type(mounts) is str else mounts: - msfin = [] - for mitem in sorted(mstr.split(',')): - nv = mitem.split('=', maxsplit=1) - miname = nv[0] - mival = nv[1] if len(nv) > 1 else None - if miname in ['src', 'source', 'dst', 'dest', 'destination', 'target']: - if mival: - mival = self._clean_path(mival) - if mival is None: - msfin.append(miname) - else: - msfin.append("{0}={1}".format(miname, mival)) - mfin.append(','.join(msfin)) - return mfin[0] if type(mounts) is str else mfin - def diffparam_annotation(self): before = self.info['config']['annotations'] or {} after = before.copy() @@ -1200,21 +1167,6 @@ def diffparam_mac_address(self): after = before return self._diff_update_and_compare('mac_address', before, after) - def diffparam_mount(self): - before = [] - if self.info['config'].get('createcommand'): - cr_com = self.info['config']['createcommand'] - for i, v in enumerate(cr_com): - if v == '--mount': - before.append(self._clean_path_in_mount_str(cr_com[i + 1])) - after = self.params.get('mount') - if not after: - after = [] - else: - after = self._clean_path_in_mount_str(after) - before, after = sorted(list(set(before))), sorted(list(set(after))) - return self._diff_update_and_compare('mount', before, after) - def diffparam_network(self): net_mode_before = self.info['hostconfig']['networkmode'] net_mode_after = '' @@ -1355,25 +1307,6 @@ def diffparam_timezone(self): after = self.params['timezone'] return self._diff_update_and_compare('timezone', before, after) - def diffparam_tmpfs(self): - before = [] - if self.info['config'].get('createcommand'): - cr_com = self.info['config']['createcommand'] - for i, v in enumerate(cr_com): - if v == '--tmpfs': - tp, tm = cr_com[i + 1].split(':') - before.append('{0}:{1}'.format(self._clean_path(tp), self._clean_path_in_mount_str(tm))) - after = [] - tmpfs = self.params.get('tmpfs') - if tmpfs: - for k, v in tmpfs.items(): - if v: - after.append('{0}:{1}'.format(self._clean_path(k), self._clean_path_in_mount_str(v))) - else: - after.append(self._clean_path(k)) - before, after = sorted(list(set(before))), sorted(list(set(after))) - return self._diff_update_and_compare('tmpfs', before, after) - def diffparam_tty(self): before = self.info['config']['tty'] after = self.params['tty'] @@ -1414,37 +1347,220 @@ def diffparam_uts(self): after = before return self._diff_update_and_compare('uts', before, after) - def diffparam_volume(self): - def clean_volume_def(vdef): - # va = [:|:][:] - va = vdef.split(':', maxsplit=2) - if len(va) == 2: - if len(va[1]) > 0 and va[1][0] == '/': - # va = [:|:] - return '{0}:{1}'.format(self._clean_path(va[0]), self._clean_path(va[1])) - # va = [:] - return '{0}:{1}'.format(self._clean_path(va[0]), self._clean_path_in_mount_str(va[1])) - elif len(va) == 3: - # va = [:|:][:] - return '{0}:{1}:{2}'.format(self._clean_path(va[0]), self._clean_path(va[1]), self._clean_path_in_mount_str(va[2])) - # va = - return '{0}'.format(self._clean_path(va[0])) + def diffparam_volumes_all(self): + '''check for difference between old and new uses of --mount, --tmpfs, --volume and + Containerfile's VOLUME.''' + + def clean_path(x): + '''Remove trailing and multiple consecutive slashes from path. Collapse '/./' to '/'.''' + if not x.rstrip("/"): + return "/" + b = x.rstrip("/") + while True: + a = b.replace("//", "/") + a = a.replace("/./", "/") + if a == b: + break + b = a + return a + + def prep_mount_args_for_comp(mounts, mtype=None): + '''Parse a mount string, using clean_path() on any known path + values. Sort the resulting mount string as a defense against + order changing as the return is likely used for comparisons.''' + + # check the code where defaults below are used if changing. unfortunately things + # are not cleanly separated and their handling requires some encoding. :( + # the 'fake' mount options are there to allow for matching with tmpfs/volume/etc + # options whilst keeping them separate from real options. + # all args are assumed to be item[=val] format and comma separated in-code. + default_mount_args = { + 'volume': 'readonly=false,chown=false,idmap=false', + 'bind': 'readonly=false,chown=false,idmap=false,bind-propagation=rprivate', + 'tmpfs': 'readonly=false,chown=false,tmpfs-mode=1777', + 'devpts': 'uid=0,gid=0,mode=600,max=1048576', + 'image': 'readwrite=false', + } + noargs_tmpfs_args = 'rw,noexec,nosuid,nodev' + default_tmpfs_args = 'rw,suid,dev,exec,size=50%' + default_volume_args = 'rw,rprivate,nosuid,nodev' # manpage says "private" but podman inspect says "rprivate" + + mfin = [] + wants_str = isinstance(mounts, str) + for mstr in [mounts] if wants_str else mounts: + mstype = mtype + msparsed = { + 'type': mtype, + } + for mitem in sorted(mstr.split(',')): + nv = mitem.split('=', maxsplit=1) + miname = nv[0] + mival = nv[1] if len(nv) > 1 else None + if miname == 'type': + mstype = mival + msparsed['type'] = mstype + break + if mstype == 'tmpfs': + if not mstr: + mstr = noargs_tmpfs_args + mstr = ','.join([default_mount_args[mstype], default_tmpfs_args, mstr]).strip(',') + elif mstype == 'volume': + mstr = ','.join([default_mount_args[mstype], default_volume_args, mstr]).strip(',') + else: + mstr = ','.join([default_mount_args[mstype], mstr]).strip(',') + if not mstype: + raise ValueError("mount type not set/found for '{}'.".format(mstr)) + for mitem in mstr.split(','): + nv = mitem.split('=', maxsplit=1) + miname = nv[0] + mival = nv[1] if len(nv) > 1 else None + if miname in ['source', 'src']: + miname = 'src' + if mival: + mival = clean_path(mival) + elif miname in ['destination', 'dest', 'target', 'dst']: + miname = 'dst' + if mival: + mival = clean_path(mival) + elif miname in ['ro', 'readonly']: + miname = 'readonly' + if not mival: + mival = 'true' + elif miname in ['rw', 'readwrite']: + miname = 'readonly' + if not mival: + mival = 'false' + elif mival == 'true': + mival = 'false' + elif mival == 'false': + mival = 'true' + elif miname in ['U', 'chown']: + miname = 'chown' + if not mival: + mival = 'true' + elif miname in ['z']: + miname = 'relabel' + mival = 'shared' + elif miname in ['Z']: + miname = 'relabel' + mival = 'private' + elif miname in ['exec', 'dev', 'suid', 'sync']: + if not mival: + mival = 'true' + elif miname in ['noexec', 'nodev', 'nosuid']: + miname = miname[2:] + mival = 'false' + elif miname in ['shared', 'slave', 'private', 'unbindable', 'rshared', 'rslave', 'runbindable', 'rprivate']: + mival = miname + miname = 'bind-propagation' + elif miname in ['idmap']: + if not mival: + mival = 'true' + elif mstype == 'tmpfs' and miname in ['size', 'mode'] and mival: + miname = 'tmpfs-{}'.format(miname) + elif miname in ['type', 'tmpfs-size', 'tmpfs-mode', 'bind-propagation', 'relabel']: + pass + elif miname.startswith('_unparsed_'): + pass + else: + miname = '_unparsed_{}'.format(miname) + # source can be optional and can be specified as empty. If it is empty + # remove it altogether so that comparisons can be made simply. + if miname == 'src' and not mival: + miname = None + if miname: + msparsed[miname] = mival + msfin = [] + for miname, mival in msparsed.items(): + if mival is None: + msfin.append(miname) + else: + msfin.append("{0}={1}".format(miname, mival)) + mfin.append(','.join(sorted(list(set(msfin))))) + return mfin[0] if wants_str else mfin + + def clean_image_vols(iv_type): + '''cleans a list or string of Containerfile VOLUME config(s).''' + if iv_type == 'bind': + return prep_volume_for_comp(image_volumes) + elif iv_type == 'tmpfs': + return prep_tmpfs_for_comp(image_volumes) + raise ValueError("invalid image volume type: {}.".format(iv_type)) + + def prep_tmpfs_for_comp(all_ms): + res = [] + wants_str = isinstance(all_ms, str) + is_dict = isinstance(all_ms, dict) + for ms in [all_ms] if wants_str else all_ms: + if is_dict: + dm = [ms, all_ms[ms]] + else: + dm = ms.split(':', maxsplit=1) + if len(dm) == 1: + res.append('dst={}'.format(dm[0])) + else: + res.append('dst={},{}'.format(dm[0], dm[1])) + return prep_mount_args_for_comp(res[0] if wants_str else res, mtype='tmpfs') + + def prep_volume_for_comp(all_ms): + res = [] + wants_str = isinstance(all_ms, str) + for ms in [all_ms] if wants_str else all_ms: + dm = ms.split(':', maxsplit=2) + mtype = 'volume' + if len(dm) == 1: + # ms is , mtype is volume + dm.append(dm[0]) # + dm.append('') # + dm[0] = '' # + else: + # ms is :: + if dm[0].startswith('/'): + mtype = 'bind' + if len(dm) == 2: + # ms is : + dm.append('') # + dm[2] = prep_mount_args_for_comp(dm[2], mtype=mtype) + res.append('src={},dst={},{}'.format(dm[0], dm[1], dm[2]).strip(',')) + return prep_mount_args_for_comp(res[0] if wants_str else res, mtype='volume') before = [] + iv_type = 'bind' # documented default + image_volumes = list(map(clean_path, self.image_info['config'].get('volumes', {}).keys())) if self.info['config'].get('createcommand'): cr_com = self.info['config']['createcommand'] for i, v in enumerate(cr_com): - if v == '--volume': - before.append(clean_volume_def(cr_com[i + 1])) - after = [] - params = self.params.get('volume') - if not params: - after = [] - else: - for p in params: - after.append(clean_volume_def(p)) + if v == '--image-volume': + if cr_com[i + 1]: + iv_type = cr_com[i + 1] + elif v == '--mount': + f = prep_mount_args_for_comp(cr_com[i + 1]) + if f: + before.append(f) + elif v == '--tmpfs': + f = prep_tmpfs_for_comp(cr_com[i + 1]) + if f: + before.append(f) + elif v == '--volume': + f = prep_volume_for_comp(cr_com[i + 1]) + if f: + before.append(f) + before.extend(clean_image_vols(iv_type)) + + after = list(self.params.get('mount') or set()) + after_iv_type = self.params.get('image_volume', 'bind') or 'bind' + after.extend(clean_image_vols(after_iv_type)) + + tvols = self.params.get('tmpfs') + if tvols: + after.extend(prep_tmpfs_for_comp(tvols)) + tvols = self.params.get('volume') + if tvols: + after.extend(prep_volume_for_comp(tvols)) + after = prep_mount_args_for_comp(after) + before, after = sorted(list(set(before))), sorted(list(set(after))) - return self._diff_update_and_compare('volume', before, after) + return self._diff_update_and_compare('volumes_all', before, after) def diffparam_volumes_from(self): # Possibly volumesfrom is not in config From 18d8ff91c189e9d2f6674c3b27b98633b65bab5d Mon Sep 17 00:00:00 2001 From: Andrew Date: Wed, 12 Oct 2022 16:41:10 +1300 Subject: [PATCH 6/8] number fields in string format and key error Fixes: ERROR: Found 7 pylint issue(s) which need to be resolved: ERROR: plugins/module_utils/podman/podman_container_lib.py:1288:37: ansible-format-automatic-specification: Format string contains automatic field numbering specification ... ERROR: plugins/module_utils/podman/podman_container_lib.py:1400:27: ansible-format-automatic-specification: Format string contains automatic field numbering specification also: File "/tmp/ansible_containers.podman.podman_container_payload_6atuqv63/ansible_containers.podman.podman_container_payload.zip/ansible_collections/containers/podman/plugins/module_utils/podman/podman_container_lib.py", line 1405, in diffparam_volumes_all KeyError: 'config' Signed-off-by: Andrew --- .../module_utils/podman/podman_container_lib.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/plugins/module_utils/podman/podman_container_lib.py b/plugins/module_utils/podman/podman_container_lib.py index cd5dec1c..104f2189 100644 --- a/plugins/module_utils/podman/podman_container_lib.py +++ b/plugins/module_utils/podman/podman_container_lib.py @@ -1409,7 +1409,7 @@ def prep_mount_args_for_comp(mounts, mtype=None): else: mstr = ','.join([default_mount_args[mstype], mstr]).strip(',') if not mstype: - raise ValueError("mount type not set/found for '{}'.".format(mstr)) + raise ValueError("mount type not set/found for '{0}'.".format(mstr)) for mitem in mstr.split(','): nv = mitem.split('=', maxsplit=1) miname = nv[0] @@ -1457,13 +1457,13 @@ def prep_mount_args_for_comp(mounts, mtype=None): if not mival: mival = 'true' elif mstype == 'tmpfs' and miname in ['size', 'mode'] and mival: - miname = 'tmpfs-{}'.format(miname) + miname = 'tmpfs-{0}'.format(miname) elif miname in ['type', 'tmpfs-size', 'tmpfs-mode', 'bind-propagation', 'relabel']: pass elif miname.startswith('_unparsed_'): pass else: - miname = '_unparsed_{}'.format(miname) + miname = '_unparsed_{0}'.format(miname) # source can be optional and can be specified as empty. If it is empty # remove it altogether so that comparisons can be made simply. if miname == 'src' and not mival: @@ -1485,7 +1485,7 @@ def clean_image_vols(iv_type): return prep_volume_for_comp(image_volumes) elif iv_type == 'tmpfs': return prep_tmpfs_for_comp(image_volumes) - raise ValueError("invalid image volume type: {}.".format(iv_type)) + raise ValueError("invalid image volume type: {0}.".format(iv_type)) def prep_tmpfs_for_comp(all_ms): res = [] @@ -1497,9 +1497,9 @@ def prep_tmpfs_for_comp(all_ms): else: dm = ms.split(':', maxsplit=1) if len(dm) == 1: - res.append('dst={}'.format(dm[0])) + res.append('dst={0}'.format(dm[0])) else: - res.append('dst={},{}'.format(dm[0], dm[1])) + res.append('dst={0},{1}'.format(dm[0], dm[1])) return prep_mount_args_for_comp(res[0] if wants_str else res, mtype='tmpfs') def prep_volume_for_comp(all_ms): @@ -1521,12 +1521,12 @@ def prep_volume_for_comp(all_ms): # ms is : dm.append('') # dm[2] = prep_mount_args_for_comp(dm[2], mtype=mtype) - res.append('src={},dst={},{}'.format(dm[0], dm[1], dm[2]).strip(',')) + res.append('src={0},dst={1},{2}'.format(dm[0], dm[1], dm[2]).strip(',')) return prep_mount_args_for_comp(res[0] if wants_str else res, mtype='volume') before = [] iv_type = 'bind' # documented default - image_volumes = list(map(clean_path, self.image_info['config'].get('volumes', {}).keys())) + image_volumes = list(map(clean_path, self.image_info.get('config', {}).get('volumes', {}).keys())) if self.info['config'].get('createcommand'): cr_com = self.info['config']['createcommand'] for i, v in enumerate(cr_com): From 5aa29dfa0932623db9c5d161f081762924e7ea07 Mon Sep 17 00:00:00 2001 From: Sagi Shnaidman Date: Sun, 26 Feb 2023 13:20:19 +0200 Subject: [PATCH 7/8] Add unittests for volume idempotency --- .../unit/plugins/modules/test_volume_idem.py | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 tests/unit/plugins/modules/test_volume_idem.py diff --git a/tests/unit/plugins/modules/test_volume_idem.py b/tests/unit/plugins/modules/test_volume_idem.py new file mode 100644 index 00000000..b6e129a6 --- /dev/null +++ b/tests/unit/plugins/modules/test_volume_idem.py @@ -0,0 +1,89 @@ +from __future__ import absolute_import, division, print_function + +__metaclass__ = type + +import pytest + +from plugins.module_utils.podman.podman_container_lib import ( + PodmanModuleParams, + PodmanContainerDiff, +) + + +@pytest.mark.parametrize( + "test_input, expected", + [ + ( + { + "cap_add": ["SYS_ADMIN"], + "name": "testcont", + "image": "testimage", + "command": None, + }, + [ + b"create", + b"--name", + b"testcont", + b"--cap-add", + b"SYS_ADMIN", + b"testimage", + ], + ), + ( + { + "stop_signal": 9, + "name": "testcont", + "image": "testimage", + "command": None, + "sig_proxy": True, + }, + [ + b"create", + b"--name", + b"testcont", + b"--stop-signal", + b"9", + b"--sig-proxy=True", + b"testimage", + ], + ), + ], +) +def test_container_add_params(test_input, expected): + podm = PodmanModuleParams( + "create", + test_input, + "4.0.0", + None, + ) + assert podm.construct_command_from_params() == expected + + +@pytest.mark.parametrize( + "test_input, expected", + [ + ( + [ + None, # module + {"conmon_pidfile": "bbb"}, # module params + {"conmonpidfile": "ccc"}, # container info + {}, # image info + "4.1.1", # podman version + ], + True, + ), + ( + [ + None, # module + {"conmon_pidfile": None}, # module params + {"conmonpidfile": "ccc"}, # container info + {}, # image info + "4.1.1", # podman version + ], + False, + ), + ], +) +def test_container_diff(test_input, expected): + diff = PodmanContainerDiff(*test_input) + assert diff.diffparam_conmon_pidfile() == expected From e685d277e36d69448a267e36ba19d689e0445109 Mon Sep 17 00:00:00 2001 From: Sagi Shnaidman Date: Sun, 26 Feb 2023 13:22:16 +0200 Subject: [PATCH 8/8] Add unittests for volume idempotency Signed-off-by: Sagi Shnaidman --- .../podman/podman_container_lib.py | 2 +- test-requirements.txt | 1 + .../tasks/idem_volumes.yml | 2 +- .../unit/plugins/modules/test_volume_idem.py | 453 ++++++++++++++++-- 4 files changed, 415 insertions(+), 43 deletions(-) diff --git a/plugins/module_utils/podman/podman_container_lib.py b/plugins/module_utils/podman/podman_container_lib.py index 104f2189..d8a4465a 100644 --- a/plugins/module_utils/podman/podman_container_lib.py +++ b/plugins/module_utils/podman/podman_container_lib.py @@ -1541,7 +1541,7 @@ def prep_volume_for_comp(all_ms): f = prep_tmpfs_for_comp(cr_com[i + 1]) if f: before.append(f) - elif v == '--volume': + elif v in ('--volume', '-v'): f = prep_volume_for_comp(cr_com[i + 1]) if f: before.append(f) diff --git a/test-requirements.txt b/test-requirements.txt index 3edd0df4..b62cece1 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -2,3 +2,4 @@ ansible-core pytest pytest-forked pytest-xdist +pytest-mock diff --git a/tests/integration/targets/podman_container_idempotency/tasks/idem_volumes.yml b/tests/integration/targets/podman_container_idempotency/tasks/idem_volumes.yml index 3fff4e34..a6cb9980 100644 --- a/tests/integration/targets/podman_container_idempotency/tasks/idem_volumes.yml +++ b/tests/integration/targets/podman_container_idempotency/tasks/idem_volumes.yml @@ -190,7 +190,7 @@ - name: check test13 assert: - that: test13 is not changed + that: test13 is changed - containers.podman.podman_container: executable: "{{ test_executable | default('podman') }}" diff --git a/tests/unit/plugins/modules/test_volume_idem.py b/tests/unit/plugins/modules/test_volume_idem.py index b6e129a6..23335734 100644 --- a/tests/unit/plugins/modules/test_volume_idem.py +++ b/tests/unit/plugins/modules/test_volume_idem.py @@ -5,58 +5,177 @@ import pytest from plugins.module_utils.podman.podman_container_lib import ( - PodmanModuleParams, PodmanContainerDiff, ) +# To run it with pytest, VSCode, debugger etc etc +# Do it from collection root directory: +# +# mkdir -p ansible_collections/containers/podman +# touch ansible_collections/__init__.py +# touch ansible_collections/containers/__init__.py +# touch ansible_collections/containers/podman/__init__.py +# touch plugins/__init__.py +# ln -s $PWD/plugins ansible_collections/containers/podman/plugins + + +# In order to test the diff function, we need to patch the _diff_update_and_compare function +def substitute_update_and_compare(instance_diff, param_name, before, after): + return before, after + + +@pytest.fixture +def diff_patched(monkeypatch): + monkeypatch.setattr( + PodmanContainerDiff, "_diff_update_and_compare", substitute_update_and_compare + ) + @pytest.mark.parametrize( "test_input, expected", [ ( - { - "cap_add": ["SYS_ADMIN"], - "name": "testcont", - "image": "testimage", - "command": None, - }, [ - b"create", - b"--name", - b"testcont", - b"--cap-add", - b"SYS_ADMIN", - b"testimage", + None, # module + {"volume": ["/mnt:/tmp"]}, # module params + { + "mounts": [ + { + "type": "bind", + "source": "/mnt", + "destination": "/tmp", + "driver": "", + "mode": "", + "options": ["rbind"], + "rw": True, + "propagation": "rprivate", + } + ], + "config": { + "createcommand": [ + "podman", + "-v", + "/mnt:/tmp", + ], + }, + }, # container info + {}, # image info + "4.1.1", # podman version ], + True, ), ( - { - "stop_signal": 9, - "name": "testcont", - "image": "testimage", - "command": None, - "sig_proxy": True, - }, [ - b"create", - b"--name", - b"testcont", - b"--stop-signal", - b"9", - b"--sig-proxy=True", - b"testimage", + None, # module + {"volume": ["/tmp"]}, # module params + { + "mounts": [ + { + "type": "volume", + "name": "f92e0010df188214805a7f1007683d42c72f3b1bc5c4a14f2e63d42d15c80308", + "source": "/home/user/.local/share/containers/storage/volumes/somehash/_data", + "destination": "/tmp", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + } + ], + "config": { + "createcommand": [ + "podman", + "-v", + "/tmp", + ], + }, + }, # container info + {}, # image info + "4.1.1", # podman version ], + True, ), ], ) -def test_container_add_params(test_input, expected): - podm = PodmanModuleParams( - "create", - test_input, - "4.0.0", - None, - ) - assert podm.construct_command_from_params() == expected +def test_basic_volume_diff(diff_patched, test_input, expected): + diff = PodmanContainerDiff(*test_input) + result = diff.diffparam_volumes_all() # before, after + assert (set(result[0]) == set(result[1])) == expected + + +@pytest.mark.parametrize( + "test_input, expected, change", + [ + ( + [ + None, # module + {"volume": ["/mnt:/data1"]}, # module params + { + "mounts": [ + { + "type": "bind", + "source": "/mnt", + "destination": "/tmp", + "driver": "", + "mode": "", + "options": ["rbind"], + "rw": True, + "propagation": "rprivate", + } + ], + "config": { + "createcommand": [ + "podman", + "-v", + "/mnt:/tmp", + ], + }, + }, # container info + {}, # image info + "4.1.1", # podman version + ], + False, + ["dst=/tmp", "dst=/data1"], + ), + ( + [ + None, # module + {"volume": ["/mnt"]}, # module params + { + "mounts": [ + { + "type": "bind", + "source": "/mnt", + "destination": "/tmp", + "driver": "", + "mode": "", + "options": ["rbind"], + "rw": True, + "propagation": "rprivate", + } + ], + "config": { + "createcommand": [ + "podman", + "-v", + "/mnt:/tmp", + ], + }, + }, # container info + {}, # image info + "4.1.1", # podman version + ], + False, + ["dst=/tmp", "dst=/mnt"], + ), + ], +) +def test_basic_volume_diff_change(diff_patched, test_input, expected, change): + diff = PodmanContainerDiff(*test_input) + result = diff.diffparam_volumes_all() # before, after + assert (result[0] == result[1]) == expected + assert change[0] in result[0][0] + assert change[1] in result[1][0] @pytest.mark.parametrize( @@ -65,25 +184,277 @@ def test_container_add_params(test_input, expected): ( [ None, # module - {"conmon_pidfile": "bbb"}, # module params - {"conmonpidfile": "ccc"}, # container info + {"volume": ["/mnt/", "/data1:/data2/", "./data"]}, # module params + { + "mounts": [ + { + "type": "volume", + "name": "647fda605f70e34d37845a3bda6d01108eb07742beb16865a7a46b5e89f0deb6", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/some_hash/_data", + "destination": "/mnt/", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "volume", + "name": "b74a7b937ecb0fbccb50a158ed872b04318fb997b0e70a3ccdd16a90196acd06", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/some_hash2/_data", + "destination": "/home/user/data", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "bind", + "source": "/data1", + "destination": "/data2/", + "driver": "", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + ], + "config": { + "createcommand": [ + "podman", + "-v", + "/mnt/", + "-v", + "/data1:/data2/", + "-v", + "./data", + ], + }, + }, # container info + {}, # image info + "4.1.1", # podman version + ], + True, + ), + ( + [ + None, # module + { + "volume": ["/mnt/", "/dev/fuse", "named_one:/named", "vvv:/datax"] + }, # module params + { + "mounts": [ + { + "type": "volume", + "name": "09e94208842f5a1e686c0c2e72df9f9d13f7a479d5e1a114a0e6287ff43685f3", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/hash/_data", + "destination": "/dev/fuse", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "volume", + "name": "named_one", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/named_one/_data", + "destination": "/named", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "volume", + "name": "vvv", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/vvv/_data", + "destination": "/datax", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "volume", + "name": "d4d5990d184da6572b2bd3ae879bd1275a52d77fdb1e92d435e874b7490f8148", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/hash1/_data", + "destination": "/mnt/", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + ], + "config": { + "createcommand": [ + "podman", + "-v", + "/mnt/", + "-v", + "/dev/fuse", + "-v", + "named_one:/named", + "-v", + "vvv:/datax", + ], + }, + }, # container info {}, # image info "4.1.1", # podman version ], True, ), + ], +) +def test_basic_multiple_volume_diff(diff_patched, test_input, expected): + diff = PodmanContainerDiff(*test_input) + result = diff.diffparam_volumes_all() # before, after + assert (set(result[0]) == set(result[1])) == expected + + +@pytest.mark.parametrize( + "test_input, expected, change", + [ + ( + [ + None, # module + {"volume": ["/data1:/data5/", "./data"]}, # module params + { + "mounts": [ + { + "type": "volume", + "name": "647fda605f70e34d37845a3bda6d01108eb07742beb16865a7a46b5e89f0deb6", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/some_hash/_data", + "destination": "/mnt/", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "volume", + "name": "b74a7b937ecb0fbccb50a158ed872b04318fb997b0e70a3ccdd16a90196acd06", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/some_hash2/_data", + "destination": "/home/user/data", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "bind", + "source": "/data1", + "destination": "/data2/", + "driver": "", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + ], + "config": { + "createcommand": [ + "podman", + "-v", + "/mnt/", + "-v", + "/data1:/data2/", + "-v", + "./data", + ], + }, + }, # container info + {}, # image info + "4.1.1", # podman version + ], + False, + ["dst=/mnt", "dst=/data5"], + ), ( [ None, # module - {"conmon_pidfile": None}, # module params - {"conmonpidfile": "ccc"}, # container info + { + "volume": ["/mnt/", "newvol:/newvol1", "named_one:/named", "vvv:/datax"] + }, # module params + { + "mounts": [ + { + "type": "volume", + "name": "09e94208842f5a1e686c0c2e72df9f9d13f7a479d5e1a114a0e6287ff43685f3", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/hash/_data", + "destination": "/dev/fuse", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "volume", + "name": "named_one", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/named_one/_data", + "destination": "/named", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "volume", + "name": "vvv", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/vvv/_data", + "destination": "/datax", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + { + "type": "volume", + "name": "d4d5990d184da6572b2bd3ae879bd1275a52d77fdb1e92d435e874b7490f8148", + "source": "/home/sshnaidm/.local/share/containers/storage/volumes/hash1/_data", + "destination": "/mnt/", + "driver": "local", + "mode": "", + "options": ["nosuid", "nodev", "rbind"], + "rw": True, + "propagation": "rprivate", + }, + ], + "config": { + "createcommand": [ + "podman", + "-v", + "/mnt/", + "-v", + "/dev/fuse", + "-v", + "named_one:/named", + "-v", + "vvv:/datax", + ], + }, + }, # container info {}, # image info "4.1.1", # podman version ], False, + ["dst=/dev/fuse", "src=newvol"], ), ], ) -def test_container_diff(test_input, expected): +def test_basic_multiple_volume_diff_change(diff_patched, test_input, expected, change): diff = PodmanContainerDiff(*test_input) - assert diff.diffparam_conmon_pidfile() == expected + result = diff.diffparam_volumes_all() # before, after + assert (result[0] == result[1]) == expected + assert change[0] in " ".join(list(result[0])) + assert change[1] in " ".join(list(result[1]))