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 support for file-based environment variables in environment-to-ini #19857

Closed

Conversation

aminosbh
Copy link

Improve environment-to-ini to allow for file content to be set as the value of an environment variable.
Useful when using docker secret and were the secret is mounted as a file in /run/secrets/<SECRET_NAME>.

Any settings in app.ini can be set or overridden with the content of a file by defining an environment variable of the form: GITEA__section_name__KEY_NAME__FILE that points to a file.

Fixes #19856

@Gusted Gusted added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jun 5, 2022
@Gusted Gusted added this to the 1.17.0 milestone Jun 5, 2022
@lunny lunny modified the milestones: 1.17.0, 1.18.0 Jun 16, 2022
@KarolNi
Copy link

KarolNi commented Jul 2, 2022

Fixes #10311

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 2, 2022
@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 21, 2022
@nesono
Copy link

nesono commented Jan 11, 2023

Any reason why this PR was discontinued? This change would be extremely useful imho.

@Doddophonique
Copy link

I agree, this is a key functionality for a lot of production environments. Having it embedded in the code is far better than implementing potentially unsafe workarounds.

@lunny
Copy link
Member

lunny commented Jan 15, 2023

Maybe another prefix like GITEA__FILE__ is better than suffix.

@nesono
Copy link

nesono commented Jan 17, 2023

Maybe another prefix like GITEA__FILE__ is better than suffix.

I found suffixed version to be quite standard with some other Docker containers, but I don't have a strong opinion. Could you land this change for us, @lunny ? It looks like you are a member.

Then, I was writing my own wrapper now, based a previous issue that was discussed here, which works as a workaround for me for now. Here's the code for completion (I modified the original prosposal to be compliant with /bin/sh syntax.

# This file has been copied from the original Docker container, as follows:
# docker run gitea/gitea:1.18.0
# docker exec <container_name>> cat /usr/bin/entrypoint > gitea.entrypoint.sh

# The patch starts here. It's a rewritten form of what was posted here
# https://github.com/go-gitea/gitea/issues/10311.
# >>> SNIP
export_secret_as_env_var()
{
    secret=$1
    envFile="${secret}_FILE"
    envFileName="$(printenv "${envFile}")"
    if [ -n "${envFileName}" ]; then
        if [ -f "${envFileName}" ]; then
          val=$(cat "${envFileName}")
          export "${secret}"="$val"
          echo "${secret} environment variable was set via secret ${envFileName}"
        else
          >&2 echo "Error: Secret ${secret} cannot be set via secret ${envFileName}. Not a file"
        fi
    else
        echo "Warn: ${secret} environment variable ist not defined in secret"
    fi
}

# Set environment variables by their respective secrets
export_secret_as_env_var "GITEA__database__PASSWD"
export_secret_as_env_var "GITEA__database__USER"
export_secret_as_env_var "GITEA__mailer__USER"
export_secret_as_env_var "GITEA__mailer__PASSWD"
# <<< SNAP

It seemed though, that the GITEA__database__PASSWD was already supported, maybe that's worthwhile to check.
I think a username should be protected, too, though. Adds yet another layer, even if it's small.

Cherrs and thanks for reading this far :)

Copy link

@dynamicat dynamicat left a comment

Choose a reason for hiding this comment

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

Can someone review approve and merge this?

@lunny lunny modified the milestones: 1.19.0, 1.20.0 Feb 3, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 27, 2023

Maybe another prefix like GITEA__FILE__ is better than suffix.

"GITEA__FILE__foo__..." conflicts with "[FILE].foo", the "FILE" part might be parsed as the "section_name", according to existing rule "GITEA__section_name__KEY_NAME".

Comment on lines +152 to +166
if isFileBased {
isFile, err := util.IsFile(value)
if err != nil {
log.Fatal("Unable to check if %s is a file. Error: %v", value, err)
}
if isFile {
if content, err := os.ReadFile(value); err == nil {
value = string(content)
} else {
log.Fatal("Failed to load value from file '%s': %v", value, err)
}
} else {
log.Fatal("File '%s' not found", value)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if isFileBased {
isFile, err := util.IsFile(value)
if err != nil {
log.Fatal("Unable to check if %s is a file. Error: %v", value, err)
}
if isFile {
if content, err := os.ReadFile(value); err == nil {
value = string(content)
} else {
log.Fatal("Failed to load value from file '%s': %v", value, err)
}
} else {
log.Fatal("File '%s' not found", value)
}
}
if isFileBased {
if content, err := os.ReadFile(value); err == nil {
value = string(content)
} else {
log.Fatal("Failed to load value from file %q: %v", value, err)
}
}

I guess this is enough?

@wxiaoguang
Copy link
Contributor

Although there are some conflicts, I think this PR could be fine-tuned and merged.

Could maintainers with writer permission do some helps?

@silverwind
Copy link
Member

silverwind commented Apr 26, 2023

Generally I think that what environment-to-ini does needs to move to the main application so that environment variables are recognized everywhere, not just with the docker container because docker is just one of many deployment option.

If someone wants to use env vars, they should be able to. This goes for systemd deployments, gitpod and more.

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label May 1, 2023
@wxiaoguang
Copy link
Contributor

Replaced by Make environment-to-ini support loading key value from file #24832

@wxiaoguang wxiaoguang closed this May 21, 2023
@GiteaBot GiteaBot removed this from the 1.20.0 milestone May 21, 2023
lunny pushed a commit that referenced this pull request May 24, 2023
Replace #19857

Close #19856
Close #10311
Close #10123

Major changes:

1. Move a lot of code from `environment-to-ini.go` to `config_env.go` to
make them testable.
2. Add `__FILE` support
3. Update documents
4. Add tests
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for file-based environment variables
10 participants