Docker compose cleanup #325

KarthikNayak · 2019-12-30T18:21:22Z

Related issue

#324

Proposed changes

Cleanup the existing docker-compose.yml and add a new Dockerfile-dc

Checklist

I have read the contributing guidelines
I have read the security policy
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation within the code base (if appropriate)
I have documented my changes in the
developer guide (if appropriate)

claassistantio · 2019-12-30T18:21:29Z

All committers have signed the CLA.

ducharmemp · 2019-12-30T20:33:38Z

I believe that this might also need a volume mount at $HOME/.oathkeeper.json or something like that? I'm actually in the process of trying to figure out how to set up my API access rules now and struggling with the fact that the full-stack example is also out of date.

I suppose another solution would be to build another image based off of oathkeeper with my config options, but that feels a bit heavy handed.

If the docker-compose isn't meant as an example just ignore me :)

KarthikNayak · 2019-12-31T03:59:30Z

@ducharmemp I'll leave that decision to @aeneasr, currently this docker-compose uses the built in .schemas folder.

aeneasr · 2020-01-02T19:25:03Z

Thank you, there is no need for any type of mounting. We're using packr as part of the build process to embed these files, as you can see here: https://github.com/ory/oathkeeper/blob/master/Makefile#L50-L56

Please update the docker image accordingly :)

KarthikNayak · 2020-01-12T16:57:41Z

@aeneasr I've updated the PR, do have a look! :D

aeneasr · 2020-01-13T11:22:36Z

Dockerfile-dc

+ADD . /app
+WORKDIR /app
+RUN go get -u github.com/gobuffalo/packr/v2/packr2
+RUN CGO_ENABLED=0 GO111MODULE=on GOOS=linux GOARCH=amd64 packr2


packr2 does not need these arguments - see the makefile for reference.

We would still need the GO111MODULE=on argument, this ensures that packr2 doesn't build the boxes as per the path instead of the modules.

right, that makes sense. You can set ENV GO111MODULE on in the dockerfile so go modules are enabled for all commands

Yup, we could do that too!

aeneasr · 2020-01-13T11:23:04Z

Dockerfile-dc

+
+ADD . /app
+WORKDIR /app
+RUN go get -u github.com/gobuffalo/packr/v2/packr2


We are using a fixed version of packr2, GO111MODULE=on go install github.com/gobuffalo/packr/v2/packr2 should be enough - see the makefile for reference

aeneasr · 2020-01-13T11:23:28Z

Dockerfile-dc

@@ -0,0 +1,21 @@
+FROM alpine:3.10


Use golang:1.13-alpine instead. You don't need golang:latest then

Alright, will change that!

aeneasr · 2020-01-13T11:24:40Z

docker-compose.yml

@@ -1,25 +1,10 @@
-version: '2'


This file needs more work - there is for example no config file being loaded. The environment variables also seem to be outdated

Alright, will check and add it!

KarthikNayak · 2020-01-28T10:04:15Z

Just an FYI, got busy with life, picking this up again!

aeneasr · 2020-01-29T13:05:23Z

All good, happens to the best of us :)

KarthikNayak · 2020-01-29T16:12:38Z

@aeneasr You mentioned some of the ENV variables to be deprecated, I tried following https://www.ory.sh/docs/oathkeeper/configure-deploy, but the documentation seems to be outdated here too (it talks about using a DB, which we no longer require). Is there a more recent updated version of the documentation?

aeneasr · 2020-01-30T13:25:06Z

Regarding docs, it was a bug that was introduced two days ago. It was resolved today so everything should be up to date again! :)

aeneasr

Thank you, that looks much better now! Unfortunately, I still have some feedback:

Please remove the (probably commited by accident) oathkeeper binary
Adding config files as part of the Docker Image is bad practice. While it would be ok for demo purposes, it's important that we work with best practices because a lot of people are looking at the stack for guidance. Adding config files (such as private keys) to a Docker Image exposes them effectively to anyone that can pull the image, which is why this is not supposed to be done like that. I've added comments how to address this in the PR.

aeneasr · 2020-02-03T08:39:26Z

Dockerfile-dc

+
+COPY --from=0 /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
+COPY --from=0 /app/oathkeeper /usr/bin/oathkeeper
+COPY --from=0 /app/.docker_compose/config.yaml config.yaml


