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: Remove build golang lambda #535

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

fix: Remove build golang lambda #535

wants to merge 35 commits into from

Conversation

wchaws
Copy link
Contributor

@wchaws wchaws commented Feb 20, 2024

Fixes #222

Test Cases

Test project can be found at https://github.com/cdklabs/cdk-ecr-deployment/blob/dev/test/example.ecr-deployment.ts.
Instructions can be found at https://github.com/cdklabs/cdk-ecr-deployment/tree/dev?tab=readme-ov-file#sample-testexampleecr-deploymentts

1. Copy from local cdk docker image asset to ECR repo.

Prerequisite:

  1. An existing ECR repo.
const image = new DockerImageAsset(this, 'CDKDockerImage', {
  directory: path.join(__dirname, 'docker'),
});

new ecrdeploy.ECRDeployment(this, 'DeployDockerImage1', {
  src: new ecrdeploy.DockerImageName(image.imageUri),
  dest: new ecrdeploy.DockerImageName(`${cdk.Aws.ACCOUNT_ID}.dkr.ecr.us-west-2.amazonaws.com/my-nginx:latest`),
});

2. Copy from a ECR repo to another ECR repo.

Prerequisite:

  1. Two existing ECR repo.
new ecrdeploy.ECRDeployment(this, 'DeployDockerImage1', {
  src: new ecrdeploy.DockerImageName(`${cdk.Aws.ACCOUNT_ID}.dkr.ecr.us-west-2.amazonaws.com/AAAAlatest`),
  dest: new ecrdeploy.DockerImageName(`${cdk.Aws.ACCOUNT_ID}.dkr.ecr.us-west-2.amazonaws.com/BBBB:latest`),
});

3. Copy from docker registry to ECR repo.

new ecrdeploy.ECRDeployment(this, 'DeployDockerImage2', {
  src: new ecrdeploy.DockerImageName('nginx:latest'),
  dest: new ecrdeploy.DockerImageName(`${cdk.Aws.ACCOUNT_ID}.dkr.ecr.us-west-2.amazonaws.com/my-nginx2:latest`),
});

4. Copy from private docker registry to ECR repo.

Prerequisite:

  1. An existing ECR repo.
  2. A private docker registry and an access token for the registry.
  3. Create a AWS Secrets Manager secret to store the access token. The format must be <username>:<password>.

Notice: The creds parameter can be the secret name or arn of the aws secrets manager in the same cdk stack region.

image
// Copy from private docker registry to ECR.
// The format of secret in aws secrets manager must be plain text! e.g. <username>:<password>
new ecrdeploy.ECRDeployment(this, 'DeployDockerImage3', {
  src: new ecrdeploy.DockerImageName('your-private-docker-registry/nginx:latest', 'username:password'),
  // src: new ecrdeploy.DockerImageName('your-private-docker-registry/nginx:latest', 'aws-secrets-manager-secret-name'),
  // src: new ecrdeploy.DockerImageName('your-private-docker-registry/nginx:latest', 'arn:aws:secretsmanager:us-west-2:000000000000:secret:id'),
  dest: new ecrdeploy.DockerImageName(`${cdk.Aws.ACCOUNT_ID}.dkr.ecr.us-west-2.amazonaws.com/my-nginx3:latest`),
}).addToPrincipalPolicy(new iam.PolicyStatement({
  effect: iam.Effect.ALLOW,
  actions: [
    'secretsmanager:GetSecretValue',
  ],
  resources: ['*'],
}));

5. Regional test

Select some regions to test if it's able to work

Signed-off-by: github-actions <[email protected]>
@wchaws wchaws changed the title Remove build golang lambda BREAKING CHANGE!: Remove build golang lambda Feb 20, 2024
@wchaws wchaws changed the title BREAKING CHANGE!: Remove build golang lambda fix: Remove build golang lambda Feb 20, 2024
@wchaws wchaws changed the title fix: Remove build golang lambda fix: Remove build golang lambda WIP Feb 20, 2024
@wchaws wchaws changed the title fix: Remove build golang lambda WIP fix: Remove build golang lambda Mar 3, 2024
@wchaws wchaws requested review from kaizencc and scanlonp March 3, 2024 14:32
@wchaws wchaws self-assigned this Mar 16, 2024
@wchaws wchaws requested a review from pahud June 13, 2024 05:52
@xazhao
Copy link

