Merge ~xiaofengw/cloud-init:xiaofengw-user-defined-script into cloud-init:master

[Xiaofeng Wang]

VMWware: add option into VMTools configuration to enable/disable custom script.

VMWware customization has already support to run a custom script during the VM customization. We add this option so that VM administrator could prevent custom script to run, then the customization will fail and customization status is set to GUESTCUST_ERROR_SCRIPT_DISABLED.

[Xiaofeng Wang (xiaofengw)]

Hi Ryan,
I update the codes according to your comments. But I found it's hard for me to add unit test for these codes. Since this change is about to add option into VMTools configuration to enable/disable custom script, it requires command "vmware-toolbox-cmd" to be installed first. It's hard for us to simulate different VMTools configuration.
Hence, I test below test cases manually:
1. [deployPkg] enable-custom-script is not set in tools.conf, then custom script is executed as expected.
2. [deployPkg] enable-custom-script is "true"t in tools.conf, then custom script is executed as expected
3. [deployPkg] enable-custom-script is "false"t in tools.conf, then custom script is not executed as expected.
4. vmware-toolbox-cmd is not installed, then custom script is executed as expected.
5. [deployPkg] enable-custom-script is set to "", then custom script is executed as expected.
6. Customization doesn't require to run custom script, then customization could finish sucessfully.

[Xiaofeng Wang (xiaofengw)]

Hi all,
The unit tests are added as required, please take a look. Thanks.

[Ryan Harper (raharper)]

I've pointed CI at this branch.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:50d8b8497e5326792cd67e04d95e9fdf7d5e3c0e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/776/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/776/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

One question about the unittest. I'm marking this WIP, when you've replied, please change the state back to Needs Review.

review: Needs Information

[Xiaofeng Wang (xiaofengw)]

> One question about the unittest. I'm marking this WIP, when you've replied,
> please change the state back to Needs Review.
Thanks, please check my answer in line.

[Xiaofeng Wang (xiaofengw)]

> > One question about the unittest. I'm marking this WIP, when you've replied,
> > please change the state back to Needs Review.
> Thanks, please check my answer in line.
This is still needed. I removed this line and the test failed with "called subp. set allowed_subp=True to allow subp" error.
I checked the _fake_subp logic, it require to set allowed_subp to True if we call util.subp function no matter util.subp is mocked or not.

[Ryan Harper (raharper)]

I can't reproduce the failure; so I wonder if it's because we're not mocking out calls to vmware tools? On what platform are you running unittests on and does it have some of the vmware tools installed?

(neipa) ~ % mkdir /tmp/test-ci
(neipa) ~ % cd /tmp/test-ci/
(neipa) test-ci % git clone -b xiaofengw-user-defined-script https://git.launchpad.net/~xiaofengw/cloud-init^C
(neipa) test-ci % git clone -b xiaofengw-user-defined-script https://git.launchpad.net/~xiaofengw/cloud-init
Cloning into 'cloud-init'...
remote: Counting objects: 29358, done.
remote: Compressing objects: 100% (11052/11052), done.
remote: Total 29358 (delta 21821), reused 25008 (delta 18013)
Receiving objects: 100% (29358/29358), 17.82 MiB | 4.03 MiB/s, done.
Resolving deltas: 100% (21821/21821), done.
Checking connectivity... done.
(neipa) test-ci % cd cloud-init/
(neipa) cloud-init % git remote add upstream https://git.launchpad.net/cloud-init
(neipa) cloud-init % git fetch upstream --tags &>/dev/null
(neipa) cloud-init % git describe
18.5-72-g50d8b84
(neipa) cloud-init % sed -i '/self.allowed_subp = True/d' tests/unittests/test_datasource/test_ovf.py
(neipa) cloud-init % git diff
diff --git a/tests/unittests/test_datasource/test_ovf.py b/tests/unittests/test_datasource/test_ovf.py
index d2964e5..9059ea3 100644
--- a/tests/unittests/test_datasource/test_ovf.py
+++ b/tests/unittests/test_datasource/test_ovf.py
@@ -122,7 +122,6 @@ class TestDatasourceOVF(CiTestCase):
         super(TestDatasourceOVF, self).setUp()
         self.datasource = dsovf.DataSourceOVF
         self.tdir = self.tmp_dir()
