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

Publish ct-test-srv container on releases #7891

Merged
merged 7 commits into from
Dec 17, 2024

Conversation

mcpherrinm
Copy link
Contributor

@mcpherrinm mcpherrinm commented Dec 15, 2024

There's an old build of ct-test-srv at https://registry.hub.docker.com/r/letsencrypt/ct-test-srv that I'd like to remove, and replace usage of it with a container pushed automatically from Boulder releases.

I've set up the Github action that I hope is generic enough to also potentially push other containers (like boulder itself) in the future by adding to the matrix.

@mcpherrinm mcpherrinm requested a review from a team as a code owner December 15, 2024 04:09
@mcpherrinm mcpherrinm force-pushed the mattm-ct-test-srv-container branch from 26c536b to cd5c64f Compare December 15, 2024 04:10
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

One overall question: is there a reason to keep these separate from try-release.yml and release.yml? Except for the (not yet used) matrix over dockerfiles, these could just be additional steps in those existing workflows, I believe.

.github/workflows/container-build.yml Outdated Show resolved Hide resolved
.github/workflows/container-build.yml Outdated Show resolved Hide resolved
.github/workflows/container-release.yml Outdated Show resolved Hide resolved
.github/workflows/container-release.yml Outdated Show resolved Hide resolved
.github/workflows/container-build.yml Outdated Show resolved Hide resolved
@mcpherrinm
Copy link
Contributor Author

I'm fine with using the existing workflows, but I was guessing the matrix might get over-complicated. I'll give it a try, though.

@mcpherrinm
Copy link
Contributor Author

If it were to be in the original workflows, would you want it to be part of make-assets.sh or the makefile?

@mcpherrinm
Copy link
Contributor Author

Oh, but I could include a new job in the same workflow, so at least the on: parameters are shared and there's not another file to update GO_VERSION in

@aarongable
Copy link
Contributor

Another job in the same file is a definite improvement. I was imagining it as these same exact steps (no overlap with the makefile or make-assets.sh), just either before or after the other steps that those workflow jobs run.

@mcpherrinm mcpherrinm marked this pull request as draft December 16, 2024 22:00
Just explicitly handle ct-test-srv.

We can re-add a matrix if/when we want a second image, but this is a lot simpler for now.
@mcpherrinm mcpherrinm marked this pull request as ready for review December 16, 2024 22:08
@mcpherrinm
Copy link
Contributor Author

OK, I went ahead and moved it to simple steps, just to build ct-test-srv, in the existing workflows. We can figure out how to handle multiple container images if and when that time comes. I think doing the minimal, straightforward thing is a good choice for now.

@mcpherrinm
Copy link
Contributor Author

There's a completely different tact we could take here too, which is to build a container with exactly the binaries from the release tarball - ct-test-srv, boulder, admin, ceremony.

aarongable
aarongable previously approved these changes Dec 16, 2024
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

LGTM. Agreed that this has less flexibility than the initial workflows, but I think the lack of proliferation of GO_VERSION locations is a bigger upside.

I think having all of the release tarball binaries in this image is also a great idea, though I slightly prefer the idea of having ct-test-srv separate from the other more-production-worthy binaries.

@aarongable aarongable requested a review from a team December 17, 2024 21:32
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Basically LGTM. I'd like to make sure I understand safe handling of GITHUB_TOKEN. Here's it's getting interpolated into the command line with echo "${{ secrets.GITHUB_TOKEN }}". In general command lines are logged.

According to https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-secrets, registered secrets are redacted from the logs. We know GITHUB_TOKEN is a registered secret because we access it using secrets.GITHUB_TOKEN.

So the redaction is the only mechanism in place keeping GITHUB_TOKEN out of the logs, right? I wish we could ask docker login to pull directly from an environment variable, but it does not appear to have that as an option.

According to https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-secrets-in-github-actions#accessing-your-secrets, "If a secret was used in the job, GitHub automatically redacts secrets printed to the log. You should avoid printing secrets to the log intentionally." But it's not clear how else to log into GHCR.

@jsha
Copy link
Contributor

jsha commented Dec 17, 2024

Ah, it looks like we already use the docker/login-action GitHub Action to log into DockerHub: https://github.com/letsencrypt/boulder/blob/2e8c3f75f1e15aad817315d48ab102e443652f16/.github/workflows/boulder-ci.yml#L74-L80

In general I prefer to use plain commands over GitHub Actions, in order to minimize dependencies. But given that we already depend on this one, and it does add a small layer of protection to the credential (it's never logged, so never needs to be redacted), I'm in favor of using docker/login-action to log into GHCR as well.

@mcpherrinm
Copy link
Contributor Author

mcpherrinm commented Dec 17, 2024

OK, so you're right relying on redaction is a bit dodgy. Instead, I can do what the previous step does and inject it into an environment variable, and then printenv it

@mcpherrinm
Copy link
Contributor Author

seems we posted past each other - I've updated to avoid printing, but could use docker/login-action too

@jsha
Copy link
Contributor

jsha commented Dec 17, 2024

I like the printenv approach, and we can also follow up and remove our dependency on docker/login-action as a separate PR.

@jsha jsha merged commit 5b94510 into letsencrypt:main Dec 17, 2024
12 checks passed
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.

3 participants