From 00da1982ce15af3068ac66346e8e6b97704f289d Mon Sep 17 00:00:00 2001
From: Ian Wienand <iwienand@redhat.com>
Date: Wed, 17 May 2017 15:54:02 +1000
Subject: [PATCH] Add a more generic tree->graph parser

This moves to a more generic config parser that doesn't have plugins
parsing part of the tree.

I understand why it ended up that way; we have "partitions" key which
has special semantics compared to others keys and there was a desire
to keep it isolated from core tree->graph code.  But this isn't really
isolated; you have to reverse-engineer several module-crossing
boundaries, extras classes and repetitive recursive functions.

Ultimately, plugins should have access to the node graph, but not
participate in configuration parsing.  This way we ensure that plugins
can't invent new methods of configuration parsing.

Note: unit tests produce the same tree -> graph conversion as the old
method.  i.e. this is not intended to have a functional change.

Change-Id: I8a5d62a076a5a50597f2f1df3a8615afba6dadb2
---
 diskimage_builder/block_device/blockdevice.py |  30 +--
 diskimage_builder/block_device/config.py      | 173 ++++++++++++++++++
 .../block_device/level0/localloop.py          |   3 -
 .../block_device/level1/__init__.py           |   4 +-
 .../block_device/level1/partition.py          |  29 ---
 .../block_device/level1/partitioning.py       |  28 ---
 diskimage_builder/block_device/level2/mkfs.py |   2 -
 .../block_device/level3/mount.py              |   2 -
 .../block_device/level4/fstab.py              |   2 -
 .../block_device/tests/config/deep_graph.yaml |   1 -
 .../config/multiple_partitions_graph.yaml     |   1 -
 .../block_device/tests/test_config.py         |  36 ++--
 diskimage_builder/block_device/tree_config.py |  60 ------
 doc/source/user_guide/building_an_image.rst   |   2 +-
 14 files changed, 193 insertions(+), 180 deletions(-)
 create mode 100644 diskimage_builder/block_device/config.py
 delete mode 100644 diskimage_builder/block_device/tree_config.py

diff --git a/diskimage_builder/block_device/blockdevice.py b/diskimage_builder/block_device/blockdevice.py
index 0cf44bf40..38da314cb 100644
--- a/diskimage_builder/block_device/blockdevice.py
+++ b/diskimage_builder/block_device/blockdevice.py
@@ -22,6 +22,8 @@ import yaml
 
 from stevedore import extension
 
+from diskimage_builder.block_device.config import \
+    config_tree_to_graph
 from diskimage_builder.block_device.exception import \
     BlockDeviceSetupException
 from diskimage_builder.block_device.utils import exec_sudo
@@ -119,30 +121,6 @@ class BlockDevice(object):
                         else:
                             v['label'] = "cloudimg-rootfs"
 
-    @staticmethod
-    def _config_tree_to_digraph(tconfig, plugin_manager):
-        """Converts a possible tree-like config into a complete digraph"""
-        dconfig = []
-        for config_entry in tconfig:
-            if len(config_entry) != 1:
-                logger.error("Invalid config entry: more than one key "
-                             "on top level [%s]" % config_entry)
-                raise BlockDeviceSetupException(
-                    "Top level config must contain exactly one key per entry")
-            logger.debug("Config entry [%s]" % config_entry)
-            config_key = list(config_entry)[0]
-            config_value = config_entry[config_key]
-            name = config_value['name'] \
-                   if 'name' in config_value else None
-            if config_key not in plugin_manager:
-                dconfig.append(config_entry)
-            else:
-                plugin_manager[config_key].plugin \
-                    .tree_config.config_tree_to_digraph(
-                        config_key, config_value, dconfig, name,
-                        plugin_manager)
-        return dconfig
-
     @staticmethod
     def _load_json(file_name):
         if os.path.exists(file_name):
@@ -211,6 +189,7 @@ class BlockDevice(object):
                 return 1
             cfg_obj = self.plugin_manager[cfg_obj_name].plugin(
                 cfg_obj_val, default_config)
+
             # At this point it is only possible to add the nodes:
             # adding the edges needs all nodes first.
             cfg_obj.insert_nodes(dg)
