Merge ~raharper/cloud-init:fix/debian-config-yaml-spaces into cloud-init:ubuntu/devel

Proposed by Ryan Harper
Status: Merged
Approved by: Dan Watkins
Approved revision: 6e6be6a5dee8b24b61f18e6d6c208255d681e87e
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~raharper/cloud-init:fix/debian-config-yaml-spaces
Merge into: cloud-init:ubuntu/devel
Diff against target: 23 lines (+6/-5)
1 file modified
debian/cloud-init.config (+6/-5)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve
Chad Smith Approve
Review via email: mp+371919@code.launchpad.net

Commit message

debian/cloud-init.config: fix yaml parsing with whitespace

Users may rewrite valid yaml adding or omitting additional white space
when defining the 'datasources_list' variable. If the yaml did not
include a space between the start of the list '[' and the members and
the end ']' then this would break the shell yaml parsing and produce
invalid yaml on dpkg-reconfigure cloud-init. Fix this issue by updating
the sed matching expression to allow for zero or more spaces after the
leading bracket and before the trailing bracket. For example, this
branch now accepts:

  datasources_list: [NoCloud, None]

LP: #1841697

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:3d47f726cc4526c778ddaf89ef288513edbd33ab
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1087/
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/1087//rebuild

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

fwiw, there is 'parse_yaml_array', in tools/ds-identify, which does better (at least did not have this bug). Unfortunately that uses functions 'trim' and 'unquote', so you can't just grab the single function.

Second option would be to first *try* to use a proper yaml parser (python). There are some rules about not using dependencies in a config script I think (only essential maybe?). Thats why we don't use cloudinit infra to load it. So you can't *depend* on python and yaml but you could try.

Here is an example that could be added to your suggested fix:

try_python_yaml() {
    # try to load something like 'datasource_list: [xxx]' and write space delimeted
    # values to stdout
    local py out="" fname="$1"
    command -v python3 >/dev/null || return
    out=$(python3 -c '
import yaml, sys;
print(",".join(yaml.load(sys.stdin.read()).get("datasource_list")))' \
    < "$fname"
    ) || return
    echo "$out"
}

Revision history for this message
Scott Moser (smoser) :
Revision history for this message
Chad Smith (chad.smith) :
Revision history for this message
Ryan Harper (raharper) wrote :

Thanks for the updated sed script Chad, going with that.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:ec8979444b661e337c84f91e415544599ea1ef81
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1096/
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/1096//rebuild

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) :
Revision history for this message
Ryan Harper (raharper) wrote :

OK, I've combined Chad's clean-up and then emitted that as the value, like this:

LISTVAL="$(sed -e "s/\s//g" -e "/^$key:/"'!'d \
               -e "s/$key:\[//;s/]//;s/,/, /g" "$file")"
RET="$key: [ ${LISTVAL} ]"

In action looks like this:

# Edge-case, single entry
root@x1:~# cat /etc/cloud/cloud.cfg.d/90_dpkg.cfg
# to update this file, run dpkg-reconfigure cloud-init
datasource_list: [ ConfigDrive ]
root@x1:~# ./test3.sh /etc/cloud/cloud.cfg.d/90_dpkg.cfg
datasource_list: [ ConfigDrive ]

# Edge-case, empty
root@x1:~# cat /etc/cloud/cloud.cfg.d/90_dpkg.cfg.empty
# to update this file, run dpkg-reconfigure cloud-init
datasource_list: []
root@x1:~# ./test3.sh /etc/cloud/cloud.cfg.d/90_dpkg.cfg.empty
datasource_list: [ ]

# the bug case
root@x1:~# cat /etc/cloud/cloud.cfg.d/90_dpkg.cfg.bad
# to update this file, run dpkg-reconfigure cloud-init
datasource_list: [ConfigDrive, None]
root@x1:~# ./test3.sh /etc/cloud/cloud.cfg.d/90_dpkg.cfg.bad
datasource_list: [ ConfigDrive, None ]

# the default case
root@x1:~# cat /etc/cloud/cloud.cfg.d/90_dpkg.cfg.new
s file, run dpkg-reconfigure cloud-init
datasource_list: [ NoCloud, ConfigDrive, OpenNebula, DigitalOcean, Azure, AltCloud, OVF, MAAS, GCE, OpenStack, CloudSigma, SmartOS, Bigstep, Scaleway, AliYun, Ec2, CloudStack, Hetzner, IBMCloud, Oracle, Exoscale, None]
root@x1:~# ./test3.sh /etc/cloud/cloud.cfg.d/90_dpkg.cfg.new
datasource_list: [ NoCloud, ConfigDrive, OpenNebula, DigitalOcean, Azure, AltCloud, OVF, MAAS, GCE, OpenStack, CloudSigma, SmartOS, Bigstep, Scaleway, AliYun, Ec2, CloudStack, Hetzner, IBMCloud, Oracle, Exoscale, None ]

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:6e6be6a5dee8b24b61f18e6d6c208255d681e87e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1097/
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/1097//rebuild

review: Approve (continuous-integration)
Revision history for this message
Chad Smith (chad.smith) wrote :

Verified in an lxc with
apt-get remove --purge cloud-init
# edit /etc/cloud/cloud.cfg.d/90_dpkg.cfg to muck with spacing of datasource_list
dpkg -i rharper's cloud-init
validate fixed standard spacing and brackets of datasource_list: [ 1, 2, 3 ]

review: Approve
Revision history for this message
Chad Smith (chad.smith) wrote :

good one. Tested again and works.

review: Approve
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

Commit message lints:
- Line #3 has 1 too many characters. Line starts with: "when defining the 'datasources_list'"...- Line #4 has 4 too many characters. Line starts with: "a space between the start"...

review: Needs Fixing
Revision history for this message
Server Team CI bot (server-team-bot) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/cloud-init.config b/debian/cloud-init.config
2index 6e9c6f7..af38ffb 100644
3--- a/debian/cloud-init.config
4+++ b/debian/cloud-init.config
5@@ -32,13 +32,14 @@ hasEc2Md() {
6 get_yaml_list() {
7 # get_yaml_list(file, key, def): return a comma delimited list with the value
8 # for the yaml array defined in 'key' from 'file'. if not found , return 'def'
9- # only really supports 'key: [en1, en2 ]' format.
10+ # only really supports 'key: [ en1, en2 ]' or 'key: [en1, en2]' formats.
11 local file="$1" key="$2" default="$3"
12 [ -f "$file" ] || return 1
13- # any thing that didn't match the key is deleted so the final 'p' only
14- # prints things that matched.
15- RET=$(sed -n -e "/^$key:/"'!'d -e "s/$key:[ \[]*//"\
16- -e "s, \]$,," -e p "$file")
17+ # strip all whitespace, delete lines not matching key:,
18+ # strip key: and [] and replace ',' with ', '
19+ LISTVAL="$(sed -e "s/\s//g" -e "/^$key:/"'!'d \
20+ -e "s/$key:\[//" -e "s/]//" -e "s/,/, /g" "$file")"
21+ RET="$key: [ ${LISTVAL} ]"
22 [ -n "$RET" ] || RET="$default"
23 }
24

Subscribers

People subscribed via source and target branches