From 477a40044bad9838ce0a17c9c3fe939da1936f96 Mon Sep 17 00:00:00 2001 From: David Shrewsbury Date: Mon, 8 Jan 2018 13:53:17 -0500 Subject: [PATCH] Add support for a secure ZooKeeper configuration The secure config file has largely been unused and ignored for v3. This add support for reading ZooKeeper credentials from the secure file. Note that actually specifying authentication credentials is left for future work, but this adds the framework necessary for that. ZooKeeper creds can be in both the normal config file and the secure file. If specified in both, the data in the secure configuration wins. Change-Id: I5d9c12c00f5e85ef258128337cdb99809f86b8ed --- doc/source/installation.rst | 9 ++-- nodepool/builder.py | 41 +++++++++++----- nodepool/cmd/builder.py | 12 +++-- nodepool/config.py | 29 ++++++++---- nodepool/tests/__init__.py | 29 +++++++----- nodepool/tests/fixtures/secure.conf | 1 - .../tests/fixtures/secure_file_config.yaml | 47 +++++++++++++++++++ .../tests/fixtures/secure_file_secure.yaml | 4 ++ nodepool/tests/test_commands.py | 2 +- nodepool/tests/test_launcher.py | 29 ++++++++++++ 10 files changed, 163 insertions(+), 40 deletions(-) delete mode 100644 nodepool/tests/fixtures/secure.conf create mode 100644 nodepool/tests/fixtures/secure_file_config.yaml create mode 100644 nodepool/tests/fixtures/secure_file_secure.yaml diff --git a/doc/source/installation.rst b/doc/source/installation.rst index aa33d271f..f258e1c70 100644 --- a/doc/source/installation.rst +++ b/doc/source/installation.rst @@ -56,9 +56,12 @@ Nodepool has one required configuration file, which defaults to ``/etc/nodepool/nodepool.yaml``. This can be changed with the ``-c`` option. The Nodepool configuration file is described in :ref:`configuration`. -Although there is support for a secure file that is used to store nodepool -configurations that contain sensitive data, this is currently not used, but -may be in the future. +There is support for a secure file that is used to store nodepool +configurations that contain sensitive data. It currently only supports +specifying ZooKeeper credentials. If ZooKeeper credentials are defined in +both configuration files, the data in the secure file takes precedence. +The secure file location can be changed with the ``-s`` option and follows +the same file format as the Nodepool configuration file. There is an optional logging configuration file, specified with the ``-l`` option. The logging configuration file can accept either: diff --git a/nodepool/builder.py b/nodepool/builder.py index 60f7dd59b..744b4bc08 100644 --- a/nodepool/builder.py +++ b/nodepool/builder.py @@ -108,13 +108,14 @@ class DibImageFile(object): class BaseWorker(threading.Thread): - def __init__(self, builder_id, config_path, interval, zk): + def __init__(self, builder_id, config_path, secure_path, interval, zk): super(BaseWorker, self).__init__() self.log = logging.getLogger("nodepool.builder.BaseWorker") self.daemon = True self._running = False self._config = None self._config_path = config_path + self._secure_path = secure_path self._zk = zk self._hostname = socket.gethostname() self._statsd = stats.get_client() @@ -146,9 +147,10 @@ class CleanupWorker(BaseWorker): and any local DIB builds. ''' - def __init__(self, name, builder_id, config_path, interval, zk): + def __init__(self, name, builder_id, config_path, secure_path, + interval, zk): super(CleanupWorker, self).__init__(builder_id, config_path, - interval, zk) + secure_path, interval, zk) self.log = logging.getLogger("nodepool.builder.CleanupWorker.%s" % name) self.name = 'CleanupWorker.%s' % name @@ -507,6 +509,8 @@ class CleanupWorker(BaseWorker): Body of run method for exception handling purposes. ''' new_config = nodepool_config.loadConfig(self._config_path) + if self._secure_path: + nodepool_config.loadSecureConfig(new_config, self._secure_path) if not self._config: self._config = new_config @@ -519,8 +523,9 @@ class CleanupWorker(BaseWorker): class BuildWorker(BaseWorker): - def __init__(self, name, builder_id, config_path, interval, zk, dib_cmd): - super(BuildWorker, self).__init__(builder_id, config_path, + def __init__(self, name, builder_id, config_path, secure_path, + interval, zk, dib_cmd): + super(BuildWorker, self).__init__(builder_id, config_path, secure_path, interval, zk) self.log = logging.getLogger("nodepool.builder.BuildWorker.%s" % name) self.name = 'BuildWorker.%s' % name @@ -781,6 +786,8 @@ class BuildWorker(BaseWorker): ''' # NOTE: For the first iteration, we expect self._config to be None new_config = nodepool_config.loadConfig(self._config_path) + if self._secure_path: + nodepool_config.loadSecureConfig(new_config, self._secure_path) if not self._config: self._config = new_config @@ -792,9 +799,10 @@ class BuildWorker(BaseWorker): class UploadWorker(BaseWorker): - def __init__(self, name, builder_id, config_path, interval, zk): + def __init__(self, name, builder_id, config_path, secure_path, + interval, zk): super(UploadWorker, self).__init__(builder_id, config_path, - interval, zk) + secure_path, interval, zk) self.log = logging.getLogger("nodepool.builder.UploadWorker.%s" % name) self.name = 'UploadWorker.%s' % name @@ -803,6 +811,8 @@ class UploadWorker(BaseWorker): Reload the nodepool configuration file. ''' new_config = nodepool_config.loadConfig(self._config_path) + if self._secure_path: + nodepool_config.loadSecureConfig(new_config, self._secure_path) if not self._config: self._config = new_config @@ -1039,17 +1049,19 @@ class NodePoolBuilder(object): ''' log = logging.getLogger("nodepool.builder.NodePoolBuilder") - def __init__(self, config_path, num_builders=1, num_uploaders=4, - fake=False): + def __init__(self, config_path, secure_path=None, + num_builders=1, num_uploaders=4, fake=False): ''' Initialize the NodePoolBuilder object. :param str config_path: Path to configuration file. + :param str secure_path: Path to secure configuration file. :param int num_builders: Number of build workers to start. :param int num_uploaders: Number of upload workers to start. :param bool fake: Whether to fake the image builds. ''' self._config_path = config_path + self._secure_path = secure_path self._config = None self._num_builders = num_builders self._build_workers = [] @@ -1090,6 +1102,8 @@ class NodePoolBuilder(object): def _getAndValidateConfig(self): config = nodepool_config.loadConfig(self._config_path) + if self._secure_path: + nodepool_config.loadSecureConfig(config, self._secure_path) if not config.zookeeper_servers.values(): raise RuntimeError('No ZooKeeper servers specified in config.') if not config.imagesdir: @@ -1127,20 +1141,23 @@ class NodePoolBuilder(object): # Create build and upload worker objects for i in range(self._num_builders): - w = BuildWorker(i, builder_id, self._config_path, + w = BuildWorker(i, builder_id, + self._config_path, self._secure_path, self.build_interval, self.zk, self.dib_cmd) w.start() self._build_workers.append(w) for i in range(self._num_uploaders): - w = UploadWorker(i, builder_id, self._config_path, + w = UploadWorker(i, builder_id, + self._config_path, self._secure_path, self.upload_interval, self.zk) w.start() self._upload_workers.append(w) if self.cleanup_interval > 0: self._janitor = CleanupWorker( - 0, builder_id, self._config_path, + 0, builder_id, + self._config_path, self._secure_path, self.cleanup_interval, self.zk) self._janitor.start() diff --git a/nodepool/cmd/builder.py b/nodepool/cmd/builder.py index 1138cba5f..7ac993b2a 100644 --- a/nodepool/cmd/builder.py +++ b/nodepool/cmd/builder.py @@ -34,6 +34,8 @@ class NodePoolBuilderApp(nodepool.cmd.NodepoolDaemonApp): parser.add_argument('-c', dest='config', default='/etc/nodepool/nodepool.yaml', help='path to config file') + parser.add_argument('-s', dest='secure', + help='path to secure config file') parser.add_argument('--build-workers', dest='build_workers', default=1, help='number of build workers', type=int) @@ -46,10 +48,12 @@ class NodePoolBuilderApp(nodepool.cmd.NodepoolDaemonApp): return parser def run(self): - self.nb = builder.NodePoolBuilder(self.args.config, - self.args.build_workers, - self.args.upload_workers, - self.args.fake) + self.nb = builder.NodePoolBuilder( + self.args.config, + secure_path=self.args.secure, + num_builders=self.args.build_workers, + num_uploaders=self.args.upload_workers, + fake=self.args.fake) signal.signal(signal.SIGINT, self.sigint_handler) diff --git a/nodepool/config.py b/nodepool/config.py index 537dfbc23..d9a710d6d 100755 --- a/nodepool/config.py +++ b/nodepool/config.py @@ -16,7 +16,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -from six.moves import configparser as ConfigParser import time import yaml @@ -66,7 +65,7 @@ def get_provider_config(provider): return OpenStackProviderConfig(provider) -def loadConfig(config_path): +def openConfig(path): retry = 3 # Since some nodepool code attempts to dynamically re-read its config @@ -75,7 +74,7 @@ def loadConfig(config_path): # attempt to reload it. while True: try: - config = yaml.load(open(config_path)) + config = yaml.load(open(path)) break except IOError as e: if e.errno == 2: @@ -85,6 +84,11 @@ def loadConfig(config_path): raise e if retry == 0: raise e + return config + + +def loadConfig(config_path): + config = openConfig(config_path) # Reset the shared os_client_config instance OpenStackProviderConfig.os_client_config = None @@ -126,8 +130,6 @@ def loadConfig(config_path): d.rebuild_age = int(diskimage.get('rebuild-age', 86400)) d.env_vars = diskimage.get('env-vars', {}) if not isinstance(d.env_vars, dict): - #self.log.error("%s: ignoring env-vars; " - # "should be a dict" % d.name) d.env_vars = {} d.image_types = set(diskimage.get('formats', [])) d.pause = bool(diskimage.get('pause', False)) @@ -149,7 +151,18 @@ def loadConfig(config_path): def loadSecureConfig(config, secure_config_path): - secure = ConfigParser.ConfigParser() - secure.readfp(open(secure_config_path)) + secure = openConfig(secure_config_path) + if not secure: # empty file + return - #config.dburi = secure.get('database', 'dburi') + # Eliminate any servers defined in the normal config + if secure.get('zookeeper-servers', []): + config.zookeeper_servers = {} + + # TODO(Shrews): Support ZooKeeper auth + for server in secure.get('zookeeper-servers', []): + z = zk.ZooKeeperConnectionConfig(server['host'], + server.get('port', 2181), + server.get('chroot', None)) + name = z.host + '_' + str(z.port) + config.zookeeper_servers[name] = z diff --git a/nodepool/tests/__init__.py b/nodepool/tests/__init__.py index 2d299d3e9..77aad1293 100644 --- a/nodepool/tests/__init__.py +++ b/nodepool/tests/__init__.py @@ -254,15 +254,17 @@ class BaseTestCase(testtools.TestCase): class BuilderFixture(fixtures.Fixture): - def __init__(self, configfile, cleanup_interval): + def __init__(self, configfile, cleanup_interval, securefile=None): super(BuilderFixture, self).__init__() self.configfile = configfile + self.securefile = securefile self.cleanup_interval = cleanup_interval self.builder = None def setUp(self): super(BuilderFixture, self).setUp() - self.builder = builder.NodePoolBuilder(self.configfile) + self.builder = builder.NodePoolBuilder( + self.configfile, secure_path=self.securefile) self.builder.cleanup_interval = self.cleanup_interval self.builder.build_interval = .1 self.builder.upload_interval = .1 @@ -278,7 +280,6 @@ class DBTestCase(BaseTestCase): def setUp(self): super(DBTestCase, self).setUp() self.log = logging.getLogger("tests") - self.secure_conf = self._setup_secure() self.setupZK() def setup_config(self, filename, images_dir=None): @@ -306,15 +307,18 @@ class DBTestCase(BaseTestCase): new_configfile = self.setup_config(filename, self._config_images_dir) os.rename(new_configfile, configfile) - def _setup_secure(self): + def setup_secure(self, filename): # replace entries in secure.conf configfile = os.path.join(os.path.dirname(__file__), - 'fixtures', 'secure.conf') + 'fixtures', filename) (fd, path) = tempfile.mkstemp() with open(configfile, 'rb') as conf_fd: - config = conf_fd.read() - os.write(fd, config) - #os.write(fd, config.format(dburi=self.dburi)) + config = conf_fd.read().decode('utf8') + data = config.format( + zookeeper_host=self.zookeeper_host, + zookeeper_port=self.zookeeper_port, + zookeeper_chroot=self.zookeeper_chroot) + os.write(fd, data.encode('utf8')) os.close(fd) return path @@ -458,7 +462,8 @@ class DBTestCase(BaseTestCase): return req def useNodepool(self, *args, **kwargs): - args = (self.secure_conf,) + args + secure_conf = kwargs.pop('secure_conf', None) + args = (secure_conf,) + args pool = launcher.NodePool(*args, **kwargs) pool.cleanup_interval = .5 pool.delete_interval = .5 @@ -470,8 +475,10 @@ class DBTestCase(BaseTestCase): self.addCleanup(app.stop) return app - def _useBuilder(self, configfile, cleanup_interval=.5): - self.useFixture(BuilderFixture(configfile, cleanup_interval)) + def _useBuilder(self, configfile, securefile=None, cleanup_interval=.5): + self.useFixture( + BuilderFixture(configfile, cleanup_interval, securefile) + ) def setupZK(self): f = ZookeeperServerFixture() diff --git a/nodepool/tests/fixtures/secure.conf b/nodepool/tests/fixtures/secure.conf deleted file mode 100644 index b7db25411..000000000 --- a/nodepool/tests/fixtures/secure.conf +++ /dev/null @@ -1 +0,0 @@ -# Empty diff --git a/nodepool/tests/fixtures/secure_file_config.yaml b/nodepool/tests/fixtures/secure_file_config.yaml new file mode 100644 index 000000000..094284a32 --- /dev/null +++ b/nodepool/tests/fixtures/secure_file_config.yaml @@ -0,0 +1,47 @@ +elements-dir: . +images-dir: '{images_dir}' + +zookeeper-servers: + - host: invalid_host + port: 1 + chroot: invalid_chroot + +labels: + - name: fake-label + min-ready: 1 + +providers: + - name: fake-provider + cloud: fake + driver: fake + region-name: fake-region + rate: 0.0001 + diskimages: + - name: fake-image + meta: + key: value + key2: value + pools: + - name: main + max-servers: 96 + availability-zones: + - az1 + networks: + - net-name + labels: + - name: fake-label + diskimage: fake-image + min-ram: 8192 + flavor-name: 'Fake' + +diskimages: + - name: fake-image + elements: + - fedora + - vm + release: 21 + env-vars: + TMPDIR: /opt/dib_tmp + DIB_IMAGE_CACHE: /opt/dib_cache + DIB_CLOUD_IMAGES: http://download.fedoraproject.org/pub/fedora/linux/releases/test/21-Beta/Cloud/Images/x86_64/ + BASE_IMAGE_FILE: Fedora-Cloud-Base-20141029-21_Beta.x86_64.qcow2 diff --git a/nodepool/tests/fixtures/secure_file_secure.yaml b/nodepool/tests/fixtures/secure_file_secure.yaml new file mode 100644 index 000000000..03a3444ca --- /dev/null +++ b/nodepool/tests/fixtures/secure_file_secure.yaml @@ -0,0 +1,4 @@ +zookeeper-servers: + - host: {zookeeper_host} + port: {zookeeper_port} + chroot: {zookeeper_chroot} diff --git a/nodepool/tests/test_commands.py b/nodepool/tests/test_commands.py index c9ee8993b..fe7829bed 100644 --- a/nodepool/tests/test_commands.py +++ b/nodepool/tests/test_commands.py @@ -31,7 +31,7 @@ class TestNodepoolCMD(tests.DBTestCase): super(TestNodepoolCMD, self).setUp() def patch_argv(self, *args): - argv = ["nodepool", "-s", self.secure_conf] + argv = ["nodepool"] argv.extend(args) self.useFixture(fixtures.MonkeyPatch('sys.argv', argv)) diff --git a/nodepool/tests/test_launcher.py b/nodepool/tests/test_launcher.py index 9b2fda2bc..975f10fcb 100644 --- a/nodepool/tests/test_launcher.py +++ b/nodepool/tests/test_launcher.py @@ -862,3 +862,32 @@ class TestLauncher(tests.DBTestCase): self.assertEqual('fake', label3_nodes[0].public_ipv4) self.assertEqual('', label3_nodes[0].public_ipv6) self.assertEqual('fake', label3_nodes[0].interface_ip) + + def test_secure_file(self): + """Test using secure.conf file""" + configfile = self.setup_config('secure_file_config.yaml') + securefile = self.setup_secure('secure_file_secure.yaml') + pool = self.useNodepool( + configfile, + secure_conf=securefile, + watermark_sleep=1) + self._useBuilder(configfile, securefile=securefile) + pool.start() + self.wait_for_config(pool) + + zk_servers = pool.config.zookeeper_servers + self.assertEqual(1, len(zk_servers)) + key = list(zk_servers.keys())[0] + self.assertEqual(self.zookeeper_host, zk_servers[key].host) + self.assertEqual(self.zookeeper_port, zk_servers[key].port) + self.assertEqual(self.zookeeper_chroot, zk_servers[key].chroot) + + image = self.waitForImage('fake-provider', 'fake-image') + self.assertEqual(image.username, 'zuul') + nodes = self.waitForNodes('fake-label') + + self.assertEqual(len(nodes), 1) + self.assertEqual(nodes[0].provider, 'fake-provider') + self.assertEqual(nodes[0].type, 'fake-label') + self.assertEqual(nodes[0].username, 'zuul') + self.assertNotEqual(nodes[0].host_keys, [])