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: preserve original docker config if credentialhelpers exists #30750

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: preserve original docker config if credentialhelpers exists #30750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9bedc44
fix: preserve original docker config if credentialhelpers exists
gyalai-aws Jul 4, 2024
ee16cb1
Merge branch 'main' into cdkasset-docker-config
gyalai-aws Jul 4, 2024
34e2878
fix: remove testing workaround
gyalai-aws Jul 4, 2024
43ee88c
chore: linting findings corrected
gyalai-aws Jul 4, 2024
dd7b91b
fix: creating public getter for configDirectory
gyalai-aws Jul 4, 2024
f1a8103
Merge branch 'main' into cdkasset-docker-config
gyalai-aws Jul 7, 2024
c806ea5
Merge branch 'main' into cdkasset-docker-config
gyalai-aws Jul 8, 2024
403697f
Merge branch 'main' into cdkasset-docker-config
gyalai-aws Jul 12, 2024
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
31 changes: 30 additions & 1 deletion packages/cdk-assets/lib/private/docker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,36 @@ export class Docker {
map[domain] = 'cdk-assets'; // Use docker-credential-cdk-assets for this domain
return map;
}, {});
fs.writeFileSync(path.join(this.configDir, 'config.json'), JSON.stringify({ credHelpers }), { encoding: 'utf-8' });

// Parsing the original Docker config file to ensure additional configuration is not lost like proxy settings
const originalDockerConfig = this.dockerConfig();
const mergedConfig = { ...originalDockerConfig, credHelpers };

fs.writeFileSync(path.join(this.configDir, 'config.json'), JSON.stringify(mergedConfig), { encoding: 'utf-8' });

return true;
}

public dockerConfigFile(): string {
return process.env.CDK_DOCKER_CONFIG_FILE ?? path.join((os.userInfo().homedir ?? os.homedir()).trim() || '/', '.docker', 'config.json');
}

public dockerConfig(): any | undefined {
try {

if (fs.existsSync(this.dockerConfigFile())) {
return JSON.parse(fs.readFileSync(this.dockerConfigFile(), { encoding: 'utf-8' }));
} else {
return {};
}
} catch (e: any) {
if (e instanceof SyntaxError) {
throw new Error(`Unable to parse \'${this.dockerConfigFile()}\' in order to determine the configuration. Please ensure \'${this.dockerConfigFile()}\' is a valid JSON.`);
}
throw e;
Comment on lines +191 to +194
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this worth throwing an error for? Since this is new behavior, we should consider falling back to the old behavior on failures. We can still give a warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is not a big concern since it is only when the config file exists and is invalid. That would indicate the user wants to use the config file, but it is not written correctly. This case also seems rare.

Still something to think about for backwards compatability.

}
}

