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.

Sign up for GitHub

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

Jump to bottom

fix: set proxy env variable when launching process #3838

Merged
merged 3 commits into from Sep 11, 2023
Merged

fix: set proxy env variable when launching process #3838

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2582105
fix: set proxy env variable when launching process
jeffmaury Sep 8, 2023
3037cdd
fix: add missing parameter
jeffmaury Sep 8, 2023
6faa9e5
refactor: exec now a class
jeffmaury Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 10 additions & 4 deletions packages/main/src/plugin/contribution-manager.spec.ts
View file
Open in desktop
Expand Up @@ -31,6 +31,7 @@ import * as exec from './util/exec.js';
import type { RunResult } from '@podman-desktop/api';
import { EventEmitter } from 'stream-json/Assembler.js';
import type { ContributionInfo } from './api/contribution-info.js';
import type { Proxy } from './proxy.js';
let contributionManager: TestContributionManager;

let composeFileExample: any;
Expand Down Expand Up @@ -86,8 +87,12 @@ const directories = {
getExtensionsStorageDirectory: () => '/fake-extensions-storage-directory',
} as unknown as Directories;

const proxy = {
isEnabled: vi.fn().mockReturnValue(false),
} as unknown as Proxy;

beforeAll(() => {
contributionManager = new TestContributionManager(apiSender, directories, containerProviderRegistry);
contributionManager = new TestContributionManager(apiSender, directories, containerProviderRegistry, proxy);
});

