Encoding hard-coded in gettextutils.Message

Bug #1225099 reported by John Warren on 2013-09-13
28
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Ceilometer
High
Ben Nemec
Cinder
High
Doug Hellmann
OpenStack Compute (nova)
High
Mathew Odden
OpenStack Heat
Fix Released
High
Ben Nemec
OpenStack Identity (keystone)
High
Mathew Odden
neutron
High
Ben Nemec
oslo-incubator
High
Mathew Odden

Bug Description

The __str__ method in openstack.common.gettextutils.Message is hard-coded to encode to utf-8 which creates problems when other logic goes to decode the string using the default encoding, e.g. when formatting log messages. Message objects with double-byte translations fail to log because of this.

Fix proposed to branch: master
Review: https://review.openstack.org/46553

Changed in oslo:
assignee: nobody → John Warren (jswarren)
status: New → In Progress
Ben Nemec (bnemec) on 2013-09-23
Changed in oslo:
importance: Undecided → High
milestone: none → havana-rc1
Mark McLoughlin (markmc) wrote :

Since Nova calls gettextutils.enable_lazy(), I'm assuming this is potentially a serious issue for Nova ... so targeting to Nova's rc1

Changed in nova:
milestone: none → havana-rc1
importance: Undecided → High
status: New → Triaged
Changed in ceilometer:
milestone: none → havana-rc1
importance: Undecided → High
status: New → Confirmed
Mark McLoughlin (markmc) wrote :

ceilometer also calls enable_lazy()

Mark McLoughlin (markmc) wrote :

Ah, cinder calls gettextutils.install('cinder', lazy=True)

Changed in cinder:
status: New → Confirmed
importance: Undecided → High
milestone: none → havana-rc1
Changed in heat:
status: New → Confirmed
importance: Undecided → High
milestone: none → havana-rc1
Mark McLoughlin (markmc) wrote :

Heat calls gettextutils.install('heat', lazy=True)

Changed in neutron:
status: New → Confirmed
milestone: none → havana-rc1
Mark McLoughlin (markmc) wrote :

Neutron calls gettextutils.install('neutron', lazy=True)

Mark McLoughlin (markmc) wrote :

ttx, dhellman and I discussed this a little on irc - conclusions:

1) We're not 100% sure what the user impact of this issue is, but we suspect it has the potential to be a critical issue for some users. Is it the case that a translation containing double-byte characters can basically render a service completely unusable?

2) Our fear is that the fix is complex enough such that there's a reasonable chance it's broken in some way

i.e. this is a critical issue and we don't have a huge amount of confidence that this fix resolves the issue without introducing further issues

That all suggests that we should disable lazy translation in all projects and revisit in Icehouse

This would involve removing the calls to gettextutils.enable_lazy() or gettextutils.install(lazy=True)

But we're eager to hear thoughts from folks more intimately familiar with the work - e.g. mrodden, luisg, bnemec, jswarren

Luis A. Garcia (luisg-8) wrote :

Sorry we missed you on IRC Mark. We met with dhellman, and bnemec on IRC.

We all agree that the current proposal of basically changing the Message class at a fundamental level so that it is immutable is a no-go for Havana, given the complexity of the fix and risk for regressions.

However given how the issue is limited to logging, there is a different approach that was proposed by dhellman that addresses the specific problem with logging instead of having to drop the whole REST API translations feature.

To recap the problem, inside python logging the Message is converted first to string then to unicode, which breaks if the message contained non-ascii characters. It is converted first to string because that is the default behavior for non-basestring classes which Message is.

dhellman proposes creating a LoggerAdapter, very similar to ContextAdapter, that will unicode() the Message objects so the loggers don't deal with Messages.

We will write, test and send out a patch shortly.

Doug Hellmann (doug-hellmann) wrote :

We still need to look at jwarren's changeset to see if the Message class has any fundamental issues.

I also still think it is safer to disable the lazy evaluation in the other projects for now, and focus on fixing translation handling more completely during Icehouse.

Fix proposed to branch: master
Review: https://review.openstack.org/49230

Changed in oslo:
assignee: John Warren (jswarren) → Mathew Odden (locke105)
John Warren (jswarren) wrote :

My comments in my changeset were addressed to people who wanted to have Message extend unicode and keep the rest of the class basically in tact. I don't see a problem with keeping Message extending UserString and handling the issue via a logging Adapter as a tactical solution. I would recommend making the changes I suggested to Message class in Icehouse.

