fix(reset): Improve cycle detector

Signed-off-by: Suleiman Dibirov <[email protected]>

Author: idsulik

loader/reset.go

@@ -28,11 +28,12 @@ import (
 type ResetProcessor struct {
 	target       interface{}
 	paths        []tree.Path
-	visitedNodes map[*yaml.Node]string
+	visitedNodes map[*yaml.Node][]string
 }
 
 // UnmarshalYAML implement yaml.Unmarshaler
 func (p *ResetProcessor) UnmarshalYAML(value *yaml.Node) error {
+	p.visitedNodes = make(map[*yaml.Node][]string)
 	resolved, err := p.resolveReset(value, tree.NewPath())
 	p.visitedNodes = nil
 	if err != nil {
@@ -144,20 +145,46 @@ func (p *ResetProcessor) applyNullOverrides(target any, path tree.Path) error {
 }
 
 func (p *ResetProcessor) checkForCycle(node *yaml.Node, path tree.Path) error {
-	if p.visitedNodes == nil {
-		p.visitedNodes = make(map[*yaml.Node]string)
-	}
+	paths := p.visitedNodes[node]
+	pathStr := path.String()
 
-	// Check for cycle by seeing if the node has already been visited at this path
-	if previousPath, found := p.visitedNodes[node]; found {
-		// If the current node has been visited, we have a cycle if the previous path is a prefix
-		if strings.HasPrefix(path.String(), strings.TrimRight(previousPath, "<<")) {
-			return fmt.Errorf("cycle detected at path: %s", previousPath)
+	for _, prevPath := range paths {
+		// If we're visiting the exact same path, it's not a cycle
+		if pathStr == prevPath {
+			continue
 		}
-	}
 
-	// Mark the current node as visited
-	p.visitedNodes[node] = path.String()
+		// If either path is using a merge key, it's legitimate YAML merging
+		if strings.Contains(prevPath, "<<") || strings.Contains(pathStr, "<<") {
+			continue
+		}
 
+		// Only consider it a cycle if one path is contained within the other
+		// and they're not in different service definitions
+		if (strings.HasPrefix(pathStr, prevPath+".") ||
+			strings.HasPrefix(prevPath, pathStr+".")) &&
+			!areInDifferentServices(pathStr, prevPath) {
+			return fmt.Errorf("cycle detected: node at path %s references node at path %s", pathStr, prevPath)
+		}
+	}
+
+	p.visitedNodes[node] = append(paths, pathStr)
 	return nil
 }
+
+// areInDifferentServices checks if two paths are in different service definitions
+func areInDifferentServices(path1, path2 string) bool {
+	// Split paths into components
+	parts1 := strings.Split(path1, ".")
+	parts2 := strings.Split(path2, ".")
+
+	// Look for the services component and compare the service names
+	for i := 0; i < len(parts1) && i < len(parts2); i++ {
+		if parts1[i] == "services" && i+1 < len(parts1) &&
+			parts2[i] == "services" && i+1 < len(parts2) {
+			// If they're different services, it's not a cycle
+			return parts1[i+1] != parts2[i+1]
+		}
+	}
+	return false
+}

loader/reset_test.go

@@ -116,7 +116,7 @@ services:
 		{
 			name: "no cycle 2",
 			config: `
-name: blah
+name: no_cycle_2
 x-templates:
   x-gluetun: &gluetun
     environment: &gluetun_env
@@ -129,6 +129,30 @@ x-templates:
     <<: *gluetun_pia
     environment:
       <<: *gluetun_env_pia
+`,
+			expectError: false,
+			errorMsg:    "",
+		},
+		{
+			name: "no cycle 3",
+			config: `
+name: no_cycle_3
+x-common:
+  &common
+  restart: unless-stopped
+
+services:
+  backend:
+    <<: *common
+    image: alpine:latest
+
+  backend-static:
+    <<: *common
+    image: alpine:latest
+
+  backend-worker:
+    <<: *common
+    image: alpine:latest
 `,
 			expectError: false,
 			errorMsg:    "",
@@ -141,7 +165,7 @@ x-healthcheck: &healthcheck
     <<: *healthcheck
 `,
 			expectError: true,
-			errorMsg:    "cycle detected at path: x-healthcheck.egress-service",
+			errorMsg:    "cycle detected: node at path x-healthcheck.egress-service.egress-service references node at path x-healthcheck.egress-service",
 		},
 	}