diff --git a/e2e/tests/up/testdata/docker-varsub-scope/.devcontainer.json b/e2e/tests/up/testdata/docker-varsub-scope/.devcontainer.json new file mode 100644 index 000000000..0a430326c --- /dev/null +++ b/e2e/tests/up/testdata/docker-varsub-scope/.devcontainer.json @@ -0,0 +1,21 @@ +{ + "name": "VarSubScope", + "image": "ghcr.io/devsy-org/test-images/go:1", + "containerEnv": { + "SHOULD_BE_LITERAL_CWF": "${containerWorkspaceFolder}", + "SHOULD_BE_LITERAL_CWFB": "${containerWorkspaceFolderBasename}", + "SHOULD_BE_LITERAL_CENV": "${containerEnv:PATH}", + "SHOULD_RESOLVE_LOCAL": "${localWorkspaceFolder}" + }, + "remoteEnv": { + "REMOTE_CWF": "${containerWorkspaceFolder}", + "REMOTE_CWFB": "${containerWorkspaceFolderBasename}", + "REMOTE_CENV": "${containerEnv:PATH}", + "REMOTE_LOCAL": "${localWorkspaceFolder}" + }, + "postCreateCommand": [ + "sh", + "-c", + "echo -n \"${SHOULD_BE_LITERAL_CWF}\" > $HOME/container-env-cwf.out && echo -n \"${SHOULD_BE_LITERAL_CWFB}\" > $HOME/container-env-cwfb.out && echo -n \"${SHOULD_BE_LITERAL_CENV}\" > $HOME/container-env-cenv.out && echo -n \"${SHOULD_RESOLVE_LOCAL}\" > $HOME/container-env-local.out && echo -n \"${REMOTE_CWF}\" > $HOME/remote-env-cwf.out && echo -n \"${REMOTE_CWFB}\" > $HOME/remote-env-cwfb.out && echo -n \"${REMOTE_CENV}\" > $HOME/remote-env-cenv.out && echo -n \"${REMOTE_LOCAL}\" > $HOME/remote-env-local.out" + ] +} diff --git a/e2e/tests/up/varsub_scope.go b/e2e/tests/up/varsub_scope.go new file mode 100644 index 000000000..ad8546364 --- /dev/null +++ b/e2e/tests/up/varsub_scope.go @@ -0,0 +1,123 @@ +package up + +import ( + "context" + "os" + "path/filepath" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = ginkgo.Describe( + "testing variable substitution phase-aware scoping", + ginkgo.Label("up-varsub-scope"), + func() { + var dtc *dockerTestContext + + ginkgo.BeforeEach(func(ctx context.Context) { + var err error + dtc = &dockerTestContext{} + dtc.initialDir, err = os.Getwd() + framework.ExpectNoError(err) + + dtc.f, err = setupDockerProvider( + filepath.Join(dtc.initialDir, "bin"), "docker", + ) + framework.ExpectNoError(err) + }) + + ginkgo.It( + "containerEnv preserves container-scoped vars as literals", + func(ctx context.Context) { + tempDir, err := dtc.setupAndUp(ctx, "tests/up/testdata/docker-varsub-scope") + framework.ExpectNoError(err) + + workspace, err := dtc.f.FindWorkspace(ctx, tempDir) + framework.ExpectNoError(err) + + // containerEnv: ${containerWorkspaceFolder} should be preserved as literal + // and resolved at runtime by the shell from the actual env var. + cwf, err := dtc.execSSHCapture(ctx, workspace.ID, "cat $HOME/container-env-cwf.out") + framework.ExpectNoError(err) + gomega.Expect(cwf).To(gomega.ContainSubstring("/workspaces/"), + "containerEnv containerWorkspaceFolder should resolve at runtime via shell") + + // containerEnv: ${containerWorkspaceFolderBasename} should be preserved as literal + cwfb, err := dtc.execSSHCapture( + ctx, + workspace.ID, + "cat $HOME/container-env-cwfb.out", + ) + framework.ExpectNoError(err) + gomega.Expect(cwfb).To(gomega.Equal(filepath.Base(tempDir)), + "containerEnv containerWorkspaceFolderBasename should resolve at runtime via shell") + + // containerEnv: ${containerEnv:PATH} should remain literal + cenv, err := dtc.execSSHCapture( + ctx, + workspace.ID, + "cat $HOME/container-env-cenv.out", + ) + framework.ExpectNoError(err) + gomega.Expect(cenv).To(gomega.ContainSubstring("/usr/local/bin"), + "containerEnv containerEnv:PATH should resolve at runtime via SubstituteContainerEnv") + + // containerEnv: ${localWorkspaceFolder} should resolve during substitution + local, err := dtc.execSSHCapture( + ctx, + workspace.ID, + "cat $HOME/container-env-local.out", + ) + framework.ExpectNoError(err) + gomega.Expect(framework.CleanString(local)). + To(gomega.Equal(framework.CleanString(tempDir)), + "containerEnv localWorkspaceFolder should resolve at substitution time") + + // remoteEnv: ${containerWorkspaceFolder} should fully resolve + remoteCwf, err := dtc.execSSHCapture( + ctx, + workspace.ID, + "cat $HOME/remote-env-cwf.out", + ) + framework.ExpectNoError(err) + gomega.Expect(framework.CleanString(remoteCwf)). + To(gomega.ContainSubstring("/workspaces/"), + "remoteEnv containerWorkspaceFolder should resolve at substitution time") + + // remoteEnv: ${containerWorkspaceFolderBasename} should fully resolve + remoteCwfb, err := dtc.execSSHCapture( + ctx, + workspace.ID, + "cat $HOME/remote-env-cwfb.out", + ) + framework.ExpectNoError(err) + gomega.Expect(remoteCwfb).To(gomega.Equal(filepath.Base(tempDir)), + "remoteEnv containerWorkspaceFolderBasename should resolve at substitution time") + + // remoteEnv: ${containerEnv:PATH} resolved via SubstituteContainerEnv + remoteCenv, err := dtc.execSSHCapture( + ctx, + workspace.ID, + "cat $HOME/remote-env-cenv.out", + ) + framework.ExpectNoError(err) + gomega.Expect(remoteCenv).To(gomega.ContainSubstring("/usr/local/bin"), + "remoteEnv containerEnv:PATH should resolve via SubstituteContainerEnv") + + // remoteEnv: ${localWorkspaceFolder} should fully resolve + remoteLocal, err := dtc.execSSHCapture( + ctx, + workspace.ID, + "cat $HOME/remote-env-local.out", + ) + framework.ExpectNoError(err) + gomega.Expect(framework.CleanString(remoteLocal)). + To(gomega.Equal(framework.CleanString(tempDir)), + "remoteEnv localWorkspaceFolder should resolve at substitution time") + }, + ginkgo.SpecTimeout(framework.TimeoutShort()), + ) + }, +) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 6a0800931..2a14c3826 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -176,7 +176,14 @@ func (r *runner) substitute( WorkspaceMount: workspaceMount, } - // substitute & load + // Substitute applies phase-aware variable scoping per the devcontainer spec + // (https://containers.dev/implementors/reference/ — "Variables in devcontainer.json"): + // - Pre-container fields (containerEnv) only resolve local-scoped variables + // (devcontainerId, localEnv, localWorkspaceFolder, localWorkspaceFolderBasename). + // - Post-container fields (remoteEnv, lifecycle commands, etc.) additionally + // resolve containerWorkspaceFolder and containerWorkspaceFolderBasename. + // - containerEnv references (${containerEnv:VAR}) are resolved later via + // SubstituteContainerEnv after the container is running. parsedConfig := &config.DevContainerConfig{} err := config.Substitute(substitutionContext, rawParsedConfig, parsedConfig) if err != nil { diff --git a/pkg/devcontainer/config/substitute.go b/pkg/devcontainer/config/substitute.go index e09781c41..59a38670a 100644 --- a/pkg/devcontainer/config/substitute.go +++ b/pkg/devcontainer/config/substitute.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "maps" "math/big" "path/filepath" "regexp" @@ -13,7 +14,10 @@ import ( "github.com/devsy-org/devsy/pkg/hash" ) -const devContainerIDLength = 20 +const ( + devContainerIDLength = 20 + containerEnvField = "containerEnv" +) type ReplaceFunction func(match, variable string, args []string) string @@ -35,6 +39,11 @@ type SubstitutionContext struct { GidMap []string `json:"GidMap,omitempty"` } +// preContainerFields lists devcontainer.json keys that are evaluated before +// the container exists. These fields must not resolve container-scoped +// variables (containerWorkspaceFolder, containerWorkspaceFolderBasename). +var preContainerFields = []string{containerEnvField} + func Substitute(substitutionCtx *SubstitutionContext, config any, out any) error { newVal := map[string]any{} err := Convert(config, &newVal) @@ -60,9 +69,27 @@ func Substitute(substitutionCtx *SubstitutionContext, config any, out any) error }, ) } - retVal := substitute0(newVal, func(match, variable string, args []string) string { + + // Two-pass substitution: pre-container fields get a restricted replacer + // that preserves container-scoped variables as literals. + fullReplace := func(match, variable string, args []string) string { return replaceWithContext(isWindows, substitutionCtx, match, variable, args) - }) + } + preFieldValues := map[string]any{} + for _, key := range preContainerFields { + if fieldVal, ok := newVal[key]; ok { + preFieldValues[key] = substitute0(fieldVal, restrictedReplace(fullReplace)) + delete(newVal, key) + } + } + + // Full substitution for remaining fields. + retVal := substitute0(newVal, fullReplace) + + // Merge pre-container fields back into the result. + if retMap, ok := retVal.(map[string]any); ok { + maps.Copy(retMap, preFieldValues) + } err = Convert(retVal, out) if err != nil { @@ -98,7 +125,7 @@ func replaceWithContainerEnv( args []string, ) string { switch variable { - case "containerEnv": + case containerEnvField: return lookupValue(false, containerEnv, args, match) default: return match @@ -139,13 +166,27 @@ func replaceWithContext( return filepath.Base(substitutionCtx.ContainerWorkspaceFolder) } return match - case "containerEnv": + case containerEnvField: return match default: return match } } +// restrictedReplace wraps a ReplaceFunction to preserve container-scoped +// variables (containerWorkspaceFolder, containerWorkspaceFolderBasename, +// containerEnv) as literals for pre-container field substitution. +func restrictedReplace(fallback ReplaceFunction) ReplaceFunction { + return func(match, variable string, args []string) string { + switch variable { + case "containerWorkspaceFolder", "containerWorkspaceFolderBasename", containerEnvField: + return match + default: + return fallback(match, variable, args) + } + } +} + func lookupValue(isWindows bool, env map[string]string, args []string, match string) string { if len(args) > 0 { envVariableName := args[0] diff --git a/pkg/devcontainer/config/substitute_test.go b/pkg/devcontainer/config/substitute_test.go index 948db9ce0..1341d8b61 100644 --- a/pkg/devcontainer/config/substitute_test.go +++ b/pkg/devcontainer/config/substitute_test.go @@ -83,7 +83,7 @@ func TestReplaceWithContextPreservesContainerEnv(t *testing.T) { ctx := &SubstitutionContext{} match := "${containerEnv:PATH}" result := replaceWithContext( - false, ctx, match, "containerEnv", []string{"PATH"}, + false, ctx, match, containerEnvField, []string{testPATHKey}, ) if result != match { t.Errorf( @@ -172,7 +172,19 @@ func TestResolveStringDefaultWithColons(t *testing.T) { } } -const testWorkspaceFolder = "/home/user/project" +const ( + testWorkspaceFolder = "/home/user/project" + testContainerWorkspaceFolder = "/workspaces/project" + testDevContainerID = "abc123" + testContainerWorkspaceFolderVar = "${containerWorkspaceFolder}" + testContainerWorkspaceFolderBasenameVar = "${containerWorkspaceFolderBasename}" + testLocalWorkspaceFolderVar = "${localWorkspaceFolder}" + testCWFKey = "CWF" + testLOCKey = "LOC" + testContainerWorkspaceFolderName = "containerWorkspaceFolder" + testContainerWorkspaceFolderBasenameName = "containerWorkspaceFolderBasename" + testPATHKey = "PATH" +) func TestDeriveDevContainerID(t *testing.T) { h := sha256.Sum256([]byte(testWorkspaceFolder)) @@ -221,3 +233,219 @@ func TestResolveDevContainerID(t *testing.T) { t.Errorf("ResolveDevContainerID() = %q, want %q (spec-based ID)", got, want) } } + +type scopeTestConfig struct { + ContainerEnv map[string]string `json:"containerEnv,omitempty"` + RemoteEnv map[string]*string `json:"remoteEnv,omitempty"` +} + +func scopeTestCtx() *SubstitutionContext { + return &SubstitutionContext{ + DevContainerID: testDevContainerID, + LocalWorkspaceFolder: testWorkspaceFolder, + ContainerWorkspaceFolder: testContainerWorkspaceFolder, + Env: map[string]string{"MY_VAR": "hello"}, + } +} + +func TestSubstituteContainerEnvScoping(t *testing.T) { + ctx := scopeTestCtx() + tests := []struct { + name string + input map[string]string + want map[string]string + }{ + { + name: "preserves containerWorkspaceFolder", + input: map[string]string{"V": testContainerWorkspaceFolderVar}, + want: map[string]string{"V": testContainerWorkspaceFolderVar}, + }, + { + name: "preserves containerWorkspaceFolderBasename", + input: map[string]string{"V": testContainerWorkspaceFolderBasenameVar}, + want: map[string]string{"V": testContainerWorkspaceFolderBasenameVar}, + }, + { + name: "preserves containerEnv references", + input: map[string]string{"V": "${containerEnv:PATH}"}, + want: map[string]string{"V": "${containerEnv:PATH}"}, + }, + { + name: "resolves local-scoped variables", + input: map[string]string{ + "LOCAL": testLocalWorkspaceFolderVar, + "BASE": "${localWorkspaceFolderBasename}", + "ENV": "${localEnv:MY_VAR}", + "DEVID": "${devcontainerId}", + }, + want: map[string]string{ + "LOCAL": testWorkspaceFolder, + "BASE": "project", + "ENV": "hello", + "DEVID": testDevContainerID, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var out scopeTestConfig + err := Substitute(ctx, scopeTestConfig{ContainerEnv: tt.input}, &out) + if err != nil { + t.Fatalf("Substitute() error: %v", err) + } + assertContainerEnv(t, out.ContainerEnv, tt.want) + }) + } +} + +func TestSubstituteRemoteEnvScoping(t *testing.T) { + ctx := scopeTestCtx() + tests := []struct { + name string + input map[string]*string + want map[string]*string + }{ + { + name: "resolves containerWorkspaceFolder", + input: map[string]*string{"V": strPtr("/prefix${containerWorkspaceFolder}")}, + want: map[string]*string{"V": strPtr("/prefix/workspaces/project")}, + }, + { + name: "resolves containerWorkspaceFolderBasename", + input: map[string]*string{"V": strPtr(testContainerWorkspaceFolderBasenameVar)}, + want: map[string]*string{"V": strPtr("project")}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var out scopeTestConfig + err := Substitute(ctx, scopeTestConfig{RemoteEnv: tt.input}, &out) + if err != nil { + t.Fatalf("Substitute() error: %v", err) + } + assertRemoteEnv(t, out.RemoteEnv, tt.want) + }) + } +} + +func TestSubstituteMixedScoping(t *testing.T) { + ctx := scopeTestCtx() + input := scopeTestConfig{ + ContainerEnv: map[string]string{ + testCWFKey: testContainerWorkspaceFolderVar, + testLOCKey: testLocalWorkspaceFolderVar, + }, + RemoteEnv: map[string]*string{ + testCWFKey: strPtr(testContainerWorkspaceFolderVar), + testLOCKey: strPtr(testLocalWorkspaceFolderVar), + }, + } + var out scopeTestConfig + err := Substitute(ctx, input, &out) + if err != nil { + t.Fatalf("Substitute() error: %v", err) + } + assertContainerEnv(t, out.ContainerEnv, map[string]string{ + testCWFKey: testContainerWorkspaceFolderVar, + testLOCKey: testWorkspaceFolder, + }) + assertRemoteEnv(t, out.RemoteEnv, map[string]*string{ + testCWFKey: strPtr(testContainerWorkspaceFolder), + testLOCKey: strPtr(testWorkspaceFolder), + }) +} + +func TestRestrictedReplacePreservesContainerVars(t *testing.T) { + ctx := &SubstitutionContext{ + ContainerWorkspaceFolder: testContainerWorkspaceFolder, + LocalWorkspaceFolder: testWorkspaceFolder, + Env: map[string]string{"HOME": "/root"}, + } + fullReplace := func(match, variable string, args []string) string { + return replaceWithContext(false, ctx, match, variable, args) + } + restricted := restrictedReplace(fullReplace) + + tests := []struct { + name string + match string + variable string + args []string + want string + }{ + { + name: "preserves containerWorkspaceFolder", + match: testContainerWorkspaceFolderVar, + variable: testContainerWorkspaceFolderName, + want: testContainerWorkspaceFolderVar, + }, + { + name: "preserves containerWorkspaceFolderBasename", + match: testContainerWorkspaceFolderBasenameVar, + variable: testContainerWorkspaceFolderBasenameName, + want: testContainerWorkspaceFolderBasenameVar, + }, + { + name: "preserves containerEnv", + match: "${containerEnv:PATH}", + variable: containerEnvField, + args: []string{testPATHKey}, + want: "${containerEnv:PATH}", + }, + { + name: "resolves localWorkspaceFolder", + match: testLocalWorkspaceFolderVar, + variable: "localWorkspaceFolder", + want: testWorkspaceFolder, + }, + { + name: "resolves localEnv", + match: "${localEnv:HOME}", + variable: "localEnv", + args: []string{"HOME"}, + want: "/root", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := restricted(tt.match, tt.variable, tt.args) + if got != tt.want { + t.Errorf("restrictedReplace() = %q, want %q", got, tt.want) + } + }) + } +} + +func assertContainerEnv(t *testing.T, got, want map[string]string) { + t.Helper() + if want == nil { + return + } + for k, wantVal := range want { + gotVal, ok := got[k] + if !ok { + t.Errorf("containerEnv[%q] missing", k) + } else if gotVal != wantVal { + t.Errorf("containerEnv[%q] = %q, want %q", k, gotVal, wantVal) + } + } +} + +func assertRemoteEnv(t *testing.T, got, want map[string]*string) { + t.Helper() + if want == nil { + return + } + for k, wantVal := range want { + gotVal, ok := got[k] + switch { + case !ok: + t.Errorf("remoteEnv[%q] missing", k) + case gotVal == nil: + t.Errorf("remoteEnv[%q] = nil, want %q", k, *wantVal) + case *gotVal != *wantVal: + t.Errorf("remoteEnv[%q] = %q, want %q", k, *gotVal, *wantVal) + } + } +}