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

Add environment variable support for Docker image #2201

Merged
merged 2 commits into from
Oct 31, 2017

Conversation

twang2218
Copy link
Contributor

  • Add gettext dependencies as we need envsubst command;
  • Modified s6's gitea setup script, instead of cp the template if no
    app.ini exist, it will substitude the envvars and generate the new
    app.ini;
  • Make /docker/etc/templates/app.ini a template contains environment
    variables;

The following environment variable can be set:

  • APP_NAME: (default: Gitea: Git with a cup of tea)
  • APP_MODE: (default: dev)
  • [server]:
    • SSH_DOMAIN: (default: localhost)
    • HTTP_PORT: (default: 3000)
    • ROOT_URL: (default: '')
    • DISABLE_SSH: (default: false)
    • SSH_PORT: (default: 22)
  • [database]:
    • DB_TYPE: (default: sqlite3)
    • DB_HOST: (default: localhost:3306)
    • DB_NAME: (default: gitea)
    • DB_USER: (default: root)
    • DB_PASSWD: (default: ``)
  • [security]:
    • INSTALL_LOCK: (default: true)
    • SECRET_KEY: (default: JPuNRXxX2G)

With these environment variables available, user can easily run the docker image with minor modifications without creating an custom app.ini, such as:

$ docker run -d -p 3000:3000 \
    -e ROOT_URL=http://dev.example.com \
    -e DB_TYPE=mysql \
    -e DB_HOST=1.2.3.4:3306 \
    -e DB_USER=gitea \
    -e DB_PASSWD=Sup3rPassw0rd \
    -e SECRET_KEY=Sup3r3ecre7 \
    gitea/gitea:latest

Signed-off-by: Tao Wang [email protected]

@lunny lunny added the type/enhancement An improvement of existing functionality label Jul 24, 2017
@lunny lunny added this to the 1.3.0 milestone Jul 24, 2017
@sapk
Copy link
Member

sapk commented Jul 30, 2017

This need rebase. This is good to be configurated by env var but

We shoudn't use a default SECRET_KEY.

I think that we shoud only set INSTALL_LOCK to true only if SECRET_KEY set by user otherwise we should display the /install page.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 30, 2017
@twang2218
Copy link
Contributor Author

@sapk Good point, I updated the PR, only set INSTALL_LOCK to true if SECRET_KEY is not empty, and INSTALL_LOCK is empty. And the default SECRET_KEY is removed.

Copy link
Contributor

@vtemian vtemian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR!

INSTALL_LOCK=true
fi

# Substitude the envionrment variables in the template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo: environment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

@@ -12,7 +12,29 @@ fi

if [ ! -f /data/gitea/conf/app.ini ]; then
mkdir -p /data/gitea/conf
cp /etc/templates/app.ini /data/gitea/conf/app.ini

# Set INSTALL_LOCK to true only if SECRET_KEY is not empty and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty obvious from the instruction itself.
Maybe a more useful comment would by why it's needed to set INSTALL_LOCK to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question, why it's needed?

It was obviously that we alway go through the installation process to setup the SECRET_KEY. However, why we have to?

I didn't find any document clear the relationship between INSTALL_LOCK and SECRET_KEY, so I read the code, and I found the SECRET_KEY will be randomly generated ONLY during the installation:

gitea/routers/install.go

Lines 315 to 320 in 2c3a229

var secretKey string
if secretKey, err = base.GetRandomString(10); err != nil {
ctx.RenderWithErr(ctx.Tr("install.secret_key_failed", err), tplInstall, &form)
return
}
cfg.Section("security").Key("SECRET_KEY").SetValue(secretKey)

	var secretKey string
	if secretKey, err = base.GetRandomString(10); err != nil {
		ctx.RenderWithErr(ctx.Tr("install.secret_key_failed", err), tplInstall, &form)
		return
	}
	cfg.Section("security").Key("SECRET_KEY").SetValue(secretKey)

Otherwise, it will try to find the user setting SECRET_KEY first, and if user is not providing the value, it will use the default string !#@FDEWREWR&*(, which is not safe and should be random generated.

SecretKey = sec.Key("SECRET_KEY").MustString("!#@FDEWREWR&*(")

	SecretKey = sec.Key("SECRET_KEY").MustString("!#@FDEWREWR&*(")

ping @sapk and @lunny , could you tell me why the default value for the SECRET_KEY is a static value, instead of a randomly generated key?

I read the issue #455 , I'm still not clear. I think the SECRET_KEY should always be generated if the value is not provided by the user, the static default string should be avoided in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twang2218 It should be.

* Add `gettext` dependencies as we need `envsubst` command;
* Modified s6's gitea setup script, instead of `cp` the template if no
`app.ini` exist, it will substitude the envvars and generate the new
`app.ini`;
* Make `/docker/etc/templates/app.ini` a template contains environment
variables;

Signed-off-by: Tao Wang <[email protected]>
@lafriks
Copy link
Member

lafriks commented Oct 31, 2017

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 31, 2017
@lunny
Copy link
Member

lunny commented Oct 31, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 31, 2017
@codecov-io
Copy link

Codecov Report

Merging #2201 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2201   +/-   ##
=======================================
  Coverage   26.85%   26.85%           
=======================================
  Files          89       89           
  Lines       17600    17600           
=======================================
  Hits         4727     4727           
  Misses      12187    12187           
  Partials      686      686

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 b0b24a2...0fca9c1. Read the comment docs.

@lunny lunny merged commit d545e32 into go-gitea:master Oct 31, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants