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

feat: support different architectures when copying images #570

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions API.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions lambda/Dockerfile
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ COPY go.mod go.sum ./

RUN go env

# RUN go mod download -x

COPY . /ws

RUN mkdir -p /asset/ && \
make OUTPUT=/asset/bootstrap

ENTRYPOINT [ "/asset/bootstrap" ]
11 changes: 8 additions & 3 deletions lambda/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,12 @@ func handler(ctx context.Context, event cfn.Event) (physicalResourceID string, d
return physicalResourceID, data, err
}

log.Printf("SrcImage: %v DestImage: %v", srcImage, destImage)
arch, err := getStrPropsDefault(event.ResourceProperties, ARCHITECTURE, "")
if err != nil {
return physicalResourceID, data, err
}

log.Printf("SrcImage: %v DestImage: %v Architecture: %v", srcImage, destImage, arch)

srcRef, err := alltransports.ParseImageName(srcImage)
if err != nil {
Expand All @@ -82,13 +87,13 @@ func handler(ctx context.Context, event cfn.Event) (physicalResourceID string, d
return physicalResourceID, data, err
}

srcOpts := NewImageOpts(srcImage)
srcOpts := NewImageOpts(srcImage, arch)
srcOpts.SetCreds(srcCreds)
srcCtx, err := srcOpts.NewSystemContext()
if err != nil {
return physicalResourceID, data, err
}
destOpts := NewImageOpts(destImage)
destOpts := NewImageOpts(destImage, arch)
destOpts.SetCreds(destCreds)
destCtx, err := destOpts.NewSystemContext()
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions lambda/main_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a new test case using and asserting the new feature, keeping the old test case as it was.

If the old test case also needs to be changed, please explain why.

Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ func TestMain(t *testing.T) {
destRef, err := alltransports.ParseImageName(destImage)
assert.NoError(t, err)

srcOpts := NewImageOpts(srcImage)
srcOpts := NewImageOpts(srcImage, "arm64")
srcCtx, err := srcOpts.NewSystemContext()
assert.NoError(t, err)
destOpts := NewImageOpts(destImage)
destOpts := NewImageOpts(destImage, "arm64")
destCtx, err := destOpts.NewSystemContext()
assert.NoError(t, err)

Expand Down
54 changes: 47 additions & 7 deletions lambda/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ import (
)

const (
SRC_IMAGE string = "SrcImage"
DEST_IMAGE string = "DestImage"
SRC_CREDS string = "SrcCreds"
DEST_CREDS string = "DestCreds"
SRC_IMAGE string = "SrcImage"
DEST_IMAGE string = "DestImage"
SRC_CREDS string = "SrcCreds"
DEST_CREDS string = "DestCreds"
ARCHITECTURE string = "Architecture"
)

type ECRAuth struct {
Expand All @@ -35,6 +36,33 @@ type ECRAuth struct {
ExpiresAt time.Time
}

var validArchs = []string{
"386",
"amd64",
"amd64p32",
"arm",
"arm64",
"arm64be",
"armbe",
"loong64",
"mips",
"mips64",
"mips64le",
"mips64p32",
"mips64p32le",
"mipsle",
"ppc",
"ppc64",
"ppc64le",
"riscv",
"riscv64",
"s390",
"s390x",
"sparc",
"sparc64",
"wasm",
}

func GetECRRegion(uri string) string {
re := regexp.MustCompile(`dkr\.ecr\.(.+?)\.`)
m := re.FindStringSubmatch(uri)
Expand Down Expand Up @@ -86,14 +114,15 @@ type ImageOpts struct {
requireECRLogin bool
region string
creds string
architecture string
}

func NewImageOpts(uri string) *ImageOpts {
func NewImageOpts(uri string, arch string) *ImageOpts {
requireECRLogin := strings.Contains(uri, "dkr.ecr")
if requireECRLogin {
return &ImageOpts{uri, requireECRLogin, GetECRRegion(uri), ""}
return &ImageOpts{uri, requireECRLogin, GetECRRegion(uri), "", arch}
} else {
return &ImageOpts{uri, requireECRLogin, "", ""}
return &ImageOpts{uri, requireECRLogin, "", "", arch}
}
}

Expand All @@ -109,6 +138,7 @@ func (s *ImageOpts) NewSystemContext() (*types.SystemContext, error) {
ctx := &types.SystemContext{
DockerRegistryUserAgent: "ecr-deployment",
DockerAuthConfig: &types.DockerAuthConfig{},
ArchitectureChoice: s.architecture,
}

if s.creds != "" {
Expand Down Expand Up @@ -184,3 +214,13 @@ func GetSecret(secretId string) (secret string, err error) {
}
return *resp.SecretString, nil
}

func IsValidGOARCH(arch string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used?

for _, validArch := range validArchs {
if arch == validArch {
return true
}
}

return false
}
2 changes: 1 addition & 1 deletion package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 24 additions & 0 deletions publish-ark-image.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this file doing? Why was it added?

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#!/bin/bash

set -e

ECR_URI="758920976184.dkr.ecr.us-east-1.amazonaws.com"
ECR_REPO_URI=$ECR_URI/cdk-ecr-deployment
# Authenticate with AWS ECR
aws ecr get-login-password --profile "$AWS_PROFILE" --region "$AWS_REGION" | docker login --username AWS --password-stdin "$ECR_URI"

# Get the current Git commit hash
GIT_COMMIT_HASH=$(git rev-parse --short HEAD)

# push to registry
# --provenance=true necessary to avoid the error https://stackoverflow.com/a/75149347/4820648
docker buildx build \
--provenance=false \
--file lambda/Dockerfile \
--push \
--tag $ECR_REPO_URI:latest \
--tag $ECR_REPO_URI:$GIT_COMMIT_HASH \
--platform linux/amd64 \
--progress=plain \
lambda/.

6 changes: 6 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ export interface ECRDeploymentProps {
*/
readonly dest: IImageName;

/**
* The architecture of the docker images to copy.
Copy link
Contributor

@mrgrain mrgrain Oct 8, 2024

Choose a reason for hiding this comment

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

Suggested change
* The architecture of the docker images to copy.
* The architecture of the docker images to copy.
*
* @default "x86"

*/
readonly architecture?: string;

/**
* The amount of memory (in MiB) to allocate to the AWS Lambda function which
* replicates the files from the CDK bucket to the destination bucket.
Expand Down Expand Up @@ -192,6 +197,7 @@ export class ECRDeployment extends Construct {
SrcCreds: props.src.creds,
DestImage: props.dest.uri,
DestCreds: props.dest.creds,
Architecture: props.architecture,
Copy link
Contributor

Choose a reason for hiding this comment

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

I reckon we should fix this and move away from defaulting to the lambda architecture to explicitly defining a default here.

},
});
}
Expand Down
2 changes: 2 additions & 0 deletions test/example.ecr-deployment.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

please add e new test.

Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,13 @@ class TestECRDeployment extends Stack {
new ecrDeploy.ECRDeployment(this, 'DeployECRImage', {
src: new ecrDeploy.DockerImageName(image.imageUri),
dest: new ecrDeploy.DockerImageName(`${repo.repositoryUri}:latest`),
architecture: 'arm64',
});

new ecrDeploy.ECRDeployment(this, 'DeployDockerImage', {
src: new ecrDeploy.DockerImageName('javacs3/javacs3:latest', 'dockerhub'),
dest: new ecrDeploy.DockerImageName(`${repo.repositoryUri}:dockerhub`),
architecture: 'arm64',
}).addToPrincipalPolicy(new iam.PolicyStatement({
effect: iam.Effect.ALLOW,
actions: [
Expand Down