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

Subscribers

People subscribed via source and target branches