Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Don't crash updating UID/GID when the image's platform is different from the native CPU arch #746

Merged
merged 14 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/dev-containers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ jobs:
registry-url: 'https://npm.pkg.github.com'
scope: '@microsoft'
- name: Install Dependencies
run: yarn install --frozen-lockfile
run: |
yarn install --frozen-lockfile
docker run --privileged --rm tonistiigi/binfmt --install all
- name: Type-Check
run: yarn type-check
- name: Package
Expand Down
2 changes: 1 addition & 1 deletion src/spec-common/cliHost.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

Expand Down
8 changes: 8 additions & 0 deletions src/spec-common/commonUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ export interface ExecFunction {
(params: ExecParameters): Promise<Exec>;
}

export type GoOS = { [OS in NodeJS.Platform]: OS extends 'win32' ? 'windows' : OS; }[NodeJS.Platform];
export type GoARCH = { [ARCH in NodeJS.Architecture]: ARCH extends 'x64' ? 'amd64' : ARCH; }[NodeJS.Architecture];

export interface PlatformInfo {
os: GoOS;
arch: GoARCH;
}

export interface PtyExec {
onData: Event<string>;
write?(data: string): void;
Expand Down
20 changes: 8 additions & 12 deletions src/spec-configuration/containerCollectionsOCI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as crypto from 'crypto';
import { Log, LogLevel } from '../spec-utils/log';
import { isLocalFile, mkdirpLocal, readLocalFile, writeLocalFile } from '../spec-utils/pfs';
import { requestEnsureAuthenticated } from './httpOCIRegistry';
import { GoARCH, GoOS, PlatformInfo } from '../spec-common/commonUtils';

export const DEVCONTAINER_MANIFEST_MEDIATYPE = 'application/vnd.devcontainers';
export const DEVCONTAINER_TAR_LAYER_MEDIATYPE = 'application/vnd.devcontainers.layer.v1+tar';
Expand Down Expand Up @@ -116,7 +117,7 @@ const regexForVersionOrDigest = /^[a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}$/;

// https://go.dev/doc/install/source#environment
// Expected by OCI Spec as seen here: https://github.com/opencontainers/image-spec/blob/main/image-index.md#image-index-property-descriptions
export function mapNodeArchitectureToGOARCH(arch: NodeJS.Architecture): string {
export function mapNodeArchitectureToGOARCH(arch: NodeJS.Architecture): GoARCH {
switch (arch) {
case 'x64':
return 'amd64';
Expand All @@ -127,7 +128,7 @@ export function mapNodeArchitectureToGOARCH(arch: NodeJS.Architecture): string {

// https://go.dev/doc/install/source#environment
// Expected by OCI Spec as seen here: https://github.com/opencontainers/image-spec/blob/main/image-index.md#image-index-property-descriptions
export function mapNodeOSToGOOS(os: NodeJS.Platform): string {
export function mapNodeOSToGOOS(os: NodeJS.Platform): GoOS {
switch (os) {
case 'win32':
return 'windows';
Expand Down Expand Up @@ -272,13 +273,13 @@ export function getCollectionRef(output: Log, registry: string, namespace: strin
export async function fetchOCIManifestIfExists(params: CommonParams, ref: OCIRef | OCICollectionRef, manifestDigest?: string): Promise<ManifestContainer | undefined> {
const { output } = params;

// Simple mechanism to avoid making a DNS request for
// Simple mechanism to avoid making a DNS request for
// something that is not a domain name.
if (ref.registry.indexOf('.') < 0 && !ref.registry.startsWith('localhost')) {
return;
}

// TODO: Always use the manifest digest (the canonical digest)
// TODO: Always use the manifest digest (the canonical digest)
// instead of the `ref.version` by referencing some lock file (if available).
let reference = ref.version;
if (manifestDigest) {
Expand Down Expand Up @@ -338,7 +339,7 @@ export async function getManifest(params: CommonParams, url: string, ref: OCIRef
}

// https://github.com/opencontainers/image-spec/blob/main/manifest.md
export async function getImageIndexEntryForPlatform(params: CommonParams, url: string, ref: OCIRef | OCICollectionRef, platformInfo: { arch: NodeJS.Architecture; os: NodeJS.Platform }, mimeType?: string): Promise<OCIImageIndexEntry | undefined> {
export async function getImageIndexEntryForPlatform(params: CommonParams, url: string, ref: OCIRef | OCICollectionRef, platformInfo: PlatformInfo, mimeType?: string): Promise<OCIImageIndexEntry | undefined> {
const { output } = params;
const response = await getJsonWithMimeType<OCIImageIndex>(params, url, ref, mimeType || 'application/vnd.oci.image.index.v1+json');
if (!response) {
Expand All @@ -351,14 +352,9 @@ export async function getImageIndexEntryForPlatform(params: CommonParams, url: s
return undefined;
}

const ociFriendlyPlatformInfo = {
arch: mapNodeArchitectureToGOARCH(platformInfo.arch),
os: mapNodeOSToGOOS(platformInfo.os),
};

// Find a manifest for the current architecture and OS.
return imageIndex.manifests.find(m => {
if (m.platform?.architecture === ociFriendlyPlatformInfo.arch && m.platform?.os === ociFriendlyPlatformInfo.os) {
if (m.platform?.architecture === platformInfo.arch && m.platform?.os === platformInfo.os) {
return m;
}
return undefined;
Expand Down Expand Up @@ -595,4 +591,4 @@ export async function getBlob(params: CommonParams, url: string, ociCacheDir: st
output.write(`Error getting blob: ${e}`, LogLevel.Error);
return;
}
}
}
2 changes: 1 addition & 1 deletion src/spec-node/configContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ async function resolveWithLocalFolder(params: DockerResolverParameters, parsedAu

const { dockerCLI, dockerComposeCLI } = params;
const { env } = common;
const cliParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI, env, output };
const cliParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI, env, output, platformInfo: params.platformInfo };
await ensureNoDisallowedFeatures(cliParams, config, additionalFeatures, idLabels);

await runInitializeCommand({ ...params, common: { ...common, output: common.lifecycleHook.output } }, config.initializeCommand, common.lifecycleHook.onDidInput);
Expand Down
10 changes: 6 additions & 4 deletions src/spec-node/containerFeatures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { ContainerError } from '../spec-common/errors';
// Environment variables must contain:
// - alpha-numeric values, or
// - the '_' character, and
// - a number cannot be the first character
// - a number cannot be the first character
export const getSafeId = (str: string) => str
.replace(/[^\w_]/g, '_')
.replace(/^[\d_]+/g, '_')
Expand Down Expand Up @@ -344,7 +344,7 @@ function getFeatureEnvVariables(f: Feature) {
const values = getFeatureValueObject(f);
const idSafe = getSafeId(f.id);
const variables = [];

if(f.internalVersion !== '2')
{
if (values) {
Expand All @@ -365,7 +365,7 @@ function getFeatureEnvVariables(f: Feature) {
variables.push(`${f.buildArg}=${getFeatureMainValue(f)}`);
}
return variables;
}
}
}

export async function getRemoteUserUIDUpdateDetails(params: DockerResolverParameters, mergedConfig: MergedDevContainerConfig, imageName: string, imageDetails: () => Promise<ImageDetails>, runArgsUser: string | undefined) {
Expand All @@ -388,6 +388,7 @@ export async function getRemoteUserUIDUpdateDetails(params: DockerResolverParame
imageName: fixedImageName,
remoteUser,
imageUser,
platform: `${details.Os}/${details.Architecture}`
};
}

Expand All @@ -399,7 +400,7 @@ export async function updateRemoteUserUID(params: DockerResolverParameters, merg
if (!updateDetails) {
return imageName;
}
const { imageName: fixedImageName, remoteUser, imageUser } = updateDetails;
const { imageName: fixedImageName, remoteUser, imageUser, platform } = updateDetails;

const dockerfileName = 'updateUID.Dockerfile';
const srcDockerfile = path.join(common.extensionPath, 'scripts', dockerfileName);
Expand All @@ -415,6 +416,7 @@ export async function updateRemoteUserUID(params: DockerResolverParameters, merg
'build',
'-f', destDockerfile,
'-t', fixedImageName,
...(platform ? ['--platform', platform] : []),
'--build-arg', `BASE_IMAGE=${imageName}`,
'--build-arg', `REMOTE_USER=${remoteUser}`,
'--build-arg', `NEW_UID=${await cliHost.getuid!()}`,
Expand Down
11 changes: 9 additions & 2 deletions src/spec-node/devContainers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import * as path from 'path';
import * as crypto from 'crypto';
import * as os from 'os';

import { mapNodeOSToGOOS, mapNodeArchitectureToGOARCH } from '../spec-configuration/containerCollectionsOCI';
import { DockerResolverParameters, DevContainerAuthority, UpdateRemoteUserUIDDefault, BindMountConsistency, getCacheFolder } from './utils';
import { createNullLifecycleHook, finishBackgroundTasks, ResolverParameters, UserEnvProbe } from '../spec-common/injectHeadless';
import { getCLIHost, loadNativeModule } from '../spec-common/commonUtils';
import { GoARCH, GoOS, getCLIHost, loadNativeModule } from '../spec-common/commonUtils';
import { resolve } from './configContainer';
import { URI } from 'vscode-uri';
import { LogLevel, LogDimensions, toErrorText, createCombinedLog, createTerminalLog, Log, makeLog, LogFormat, createJSONLog, createPlainLog, LogHandler, replaceAllLog } from '../spec-utils/log';
Expand Down Expand Up @@ -163,12 +164,17 @@ export async function createDockerParams(options: ProvisionOptions, disposables:
env: cliHost.env,
output: common.output,
}, dockerPath, dockerComposePath);
const platformInfo = {
os: (<GoOS | undefined> common.buildxPlatform?.slice(0, common.buildxPlatform.indexOf('/'))) || mapNodeOSToGOOS(cliHost.platform),
arch: (<GoARCH | undefined> common.buildxPlatform?.slice(common.buildxPlatform.indexOf('/'))) || mapNodeArchitectureToGOARCH(cliHost.arch),
trxcllnt marked this conversation as resolved.
Show resolved Hide resolved
};
const buildKitVersion = options.useBuildKit === 'never' ? undefined : (await dockerBuildKitVersion({
cliHost,
dockerCLI: dockerPath,
dockerComposeCLI,
env: cliHost.env,
output
output,
platformInfo
}));
return {
common,
Expand All @@ -195,6 +201,7 @@ export async function createDockerParams(options: ProvisionOptions, disposables:
buildxPush: common.buildxPush,
buildxOutput: common.buildxOutput,
buildxCacheTo: common.buildxCacheTo,
platformInfo
};
}

Expand Down
13 changes: 9 additions & 4 deletions src/spec-node/devContainersSpecCLI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import { featuresResolveDependenciesHandler, featuresResolveDependenciesOptions
import { getFeatureIdWithoutVersion } from '../spec-configuration/containerFeaturesOCI';
import { featuresUpgradeHandler, featuresUpgradeOptions } from './upgradeCommand';
import { readFeaturesConfig } from './featureUtils';
import { mapNodeOSToGOOS, mapNodeArchitectureToGOARCH } from '../spec-configuration/containerCollectionsOCI';

const defaultDefaultUserEnvProbe: UserEnvProbe = 'loginInteractiveShell';

Expand Down Expand Up @@ -601,7 +602,7 @@ async function doBuild({
throw new ContainerError({ description: '--push true cannot be used with --output.' });
}

const buildParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI, env, output };
const buildParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI, env, output, platformInfo: params.platformInfo };
await ensureNoDisallowedFeatures(buildParams, config, additionalFeatures, undefined);

// Support multiple use of `--image-name`
Expand All @@ -622,8 +623,8 @@ async function doBuild({
}
} else if ('dockerComposeFile' in config) {

if (buildxPlatform || buildxPush) {
throw new ContainerError({ description: '--platform or --push not supported.' });
if (buildxPush) {
trxcllnt marked this conversation as resolved.
Show resolved Hide resolved
throw new ContainerError({ description: '--push not supported.' });
}

if (buildxOutput) {
Expand Down Expand Up @@ -1011,7 +1012,11 @@ async function readConfiguration({
dockerCLI,
dockerComposeCLI,
env: cliHost.env,
output
output,
platformInfo: {
os: mapNodeOSToGOOS(cliHost.platform),
arch: mapNodeArchitectureToGOARCH(cliHost.arch),
}
};
const { container, idLabels } = await findContainerAndIdLabels(params, containerId, providedIdLabels, workspaceFolder, configPath?.fsPath);
if (container) {
Expand Down
10 changes: 5 additions & 5 deletions src/spec-node/dockerCompose.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const serviceLabel = 'com.docker.compose.service';
export async function openDockerComposeDevContainer(params: DockerResolverParameters, workspace: Workspace, config: SubstitutedConfig<DevContainerFromDockerComposeConfig>, idLabels: string[], additionalFeatures: Record<string, string | boolean | Record<string, string | boolean>>): Promise<ResolverResult> {
const { common, dockerCLI, dockerComposeCLI } = params;
const { cliHost, env, output } = common;
const buildParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI, env, output };
const buildParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI, env, output, platformInfo: params.platformInfo };
return _openDockerComposeDevContainer(params, buildParams, workspace, config, getRemoteWorkspaceFolder(config.config), idLabels, additionalFeatures);
}

Expand Down Expand Up @@ -150,7 +150,7 @@ export async function buildAndExtendDockerCompose(configWithRaw: SubstitutedConf
const { cliHost, env, output } = common;
const { config } = configWithRaw;

const cliParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI: dockerComposeCLIFunc, env, output };
const cliParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI: dockerComposeCLIFunc, env, output, platformInfo: params.platformInfo };
const composeConfig = await readDockerComposeConfig(cliParams, localComposeFiles, envFile);
const composeService = composeConfig.services[config.service];

Expand Down Expand Up @@ -406,7 +406,7 @@ async function startContainer(params: DockerResolverParameters, buildParams: Doc
// Note: As a fallback, persistedFolder is set to the build's tmpDir() directory
const additionalLabels = labels ? idLabels.concat(Object.keys(labels).map(key => `${key}=${labels[key]}`)) : idLabels;
const overrideFilePath = await writeFeaturesComposeOverrideFile(updatedImageName, currentImageName, mergedConfig, config, versionPrefix, imageDetails, service, additionalLabels, params.additionalMounts, persistedFolder, featuresStartOverrideFilePrefix, buildCLIHost, params, output);

if (overrideFilePath) {
// Add file path to override file as parameter
composeGlobalArgs.push('-f', overrideFilePath);
Expand Down Expand Up @@ -714,7 +714,7 @@ export function dockerComposeCLIConfig(params: Omit<PartialExecParameters, 'cmd'

/**
* Convert mount command arguments to Docker Compose volume
* @param mount
* @param mount
* @returns mount command representation for Docker compose
*/
function convertMountToVolume(mount: Mount): string {
Expand All @@ -731,7 +731,7 @@ function convertMountToVolume(mount: Mount): string {

/**
* Convert mount command arguments to volume top-level element
* @param mount
* @param mount
* @returns mount object representation as volumes top-level element
*/
function convertMountToVolumeTopLevelElement(mount: Mount): string {
Expand Down
2 changes: 1 addition & 1 deletion src/spec-node/imageMetadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ export async function getImageBuildInfo(params: DockerResolverParameters | Docke
const cwdEnvFile = cliHost.path.join(cliHost.cwd, '.env');
const envFile = Array.isArray(config.dockerComposeFile) && config.dockerComposeFile.length === 0 && await cliHost.isFile(cwdEnvFile) ? cwdEnvFile : undefined;
const composeFiles = await getDockerComposeFilePaths(cliHost, config, cliHost.env, cliHost.cwd);
const buildParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI, env: cliHost.env, output };
const buildParams: DockerCLIParameters = { cliHost, dockerCLI, dockerComposeCLI, env: cliHost.env, output, platformInfo: params.platformInfo };

const composeConfig = await readDockerComposeConfig(buildParams, composeFiles, envFile);
const services = Object.keys(composeConfig.services || {});
Expand Down
7 changes: 6 additions & 1 deletion src/spec-node/upgradeCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { Lockfile, generateLockfile, getLockfilePath, writeLockfile } from '../s
import { isLocalFile, readLocalFile, writeLocalFile } from '../spec-utils/pfs';
import { readFeaturesConfig } from './featureUtils';
import { DevContainerConfig } from '../spec-configuration/configuration';
import { mapNodeArchitectureToGOARCH, mapNodeOSToGOOS } from '../spec-configuration/containerCollectionsOCI';

export function featuresUpgradeOptions(y: Argv) {
return y
Expand Down Expand Up @@ -92,6 +93,10 @@ async function featuresUpgrade({
dockerComposeCLI,
env: cliHost.env,
output,
platformInfo: {
os: mapNodeOSToGOOS(cliHost.platform),
arch: mapNodeArchitectureToGOARCH(cliHost.arch),
}
};

const workspace = workspaceFromPath(cliHost.path, workspaceFolder);
Expand Down Expand Up @@ -199,4 +204,4 @@ const lastDelimiter = /[:@][^/]*$/;
function getFeatureIdWithoutVersion(featureId: string) {
const m = lastDelimiter.exec(featureId);
return m ? featureId.substring(0, m.index) : featureId;
}
}
12 changes: 7 additions & 5 deletions src/spec-node/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import * as crypto from 'crypto';
import * as os from 'os';

import { ContainerError, toErrorText } from '../spec-common/errors';
import { CLIHost, runCommandNoPty, runCommand, getLocalUsername } from '../spec-common/commonUtils';
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';
Expand Down Expand Up @@ -117,6 +117,7 @@ export interface DockerResolverParameters {
buildxPush: boolean;
buildxOutput: string | undefined;
buildxCacheTo: string | undefined;
platformInfo: PlatformInfo;
}

export interface ResolverResult {
Expand Down Expand Up @@ -222,10 +223,9 @@ export async function inspectDockerImage(params: DockerResolverParameters | Dock
if (!pullImageOnError) {
throw err;
}
const cliHost = 'cliHost' in params ? params.cliHost : params.common.cliHost;
const output = 'cliHost' in params ? params.output : params.common.output;
try {
return await inspectImageInRegistry(output, { arch: cliHost.arch, os: cliHost.platform }, imageName);
return await inspectImageInRegistry(output, params.platformInfo, imageName);
} catch (err2) {
output.write(`Error fetching image details: ${err2?.message}`);
}
Expand All @@ -244,7 +244,7 @@ export async function inspectDockerImage(params: DockerResolverParameters | Dock
}
}

export async function inspectImageInRegistry(output: Log, platformInfo: { arch: NodeJS.Architecture; os: NodeJS.Platform }, name: string): Promise<ImageDetails> {
export async function inspectImageInRegistry(output: Log, platformInfo: PlatformInfo, name: string): Promise<ImageDetails> {
const resourceAndVersion = qualifyImageName(name);
const params = { output, env: process.env };
const ref = getRef(output, resourceAndVersion);
Expand Down Expand Up @@ -295,6 +295,8 @@ export async function inspectImageInRegistry(output: Log, platformInfo: { arch:
return {
Id: targetDigest,
Config: obj.config,
Os: platformInfo.os,
Architecture: platformInfo.arch,
};
}

Expand Down Expand Up @@ -464,7 +466,7 @@ export async function runInitializeCommand(params: DockerResolverParameters, use
infoOutput.raw(`\x1b[1mRunning the ${hookName} from devcontainer.json...\x1b[0m\r\n\r\n`);
}

// If we have a command name then the command is running in parallel and
// If we have a command name then the command is running in parallel and
// we need to hold output until the command is done so that the output
// doesn't get interleaved with the output of other commands.
const print = name ? 'end' : 'continuous';
Expand Down
Loading
Loading