Merge ~ajorgens/cloud-init:user-shell into cloud-init:master

[Andrew Jorgensen]

Log a helpful message if a user script doesn't include a usable #!

A patch to allow scripts missing a #! to run by using shell=True was proposed but rejected. Instead we emit a log message to help the user understand what went wrong.

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

FAILED: Continuous integration, rev:74223d8aef2d22ef1d43e85f1a9881960f7cd5da
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~ajorgens/cloud-init/+git/cloud-init/+merge/325857/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/7/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/7
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/7
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/7
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/7
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/7

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

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

I kind of see this as something that should be fixed in another way.

a. The user gave cloud-init bad data.
b. cloud-init probably did a bad job of telling them that.

I don't think, though, that a + b =
c. cloud-init should make a guess as to what they meant to do.

It feels like you're trying to fix the scenario where a user typed something at
a prompt and it worked and they just want to put that in a script. However,
the prompt they most likely typed at was bash, and the fix you've proposed is
to execute that code with posix sh, which is *not* bash. So you've allowed the
ignorant user to remain in a state of bliss a little longer, but I'd really
rather somehow just error and educate them.

The suggested fix here ends up with cloud-init doing:
  chmod 755 FILE
  subp(["sh", FILE])
rather than just
  chmod 755 FILE
  subp(FILE)

(Note that I'm familiar with shell=True in subprocess.popen, the above I think just illustrates the difference better).

I believe that that makes the situation better for this user-data, which is valid posix shell:
  echo hi mom

But ends up failing on this (bash specific) user-data:
  x=( "hi" "mom")
  echo "${x[@]}"

I don't know exactly why, but it seems to work correctly on this:
 #!/usr/bin/env python
 print("hi mom")

[Scott Moser (smoser)]

I'm going to say 'needs fixing'.
Because at very least this needs to warn somewhere that "hey, you really werent specific, and you really should be".

review: Needs Fixing

[Andrew Jorgensen (ajorgens)]

When a shell tries to execute a script that doesn't have a #! it simply tries to interpret it itself, rather than hand it off to something else (or re-exec itself, not actually sure which but the same end result).

Missing a #! could happen for a variety of reasons, but the only case where it will be seen in a user-provided script under cloud-init is when it's part of a MIME-Multipart (or the YAML equiv.) document with the right content type, so we know it's intended to be executed.

subprocess.popen does use /bin/sh explicitly . Different distros provide different shells as /bin/sh. Amazon Linux happens to use bash rather than some more minimal posix shell:

$ rpm -qf /bin/sh
bash-4.1.2-15.21.al12.x86_64

SUSE and Red Hat both use bash also:

> rpm -qf /bin/sh
bash-4.3-78.39.x86_64

$ rpm -qf /bin/sh
bash-4.2.46-21.el7_3.x86_64

Ubuntu uses Dash, but via alternatives:
(sidenote: There's an Ubuntu "sliminess" section on the Almquist Shell Wikipedia article? https://en.wikipedia.org/wiki/Almquist_shell#Slimness)

$ dpkg -S /bin/sh
diversion by dash from: /bin/sh
diversion by dash to: /bin/sh.distrib
dash: /bin/sh

So it seems Ubuntu is the odd man out here (FWIW I think Dash is probably the better choice for /bin/sh) which is why the concern didn't occur to me.

There's no reason I couldn't ship a binary executable as a user script, so I'm reluctant to peek at the file to decide if I need to warn the user that their file might get interpreted with /bin/sh, which might not be bash. I'd have to look far enough to tell that it's not in the set of executable types the running Linux kernel knows how to execute. I'm not even sure that's possible, since the kernel can be told how to run things like .NET assemblies and Windows executables.

The user in this case has expressed intent that it be executed and running it under a shell gives it the best possible chance of executing successfully.

Further thoughts on this?

[Andrew Jorgensen (ajorgens)]

> There's no reason I couldn't ship a binary executable as a user script
Hmm, need to test that assumption. You've got the content-type as text/x-shellscript.

[Andrew Jorgensen (ajorgens)]

> > There's no reason I couldn't ship a binary executable as a user script
> Hmm, need to test that assumption. You've got the content-type as text/x-shellscript.

Fails because write_file presumes files are text.

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/cloudinit/handlers/__init__.py", line 103, in run_part
    payload, frequency)
  File "/usr/lib/python3/dist-packages/cloudinit/handlers/shell_script.py", line 43, in handle_part
    util.write_file(path, payload, 0o700)
  File "/usr/lib/python3/dist-packages/cloudinit/util.py", line 1711, in write_file
    content = encode_text(content)
  File "/usr/lib/python3/dist-packages/cloudinit/util.py", line 154, in encode_text
    return text.encode(encoding)
UnicodeEncodeError: 'utf-8' codec can't encode character '\udcf0' in position 24: surrogates not allowed

[Andrew Jorgensen (ajorgens)]

So I guess peeking at the first two bytes of the file might be okay to do, but I also think a better thing to do would be to make binaries work. :-/

[Scott Moser (smoser)]

I talked with Andrew some in irc.
We decided to not use 'shell=True', but rather to issue a WARNING more appropriately when failure is seen.

see discussion at https://irclogs.ubuntu.com/2017/07/10/%23cloud-init.html#t18:12

review: Needs Fixing

[Scott Moser (smoser)]

I've marked this as work-in-progress.
When you have addressed the above, please set back to 'needs review'

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

PASSED: Continuous integration, rev:8f4a377e1b0549a6f4d497ded4fe3552ccb1bbd4
https://jenkins.ubuntu.com/server/job/cloud-init-ci/138/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: CentOS 6 & 7: Build & Test
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I'm not certain that i like the location of this log.... its awfully generic. some unit test suggestions inline.

Could we move it to be a more specific path that was used by the run parts ? If you did move up, you'd also want to verify that the exit_code of the ProcessExecutionError was '-'.

I'm interested in thoughts on moving to a more generic place.

If you're opposed to that, lets at least
 a.) change it to WARN (very few places in cloud-init use ERROR) for better or worse, but consistency is good for now.
 b.) maybe list the file in the error ?
     LOG.warn("Exec format error executing %s" % (args if shell is True else args[0]))

