adding checks for cyclic dependencies (#558)

pyproject.toml

@@ -3,7 +3,7 @@
 
 [tool.poetry]
 name = "aws-service-catalog-puppet"
-version = "0.190.0"
+version = "0.191.0"
 description = "Making it easier to deploy ServiceCatalog products"
 classifiers = ["Development Status :: 5 - Production/Stable", "Intended Audience :: Developers", "Programming Language :: Python :: 3", "License :: OSI Approved :: Apache Software License", "Operating System :: OS Independent", "Natural Language :: English"]
 homepage = "https://service-catalog-tools-workshop.com/"

servicecatalog_puppet/commands/manifest.py

@@ -9,7 +9,7 @@
 import yamale
 import yaml
 from betterboto import client as betterboto_client
-
+import networkx as nx
 from servicecatalog_puppet import asset_helpers
 from servicecatalog_puppet import config
 from servicecatalog_puppet import constants
@@ -192,6 +192,8 @@ def validate(f):
         manifest.get("defaults", {}).get("accounts", {}).get("regions_enabled", False)
     )
 
+    graph = nx.DiGraph()
+
     tags_defined_by_accounts = {}
     for account in manifest.get("accounts"):
         account_entry_is_an_overwrite_or_append = account.get(
@@ -248,25 +250,66 @@ def validate(f):
                     )
                     dd = depends_on.get("name")
                     if manifest.get(tt).get(dd) is None:
-                        print_utils.warn(f"{collection_type}.{collection_name} uses {depends_on} in depends_on that does not exist",)
+                        print_utils.warn(
+                            f"{collection_type}.{collection_name} uses {depends_on} in depends_on that does not exist",
+                        )
 
             #
             # Check depends_on is present when parameters names match outputs defined elsewhere
             #
-            for parameter_name, parameter_details in collection_item.get("parameters", {}).items():
+            for parameter_name, parameter_details in collection_item.get(
+                "parameters", {}
+            ).items():
                 if parameter_details.get("ssm"):
                     output_name = parameter_details.get("ssm").get("name")
-                    for needle_section_name in constants.ALL_SECTION_NAMES_THAT_GENERATE_OUTPUTS:
-                        for needle_action_name, needle_action_details in manifest.get(needle_section_name, {}).items():
-                            for needle_output in needle_action_details.get("outputs", {}).get("ssm", []):
+                    for (
+                        needle_section_name
+                    ) in constants.ALL_SECTION_NAMES_THAT_GENERATE_OUTPUTS:
+                        for needle_action_name, needle_action_details in manifest.get(
+                            needle_section_name, {}
+                        ).items():
+                            for needle_output in needle_action_details.get(
+                                "outputs", {}
+                            ).get("ssm", []):
                                 if output_name == needle_output.get("param_name"):
                                     found = False
-                                    for dependency in collection_item.get("depends_on", []):
-                                        plural = constants.SECTION_SINGULAR_TO_PLURAL.get(dependency.get("type", constants.LAUNCH))
-                                        if dependency.get("name") == needle_action_name and plural == needle_section_name:
+                                    for dependency in collection_item.get(
+                                        "depends_on", []
+                                    ):
+                                        plural = constants.SECTION_SINGULAR_TO_PLURAL.get(
+                                            dependency.get("type", constants.LAUNCH)
+                                        )
+                                        if (
+                                            dependency.get("name") == needle_action_name
+                                            and plural == needle_section_name
+                                        ):
                                             found = True
                                     if not found:
-                                        print_utils.error(f"{output_name} is used in {collection_type}.{collection_name} from {needle_section_name}.{needle_action_name} but is not in depends_on")
+                                        print_utils.error(
+                                            f"{output_name} is used in {collection_type}.{collection_name} from {needle_section_name}.{needle_action_name} but is not in depends_on"
+                                        )
+
+            #
+            # build graph to check for issues
+            #
+            uid = f"{collection_type}|{collection_name}"
+            data = collection_item
+            graph.add_nodes_from(
+                [(uid, data),]
+            )
+            for dependency in collection_item.get("depends_on", []):
+                plural = constants.SECTION_SINGULAR_TO_PLURAL.get(
+                    dependency.get("type", constants.LAUNCH)
+                )
+                duid = f"{plural}|{dependency.get('name')}"
+                graph.add_edge(uid, duid)
+    try:
+        cycle = nx.find_cycle(graph)
+        raise Exception(
+            f"found cyclic dependency in your manifest file between: {cycle}"
+        )
+    except nx.exception.NetworkXNoCycle:
+        pass
 
     print_utils.echo("Finished validating: {}".format(f.name))
     print_utils.echo("Finished validating: OK")

servicecatalog_puppet/commands/task_reference.py

@@ -9,6 +9,7 @@
 from servicecatalog_puppet.workflow.dependencies import (
     get_dependencies_for_task_reference,
 )
+import networkx as nx
 
 logger = logging.getLogger(constants.PUPPET_LOGGER_NAME)
 
@@ -29,6 +30,25 @@ def get_spoke_local_portfolio_common_args(
     )
 
 
