Skip to content

Commit

Permalink
Make LocalSettingsContext more robust to priority
Browse files Browse the repository at this point in the history 
The relation data for for the LocalSettings context could cause the
priority sorting to break if the priority key wasn't cmpable (e.g. using
<, > or ==).  This patch fixes the associated bug, by making the sorting
extra robust and ensuring that un-cmp-able values are 'greater' (e.g.
further down the list) that cmp-able values, and equal to each other.
E.g. a partially ordered set.

This change also pins jsonschema in test-requirements.txt to fix
py310 failures due to newer versions of jsonschema pulling in rpds-py
and therefore the Rust toolchain.

Change-Id: I6bbf7e5f81a772ffc6ea859c9ab7c05f2eb9fdc5
Closes-bug: #2023404
(cherry picked from commit e8d0ca3)
  • Loading branch information
@ajkavanagh
ajkavanagh authored and Corey Bryant committed Jul 11, 2023
1 parent 7cb1d9f commit 4bb148c
Show file tree
Hide file tree
Showing 3 changed files with 182 additions and 2 deletions.
73 changes: 71 additions & 2 deletions hooks/horizon_contexts.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

# vim: set ts=4:et

import functools
import json

from charmhelpers.core.hookenv import (
Expand Down Expand Up @@ -382,11 +383,79 @@ def __call__(self):
ctxt = {
'settings': [
'# {0}\n{1}'.format(u, rd['local-settings'])
for u, rd in sorted(relations,
key=lambda r: r[1]['priority'])]
for u, rd in sorted(
relations,
key=functools.cmp_to_key(self._priority_sort_cmp))]
}
return ctxt

@staticmethod
def _priority_sort_cmp(unit_rdata_l, unit_rdata_r):
"""Sort two replates for __call__ .
This is used from functools.cmp_to_key() function.
It compares items in the (unit, rdata) as rdata['priority'] where
* No 'priority' value > one that is present; technically this isn't
possible, but is defensive here.
* None > that something that is present.
* types are int-ed() and attempted to be compared
* Two identical types are cmp-ed
* If types won't compare or can't be converted to an int, then they
are assumed to be non comparable and return 'equal' (0)
:param unit_rdata_l: the (unit, relation_data) on the left.
:type unit_data_l: Tuple[str, Any]
:param unit_rdata_r: the (unit, relation_data) on the right.
:type unit_data_r: Tuple[str, Any]
:returns: a cmp return value (-1 is l<r, +1 is l>r, 0 is l==r)
:rtype: int
"""
try:
priority_l = unit_rdata_l[1].get('priority')
if priority_l is None:
# cmp(None, Anything) is 1
return 1
except IndexError:
# cmp( <missing>, Anything) is 1: i.e. missing to end of list
return 1
try:
priority_r = unit_rdata_r[1]['priority']
if priority_r is None:
# cmp(Anything, None) is -1
return -1
except IndexError:
# cmp( Anything, <missing>) is -1: i.e. missing already at end
return -1

# define a _cmp function.
def _cmp(a, b):
if a < b:
return -1
elif a > b:
return 1
else:
return 0

# try converting to ints before doing strings, as the strings may be
# numbers.
try:
return _cmp(int(priority_l), int(priority_r))
except ValueError:
pass

# if they are the same type, try comparing them.
if type(priority_l) == type(priority_r):
try:
return _cmp(priority_l, priority_r)
except TypeError:
# types don't support comparisions
return 0
# different types, that won't become ints, and also don't support
# so just say they are the same.
return 0


class WebSSOFIDServiceProviderContext(OSContextGenerator):
interfaces = ['websso-fid-service-provider']
Expand Down
2 changes: 2 additions & 0 deletions test-requirements.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,5 @@ git+https://github.com/openstack-charmers/zaza-openstack-tests.git@stable/zed#eg
git+https://opendev.org/openstack/tempest.git#egg=tempest

croniter # needed for charm-rabbitmq-server unit tests

jsonschema<4.18.0 # jsonschema 4.18.0 depends on Rust (via rpds-py)
109 changes: 109 additions & 0 deletions unit_tests/test_horizon_contexts.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1363,6 +1363,115 @@ def test_LocalSettingsContextJSON(self):
'# horizon-plugin/0\n'
'FOO = True']})