xazhao commented Aug 13, 2024

Starting to look into this PR. Because I'm not familiar with this L2, could you please add some details to help me understand the change?

  • Reason of changes
  • What are changes

I see Python is added and Go is removed. Is there a reason for that? And are you removing all Go code from the repo?

ARCH=x86_64 # or arm64, x86_64, armv6, i386, s390x
VERSION=v0.19.0

curl -sL "https://github.com/google/go-containerregistry/releases/download/${VERSION}/go-containerregistry_${OS}_${ARCH}.tar.gz" > go-containerregistry.tar.gz
Copy link

Choose a reason for hiding this comment

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

Does this script run on customer's machine?

Copy link

Choose a reason for hiding this comment

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

Correct me if I'm wrong: this shell script will run whenever customers run synth().

Instead of downloading it every time when customers use the construct, can we do this part in our release process and publish the layer.zip. So they don't need to connect to Github which has availability risk.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

This PR is doing too much.

It's changing how the asset is built, as well as replacing the code inside the asset and changing functionality.

Those might all be valid changes, but I'd feel a lot better if they were submitted separately in different PRs.

Comment on lines 55 to +59
## Sample: [test/example.ecr-deployment.ts](./test/example.ecr-deployment.ts)

```shell
# Generate crane lambda layer firstly.
./layer/build.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

This section needs some clarification that these instructions only apply if you're developing on the repository, not if you're a user of the construct.

README.md is typically consumer-targeted, and the section title says "Sample", so it should (mostly) only contain instructions for consumers.

These instructions would make more sense in CONTRIBUTING.md in a section title "How to run tests".

If you want to keep them here, clearly label the instructions as intended for developers, not for consumers.

To support a new docker image source(like docker tarball in s3), you need to implement [image transport interface](https://github.com/containers/image/blob/master/types/types.go). You could take a look at [docker-archive](https://github.com/containers/image/blob/ccb87a8d0f45cf28846e307eb0ec2b9d38a458c2/docker/archive/transport.go) transport for a good start.

To test the `lambda` folder, `make test`.
The underlying implementation depends on the https://github.com/google/go-containerregistry.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean?

Comment on lines +1 to +5
import re
import boto3
import base64
import subprocess
import logging
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like making 2 distinct changes in a single PR here.

The problem was: the Lambda code is downloaded at synth time.

The expected solution was: the Lambda code is built at release time and bundled into the NPM package.

You are doing that, and also replacing the code in the Lambda layer with a downloaded binary and a Python wrapper at the same time.

This seems to me like it doesn't appreciably simplify the solution to the immediate problem, as well as introduces another layer of risk that I'd rather avoid. The proposed layer code change should be submitted as a separate PR.

resourceType: 'Custom::CDKBucketDeployment',
properties: {
Time: Date.now().toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior can/should probably be triggered more intelligently. For example, by examining whether or not a tag is supplied, or adding a triggerEveryDeployment?: boolean/mutableSourceUri?: boolean property.

And in any case, functionality changes should not be part of the PR whose sole purpose it is to make the package behave more reliably.

Comment on lines -14 to -23
/**
* Image to use to build Golang lambda for custom resource, if download fails or is not wanted.
*
* Might be needed for local build if all images need to come from own registry.
*
* Note that image should use yum as a package manager and have golang available.
*
* @default public.ecr.aws/sam/build-go1.x:latest
*/
readonly buildImage?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

For maximum user impact, it would be better to leave this parameter in place, and to not bump the Major Version. That way, all users currently using the affected version can transparently upgrade to a fixed version. Ignore the property if supplied and report a warning.

Then, in a future Major Version you can remove this parameter, to have a correct API contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove build golang lambda
3 participants