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

@@ -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:

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()
 

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)
 

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

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()

nodepool/tests/fixtures/secure.conf

@@ -1 +0,0 @@
-# Empty

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

nodepool/tests/fixtures/secure_file_secure.yaml

@@ -0,0 +1,4 @@
+zookeeper-servers:
+  - host: {zookeeper_host}
+    port: {zookeeper_port}
+    chroot: {zookeeper_chroot}

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))
 

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, [])