Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docker: rootless image #10154

Open
wants to merge 11 commits into
base: master
from
Open

docker: rootless image #10154

wants to merge 11 commits into from

Conversation

@sapk
Copy link
Member

sapk commented Feb 5, 2020

Replace #7129

This solution propose a new docker image tag -rootless with a image using internal ssh running with an unprivileged user.

The main advantage is to provide a breaking solution that users can transition slowly and when adoption and returns are good should become the default.

This will allow us to break past mistake/compatibility for docker image.

How to migrate/test
  • Backup your setup
  • Change volume mountpoint from /data to /var/lib/gitea
  • Rename folder (inside volume) gitea to custom
  • chown -R uid:gid /data volume of --user flag if needed
  • Edit app.ini if needed
    • Set START_SSH_SERVER = true
  • Use image gitea/gitea:latest-rootless (when merged or use sapk/gitea:rootless for testing)
docker/rootless/usr/bin/setup Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 label Feb 6, 2020
sapk added 2 commits Feb 6, 2020
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 6, 2020

Codecov Report

Merging #10154 into master will increase coverage by 0.22%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10154      +/-   ##
==========================================
+ Coverage   43.42%   43.64%   +0.22%     
==========================================
  Files         576      576              
  Lines       79642    79681      +39     
==========================================
+ Hits        34582    34775     +193     
+ Misses      40784    40616     -168     
- Partials     4276     4290      +14
Impacted Files Coverage Δ
models/user.go 49.21% <33.33%> (-0.17%) ⬇️
modules/queue/unique_queue_disk_channel.go 52.56% <0%> (-1.93%) ⬇️
services/pull/check.go 31.48% <0%> (-1.86%) ⬇️
models/issue_label.go 63.79% <0%> (-0.69%) ⬇️
models/error.go 30.4% <0%> (-0.55%) ⬇️
services/pull/pull.go 33.93% <0%> (+1.5%) ⬆️
routers/api/v1/repo/label.go 84% <0%> (+84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47c170b...a08eb08. Read the comment docs.

@sapk sapk removed the status/wip label Feb 6, 2020
@lafriks lafriks added this to the 1.12.0 milestone Feb 6, 2020
sapk added 3 commits Feb 7, 2020
Copy link
Contributor

zeripath left a comment

This looks fine to me, I guess one thing I might suggest is to put a note in the app.ini stating that only internal SSH is supported on rootless installs

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Feb 9, 2020

Speaking of “breaking” items, maybe we make the binary in this image FHS compliant?

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Feb 9, 2020

This looks great! Thanks @sapk for your work on this.

In addition to @zeripath’s review could you add a small section to our documentation’s docker section re:rootless?

@sapk

This comment has been minimized.

Copy link
Member Author

sapk commented Feb 9, 2020

For FHS, I am hesitating for the binary path:

  • 👍 /usr/local/bin/gitea
  • 👎 /usr/lib/gitea/gitea
  • 😄 /usr/bin/gitea

Since we don't have any file I would be in favor to put the binary directly to /usr/local/bin/gitea

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Feb 9, 2020

I think /usr/bin would even be appropriate as it's a pre-installed package

@sapk

This comment has been minimized.

Copy link
Member Author

sapk commented Feb 9, 2020

@zeripath I have looked at docker-library official image and they tend to kept under /usr/local/bin everything that are not package managed (apt, apk, ...).
I will try to follow that and also move entrypoint and setup scripts there.
Ex for redis: https://github.com/docker-library/redis/blob/master/5.0/alpine/Dockerfile#L49

root@2a327f2bc55e:/data# ls /usr/local/bin/
docker-entrypoint.sh  gosu  redis-benchmark  redis-check-aof  redis-check-rdb  redis-cli  redis-sentinel  redis-server
sapk added 2 commits Feb 9, 2020
@sapk

This comment has been minimized.

Copy link
Member Author

sapk commented Feb 9, 2020

The only part that still isn't FHS compliant is that the config file is currently under /var/lib/gitea/custom/conf/app.ini but I hesitate to create a second volume only for the config file.

sapk added 2 commits Feb 9, 2020

TODO

# SSH Container Passthrough (not tested)

This comment has been minimized.

Copy link
@lafriks

lafriks Feb 9, 2020

Member

This will not work on rootless as built-in ssh server will not generate authorized_keys file

identifier: "install-with-docker-rootless"
---

# Installation with Docker

This comment has been minimized.

Copy link
@techknowlogick

techknowlogick Feb 9, 2020

Member

As most of these instructions overlap with regular docker image, perhaps instead of this page, a small section in regular docker page with details of what rootless image is, and how to get it starting at 1.12.0-dev(by appending -rootless to tag name)

This comment has been minimized.

Copy link
@sapk

sapk Feb 10, 2020

Author Member

Possible but some option/env are not anymore used like USER_UID and USER_GID.
I was thinking it might be clearer to use a dedicated page that will replace the other one when becoming default.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Feb 13, 2020

@sapk, create a link?

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Feb 13, 2020

And I think we could also create some necessary envs to skip the installation guide which will override app.ini. Maybe another PR.

curl \
gettext \
git \
sqlite \

This comment has been minimized.

Copy link
@lunny

lunny Feb 13, 2020

Member

Do we really need sqlite?

@sapk

This comment has been minimized.

Copy link
Member Author

sapk commented Feb 13, 2020

Rethinking of the config file for FHS, It maybe a good idea to separate it from the data folder since in most cases it is not needed to backup it and can be generated at each startup of container. The config file could still be backup but it would need to be explicitly defined (with an other volume) if needed. An other advantage is that someone that want to configure via variable would just need to restart the container and not rewrite the config file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.