- self.allowed_subp = True

     def test_get_data_false_on_none_dmi_data(self):
         """When dmi for system-product-name is None, get_data returns False."""
(neipa) cloud-init % tox
....
  py27: commands succeeded
  py3: commands succeeded
  xenial: commands succeeded
  pycodestyle: commands succeeded
  pyflakes: commands succeeded
  pylint: commands succeeded
  congratulations :)

review: Needs Information

[Xiaofeng Wang (xiaofengw)]

I run the unit test on Ubuntu 16.04 and the vmware tools is installed. And yes, I didn't mock out calls to vmware tools, but I has mocked the calls to subp().
In file cloudinit/tests/helper.py, if we didn't change "allowed_subp" to true, util.subp will point to self._fake_subp, then _fake_subp will always raise exception if self.allowed_subp is false. So I wonder which OS are you using, I want to have a try on that OS. Thanks.

> I can't reproduce the failure; so I wonder if it's because we're not mocking
> out calls to vmware tools? On what platform are you running unittests on and
> does it have some of the vmware tools installed?
>
> (neipa) ~ % mkdir /tmp/test-ci
> (neipa) ~ % cd /tmp/test-ci/
> (neipa) test-ci % git clone -b xiaofengw-user-defined-script
> https://git.launchpad.net/~xiaofengw/cloud-init^C
> (neipa) test-ci % git clone -b xiaofengw-user-defined-script
> https://git.launchpad.net/~xiaofengw/cloud-init
> Cloning into 'cloud-init'...
> remote: Counting objects: 29358, done.
> remote: Compressing objects: 100% (11052/11052), done.
> remote: Total 29358 (delta 21821), reused 25008 (delta 18013)
> Receiving objects: 100% (29358/29358), 17.82 MiB | 4.03 MiB/s, done.
> Resolving deltas: 100% (21821/21821), done.
> Checking connectivity... done.
> (neipa) test-ci % cd cloud-init/
> (neipa) cloud-init % git remote add upstream https://git.launchpad.net/cloud-
> init
> (neipa) cloud-init % git fetch upstream --tags &>/dev/null
> (neipa) cloud-init % git describe
> 18.5-72-g50d8b84
> (neipa) cloud-init % sed -i '/self.allowed_subp = True/d'
> tests/unittests/test_datasource/test_ovf.py
> (neipa) cloud-init % git diff
> diff --git a/tests/unittests/test_datasource/test_ovf.py
> b/tests/unittests/test_datasource/test_ovf.py
> index d2964e5..9059ea3 100644
> --- a/tests/unittests/test_datasource/test_ovf.py
> +++ b/tests/unittests/test_datasource/test_ovf.py
> @@ -122,7 +122,6 @@ class TestDatasourceOVF(CiTestCase):
> super(TestDatasourceOVF, self).setUp()
> self.datasource = dsovf.DataSourceOVF
> self.tdir = self.tmp_dir()
> - self.allowed_subp = True
>
> def test_get_data_false_on_none_dmi_data(self):
> """When dmi for system-product-name is None, get_data returns
> False."""
> (neipa) cloud-init % tox
> ....
> py27: commands succeeded
> py3: commands succeeded
> xenial: commands succeeded
> pycodestyle: commands succeeded
> pyflakes: commands succeeded
> pylint: commands succeeded
> congratulations :)

35d5d4c... by Xiaofeng Wang on 2019-08-01

Add the vmware-toolbox-cmd into allowed_subp command other than
set the allowed_subp to True.O
 Changes to be committed:
 modified: tests/unittests/test_datasource/test_ovf.py

[Xiaofeng Wang (xiaofengw)]

