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

migrate database if app.ini found #5290

Merged
merged 5 commits into from Jan 5, 2019

Conversation

6 participants
@pciavald
Copy link
Contributor

pciavald commented Nov 7, 2018

Hi,

For my installation of gitea, I needed unattended initial config. @techknowlogick told me on Discord that it could be done using then new migrate command, gitea migrate -c /data/gitea/conf/app.ini.

I've added in the Docker entrypoint:

  • check if /data/gitea/conf/app.ini exists
  • change mode and owner
  • migrate database

It works correctly on my machine, in my docker-compose the data volume is mapped with only the config file and the platform auto-inits. However I can see in the logs this error which does not affect behaviour:

migrate: /data/gitea/conf/app.ini: Permission denied
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 7, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@cbc14df). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5290   +/-   ##
=========================================
  Coverage          ?   37.83%           
=========================================
  Files             ?      322           
  Lines             ?    47489           
  Branches          ?        0           
=========================================
  Hits              ?    17968           
  Misses            ?    26935           
  Partials          ?     2586

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 cbc14df...81be951. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 label Nov 7, 2018

chmod 644 /data/gitea/conf/app.ini
chown -R 1000:1000 /data/git /data/gitea
su - git -c gitea migrate -c /data/gitea/conf/app.ini
fi

This comment has been minimized.

@techknowlogick

techknowlogick Nov 7, 2018

Member

Please use ${USER}, ${USER_UID}, and ${USER_GID} variables instead of hardcoding them.

This comment has been minimized.

@pciavald

pciavald Nov 7, 2018

Contributor

${USER_UID}, ${USER_GID}, $UID and $GID are undefined, i'll use $USER:$USER is that ok ?

This comment has been minimized.

@pciavald

pciavald Nov 7, 2018

Contributor

I see these variables (USER_UID and USER_GID) should be defined by default to 1000 if not specified by the docker-compose environment key, and they're not defined in my docker-compose.

So I don't understand why they are undefined, i've echoed them in the entrypoint and only USER was defined. Using $USER:$USER in the chown may create problems if a developer defines the USER_UID and USER_GID in their env.

This comment has been minimized.

@zeripath

zeripath Nov 7, 2018

Contributor

The original entry point script just checks for the variables and if they're there does something. That's why they're undefined.

You may have to adjust it to define the variables if they're not there or empty.

if [ -z "${USER_GID}" ]; then
  USER_GID="`id -g ${USER}`"
fi

And similarly for UID. Otherwise I'd be suspicious that the chown could choose the wrong UID and GID in cases were they were set explicitly - but I'd happily defer to more expert opinion on this.

This comment has been minimized.

@techknowlogick

techknowlogick Nov 8, 2018

Member

Ah yes. You are right @pciavald, I missed that they were undefined. I think that @zeripath's approach of setting a default for the values is a good one.

Show resolved Hide resolved docker/usr/bin/entrypoint Outdated

techknowlogick added some commits Jan 5, 2019

@techknowlogick techknowlogick added this to the 1.8.0 milestone Jan 5, 2019

@bkcsoft bkcsoft added lgtm/need 1 and removed lgtm/need 2 labels Jan 5, 2019

@bkcsoft bkcsoft added lgtm/done and removed lgtm/need 1 labels Jan 5, 2019

@techknowlogick techknowlogick merged commit 0236856 into go-gitea:master Jan 5, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
@apricote
Copy link
Contributor

apricote left a comment

I think there is a small security issue caused by this PR

@@ -22,6 +30,13 @@ for FOLDER in /data/gitea/conf /data/gitea/log /data/git /data/ssh; do
mkdir -p ${FOLDER}
done

if [ -f /data/gitea/conf/app.ini ]; then
echo "Found app.ini config file, migrating database"
chmod 644 /data/gitea/conf/app.ini

This comment has been minimized.

@apricote

apricote Jan 11, 2019

Contributor

This will allow anyone with access to the server to read the app.ini, exposing any credentials (database, metrics, smtp).

This comment has been minimized.

@zeripath

zeripath Jan 11, 2019

Contributor

Hmm... I guess you're right. I guess this should be 600. In some ways this is not so bad because this for docker - you shouldn't have other users on it in any case, that's not the docker way.

This comment has been minimized.

@apricote

apricote Jan 11, 2019

Contributor

Is there any reason to change the file permissions?

This comment has been minimized.

@zeripath

zeripath Jan 11, 2019

Contributor

Possibly not. If you've not set the INSTALL_LOCK setting in your app.ini and you've got the wrong mode then you won't be able to use /install. However I guess we shouldn't be being preventing people from shooting themselves in the foot like that. Pop a PR on if you're able otherwise I can when I'm next at a dev box. @techknowlogick do you agree that perhaps we shouldn't changing the mode here?

This comment has been minimized.

@zeripath

zeripath Jan 11, 2019

Contributor

@pciavald What was your reasoning for changing the mode?

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Jan 14, 2019

@techknowlogick I think unfortunately there may still be problems with this.

The correct place for automigration is likely within the gitea script within the s6 folder. That would require a little refactoring of that to make it work.

However a simple improvement is that we should change the migrate line to:

(cd /app/gitea; exec su-exec $USER /app/gitea/gitea migrate)

as per my comment:

#5688 (comment)

Then this will more closely match how docker starts Gitea so if people have used relative paths etc in their app.ini we're not affected by these.

I won't have a chance to write a pr for this for several days so if you can or get a friendly minion please do.

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