Generic way for configuring Gitea app.ini #240

Merged
luhahn merged 8 commits from justusbunsi/helm-chart:feature/rework-sensitive-ini-settings into master 8 months ago
Collaborator

With the result of PR #239 it is much easier to provide additional values to the app.ini configuration from different sources.
These changes adds an additionalConfigSources field where the users can define such sources. This enables the users to choose
on their own whether to store values in values.yaml or load them from Kuberetes Secrets or ConfigMaps.

With the result of PR #239 it is much easier to provide additional values to the _app.ini_ configuration from different sources. These changes adds an _additionalConfigSources_ field where the users can define such sources. This enables the users to choose on their own whether to store values in _values.yaml_ or load them from Kuberetes Secrets or ConfigMaps. - Fixes #243 - Fixes #174 - Fixes #260
justusbunsi added this to the 5.0.0 milestone 9 months ago
justusbunsi added the
kind/feature
status/needs-feedback
status/wip
labels 9 months ago
justusbunsi added 2 commits 9 months ago
58dfa3de86 Rework app.ini generation
119d7352b7 Describe additional config sources for app.ini
Poster
Collaborator

Currently, I just wrote down my thoughts without implementing it, because I need a second opinion on what the users should actually define and how. Both ways require some structure for the references Kubernetes resources:

  • Use the current approach, mounted them as files and then converted into environment variables, or
  • Inject them directly as environment variables and let the user take care of things like GITEA__LOG_0x2E_CONSOLE__STDERR=false

The first approach would keep prefixing with GITEA_ to the Helm Chart as well as converting . into 0x2E. The second one is easier for the Helm Chart but more complex for the users.

@luhahn & @lunny, shall I proceed with the first approach? Details here.

EDIT: After chatting with Luhahn, I'll proceed with the current approach (the first one).

Currently, I just wrote down my thoughts without implementing it, because I need a second opinion on what the users should actually define and how. Both ways require some structure for the references Kubernetes resources: - Use the current approach, mounted them as files and then converted into environment variables, or - Inject them directly as environment variables and let the user take care of things like `GITEA__LOG_0x2E_CONSOLE__STDERR=false` The first approach would keep prefixing with `GITEA_` to the Helm Chart as well as converting `.` into `0x2E`. The second one is easier for the Helm Chart but more complex for the users. @luhahn & @lunny, shall I proceed with the first approach? Details [here](https://gitea.com/justusbunsi/helm-chart/src/branch/feature/rework-sensitive-ini-settings/README.md#additional-_app-ini_-settings). EDIT: After chatting with Luhahn, I'll proceed with the current approach (the first one).
justusbunsi force-pushed feature/rework-sensitive-ini-settings from 119d7352b7 to 5def46b650 9 months ago
Poster
Collaborator

Just realized that this fixes neither #174 nor the mentioned oauth topic since these values are not configured inside app.ini. 🤦‍♂️

But I think it is still a neat addition to the chart giving more flexibility about how to configure Gitea.

See PR comment.

~~Just realized that this fixes neither #174 nor the mentioned oauth topic since these values are not configured inside app.ini. 🤦‍♂️~~ ~~But I think it is still a neat addition to the chart giving more flexibility about how to configure Gitea.~~ See PR comment.
justusbunsi added the
kind/security
label 9 months ago
justusbunsi changed title from WIP: Rework sensitive ini settings to WIP: Generic way for configuring Gitea 9 months ago
justusbunsi removed this from the 5.0.0 milestone 9 months ago
justusbunsi added this to the 5.0.0 milestone 9 months ago
justusbunsi added the
status/blocked
label 9 months ago
justusbunsi added a new dependency 9 months ago
justusbunsi force-pushed feature/rework-sensitive-ini-settings from 5def46b650 to 6ead599695 9 months ago
justusbunsi added 1 commit 9 months ago
df3539a056 Process additional config sources
justusbunsi reviewed 9 months ago
{{- end }}
{{- end }}
env2ini::load_additional_config_sources
Poster
Collaborator

The functions I wrote above gives us the possibility to completely rewrite the config.yaml file content. We could split scripts from data and use a ConfigMap for the script instead of Secret. The values that are currently rendered by the Helm template engine could be written to a separate Kubernetes Secret which then gets processed along with the additional config sources.

I think it's out of scope for this PR but I really like the idea of separating user input from Chart script logic.

The functions I wrote above gives us the possibility to completely rewrite the `config.yaml` file content. We could split scripts from data and use a ConfigMap for the script instead of Secret. The values that are currently rendered by the Helm template engine could be written to a separate Kubernetes Secret which then gets processed along with the additional config sources. I think it's out of scope for this PR but I really like the idea of separating user input from Chart script logic.
Poster
Collaborator

😆 I had time to implement this as well.

😆 I had time to implement this as well.
justusbunsi marked this conversation as resolved
Poster
Collaborator

Hi @lord-kyron and @volker.raschek. You've opened several issues in the past asking for options to securely pass sensitive data to the Gitea app.ini. This PR provides a really generic way to craft the app.ini from various sources.

Feel free to check it out and experiment with it. I'd be happy for any user feedback. Detailed description on how to use it can be found here.

⚠️ This PR is based on different open PRs and contains some breaking changes, so it is considered bloody edge. Please have a look at #244 and #248.

Hi @lord-kyron and @volker.raschek. You've opened several issues in the past asking for options to securely pass sensitive data to the Gitea _app.ini_. This PR provides a really generic way to craft the _app.ini_ from various sources. Feel free to check it out and experiment with it. I'd be happy for any user feedback. Detailed description on how to use it can be found [here](https://gitea.com/justusbunsi/helm-chart/src/branch/feature/rework-sensitive-ini-settings/README.md#additional-_app-ini_-settings). ⚠️ This PR is based on different open PRs and contains some breaking changes, so it is **considered bloody edge**. Please have a look at #244 and #248.
justusbunsi changed title from WIP: Generic way for configuring Gitea to Generic way for configuring Gitea 9 months ago
justusbunsi removed the
status/wip
label 9 months ago
justusbunsi reviewed 8 months ago
# xargs echo -n trims all whitespaces and a trailing new line
local setting=$(awk -F '=' '{print $1}' <<< "${line}" | xargs echo -n)
local value=$(awk -F '=' '{print $NF}' <<< "${line}" | xargs echo -n)
Poster
Collaborator

TODO: Proper handling for values with = and " inside.

TODO: Proper handling for values with `=` and `"` inside.
justusbunsi marked this conversation as resolved
justusbunsi force-pushed feature/rework-sensitive-ini-settings from df3539a056 to b206414764 8 months ago
justusbunsi added 2 commits 8 months ago
09607b6c8e Improve source handling
9089c169ed Improve log readability
justusbunsi reviewed 8 months ago
justusbunsi left a comment
Poster
Collaborator

Some thoughts for reviewers

Some thoughts for reviewers
fi
local value=''
local regex="^${setting}(\s*)=(\s*)(.*)"
Poster
Collaborator

Users can have spaces around the = character in their config sources. Just like it is possible within app.ini. This regex takes this into account when stripping the setting name from the line to parse.

Users can have spaces around the `=` character in their config sources. Just like it is possible within `app.ini`. This regex takes this into account when stripping the setting name from the line to parse.
justusbunsi marked this conversation as resolved
value="${BASH_REMATCH[3]}"
else
env2ini::log ' ! invalid setting'
exit 1
Poster
Collaborator

I've decided to let the script fail in case any line is not processable. That way the user is required to fix their configuration before the currently applied breaks somehow. Open for discussion here.

I've decided to let the script fail in case any line is not processable. That way the user is required to fix their configuration before the currently applied breaks somehow. Open for discussion here.
justusbunsi marked this conversation as resolved
justusbunsi removed the
status/needs-feedback
label 8 months ago
justusbunsi added 2 commits 8 months ago
30d8049b3b Process full Gitea config with bash script
7bcfd26322 Fix global settings processing in app.ini
justusbunsi changed title from Generic way for configuring Gitea to Generic way for configuring Gitea app.ini 8 months ago
justusbunsi reviewed 8 months ago
env2ini::log " + '${setting}'"
if [[ -z "${section}" ]]; then
export "ENV_TO_INI____${setting^^}=${value}" # '^^' makes the variable content uppercase
Poster
Collaborator

____ ensures that global settings inside app.ini are actually taken into account and stored at the correct ini level.

This broke with the switch to environment-to-ini tool.

`____` ensures that global settings inside app.ini are actually taken into account and stored at the correct ini level. This broke with the switch to `environment-to-ini` tool.
justusbunsi marked this conversation as resolved
Poster
Collaborator
Glossary for this comment:
  - Inline sources:     `gitea.config` from values.yaml files
  - Additional sources: Kubernetes resources defined and provided by the users

The latest changes align the processing for inline sources with additional sources. Therefore I've added another Kubernetes secret where the inline sources are stored the same way as additional sources requires it.

Logging helps to understand what setting is used for the final app.ini and from which source it came. In case of duplicate settings, there is a (natural) override mechanism:

  • User values.yaml over default Helm Chart values.yaml settings
  • Settings from additional sources over inline sources

The already ensured persistence for "secret" settings remains untouched.

```plain Glossary for this comment: - Inline sources: `gitea.config` from values.yaml files - Additional sources: Kubernetes resources defined and provided by the users ``` The latest changes align the processing for inline sources with additional sources. Therefore I've added another Kubernetes secret where the inline sources are stored the same way as additional sources requires it. Logging helps to understand what setting is used for the final `app.ini` and from which source it came. In case of duplicate settings, there is a (natural) override mechanism: - User `values.yaml` over default Helm Chart `values.yaml` settings - Settings from additional sources over inline sources The already ensured persistence for "secret" settings remains untouched.
luhahn approved these changes 8 months ago
luhahn left a comment
Collaborator

LGTM, already tested a bunch of values

LGTM, already tested a bunch of values
justusbunsi force-pushed feature/rework-sensitive-ini-settings from 7bcfd26322 to b486175eca 8 months ago
justusbunsi force-pushed feature/rework-sensitive-ini-settings from b486175eca to e3d01dd8f6 8 months ago
justusbunsi removed the
status/blocked
label 8 months ago
justusbunsi force-pushed feature/rework-sensitive-ini-settings from e3d01dd8f6 to c33e34e956 8 months ago
Poster
Collaborator

Ok. Kept it in sync with head branch the last 24 hours. 😉

Ok. Kept it in sync with head branch the last 24 hours. 😉

Hello !

I will try this PR in the afternoon.

Having reviewed the changes, I find the following benefits :

  • it solves a real pain (secrets not stored in values.yaml when using a gitops approach)
  • no more Bash code generation from templates (maintenability improved)

From a very first sight, it seems there is a typo in commit 30e4f75b (line 302-304).

        {{- if .Values.extraVolumeMounts }}
        {{- toYaml .Values.extraVolumeMounts | nindent 12 }}
        {{- end }}

Those lines are volume mounts in a volume section with the wrong indentation. It breaks templating when defining extra volume mounts in values.yaml.

Hello ! I will try this PR in the afternoon. Having reviewed the changes, I find the following benefits : - it solves a real pain (secrets not stored in values.yaml when using a gitops approach) - no more Bash code generation from templates (maintenability improved) From a very first sight, it seems there is a typo in commit 30e4f75b (line 302-304). ```yaml {{- if .Values.extraVolumeMounts }} {{- toYaml .Values.extraVolumeMounts | nindent 12 }} {{- end }} ``` Those lines are volume mounts in a volume section with the wrong indentation. It breaks templating when defining extra volume mounts in values.yaml.
justusbunsi added 1 commit 8 months ago
52541836ea Fix indention level for extraVolumeMounts
Poster
Collaborator

Hello !

I will try this PR in the afternoon.

Awesome.

From a very first sight, it seems there is a typo in commit 30e4f75b (line 302-304).

Good catch. Fixed.

> Hello ! > > I will try this PR in the afternoon. Awesome. > From a very first sight, it seems there is a typo in commit 30e4f75b (line 302-304). Good catch. Fixed.
justusbunsi added 1 commit 8 months ago
073a56ff86 Stop trying to map mounts as volumes
lotherk approved these changes 8 months ago
lotherk left a comment
Collaborator

looks good to me

looks good to me
wxiaoguang approved these changes 8 months ago
luhahn merged commit 7b0a1c7ae6 into master 8 months ago
justusbunsi deleted branch feature/rework-sensitive-ini-settings 8 months ago

Reviewers

luhahn approved these changes 8 months ago
lotherk approved these changes 8 months ago
wxiaoguang approved these changes 8 months ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as 7b0a1c7ae6.
Sign in to join this conversation.
Loading…
There is no content yet.