Hi Ryan,
I update the unittest by adding "vmware-toolbox-cmd" into allowed_subp, removing to set allowed_subp to True.
Please review and comments. Thanks.

> I can't reproduce the failure; so I wonder if it's because we're not mocking
> out calls to vmware tools? On what platform are you running unittests on and
> does it have some of the vmware tools installed?
>
> (neipa) ~ % mkdir /tmp/test-ci
> (neipa) ~ % cd /tmp/test-ci/
> (neipa) test-ci % git clone -b xiaofengw-user-defined-script
> https://git.launchpad.net/~xiaofengw/cloud-init^C
> (neipa) test-ci % git clone -b xiaofengw-user-defined-script
> https://git.launchpad.net/~xiaofengw/cloud-init
> Cloning into 'cloud-init'...
> remote: Counting objects: 29358, done.
> remote: Compressing objects: 100% (11052/11052), done.
> remote: Total 29358 (delta 21821), reused 25008 (delta 18013)
> Receiving objects: 100% (29358/29358), 17.82 MiB | 4.03 MiB/s, done.
> Resolving deltas: 100% (21821/21821), done.
> Checking connectivity... done.
> (neipa) test-ci % cd cloud-init/
> (neipa) cloud-init % git remote add upstream https://git.launchpad.net/cloud-
> init
> (neipa) cloud-init % git fetch upstream --tags &>/dev/null
> (neipa) cloud-init % git describe
> 18.5-72-g50d8b84
> (neipa) cloud-init % sed -i '/self.allowed_subp = True/d'
> tests/unittests/test_datasource/test_ovf.py
> (neipa) cloud-init % git diff
> diff --git a/tests/unittests/test_datasource/test_ovf.py
> b/tests/unittests/test_datasource/test_ovf.py
> index d2964e5..9059ea3 100644
> --- a/tests/unittests/test_datasource/test_ovf.py
> +++ b/tests/unittests/test_datasource/test_ovf.py
> @@ -122,7 +122,6 @@ class TestDatasourceOVF(CiTestCase):
> super(TestDatasourceOVF, self).setUp()
> self.datasource = dsovf.DataSourceOVF
> self.tdir = self.tmp_dir()
> - self.allowed_subp = True
>
> def test_get_data_false_on_none_dmi_data(self):
> """When dmi for system-product-name is None, get_data returns
> False."""
> (neipa) cloud-init % tox
> ....
> py27: commands succeeded
> py3: commands succeeded
> xenial: commands succeeded
> pycodestyle: commands succeeded
> pyflakes: commands succeeded
> pylint: commands succeeded
> congratulations :)

[Ryan Harper (raharper)]

I'm running on Ubuntu 16.04 as well, but I do not have vmware-toolbox-cmd installed.

We don't want the host environment to affect the test results; so the testing
should mock out calls to the host.

[Xiaofeng Wang (xiaofengw)]

Thanks Ryan,
I update the codes and stop setting allowed_subp to Yes, also mock out the calls to the host. Pleae take another review. Thanks.

> I'm running on Ubuntu 16.04 as well, but I do not have vmware-toolbox-cmd
> installed.
>
> We don't want the host environment to affect the test results; so the testing
> should mock out calls to the host.

[Ryan Harper (raharper)]

I think you need to push the changes?

[Xiaofeng Wang (xiaofengw)]

Please check this commit, thanks.
https://git.launchpad.net/~xiaofengw/cloud-init/commit/?id=4ba075cf3566ca274955aa0b463553a24a37428b

> I think you need to push the changes?

[Xiaofeng Wang (xiaofengw)]

And the call to subp is already mocked in the original commit.
> Please check this commit, thanks.
> https://git.launchpad.net/~xiaofengw/cloud-
> init/commit/?id=4ba075cf3566ca274955aa0b463553a24a37428b
>
> > I think you need to push the changes?

[Xiaofeng Wang (xiaofengw)]

> I think you need to push the changes?