const originalConsoleLogMethod = console.log;
Expand Down Expand Up @@ -435,7 +440,7 @@ describe('startVms', () => {

describe('startVM', () => {
beforeAll(() => {
contributionManager = new TestContributionManager(apiSender, directories, containerProviderRegistry);
contributionManager = new TestContributionManager(apiSender, directories, containerProviderRegistry, proxy);
});

test('start a VM without compose file', async () => {
Expand Down Expand Up @@ -473,7 +478,7 @@ describe('startVM', () => {

describe('isPodmanDesktopServiceAlive', () => {
beforeAll(() => {
contributionManager = new TestContributionManager(apiSender, directories, containerProviderRegistry);
contributionManager = new TestContributionManager(apiSender, directories, containerProviderRegistry, proxy);
});

test('is Alive', async () => {
Expand Down Expand Up @@ -544,7 +549,7 @@ describe('isPodmanDesktopServiceAlive', () => {

describe('waitForRunningState', () => {
beforeAll(() => {
contributionManager = new TestContributionManager(apiSender, directories, containerProviderRegistry);
contributionManager = new TestContributionManager(apiSender, directories, containerProviderRegistry, proxy);
});

test('should work after few times', async () => {
Expand Down Expand Up @@ -597,6 +602,7 @@ test('execComposeCommand', async () => {
// check
expect(exec.exec).toBeCalledWith(
'/my/compose',
proxy,
['arg1', 'arg2'],
expect.objectContaining({ env: { DOCKER_HOST: 'unix:///my/socket' }, cwd: '/fake/directory' }),
);
Expand Down
6 changes: 4 additions & 2 deletions packages/main/src/plugin/contribution-manager.ts
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@ import * as jsYaml from 'js-yaml';
import type { RunResult } from '@podman-desktop/api';
import type { ContainerProviderRegistry } from './container-registry.js';
import { isLinux, isMac, isWindows } from '../util.js';
import type { Proxy } from './proxy.js';

export interface DockerExtensionMetadata {
name: string;
Expand Down Expand Up @@ -73,6 +74,7 @@ export class ContributionManager {
private apiSender: ApiSenderType,
private directories: Directories,
private containerRegistry: ContainerProviderRegistry,
private proxy: Proxy,
) {}

// load the existing contributions
Expand Down Expand Up @@ -169,7 +171,7 @@ export class ContributionManager {

// check if docker-compose can be executed
try {
await exec(binary, ['--version']);
await exec(binary, this.proxy, ['--version']);
// yes, return it
return binary;
} catch (error) {
Expand Down Expand Up @@ -346,7 +348,7 @@ export class ContributionManager {
// add DOCKER_HOST
DOCKER_HOST: DOCKER_HOST,
};
return exec(composeBinary, args, { env, cwd });
return exec(composeBinary, this.proxy, args, { env, cwd });
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down
2 changes: 1 addition & 1 deletion packages/main/src/plugin/extension-loader.ts
View file
Open in desktop
Expand Up @@ -930,7 +930,7 @@ export class ExtensionLoader {
args?: string[],
options?: containerDesktopAPI.RunOptions,
): Promise<containerDesktopAPI.RunResult> => {
return exec(command, args, options);
return exec(command, this.proxy, args, options);
},
};

Expand Down
2 changes: 1 addition & 1 deletion packages/main/src/plugin/index.ts
View file
Open in desktop
Expand Up @@ -698,7 +698,7 @@ export class PluginSystem {
// setup security restrictions on links
await this.setupSecurityRestrictionsOnLinks(messageBox);

const contributionManager = new ContributionManager(apiSender, directories, containerProviderRegistry);
const contributionManager = new ContributionManager(apiSender, directories, containerProviderRegistry, proxy);
this.ipcHandle('container-provider-registry:listContainers', async (): Promise<ContainerInfo[]> => {
return containerProviderRegistry.listContainers();
});
Expand Down
134 changes: 130 additions & 4 deletions packages/main/src/plugin/util/exec.spec.ts
View file
Open in desktop
Expand Up @@ -22,10 +22,15 @@ import * as util from '../../util.js';
import type { ChildProcessWithoutNullStreams } from 'child_process';
import { spawn } from 'child_process';
import type { Readable } from 'node:stream';
import type { Proxy } from '../proxy.js';

/* eslint-disable @typescript-eslint/no-explicit-any */

describe('exec', () => {
const proxy: Proxy = {
isEnabled: vi.fn().mockReturnValue(false),
} as unknown as Proxy;

test('should run the command and resolve with the result', async () => {
const command = 'echo';
const args = ['Hello, World!'];
Expand All @@ -51,7 +56,7 @@ describe('exec', () => {
}),
} as any);

const { stdout } = await exec(command, args);
const { stdout } = await exec(command, proxy, args);

expect(spawnMock).toHaveBeenCalledWith(command, args, { env: expect.any(Object) });
expect(stdout).toBeDefined();
Expand Down Expand Up @@ -84,7 +89,7 @@ describe('exec', () => {
}),
} as any);

const { stdout } = await exec(command, args, { cwd });
const { stdout } = await exec(command, proxy, args, { cwd });

// caller should contains the cwd provided
expect(spawnMock).toHaveBeenCalledWith(command, args, expect.objectContaining({ cwd: cwd }));
Expand Down Expand Up @@ -116,7 +121,7 @@ describe('exec', () => {
}),
} as any);

await expect(exec(command)).rejects.toThrowError(/Command execution failed with exit code 1/);
await expect(exec(command, proxy)).rejects.toThrowError(/Command execution failed with exit code 1/);
});

test('should reject with an error when the execution is cancelled', async () => {
Expand Down Expand Up @@ -154,11 +159,132 @@ describe('exec', () => {
handler();
});

await expect(exec(command, args, options)).rejects.toThrowError(/Execution cancelled/);
await expect(exec(command, proxy, args, options)).rejects.toThrowError(/Execution cancelled/);

expect((childProcessMock as any).kill).toHaveBeenCalled();
expect(options.logger.error).toHaveBeenCalledWith('Execution cancelled');
});

test('should run the command and set HTTP_PROXY', async () => {
const command = 'echo';
const args = ['Hello, World!'];

vi.mock('child_process', () => {
return {
spawn: vi.fn(),
};
});

const on: any = vi.fn().mockImplementationOnce((event: string, cb: (arg0: string) => string) => {
if (event === 'data') {
cb('Hello, World!');
}
}) as unknown as Readable;
const spawnMock = vi.mocked(spawn).mockReturnValue({
stdout: { on, setEncoding: vi.fn() },
stderr: { on, setEncoding: vi.fn() },
on: vi.fn().mockImplementation((event: string, cb: (arg0: number) => void) => {
if (event === 'exit') {
cb(0);
}
}),
} as any);

const httpProxy = {
isEnabled: vi.fn().mockReturnValue(true),
proxy: {
httpProxy: '127.0.0.1:8888',
},
} as unknown as Proxy;

const { stdout } = await exec(command, httpProxy, args);

expect(spawnMock).toHaveBeenCalledWith(command, args, {
env: expect.objectContaining({ HTTP_PROXY: 'http://127.0.0.1:8888' }),
});
expect(stdout).toBeDefined();
expect(stdout).toContain('Hello, World!');
});

test('should run the command and set HTTPS_PROXY', async () => {
const command = 'echo';
const args = ['Hello, World!'];

vi.mock('child_process', () => {
return {
spawn: vi.fn(),
};
});

const on: any = vi.fn().mockImplementationOnce((event: string, cb: (arg0: string) => string) => {
if (event === 'data') {
cb('Hello, World!');
}
}) as unknown as Readable;
const spawnMock = vi.mocked(spawn).mockReturnValue({
stdout: { on, setEncoding: vi.fn() },
stderr: { on, setEncoding: vi.fn() },
on: vi.fn().mockImplementation((event: string, cb: (arg0: number) => void) => {
if (event === 'exit') {
cb(0);
}
}),
} as any);

const httpsProxy = {
isEnabled: vi.fn().mockReturnValue(true),
proxy: {
httpsProxy: '127.0.0.1:8888',
},
} as unknown as Proxy;

const { stdout } = await exec(command, httpsProxy, args);

expect(spawnMock).toHaveBeenCalledWith(command, args, {
env: expect.objectContaining({ HTTPS_PROXY: 'http://127.0.0.1:8888' }),
});
expect(stdout).toBeDefined();
expect(stdout).toContain('Hello, World!');
});

test('should run the command and set NO_PROXY', async () => {
const command = 'echo';
const args = ['Hello, World!'];

vi.mock('child_process', () => {
return {
spawn: vi.fn(),
};
});

const on: any = vi.fn().mockImplementationOnce((event: string, cb: (arg0: string) => string) => {
if (event === 'data') {
cb('Hello, World!');
}
}) as unknown as Readable;
const spawnMock = vi.mocked(spawn).mockReturnValue({
stdout: { on, setEncoding: vi.fn() },
stderr: { on, setEncoding: vi.fn() },
on: vi.fn().mockImplementation((event: string, cb: (arg0: number) => void) => {
if (event === 'exit') {
cb(0);
}
}),
} as any);

const noProxy = {
isEnabled: vi.fn().mockReturnValue(true),
proxy: {
noProxy: '127.0.0.1',
},
} as unknown as Proxy;

const { stdout } = await exec(command, noProxy, args);

expect(spawnMock).toHaveBeenCalledWith(command, args, { env: expect.objectContaining({ NO_PROXY: '127.0.0.1' }) });
expect(stdout).toBeDefined();
expect(stdout).toContain('Hello, World!');
});
});

vi.mock('./util', () => {
Expand Down
15 changes: 14 additions & 1 deletion packages/main/src/plugin/util/exec.ts
View file
Open in desktop
Expand Up @@ -20,16 +20,29 @@ import type { ChildProcessWithoutNullStreams } from 'child_process';
import { spawn } from 'child_process';
import type { RunError, RunOptions, RunResult } from '@podman-desktop/api';
import { isMac, isWindows } from '../../util.js';
import type { Proxy } from '../proxy.js';

export const macosExtraPath = '/usr/local/bin:/opt/homebrew/bin:/opt/local/bin:/opt/podman/bin';

export function exec(command: string, args?: string[], options?: RunOptions): Promise<RunResult> {
export function exec(command: string, proxy: Proxy, args?: string[], options?: RunOptions): Promise<RunResult> {
jeffmaury marked this conversation as resolved.
Show resolved Hide resolved
let env = Object.assign({}, process.env);

if (options?.env) {
env = Object.assign(env, options.env);
}

if (proxy.isEnabled()) {
if (proxy.proxy?.httpsProxy) {
env.HTTPS_PROXY = 'http://' + proxy.proxy.httpsProxy;
jeffmaury marked this conversation as resolved.
Show resolved Hide resolved
}
if (proxy.proxy?.httpProxy) {
env.HTTP_PROXY = 'http://' + proxy.proxy.httpProxy;
}
if (proxy.proxy?.noProxy) {
env.NO_PROXY = proxy.proxy.noProxy;
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it means that if extension is setting a proxy data, it'll be replaced all the times by Podman Desktop ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I don't see the use case

if (isMac() || isWindows()) {
env.PATH = getInstallationPath(env.PATH);
} else if (env.FLATPAK_ID) {
Expand Down