As I said in the summary comment, adding config files as part of the build process is bad practice. Let's remove these. I've created another image for another project which makes use of volumes to mount the config. It uses a standard alpine-based image with a custom user: https://github.com/ory/kratos/blob/master/.docker/Dockerfile

Then, we simply mount the folder where the config files are at and use the �config flag to define where the config files is saved.

I think a 1:1 copy should work more or less for oathkeeper too. The cool thing is, hot reloading will work! So you can edit your config file on the host and oathkeeper will reload it!

You're right, I'm just wondering if there's a better way to even add the private keys. I've made the changes you've suggested here.

aeneasr · 2020-02-03T08:40:31Z

Dockerfile-dc

+RUN packr2
+RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build
+
+FROM scratch


I think we can use the alpine-based image here, as people might want to docker exec into it to debug or whatever. That way, they'll have a shell available!

Check out another docker image that uses an alpine-based linux and adds a user ory. I'd recommend doing something similar: https://github.com/ory/kratos/blob/master/.docker/Dockerfile

Got rid of the multistage multi image Docker and using golang:1.13-alpine

Add a new file 'Dockerfile-dc' which will primarily be used by Docker Compose to build docker images. Unlike the existing Dockerfile which depends on the Makefile to build the binary, this Dockerfile copies the source code and builds the binary. Copied from: https://github.com/ory/kratos/blob/master/.docker/Dockerfile Signed-off-by: karthik nayak <[email protected]>

Oathkeeper has gone through a couple of changes since the initial draft of the docker compose file, considering these changes and the newly introduced Dockerfile in the previous commit, make these changes to the docker-compose.yml: 1. Bump the version of the compose file to 3. 2. Remove the need for the postgres database app, since Oathkeeper no longer needs a database. 3. Remove the need for the migration app, since we no longer need to migrate since there is no database and the option is deprecated. 4. Use the newly defined Dockerfile 'Dockerfile-dc'. 5. We now serve both API and PROXY from the same app, so we don't need two instances of the app. 6. Add sample config, rules and JWK files to `.docker_compose`, mount this via a volume mount. Signed-off-by: karthik nayak <[email protected]>

aeneasr

Sorry for the late reply and thank you for your awesome work! Good job!

aeneasr requested changes Jan 13, 2020

View reviewed changes

aeneasr requested changes Feb 3, 2020

View reviewed changes

KarthikNayak added 2 commits February 16, 2020 17:11

KarthikNayak requested a review from aeneasr February 21, 2020 10:37

aeneasr approved these changes Mar 2, 2020

View reviewed changes

aeneasr merged commit 1247381 into ory:master Mar 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docker compose cleanup #325

Docker compose cleanup #325

KarthikNayak commented Dec 30, 2019

claassistantio commented Dec 30, 2019 •

edited

Loading

ducharmemp commented Dec 30, 2019

KarthikNayak commented Dec 31, 2019

aeneasr commented Jan 2, 2020

KarthikNayak commented Jan 12, 2020

aeneasr Jan 13, 2020

KarthikNayak Jan 13, 2020

KarthikNayak Jan 28, 2020

aeneasr Jan 29, 2020

KarthikNayak Jan 29, 2020

aeneasr Jan 13, 2020

KarthikNayak Jan 13, 2020

aeneasr Jan 13, 2020

KarthikNayak Jan 13, 2020

aeneasr Jan 13, 2020

KarthikNayak Jan 13, 2020

KarthikNayak commented Jan 28, 2020

aeneasr commented Jan 29, 2020

KarthikNayak commented Jan 29, 2020

aeneasr commented Jan 30, 2020

aeneasr left a comment

aeneasr Feb 3, 2020 •

edited

Loading

KarthikNayak Feb 17, 2020

aeneasr Feb 3, 2020

KarthikNayak Feb 17, 2020

aeneasr left a comment

Docker compose cleanup #325

Docker compose cleanup #325

Conversation

KarthikNayak commented Dec 30, 2019

Related issue

Proposed changes

Checklist

claassistantio commented Dec 30, 2019 • edited Loading

ducharmemp commented Dec 30, 2019

KarthikNayak commented Dec 31, 2019

aeneasr commented Jan 2, 2020

KarthikNayak commented Jan 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarthikNayak commented Jan 28, 2020

aeneasr commented Jan 29, 2020

KarthikNayak commented Jan 29, 2020

aeneasr commented Jan 30, 2020

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

claassistantio commented Dec 30, 2019 •

edited

Loading

aeneasr Feb 3, 2020 •

edited

Loading