From 2ae69dc6519bd42c388ae61d45b312f88b69b75c Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 10 May 2018 16:44:18 +0300 Subject: [PATCH] Further fixes to the Kubernetes config provider, and a new set of proper unit tests --- endpoints/api/suconfig.py | 2 +- endpoints/api/superuser.py | 2 +- util/config/provider/basefileprovider.py | 12 +- util/config/provider/baseprovider.py | 25 ++- util/config/provider/fileprovider.py | 20 +-- util/config/provider/k8sprovider.py | 52 ++++-- util/config/provider/test/test_k8sprovider.py | 170 +++++++++++++----- util/config/provider/testprovider.py | 5 +- 8 files changed, 181 insertions(+), 107 deletions(-) diff --git a/endpoints/api/suconfig.py b/endpoints/api/suconfig.py index 316c14169..836f09f80 100644 --- a/endpoints/api/suconfig.py +++ b/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 } diff --git a/endpoints/api/superuser.py b/endpoints/api/superuser.py index dd96e0849..9fdee1121 100644 --- a/endpoints/api/superuser.py +++ b/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. diff --git a/util/config/provider/basefileprovider.py b/util/config/provider/basefileprovider.py index a163d4275..7d0572351 100644 --- a/util/config/provider/basefileprovider.py +++ b/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) diff --git a/util/config/provider/baseprovider.py b/util/config/provider/baseprovider.py index 5a616895f..052b32e76 100644 --- a/util/config/provider/baseprovider.py +++ b/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 """ diff --git a/util/config/provider/fileprovider.py b/util/config/provider/fileprovider.py index ee5de899d..feae57b80 100644 --- a/util/config/provider/fileprovider.py +++ b/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. diff --git a/util/config/provider/k8sprovider.py b/util/config/provider/k8sprovider.py index 2760c6fad..835a85b4d 100644 --- a/util/config/provider/k8sprovider.py +++ b/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) diff --git a/util/config/provider/test/test_k8sprovider.py b/util/config/provider/test/test_k8sprovider.py index de3c5a7e2..b0e9dedda 100644 --- a/util/config/provider/test/test_k8sprovider.py +++ b/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') diff --git a/util/config/provider/testprovider.py b/util/config/provider/testprovider.py index 3e61c7307..55158444b 100644 --- a/util/config/provider/testprovider.py +++ b/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)