@@ -238,8 +217,7 @@ class BlockDevice(object):
         with open(self.params['config'], "rt") as config_fd:
             self.config = yaml.safe_load(config_fd)
         logger.debug("Config before merge [%s]" % self.config)
-        self.config = self._config_tree_to_digraph(self.config,
-                                                   self.plugin_manager)
+        self.config = config_tree_to_graph(self.config)
         logger.debug("Config before merge [%s]" % self.config)
         self._merge_into_config()
         logger.debug("Final config [%s]" % self.config)
diff --git a/diskimage_builder/block_device/config.py b/diskimage_builder/block_device/config.py
new file mode 100644
index 000000000..1fef2a0a4
--- /dev/null
+++ b/diskimage_builder/block_device/config.py
@@ -0,0 +1,173 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import logging
+
+from stevedore import extension
+
+from diskimage_builder.block_device.exception import \
+    BlockDeviceSetupException
+
+
+logger = logging.getLogger(__name__)
+
+_extensions = extension.ExtensionManager(
+    namespace='diskimage_builder.block_device.plugin',
+    invoke_on_load=False)
+
+
+# check if a given name is registered as a plugin
+def is_a_plugin(name):
+    return any(
+        _extensions.map(lambda x: x.name == name))
+
+
+def recurse_config(config, parent_base=None):
+    """Convert a config "tree" to it's canonical name/base graph version
+
+    This is a recursive function to convert a YAML layout "tree"
+    config into a "flat" graph-based config.
+
+    Arguments:
+    :param config: the incoming config dictionary
+    :param parent_base: the name of the parent node, if any
+    :return: a list of expanded, graph-based config items
+
+    """
+    output = []
+    this = {}
+
+    # We should only have one key, with multiple values, being the
+    # config entries.  e.g. (this was checked by config_tree_to_graph)
+    #  mkfs:
+    #    type: ext4
+    #    label: 1234
+    assert len(config.items()) == 1
+    for k, v in config.items():
+        key = k
+        values = v
+
+    # If we don't have a base, we take the parent base; first element
+    # can have no base, however.
+    if 'base' not in values:
+        if parent_base is not None:
+            this['base'] = parent_base
+    else:
+        this['base'] = values['base']
+
+    # If we don't have a name, it is made up as "key_base"
+    if 'name' not in values:
+        this['name'] = "%s_%s" % (key, this['base'])
+    else:
+        this['name'] = values['name']
+
+    # Go through the the values dictionary.  Either this is a "plugin"
+    # key that needs to be recursed, or it is a value that is part of
+    # this config entry.
+    for nk, nv in values.items():
+        if nk == "partitions":
+            # "partitions" is a special key of the "partitioning"
+            # object.  It is a list.  Each list-entry gets treated
+            # as a top-level entry, so we need to recurse it's
+            # keys.  But instead of becoming its own entry in the
+            # graph, it gets attached to the .partitions attribute
+            # of the parent. (see end for example)
+            this['partitions'] = []
+            for partition in nv:
+                new_part = {}
+                for pk, pv in partition.items():
+                    if is_a_plugin(pk):
+                        output.extend(
+                            recurse_config({pk: pv}, partition['name']))
+                    else:
+                        new_part[pk] = pv
+                new_part['base'] = this['base']
+                this['partitions'].append(new_part)
+        elif is_a_plugin(nk):
+            # is this key a plugin directive?  If so, we recurse
+            # into it.
+            output.extend(recurse_config({nk: nv}, this['name']))
+        else:
+            # A value entry; just save as part of this entry
+            this[nk] = nv
+
+    output.append({k: this})
+    return output
+
+
+def config_tree_to_graph(config):
+    """Turn a YAML config into a graph config
+
+    Our YAML config is a list of entries.  Each
+
+    Arguments:
+    :parm config: YAML config; either graph or tree
+    :return: graph-based result
+
+    """
+    output = []
+
+    for entry in config:
+        # Top-level entries should be a dictionary and have a plugin
+        # registered for it
+        if not isinstance(entry, dict):
+            raise BlockDeviceSetupException(
+                "Config entry not a dict: %s" % entry)
+
+        keys = list(entry.keys())
+
+        if len(keys) != 1:
+            raise BlockDeviceSetupException(
+                "Config entry top-level should be a single dict: %s" % entry)
+
+        if not is_a_plugin(keys[0]):
+            raise BlockDeviceSetupException(
+                "Config entry is not a plugin value: %s" % entry)
+
+        output.extend(recurse_config(entry))
+
+    return output
+
+
+#
+# On partitioning: objects
+#
+# To be concrete --
+#
+# partitioning:
+#   base: loop0
+#   name: mbr
+#   partitions:
+#     - name: partition1
+#       foo: bar
+#       mkfs:
+#         type: xfs
+#         mount:
+#           mount_point: /
+#
+# gets turned into the following graph:
+#
+# partitioning:
+#   partitions:
+#     - name: partition1
+#       base: image0
+#       foo: bar
+#
+# mkfs:
+#   base: partition1
+#   name: mkfs_partition1
+#   type: xfs
+#
+# mount:
+#   base: mkfs_partition1
+#   name: mount_mkfs_partition1
+#   mount_point: /
diff --git a/diskimage_builder/block_device/level0/localloop.py b/diskimage_builder/block_device/level0/localloop.py
index a776ecaea..aa33531dd 100644
--- a/diskimage_builder/block_device/level0/localloop.py
+++ b/diskimage_builder/block_device/level0/localloop.py
@@ -18,7 +18,6 @@ import subprocess
 
 from diskimage_builder.block_device.exception import \
     BlockDeviceSetupException
