Skip to content

lxd/instance: use SSH by default, allow switching to exec #114

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.

Sign up for GitHub

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

Merged
OddBloke merged 2 commits into from
Feb 10, 2021
Merged

lxd/instance: use SSH by default, allow switching to exec #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee0742b
test_instance: move LXD-only test to LXD-specific test path
OddBloke Feb 9, 2021
64b0321
lxd/instance: use SSH by default, allow switching to `exec`
OddBloke Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ matrix:
include:
- name: run-lxd-example
install:
# Generate an SSH key to connect to test instances
- ssh-keygen -N "" -f ~/.ssh/id_rsa
- pip install -r requirements.txt
script:
- sudo apt-get remove --yes --purge lxd lxd-client
Expand All @@ -22,6 +24,8 @@ matrix:
- sg lxd -c "python examples/lxd.py"
- name: run-cloudinit-integration-tests
install:
# Generate an SSH key to connect to test instances
- ssh-keygen -N "" -f ~/.ssh/id_rsa
- pip install -r requirements.txt
- pip install pytest
script:
Expand Down
44 changes: 13 additions & 31 deletions pycloudlib/lxd/cloud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,6 @@ class _BaseLXD(BaseCloud):
_daily_remote = 'ubuntu-daily'
_releases_remote = 'ubuntu'

def __init__(self, tag, timestamp_suffix=True):
"""Initialize LXD cloud class.

Args:
tag: string used to name and tag resources with
timestamp_suffic: Append a timestamped suffix to the tag string.
"""
super().__init__(tag, timestamp_suffix)

# User must manually specify the key pair to be used
self.key_pair = None

def clone(self, base, new_instance_name):
"""Create copy of an existing instance or snapshot.

Expand Down Expand Up @@ -121,21 +109,7 @@ def get_instance(self, instance_id):
The existing instance as a LXD instance object

"""
instance = LXDInstance(instance_id)

if self.key_pair:
local_path = "/tmp/{}-authorized-keys".format(instance_id)

instance.pull_file(
remote_path="/home/ubuntu/.ssh/authorized_keys",
local_path=local_path
)

with open(local_path, "r") as f:
Copy link
Copy Markdown

@lucasmoura lucasmoura Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this code is not optimal at all, but the reason I have added it here is that we could change the ssh keys here, but still have launched instances that are using an old key. In that situation, we would provided an unusable instance.

However, I know that this is not optimal because, for example, if we can't exec into the machine, that pull_file call will not work. So maybe we can drop this check and live with this possibility.

But if you think there is a smarter way to handle that, no problem at all

Copy link
Copy Markdown
Contributor Author

