Handle gitea config values containing spaces #261

nmasse-itix · 2021-12-09T10:34:16Z

nmasse-itix commented

2021-12-09 10:34:16 +00:00

Hello,

I'm proposing a fix for #260.

I tried to find a way to properly encode bash strings from Helm templates but found no perfect solution.

The quote function properly adds double quote around the string, correctly escapes double quotes within the string but fails to escape dollar signs.

{{ .Values.foo | quote }}

The squote function properly adds simple quote around the string but fails to escape single quotes within the string.

{{ .Values.foo | squote }}

Since spaces and dollar signs seem more common than single quote, I'm proposing the following fallback:

add a comment stating the pitfall
add single quotes around the string

Hello, I'm proposing a fix for #260. I tried to find a way to properly encode bash strings from Helm templates but found no perfect solution. The quote function properly adds double quote around the string, correctly escapes double quotes within the string but fails to escape dollar signs. ``` {{ .Values.foo | quote }} ``` The squote function properly adds simple quote around the string but fails to escape single quotes within the string. ``` {{ .Values.foo | squote }} ``` Since spaces and dollar signs seem more common than single quote, I'm proposing the following fallback: - add a comment stating the pitfall - add single quotes around the string

nmasse-itix added 1 commit 2021-12-09 10:34:17 +00:00

continuous-integration/drone/pr Build is passing

Details

9e4bb5ec2b add quotes to environment variables

Signed-off-by: Nicolas MASSE <nicolas.masse@itix.fr>

luhahn commented

2021-12-13 08:45:56 +00:00

Hey, thank you very much for finding this :)

I will have a look at the problem and your PR today!

Hey, thank you very much for finding this :) I will have a look at the problem and your PR today!

👍 1

luhahn added this to the 5.0.0 milestone 2021-12-13 08:46:12 +00:00

luhahn added the

kind

bug

label 2021-12-13 08:46:36 +00:00

luhahn commented

2021-12-13 09:58:56 +00:00

we could use something like this approach:

export ENV_TO_INI__{{ $key | upper | replace "." "_0X2E_" | replace "-" "_0X2D_" }}__{{ $n_key | upper }}={{ $n_value | quote | replace "$" "\\$" }}

This way we can have the benefits of the double quotes and also escape all dollar symbols automatically.
This might lead to issues, if the user also tries to escape the dollar symbol.

However this should be documented to prevent issues.

we could use something like this approach: ```yaml export ENV_TO_INI__{{ $key | upper | replace "." "_0X2E_" | replace "-" "_0X2D_" }}__{{ $n_key | upper }}={{ $n_value | quote | replace "$" "\\$" }} ``` This way we can have the benefits of the double quotes and also escape all dollar symbols automatically. This might lead to issues, if the user also tries to escape the dollar symbol. However this should be documented to prevent issues.

luhahn commented

2021-12-17 09:17:38 +00:00

@justusbunsi @nmasse-itix what do you think about the solution i posted? Open for discussion here :)

nmasse-itix commented

2021-12-17 11:18:55 +00:00

I think it's a good idea. I will update my PR soon.

thanks for your review !

I think it's a good idea. I will update my PR soon. thanks for your review !

justusbunsi commented

2021-12-17 19:29:11 +00:00

@luhahn Modifying the value is generally not a good idea, IMO. The following configuration would be rendered as export ENV_TO_INI__DATABASE__PASSWD="p@sw0rc|-ca\$h", which is correct. We are probably missing some necessary shell script escaping for other values.

gitea:
  config:
    database:
      PASSWD: "p@sw0rc|-ca$h"

As a fix for this bug it will work. Thanks for finding it @nmasse-itix.

We can streamline it later with #240. It would be possible to always generate secrets/configmaps for the app.ini configuration inside the values.yaml which would fit into the concept of the proposed additionalConfigSources. Then it would be one way to process app.ini configuration. Thoughts?

@luhahn Modifying the value is generally not a good idea, IMO. The following configuration would be rendered as `export ENV_TO_INI__DATABASE__PASSWD="p@sw0rc|-ca\$h"`, which is correct. We are probably missing some necessary shell script escaping for other values. ```yaml gitea: config: database: PASSWD: "p@sw0rc|-ca$h" ``` As a fix for this bug it will work. Thanks for finding it @nmasse-itix. --- We can streamline it later with #240. It would be possible to always generate secrets/configmaps for the `app.ini` configuration inside the values.yaml which would fit into the concept of the proposed _additionalConfigSources_. Then it would be one way to process `app.ini` configuration. Thoughts?

luhahn commented

2021-12-18 10:49:55 +00:00

@justusbunsi yes it would be rendered that way, but it will also be executed in a shell script which will escape the dollar sign and should correctly export it in the PASSWD environment variable without the backslash

Edit: think i missunderstood your post. :D Sorry for the noise

@justusbunsi yes it would be rendered that way, but it will also be executed in a shell script which will escape the dollar sign and should correctly export it in the PASSWD environment variable without the backslash Edit: think i missunderstood your post. :D Sorry for the noise

nmasse-itix added 1 commit 2021-12-20 10:19:18 +00:00

continuous-integration/drone/pr Build is failing

Details

0f5dacf762 handle single quotes, double quotes and dollar signs in config values

Signed-off-by: Nicolas MASSE <nicolas.masse@itix.fr>

nmasse-itix commented

2021-12-20 10:21:53 +00:00

I updated the PR with the suggestion so that single quotes, double quotes and dollar signs are escaped. I also updated the comment to reflect this.

justusbunsi commented

2021-12-20 11:45:51 +00:00

Hi @nmasse-itix. Your PR made me think of some other potential risks of breakage. A few weeks ago I started PR #240 to make the app.ini creation even more flexible. Prior to your bug report, most of those risks would've been still there after merging that PR. Thanks to you I could mitigate these issues.
In addition to that, I rewrote the whole app.ini creation within the mentioned PR which implicitly fixes this issue, too. Both, your and mine PR are part of release 5.0. Is it ok for you that we close your PR in favor of the rewrite-PR?

Hi @nmasse-itix. Your PR made me think of some other potential risks of breakage. A few weeks ago I started PR #240 to make the `app.ini` creation even more flexible. Prior to your bug report, most of those risks would've been still there after merging that PR. Thanks to you I could mitigate these issues. In addition to that, I rewrote the whole `app.ini` creation within the mentioned PR which implicitly fixes this issue, too. Both, your and mine PR are part of release 5.0. Is it ok for you that we close your PR in favor of the rewrite-PR?

nmasse-itix commented

2021-12-20 13:30:56 +00:00

Sure ! #240 is a rework providing much better value for users. Let's try to get #240 approved first and if it gets merged, we can close this PR.

/hold

Sure ! #240 is a rework providing much better value for users. Let's try to get #240 approved first and if it gets merged, we can close this PR. /hold

1

nmasse-itix commented

2021-12-22 13:41:11 +00:00

#240 has been merged. Let's close this PR that is not needed anymore.

nmasse-itix closed this pull request

2021-12-22 13:41:12 +00:00

continuous-integration/drone/pr Build is failing

Details

check-and-test / check-and-test (pull_request)

Required

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Milestone

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gitea/helm-chart#261