containers / podman

Podman: A tool for managing OCI containers and pods.
https://podman.io
Apache License 2.0
23.59k stars 2.4k forks source link

TZ environment variable from image overwrites timezone defined in containers.conf #24191

Open r10r opened 2 weeks ago

r10r commented 2 weeks ago

Issue Description

I think it is bad practise to set the timezone in a container image by using the the TZ variable. But some image define it nevertheless. A timezone defined in /etc/containers/containers.conf will be overwritten by the TZ variable in the image, which is not what I expect. I can overwrite the TZ variable by passing --env TZ=:/etc/localtime to podman. But this must be done for every container.

Idea

When a timezone is defined in /etc/containers/containers.conf and the image defines a TZ environment variable, then either clear it or overwrite it with TZ=:/etc/localtime unless a specific timezone is passed as environment variable with --env TZ=Somewhere

See also https://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html

Steps to reproduce the issue

Dockerfile for test image

FROM alpine:latest
RUN apk add --no-cache tzdata
ENV TZ="America/Los_Angeles" 

timezone configuration in /etc/containers/containers.conf

[containers]
tz = "Europe/Berlin"
podman run test date +%Z

Describe the results you received

PDT

Timezone is set to America/Los_Angeles

Describe the results you expected

CEST

Timezone is set to "Europe/Berlin" as defined in /etc/containers/containers.conf

podman info output

version: APIVersion: 5.2.2 Built: 1726479282 BuiltTime: Mon Sep 16 09:34:42 2024 GitCommit: "" GoVersion: go1.23.1 Os: linux OsArch: linux/amd64 Version: 5.2.2

Podman in a container

No

Privileged Or Rootless

None

Upstream Latest Release

Yes

Additional environment details

Additional environment details

Additional information

Additional information like issue happens only occasionally or issue happens with a particular architecture or on a particular setting

Luap99 commented 2 weeks ago

This works as designed. The config option does not set a timezone variable at all, instead it creates the proper /etc/localtime symlink in the container or if there is no tzdata in the container copies the file from the host.

So that does not effect the environment variable in any way so if the image sets its own TZ env this must be passed into the container unless overwritten or unset (--unsetenv). And of course normal application will always prefer the env var. If you want to set a default env var you can use the env key in containers.conf and overwrite the TZ env that way.

I don't think it is a good idea to suddenly overwrite a env from the image when a timezone is specified, if the image is hard coding this env it may have a reason for that and if we now suddenly start changing the behavior it may break others. I do agree that it may not be want most users want but I think if the env var is a concern that the default env should be set in containers.conf too rather than adding special logic to podman for the env var as well.

In any case I do not consider this a bug

r10r commented 2 weeks ago

@Luap99 Thanks for your support 👍

So that does not effect the environment variable in any way so if the image sets its own TZ env this must be passed into the container unless overwritten or unset (--unsetenv). And of course normal application will always prefer the env var. If you want to set a default env var you can use the env key in containers.conf and overwrite the TZ env that way.

And that exactly is the issue. It is not possible to overwrite TZ using the env key in containers.conf if the TZ variable is defined in the container image. I tried that and it does not work. The ENV variable defined in the image overwrites the default variable from containers.conf containers.env

Maybe a section to overwrite environment variables in containers.conf is an option e.g containers.env_overwrite ?

Luap99 commented 2 weeks ago

The ENV variable defined in the image overwrites the default variable from containers.conf containers.env

That sounds like a bug, user specified env should always overwrite the image env unless I am overlooking something here.

Luap99 commented 2 weeks ago

@mheon Do you know why this is? https://github.com/containers/podman/blob/6b0ad8269ca581dd7803ba16cf2b9772139895ab/pkg/specgen/generate/container.go#L127-L136

Basically what I think it comes down to is there is the actual default env $PATH set by us in c/common and of course images should be able to overwrite that. But then if a users adds specific envs there we treat them the same way that the image wins which seems odd for other env vars I would say.

mheon commented 2 weeks ago

Not my code originally so I can't say for sure. My understanding of precedence was Default vars < Image vars < User-added vars. Is this implying that images don't override the defaults, only get added when a variable with that name doesn't exist?

Luap99 commented 2 weeks ago

Default vars < Image vars < User-added

That is what we do but does that make sense? This example here clearly shows there is a need for a generic env overwrite for images and in my head I always assumed Image vars < config defaults < user-added. I guess the $PATH example is the one thing were we like the current way but otherwise I think config default should overwrite images IMO.

Given this can both ways the suggestion of a new env_overwrite field is likely the best way forward if we do not want to alter the existing behavior.

mheon commented 2 weeks ago

Using image-provided over defaults always seemed to make sense, but $PATH and $TZ do seem to be exceptions and there may be others. I do not want to change current behavior as it seems generally correct, but having a way to specify a default should be used instead makes sense. Probably a real pain code-wise, though, we don't have a consistent place where all the defaults are added (e.g. a lot of things like CONTAINER= get added in different places in Libpod vs stuff that comes in directly via the spec)

rhatdan commented 2 weeks ago

I think it is best to use env_override as a new containers.conf entry to override Image defaults.

The way containers.conf was designed was

BuiltinDefaults Env -> Containers.conf Env overrides -> Image Env -> User --env* Overrides

Thinking in the most case Image understands best. With PATH being the best example of this.

BuiltinDefaults Env -> Containers.conf Env overrides -> Image Env -> Containers.conf EnvOverrides -> User --env* Overrides

Where last one wins.