-from diskimage_builder.block_device.tree_config import TreeConfig
 from diskimage_builder.block_device.utils import parse_abs_size_spec
 from diskimage_builder.graph.digraph import Digraph
 
@@ -32,8 +31,6 @@ class LocalLoop(Digraph.Node):
     This class handles local loop devices that can be used
     for VM image installation.
     """
-    tree_config = TreeConfig("local_loop")
-
     def __init__(self, config, default_config):
         logger.debug("Creating LocalLoop object; config [%s] "
                      "default_config [%s]" % (config, default_config))
diff --git a/diskimage_builder/block_device/level1/__init__.py b/diskimage_builder/block_device/level1/__init__.py
index 145b0b45b..0e31c834b 100644
--- a/diskimage_builder/block_device/level1/__init__.py
+++ b/diskimage_builder/block_device/level1/__init__.py
@@ -14,7 +14,5 @@
 # under the License.
 
 from diskimage_builder.block_device.level1.partitioning import Partitioning
-from diskimage_builder.block_device.level1.partitioning \
-    import PartitioningTreeConfig
 
-__all__ = [Partitioning, PartitioningTreeConfig]
+__all__ = [Partitioning]
diff --git a/diskimage_builder/block_device/level1/partition.py b/diskimage_builder/block_device/level1/partition.py
index a6954b717..7a61176c8 100644
--- a/diskimage_builder/block_device/level1/partition.py
+++ b/diskimage_builder/block_device/level1/partition.py
@@ -14,44 +14,15 @@ import logging
 
 from diskimage_builder.block_device.exception import \
     BlockDeviceSetupException
-from diskimage_builder.block_device.tree_config import TreeConfig
 from diskimage_builder.graph.digraph import Digraph
 
 
 logger = logging.getLogger(__name__)
 
 
-class PartitionTreeConfig(object):
-
-    @staticmethod
-    def config_tree_to_digraph(config_key, config_value, pconfig, dconfig,
-                               base_name, plugin_manager):
-        logger.debug("called [%s] [%s] [%s]"
-                     % (config_key, config_value, base_name))
-        assert config_key == Partition.type_string
-
-        for partition in config_value:
-            name = partition['name']
-            nconfig = {
-                'name': name,
-                'base': base_name
-            }
-            for k, v in partition.items():
-                if k not in plugin_manager:
-                    nconfig[k] = v
-                else:
-                    plugin_manager[k].plugin \
-                                     .tree_config.config_tree_to_digraph(
-                                         k, v, dconfig, name, plugin_manager)
-            pconfig.append(nconfig)
-
-        logger.debug("finished [%s] [%s]" % (nconfig, dconfig))
-
-
 class Partition(Digraph.Node):
 
     type_string = "partitions"
-    tree_config = TreeConfig("partitions")
 
     flag_boot = 1
     flag_primary = 2
diff --git a/diskimage_builder/block_device/level1/partitioning.py b/diskimage_builder/block_device/level1/partitioning.py
index 6a8c8880d..e11ae962e 100644
--- a/diskimage_builder/block_device/level1/partitioning.py
+++ b/diskimage_builder/block_device/level1/partitioning.py
@@ -22,8 +22,6 @@ from diskimage_builder.block_device.exception import \
 from diskimage_builder.block_device.level1.mbr import MBR
 from diskimage_builder.block_device.level1.partition import \
     Partition
-from diskimage_builder.block_device.level1.partition import \
-    PartitionTreeConfig
 from diskimage_builder.block_device.utils import exec_sudo
 from diskimage_builder.block_device.utils import parse_abs_size_spec
 from diskimage_builder.block_device.utils import parse_rel_size_spec
@@ -33,34 +31,8 @@ from diskimage_builder.graph.digraph import Digraph
 logger = logging.getLogger(__name__)
 
 
-class PartitioningTreeConfig(object):
-
-    @staticmethod
-    def config_tree_to_digraph(config_key, config_value, dconfig,
-                               default_base_name, plugin_manager):
-        logger.debug("called [%s] [%s] [%s]"
-                     % (config_key, config_value, default_base_name))
-        assert config_key == "partitioning"
-        base_name = config_value['base'] if 'base' in config_value \
-                    else default_base_name
-        nconfig = {'base': base_name}
-        for k, v in config_value.items():
-            if k != 'partitions':
-                nconfig[k] = v
-            else:
-                pconfig = []
-                PartitionTreeConfig.config_tree_to_digraph(
-                    k, v, pconfig, dconfig, base_name, plugin_manager)
-                nconfig['partitions'] = pconfig
-
-        dconfig.append({config_key: nconfig})
-        logger.debug("finished new [%s] complete [%s]" % (nconfig, dconfig))
-
-
 class Partitioning(Digraph.Node):
 
-    tree_config = PartitioningTreeConfig()
-
     def __init__(self, config, default_config):
         logger.debug("Creating Partitioning object; config [%s]" % config)
         # Because using multiple partitions of one base is done
diff --git a/diskimage_builder/block_device/level2/mkfs.py b/diskimage_builder/block_device/level2/mkfs.py
index ba4469c5b..9d891597e 100644
--- a/diskimage_builder/block_device/level2/mkfs.py
+++ b/diskimage_builder/block_device/level2/mkfs.py
@@ -17,7 +17,6 @@ import uuid
 
 from diskimage_builder.block_device.exception \
     import BlockDeviceSetupException
-from diskimage_builder.block_device.tree_config import TreeConfig
 from diskimage_builder.block_device.utils import exec_sudo
 from diskimage_builder.graph.digraph import Digraph
 
@@ -164,7 +163,6 @@ class Mkfs(object):
     """
 
     type_string = "mkfs"
