Always specify config file in command and always run as gemstash user. by hadrianvalentine · Pull Request #7 · Intellection/docker-gemstash

hadrianvalentine · 2020-09-18T13:56:17Z

Problem

Currently a bug exists where we only specify the gemstash config when the root user is used.
See output from top below:
v1 Gemstash

v2 Gemstash

As you can see, when we run gemstash v2, which uses rootless containers in, we do not specify a config file. The result of this is that gemstash writes and reads data to and from /home/gemstash/.gemstash instead of /home/gemstash/data where we mount our data volume. As a result we're not able to query gem versions from our restored database.

Solution

Remove the entrypoint.sh file and specify a simpler entrypoint in the Dockerfile.
Results from local build

zacblazic · 2020-09-21T08:02:58Z

Really good job on finding this. 🎉

I'd change the approach you've taken in fixing it though.

I'm thinking we should probably drop the entrypoint.sh entirely as we shouldn't be supporting running as root anyways. You'd probably then set the USER to gemstash and modify the CMD to include tini and the --config-file flag.

Thoughts?

itskingori · 2020-09-21T12:07:45Z

I'm thinking we should probably drop the entrypoint.sh entirely as we shouldn't be supporting running as root anyways. You'd probably then set the USER to gemstash and modify the CMD to include tini and the --config-file flag.

I agree that we might as well clean it up. Using entry points was pre-securitycontext where it was a hacky way to force the app running as a certain user. Better to keep the image generic and tweak runtime configuration outside the docker image.

hadrianvalentine · 2020-09-22T08:22:38Z

SGTM 👌

hadrianvalentine · 2020-09-22T09:23:59Z

I'm thinking we should probably drop the entrypoint.sh entirely as we shouldn't be supporting running as root anyways. You'd probably then set the USER to gemstash and modify the CMD to include tini and the --config-file flag.

Done
8fd658c
ef265b9

Also, updated the description with a screen shot of the process from local testing.

Co-authored-by: King'ori Maina <j@kingori.co>

itskingori · 2020-09-23T06:55:41Z

In 5912c04 you removed app/config.yml.erb. I think then we have two options:

You can return it in and update the docker-compose.*.yml files to point to it ... which are used to test it.
Or ... we remove the docker-compose.*.yml so that we don't need it.

In #7 (comment) I made the argument that the Dockefile should be kept generic (which you have done) ... it's just that this repo has docker-compose files that are representative of what would be our kubernetes manifests. And right now they are broken without a custom command:

I'm leaning on no.2 as most of what the docker-compose files demonstrate would be in the Gemstash docs. But keen on Zac's comments. Either way, I don't think this should be a blocker as it seems to me this is turning into a bit of a 🐰 🕳️ for you. We can merge this for the sake of progress and address it later.

zacblazic · 2020-09-23T09:03:08Z

In 5912c04 you removed app/config.yml.erb. I think then we have two options:

You can return it in and update the docker-compose.*.yml files to point to it ... which are used to test it.

Or ... we remove the docker-compose.*.yml so that we don't need it.

In #7 (comment) I made the argument that the Dockefile should be kept generic (which you have done) ... it's just that this repo has docker-compose files that are representative of what would be our kubernetes manifests. And right now they are broken without a custom command:

I'm leaning on no.2 as most of what the docker-compose files demonstrate would be in the Gemstash docs. But keen on Zac's comments. Either way, I don't think this should be a blocker as it seems to me this is turning into a bit of a 🐰 🕳️ for you. We can merge this for the sake of progress and address it later.

I have a bit of a different opinion on the generic-ness idea, but I think I have good a reason for that.

This is a public repository and I'd say the idea would be that anyone else could easily get Gemstash up and running on whatever container runtime or orchestration tooling they're using. To support this notion, you need to be able to spin up the container with as little configuration as possible for the general use-case.

That said I think there are two options which sort of align with your first point.

Non-generic

A configuration file with good defaults and/or environment variable support, added to the image.
A default CMD that makes use of that configuration file.
Specification of the init process (i.e. tini) as the ENTRYPOINT if that is how we intend people to use it.
Specification of the USER to the intended user.
A docker-compose file(s) that sets the required environment variables for the configuration, plus other things like volumes etc.

Generic

A configuration file with good defaults and/or environment variable support, which doesn't necessarily have to be added to the image (i.e. can be used via docker-compose). However, I think always putting a default configuration in the image is a good idea.
Either set no CMD or set it to /bin/sh or similar.
No ENTRYPOINT.
Specification of the USER to the intended user. This is important even for the generic approach as it will allow the container to run rootless by default.
A docker-compose file(s) that mounts a configuration file (if the configuration file is not added to the image), sets the entrypoint, command, environment variables and other things like volumes etc.

As I mentioned I'm in favour of the non-generic approach. Both will be easy to get started from a docker-compose perspective, but for the generic approach, moving it to another platform such as Kubernetes will require that the user always specifies things like command, entrypoint, custom configuration file etc. The non-generic approach will however provide a simpler setup process while still having the flexibility to override the defaults we set in the Dockerfile.

