distrib_codename alpha comparison will soon yield unintended behavior

Bug #1659575 reported by Ryan Beisner on 2017-01-26
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Charm Helpers
High
Alex Kavanagh
OpenStack keystone charm
High
Alex Kavanagh
OpenStack nova-cloud-controller charm
High
Alex Kavanagh
OpenStack nova-compute charm
High
Alex Kavanagh
nova-cloud-controller (Juju Charms Collection)
High
Unassigned
nova-compute (Juju Charms Collection)
High
Unassigned

Bug Description

Presuming that after 'z' will come 'a' - with regard to distro codename, several charms are about to have unintended behavior due to the use of alpha comparison logic. The numeric version comparison should be used instead.

I've not done a full audit of all OpenStack Charms but that needs to be done as soon as possible, and associated with this bug.

Here are a some examples which need to be fixed:

https://github.com/openstack/charm-nova-compute/search?utf8=%E2%9C%93&q=distro_codename

if distro_codename < "wily":

if distro_codename >= 'yakkety':

.

Here is an example of the safe way to do it:

https://github.com/openstack/charm-nova-cloud-controller/search?utf8=%E2%9C%93&q=DISTRIB_RELEASE&type=Code

if lsb_release()['DISTRIB_RELEASE'] >= '15.10':

Related branches

Ryan Beisner (1chb1n) wrote :

Some of these alpha comparisons are in charm repos (ie. hook code), and some of them are in charmhelpers.

Here are some examples of trouble-to-come from charmhelpers:

contrib/openstack/context.py: if lsb_release()['DISTRIB_CODENAME'].lower() > 'trusty':

contrib/hardening/ssh/checks/config.py: if lsb_release()['DISTRIB_CODENAME'].lower() >= 'trusty':

Alex Kavanagh (ajkavanagh) wrote :

I'm going to be the one to say it, but this was a bad idea from the start. charms.openstack mostly implements indexed listed comparisons - e.g. releases.index(release) > releases.index('trusty').

I'm guessing an audit of the charms, charm-helpers and then fix?

Ryan Beisner (1chb1n) on 2017-01-26
description: updated
James Page (james-page) wrote :

"I'm going to be the one to say it, but this was a bad idea from the start."

Agreed and I like the way you solved this issue for charms.openstack, so lets follow that route!

Changed in nova-compute (Juju Charms Collection):
status: New → Triaged
importance: Undecided → High
Changed in nova-cloud-controller (Juju Charms Collection):
importance: Undecided → High
status: New → Triaged
Changed in charm-helpers:
importance: Undecided → High
status: New → Triaged
James Page (james-page) on 2017-02-23
Changed in charm-nova-compute:
importance: Undecided → High
status: New → Triaged
Changed in nova-compute (Juju Charms Collection):
status: Triaged → Invalid
James Page (james-page) on 2017-02-23
Changed in charm-nova-cloud-controller:
importance: Undecided → High
status: New → Triaged
Changed in nova-cloud-controller (Juju Charms Collection):
status: Triaged → Invalid
Changed in charm-helpers:
assignee: nobody → Alex Kavanagh (ajkavanagh)
Changed in charm-nova-cloud-controller:
assignee: nobody → Alex Kavanagh (ajkavanagh)
Changed in charm-nova-compute:
assignee: nobody → Alex Kavanagh (ajkavanagh)
Changed in charm-keystone:
importance: Undecided → High
assignee: nobody → Alex Kavanagh (ajkavanagh)
status: New → In Progress
Changed in charm-helpers:
status: Triaged → Fix Committed
Changed in charm-helpers:
status: Fix Committed → In Progress

Reviewed: https://review.openstack.org/453276
Committed: https://git.openstack.org/cgit/openstack/charm-nova-compute/commit/?id=2358f94b30349a8543e75179750d1d61dba03a9f
Submitter: Jenkins
Branch: master

commit 2358f94b30349a8543e75179750d1d61dba03a9f
Author: Alex Kavanagh <email address hidden>
Date: Tue Apr 4 18:53:20 2017 +0100

    Fix alphanumeric comparisons for openstack and ubuntu releases

    - sync charmhelpers with fix-alpha helpers
    - fix up code where the alpha comparisons are done
    - fix tests which assumed mocks would just work on os_release()

    Change-Id: Iffd4ed0aecbb966fe493fe0f24bf2422aab55bb6
    Related-Bug: #1659575

Changed in charm-nova-cloud-controller:
status: Triaged → In Progress
Changed in charm-nova-compute:
status: Triaged → In Progress

Reviewed: https://review.openstack.org/447457
Committed: https://git.openstack.org/cgit/openstack/charm-keystone/commit/?id=b8b5acd44d5ecf7490b31802cf2459c215a95822
Submitter: Jenkins
Branch: master

commit b8b5acd44d5ecf7490b31802cf2459c215a95822
Author: Alex Kavanagh <email address hidden>
Date: Mon Mar 20 11:37:16 2017 +0000

    Fix alphanumeric comparisons for openstack and ubuntu releases

    - sync charmhelpers with fix-alpha helpers
    - fix up code where the alpha comparisons are done
    - fix tests which assumed mocks would just work on os_release()

    Change-Id: I9f4a3b15e53c757c2ae5ffb2eb45b6cdaecf4c8e
    Related-Bug: #1659575

Reviewed: https://review.openstack.org/453261
Committed: https://git.openstack.org/cgit/openstack/charm-nova-cloud-controller/commit/?id=f9b97ac66c8dd09349e325536fdeee0569abde0c
Submitter: Jenkins
Branch: master

commit f9b97ac66c8dd09349e325536fdeee0569abde0c
Author: Alex Kavanagh <email address hidden>
Date: Tue Apr 4 17:53:31 2017 +0100

    Fix alphanumeric comparisons for openstack and ubuntu releases

    - sync charmhelpers with fix-alpha helpers
    - fix up code where the alpha comparisons are done
    - fix tests which assumed mocks would just work on os_release()

    Change-Id: I48b4853ed343bd3188c16f2bc432b4ca0badc473
    Related-Bug: #1659575

Changed in charm-helpers:
status: In Progress → Fix Released
Changed in charm-keystone:
status: In Progress → Fix Committed
milestone: none → 17.08
Changed in charm-nova-cloud-controller:
status: In Progress → Fix Committed
milestone: none → 17.08
Changed in charm-nova-compute:
status: In Progress → Fix Committed
milestone: none → 17.08
James Page (james-page) on 2017-09-12
Changed in charm-nova-compute:
status: Fix Committed → Fix Released
Changed in charm-nova-cloud-controller:
status: Fix Committed → Fix Released
Changed in charm-keystone:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers