Further fixes to the Kubernetes config provider, and a new set of proper unit tests

endpoints/api/suconfig.py

@@ -291,7 +291,7 @@ class SuperUserConfigFile(ApiResource):
       if not uploaded_file:
         abort(400)
 
-      config_provider.save_volume_file(filename, uploaded_file)
+      config_provider.save_volume_file(uploaded_file, filename)
       return {
         'status': True
       }

endpoints/api/superuser.py

@@ -927,7 +927,7 @@ class SuperUserCustomCertificate(ApiResource):
 
       logger.debug('Saving custom certificate %s', certpath)
       cert_full_path = config_provider.get_volume_path(EXTRA_CA_DIRECTORY, certpath)
-      config_provider.save_volume_file(cert_full_path, uploaded_file)
+      config_provider.save_volume_file(uploaded_file, cert_full_path)
       logger.debug('Saved custom certificate %s', certpath)
 
       # Validate the certificate.

util/config/provider/basefileprovider.py

@@ -39,14 +39,14 @@ class BaseFileProvider(BaseProvider):
   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 volume_file_exists(self, relative_file_path):
+    return os.path.exists(os.path.join(self.config_volume, relative_file_path))
 
-  def get_volume_file(self, filename, mode='r'):
-    return open(os.path.join(self.config_volume, filename), mode=mode)
+  def get_volume_file(self, relative_file_path, mode='r'):
+    return open(os.path.join(self.config_volume, relative_file_path), mode=mode)
 
-  def get_volume_path(self, directory, filename):
-    return os.path.join(directory, filename)
+  def get_volume_path(self, directory, relative_file_path):
+    return os.path.join(directory, relative_file_path)
 
   def list_volume_directory(self, path):
     dirpath = os.path.join(self.config_volume, path)

util/config/provider/baseprovider.py

@@ -89,20 +89,17 @@ class BaseProvider(object):
     """ Returns whether the config override volume exists. """
 
   @abstractmethod
-  def volume_file_exists(self, filename):
-    """ Returns whether the file with the given name exists under the config override volume. """
+  def volume_file_exists(self, relative_file_path):
+    """ Returns whether the file with the given relative path exists under the config override
+        volume. """
 
   @abstractmethod
-  def get_volume_file(self, filename, mode='r'):
-    """ Returns a Python file referring to the given name under the config override volume. """
+  def get_volume_file(self, relative_file_path, mode='r'):
+    """ Returns a Python file referring to the given path under the config override volume. """
 
   @abstractmethod
-  def write_volume_file(self, filename, contents):
-    """ Writes the given contents to the config override volumne, with the given filename. """
-
-  @abstractmethod
-  def remove_volume_file(self, filename):
-    """ Removes the config override volume file with the given filename. """
+  def remove_volume_file(self, relative_file_path):
+    """ Removes the config override volume file with the given path. """
 
   @abstractmethod
   def list_volume_directory(self, path):
@@ -111,9 +108,9 @@ class BaseProvider(object):
     """
 
   @abstractmethod
-  def save_volume_file(self, filename, flask_file):
+  def save_volume_file(self, flask_file, relative_file_path):
     """ Saves the given flask file to the config override volume, with the given
-        filename.
+        relative path.
     """
 
   @abstractmethod
@@ -124,5 +121,5 @@ class BaseProvider(object):
 
   @abstractmethod
   def get_volume_path(self, directory, filename):
-    """ Helper for constructing file paths, which may differ between providers. For example,
-    kubernetes can't have subfolders in configmaps """
+    """ Helper for constructing relative file paths, which may differ between providers.
+        For example, kubernetes can't have subfolders in configmaps """

util/config/provider/fileprovider.py

@@ -30,24 +30,12 @@ class FileConfigProvider(BaseFileProvider):
   def save_config(self, config_obj):
     export_yaml(config_obj, self.yaml_path)
 
-  def write_volume_file(self, filename, contents):
-    filepath = os.path.join(self.config_volume, filename)
-    _ensure_parent_dir(filepath)
-
-    try:
-      with open(filepath, mode='w') as f:
-        f.write(contents)
-    except IOError as ioe:
-      raise CannotWriteConfigException(str(ioe))
-
-    return filepath
-
-  def remove_volume_file(self, filename):
-    filepath = os.path.join(self.config_volume, filename)
+  def remove_volume_file(self, relative_file_path):
+    filepath = os.path.join(self.config_volume, relative_file_path)
     os.remove(filepath)
 
-  def save_volume_file(self, filename, flask_file):
-    filepath = os.path.join(self.config_volume, filename)
+  def save_volume_file(self, flask_file, relative_file_path):
+    filepath = os.path.join(self.config_volume, relative_file_path)
     _ensure_parent_dir(filepath)
 
     # Write the file.

util/config/provider/k8sprovider.py

@@ -4,6 +4,7 @@ import json
 import base64
 import time
 
+from cStringIO import StringIO
 from requests import Request, Session
 
 from util.config.provider.baseprovider import CannotWriteConfigException, get_yaml
@@ -25,15 +26,20 @@ QE_CONFIG_SECRET = os.environ.get('QE_K8S_CONFIG_SECRET', 'quay-enterprise-confi
 class KubernetesConfigProvider(BaseFileProvider):
   """ Implementation of the config provider that reads and writes configuration
       data from a Kubernetes Secret. """
-  def __init__(self, config_volume, yaml_filename, py_filename):
+  def __init__(self, config_volume, yaml_filename, py_filename, api_host=None,
+               service_account_token_path=None):
     super(KubernetesConfigProvider, self).__init__(config_volume, yaml_filename, py_filename)
+    service_account_token_path = service_account_token_path or SERVICE_ACCOUNT_TOKEN_PATH
+    api_host = api_host or KUBERNETES_API_HOST
 
     # Load the service account token from the local store.
-    if not os.path.exists(SERVICE_ACCOUNT_TOKEN_PATH):
+    if not os.path.exists(service_account_token_path):
       raise Exception('Cannot load Kubernetes service account token')
 
-    with open(SERVICE_ACCOUNT_TOKEN_PATH, 'r') as f:
+    with open(service_account_token_path, 'r') as f:
       self._service_token = f.read()
+    
+    self._api_host = api_host
 
   @property
   def provider_id(self):
@@ -44,13 +50,16 @@ class KubernetesConfigProvider(BaseFileProvider):
     # in Kubernetes secrets.
     return "_".join([directory.rstrip('/'), filename])
 
-  def volume_file_exists(self, filename):
+  def volume_file_exists(self, relative_file_path):
+    if '/' in relative_file_path:
+      raise Exception('Expected path from get_volume_path, but found slashes')
+
     # NOTE: Overridden because we don't have subdirectories, which aren't supported
     # in Kubernetes secrets.
     secret = self._lookup_secret()
     if not secret or not secret.get('data'):
       return False
-    return filename in secret['data']
+    return relative_file_path in secret['data']
 
   def list_volume_directory(self, path):
     # NOTE: Overridden because we don't have subdirectories, which aren't supported
@@ -69,22 +78,24 @@ class KubernetesConfigProvider(BaseFileProvider):
   def save_config(self, config_obj):
     self._update_secret_file(self.yaml_filename, get_yaml(config_obj))
 
-  def write_volume_file(self, filename, contents):
+  def remove_volume_file(self, relative_file_path):
     try:
-      self._update_secret_file(filename, contents)
+      self._update_secret_file(relative_file_path, None)
     except IOError as ioe:
       raise CannotWriteConfigException(str(ioe))
 
-  def remove_volume_file(self, filename):
+  def save_volume_file(self, flask_file, relative_file_path):
+    # Write the file to a temp location.
+    buf = StringIO()
     try:
-      self._update_secret_file(filename, None)
-    except IOError as ioe:
-      raise CannotWriteConfigException(str(ioe))
+      try:
+        flask_file.save(buf)
+      except IOError as ioe:
+        raise CannotWriteConfigException(str(ioe))
 
-  def save_volume_file(self, filename, flask_file):
-    filepath = super(KubernetesConfigProvider, self).save_volume_file(filename, flask_file)
-    with open(filepath, 'r') as f:
-      self.write_volume_file(filename, f.read())
+      self._update_secret_file(relative_file_path, buf.getvalue())
+    finally:
+      buf.close()
 
   def _assert_success(self, response):
     if response.status_code != 200:
@@ -92,7 +103,10 @@ class KubernetesConfigProvider(BaseFileProvider):
                    response.text)
       raise CannotWriteConfigException('Kubernetes API call failed: %s' % response.text)
 
-  def _update_secret_file(self, filename, value=None):
+  def _update_secret_file(self, relative_file_path, value=None):
+    if '/' in relative_file_path:
+      raise Exception('Expected path from get_volume_path, but found slashes')
+
     # Check first that the namespace for Quay Enterprise exists. If it does not, report that
     # as an error, as it seems to be a common issue.
     namespace_url = 'namespaces/%s' % (QE_NAMESPACE)
@@ -119,9 +133,9 @@ class KubernetesConfigProvider(BaseFileProvider):
     secret['data'] = secret.get('data', {})
 
     if value is not None:
-      secret['data'][filename] = base64.b64encode(value)
+      secret['data'][relative_file_path] = base64.b64encode(value)
     else:
-      secret['data'].pop(filename)
+      secret['data'].pop(relative_file_path)
 
     self._assert_success(self._execute_k8s_api('PUT', secret_url, secret))
 
@@ -164,7 +178,7 @@ class KubernetesConfigProvider(BaseFileProvider):
 
     data = json.dumps(data) if data else None
     session = Session()
-    url = 'https://%s/api/v1/%s' % (KUBERNETES_API_HOST, relative_url)
+    url = 'https://%s/api/v1/%s' % (self._api_host, relative_url)
 
     request = Request(method, url, data=data, headers=headers)
     return session.send(request.prepare(), verify=False, timeout=2)

util/config/provider/test/test_k8sprovider.py

@@ -1,60 +1,138 @@
+import base64
+import os
+import json
+import uuid
+
 import pytest
-from mock import Mock
+
+from contextlib import contextmanager
+from collections import namedtuple
+from httmock import urlmatch, HTTMock
 
 from util.config.provider import KubernetesConfigProvider
 
-from test.fixtures import *
+def normalize_path(path):
+  return path.replace('/', '_')
+
+@contextmanager
+def fake_kubernetes_api(tmpdir_factory, files=None):
+  hostname = 'kubapi'
+  service_account_token_path = str(tmpdir_factory.mktemp("k8s").join("serviceaccount"))
+  auth_header = str(uuid.uuid4())
+
+  with open(service_account_token_path, 'w') as f:
+    f.write(auth_header)
+
+  global secret
+  secret = {
+    'data': {}
+  }
+
+  def write_file(config_dir, filepath, value):
+    normalized_path = normalize_path(filepath)
+    absolute_path = str(config_dir.join(normalized_path))
+    try:
+      os.makedirs(os.path.dirname(absolute_path))
+    except OSError:
+      pass
+
+    with open(absolute_path, 'w') as f:
+      f.write(value)
+
+  config_dir = tmpdir_factory.mktemp("config")
+  if files:
+    for filepath, value in files.iteritems():
+      normalized_path = normalize_path(filepath)
+      write_file(config_dir, filepath, value)
+      secret['data'][normalized_path] = base64.b64encode(value)
+
+  @urlmatch(netloc=hostname,
+            path='/api/v1/namespaces/quay-enterprise/secrets/quay-enterprise-config-secret$',
+            method='get')
+  def get_secret(_, __):
+    return {'status_code': 200, 'content': json.dumps(secret)}
+
+  @urlmatch(netloc=hostname,
+            path='/api/v1/namespaces/quay-enterprise/secrets/quay-enterprise-config-secret$',
+            method='put')
+  def put_secret(_, request):
+    updated_secret = json.loads(request.body)
+    for filepath, value in updated_secret['data'].iteritems():
+      if filepath not in secret['data']:
+        # Add
+        write_file(config_dir, filepath, base64.b64decode(value))
+
+    for filepath in secret['data']:
+      if filepath not in updated_secret['data']:
+        # Remove.
+        normalized_path = normalize_path(filepath)
+        os.remove(str(config_dir.join(normalized_path)))
+
+    secret['data'] = updated_secret['data']
+    return {'status_code': 200, 'content': json.dumps(secret)}
+
+  @urlmatch(netloc=hostname, path='/api/v1/namespaces/quay-enterprise$')
+  def get_namespace(_, __):
+    return {'status_code': 200, 'content': json.dumps({})}
+
+  @urlmatch(netloc=hostname)
+  def catch_all(url, _):
+    print url
+    return {'status_code': 404, 'content': '{}'}
+
+  with HTTMock(get_secret, put_secret, get_namespace, catch_all):
+    provider = KubernetesConfigProvider(str(config_dir), 'config.yaml', 'config.py',
+                                        api_host=hostname,
+                                        service_account_token_path=service_account_token_path)
+
+    # Validate all the files.
+    for filepath, value in files.iteritems():
+      normalized_path = normalize_path(filepath)
+      assert provider.volume_file_exists(normalized_path)
+      with provider.get_volume_file(normalized_path) as f:
+        assert f.read() == value
+
+    yield provider
 
 
-class TestKubernetesConfigProvider(KubernetesConfigProvider):
-  def __init__(self):
-    self.config_volume = ''
-    self.yaml_filename = 'yaml_filename'
-    self.py_filename = None
+def test_basic_config(tmpdir_factory):
+  basic_files = {
+    'config.yaml': 'FOO: bar',
+  }
 
