From e2bf53cc522115e40150e0d6b8a395959e3adadf Mon Sep 17 00:00:00 2001 From: Gordon Date: Mon, 4 Dec 2023 19:50:13 -0500 Subject: [PATCH] Fix non-determinism with visualizer yamls --- pkg/construct2/paths.go | 15 ++++++- pkg/engine2/engine_test.go | 2 +- .../testdata/ecs_rds.dataflow-viz.yaml | 2 +- pkg/engine2/testdata/ecs_rds.expect.yaml | 10 ++--- pkg/engine2/testdata/k8s_api.expect.yaml | 2 +- pkg/engine2/visualizer.go | 42 ++++++++++++------- pkg/graph_addons/logger.go | 41 ++---------------- 7 files changed, 51 insertions(+), 63 deletions(-) diff --git a/pkg/construct2/paths.go b/pkg/construct2/paths.go index abe45763e..0e9d8ffc3 100644 --- a/pkg/construct2/paths.go +++ b/pkg/construct2/paths.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "math" + "sort" "strings" "github.com/dominikbraun/graph" @@ -119,8 +120,17 @@ func bellmanFord(g Graph, source ResourceId, skipEdge func(Edge) bool) (*bellman } dist[source] = 0 + // Sort the keys to ensure deterministic results. It adds +O(N) to the runtime, but + // when it's already O(N * E), it doesn't matter. + sortedKeys := make([]ResourceId, 0, len(adjacencyMap)) + for key := range adjacencyMap { + sortedKeys = append(sortedKeys, key) + } + sort.Sort(sortedIds(sortedKeys)) + for i := 0; i < len(adjacencyMap)-1; i++ { - for key, edges := range adjacencyMap { + for _, key := range sortedKeys { + edges := adjacencyMap[key] for _, edge := range edges { if skipEdge(edge) { continue @@ -134,7 +144,8 @@ func bellmanFord(g Graph, source ResourceId, skipEdge func(Edge) bool) (*bellman } } - for _, edges := range adjacencyMap { + for _, key := range sortedKeys { + edges := adjacencyMap[key] for _, edge := range edges { if skipEdge(edge) { continue diff --git a/pkg/engine2/engine_test.go b/pkg/engine2/engine_test.go index 7f635aad2..ebc1f6447 100644 --- a/pkg/engine2/engine_test.go +++ b/pkg/engine2/engine_test.go @@ -169,7 +169,7 @@ func assertYamlMatches(t *testing.T, expectStr, actualStr string, name string) { case diff.DELETE: t.Errorf("[%s] %s %s: %v", name, c.Type, path, c.From) case diff.UPDATE: - t.Errorf("[%s] %s %s: %v -> %v", name, c.Type, path, c.From, c.To) + t.Errorf("[%s] %s %s: %v to %v", name, c.Type, path, c.From, c.To) } } } diff --git a/pkg/engine2/testdata/ecs_rds.dataflow-viz.yaml b/pkg/engine2/testdata/ecs_rds.dataflow-viz.yaml index 0aa0e48a8..496e888be 100755 --- a/pkg/engine2/testdata/ecs_rds.dataflow-viz.yaml +++ b/pkg/engine2/testdata/ecs_rds.dataflow-viz.yaml @@ -12,6 +12,6 @@ resources: parent: vpc/vpc-0 ecs_service/ecs_service_0 -> rds_instance/rds-instance-2: - path: aws:subnet:vpc-0:subnet-1,aws:security_group:vpc-0:security_group-rds-instance-2-1 + path: aws:ecs_task_definition:ecs_service_0,aws:iam_role:ecs_service_0-execution-role diff --git a/pkg/engine2/testdata/ecs_rds.expect.yaml b/pkg/engine2/testdata/ecs_rds.expect.yaml index 7ef0732ad..2e269ea2a 100755 --- a/pkg/engine2/testdata/ecs_rds.expect.yaml +++ b/pkg/engine2/testdata/ecs_rds.expect.yaml @@ -175,6 +175,11 @@ resources: Protocol: "-1" ToPort: 0 IngressRules: + - Description: Allow ingress traffic from within the same security group + FromPort: 0 + Protocol: "-1" + Self: true + ToPort: 0 - CidrBlocks: - 10.0.128.0/18 Description: Allow ingress traffic from ip addresses within the subnet subnet-0 @@ -187,11 +192,6 @@ resources: FromPort: 0 Protocol: "-1" ToPort: 0 - - Description: Allow ingress traffic from within the same security group - FromPort: 0 - Protocol: "-1" - Self: true - ToPort: 0 Vpc: aws:vpc:vpc-0 aws:route_table:route_table-subnet-0-0: Routes: diff --git a/pkg/engine2/testdata/k8s_api.expect.yaml b/pkg/engine2/testdata/k8s_api.expect.yaml index f40309e97..e949f5943 100755 --- a/pkg/engine2/testdata/k8s_api.expect.yaml +++ b/pkg/engine2/testdata/k8s_api.expect.yaml @@ -120,8 +120,8 @@ resources: aws:load_balancer:rest-api-4-integbcc77100: Scheme: internal Subnets: - - aws:subnet:vpc-0:subnet-0 - aws:subnet:vpc-0:subnet-1 + - aws:subnet:vpc-0:subnet-0 Type: network aws:load_balancer_listener:rest_api_4_integration_0-pod2: DefaultActions: diff --git a/pkg/engine2/visualizer.go b/pkg/engine2/visualizer.go index c2492596d..2a98a8400 100644 --- a/pkg/engine2/visualizer.go +++ b/pkg/engine2/visualizer.go @@ -116,7 +116,18 @@ func (e *Engine) GetViewsDag(view View, ctx solution_context.SolutionContext) (c } zap.S().Debugf("%s paths: %d", src, len(deps.Paths)) sort.Slice(deps.Paths, func(i, j int) bool { - return len(deps.Paths[i]) < len(deps.Paths[j]) + li, lj := len(deps.Paths[i]), len(deps.Paths[j]) + if li != lj { + return li < lj + } + for pathIdx := 0; pathIdx < li; pathIdx++ { + resI := deps.Paths[i][pathIdx] + resJ := deps.Paths[j][pathIdx] + if resI != resJ { + return construct.ResourceIdLess(resI, resJ) + } + } + return false }) seenSmall := make(set.Set[construct.ResourceId]) for _, path := range deps.Paths { @@ -127,24 +138,25 @@ func (e *Engine) GetViewsDag(view View, ctx solution_context.SolutionContext) (c dstTag := e.GetResourceVizTag(string(DataflowView), dst) switch dstTag { case ParentIconTag: - if node.Parent.IsZero() { - template, err := e.Kb.GetResourceTemplate(dst) + if !node.Parent.IsZero() { + continue + } + template, err := e.Kb.GetResourceTemplate(dst) + if err != nil { + errs = errors.Join(errs, err) + continue + } + if collectionutil.Contains(template.Classification.Is, "cluster") || + collectionutil.Contains(template.Classification.Is, "network") { + hasPath, err := HasParent(topo, ctx, src, dst) if err != nil { errs = errors.Join(errs, err) - continue } - if collectionutil.Contains(template.Classification.Is, "cluster") || - collectionutil.Contains(template.Classification.Is, "network") { - hasPath, err := HasParent(topo, ctx, src, dst) - if err != nil { - errs = errors.Join(errs, err) - } - if node.Parent.IsZero() && hasPath { - node.Parent = dst - } - } else { - errs = errors.Join(errs, createEdgeIfPath(topo, ctx, src, dst, path)) + if node.Parent.IsZero() && hasPath { + node.Parent = dst } + } else { + errs = errors.Join(errs, createEdgeIfPath(topo, ctx, src, dst, path)) } case BigIconTag: errs = errors.Join(errs, createEdgeIfPath(topo, ctx, src, dst, path)) diff --git a/pkg/graph_addons/logger.go b/pkg/graph_addons/logger.go index 0a1d51bf2..84f70e7d3 100644 --- a/pkg/graph_addons/logger.go +++ b/pkg/graph_addons/logger.go @@ -6,13 +6,10 @@ import ( ) type LoggingGraph[K comparable, T any] struct { - Log *zap.SugaredLogger - Graph graph.Graph[K, T] - Hash func(T) K -} + graph.Graph[K, T] -func (g LoggingGraph[K, T]) Traits() *graph.Traits { - return g.Graph.Traits() + Log *zap.SugaredLogger + Hash func(T) K } func (g LoggingGraph[K, T]) AddVertex(value T, options ...func(*graph.VertexProperties)) error { @@ -30,14 +27,6 @@ func (g LoggingGraph[K, T]) AddVerticesFrom(other graph.Graph[K, T]) error { return g.Graph.AddVerticesFrom(other) } -func (g LoggingGraph[K, T]) Vertex(hash K) (T, error) { - return g.Graph.Vertex(hash) -} - -func (g LoggingGraph[K, T]) VertexWithProperties(hash K) (T, graph.VertexProperties, error) { - return g.Graph.VertexWithProperties(hash) -} - func (g LoggingGraph[K, T]) RemoveVertex(hash K) error { err := g.Graph.RemoveVertex(hash) if err != nil { @@ -63,14 +52,6 @@ func (g LoggingGraph[K, T]) AddEdgesFrom(other graph.Graph[K, T]) error { return g.Graph.AddEdgesFrom(other) } -func (g LoggingGraph[K, T]) Edge(sourceHash K, targetHash K) (graph.Edge[T], error) { - return g.Graph.Edge(sourceHash, targetHash) -} - -func (g LoggingGraph[K, T]) Edges() ([]graph.Edge[K], error) { - return g.Graph.Edges() -} - func (g LoggingGraph[K, T]) UpdateEdge(source K, target K, options ...func(properties *graph.EdgeProperties)) error { err := g.Graph.UpdateEdge(source, target, options...) if err != nil { @@ -91,14 +72,6 @@ func (g LoggingGraph[K, T]) RemoveEdge(source K, target K) error { return err } -func (g LoggingGraph[K, T]) AdjacencyMap() (map[K]map[K]graph.Edge[K], error) { - return g.Graph.AdjacencyMap() -} - -func (g LoggingGraph[K, T]) PredecessorMap() (map[K]map[K]graph.Edge[K], error) { - return g.Graph.PredecessorMap() -} - func (g LoggingGraph[K, T]) Clone() (graph.Graph[K, T], error) { cloned, err := g.Graph.Clone() if err != nil { @@ -106,11 +79,3 @@ func (g LoggingGraph[K, T]) Clone() (graph.Graph[K, T], error) { } return LoggingGraph[K, T]{Log: g.Log, Graph: cloned}, nil } - -func (g LoggingGraph[K, T]) Order() (int, error) { - return g.Graph.Order() -} - -func (g LoggingGraph[K, T]) Size() (int, error) { - return g.Graph.Size() -}