feat(env): support multiline in env-file #19096

BlackHole1 · 2023-07-03T14:47:48Z

Does this PR introduce a user-facing change?

Support multiline in `--env-file`

/cc @Luap99 @vrothberg

pkg/env/env.go

Luap99

I am not so sure if this is not breaking to much. Keeping backwards and docker compatibility is really important. We do not know this is used today but I think the docker cli behaves the same way so changing how quotes, etc.. behave seems wrong to me.
I mainly thought this was about allowing newlines which I think can be done without breaking people.

pkg/env/env.go

BlackHole1 · 2023-07-04T09:33:50Z

I am not so sure if this is not breaking to much. Keeping backwards and docker compatibility is really important. We do not know this is used today but I think the docker cli behaves the same way so changing how quotes, etc.. behave seems wrong to me. I mainly thought this was about allowing newlines which I think can be done without breaking people.

I need to reconfirm the handling of quotation marks in the previous code. The current implementation will discard all quotation marks.

Luap99 · 2023-07-04T09:57:02Z

This is what I see so far:

$ cat test.env 
FOO1='bar'
FOO2="bar"
FOO3=  bar
FOO4=b\nar

# current podman
$ podman run --env-file test.env alpine printenv
...
FOO3=  bar
FOO4=b\nar
FOO1='bar'
FOO2="bar"
...
# your PR
$ bin/podman run --env-file test.env alpine printenv
...
FOO3=bar
FOO4=b\nar
FOO1='bar'
FOO2=bar
...

BlackHole1 · 2023-07-04T11:47:49Z

Hey @Luap99 👋. I fixed the issues with FOO2 and FOO3, and also added unit tests for some edge cases.

Here are examples of how this PR handles multi-line environment variables:

case 1

$ cat test.env 
FOO="aa
bb"

$ bin/podman run --env-file test.env alpine printenv FOO
aa
bb

case 2

$ cat test.env 
FOO='aa
bb'

$ bin/podman run --env-file test.env alpine printenv FOO
aa
bb

case 3

$ cat test.env 
FOO=`aa
bb`

$ bin/podman run --env-file test.env alpine printenv FOO
Error: parsing file "./test.env": only support multi-line environment variables surrounded by double quotation marks or single quotation marks. invalid variable: "FOO=`aa\nbb`\n"

exit code: 125

rhatdan · 2023-07-06T13:18:39Z

LGTM

hedayat · 2023-07-07T15:25:46Z

Thanks a lot for this PR. However, apparently it always replaces '\n' in a multi-line variable with a new line character (I mean replacing \\n with \n), which I suggest not to do (or do it if the value is surrounded by double quotes); which aligns with docker-compose behavior. And I guess if they are going to be replaced in double quotes case, other escape chars like \t should also be replaced).

Thanks again

BlackHole1 · 2023-07-10T01:56:50Z

Thanks a lot for this PR. However, apparently it always replaces '\n' in a multi-line variable with a new line character (I mean replacing \\n with \n), which I suggest not to do (or do it if the value is surrounded by double quotes); which aligns with docker-compose behavior. And I guess if they are going to be replaced in double quotes case, other escape chars like \t should also be replaced).

Thanks again

Are you suggesting that deleting this line would make it consistent with Docker's behavior?
value = strings.NewReplacer("\\n", "\n", "\\r", "\r").Replace(value)

BTW. I've noticed that the env-file in my local docker run doesn't support multiple lines.

I believe it would make sense to remove value = strings.NewReplacer("\\n", "\n", "\\r", "\r").Replace(value). I did this because I referred to the implementation of dotenv, but I realized it is unnecessary. We should not handle it this way as it can lead to unexpected bugs. Users may report a new bug: "Why did my backslash disappear?"

hedayat · 2023-07-10T11:19:57Z

Are you suggesting that deleting this line would make it consistent with Docker's behavior? value = strings.NewReplacer("\\n", "\n", "\\r", "\r").Replace(value)