def test_LocalSettingsContext_unusual_priority(self):
# First, left priority missing.
self.related_units.side_effect = [['horizon-plugin/0'],
['horizon-plugin-too/0']]
self.relation_get.side_effect = [{'local-settings': 'FOO = True'},
{'priority': 60,
'local-settings': 'BAR = False'}]
self.assertEqual(horizon_contexts.LocalSettingsContext()(),
{'settings': []})
# First, right priority missing.
self.related_units.side_effect = [['horizon-plugin/0'],
['horizon-plugin-too/0']]
self.relation_get.side_effect = [{'priority': 99,
'local-settings': 'FOO = True'},
{'local-settings': 'BAR = False'}]
self.assertEqual(horizon_contexts.LocalSettingsContext()(),
{'settings': []})
# Left priority is None.
self.relation_ids.return_value = ['plugin:0', 'plugin-too:0']
self.related_units.side_effect = [['horizon-plugin/0'],
['horizon-plugin-too/0']]
self.relation_get.side_effect = [{'priority': None,
'local-settings': 'FOO = True'},
{'priority': 60,
'local-settings': 'BAR = False'}]
self.assertEqual(horizon_contexts.LocalSettingsContext()(),
{'settings': ['# horizon-plugin-too/0\n'
'BAR = False',
'# horizon-plugin/0\n'
'FOO = True']})
# Right priority is None.
self.relation_ids.return_value = ['plugin:0', 'plugin-too:0']
self.related_units.side_effect = [['horizon-plugin/0'],
['horizon-plugin-too/0']]
self.relation_get.side_effect = [{'priority': 99,
'local-settings': 'FOO = True'},
{'priority': None,
'local-settings': 'BAR = False'}]
self.assertEqual(horizon_contexts.LocalSettingsContext()(),
{'settings': ['# horizon-plugin/0\n'
'FOO = True',
'# horizon-plugin-too/0\n'
'BAR = False']})
# Left priority is stringy number.
self.relation_ids.return_value = ['plugin:0', 'plugin-too:0']
self.related_units.side_effect = [['horizon-plugin/0'],
['horizon-plugin-too/0']]
self.relation_get.side_effect = [{'priority': "99",
'local-settings': 'FOO = True'},
{'priority': 60,
'local-settings': 'BAR = False'}]
self.assertEqual(horizon_contexts.LocalSettingsContext()(),
{'settings': ['# horizon-plugin-too/0\n'
'BAR = False',
'# horizon-plugin/0\n'
'FOO = True']})
# Right priority is stringy number.
self.relation_ids.return_value = ['plugin:0', 'plugin-too:0']
self.related_units.side_effect = [['horizon-plugin/0'],
['horizon-plugin-too/0']]
self.relation_get.side_effect = [{'priority': 99,
'local-settings': 'FOO = True'},
{'priority': "60",
'local-settings': 'BAR = False'}]
self.assertEqual(horizon_contexts.LocalSettingsContext()(),
{'settings': ['# horizon-plugin-too/0\n'
'BAR = False',
'# horizon-plugin/0\n'
'FOO = True']})
# Both priorities are strings
self.relation_ids.return_value = ['plugin:0', 'plugin-too:0']
self.related_units.side_effect = [['horizon-plugin/0'],
['horizon-plugin-too/0']]
self.relation_get.side_effect = [{'priority': "99",
'local-settings': 'FOO = True'},
{'priority': "60",
'local-settings': 'BAR = False'}]
self.assertEqual(horizon_contexts.LocalSettingsContext()(),
{'settings': ['# horizon-plugin-too/0\n'
'BAR = False',
'# horizon-plugin/0\n'
'FOO = True']})
# Left priority is weired json object
self.relation_ids.return_value = ['plugin:0', 'plugin-too:0']
self.related_units.side_effect = [['horizon-plugin/0'],
['horizon-plugin-too/0']]
self.relation_get.side_effect = [{'priority': "{'a': 1}",
'local-settings': 'FOO = True'},
{'priority': "60",
'local-settings': 'BAR = False'}]
self.assertEqual(horizon_contexts.LocalSettingsContext()(),
{'settings': ['# horizon-plugin-too/0\n'
'BAR = False',
'# horizon-plugin/0\n'
'FOO = True']})
# right priority is weired json object
self.relation_ids.return_value = ['plugin:0', 'plugin-too:0']
self.related_units.side_effect = [['horizon-plugin/0'],
['horizon-plugin-too/0']]
self.relation_get.side_effect = [{'priority': "99",
'local-settings': 'FOO = True'},
{'priority': "[1,2,3]",
'local-settings': 'BAR = False'}]
self.assertEqual(horizon_contexts.LocalSettingsContext()(),
{'settings': ['# horizon-plugin/0\n'
'FOO = True',
'# horizon-plugin-too/0\n'
'BAR = False']})

def test_WebSSOFIDServiceProviderContext(self):
def relation_ids_side_effect(rname):
return {
Expand Down

0 comments on commit 4bb148c

Please sign in to comment.