From ce8d431fad81547d1e76f264a172d27e2b4fccb1 Mon Sep 17 00:00:00 2001
From: Larry Dewey <ldewey@suse.com>
Date: Tue, 7 Jan 2020 02:48:11 -0700
Subject: [PATCH] virt: adding kernel boot parameters to libvirt xml
 #55245 (#197)

* virt: adding kernel boot parameters to libvirt xml

SUSE's autoyast and Red Hat's kickstart take advantage of kernel paths,
initrd paths, and kernel boot command line parameters. These changes
provide the option of using these, and will allow salt and
autoyast/kickstart to work together.

Signed-off-by: Larry Dewey <ldewey@suse.com>

* virt: Download linux and initrd

Signed-off-by: Larry Dewey <ldewey@suse.com>
---
 salt/modules/virt.py                     | 129 ++++++++++++++++++++++-
 salt/states/virt.py                      |  29 ++++-
 salt/templates/virt/libvirt_domain.jinja |  12 ++-
 salt/utils/virt.py                       |  45 +++++++-
 tests/unit/modules/test_virt.py          |  79 +++++++++++++-
 tests/unit/states/test_virt.py           |  19 +++-
 6 files changed, 302 insertions(+), 11 deletions(-)

diff --git a/salt/modules/virt.py b/salt/modules/virt.py
index dedcf8cb6f..0f62856f5c 100644
--- a/salt/modules/virt.py
+++ b/salt/modules/virt.py
@@ -106,6 +106,8 @@ import salt.utils.templates
 import salt.utils.validate.net
 import salt.utils.versions
 import salt.utils.yaml
+
+from salt.utils.virt import check_remote, download_remote
 from salt.exceptions import CommandExecutionError, SaltInvocationError
 from salt.ext import six
 from salt.ext.six.moves import range  # pylint: disable=import-error,redefined-builtin
@@ -119,6 +121,8 @@ JINJA = jinja2.Environment(
     )
 )
 
