From 6d604a656a8bccbe41e6bb49a6620ebb25b21575 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 9 Jan 2015 16:23:31 -0500 Subject: [PATCH] Move config handling into a provider class to make testing much easier --- app.py | 64 +++----- endpoints/api/suconfig.py | 43 +++--- static/css/quay.css | 1 + .../directives/config/config-file-field.html | 4 +- test/test_suconfig_api.py | 44 +++--- util/config/configutil.py | 29 ---- util/config/provider.py | 139 ++++++++++++++++++ util/config/validator.py | 4 +- 8 files changed, 207 insertions(+), 121 deletions(-) create mode 100644 util/config/provider.py diff --git a/app.py b/app.py index f00cc23db..b1281b760 100644 --- a/app.py +++ b/app.py @@ -2,7 +2,7 @@ import logging import os import json -from flask import Flask as BaseFlask, Config as BaseConfig, request, Request +from flask import Flask, Config, request, Request from flask.ext.principal import Principal from flask.ext.login import LoginManager from flask.ext.mail import Mail @@ -14,48 +14,33 @@ from data import model from data import database from data.userfiles import Userfiles from data.users import UserAuthentication -from util.analytics import Analytics -from util.exceptionlog import Sentry -from util.queuemetrics import QueueMetrics -from util.names import urn_generator -from util.oauth import GoogleOAuthConfig, GithubOAuthConfig -from util.config.configutil import import_yaml, generate_secret_key -from data.billing import Billing from data.buildlogs import BuildLogs from data.archivedlogs import LogArchive from data.queue import WorkQueue from data.userevent import UserEventsBuilderModule + from avatars.avatars import Avatar - -class Config(BaseConfig): - """ Flask config enhanced with a `from_yamlfile` method """ - - def from_yamlfile(self, config_file): - import_yaml(self, config_file) - -class Flask(BaseFlask): - """ Extends the Flask class to implement our custom Config class. """ - - def make_config(self, instance_relative=False): - root_path = self.instance_path if instance_relative else self.root_path - return Config(root_path, self.default_config) - - -OVERRIDE_CONFIG_DIRECTORY = 'conf/stack/' -OVERRIDE_CONFIG_YAML_FILENAME = OVERRIDE_CONFIG_DIRECTORY + 'config.yaml' -OVERRIDE_CONFIG_PY_FILENAME = OVERRIDE_CONFIG_DIRECTORY + 'config.py' - -OVERRIDE_CONFIG_KEY = 'QUAY_OVERRIDE_CONFIG' -LICENSE_FILENAME = OVERRIDE_CONFIG_DIRECTORY + 'license.enc' +from util.analytics import Analytics +from data.billing import Billing +from util.config.provider import FileConfigProvider, TestConfigProvider +from util.exceptionlog import Sentry +from util.names import urn_generator +from util.oauth import GoogleOAuthConfig, GithubOAuthConfig +from util.queuemetrics import QueueMetrics +from util.config.configutil import generate_secret_key app = Flask(__name__) logger = logging.getLogger(__name__) profile = logging.getLogger('profile') +CONFIG_PROVIDER = FileConfigProvider('conf/stack/', 'config.yaml', 'config.py') +# Instantiate the default configuration (for test or for normal operation). if 'TEST' in os.environ: + CONFIG_PROVIDER = TestConfigProvider() + from test.testconfig import TestConfig logger.debug('Loading test config.') app.config.from_object(TestConfig()) @@ -63,20 +48,17 @@ else: from config import DefaultConfig logger.debug('Loading default config.') app.config.from_object(DefaultConfig()) - - if os.path.exists(OVERRIDE_CONFIG_PY_FILENAME): - logger.debug('Applying config file: %s', OVERRIDE_CONFIG_PY_FILENAME) - app.config.from_pyfile(OVERRIDE_CONFIG_PY_FILENAME) - - if os.path.exists(OVERRIDE_CONFIG_YAML_FILENAME): - logger.debug('Applying config file: %s', OVERRIDE_CONFIG_YAML_FILENAME) - app.config.from_yamlfile(OVERRIDE_CONFIG_YAML_FILENAME) - - environ_config = json.loads(os.environ.get(OVERRIDE_CONFIG_KEY, '{}')) - app.config.update(environ_config) - app.teardown_request(database.close_db_filter) +# Load the override config via the provider. +CONFIG_PROVIDER.update_app_config(app.config) + +# Update any configuration found in the override environment variable. +OVERRIDE_CONFIG_KEY = 'QUAY_OVERRIDE_CONFIG' + +environ_config = json.loads(os.environ.get(OVERRIDE_CONFIG_KEY, '{}')) +app.config.update(environ_config) + class RequestWithId(Request): request_gen = staticmethod(urn_generator(['request'])) diff --git a/endpoints/api/suconfig.py b/endpoints/api/suconfig.py index d403b617b..50b3635cf 100644 --- a/endpoints/api/suconfig.py +++ b/endpoints/api/suconfig.py @@ -3,17 +3,16 @@ import os import json from flask import abort -from endpoints.api import (ApiResource, nickname, resource, internal_only, show_if, hide_if, +from endpoints.api import (ApiResource, nickname, resource, internal_only, show_if, require_fresh_login, request, validate_json_request, verify_not_prod) from endpoints.common import common_login -from app import app, OVERRIDE_CONFIG_YAML_FILENAME, OVERRIDE_CONFIG_DIRECTORY +from app import app, CONFIG_PROVIDER from data import model from auth.permissions import SuperUserPermission from auth.auth_context import get_authenticated_user from data.database import User -from util.config.configutil import (import_yaml, export_yaml, add_enterprise_config_defaults, - set_config_value) +from util.config.configutil import add_enterprise_config_defaults from util.config.validator import validate_service_for_config, SSL_FILENAMES import features @@ -32,10 +31,6 @@ def database_has_users(): """ Returns whether the database has any users defined. """ return bool(list(User.select().limit(1))) -def config_file_exists(): - """ Returns whether a configuration file exists. """ - return os.path.exists(OVERRIDE_CONFIG_YAML_FILENAME) - @resource('/v1/superuser/registrystatus') @internal_only @@ -48,12 +43,13 @@ class SuperUserRegistryStatus(ApiResource): @verify_not_prod def get(self): """ Returns whether a valid configuration, database and users exist. """ + file_exists = CONFIG_PROVIDER.yaml_exists() return { - 'dir_exists': os.path.exists(OVERRIDE_CONFIG_DIRECTORY), - 'file_exists': os.path.exists(OVERRIDE_CONFIG_YAML_FILENAME), + 'dir_exists': CONFIG_PROVIDER.volume_exists(), + 'file_exists': file_exists, 'is_testing': app.config['TESTING'], 'valid_db': database_is_valid(), - 'ready': not app.config['TESTING'] and config_file_exists() and bool(app.config['SUPER_USERS']) + 'ready': not app.config['TESTING'] and file_exists and bool(app.config['SUPER_USERS']) } @@ -88,12 +84,7 @@ class SuperUserConfig(ApiResource): def get(self): """ Returns the currently defined configuration, if any. """ if SuperUserPermission().can(): - config_object = {} - try: - import_yaml(config_object, OVERRIDE_CONFIG_YAML_FILENAME) - except Exception: - config_object = None - + config_object = CONFIG_PROVIDER.get_yaml() return { 'config': config_object } @@ -107,7 +98,7 @@ class SuperUserConfig(ApiResource): """ Updates the config.yaml file. """ # Note: This method is called to set the database configuration before super users exists, # so we also allow it to be called if there is no valid registry configuration setup. - if not config_file_exists() or SuperUserPermission().can(): + if not CONFIG_PROVIDER.yaml_exists() or SuperUserPermission().can(): config_object = request.get_json()['config'] hostname = request.get_json()['hostname'] @@ -115,7 +106,7 @@ class SuperUserConfig(ApiResource): add_enterprise_config_defaults(config_object, app.config['SECRET_KEY'], hostname) # Write the configuration changes to the YAML file. - export_yaml(config_object, OVERRIDE_CONFIG_YAML_FILENAME) + CONFIG_PROVIDER.save_yaml(config_object) return { 'exists': True, @@ -139,7 +130,7 @@ class SuperUserConfigFile(ApiResource): if SuperUserPermission().can(): return { - 'exists': os.path.exists(os.path.join(OVERRIDE_CONFIG_DIRECTORY, filename)) + 'exists': CONFIG_PROVIDER.volume_file_exists(filename) } abort(403) @@ -156,7 +147,7 @@ class SuperUserConfigFile(ApiResource): if not uploaded_file: abort(400) - uploaded_file.save(os.path.join(OVERRIDE_CONFIG_DIRECTORY, filename)) + CONFIG_PROVIDER.save_volume_file(filename, uploaded_file) return { 'status': True } @@ -209,7 +200,7 @@ class SuperUserCreateInitialSuperUser(ApiResource): # # We do this special security check because at the point this method is called, the database # is clean but does not (yet) have any super users for our permissions code to check against. - if config_file_exists() and not database_has_users(): + if CONFIG_PROVIDER.yaml_exists() and not database_has_users(): data = request.get_json() username = data['username'] password = data['password'] @@ -219,7 +210,11 @@ class SuperUserCreateInitialSuperUser(ApiResource): superuser = model.create_user(username, password, email, auto_verify=True) # Add the user to the config. - set_config_value(OVERRIDE_CONFIG_YAML_FILENAME, 'SUPER_USERS', [username]) + config_object = CONFIG_PROVIDER.get_yaml() + config_object['SUPER_USERS'] = [username] + CONFIG_PROVIDER.save_yaml(config_object) + + # Update the in-memory config for the new superuser. app.config['SUPER_USERS'] = [username] # Conduct login with that user. @@ -262,7 +257,7 @@ class SuperUserConfigValidate(ApiResource): # Note: This method is called to validate the database configuration before super users exists, # so we also allow it to be called if there is no valid registry configuration setup. Note that # this is also safe since this method does not access any information not given in the request. - if not config_file_exists() or SuperUserPermission().can(): + if not CONFIG_PROVIDER.yaml_exists() or SuperUserPermission().can(): config = request.get_json()['config'] return validate_service_for_config(service, config) diff --git a/static/css/quay.css b/static/css/quay.css index 0ae6bf47b..08d8361af 100644 --- a/static/css/quay.css +++ b/static/css/quay.css @@ -4923,6 +4923,7 @@ i.slack-icon { border: 1px solid #eee; vertical-align: middle; padding: 4px; + max-width: 150px; } .modal-footer.alert { diff --git a/static/directives/config/config-file-field.html b/static/directives/config/config-file-field.html index a819c7300..7e4710905 100644 --- a/static/directives/config/config-file-field.html +++ b/static/directives/config/config-file-field.html @@ -2,9 +2,7 @@ {{ filename }} {{ filename }} not found in mounted config directory: - - - + Uploading file as {{ filename }}... {{ uploadProgress }}% diff --git a/test/test_suconfig_api.py b/test/test_suconfig_api.py index 44234d7d6..9cf72f573 100644 --- a/test/test_suconfig_api.py +++ b/test/test_suconfig_api.py @@ -1,19 +1,28 @@ from test.test_api_usage import ApiTestCase, READ_ACCESS_USER, ADMIN_ACCESS_USER from endpoints.api.suconfig import (SuperUserRegistryStatus, SuperUserConfig, SuperUserConfigFile, SuperUserCreateInitialSuperUser, SuperUserConfigValidate) -from app import OVERRIDE_CONFIG_YAML_FILENAME +from app import CONFIG_PROVIDER from data.database import User import unittest -import os + + +class ConfigForTesting(object): + def __enter__(self): + CONFIG_PROVIDER.reset_for_test() + return CONFIG_PROVIDER + + def __exit__(self, type, value, traceback): + pass class TestSuperUserRegistryStatus(ApiTestCase): def test_registry_status(self): - json = self.getJsonResponse(SuperUserRegistryStatus) - self.assertTrue(json['is_testing']) - self.assertTrue(json['valid_db']) - self.assertFalse(json['file_exists']) - self.assertFalse(json['ready']) + with ConfigForTesting(): + json = self.getJsonResponse(SuperUserRegistryStatus) + self.assertTrue(json['is_testing']) + self.assertTrue(json['valid_db']) + self.assertFalse(json['file_exists']) + self.assertFalse(json['ready']) class TestSuperUserConfigFile(ApiTestCase): @@ -58,7 +67,7 @@ class TestSuperUserCreateInitialSuperUser(ApiTestCase): self.postResponse(SuperUserCreateInitialSuperUser, data=data, expected_code=403) def test_config_file_with_db_users(self): - try: + with ConfigForTesting(): # Write some config. self.putJsonResponse(SuperUserConfig, data=dict(config={}, hostname='foobar')) @@ -66,11 +75,9 @@ class TestSuperUserCreateInitialSuperUser(ApiTestCase): # fail. data = dict(username='cooluser', password='password', email='fake@example.com') self.postResponse(SuperUserCreateInitialSuperUser, data=data, expected_code=403) - finally: - os.remove(OVERRIDE_CONFIG_YAML_FILENAME) def test_config_file_with_no_db_users(self): - try: + with ConfigForTesting(): # Write some config. self.putJsonResponse(SuperUserConfig, data=dict(config={}, hostname='foobar')) @@ -90,9 +97,6 @@ class TestSuperUserCreateInitialSuperUser(ApiTestCase): result = self.getJsonResponse(SuperUserConfig) self.assertEquals(['cooluser'], result['config']['SUPER_USERS']) - finally: - os.remove(OVERRIDE_CONFIG_YAML_FILENAME) - class TestSuperUserConfigValidate(ApiTestCase): def test_nonsuperuser_noconfig(self): @@ -104,7 +108,7 @@ class TestSuperUserConfigValidate(ApiTestCase): def test_nonsuperuser_config(self): - try: + with ConfigForTesting(): # The validate config call works if there is no config.yaml OR the user is a superuser. # Add a config, and verify it breaks when unauthenticated. json = self.putJsonResponse(SuperUserConfig, data=dict(config={}, hostname='foobar')) @@ -120,8 +124,6 @@ class TestSuperUserConfigValidate(ApiTestCase): data=dict(config={})) self.assertFalse(result['status']) - finally: - os.remove(OVERRIDE_CONFIG_YAML_FILENAME) class TestSuperUserConfig(ApiTestCase): @@ -142,14 +144,14 @@ class TestSuperUserConfig(ApiTestCase): self.assertIsNone(json['config']) def test_put(self): - try: + with ConfigForTesting() as config: # The update config call works if there is no config.yaml OR the user is a superuser. First # try writing it without a superuser present. json = self.putJsonResponse(SuperUserConfig, data=dict(config={}, hostname='foobar')) self.assertTrue(json['exists']) - # Verify the config.yaml file exists. - self.assertTrue(os.path.exists(OVERRIDE_CONFIG_YAML_FILENAME)) + # Verify the config file exists. + self.assertTrue(config.yaml_exists()) # Try writing it again. This should now fail, since the config.yaml exists. self.putResponse(SuperUserConfig, data=dict(config={}, hostname='barbaz'), expected_code=403) @@ -170,8 +172,6 @@ class TestSuperUserConfig(ApiTestCase): json = self.getJsonResponse(SuperUserConfig) self.assertIsNotNone(json['config']) - finally: - os.remove(OVERRIDE_CONFIG_YAML_FILENAME) if __name__ == '__main__': unittest.main() \ No newline at end of file diff --git a/util/config/configutil.py b/util/config/configutil.py index 981a9150c..7a31390c9 100644 --- a/util/config/configutil.py +++ b/util/config/configutil.py @@ -7,35 +7,6 @@ def generate_secret_key(): return str(cryptogen.getrandbits(256)) -def import_yaml(config_obj, config_file): - with open(config_file) as f: - c = yaml.safe_load(f) - if not c: - logger.debug('Empty YAML config file') - return - - if isinstance(c, str): - raise Exception('Invalid YAML config file: ' + str(c)) - - for key in c.iterkeys(): - if key.isupper(): - config_obj[key] = c[key] - - -def export_yaml(config_obj, config_file): - with open(config_file, 'w') as f: - f.write(yaml.safe_dump(config_obj, encoding='utf-8', allow_unicode=True)) - - -def set_config_value(config_file, config_key, value): - """ Loads the configuration from the given YAML config file, sets the given key to - the given value, and then writes it back out to the given YAML config file. """ - config_obj = {} - import_yaml(config_obj, config_file) - config_obj[config_key] = value - export_yaml(config_obj, config_file) - - def add_enterprise_config_defaults(config_obj, current_secret_key, hostname): """ Adds/Sets the config defaults for enterprise registry config. """ # These have to be false. diff --git a/util/config/provider.py b/util/config/provider.py new file mode 100644 index 000000000..c4ddbc97c --- /dev/null +++ b/util/config/provider.py @@ -0,0 +1,139 @@ +import os +import yaml +import logging +import json + +logger = logging.getLogger(__name__) + +def _import_yaml(config_obj, config_file): + with open(config_file) as f: + c = yaml.safe_load(f) + if not c: + logger.debug('Empty YAML config file') + return + + if isinstance(c, str): + raise Exception('Invalid YAML config file: ' + str(c)) + + for key in c.iterkeys(): + if key.isupper(): + config_obj[key] = c[key] + + return config_obj + + +def _export_yaml(config_obj, config_file): + with open(config_file, 'w') as f: + f.write(yaml.safe_dump(config_obj, encoding='utf-8', allow_unicode=True)) + + +class BaseProvider(object): + """ A configuration provider helps to load, save, and handle config override in the application. + """ + + def update_app_config(self, app_config): + """ Updates the given application config object with the loaded override config. """ + raise NotImplementedError + + def get_yaml(self): + """ Returns the contents of the YAML config override file, or None if none. """ + raise NotImplementedError + + def save_yaml(self, config_object): + """ Updates the contents of the YAML config override file to those given. """ + raise NotImplementedError + + def yaml_exists(self): + """ Returns true if a YAML config override file exists in the config volume. """ + raise NotImplementedError + + def volume_exists(self): + """ Returns whether the config override volume exists. """ + raise NotImplementedError + + def volume_file_exists(self, filename): + """ Returns whether the file with the given name exists under the config override volume. """ + raise NotImplementedError + + def save_volume_file(self, filename, flask_file): + """ Saves the given flask file to the config override volume, with the given + filename. + """ + raise NotImplementedError + + +class FileConfigProvider(BaseProvider): + """ Implementation of the config provider that reads the data from the file system. """ + def __init__(self, config_volume, yaml_filename, py_filename): + self.config_volume = config_volume + self.yaml_filename = yaml_filename + self.py_filename = py_filename + + self.yaml_path = os.path.join(config_volume, yaml_filename) + self.py_path = os.path.join(config_volume, py_filename) + + def update_app_config(self, app_config): + if os.path.exists(self.py_path): + logger.debug('Applying config file: %s', self.py_path) + app_config.from_pyfile(self.py_path) + + if os.path.exists(self.yaml_path): + logger.debug('Applying config file: %s', self.yaml_path) + _import_yaml(app_config, self.yaml_path) + + def get_yaml(self): + if not os.path.exists(self.yaml_path): + return None + + config_obj = {} + _import_yaml(config_obj, self.yaml_path) + return config_obj + + def save_yaml(self, config_obj): + _export_yaml(config_obj, self.yaml_path) + + def yaml_exists(self): + return self.volume_file_exists(self.yaml_filename) + + def volume_exists(self): + return os.path.exists(self.config_volume) + + def volume_file_exists(self, filename): + return os.path.exists(os.path.join(self.config_volume, filename)) + + def save_volume_file(self, filename, flask_file): + flask_file.save(os.path.join(self.config_volume, filename)) + + +class TestConfigProvider(BaseProvider): + """ Implementation of the config provider for testing. Everything is kept in-memory instead on + the real file system. """ + def __init__(self): + self.files = {} + + def update_app_config(self, app_config): + pass + + def get_yaml(self): + if not 'config.yaml' in self.files: + return None + + return json.loads(self.files.get('config.yaml', '{}')) + + def save_yaml(self, config_obj): + self.files['config.yaml'] = json.dumps(config_obj) + + def yaml_exists(self): + return 'config.yaml' in self.files + + def volume_exists(self): + return True + + def volume_file_exists(self, filename): + return filename in self.files + + def save_volume_file(self, filename, flask_file): + self.files[filename] = '' + + def reset_for_test(self): + self.files = {} diff --git a/util/config/validator.py b/util/config/validator.py index 969f26fed..ec6d6f708 100644 --- a/util/config/validator.py +++ b/util/config/validator.py @@ -9,7 +9,7 @@ from flask import Flask from flask.ext.mail import Mail, Message from data.database import validate_database_url, User from storage import get_storage_driver -from app import app, OVERRIDE_CONFIG_DIRECTORY +from app import app, CONFIG_PROVIDER from auth.auth_context import get_authenticated_user from util.oauth import GoogleOAuthConfig, GithubOAuthConfig @@ -138,7 +138,7 @@ def _validate_ssl(config): return for filename in SSL_FILENAMES: - if not os.path.exists(os.path.join(OVERRIDE_CONFIG_DIRECTORY, filename)): + if not CONFIG_PROVIDER.volume_file_exists(filename): raise Exception('Missing required SSL file: %s' % filename)