Fix proposed to branch: master
Review: https://review.openstack.org/49266

Changed in heat:
assignee: nobody → Ben Nemec (bnemec)
status: Confirmed → In Progress

Fix proposed to branch: master
Review: https://review.openstack.org/49275

Changed in cinder:
assignee: nobody → Doug Hellmann (doug-hellmann)
status: Confirmed → In Progress

Fix proposed to branch: master
Review: https://review.openstack.org/49278

Changed in ceilometer:
assignee: nobody → Ben Nemec (bnemec)
status: Confirmed → In Progress
Changed in oslo:
status: In Progress → Fix Committed

Reviewed: https://review.openstack.org/49230
Committed: http://github.com/openstack/oslo-incubator/commit/572cfecb4da5c618a407b8de2a7feae9c3f35ce4
Submitter: Jenkins
Branch: master

commit 572cfecb4da5c618a407b8de2a7feae9c3f35ce4
Author: Matt Odden <email address hidden>
Date: Tue Oct 1 05:11:43 2013 +0000

    Make Messages unicode before hitting logging

    Some of the log handlers in the python standard library
    are hitting issues with Messages since they check for
    basestring types only, and attempt to coerce anything else
    to string. This leads to Messages being coerced to bad encodings,
    resulting in unhandled exceptions.

    This change adds a check that looks for non-basestring
    objects and coerces them to unicode before they hit the problematic
    logging areas.

    bug 1225099

    Change-Id: I0bff6b52205e3c88db38876b8c6de1bd820f460a
    Co-authored-by: Luis A. Garcia <email address hidden>

Fix proposed to branch: master
Review: https://review.openstack.org/49287

Changed in neutron:
assignee: nobody → Ben Nemec (bnemec)
status: Confirmed → In Progress
Changed in neutron:
importance: Undecided → High
Mark McLoughlin (markmc) wrote :
Changed in nova:
assignee: nobody → Mathew Odden (locke105)
status: Triaged → In Progress
Thierry Carrez (ttx) on 2013-10-02
Changed in oslo:
status: Fix Committed → Fix Released
Mark McLoughlin (markmc) wrote :

We used this etherpad for coordinating: https://etherpad.openstack.org/disable-lazy-translation

Reviewed: https://review.openstack.org/49266
Committed: http://github.com/openstack/heat/commit/53b9314799c7d83779032c4535734f1c05326a27
Submitter: Jenkins
Branch: master

commit 53b9314799c7d83779032c4535734f1c05326a27
Author: Ben Nemec <email address hidden>
Date: Tue Oct 1 21:39:07 2013 +0000

    Disable lazy translation

    Late in the Havana cycle bug 1225099 was found in the lazy
    translation code, and to be safe it was decided to disable lazy
    translation for Havana. This change does that.

    Change-Id: Ia934a7df9386baf6ae8eb9ff48c24386c47ecd23
    Partial-bug: 1225099

Reviewed: https://review.openstack.org/49275
Committed: http://github.com/openstack/cinder/commit/52a98c29755f893154263a7793c0135c763938f6
Submitter: Jenkins
Branch: master

commit 52a98c29755f893154263a7793c0135c763938f6
Author: Doug Hellmann <email address hidden>
Date: Tue Oct 1 18:01:59 2013 -0400

    Disable lazy translation

    Late in the Havana cycle bug 1225099 was found in the lazy
    translation code, and to be safe it was decided to disable lazy
    translation for Havana. This change does that.

    Change-Id: Ia934a7df9386baf6ae8eb9ff48c24386c47ecd23
    Partial-bug: 1225099

Thierry Carrez (ttx) on 2013-10-02
Changed in heat:
status: In Progress → Fix Committed
Changed in cinder:
status: In Progress → Fix Committed

Reviewed: https://review.openstack.org/49278
Committed: http://github.com/openstack/ceilometer/commit/6840df8e5e48117438a8431bcf428343f988ddc4
Submitter: Jenkins
Branch: master

commit 6840df8e5e48117438a8431bcf428343f988ddc4
Author: Ben Nemec <email address hidden>
Date: Tue Oct 1 22:19:51 2013 +0000

    Disable lazy translation

    Late in the Havana cycle bug 1225099 was found in the lazy
    translation code, and to be safe it was decided to disable lazy
    translation for Havana. This change does that.

    Change-Id: Ia934a7df9386baf6ae8eb9ff48c24386c47ecd23
    Partial-bug: 1225099

