-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Ensure proper root permissions in integration tests #664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f78b1b3
d23db0a
5c08bf0
dc3badb
5312058
44840f1
95e7fd8
fa640f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| # This file is part of cloud-init. See LICENSE file for license information. | ||
| import logging | ||
| import os | ||
| import uuid | ||
| from tempfile import NamedTemporaryFile | ||
|
|
||
| from pycloudlib.instance import BaseInstance | ||
| from pycloudlib.result import Result | ||
|
|
||
| from tests.integration_tests import integration_settings | ||
|
|
||
|
|
@@ -18,6 +20,11 @@ | |
| log = logging.getLogger('integration_testing') | ||
|
|
||
|
|
||
| def _get_tmp_path(): | ||
| tmp_filename = str(uuid.uuid4()) | ||
| return '/var/tmp/{}.tmp'.format(tmp_filename) | ||
|
|
||
|
|
||
| class IntegrationInstance: | ||
| use_sudo = True | ||
|
|
||
|
|
@@ -30,21 +37,39 @@ def __init__(self, cloud: 'IntegrationCloud', instance: BaseInstance, | |
| def destroy(self): | ||
| self.instance.delete() | ||
|
|
||
| def execute(self, command): | ||
| return self.instance.execute(command) | ||
| def execute(self, command, *, use_sudo=None) -> Result: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is '*' for ? you dn't use it anywhere.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Designating keyword only arguments. https://www.python.org/dev/peps/pep-3102/ instance.execute('cat stuff', True)You have to do instance.execute('cat stuff', use_sudo=True) |
||
| 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) | ||
|
|
||
| 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('cp {} {}'.format(remote_path, tmp_path)) | ||
| self.instance.pull_file(tmp_path, local_path) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're not cleaning up the temp file on the other side. And it seems like if 'pull_file' is broken, then you should fix it in pycloudlib rather than working around with a temp file. Now, instead of just using 'pull_file', you'll have:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed my tmp file to use a uuid instead so there's no possibility of collisions. I don't think I care about cleaning up the remote end. These are relatively short-lived tests, so I see no harm and it saves us an ssh call. |
||
|
|
||
| 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)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps 'push_file' needs perms and 'owner' arguments ? using 'mv' like this will get you some permissions, but it seems arbitrary. What permissions and ownership should the user expect after calling push_file ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the conclusion Dan and I came to is, if somebody needs specific permissions they can implement it. |
||
|
|
||
| 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('cat {}'.format(remote_path)) | ||
| if result.failed: | ||
| # 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' | ||
| '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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, ( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed some additional debug output for this failure
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that |
||
| "'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.""" | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.