Merge ~vorlon/cloud-init:lp.1134036 into cloud-init:master

Proposed by Steve Langasek
Status: Work in progress
Proposed branch: ~vorlon/cloud-init:lp.1134036
Merge into: cloud-init:master
Diff against target: 133 lines (+32/-66)
1 file modified
tools/Z99-cloud-locale-test.sh (+32/-66)
Reviewer Review Type Date Requested Status
Scott Moser Needs Information
Server Team CI bot continuous-integration Approve
Review via email: mp+348101@code.launchpad.net

Commit message

Fix up invalid locales in the login environment; don't print warning messages.

Invalid locales in the environment at login (e.g. as provided by ssh
SendEnv from a client system which has a different set of available locales
than the server) currently result in a large warning message (once, per
login user, per instance) and a broken environment.

On Ubuntu systems, and probably others, this broken environment results in
wrong runtime behavior vs if no locale had been passed at all by the client;
in particular, Ubuntu systems will give UTF-8 as a character set by default
(either the en_US.UTF-8 or C.UTF-8 locale, depending on the version of
Ubuntu), and if an invalid locale is specified, the login instead will use
ASCII as the only supported character set.

This change reworks cloud-init's profile handling to:
 - fix up any invalid locale settings detected in the environment, so that
   the login session does not unnecessarily lose non-ascii character support
   due to a requested missing locale
 - remove the warning message, which is of disputed value.

This approximates the OpenSSH upstream guidance regarding a correct design
for this in OpenSSH itself, as described at
<https://bugzilla.mindrot.org/show_bug.cgi?id=1346#c38>, but will work on
all systems which do not have an ssh client+server that implement that does
not implement this proposed design (i.e.: all clients and servers currently
in existence).

LP: #1134036

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

FAILED: Continuous integration, rev:70743abe09a3f20d4c20995ea745fff49edf3edc
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/~vorlon/cloud-init/+git/cloud-init/+merge/348101/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/97/
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/97/rebuild

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

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

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

First thought, a question:
  Would it be better to put this into base_files or even openssh-server ?
The scenario is not at all related to cloud-init. I'd be just as happy
to have cloud-init disable its behavior based on a fix present in another
package.

You need to fix the commit message.
lines should only be 74 chars wide.

I suggest for a subject line:
 Z99-cloud-locale-test: Fix invalid locale settings, do not print messages.

http://paste.ubuntu.com/p/s8dgFRnx43/

I've played around a bit and suggest the above.

Things I've changed there:

 * fix some complaints from shellcheck
 * make 'locale_fixup' take a default locale as param 1, using UTF-8.C as the default.
 * Avoided the double 'case' by just checking for empty 'val' or val that starts with '"'.
   I admit the primary reason for this is that 'vim' syntax highlighting didn't like
   the \"*\" that you had. My replaced version seems to be slightly faster, but
   that speedup is nominal compared to the subshells involved in $(locale | function)
 * I dropped the 'LANG' section because I can't figure out how to trip it.
   Also here the check for presense of 'bad' in 'bad_set' is now safer. if both
   LANG and LANGUAGE ever *did* make it into the 'bad_set', then yours would have
   LANG match LAUNGUAGE.
 * append to a variable and a single 'echo' rather than multiple lines in output.

Revision history for this message
Scott Moser (smoser) wrote :

just realized... if we *did* put a fix like this in any other package than cloud-init, cloud-init's existing code would just start doing nothing (no warning).

as long as the other package's /etc/profile.d/ script ran before cloud-init's, when cloud-init looked at the locale all would be good.

Revision history for this message
Scott Moser (smoser) :
review: Needs Information

Unmerged commits

70743ab... by Steve Langasek

Fix up invalid locales in the login environment; don't print warning messages.

Invalid locales in the environment at login (e.g. as provided by ssh
SendEnv from a client system which has a different set of available locales
than the server) currently result in a large warning message (once, per
login user, per instance) and a broken environment.

On Ubuntu systems, and probably others, this broken environment results in
wrong runtime behavior vs if no locale had been passed at all by the client;
in particular, Ubuntu systems will give UTF-8 as a character set by default
(either the en_US.UTF-8 or C.UTF-8 locale, depending on the version of
Ubuntu), and if an invalid locale is specified, the login instead will use
ASCII as the only supported character set.

This change reworks cloud-init's profile handling to:
 - fix up any invalid locale settings detected in the environment, so that
   the login session does not unnecessarily lose non-ascii character support
   due to a requested missing locale
 - remove the warning message, which is of disputed value.

This approximates the OpenSSH upstream guidance regarding a correct design
for this in OpenSSH itself, as described at
<https://bugzilla.mindrot.org/show_bug.cgi?id=1346#c38>, but will work on
all systems which do not have an ssh client+server that implement that does
not implement this proposed design (i.e.: all clients and servers currently
in existence).