-    self.yaml_path = os.path.join(self.config_volume, self.yaml_filename)
-
-    self._service_token = 'service_token'
-    self._execute_k8s_api = Mock()
+  with fake_kubernetes_api(tmpdir_factory, files=basic_files) as provider:
+    assert provider.config_exists()
+    assert provider.get_config() is not None
+    assert provider.get_config()['FOO'] == 'bar'
 
 
-@pytest.mark.parametrize('directory,filename,expected', [
-  ("directory", "file", "directory_file"),
-  ("directory/dir", "file", "directory/dir_file"),
-  ("directory/dir/", "file", "directory/dir_file"),
-  ("directory", "file/test", "directory_file/test"),
+@pytest.mark.parametrize('filepath', [
+  'foo',
+  'foo/meh',
+  'foo/bar/baz',
 ])
-def test_get_volume_path(directory, filename, expected):
-  provider = TestKubernetesConfigProvider()
-  assert expected == provider.get_volume_path(directory, filename)
+def test_remove_file(filepath, tmpdir_factory):
+  basic_files = {
+    filepath: 'foo',
+  }
+
+  with fake_kubernetes_api(tmpdir_factory, files=basic_files) as provider:
+    normalized_path = normalize_path(filepath)
+    assert provider.volume_file_exists(normalized_path)
+    provider.remove_volume_file(normalized_path)
+    assert not provider.volume_file_exists(normalized_path)
 
 
-@pytest.mark.parametrize('response,expected', [
-  (Mock(text="{\"data\": {\"license\":\"test\"}}", status_code=200), {"data": {"license":"test"}}),
-  (Mock(text="{\"data\": {\"license\":\"test\"}}", status_code=404), None),
-])
-def test_lookup_secret(response, expected):
-  provider = TestKubernetesConfigProvider()
-  provider._execute_k8s_api.return_value = response
-  assert expected == provider._lookup_secret()
-  
-  
-@pytest.mark.parametrize('response,key,expected', [
-  (Mock(text="{\"data\": {\"license\":\"test\"}}", status_code=200), "license", True),
-  (Mock(text="{\"data\": {\"license\":\"test\"}}", status_code=200), "config.yaml", False),
-  (Mock(text="", status_code=404), "license", False),
-])
-def test_volume_file_exists(response, key, expected):
-  provider = TestKubernetesConfigProvider()
-  provider._execute_k8s_api.return_value = response
-  assert expected == provider.volume_file_exists(key)
+class TestFlaskFile(object):
+  def save(self, buf):
+    buf.write('hello world!')
 
 
-@pytest.mark.parametrize('response,expected', [
-  (Mock(text="{\"data\": {\"extra_license\":\"test\"}}", status_code=200), ["license"]),
-  (Mock(text="", status_code=404), []),
-])
-def test_list_volume_directory(response, expected):
-  provider = TestKubernetesConfigProvider()
-  provider._execute_k8s_api.return_value = response
-  assert expected == provider.list_volume_directory("extra")
+def test_save_file(tmpdir_factory):
+  basic_files = {}
+
+  with fake_kubernetes_api(tmpdir_factory, files=basic_files) as provider:
+    assert not provider.volume_file_exists('testfile')
+    flask_file = TestFlaskFile()
+    provider.save_volume_file(flask_file, 'testfile')
+    assert provider.volume_file_exists('testfile')

util/config/provider/testprovider.py

@@ -45,12 +45,9 @@ class TestConfigProvider(BaseProvider):
 
     return filename in self.files
 
-  def save_volume_file(self, filename, flask_file):
+  def save_volume_file(self, flask_file, filename):
     self.files[filename] = flask_file.read()
 
-  def write_volume_file(self, filename, contents):
-    self.files[filename] = contents
-
   def get_volume_file(self, filename, mode='r'):
     if filename in REAL_FILES:
       return open(filename, mode=mode)