From f78b1b3b1467ff8959ab2b560e09a2a404837046 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Fri, 13 Nov 2020 09:03:36 -0600 Subject: [PATCH 1/8] Ensure proper root permissions in integration tests Tests previously assumed that when executing commands and transferring files that user will have root permissions. This change updated integration testing infrastructure so that is true. --- tests/integration_tests/instances.py | 48 +++++++++++++++---- .../modules/test_users_groups.py | 5 +- .../modules/test_write_files.py | 4 +- 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/tests/integration_tests/instances.py b/tests/integration_tests/instances.py index ca0b38d5486..bdc0018f1de 100644 --- a/tests/integration_tests/instances.py +++ b/tests/integration_tests/instances.py @@ -1,9 +1,12 @@ # This file is part of cloud-init. See LICENSE file for license information. import logging import os +import random +import string from tempfile import NamedTemporaryFile from pycloudlib.instance import BaseInstance +from pycloudlib.result import Result from tests.integration_tests import integration_settings @@ -18,6 +21,16 @@ log = logging.getLogger('integration_testing') +class CalledProcessException(Exception): + pass + + +def _get_tmp_path(): + tmp_filename = ''.join([random.choice( + string.ascii_letters + string.digits) for _ in range(20)]) + return '/var/tmp/{}.tmp'.format(tmp_filename) + + class IntegrationInstance: use_sudo = True @@ -30,21 +43,38 @@ def __init__(self, cloud: 'IntegrationCloud', instance: BaseInstance, def destroy(self): self.instance.delete() - def execute(self, command): + def execute(self, command) -> Result: + if self.use_sudo: + if isinstance(command, str): + command = 'sudo {}'.format(command) + elif isinstance(command, list): + command = ['sudo'] + command return self.instance.execute(command) - def pull_file(self, remote_file, local_file): - self.instance.pull_file(remote_file, local_file) + def pull_file(self, remote_path, local_path): + # First copy to a temporary directory because of permissions issues + tmp_path = _get_tmp_path() + self.instance.execute('mv {} {}'.format(remote_path, tmp_path)) + self.instance.pull_file(tmp_path, local_path) def push_file(self, local_path, remote_path): - self.instance.push_file(local_path, remote_path) + # First push to a temporary directory because of permissions issues + tmp_path = _get_tmp_path() + self.instance.push_file(local_path, tmp_path) + self.execute('mv {} {}'.format(tmp_path, remote_path)) def read_from_file(self, remote_path) -> str: - tmp_file = NamedTemporaryFile('r') - self.pull_file(remote_path, tmp_file.name) - with tmp_file as f: - contents = f.read() - return contents + result = self.execute('/bin/cat {}'.format(remote_path)) + if result.failed: + raise CalledProcessException( + 'Failed reading remote file via cat: {}\n' + 'Return code: {}\n' + 'Stderr: {}\n' + 'Stdout: {}'.format( + remote_path, result.return_code, + result.stderr, result.stdout) + ) + return result.stdout def write_to_file(self, remote_path, contents: str): # Writes file locally and then pushes it rather diff --git a/tests/integration_tests/modules/test_users_groups.py b/tests/integration_tests/modules/test_users_groups.py index 6a085a8fb4c..6a51f5a633a 100644 --- a/tests/integration_tests/modules/test_users_groups.py +++ b/tests/integration_tests/modules/test_users_groups.py @@ -70,7 +70,10 @@ class TestUsersGroups: def test_users_groups(self, regex, getent_args, class_client): """Use getent to interrogate the various expected outcomes""" result = class_client.execute(["getent"] + getent_args) - assert re.search(regex, result.stdout) is not None + assert re.search(regex, result.stdout) is not None, ( + "'getent {}' resulted in '{}', " + "but expected to match regex {}".format( + ' '.join(getent_args), result.stdout, regex)) def test_user_root_in_secret(self, class_client): """Test root user is in 'secret' group.""" diff --git a/tests/integration_tests/modules/test_write_files.py b/tests/integration_tests/modules/test_write_files.py index 15832ae3caf..e139d206fc2 100644 --- a/tests/integration_tests/modules/test_write_files.py +++ b/tests/integration_tests/modules/test_write_files.py @@ -51,8 +51,8 @@ class TestWriteFiles: @pytest.mark.parametrize( "cmd,expected_out", ( ("file /root/file_b64", ASCII_TEXT), - ("md5sum Date: Tue, 17 Nov 2020 08:19:14 -0600 Subject: [PATCH 2/8] take 2 --- tests/integration_tests/instances.py | 7 +------ tests/integration_tests/modules/test_write_files.py | 4 ++-- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/tests/integration_tests/instances.py b/tests/integration_tests/instances.py index bdc0018f1de..f49c1000c3f 100644 --- a/tests/integration_tests/instances.py +++ b/tests/integration_tests/instances.py @@ -44,12 +44,7 @@ def destroy(self): self.instance.delete() def execute(self, command) -> Result: - if self.use_sudo: - if isinstance(command, str): - command = 'sudo {}'.format(command) - elif isinstance(command, list): - command = ['sudo'] + command - return self.instance.execute(command) + return self.instance.execute(command, use_sudo=self.use_sudo) def pull_file(self, remote_path, local_path): # First copy to a temporary directory because of permissions issues diff --git a/tests/integration_tests/modules/test_write_files.py b/tests/integration_tests/modules/test_write_files.py index e139d206fc2..15832ae3caf 100644 --- a/tests/integration_tests/modules/test_write_files.py +++ b/tests/integration_tests/modules/test_write_files.py @@ -51,8 +51,8 @@ class TestWriteFiles: @pytest.mark.parametrize( "cmd,expected_out", ( ("file /root/file_b64", ASCII_TEXT), - ("md5sum /root/file_binary", "3801184b97bb8c6e63fa0e1eae2920d7"), - ("sha256sum /root/file_binary", ( + ("md5sum Date: Tue, 17 Nov 2020 13:24:19 -0600 Subject: [PATCH 3/8] [squash] review comments --- tests/integration_tests/instances.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/integration_tests/instances.py b/tests/integration_tests/instances.py index f49c1000c3f..8f403f9783a 100644 --- a/tests/integration_tests/instances.py +++ b/tests/integration_tests/instances.py @@ -21,10 +21,6 @@ log = logging.getLogger('integration_testing') -class CalledProcessException(Exception): - pass - - def _get_tmp_path(): tmp_filename = ''.join([random.choice( string.ascii_letters + string.digits) for _ in range(20)]) @@ -43,13 +39,15 @@ def __init__(self, cloud: 'IntegrationCloud', instance: BaseInstance, def destroy(self): self.instance.delete() - def execute(self, command) -> Result: - return self.instance.execute(command, use_sudo=self.use_sudo) + def execute(self, command, *, use_sudo=None) -> Result: + if use_sudo is None: + use_sudo = self.use_sudo + return self.instance.execute(command, use_sudo=use_sudo) def pull_file(self, remote_path, local_path): # First copy to a temporary directory because of permissions issues tmp_path = _get_tmp_path() - self.instance.execute('mv {} {}'.format(remote_path, tmp_path)) + self.instance.execute('cp {} {}'.format(remote_path, tmp_path)) self.instance.pull_file(tmp_path, local_path) def push_file(self, local_path, remote_path): @@ -61,7 +59,9 @@ def push_file(self, local_path, remote_path): def read_from_file(self, remote_path) -> str: result = self.execute('/bin/cat {}'.format(remote_path)) if result.failed: - raise CalledProcessException( + # TODO: Raise here whatever pycloudlib raises when it has + # a consistent error response + raise IOError( 'Failed reading remote file via cat: {}\n' 'Return code: {}\n' 'Stderr: {}\n' From dc3badb0cbf6e681706ee861132b0a267ae30c48 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 18 Nov 2020 13:34:03 -0600 Subject: [PATCH 4/8] [squash] Also bumping pycloudlib version --- integration-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-requirements.txt b/integration-requirements.txt index e8ddb648b7d..3648a0f16d2 100644 --- a/integration-requirements.txt +++ b/integration-requirements.txt @@ -1,5 +1,5 @@ # PyPI requirements for cloud-init integration testing # https://cloudinit.readthedocs.io/en/latest/topics/integration_tests.html # -pycloudlib @ git+https://github.com/canonical/pycloudlib.git@9211c0e5b34794595565d4626bc41ddbe14994f2 +pycloudlib @ git+https://github.com/canonical/pycloudlib.git@4b8d2cd5ac6316810ce16d081842da575625ca4f pytest From 5312058c09976f0f6098521d3b1bf9a85ab20462 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Thu, 19 Nov 2020 09:34:06 -0600 Subject: [PATCH 5/8] [squash] root use_sudo=False check --- tests/integration_tests/instances.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration_tests/instances.py b/tests/integration_tests/instances.py index 8f403f9783a..ee42da3a069 100644 --- a/tests/integration_tests/instances.py +++ b/tests/integration_tests/instances.py @@ -40,6 +40,8 @@ def destroy(self): self.instance.delete() def execute(self, command, *, use_sudo=None) -> Result: + if self.instance.username == 'root' and use_sudo is False: + raise Exception('Root user cannot run unprivileged') if use_sudo is None: use_sudo = self.use_sudo return self.instance.execute(command, use_sudo=use_sudo) From 44840f1d5c575ed6266a5fd5de0415e427add63a Mon Sep 17 00:00:00 2001 From: James Falcon Date: Fri, 20 Nov 2020 09:41:20 -0600 Subject: [PATCH 6/8] avoid collisions in tmpfile generation --- tests/integration_tests/instances.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/instances.py b/tests/integration_tests/instances.py index ee42da3a069..9e117078ea1 100644 --- a/tests/integration_tests/instances.py +++ b/tests/integration_tests/instances.py @@ -3,6 +3,7 @@ import os import random import string +import uuid from tempfile import NamedTemporaryFile from pycloudlib.instance import BaseInstance @@ -22,8 +23,7 @@ def _get_tmp_path(): - tmp_filename = ''.join([random.choice( - string.ascii_letters + string.digits) for _ in range(20)]) + tmp_filename = str(uuid.uuid4()) return '/var/tmp/{}.tmp'.format(tmp_filename) From 95e7fd850571a2a3d6d5cedfcdd0bfa0bf3a4b85 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Fri, 20 Nov 2020 09:53:05 -0600 Subject: [PATCH 7/8] Removing the path from cat --- tests/integration_tests/instances.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/instances.py b/tests/integration_tests/instances.py index 9e117078ea1..4fafebdca04 100644 --- a/tests/integration_tests/instances.py +++ b/tests/integration_tests/instances.py @@ -59,7 +59,7 @@ def push_file(self, local_path, remote_path): self.execute('mv {} {}'.format(tmp_path, remote_path)) def read_from_file(self, remote_path) -> str: - result = self.execute('/bin/cat {}'.format(remote_path)) + result = self.execute('cat {}'.format(remote_path)) if result.failed: # TODO: Raise here whatever pycloudlib raises when it has # a consistent error response From fa640f538c328d6e1d1ad298dbf51b28bb9885a1 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Mon, 23 Nov 2020 15:36:06 -0600 Subject: [PATCH 8/8] flake8 --- tests/integration_tests/instances.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/integration_tests/instances.py b/tests/integration_tests/instances.py index 4fafebdca04..9b13288c696 100644 --- a/tests/integration_tests/instances.py +++ b/tests/integration_tests/instances.py @@ -1,8 +1,6 @@ # This file is part of cloud-init. See LICENSE file for license information. import logging import os -import random -import string import uuid from tempfile import NamedTemporaryFile