LP: #1134036

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/tools/Z99-cloud-locale-test.sh b/tools/Z99-cloud-locale-test.sh
2index 4978d87..38cfd05 100644
3--- a/tools/Z99-cloud-locale-test.sh
4+++ b/tools/Z99-cloud-locale-test.sh
5@@ -3,16 +3,19 @@
6 #
7 # Author: Ben Howard <ben.howard@canonical.com>
8 # Author: Scott Moser <scott.moser@ubuntu.com>
9-# (c) 2012, Canonical Group, Ltd.
10+# Author: Steve Langasek <steve.langasek@ubuntu.com>
11+# (c) 2012, 2018, Canonical Group, Ltd.
12 #
13 # This file is part of cloud-init. See LICENSE file for license information.
14
15-# Purpose: Detect invalid locale settings and inform the user
16-# of how to fix them.
17+# Purpose: Ensure the user has a valid locale upon remote login.
18
19-locale_warn() {
20+locale_fixup() {
21 local bad_names="" bad_lcs="" key="" val="" var="" vars="" bad_kv=""
22 local w1 w2 w3 w4 remain
23+ # if we are given an invalid locale, try to set to a
24+ # least-common-denominator UTF-8 locale for maximal compatibility.
25+ local default_locale=C.UTF-8
26
27 # if shell is zsh, act like sh only for this function (-L).
28 # The behavior change will not permenently affect user's shell.
29@@ -22,79 +25,42 @@ locale_warn() {
30 # VARIABLE=
31 # VARIABLE="value"
32 # locale: Cannot set LC_SOMETHING to default locale
33+ # glibc's 'locale' command outputs VARIABLE=foo for variables set in
34+ # the environment, and VARIABLE="foo" for locale settings that are
35+ # derived. Use this to minimize the set of variables we are overriding
36+ # to those that are specifically detected to be broken.
37 while read -r w1 w2 w3 w4 remain; do
38 case "$w1" in
39 locale:) bad_names="${bad_names} ${w4}";;
40 *)
41 key=${w1%%=*}
42 val=${w1#*=}
43- val=${val#\"}
44- val=${val%\"}
45- vars="${vars} $key=$val";;
46+ case $val in
47+ \"*\"|"")
48+ # quoted means inferred; don't override it,
49+ # nor an empty value
50+ ;;
51+ *)
52+ set_vars="$key $set_vars"
53+ ;;
54+ esac
55 esac
56 done
57 for bad in $bad_names; do
58- for var in ${vars}; do
59- [ "${bad}" = "${var%=*}" ] || continue
60- val=${var#*=}
61- [ "${bad_lcs#* ${val}}" = "${bad_lcs}" ] &&
62- bad_lcs="${bad_lcs} ${val}"
63- bad_kv="${bad_kv} $bad=$val"
64- break
65- done
66- done
67- bad_lcs=${bad_lcs# }
68- bad_kv=${bad_kv# }
69- [ -n "$bad_lcs" ] || return 0
70-
71- printf "_____________________________________________________________________\n"
72- printf "WARNING! Your environment specifies an invalid locale.\n"
73- printf " The unknown environment variables are:\n %s\n" "$bad_kv"
74- printf " This can affect your user experience significantly, including the\n"
75- printf " ability to manage packages. You may install the locales by running:\n\n"
76-
77- local bad invalid="" to_gen="" sfile="/usr/share/i18n/SUPPORTED"
78- local pkgs=""
79- if [ -e "$sfile" ]; then
80- for bad in ${bad_lcs}; do
81- grep -q -i "${bad}" "$sfile" &&
82- to_gen="${to_gen} ${bad}" ||
83- invalid="${invalid} ${bad}"
84- done
85- else
86- printf " sudo apt-get install locales\n"
87- to_gen=$bad_lcs
88- fi
89- to_gen=${to_gen# }
90-
91- local pkgs=""
92- for bad in ${to_gen}; do
93- pkgs="${pkgs} language-pack-${bad%%_*}"
94- done
95- pkgs=${pkgs# }
96-
97- if [ -n "${pkgs}" ]; then
98- printf " sudo apt-get install ${pkgs# }\n"
99- printf " or\n"
100- printf " sudo locale-gen ${to_gen# }\n"
101- printf "\n"
102- fi
103- for bad in ${invalid}; do
104- printf "WARNING: '${bad}' is an invalid locale\n"
105+ case $set_vars in
106+ *$bad*)
107+ echo "$bad=$default_locale"
108+ echo export $bad
109+ ;;
110+ *LANG\ *)
111+ echo "LANG=$default_locale"
112+ echo export LANG
113+ ;;
114+ esac
115 done
116-
117- printf "To see all available language packs, run:\n"
118- printf " apt-cache search \"^language-pack-[a-z][a-z]$\"\n"
119- printf "To disable this message for all users, run:\n"
120- printf " sudo touch /var/lib/cloud/instance/locale-check.skip\n"
121- printf "_____________________________________________________________________\n\n"
122-
123- # only show the message once
124- : > ~/.cloud-locale-test.skip 2>/dev/null || :
125 }
126
127-[ -f ~/.cloud-locale-test.skip -o -f /var/lib/cloud/instance/locale-check.skip ] ||
128- locale 2>&1 | locale_warn
129+eval $(locale 2>&1 | locale_fixup)
130
131-unset locale_warn
132+unset locale_fixup
133 # vi: ts=4 expandtab

Subscribers

People subscribed via source and target branches