also... we could even have ProcessExecutionError show this itself. I fully realize that is *more* generic rather than less generic :)

review: Needs Information

[Scott Moser (smoser)]

It seems right to me to just move this to ProcessExecutionError.
We could update 'reason' in the __init__ there to contain the error message if exit_code == self.empty_attr and errno is ENOEXEC

[Andrew Jorgensen (ajorgens)]

Looking at subp as it was, it looks like 'reason' holds the exception object. Did you mean that 'description' should contain the message?

[Andrew Jorgensen (ajorgens)]

Updated with the helpfulness pushed into the ProcessExecutionError instead.

It might also be reasonable to push something into util.runparts to suppress the unfriendliness of the exception, but I think this is sufficient and fairly simple as is.

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

PASSED: Continuous integration, rev:5d1034b426e89f1d1799427ec8470d5f152badab
https://jenkins.ubuntu.com/server/job/cloud-init-ci/203/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

review: Approve

diff --git a/cloudinit/util.py b/cloudinit/util.py
index ce2c603..609e94c 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -12,7 +12,6 @@ import contextlib
 import copy as obj_copy
 import ctypes
 import email
-import errno
 import glob
 import grp
 import gzip
@@ -34,6 +33,8 @@ import sys
 import tempfile
 import time
 
+from errno import ENOENT, ENOEXEC
+
 from base64 import b64decode, b64encode
 from six.moves.urllib import parse as urlparse
 
@@ -239,7 +240,10 @@ class ProcessExecutionError(IOError):
             self.cmd = cmd
 
         if not description:
-            self.description = 'Unexpected error while running command.'
+            if not exit_code and errno == ENOEXEC:
+                self.description = 'Exec format error. Missing #! in script?'
+            else:
+                self.description = 'Unexpected error while running command.'
         else:
             self.description = description
 
@@ -433,7 +437,7 @@ def read_conf(fname):
     try:
         return load_yaml(load_file(fname), default={})
     except IOError as e:
-        if e.errno == errno.ENOENT:
+        if e.errno == ENOENT:
             return {}
         else:
             raise
@@ -901,7 +905,7 @@ def read_file_or_url(url, timeout=5, retries=10,
             contents = load_file(file_path, decode=False)
         except IOError as e:
             code = e.errno
-            if e.errno == errno.ENOENT:
+            if e.errno == ENOENT:
                 code = url_helper.NOT_FOUND
             raise url_helper.UrlError(cause=e, code=code, headers=None,
                                       url=url)
@@ -1247,7 +1251,7 @@ def find_devs_with(criteria=None, oformat='device',
     try:
         (out, _err) = subp(cmd, rcs=[0, 2])
     except ProcessExecutionError as e:
-        if e.errno == errno.ENOENT:
+        if e.errno == ENOENT:
             # blkid not found...
             out = ""
         else:
@@ -1285,7 +1289,7 @@ def load_file(fname, read_cb=None, quiet=False, decode=True):
     except IOError as e:
         if not quiet:
             raise
-        if e.errno != errno.ENOENT:
+        if e.errno != ENOENT:
             raise
     contents = ofh.getvalue()
     LOG.debug("Read %s bytes from %s", len(contents), fname)
@@ -1653,7 +1657,7 @@ def del_file(path):
     try:
         os.unlink(path)
     except OSError as e:
-        if e.errno != errno.ENOENT:
+        if e.errno != ENOENT:
             raise e
 
 
@@ -2281,7 +2285,7 @@ def pathprefix2dict(base, required=None, optional=None, delim=os.path.sep):
         try:
             ret[f] = load_file(base + delim + f, quiet=False, decode=False)
         except IOError as e:
-            if e.errno != errno.ENOENT:
+            if e.errno != ENOENT:
                 raise
             if f in required:
                 missing.append(f)
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index f38a664..5f11c88 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -568,7 +568,8 @@ class TestReadSeeded(helpers.TestCase):
         self.assertEqual(found_ud, ud)
 
 
-class TestSubp(helpers.TestCase):
+class TestSubp(helpers.CiTestCase):
+    with_logs = True
 
     stdin2err = [BASH, '-c', 'cat >&2']
     stdin2out = ['cat']
@@ -650,6 +651,16 @@ class TestSubp(helpers.TestCase):
         self.assertEqual(
             ['FOO=BAR', 'HOME=/myhome', 'K1=V1', 'K2=V2'], out.splitlines())
 
+    def test_subp_warn_missing_shebang(self):
+        """Warn on no #! in script"""
+        noshebang = self.tmp_path('noshebang')
+        util.write_file(noshebang, 'true\n')
+
+        os.chmod(noshebang, os.stat(noshebang).st_mode | stat.S_IEXEC)
+        self.assertRaisesRegexp(util.ProcessExecutionError,
+                                'Missing #! in script\?',
+                                util.subp, (noshebang,))
+
     def test_returns_none_if_no_capture(self):
         (out, err) = util.subp(self.stdin2out, data=b'', capture=False)
         self.assertIsNone(err)