Yes!

BTW. I've noticed that the env-file in my local docker run doesn't support multiple lines.

It's also correct! I also find it while I was testing your branch. The interesting thing is that while docker itself doesn't seem to properly support mult-line env files, docker-compose does support them and it is working fine. Actually, I was comparing with docker-compose behavior. (Note that I've not tested against the new docker compose plugin, but the older docker-compose one). Unlike podman-compose, it seems that docker-compose handles env files itself.

I believe it would make sense to remove value = strings.NewReplacer("\\n", "\n", "\\r", "\r").Replace(value). I did this because I referred to the implementation of dotenv, but I realized it is unnecessary. We should not handle it this way as it can lead to unexpected bugs. Users may report a new bug: "Why did my backslash disappear?"

Yes, and it makes it impossible to embed \n itself inside the value (and the project I was trying to run with podman-compose had exactly that). And removing that line actually fixed the problem and I was able to run with podman-compose just fine.

I've not tried a multi-line value surrounded by double quotes to see the docker-compose behavior, it is likely that it replaces escape characters in that case. But I guess it's not that important, as the user is able to use newline itself rather than using \n to insert a new line in the value.

BlackHole1 · 2023-07-10T12:13:12Z

Yes, and it makes it impossible to embed \n itself inside the value (and the project I was trying to run with podman-compose had exactly that). And removing that line actually fixed the problem and I was able to run with podman-compose just fine.

@hedayat Thank you for your test!

rhatdan · 2023-07-10T18:56:42Z

@vrothberg @Luap99 PTAL

rhatdan · 2023-07-13T17:31:15Z

@vrothberg @Luap99 Reping

vrothberg

LGTM
@Luap99 PTAL

docs/source/markdown/options/env-file.md

Signed-off-by: Black-Hole1 <[email protected]>

Luap99

LGTM

vrothberg

/lgtm
/hold

openshift-ci · 2023-07-31T12:30:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BlackHole1, Luap99, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2023-07-31T19:37:24Z

/hold cancel

edsantiago · 2023-08-08T19:21:36Z

Heads-up everybody, this PR broke the use of octothorpe in environment variables:

$ bin/podman run --env aaa='bbb#ccc' quay.io/libpod/testimage:20221018 printenv aaa
bbb    <---- should be bbb#ccc

A community member will be filing an issue. This is just an early notice so someone can start working on a fix.

rhatdan · 2023-08-08T20:34:30Z

@BlackHole1 ^^

BlackHole1 · 2023-08-09T00:11:05Z

@rhatdan @edsantiago Good Catch. I will be submitting a fix PR for this in the next couple of days

kajinamit · 2023-08-09T03:53:47Z

We noticed that we no longer able to pass a value containing new line (\n) and only the first line is passed to the container and that breaks some of our usage.

We are now using the CentOS Stream 9 package built from the hash 1440985 , while we didn't see the issue when we used the package built from hash 38e6fab , and indeed this change appears in the diffs between these two hash.

BlackHole1 · 2023-08-09T04:13:25Z

Heads-up everybody, this PR broke the use of octothorpe in environment variables:
$ bin/podman run --env aaa='bbb#ccc' quay.io/libpod/testimage:20221018 printenv aaa
bbb    <---- should be bbb#ccc
A community member will be filing an issue. This is just an early notice so someone can start working on a fix.

Hi @edsantiago I have just fixed this issue locally. I will submit a PR in 1 hour (because I'm having lunch now 😄). This problem is quite interesting, and I will provide a detailed explanation in the PR.

BlackHole1 · 2023-08-09T04:16:26Z

We noticed that we no longer able to pass a value containing new line (\n) and only the first line is passed to the container and that breaks some of our usage.

We are now using the CentOS Stream 9 package built from the hash 1440985 , while we didn't see the issue when we used the package built from hash 38e6fab , and indeed this change appears in the diffs between these two hash.

@kajinamit Can you provide a detailed explanation of the issue you encountered? Was it related to the --env or the --env-file?

kajinamit · 2023-08-09T04:32:46Z

We are using --env instead of --env-file and the issue appears in case we pass the value containing \n using that option. (We are not using the podman CLI directly but calling it via ansible-podman-collections).

I've reported my issue to CentOS 9 Stream https://bugzilla.redhat.com/show_bug.cgi?id=2230212 , because I faced the issue using the podman package from CentOS 9 Stream. That contains a few more details.

BlackHole1 · 2023-08-09T04:50:36Z

We are using --env instead of --env-file and the issue appears in case we pass the value containing \n using that option. (We are not using the podman CLI directly but calling it via ansible-podman-collections).

I've reported my issue to CentOS 9 Stream https://bugzilla.redhat.com/show_bug.cgi?id=2230212 , because I faced the issue using the podman package from CentOS 9 Stream. That contains a few more details.

OK. I will submit the fix PR within an hour. Thank you for your feedback.

We've made rather a mess of those options, due to lack of testing. Here we have a first step toward regression tests. --env is OK, but there are three special-case exceptions in --env-file for three incompatibilities introduced by containers#19096. To be continued, but probably in future PRs. We need this ASAP to prevent us from making any more regressions. Signed-off-by: Ed Santiago <[email protected]>

openshift-ci bot added the release-note label Jul 3, 2023

BlackHole1 force-pushed the support-new-line branch from 2fc06fe to fd7642c Compare July 3, 2023 14:49

BlackHole1 changed the title ~~feat(env): support muluiline in env-file~~ feat(env): support multiline in env-file Jul 3, 2023

BlackHole1 force-pushed the support-new-line branch from fd7642c to 5b8dd87 Compare July 3, 2023 14:49

BlackHole1 changed the title ~~feat(env): support multiline in env-file~~ WIP: feat(env): support multiline in env-file Jul 3, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2023

BlackHole1 force-pushed the support-new-line branch from 5b8dd87 to 7e6dcf3 Compare July 3, 2023 14:53

BlackHole1 changed the title ~~WIP: feat(env): support multiline in env-file~~ feat(env): support multiline in env-file Jul 3, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2023

BlackHole1 force-pushed the support-new-line branch 2 times, most recently from 3b5912a to 5908c42 Compare July 4, 2023 03:33

BlackHole1 commented Jul 4, 2023

View reviewed changes

pkg/env/env.go Show resolved Hide resolved

pkg/env/env.go Outdated Show resolved Hide resolved

pkg/env/env.go Outdated Show resolved Hide resolved

pkg/env/env.go Outdated Show resolved Hide resolved

BlackHole1 force-pushed the support-new-line branch from 5908c42 to 588d231 Compare July 4, 2023 03:49

openshift-ci bot requested a review from Luap99 July 4, 2023 06:40

BlackHole1 force-pushed the support-new-line branch from 588d231 to 1806e71 Compare July 4, 2023 08:05

Luap99 reviewed Jul 4, 2023

View reviewed changes

pkg/env/env.go Outdated Show resolved Hide resolved

pkg/env/env.go Outdated Show resolved Hide resolved

BlackHole1 force-pushed the support-new-line branch from 1806e71 to dbf9192 Compare July 4, 2023 11:36

BlackHole1 force-pushed the support-new-line branch from dbf9192 to 33ab92f Compare July 5, 2023 02:00

BlackHole1 requested a review from Luap99 July 5, 2023 05:30

BlackHole1 force-pushed the support-new-line branch from 33ab92f to b4a268d Compare July 10, 2023 02:28

vrothberg approved these changes Jul 31, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2023

Luap99 reviewed Jul 31, 2023

View reviewed changes

docs/source/markdown/options/env-file.md Outdated Show resolved Hide resolved

docs(env-file): improve document description

c67ef7c

Signed-off-by: Black-Hole1 <[email protected]>

BlackHole1 force-pushed the support-new-line branch from 4e81015 to c67ef7c Compare July 31, 2023 12:24

Luap99 approved these changes Jul 31, 2023

View reviewed changes

vrothberg approved these changes Jul 31, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Jul 31, 2023

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 31, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2023

openshift-merge-robot merged commit ad9015f into containers:main Jul 31, 2023

BlackHole1 deleted the support-new-line branch August 1, 2023 01:17

BlackHole1 mentioned this pull request Aug 9, 2023

fix(env): parsing --env incorrect in cli #19560

Merged

This was referenced Aug 9, 2023

env-file not handling multiline nor quotes #19565

Closed

[v4.6] fix(env): parsing --env incorrect in cli #19566

Closed

edsantiago mentioned this pull request Aug 9, 2023

systests: tests for --env and --env-file #19573

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 8, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(env): support multiline in env-file #19096

feat(env): support multiline in env-file #19096

BlackHole1 commented Jul 3, 2023 •

edited

Loading

Luap99 left a comment

BlackHole1 commented Jul 4, 2023 •

edited

Loading

Luap99 commented Jul 4, 2023

BlackHole1 commented Jul 4, 2023 •

edited

Loading

rhatdan commented Jul 6, 2023

hedayat commented Jul 7, 2023

BlackHole1 commented Jul 10, 2023 •

edited

Loading

hedayat commented Jul 10, 2023 •

edited

Loading

BlackHole1 commented Jul 10, 2023

rhatdan commented Jul 10, 2023

rhatdan commented Jul 13, 2023

vrothberg left a comment

Luap99 left a comment

vrothberg left a comment

openshift-ci bot commented Jul 31, 2023

rhatdan commented Jul 31, 2023

edsantiago commented Aug 8, 2023

rhatdan commented Aug 8, 2023

BlackHole1 commented Aug 9, 2023 •

edited

Loading

kajinamit commented Aug 9, 2023 •

edited

Loading

BlackHole1 commented Aug 9, 2023

BlackHole1 commented Aug 9, 2023 •

edited

Loading

kajinamit commented Aug 9, 2023

BlackHole1 commented Aug 9, 2023

feat(env): support multiline in env-file #19096

feat(env): support multiline in env-file #19096

Conversation

BlackHole1 commented Jul 3, 2023 • edited Loading

Does this PR introduce a user-facing change?

Luap99 left a comment

Choose a reason for hiding this comment

BlackHole1 commented Jul 4, 2023 • edited Loading

Luap99 commented Jul 4, 2023

BlackHole1 commented Jul 4, 2023 • edited Loading

case 1

case 2

case 3

rhatdan commented Jul 6, 2023

hedayat commented Jul 7, 2023

BlackHole1 commented Jul 10, 2023 • edited Loading

hedayat commented Jul 10, 2023 • edited Loading

BlackHole1 commented Jul 10, 2023

rhatdan commented Jul 10, 2023

rhatdan commented Jul 13, 2023

vrothberg left a comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 31, 2023

rhatdan commented Jul 31, 2023

edsantiago commented Aug 8, 2023

rhatdan commented Aug 8, 2023

BlackHole1 commented Aug 9, 2023 • edited Loading

kajinamit commented Aug 9, 2023 • edited Loading

BlackHole1 commented Aug 9, 2023

BlackHole1 commented Aug 9, 2023 • edited Loading

kajinamit commented Aug 9, 2023

BlackHole1 commented Aug 9, 2023

BlackHole1 commented Jul 3, 2023 •

edited

Loading

BlackHole1 commented Jul 4, 2023 •

edited

Loading

BlackHole1 commented Jul 4, 2023 •

edited

Loading

BlackHole1 commented Jul 10, 2023 •

edited

Loading

hedayat commented Jul 10, 2023 •

edited

Loading

BlackHole1 commented Aug 9, 2023 •

edited

Loading

kajinamit commented Aug 9, 2023 •

edited

Loading

BlackHole1 commented Aug 9, 2023 •

edited

Loading