+def ensure_no_cyclic_dependencies(name, tasks):
+    graph = nx.DiGraph()
+    for t_name, t in tasks.items():
+        uid = t_name
+        data = t
+        graph.add_nodes_from(
+            [(uid, data),]
+        )
+        for duid in t.get("dependencies_by_reference", []):
+            graph.add_edge(uid, duid)
+    try:
+        cycle = nx.find_cycle(graph)
+        raise Exception(
+            f"found cyclic dependency task reference file {name} between: {cycle}"
+        )
+    except nx.exception.NetworkXNoCycle:
+        pass
+
+
 def generate_complete_task_reference(puppet_account_id, manifest, output_file_path):
     default_region = config.get_home_region(puppet_account_id)
     regions_in_use = config.get_regions()
@@ -457,6 +477,7 @@ def generate_complete_task_reference(puppet_account_id, manifest, output_file_pa
             all_tasks[dep]["reverse_dependencies_by_reference"].append(task_reference)
 
     reference = dict(all_tasks=all_tasks,)
+    ensure_no_cyclic_dependencies("complete task reference", all_tasks)
     open(output_file_path, "w").write(yaml_utils.dump(reference))
     return reference
 
@@ -1357,7 +1378,8 @@ def handle_launches(
             puppet_account_id=puppet_account_id,
             task_reference=describe_provisioning_params_ref,
             dependencies_by_reference=[
-                hub_portfolio_ref,  # TODO check this still works for a new portfolio after changing it from: portfolio_deploying_from
+                hub_portfolio_ref,
+                # TODO check this still works for a new portfolio after changing it from: portfolio_deploying_from
             ],  # associations are added here and so this is a dependency
             reverse_dependencies_by_reference=[],
             account_id=puppet_account_id,
@@ -1507,6 +1529,7 @@ def generate_hub_task_reference(puppet_account_id, all_tasks, output_file_path):
                 )
 
     result = dict(all_tasks=tasks_to_include)
+    ensure_no_cyclic_dependencies("hub task reference", tasks_to_include)
     open(output_file_path, "w").write(yaml_utils.dump(result))
     return result
 
@@ -1531,6 +1554,7 @@ def generate_overridden_task_reference(
                 overridden_tasks[task_name] = task
 
     result = dict(all_tasks=overridden_tasks)
+    ensure_no_cyclic_dependencies("overridden task reference", overridden_tasks)
     open(output_file_path, "w").write(yaml_utils.dump(result))
     return result
 

servicecatalog_puppet/print_utils.py

@@ -4,22 +4,16 @@
 
 
 def echo(message):
-    click.secho(
-        message,
-    )
+    click.secho(message,)
 
 
 def warn(message):
     click.secho(
-        message,
-        err=True,
-        fg="yellow",
+        message, err=True, fg="yellow",
     )
 
 
 def error(message):
     click.secho(
-        message,
-        err=True,
-        fg="red",
-    )
+        message, err=True, fg="red",
+    )

setup.py

@@ -62,7 +62,7 @@
 
 setup_kwargs = {
     'name': 'aws-service-catalog-puppet',
-    'version': '0.190.0',
+    'version': '0.191.0',
     'description': 'Making it easier to deploy ServiceCatalog products',
     'long_description': '# aws-service-catalog-puppet\n\n![logo](./docs/logo.png) \n\n## Badges\n\n[![codecov](https://codecov.io/gh/awslabs/aws-service-catalog-puppet/branch/master/graph/badge.svg?token=e8M7mdsmy0)](https://codecov.io/gh/awslabs/aws-service-catalog-puppet)\n\n\n## What is it?\nThis is a python3 framework that makes it easier to share multi region AWS Service Catalog portfolios and makes it \npossible to provision products into accounts declaratively using a metadata based rules engine.\n\nWith this framework you define your accounts in a YAML file.  You give each account a set of tags, a default region and \na set of enabled regions.\n\nOnce you have done this you can define portfolios should be shared with each set of accounts using the tags and you \ncan specify which regions the shares occur in.\n\nIn addition to this, you can also define products that should be provisioned into accounts using the same tag based \napproach.  The framework will assume role into the target account and provision the product on your behalf.\n\n\n## Getting started\n\nYou can read the [installation how to](https://service-catalog-tools-workshop.com/30-how-tos/10-installation/30-service-catalog-puppet.html)\nor you can read through the [every day use](https://service-catalog-tools-workshop.com/30-how-tos/50-every-day-use.html)\nguides.\n\nYou can read the [documentation](https://aws-service-catalog-puppet.readthedocs.io/en/latest/) to understand the inner \nworkings. \n\n\n## Going further\n\nThe framework is one of a pair.  The other is [aws-service-catalog-factory](https://github.com/awslabs/aws-service-catalog-factory).\nWith Service Catalog Factory you can create pipelines that deploy multi region portfolios very easily. \n\n## License\n\nThis library is licensed under the Apache 2.0 License. \n \n',
     'author': 'Eamonn Faherty',