From e20295f5735d324bc20e4ea04fa41183c8a66ec8 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 6 Apr 2018 17:23:50 -0400 Subject: [PATCH] Fix Kubernetes config provider for recent changes in Kub API Kubernetes secret volumes are now mounted as read-only, so we have to write the files *only* via the Kub API Fixes https://jira.coreos.com/browse/QUAY-911 --- conf/nginx/server-base.conf.jnj | 8 ++ util/config/provider/__init__.py | 3 - util/config/provider/basefileprovider.py | 70 ++++++++++++++++++ util/config/provider/baseprovider.py | 30 ++++---- util/config/provider/fileprovider.py | 70 ++---------------- util/config/provider/k8sprovider.py | 73 ++++++++++++------- util/config/provider/test/test_k8sprovider.py | 8 +- 7 files changed, 153 insertions(+), 109 deletions(-) create mode 100644 util/config/provider/basefileprovider.py diff --git a/conf/nginx/server-base.conf.jnj b/conf/nginx/server-base.conf.jnj index d017c02ea..1d1d965e1 100644 --- a/conf/nginx/server-base.conf.jnj +++ b/conf/nginx/server-base.conf.jnj @@ -99,6 +99,14 @@ location ~ ^/cnr { limit_req zone=repositories burst=10; } +location ~ ^/api/suconfig { + proxy_pass http://web_app_server; + + # For suconfig, set our read timeout as super large for both DB migrations + # and awaiting for secrets to be updated. + proxy_read_timeout 2000; +} + location ~ ^/v2 { # If we're being accessed via v1.quay.io, pretend we don't support v2. if ($host = "v1.quay.io") { diff --git a/util/config/provider/__init__.py b/util/config/provider/__init__.py index ce2301912..50a31ed9d 100644 --- a/util/config/provider/__init__.py +++ b/util/config/provider/__init__.py @@ -2,8 +2,6 @@ from util.config.provider.fileprovider import FileConfigProvider from util.config.provider.testprovider import TestConfigProvider from util.config.provider.k8sprovider import KubernetesConfigProvider -import os - def get_config_provider(config_volume, yaml_filename, py_filename, testing=False, kubernetes=False): """ Loads and returns the config provider for the current environment. """ if testing: @@ -13,4 +11,3 @@ def get_config_provider(config_volume, yaml_filename, py_filename, testing=False return KubernetesConfigProvider(config_volume, yaml_filename, py_filename) return FileConfigProvider(config_volume, yaml_filename, py_filename) - diff --git a/util/config/provider/basefileprovider.py b/util/config/provider/basefileprovider.py new file mode 100644 index 000000000..a163d4275 --- /dev/null +++ b/util/config/provider/basefileprovider.py @@ -0,0 +1,70 @@ +import os +import logging + +from util.config.provider.baseprovider import (BaseProvider, import_yaml, export_yaml, + CannotWriteConfigException) + +logger = logging.getLogger(__name__) + +class BaseFileProvider(BaseProvider): + """ Base 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_config(self): + if not self.config_exists(): + return None + + config_obj = {} + import_yaml(config_obj, self.yaml_path) + return config_obj + + def config_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 get_volume_file(self, filename, mode='r'): + return open(os.path.join(self.config_volume, filename), mode=mode) + + def get_volume_path(self, directory, filename): + return os.path.join(directory, filename) + + def list_volume_directory(self, path): + dirpath = os.path.join(self.config_volume, path) + if not os.path.exists(dirpath): + return None + + if not os.path.isdir(dirpath): + return None + + return os.listdir(dirpath) + + def requires_restart(self, app_config): + file_config = self.get_config() + if not file_config: + return False + + for key in file_config: + if app_config.get(key) != file_config[key]: + return True + + return False diff --git a/util/config/provider/baseprovider.py b/util/config/provider/baseprovider.py index 816f69bbc..5a616895f 100644 --- a/util/config/provider/baseprovider.py +++ b/util/config/provider/baseprovider.py @@ -1,6 +1,9 @@ import logging import yaml +from abc import ABCMeta, abstractmethod +from six import add_metaclass + from jsonschema import validate, ValidationError from util.config.schema import CONFIG_SCHEMA @@ -56,6 +59,7 @@ def export_yaml(config_obj, config_file): raise CannotWriteConfigException(str(ioe)) +@add_metaclass(ABCMeta) class BaseProvider(object): """ A configuration provider helps to load, save, and handle config override in the application. """ @@ -64,61 +68,61 @@ class BaseProvider(object): def provider_id(self): raise NotImplementedError + @abstractmethod def update_app_config(self, app_config): """ Updates the given application config object with the loaded override config. """ - raise NotImplementedError + @abstractmethod def get_config(self): """ Returns the contents of the config override file, or None if none. """ - raise NotImplementedError + @abstractmethod def save_config(self, config_object): """ Updates the contents of the config override file to those given. """ - raise NotImplementedError + @abstractmethod def config_exists(self): """ Returns true if a config override file exists in the config volume. """ - raise NotImplementedError + @abstractmethod def volume_exists(self): """ Returns whether the config override volume exists. """ - raise NotImplementedError + @abstractmethod def volume_file_exists(self, filename): """ Returns whether the file with the given name exists under the config override volume. """ - raise NotImplementedError + @abstractmethod def get_volume_file(self, filename, mode='r'): """ Returns a Python file referring to the given name under the config override volume. """ - raise NotImplementedError + @abstractmethod def write_volume_file(self, filename, contents): """ Writes the given contents to the config override volumne, with the given filename. """ - raise NotImplementedError + @abstractmethod def remove_volume_file(self, filename): """ Removes the config override volume file with the given filename. """ - raise NotImplementedError + @abstractmethod def list_volume_directory(self, path): """ Returns a list of strings representing the names of the files found in the config override directory under the given path. If the path doesn't exist, returns None. """ - raise NotImplementedError + @abstractmethod def save_volume_file(self, filename, flask_file): """ Saves the given flask file to the config override volume, with the given filename. """ - raise NotImplementedError + @abstractmethod def requires_restart(self, app_config): """ If true, the configuration loaded into memory for the app does not match that on disk, indicating that this container requires a restart. """ - raise NotImplementedError + @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 """ - raise NotImplementedError diff --git a/util/config/provider/fileprovider.py b/util/config/provider/fileprovider.py index 705e14cf4..ee5de899d 100644 --- a/util/config/provider/fileprovider.py +++ b/util/config/provider/fileprovider.py @@ -1,8 +1,9 @@ import os import logging -from util.config.provider.baseprovider import (BaseProvider, import_yaml, export_yaml, - CannotWriteConfigException) +from util.config.provider.baseprovider import export_yaml, CannotWriteConfigException +from util.config.provider.basefileprovider import BaseFileProvider + logger = logging.getLogger(__name__) @@ -16,52 +17,19 @@ def _ensure_parent_dir(filepath): raise CannotWriteConfigException(str(ioe)) -class FileConfigProvider(BaseProvider): - """ Implementation of the config provider that reads the data from the file system. """ +class FileConfigProvider(BaseFileProvider): + """ Implementation of the config provider that reads and writes the data + from/to 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) + super(FileConfigProvider, self).__init__(config_volume, yaml_filename, py_filename) @property def provider_id(self): return 'file' - 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_config(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_config(self, config_obj): export_yaml(config_obj, self.yaml_path) - def config_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 get_volume_file(self, filename, mode='r'): - return open(os.path.join(self.config_volume, filename), mode=mode) - def write_volume_file(self, filename, contents): filepath = os.path.join(self.config_volume, filename) _ensure_parent_dir(filepath) @@ -78,16 +46,6 @@ class FileConfigProvider(BaseProvider): filepath = os.path.join(self.config_volume, filename) os.remove(filepath) - def list_volume_directory(self, path): - dirpath = os.path.join(self.config_volume, path) - if not os.path.exists(dirpath): - return None - - if not os.path.isdir(dirpath): - return None - - return os.listdir(dirpath) - def save_volume_file(self, filename, flask_file): filepath = os.path.join(self.config_volume, filename) _ensure_parent_dir(filepath) @@ -99,17 +57,3 @@ class FileConfigProvider(BaseProvider): raise CannotWriteConfigException(str(ioe)) return filepath - - def requires_restart(self, app_config): - file_config = self.get_config() - if not file_config: - return False - - for key in file_config: - if app_config.get(key) != file_config[key]: - return True - - return False - - 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 c3965f015..2760c6fad 100644 --- a/util/config/provider/k8sprovider.py +++ b/util/config/provider/k8sprovider.py @@ -2,11 +2,13 @@ import os import logging import json import base64 +import time from requests import Request, Session -from util.config.provider.baseprovider import get_yaml, CannotWriteConfigException -from util.config.provider.fileprovider import FileConfigProvider +from util.config.provider.baseprovider import CannotWriteConfigException, get_yaml +from util.config.provider.basefileprovider import BaseFileProvider + logger = logging.getLogger(__name__) @@ -20,14 +22,12 @@ SERVICE_ACCOUNT_TOKEN_PATH = '/var/run/secrets/kubernetes.io/serviceaccount/toke QE_NAMESPACE = os.environ.get('QE_K8S_NAMESPACE', 'quay-enterprise') QE_CONFIG_SECRET = os.environ.get('QE_K8S_CONFIG_SECRET', 'quay-enterprise-config-secret') -class KubernetesConfigProvider(FileConfigProvider): +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): super(KubernetesConfigProvider, self).__init__(config_volume, yaml_filename, py_filename) - self.yaml_filename = yaml_filename - # Load the service account token from the local store. if not os.path.exists(SERVICE_ACCOUNT_TOKEN_PATH): raise Exception('Cannot load Kubernetes service account token') @@ -35,48 +35,47 @@ class KubernetesConfigProvider(FileConfigProvider): with open(SERVICE_ACCOUNT_TOKEN_PATH, 'r') as f: self._service_token = f.read() - # Make sure the configuration volume exists. - if not self.volume_exists(): - os.makedirs(config_volume) - @property def provider_id(self): return 'k8s' - def save_config(self, config_obj): - self._update_secret_file(self.yaml_filename, get_yaml(config_obj)) - super(KubernetesConfigProvider, self).save_config(config_obj) + def get_volume_path(self, directory, filename): + # NOTE: Overridden to ensure we don't have subdirectories, which aren't supported + # in Kubernetes secrets. + return "_".join([directory.rstrip('/'), filename]) - def write_volume_file(self, filename, contents): - super(KubernetesConfigProvider, self).write_volume_file(filename, contents) - - try: - self._update_secret_file(filename, contents) - except IOError as ioe: - raise CannotWriteConfigException(str(ioe)) - def volume_file_exists(self, filename): + # 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'] - def list_volume_directory(self, path): + # NOTE: Overridden because we don't have subdirectories, which aren't supported + # in Kubernetes secrets. secret = self._lookup_secret() - + if not secret: return [] - + paths = [] for filename in secret.get('data', {}): if filename.startswith(path): paths.append(filename[len(path) + 1:]) return paths - def remove_volume_file(self, filename): - super(KubernetesConfigProvider, self).remove_volume_file(filename) + def save_config(self, config_obj): + self._update_secret_file(self.yaml_filename, get_yaml(config_obj)) + def write_volume_file(self, filename, contents): + try: + self._update_secret_file(filename, contents) + except IOError as ioe: + raise CannotWriteConfigException(str(ioe)) + + def remove_volume_file(self, filename): try: self._update_secret_file(filename, None) except IOError as ioe: @@ -126,6 +125,27 @@ class KubernetesConfigProvider(FileConfigProvider): self._assert_success(self._execute_k8s_api('PUT', secret_url, secret)) + # Wait until the local mounted copy of the secret has been updated, as + # this is an eventual consistency operation, but the caller expects immediate + # consistency. + while True: + matching_files = set() + for secret_filename, encoded_value in secret['data'].iteritems(): + expected_value = base64.b64decode(encoded_value) + try: + with self.get_volume_file(secret_filename) as f: + contents = f.read() + + if contents == expected_value: + matching_files.add(secret_filename) + except IOError: + continue + + if matching_files == set(secret['data'].keys()): + break + + # Sleep for a second and then try again. + time.sleep(1) def _lookup_secret(self): secret_url = 'namespaces/%s/secrets/%s' % (QE_NAMESPACE, QE_CONFIG_SECRET) @@ -148,6 +168,3 @@ 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, filename): - return "_".join([directory.rstrip('/'), filename]) \ No newline at end of file diff --git a/util/config/provider/test/test_k8sprovider.py b/util/config/provider/test/test_k8sprovider.py index d64dcee7b..de3c5a7e2 100644 --- a/util/config/provider/test/test_k8sprovider.py +++ b/util/config/provider/test/test_k8sprovider.py @@ -8,7 +8,12 @@ from test.fixtures import * class TestKubernetesConfigProvider(KubernetesConfigProvider): def __init__(self): + self.config_volume = '' self.yaml_filename = 'yaml_filename' + self.py_filename = None + + self.yaml_path = os.path.join(self.config_volume, self.yaml_filename) + self._service_token = 'service_token' self._execute_k8s_api = Mock() @@ -21,7 +26,6 @@ class TestKubernetesConfigProvider(KubernetesConfigProvider): ]) def test_get_volume_path(directory, filename, expected): provider = TestKubernetesConfigProvider() - assert expected == provider.get_volume_path(directory, filename) @@ -53,4 +57,4 @@ def test_volume_file_exists(response, key, expected): def test_list_volume_directory(response, expected): provider = TestKubernetesConfigProvider() provider._execute_k8s_api.return_value = response - assert expected == provider.list_volume_directory("extra") \ No newline at end of file + assert expected == provider.list_volume_directory("extra")