/**
* Removes any configured Docker config directory.
* All future commands (e.g., `build`, `push`) will use the default config.
Expand Down Expand Up @@ -209,6 +234,10 @@ export class Docker {
}
return flag;
}

public get configDirectory(): string | undefined {
return this.configDir;
}
}

export interface DockerFactoryOptions {
Expand Down
184 changes: 175 additions & 9 deletions packages/cdk-assets/test/private/docker.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import { readFileSync } from 'fs';
import * as os from 'os';
import * as path from 'path';
import * as mockfs from 'mock-fs';
import { Docker } from '../../lib/private/docker';
import { ShellOptions, ProcessFailedError } from '../../lib/private/shell';

type ShellExecuteMock = jest.SpyInstance<ReturnType<Docker['execute']>, Parameters<Docker['execute']>>;

const _ENV = process.env;

describe('Docker', () => {
describe('exists', () => {
let docker: Docker;
Expand Down Expand Up @@ -42,9 +48,9 @@ describe('Docker', () => {
test('throws when the error is a shell failure but the exit code is unrecognized', async () => {
makeShellExecuteMock(() => {
throw new (class extends Error implements ProcessFailedError {
public readonly code = 'PROCESS_FAILED'
public readonly exitCode = 47
public readonly signal = null
public readonly code = 'PROCESS_FAILED';
public readonly exitCode = 47;
public readonly signal = null;

constructor() {
super('foo');
Expand All @@ -58,9 +64,9 @@ describe('Docker', () => {
test('returns false when the error is a shell failure and the exit code is 1 (Docker)', async () => {
makeShellExecuteMock(() => {
throw new (class extends Error implements ProcessFailedError {
public readonly code = 'PROCESS_FAILED'
public readonly exitCode = 1
public readonly signal = null
public readonly code = 'PROCESS_FAILED';
public readonly exitCode = 1;
public readonly signal = null;

constructor() {
super('foo');
Expand All @@ -76,9 +82,9 @@ describe('Docker', () => {
test('returns false when the error is a shell failure and the exit code is 125 (Podman)', async () => {
makeShellExecuteMock(() => {
throw new (class extends Error implements ProcessFailedError {
public readonly code = 'PROCESS_FAILED'
public readonly exitCode = 125
public readonly signal = null
public readonly code = 'PROCESS_FAILED';
public readonly exitCode = 125;
public readonly signal = null;

constructor() {
super('foo');
Expand All @@ -91,4 +97,164 @@ describe('Docker', () => {
expect(imageExists).toBe(false);
});
});

describe('dockerConfigFile', () => {
let docker: Docker;

beforeEach(() => {
docker = new Docker();
process.env = { ..._ENV };
});

afterEach(() => {
process.env = _ENV;
});

test('Can be overridden by CDK_DOCKER_CONFIG_FILE', () => {
const configFile = '/tmp/insertfilenamehere_docker_config.json';
process.env.CDK_DOCKER_CONFIG_FILE = configFile;

expect(docker.dockerConfigFile()).toEqual(configFile);
});

test('Uses homedir if no process env is set', () => {
expect(docker.dockerConfigFile()).toEqual(path.join(os.userInfo().homedir, '.docker', 'config.json'));
});
});

describe('dockerConfig', () => {
let docker: Docker;
const configFile = '/tmp/foo/bar/does/not/exist/config.json';
afterEach(() => {
jest.resetModules();
jest.resetAllMocks();
});

beforeEach(() => {
docker = new Docker();
process.env.CDK_DOCKER_CONFIG_FILE = configFile;
});

afterEach(() => {
mockfs.restore();
process.env = { ..._ENV };
});

test('returns empty object if no config exists', () => {
expect(docker.dockerConfig()).toEqual({});
});

test('returns parsed config if it exists', () => {
mockfs({
[configFile]: JSON.stringify({
proxies: {
default: {
httpProxy: 'http://proxy.com',
httpsProxy: 'https://proxy.com',
noProxy: '.amazonaws.com',
},
},
}),
});

const config = docker.dockerConfig();
expect(config).toBeDefined();
expect(config.proxies).toEqual({
default: {
httpProxy: 'http://proxy.com',
httpsProxy: 'https://proxy.com',
noProxy: '.amazonaws.com',
},
});
});
});

describe('configureCdkCredentials', () => {
let docker: Docker;
const credsFile = '/tmp/foo/bar/does/not/exist/config-cred.json';
const configFile = '/tmp/foo/bar/does/not/exist/config.json';

afterEach(() => {
jest.resetModules();
jest.resetAllMocks();
});

beforeEach(() => {
docker = new Docker();
process.env.CDK_DOCKER_CREDS_FILE = credsFile;
process.env.CDK_DOCKER_CONFIG_FILE = configFile;
});

afterEach(() => {
mockfs.restore();
process.env = { ..._ENV };
});

test('returns false if no cred config exists', () => {
expect(docker.configureCdkCredentials()).toBeFalsy();
});
Comment on lines +193 to +195
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this expected behavior before, or is this new?

If not new, would this have been covered by one of the above tests - i.e. would this have resulted in a shell error?


test('returns true if cred config exists', () => {
mockfs({
[credsFile]: JSON.stringify({
version: '0.1',
domainCredentials: {
'test1.example.com': { secretsManagerSecretId: 'mySecret' },
'test2.example.com': { ecrRepository: 'arn:aws:ecr:bar' },
},
}),
});

expect(docker.configureCdkCredentials()).toBeTruthy();

const config = JSON.parse(readFileSync(path.join(docker.configDirectory!, 'config.json'), 'utf-8'));
expect(config).toBeDefined();
expect(config).toEqual({
credHelpers: {
'test1.example.com': 'cdk-assets',
'test2.example.com': 'cdk-assets',
},
});
});

test('returns true if cred config and docker config exists', () => {
mockfs({
[credsFile]: JSON.stringify({
version: '0.1',
domainCredentials: {
'test1.example.com': { secretsManagerSecretId: 'mySecret' },
'test2.example.com': { ecrRepository: 'arn:aws:ecr:bar' },
},
}),
[configFile]: JSON.stringify({
proxies: {
default: {
httpProxy: 'http://proxy.com',
httpsProxy: 'https://proxy.com',
noProxy: '.amazonaws.com',
},
},
}),
});

expect(docker.configureCdkCredentials()).toBeTruthy();

const config = JSON.parse(readFileSync(path.join(docker.configDirectory!, 'config.json'), 'utf-8'));
expect(config).toBeDefined();
expect(config).toEqual({
credHelpers: {
'test1.example.com': 'cdk-assets',
'test2.example.com': 'cdk-assets',
},
proxies: {
default: {
httpProxy: 'http://proxy.com',
httpsProxy: 'https://proxy.com',
noProxy: '.amazonaws.com',
},
},
});
});
});
});

Loading