-
Notifications
You must be signed in to change notification settings - Fork 879
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.
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
tolerate more mime types for cc part of user-data. #234
tolerate more mime types for cc part of user-data. #234
Conversation
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 ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least would like feedback from @blackboxsw or someone else to collaborate here.
cloudinit/user_data.py
Outdated
@@ -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"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be safe by checking for the #cloud-config header also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I will test whether your proposed change will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garloff is the comment below the user-data (the multi-part mime)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garloff is the comment below the user-data (the multi-part mime)?
Yes.
|
Is this the hunk that's not working? |
Correct, this piece is ignored without the patch. |
Thanks! I've reproduced the issue like so:
The error message shows up in cloud-init.log
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
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>
Excellent! Thank you for verifying.
IMHO, the easiest path is to create a new PR. I'm also fine
I can do that, but I'd prefer you to have credit for finding If you do want to rebase or start a new PR, this is a unittest
|
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>
There is PR#290 now, where I will try to provide a clean history. |
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>
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>
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