@OddBloke OddBloke Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment, this is the code I was hoping to discuss! You're correct, the issue I found is that we don't necessarily have exec access when this runs (and, indeed, we definitely don't when this runs in the demo scripts, which was causing CI failure). Adding an instance._wait_for_execute() call in get_instance doesn't seem like the right thing to do: if the instance isn't expected to be accessible, that would cause a long delay.

I think this logic is less necessary than it was previously, however, because instance.key_pair no longer determines our execute behaviour: even if instance.key_pair is None, we will still attempt to SSH in (using any keys available to paramiko).

However, I don't think we can say it's entirely unnecessary: while we don't need it to determine the SSH key to use (and, actually, never did because paramiko will try all keys by default), it did serve a purpose previously: if SSH execution could work (determined via exec), the LXDInstance would be configured to execute via SSH instead of via exec, otherwise it would be configured to use exec. We lose that behaviour here: we will always return a LXDInstance configured to execute via SSH.

cloud-init's tests only have a single place which uses get_instance (when CLOUD_INIT_EXISTING_INSTANCE_ID is set). It actually has a similar issue to the one we're trying to solve here: previously with a local SSH key configured, it would have always configured SSH access (even to instances for which SSH wouldn't work: the existence of an authorized key does not indicate, e.g., working networking), and would otherwise have always configured exec access. I need to update canonical/cloud-init#802 to handle things differently: we should set instance.execute_via_ssh after the instance has been instantiated. We would still not require the "execution method auto-detection" which is being removed here.

Does UA client have test code which would be affected by this removal? Or can we safely remove it (and modify the docstring here to explain the execution behaviour)?

Copy link
Copy Markdown

@lucasmoura lucasmoura Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not known about the fact that paramiko would try to access the machine with all of the keys that is has seen. If that is the case, I don't see any problem removing the code and it should not affect uaclient at all.

Thanks for all the provided context @OddBloke :)

if self.key_pair.public_key_content in f.read():
instance.key_pair = self.key_pair

return instance
return LXDInstance(instance_id, key_pair=self.key_pair)

def _normalize_image_id(self, image_id: str) -> str:
if ':' not in image_id:
Expand Down Expand Up @@ -228,7 +202,7 @@ def _prepare_command(
def init(
self, name, image_id, ephemeral=False, network=None, storage=None,
inst_type=None, profile_list=None, user_data=None,
config_dict=None):
config_dict=None, execute_via_ssh=True):
"""Init a container.

This will initialize a container, but not launch or start it.
Expand All @@ -245,6 +219,8 @@ def init(
profile_list: list, optional, profile(s) to use
user_data: used by cloud-init to run custom scripts/configuration
config_dict: dict, optional, configuration values to pass
execute_via_ssh: bool, optional, execute commands on the instance
via SSH if True (the default)

Returns:
The created LXD instance object
Expand Down Expand Up @@ -272,11 +248,14 @@ def init(

self._log.debug('Created %s', name)

return LXDInstance(name, self.key_pair)
return LXDInstance(
name, self.key_pair, execute_via_ssh=execute_via_ssh
)

def launch(self, image_id, instance_type=None, user_data=None, wait=True,
name=None, ephemeral=False, network=None, storage=None,
profile_list=None, config_dict=None, **kwargs):
profile_list=None, config_dict=None, execute_via_ssh=True,
**kwargs):
"""Set up and launch a container.

This will init and start a container with the provided settings.
Expand All @@ -293,6 +272,8 @@ def launch(self, image_id, instance_type=None, user_data=None, wait=True,
storage: string, storage name to use
profile_list: list, profile(s) to use
config_dict: dict, configuration values to pass
execute_via_ssh: bool, optional, execute commands on the instance
via SSH if True (the default)

Returns:
The created LXD instance object
Expand All @@ -307,7 +288,8 @@ def launch(self, image_id, instance_type=None, user_data=None, wait=True,
inst_type=instance_type,
profile_list=profile_list,
user_data=user_data,
config_dict=config_dict
config_dict=config_dict,
execute_via_ssh=execute_via_ssh,
)
instance.start(wait)

Expand Down
5 changes: 3 additions & 2 deletions pycloudlib/lxd/instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class LXDInstance(BaseInstance):
_type = 'lxd'
_is_vm = None

def __init__(self, name, key_pair=None):
def __init__(self, name, key_pair=None, execute_via_ssh=True):
"""Set up instance.

Args:
Expand All @@ -23,14 +23,15 @@ def __init__(self, name, key_pair=None):
super().__init__(key_pair=key_pair)

self._name = name
self.execute_via_ssh = execute_via_ssh

def __repr__(self):
"""Create string representation for class."""
return 'LXDInstance(name={})'.format(self.name)

def _run_command(self, command, stdin):
"""Run command in the instance."""
if self.key_pair:
if self.execute_via_ssh:
return super()._run_command(command, stdin)

base_cmd = [
Expand Down
22 changes: 22 additions & 0 deletions pycloudlib/lxd/tests/test_instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
"""Tests for pycloudlib.lxd.instance."""
from unittest import mock

from pycloudlib.lxd.instance import LXDInstance


class TestExecute:
"""Tests covering pycloudlib.lxd.instance.Instance.execute."""

def test_all_rcs_acceptable_when_using_exec(self):
"""Test that we invoke util.subp with rcs=None for exec calls.

rcs=None means that we will get a Result object back for all return
codes, rather than an exception for non-zero return codes.
"""
instance = LXDInstance(None, execute_via_ssh=False)
with mock.patch("pycloudlib.lxd.instance.subp") as m_subp:
instance.execute("some_command")
assert 1 == m_subp.call_count
args, kwargs = m_subp.call_args
assert "exec" in args[0]
assert kwargs.get("rcs", mock.sentinel.not_none) is None
31 changes: 0 additions & 31 deletions pycloudlib/tests/test_instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import pytest

from pycloudlib.instance import BaseInstance
from pycloudlib.lxd.instance import LXDInstance
from pycloudlib.result import Result

# Disable this pylint check as fixture usage incorrectly triggers it:
Expand All @@ -21,36 +20,6 @@ def concrete_instance_cls():
yield BaseInstance


class TestExecute:
"""Tests covering pycloudlib.instance.Instance.execute.

TODO: There are elements of `execute` which could be refactored onto the
relevant subclasses. Some of these tests should move along with that
refactor.
"""

@pytest.mark.parametrize(
"instance_cls,cloud_name",
(
(LXDInstance, "lxd"),
),
)
def test_all_rcs_acceptable(self, instance_cls, cloud_name):
"""Test that we invoke util.subp with rcs=None.

rcs=None means that we will get a Result object back for all return
codes, rather than an exception for non-zero return codes.
"""
instance = instance_cls(None)
with mock.patch(
"pycloudlib.{}.instance.subp".format(cloud_name)
) as m_subp:
instance.execute("some_command")
assert 1 == m_subp.call_count
_args, kwargs = m_subp.call_args
assert kwargs.get("rcs", mock.sentinel.not_none) is None


class TestWait:
"""Tests covering pycloudlib.instance.Instance.wait."""

Expand Down