Hi Ryan,
I followed your suggestion to mock the guestcust_util.get_tools_config and still get below error. Also, I tried to setup another ubuntu 16.04 and run the test, also couldn't fix this issue.
Could you give me some help on this issue? Thanks.
  File "/home/cc/cloud-init/cloudinit/sources/DataSourceOVF.py", line 159, in _get_data
    "true")
  File "/home/cc/cloud-init/cloudinit/sources/helpers/vmware/imc/guestcust_util.py", line 140, in get_tools_config
    (outText, _) = util.subp(cmd)
  File "/home/cc/cloud-init/cloudinit/tests/helpers.py", line 184, in _fake_subp
    ["%s=%s" % (k, repr(v)) for k, v in kwargs.items()]))
Exception: called subp. set self.allowed_subp=True to allow
 subp(['vmware-toolbox-cmd', 'config', 'get', 'deployPkg', 'enable-custom-scripts'])

[Ryan Harper (raharper)]

Let's get this branch rebased onto master to clean up the merge conflicts. Once you've got that done, please move the branch back to Needs Review.

Unmerged commits

35d5d4c... by Xiaofeng Wang on 2019-08-01

Add the vmware-toolbox-cmd into allowed_subp command other than
set the allowed_subp to True.O
 Changes to be committed:
 modified: tests/unittests/test_datasource/test_ovf.py

0ed0d32... by Xiaofeng Wang on 2019-06-20

add unit tests for the custom script option

9a7c180... by Xiaofeng Wang on 2019-05-24

VMWware: add option into VMTools configuration to enable/disable custom script.

VMWware customization has already support to run a custom script during the VM customization.
We add this option so that VM administrator could prevent custom script to run, then the
customization will fail and customization status is set to GUESTCUST_ERROR_SCRIPT_DISABLED.

diff --git a/cloudinit/sources/DataSourceOVF.py b/cloudinit/sources/DataSourceOVF.py
index dd941d2..793aa7f 100644
--- a/cloudinit/sources/DataSourceOVF.py
+++ b/cloudinit/sources/DataSourceOVF.py
@@ -40,11 +40,15 @@ from cloudinit.sources.helpers.vmware.imc.guestcust_state \
 from cloudinit.sources.helpers.vmware.imc.guestcust_util import (
     enable_nics,
     get_nics_to_enable,
-    set_customization_status
+    set_customization_status,
+    get_tools_config
 )
 
 LOG = logging.getLogger(__name__)
 
+CONFGROUPNAME_GUESTCUSTOMIZATION = "deployPkg"
+GUESTCUSTOMIZATION_ENABLE_CUST_SCRIPTS = "enable-custom-scripts"
+
 
 class DataSourceOVF(sources.DataSource):
 
@@ -148,9 +152,29 @@ class DataSourceOVF(sources.DataSource):
                     product_marker, os.path.join(self.paths.cloud_dir, 'data'))
                 special_customization = product_marker and not hasmarkerfile
                 customscript = self._vmware_cust_conf.custom_script_name
+<<<<<<< cloudinit/sources/DataSourceOVF.py
                 ccScriptsDir = os.path.join(
                     self.paths.get_cpath("scripts"),
                     "per-instance")
