env-file not handling multiline nor quotes

[edsantiago]

$ cat >foo
dquote="abc"
squote='abc'
newline1=aaa\
bbb
newline2="aaa
bbb"
$ bin/podman run --env-file foo quay.io/libpod/testimage:20221018 env   [removed PATH/etc]
newline2=aaa       <----- this one is correct
bbb
dquote="abc"       <----- should not include quotes
squote='abc'       <-------- ditto
newline1=aaa\      <------ did not get second line

The format for env-file is not documented anywhere. However, judging from comments in #19096 and from the way bash parses env lines, I believe that:

• single and double quotes should be stripped if they match at beginning/end of expression
• trailing backslash should mean "continue reading next line but without adding newline"

Whatever PR fixes this, do not under any circumstances allow it to merge until I have written a comprehensive test suite for it.

[BlackHole1]

Hi @edsantiago. I'm not very certain whether there's truly an issue here. The behavior of pull request #19096 should remain consistent with previous implementations, so I believe this should be expected.

[BlackHole1]

until I have written a comprehensive test suite for it.

I think this is great, but I have another idea: perhaps we should define the behavior for "multi-line" just like the newline1 in your example. In my opinion, this shouldn't be supported. Also, the current implementation for multi-line is inspired by dotenv.

[BlackHole1]

In the initial implementation of PR #19096, I would remove the surrounding quotes. However, this would lead to inconsistency with the previous behavior and potentially cause larger issues. See: #19096 (comment)

[edsantiago]

@BlackHole1 I did not mean to suggest that this is your problem. It is not. It's not a regression, nor related to anything you did. This is our problem, and it is longstanding.

[rhatdan]

How does Docker handle @edsantiago env-file?

[BlackHole1]

The format for env-file is not documented anywhere

In fact, it has already been documented. See: c67ef7c

[BlackHole1]

How does Docker handle @edsantiago env-file?

Regarding this matter, I've investigated previously, and Docker's env-file doesn't support multiple lines.

Ref: #19096 (comment)

[BlackHole1]

I believe a better approach would be to define the behavior for multi-line functionality through RFC before actual implementation. This should be accompanied by a substantial number of comprehensive tests. Without RFCs, it could pose difficulties for future maintenance. Currently, the support for multi-line functionality is achieved using complex regular expressions, which can prove inconvenient for later maintenance (even though it was copied from dotenv).

I have an idea in mind, which involves implementing multi-line functionality through the use of an AST, and having it managed separately in another repository.

Regarding issue #19096, I remember it wasn't merged into version 4.6. Therefore, I suggest that we consider rolling back both #19096 and #19560. Once a universally acceptable solution has been determined by the community, we can proceed to re-implement it

[edsantiago]

There is one regression from 4.6:

$ cat >hashenv
foo=abc#def
$ bin/podman run --env-file hashenv quay.io/libpod/testimage:20221018 printenv foo        <--- main (bad)
abc
$ /usr/bin/podman run --env-file hashenv quay.io/libpod/testimage:20221018 printenv foo   <--- 4.6 (good)
abc#def

[edsantiago]

How does Docker handle @edsantiago env-file?

Looks like we're almost bug-for-bug compatible with docker[1]:

• Docker does not correctly handle single or double quotes, and this is clearly a bug in their code and in ours. (Possibly one we can't fix, if we wish to remain compatible).
• Docker does not handle multi-line settings, neither with double quotes nor without. In this sense we were compatible, until feat(env): support multiline in env-file #19096
• Docker correctly handles octothorpe in env-file. We did, but no longer do as of feat(env): support multiline in env-file #19096.

To remain docker compatible, we should probably revert both #19096 and #19560, with apologies to @BlackHole1 .

I would prefer to break docker compatibility, because env-file currently violates POLA. But this isn't my decision.

Regardless of what we do, we must:

1. Create an extensive test suite; and
2. Document env-file

[1] moby-engine-20.10.23-1.fc38.x86_64

[Luap99]

The root cause was #18724. As I said in the issue we should stay backwards compatible. I think the only use case was to allow multi line newlines envvars.

The docker and old podman env file logic is dead simple it is [VAR]=[VALUE] sparated by newlines. I wouldn't argue that not handling quotes etc is a bug, there is simply no real standard for env files. Using POSIX shell rules definitely makes sense as most people would understand this but changing behaviour could break things.

In general the question is what do we want? IMO docker compatibility is important, so far the only users asked for multiline env var support in #18724 and I hoped that #19096 would only do that.

I think reverting for now is a good choice but then we need to keep in mind that variables with newlines are not supported.

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[Luap99]

4.7.0-rc1 was released and these patches are in.
If we want to revert we have to do it now before the final release.

IMO we should stick top backwards and docker compatibility. The old format was much simpler:

VAR=VALUE
HOSTVAR
GLOBVAR*
#comment

So we had either = sign separated env vars, only the env name in which case we lookup the value from the current environment, or a glob var in which case we add all host environment variables starting with that name. Add comment lines must start with #
This logic was not 100% docker compatible but still very close.

Given that this has worked fine without users complaining I see no reason to change this much. The request that started this change is #18724 and there support for newlines was asked.

Adding new support for handling quotes support the export keyword may make it better compatible with shell env files but the current code is extremely hard to understand, the regex is just insanity. And the risk of breaking compatibility is high. As @edsantiago shows at the very least we have regression regarding the # handling.

[jntesteves]

Hi, I just saw the env-file change on the changelog for v4.7.0, and I was very surprised by seeing this breaking change in a supposedly minor update. I think this change should be treated as a bug in this version and the whole feature should be reverted in a hotfix v4.7.1.

I agree with @Luap99 above in that not only backwards but also docker compatibility is desirable here. Edit: and simplicity, and maintainability.

Also, the new regex logic seems too complex to be something maintained in podman instead of a more generic lib. Inventing a new format for this seems a bit much in my opinion. I'm not a user of dotenv but from what I gather it seems to be a de-facto standard for this kind of thing, so if greater flexibility is needed, I'd suggest sticking to the standard and deferring parsing to a lib that already does that, and introducing a new option like --env-file-v2 (or --dotenv-file) for that. Since that is backwards-compatible, it might also be easier to collaborate with moby-engine developers to move forward together on the new standard.

[Luap99]

@jntesteves Does this actively effect you, i.e actually break some of your files?

I am fine with reverting it for a possible 4.7.1.

[jntesteves]

@jntesteves Does this actively effect you, i.e actually break some of your files?

As a downstream consumer of podman it changes my API in the same way it changes podman's, but I'm too small to be of your concern and my API isn't even stable yet anyway. I'm just the kind of person overly-concerned with API changes. I'm sorry for pinging you with a comment that is little more than an opinion, it was inappropriate.

[networkRob]

This change did break an existing Bitwarden pod deployment I have. I leverage an env file being mounted into the container with the --env-file flag. One of the env vars I have is for a DB password for to connect to the local db. The DB password has the # character in the value. For example:

BW_DB_PASSWORD=someRandom#Password

In 4.6.2 and earlier, that value was retained in full.

With 4.7.0, that var would be brought into the container as only:

BW_DB_PASSWORD=someRandom

Which causes a service to crash and not be able to connect to the DB.

[jntesteves]

This change did break an existing Bitwarden pod deployment I have. I leverage an env file being mounted into the container with the --env-file flag. One of the env vars I have is for a DB password for to connect to the local db. The DB password has the # character in the value. For example:

BW_DB_PASSWORD=someRandom#Password

In 4.6.2 and earlier, that value was retained in full.

With 4.7.0, that var would be brought into the container as only:

BW_DB_PASSWORD=someRandom

Which causes a service to crash and not be able to connect to the DB.

That problem was mentioned above in #19565 (comment)
It's a bug in the current implementation, not an issue of the "intended design". Although, I can see the same issue happening with values that include both single- and double-quotation marks in them, they will be impossible to represent in the new format.

But even if the intended design was good when working properly, right now it is broken with all the bugs mentioned in this issue.

[Luap99]

I am going to open a PR to revert it for 4.7.1, this was clearly not the right choice.