-    tree_config = TreeConfig("mkfs")
 
     def __init__(self, config, default_config):
         logger.debug("Create Mkfs object; config [%s]" % config)
diff --git a/diskimage_builder/block_device/level3/mount.py b/diskimage_builder/block_device/level3/mount.py
index 1046f29c5..b709f4513 100644
--- a/diskimage_builder/block_device/level3/mount.py
+++ b/diskimage_builder/block_device/level3/mount.py
@@ -17,7 +17,6 @@ import os
 
 from diskimage_builder.block_device.exception \
     import BlockDeviceSetupException
-from diskimage_builder.block_device.tree_config import TreeConfig
 from diskimage_builder.block_device.utils import exec_sudo
 from diskimage_builder.block_device.utils import sort_mount_points
 from diskimage_builder.graph.digraph import Digraph
@@ -131,7 +130,6 @@ class MountPoint(Digraph.Node):
 class Mount(object):
 
     type_string = "mount"
-    tree_config = TreeConfig("mount")
 
     def __init__(self, config, params):
         logger.debug("Mounting object; config [%s]" % config)
diff --git a/diskimage_builder/block_device/level4/fstab.py b/diskimage_builder/block_device/level4/fstab.py
index 7f3cacd75..cf4508842 100644
--- a/diskimage_builder/block_device/level4/fstab.py
+++ b/diskimage_builder/block_device/level4/fstab.py
@@ -14,7 +14,6 @@
 
 import logging
 
