Features contribute lifecycle hooks (#390)

* refactor Feature interface

* Feat: Add in inline command lifecycle hook contribution from Features

* markerfile to exercise order of execution

* add in two helper scripts with the same name in two different Features

* test with ${featureRoot}

* make Feature artifacts executable by all users

* test for advanced lifehook scenario

* add

* typecheck

* improve test

* refactor variable names from 'postCreate' -> 'lifecycleHook'

* refactor variable names from 'postCreate' -> 'lifecycleHook'

* update test for running in parallel

* update test

* Display the 'origin' of the lifecycle command in the output log during 'up'

Logs like 'Running the postCreateCommand from devcontainer.json'
are updated to include the origin of the command

e.g: 'Running the postCreateCommand from Feature 'common-utils'.

This patch adds in a mapping object that is passed along and used to
map a command to an origin.

This change is a bit less efficient, with the added benefit of not
needing to modify the existing merging logic.

* Change copy path for Features on-container from /tmp-build-features to /opt/build-features

This is to support resuming a container in Codespaces with
Feature-contributed lifecycle hooks. Codespaces bind-mounts a different
volume on top of /tmp, so the container's /tmp directory is not
persisted.

* run lifecycle commands in Feature install order

* stop passing config object where unused

* Rename LifecycleCommandOriginMap -> LifecycleHooksInstallMap

* add test to assert installsAfter is respected

* rm -rf test dir

* assert contents in metadata label

* unused test stub

* exercise dev container test cmd

* increase test timeouts

* '' -> ''

* pass contentSourceRootPath as a variable instead of string subst

* remove dead code referencing "hasAcquires"

* cannot change line 256

* persist final Features copy to /usr/share/devcontainer/features

* update existing test

* feature dir need not be writable by anyone but its owner

* add test to assert resume will trigger lifecycle hooks

* Test: "${featureRootFolder} substituted lifecycle hooks trigger on resume

* missing semicolon

* preserve featureRootFolder in metadata and replace right before usage

* rename metadata entry to featureRootFolder for clarity and only emit in featureRaw

* merged config has substituted featureRootFolder values

* update tests to remove ${featureRootFolder}

* remove ${featureRootFolder} variable substiution

* move Features container build folder back to /tmp

Author: joshspicer

.gitignore

@@ -11,3 +11,5 @@ build-tmp
 .DS_Store
 .env
 output
+*.testMarker
+src/test/container-features/configs/temp_lifecycle-hooks-alternative-order

src/spec-common/injectHeadless.ts

@@ -168,4 +168,4 @@ function devcontainerIdForLabels(idLabels: Record<string, string>): string {
 		.toString(32)
 		.padStart(52, '0');
 	return uniqueId;
-}
+}

src/spec-common/variableSubstitution.ts

@@ -23,34 +23,55 @@ export const V1_DEVCONTAINER_FEATURES_FILE_NAME = 'devcontainer-features.json';
 // v2
 export const DEVCONTAINER_FEATURE_FILE_NAME = 'devcontainer-feature.json';
 
