From 01b59e8d66c5efbd8c2124667d32e1053aa1b16b Mon Sep 17 00:00:00 2001 From: Evan Cordell Date: Wed, 17 May 2017 08:12:09 -0400 Subject: [PATCH 1/3] ConfigProviders abstract over path construction Fixes issue where certs can't be uploaded in UI in k8s --- endpoints/api/superuser.py | 6 +++--- util/config/provider/baseprovider.py | 5 +++++ util/config/provider/fileprovider.py | 3 +++ util/config/provider/k8sprovider.py | 16 ++++++++++++++++ util/config/provider/testprovider.py | 5 +++++ 5 files changed, 32 insertions(+), 3 deletions(-) diff --git a/endpoints/api/superuser.py b/endpoints/api/superuser.py index 356193308..dcecf6ea0 100644 --- a/endpoints/api/superuser.py +++ b/endpoints/api/superuser.py @@ -852,7 +852,7 @@ class SuperUserCustomCertificates(ApiResource): cert_views = [] for extra_cert_path in extra_certs_found: try: - cert_full_path = os.path.join(EXTRA_CA_DIRECTORY, extra_cert_path) + cert_full_path = config_provider.get_volume_path(EXTRA_CA_DIRECTORY, extra_cert_path) with config_provider.get_volume_file(cert_full_path) as f: certificate = load_certificate(f.read()) cert_views.append({ @@ -900,7 +900,7 @@ class SuperUserCustomCertificate(ApiResource): abort(400) logger.debug('Saving custom certificate %s', certpath) - cert_full_path = os.path.join(EXTRA_CA_DIRECTORY, certpath) + cert_full_path = config_provider.get_volume_path(EXTRA_CA_DIRECTORY, certpath) config_provider.save_volume_file(cert_full_path, uploaded_file) logger.debug('Saved custom certificate %s', certpath) @@ -934,7 +934,7 @@ class SuperUserCustomCertificate(ApiResource): @verify_not_prod def delete(self, certpath): if SuperUserPermission().can(): - cert_full_path = os.path.join(EXTRA_CA_DIRECTORY, certpath) + cert_full_path = config_provider.get_volume_path(EXTRA_CA_DIRECTORY, certpath) config_provider.remove_volume_file(cert_full_path) return '', 204 diff --git a/util/config/provider/baseprovider.py b/util/config/provider/baseprovider.py index e5af29fa4..91ac64f74 100644 --- a/util/config/provider/baseprovider.py +++ b/util/config/provider/baseprovider.py @@ -109,6 +109,11 @@ class BaseProvider(object): indicating that this container requires a restart. """ raise NotImplementedError + + def get_volume_path(self, directory, file): + """ Helper for constructing file paths, which may differ between providers. For example, + kubernetes can't have subfolders in configmaps """ + raise NotImplementedError def _get_license_file(self): """ Returns the contents of the license file. """ diff --git a/util/config/provider/fileprovider.py b/util/config/provider/fileprovider.py index fdaf860b0..1cefa9794 100644 --- a/util/config/provider/fileprovider.py +++ b/util/config/provider/fileprovider.py @@ -110,3 +110,6 @@ class FileConfigProvider(BaseProvider): return True return False + + def get_volume_path(self, directory, file): + return os.path.join(directory, file) diff --git a/util/config/provider/k8sprovider.py b/util/config/provider/k8sprovider.py index 0feb6ca60..743892822 100644 --- a/util/config/provider/k8sprovider.py +++ b/util/config/provider/k8sprovider.py @@ -54,6 +54,19 @@ class KubernetesConfigProvider(FileConfigProvider): self._update_secret_file(filename, contents) except IOError as ioe: raise CannotWriteConfigException(str(ioe)) + + def volume_file_exists(self, filename): + secret = self._lookup_secret() + return filename in secret + + + def list_volume_directory(self, path): + secret = self._lookup_secret() + + paths = [] + for filename in secret: + if filename.startswith(path): + paths.append(filename[len(path) + 1:]) def remove_volume_file(self, filename): super(KubernetesConfigProvider, self).remove_volume_file(filename) @@ -130,3 +143,6 @@ class KubernetesConfigProvider(FileConfigProvider): request = Request(method, url, data=data, headers=headers) return session.send(request.prepare(), verify=False, timeout=2) + + def get_volume_path(self, directory, file): + return "_".join([directory, file]) \ No newline at end of file diff --git a/util/config/provider/testprovider.py b/util/config/provider/testprovider.py index bae362bda..d0a81e550 100644 --- a/util/config/provider/testprovider.py +++ b/util/config/provider/testprovider.py @@ -1,5 +1,6 @@ import json import io +import os from datetime import datetime, timedelta from util.config.provider.baseprovider import BaseProvider @@ -88,3 +89,7 @@ class TestConfigProvider(BaseProvider): def reset_for_test(self): self._config['SUPER_USERS'] = ['devtable'] self.files = {} + + def get_volume_path(self, directory, file): + return os.path.join(directory, file) + From 0c059587390a137ab49fab2e1af7919d06f4ac8c Mon Sep 17 00:00:00 2001 From: Evan Cordell Date: Tue, 23 May 2017 13:59:09 -0400 Subject: [PATCH 2/3] Update cert install scripts to read prefixed names --- conf/init/certs_install.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/conf/init/certs_install.sh b/conf/init/certs_install.sh index 9f440d8a6..b9fd46dbe 100755 --- a/conf/init/certs_install.sh +++ b/conf/init/certs_install.sh @@ -23,5 +23,13 @@ if [ -f /conf/stack/extra_ca_certs ]; then cat /conf/stack/extra_ca_certs >> /venv/lib/python2.7/site-packages/requests/cacert.pem fi +## Add extra trusted certificates (prefixed) +for f in $(ls /conf/stack/extra_ca* | grep -v ':$') +do + echo "Installing extra cert $f" + cp "/conf/stack/$f" /usr/local/share/ca-certificates/ + cat "/conf/stack/$f" >> /venv/lib/python2.7/site-packages/requests/cacert.pem +done + # Update all CA certificates. update-ca-certificates From 20da91d879a4643c235e716ecdcfdc5a481258c4 Mon Sep 17 00:00:00 2001 From: Evan Cordell Date: Tue, 23 May 2017 15:43:21 -0400 Subject: [PATCH 3/3] Add tests for providers and update install script --- conf/init/certs_install.sh | 6 ++-- util/config/provider/baseprovider.py | 2 +- util/config/provider/fileprovider.py | 4 +-- util/config/provider/k8sprovider.py | 4 +-- .../config/provider/test/test_fileprovider.py | 29 +++++++++++++++++++ util/config/provider/test/test_k8sprovider.py | 25 ++++++++++++++++ util/config/provider/testprovider.py | 4 +-- 7 files changed, 64 insertions(+), 10 deletions(-) create mode 100644 util/config/provider/test/test_fileprovider.py create mode 100644 util/config/provider/test/test_k8sprovider.py diff --git a/conf/init/certs_install.sh b/conf/init/certs_install.sh index b9fd46dbe..1d680206a 100755 --- a/conf/init/certs_install.sh +++ b/conf/init/certs_install.sh @@ -24,11 +24,11 @@ if [ -f /conf/stack/extra_ca_certs ]; then fi ## Add extra trusted certificates (prefixed) -for f in $(ls /conf/stack/extra_ca* | grep -v ':$') +for f in $(ls -p /conf/stack/extra_ca* | grep -v ':$' | grep -v '/$') do echo "Installing extra cert $f" - cp "/conf/stack/$f" /usr/local/share/ca-certificates/ - cat "/conf/stack/$f" >> /venv/lib/python2.7/site-packages/requests/cacert.pem + cp "$f" /usr/local/share/ca-certificates/ + cat "$f" >> /venv/lib/python2.7/site-packages/requests/cacert.pem done # Update all CA certificates. diff --git a/util/config/provider/baseprovider.py b/util/config/provider/baseprovider.py index 91ac64f74..663c64588 100644 --- a/util/config/provider/baseprovider.py +++ b/util/config/provider/baseprovider.py @@ -110,7 +110,7 @@ class BaseProvider(object): """ raise NotImplementedError - def get_volume_path(self, directory, file): + 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 """ raise NotImplementedError diff --git a/util/config/provider/fileprovider.py b/util/config/provider/fileprovider.py index 1cefa9794..705e14cf4 100644 --- a/util/config/provider/fileprovider.py +++ b/util/config/provider/fileprovider.py @@ -111,5 +111,5 @@ class FileConfigProvider(BaseProvider): return False - def get_volume_path(self, directory, file): - return os.path.join(directory, file) + def get_volume_path(self, directory, filename): + return os.path.join(directory, filename) diff --git a/util/config/provider/k8sprovider.py b/util/config/provider/k8sprovider.py index 743892822..7af71634d 100644 --- a/util/config/provider/k8sprovider.py +++ b/util/config/provider/k8sprovider.py @@ -144,5 +144,5 @@ class KubernetesConfigProvider(FileConfigProvider): request = Request(method, url, data=data, headers=headers) return session.send(request.prepare(), verify=False, timeout=2) - def get_volume_path(self, directory, file): - return "_".join([directory, file]) \ No newline at end of file + def get_volume_path(self, directory, filename): + return "_".join([directory.rstrip('/'), filename]) \ No newline at end of file diff --git a/util/config/provider/test/test_fileprovider.py b/util/config/provider/test/test_fileprovider.py new file mode 100644 index 000000000..7a1336085 --- /dev/null +++ b/util/config/provider/test/test_fileprovider.py @@ -0,0 +1,29 @@ +import pytest + +from util.config.provider import FileConfigProvider + +from test.fixtures import * + + +class TestFileConfigProvider(FileConfigProvider): + def __init__(self): + self.yaml_filename = 'yaml_filename' + self._service_token = 'service_token' + self.config_volume = 'config_volume' + self.py_filename = 'py_filename' + self.yaml_path = os.path.join(self.config_volume, self.yaml_filename) + self.py_path = os.path.join(self.config_volume, self.py_filename) + + +@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"), +]) +def test_get_volume_path(directory, filename, expected): + provider = TestFileConfigProvider() + + assert expected == provider.get_volume_path(directory, filename) + + diff --git a/util/config/provider/test/test_k8sprovider.py b/util/config/provider/test/test_k8sprovider.py new file mode 100644 index 000000000..c31f69596 --- /dev/null +++ b/util/config/provider/test/test_k8sprovider.py @@ -0,0 +1,25 @@ +import pytest + +from util.config.provider import KubernetesConfigProvider + +from test.fixtures import * + + +class TestKubernetesConfigProvider(KubernetesConfigProvider): + def __init__(self): + self.yaml_filename = 'yaml_filename' + self._service_token = 'service_token' + + +@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"), +]) +def test_get_volume_path(directory, filename, expected): + provider = TestKubernetesConfigProvider() + + assert expected == provider.get_volume_path(directory, filename) + + diff --git a/util/config/provider/testprovider.py b/util/config/provider/testprovider.py index d0a81e550..0074e1e06 100644 --- a/util/config/provider/testprovider.py +++ b/util/config/provider/testprovider.py @@ -90,6 +90,6 @@ class TestConfigProvider(BaseProvider): self._config['SUPER_USERS'] = ['devtable'] self.files = {} - def get_volume_path(self, directory, file): - return os.path.join(directory, file) + def get_volume_path(self, directory, filename): + return os.path.join(directory, filename)