Rework app.ini generation #239

We should document that the Chart now requires a Gitea image with environment-to-ini inside. Older images won't work anymore. Since this tool exists since 2019, but I think it's no breaking change in 2021.

justusbunsi added the

kind

enhancement

label 2021-11-13 11:47:35 +00:00

luhahn force-pushed app-ini-rework from 9376bd7886 to ccd459517d

2021-11-17 11:06:52 +00:00

Compare

luhahn force-pushed app-ini-rework from df2797a765 to 04169e9440

2021-11-17 11:56:30 +00:00

Compare

luhahn changed title from ~~WIP: Rework app.ini generation~~ to Rework app.ini generation

2021-11-17 14:26:15 +00:00

justusbunsi requested changes 2021-11-17 20:03:20 +00:00

README.md Outdated

              @@ -40,0 +46,4 @@  
        
     The app.ini generation has changed and now utilizes the environment-to-ini  
     script provided by newer Gitea versions.  
     :warning: Since environment to ini is used it is not possible to use Gitea  
  

justusbunsi commented

2021-11-17 19:14:37 +00:00

Let's add a line break right before the warning to have it in separate line. Or something like the following to have it more outstanding from the rest of this chapter.

...by newer Gitea versions. > :boom: The Helm Chart now requires Gitea versions of at least 1.11.0.

Let's add a line break right before the warning to have it in separate line. Or something like the following to have it more outstanding from the rest of this chapter. ``` ...by newer Gitea versions. > :boom: The Helm Chart now requires Gitea versions of at least 1.11.0. ```

luhahn marked this conversation as resolved

README.md Outdated

              @@ -40,0 +53,4 @@  
        
     #### Secret Key generation  
        
     Gitea secret keys are now generated automatically if not provided. The values

justusbunsi commented

2021-11-17 19:22:57 +00:00

The current text leaves room for interpretation regarding "automatically if not provided". I guess we should structure it a bit more on how the Chart in v1.5.0 will behave in various situations.

New installs: By default automatically generated | you can provide your own, if you already have them using gitea.config
Existing installs: Neither automatic creation nor replacement in the existing app.ini
Existing installs: Still able to define those values in gitea.config

To me, such structure makes it sound less breaking.

The current text leaves room for interpretation regarding "automatically if not provided". I guess we should structure it a bit more on how the Chart in v1.5.0 will behave in various situations. - New installs: By default automatically generated | you can provide your own, if you already have them using `gitea.config` - Existing installs: Neither automatic creation nor replacement in the existing _app.ini_ - Existing installs: Still able to define those values in `gitea.config` To me, such structure makes it sound less breaking.

luhahn marked this conversation as resolved

README.md Outdated

              @@ -40,0 +57,4 @@  
     are not set, if an app.ini already is existing.  
     You can enforce new secret keys, if `gitea.forceSecrets` is set to true.  
        
     :warning: When secret keys are changed if already provided, it is possible,  
  

justusbunsi commented

2021-11-17 19:26:56 +00:00

Same as above. Let's make it more outstanding.

> :rotating_light: Although the Chart provides resetting secret keys, it is > not advisable to do so for existing installations. Certain settings like > _LDAP_ would not be readable anymore.

Same as above. Let's make it more outstanding. ``` > :rotating_light: Although the Chart provides resetting secret keys, it is > not advisable to do so for existing installations. Certain settings like > _LDAP_ would not be readable anymore. ```

luhahn marked this conversation as resolved

README.md Outdated

              @@ -535,0 +555,4 @@  
     | initPreScript | Bash script copied verbatim to start of init container | |  
     | securityContext | Run as a specific securityContext | `{}` |  
     | schedulerName | Use an alternate scheduler, e.g. "stork" | |  
     | `gitea.forceSecrets` | Enforce new secret key generation (SECRET_KEY, INTERNAL_TOKEN, etc.) | `force` |  
  

justusbunsi commented

2021-11-17 19:39:54 +00:00

I think the default is either true or false ?. Why highlight the parameter instead of plain text as the rest in this table?

And maybe use the same hint underneath this table is it is used in the changelog notes to tell people about the dangers of resetting such values for existing installs.

I think the default is either `true` or `false` ?. Why highlight the parameter instead of plain text as the rest in this table? And maybe use the same hint underneath this table is it is used in the changelog notes to tell people about the dangers of resetting such values for existing installs.

