From 1b1e7c583c68a166ba2b826b7fd2606e10d358ce Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Fri, 18 Oct 2019 14:55:26 +0100 Subject: [PATCH] Policyd override implementation This patchset implements policy overrides for octavia. It uses the code in charmhelpers [1] which has been modified to support the richer and more complex approach to handling policy overrides. [1]: https://github.com/juju/charm-helpers/pull/393 func-test-pr: https://github.com/openstack-charmers/zaza-openstack-tests/pull/126 Change-Id: Ib51fd2c7c540c680083c2928eab4ce4df0d43e23 Closed-Bug: #1741723 --- README.md | 70 ++++- charmhelpers/contrib/openstack/context.py | 4 +- charmhelpers/contrib/openstack/policyd.py | 308 ++++++++++++++------- charmhelpers/contrib/openstack/utils.py | 11 +- charmhelpers/contrib/storage/linux/ceph.py | 35 ++- charmhelpers/core/hookenv.py | 18 ++ config.yaml | 11 + docs/01-policyd-overrides.md | 150 ++++++++++ hooks/horizon_contexts.py | 30 +- hooks/horizon_hooks.py | 9 +- hooks/horizon_utils.py | 274 ++++++++++++++++-- metadata.yaml | 4 + templates/ocata/local_settings.py | 13 + tests/tests.yaml | 7 + tox.ini | 2 +- unit_tests/test_horizon_contexts.py | 25 ++ unit_tests/test_horizon_utils.py | 180 +++++++++++- unit_tests/test_utils.py | 28 +- 18 files changed, 1016 insertions(+), 163 deletions(-) create mode 100644 docs/01-policyd-overrides.md diff --git a/README.md b/README.md index 39790d27..2b646954 100644 --- a/README.md +++ b/README.md @@ -106,9 +106,77 @@ Once the option is enabled a custom theme can be provided via a juju resource. The resource should be a .tgz file with the contents of your custom theme. If the file 'local_settings.py' is included it will be sourced. - juju attach-resource openstack-dashboard theme=theme.tgz + juju attach-resource openstack-dashboard theme=theme.tgz Repeating the attach-resource will update the theme and turning off the custom-theme option will return to the default. [themes]: https://docs.openstack.org/horizon/latest/configuration/themes.html + +Policy Overrides +================ + +This feature allows for policy overrides using the `POLICY_DIRS` override +feature of horizon (the OpenStack dashboard project). This is an **advanced** +feature and the policies that the OpenStack dashboard supports should be +clearly understood before trying to override, or add to, the default policies +that the dashboard uses. The charm also has some policy defaults. They should +also be understood before being overridden. + +> **Caution**: It is possible to break the system (for tenants and other + services) if policies are incorrectly applied to the service. + +Policy overrides are YAML files that contain rules that will add to, or +override, existing policy rules in the service. This charm owns the +`POLICY_DIRS` directory, and as such, any manual changes to it will +be overwritten on charm upgrades. + +The Juju resource `policyd-override` must be a ZIP file that contains at least +one directory that corresponds with the OpenStack services that the OpenStack +dashboard has policy override support for. These directory names correspond to +the follow service/charms: + +- `compute` - the compute service provided by Nova +- `identity` - the identity service provided by Keystone +- `image` - the image service provided by Glance +- `network` - the networking service provided by Neutron +- `volume` - the volume service provided by Cinder + +The files in the directory/directories must be YAML files. Thus, to provide +overrides for the `compute` and `identity` services, the resource ZIP file +should contain something like: + + \ compute - compute-override1.yaml + | \ compute-override2.yaml + | + \ identity - identity-override1.yaml + | identity-override2.yaml + \ identity-override3.yaml + +The names of the YAML files is not important. The names of the directories +**is** important and must match the list above. Any other files/directories in +the ZIP are ignored. + +The resource file, say `overrides.zip`, is attached to the charm by: + + + juju attach-resource keystone policyd-override=overrides.zip + +The policy override is enabled in the charm using: + + juju config keystone use-policyd-override=true + +When `use-policyd-override` is `True` the status line of the charm will be +prefixed with `PO:` indicating that policies have been overridden. If the +installation of the policy override YAML files failed for any reason then the +status line will be prefixed with `PO (broken):`. The log file for the charm +will indicate the reason. No policy override files are installed if the `PO +(broken):` is shown. The status line indicates that the overrides are broken, +not that the policy for the service has failed. The policy will be the defaults +for the charm and service. + +Policy overrides on one service may affect the functionality of another +service. Therefore, it may be necessary to provide policy overrides for +multiple service charms to achieve a consistent set of policies across the +OpenStack system. The charms for the other services that may need overrides +should be checked to ensure that they support overrides before proceeding. diff --git a/charmhelpers/contrib/openstack/context.py b/charmhelpers/contrib/openstack/context.py index a3d48c41..9b80b6d6 100644 --- a/charmhelpers/contrib/openstack/context.py +++ b/charmhelpers/contrib/openstack/context.py @@ -1940,7 +1940,7 @@ class VolumeAPIContext(InternalEndpointContext): as well as the catalog_info string that would be supplied. Returns a dict containing the volume_api_version and the volume_catalog_info. """ - rel = os_release(self.pkg, base='icehouse') + rel = os_release(self.pkg) version = '2' if CompareOpenStackReleases(rel) >= 'pike': version = '3' @@ -2140,7 +2140,7 @@ class VersionsContext(OSContextGenerator): self.pkg = pkg def __call__(self): - ostack = os_release(self.pkg, base='icehouse') + ostack = os_release(self.pkg) osystem = lsb_release()['DISTRIB_CODENAME'].lower() return { 'openstack_release': ostack, diff --git a/charmhelpers/contrib/openstack/policyd.py b/charmhelpers/contrib/openstack/policyd.py index 1adf2472..53662142 100644 --- a/charmhelpers/contrib/openstack/policyd.py +++ b/charmhelpers/contrib/openstack/policyd.py @@ -17,9 +17,11 @@ import contextlib import os import six import shutil +import sys import yaml import zipfile +import charmhelpers import charmhelpers.core.hookenv as hookenv import charmhelpers.core.host as ch_host @@ -115,8 +117,8 @@ library for further details). default: False description: | If True then use the resource file named 'policyd-override' to install - override yaml files in the service's policy.d directory. The resource - file should be a zip file containing at least one yaml file with a .yaml + override YAML files in the service's policy.d directory. The resource + file should be a ZIP file containing at least one yaml file with a .yaml or .yml extension. If False then remove the overrides. """ @@ -134,14 +136,14 @@ resources: Policy Overrides ---------------- -This service allows for policy overrides using the `policy.d` directory. This -is an **advanced** feature and the policies that the service supports should be -clearly and unambiguously understood before trying to override, or add to, the -default policies that the service uses. +This feature allows for policy overrides using the `policy.d` directory. This +is an **advanced** feature and the policies that the OpenStack service supports +should be clearly and unambiguously understood before trying to override, or +add to, the default policies that the service uses. The charm also has some +policy defaults. They should also be understood before being overridden. -The charm also has some policy defaults. They should also be understood before -being overridden. It is possible to break the system (for tenants and other -services) if policies are incorrectly applied to the service. +> **Caution**: It is possible to break the system (for tenants and other + services) if policies are incorrectly applied to the service. Policy overrides are YAML files that contain rules that will add to, or override, existing policy rules in the service. The `policy.d` directory is @@ -149,30 +151,16 @@ a place to put the YAML override files. This charm owns the `/etc/keystone/policy.d` directory, and as such, any manual changes to it will be overwritten on charm upgrades. -Policy overrides are provided to the charm using a resource file called -`policyd-override`. This is attached to the charm using (for example): +Overrides are provided to the charm using a Juju resource called +`policyd-override`. The resource is a ZIP file. This file, say +`overrides.zip`, is attached to the charm by: - juju attach-resource policyd-override= -The `` is the name that this charm is deployed as, with -`` being the resource file containing the policy overrides. + juju attach-resource policyd-override=overrides.zip -The format of the resource file is a ZIP file (.zip extension) containing at -least one YAML file with an extension of `.yaml` or `.yml`. Note that any -directories in the ZIP file are ignored; all of the files are flattened into a -single directory. There must not be any duplicated filenames; this will cause -an error and nothing in the resource file will be applied. +The policy override is enabled in the charm using: -(ed. next part is optional is the charm supports some form of -template/substitution on a read file) - -If a (ed. "one or more of") [`.j2`, `.tmpl`, `.tpl`] file is found in the -resource file then the charm will perform a substitution with charm variables -taken from the config or relations. (ed. edit as appropriate to include the -variable). - -To enable the policy overrides the config option `use-policyd-override` must be -set to `True`. + juju config use-policyd-override=true When `use-policyd-override` is `True` the status line of the charm will be prefixed with `PO:` indicating that policies have been overridden. If the @@ -180,12 +168,8 @@ installation of the policy override YAML files failed for any reason then the status line will be prefixed with `PO (broken):`. The log file for the charm will indicate the reason. No policy override files are installed if the `PO (broken):` is shown. The status line indicates that the overrides are broken, -not that the policy for the service has failed - they will be the defaults for -the charm and service. - -If the policy overrides did not install then *either* attach a new, corrected, -resource file *or* disable the policy overrides by setting -`use-policyd-override` to False. +not that the policy for the service has failed. The policy will be the defaults +for the charm and service. Policy overrides on one service may affect the functionality of another service. Therefore, it may be necessary to provide policy overrides for @@ -251,7 +235,10 @@ def maybe_do_policyd_overrides(openstack_release, blacklist_paths=None, blacklist_keys=None, template_function=None, - restart_handler=None): + restart_handler=None, + user=None, + group=None, + config_changed=False): """If the config option is set, get the resource file and process it to enable the policy.d overrides for the service passed. @@ -280,6 +267,11 @@ def maybe_do_policyd_overrides(openstack_release, directory. However, for any services where this is buggy then a restart_handler can be used to force the policy.d files to be read. + If the config_changed param is True, then the handling is slightly + different: It will only perform the policyd overrides if the config is True + and the success file doesn't exist. Otherwise, it does nothing as the + resource file has already been processed. + :param openstack_release: The openstack release that is installed. :type openstack_release: str :param service: the service name to construct the policy.d directory for. @@ -295,16 +287,43 @@ def maybe_do_policyd_overrides(openstack_release, :param restart_handler: The function to call if the service should be restarted. :type restart_handler: Union[None, Callable[]] + :param user: The user to create/write files/directories as + :type user: Union[None, str] + :param group: the group to create/write files/directories as + :type group: Union[None, str] + :param config_changed: Set to True for config_changed hook. + :type config_changed: bool """ + _user = service if user is None else user + _group = service if group is None else group + if not is_policyd_override_valid_on_this_release(openstack_release): + return + hookenv.log("Running maybe_do_policyd_overrides", + level=POLICYD_LOG_LEVEL_DEFAULT) config = hookenv.config() try: if not config.get(POLICYD_CONFIG_NAME, False): + clean_policyd_dir_for(service, + blacklist_paths, + user=_user, + group=_group) + if (os.path.isfile(_policy_success_file()) + and restart_handler is not None + and callable(restart_handler)): + restart_handler() remove_policy_success_file() - clean_policyd_dir_for(service, blacklist_paths) return - except Exception: + except Exception as e: + hookenv.log("... ERROR: Exception is: {}".format(str(e)), + level=POLICYD_CONFIG_NAME) + import traceback + hookenv.log(traceback.format_exc(), level=POLICYD_LOG_LEVEL_DEFAULT) return - if not is_policyd_override_valid_on_this_release(openstack_release): + # if the policyd overrides have been performed when doing config_changed + # just return + if config_changed and is_policy_success_file_set(): + hookenv.log("... already setup, so skipping.", + level=POLICYD_LOG_LEVEL_DEFAULT) return # from now on it should succeed; if it doesn't then status line will show # broken. @@ -316,49 +335,18 @@ def maybe_do_policyd_overrides(openstack_release, restart_handler() -def maybe_do_policyd_overrides_on_config_changed(openstack_release, - service, - blacklist_paths=None, - blacklist_keys=None, - template_function=None, - restart_handler=None): - """This function is designed to be called from the config changed hook - handler. It will only perform the policyd overrides if the config is True - and the success file doesn't exist. Otherwise, it does nothing as the - resource file has already been processed. +@charmhelpers.deprecate("Use maybe_do_poliyd_overrrides instead") +def maybe_do_policyd_overrides_on_config_changed(*args, **kwargs): + """This function is designed to be called from the config changed hook. + + DEPRECATED: please use maybe_do_policyd_overrides() with the param + `config_changed` as `True`. See maybe_do_policyd_overrides() for more details on the params. - - :param openstack_release: The openstack release that is installed. - :type openstack_release: str - :param service: the service name to construct the policy.d directory for. - :type service: str - :param blacklist_paths: optional list of paths to leave alone - :type blacklist_paths: Union[None, List[str]] - :param blacklist_keys: optional list of keys that mustn't appear in the - yaml file's - :type blacklist_keys: Union[None, List[str]] - :param template_function: Optional function that can modify the string - prior to being processed as a Yaml document. - :type template_function: Union[None, Callable[[str], str]] - :param restart_handler: The function to call if the service should be - restarted. - :type restart_handler: Union[None, Callable[]] """ - config = hookenv.config() - try: - if not config.get(POLICYD_CONFIG_NAME, False): - remove_policy_success_file() - clean_policyd_dir_for(service, blacklist_paths) - return - except Exception: - return - # if the policyd overrides have been performed just return - if os.path.isfile(_policy_success_file()): - return - maybe_do_policyd_overrides( - openstack_release, service, blacklist_paths, blacklist_keys, - template_function, restart_handler) + if 'config_changed' not in kwargs.keys(): + kwargs['config_changed'] = True + return maybe_do_policyd_overrides(*args, **kwargs) def get_policy_resource_filename(): @@ -375,13 +363,16 @@ def get_policy_resource_filename(): @contextlib.contextmanager -def open_and_filter_yaml_files(filepath): +def open_and_filter_yaml_files(filepath, has_subdirs=False): """Validate that the filepath provided is a zip file and contains at least one (.yaml|.yml) file, and that the files are not duplicated when the zip file is flattened. Note that the yaml files are not checked. This is the first stage in validating the policy zipfile; individual yaml files are not checked for validity or black listed keys. + If the has_subdirs param is True, then the files are flattened to the first + directory, and the files in the root are ignored. + An example of use is: with open_and_filter_yaml_files(some_path) as zfp, g: @@ -390,6 +381,8 @@ def open_and_filter_yaml_files(filepath): :param filepath: a filepath object that can be opened by zipfile :type filepath: Union[AnyStr, os.PathLike[AntStr]] + :param has_subdirs: Keep first level of subdirectories in yaml file. + :type has_subdirs: bool :returns: (zfp handle, a generator of the (name, filename, ZipInfo object) tuples) as a tuple. @@ -402,7 +395,7 @@ def open_and_filter_yaml_files(filepath): with zipfile.ZipFile(filepath, 'r') as zfp: # first pass through; check for duplicates and at least one yaml file. names = collections.defaultdict(int) - yamlfiles = _yamlfiles(zfp) + yamlfiles = _yamlfiles(zfp, has_subdirs) for name, _, _, _ in yamlfiles: names[name] += 1 # There must be at least 1 yaml file. @@ -418,26 +411,49 @@ def open_and_filter_yaml_files(filepath): yield (zfp, yamlfiles) -def _yamlfiles(zipfile): +def _yamlfiles(zipfile, has_subdirs=False): """Helper to get a yaml file (according to POLICYD_VALID_EXTS extensions) and the infolist item from a zipfile. + If the `has_subdirs` param is True, the the only yaml files that have a + directory component are read, and then first part of the directory + component is kept, along with the filename in the name. e.g. an entry with + a filename of: + + compute/someotherdir/override.yaml + + is returned as: + + compute/override, yaml, override.yaml, + + This is to help with the special, additional, processing that the dashboard + charm requires. + :param zipfile: the zipfile to read zipinfo items from :type zipfile: zipfile.ZipFile - :returns: generator of (name, ext, filename, info item) for each self-identified - yaml file. + :param has_subdirs: Keep first level of subdirectories in yaml file. + :type has_subdirs: bool + :returns: generator of (name, ext, filename, info item) for each + self-identified yaml file. :rtype: List[(str, str, str, zipfile.ZipInfo)] """ - l = [] + files = [] for infolist_item in zipfile.infolist(): - if infolist_item.is_dir(): - continue - _, name_ext = os.path.split(infolist_item.filename) + try: + if infolist_item.is_dir(): + continue + except AttributeError: + # fallback to "old" way to determine dir entry for pre-py36 + if infolist_item.filename.endswith('/'): + continue + _dir, name_ext = os.path.split(infolist_item.filename) name, ext = os.path.splitext(name_ext) + if has_subdirs and _dir != "": + name = os.path.join(_dir.split(os.path.sep)[0], name) ext = ext.lower() if ext and ext in POLICYD_VALID_EXTS: - l.append((name, ext, name_ext, infolist_item)) - return l + files.append((name, ext, name_ext, infolist_item)) + return files def read_and_validate_yaml(stream_or_doc, blacklist_keys=None): @@ -483,9 +499,6 @@ def read_and_validate_yaml(stream_or_doc, blacklist_keys=None): def policyd_dir_for(service): """Return the policy directory for the named service. - This assumes the default name of "policy.d" which is kept across all - charms. - :param service: str :returns: the policy.d override directory. :rtype: os.PathLike[str] @@ -493,7 +506,7 @@ def policyd_dir_for(service): return os.path.join("/", "etc", service, "policy.d") -def clean_policyd_dir_for(service, keep_paths=None): +def clean_policyd_dir_for(service, keep_paths=None, user=None, group=None): """Clean out the policyd directory except for items that should be kept. The keep_paths, if used, should be set to the full path of the files that @@ -506,12 +519,19 @@ def clean_policyd_dir_for(service, keep_paths=None): :type service: str :param keep_paths: optional list of paths to not delete. :type keep_paths: Union[None, List[str]] + :param user: The user to create/write files/directories as + :type user: Union[None, str] + :param group: the group to create/write files/directories as + :type group: Union[None, str] """ + _user = service if user is None else user + _group = service if group is None else group keep_paths = keep_paths or [] path = policyd_dir_for(service) + hookenv.log("Cleaning path: {}".format(path), level=hookenv.DEBUG) if not os.path.exists(path): - ch_host.mkdir(path, owner=service, group=service, perms=0o775) - _scanner = os.scandir if six.PY3 else _py2_scandir + ch_host.mkdir(path, owner=_user, group=_group, perms=0o775) + _scanner = os.scandir if sys.version_info > (3, 4) else _py2_scandir for direntry in _scanner(path): # see if the path should be kept. if direntry.path in keep_paths: @@ -523,6 +543,22 @@ def clean_policyd_dir_for(service, keep_paths=None): os.remove(direntry.path) +def maybe_create_directory_for(path, user, group): + """For the filename 'path', ensure that the directory for that path exists. + + Note that if the directory already exists then the permissions are NOT + changed. + + :param path: the filename including the path to it. + :type path: str + :param user: the user to create the directory as + :param group: the group to create the directory as + """ + _dir, _ = os.path.split(path) + if not os.path.exists(_dir): + ch_host.mkdir(_dir, owner=user, group=group, perms=0o775) + + @contextlib.contextmanager def _py2_scandir(path): """provide a py2 implementation of os.scandir if this module ever gets used @@ -558,6 +594,11 @@ def path_for_policy_file(service, name): It is constructed using policyd_dir_for(), the name and the ".yaml" extension. + For horizon, for example, it's a bit more complicated. The name param is + actually "override_service_dir/a_name", where target_service needs to be + one the allowed horizon override services. This translation and check is + done in the _yamlfiles() function. + :param service: the service name :type service: str :param name: the name for the policy override @@ -585,6 +626,22 @@ def remove_policy_success_file(): pass +def set_policy_success_file(): + """Set the file that indicates successful policyd override.""" + open(_policy_success_file(), "w").close() + + +def is_policy_success_file_set(): + """Returns True if the policy success file has been set. + + This indicates that policies are overridden and working properly. + + :returns: True if the policy file is set + :rtype: bool + """ + return os.path.isfile(_policy_success_file()) + + def policyd_status_message_prefix(): """Return the prefix str for the status line. @@ -594,7 +651,7 @@ def policyd_status_message_prefix(): :returns: the prefix :rtype: str """ - if os.path.isfile(_policy_success_file()): + if is_policy_success_file_set(): return "PO:" return "PO (broken):" @@ -603,7 +660,11 @@ def process_policy_resource_file(resource_file, service, blacklist_paths=None, blacklist_keys=None, - template_function=None): + template_function=None, + preserve_topdir=False, + preprocess_filename=None, + user=None, + group=None): """Process the resource file (which should contain at least one yaml file) and write those files to the service's policy.d directory. @@ -623,6 +684,16 @@ def process_policy_resource_file(resource_file, its file path reconstructed. This, also, must not match any path in the black list. + The yaml filename can be modified in two ways. If the `preserve_topdir` + param is True, then files will be flattened to the this top dir. This + allows for creating sets of files that can be grouped into a single level + tree structure. + + Secondly, if the `preprocess_filename` param is not None and callable() + then the name is passed to that function for preprocessing before being + converted to the end location. This is to allow munging of the filename + prior to being tested for a blacklist path. + If any error occurs, then the policy.d directory is cleared, the error is written to the log, and the status line will eventually show as failed. @@ -638,17 +709,39 @@ def process_policy_resource_file(resource_file, :param template_function: Optional function that can modify the yaml document. :type template_function: Union[None, Callable[[AnyStr], AnyStr]] + :param preserve_topdir: Keep the toplevel subdir + :type preserve_topdir: bool + :param preprocess_filename: Optional function to use to process filenames + extracted from the resource file. + :type preprocess_filename: Union[None, Callable[[AnyStr]. AnyStr]] + :param user: The user to create/write files/directories as + :type user: Union[None, str] + :param group: the group to create/write files/directories as + :type group: Union[None, str] :returns: True if the processing was successful, False if not. :rtype: boolean """ + hookenv.log("Running process_policy_resource_file", level=hookenv.DEBUG) blacklist_paths = blacklist_paths or [] completed = False + _preprocess = None + if preprocess_filename is not None and callable(preprocess_filename): + _preprocess = preprocess_filename + _user = service if user is None else user + _group = service if group is None else group try: - with open_and_filter_yaml_files(resource_file) as (zfp, gen): + with open_and_filter_yaml_files( + resource_file, preserve_topdir) as (zfp, gen): # first clear out the policy.d directory and clear success remove_policy_success_file() - clean_policyd_dir_for(service, blacklist_paths) + clean_policyd_dir_for(service, + blacklist_paths, + user=_user, + group=_group) for name, ext, filename, zipinfo in gen: + # See if the name should be preprocessed. + if _preprocess is not None: + name = _preprocess(name) # construct a name for the output file. yaml_filename = path_for_policy_file(service, name) if yaml_filename in blacklist_paths: @@ -666,8 +759,12 @@ def process_policy_resource_file(resource_file, "available".format(filename)) doc = template_function(doc) yaml_doc = read_and_validate_yaml(doc, blacklist_keys) - with open(yaml_filename, "wt") as f: - yaml.dump(yaml_doc, f) + # we may have to create the directory + maybe_create_directory_for(yaml_filename, _user, _group) + ch_host.write_file(yaml_filename, + yaml.dump(yaml_doc).encode('utf-8'), + _user, + _group) # Every thing worked, so we mark up a success. completed = True except (BadZipFile, BadPolicyZipFile, BadPolicyYamlFile) as e: @@ -691,10 +788,13 @@ def process_policy_resource_file(resource_file, hookenv.log("Processing {} failed: cleaning policy.d directory" .format(resource_file), level=POLICYD_LOG_LEVEL_DEFAULT) - clean_policyd_dir_for(service, blacklist_paths) + clean_policyd_dir_for(service, + blacklist_paths, + user=_user, + group=_group) else: # touch the success filename hookenv.log("policy.d overrides installed.", level=POLICYD_LOG_LEVEL_DEFAULT) - open(_policy_success_file(), "w").close() + set_policy_success_file() return completed diff --git a/charmhelpers/contrib/openstack/utils.py b/charmhelpers/contrib/openstack/utils.py index ac96f844..9ed96f00 100644 --- a/charmhelpers/contrib/openstack/utils.py +++ b/charmhelpers/contrib/openstack/utils.py @@ -531,7 +531,7 @@ def reset_os_release(): _os_rel = None -def os_release(package, base='essex', reset_cache=False): +def os_release(package, base=None, reset_cache=False): ''' Returns OpenStack release codename from a cached global. @@ -542,6 +542,8 @@ def os_release(package, base='essex', reset_cache=False): the installation source, the earliest release supported by the charm should be returned. ''' + if not base: + base = UBUNTU_OPENSTACK_RELEASE[lsb_release()['DISTRIB_CODENAME']] global _os_rel if reset_cache: reset_os_release() @@ -670,7 +672,10 @@ def openstack_upgrade_available(package): codename = get_os_codename_install_source(src) avail_vers = get_os_version_codename_swift(codename) else: - avail_vers = get_os_version_install_source(src) + try: + avail_vers = get_os_version_install_source(src) + except: + avail_vers = cur_vers apt.init() return apt.version_compare(avail_vers, cur_vers) >= 1 @@ -1693,7 +1698,7 @@ def enable_memcache(source=None, release=None, package=None): if release: _release = release else: - _release = os_release(package, base='icehouse') + _release = os_release(package) if not _release: _release = get_os_codename_install_source(source) diff --git a/charmhelpers/contrib/storage/linux/ceph.py b/charmhelpers/contrib/storage/linux/ceph.py index e13dfa8b..104977af 100644 --- a/charmhelpers/contrib/storage/linux/ceph.py +++ b/charmhelpers/contrib/storage/linux/ceph.py @@ -422,6 +422,8 @@ def enabled_manager_modules(): cmd = ['ceph', 'mgr', 'module', 'ls'] try: modules = check_output(cmd) + if six.PY3: + modules = modules.decode('UTF-8') except CalledProcessError as e: log("Failed to list ceph modules: {}".format(e), WARNING) return [] @@ -1185,6 +1187,15 @@ class CephBrokerRq(object): self.request_id = str(uuid.uuid1()) self.ops = [] + def add_op(self, op): + """Add an op if it is not already in the list. + + :param op: Operation to add. + :type op: dict + """ + if op not in self.ops: + self.ops.append(op) + def add_op_request_access_to_group(self, name, namespace=None, permission=None, key_name=None, object_prefix_permissions=None): @@ -1198,7 +1209,7 @@ class CephBrokerRq(object): 'rwx': ['prefix1', 'prefix2'], 'class-read': ['prefix3']} """ - self.ops.append({ + self.add_op({ 'op': 'add-permissions-to-key', 'group': name, 'namespace': namespace, 'name': key_name or service_name(), @@ -1251,11 +1262,11 @@ class CephBrokerRq(object): if pg_num and weight: raise ValueError('pg_num and weight are mutually exclusive') - self.ops.append({'op': 'create-pool', 'name': name, - 'replicas': replica_count, 'pg_num': pg_num, - 'weight': weight, 'group': group, - 'group-namespace': namespace, 'app-name': app_name, - 'max-bytes': max_bytes, 'max-objects': max_objects}) + self.add_op({'op': 'create-pool', 'name': name, + 'replicas': replica_count, 'pg_num': pg_num, + 'weight': weight, 'group': group, + 'group-namespace': namespace, 'app-name': app_name, + 'max-bytes': max_bytes, 'max-objects': max_objects}) def add_op_create_erasure_pool(self, name, erasure_profile=None, weight=None, group=None, app_name=None, @@ -1283,12 +1294,12 @@ class CephBrokerRq(object): :param max_objects: Maximum objects quota to apply :type max_objects: int """ - self.ops.append({'op': 'create-pool', 'name': name, - 'pool-type': 'erasure', - 'erasure-profile': erasure_profile, - 'weight': weight, - 'group': group, 'app-name': app_name, - 'max-bytes': max_bytes, 'max-objects': max_objects}) + self.add_op({'op': 'create-pool', 'name': name, + 'pool-type': 'erasure', + 'erasure-profile': erasure_profile, + 'weight': weight, + 'group': group, 'app-name': app_name, + 'max-bytes': max_bytes, 'max-objects': max_objects}) def set_ops(self, ops): """Set request ops to provided value. diff --git a/charmhelpers/core/hookenv.py b/charmhelpers/core/hookenv.py index 4744eb43..39b1cd09 100644 --- a/charmhelpers/core/hookenv.py +++ b/charmhelpers/core/hookenv.py @@ -119,6 +119,24 @@ def log(message, level=None): raise +def action_log(message): + """Write an action progress message""" + command = ['action-log'] + if not isinstance(message, six.string_types): + message = repr(message) + command += [message[:SH_MAX_ARG]] + # Missing action-log should not cause failures in unit tests + # Send action_log output to stderr + try: + subprocess.call(command) + except OSError as e: + if e.errno == errno.ENOENT: + message = "action-log: {}".format(message) + print(message, file=sys.stderr) + else: + raise + + class Serializable(UserDict): """Wrapper, an object that can be serialized to yaml or json""" diff --git a/config.yaml b/config.yaml index 410c8141..3b736a44 100644 --- a/config.yaml +++ b/config.yaml @@ -375,3 +375,14 @@ options: OpenStack Train, consistency groups have been dropped and replaced by the generic group feature. Setting this option for OpenStack Train or above will not do anything. + use-policyd-override: + type: boolean + default: False + description: | + If True then use the resource named 'policyd-override' to install + override YAML files in the horizon's policy directories. The resource + file should be a ZIP file containing YAML policy files. These are to be + placed into directories that indicate the service that the policy file + belongs to. Please see the README of the charm for further details. + . + If False then remove/disable any overrides in force. diff --git a/docs/01-policyd-overrides.md b/docs/01-policyd-overrides.md new file mode 100644 index 00000000..cef28ba8 --- /dev/null +++ b/docs/01-policyd-overrides.md @@ -0,0 +1,150 @@ +# How policy.d overrides with with the dashboard charm + +This document is a development note to explain how the policy.d overrides is +implemented in the charm. + +## Background + +Policy overrides for most OpenStack services use the oslo.policy module in +a simple fashion where the default `/etc//policy.d/` directory is +used. A YAML or JSON file is dropped into this directory and the service +(which may need to be restarted) picks up the policy overrides and applies +them. + +The Horizon (OpenStack dashboard) service unfortunately operates quite +differently. The issue is that the policy files *by default* live in the +package area of the system +(`/usr/lib/python3/dist-packages/openstack_dashboard/conf`) which are also +written by the templates. Thus, the situation is that the packages themselves +carry policy overrides as the directory (on a `bionic-stein`) look like: + + . + ./nova_policy.json + ./neutron_policy.json + ./nova_policy.d + ./nova_policy.d/api-extensions.yaml + ./keystonev3_policy.json + ./heat_policy.json + ./glance_policy.json + ./keystone_policy.json + ./cinder_policy.json + ./cinder_policy.d + ./cinder_policy.d/consistencygroup.yaml + +The `keystonev3_policy.json` is *also* written by the charm to provide the +`cloud_admin` rule: + +```json +{ + "admin_required": "role:Admin", + "cloud_admin": "rule:admin_required and domain_id:3d0ec224504f4d1b9eea4d3e643b4679", + "service_role": "role:service", + ... +} +``` + +This is produced by the template `./rocky/keystonev3_policy.json` which starts +with: + +```json +{ + "admin_required": "role:Admin", + "cloud_admin": "rule:admin_required and domain_id:{{ admin_domain_id }}", + "service_role": "role:service", + .... +} +``` + +That is, the context key `admin_domain_id` is written to the packaged area of +openstack_dashboard packages using the charm template system. + +## Issues for the policy.d overrides + +The key issues for the policy.d overrides are: + +1. The overrides need to be able to be removed and the existing, packaged, + policies should be cleanly restored to the packaged versions. +2. They have to be consistently applied and maintained during various hooks + that may update the configuration and thus the templates that get written to + the packaged area. +3. The OpenStack dashboard can only be configured to read its policy files from + one place, and that is (by default) + `/usr/lib/python3/dist-packages/openstack_dashboard/conf`. But it can be + changed using the configuration setting `POLICY_FILES_PATH` in the + `local_settings.conf`. + +The first issue is basically: the charm must be able to delete any policy +override files that have been implemented with the configuration option +`use-policyd-override` is set to `false` after previously having been set to +`true`. This essentially means that policy overrides **can't** be written to +the package area (`/usr/lib/python3/...`) without accounting for what the +packages placed there; this is brittle, so an implementation constraint is that +the *policy override files mustn't be written to the package area*. + +The second issue is that hooks may update the configuration and these have to +be reflected in the configuration files that the OpenStack dashboard service is +using. + +Thirdly, the `local_settings.py` needs to have the `POLICY_FILES` setting +updated by the policy file overrides, if an override is for one of `compute`, +`identity`, `image`, `network`, and/or `volume`. + +All of this means that handling policy overrides is much more complicated than +for other OpenStack charms. + +## How the policy.d overrides actually work on this charm + +The approach taken by the OpenStack dashboard charm is to: + +1. Add sections to the `local_settings` template to (if the + `use-policyd-override` is `true`): + * Set the `POLICY_FILES_PATH` to `/etc/openstack-dashboard/policy.d/` + * Set the `POLICY_DIRS` to map the `compute`, `identity`, `image`, `network`, + and/or `volume` to paths in `/etc/horizon/policy.d/nova_policy.d`, (etc.) + for those overrides that exist in the associated policy override ZIP + resource file. Note that this file *has* to have `compute`, `identity` ... + as directories in the policy ZIP file. +2. After configs are rendered, copy the entire directory tree from + `/usr/lib/python3/dist-packages/openstack_dashboard/conf/` to + `/etc/openstack-dashboard/policy.d/` +3. Process the policy files in the ZIP policy overrides file and place them + into `/etc/openstack-dashboard/policy.d/nova_policy.d`, (etc...) so that the + overrides can come into play. +4. Ensure that `apache2` is stopped and restarted so that the policies actually + get loaded. +5. *Any* time the configuration is re-rendered, the policies are updated. + +This allows the `/etc/openstack-dashboard/policy.d/` directory to be deleted +whenever a policy override is updated or cleared, as the 'pristine' policies +will always be in the package directories. + +In order to be consistent in ensuring that the `/etc/horizon/policy.d/` files +are updated, the `OSConfigRenderer` class is subclassed as +`PolicyOverridesOSConfigRenderer` to provide a wrap around the `write_all` +method that copies the directory into `/etc/horizon/policy.d/` as needed. + +A *blacklist* helper is provided that essentially blacklists the files in the +`.../conf` directory, as mapped into the `.../policy.d/` directory, and this is +supplied to the helper functions in charmhelpers. This is to ensure that any +template policy files that the charm writes is not unintentionally overriden by +an override file. This blacklist also ensures that when the directory is +deleted, that the policies from `.../conf/` will be retained. + +## Implementation issues - and how they are solved + +An issue for the implementation is that the charm writes configuration files to +the `.../conf` directory, but the charm *also* needs to use the files in the +`.../conf` directory when checking whether the policy overrides are acceptable. + +The issue is what goes into the `local_settings.py` file for the +`POLICY_FILES_PATH` and `POLICY_DIRS` configuration options. Both of these are +determined by whether the policy overrides are acceptable, but the charm can't +discover that until the `.../conf` templates are written and the policy +overrides resource ZIP file is analyzed. Only then can the context for +`POLICY_FILES_PATH` and `POLICY_DIRS` be determined, and finally the +`local_settings.py` file be written. + +Essentially, `CONFIGS.write_all()` needs to perform the validation, which is +different from the other charms. `CONFIGS.write_all()` needs to do all the +`CONFIGS` templates *apart* from `local_settings.py`, then do the policyd +overrides processing and then do the local_settings. diff --git a/hooks/horizon_contexts.py b/hooks/horizon_contexts.py index ff4731f4..3e6e53f9 100644 --- a/hooks/horizon_contexts.py +++ b/hooks/horizon_contexts.py @@ -40,6 +40,7 @@ from charmhelpers.contrib.network.ip import ( format_ipv6_addr, get_relation_ip, ) +import charmhelpers.contrib.openstack.policyd as policyd from charmhelpers.core.host import pwgen @@ -186,7 +187,7 @@ class HorizonContext(OSContextGenerator): "custom_theme": config('custom-theme'), "secret": config('secret') or pwgen(), 'support_profile': config('profile') - if config('profile') in ['cisco'] else None, + if config('profile') in ['cisco'] else None, "neutron_network_dvr": config("neutron-network-dvr"), "neutron_network_l3ha": config("neutron-network-l3ha"), "neutron_network_lb": config("neutron-network-lb"), @@ -194,7 +195,7 @@ class HorizonContext(OSContextGenerator): "neutron_network_vpn": config("neutron-network-vpn"), "cinder_backup": config("cinder-backup"), "allow_password_autocompletion": - config("allow-password-autocompletion"), + config("allow-password-autocompletion"), "password_retrieve": config("password-retrieve"), 'default_domain': config('default-domain'), 'multi_domain': False if config('default-domain') else True, @@ -210,6 +211,31 @@ class HorizonContext(OSContextGenerator): return ctxt +class PolicydContext(OSContextGenerator): + + def __init__(self, policyd_extract_policy_dirs_fn): + self.policyd_extract_policy_dirs_fn = policyd_extract_policy_dirs_fn + + def __call__(self): + """Policyd variables for the local_settings.py configuration file. + + :returns: The context to help set vars in the localsettings. + :rtype: Dict[str, ANY] + """ + activated = (config('use-policyd-override') + and policyd.is_policy_success_file_set()) + + if activated: + return { + 'policyd_overrides_activated': activated, + 'policy_dirs': self.policyd_extract_policy_dirs_fn(), + } + else: + return { + 'policyd_overrides_activated': activated + } + + class ApacheContext(OSContextGenerator): def __call__(self): ''' Grab cert and key from configuraton for SSL config ''' diff --git a/hooks/horizon_hooks.py b/hooks/horizon_hooks.py index 073c0eab..637642fe 100755 --- a/hooks/horizon_hooks.py +++ b/hooks/horizon_hooks.py @@ -104,6 +104,7 @@ from hooks.horizon_utils import ( remove_old_packages, ) + hooks = Hooks() # Note that CONFIGS is now set up via resolve_CONFIGS so that it is not a # module load time constraint. @@ -174,8 +175,8 @@ def config_changed(): else: localhost = 'localhost' - if (os_release('openstack-dashboard') == 'icehouse' and - config('offline-compression') in ['no', 'False']): + if (os_release('openstack-dashboard') == 'icehouse' + and config('offline-compression') in ['no', 'False']): apt_install(filter_installed_packages(['python-lesscpy']), fatal=True) @@ -367,8 +368,8 @@ def websso_trusted_dashboard_changed(): return # TODO: check for vault relation in order to determine url scheme - tls_configured = (relation_ids('certificates') or - config('ssl-key') or config('enforce-ssl')) + tls_configured = (relation_ids('certificates') + or config('ssl-key') or config('enforce-ssl')) scheme = 'https://' if tls_configured else 'http://' hostname = resolve_address(endpoint_type=PUBLIC, override=True) diff --git a/hooks/horizon_utils.py b/hooks/horizon_utils.py index e72b96b5..44712efa 100644 --- a/hooks/horizon_utils.py +++ b/hooks/horizon_utils.py @@ -17,12 +17,14 @@ from collections import OrderedDict from copy import deepcopy import os +import shutil import subprocess import time import tarfile import charmhelpers.contrib.openstack.context as context import charmhelpers.contrib.openstack.templating as templating +import charmhelpers.contrib.openstack.policyd as policyd from charmhelpers.contrib.openstack.utils import ( configure_installation_source, @@ -38,15 +40,21 @@ from charmhelpers.contrib.openstack.utils import ( ) from charmhelpers.core.hookenv import ( config, + DEBUG, + ERROR, + hook_name, + INFO, log, resource_get, ) from charmhelpers.core.host import ( cmp_pkgrevno, + CompareHostReleases, lsb_release, + mkdir, path_hash, service, - CompareHostReleases, + write_file, ) from charmhelpers.fetch import ( apt_upgrade, @@ -89,31 +97,39 @@ REQUIRED_INTERFACES = { 'identity': ['identity-service'], } +POLICYD_HORIZON_SERVICE_TO_DIR = { + 'identity': 'keystone_policy.d', + 'compute': 'nova_policy.d', + 'volume': 'cinder_policy.d', + 'image': 'glance_policy.d', + 'network': 'neutron_policy.d', + 'orchestration': 'heat_policy.d', +} + APACHE_CONF_DIR = "/etc/apache2" LOCAL_SETTINGS = "/etc/openstack-dashboard/local_settings.py" DASHBOARD_CONF_DIR = "/etc/openstack-dashboard/" +DASHBOARD_PKG_DIR = "/usr/share/openstack-dashboard/openstack_dashboard" HAPROXY_CONF = "/etc/haproxy/haproxy.cfg" -APACHE_CONF = "%s/conf.d/openstack-dashboard.conf" % (APACHE_CONF_DIR) -APACHE_24_CONF = "%s/conf-available/openstack-dashboard.conf" \ - % (APACHE_CONF_DIR) -PORTS_CONF = "%s/ports.conf" % (APACHE_CONF_DIR) -APACHE_24_SSL = "%s/sites-available/default-ssl.conf" % (APACHE_CONF_DIR) -APACHE_24_DEFAULT = "%s/sites-available/000-default.conf" % (APACHE_CONF_DIR) -APACHE_SSL = "%s/sites-available/default-ssl" % (APACHE_CONF_DIR) -APACHE_DEFAULT = "%s/sites-available/default" % (APACHE_CONF_DIR) +APACHE_CONF = os.path.join(APACHE_CONF_DIR, "conf.d/openstack-dashboard.conf") +APACHE_24_CONF = os.path.join(APACHE_CONF_DIR, + "conf-available/openstack-dashboard.conf") +PORTS_CONF = os.path.join(APACHE_CONF_DIR, "ports.conf") +APACHE_24_SSL = os.path.join(APACHE_CONF_DIR, + "sites-available/default-ssl.conf") +APACHE_24_DEFAULT = os.path.join(APACHE_CONF_DIR, + "sites-available/000-default.conf") +APACHE_SSL = os.path.join(APACHE_CONF_DIR, "sites-available/default-ssl") +APACHE_DEFAULT = os.path.join(APACHE_CONF_DIR, "sites-available/default") INSTALL_DIR = "/usr/share/openstack-dashboard" -ROUTER_SETTING = ('/usr/share/openstack-dashboard/openstack_dashboard/enabled/' - '_40_router.py') -KEYSTONEV3_POLICY = ('/usr/share/openstack-dashboard/openstack_dashboard/conf/' - 'keystonev3_policy.json') -CONSISTENCY_GROUP_POLICY = ('/usr/share/openstack-dashboard/' - 'openstack_dashboard/conf/cinder_policy.d/' - 'consistencygroup.yaml') +ROUTER_SETTING = os.path.join(DASHBOARD_PKG_DIR, 'enabled/_40_router.py') +KEYSTONEV3_POLICY = os.path.join(DASHBOARD_PKG_DIR, + 'conf/keystonev3_policy.json') +CONSISTENCY_GROUP_POLICY = os.path.join( + DASHBOARD_PKG_DIR, 'conf/cinder_policy.d/consistencygroup.yaml') TEMPLATES = 'templates' -CUSTOM_THEME_DIR = ("/usr/share/openstack-dashboard/openstack_dashboard/" - "themes/custom") -LOCAL_DIR = ('/usr/share/openstack-dashboard/openstack_dashboard/local/' - 'local_settings.d') +CUSTOM_THEME_DIR = os.path.join(DASHBOARD_PKG_DIR, "themes/custom") +LOCAL_DIR = os.path.join(DASHBOARD_PKG_DIR, 'local/local_settings.d') CONFIG_FILES = OrderedDict([ (LOCAL_SETTINGS, { @@ -122,7 +138,9 @@ CONFIG_FILES = OrderedDict([ context.SyslogContext(), horizon_contexts.LocalSettingsContext(), horizon_contexts.ApacheSSLContext(), - horizon_contexts.WebSSOFIDServiceProviderContext()], + horizon_contexts.WebSSOFIDServiceProviderContext(), + horizon_contexts.PolicydContext( + lambda: read_policyd_dirs())], 'services': ['apache2', 'memcached'] }), (APACHE_CONF, { @@ -185,15 +203,15 @@ CONFIG_FILES = OrderedDict([ def register_configs(): ''' Register config files with their respective contexts. ''' release = os_release('openstack-dashboard') - configs = templating.OSConfigRenderer(templates_dir=TEMPLATES, - openstack_release=release) + configs = HorizonOSConfigRenderer(templates_dir=TEMPLATES, + openstack_release=release) confs = [LOCAL_SETTINGS, HAPROXY_CONF, PORTS_CONF] - if (CompareOpenStackReleases(release) >= 'queens' and - CompareOpenStackReleases(release) <= 'stein'): + if (CompareOpenStackReleases(release) >= 'queens' + and CompareOpenStackReleases(release) <= 'stein'): configs.register( CONSISTENCY_GROUP_POLICY, CONFIG_FILES[CONSISTENCY_GROUP_POLICY]['hook_contexts']) @@ -225,6 +243,208 @@ def register_configs(): return configs +class HorizonOSConfigRenderer(templating.OSConfigRenderer): + + def write_all(self): + """Write all of the config files. + + This function subclasses the parent version of the function such that + if the hook is config-changed or upgrade-charm then it defers writing + the LOCAL_SETTINGS file until after processing the policyd stuff. + """ + _hook = hook_name() + if _hook not in ('upgrade-charm', 'config-changed'): + return super(HorizonOSConfigRenderer, self).write_all() + # Otherwise, first do all the other templates + for k in self.templates.keys(): + if k != LOCAL_SETTINGS: + self.write(k) + # Now do the policy overrides thing + maybe_handle_policyd_override(os_release('openstack-dashboard'), + _hook) + # Finally, let's do the LOCAL_SETTINGS if the policyd worked. + self.write(LOCAL_SETTINGS) + + +def maybe_handle_policyd_override(openstack_release, hook): + """Handle the use-policy-override config flag and resource file. + + This function checks that policy overrides are supported on this release, + that the config flag is enabled, and then processes the resources, copies + the package policies to the config area, loads the override files. In the + case where the config flag is false, it removes the policy overrides by + deleting the config area policys. Note that the template for + `local_settings.py` controls where the horizon service actually reads the + policies from. + + Note that for the 'config-changed' hook, the function is only interested in + whether the config value of `use-policy-override` matches the current + status of the policy overrides success file. If it doesn't, either the + config area policies are removed (i.e. False) or the policy overrides file + is processed. + + :param openstack_release: The release of OpenStack installed. + :type openstack_release: str + :param hook: The hook name + :type hook: str + """ + log("Seeing if policyd overrides need doing", level=INFO) + if not policyd.is_policyd_override_valid_on_this_release( + openstack_release): + log("... policy overrides not valid on this release: {}" + .format(openstack_release), + level=INFO) + return + # if policy config is not set, then remove the entire directory + _config = config() + if not _config.get(policyd.POLICYD_CONFIG_NAME, False): + _dir = policyd.policyd_dir_for('openstack-dashboard') + if os.path.exists(_dir): + log("... config is cleared, and removing {}".format(_dir), INFO) + shutil.rmtree(_dir) + else: + log("... nothing to do", INFO) + policyd.remove_policy_success_file() + return + # config-change and the policyd overrides have been performed just return + if hook == "config-changed" and policyd.is_policy_success_file_set(): + log("... already setup, so skipping.", level=INFO) + return + # from now on it should succeed; if it doesn't then status line will show + # broken. + resource_filename = policyd.get_policy_resource_filename() + restart = policyd.process_policy_resource_file( + resource_filename, + 'openstack-dashboard', + blacklist_paths=blacklist_policyd_paths(), + preserve_topdir=True, + preprocess_filename=policyd_preprocess_name, + user='horizon', + group='horizon') + copy_conf_to_policyd() + if restart: + service('stop', 'apache2') + service('start', 'apache2') + log("Policy override processing complete.", level=INFO) + + +def blacklist_policyd_paths(): + """Process the .../conf directory and create a list of blacklisted paths. + + This is so that the policyd helpers don't delete the copied files from the + .../conf directory. + + :returns: list of blacklisted paths. + :rtype: [str] + """ + conf_dir = os.path.join(DASHBOARD_PKG_DIR, 'conf') + conf_parts_count = len(conf_dir.split(os.path.sep)) + policy_dir = policyd.policyd_dir_for('openstack-dashboard') + paths = [] + for root, _, files in os.walk(conf_dir): + # make _root relative to the conf_dir + _root = os.path.sep.join(root.split(os.path.sep)[conf_parts_count:]) + for file in files: + paths.append(os.path.join(policy_dir, _root, file)) + log("blacklisted paths: {}".format(", ".join(paths)), INFO) + return paths + + +def copy_conf_to_policyd(): + """Walk the conf_dir and copy everything into the policy_dir. + + This is used after processing the policy.d resource file to put the package + and templated policy files in DASHBOARD_PKG_DIR/conf/ into the + /etc/openstack-dashboard/policy.d/ + """ + log("policyd: copy files from conf to /etc/openstack-dashboard/policy.d", + level=INFO) + conf_dir = os.path.join(DASHBOARD_PKG_DIR, 'conf') + conf_parts_count = len(conf_dir.split(os.path.sep)) + policy_dir = policyd.policyd_dir_for('openstack-dashboard') + for root, dirs, files in os.walk(conf_dir): + # make _root relative to the conf_dir + _root = os.path.sep.join(root.split(os.path.sep)[conf_parts_count:]) + # make any dirs necessary + for d in dirs: + _dir = os.path.join(policy_dir, _root, d) + if not os.path.exists(_dir): + mkdir(_dir, owner='horizon', group='horizon', perms=0o775) + # now copy the files. + for f in files: + source = os.path.join(conf_dir, _root, f) + dest = os.path.join(policy_dir, _root, f) + with open(source, 'r') as fh: + content = fh.read() + write_file(dest, content, 'horizon', 'horizon') + log("...done.", level=INFO) + + +def read_policyd_dirs(): + """Return a mapping of policy type to directory name. + + This returns a subset of: + + { + 'identity': ['keystone_policy.d'], + 'compute': ['nova_policy.d'], + 'volume': ['cinder_policy.d'], + 'image': ['glance_policy.d'], + 'network': ['neutron_policy.d'], + } + + depending on what is actually set in the policy directory that has + been written. + + :returns: mapping of type to policyd dir name. + :rtype: Dict[str, List[str]] + """ + policy_dir = policyd.policyd_dir_for('openstack-dashboard') + try: + _, dirs, _ = list(os.walk(policy_dir))[0] + return {k: [v] for k, v in POLICYD_HORIZON_SERVICE_TO_DIR.items() + if v in dirs} + except IndexError: + # The directory doesn't exist to return an empty dictionary + return {} + except Exception: + # Something else went wrong; log it but don't fail. + log("read_policyd_dirs went wrong -- need to fix this!!", ERROR) + import traceback + log(traceback.format_exc(), ERROR) + return {} + + +def policyd_preprocess_name(name): + """Try to preprocess the name supplied to the one horizon expects. + + This takes a name of the form "compute/file01.yaml" and converts it to + "nova_policy.d/file01.yaml" to match the expectations of the service. + + It raises policyd's BadPolicyYamlFile exception if the file can't be + converted and should be skipped. + + :param name: The name to convert + :type name: AnyStr + :raises: charmhelpers.contrib.openstack.policyd.BadPolicyYamlFile + :returns: the converted name + :rtype: str + """ + if os.path.sep not in name: + raise policyd.BadPolicyYamlFile("No prefix for section") + horizon_service, name = os.path.split(name) + try: + policy_dir = POLICYD_HORIZON_SERVICE_TO_DIR[horizon_service] + name = os.path.join(policy_dir, name) + except KeyError: + log("horizon override service {} from {} not recognised, so ignoring" + .format(horizon_service, name), + level=DEBUG) + raise policyd.BadPolicyYamlFile("Bad prefix : {}" + .format(horizon_service)) + return name + + def restart_map(): ''' Determine the correct resource map to be passed to @@ -358,8 +578,8 @@ def setup_ipv6(): # Need haproxy >= 1.5.3 for ipv6 so for Trusty if we are <= Kilo we need to # use trusty-backports otherwise we can use the UCA. _os_release = os_release('openstack-dashboard') - if (ubuntu_rel == 'trusty' and - CompareOpenStackReleases(_os_release) < 'liberty'): + if (ubuntu_rel == 'trusty' + and CompareOpenStackReleases(_os_release) < 'liberty'): add_source('deb http://archive.ubuntu.com/ubuntu trusty-backports ' 'main') apt_update() diff --git a/metadata.yaml b/metadata.yaml index 312e8337..3506477d 100644 --- a/metadata.yaml +++ b/metadata.yaml @@ -46,3 +46,7 @@ resources: type: file filename: theme.tgz description: "Custom dashboard theme" + policyd-override: + type: file + filename: policyd-override.zip + description: The policy.d overrides file diff --git a/templates/ocata/local_settings.py b/templates/ocata/local_settings.py index 4c95fc44..e0f36480 100644 --- a/templates/ocata/local_settings.py +++ b/templates/ocata/local_settings.py @@ -507,6 +507,19 @@ TIME_ZONE = "UTC" # Path to directory containing policy.json files #POLICY_FILES_PATH = os.path.join(ROOT_PATH, "conf") +{% if policyd_overrides_activated -%} +# Policies are overriden and all policies are here rather than in package conf +POLICY_FILES_PATH = '/etc/openstack-dashboard/policy.d/' + +# These are matched from the defaults + any in the overrides +{% if policy_dirs -%} +POLICY_DIRS = { +{% for k, vs in policy_dirs.items() -%} + "{{ k }}": [{% for v in vs -%}"{{ v }}", {% endfor -%}], +{% endfor -%} +} +{% endif -%} +{% endif -%} # Map of local copy of service policy files. # Please insure that your identity policy file matches the one being used on diff --git a/tests/tests.yaml b/tests/tests.yaml index 81f36e5b..9e4a3e17 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -19,5 +19,12 @@ gate_bundles: dev_bundles: - disco-train +configure: + - zaza.openstack.charm_tests.keystone.setup.add_demo_user + tests: - zaza.openstack.charm_tests.openstack_dashboard.tests.OpenStackDashboardTests + - zaza.openstack.charm_tests.openstack_dashboard.tests.OpenStackDashboardPolicydTests +tests_options: + policyd: + service: openstack-dashboard diff --git a/tox.ini b/tox.ini index 20dbbfc5..7a184be7 100644 --- a/tox.ini +++ b/tox.ini @@ -111,5 +111,5 @@ commands = functest-run-suite --keep-model --bundle {posargs} [flake8] -ignore = E402,E226 +ignore = E402,E226,W503 exclude = */charmhelpers diff --git a/unit_tests/test_horizon_contexts.py b/unit_tests/test_horizon_contexts.py index 7d7418c8..d76ecd87 100644 --- a/unit_tests/test_horizon_contexts.py +++ b/unit_tests/test_horizon_contexts.py @@ -817,3 +817,28 @@ class TestHorizonContexts(CharmTestCase): }, ] }) + + @patch.object(horizon_contexts.policyd, 'is_policy_success_file_set') + def test_policyd_context(self, mock_is_policy_success_file_set): + self.test_config.set('use-policyd-override', True) + + def extract_dirs_func(): + return {'a': ['a-dir']} + + mock_is_policy_success_file_set.return_value = True + self.assertEqual( + horizon_contexts.PolicydContext(extract_dirs_func)(), { + 'policyd_overrides_activated': True, + 'policy_dirs': {'a': ['a-dir']}, + }) + mock_is_policy_success_file_set.return_value = False + self.assertEqual( + horizon_contexts.PolicydContext(extract_dirs_func)(), { + 'policyd_overrides_activated': False, + }) + mock_is_policy_success_file_set.return_value = True + self.test_config.set('use-policyd-override', False) + self.assertEqual( + horizon_contexts.PolicydContext(extract_dirs_func)(), { + 'policyd_overrides_activated': False, + }) diff --git a/unit_tests/test_horizon_utils.py b/unit_tests/test_horizon_utils.py index 51530df1..2a00368b 100644 --- a/unit_tests/test_horizon_utils.py +++ b/unit_tests/test_horizon_utils.py @@ -14,12 +14,15 @@ from mock import MagicMock, patch, call from collections import OrderedDict -import charmhelpers.contrib.openstack.templating as templating -templating.OSConfigRenderer = MagicMock() +# import charmhelpers.contrib.openstack.templating as templating +# templating.OSConfigRenderer = MagicMock() import hooks.horizon_utils as horizon_utils -from unit_tests.test_utils import CharmTestCase +from unit_tests.test_utils import ( + CharmTestCase, + patch_open, +) TO_PATCH = [ 'config', @@ -33,6 +36,7 @@ TO_PATCH = [ 'os_release', 'os_application_version_set', 'reset_os_release', + 'HorizonOSConfigRenderer', ] @@ -40,6 +44,7 @@ class TestHorizonUtils(CharmTestCase): def setUp(self): super(TestHorizonUtils, self).setUp(horizon_utils, TO_PATCH) + self.config.side_effect = self.test_config.get def test_determine_packages(self): horizon_utils.os_release.return_value = 'icehouse' @@ -141,7 +146,7 @@ class TestHorizonUtils(CharmTestCase): @patch.object(horizon_utils, 'determine_packages') def test_do_openstack_upgrade(self, determine_packages): - self.config.return_value = 'cloud:precise-havana' + self.test_config.set('openstack-origin', 'cloud:precise-havana') self.get_os_codename_install_source.return_value = 'havana' horizon_utils.os_release.return_value = 'icehouse' configs = MagicMock() @@ -198,6 +203,173 @@ class TestHorizonUtils(CharmTestCase): call(conf, horizon_utils.CONFIG_FILES[conf]['hook_contexts'])) configs.register.assert_has_calls(calls) + @patch('shutil.rmtree') + @patch('os.path.exists') + @patch.object(horizon_utils.policyd, 'remove_policy_success_file') + @patch.object(horizon_utils.policyd, 'policyd_dir_for') + @patch.object(horizon_utils.policyd, + 'is_policyd_override_valid_on_this_release') + def test_maybe_handle_policyd_override_config_false( + self, + mock_valid, + mock_policyd_dir_for, + mock_remove_policy_success_file, + mock_os_path_exists, + mock_shutils_rmtree, + ): + self.test_config.set('use-policyd-override', False) + mock_valid.return_value = True + mock_policyd_dir_for.return_value = 'a_dir' + mock_os_path_exists.return_value = True + horizon_utils.maybe_handle_policyd_override( + 'a_release', 'config-changed') + mock_policyd_dir_for.assert_called_once_with('openstack-dashboard') + mock_shutils_rmtree.assert_called_once_with('a_dir') + mock_remove_policy_success_file.assert_called_once_with() + + @patch.object(horizon_utils.policyd, 'get_policy_resource_filename') + @patch.object(horizon_utils.policyd, 'is_policy_success_file_set') + @patch.object(horizon_utils.policyd, + 'is_policyd_override_valid_on_this_release') + def test_maybe_handle_policyd_override_config_changed_done( + self, + mock_valid, + mock_is_policy_success_file_set, + mock_get_policy_resource_filename, + ): + self.test_config.set('use-policyd-override', True) + mock_valid.return_value = True + mock_is_policy_success_file_set.return_value = True + horizon_utils.maybe_handle_policyd_override( + 'a_release', 'config-changed') + # test that the function bailed before getting to the resource file get + mock_get_policy_resource_filename.assert_not_called() + + @patch.object(horizon_utils, 'service') + @patch.object(horizon_utils, 'copy_conf_to_policyd') + @patch.object(horizon_utils, 'blacklist_policyd_paths') + @patch.object(horizon_utils.policyd, 'process_policy_resource_file') + @patch.object(horizon_utils.policyd, 'get_policy_resource_filename') + @patch.object(horizon_utils.policyd, 'is_policy_success_file_set') + @patch.object(horizon_utils.policyd, + 'is_policyd_override_valid_on_this_release') + def test_maybe_handle_policyd_override_config_changed_full_run( + self, + mock_valid, + mock_is_policy_success_file_set, + mock_get_policy_resource_filename, + mock_process_policy_resource_file, + mock_blacklist_policyd_paths, + mock_copy_conf_to_policyd, + mock_service, + ): + self.test_config.set('use-policyd-override', True) + mock_valid.return_value = True + mock_is_policy_success_file_set.return_value = False + mock_get_policy_resource_filename.return_value = "resource-file" + mock_blacklist_policyd_paths.return_value = ['a-path'] + + # test no restart + mock_process_policy_resource_file.return_value = False + horizon_utils.maybe_handle_policyd_override( + 'a_release', 'config-changed') + mock_get_policy_resource_filename.assert_called_once_with() + mock_process_policy_resource_file.assert_called_once_with( + 'resource-file', + 'openstack-dashboard', + blacklist_paths=['a-path'], + preserve_topdir=True, + preprocess_filename=horizon_utils.policyd_preprocess_name, + user='horizon', + group='horizon') + mock_copy_conf_to_policyd.assert_called_once_with() + mock_service.assert_not_called() + + # test with restart + mock_process_policy_resource_file.return_value = True + horizon_utils.maybe_handle_policyd_override( + 'a_release', 'config-changed') + mock_service.assert_has_calls([call('stop', 'apache2'), + call('start', 'apache2')]) + + @patch.object(horizon_utils, 'DASHBOARD_PKG_DIR', new='/some/dir') + @patch('os.walk') + @patch.object(horizon_utils.policyd, 'policyd_dir_for') + def test_blacklist_policyd_paths(self, mock_policyd_dir_for, mock_os_walk): + mock_policyd_dir_for.return_value = '/etc' + # Note '/some/dir' below has to match the patch on DASHBOAD_PKG_DIR + # above. + mock_os_walk.return_value = [ + ('/some/dir/conf', ['a-dir'], ['file1']), + ('/some/dir/conf/a-dir', [], ['file2'])] + paths = horizon_utils.blacklist_policyd_paths() + mock_policyd_dir_for.assert_called_once_with('openstack-dashboard') + self.assertEqual(paths, ['/etc/file1', '/etc/a-dir/file2']) + + @patch.object(horizon_utils, 'DASHBOARD_PKG_DIR', new='/some/dir') + @patch.object(horizon_utils, 'mkdir') + @patch.object(horizon_utils, 'write_file') + @patch('os.path.exists') + @patch('os.walk') + @patch.object(horizon_utils.policyd, 'policyd_dir_for') + def test_copy_conf_to_policyd( + self, + mock_policyd_dir_for, + mock_os_walk, + mock_os_path_exists, + mock_write_file, + mock_mkdir, + ): + mock_policyd_dir_for.return_value = '/etc' + # Note '/some/dir' below has to match the patch on DASHBOAD_PKG_DIR + # above. + mock_os_walk.return_value = [ + ('/some/dir/conf', ['a-dir'], ['file1']), + ('/some/dir/conf/a-dir', [], ['file2'])] + mock_os_path_exists.return_value = False + + with patch_open() as (_open, _file): + _file.read.side_effect = ['content1', 'content2'] + horizon_utils.copy_conf_to_policyd() + mock_mkdir.assert_called_once_with( + '/etc/a-dir', owner='horizon', group='horizon', perms=0o775) + _open.assert_has_calls([ + call('/some/dir/conf/file1', 'r'), + call('/some/dir/conf/a-dir/file2', 'r')]) + mock_write_file.assert_has_calls([ + call('/etc/file1', 'content1', 'horizon', 'horizon'), + call('/etc/a-dir/file2', 'content2', 'horizon', 'horizon')]) + + @patch.object(horizon_utils, 'POLICYD_HORIZON_SERVICE_TO_DIR', + new={'a': 'a-dir', 'b': 'b-dir', 'c': 'c-dir'}) + @patch('os.walk') + @patch.object(horizon_utils.policyd, 'policyd_dir_for') + def test_read_policyd_dirs( + self, + mock_policyd_dir_for, + mock_os_walk, + ): + mock_policyd_dir_for.return_value = '/etc' + # Note '/some/dir' below has to match the patch on DASHBOAD_PKG_DIR + # above. + mock_os_walk.return_value = [ + ('/some/dir/conf', ['b-dir'], ['file1']), + ('/some/dir/conf/b-dir', [], ['file2'])] + self.assertEqual(horizon_utils.read_policyd_dirs(), {'b': ['b-dir']}) + + @patch.object(horizon_utils, 'POLICYD_HORIZON_SERVICE_TO_DIR', + new={'a': 'a-dir', 'b': 'b-dir', 'c': 'c-dir'}) + def test_policyd_preprocess_name(self): + # test with no separator + with self.assertRaises(horizon_utils.policyd.BadPolicyYamlFile): + horizon_utils.policyd_preprocess_name("a-name") + # test unrecognised service + with self.assertRaises(horizon_utils.policyd.BadPolicyYamlFile): + horizon_utils.policyd_preprocess_name("d/a-name") + # finally check that the appropriate change is made + self.assertEqual(horizon_utils.policyd_preprocess_name('b/name'), + "b-dir/name") + def test_assess_status(self): with patch.object(horizon_utils, 'assess_status_func') as asf: callee = MagicMock() diff --git a/unit_tests/test_utils.py b/unit_tests/test_utils.py index bef5ab4d..48ef3f84 100644 --- a/unit_tests/test_utils.py +++ b/unit_tests/test_utils.py @@ -12,12 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. +import io import logging -import unittest import os +import unittest import yaml -from mock import patch +from contextlib import contextmanager +from mock import patch, MagicMock def load_config(): @@ -83,7 +85,9 @@ class TestConfig(object): def __init__(self): self.config = get_default_config() - def get(self, attr): + def get(self, attr=None): + if attr is None: + return self.config.copy() try: return self.config[attr] except KeyError: @@ -112,3 +116,21 @@ class TestRelation(object): elif attr in self.relation_data: return self.relation_data[attr] return None + + +@contextmanager +def patch_open(): + '''Patch open() to allow mocking both open() itself and the file that is + yielded. + + Yields the mock for "open" and "file", respectively.''' + mock_open = MagicMock(spec=open) + mock_file = MagicMock(spec=io.FileIO) + + @contextmanager + def stub_open(*args, **kwargs): + mock_open(*args, **kwargs) + yield mock_file + + with patch('builtins.open', stub_open): + yield mock_open, mock_file