Page MenuHomePhabricator

[services][identity] Dockerfile for the identity service
ClosedPublic

Authored by varun on Mar 31 2022, 1:38 PM.
Tags
None
Referenced Files
F3113686: D3593.id11460.diff
Thu, Oct 31, 6:20 PM
Unknown Object (File)
Tue, Oct 22, 5:13 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM
Unknown Object (File)
Sun, Oct 13, 9:19 PM

Details

Summary

We want to containerize the identity service so we can run it on any system that has a Docker engine. This Dockerfile:

  1. Uses Rust as the base image
  2. Creates a new empty shell project
  3. Copies over the manifests (Cago.toml and Cargo.lock)
  4. Builds the app's dependencies to cache them
  5. Copies over the source code
  6. Builds a release version of the binary
  7. Runs the binary

Depends on D3590

Test Plan

Built the docker image with docker build -t identity .. Then ran the container with docker run -dp 50054:50054 --rm --name identity1 identity. Then used BloomRPC to make calls to the service.

Diff Detail

Repository
rCOMM Comm
Branch
varun/identity_service (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun requested review of this revision.Mar 31 2022, 1:43 PM
  1. This should expect the context to be the root directory, like the other services. See how in docker-compose.yml the other services specify the context is "../". Pretty sure the whole point is just to get access to the common proto definitions for now, but it's good for the service builds to be as similar as possible even though this is Rust and the others are C++.
  1. Create a "comm" user with "useradd -m comm" and run everything as that user with "USER comm". See https://linear.app/comm/issue/ENG-780/run-as-non-root-in-service-containers.
services/identity/Dockerfile
3 ↗(On Diff #10967)

This is confusing. I assume this is to cache downloaded dependencies in a Docker layer. If so, write a comment explaining this.

Also, I'm pretty sure USER is already root here.

4 ↗(On Diff #10967)

I don't like this as a top-level directory. Make it "/app/identity" at least.

6 ↗(On Diff #10967)

nit: Drop the ./, it's implied.

You can merge these into "COPY Cargo.toml Cargo.lock ."

12 ↗(On Diff #10967)

Merge these lines. `COPY build.rs src/ proto/ ." Or even better "COPY . ." with a .dockerignore on anything you don't need copied, such as "target".

16 ↗(On Diff #10967)

What's the point of this?

17 ↗(On Diff #10967)

Why do you need rustfmt?

jim requested changes to this revision.Apr 4 2022, 1:28 PM
This revision now requires changes to proceed.Apr 4 2022, 1:28 PM
varun added inline comments.
services/identity/Dockerfile
16 ↗(On Diff #10967)

Just to reduce the image size

17 ↗(On Diff #10967)

the build.rs script depends on it. tonic-build is configurable, though, so there's probably a way to disable formatting when building a release binary. didn't think it was worth exploring that.

Does it make sense to include changes to services/docker-compose.yml in this diff? The context: ../ line would be more obvious if both were included

remove vcs from container rust project

change workdir to $HOME/app/identity

change workdir and create directory as comm user

One thing to fix before landing

services/identity/Dockerfile
24 ↗(On Diff #11462)

This should go right after the USER declaration

This revision is now accepted and ready to land.Apr 18 2022, 5:45 AM

@varun, you landed this without addressing @jimpo's comment. This is really concerning to see – it calls into question whether it's safe for people to accept your diffs with comments.

It is absolutely necessary before landing ANY diff to scan through to see if there are comments. Just because a diff shows up as accepted, does NOT mean you can land it without reading through the comments.

Please address this comment in a follow-up ASAP or create a Linear task to track it.