Merge ~ahasenack/ubuntu/+source/base-files:eoan-base-file-handle-empty-cloud-id-1841597 into ubuntu/+source/base-files:ubuntu/devel

Proposed by Andreas Hasenack on 2019-08-27
Status: Merged
Approved by: Andreas Hasenack on 2019-08-30
Approved revision: f1d54c80e8841754b37eb7b17caf5d93157166e1
Merged at revision: f1d54c80e8841754b37eb7b17caf5d93157166e1
Proposed branch: ~ahasenack/ubuntu/+source/base-files:eoan-base-file-handle-empty-cloud-id-1841597
Merge into: ubuntu/+source/base-files:ubuntu/devel
Diff against target: 56 lines (+19/-5)
2 files modified
debian/changelog (+7/-0)
motd/50-motd-news (+12/-5)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  2019-08-27 Approve on 2019-08-28
Canonical Server Team 2019-08-27 Pending
Review via email: mp+371905@code.launchpad.net

Description of the change

Set cloud-id to unknown if the tool exited with a non-zero status, or printed an empty string.

I wanted to use set -o pipefail, so I could check the exit status of cloud-id in this line below, but dash doesn't have this option:

cloud_id=$(/usr/bin/cloud-id 2>/dev/null | cut -c -40 | tr -c -d '[:alnum:]')

Therefore I have to call cloud-id first, check $?, and then proceed if that is zero, and this is this branch. I checked if shell chars in the cloud-id output would break the script, and looks like they don't, at least in this test where I replaced /usr/bin/cloud-id with a script of mine, outputting all sorts of nasty things:

++ /usr/bin/cloud-id
$(id)'d_id='"\'\'';id nasty things
$(id)' ']'e 0 -o -z '"\'\'';id nasty things
$(id)'o '"\'\'';id nasty things
++ cut -c -40
++ tr -c -d '[:alnum:]'
+ cloud_id=idnastythingsid
+ read up idle
+ uptime=uptime/15283.00/15247.00
+ USER_AGENT='curl/7.65.3-1ubuntu1 Ubuntu/Eoan/Ermine/(development/branch) GNU/Linux/4.15.0-58-generic/x86_64 Intel(R)/Core(TM)/i7-7600U/CPU/@/2.80GHz uptime/15283.00/15247.00 cloud_id/idnastythingsid'

I also got a suggestion to use head or tail -n 1 in the cloud-id output, but I think it's best to have the tr call delete the \n and join multiple lines if that ever happens, which it does today. I rather not overoptimize this client-side branch, and deal with this on the server if I have to, unless it becomes a real problem.

Just to remember, the server side will do this kind of parsing:
+ RewriteEngine on
+ # list in the regexp all the clouds which should get motd support
+ RewriteCond "%{HTTP_USER_AGENT}" "cloud_id/(aws|azure|gce)\b"
+ RewriteRule "^/$" "/index-%1.txt" [L]

Specific for those 3 clouds at the moment, spelled like that. And if the corresponding index-$cloud.txt file doesn't exist, the 404 document is index.txt, which exists as that is the common motd message content.

To post a comment you must log in.
Christian Ehrhardt  (paelzer) wrote :

Two minor comments inline - one a potential rework here one a cloud-init bug/feature if you like it, but generally LGTM already +1

review: Approve
Andreas Hasenack (ahasenack) wrote :

I pushed a new alternative, using a tempfile for the cloud-id data, please take another look

Andreas Hasenack (ahasenack) wrote :

Lastly I renamed CLOUD_ID (the tempfile) to just CLOUD, to avoid confusion with the variable cloud_id.

Tagging and uploading f1d54c80e8841754b37eb7b17caf5d93157166e1

$ git push pkg upload/10.2ubuntu6
Enumerating objects: 14, done.
Counting objects: 100% (14/14), done.
Delta compression using up to 2 threads
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 1.30 KiB | 222.00 KiB/s, done.
Total 9 (delta 6), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/base-files
 * [new tag] upload/10.2ubuntu6 -> upload/10.2ubuntu6

$ dput ubuntu ../base-files_10.2ubuntu6_source.changes
Checking signature on .changes
gpg: ../base-files_10.2ubuntu6_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../base-files_10.2ubuntu6.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading base-files_10.2ubuntu6.dsc: done.
  Uploading base-files_10.2ubuntu6.tar.xz: done.
  Uploading base-files_10.2ubuntu6_source.buildinfo: done.
  Uploading base-files_10.2ubuntu6_source.changes: done.
Successfully uploaded packages.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 5054e04..c5dd058 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+base-files (10.2ubuntu6) eoan; urgency=medium
7+
8+ * motd/50-motd-news: set cloud-id to unknown if the tool exited with a
9+ non-zero status, or printed an empty string (LP: #1841597)
10+
11+ -- Andreas Hasenack <andreas@canonical.com> Tue, 27 Aug 2019 15:31:30 -0300
12+
13 base-files (10.2ubuntu5) eoan; urgency=medium
14
15 * motd/50-motd-news: add cloud_id to user-agent string (LP: #1840946)
16diff --git a/motd/50-motd-news b/motd/50-motd-news
17index 026429f..838eeb0 100644
18--- a/motd/50-motd-news
19+++ b/motd/50-motd-news
20@@ -67,7 +67,8 @@ fi
21 # Generate our temp files, clean up when done
22 NEWS=$(mktemp) || exit 1
23 ERR=$(mktemp) || exit 1
24-trap "rm -f $NEWS $ERR" HUP INT QUIT ILL TRAP KILL BUS TERM
25+CLOUD=$(mktemp) || exit 1
26+trap "rm -f $NEWS $ERR $CLOUD" HUP INT QUIT ILL TRAP KILL BUS TERM
27
28 # Construct a user agent, similar to Firefox/Chrome/Safari/IE to
29 # ensure a proper, tailored, accurate message of the day
30@@ -84,10 +85,16 @@ codename="$DISTRIB_CODENAME"
31 platform="$(uname -o)/$(uname -r)/$(uname -m)"
32 arch="$(uname -m)"
33 cpu="$(grep -m1 "^model name" /proc/cpuinfo | sed -e "s/.*: //" -e "s:\s\+:/:g")"
34+cloud_id="unknown"
35 if [ -x /usr/bin/cloud-id ]; then
36- cloud_id=$(/usr/bin/cloud-id 2>/dev/null | cut -c -40 | tr -c -d '[:alnum:]')
37-else
38- cloud_id="unknown"
39+ /usr/bin/cloud-id > "$CLOUD" 2>/dev/null
40+ if [ "$?" -eq "0" ]; then
41+ # sanitize it a bit, just in case
42+ cloud_id=$(cut -c -40 "${CLOUD}" | tr -c -d '[:alnum:]')
43+ if [ -z "${cloud_id}" ]; then
44+ cloud_id="unknown"
45+ fi
46+ fi
47 fi
48
49 # Some messages may only be pertinent before or after some amount of uptime
50@@ -124,5 +131,5 @@ for u in $URLS; do
51 : > "$CACHE"
52 fi
53 done
54-rm -f "$NEWS" "$ERR"
55+rm -f "$NEWS" "$ERR" "$CLOUD"
56 exit 0

Subscribers

People subscribed via source and target branches