That said, I echo your notion that this should not block @hadrianvalentine too much. I'm okay with whatever we do but just dropping my opinion!

hadrianvalentine · 2020-09-23T09:27:27Z

I should've explained where I think this should go in light of King's initial suggestion of being more generic. I don't think there's any reason we should be explicitly specifying the config location when Gemstash already has a sensible default. I was planning on mounting the config file to the appropriate default locations from a configmap. In the config map we would specify base_path which is what gemstash would use to store that data. If we do that we'll have a generic image that should just work and how we handle configuration will be more inline with what we're currently doing for other services.

itskingori · 2020-09-23T09:29:04Z

I should've explained where I think this should go in light of King's initial suggestion of being more generic. I don't think there's any reason we should be explicitly specifying the config location when Gemstash already has a sensible default.

Ah, yes. That location is the default so it should Just Work™! 🤦🏽‍♂️ Then I think we're good to go!! 💪🏽

zacblazic · 2020-09-23T09:34:07Z

@@ -1,33 +0,0 @@
-:base_path: <%= "#{ENV['GEMSTASH_HOME']}/data" %>


We should put this back.

🤒 ... this is true!

#7 (comment)
☝️ As suggested above, I was going to put it in the config map.

Oops, I'm referring to the file itself.

Right right! 🤕

I blame the pain meds! 😅 Lemme take a step back.

itskingori · 2020-09-23T09:38:33Z

I should've explained where I think this should go in light of King's initial suggestion of being more generic. I don't think there's any reason we should be explicitly specifying the config location when Gemstash already has a sensible default.

Ah, yes. That location is the default so it should Just Work™! 🤦🏽‍♂️ Then I think we're good to go!! 💪🏽

I take this back. I think the config file shouldn't have been deleted. We read envs through it.

itskingori · 2020-09-23T09:39:24Z

Sorry @hadrianvalentine I feel like I've misled you a bit. 🙈 🙉 🙊

hadrianvalentine · 2020-09-23T09:41:41Z

Sorry @hadrianvalentine I feel like I've misled you a bit. 🙈 🙉 🙊

I don't think so. Unless I'm missing something, we should be able to remove config here and specify it in our manifests instead.

itskingori · 2020-09-23T09:42:59Z

I don't think so. Unless I'm missing something, we should be able to remove config here and specify it in our manifests instead.

Yeah. I think I'm responding too quickly without thinking everything through ... so gonna take a step back for now. 🙈

zacblazic · 2020-09-23T09:44:52Z

Sorry @hadrianvalentine I feel like I've misled you a bit. 🙈 🙉 🙊

I don't think so. Unless I'm missing something, we should be able to remove config here and specify it in our manifests instead.

We can't remove it from here without breaking docker-compose etc. IMO we should keep the config baked into the image and just override with a config map if necessary.

hadrianvalentine · 2020-09-23T09:49:57Z

We can't remove it from here without breaking docker-compose etc. IMO we should keep the config baked into the image and just override with a config map if necessary.

Gemstash will revert to its default config. Gemstash isn't aware of that file unless you explicitly tell it to be. I've just build and run successfully using docker compose.

hadrianvalentine · 2020-09-23T09:51:08Z

Perhaps we should remove volumes though to make it more generic?

itskingori · 2020-09-23T09:52:40Z

Perhaps we should remove volumes though to make it more generic?

Agreed. I don't like image that specify VOLUME. Should be up to the user to mount a volume as they see fit.

zacblazic · 2020-09-23T09:53:10Z

We can't remove it from here without breaking docker-compose etc. IMO we should keep the config baked into the image and just override with a config map if necessary.

Gemstash will revert to its default config. Gemstash isn't aware of that file unless you explicitly tell it to be. I've just build and run successfully using docker compose.

Could you post what the config looks like? I don't think it will support the environment variables we use in our compose files:

hadrianvalentine · 2020-09-23T10:08:43Z

These appear to be the defaults for the version we're currently using unless a config file is present:
https://github.com/rubygems/gemstash/blob/v2.0.0/lib/gemstash/configuration.rb#L8-L18

I think the only thing we'd need to change is removing the volume from the dockerfile and the docker-compose files since the user should be able to specify their own volume location via config.

Thoughts?

zacblazic · 2020-09-23T10:27:51Z

These appear to be the defaults for the version we're currently using unless a config file is present:
https://github.com/rubygems/gemstash/blob/v2.0.0/lib/gemstash/configuration.rb#L8-L18

Yeah, so the default one won't work with our docker-compose files in this repository. If we want to toss the config we need to toss the docker-compose files and remove references to them in the readme.

I think the only thing we'd need to change is removing the volume from the dockerfile and the docker-compose files since the user should be able to specify their own volume location via config.

Thoughts?

Don't mind removing the VOLUME from the Dockerfile, as for the docker-compose files, see above.

hadrianvalentine · 2020-09-25T08:08:51Z

I've been thinking about this issue. I think there are two solutions to the compose file issue (aside from just deleting them):

