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

Verify binaries are for correct platform #223

Merged
merged 3 commits into from
Jul 2, 2024
Merged

Verify binaries are for correct platform #223

merged 3 commits into from
Jul 2, 2024

Conversation

stephanos
Copy link
Contributor

@stephanos stephanos commented Jun 21, 2024

What was changed

Add verification step for Docker images before releasing them.

PS: I didn't add this to the build step because the risk is high that it would fail silently without anyone noticing.

Why?

To ensure we release the right binary for each platform.

Checklist

  1. Closes

  2. How was this tested:

Running from terminal:

Pass

DST_REPO=temporalio SRC_REPO=temporaliotest PASSWORD=wrong USERNAME=wrong IMAGES=server TAG=dummy COMMIT=bc8c2cd1928c4a6dd3330b48a370cf2c10672968 go run src/image_copy/main.go

verifying binaries in '/usr/local/bin' from 'temporaliotest/server:sha-bc8c2cd' for platform 'linux/arm64' are 'ARM'
Unable to find image 'temporaliotest/server:sha-bc8c2cd' locally
sha-bc8c2cd: Pulling from temporaliotest/server
Digest: sha256:aefb7a40c9bb176e96fb4611c3349631afe61a48b6094be551f2e475d9dccdd6
Status: Downloaded newer image for temporaliotest/server:sha-bc8c2cd
/usr/local/bin/dockerize is ARM
/usr/local/bin/tctl is ARM
/usr/local/bin/tctl-authorization-plugin is ARM
/usr/local/bin/temporal is ARM
/usr/local/bin/temporal-server is ARM

verifying binaries in '/usr/local/bin' from 'temporaliotest/server:sha-bc8c2cd' for platform 'linux/amd64' are 'x86'
Unable to find image 'temporaliotest/server:sha-bc8c2cd' locally
sha-bc8c2cd: Pulling from temporaliotest/server
Digest: sha256:aefb7a40c9bb176e96fb4611c3349631afe61a48b6094be551f2e475d9dccdd6
Status: Downloaded newer image for temporaliotest/server:sha-bc8c2cd
/usr/local/bin/dockerize is x86
/usr/local/bin/tctl is x86
/usr/local/bin/tctl-authorization-plugin is x86
/usr/local/bin/temporal is x86
/usr/local/bin/temporal-server is x86

Fail

DST_REPO=temporalio SRC_REPO=temporaliotest PASSWORD=wrong USERNAME=wrong IMAGES=server TAG=dummy COMMIT=e3ae365 go run src/image_copy/main.go                                 

verifying binaries in '/usr/local/bin' from 'temporaliotest/server:sha-e3ae365' for platform 'linux/arm64' are 'ARM'
/usr/local/bin/dockerize is ARM
/usr/local/bin/tctl is ARM
/usr/local/bin/tctl-authorization-plugin is ARM
/usr/local/bin/temporal is ARM
/usr/local/bin/temporal-server is not ARM
/usr/local/bin/temporal-server: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, Go BuildID=zYjsJtF6wN_KxfTbdu3M/JvdfsrR994VzMaLyzT7r/zPP_QXPvSn-BxercUyWN/93737e5xSZZUcGmDZcrc, with debug_info, not stripped

2024/06/21 13:50:04 exit status 1
exit status 1
  1. Any docs updates needed?

verifyImage(src, "linux/arm64", "ARM")
verifyImage(src, "linux/amd64", "x86")

srcWithProto := fmt.Sprintf("docker://%s", src)
Copy link
Contributor Author

@stephanos stephanos Jun 21, 2024

Choose a reason for hiding this comment

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

The docker run was failing with the docker:// prefix.

@stephanos stephanos changed the title Verify binaries Verify binaries are for correct platform Jun 21, 2024
@stephanos stephanos marked this pull request as ready for review June 21, 2024 21:06
@stephanos stephanos requested a review from a team as a code owner June 21, 2024 21:06
"--entrypoint=sh",
imageTag,
"-c", `
apk add -U file > /dev/null
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use text.Template to clean this up

Copy link
Contributor Author

@stephanos stephanos Jul 2, 2024

Choose a reason for hiding this comment

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

@tdeebswihart what issue/advantage do you see here? I think string interpolation is fine for this use case. Since the quotes are easy to get right, escaping isn't an issue and it gives you helpful syntax highlighting to see where the variables are.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't, it's just a nit

@@ -67,21 +70,68 @@ func getTags(dstTag string, updateMajor bool) []string {
return tags
}

func verifyImage(imageTag, platform, check string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should rename check to something like expectedArchitecture as that's what we're looking for

Comment on lines 108 to 114
cmd.Stdout = &out
cmd.Stderr = &out
err := cmd.Run()
fmt.Println(out.String())
if err != nil {
log.Fatal(err.Error())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cmd.Stdout = &out
cmd.Stderr = &out
err := cmd.Run()
fmt.Println(out.String())
if err != nil {
log.Fatal(err.Error())
}
out, err := cmd.CombinedOutput()
fmt.Println(string(out))
if err != nil {
log.Fatal(err.Error())
}

CombinedOutput does all that for you

@stephanos stephanos merged commit afb837c into main Jul 2, 2024
11 checks passed
@stephanos stephanos deleted the verify-binaries branch July 2, 2024 18:54
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.

2 participants