From b8dfc25ae511c7e0b1a8b30bc8375a76b1c7e5be Mon Sep 17 00:00:00 2001 From: David Ames Date: Fri, 22 Mar 2019 23:24:33 +0000 Subject: [PATCH] Standardize reactive practices Rely on updated interfaces. Use standard practices for endpoint handling. Remove unused code. --- .gitignore | 2 + .../charm/openstack/keystone_saml_mellon.py | 72 ++++++----- src/reactive/keystone_saml_mellon_handlers.py | 119 +++++------------- .../test_keystone_saml_mellon_handlers.py | 63 +++------- ...ib_charm_openstack_keystone_saml_mellon.py | 42 +++---- 5 files changed, 108 insertions(+), 190 deletions(-) diff --git a/.gitignore b/.gitignore index 2aaec7a..875e8f2 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,5 @@ *.pyc build src/tests/bundles/overlays/local-charm-overlay.yaml.j2 +interfaces +layers diff --git a/src/lib/charm/openstack/keystone_saml_mellon.py b/src/lib/charm/openstack/keystone_saml_mellon.py index 94fc428..b49f59b 100644 --- a/src/lib/charm/openstack/keystone_saml_mellon.py +++ b/src/lib/charm/openstack/keystone_saml_mellon.py @@ -16,14 +16,16 @@ import charmhelpers.core as core import charmhelpers.core.host as ch_host import charmhelpers.core.hookenv as hookenv -import charmhelpers.core.unitdata as unitdata import charmhelpers.contrib.openstack.templating as os_templating -import charmhelpers.contrib.openstack.utils as os_utils import charms_openstack.charm import charms_openstack.adapters +from charms.reactive.relations import ( + endpoint_from_flag, +) + import os import subprocess @@ -31,14 +33,6 @@ from lxml import etree from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import serialization -# release detection is done via keystone package given that -# openstack-origin is not present in the subordinate charm -# see https://github.com/juju/charm-helpers/issues/83 -from charms_openstack.charm.core import ( - register_os_release_selector -) -OPENSTACK_RELEASE_KEY = 'charmers.openstack-release-version' - CONFIGS = (IDP_METADATA, SP_METADATA, SP_PRIVATE_KEY, SP_LOCATION_CONFIG,) = [ os.path.join('/etc/apache2/mellon', @@ -48,20 +42,7 @@ CONFIGS = (IDP_METADATA, SP_METADATA, SP_PRIVATE_KEY, 'sp-pk.{}.pem', 'sp-location.{}.conf']] - -@register_os_release_selector -def select_release(): - """Determine the release based on the keystone package version. - - Note that this function caches the release after the first install so - that it doesn't need to keep going and getting it from the package - information. - """ - release_version = unitdata.kv().get(OPENSTACK_RELEASE_KEY, None) - if release_version is None: - release_version = os_utils.os_release('keystone') - unitdata.kv().set(OPENSTACK_RELEASE_KEY, release_version) - return release_version +KEYSTONE_FID_ENDPOINT = "keystone-fid-service-provider.connected" class KeystoneSAMLMellonConfigurationAdapter( @@ -73,6 +54,7 @@ class KeystoneSAMLMellonConfigurationAdapter( self._sp_private_key = None self._sp_signing_keyinfo = None self._validation_errors = {} + self._fid_data = self.get_fid_data() @property def validation_errors(self): @@ -101,17 +83,24 @@ class KeystoneSAMLMellonConfigurationAdapter( def sp_location_config(self): return SP_LOCATION_CONFIG + def get_fid_data(self): + fid_sp = endpoint_from_flag(KEYSTONE_FID_ENDPOINT) + if fid_sp: + return fid_sp.all_joined_units.received + else: + return {} + @property def keystone_host(self): - return unitdata.kv().get('hostname') + return self.get_fid_data().get("hostname") @property def keystone_port(self): - return unitdata.kv().get('port') + return self.get_fid_data().get("port") @property def tls_enabled(self): - return unitdata.kv().get('tls-enabled') + return self.get_fid_data().get("tls-enabled") @property def keystone_base_url(self): @@ -256,6 +245,13 @@ class KeystoneSAMLMellonCharm(charms_openstack.charm.OpenStackCharm): # First release supported release = 'mitaka' + release_pkg = 'keystone-common' + + # Required relations + required_relations = [ + 'keystone-fid-service-provider', + 'websso-fid-service-provider'] + # List of packages to install for this charm packages = ['libapache2-mod-auth-mellon'] @@ -275,6 +271,13 @@ class KeystoneSAMLMellonCharm(charms_openstack.charm.OpenStackCharm): # ownership. group = 'www-data' + @property + def restart_map(self): + _map = {} + for config in CONFIGS: + _map[config] = [] + return _map + def configuration_complete(self): """Determine whether sufficient configuration has been provided via charm config options and resources. @@ -292,19 +295,20 @@ class KeystoneSAMLMellonCharm(charms_openstack.charm.OpenStackCharm): return all(required_config.values()) - def assess_status(self): - """Determine the current application status for the charm""" - hookenv.application_version_set(self.application_version) + def custom_assess_status_check(self): + """Custom asses status. + + Check the configuration is complete. + """ if not self.configuration_complete(): errors = [ '{}: {}'.format(k, v) for k, v in self.options.validation_errors.items() if v] status_msg = 'Configuration is incomplete. {}'.format( ','.join(errors)) - hookenv.status_set('blocked', status_msg) - else: - hookenv.status_set('active', - 'Unit is ready') + return 'blocked', status_msg + # Nothing to report + return None, None def render_config(self): """ diff --git a/src/reactive/keystone_saml_mellon_handlers.py b/src/reactive/keystone_saml_mellon_handlers.py index 05fe788..2affbd1 100644 --- a/src/reactive/keystone_saml_mellon_handlers.py +++ b/src/reactive/keystone_saml_mellon_handlers.py @@ -13,16 +13,11 @@ # See the License for the specific language governing permissions and # limitations under the License. -import uuid - # import to trigger openstack charm metaclass init -import charm.openstack.keystone_saml_mellon # noqa +import charm.openstack.keystone_saml_mellon as keystone_saml_mellon # noqa import charms_openstack.charm as charm import charms.reactive as reactive -import charms.reactive.flags as flags - -import charmhelpers.core.unitdata as unitdata from charms.reactive.relations import ( endpoint_from_flag, @@ -30,34 +25,11 @@ from charms.reactive.relations import ( charm.use_defaults( 'charm.installed', - 'update-status') - -# if config has been changed we need to re-evaluate flags -# config.changed is set and cleared (atexit) in layer-basic -flags.register_trigger(when='config.changed', - clear_flag='config.rendered') -flags.register_trigger(when='upgraded', clear_flag='config.rendered') -flags.register_trigger(when='config.changed', - clear_flag='config.complete') -flags.register_trigger( - when='endpoint.keystone-fid-service-provider.changed', - clear_flag='keystone-data.complete' -) + 'update-status', + 'upgrade-charm') -@reactive.hook('upgrade-charm') -def default_upgrade_charm(): - """Default handler for the 'upgrade-charm' hook. - This calls the charm.singleton.upgrade_charm() function as a default. - """ - reactive.set_state('upgraded') - - -# clear the upgraded state once config.rendered is set again -flags.register_trigger(when='config.rendered', clear_flag='upgraded') - - -@reactive.when_not('endpoint.keystone-fid-service-provider.joined') +@reactive.when_not('keystone-fid-service-provider.connected') def keystone_departed(): """ Service restart should be handled on the keystone side @@ -67,69 +39,46 @@ def keystone_departed(): charm_instance.remove_config() -@reactive.when('endpoint.keystone-fid-service-provider.joined') -@reactive.when_not('config.complete') -def config_changed(): - with charm.provide_charm_instance() as charm_instance: - if charm_instance.configuration_complete(): - flags.set_flag('config.complete') - - -@reactive.when('endpoint.keystone-fid-service-provider.joined') -@reactive.when_not('keystone-data.complete') -def keystone_data_changed(fid_sp): - primary_data = fid_sp.all_joined_units[0].received - if primary_data: - hostname = primary_data.get('hostname') - port = primary_data.get('port') - tls_enabled = primary_data.get('tls-enabled') - # a basic check on the fact that keystone provided us with - # hostname and port information - if hostname and port: - # save hostname and port data in local storage for future - # use - in case config is incomplete but a relation is - # we need to store this across charm hook invocations - unitdb = unitdata.kv() - unitdb.set('hostname', hostname) - unitdb.set('port', port) - unitdb.set('tls-enabled', tls_enabled) - flags.set_flag('keystone-data.complete') - - -@reactive.when('endpoint.keystone-fid-service-provider.joined') -@reactive.when('config.complete') -@reactive.when('keystone-data.complete') -@reactive.when_not('config.rendered') -def render_config(): +@reactive.when('keystone-fid-service-provider.connected') +def publish_sp_fid(): # don't always have a relation context - obtain from the flag fid_sp = endpoint_from_flag( - 'endpoint.keystone-fid-service-provider.joined') + keystone_saml_mellon.KEYSTONE_FID_ENDPOINT) with charm.provide_charm_instance() as charm_instance: - charm_instance.render_config() - flags.set_flag('config.rendered') - # Trigger keystone restart. The relation is container-scoped - # so a per-unit db of a remote unit will only contain a nonce - # of a single subordinate - restart_nonce = str(uuid.uuid4()) - fid_sp.publish(restart_nonce, - charm_instance.options.protocol_name, + fid_sp.publish(charm_instance.options.protocol_name, charm_instance.options.remote_id_attribute) -@reactive.when('endpoint.websso-fid-service-provider.joined') -@reactive.when('config.complete') -@reactive.when('keystone-data.complete') -@reactive.when('config.rendered') +@reactive.when('keystone-fid-service-provider.available') +def render_config(): + # don't always have a relation context - obtain from the flag + fid_sp = endpoint_from_flag( + keystone_saml_mellon.KEYSTONE_FID_ENDPOINT) + with charm.provide_charm_instance() as charm_instance: + if charm_instance.configuration_complete(): + print("COMPLETE") + charm_instance.render_config() + # Trigger keystone restart. The relation is container-scoped + # so a per-unit db of a remote unit will only contain a nonce + # of a single subordinate + print("CHECK_anyfile") + if reactive.any_file_changed(keystone_saml_mellon.CONFIGS): + print("TRUE_anyfile") + fid_sp.request_restart() + + +@reactive.when('websso-fid-service-provider.connected') def configure_websso(): # don't always have a relation context - obtain from the flag websso_fid_sp = endpoint_from_flag( - 'endpoint.websso-fid-service-provider.joined') + 'websso-fid-service-provider.connected') with charm.provide_charm_instance() as charm_instance: - # publish config options for all remote units of a given rel - options = charm_instance.options - websso_fid_sp.publish(options.protocol_name, - options.idp_name, - options.user_facing_name) + if charm_instance.configuration_complete(): + # publish config options for all remote units of a given rel + options = charm_instance.options + websso_fid_sp.publish(options.protocol_name, + options.idp_name, + options.user_facing_name) @reactive.when_not('always.run') diff --git a/unit_tests/test_keystone_saml_mellon_handlers.py b/unit_tests/test_keystone_saml_mellon_handlers.py index ef9387a..37510d5 100644 --- a/unit_tests/test_keystone_saml_mellon_handlers.py +++ b/unit_tests/test_keystone_saml_mellon_handlers.py @@ -34,26 +34,16 @@ class TestRegisteredHooks(test_utils.TestRegisteredHooks): 'default_upgrade_charm': ('upgrade-charm',), }, 'when': { + 'publish_sp_fid': ( + 'keystone-fid-service-provider.connected',), 'render_config': ( - 'endpoint.keystone-fid-service-provider.joined', - 'config.complete', - 'keystone-data.complete',), - 'config_changed': ( - 'endpoint.keystone-fid-service-provider.joined',), - 'keystone_data_changed': ( - 'endpoint.keystone-fid-service-provider.joined',), + 'keystone-fid-service-provider.available',), 'configure_websso': ( - 'endpoint.websso-fid-service-provider.joined', - 'config.complete', - 'keystone-data.complete', - 'config.rendered',), + 'websso-fid-service-provider.connected',), }, 'when_not': { - 'config_changed': ('config.complete',), 'keystone_departed': ( - 'endpoint.keystone-fid-service-provider.joined',), - 'keystone_data_changed': ('keystone-data.complete',), - 'render_config': ('config.rendered',), + 'keystone-fid-service-provider.connected',), 'assess_status': ('always.run',), }, } @@ -75,16 +65,8 @@ class TestKeystoneSAMLMellonHandlers(test_utils.PatchHelper): self.keystone_saml_mellon_charm) self.provide_charm_instance().__exit__.return_value = None - self.patch_object(handlers, 'flags') - - self.uuid = 'uuid-uuid' - self.patch_object(handlers.uuid, 'uuid4') - self.uuid4.return_value = self.uuid - - self.patch_object(handlers, 'unitdata', + self.patch_object(handlers.reactive, 'any_file_changed', new=mock.MagicMock()) - self.kv = mock.MagicMock() - self.unitdata.kv.return_value = self.kv self.patch_object(handlers, 'endpoint_from_flag', new=mock.MagicMock()) @@ -116,30 +98,25 @@ class TestKeystoneSAMLMellonHandlers(test_utils.PatchHelper): handlers.keystone_departed() self.keystone_saml_mellon_charm.remove_config.assert_called_once_with() - def test_keystone_data_changed(self): - kv_set_calls = [ - mock.call("tls-enabled", True), - mock.call("port", "5000"), - mock.call("hostname", "keystone-0"), - ] - - handlers.keystone_data_changed(self.endpoint) - - self.kv.set.has_calls(kv_set_calls) - self.flags.set_flag.assert_called_once_with('keystone-data.complete') + def test_publish_sp_fid(self): + handlers.publish_sp_fid() + self.endpoint.publish.assert_called_once_with( + self.protocol_name, self.remote_id_attribute) def test_render_config(self): + # No restart + self.any_file_changed.return_value = False + (self.keystone_saml_mellon_charm + .configuration_complete.return_value) = True + handlers.render_config() self.keystone_saml_mellon_charm.render_config.assert_called_once_with() - self.flags.set_flag.assert_called_once_with('config.rendered') - self.endpoint.publish.assert_called_once_with( - self.uuid, self.protocol_name, self.remote_id_attribute) + self.endpoint.request_restart.assert_not_called() - def test_config_changed(self): - handlers.config_changed() - (self.keystone_saml_mellon_charm.configuration_complete - .return_value) = True - self.flags.set_flag.assert_called_once_with('config.complete') + # Restart + self.any_file_changed.return_value = True + handlers.render_config() + self.endpoint.request_restart.assert_called_once_with() def test_configure_websso(self): handlers.configure_websso() diff --git a/unit_tests/test_lib_charm_openstack_keystone_saml_mellon.py b/unit_tests/test_lib_charm_openstack_keystone_saml_mellon.py index cf67e77..17812ee 100644 --- a/unit_tests/test_lib_charm_openstack_keystone_saml_mellon.py +++ b/unit_tests/test_lib_charm_openstack_keystone_saml_mellon.py @@ -45,13 +45,10 @@ class Helper(test_utils.PatchHelper): self.patch_release( keystone_saml_mellon.KeystoneSAMLMellonCharm.release) - self.patch_object(keystone_saml_mellon, 'unitdata', - new=mock.MagicMock()) - self.kv = mock.MagicMock() - self.unitdata.kv.return_value = self.kv - - self.patch_object(keystone_saml_mellon.os_utils, 'os_release', + self.patch_object(keystone_saml_mellon, 'endpoint_from_flag', new=mock.MagicMock()) + self.endpoint = mock.MagicMock() + self.endpoint_from_flag.return_value = self.endpoint self.idp_name = "samltest" self.protocol_name = "mapped" @@ -100,19 +97,6 @@ class Helper(test_utils.PatchHelper): self.open.return_value = self.fileobj -class TestKeystoneSAMLMellonUtils(Helper): - - def test_select_release(self): - self.kv.get.return_value = 'mitaka' - self.assertEqual( - keystone_saml_mellon.select_release(), 'mitaka') - - self.kv.get.return_value = None - self.os_release.return_value = 'rocky' - self.assertEqual( - keystone_saml_mellon.select_release(), 'rocky') - - class TestKeystoneSAMLMellonConfigurationAdapter(Helper): def setUp(self): @@ -120,12 +104,13 @@ class TestKeystoneSAMLMellonConfigurationAdapter(Helper): self.hostname = "keystone-sp.local" self.port = "5000" self.tls_enabled = True - self.unitdata_data = { + self.endpoint_data = { "hostname": self.hostname, "port": self.port, "tls-enabled": self.tls_enabled, } - self.kv.get.side_effect = FakeConfig(self.unitdata_data) + self.endpoint.all_joined_units.received.get.side_effect = ( + FakeConfig(self.endpoint_data)) self.base_url = "https://{}:{}".format(self.hostname, self.port) def test_validation_errors(self): @@ -338,19 +323,20 @@ class TestKeystoneSAMLMellonCharm(Helper): self.sp_signing_keyinfo.__bool__.return_value = False self.assertFalse(ksm.configuration_complete()) - def test_assess_status(self): + def test_custom_assess_status_check(self): ksm = keystone_saml_mellon.KeystoneSAMLMellonCharm() - ksm.assess_status() - self.application_version_set.asert_called_once_with() - self.status_set.assert_called_once_with("active", "Unit is ready") + self.assertEqual( + ksm.custom_assess_status_check(), + (None, None)) # One option not ready self.status_set.reset_mock() self.sp_signing_keyinfo.__bool__.return_value = False ksm.options._validation_errors = {"idp-metadata": "malformed"} - ksm.assess_status() - self.status_set.assert_called_once_with( - "blocked", "Configuration is incomplete. idp-metadata: malformed") + self.assertEqual( + ksm.custom_assess_status_check(), + ("blocked", + "Configuration is incomplete. idp-metadata: malformed")) def test_render_config(self): ksm = keystone_saml_mellon.KeystoneSAMLMellonCharm()