One is to read the config file and continue to back it into the image. We can then decide if we use it or if we mount config over it with Kubernetes.
The other is to use docker configs https://docs.docker.com/engine/swarm/configs/.
This appears to be compatible with docker-compose 3 https://docs.docker.com/compose/compose-file/#configs.

I think quite like the docker configs approach because it means we can keep the images clean and not bake in unnecessary config. Thoughts?

itskingori · 2020-09-25T08:17:14Z

I think quite like the docker configs approach because it means we can keep the images clean and not bake in unnecessary config. Thoughts?

I like the config approach too! If you're willing to give it a stab I'm ok with that.

This reverts commit 91761f4.

This reverts commit 587b2b7.

This reverts commit 9cbb3ad.

This reverts commit 5912c04.

hadrianvalentine · 2020-09-29T10:25:35Z

Reverted changes to remove config from images as discussed in standup.

hadrianvalentine · 2020-09-29T11:56:55Z

Update the changelog c2df0e9.

Always specify config file in command and always run as gemstash user.

17ba8f8

hadrianvalentine requested a review from a team as a code owner September 18, 2020 13:56

hadrianvalentine added 2 commits September 22, 2020 11:18

Stop using entrypoint file in build and install Bundler.

8fd658c

Remove entrypoint file.

ef265b9

hadrianvalentine commented Sep 22, 2020

View reviewed changes

Comment thread Dockerfile

itskingori reviewed Sep 22, 2020

View reviewed changes

Comment thread Dockerfile

hadrianvalentine and others added 2 commits September 22, 2020 12:30

Remove config file.

5912c04

Remove entrypoint and configf file argument.

9cbb3ad

Co-authored-by: King'ori Maina <j@kingori.co>

itskingori previously approved these changes Sep 23, 2020

View reviewed changes

zacblazic requested changes Sep 23, 2020

View reviewed changes

Set user.

587b2b7

hadrianvalentine dismissed itskingori’s stale review via 587b2b7 September 23, 2020 09:37

Stop specifying volume in Dockerfile and docker-compose files.

91761f4

hadrianvalentine added 4 commits September 29, 2020 12:21

Revert "Stop specifying volume in Dockerfile and docker-compose files."

f919190

This reverts commit 91761f4.

Revert "Set user."

40445a4

This reverts commit 587b2b7.

Revert "Remove entrypoint and configf file argument."

b38e246

This reverts commit 9cbb3ad.

Revert "Remove config file."

c2ae036

This reverts commit 5912c04.

zacblazic previously approved these changes Sep 29, 2020

View reviewed changes

Update changelog.

c2df0e9

hadrianvalentine dismissed zacblazic’s stale review via c2df0e9 September 29, 2020 11:56

zacblazic approved these changes Sep 29, 2020

View reviewed changes

hadrianvalentine merged commit 6bf18e9 into master Sep 29, 2020

delete-merged-branch Bot deleted the use_config_file branch September 29, 2020 11:59

		@@ -1,33 +0,0 @@
		:base_path: <%= "#{ENV['GEMSTASH_HOME']}/data" %>

Conversation

hadrianvalentine commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Uh oh!

zacblazic commented Sep 21, 2020

Uh oh!

itskingori commented Sep 21, 2020

Uh oh!

hadrianvalentine commented Sep 22, 2020

Uh oh!

Uh oh!

hadrianvalentine commented Sep 22, 2020

Uh oh!

Uh oh!

itskingori commented Sep 23, 2020

Uh oh!

zacblazic commented Sep 23, 2020

Uh oh!

hadrianvalentine commented Sep 23, 2020

Uh oh!

itskingori commented Sep 23, 2020

Uh oh!

zacblazic Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

itskingori Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

hadrianvalentine Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

zacblazic Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

itskingori Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

itskingori Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itskingori commented Sep 23, 2020

Uh oh!

itskingori commented Sep 23, 2020

Uh oh!

hadrianvalentine commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itskingori commented Sep 23, 2020 • edited by hadrianvalentine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zacblazic commented Sep 23, 2020 • edited by hadrianvalentine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hadrianvalentine commented Sep 23, 2020

Uh oh!

hadrianvalentine commented Sep 23, 2020

Uh oh!

itskingori commented Sep 23, 2020

Uh oh!

zacblazic commented Sep 23, 2020

Uh oh!

hadrianvalentine commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zacblazic commented Sep 23, 2020

Uh oh!

hadrianvalentine commented Sep 25, 2020

Uh oh!

itskingori commented Sep 25, 2020

Uh oh!

hadrianvalentine commented Sep 29, 2020

Uh oh!

hadrianvalentine commented Sep 29, 2020

Uh oh!

Reviewers

Assignees

hadrianvalentine commented Sep 18, 2020 •

edited

Loading

itskingori Sep 23, 2020 •

edited

Loading

hadrianvalentine commented Sep 23, 2020 •

edited

Loading

itskingori commented Sep 23, 2020 •

edited by hadrianvalentine

Loading

zacblazic commented Sep 23, 2020 •

edited by hadrianvalentine

Loading

hadrianvalentine commented Sep 23, 2020 •

edited

Loading