Thierry Carrez (ttx) on 2013-10-02
Changed in ceilometer:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2013-10-02
Changed in ceilometer:
status: Fix Committed → Fix Released

Reviewed: https://review.openstack.org/49277
Committed: http://github.com/openstack/nova/commit/e355d476187951181ed644ebc2d0f7e858cb3cf4
Submitter: Jenkins
Branch: master

commit e355d476187951181ed644ebc2d0f7e858cb3cf4
Author: Matt Odden <email address hidden>
Date: Tue Oct 1 09:20:04 2013 +0000

    Disable lazy gettext

    This change disables lazy gettext functionality in Nova,
    due to problems with character encoding and logging
    detailed in bug 1225099.

    It is part of a series of commits across all projects with
    lazy gettext enabled.

    Change-Id: Ia934a7df9386baf6ae8eb9ff48c24386c47ecd23
    Partial-bug: 1225099

Thierry Carrez (ttx) on 2013-10-02
Changed in nova:
status: In Progress → Fix Committed

Reviewed: https://review.openstack.org/49287
Committed: http://github.com/openstack/neutron/commit/76e7542f0f54779f469df7125ff7b94480c44963
Submitter: Jenkins
Branch: master

commit 76e7542f0f54779f469df7125ff7b94480c44963
Author: Ben Nemec <email address hidden>
Date: Tue Oct 1 23:15:23 2013 +0000

    Disable lazy translation

    Late in the Havana cycle bug 1225099 was found in the lazy
    translation code, and to be safe it was decided to disable lazy
    translation for Havana. This change does that.

    Change-Id: Ia934a7df9386baf6ae8eb9ff48c24386c47ecd23
    Partial-bug: 1225099

Changed in neutron:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2013-10-03
Changed in nova:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2013-10-03
Changed in neutron:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2013-10-03
Changed in heat:
status: Fix Committed → Fix Released
Matt Riedemann (mriedem) wrote :
Changed in keystone:
status: New → In Progress
Changed in keystone:
assignee: nobody → Mathew Odden (locke105)
Thierry Carrez (ttx) on 2013-10-04
Changed in cinder:
status: Fix Committed → Fix Released
Dolph Mathews (dolph) on 2013-10-04
tags: added: havana-backport-potential

Reviewed: https://review.openstack.org/49285
Committed: http://github.com/openstack/keystone/commit/c3b92295b718a41c3136876eb39297081015a97c
Submitter: Jenkins
Branch: master

commit c3b92295b718a41c3136876eb39297081015a97c
Author: Matt Odden <email address hidden>
Date: Tue Oct 1 08:52:15 2013 +0000

    Disable lazy gettext

    This change disables lazy gettext functionality in Keystone,
    due to problems with character encoding and logging
    detailed in bug 1225099.

    It is part of a series of commits across all projects with
    lazy gettext enabled.

    Change-Id: Ia934a7df9386baf6ae8eb9ff48c24386c47ecd23
    Partial-bug: 1225099

Thierry Carrez (ttx) on 2013-10-08
tags: added: havana-rc-potential
removed: havana-backport-potential
Thierry Carrez (ttx) on 2013-10-15
tags: added: havana-backport-potential
removed: havana-rc-potential
Thierry Carrez (ttx) on 2013-10-16
Changed in keystone:
status: In Progress → Fix Committed
tags: added: havana-rc-potential
removed: havana-backport-potential
Changed in keystone:
importance: Undecided → High
Thierry Carrez (ttx) on 2013-10-16
Changed in keystone:
milestone: none → havana-rc4
Thierry Carrez (ttx) on 2013-10-16
tags: removed: havana-rc-potential
Thierry Carrez (ttx) on 2013-10-16
Changed in keystone:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2013-10-17
Changed in oslo:
milestone: havana-rc1 → 2013.2
Thierry Carrez (ttx) on 2013-10-17
Changed in ceilometer:
milestone: havana-rc1 → 2013.2
Thierry Carrez (ttx) on 2013-10-17
Changed in heat:
milestone: havana-rc1 → 2013.2
Thierry Carrez (ttx) on 2013-10-17
Changed in cinder:
milestone: havana-rc1 → 2013.2
Thierry Carrez (ttx) on 2013-10-17
Changed in neutron:
milestone: havana-rc1 → 2013.2
Thierry Carrez (ttx) on 2013-10-17
Changed in nova:
milestone: havana-rc1 → 2013.2
Thierry Carrez (ttx) on 2013-10-17
Changed in keystone:
milestone: havana-rc4 → 2013.2
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Related blueprints