luhahn commented

2021-11-18 10:03:08 +00:00

This is needed because of the linter. It would complain that gitea needs to be Gitea.
I now highlighted simply all the values in the values description.

This is needed because of the linter. It would complain that gitea needs to be Gitea. I now highlighted simply all the values in the values description.

😆 1

luhahn marked this conversation as resolved

templates/gitea/config.yaml Outdated

              @@ -136,0 +142,4 @@  
      {{- if not .Values.gitea.forceSecrets }}  
      if [ ! -f ${GITEA_APP_INI} ]; then  
      {{- end }}  
      {{- if not (hasKey .Values.gitea.config.security "INTERNAL_TOKEN") }}  
  

justusbunsi commented

2021-11-17 20:01:30 +00:00

? I really like that the recreate script is not rendered if a value is already provided via configuration. It's a nice safety guard for us.

There's nothing to do here. ?

? I really like that the recreate script is not rendered if a value is already provided via configuration. It's a nice safety guard for us. There's nothing to do here. ?

luhahn commented

2021-11-18 10:03:59 +00:00

reworked this slighty

I removed the file check guard on the export/key generation and only added it to the unset section.

reworked this slighty I removed the file check guard on the export/key generation and only added it to the unset section.

luhahn marked this conversation as resolved

templates/gitea/config.yaml Outdated

              @@ -149,0 +173,4 @@  
      unset ENV_TO_INI__SECURITY__INTERNAL_TOKEN  
      unset ENV_TO_INI__SECURITY__SECRET_KEY  
      unset ENV_TO_INI__OAUTH2__JWT_SECRET  
      {{- end }}  
  

justusbunsi commented

2021-11-17 19:51:39 +00:00

If I read this correctly, this section shall ensure these environment variables got removed if no forceful updated is requested.

Unluckily, this breaks initial installs when such values are provided via gitea.config. They get unset by this code block before being applied.

Maybe it is enough to set the forceSecrets to false by default and explicitly mention that it would be a bad idea for existing installs to change those values afterwards. Instead of unset these values here. If the users choose to change the values anyway, we can't do something about it. ?‍♂️

If I read this correctly, this section shall ensure these environment variables got removed if no forceful updated is requested. Unluckily, this breaks initial installs when such values are provided via `gitea.config`. They get unset by this code block before being applied. Maybe it is enough to set the `forceSecrets` to _false_ by default and explicitly mention that it would be a bad idea for existing installs to change those values afterwards. Instead of unset these values here. If the users choose to change the values anyway, we can't do something about it. ?‍♂️

luhahn commented

2021-11-18 10:15:44 +00:00

I added an if to check if the app.ini exists. This way we can unset the values if they are set either via config or are autogenerated and forceSecrets is false

luhahn marked this conversation as resolved

templates/gitea/statefulset.yaml Outdated

              @@ -240,6 +263,7 @@ spec:  
      - name: config  
      secret:  
      secretName: {{ include "gitea.fullname" . }}  
      defaultMode: 0777  
  

justusbunsi commented

2021-11-17 19:33:10 +00:00

Would execute permissions not be enough?

Would `execute` permissions not be enough?

luhahn marked this conversation as resolved

values.yaml Outdated

              @@ -141,6 +141,8 @@ signing:  
      gpgHome: /data/git/.gnupg  
        
     gitea:  
      forceSecrets: true  
  

justusbunsi commented

2021-11-17 19:38:49 +00:00

This must be false to not break existing installs right away.

This must be `false` to not break existing installs right away.

luhahn marked this conversation as resolved

luhahn force-pushed app-ini-rework from 04169e9440 to 9efbafdfa0

2021-11-18 10:01:43 +00:00

Compare

luhahn added 1 commit 2021-11-18 10:30:02 +00:00

Correct init script permissions

continuous-integration/drone/pr Build is passing

Details

continuous-integration/drone/push Build is passing

Details

8080efb2df

luhahn requested review from justusbunsi 2021-11-18 10:30:09 +00:00

justusbunsi requested changes 2021-11-18 18:36:46 +00:00

templates/gitea/config.yaml Outdated

              @@ -135,1 +142,3 @@  
      {{- /* autogenerate app.ini */ -}}  
      {{- if not (hasKey .Values.gitea.config.security "INTERNAL_TOKEN") }}  
      export ENV_TO_INI__SECURITY__INTERNAL_TOKEN=$(gitea generate secret INTERNAL_TOKEN)  
      {{- end }}  
  

justusbunsi commented

2021-11-18 18:21:35 +00:00

Unluckily, these checks interfere with gitea.enforceAppSecretRecreation. If you have one or more of those settings defined, the script won't generate a new value for them.

I think the rendering behaviour should be:

enforceAppSecretRecreation: false
- check for provided keys from values.yaml and use them; don't generate new ones
- app.ini does not exist?
  - proceed...
- app.ini does exist?
  - unset the persistent values to prevent overwriting
enforceAppSecretRecreation: true
- right before environment-to-ini execution
  - generate new values for persistent values to overwrite the old or provided ones

Did I miss anything?

Unluckily, these checks interfere with `gitea.enforceAppSecretRecreation`. If you have one or more of those settings defined, the script won't generate a new value for them. I think the rendering behaviour should be: - `enforceAppSecretRecreation: false` - check for provided keys from values.yaml and use them; don't generate new ones - app.ini does not exist? - proceed... - app.ini does exist? - unset the persistent values to prevent overwriting - `enforceAppSecretRecreation: true` - right before environment-to-ini execution - generate new values for persistent values to overwrite the old or provided ones Did I miss anything?

luhahn marked this conversation as resolved

values.yaml Outdated

              @@ -141,6 +141,8 @@ signing:  
      gpgHome: /data/git/.gnupg  
        
     gitea:  
      enforceAppSecretRecreation: false  
  

justusbunsi commented

2021-11-18 18:36:06 +00:00

After playing around a bit, I am no longer sure this option is a good idea.

The configure-gitea init container does not run successfully on existing installations when configuring LDAP authentication sources as well. By changing the SECRET_KEY, the already configured authentication sources are no longer readable and neither creating nor editing them is possible, which leaves Gitea in a state where it can't do anything an exits.

This would break an upgrade from the chart.

In addition, I have strong concerns about how the shell script behaves with enforceAppSecretRecreation enabled during a scale-down and subsequent scale-up. Since this does not change the shell script, it still says that new values should be generated. I don't think that's the user's plan when doing e.g. a node maintenance or something, where Gitea is moved or stopped for a short time.

It might be best to just prevent overwriting these values completely if there already is an app.ini file. This way, neither a generated one nor a potentially changed value from the values.yaml will break an existing installation.

I too thought this would work better than it does.

After playing around a bit, I am no longer sure this option is a good idea. The `configure-gitea` init container does not run successfully on existing installations when configuring LDAP authentication sources as well. By changing the SECRET_KEY, the already configured authentication sources are no longer readable and neither creating nor editing them is possible, which leaves Gitea in a state where it can't do anything an exits. This would break an upgrade from the chart. In addition, I have strong concerns about how the shell script behaves with `enforceAppSecretRecreation` enabled during a scale-down and subsequent scale-up. Since this does not change the shell script, it still says that new values should be generated. I don't think that's the user's plan when doing e.g. a node maintenance or something, where Gitea is moved or stopped for a short time. It might be best to just prevent overwriting these values completely if there already is an app.ini file. This way, neither a generated one nor a potentially changed value from the values.yaml will break an existing installation. I too thought this would work better than it does.

luhahn commented

2021-11-19 09:40:22 +00:00

You're right, I've tested some scenarios as well and they all seem to brick gitea.
(I've previously tested with 1.14.6, which was fine :D, so I didn't noticed the problems)

-> removed the setting. User can still manually change the app.ini inside the container if they really need to.

You're right, I've tested some scenarios as well and they all seem to brick gitea. (I've previously tested with 1.14.6, which was fine :D, so I didn't noticed the problems) -> removed the setting. User can still manually change the app.ini inside the container if they really need to.

luhahn marked this conversation as resolved

luhahn force-pushed app-ini-rework from 6b98d40083 to 38077e63da

2021-11-19 09:39:56 +00:00

Compare

justusbunsi approved these changes 2021-11-19 18:31:26 +00:00

justusbunsi left a comment

?

justusbunsi added a new dependency 2021-11-19 18:42:22 +00:00

#248 Drop custom probes

justusbunsi removed a dependency 2021-11-19 18:42:38 +00:00

#248 Drop custom probes

justusbunsi added a new dependency 2021-11-19 18:48:13 +00:00