Fix graph cycle detection. Improve graph validation for tests. (#173)

* Add more validate stuff.

* Add logging

* Fix graph cycle stuff.

* Remove unused errors

* Remove test that doesn't test anything new.

---------

Co-authored-by: Marcos Gaeta <mgaeta89@gmail.com>

pkg/sync/expand/cycle.go

@@ -55,7 +55,10 @@ func (g *EntitlementGraph) removeNode(nodeID int) {
  // Delete from reverse mapping.
  if node, ok := g.Nodes[nodeID]; ok {
  for _, entitlementID := range node.EntitlementIDs {
- delete(g.EntitlementsToNodes, entitlementID)
+ entNodeId, ok := g.EntitlementsToNodes[entitlementID]
+ if ok && entNodeId == nodeID {
+ delete(g.EntitlementsToNodes, entitlementID)
+ }
  }
  }
 

pkg/sync/expand/graph.go

@@ -37,8 +37,8 @@ type Node struct {
 // This is because the graph can have cycles, and we address them by reducing
 // _all_ nodes in a cycle into a single node.
 type EntitlementGraph struct {
- NextNodeID int `json:"node_count"`  // Automatically incremented so that each node has a unique ID.
- NextEdgeID int `json:"edge_count"`  // Automatically incremented so that each edge has a unique ID.
+ NextNodeID int `json:"next_node_id"` // Automatically incremented so that each node has a unique ID.
+ NextEdgeID int `json:"next_edge_id"` // Automatically incremented so that each edge has a unique ID.
  Nodes map[int]Node `json:"nodes"` // The mapping of all node IDs to nodes.
  EntitlementsToNodes map[string]int `json:"entitlements_to_nodes"` // Internal mapping of entitlements to nodes for quicker lookup.
  SourcesToDestinations map[int]map[int]int `json:"sources_to_destinations"` // Internal mapping of outgoing edges by node ID.

pkg/sync/expand/validate.go

@@ -3,6 +3,7 @@ package expand
 import (
  "errors"
  "fmt"
+ "slices"
  "strings"
 )
 
@@ -66,13 +67,21 @@ func (g *EntitlementGraph) Str() string {
 
 // validateEdges validates that for every edge, both nodes actually exists.
 func (g *EntitlementGraph) validateEdges() error {
- for _, edge := range g.Edges {
+ for edgeId, edge := range g.Edges {
  if _, ok := g.Nodes[edge.SourceID]; !ok {
  return ErrNoEntitlement
  }
  if _, ok := g.Nodes[edge.DestinationID]; !ok {
  return ErrNoEntitlement
  }
+
+ if g.SourcesToDestinations[edge.SourceID][edge.DestinationID] != edgeId {
+ return fmt.Errorf("edge %v does not match source %v to destination %v", edgeId, edge.SourceID, edge.DestinationID)
+ }
+
+ if g.DestinationsToSources[edge.DestinationID][edge.SourceID] != edgeId {
+ return fmt.Errorf("edge %v does not match destination %v to source %v", edgeId, edge.DestinationID, edge.SourceID)
+ }
  }
  return nil
 }
@@ -91,6 +100,23 @@ func (g *EntitlementGraph) validateNodes() error {
  return fmt.Errorf("entitlement %v is in multiple nodes: %v %v", entID, nodeID, seenEntitlements[entID])
  }
  seenEntitlements[entID] = nodeID
+ entNodeId, ok := g.EntitlementsToNodes[entID]
+ if !ok {
+ return fmt.Errorf("entitlement %v is not in EntitlementsToNodes. should be in node %v", entID, nodeID)
+ }
+ if entNodeId != nodeID {
+ return fmt.Errorf("entitlement %v is in node %v but should be in node %v", entID, entNodeId, nodeID)
+ }
+ }
+ }
+
+ for entID, nodeID := range g.EntitlementsToNodes {
+ node, ok := g.Nodes[nodeID]
+ if !ok {
+ return fmt.Errorf("entitlement %v is in EntitlementsToNodes but not in Nodes", entID)
+ }
+ if !slices.Contains(node.EntitlementIDs, entID) {
+ return fmt.Errorf("entitlement %v is in EntitlementsToNodes but not in node %v", entID, nodeID)
  }
  }
  return nil

pkg/sync/syncer.go

@@ -815,7 +815,11 @@ func (s *syncer) SyncGrantExpansion(ctx context.Context) error {
  if entitlementGraph.Loaded {
  cycle := entitlementGraph.GetFirstCycle()
  if cycle != nil {
- l.Warn("cycle detected in entitlement graph", zap.Any("cycle", cycle))
+ l.Warn(
+ "cycle detected in entitlement graph",
+ zap.Any("cycle", cycle),
+ zap.Any("initial graph", entitlementGraph),
+ )
  if dontFixCycles {
  return fmt.Errorf("cycles detected in entitlement graph")
  }
@@ -1302,7 +1306,11 @@ func (s *syncer) expandGrantsForEntitlements(ctx context.Context) error {
  }
 
  if graph.Depth > maxDepth {
- l.Error("expandGrantsForEntitlements: exceeded max depth", zap.Any("graph", graph), zap.Int("max_depth", maxDepth))
+ l.Error(
+ "expandGrantsForEntitlements: exceeded max depth",
+ zap.Any("graph", graph),
+ zap.Int("max_depth", maxDepth),
+ )
  s.state.FinishAction(ctx)
  return fmt.Errorf("exceeded max depth")
  }