diff --git a/pkg/devcontainer/feature/extend.go b/pkg/devcontainer/feature/extend.go index c347d01c9..21dc605a0 100644 --- a/pkg/devcontainer/feature/extend.go +++ b/pkg/devcontainer/feature/extend.go @@ -754,6 +754,7 @@ func addHardDependencies( if err := g.AddEdge(depKey, featureKey); err != nil { return err } + g.MarkEdgeHard(depKey, featureKey) } } return nil @@ -772,10 +773,26 @@ func addSoftDependencies( continue } + if depKey == featureKey { + continue + } + if hasHardDependency(feature, id, normalizedID) { continue } + if g.IsReachableViaHardEdges(featureKey, depKey) { + continue + } + + if g.IsReachable(featureKey, depKey) { + return fmt.Errorf( + "circular dependency detected: feature %q has installsAfter dependency on %q which creates a cycle", + feature.ConfigID, + normalizedID, + ) + } + if err := g.AddEdge(depKey, featureKey); err != nil { return err } diff --git a/pkg/devcontainer/feature/extend_test.go b/pkg/devcontainer/feature/extend_test.go index 0e8cddc3f..5cd0ca321 100644 --- a/pkg/devcontainer/feature/extend_test.go +++ b/pkg/devcontainer/feature/extend_test.go @@ -269,6 +269,172 @@ func (suite *ExtendTestSuite) TestFeatureOrderWithDependencies_SameDependsOnAndI suite.Equal("dev-code", installationOrder[1].ConfigID) } +func (suite *ExtendTestSuite) TestFeatureOrder_SelfReferenceInInstallsAfter() { + selfRefID := normalizeFeatureID("self-ref-feature") + otherID := normalizeFeatureID("other-feature") + features := []*config.FeatureSet{ + { + ConfigID: selfRefID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{"self-ref-feature"}, + }, + }, + { + ConfigID: otherID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{}, + }, + }, + } + + installationOrder, err := getOrderedFeatureSets(features) + suite.Require().NoError(err) + suite.Len(installationOrder, 2) +} + +func (suite *ExtendTestSuite) TestFeatureOrder_ForwardReferenceInInstallsAfter() { + featureAID := normalizeFeatureID(testFeatureA) + featureBID := normalizeFeatureID(testFeatureB) + features := []*config.FeatureSet{ + { + ConfigID: featureAID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{testFeatureB}, + }, + }, + { + ConfigID: featureBID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{}, + }, + }, + } + + installationOrder, err := getOrderedFeatureSets(features) + suite.Require().NoError(err) + suite.Len(installationOrder, 2) + suite.Equal(featureBID, installationOrder[0].ConfigID) + suite.Equal(featureAID, installationOrder[1].ConfigID) +} + +func (suite *ExtendTestSuite) TestFeatureOrder_MutualInstallsAfterProducesError() { + featureAID := normalizeFeatureID(testFeatureA) + featureBID := normalizeFeatureID(testFeatureB) + features := []*config.FeatureSet{ + { + ConfigID: featureAID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{testFeatureB}, + }, + }, + { + ConfigID: featureBID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{testFeatureA}, + }, + }, + } + + _, err := getOrderedFeatureSets(features) + suite.Error(err) + suite.Contains(err.Error(), "circular dependency detected") +} + +func (suite *ExtendTestSuite) TestFeatureOrder_ThreeNodePureSoftCycleProducesError() { + featureAID := normalizeFeatureID(testFeatureA) + featureBID := normalizeFeatureID(testFeatureB) + featureCID := normalizeFeatureID(testFeatureC) + features := []*config.FeatureSet{ + { + ConfigID: featureAID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{testFeatureB}, + }, + }, + { + ConfigID: featureBID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{testFeatureC}, + }, + }, + { + ConfigID: featureCID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{testFeatureA}, + }, + }, + } + + _, err := getOrderedFeatureSets(features) + suite.Error(err) + suite.Contains(err.Error(), "circular dependency detected") +} + +func (suite *ExtendTestSuite) TestFeatureOrder_InstallsAfterWithHardDepDoesNotCycle() { + featureAID := normalizeFeatureID(testFeatureA) + featureBID := normalizeFeatureID(testFeatureB) + features := []*config.FeatureSet{ + { + ConfigID: featureAID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{ + testFeatureB: map[string]any{}, + }, + InstallsAfter: []string{testFeatureB}, + }, + }, + { + ConfigID: featureBID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{testFeatureA}, + }, + }, + } + + installationOrder, err := getOrderedFeatureSets(features) + suite.Require().NoError(err) + suite.Len(installationOrder, 2) + suite.Equal(featureBID, installationOrder[0].ConfigID) + suite.Equal(featureAID, installationOrder[1].ConfigID) +} + +func (suite *ExtendTestSuite) TestFeatureOrder_TrueCircularDependencyStillDetected() { + featureAID := normalizeFeatureID(testFeatureA) + featureBID := normalizeFeatureID(testFeatureB) + features := []*config.FeatureSet{ + { + ConfigID: featureAID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{ + testFeatureB: map[string]any{}, + }, + }, + }, + { + ConfigID: featureBID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{ + testFeatureA: map[string]any{}, + }, + }, + }, + } + + _, err := getOrderedFeatureSets(features) + suite.Error(err) + suite.Contains(err.Error(), "circular") +} + func (suite *ExtendTestSuite) TestComputeFeatureOrder_NoOverride() { devContainer := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ diff --git a/pkg/devcontainer/graph/graph.go b/pkg/devcontainer/graph/graph.go index f19a2618e..5c4fd496f 100644 --- a/pkg/devcontainer/graph/graph.go +++ b/pkg/devcontainer/graph/graph.go @@ -9,16 +9,18 @@ import ( ) type Graph[T comparable] struct { - nodes map[string]T - edges map[string][]string - inDegree map[string]int + nodes map[string]T + edges map[string][]string + inDegree map[string]int + hardEdges map[string]map[string]bool } func NewGraph[T comparable]() *Graph[T] { return &Graph[T]{ - nodes: make(map[string]T), - edges: make(map[string][]string), - inDegree: make(map[string]int), + nodes: make(map[string]T), + edges: make(map[string][]string), + inDegree: make(map[string]int), + hardEdges: make(map[string]map[string]bool), } } @@ -183,6 +185,60 @@ func (g *Graph[T]) RemoveChildren(id string) error { return nil } +func (g *Graph[T]) IsReachable(from, to string) bool { + if from == to { + return true + } + visited := make(map[string]bool) + queue := []string{from} + for len(queue) > 0 { + current := queue[0] + queue = queue[1:] + for _, neighbor := range g.edges[current] { + if neighbor == to { + return true + } + if !visited[neighbor] { + visited[neighbor] = true + queue = append(queue, neighbor) + } + } + } + return false +} + +func (g *Graph[T]) MarkEdgeHard(from, to string) { + if g.hardEdges[from] == nil { + g.hardEdges[from] = make(map[string]bool) + } + g.hardEdges[from][to] = true +} + +func (g *Graph[T]) IsReachableViaHardEdges(from, to string) bool { + if from == to { + return true + } + visited := make(map[string]bool) + queue := []string{from} + for len(queue) > 0 { + current := queue[0] + queue = queue[1:] + for _, neighbor := range g.edges[current] { + if !g.hardEdges[current][neighbor] { + continue + } + if neighbor == to { + return true + } + if !visited[neighbor] { + visited[neighbor] = true + queue = append(queue, neighbor) + } + } + } + return false +} + func (g *Graph[T]) HasNode(id string) bool { _, exists := g.nodes[id] return exists diff --git a/pkg/devcontainer/graph/graph_test.go b/pkg/devcontainer/graph/graph_test.go index 55c0f108a..523384c88 100644 --- a/pkg/devcontainer/graph/graph_test.go +++ b/pkg/devcontainer/graph/graph_test.go @@ -501,6 +501,25 @@ func (suite *GraphTestSuite) TestEdgeCases() { } } +func (suite *GraphTestSuite) TestIsReachable() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddNode("C", "dataC")) + suite.Require().NoError(suite.graph.AddNode("D", "dataD")) + + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + suite.Require().NoError(suite.graph.AddEdge("B", "C")) + + suite.True(suite.graph.IsReachable("A", "B")) + suite.True(suite.graph.IsReachable("A", "C")) + suite.True(suite.graph.IsReachable("B", "C")) + suite.False(suite.graph.IsReachable("C", "A")) + suite.False(suite.graph.IsReachable("C", "B")) + suite.False(suite.graph.IsReachable("A", "D")) + suite.False(suite.graph.IsReachable("D", "A")) + suite.True(suite.graph.IsReachable("A", "A")) +} + func (suite *GraphTestSuite) TestLargeGraph() { nodeCount := 100 diff --git a/pkg/devcontainer/metadata/metadata.go b/pkg/devcontainer/metadata/metadata.go index 85346e115..d329c60be 100644 --- a/pkg/devcontainer/metadata/metadata.go +++ b/pkg/devcontainer/metadata/metadata.go @@ -73,12 +73,11 @@ func FeatureConfigToImageMetadata(feature *config.FeatureConfig) *config.ImageMe Customizations: feature.Customizations, }, NonComposeBase: config.NonComposeBase{ - ContainerEnv: feature.ContainerEnv, - Mounts: feature.Mounts, - Init: feature.Init, - Privileged: feature.Privileged, - CapAdd: feature.CapAdd, - SecurityOpt: feature.SecurityOpt, + Mounts: feature.Mounts, + Init: feature.Init, + Privileged: feature.Privileged, + CapAdd: feature.CapAdd, + SecurityOpt: feature.SecurityOpt, }, } } diff --git a/pkg/devcontainer/metadata/metadata_test.go b/pkg/devcontainer/metadata/metadata_test.go index 7c9436796..37693a9ad 100644 --- a/pkg/devcontainer/metadata/metadata_test.go +++ b/pkg/devcontainer/metadata/metadata_test.go @@ -8,7 +8,7 @@ import ( "github.com/devsy-org/devsy/pkg/devcontainer/config" ) -func TestFeatureConfigToImageMetadata_IncludesContainerEnv(t *testing.T) { +func TestFeatureConfigToImageMetadata_ExcludesContainerEnv(t *testing.T) { feature := &config.FeatureConfig{ ContainerEnv: map[string]string{ "FOO": "bar", @@ -18,11 +18,11 @@ func TestFeatureConfigToImageMetadata_IncludesContainerEnv(t *testing.T) { got := FeatureConfigToImageMetadata(feature) - if got.ContainerEnv == nil { - t.Fatal("expected ContainerEnv to be included in per-feature metadata, got nil") - } - if got.ContainerEnv["FOO"] != "bar" || got.ContainerEnv["BAZ"] != "qux" { - t.Fatalf("expected ContainerEnv to contain FOO=bar and BAZ=qux, got %v", got.ContainerEnv) + if got.ContainerEnv != nil { + t.Fatalf( + "expected ContainerEnv to NOT be in per-feature metadata (applied via Dockerfile ENV only), got %v", + got.ContainerEnv, + ) } }