-export interface Feature {
+export type Feature = SchemaFeatureBaseProperties & SchemaFeatureLifecycleHooks & DeprecatedSchemaFeatureProperties & InternalFeatureProperties;
+
+export const FEATURES_CONTAINER_TEMP_DEST_FOLDER = '/tmp/dev-container-features';
+
+export interface SchemaFeatureLifecycleHooks {
+	onCreateCommand?: string | string[];
+	updateContentCommand?: string | string[];
+	postCreateCommand?: string | string[];
+	postStartCommand?: string | string[];
+	postAttachCommand?: string | string[];
+}
+
+// Properties who are members of the schema
+export interface SchemaFeatureBaseProperties {
 	id: string;
 	version?: string;
 	name?: string;
 	description?: string;
-	cachePath?: string;
-	internalVersion?: string; // set programmatically
-	consecutiveId?: string;
 	documentationURL?: string;
 	licenseURL?: string;
 	options?: Record<string, FeatureOption>;
-	buildArg?: string; // old properties for temporary compatibility
 	containerEnv?: Record<string, string>;
 	mounts?: Mount[];
 	init?: boolean;
 	privileged?: boolean;
 	capAdd?: string[];
 	securityOpt?: string[];
 	entrypoint?: string;
-	include?: string[];
-	exclude?: string[];
-	value: boolean | string | Record<string, boolean | string | undefined>; // set programmatically
-	included: boolean; // set programmatically
 	customizations?: VSCodeCustomizations;
 	installsAfter?: string[];
 	deprecated?: boolean;
 	legacyIds?: string[];
-	currentId?: string; // set programmatically
+}
+
+// Properties that are set programmatically for book-keeping purposes
+export interface InternalFeatureProperties {
+	cachePath?: string;
+	internalVersion?: string;
+	consecutiveId?: string;
+	value: boolean | string | Record<string, boolean | string | undefined>;
+	currentId?: string;
+	included: boolean;
+}
+
+// Old or deprecated properties maintained for backwards compatibility
+export interface DeprecatedSchemaFeatureProperties {
+	buildArg?: string;
+	include?: string[];
+	exclude?: string[];
 }
 
 export type FeatureOption = {
@@ -218,27 +239,25 @@ export function getSourceInfoString(srcInfo: SourceInformation): string {
 }
 
 // TODO: Move to node layer.
-export function getContainerFeaturesBaseDockerFile() {
+export function getContainerFeaturesBaseDockerFile(contentSourceRootPath: string) {
 	return `
-#{featureBuildStages}
 
 #{nonBuildKitFeatureContentFallback}
 
 FROM $_DEV_CONTAINERS_BASE_IMAGE AS dev_containers_feature_content_normalize
 USER root
-COPY --from=dev_containers_feature_content_source {contentSourceRootPath}/devcontainer-features.builtin.env /tmp/build-features/
-RUN chmod -R 0700 /tmp/build-features
+COPY --from=dev_containers_feature_content_source ${path.posix.join(contentSourceRootPath, 'devcontainer-features.builtin.env')} /tmp/build-features/
+RUN chmod -R 0755 /tmp/build-features/
 
 FROM $_DEV_CONTAINERS_BASE_IMAGE AS dev_containers_target_stage
 
 USER root
 
-COPY --from=dev_containers_feature_content_normalize /tmp/build-features /tmp/build-features
+RUN mkdir -p ${FEATURES_CONTAINER_TEMP_DEST_FOLDER}
+COPY --from=dev_containers_feature_content_normalize /tmp/build-features/ ${FEATURES_CONTAINER_TEMP_DEST_FOLDER}
 
 #{featureLayer}
 
-#{copyFeatureBuildStages}
-
 #{containerEnv}
 
 ARG _DEV_CONTAINERS_IMAGE_USER=root
@@ -312,33 +331,36 @@ function escapeQuotesForShell(input: string) {
 	return input.replace(new RegExp(`'`, 'g'), `'\\''`);
 }
 
-export function getFeatureLayers(featuresConfig: FeaturesConfig, containerUser: string, remoteUser: string, useBuildKitBuildContexts = false, contentSourceRootPath = '/tmp/build-features/') {
+export function getFeatureLayers(featuresConfig: FeaturesConfig, containerUser: string, remoteUser: string, useBuildKitBuildContexts = false, contentSourceRootPath = '/tmp/build-features') {
+
+	const builtinsEnvFile = `${path.posix.join(FEATURES_CONTAINER_TEMP_DEST_FOLDER, 'devcontainer-features.builtin.env')}`;
 	let result = `RUN \\
-echo "_CONTAINER_USER_HOME=$(getent passwd ${containerUser} | cut -d: -f6)" >> /tmp/build-features/devcontainer-features.builtin.env && \\
-echo "_REMOTE_USER_HOME=$(getent passwd ${remoteUser} | cut -d: -f6)" >> /tmp/build-features/devcontainer-features.builtin.env
+echo "_CONTAINER_USER_HOME=$(getent passwd ${containerUser} | cut -d: -f6)" >> ${builtinsEnvFile} && \\
+echo "_REMOTE_USER_HOME=$(getent passwd ${remoteUser} | cut -d: -f6)" >> ${builtinsEnvFile}
 
 `;
 
 	// Features version 1
 	const folders = (featuresConfig.featureSets || []).filter(y => y.internalVersion !== '2').map(x => x.features[0].consecutiveId);
 	folders.forEach(folder => {
 		const source = path.posix.join(contentSourceRootPath, folder!);
+		const dest = path.posix.join(FEATURES_CONTAINER_TEMP_DEST_FOLDER, folder!);
 		if (!useBuildKitBuildContexts) {
-			result += `COPY --chown=root:root --from=dev_containers_feature_content_source ${source} /tmp/build-features/${folder}
-RUN chmod -R 0700 /tmp/build-features/${folder} \\
-&& cd /tmp/build-features/${folder} \\
+			result += `COPY --chown=root:root --from=dev_containers_feature_content_source ${source} ${dest}
+RUN chmod -R 0755 ${dest} \\
+&& cd ${dest} \\
 && chmod +x ./install.sh \\
 && ./install.sh
 
 `;
 		} else {
 			result += `RUN --mount=type=bind,from=dev_containers_feature_content_source,source=${source},target=/tmp/build-features-src/${folder} \\
-    cp -ar /tmp/build-features-src/${folder} /tmp/build-features/ \\
- && chmod -R 0700 /tmp/build-features/${folder} \\
- && cd /tmp/build-features/${folder} \\
+    cp -ar /tmp/build-features-src/${folder} ${FEATURES_CONTAINER_TEMP_DEST_FOLDER} \\
+ && chmod -R 0755 ${dest} \\
+ && cd ${dest} \\
  && chmod +x ./install.sh \\
  && ./install.sh \\
- && rm -rf /tmp/build-features/${folder}
+ && rm -rf ${dest}
 
 `;
 		}
@@ -348,24 +370,25 @@ RUN chmod -R 0700 /tmp/build-features/${folder} \\
 		featureSet.features.forEach(feature => {
 			result += generateContainerEnvs(feature);
 			const source = path.posix.join(contentSourceRootPath, feature.consecutiveId!);
+			const dest = path.posix.join(FEATURES_CONTAINER_TEMP_DEST_FOLDER, feature.consecutiveId!);
 			if (!useBuildKitBuildContexts) {
 				result += `
-COPY --chown=root:root --from=dev_containers_feature_content_source ${source} /tmp/build-features/${feature.consecutiveId}
-RUN chmod -R 0700 /tmp/build-features/${feature.consecutiveId} \\
-&& cd /tmp/build-features/${feature.consecutiveId} \\
+COPY --chown=root:root --from=dev_containers_feature_content_source ${source} ${dest}
+RUN chmod -R 0755 ${dest} \\
+&& cd ${dest} \\
 && chmod +x ./devcontainer-features-install.sh \\
 && ./devcontainer-features-install.sh
 
 `;
 			} else {
 				result += `
 RUN --mount=type=bind,from=dev_containers_feature_content_source,source=${source},target=/tmp/build-features-src/${feature.consecutiveId} \\
-    cp -ar /tmp/build-features-src/${feature.consecutiveId} /tmp/build-features/ \\
- && chmod -R 0700 /tmp/build-features/${feature.consecutiveId} \\
- && cd /tmp/build-features/${feature.consecutiveId} \\
+    cp -ar /tmp/build-features-src/${feature.consecutiveId} ${FEATURES_CONTAINER_TEMP_DEST_FOLDER} \\
+ && chmod -R 0755 ${dest} \\
+ && cd ${dest} \\
  && chmod +x ./devcontainer-features-install.sh \\
  && ./devcontainer-features-install.sh \\
- && rm -rf /tmp/build-features/${feature.consecutiveId}
+ && rm -rf ${dest}
 
 `;
 			}
@@ -938,6 +961,11 @@ async function fetchFeatures(params: { extensionPath: string; cwd: string; outpu
 			feature.cachePath = featCachePath;
 			feature.consecutiveId = consecutiveId;
 
+			if (!feature.consecutiveId || !feature.id || !featureSet?.sourceInformation || !featureSet.sourceInformation.userFeatureId) {
+				const err = 'Internal Features error. Missing required attribute(s).';
+				throw new Error(err);
+			}
+
 			const featureDebugId = `${feature.consecutiveId}_${sourceInfoType}`;
 			output.write(`* Fetching feature: ${featureDebugId}`);
 

src/spec-configuration/containerFeaturesConfiguration.ts

@@ -56,7 +56,7 @@ async function resolveWithLocalFolder(params: DockerResolverParameters, parsedAu
 	const configWithRaw = addSubstitution(configs.config, config => beforeContainerSubstitute(envListToObj(idLabels), config));
 	const { config } = configWithRaw;
 
-	await runUserCommand({ ...params, common: { ...common, output: common.postCreate.output } }, config.initializeCommand, common.postCreate.onDidInput);
+	await runUserCommand({ ...params, common: { ...common, output: common.lifecycleHook.output } }, config.initializeCommand, common.lifecycleHook.onDidInput);
 
 	let result: ResolverResult;
 	if (isDockerFileConfig(config) || 'image' in config) {

src/spec-node/configContainer.ts

@@ -285,13 +285,10 @@ async function getFeaturesBuildOptions(params: DockerResolverParameters, devCont
 	// When copying via buildkit, the content is accessed via '.' (i.e. in the context root)
 	// When copying via temp image, the content is in '/tmp/build-features'
 	const contentSourceRootPath = useBuildKitBuildContexts ? '.' : '/tmp/build-features/';
-	const dockerfile = getContainerFeaturesBaseDockerFile()
+	const dockerfile = getContainerFeaturesBaseDockerFile(contentSourceRootPath)
 		.replace('#{nonBuildKitFeatureContentFallback}', useBuildKitBuildContexts ? '' : `FROM ${buildContentImageName} as dev_containers_feature_content_source`)
-		.replace('{contentSourceRootPath}', contentSourceRootPath)
-		.replace('#{featureBuildStages}', getFeatureBuildStages(featuresConfig, buildStageScripts, contentSourceRootPath))
 		.replace('#{featureLayer}', getFeatureLayers(featuresConfig, containerUser, remoteUser, useBuildKitBuildContexts, contentSourceRootPath))
 		.replace('#{containerEnv}', generateContainerEnvs(featuresConfig))
-		.replace('#{copyFeatureBuildStages}', getCopyFeatureBuildStages(featuresConfig, buildStageScripts))
 		.replace('#{devcontainerMetadata}', getDevcontainerMetadataLabel(imageMetadata, common.experimentalImageMetadata))
 		.replace('#{containerEnvMetadata}', getContainerEnvMetadata(devContainerConfig.config.containerEnv))
 		;
@@ -389,31 +386,6 @@ export function findContainerUsers(imageMetadata: SubstitutedConfig<ImageMetadat
 	return { containerUser, remoteUser };
 }
 
-function getFeatureBuildStages(featuresConfig: FeaturesConfig, buildStageScripts: Record<string, { hasAcquire: boolean; hasConfigure: boolean } | undefined>[], contentSourceRootPath: string) {
-	return ([] as string[]).concat(...featuresConfig.featureSets
-		.map((featureSet, i) => featureSet.features
-			.filter(f => (includeAllConfiguredFeatures || f.included) && f.value && buildStageScripts[i][f.id]?.hasAcquire)
-			.map(f => `FROM mcr.microsoft.com/vscode/devcontainers/base:0-focal as ${getSourceInfoString(featureSet.sourceInformation)}_${f.id}
-COPY --from=dev_containers_feature_content_normalize ${path.posix.join(contentSourceRootPath, getSourceInfoString(featureSet.sourceInformation), 'features', f.id)} ${path.posix.join('/tmp/build-features', getSourceInfoString(featureSet.sourceInformation), 'features', f.id)}
-COPY --from=dev_containers_feature_content_normalize ${path.posix.join(contentSourceRootPath, getSourceInfoString(featureSet.sourceInformation), 'common')} ${path.posix.join('/tmp/build-features', getSourceInfoString(featureSet.sourceInformation), 'common')}
-RUN cd ${path.posix.join('/tmp/build-features', getSourceInfoString(featureSet.sourceInformation), 'features', f.id)} && set -a && . ./devcontainer-features.env && set +a && ./bin/acquire`
-			)
-		)
-	).join('\n\n');
-}
-
-function getCopyFeatureBuildStages(featuresConfig: FeaturesConfig, buildStageScripts: Record<string, { hasAcquire: boolean; hasConfigure: boolean } | undefined>[]) {
-	return ([] as string[]).concat(...featuresConfig.featureSets
-		.map((featureSet, i) => featureSet.features
-			.filter(f => (includeAllConfiguredFeatures || f.included) && f.value && buildStageScripts[i][f.id]?.hasAcquire)
-			.map(f => {
-				const featurePath = path.posix.join('/usr/local/devcontainer-features', getSourceInfoString(featureSet.sourceInformation), f.id);
-				return `COPY --from=${getSourceInfoString(featureSet.sourceInformation)}_${f.id} ${featurePath} ${featurePath}${buildStageScripts[i][f.id]?.hasConfigure ? `
-RUN cd ${path.posix.join('/tmp/build-features', getSourceInfoString(featureSet.sourceInformation), 'features', f.id)} && set -a && . ./devcontainer-features.env && set +a && ./bin/configure` : ''}`;
-			})
-		)
-	).join('\n\n');
-}
 
 function getFeatureEnvVariables(f: Feature) {
 	const values = getFeatureValueObject(f);

src/spec-node/containerFeatures.ts

@@ -8,7 +8,7 @@ import * as crypto from 'crypto';
 import * as os from 'os';
 
 import { DockerResolverParameters, DevContainerAuthority, UpdateRemoteUserUIDDefault, BindMountConsistency, getCacheFolder } from './utils';
-import { createNullPostCreate, finishBackgroundTasks, ResolverParameters, UserEnvProbe } from '../spec-common/injectHeadless';
+import { createNullLifecycleHook, finishBackgroundTasks, ResolverParameters, UserEnvProbe } from '../spec-common/injectHeadless';
 import { getCLIHost, loadNativeModule } from '../spec-common/commonUtils';
 import { resolve } from './configContainer';
 import { URI } from 'vscode-uri';
@@ -122,7 +122,7 @@ export async function createDockerParams(options: ProvisionOptions, disposables:
 		output,
 		allowSystemConfigChange: true,
 		defaultUserEnvProbe: options.defaultUserEnvProbe,
-		postCreate: createNullPostCreate(options.postCreateEnabled, options.skipNonBlocking, output),
+		lifecycleHook: createNullLifecycleHook(options.postCreateEnabled, options.skipNonBlocking, output),
 		getLogLevel: () => options.logLevel,
 		onDidChangeLogLevel: () => ({ dispose() { } }),
 		loadNativeModule,

src/spec-node/devContainers.ts

@@ -13,8 +13,7 @@ import { SubstitutedConfig, createContainerProperties, createFeaturesTempFolder,
 import { URI } from 'vscode-uri';
 import { ContainerError } from '../spec-common/errors';
 import { Log, LogLevel, makeLog, mapLogLevel } from '../spec-utils/log';
-import { probeRemoteEnv, runPostCreateCommands, runRemoteCommand, UserEnvProbe, setupInContainer } from '../spec-common/injectHeadless';
-import { bailOut, buildNamedImageAndExtend } from './singleContainer';
+import { probeRemoteEnv, runLifecycleHooks, runRemoteCommand, UserEnvProbe, setupInContainer } from '../spec-common/injectHeadless';
 import { extendImage } from './containerFeatures';
 import { DockerCLIParameters, dockerPtyCLI, inspectContainer } from '../spec-shutdown/dockerUtils';
 import { buildAndExtendDockerCompose, dockerComposeCLIConfig, getDefaultImageName, getProjectName, readDockerComposeConfig, readVersionPrefix } from './dockerCompose';
@@ -31,10 +30,11 @@ import { featuresPublishHandler, featuresPublishOptions } from './featuresCLI/pu
 import { featureInfoTagsHandler, featuresInfoTagsOptions } from './featuresCLI/infoTags';
 import { beforeContainerSubstitute, containerSubstitute, substitute } from '../spec-common/variableSubstitution';
 import { getPackageConfig, PackageConfiguration } from '../spec-utils/product';
-import { getDevcontainerMetadata, getImageBuildInfo, getImageMetadataFromContainer, ImageMetadataEntry, mergeConfiguration, MergedDevContainerConfig } from './imageMetadata';
+import { getDevcontainerMetadata, getImageBuildInfo, getImageMetadataFromContainer, ImageMetadataEntry, lifecycleCommandOriginMapFromMetadata, mergeConfiguration, MergedDevContainerConfig } from './imageMetadata';
 import { templatesPublishHandler, templatesPublishOptions } from './templatesCLI/publish';
 import { templateApplyHandler, templateApplyOptions } from './templatesCLI/apply';
 import { featuresInfoManifestHandler, featuresInfoManifestOptions } from './featuresCLI/infoManifest';
+import { bailOut, buildNamedImageAndExtend } from './singleContainer';
 
 const defaultDefaultUserEnvProbe: UserEnvProbe = 'loginInteractiveShell';
 
@@ -423,7 +423,7 @@ async function doSetUp({
 		const imageMetadata = getImageMetadataFromContainer(container, config, undefined, undefined, true, 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);
+		await setupInContainer(common, containerProperties, mergedConfig, lifecycleCommandOriginMapFromMetadata(imageMetadata));
 		return {
 			outcome: 'success' as 'success',
 			dispose,
@@ -834,7 +834,7 @@ async function doRunUserCommands({
 		const containerProperties = await createContainerProperties(params, container.Id, configs?.workspaceConfig.workspaceFolder, mergedConfig.remoteUser);
 		const updatedConfig = containerSubstitute(cliHost.platform, config.config.configFilePath, containerProperties.env, mergedConfig);
 		const remoteEnv = probeRemoteEnv(common, containerProperties, updatedConfig);
-		const result = await runPostCreateCommands(common, containerProperties, updatedConfig, remoteEnv, stopForPersonalization);
+		const result = await runLifecycleHooks(common, lifecycleCommandOriginMapFromMetadata(imageMetadata), containerProperties, updatedConfig, remoteEnv, stopForPersonalization);
 		return {
 			outcome: 'success' as 'success',
 			result,

src/spec-node/devContainersSpecCLI.ts

@@ -17,7 +17,7 @@ import { Log, LogLevel, makeLog, terminalEscapeSequences } from '../spec-utils/l
 import { getExtendImageBuildInfo, updateRemoteUserUID } from './containerFeatures';
 import { Mount, parseMount } from '../spec-configuration/containerFeaturesConfiguration';
 import path from 'path';
-import { getDevcontainerMetadata, getImageBuildInfoFromDockerfile, getImageBuildInfoFromImage, getImageMetadataFromContainer, ImageBuildInfo, mergeConfiguration, MergedDevContainerConfig } from './imageMetadata';
+import { getDevcontainerMetadata, getImageBuildInfoFromDockerfile, getImageBuildInfoFromImage, getImageMetadataFromContainer, ImageBuildInfo, lifecycleCommandOriginMapFromMetadata, mergeConfiguration, MergedDevContainerConfig } from './imageMetadata';
 import { ensureDockerfileHasFinalStageName } from './dockerfileUtils';
 
 const projectLabel = 'com.docker.compose.project';
@@ -68,13 +68,13 @@ async function _openDockerComposeDevContainer(params: DockerResolverParameters,
 			// 	collapsedFeaturesConfig = collapseFeaturesConfig(featuresConfig);
 		}
 
-		const configs = getImageMetadataFromContainer(container, configWithRaw, undefined, idLabels, common.experimentalImageMetadata, common.output).config;
-		const mergedConfig = mergeConfiguration(configWithRaw.config, configs);
+		const imageMetadata = getImageMetadataFromContainer(container, configWithRaw, undefined, idLabels, common.experimentalImageMetadata, common.output).config;
+		const mergedConfig = mergeConfiguration(configWithRaw.config, imageMetadata);
 		containerProperties = await createContainerProperties(params, container.Id, remoteWorkspaceFolder, mergedConfig.remoteUser);
 
 		const {
 			remoteEnv: extensionHostEnv,
-		} = await setupInContainer(common, containerProperties, mergedConfig);
+		} = await setupInContainer(common, containerProperties, mergedConfig, lifecycleCommandOriginMapFromMetadata(imageMetadata));
 
 		return {
 			params: common,