Move config handling into a provider class to make testing much easier

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
 
-
+from util.analytics import Analytics
-class Config(BaseConfig):
+from data.billing import Billing
-  """ Flask config enhanced with a `from_yamlfile` method """
+from util.config.provider import FileConfigProvider, TestConfigProvider
-
+from util.exceptionlog import Sentry
-  def from_yamlfile(self, config_file):
+from util.names import urn_generator
-    import_yaml(self, config_file)
+from util.oauth import GoogleOAuthConfig, GithubOAuthConfig
-
+from util.queuemetrics import QueueMetrics
-class Flask(BaseFlask):
+from util.config.configutil import generate_secret_key
-  """ 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'
 
 
 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())
+  app.teardown_request(database.close_db_filter)
 
-  if os.path.exists(OVERRIDE_CONFIG_PY_FILENAME):
+# Load the override config via the provider.
-    logger.debug('Applying config file: %s', OVERRIDE_CONFIG_PY_FILENAME)
+CONFIG_PROVIDER.update_app_config(app.config)
-    app.config.from_pyfile(OVERRIDE_CONFIG_PY_FILENAME)
 
-  if os.path.exists(OVERRIDE_CONFIG_YAML_FILENAME):
+# Update any configuration found in the override environment variable.
-    logger.debug('Applying config file: %s', OVERRIDE_CONFIG_YAML_FILENAME)
+OVERRIDE_CONFIG_KEY = 'QUAY_OVERRIDE_CONFIG'
-    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)
-
 
 class RequestWithId(Request):
   request_gen = staticmethod(urn_generator(['request']))

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,
+from util.config.configutil import add_enterprise_config_defaults
-                                    set_config_value)
 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),
+      'dir_exists': CONFIG_PROVIDER.volume_exists(),
-      'file_exists': os.path.exists(OVERRIDE_CONFIG_YAML_FILENAME),
+      '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 = {}
+      config_object = CONFIG_PROVIDER.get_yaml()
-      try:
-        import_yaml(config_object, OVERRIDE_CONFIG_YAML_FILENAME)
-      except Exception:
-        config_object = None
-
       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)
 

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 {

static/directives/config/config-file-field.html

@@ -2,10 +2,8 @@
   <span ng-show="uploadProgress == null">
     <span ng-if="hasFile"><code>{{ filename }}</code></span>
     <span class="nofile" ng-if="!hasFile"><code>{{ filename }}</code> not found in mounted config directory: </span>
-    <span ng-if="!hasFile">
     <input type="file" ng-file-select="onFileSelect($files)">
   </span>
-  </span>
   <span ng-show="uploadProgress != null">
     Uploading file as <strong>{{ filename }}</strong>... {{ uploadProgress }}%
   </span>

test/test_suconfig_api.py

@@ -1,14 +1,23 @@
 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):
+    with ConfigForTesting():
       json = self.getJsonResponse(SuperUserRegistryStatus)
       self.assertTrue(json['is_testing'])
       self.assertTrue(json['valid_db'])
@@ -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.
+      # Verify the config file exists.
-      self.assertTrue(os.path.exists(OVERRIDE_CONFIG_YAML_FILENAME))
+      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()

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.

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 = {}

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)