-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(aws-cdk): allow uploading Docker images as assets #982
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,12 +21,15 @@ export interface Uploaded { | |
| } | ||
|
|
||
| export class ToolkitInfo { | ||
| public readonly sdk: SDK; | ||
|
|
||
| constructor(private readonly props: { | ||
| sdk: SDK, | ||
| bucketName: string, | ||
| bucketEndpoint: string, | ||
| environment: cxapi.Environment | ||
| }) { } | ||
| }) { | ||
| } | ||
|
|
||
| public get bucketUrl() { | ||
| return `https://${this.props.bucketEndpoint}`; | ||
|
|
@@ -73,6 +76,86 @@ export class ToolkitInfo { | |
| return { filename, key, changed: true }; | ||
| } | ||
|
|
||
| /** | ||
| * Prepare an ECR repository for uploading to using Docker | ||
| */ | ||
| public async prepareEcrRepository(id: string, imageTag: string): Promise<EcrRepositoryInfo> { | ||
rix0rrr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const ecr = await this.props.sdk.ecr(this.props.environment, Mode.ForWriting); | ||
|
|
||
| // Create the repository if it doesn't exist yet | ||
| const repositoryName = 'cdk/' + id.replace(/[:/]/g, '-').toLowerCase(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it really make sense to use a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm treating this akin to the Asset bucket. Why should the user not be allowed to care about the asset bucket name, but be allowed to care about repository names?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay- I agree |
||
|
|
||
| let repository; | ||
| try { | ||
| debug(`${repositoryName}: checking for repository.`); | ||
| const describeResponse = await ecr.describeRepositories({ repositoryNames: [repositoryName] }).promise(); | ||
| repository = describeResponse.repositories![0]; | ||
| } catch (e) { | ||
| if (e.code !== 'RepositoryNotFoundException') { throw e; } | ||
| } | ||
|
|
||
| if (repository) { | ||
| try { | ||
| debug(`${repositoryName}: checking for image ${imageTag}`); | ||
| await ecr.describeImages({ repositoryName, imageIds: [{ imageTag }] }).promise(); | ||
|
|
||
| // If we got here, the image already exists. Nothing else needs to be done. | ||
| return { | ||
| alreadyExists: true, | ||
| repositoryUri: repository.repositoryUri!, | ||
| repositoryArn: repository.repositoryArn!, | ||
| }; | ||
| } catch (e) { | ||
| if (e.code !== 'ImageNotFoundException') { throw e; } | ||
| } | ||
| } else { | ||
| debug(`${repositoryName}: creating`); | ||
| const response = await ecr.createRepository({ repositoryName }).promise(); | ||
| repository = response.repository!; | ||
|
|
||
| // Better put a lifecycle policy on this so as to not cost too much money | ||
| await ecr.putLifecyclePolicy({ | ||
| repositoryName, | ||
| lifecyclePolicyText: JSON.stringify(DEFAULT_REPO_LIFECYCLE) | ||
| }).promise(); | ||
| } | ||
|
|
||
| // The repo exists, image just needs to be uploaded. Get auth to do so. | ||
|
|
||
| debug(`Fetching ECR authorization token`); | ||
| const authData = (await ecr.getAuthorizationToken({ }).promise()).authorizationData || []; | ||
| if (authData.length === 0) { | ||
| throw new Error('No authorization data received from ECR'); | ||
| } | ||
| const token = Buffer.from(authData[0].authorizationToken!, 'base64').toString('ascii'); | ||
| const [username, password] = token.split(':'); | ||
|
|
||
| return { | ||
| alreadyExists: false, | ||
| repositoryUri: repository.repositoryUri!, | ||
| repositoryArn: repository.repositoryArn!, | ||
| username, | ||
| password, | ||
| endpoint: authData[0].proxyEndpoint!, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| export type EcrRepositoryInfo = CompleteEcrRepositoryInfo | UploadableEcrRepositoryInfo; | ||
|
|
||
| export interface CompleteEcrRepositoryInfo { | ||
| repositoryUri: string; | ||
| repositoryArn: string; | ||
| alreadyExists: true; | ||
| } | ||
|
|
||
| export interface UploadableEcrRepositoryInfo { | ||
| repositoryUri: string; | ||
| repositoryArn: string; | ||
| alreadyExists: false; | ||
| username: string; | ||
| password: string; | ||
| endpoint: string; | ||
| } | ||
|
|
||
| async function objectExists(s3: aws.S3, bucket: string, key: string) { | ||
|
|
@@ -114,3 +197,18 @@ function getOutputValue(stack: aws.CloudFormation.Stack, output: string): string | |
| } | ||
| return result; | ||
| } | ||
|
|
||
| const DEFAULT_REPO_LIFECYCLE = { | ||
| rules: [ | ||
| { | ||
| rulePriority: 100, | ||
| description: 'Retain only 5 images', | ||
| selection: { | ||
| tagStatus: 'any', | ||
| countType: 'imageCountMoreThan', | ||
| countNumber: 5, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems excessively low to me. Especially since it is not user-customizable.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it should be like 3 months or something
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In what scenario will those old images be used though? Every repository is tied to a single stack in a single region, and once the stack is updated all references will point to the new image, which immediately follows deployment. I can see retaining two images for quick rollback purposes, and I put a little more margin on there "just because" that feels safe. But 3 months? Don't forget that these images cost money as well. We already have a user starting to complain about the size of their asset bucket.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep in mind, we can't tell if these images are currently deployed somewhere. So if 5 builds happen that fail testing and never get deployed, my production image will be deleted. Time-based policy means I'm banking on deploying at least one a quarter. I would actually say 6 months is safer... |
||
| }, | ||
| action: { type: 'expire' } | ||
| } | ||
| ] | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's
jsiidealing with this? Poorly, I think :(There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Doesn't actually matter as no user ever needs to call this (I hope).
@eladb, why don't we bundle
@aws-cdk/cx-apias part of@aws-cdk/cdk?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we we need to bundle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's an implementation detail that there's a shared package with definitions between
aws-cdkand@aws-cdk/cdk. Nobody else should need to see or interact with it. Therefore it does not need to be exposed over jsii.As a practical matter, not exposing over jsii means we can safely use TypeScript-isms that otherwise wouldn't be allowed.