+CACHE_DIR = '/var/lib/libvirt/saltinst'
+
 VIRT_STATE_NAME_MAP = {0: 'running',
                        1: 'running',
                        2: 'running',
@@ -532,6 +536,7 @@ def _gen_xml(name,
              os_type,
              arch,
              graphics=None,
+             boot=None,
              **kwargs):
     '''
     Generate the XML string to define a libvirt VM
@@ -568,11 +573,15 @@ def _gen_xml(name,
     else:
         context['boot_dev'] = ['hd']
 
+    context['boot'] = boot if boot else {}
+
     if os_type == 'xen':
         # Compute the Xen PV boot method
         if __grains__['os_family'] == 'Suse':
-            context['kernel'] = '/usr/lib/grub2/x86_64-xen/grub.xen'
-            context['boot_dev'] = []
+            if not boot or not boot.get('kernel', None):
+                context['boot']['kernel'] = \
+                        '/usr/lib/grub2/x86_64-xen/grub.xen'
+                context['boot_dev'] = []
 
     if 'serial_type' in kwargs:
         context['serial_type'] = kwargs['serial_type']
@@ -1115,6 +1124,34 @@ def _get_merged_nics(hypervisor, profile, interfaces=None, dmac=None):
     return nicp
 
 
+def _handle_remote_boot_params(orig_boot):
+    """
+    Checks if the boot parameters contain a remote path. If so, it will copy
+    the parameters, download the files specified in the remote path, and return
+    a new dictionary with updated paths containing the canonical path to the
+    kernel and/or initrd
+
+    :param orig_boot: The original boot parameters passed to the init or update
+    functions.
+    """
+    saltinst_dir = None
+    new_boot = orig_boot.copy()
+
+    try:
+        for key in ['kernel', 'initrd']:
+            if check_remote(orig_boot.get(key)):
+                if saltinst_dir is None:
+                    os.makedirs(CACHE_DIR)
+                    saltinst_dir = CACHE_DIR
+
+                new_boot[key] = download_remote(orig_boot.get(key),
+                                                saltinst_dir)
+
+        return new_boot
+    except Exception as err:
+        raise err
+
+
 def init(name,
          cpu,
          mem,
@@ -1136,6 +1173,7 @@ def init(name,
          graphics=None,
          os_type=None,
          arch=None,
+         boot=None,
          **kwargs):
     '''
     Initialize a new vm
@@ -1266,6 +1304,22 @@ def init(name,
     :param password: password to connect with, overriding defaults
 
                      .. versionadded:: 2019.2.0
+    :param boot:
+        Specifies kernel for the virtual machine, as well as boot parameters
+        for the virtual machine. This is an optionl parameter, and all of the
+        keys are optional within the dictionary. If a remote path is provided
+        to kernel or initrd, salt will handle the downloading of the specified
+        remote fild, and will modify the XML accordingly.
+
+        .. code-block:: python
+
+            {
+                'kernel': '/root/f8-i386-vmlinuz',
+                'initrd': '/root/f8-i386-initrd',
+                'cmdline': 'console=ttyS0 ks=http://example.com/f8-i386/os/'
+            }
+
+        .. versionadded:: neon
 
     .. _init-nic-def:
 
@@ -1513,7 +1567,11 @@ def init(name,
     if arch is None:
         arch = 'x86_64' if 'x86_64' in arches else arches[0]
 
-    vm_xml = _gen_xml(name, cpu, mem, diskp, nicp, hypervisor, os_type, arch, graphics, **kwargs)
+    if boot is not None:
+        boot = _handle_remote_boot_params(boot)
+
+    vm_xml = _gen_xml(name, cpu, mem, diskp, nicp, hypervisor, os_type, arch,
+                      graphics, boot, **kwargs)
     conn = __get_conn(**kwargs)
     try:
         conn.defineXML(vm_xml)
@@ -1692,6 +1750,7 @@ def update(name,
            interfaces=None,
            graphics=None,
            live=True,
+           boot=None,
            **kwargs):
     '''
     Update the definition of an existing domain.
@@ -1727,6 +1786,23 @@ def update(name,
     :param username: username to connect with, overriding defaults
     :param password: password to connect with, overriding defaults
 
+    :param boot:
+        Specifies kernel for the virtual machine, as well as boot parameters
+        for the virtual machine. This is an optionl parameter, and all of the
+        keys are optional within the dictionary. If a remote path is provided
+        to kernel or initrd, salt will handle the downloading of the specified
+        remote fild, and will modify the XML accordingly.
+
+        .. code-block:: python
+
+            {
+                'kernel': '/root/f8-i386-vmlinuz',
+                'initrd': '/root/f8-i386-initrd',
+                'cmdline': 'console=ttyS0 ks=http://example.com/f8-i386/os/'
+            }
+
+        .. versionadded:: neon
+
     :return:
 
         Returns a dictionary indicating the status of what has been done. It is structured in
@@ -1767,6 +1843,10 @@ def update(name,
     # Compute the XML to get the disks, interfaces and graphics
     hypervisor = desc.get('type')
     all_disks = _disk_profile(disk_profile, hypervisor, disks, name, **kwargs)
+
+    if boot is not None:
+        boot = _handle_remote_boot_params(boot)
+
     new_desc = ElementTree.fromstring(_gen_xml(name,
                                                cpu,
                                                mem,
@@ -1776,6 +1856,7 @@ def update(name,
                                                domain.OSType(),
                                                desc.find('.//os/type').get('arch'),
                                                graphics,
+                                               boot,
                                                **kwargs))
 
     # Update the cpu
@@ -1785,6 +1866,48 @@ def update(name,
         cpu_node.set('current', six.text_type(cpu))
         need_update = True
 
+    # Update the kernel boot parameters
+    boot_tags = ['kernel', 'initrd', 'cmdline']
+    parent_tag = desc.find('os')
+
+    # We need to search for each possible subelement, and update it.
+    for tag in boot_tags:
+        # The Existing Tag...
+        found_tag = desc.find(tag)
+
+        # The new value
+        boot_tag_value = boot.get(tag, None) if boot else None
+
+        # Existing tag is found and values don't match
+        if found_tag and found_tag.text != boot_tag_value:
+
+            # If the existing tag is found, but the new value is None
+            # remove it. If the existing tag is found, and the new value
+            # doesn't match update it. In either case, mark for update.
+            if boot_tag_value is None \
+               and boot is not None   \
+               and parent_tag is not None:
+                ElementTree.remove(parent_tag, tag)
+            else:
+                found_tag.text = boot_tag_value
+
+            need_update = True
+
+        # Existing tag is not found, but value is not None
+        elif found_tag is None and boot_tag_value is not None:
+
+            # Need to check for parent tag, and add it if it does not exist.
+            # Add a subelement and set the value to the new value, and then
+            # mark for update.
+            if parent_tag is not None:
+                child_tag = ElementTree.SubElement(parent_tag, tag)
+            else:
+                new_parent_tag = ElementTree.Element('os')
+                child_tag = ElementTree.SubElement(new_parent_tag, tag)
+
+            child_tag.text = boot_tag_value
+            need_update = True
+
     # Update the memory, note that libvirt outputs all memory sizes in KiB
     for mem_node_name in ['memory', 'currentMemory']:
         mem_node = desc.find(mem_node_name)
diff --git a/salt/states/virt.py b/salt/states/virt.py
index 68e9ac6fb6..c700cae849 100644
--- a/salt/states/virt.py
+++ b/salt/states/virt.py
@@ -264,7 +264,8 @@ def running(name,
             username=None,
             password=None,
             os_type=None,
-            arch=None):
+            arch=None,
+            boot=None):
     '''
     Starts an existing guest, or defines and starts a new VM with specified arguments.
 
@@ -349,6 +350,23 @@ def running(name,
 
         .. versionadded:: Neon
 
+    :param boot:
+        Specifies kernel for the virtual machine, as well as boot parameters
+        for the virtual machine. This is an optionl parameter, and all of the
+        keys are optional within the dictionary. If a remote path is provided
+        to kernel or initrd, salt will handle the downloading of the specified
+        remote fild, and will modify the XML accordingly.
+
+        .. code-block:: python
+
+            {
+                'kernel': '/root/f8-i386-vmlinuz',
+                'initrd': '/root/f8-i386-initrd',
+                'cmdline': 'console=ttyS0 ks=http://example.com/f8-i386/os/'
+            }
+
+        .. versionadded:: neon
+
     .. rubric:: Example States
 
     Make sure an already-defined virtual machine called ``domain_name`` is running:
@@ -413,7 +431,8 @@ def running(name,
                                                      live=False,
                                                      connection=connection,
                                                      username=username,
-                                                     password=password)
+                                                     password=password,
+                                                     boot=boot)
                     if status['definition']:
                         action_msg = 'updated and started'
                 __salt__['virt.start'](name)
@@ -431,7 +450,8 @@ def running(name,
                                                      graphics=graphics,
                                                      connection=connection,
                                                      username=username,
-                                                     password=password)
+                                                     password=password,
+                                                     boot=boot)
                     ret['changes'][name] = status
                     if status.get('errors', None):
                         ret['comment'] = 'Domain {0} updated, but some live update(s) failed'.format(name)
@@ -466,7 +486,8 @@ def running(name,
                                   priv_key=priv_key,
                                   connection=connection,
                                   username=username,
-                                  password=password)
+                                  password=password,
+                                  boot=boot)
             ret['changes'][name] = 'Domain defined and started'
             ret['comment'] = 'Domain {0} defined and started'.format(name)
     except libvirt.libvirtError as err:
diff --git a/salt/templates/virt/libvirt_domain.jinja b/salt/templates/virt/libvirt_domain.jinja
index 0b4c3fc2d6..fdaea168f2 100644
--- a/salt/templates/virt/libvirt_domain.jinja
+++ b/salt/templates/virt/libvirt_domain.jinja
@@ -5,7 +5,17 @@
         <currentMemory unit='KiB'>{{ mem }}</currentMemory>
         <os>
                 <type arch='{{ arch }}'>{{ os_type }}</type>
-                {% if kernel %}<kernel>{{ kernel }}</kernel>{% endif %}
+                {% if boot %}
+                  {% if 'kernel' in boot %}
+                    <kernel>{{ boot.kernel }}</kernel>
+                  {% endif %}
+                  {% if 'initrd' in boot %}
+                    <initrd>{{ boot.initrd }}</initrd>
+                  {% endif %}
+                  {% if 'cmdline' in boot %}
+                    <cmdline>{{ boot.cmdline }}</cmdline>
+                  {% endif %}
+                {% endif %}
                 {% for dev in boot_dev %}
                 <boot dev='{{ dev }}' />
                 {% endfor %}
diff --git a/salt/utils/virt.py b/salt/utils/virt.py
index 9dad849c0e..b36adba81c 100644
--- a/salt/utils/virt.py
+++ b/salt/utils/virt.py
@@ -6,16 +6,59 @@ from __future__ import absolute_import, print_function, unicode_literals
 
 # Import python libs
 import os
+import re
 import time
 import logging
+import hashlib
+
+# pylint: disable=E0611
+from salt.ext.six.moves.urllib.parse import urlparse
+from salt.ext.six.moves.urllib import request
 
 # Import salt libs
 import salt.utils.files
 
-
 log = logging.getLogger(__name__)
 
 
+def download_remote(url, dir):
+    """
+    Attempts to download a file specified by 'url'
+
+    :param url: The full remote path of the file which should be downloaded.
+    :param dir: The path the file should be downloaded to.
+    """
+
+    try:
+        rand = hashlib.md5(os.urandom(32)).hexdigest()
+        remote_filename = urlparse(url).path.split('/')[-1]
+        full_directory = \
+            os.path.join(dir, "{}-{}".format(rand, remote_filename))
+        with salt.utils.files.fopen(full_directory, 'wb') as file,\
+                request.urlopen(url) as response:
+            file.write(response.rease())
+
+        return full_directory
+
+    except Exception as err:
+        raise err
+
+
+def check_remote(cmdline_path):
+    """
+    Checks to see if the path provided contains ftp, http, or https. Returns
+    the full path if it is found.
+
+    :param cmdline_path: The path to the initrd image or the kernel
+    """
+    regex = re.compile('^(ht|f)tps?\\b')
+
+    if regex.match(urlparse(cmdline_path).scheme):
+        return True
+
+    return False
+
+
 class VirtKey(object):
     '''
     Used to manage key signing requests.
diff --git a/tests/unit/modules/test_virt.py b/tests/unit/modules/test_virt.py
index 6f594a8ff3..4bdb933a2d 100644
--- a/tests/unit/modules/test_virt.py
+++ b/tests/unit/modules/test_virt.py
@@ -10,6 +10,7 @@ from __future__ import absolute_import, print_function, unicode_literals
 import os
 import re
 import datetime
+import shutil
 
 # Import Salt Testing libs
 from tests.support.mixins import LoaderModuleMockMixin
@@ -23,6 +24,7 @@ import salt.modules.config as config
 from salt._compat import ElementTree as ET
 import salt.config
 import salt.syspaths
+import tempfile
 from salt.exceptions import CommandExecutionError
 
 # Import third party libs
@@ -30,7 +32,6 @@ from salt.ext import six
 # pylint: disable=import-error
 from salt.ext.six.moves import range  # pylint: disable=redefined-builtin
 
-
 # pylint: disable=invalid-name,protected-access,attribute-defined-outside-init,too-many-public-methods,unused-argument
 
 
@@ -610,6 +611,7 @@ class VirtTestCase(TestCase, LoaderModuleMockMixin):
                 'xen',
                 'xen',
                 'x86_64',
+                boot=None
                 )
             root = ET.fromstring(xml_data)
             self.assertEqual(root.attrib['type'], 'xen')
@@ -1123,6 +1125,67 @@ class VirtTestCase(TestCase, LoaderModuleMockMixin):
                 self.assertFalse('<interface' in definition)
                 self.assertFalse('<disk' in definition)
 
+                # Ensure the init() function allows creating VM without NIC and
+                # disk but with boot parameters.
+
+                defineMock.reset_mock()
+                mock_run.reset_mock()
+                boot = {
+                    'kernel': '/root/f8-i386-vmlinuz',
+                    'initrd': '/root/f8-i386-initrd',
+                    'cmdline':
+                        'console=ttyS0 ks=http://example.com/f8-i386/os/'
+                }
+                retval = virt.init('test vm boot params',
+                                   2,
+                                   1234,
+                                   nic=None,
+                                   disk=None,
+                                   seed=False,
+                                   start=False,
+                                   boot=boot)
+                definition = defineMock.call_args_list[0][0][0]
+                self.assertEqual('<kernel' in definition, True)
+                self.assertEqual('<initrd' in definition, True)
+                self.assertEqual('<cmdline' in definition, True)
+                self.assertEqual(retval, True)
+
+                # Verify that remote paths are downloaded and the xml has been
+                # modified
+                mock_response = MagicMock()
+                mock_response.read = MagicMock(return_value='filecontent')
+                cache_dir = tempfile.mkdtemp()
+
+                with patch.dict(virt.__dict__, {'CACHE_DIR': cache_dir}):
+                    with patch('salt.ext.six.moves.urllib.request.urlopen',
+                               MagicMock(return_value=mock_response)):
+                        with patch('salt.utils.files.fopen',
+                                   return_value=mock_response):
+
+                            defineMock.reset_mock()
+                            mock_run.reset_mock()
+                            boot = {
+                                'kernel':
+                                    'https://www.example.com/download/vmlinuz',
+                                'initrd': '',
+                                'cmdline':
+                                    'console=ttyS0 '
+                                    'ks=http://example.com/f8-i386/os/'
+                            }
+
+                            retval = virt.init('test remote vm boot params',
+                                               2,
+                                               1234,
+                                               nic=None,
+                                               disk=None,
+                                               seed=False,
+                                               start=False,
+                                               boot=boot)
+                            definition = defineMock.call_args_list[0][0][0]
+                            self.assertEqual(cache_dir in definition, True)
+
+                    shutil.rmtree(cache_dir)
+
                 # Test case creating disks
                 defineMock.reset_mock()
                 mock_run.reset_mock()
@@ -1222,6 +1285,20 @@ class VirtTestCase(TestCase, LoaderModuleMockMixin):
         self.assertEqual(setxml.find('vcpu').text, '2')
         self.assertEqual(setvcpus_mock.call_args[0][0], 2)
 
+        boot = {
+            'kernel': '/root/f8-i386-vmlinuz',
+            'initrd': '/root/f8-i386-initrd',
+            'cmdline':
+                'console=ttyS0 ks=http://example.com/f8-i386/os/'
+        }
+
+        # Update with boot parameter case
+        self.assertEqual({
+                'definition': True,
+                'disk': {'attached': [], 'detached': []},
+                'interface': {'attached': [], 'detached': []}
+            }, virt.update('my vm', boot=boot))
+
         # Update memory case
         setmem_mock = MagicMock(return_value=0)
         domain_mock.setMemoryFlags = setmem_mock
diff --git a/tests/unit/states/test_virt.py b/tests/unit/states/test_virt.py
index 2af5caca1b..109faf5fba 100644
--- a/tests/unit/states/test_virt.py
+++ b/tests/unit/states/test_virt.py
@@ -249,7 +249,7 @@ class LibvirtTestCase(TestCase, LoaderModuleMockMixin):
                                               mem=2048,
                                               image='/path/to/img.qcow2'), ret)
             init_mock.assert_called_with('myvm', cpu=2, mem=2048, image='/path/to/img.qcow2',
-                                         os_type=None, arch=None,
+                                         os_type=None, arch=None, boot=None,
                                          disk=None, disks=None, nic=None, interfaces=None,
                                          graphics=None, hypervisor=None,
                                          seed=True, install=True, pub_key=None, priv_key=None,
@@ -314,6 +314,7 @@ class LibvirtTestCase(TestCase, LoaderModuleMockMixin):
                                          graphics=graphics,
                                          hypervisor='qemu',
                                          seed=False,
+                                         boot=None,
                                          install=False,
                                          pub_key='/path/to/key.pub',
                                          priv_key='/path/to/key',
@@ -338,6 +339,22 @@ class LibvirtTestCase(TestCase, LoaderModuleMockMixin):
                         'comment': 'Domain myvm updated, restart to fully apply the changes'})
             self.assertDictEqual(virt.running('myvm', update=True, cpu=2), ret)
 
+        # Working update case when running with boot params
+        boot = {
+            'kernel': '/root/f8-i386-vmlinuz',
+            'initrd': '/root/f8-i386-initrd',
+            'cmdline': 'console=ttyS0 ks=http://example.com/f8-i386/os/'
+        }
+
+        with patch.dict(virt.__salt__, {  # pylint: disable=no-member
+                    'virt.vm_state': MagicMock(return_value={'myvm': 'running'}),
+                    'virt.update': MagicMock(return_value={'definition': True, 'cpu': True})
+                }):
+            ret.update({'changes': {'myvm': {'definition': True, 'cpu': True}},
+                        'result': True,
+                        'comment': 'Domain myvm updated, restart to fully apply the changes'})
+            self.assertDictEqual(virt.running('myvm', update=True, boot=boot), ret)
+
         # Working update case when stopped
         with patch.dict(virt.__salt__, {  # pylint: disable=no-member
                     'virt.vm_state': MagicMock(return_value={'myvm': 'stopped'}),
-- 
2.23.0