-from diskimage_builder.block_device.tree_config import TreeConfig
 from diskimage_builder.graph.digraph import Digraph
 
 
@@ -24,7 +23,6 @@ logger = logging.getLogger(__name__)
 class Fstab(Digraph.Node):
 
     type_string = "fstab"
-    tree_config = TreeConfig("fstab")
 
     def __init__(self, config, params):
         logger.debug("Fstab object; config [%s]" % config)
diff --git a/diskimage_builder/block_device/tests/config/deep_graph.yaml b/diskimage_builder/block_device/tests/config/deep_graph.yaml
index 993705a1e..8ee7246a1 100644
--- a/diskimage_builder/block_device/tests/config/deep_graph.yaml
+++ b/diskimage_builder/block_device/tests/config/deep_graph.yaml
@@ -1,5 +1,4 @@
 - local_loop:
-    base: image0
     name: image0
 
 - partitioning:
diff --git a/diskimage_builder/block_device/tests/config/multiple_partitions_graph.yaml b/diskimage_builder/block_device/tests/config/multiple_partitions_graph.yaml
index 42c62282c..8a46086a7 100644
--- a/diskimage_builder/block_device/tests/config/multiple_partitions_graph.yaml
+++ b/diskimage_builder/block_device/tests/config/multiple_partitions_graph.yaml
@@ -1,5 +1,4 @@
 - local_loop:
-    base: image0
     name: image0
 
 - partitioning:
diff --git a/diskimage_builder/block_device/tests/test_config.py b/diskimage_builder/block_device/tests/test_config.py
index ab0ae8bfc..af2ee9ada 100644
--- a/diskimage_builder/block_device/tests/test_config.py
+++ b/diskimage_builder/block_device/tests/test_config.py
@@ -18,6 +18,10 @@ import yaml
 
 from diskimage_builder.block_device.blockdevice \
     import BlockDevice
+from diskimage_builder.block_device.config \
+    import config_tree_to_graph
+from diskimage_builder.block_device.exception import \
+    BlockDeviceSetupException
 
 
 logger = logging.getLogger(__name__)
@@ -62,52 +66,40 @@ class TestGraphGeneration(TestConfig):
         self.bd = BlockDevice(self.fake_default_config)
 
 
-# NOTE: inherits from TestGraphGeneration for simplicity to get
-# BlockDevice.plugin_manager object for _config_tree_to_diagraph.
-# Config parsing can be moved separately (and not require a
-# BlockDevice object) in a later change.
-class TestConfigParsing(TestGraphGeneration):
+class TestConfigParsing(TestConfig):
     """Test parsing config file into a graph"""
 
     def test_config_bad_plugin(self):
-        # Currently, configuration parsing does not notice a missing
-        # plugin.  This is left as a stub
-        return
-        # config = self.load_config_file('bad_plugin.yaml')
-        # self.assertRaises(BlockDeviceSetupException,
-        #                  self.bd._config_tree_to_digraph,
-        #                  config, self.bd.plugin_manager)
+        config = self.load_config_file('bad_plugin.yaml')
+        self.assertRaises(BlockDeviceSetupException,
+                          config_tree_to_graph,
+                          config)
 
     # a graph should remain the same
     def test_graph(self):
         graph = self.load_config_file('simple_graph.yaml')
-        parsed_graph = self.bd._config_tree_to_digraph(graph,
-                                                       self.bd.plugin_manager)
-        self.assertEqual(parsed_graph, graph)
+        parsed_graph = config_tree_to_graph(graph)
+        self.assertItemsEqual(parsed_graph, graph)
 
     # equivalence of simple tree to graph
     def test_simple_tree(self):
         tree = self.load_config_file('simple_tree.yaml')
         graph = self.load_config_file('simple_graph.yaml')
