feat: add ability to load env vars via profile

[loganprice]

No description provided.

[jandubois]

LGTM. I've verified that the variables are available both in the boot scripts and in user sessions.

I wish lima.env and lima.profile had newlines at the end, but that is maybe just personal preference. Same way I would have called the lima.profile file lima.sh, just to match the name it gets under /etc/profile.d.

But I'm fine with merging as-is once it passes CI.

[jandubois]

I have to revoke my approval; the missing newline on lima.env is causing a failure:

/mnt/lima-cidata/boot/03-etc-hosts.sh: line 5: LIMA_CIDATA_SLIRP_GATEWAY: unbound variable

Please add a trailing newline to lima.profile as well!

@AkihiroSuda What is the reason lima.env does not include the export keyword, so it could be sourced directly from the shell instead of being read like this:

while read -r line; do export "$line"; done <"${LIMA_CIDATA_MNT}"/lima.env

[jandubois]

The problem is read returns a non-zero exit code if it encounters EOF, even though the string is still read into the variable.

Adding the newline back is the quick fix to make it work again. Changing the format of lima.env to include the export keyword and source it directly will require approval from @AkihiroSuda first.

[AkihiroSuda]

What is the reason lima.env does not include the export keyword

Just because other env files such as /etc/environment and .env file (of docker compose) do not include export keyword.

[jandubois]

BTW, I noticed that /etc/profile.d is not sourced when I run sudo bash, but only for sudo bash --login, so I'm not sure if this is the right approach.

Tempted to say we switch to /etc/environment and make use of PAM a requirement for this to work, but need to think about it some more.

[jandubois]

Tempted to say we switch to /etc/environment and make use of PAM a requirement for this to work, but need to think about it some more.

While testing this with the default image, I see the settings from /etc/environment under sudo bash, but not in the normal user shell. I don't know why this happens; I didn't see anything relevant in the pam config files. 😞

I now wonder if we should install the variables twice, under /etc/environment as well as under /etc/profile.d. The /etc/environment file should be usable as an EnvironmentFile with systemd (I haven't tested it, and that practice seems not recommended, but I think it would be fine for a Lima instance).

Please tell me I'm confused, and that there is a simple solution to this!

[jandubois]

I see the settings from /etc/environment under sudo bash, but not in the normal user shell

This seems to be due to session persistence. When I run loginctl kill-session 2, then I see the /etc/environment variables on the new login.

So right now I feel that using just /etc/environment is the right thing to do.

Sorry for all the flip-flopping! @AkihiroSuda could you chime in with your opinion?

[AkihiroSuda]

Yes, /etc/environment SGTM.
We do not need to kill systemd session if we write /etc/environments in 00-env.sh

[jandubois]

We do not need to kill systemd session if we write /etc/environments in 00-env.sh

We would write it in boot.sh (like this PR already does). That way we can also export the variables at the same time, and all the boot scripts will inherit them automatically.

Since /etc/environment is not necessarily empty, we can't just out-right replace it. So we need to delete each variable before we add it back in, otherwise we end up with duplicate entries after each reboot.

[jandubois]

Please also rebase your PR on the latest master to resolve a problem with Github Actions and our test workflow!

[jandubois]

@loganprice Thanks for rebasing the branch; it is now passing all CI tests.

I'm going to wait until you rewrite the implementation to generate /etc/environment instead of /etc/profile.d before doing another review.

Let me know if you need any further clarification; I know I have confused everyone (including myself) by going back and forth between the different options.

[loganprice]

I'm going to wait until you rewrite the implementation to generate /etc/environment instead of /etc/profile.d before doing another review.
Changes have been made but with the golang templates syntax breaks the shell check

[jandubois]

Changes have been made but with the golang templates syntax breaks the shell check

I'll have a look; I already have some other feedback...

[jandubois]

the golang templates syntax breaks the shell check

I think this is an argument in favour of creating a lima.environment file with the contents to keep the templating out of the shell files. There are further complications with values including quotes or spaces that can be avoided by using a separate file instead of sed and echo:

#LIMA-START
{{- range $key, $val := .Env}}
{{$key}}={{$val}}
{{- end}}
#LIMA-END

We can then update /etc/environment like this:

sed -i '/#LIMA-START/,/#LIMA-END/d' /etc/environment
cat "${LIMA_CIDATA_MNT}/lima.environment" >> /etc/environment

The first line removes an existing block of our variables during a reboot and the second one appends it again. This way we maintain any other changes the user might have made to the file (as long as they don't insert them between our marker lines).

I would prefer to remove the env.* variables from lima.env and instead source lima.environment inside boot.sh:

# shellcheck disable=SC2163
while read -r line; do export "$line"; done <"${LIMA_CIDATA_MNT}"/lima.environment

I know it will be functionally identical, but having the same variables/values defined in multiple places always makes me nervous. Use a single source of truth. 😄

Keeping the values in a data file also solves the shellcheck issue.

@AkihiroSuda I just learned that it is impossible to define an environment variable whose value includes the # sign. There are no quoting/escaping rules; when the pam_env parser sees a # it replaces it with a \0 and thereby drops the rest of the line. Not happy about this, but I think for now this is acceptable?

[AkihiroSuda]

I think for now this is acceptable?

SGTM

[jandubois]

The ^ at the beginning of the regular expression is implied:

$ expr " #FOO" : "#"
0
$ expr "#FOO" : "#"
1

On some expr versions (e.g. busybox as used by Alpine) it generates some noise on stderr:

$ expr "#FOO" : "^#"
expr: warning: '^#': using '^' as the first character
of a basic regular expression is not portable; it is ignored
1

So the current version works, but I would prefer to avoid the warning and simplify as:

Suggested change

	[ "$(expr "$line" : '^#.*')" -eq 0 ] && export "$line"
	[ "$(expr "$line" : '#')" -eq 0 ] && export "$line"

Otherwise, it works exactly as I would have hoped, and I would have approved, except for this little nit.

Thank you for staying with my changing opinions on how this should be implemented!

[jandubois]

Oh, almost forgot: could you please squash the previous commits that have effectively been reverted? And also clean up the resulting commit message. Thank you!

[jandubois]

Thank you very much; it is all good now.

Curiously, when I test again now with Alpine, the /etc/environment changes are not visible in the user environment (but are visible after sudo sh). I thought they were visible in the user environment when I tested the previous version. Anyways, I don't see how this can be related to the latest change, so I think this will have to be addressed in the Alpine configuration.

[AkihiroSuda]

Late to the party, but lima.env vs lima.environment is really confusing 😓
Could you update docs/internal.md to clarify the difference?

[jandubois]

Late to the party, but lima.env vs lima.environment is really confusing 😓

Sorry, that was my fault. lime.environment → /etc/environment. lime.env is just CIDATA during boot.sh processing.

Please suggest better names, and we can rename (in addition to documentation).

[AkihiroSuda]

I’d just name it “etc_environment”