Fix containerEnv substitution (microsoft/vscode-remote-release#10033)

devcontainers · Jul 9, 2024 · 5eaf712 · 5eaf712
1 parent 4a83aae
commit 5eaf712
Show file tree

Hide file tree

Showing 10 changed files with 41 additions and 24 deletions.
diff --git a/src/spec-common/injectHeadless.ts b/src/spec-common/injectHeadless.ts
@@ -323,18 +323,21 @@ export function getSystemVarFolder(params: ResolverParameters): string {
 	return params.containerSystemDataFolder || '/var/devcontainer';
 }
 
-export async function setupInContainer(params: ResolverParameters, containerProperties: ContainerProperties, config: CommonMergedDevContainerConfig, lifecycleCommandOriginMap: LifecycleHooksInstallMap) {
+export async function setupInContainer(params: ResolverParameters, containerProperties: ContainerProperties, config: CommonDevContainerConfig, mergedConfig: CommonMergedDevContainerConfig, lifecycleCommandOriginMap: LifecycleHooksInstallMap) {
 	await patchEtcEnvironment(params, containerProperties);
 	await patchEtcProfile(params, containerProperties);
 	const computeRemoteEnv = params.computeExtensionHostEnv || params.lifecycleHook.enabled;
 	const updatedConfig = containerSubstitute(params.cliHost.platform, config.configFilePath, containerProperties.env, config);
-	const remoteEnv = computeRemoteEnv ? probeRemoteEnv(params, containerProperties, updatedConfig) : Promise.resolve({});
+	const updatedMergedConfig = containerSubstitute(params.cliHost.platform, mergedConfig.configFilePath, containerProperties.env, mergedConfig);
+	const remoteEnv = computeRemoteEnv ? probeRemoteEnv(params, containerProperties, updatedMergedConfig) : Promise.resolve({});
 	const secretsP = params.secretsP || Promise.resolve({});
 	if (params.lifecycleHook.enabled) {
-		await runLifecycleHooks(params, lifecycleCommandOriginMap, containerProperties, updatedConfig, remoteEnv, secretsP, false);
+		await runLifecycleHooks(params, lifecycleCommandOriginMap, containerProperties, updatedMergedConfig, remoteEnv, secretsP, false);
 	}
 	return {
 		remoteEnv: params.computeExtensionHostEnv ? await remoteEnv : {},
+		updatedConfig,
+		updatedMergedConfig,
 	};
 }
 

diff --git a/src/spec-node/devContainersSpecCLI.ts b/src/spec-node/devContainersSpecCLI.ts
@@ -467,17 +467,16 @@ async function doSetUp({
 			bailOut(common.output, 'Dev container not found.');
 		}
 
-		const config1 = addSubstitution(config0, config => beforeContainerSubstitute(undefined, config));
-		const config = addSubstitution(config1, config => containerSubstitute(cliHost.platform, config1.config.configFilePath, envListToObj(container.Config.Env), config));
+		const config = addSubstitution(config0, config => beforeContainerSubstitute(undefined, config));
 
 		const imageMetadata = getImageMetadataFromContainer(container, config, undefined, undefined, output).config;
 		const mergedConfig = mergeConfiguration(config.config, imageMetadata);
 		const containerProperties = await createContainerProperties(params, container.Id, configs?.workspaceConfig.workspaceFolder, mergedConfig.remoteUser);
-		await setupInContainer(common, containerProperties, mergedConfig, lifecycleCommandOriginMapFromMetadata(imageMetadata));
+		const res = await setupInContainer(common, containerProperties, config.config, mergedConfig, lifecycleCommandOriginMapFromMetadata(imageMetadata));
 		return {
 			outcome: 'success' as 'success',
-			configuration: includeConfig ? config.config : undefined,
-			mergedConfiguration: includeMergedConfig ? mergedConfig : undefined,
+			configuration: includeConfig ? res.updatedConfig : undefined,
+			mergedConfiguration: includeMergedConfig ? res.updatedMergedConfig : undefined,
 			dispose,
 		};
 	} catch (originalError) {

diff --git a/src/spec-node/dockerCompose.ts b/src/spec-node/dockerCompose.ts
@@ -76,13 +76,15 @@ async function _openDockerComposeDevContainer(params: DockerResolverParameters,
 
 		const {
 			remoteEnv: extensionHostEnv,
-		} = await setupInContainer(common, containerProperties, mergedConfig, lifecycleCommandOriginMapFromMetadata(imageMetadata));
+			updatedConfig,
+			updatedMergedConfig,
+		} = await setupInContainer(common, containerProperties, config, mergedConfig, lifecycleCommandOriginMapFromMetadata(imageMetadata));
 
 		return {
 			params: common,
 			properties: containerProperties,
-			config,
-			mergedConfig,
+			config: updatedConfig,
+			mergedConfig: updatedMergedConfig,
 			resolvedAuthority: {
 				extensionHostEnv,
 			},

diff --git a/src/spec-node/singleContainer.ts b/src/spec-node/singleContainer.ts
@@ -100,13 +100,15 @@ async function setupContainer(container: ContainerDetails, params: DockerResolve
 	const { common } = params;
 	const {
 		remoteEnv: extensionHostEnv,
-	} = await setupInContainer(common, containerProperties, mergedConfig, lifecycleCommandOriginMapFromMetadata(imageMetadata));
+		updatedConfig,
+		updatedMergedConfig,
+	} = await setupInContainer(common, containerProperties, config, mergedConfig, lifecycleCommandOriginMapFromMetadata(imageMetadata));
 
 	return {
 		params: common,
 		properties: containerProperties,
-		config,
-		mergedConfig,
+		config: updatedConfig,
+		mergedConfig: updatedMergedConfig,
 		resolvedAuthority: {
 			extensionHostEnv,
 		},

diff --git a/src/spec-node/utils.ts b/src/spec-node/utils.ts
@@ -11,7 +11,7 @@ import { ContainerError, toErrorText } from '../spec-common/errors';
 import { CLIHost, runCommandNoPty, runCommand, getLocalUsername, PlatformInfo } from '../spec-common/commonUtils';
 import { Log, LogLevel, makeLog, nullLog } from '../spec-utils/log';
 
-import { ContainerProperties, getContainerProperties, LifecycleCommand, ResolverParameters } from '../spec-common/injectHeadless';
+import { CommonDevContainerConfig, ContainerProperties, getContainerProperties, LifecycleCommand, ResolverParameters } from '../spec-common/injectHeadless';
 import { Workspace } from '../spec-utils/workspaces';
 import { URI } from 'vscode-uri';
 import { ShellServer } from '../spec-common/shellServer';
@@ -125,7 +125,7 @@ export interface DockerResolverParameters {
 export interface ResolverResult {
 	params: ResolverParameters;
 	properties: ContainerProperties;
-	config: DevContainerConfig;
+	config: CommonDevContainerConfig;
 	mergedConfig: MergedDevContainerConfig;
 	resolvedAuthority: { extensionHostEnv?: { [key: string]: string | null } };
 	tunnelInformation: { environmentTunnels?: { remoteAddress: { port: number; host: string }; localAddress: string }[] };

diff --git a/src/test/cli.set-up.test.ts b/src/test/cli.set-up.test.ts
@@ -24,11 +24,13 @@ describe('Dev Containers CLI', function () {
 	describe('Command set-up', () => {
 		it('should succeed and run postAttachCommand from config', async () => {
 
-			const containerId = (await shellExec(`docker run -d alpine:3.17 sleep inf`)).stdout.trim();
+			const containerId = (await shellExec(`docker run -d -e TEST_CE=TEST_VALUE alpine:3.17 sleep inf`)).stdout.trim();
 
-			const res = await shellExec(`${cli} set-up --container-id ${containerId} --config ${__dirname}/configs/set-up-with-config/devcontainer.json`);
+			const res = await shellExec(`${cli} set-up --container-id ${containerId} --config ${__dirname}/configs/set-up-with-config/devcontainer.json --include-configuration --include-merged-configuration`);
 			const response = JSON.parse(res.stdout);
 			assert.equal(response.outcome, 'success');
+			assert.equal(response.configuration?.remoteEnv?.TEST_RE, 'TEST_VALUE');
+			assert.equal(response.mergedConfiguration?.remoteEnv?.TEST_RE, 'TEST_VALUE');
 
 			await shellExec(`docker exec ${containerId} test -f /postAttachCommand.txt`);
 			await shellExec(`docker rm -f ${containerId}`);
@@ -37,11 +39,13 @@ describe('Dev Containers CLI', function () {
 		it('should succeed and run postCreateCommand from metadata', async () => {
 
 			await shellExec(`docker build -t devcontainer-set-up-test ${__dirname}/configs/set-up-with-metadata`);
-			const containerId = (await shellExec(`docker run -d devcontainer-set-up-test sleep inf`)).stdout.trim();
+			const containerId = (await shellExec(`docker run -d -e TEST_CE=TEST_VALUE2 devcontainer-set-up-test sleep inf`)).stdout.trim();
 
-			const res = await shellExec(`${cli} set-up --container-id ${containerId}`);
+			const res = await shellExec(`${cli} set-up --container-id ${containerId} --include-configuration --include-merged-configuration`);
 			const response = JSON.parse(res.stdout);
 			assert.equal(response.outcome, 'success');
+			assert.equal(Object.keys(response.configuration).length, 0);
+			assert.equal(response.mergedConfiguration?.remoteEnv?.TEST_RE, 'TEST_VALUE2');
 
 			await shellExec(`docker exec ${containerId} test -f /postCreateCommand.txt`);
 			await shellExec(`docker rm -f ${containerId}`);

diff --git a/src/test/cli.up.test.ts b/src/test/cli.up.test.ts
@@ -26,9 +26,11 @@ describe('Dev Containers CLI', function () {
 
 	describe('Command up', () => {
 		it('should execute successfully with valid config', async () => {
-			const res = await shellExec(`${cli} up --workspace-folder ${__dirname}/configs/image`);
+			const res = await shellExec(`${cli} up --workspace-folder ${__dirname}/configs/image --include-configuration --include-merged-configuration`);
 			const response = JSON.parse(res.stdout);
 			assert.equal(response.outcome, 'success');
+			assert.equal(response.configuration?.remoteEnv?.TEST_RE, 'TEST_VALUE3');
+			assert.equal(response.mergedConfiguration?.remoteEnv?.TEST_RE, 'TEST_VALUE3');
 			const containerId: string = response.containerId;
 			assert.ok(containerId, 'Container id not found.');
 			await shellExec(`docker rm -f ${containerId}`);

diff --git a/src/test/configs/image/.devcontainer.json b/src/test/configs/image/.devcontainer.json
@@ -1,10 +1,12 @@
 {
 	"image": "ubuntu:latest",
 	"postCreateCommand": "echo \"Val: $TEST\" > /postCreateCommand.txt",
+	"runArgs": ["-e", "TEST_CE=TEST_VALUE3"],
 	"remoteEnv": {
 		"TEST": "ENV",
 		"TEST_ESCAPING": "{\n  \"fo$o\": \"ba'r\"\n}",
 		"LOCAL_PATH": "${localEnv:PATH}",
-		"CONTAINER_PATH": "${containerEnv:PATH}"
+		"CONTAINER_PATH": "${containerEnv:PATH}",
+		"TEST_RE": "${containerEnv:TEST_CE}"
 	}
 }
diff --git a/src/test/configs/set-up-with-config/devcontainer.json b/src/test/configs/set-up-with-config/devcontainer.json
@@ -1,3 +1,6 @@
 {
-	"postAttachCommand": "touch /postAttachCommand.txt"
+	"postAttachCommand": "touch /postAttachCommand.txt",
+	"remoteEnv": {
+		"TEST_RE": "${containerEnv:TEST_CE}"
+	}
 }
diff --git a/src/test/configs/set-up-with-metadata/Dockerfile b/src/test/configs/set-up-with-metadata/Dockerfile
@@ -1,3 +1,3 @@
 FROM alpine:3.17
 
-LABEL "devcontainer.metadata"="{ \"postCreateCommand\": \"touch /postCreateCommand.txt\" }"
+LABEL "devcontainer.metadata"="{ \"postCreateCommand\": \"touch /postCreateCommand.txt\", \"remoteEnv\": { \"TEST_RE\": \"\${containerEnv:TEST_CE}\" } }"