tolerate more mime types for cc part of user-data. #234

garloff · 2020-03-05T09:54:36Z

Some cloud environments pass multipart user-data as text/x-shellscript.
(I have observed this from old heat on OTC.)
We need to make sure that it is accepted by cloud-init as user-data.
Note that more intelligent logic might pass it correctly as text/x-yaml
which cloud-init should tolerate as well.

Signed-off-by: Kurt Garloff scs@garloff.de

Some cloud environments pass multipart user-data as text/x-shellscript. (I have observed this from old heat on OTC.) We need to make sure that it is accepted by cloud-init as user-data. Note that more intelligent logic might pass it correctly as text/x-yaml which cloud-init should tolerate as well. Signed-off-by: Kurt Garloff <scs@garloff.de>

It insists on ancient narrow terminals, so adjust ...

…nfig-mimetypes

smoser

At least would like feedback from @blackboxsw or someone else to collaborate here.

smoser · 2020-03-10T13:15:37Z

cloudinit/user_data.py

@@ -30,7 +30,8 @@
 CONTENT_TYPE = 'Content-Type'

 # Various special content types that cause special actions
-TYPE_NEEDED = ["text/plain", "text/x-not-multipart"]
+TYPE_NEEDED = ["text/plain", "text/x-not-multipart",
+               "text/x-shellscript", "text/x-yaml"]


I appreciate the goal here, but I don't really like the implementation.
I also accept that the reason this is not clear is my code. But please don't restrict me from making good decisions now because I made bad decisions in the past.

The strings you've added (text/x-shellscript) very much do not fit in 'TYPE_NEEDED'.
They're more 'type already supplied'.

I think more what we want to do is something like below. Ie, we already have
a list of types that we accept, lets just accept all of them. This would need
testing, and unfortunately (/me shakes fist at 10-years-ago-self) there are not
good unit tests on any of this code.

--- a/cloudinit/user_data.py +++ b/cloudinit/user_data.py @@ -117,7 +117,7 @@ class UserDataProcessor(object): ctype_orig = UNDEF_TYPE if ctype_orig in TYPE_NEEDED: ctype = find_ctype(payload) - if ctype is None: + if ctype in handlers.INCLUSION_TYPES_MAP.values() or ctype is None: ctype = ctype_orig # In the case where the data was compressed, we want to make sure

Shouldn't we be safe by checking for the #cloud-config header also?

Do you think that is necessary?
To me, if someone went to the trouble of creating a mime type of 'text/part-handler' then they probably meant that to be done. Or if they didn't... failure is fine.

I'm pretty sure that the rest of the code will trust the type. the prefixes are only used if there is a TYPE_NEEDED.

Fair enough. I will test whether your proposed change will work.

Does not work, I'm afraid:

[ 18.302114] cloud-init[1122]: Cloud-init v. 19.4-33-gbb4131a2-0ubuntu1~18.04.1 running 'modules:config' at Fri, 13 Mar 2020 21:02:24 +0000. Up 17.80 seconds. [[0;32m OK [0m] Started Apply the settings specified in cloud-config. Starting Execute cloud user/final scripts... [ 18.871485] cloud-init[1180]: Cloud-init v. 19.4-33-gbb4131a2-0ubuntu1~18.04.1 running 'modules:final' at Fri, 13 Mar 2020 21:02:25 +0000. Up 18.75 seconds. [ 18.873764] cloud-init[1180]: 2020-03-13 21:02:25,815 - util.py[WARNING]: Failed running /var/lib/cloud/instance/scripts/userdata [-] [ 18.878276] cloud-init[1180]: 2020-03-13 21:02:25,822 - cc_scripts_user.py[WARNING]: Failed to run module scripts-user (scripts in /var/lib/cloud/instance/scripts) [ 18.880637] cloud-init[1180]: 2020-03-13 21:02:25,825 - util.py[WARNING]: Running module scripts-user (<module 'cloudinit.config.cc_scripts_user' from '/usr/lib/python3/dist-packages/cloudinit/config/cc_scripts_user.py'>) failed

Should I attach the user-data maybe?

@garloff is the comment below the user-data (the multi-part mime)?

@garloff is the comment below the user-data (the multi-part mime)?

Yes.

garloff · 2020-03-13T21:35:08Z

Content-Type: multipart/mixed; boundary="===============4023107748017163298=="
MIME-Version: 1.0

--===============4023107748017163298==
Content-Type: text/cloud-config; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="cloud-config"



# Capture all subprocess output into a logfile
# Useful for troubleshooting cloud-init issues
output: {all: '| tee -a /var/log/cloud-init-output.log'}

--===============4023107748017163298==
Content-Type: text/cloud-boothook; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="boothook.sh"

#!/bin/bash

# FIXME(shadower) this is a workaround for cloud-init 0.6.3 present in Ubuntu
# 12.04 LTS:
# https://bugs.launchpad.net/heat/+bug/1257410
#
# The old cloud-init doesn't create the users directly so the commands to do
# this are injected though nova_utils.py.
#
# Once we drop support for 0.6.3, we can safely remove this.

# in case heat-cfntools has been installed from package but no symlinks
# are yet in /opt/aws/bin/
cfn-create-aws-symlinks

# Do not remove - the cloud boothook should always return success
exit 0

--===============4023107748017163298==
Content-Type: text/part-handler; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="part-handler.py"

# part-handler
#
#    Licensed under the Apache License, Version 2.0 (the "License"); you may
#    not use this file except in compliance with the License. You may obtain
#    a copy of the License at
#
#         http://www.apache.org/licenses/LICENSE-2.0
#
#    Unless required by applicable law or agreed to in writing, software
#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
#    License for the specific language governing permissions and limitations
#    under the License.
# 
import datetime
import errno
import os
import sys


def list_types():
    return(["text/x-cfninitdata"])


def handle_part(data, ctype, filename, payload):
    if ctype == "__begin__":
        try:
            os.makedirs('/var/lib/heat-cfntools', int("700", 8))
        except OSError:
            ex_type, e, tb = sys.exc_info()
            if e.errno != errno.EEXIST:
                raise
        return

    if ctype == "__end__":
        return

    timestamp = datetime.datetime.now()
    with open('/var/log/part-handler.log', 'a') as log:
        log.write('%s filename:%s, ctype:%s\n' % (timestamp, filename, ctype))

    if ctype == 'text/x-cfninitdata':
        with open('/var/lib/heat-cfntools/%s' % filename, 'w') as f:
            f.write(payload)

        # TODO(sdake) hopefully temporary until users move to heat-cfntools-1.3
        with open('/var/lib/cloud/data/%s' % filename, 'w') as f:
            f.write(payload)

--===============4023107748017163298==
Content-Type: text/x-shellscript; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="userdata"

#cloud-config
final_message: The system is finally up, after $UPTIME seconds
package_update: true
package_upgrade: true
packages: [ifupdown]
[...]

--===============4023107748017163298==
Content-Type: text/x-cfninitdata; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="cfn-init-data"

{"os-collect-config": {"request": {"metadata_url": "https://swift.eu-de.otc.t-systems.com/v1/AUTH_59de21b288c14c449a05bc9106f80223/testbed-node_0_server-wwbqkkbuovig/04cf716f-31aa-4486-bc2d-5367d9b2cbd0?temp_url_sig=04d9277a33d01743b5bb29cbc475f7ee62c001ea&temp_url_expires=2147483587"}, "collectors": ["ec2", "request", "local"]}, "deployments": []}
--===============4023107748017163298==
Content-Type: text/x-cfninitdata; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="cfn-watch-server"

http://127.0.0.1:8003/

raharper · 2020-03-24T18:23:39Z

Content-Type: text/x-shellscript; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="userdata"

#cloud-config
final_message: The system is finally up, after $UPTIME seconds
package_update: true
package_upgrade: true
packages: [ifupdown]
[...]

Is this the hunk that's not working?

garloff · 2020-03-25T12:04:02Z

Content-Type: text/x-shellscript; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="userdata"
#cloud-config
final_message: The system is finally up, after $UPTIME seconds
package_update: true
package_upgrade: true
packages: [ifupdown]
[...]

Is this the hunk that's not working?

Correct, this piece is ignored without the patch.

raharper · 2020-03-25T19:14:00Z

Thanks! I've reproduced the issue like so:

% cd cloud-init
% cat <<EOF >userdata
#cloud-config
final_message: The system is finally up, after $UPTIME seconds
package_update: true
package_upgrade: true
packages: [ifupdown]
EOF
% tools/make-mime.py --attach userdata:x-shellscript  > userdata-mime
% lxc init ubuntu-daily:focal f3
% lxc config set f3 user.user-data - < userdata-mime
% lxc launch f3

The error message shows up in cloud-init.log

2020-03-25 18:41:12,973 - util.py[DEBUG]: Failed running /var/lib/cloud/instance/scripts/userdata [-]
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/cloudinit/util.py", line 2063, in subp
    sp = subprocess.Popen(bytes_args, stdout=stdout,
  File "/usr/lib/python3.8/subprocess.py", line 854, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.8/subprocess.py", line 1702, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
OSError: [Errno 8] Exec format error: b'/var/lib/cloud/instance/scripts/userdata'

During handling of the above exception, another exception occurred:

Due to cloud-init attempting to call runparts on the content of the message.

@smoser had the right idea of checking if the ctype_orig content is one of the include_types we translate, but the fix was in the wrong spot. The following patch checks if the orig type is in the map, and if so, we can call find_ctype() to update ctype to the correct value.

diff --git a/cloudinit/user_data.py b/cloudinit/user_data.py
index 6f41b03a..670dbee6 100644
--- a/cloudinit/user_data.py
+++ b/cloudinit/user_data.py
@@ -25,6 +25,7 @@ LOG = logging.getLogger(__name__)
 NOT_MULTIPART_TYPE = handlers.NOT_MULTIPART_TYPE
 PART_FN_TPL = handlers.PART_FN_TPL
 OCTET_TYPE = handlers.OCTET_TYPE
+INCLUDE_MAP = handlers.INCLUSION_TYPES_MAP
 
 # Saves typing errors
 CONTENT_TYPE = 'Content-Type'
@@ -115,7 +116,8 @@ class UserDataProcessor(object):
             # Attempt to figure out the payloads content-type
             if not ctype_orig:
                 ctype_orig = UNDEF_TYPE
-            if ctype_orig in TYPE_NEEDED:
+            if ctype_orig in TYPE_NEEDED or (ctype_orig in
+                                             INCLUDE_MAP.values()):
                 ctype = find_ctype(payload)
             if ctype is None:
                 ctype = ctype_orig

raharper

I left a comment with a suggested change that should work. Please take a look and let me know.

This reverts commit 1f476de.

This reverts commit b2f4dd3.

garloff · 2020-03-26T23:26:49Z

I left a comment with a suggested change that should work. Please take a look and let me know.

I can confirm that your patch works.
So how do we best close this one:

I could revert my changes and merge yours. But this would misattribute your work (even if I clarify this in the comment)
Should I try to rebase and cleanup the patch history?
Or a new branch?
Do you want to go forward and create a mergable PR?

This patch if from Ryan Harper (inspired by my bug report and a suggestion from Scott Moser) from the discussion on PR canonical#234. cloud-init would not accept unexpected mime-types in multipart user-data for the cloud-config piece before; now it does. Problem was observed and reproduced with old heat on OTC and is now fixed. Signed-off-by: Kurt Garloff <kurt@garloff.de>

raharper · 2020-03-27T16:36:44Z

I left a comment with a suggested change that should work. Please take a look and let me know.

I can confirm that your patch works.

Excellent! Thank you for verifying.

So how do we best close this one:

* I could revert my changes and merge yours. But this would misattribute your work (even if I clarify this in the comment)

* Should I try to rebase and cleanup the patch history?

* Or a new branch?

IMHO, the easiest path is to create a new PR. I'm also fine
with a rebase and squash. You could add the suggested patch
as a new commit, rebase on master, and then drop your original
patch. Then just a push --force to this branch should clean
things up.

* Do you want to go forward and create a mergable PR?

I can do that, but I'd prefer you to have credit for finding
the issue and fixing cloud-init. Just let me know your preference.

If you do want to rebase or start a new PR, this is a unittest
which verifies the fix as well.

diff --git a/tests/unittests/test_data.py b/tests/unittests/test_data.py
index 74cc26ec..a4261609 100644
--- a/tests/unittests/test_data.py
+++ b/tests/unittests/test_data.py
@@ -213,6 +213,40 @@ c: d
         self.assertEqual(1, len(cc))
         self.assertEqual('c', cc['a'])

+    def test_cloud_config_as_x_shell_script(self):
+        blob_cc = '''
+#cloud-config
+a: b
+c: d
+'''
+        message_cc = MIMEBase("text", "x-shellscript")
+        message_cc.set_payload(blob_cc)
+
+        blob_jp = '''
+#cloud-config-jsonp
+[
+     { "op": "replace", "path": "/a", "value": "c" },
+     { "op": "remove", "path": "/c" }
+]
+'''
+
+        message_jp = MIMEBase('text', "cloud-config-jsonp")
+        message_jp.set_payload(blob_jp)
+
+        message = MIMEMultipart()
+        message.attach(message_cc)
+        message.attach(message_jp)
+
+        self.reRoot()
+        ci = stages.Init()
+        ci.datasource = FakeDataSource(str(message))
+        ci.fetch()
+        ci.consume_data()
+        cc_contents = util.load_file(ci.paths.get_ipath("cloud_config"))
+        cc = util.load_yaml(cc_contents)
+        self.assertEqual(1, len(cc))
+        self.assertEqual('c', cc['a'])
+

Test case from Ryan Harper in discussion of PR#234 (canonical#234). This should demonstrate that cloud-init (without the subsequent patches) will fail to accept multipart user-data documents with the cloud-config piece (mis)declared as text/x-shellscript. This happens on old heat on OpenTelekomCloud. Let's watch this fail the CI and then add the fix ... Signed-off-by: Kurt Garloff <kurt@garloff.de>

garloff · 2020-03-30T16:55:05Z

There is PR#290 now, where I will try to provide a clean history.
I pushed the testcase first to trigger an expected CI fail, then pushing the fix (TDD).
As soon as I have pushed the fix and submitted #290, I will close this one.
Thanks for the great advice, Ryan and Scott!

This patch if from Ryan Harper (inspired by my bug report and a suggestion from Scott Moser) from the discussion on PR canonical#234. cloud-init would not accept unexpected mime-types in multipart user-data for the cloud-config piece before; now it does. Problem was observed and reproduced with old heat on OTC and is now fixed. Signed-off-by: Kurt Garloff <kurt@garloff.de>

garloff · 2020-03-31T06:16:42Z

This one can be closed, as it's superceded by PR #290, which is ready for review now.

On some platforms (old heat on OpenTelekomCloud), the user-data mime part is mislabeled x-shellscript. cloud-init would not accept this unexpected mime-type in multipart user-data. Cloud-init will now run find_ctype() on the content of the mime-part to check if it matches known include types. This patch is from Ryan Harper (inspired by my bug report and a suggestion from Scott Moser) from the discussion on PR #234. Signed-off-by: Kurt Garloff <kurt@garloff.de>

garloff added 3 commits March 5, 2020 10:40

Avoid violating pycodestyle violation.

1f476de

It insists on ancient narrow terminals, so adjust ...

Merge remote-tracking branch 'upstream/master' into tolerate-more-cco…

556eb92

…nfig-mimetypes

smoser requested changes Mar 10, 2020

View reviewed changes

Merge branch 'master' into tolerate-more-cconfig-mimetypes

6687b96

OddBloke assigned raharper Mar 24, 2020

Merge branch 'master' into tolerate-more-cconfig-mimetypes

e786818

raharper requested changes Mar 25, 2020

View reviewed changes

OddBloke and others added 4 commits March 26, 2020 12:47

Merge branch 'master' into tolerate-more-cconfig-mimetypes

a2bb184

Merge branch 'master' into tolerate-more-cconfig-mimetypes

3640328

Revert "Avoid violating pycodestyle violation."

70ddf96

This reverts commit 1f476de.

Revert "tolerate more mime types for cc part of user-data."

7168657

This reverts commit b2f4dd3.

garloff mentioned this pull request Mar 30, 2020

Test acceptance of x-shellscript as cloud-config. #290

Merged

Merge branch 'master' into tolerate-more-cconfig-mimetypes

4960778

garloff closed this Mar 31, 2020

garloff deleted the tolerate-more-cconfig-mimetypes branch March 31, 2020 16:26

lucasmoura mentioned this pull request Jun 10, 2020

Manual verification for config in x-shellscript cloud-init/ubuntu-sru#111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tolerate more mime types for cc part of user-data. #234

tolerate more mime types for cc part of user-data. #234

garloff commented Mar 5, 2020

smoser left a comment

smoser Mar 10, 2020

garloff Mar 10, 2020

smoser Mar 11, 2020

garloff Mar 12, 2020

garloff Mar 13, 2020

raharper Mar 24, 2020

garloff Mar 25, 2020

garloff commented Mar 13, 2020

raharper commented Mar 24, 2020

garloff commented Mar 25, 2020

raharper commented Mar 25, 2020

raharper left a comment

garloff commented Mar 26, 2020

raharper commented Mar 27, 2020

garloff commented Mar 30, 2020

garloff commented Mar 31, 2020

tolerate more mime types for cc part of user-data. #234

tolerate more mime types for cc part of user-data. #234

Conversation

garloff commented Mar 5, 2020

smoser left a comment

Choose a reason for hiding this comment

smoser Mar 10, 2020

Choose a reason for hiding this comment

garloff Mar 10, 2020

Choose a reason for hiding this comment

smoser Mar 11, 2020

Choose a reason for hiding this comment

garloff Mar 12, 2020

Choose a reason for hiding this comment

garloff Mar 13, 2020

Choose a reason for hiding this comment

raharper Mar 24, 2020

Choose a reason for hiding this comment

garloff Mar 25, 2020

Choose a reason for hiding this comment

garloff commented Mar 13, 2020

raharper commented Mar 24, 2020

garloff commented Mar 25, 2020

raharper commented Mar 25, 2020

raharper left a comment

Choose a reason for hiding this comment

garloff commented Mar 26, 2020

raharper commented Mar 27, 2020

garloff commented Mar 30, 2020

garloff commented Mar 31, 2020