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
Update tests to run commands on LXD as non root #694
Update tests to run commands on LXD as non root #694
Conversation
efb84ce
to
dda0743
Compare
4474fd5
to
7e80caf
Compare
assert ( | ||
"XAAAAAtzc2gtZWQyNTUxOQAAACDbnQGUruL42aVVsyHeaV5mYNT" | ||
"OhteXao0Nl5DVThJ2+Q") in out | ||
@pytest.mark.parametrize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice parametrize use here
@@ -33,14 +33,18 @@ class TestSshAuthkeyFingerprints: | |||
|
|||
@pytest.mark.user_data(USER_DATA_SSH_AUTHKEY_DISABLE) | |||
def test_ssh_authkey_fingerprints_disable(self, client): | |||
cloudinit_output = client.read_from_file("/var/log/cloud-init.log") | |||
cloudinit_output = client.read_from_file( | |||
"/var/log/cloud-init.log", use_sudo=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need use_sudo here, /var/log/cloud-init.log is world readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good Lucas. I think we determined a couple PRs ago through @TheRealFalcon branch that integration tests will work toward a common non-root/ssh only approach to all clouds at the moment for simplicity. If we need to specfically grow/tailor an LXD exec root-only approach to driving all cloud platforms, then we can grow that in pycloudlib and reflect some of those changes into cloud-init integration tests in the future. I'll leave you to review and discuss a couple of the comments (no need for use_sudo in a couple places) and will check back on this.
) | ||
) | ||
def test_ssh_provided_info(self, config_path, expected_out, class_client): | ||
out = class_client.read_from_file(config_path, use_sudo=True).strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't think use_sudo=True is needed here either.
out = class_client.read_from_file(config_path, use_sudo=True).strip() | |
out = class_client.read_from_file(config_path).strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do need the use_sudo
for some of these operations. Running it without we get errors like:
cat: /etc/ssh/ssh_host_ed25519_key: Permission denied
cat: /etc/ssh/ssh_host_ed25519_key.pub: Permission denied
cat: /etc/ssh/ssh_host_ecdsa_key: Permission denied
But I do agree that the sshd
file can be read without sudo. I think for it, I can remove it from the parametrize matrix
sshd_config = class_client.read_from_file( | ||
"/etc/ssh/sshd_config", use_sudo=True | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for use_sudo here: ls -l /etc/ssh/sshd_config world readable -rw-r--r--
sshd_config = class_client.read_from_file( | |
"/etc/ssh/sshd_config", use_sudo=True | |
) | |
sshd_config = class_client.read_from_file("/etc/ssh/sshd_config") |
tests/integration_tests/instances.py
Outdated
'{sudo} apt-get update -q && ' | ||
'{sudo} apt-get install -qy cloud-init' | ||
).format(sudo='sudo' if self.use_sudo else '', repo=repo) | ||
'sudo add-apt-repository {repo} -y && ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think this should always be sudo for this operation, the only subclass of IntegrationInstance which sets use_sudo = False is Lxd, if we are saying for this iteration that we want to keep all cloud platforms using non-root user for most commands, then I'd say this is a hardcode "sudo" command. I'd also say the same with the related install_deb and install_proposed_image. I'll let you guys chat about this tomorrow. But, wouldn't this branch be the point where we drop the self.use_sudo stuff from all clouds, and just assume yes on all clouds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right here. My assumption was that some commands would not need to use sudo by default and we should avoid that. However, the other clouds do not follow this pattern. I think it is better to drop use_sudo
as mentioned.
I will update the code
178acac
to
67217c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this Lucas! I have a few inline comments; the most important is the first in test_set_password.py
.
tests/integration_tests/conftest.py
Outdated
@@ -105,7 +105,9 @@ def setup_image(session_cloud): | |||
client.install_proposed_image() | |||
elif integration_settings.CLOUD_INIT_SOURCE.startswith('ppa:'): | |||
client = session_cloud.launch() | |||
client.install_ppa(integration_settings.CLOUD_INIT_SOURCE) | |||
client.install_ppa( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few places where we aren't changing code but are making it take up more space (I think from a previous iteration of this proposal?). Can we clean these up?
tests/integration_tests/instances.py
Outdated
if use_sudo is None: | ||
use_sudo = self.use_sudo | ||
use_sudo = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this if
clause is only present because the default was dependent on the cloud previously. Now that the default is True
everywhere, I think we can just have it in the method signature.
tests/integration_tests/instances.py
Outdated
@@ -56,8 +54,8 @@ def push_file(self, local_path, remote_path): | |||
self.instance.push_file(str(local_path), tmp_path) | |||
self.execute('mv {} {}'.format(tmp_path, str(remote_path))) | |||
|
|||
def read_from_file(self, remote_path) -> str: | |||
result = self.execute('cat {}'.format(remote_path)) | |||
def read_from_file(self, remote_path, use_sudo=None) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing passes use_sudo
to read_from_file
, so I think we can drop this change until we need it.
tests/integration_tests/instances.py
Outdated
remote_script = 'dpkg -i {path}'.format( | ||
path=remote_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remote_script = 'dpkg -i {path}'.format( | |
path=remote_path) | |
remote_script = 'dpkg -i {}'.format(remote_path) |
@@ -22,6 +22,7 @@ | |||
- name: tom | |||
# md5 gotomgo | |||
passwd: "$1$S7$tT1BEDIYrczeryDQJfdPe0" | |||
sudo: ALL=(ALL) NOPASSWD:ALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this actually revealing a bug in the planned implementation: we assume that the default user (with passwordless sudo) will be created as UID 1000. With this user-data, though, tom
is UID 1000. To maintain the interface with other platforms (we access the instance as ubuntu
and use sudo
to gain UID 0), I think pycloudlib
will need to look up the UID of self.username
before executing commands.
(As an aside: examining the cc_users_groups
code, there is no particular guarantee as to what order users will be created in: it depends on dict iteration behaviour. This behaviour is not guaranteed to be order of insertion before Python 3.7 (and isn't order of insertion before CPython 3.6). So we're likely to find that tom
will not be UID 1000 some of the time on xenial. On 3.6+, as we remove the default user and re-add it, I believe the default user is guaranteed to be the last user created.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I honestly thought about that issue, but I assumed that we would always have a non-root user as 1000 to use. But you are totally correct that this assumption is not good and that we should be consistent on using only the ubuntu user to run the commands across different instance types. I will update the pycloudlib code to address that
out = class_client.execute( | ||
"test -e {}".format(ssh_key_path) | ||
) | ||
out = class_client.execute("test -e {}".format(ssh_key_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is removing needless lines, let's keep this one.
67217c4
to
2943156
Compare
3b7f30b
to
6d902ff
Compare
pycloudlib will stop running commands as root by default on LXD. To align with that change and make the behavior consistent with other clouds we support, our LXD instances will now run the commands with sudo by default.
6d902ff
to
1712c82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the iteration!
Chad is OOO and the proposal has changed substantially since he reviewed it
For us to support running commands through LXD as non-root, as described in this PR, we need to update some our integration tests for this new scenario