+=======
+
+                custScriptConfig = get_tools_config(
+                    CONFGROUPNAME_GUESTCUSTOMIZATION,
+                    GUESTCUSTOMIZATION_ENABLE_CUST_SCRIPTS,
+                    "true")
+                if custScriptConfig.lower() == "false":
+                    # Update the customization status if there is a
+                    # custom script is disabled
+                    if special_customization and customscript:
+                        LOG.debug(
+                            "Custom script is disabled by VM Administrator")
+                        set_customization_status(
+                            GuestCustStateEnum.GUESTCUST_STATE_RUNNING,
+                            GuestCustErrorEnum.GUESTCUST_ERROR_SCRIPT_DISABLED)
+                        raise RuntimeError(
+                            "Custom script is disabled by VM Administrator")
+
+>>>>>>> cloudinit/sources/DataSourceOVF.py
             except Exception as e:
                 _raise_error_status(
                     "Error parsing the customization Config File",
diff --git a/cloudinit/sources/helpers/vmware/imc/guestcust_error.py b/cloudinit/sources/helpers/vmware/imc/guestcust_error.py
index db5a00d..65ae739 100644
--- a/cloudinit/sources/helpers/vmware/imc/guestcust_error.py
+++ b/cloudinit/sources/helpers/vmware/imc/guestcust_error.py
@@ -10,5 +10,6 @@ class GuestCustErrorEnum(object):
     """Specifies different errors of Guest Customization engine"""
 
     GUESTCUST_ERROR_SUCCESS = 0
+    GUESTCUST_ERROR_SCRIPT_DISABLED = 6
 
 # vi: ts=4 expandtab
diff --git a/cloudinit/sources/helpers/vmware/imc/guestcust_util.py b/cloudinit/sources/helpers/vmware/imc/guestcust_util.py
index a590f32..6c5398d 100644
--- a/cloudinit/sources/helpers/vmware/imc/guestcust_util.py
+++ b/cloudinit/sources/helpers/vmware/imc/guestcust_util.py
@@ -7,6 +7,7 @@
 
 import logging
 import os
+import re
 import time
 
 from cloudinit import util
@@ -117,4 +118,36 @@ def enable_nics(nics):
     logger.warning("Can't connect network interfaces after %d attempts",
                    enableNicsWaitRetries)
 
+
+def get_tools_config(section, key, defaultVal):
+    """ Return the value of [section] key from VMTools configuration.
+
+        @param section: String of section to read from VMTools config
+        @returns: String value from key in [section] or defaultVal if
+                  [section] is not present or vmware-toolbox-cmd is
+                  not installed.
+    """
+
+    if not util.which('vmware-toolbox-cmd'):
+        logger.debug(
+            'vmware-toolbox-cmd not installed, returning default value')
+        return defaultVal
+
+    retValue = defaultVal
+    cmd = ['vmware-toolbox-cmd', 'config', 'get', section, key]
+
+    try:
+        (outText, _) = util.subp(cmd)
+        m = re.match(r'(.+)=(.*)', outText)
+        if m:
+            retValue = m.group(2).strip()
+            logger.debug("Get tools config: [%s] %s = %s",
+                         section, key, retValue)
+    except util.ProcessExecutionError as e:
+        logger.error("Failed running %s[%s]", cmd, e.exit_code)
+        logger.exception(e)
+
+    return retValue
+
+
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_datasource/test_ovf.py b/tests/unittests/test_datasource/test_ovf.py
index 349d54c..4248512 100644
--- a/tests/unittests/test_datasource/test_ovf.py
+++ b/tests/unittests/test_datasource/test_ovf.py
@@ -16,6 +16,7 @@ from cloudinit.helpers import Paths
 from cloudinit.sources import DataSourceOVF as dsovf
 from cloudinit.sources.helpers.vmware.imc.config_custom_script import (
     CustomScriptNotFound)
+from cloudinit.sources.helpers.vmware.imc import guestcust_util
 
 MPATH = 'cloudinit.sources.DataSourceOVF.'
 
@@ -116,6 +117,7 @@ class TestMarkerFiles(CiTestCase):
 class TestDatasourceOVF(CiTestCase):
 
     with_logs = True
+    allowed_subp = ['vmware-toolbox-cmd']
 
     def setUp(self):
         super(TestDatasourceOVF, self).setUp()
@@ -182,6 +184,46 @@ class TestDatasourceOVF(CiTestCase):
         self.assertIn('Script %s not found!!' % customscript,
                       str(context.exception))
 
+    def test_get_data_cust_script_disabled(self):
+        """If custom script is disabled by VMware tools configuration,
+        raise a RuntimeError.
+        """
+        paths = Paths({'cloud_dir': self.tdir})
+        ds = self.datasource(
+            sys_cfg={'disable_vmware_customization': False}, distro={},
+            paths=paths)
+        # Prepare the conf file
+        conf_file = self.tmp_path('test-cust', self.tdir)
+        conf_content = dedent("""\
+            [CUSTOM-SCRIPT]
+            SCRIPT-NAME = test-script
+            [MISC]
+            MARKER-ID = 12345345
+            """)
+        util.write_file(conf_file, conf_content)
+        # Prepare the custom sript
+        customscript = self.tmp_path('test-script', self.tdir)
+        util.write_file(customscript, "This is the post cust script")
+
+        with mock.patch.object(util, 'which', return_value='/dummy/path'):
+            # Mock the cust script is disabeld (FALSE).
+            with mock.patch.object(util, 'subp',
+                                   return_value=(' key = FALSE', b'')):
+                with mock.patch.object(guestcust_util,
+                                       'set_customization_status',
+                                       return_value=('msg', b'')):
+                    with self.assertRaises(RuntimeError) as context:
+                        wrap_and_call(
+                            'cloudinit.sources.DataSourceOVF',
+                            {'util.read_dmi_data': 'vmware',
+                             'util.del_dir': True,
+                             'search_file': self.tdir,
+                             'wait_for_imc_cfg_file': conf_file,
+                             'get_nics_to_enable': ''},
+                            ds.get_data)
+        self.assertIn('Custom script is disabled by VM Administrator',
+                      str(context.exception))
+
     def test_get_data_non_vmware_seed_platform_info(self):
         """Platform info properly reports when on non-vmware platforms."""
         paths = Paths({'cloud_dir': self.tdir, 'run_dir': self.tdir})
diff --git a/tests/unittests/test_vmware/test_guestcust_util.py b/tests/unittests/test_vmware/test_guestcust_util.py
new file mode 100644
index 0000000..345b5ff
--- /dev/null
+++ b/tests/unittests/test_vmware/test_guestcust_util.py
@@ -0,0 +1,59 @@
+# Copyright (C) 2019 Canonical Ltd.
+# Copyright (C) 2019 VMware INC.
+#
+# Author: Xiaofeng Wang <[e-mail]>
+#
+# This file is part of cloud-init. See LICENSE file for license information.
+
+from cloudinit import util
+from cloudinit.sources.helpers.vmware.imc.guestcust_util import (
+    get_tools_config,
+)
+from cloudinit.tests.helpers import CiTestCase, mock
+
+
+class TestGuestCustUtil(CiTestCase):
+    def test_get_tools_config_not_installed(self):
+        """
+        This test is designed to verify the behavior if vmware-toolbox-cmd
+        is not installed.
+        """
+        with mock.patch.object(util, 'which', return_value=None):
+            self.assertEqual(
+                get_tools_config('section', 'key', 'defaultVal'), 'defaultVal')
+
+    def test_get_tools_config_internal_exception(self):
+        """
+        This test is designed to verify the behavior if internal exception
+        is raised.
+        """
+        with mock.patch.object(util, 'which', return_value='/dummy/path'):
+            with mock.patch.object(util, 'subp',
+                                   return_value=('key=value', b''),
+                                   side_effect=util.ProcessExecutionError(
+                                       "subp failed", exit_code=99)):
+                # verify return value is 'defaultVal', not 'value'.
+                self.assertEqual(
+                    get_tools_config('section', 'key', 'defaultVal'),
+                    'defaultVal')
+
+    def test_get_tools_config_normal(self):
+        """
+        This test is designed to verify the value could be parsed from
+        key = value of the given [section]
+        """
+        with mock.patch.object(util, 'which', return_value='/dummy/path'):
+            # value is not blank
+            with mock.patch.object(util, 'subp',
+                                   return_value=('key =   value  ', b'')):
+                self.assertEqual(
+                    get_tools_config('section', 'key', 'defaultVal'),
+                    'value')
+            # value is blank
+            with mock.patch.object(util, 'subp',
+                                   return_value=('key = ', b'')):
+                self.assertEqual(
+                    get_tools_config('section', 'key', 'defaultVal'),
+                    '')
+
+# vi: ts=4 expandtab