From 88025d0733f765cad08cef1e643143cfd0f144b9 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Mon, 24 Oct 2016 17:32:30 -0400 Subject: [PATCH 01/10] Basic verification architecture added. --- awx/api/generics.py | 8 ++++++++ awx/api/views.py | 25 +++++++++++++++++++++++++ awx/main/scheduler/dag_simple.py | 3 +++ 3 files changed, 36 insertions(+) diff --git a/awx/api/generics.py b/awx/api/generics.py index 4c4247b23d..7b58f14edb 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -370,6 +370,9 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView): # Base class for a sublist view that allows for creating subobjects and # attaching/detaching them from the parent. + def is_valid_relation(self, parent, sub): + return None + def get_description_context(self): d = super(SubListCreateAttachDetachAPIView, self).get_description_context() d.update({ @@ -406,6 +409,11 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView): skip_sub_obj_read_check=created): raise PermissionDenied() + # Verify that the relationship to be added is valid. + attach_errors = self.is_valid_relation(parent, sub) + if attach_errors is not None: + return Response(attach_errors, status=status.HTTP_400_BAD_REQUEST) + # Attach the object to the collection. if sub not in relationship.all(): relationship.add(sub) diff --git a/awx/api/views.py b/awx/api/views.py index 699d5c9e81..90621155c5 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -69,6 +69,8 @@ from awx.api.renderers import * # noqa from awx.api.serializers import * # noqa from awx.api.metadata import RoleMetadata from awx.main.consumers import emit_channel_notification +from awx.main.scheduler.dag_simple import SimpleDAG + logger = logging.getLogger('awx.api.views') @@ -2656,6 +2658,29 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su self.check_parent_access(parent) return getattr(parent, self.relationship).all() + def is_valid_relation(self, parent, sub): + workflow_nodes = parent.workflow_job_template.workflow_job_template_nodes.all() + graph = SimpleDAG() + for workflow_node in workflow_nodes: + graph.add_node(workflow_node) + + find = False + for node_type in ['success_nodes', 'failure_nodes', 'always_nodes']: + for workflow_node in workflow_nodes: + related_nodes = getattr(workflow_node, node_type).all() + for related_node in related_nodes: + graph.add_edge(workflow_node, related_node, node_type) + if not find and parent == workflow_node and\ + sub == related_node and self.relationship == node_type: + find = True + if not find: + graph.add_edge(parent, sub, self.relationship) + + if graph.cycle_detected(): + return {"Error": "cycle detected!"} + + return None + class WorkflowJobTemplateNodeSuccessNodesList(WorkflowJobTemplateNodeChildrenBaseList): relationship = 'success_nodes' diff --git a/awx/main/scheduler/dag_simple.py b/awx/main/scheduler/dag_simple.py index aeb0ff759e..3fbb16901f 100644 --- a/awx/main/scheduler/dag_simple.py +++ b/awx/main/scheduler/dag_simple.py @@ -138,3 +138,6 @@ class SimpleDAG(object): roots.append(n) return roots + # TODO + def cycle_detected(self): + return False From 279255551060d281f6133dcf63a2eb7d97533e98 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Tue, 25 Oct 2016 17:02:37 -0400 Subject: [PATCH 02/10] Cycle detector added. --- awx/main/scheduler/dag_simple.py | 37 ++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/awx/main/scheduler/dag_simple.py b/awx/main/scheduler/dag_simple.py index 3fbb16901f..f28ad83184 100644 --- a/awx/main/scheduler/dag_simple.py +++ b/awx/main/scheduler/dag_simple.py @@ -138,6 +138,39 @@ class SimpleDAG(object): roots.append(n) return roots - # TODO - def cycle_detected(self): + def _find_cycle(self, node): + stack = [node] + node['metadata']['color'] = 'gray' + + while len(stack) > 0: + if stack[-1]['metadata']['count'] == len(stack[-1]['metadata']['adj_list']): + stack[-1]['metadata']['color'] = 'black' + stack.pop() + else: + to_push = stack[-1]['metadata']['adj_list'][stack[-1]['metadata']['count']] + stack[-1]['metadata']['count'] += 1 + if to_push['metadata']['color'] == 'gray': + return True + elif to_push['metadata']['color'] == 'white': + to_push['metadata']['color'] = 'gray' + stack.append(to_push) + + return False + + def _clean_meta(self): + for node in self.nodes: + node['metadata'] = None + + def cycle_detected(self): + for node in self.nodes: + node['metadata'] = {"adj_list": [], "color": "white", "count": 0} + for edge in self.edges: + self.nodes[edge[0]]['metadata']['adj_list'].append(self.nodes[edge[1]]) + + for node in self.nodes: + if node['metadata']['color'] == 'white' and self._find_cycle(node): + self._clean_meta() + return True + + self._clean_meta() return False From ad04015c9f17d35b441764c16eaee8f665ecb352 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Tue, 25 Oct 2016 18:53:31 -0400 Subject: [PATCH 03/10] Multi-ancestor detector added. --- awx/api/views.py | 6 ++++-- awx/main/scheduler/dag_simple.py | 13 +++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index 90621155c5..a5a85f2fdf 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -71,7 +71,6 @@ from awx.api.metadata import RoleMetadata from awx.main.consumers import emit_channel_notification from awx.main.scheduler.dag_simple import SimpleDAG - logger = logging.getLogger('awx.api.views') def api_exception_handler(exc, context): @@ -2677,7 +2676,10 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su graph.add_edge(parent, sub, self.relationship) if graph.cycle_detected(): - return {"Error": "cycle detected!"} + return {"Error": "Cycle detected!"} + + if graph.multi_ancestor_detected(): + return {"Error": "Multiple ancestor detected!"} return None diff --git a/awx/main/scheduler/dag_simple.py b/awx/main/scheduler/dag_simple.py index f28ad83184..e629eccd98 100644 --- a/awx/main/scheduler/dag_simple.py +++ b/awx/main/scheduler/dag_simple.py @@ -174,3 +174,16 @@ class SimpleDAG(object): self._clean_meta() return False + + def multi_ancestor_detected(self): + for node in self.nodes: + node['metadata'] = {"ancestor": None} + for edge in self.edges: + if self.nodes[edge[1]]['metadata']['ancestor'] is None: + self.nodes[edge[1]]['metadata']['ancestor'] = self.nodes[edge[0]] + else: + self._clean_meta() + return True + + self._clean_meta() + return False From 02cccf35d3363a3bb29e576bd155f199d3e9c2ef Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Thu, 27 Oct 2016 16:27:42 -0400 Subject: [PATCH 04/10] Delete created resource when it fails the relationship verification. --- awx/api/generics.py | 2 ++ awx/api/views.py | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/awx/api/generics.py b/awx/api/generics.py index 7b58f14edb..7a818722bc 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -412,6 +412,8 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView): # Verify that the relationship to be added is valid. attach_errors = self.is_valid_relation(parent, sub) if attach_errors is not None: + if created: + sub.delete() return Response(attach_errors, status=status.HTTP_400_BAD_REQUEST) # Attach the object to the collection. diff --git a/awx/api/views.py b/awx/api/views.py index a5a85f2fdf..910eae869b 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2669,8 +2669,10 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su related_nodes = getattr(workflow_node, node_type).all() for related_node in related_nodes: graph.add_edge(workflow_node, related_node, node_type) - if not find and parent == workflow_node and\ - sub == related_node and self.relationship == node_type: + if (not find and + parent == workflow_node and + sub == related_node and + self.relationship == node_type): find = True if not find: graph.add_edge(parent, sub, self.relationship) From 7472cf0dc92a71657a7b0d3f4a0594575e117950 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Fri, 28 Oct 2016 14:05:44 -0400 Subject: [PATCH 05/10] Refactor for better performance. --- awx/api/generics.py | 4 +-- awx/api/views.py | 40 +++++++++++++------------ awx/main/scheduler/dag_simple.py | 50 -------------------------------- 3 files changed, 24 insertions(+), 70 deletions(-) diff --git a/awx/api/generics.py b/awx/api/generics.py index 7a818722bc..e474a1bafc 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -370,7 +370,7 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView): # Base class for a sublist view that allows for creating subobjects and # attaching/detaching them from the parent. - def is_valid_relation(self, parent, sub): + def is_valid_relation(self, parent, sub, created=False): return None def get_description_context(self): @@ -410,7 +410,7 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView): raise PermissionDenied() # Verify that the relationship to be added is valid. - attach_errors = self.is_valid_relation(parent, sub) + attach_errors = self.is_valid_relation(parent, sub, created=created) if attach_errors is not None: if created: sub.delete() diff --git a/awx/api/views.py b/awx/api/views.py index 910eae869b..84306332b0 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -69,7 +69,6 @@ from awx.api.renderers import * # noqa from awx.api.serializers import * # noqa from awx.api.metadata import RoleMetadata from awx.main.consumers import emit_channel_notification -from awx.main.scheduler.dag_simple import SimpleDAG logger = logging.getLogger('awx.api.views') @@ -2657,31 +2656,36 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su self.check_parent_access(parent) return getattr(parent, self.relationship).all() - def is_valid_relation(self, parent, sub): - workflow_nodes = parent.workflow_job_template.workflow_job_template_nodes.all() - graph = SimpleDAG() + def is_valid_relation(self, parent, sub, created=False): + if created: + return None + + workflow_nodes = parent.workflow_job_template.workflow_job_template_nodes.all().\ + prefetch_related('success_nodes', 'failure_nodes', 'always_nodes') + graph = {} for workflow_node in workflow_nodes: - graph.add_node(workflow_node) + graph[workflow_node.pk] = dict(node_object=workflow_node, metadata={'parent': None}) find = False for node_type in ['success_nodes', 'failure_nodes', 'always_nodes']: for workflow_node in workflow_nodes: - related_nodes = getattr(workflow_node, node_type).all() + parent_node = graph[workflow_node.pk] + related_nodes = getattr(parent_node['node_object'], node_type).all() for related_node in related_nodes: - graph.add_edge(workflow_node, related_node, node_type) - if (not find and - parent == workflow_node and - sub == related_node and - self.relationship == node_type): + sub_node = graph[related_node.pk] + sub_node['metadata']['parent'] = parent_node + if not find and parent == workflow_node and sub == related_node and self.relationship == node_type: find = True if not find: - graph.add_edge(parent, sub, self.relationship) - - if graph.cycle_detected(): - return {"Error": "Cycle detected!"} - - if graph.multi_ancestor_detected(): - return {"Error": "Multiple ancestor detected!"} + sub_node = graph[sub.pk] + parent_node = graph[parent.pk] + if sub_node['metadata']['parent'] is not None: + return {"Error": "Multiple ancestor detected!"} + iter_node = parent_node + while iter_node is not None: + if iter_node == sub_node: + return {"Error": "Cycle detected!"} + iter_node = iter_node['metadata']['parent'] return None diff --git a/awx/main/scheduler/dag_simple.py b/awx/main/scheduler/dag_simple.py index e629eccd98..c6aa6247c0 100644 --- a/awx/main/scheduler/dag_simple.py +++ b/awx/main/scheduler/dag_simple.py @@ -137,53 +137,3 @@ class SimpleDAG(object): if len(self.get_dependents(n['node_object'])) < 1: roots.append(n) return roots - - def _find_cycle(self, node): - stack = [node] - node['metadata']['color'] = 'gray' - - while len(stack) > 0: - if stack[-1]['metadata']['count'] == len(stack[-1]['metadata']['adj_list']): - stack[-1]['metadata']['color'] = 'black' - stack.pop() - else: - to_push = stack[-1]['metadata']['adj_list'][stack[-1]['metadata']['count']] - stack[-1]['metadata']['count'] += 1 - if to_push['metadata']['color'] == 'gray': - return True - elif to_push['metadata']['color'] == 'white': - to_push['metadata']['color'] = 'gray' - stack.append(to_push) - - return False - - def _clean_meta(self): - for node in self.nodes: - node['metadata'] = None - - def cycle_detected(self): - for node in self.nodes: - node['metadata'] = {"adj_list": [], "color": "white", "count": 0} - for edge in self.edges: - self.nodes[edge[0]]['metadata']['adj_list'].append(self.nodes[edge[1]]) - - for node in self.nodes: - if node['metadata']['color'] == 'white' and self._find_cycle(node): - self._clean_meta() - return True - - self._clean_meta() - return False - - def multi_ancestor_detected(self): - for node in self.nodes: - node['metadata'] = {"ancestor": None} - for edge in self.edges: - if self.nodes[edge[1]]['metadata']['ancestor'] is None: - self.nodes[edge[1]]['metadata']['ancestor'] = self.nodes[edge[0]] - else: - self._clean_meta() - return True - - self._clean_meta() - return False From 53eb198abbbbe8c49e2b847e4e5cd05c8b02cd28 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Sun, 30 Oct 2016 14:58:32 -0400 Subject: [PATCH 06/10] Functional test added. --- awx/main/tests/functional/models/test_workflow.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/awx/main/tests/functional/models/test_workflow.py b/awx/main/tests/functional/models/test_workflow.py index 0c3d5d88be..b8053ff9b2 100644 --- a/awx/main/tests/functional/models/test_workflow.py +++ b/awx/main/tests/functional/models/test_workflow.py @@ -90,6 +90,17 @@ class TestWorkflowJobTemplate: assert len(parent_qs) == 1 assert parent_qs[0] == wfjt.workflow_job_template_nodes.all()[1] + def test_topology_validator(self, wfjt): + from awx.api.views import WorkflowJobTemplateNodeChildrenBaseList + test_view = WorkflowJobTemplateNodeChildrenBaseList() + nodes = wfjt.workflow_job_template_nodes.all() + node_assoc = WorkflowJobTemplateNode.objects.create(workflow_job_template=wfjt) + nodes[2].always_nodes.add(node_assoc) + # test cycle validation + assert test_view.is_valid_relation(node_assoc, nodes[0]) == {'Error': 'Cycle detected!'} + # test multi-ancestor validation + assert test_view.is_valid_relation(node_assoc, nodes[1]) == {'Error': 'Multiple ancestor detected!'} + @pytest.mark.django_db class TestWorkflowJobFailure: """ From a85075aceab787a10642f6dedf369b48055a919e Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Sun, 30 Oct 2016 15:36:28 -0400 Subject: [PATCH 07/10] Mutex validator added. --- awx/api/views.py | 5 +++++ awx/main/tests/functional/models/test_workflow.py | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/awx/api/views.py b/awx/api/views.py index 84306332b0..1e5c4284b9 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2657,6 +2657,11 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su return getattr(parent, self.relationship).all() def is_valid_relation(self, parent, sub, created=False): + mutex_list = ('success_nodes', 'failure_nodes') if self.relationship == 'always_nodes' else ('always_nodes',) + for relation in mutex_list: + if getattr(parent, relation).all().exists(): + return {'Error': 'Cannot associate {0} when {1} have been associated.'.format(self.relationship, relation)} + if created: return None diff --git a/awx/main/tests/functional/models/test_workflow.py b/awx/main/tests/functional/models/test_workflow.py index b8053ff9b2..0d39d6c091 100644 --- a/awx/main/tests/functional/models/test_workflow.py +++ b/awx/main/tests/functional/models/test_workflow.py @@ -100,6 +100,11 @@ class TestWorkflowJobTemplate: assert test_view.is_valid_relation(node_assoc, nodes[0]) == {'Error': 'Cycle detected!'} # test multi-ancestor validation assert test_view.is_valid_relation(node_assoc, nodes[1]) == {'Error': 'Multiple ancestor detected!'} + # test mutex validation + test_view.relationship = 'failure_nodes' + node_assoc_1 = WorkflowJobTemplateNode.objects.create(workflow_job_template=wfjt) + assert (test_view.is_valid_relation(nodes[2], node_assoc_1) == + {'Error': 'Cannot associate failure_nodes when always_nodes have been associated.'}) @pytest.mark.django_db class TestWorkflowJobFailure: From ff1fdb4f5b6b07ca59fbc519534773bd7d31380b Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Tue, 1 Nov 2016 15:07:36 -0400 Subject: [PATCH 08/10] Error message text improvement. --- awx/api/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index 1e5c4284b9..be4dfe0154 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2685,11 +2685,11 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su sub_node = graph[sub.pk] parent_node = graph[parent.pk] if sub_node['metadata']['parent'] is not None: - return {"Error": "Multiple ancestor detected!"} + return {"Error": "Multiple parent relationship not allowed."} iter_node = parent_node while iter_node is not None: if iter_node == sub_node: - return {"Error": "Cycle detected!"} + return {"Error": "Cycle detected."} iter_node = iter_node['metadata']['parent'] return None From d1ec44f20acfd03bfd574a2fc03ddbf2ae79ebfb Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Tue, 1 Nov 2016 15:35:54 -0400 Subject: [PATCH 09/10] i18n changes. --- awx/api/views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index be4dfe0154..b84a226ce2 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2660,7 +2660,7 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su mutex_list = ('success_nodes', 'failure_nodes') if self.relationship == 'always_nodes' else ('always_nodes',) for relation in mutex_list: if getattr(parent, relation).all().exists(): - return {'Error': 'Cannot associate {0} when {1} have been associated.'.format(self.relationship, relation)} + return {'Error': _('Cannot associate {0} when {1} have been associated.'.format(self.relationship, relation))} if created: return None @@ -2685,11 +2685,11 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su sub_node = graph[sub.pk] parent_node = graph[parent.pk] if sub_node['metadata']['parent'] is not None: - return {"Error": "Multiple parent relationship not allowed."} + return {"Error": _("Multiple parent relationship not allowed.")} iter_node = parent_node while iter_node is not None: if iter_node == sub_node: - return {"Error": "Cycle detected."} + return {"Error": _("Cycle detected.")} iter_node = iter_node['metadata']['parent'] return None From 843c48934e952665fcb0483c6ee4dc59a34c7de6 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Wed, 2 Nov 2016 11:36:08 -0400 Subject: [PATCH 10/10] Refactor cycle detector to prevent potential inf loop. --- awx/api/views.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index b84a226ce2..4022558385 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2660,7 +2660,7 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su mutex_list = ('success_nodes', 'failure_nodes') if self.relationship == 'always_nodes' else ('always_nodes',) for relation in mutex_list: if getattr(parent, relation).all().exists(): - return {'Error': _('Cannot associate {0} when {1} have been associated.'.format(self.relationship, relation))} + return {'Error': 'Cannot associate {0} when {1} have been associated.'.format(self.relationship, relation)} if created: return None @@ -2669,7 +2669,7 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su prefetch_related('success_nodes', 'failure_nodes', 'always_nodes') graph = {} for workflow_node in workflow_nodes: - graph[workflow_node.pk] = dict(node_object=workflow_node, metadata={'parent': None}) + graph[workflow_node.pk] = dict(node_object=workflow_node, metadata={'parent': None, 'traversed': False}) find = False for node_type in ['success_nodes', 'failure_nodes', 'always_nodes']: @@ -2685,11 +2685,13 @@ class WorkflowJobTemplateNodeChildrenBaseList(EnforceParentRelationshipMixin, Su sub_node = graph[sub.pk] parent_node = graph[parent.pk] if sub_node['metadata']['parent'] is not None: - return {"Error": _("Multiple parent relationship not allowed.")} - iter_node = parent_node + return {"Error": "Multiple parent relationship not allowed."} + sub_node['metadata']['parent'] = parent_node + iter_node = sub_node while iter_node is not None: - if iter_node == sub_node: - return {"Error": _("Cycle detected.")} + if iter_node['metadata']['traversed']: + return {"Error": "Cycle detected."} + iter_node['metadata']['traversed'] = True iter_node = iter_node['metadata']['parent'] return None