Page MenuHomePhabricator
Differential D3593

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

Authored by varun on Mar 31 2022, 1:38 PM.
Tags
None
Referenced Files
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
Unknown Object (File)
Sun, Oct 13, 9:19 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
• benschac
tomek

Details

Reviewers
• jim
• karol
atul
Commits
rCOMMdd5892941ba1: [services][identity] Dockerfile for the identity service
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 created this revision.Mar 31 2022, 1:38 PM
Herald added subscribers: • abosh, • benschac, • adrian and 2 others. · View Herald TranscriptMar 31 2022, 1:38 PM
Harbormaster completed remote builds in B7787: Diff 10922.Mar 31 2022, 1:43 PM
varun requested review of this revision.Mar 31 2022, 1:43 PM
varun added 1 blocking reviewer(s): • jim.Apr 1 2022, 1:10 PM
varun updated this revision to Diff 10967.Apr 1 2022, 1:14 PM

add newline

Harbormaster completed remote builds in B7805: Diff 10967.Apr 1 2022, 1:21 PM
varun mentioned this in D3569: [services][identity] initial commit for the identity service..Apr 1 2022, 2:19 PM
• jim added a comment.Apr 4 2022, 1:28 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 planned changes to this revision.Apr 5 2022, 12:16 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.

varun added a child revision: D3661: [services][identity] Add .dockerignore file for Identity Service.Apr 7 2022, 3:18 PM
varun updated this revision to Diff 11316.Apr 11 2022, 12:21 PM

address feedback

varun removed a parent revision: D3590: [services][identity] add identity service port to the .env file for docker-compose.Apr 11 2022, 12:21 PM
varun removed a child revision: D3661: [services][identity] Add .dockerignore file for Identity Service.
varun added a parent revision: D3661: [services][identity] Add .dockerignore file for Identity Service.
Harbormaster completed remote builds in B8091: Diff 11316.Apr 11 2022, 12:27 PM
ashoat added a comment.Apr 12 2022, 11:24 AM

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

varun updated this revision to Diff 11448.Apr 13 2022, 12:07 PM
varun added a parent revision: D3590: [services][identity] add identity service port to the .env file for docker-compose.

Add Identity service to docker-compose.yml

Harbormaster failed remote builds in B8179: Diff 11448!Apr 13 2022, 12:10 PM
varun updated this revision to Diff 11459.Apr 13 2022, 1:06 PM

remove vcs from container rust project

Harbormaster failed remote builds in B8189: Diff 11459!Apr 13 2022, 1:09 PM
varun updated this revision to Diff 11460.Apr 13 2022, 1:11 PM

undo last change

Harbormaster failed remote builds in B8190: Diff 11460!Apr 13 2022, 1:19 PM
varun updated this revision to Diff 11461.Apr 13 2022, 1:21 PM

change workdir to $HOME/app/identity

Harbormaster failed remote builds in B8191: Diff 11461!Apr 13 2022, 1:24 PM
varun updated this revision to Diff 11462.Apr 13 2022, 1:58 PM

change workdir and create directory as comm user

Harbormaster completed remote builds in B8192: Diff 11462.Apr 13 2022, 2:03 PM
varun added 1 blocking reviewer(s): • jim.Apr 14 2022, 8:40 AM
• jim accepted this revision.Apr 18 2022, 5:45 AM

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
Closed by commit rCOMMdd5892941ba1: [services][identity] Dockerfile for the identity service (authored by varun). · Explain WhyApr 18 2022, 12:35 PM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMMdd5892941ba1: [services][identity] Dockerfile for the identity service.
ashoat added a comment.Apr 18 2022, 9:38 PM

@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.

varun mentioned this in D3818: [Identity] Reorder Dockerfile contents.Apr 22 2022, 2:28 PM

Revision Contents
Changeset List

PathSize
services/
13 lines
identity/
27 lines

Diff 11459

View Options

services/docker-compose.yml

Loading...
View Options

services/identity/Dockerfile

Loading...