Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
r10r opened this issue Oct 7, 2024 · 9 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. stale-issue

Comments

@r10r
Copy link

r10r commented Oct 7, 2024

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

@r10r r10r added the kind/bug Categorizes issue or PR as related to a bug. label Oct 7, 2024
@Luap99
Copy link
Member

Luap99 commented Oct 7, 2024

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

@Luap99 Luap99 added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Oct 7, 2024
@r10r
Copy link
Author

r10r commented Oct 8, 2024

@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
Copy link
Member

Luap99 commented Oct 8, 2024

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
Copy link
Member

Luap99 commented Oct 8, 2024

@mheon Do you know why this is?

// Image Environment defaults
if inspectData != nil {
// Image envs from the image if they don't exist
// already, overriding the default environments
envs, err = envLib.ParseSlice(inspectData.Config.Env)
if err != nil {
return nil, fmt.Errorf("env fields from image failed to parse: %w", err)
}
defaultEnvs = envLib.Join(envLib.DefaultEnvVariables(), envLib.Join(defaultEnvs, envs))
}

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
Copy link
Member

mheon commented Oct 8, 2024

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
Copy link
Member

Luap99 commented Oct 8, 2024

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
Copy link
Member

mheon commented Oct 8, 2024

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
Copy link
Member

rhatdan commented Oct 8, 2024

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.

Copy link

github-actions bot commented Nov 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. stale-issue
Projects
None yet
Development

No branches or pull requests

4 participants