From 6c1756f85260f81754921536a849ca14e34a4f0b Mon Sep 17 00:00:00 2001 From: bmccandless Date: Wed, 7 Oct 2020 15:38:42 -0700 Subject: [PATCH] Enhance the AppConfig with external config sources. (#1904) * Enhance the AppConfig with external config sources. The external config sources are currently environment variables and AWS secrets manager. The config file can be augmented with a section describing how environmen variables and secrets can update config parameters. benefits: - it will enable the config to draw from more than one secret. This is useful for shared secrets between cellxgene and data portal, as well as auth0 secrets. - it will make it very straightforward to check the config before a deployment. Part of #1859 --- server/common/aws_secret_utils.py | 2 +- server/common/config/app_config.py | 70 +++++- server/common/config/base_config.py | 19 ++ server/common/config/dataset_config.py | 24 +- server/common/config/external_config.py | 96 ++++++++ server/common/config/server_config.py | 30 +-- server/common/utils/type_conversion_utils.py | 14 ++ server/default_config.py | 55 +++++ server/test/__init__.py | 8 +- server/test/unit/common/config/__init__.py | 51 +++- .../unit/common/config/test_app_config.py | 74 ++++++ .../unit/common/config/test_dataset_config.py | 4 +- .../common/config/test_external_config.py | 231 ++++++++++++++++++ .../unit/common/config/test_server_config.py | 9 +- 14 files changed, 632 insertions(+), 55 deletions(-) create mode 100644 server/common/config/external_config.py create mode 100644 server/test/unit/common/config/test_external_config.py diff --git a/server/common/aws_secret_utils.py b/server/common/aws_secret_utils.py index 070ed160..ce40794d 100644 --- a/server/common/aws_secret_utils.py +++ b/server/common/aws_secret_utils.py @@ -18,6 +18,6 @@ def get_secret_key(region_name, secret_name): return secret except Exception as e: logging.critical(f"Caught exception during get_secret_key, {e}", exc_info=True) - raise SecretKeyRetrievalError + raise SecretKeyRetrievalError(str(e)) return None diff --git a/server/common/config/app_config.py b/server/common/config/app_config.py index 422dcb8e..5cd4c4a5 100644 --- a/server/common/config/app_config.py +++ b/server/common/config/app_config.py @@ -4,6 +4,7 @@ from flatten_dict import unflatten from server.default_config import get_default_config from server.common.config.dataset_config import DatasetConfig from server.common.config.server_config import ServerConfig +from server.common.config.external_config import ExternalConfig from server.common.errors import ConfigurationError @@ -44,6 +45,9 @@ class AppConfig(object): # dataroot config self.dataroot_config = {} + # external config + self.external_config = ExternalConfig(self, self.default_config["external"]) + # Set to true when config_completed is called self.is_completed = False @@ -61,6 +65,7 @@ class AppConfig(object): self.default_dataset_config.check_config() for dataset_config in self.dataroot_config.values(): dataset_config.check_config() + self.external_config.check_config() def update_server_config(self, **kw): self.server_config.update(**kw) @@ -73,6 +78,51 @@ class AppConfig(object): value.update(**kw) self.is_complete = False + def update_single_config_from_path_and_value(self, path, value): + """Update a single config parameter with the value. + Path is a list of string, that gives a path to the config parameter to be updated. + For example, path may be ["server","app","port"]. + """ + self.is_complete = False + if not isinstance(path, list): + raise ConfigurationError(f"path must be a list of strings, got '{str(path)}'") + for part in path: + if not isinstance(part, str): + raise ConfigurationError(f"path must be a list of strings, got '{str(path)}'") + + if len(path) < 1 or path[0] not in ("server", "dataset", "per_dataset_config"): + raise ConfigurationError("path must start with 'server', 'dataset', or 'per_dataset_config'") + + if path[0] == "server": + attr = "__".join(path[1:]) + try: + self.update_server_config(**{attr: value}) + except ConfigurationError: + raise ConfigurationError(f"unknown config parameter at path: '{str(path)}'") + elif path[0] == "dataset": + attr = "__".join(path[1:]) + try: + self.update_default_dataset_config(**{attr: value}) + except ConfigurationError: + raise ConfigurationError(f"unknown config parameter at path: '{str(path)}'") + + elif path[0] == "per_dataset_config": + if len(path) < 2: + raise ConfigurationError(f"missing dataroot when using per_dataset_config: got '{path}'") + dataroot = path[1] + if dataroot not in self.dataroot_config: + dataroots = str(list(self.dataroot_config.keys())) + raise ConfigurationError( + f"unknown dataroot when using per_dataset_config: got '{path}'," + f" dataroots specified in config are {dataroots}" + ) + + attr = "__".join(path[2:]) + try: + self.dataroot_config[dataroot].update(**{attr: value}) + except ConfigurationError: + raise ConfigurationError(f"unknown config parameter at path: '{str(path)}'") + def update_from_config_file(self, config_file): try: with open(config_file) as yml_file: @@ -94,12 +144,16 @@ class AppConfig(object): # then apply the per dataset configuration self.dataroot_config[key].update_from_config(dataroot_config, f"per_dataset_config__{key}") + if config.get("external"): + self.external_config.update_from_config(config["external"], "external") + self.is_complete = False - def write_config(self, config_file): - """output the config to a yaml file""" + def config_to_dict(self): + """return the configuration as an unflattened dict""" server = self.server_config.create_mapping(self.server_config.default_config) dataset = self.default_dataset_config.create_mapping(self.default_dataset_config.default_config) + external = self.external_config.create_mapping(self.external_config.default_config) config = dict(server={}, dataset={}) for attrname in server.keys(): config["server__" + attrname] = getattr(self.server_config, attrname) @@ -111,15 +165,23 @@ class AppConfig(object): dataset = dataroot_config.create_mapping(dataroot_config.default_config) for attrname in dataset.keys(): config[f"per_dataset_config__{dataroot_tag}__" + attrname] = getattr(dataroot_config, attrname) + for attrname in external.keys(): + config["external__" + attrname] = getattr(self.external_config, attrname) config = unflatten(config, splitter=lambda key: key.split("__")) + return config + + def write_config(self, config_file): + """output the config to a yaml file""" + config = self.config_to_dict() yaml.dump(config, open(config_file, "w")) def changes_from_default(self): """Return all the attribute that are different from the default""" diff_server = self.server_config.changes_from_default() diff_dataset = self.default_dataset_config.changes_from_default() - diff = dict(server=diff_server, dataset=diff_dataset) + diff_external = self.external.changes_from_default() + diff = dict(server=diff_server, dataset=diff_dataset, external=diff_external) return diff def add_dataroot_config(self, dataroot_tag, **kw): @@ -154,6 +216,8 @@ class AppConfig(object): # messages we can give correct context for attributes with bad value. context = dict(messagefn=messagefn) + # complete config for external_config first, since this may update values in the other sections + self.external_config.complete_config(context) self.server_config.complete_config(context) self.default_dataset_config.complete_config(context) for dataroot_config in self.dataroot_config.values(): diff --git a/server/common/config/base_config.py b/server/common/config/base_config.py index a0b46f1b..6b9087e7 100644 --- a/server/common/config/base_config.py +++ b/server/common/config/base_config.py @@ -80,8 +80,27 @@ class BaseConfig(object): raise ConfigurationError(f"The attr '{key}' has not been checked") def update(self, **kw): + """Update the attributes defined in kw with their new values.""" for key, value in kw.items(): if not hasattr(self, key): + + # check if the key is setting into a dictval entry. + found_dictval = False + for dictval in self.dictval_cases: + dictvalname = "__".join(dictval) + if dictvalname + "__" in key: + dictkey = key[len(dictvalname) + 2 :] + curdictval = getattr(self, dictvalname) + if curdictval is None: + setattr(self, dictvalname, dict(dictkey=value)) + else: + curdictval[dictkey] = value + + found_dictval = True + break + + if found_dictval: + continue raise ConfigurationError(f"unknown config parameter {key}.") try: if type(value) == tuple: diff --git a/server/common/config/dataset_config.py b/server/common/config/dataset_config.py index 8c8231ac..7586ec2f 100644 --- a/server/common/config/dataset_config.py +++ b/server/common/config/dataset_config.py @@ -30,22 +30,18 @@ class DatasetConfig(BaseConfig): self.user_annotations__type = default_config["user_annotations"]["type"] self.user_annotations__local_file_csv__directory = default_config["user_annotations"]["local_file_csv"][ "directory" - ] # noqa E501 + ] self.user_annotations__local_file_csv__file = default_config["user_annotations"]["local_file_csv"]["file"] self.user_annotations__ontology__enable = default_config["user_annotations"]["ontology"]["enable"] self.user_annotations__ontology__obo_location = default_config["user_annotations"]["ontology"][ "obo_location" - ] # noqa E501 + ] self.user_annotations__hosted_tiledb_array__db_uri = default_config["user_annotations"][ "hosted_tiledb_array" - ][ - "db_uri" - ] # noqa E501 + ]["db_uri"] self.user_annotations__hosted_tiledb_array__hosted_file_directory = default_config["user_annotations"][ "hosted_tiledb_array" - ][ - "hosted_file_directory" - ] # noqa E501 + ]["hosted_file_directory"] self.embeddings__names = default_config["embeddings"]["names"] self.embeddings__enable_reembedding = default_config["embeddings"]["enable_reembedding"] @@ -98,20 +94,20 @@ class DatasetConfig(BaseConfig): self.validate_correct_type_of_configuration_attribute("user_annotations__type", str) self.validate_correct_type_of_configuration_attribute( "user_annotations__local_file_csv__directory", (type(None), str) - ) # noqa E501 + ) self.validate_correct_type_of_configuration_attribute( "user_annotations__local_file_csv__file", (type(None), str) - ) # noqa E501 + ) self.validate_correct_type_of_configuration_attribute("user_annotations__ontology__enable", bool) self.validate_correct_type_of_configuration_attribute( "user_annotations__ontology__obo_location", (type(None), str) - ) # noqa E501 + ) self.validate_correct_type_of_configuration_attribute( "user_annotations__hosted_tiledb_array__db_uri", (type(None), str) - ) # noqa E501 + ) self.validate_correct_type_of_configuration_attribute( "user_annotations__hosted_tiledb_array__hosted_file_directory", (type(None), str) - ) # noqa E501 + ) if self.user_annotations__enable: server_config = self.app_config.server_config if not self.app__authentication_enable: @@ -166,7 +162,7 @@ class DatasetConfig(BaseConfig): self.validate_correct_type_of_configuration_attribute("user_annotations__hosted_tiledb_array__db_uri", str) self.validate_correct_type_of_configuration_attribute( "user_annotations__hosted_tiledb_array__hosted_file_directory", str - ) # noqa E501 + ) self.user_annotations = AnnotationsHostedTileDB( directory_path=self.user_annotations__hosted_tiledb_array__hosted_file_directory, db=DbUtils(self.user_annotations__hosted_tiledb_array__db_uri), diff --git a/server/common/config/external_config.py b/server/common/config/external_config.py new file mode 100644 index 00000000..bebfbfa9 --- /dev/null +++ b/server/common/config/external_config.py @@ -0,0 +1,96 @@ +import os + +from server.common.config.base_config import BaseConfig +from server.common.errors import ConfigurationError +from server.common.config import get_secret_key +from server.common.errors import SecretKeyRetrievalError +from server.common.utils.type_conversion_utils import convert_string_to_value + + +class ExternalConfig(BaseConfig): + """Manages the config attribute associated with external configuration sources, such as + environment variables or the AWS Secrets Manager.""" + + def __init__(self, app_config, default_config): + super().__init__(app_config, default_config) + try: + self.environment = default_config["environment"] + self.aws_secrets_manager__region = default_config["aws_secrets_manager"]["region"] + self.aws_secrets_manager__secrets = default_config["aws_secrets_manager"]["secrets"] + + except KeyError as e: + raise ConfigurationError(f"Unexpected config: {str(e)}") + + def complete_config(self, context): + self.handle_environment(context) + self.handle_aws_secrets_manager(context) + + def handle_environment(self, context): + """For each environment variable defined, get the value (if it is set), + and set the specified config parameter""" + self.validate_correct_type_of_configuration_attribute("environment", list) + for envdict in self.environment: + name = envdict.get("name") + if name is None: + raise ConfigurationError("environment: 'name' is missing") + required = envdict.get("required", False) + if type(required) != bool: + raise ConfigurationError("environment: 'required' must be a bool") + path = envdict.get("path") + if path is None: + raise ConfigurationError("environment: 'path' is missing") + + value = os.environ.get(name) + if value is None: + if required: + raise ConfigurationError(f"required environment variable '{name}' not set") + else: + value = convert_string_to_value(value) + self.app_config.update_single_config_from_path_and_value(path, value) + + def handle_aws_secrets_manager(self, context): + """For each aws secret defined, get the key/values, and set the specified config parameter""" + self.validate_correct_type_of_configuration_attribute("aws_secrets_manager__region", (type(None), str)) + self.validate_correct_type_of_configuration_attribute("aws_secrets_manager__secrets", list) + + if not self.aws_secrets_manager__secrets: + return + + self.validate_correct_type_of_configuration_attribute("aws_secrets_manager__region", str) + + for secret in self.aws_secrets_manager__secrets: + secret_name = secret.get("name") + if secret_name is None: + raise ConfigurationError("aws_secrets_manager: 'name' is missing") + if not isinstance(secret_name, str): + raise ConfigurationError("aws_secrets_manager: 'name' must be a string") + + try: + secret_dict = get_secret_key(self.aws_secrets_manager__region, secret_name) + except SecretKeyRetrievalError as e: + raise ConfigurationError(f"Unable to retrieve secret {secret_name}: {str(e)}") + + values = secret.get("values") + if values is None: + raise ConfigurationError("aws_secrets_manager: 'values' is missing") + if not isinstance(values, list): + raise ConfigurationError("aws_secrets_manager: 'values' must be a list") + + for value in values: + key = value.get("key") + if key is None: + raise ConfigurationError(f"missing 'key' in secret values: {secret_name}") + path = value.get("path") + if path is None: + raise ConfigurationError(f"missing 'path' in secret values: {secret_name}") + required = value.get("required", False) + if type(required) != bool: + raise ConfigurationError(f"wrong type for 'required' in secret values: {secret_name}") + + secret_value = secret_dict.get(key) + if secret_value is None: + if required: + raise ConfigurationError(f"required secret '{secret_name}:{key}' not set") + else: + secret_value = convert_string_to_value(secret_value) + self.app_config.update_single_config_from_path_and_value(path, secret_value) diff --git a/server/common/config/server_config.py b/server/common/config/server_config.py index 508b3d4f..6c4b5570 100644 --- a/server/common/config/server_config.py +++ b/server/common/config/server_config.py @@ -44,17 +44,17 @@ class ServerConfig(BaseConfig): self.authentication__type = default_config["authentication"]["type"] self.authentication__params_oauth__oauth_api_base_url = default_config["authentication"]["params_oauth"][ "oauth_api_base_url" - ] # noqa E501 + ] self.authentication__params_oauth__client_id = default_config["authentication"]["params_oauth"]["client_id"] self.authentication__params_oauth__client_secret = default_config["authentication"]["params_oauth"][ "client_secret" - ] # noqa E501 + ] self.authentication__params_oauth__jwt_decode_options = default_config["authentication"]["params_oauth"][ "jwt_decode_options" - ] # noqa E501 + ] self.authentication__params_oauth__session_cookie = default_config["authentication"]["params_oauth"][ "session_cookie" - ] # noqa E501 + ] self.authentication__params_oauth__cookie = default_config["authentication"]["params_oauth"]["cookie"] self.multi_dataset__dataroot = default_config["multi_dataset"]["dataroot"] @@ -62,10 +62,10 @@ class ServerConfig(BaseConfig): self.multi_dataset__allowed_matrix_types = default_config["multi_dataset"]["allowed_matrix_types"] self.multi_dataset__matrix_cache__max_datasets = default_config["multi_dataset"]["matrix_cache"][ "max_datasets" - ] # noqa E501 + ] self.multi_dataset__matrix_cache__timelimit_s = default_config["multi_dataset"]["matrix_cache"][ "timelimit_s" - ] # noqa E501 + ] self.single_dataset__datapath = default_config["single_dataset"]["datapath"] self.single_dataset__obs_names = default_config["single_dataset"]["obs_names"] @@ -151,11 +151,6 @@ class ServerConfig(BaseConfig): if not self.app__verbose: sys.tracebacklimit = 0 - # secret key: - # first, from CXG_SECRET_KEY environment variable - # second, from config file - self.app__flask_secret_key = os.environ.get("CXG_SECRET_KEY", self.app__flask_secret_key) - # CSP Directives are a dict of string: list(string) or string: string if self.app__csp_directives is not None: for k, v in self.app__csp_directives.items(): @@ -178,25 +173,20 @@ class ServerConfig(BaseConfig): ptypes = str if self.authentication__type == "oauth" else (type(None), str) self.validate_correct_type_of_configuration_attribute( "authentication__params_oauth__oauth_api_base_url", ptypes - ) # noqa E501 + ) self.validate_correct_type_of_configuration_attribute("authentication__params_oauth__client_id", ptypes) self.validate_correct_type_of_configuration_attribute("authentication__params_oauth__client_secret", ptypes) self.validate_correct_type_of_configuration_attribute( "authentication__params_oauth__jwt_decode_options", (type(None), dict) - ) # noqa E501 + ) self.validate_correct_type_of_configuration_attribute("authentication__params_oauth__session_cookie", bool) if self.authentication__params_oauth__session_cookie: self.validate_correct_type_of_configuration_attribute( "authentication__params_oauth__cookie", (type(None), dict) - ) # noqa E501 + ) else: self.validate_correct_type_of_configuration_attribute("authentication__params_oauth__cookie", dict) - # secret key: first, from CXG_OAUTH_CLIENT_SECRET environment variable - # second, from config file - self.authentication__params_oauth__client_secret = os.environ.get( - "CXG_OAUTH_CLIENT_SECRET", self.authentication__params_oauth__client_secret - ) self.auth = AuthTypeFactory.create(self.authentication__type, self) if self.auth is None: @@ -286,7 +276,7 @@ class ServerConfig(BaseConfig): self.validate_correct_type_of_configuration_attribute("multi_dataset__matrix_cache__max_datasets", int) self.validate_correct_type_of_configuration_attribute( "multi_dataset__matrix_cache__timelimit_s", (type(None), int, float) - ) # noqa E501 + ) if self.multi_dataset__dataroot is None: return diff --git a/server/common/utils/type_conversion_utils.py b/server/common/utils/type_conversion_utils.py index 4bc6bf88..ccda1177 100644 --- a/server/common/utils/type_conversion_utils.py +++ b/server/common/utils/type_conversion_utils.py @@ -151,3 +151,17 @@ def convert_pandas_series_to_numpy(series_to_convert: pd.Series, dtype): logging.error("Cannot convert a pandas Series object to an integer dtype if it contains NaNs.") return series_to_convert.to_numpy(dtype) + + +def convert_string_to_value(value: str): + """convert a string to value with the most appropriate type""" + if value.lower() == "true": + return True + if value.lower() == "false": + return False + if value == "null": + return None + try: + return eval(value) + except: # noqa E722 + return value diff --git a/server/default_config.py b/server/default_config.py index 88e96fe5..20922ef9 100644 --- a/server/default_config.py +++ b/server/default_config.py @@ -204,6 +204,61 @@ dataset: enable: true lfc_cutoff: 0.01 top_n: 10 + +external: + # You can retrieve configuration parameters from this config file, the environment, + # the AWS secrets manager, or from the "cellxgene launch" command line arguments. + # They are applied in that order, meaning that if a parameter is defined in more + # than one location, the last one applied takes effect. + + # environment variables: + # This section describes how to map environment variables to configuration parameters. + # The format is a list defining an environment variable. + # Each entry in the list is a dictionary with three entries: + # name: the name of the environment variable + # path: the path within the cellxgene configuration to update. + # required: (default=False) a boolean. If true, then it is an error if the environment variable is not set. + + environment: + - name: CXG_SECRET_KEY + path: [server, app, flask_secret_key] + required: false + - name: CXG_OAUTH_CLIENT_SECRET + path: [server, authentication, params_oauth, client_secret] + required: false + + # AWS Secrets Manager + # This section describes how to map aws secrets to configuration parameters. + # The format is the region for the secrets manager, then a list of secrets. + # each secret has a name, and a list of values. + # Each entry in the list of values is a dictionary with three entries: + # key: the key of the aws secret. + # path: the path within the cellxgene configuration to update. + # required: (default=False) a boolean. If true, then it is an error if the key does not exist in the secret. + # + # example: + # aws_secrets_manager: + # region: us-west-2 + # - name: my_first_secret + # values: + # - key: flask_secret_key + # path: [server, app, flask_secret_key] + # required: true + # - key: db_uri + # path: [dataset, user_annotations, hosted_tiledb_array, db_uri] + # required: true + # - name: my_auth_secret + # values: + # - key: client_secret + # path: [server, authentication, params_oauth, client_secret] + # required: true + # - key: client_id + # path: [server, authentication, params_oauth, client_id] + # required: true + + aws_secrets_manager: + region: null + secrets: [] """ diff --git a/server/test/__init__.py b/server/test/__init__.py index bbb1de0b..586aa798 100644 --- a/server/test/__init__.py +++ b/server/test/__init__.py @@ -117,7 +117,7 @@ def random_string(n): return "".join(random.choice(string.ascii_letters) for _ in range(n)) -def start_test_server(command_line_args=[], app_config=None): +def start_test_server(command_line_args=[], app_config=None, env=None): """ Command line arguments can be passed in, as well as an app_config. This function is meant to be used like this, for example: @@ -155,7 +155,7 @@ def start_test_server(command_line_args=[], app_config=None): command.extend(["-c", config_file]) server = f"http://localhost:{port}" - ps = Popen(command) + ps = Popen(command, env=env) for _ in range(10): try: @@ -178,10 +178,10 @@ def stop_test_server(ps): @contextmanager -def test_server(command_line_args=[], app_config=None): +def test_server(command_line_args=[], app_config=None, env=None): """A context to run the cellxgene server.""" - ps, server = start_test_server(command_line_args, app_config) + ps, server = start_test_server(command_line_args, app_config, env) try: yield server finally: diff --git a/server/test/unit/common/config/__init__.py b/server/test/unit/common/config/__init__.py index fa730df2..7c06b311 100644 --- a/server/test/unit/common/config/__init__.py +++ b/server/test/unit/common/config/__init__.py @@ -3,6 +3,7 @@ import shutil import unittest import random from unittest import mock +import yaml from server.test import FIXTURES_ROOT @@ -133,6 +134,9 @@ class ConfigTests(unittest.TestCase): enable_difexp="true", lfc_cutoff=0.01, top_n=10, + environment=None, + aws_secrets_manager_region=None, + aws_secrets_manager_secrets=[], config_file_name="app_config.yml", ): random_num = random.randrange(999999) @@ -201,13 +205,17 @@ class ConfigTests(unittest.TestCase): top_n=top_n, config_file_name=f"temp_dataset_config_{random_num}.yml", ) - with open(server_config) as server_config: - with open(dataset_config) as dataset_config: - with open(configfile, "w") as app_config_file: - for line in server_config: - app_config_file.write(line) - for line in dataset_config: - app_config_file.write(line) + external_config = self.custom_external_config( + environment=environment, + aws_secrets_manager_region=aws_secrets_manager_region, + aws_secrets_manager_secrets=aws_secrets_manager_secrets, + config_file_name=f"temp_external_config_{random_num}.yml", + ) + + with open(configfile, "w") as app_config_file: + app_config_file.write(open(server_config).read()) + app_config_file.write(open(dataset_config).read()) + app_config_file.write(open(external_config).read()) return configfile @@ -244,3 +252,32 @@ class ConfigTests(unittest.TestCase): dataset_config_file.write(dataset_config) return configfile + + def custom_external_config( + self, + environment=None, + aws_secrets_manager_region=None, + aws_secrets_manager_secrets=[], + config_file_name="external_config.yaml", + ): + # set to the default if environment is None + if environment is None: + environment = [ + dict(name="CXG_SECRET_KEY", path=["server", "app", "flask_secret_key"], required=False), + dict( + name="CXG_OAUTH_CLIENT_SECRET", + path=["server", "authentication", "params_oauth", "client_secret"], + required=False, + ), + ] + external_config = { + "external": { + "environment": environment, + "aws_secrets_manager": {"region": aws_secrets_manager_region, "secrets": aws_secrets_manager_secrets}, + } + } + + configfile = os.path.join(self.tmp_fixtures_directory, config_file_name) + with open(configfile, "w") as external_config_file: + yaml.dump(external_config, external_config_file) + return configfile diff --git a/server/test/unit/common/config/test_app_config.py b/server/test/unit/common/config/test_app_config.py index 2dc3273d..362ab330 100644 --- a/server/test/unit/common/config/test_app_config.py +++ b/server/test/unit/common/config/test_app_config.py @@ -138,3 +138,77 @@ class AppConfigTest(ConfigTests): dataset_changes = app_config.default_dataset_config.changes_from_default() self.assertEqual(server_changes, []) self.assertEqual(dataset_changes, [("user_annotations__enable", False, True)]) + + def test_simple_update_single_config_from_path_and_value(self): + """Update a simple config parameter""" + + config = AppConfig() + config.server_config.multi_dataset__dataroot = dict( + s1=dict(dataroot="my_dataroot_s1", base_url="my_baseurl_s1"), + s2=dict(dataroot="my_dataroot_s2", base_url="my_baseurl_s2"), + ) + config.add_dataroot_config("s1") + config.add_dataroot_config("s2") + + # test simple value in server + config.update_single_config_from_path_and_value(["server", "app", "flask_secret_key"], "mysecret") + self.assertEqual(config.server_config.app__flask_secret_key, "mysecret") + + # test simple value in default dataset + config.update_single_config_from_path_and_value( + ["dataset", "user_annotations", "hosted_tiledb_array", "db_uri"], "mydburi", + ) + self.assertEqual(config.default_dataset_config.user_annotations__hosted_tiledb_array__db_uri, "mydburi") + self.assertEqual(config.dataroot_config["s1"].user_annotations__hosted_tiledb_array__db_uri, "mydburi") + self.assertEqual(config.dataroot_config["s2"].user_annotations__hosted_tiledb_array__db_uri, "mydburi") + + # test simple value in specific dataset + config.update_single_config_from_path_and_value( + ["per_dataset_config", "s1", "user_annotations", "hosted_tiledb_array", "db_uri"], "s1dburi" + ) + self.assertEqual(config.default_dataset_config.user_annotations__hosted_tiledb_array__db_uri, "mydburi") + self.assertEqual(config.dataroot_config["s1"].user_annotations__hosted_tiledb_array__db_uri, "s1dburi") + self.assertEqual(config.dataroot_config["s2"].user_annotations__hosted_tiledb_array__db_uri, "mydburi") + + # error checking + bad_paths = [ + ( + ["dataset", "does", "not", "exist"], + "unknown config parameter at path: '['dataset', 'does', 'not', 'exist']'", + ), + (["does", "not", "exist"], "path must start with 'server', 'dataset', or 'per_dataset_config'"), + ([], "path must start with 'server', 'dataset', or 'per_dataset_config'"), + (["per_dataset_config"], "missing dataroot when using per_dataset_config: got '['per_dataset_config']'"), + ( + ["per_dataset_config", "unknown"], + "unknown dataroot when using per_dataset_config: got '['per_dataset_config', 'unknown']'," + " dataroots specified in config are ['s1', 's2']", + ), + ([1, 2, 3], "path must be a list of strings, got '[1, 2, 3]'"), + ("string", "path must be a list of strings, got 'string'"), + ] + for bad_path, error_message in bad_paths: + with self.assertRaises(ConfigurationError) as config_error: + config.update_single_config_from_path_and_value(bad_path, "value") + + self.assertEqual(config_error.exception.message, error_message) + + def test_dict_update_single_config_from_path_and_value(self): + """Update a config parameter that has a value of dict""" + + # the path leads to a dict config param, set the config parameter to the new value + config = AppConfig() + config.update_single_config_from_path_and_value( + ["server", "authentication", "params_oauth", "cookie"], dict(key="mykey1", max_age=100) + ) + self.assertEqual(config.server_config.authentication__params_oauth__cookie, dict(key="mykey1", max_age=100)) + + # the path leads to an entry within a dict config param, the value is simple + config = AppConfig() + config.server_config.authentication__params_oauth__cookie = dict(key="mykey1", max_age=100) + config.update_single_config_from_path_and_value( + ["server", "authentication", "params_oauth", "cookie", "httponly"], True, + ) + self.assertEqual( + config.server_config.authentication__params_oauth__cookie, dict(key="mykey1", max_age=100, httponly=True) + ) diff --git a/server/test/unit/common/config/test_dataset_config.py b/server/test/unit/common/config/test_dataset_config.py index a32d1f87..5ad66e44 100644 --- a/server/test/unit/common/config/test_dataset_config.py +++ b/server/test/unit/common/config/test_dataset_config.py @@ -155,7 +155,7 @@ class TestDatasetConfig(ConfigTests): # test for illegal url_dataroots for illegal in ("../b", "!$*", "\\n", "", "(bad)"): config.update_server_config( - multi_dataset__dataroot={"tag": {"base_url": illegal, "dataroot": "{PROJECT_ROOT}/example-dataset"}} + multi_dataset__dataroot={"tag": {"base_url": illegal, "dataroot": f"{PROJECT_ROOT}/example-dataset"}} ) with self.assertRaises(ConfigurationError): config.complete_config() @@ -163,7 +163,7 @@ class TestDatasetConfig(ConfigTests): # test for legal url_dataroots for legal in ("d", "this.is-okay_", "a/b"): config.update_server_config( - multi_dataset__dataroot={"tag": {"base_url": legal, "dataroot": "{PROJECT_ROOT}/example-dataset"}} + multi_dataset__dataroot={"tag": {"base_url": legal, "dataroot": f"{PROJECT_ROOT}/example-dataset"}} ) config.complete_config() diff --git a/server/test/unit/common/config/test_external_config.py b/server/test/unit/common/config/test_external_config.py new file mode 100644 index 00000000..5e3825e6 --- /dev/null +++ b/server/test/unit/common/config/test_external_config.py @@ -0,0 +1,231 @@ +import os +from unittest.mock import patch + +import requests + +from server.common.errors import ConfigurationError +from server.common.config.app_config import AppConfig +from server.test import test_server, FIXTURES_ROOT +from server.common.utils.type_conversion_utils import convert_string_to_value +from server.test.unit.common.config import ConfigTests + + +class TestExternalConfig(ConfigTests): + def test_type_convert(self): + # The values from environment variables and aws secrets are returned as strings. + # These values need to be converted to the proper types. + + self.assertEqual(convert_string_to_value("1"), int(1)) + self.assertEqual(convert_string_to_value("1.1"), float(1.1)) + self.assertEqual(convert_string_to_value("string"), "string") + self.assertEqual(convert_string_to_value("true"), True) + self.assertEqual(convert_string_to_value("True"), True) + self.assertEqual(convert_string_to_value("false"), False) + self.assertEqual(convert_string_to_value("False"), False) + self.assertEqual(convert_string_to_value("null"), None) + self.assertEqual(convert_string_to_value("None"), None) + self.assertEqual(convert_string_to_value("{'a':10, 'b':'string'}"), dict(a=int(10), b="string")) + + def test_environment_variable(self): + configfile = self.custom_external_config( + environment=[ + dict(name="DATAPATH", path=["server", "single_dataset", "datapath"], required=True), + dict(name="DIFFEXP", path=["dataset", "diffexp", "enable"], required=True), + ], + config_file_name="environment_external_config.yaml", + ) + + env = os.environ + env["DATAPATH"] = f"{FIXTURES_ROOT}/pbmc3k.cxg" + env["DIFFEXP"] = "False" + with test_server(command_line_args=["-c", configfile], env=env) as server: + session = requests.Session() + response = session.get(f"{server}/api/v0.2/config") + data_config = response.json() + self.assertEqual(data_config["config"]["displayNames"]["dataset"], "pbmc3k") + self.assertTrue(data_config["config"]["parameters"]["disable-diffexp"]) + + env["DATAPATH"] = f"{FIXTURES_ROOT}/a95c59b4-7f5d-4b80-ad53-a694834ca18b.h5ad" + env["DIFFEXP"] = "True" + with test_server(command_line_args=["-c", configfile], env=env) as server: + session = requests.Session() + response = session.get(f"{server}/api/v0.2/config") + data_config = response.json() + self.assertEqual(data_config["config"]["displayNames"]["dataset"], "a95c59b4-7f5d-4b80-ad53-a694834ca18b") + self.assertFalse(data_config["config"]["parameters"]["disable-diffexp"]) + + def test_environment_variable_errors(self): + + # no name + app_config = AppConfig() + app_config.external_config.environment = [dict(required=True, path=["this", "is", "a", "path"])] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "environment: 'name' is missing") + + # required has wrong type + app_config = AppConfig() + app_config.external_config.environment = [ + dict(name="myenvar", required="optional", path=["this", "is", "a", "path"]) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "environment: 'required' must be a bool") + + # no path + app_config = AppConfig() + app_config.external_config.environment = [dict(name="myenvar", required=True)] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "environment: 'path' is missing") + + # required environment variable is not set + app_config = AppConfig() + app_config.external_config.environment = [ + dict(name="THIS_ENV_IS_NOT_SET", required=True, path=["this", "is", "a", "path"]) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "required environment variable 'THIS_ENV_IS_NOT_SET' not set") + + @patch("server.common.config.external_config.get_secret_key") + def test_aws_secrets_manager(self, mock_get_secret_key): + mock_get_secret_key.return_value = { + "oauth_client_secret": "mock_oauth_secret", + "db_uri": "mock_db_uri", + } + configfile = self.custom_external_config( + aws_secrets_manager_region="us-west-2", + aws_secrets_manager_secrets=[ + dict( + name="my_secret", + values=[ + dict(key="flask_secret_key", path=["server", "app", "flask_secret_key"], required=False), + dict( + key="db_uri", + path=["dataset", "user_annotations", "hosted_tiledb_array", "db_uri"], + required=True, + ), + dict( + key="oauth_client_secret", + path=["server", "authentication", "params_oauth", "client_secret"], + required=True, + ), + ], + ) + ], + config_file_name="secret_external_config.yaml", + ) + + app_config = AppConfig() + app_config.update_from_config_file(configfile) + app_config.server_config.single_dataset__datapath = f"{FIXTURES_ROOT}/pbmc3k.cxg" + app_config.server_config.app__flask_secret_key = "original" + app_config.server_config.single_dataset__datapath = f"{FIXTURES_ROOT}/pbmc3k.cxg" + + app_config.complete_config() + + self.assertEqual(app_config.server_config.app__flask_secret_key, "original") + self.assertEqual(app_config.server_config.authentication__params_oauth__client_secret, "mock_oauth_secret") + self.assertEqual(app_config.default_dataset_config.user_annotations__hosted_tiledb_array__db_uri, "mock_db_uri") + + @patch("server.common.config.external_config.get_secret_key") + def test_aws_secrets_manager_error(self, mock_get_secret_key): + mock_get_secret_key.return_value = { + "oauth_client_secret": "mock_oauth_secret", + "db_uri": "mock_db_uri", + } + + # no region + app_config = AppConfig() + app_config.external_config.aws_secrets_manager__region = None + app_config.external_config.aws_secrets_manager__secrets = [ + dict(name="secret1", values=[dict(key="key1", required=True, path=["this", "is", "my", "path"])]) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual( + config_error.exception.message, + "Invalid type for attribute: aws_secrets_manager__region, expected type str, got NoneType", + ) + + # missing secret name + app_config = AppConfig() + app_config.external_config.aws_secrets_manager__region = "us-west-2" + app_config.external_config.aws_secrets_manager__secrets = [ + dict(values=[dict(key="db_uri", required=True, path=["this", "is", "my", "path"])]) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "aws_secrets_manager: 'name' is missing") + + # secret name wrong type + app_config = AppConfig() + app_config.external_config.aws_secrets_manager__region = "us-west-2" + app_config.external_config.aws_secrets_manager__secrets = [ + dict(name=1, values=[dict(key="db_uri", required=True, path=["this", "is", "my", "path"])]) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "aws_secrets_manager: 'name' must be a string") + + # missing values name + app_config = AppConfig() + app_config.external_config.aws_secrets_manager__region = "us-west-2" + app_config.external_config.aws_secrets_manager__secrets = [dict(name="mysecret")] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "aws_secrets_manager: 'values' is missing") + + # values wrong type + app_config = AppConfig() + app_config.external_config.aws_secrets_manager__region = "us-west-2" + app_config.external_config.aws_secrets_manager__secrets = [ + dict(name="mysecret", values=dict(key="db_uri", required=True, path=["this", "is", "my", "path"])) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "aws_secrets_manager: 'values' must be a list") + + # entry missing key + app_config = AppConfig() + app_config.external_config.aws_secrets_manager__region = "us-west-2" + app_config.external_config.aws_secrets_manager__secrets = [ + dict(name="mysecret", values=[dict(required=True, path=["this", "is", "my", "path"])]) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "missing 'key' in secret values: mysecret") + + # entry required is wrong type + app_config = AppConfig() + app_config.external_config.aws_secrets_manager__region = "us-west-2" + app_config.external_config.aws_secrets_manager__secrets = [ + dict(name="mysecret", values=[dict(key="db_uri", required="optional", path=["this", "is", "my", "path"])]) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "wrong type for 'required' in secret values: mysecret") + + # entry missing path + app_config = AppConfig() + app_config.external_config.aws_secrets_manager__region = "us-west-2" + app_config.external_config.aws_secrets_manager__secrets = [ + dict(name="mysecret", values=[dict(key="db_uri", required=True)]) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "missing 'path' in secret values: mysecret") + + # secret missing required key + app_config = AppConfig() + app_config.external_config.aws_secrets_manager__region = "us-west-2" + app_config.external_config.aws_secrets_manager__secrets = [ + dict( + name="mysecret", + values=[dict(key="KEY_DOES_NOT_EXIST", required=True, path=["this", "is", "a", "path"])], + ) + ] + with self.assertRaises(ConfigurationError) as config_error: + app_config.complete_config() + self.assertEqual(config_error.exception.message, "required secret 'mysecret:KEY_DOES_NOT_EXIST' not set") diff --git a/server/test/unit/common/config/test_server_config.py b/server/test/unit/common/config/test_server_config.py index 78ee9570..65974523 100644 --- a/server/test/unit/common/config/test_server_config.py +++ b/server/test/unit/common/config/test_server_config.py @@ -105,13 +105,14 @@ class TestServerConfig(ConfigTests): self.config = AppConfig() self.config.server_config.handle_app(self.context) self.assertEqual(self.config.server_config.app__port, 4008) + del os.environ["CXG_SERVER_PORT"] def test_handle_app__can_get_secret_key_from_envvar_or_config_file_with_envvar_given_preference(self): config = self.get_config(flask_secret_key="KEY_FROM_FILE") self.assertEqual(config.server_config.app__flask_secret_key, "KEY_FROM_FILE") os.environ["CXG_SECRET_KEY"] = "KEY_FROM_ENV" - config.server_config.handle_app(self.context) + config.external_config.handle_environment(self.context) self.assertEqual(config.server_config.app__flask_secret_key, "KEY_FROM_ENV") def test_handle_app__sets_web_base_url(self): @@ -124,7 +125,7 @@ class TestServerConfig(ConfigTests): self.assertEqual(config.server_config.authentication__params_oauth__client_secret, "KEY_FROM_FILE") os.environ["CXG_OAUTH_CLIENT_SECRET"] = "KEY_FROM_ENV" - config.server_config.handle_authentication() + config.external_config.handle_environment(self.context) self.assertEqual(config.server_config.authentication__params_oauth__client_secret, "KEY_FROM_ENV") @@ -215,7 +216,7 @@ class TestServerConfig(ConfigTests): # test for illegal url_dataroots for illegal in ("../b", "!$*", "\\n", "", "(bad)"): self.config.update_server_config( - multi_dataset__dataroot={"tag": {"base_url": illegal, "dataroot": "{PROJECT_ROOT}/example-dataset"}} + multi_dataset__dataroot={"tag": {"base_url": illegal, "dataroot": f"{PROJECT_ROOT}/example-dataset"}} ) with self.assertRaises(ConfigurationError): self.config.complete_config() @@ -224,7 +225,7 @@ class TestServerConfig(ConfigTests): # test for legal url_dataroots for legal in ("d", "this.is-okay_", "a/b"): self.config.update_server_config( - multi_dataset__dataroot={"tag": {"base_url": legal, "dataroot": "{PROJECT_ROOT}/example-dataset"}} + multi_dataset__dataroot={"tag": {"base_url": legal, "dataroot": f"{PROJECT_ROOT}/example-dataset"}} ) self.config.complete_config()