Consider environment variables during app.ini creation #298

						
				@ -359,0 +367,4 @@

				described in detail on the [env-to-ini](https://github.com/go-gitea/gitea/tree/main/contrib/environment-to-ini)

				page.

				Note that the Prefix on this helm chart is "ENV_TO_INI"

justusbunsi commented

2022-02-27 14:07:57 +00:00

We should explicitly mention the override order inline values.yaml -> additionalConfigSources -> existing environment variables prefixed with ENV_TO_INI.

We should explicitly mention the override order `inline values.yaml -> additionalConfigSources -> existing environment variables prefixed with ENV_TO_INI`.

luhahn marked this conversation as resolved

README.md Outdated

						
				@ -359,0 +374,4 @@

				```yaml

				statefulset:

				  env:

justusbunsi commented

2022-02-27 13:55:28 +00:00

Reusing the existing env key would risk exposing sensitive data to the runtime container after generating the desired app.ini. as this key is added to all containers. Additionally, my first thought was: The Gitea image interprets such environment variables and injects them into the app.ini. Luckily, we use a dedicated prefix which prevents such modification. It might create confusion with the prefix GITEA__ in a long run though. Data disclosure remains a problem in my opinion.
What about having a dedicated gitea.additionalConfigFromEnvs next to gitea.additionalConfigSources which only gets loaded into the init container generating the app.ini? That way it would also be kind of grouped with the other config sources.

Reusing the existing `env` key would risk exposing sensitive data to the runtime container after generating the desired `app.ini`. as this key is added to all containers. Additionally, my first thought was: _The Gitea image interprets such environment variables and injects them into the `app.ini`._ Luckily, we use a dedicated prefix which prevents such modification. It might create confusion with the prefix `GITEA__` in a long run though. Data disclosure remains a problem in my opinion. What about having a dedicated `gitea.additionalConfigFromEnvs` next to `gitea.additionalConfigSources` which only gets loaded into the init container generating the `app.ini`? That way it would also be kind of grouped with the other config sources.

luhahn marked this conversation as resolved

templates/gitea/config.yaml Outdated

						
				@ -64,0 +69,4 @@

				      # If the env variable already exists, we keep its value ${existing_var:-default_value}

				      # '^^' makes the variable content uppercase

				      nested_subst_tmp=ENV_TO_INI__${masked_section^^}__${setting^^}

				      export "ENV_TO_INI__${masked_section^^}__${setting^^}=${!nested_subst_tmp:-$value}"

justusbunsi commented

2022-02-27 14:19:41 +00:00

I see an unresolved issue with env2ini::generate_initial_secrets which generates values that must not change for existing installations (whether existing Helm Chart installation or migrating an existing environment to use the Helm Chart). Prior to that PR it was not an issue and expected behavior to override the default values from env2ini::generate_initial_secrets. Now it is possible to provided such values via environment variables that are being overridden by this method before even processed.

Maybe it could help to read all existing ENV_TO_INI variables into a file prior to any environmental change and restore these values after injecting the other configuration sources to the environment variables. That way it would also be ensured that such externally defined variables would override any other value set during processing.
It would also eliminate the need to use nested substitution. ?

The processing could look like this:

env2ini::read_existing_custom_envs

env2ini::generate_initial_secrets
env2ini::load_config_sources '/env-to-ini-mounts/inlines/'
env2ini::load_config_sources '/env-to-ini-mounts/additionals/'

env2ini::restore_existing_custom_envs

I see an unresolved issue with `env2ini::generate_initial_secrets` which generates values that must not change for existing installations (whether existing Helm Chart installation or migrating an existing environment to use the Helm Chart). Prior to that PR it was not an issue and expected behavior to override the default values from `env2ini::generate_initial_secrets`. Now it is possible to provided such values via environment variables that are being overridden by this method before even processed. Maybe it could help to read all existing `ENV_TO_INI` variables into a file prior to any environmental change and restore these values after injecting the other configuration sources to the environment variables. That way it would also be ensured that such externally defined variables would override any other value set during processing. It would also eliminate the need to use nested substitution. ? The processing could look like this: ```bash env2ini::read_existing_custom_envs env2ini::generate_initial_secrets env2ini::load_config_sources '/env-to-ini-mounts/inlines/' env2ini::load_config_sources '/env-to-ini-mounts/additionals/' env2ini::restore_existing_custom_envs ```

luhahn marked this conversation as resolved

justusbunsi commented

2022-02-27 14:32:22 +00:00

Would also be a replacement for #279.

👍 1

luhahn force-pushed use-env-variables from fa7aaff3c3 to 9c5213da41

2022-03-01 14:45:39 +00:00

Compare

luhahn force-pushed use-env-variables from 9c5213da41 to b623d8a419

2022-03-01 14:51:58 +00:00

Compare

luhahn force-pushed use-env-variables from b623d8a419 to 4b6faf8720

2022-03-02 08:13:49 +00:00

Compare

luhahn force-pushed use-env-variables from 4b6faf8720 to 3de5f6f4ef

2022-03-02 09:13:47 +00:00

Compare

luhahn requested review from justusbunsi 2022-03-02 09:14:25 +00:00

luhahn force-pushed use-env-variables from 3de5f6f4ef to 84ea4fc5c5

2022-03-04 09:55:46 +00:00

Compare

pat-s commented

2022-03-05 16:12:17 +00:00

@luhahn Just a minor thing: Currently your README changes have a forced line-breaking at 80 chars or similar.
This makes it hard to review single sentences and probably also does not go well with https://gitea.com/gitea/helm-chart/src/branch/master/.markdownlint.yaml.

I wonder if a simple markdownlint check should be added to drone to automate this?

EDIT: Apparently there's already a check in the drone yaml:

		.drone.yml
		Lines 26 to 30 in 9530967163
	
				- name: markdown lint

				  pull: always

				  image: docker.io/volkerraschek/markdownlint:latest

				  commands:

				  - markdownlint *.md

@luhahn Just a minor thing: Currently your README changes have a forced line-breaking at 80 chars or similar. This makes it hard to review single sentences and probably also does not go well with https://gitea.com/gitea/helm-chart/src/branch/master/.markdownlint.yaml. I wonder if a simple markdownlint check should be added to drone to automate this? EDIT: Apparently there's already a check in the drone yaml: https://gitea.com/gitea/helm-chart/src/commit/95309671630d7d3bcba97f389da8bd6a8847c8f7/.drone.yml#L26-L30

justusbunsi commented

2022-03-06 12:50:28 +00:00

@luhahn Just a minor thing: Currently your README changes have a forced line-breaking at 80 chars or similar.
This makes it hard to review single sentences and probably also does not go well with https://gitea.com/gitea/helm-chart/src/branch/master/.markdownlint.yaml.

I wonder if a simple markdownlint check should be added to drone to automate this?

EDIT: Apparently there's already a check in the drone yaml:

.drone.yml Lines 26 to 30 in 9530967163

- name: markdown lint

pull: always

image: docker.io/volkerraschek/markdownlint:latest

commands:

- markdownlint *.md

@pat-s I agree this is not the most readable setting right now. Would be another PR to globally change the line length for Markdown Lint.

> @luhahn Just a minor thing: Currently your README changes have a forced line-breaking at 80 chars or similar. > This makes it hard to review single sentences and probably also does not go well with https://gitea.com/gitea/helm-chart/src/branch/master/.markdownlint.yaml. > > I wonder if a simple markdownlint check should be added to drone to automate this? > > EDIT: Apparently there's already a check in the drone yaml: https://gitea.com/gitea/helm-chart/src/commit/95309671630d7d3bcba97f389da8bd6a8847c8f7/.drone.yml#L26-L30 @pat-s I agree this is not the most readable setting right now. Would be another PR to globally change the line length for Markdown Lint.

justusbunsi reviewed 2022-03-06 14:33:48 +00:00

templates/gitea/config.yaml Outdated

						
				@ -104,12 +104,18 @@ stringData:

				      env2ini::log "...Initial secrets generated\n"

				    }

				    env | grep "ENV_TO_INI" >> /tmp/existing-envs

justusbunsi commented

2022-03-06 13:47:29 +00:00

Better use > instead of >>. The directory /tmp is an emptyDir mount. An error within this init container would cause the file content to be appended over and over again. It's probably better to just freshly create the file.

Better use `>` instead of `>>`. The directory `/tmp` is an _emptyDir_ mount. An error within this init container would cause the file content to be appended over and over again. It's probably better to just freshly create the file.

luhahn marked this conversation as resolved

templates/gitea/config.yaml Outdated

						
				@ -111,2 +114,4 @@

				    env2ini::load_config_sources '/env-to-ini-mounts/additionals/'

				    # load existing envs to override auto generated envs

				    source /tmp/existing-envs

justusbunsi commented

2022-03-06 14:33:26 +00:00

I've noticed two different things that need to be handled:

? My initial thoughts about source are not fully correct. It won't work for unescaped special characters. =, ", ', <, > and others will break the init container because reading the existing environment variables using env won't generate a line with proper bash syntax. Imagine having a complex database password with special characters. We could do similar line-by-line processing as we already do with for additionalConfigSources. Most of the edge cases (maybe all) are already handled by it. Suggestion below.
As /tmp is an emptyDir mount which is shared across containers, this file should be removed after loading its content to prevent leaving sensitive data behind.

Suggestion:

This is a slightly modified version of env2ini::read_config_to_env. It does not require section handling as these sections are already included in the environment variable name.

function env2ini::reload_preset_envs() {
  env2ini::log "Reloading preset envs..."

  while read -r line; do
    if [[ -z "${line}" ]]; then
      # skip empty line
      return
    fi

    # 'xargs echo -n' trims all leading/trailing whitespaces and a trailing new line
    local setting="$(awk -F '=' '{print $1}' <<< "${line}" | xargs echo -n)"

    if [[ -z "${setting}" ]]; then
      env2ini::log '  ! invalid setting'
      exit 1
    fi

    local value=''
    local regex="^${setting}(\s*)=(\s*)(.*)"
    if [[ $line =~ $regex ]]; then
      value="${BASH_REMATCH[3]}"
    else
      env2ini::log '  ! invalid setting'
      exit 1
    fi

    env2ini::log "  + '${setting}'"

    export "${setting^^}=${value}"                           # '^^' makes the variable content uppercase
  done < "/tmp/existing-envs"

  rm /tmp/existing-envs
}

The processing could be:

- env | grep "ENV_TO_INI" >> /tmp/existing-envs
- sed -i -e 's/^/export /' /tmp/existing-envs
+ env | grep "ENV_TO_INI" > /tmp/existing-envs

# ... existing code for processing additionalConfigSources

- source /tmp/existing-envs
+ env2ini::reload_preset_envs

I've noticed two different things that need to be handled: - ? My initial thoughts about `source` are not fully correct. It won't work for unescaped special characters. `=`, `"`, `'`, `<`, `>` and others will break the init container because reading the existing environment variables using `env` won't generate a line with proper bash syntax. Imagine having a complex database password with special characters. We could do similar line-by-line processing as we already do with for `additionalConfigSources`. Most of the edge cases (maybe all) are already handled by it. Suggestion below. - As `/tmp` is an _emptyDir_ mount which is shared across containers, this file should be removed after loading its content to prevent leaving sensitive data behind. --- Suggestion: This is a slightly modified version of `env2ini::read_config_to_env`. It does not require section handling as these sections are already included in the environment variable name. ```bash function env2ini::reload_preset_envs() { env2ini::log "Reloading preset envs..." while read -r line; do if [[ -z "${line}" ]]; then # skip empty line return fi # 'xargs echo -n' trims all leading/trailing whitespaces and a trailing new line local setting="$(awk -F '=' '{print $1}' <<< "${line}" | xargs echo -n)" if [[ -z "${setting}" ]]; then env2ini::log ' ! invalid setting' exit 1 fi local value='' local regex="^${setting}(\s*)=(\s*)(.*)" if [[ $line =~ $regex ]]; then value="${BASH_REMATCH[3]}" else env2ini::log ' ! invalid setting' exit 1 fi env2ini::log " + '${setting}'" export "${setting^^}=${value}" # '^^' makes the variable content uppercase done < "/tmp/existing-envs" rm /tmp/existing-envs } ``` The processing could be: ```diff - env | grep "ENV_TO_INI" >> /tmp/existing-envs - sed -i -e 's/^/export /' /tmp/existing-envs + env | grep "ENV_TO_INI" > /tmp/existing-envs # ... existing code for processing additionalConfigSources - source /tmp/existing-envs + env2ini::reload_preset_envs ```

luhahn marked this conversation as resolved

luhahn force-pushed use-env-variables from e1fb479852 to 549922a6b5

2022-03-07 14:52:06 +00:00

Compare

luhahn force-pushed use-env-variables from 549922a6b5 to 70dd892559

2022-03-07 14:52:22 +00:00

Compare

justusbunsi changed title from ~~Improve user defined Environment variables~~ to Consider environment variables during app.ini creation

2022-03-08 15:26:18 +00:00

justusbunsi approved these changes 2022-03-08 15:30:00 +00:00

justusbunsi left a comment

LGTM. Awesome addition. ?