[Identity] Avoid secret generation and slim down size of identity server docker image
ClosedPublic
Actions

Authored by • jon on Jul 19 2023, 7:05 PM.

Details

Reviewers

varun
bartek

Commits

rCOMMd589005d027b: [Identity] Avoid secret generation and slim down size of identity server docker…

Summary

Secrets should not be generated as part of the docker image build. Instead, they need to be mounted in to the container.

Also reduces the image size from 1.81GB to 108MB.

https://linear.app/comm/issue/ENG-4419

Test Plan

docker build -f services/identity/Dockerfile .
# or
cd services
docker compose build identity

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jon created this revision.Jul 19 2023, 7:05 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 19 2023, 7:05 PM

• jon added a child revision: D8581: [Docs] Document how to deploy identity service.Jul 19 2023, 7:08 PM

Harbormaster completed remote builds in B21073: Diff 28875.Jul 19 2023, 7:24 PM

• jon requested review of this revision.Jul 19 2023, 7:24 PM

Nice! I was recently trying something similar for blob service dockerfile, but I could only get down to ~600MB in the runner stage, so 108MB is impressive. (I used a different base image for the runner stage, rust-slim is 250MB already)

Secrets should not be generated as part of the docker image build. Instead, they need to be mounted in to the container.

Shouldn't the volume mapping be added to the docker-compose file? IIRC @varun uses it for deployment.

services/identity/Dockerfile
31–34	Why are you setting `WORKDIR` to some unused directory while copying the executable to `/usr/local/bin`? It doesn't matter much because it's in the PATH but curious about your motivation

bartek added inline comments.Jul 20 2023, 1:07 AM

services/identity/Dockerfile
21	I think you should add the `--locked` flag because `cargo install` seems to ignore `Cargo.lock` by default: https://doc.rust-lang.org/cargo/commands/cargo-install.html#dealing-with-the-lockfile Also tip: With the `--root` flag you can possibly change installation path from `/usr/local/cargo/bin` to another dir

bartek added inline comments.Jul 20 2023, 2:52 AM

services/identity/Dockerfile
28–29	Doesn't `WORKDIR /home/comm/app/identity` already create the directory? (docs link) Shouldn't you switch users with the `USER comm` instruction? Without this, you're still running as root

same questions as @bartek

This revision now requires changes to proceed.Jul 23 2023, 8:07 AM

Address feedback

• jon added inline comments.Jul 24 2023, 6:36 PM

services/identity/Dockerfile
21	I think you should add the --locked Ah, didn't realize, thanks. Also tip: With the --root flag you can possibly change installation path This just needs to match the `COPY --from=builder` line. I don't have a strong preference.
28–29	Doesn't WORKDIR /home/comm/app/identity already create the directory? (docs link) Apparently it does. just used to the bash-ism of `mkdir -p` before using a directory Shouldn't you switch users with the USER comm instruction? Without this, you're still running as root I should, looks like I forgot to carry this over
31–34	Why are you setting WORKDIR to some unused directory For consistency with the builder + legacy behavior. Although `/home/comm` would also work. Honestly didn't give this a lot of thought other than "do something is it's a strict improvement on the old" It doesn't matter much because it's in the PATH That was the goal, really dislike running relative command (e.g. `./target/...`), as there's a implicit dependency on the current work directory.

Harbormaster completed remote builds in B21162: Diff 28984.Jul 24 2023, 6:55 PM

Thanks for explanations!

services/identity/Dockerfile
21	Also tip: With the --root flag you can possibly change installation path This just needs to match the `COPY --from=builder` line. I don't have a strong preference. Yeah, that was just a FYI comment, no preference either