-        parsed_graph = self.bd.\
-                       _config_tree_to_digraph(tree,
-                                               self.bd.plugin_manager)
+        parsed_graph = config_tree_to_graph(tree)
         self.assertItemsEqual(parsed_graph, graph)
 
     # equivalence of a deeper tree to graph
     def test_deep_tree(self):
         tree = self.load_config_file('deep_tree.yaml')
         graph = self.load_config_file('deep_graph.yaml')
-        parsed_graph = self.bd.\
-                       _config_tree_to_digraph(tree, self.bd.plugin_manager)
+        parsed_graph = config_tree_to_graph(tree)
         self.assertItemsEqual(parsed_graph, graph)
 
     # equivalence of a complicated multi-partition tree to graph
     def test_multipart_tree(self):
         tree = self.load_config_file('multiple_partitions_tree.yaml')
         graph = self.load_config_file('multiple_partitions_graph.yaml')
-        parsed_graph = self.bd._config_tree_to_digraph(tree,
-                                                       self.bd.plugin_manager)
+        parsed_graph = config_tree_to_graph(tree)
         logger.debug(parsed_graph)
         self.assertItemsEqual(parsed_graph, graph)
 
diff --git a/diskimage_builder/block_device/tree_config.py b/diskimage_builder/block_device/tree_config.py
deleted file mode 100644
index d7c5531be..000000000
--- a/diskimage_builder/block_device/tree_config.py
+++ /dev/null
@@ -1,60 +0,0 @@
-# Copyright 2016-2017 Andreas Florath (andreas@florath.net)
-#
-# Licensed under the Apache License, Version 2.0 (the "License"); you may
-# not use this file except in compliance with the License. You may obtain
-# a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-# License for the specific language governing permissions and limitations
-# under the License.
-
-import logging
-
-logger = logging.getLogger(__name__)
-
-
-class TreeConfig(object):
-    """Supports simple tree-like configuration
-
-    When using the new (complete) configuration there is a need to
-    specify the complete digraph of block level configurations.  This
-    provides great flexibility for the cost of complex configurations:
-    each and every single element must be completely specified.  In
-    many simple use cases the configuration flexibility is not
-    needed.
-
-    With the help of this object the simple to use and short tree-like
-    configuration is converted automatically into the complete digraph
-    configuration which can be used to create the block device
-    elements.
-    """
-
-    def __init__(self, type_string):
-        self.type_string = type_string
-
-    def config_tree_to_digraph(self, config_key, config_value, dconfig,
-                               default_base_name, plugin_manager):
-        logger.debug("called [%s] [%s] [%s]"
-                     % (config_key, config_value, default_base_name))
-        base_name = config_value['base'] if 'base' in config_value \
-                    else default_base_name
-        name = config_value['name'] \
-               if 'name' in config_value \
-                  else "%s_%s" % (config_key, base_name)
-        assert config_key == self.type_string
-
-        nconfig = {'base': base_name, 'name': name}
-        for k, v in config_value.items():
-            if k not in plugin_manager:
-                nconfig[k] = v
-            else:
-                plugin_manager[k].plugin \
-                    .tree_config.config_tree_to_digraph(
-                        k, v, dconfig, name, plugin_manager)
-
-        dconfig.append({self.type_string: nconfig})
-        logger.debug("finished new [%s] complete [%s]" % (nconfig, dconfig))
diff --git a/doc/source/user_guide/building_an_image.rst b/doc/source/user_guide/building_an_image.rst
index e7c8e5d49..c68f68757 100644
--- a/doc/source/user_guide/building_an_image.rst
+++ b/doc/source/user_guide/building_an_image.rst
@@ -157,7 +157,7 @@ is exactly the same as writing
    mount:
      name: mount_root_fs
      base: root_fs
-       mount_point: /
+     mount_point: /
 
 Non existing `name` and `base` entries in the tree notation are
 automatically generated: the `name` is the name of the base module