Merge pull request #18756 from Luap99/tz

libpod: fix timezone handling

libpod/container_internal.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -1669,6 +1670,48 @@ func (c *Container) mountStorage() (_ string, deferredErr error) {
 		}
 	}
 
+	tz := c.Timezone()
+	if tz != "" {
+		timezonePath := filepath.Join("/usr/share/zoneinfo", tz)
+		if tz == "local" {
+			timezonePath, err = filepath.EvalSymlinks("/etc/localtime")
+			if err != nil {
+				return "", fmt.Errorf("finding local timezone for container %s: %w", c.ID(), err)
+			}
+		}
+		// make sure to remove any existing localtime file in the container to not create invalid links
+		err = unix.Unlinkat(etcInTheContainerFd, "localtime", 0)
+		if err != nil && !errors.Is(err, fs.ErrNotExist) {
+			return "", fmt.Errorf("removing /etc/localtime: %w", err)
+		}
+
+		hostPath, err := securejoin.SecureJoin(mountPoint, timezonePath)
+		if err != nil {
+			return "", fmt.Errorf("resolve zoneinfo path in the container: %w", err)
+		}
+
+		_, err = os.Stat(hostPath)
+		if err != nil {
+			// file does not exists which means tzdata is not installed in the container, just create /etc/locatime which a copy from the host
+			logrus.Debugf("Timezone %s does not exist in the container, create our own copy from the host", timezonePath)
+			localtimePath, err := c.copyTimezoneFile(timezonePath)
+			if err != nil {
+				return "", fmt.Errorf("setting timezone for container %s: %w", c.ID(), err)
+			}
+			if c.state.BindMounts == nil {
+				c.state.BindMounts = make(map[string]string)
+			}
+			c.state.BindMounts["/etc/localtime"] = localtimePath
+		} else {
+			// file exists lets just symlink according to localtime(5)
+			logrus.Debugf("Create locatime symlink for %s", timezonePath)
+			err = unix.Symlinkat(".."+timezonePath, etcInTheContainerFd, "localtime")
+			if err != nil {
+				return "", fmt.Errorf("creating /etc/localtime symlink: %w", err)
+			}
+		}
+	}
+
 	// Request a mount of all named volumes
 	for _, v := range c.config.NamedVolumes {
 		vol, err := c.mountNamedVolume(v, mountPoint)

libpod/container_internal_common.go

@@ -1949,38 +1949,6 @@ func (c *Container) makeBindMounts() error {
 		}
 	}
 
-	// Make /etc/localtime
-	ctrTimezone := c.Timezone()
-	if ctrTimezone != "" {
-		// validate the format of the timezone specified if it's not "local"
-		if ctrTimezone != "local" {
-			_, err = time.LoadLocation(ctrTimezone)
-			if err != nil {
-				return fmt.Errorf("finding timezone for container %s: %w", c.ID(), err)
-			}
-		}
-		if _, ok := c.state.BindMounts["/etc/localtime"]; !ok {
-			var zonePath string
-			if ctrTimezone == "local" {
-				zonePath, err = filepath.EvalSymlinks("/etc/localtime")
-				if err != nil {
-					return fmt.Errorf("finding local timezone for container %s: %w", c.ID(), err)
-				}
-			} else {
-				zone := filepath.Join("/usr/share/zoneinfo", ctrTimezone)
-				zonePath, err = filepath.EvalSymlinks(zone)
-				if err != nil {
-					return fmt.Errorf("setting timezone for container %s: %w", c.ID(), err)
-				}
-			}
-			localtimePath, err := c.copyTimezoneFile(zonePath)
-			if err != nil {
-				return fmt.Errorf("setting timezone for container %s: %w", c.ID(), err)
-			}
-			c.state.BindMounts["/etc/localtime"] = localtimePath
-		}
-	}
-
 	_, hasRunContainerenv := c.state.BindMounts["/run/.containerenv"]
 	if !hasRunContainerenv {
 	Loop:

libpod/options.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"strings"
 	"syscall"
+	"time"
 
 	"github.com/containers/buildah/pkg/parse"
 	nettypes "github.com/containers/common/libnetwork/types"
@@ -1814,15 +1815,10 @@ func WithTimezone(path string) CtrCreateOption {
 			return define.ErrCtrFinalized
 		}
 		if path != "local" {
-			zone := filepath.Join("/usr/share/zoneinfo", path)
-
-			file, err := os.Stat(zone)
+			// validate the format of the timezone specified if it's not "local"
+			_, err := time.LoadLocation(path)
 			if err != nil {
-				return err
-			}
-			// We don't want to mount a timezone directory
-			if file.IsDir() {
-				return errors.New("invalid timezone: is a directory")
+				return fmt.Errorf("finding timezone: %w", err)
 			}
 		}
 

test/e2e/create_test.go

@@ -461,11 +461,15 @@ var _ = Describe("Podman create", func() {
 	It("podman create --tz", func() {
 		session := podmanTest.Podman([]string{"create", "--tz", "foo", "--name", "bad", ALPINE, "date"})
 		session.WaitWithDefaultTimeout()
-		Expect(session).To(ExitWithError())
+		Expect(session).To(Exit(125))
+		Expect(session.ErrorToString()).To(
+			Equal("Error: running container create option: finding timezone: unknown time zone foo"))
 
 		session = podmanTest.Podman([]string{"create", "--tz", "America", "--name", "dir", ALPINE, "date"})
 		session.WaitWithDefaultTimeout()
-		Expect(session).To(ExitWithError())
+		Expect(session).To(Exit(125))
+		Expect(session.ErrorToString()).To(
+			Equal("Error: running container create option: finding timezone: is a directory"))
 
 		session = podmanTest.Podman([]string{"create", "--tz", "Pacific/Honolulu", "--name", "zone", ALPINE, "date"})
 		session.WaitWithDefaultTimeout()

test/e2e/run_test.go

@@ -1611,6 +1611,7 @@ USER mail`, BB)
 		tzFile := filepath.Join(testDir, "tzfile.txt")
 		file, err := os.Create(tzFile)
 		Expect(err).ToNot(HaveOccurred())
+		defer os.Remove(tzFile)
 
 		_, err = file.WriteString("Hello")
 		Expect(err).ToNot(HaveOccurred())
@@ -1620,18 +1621,8 @@ USER mail`, BB)
 		session := podmanTest.Podman([]string{"run", "--tz", badTZFile, "--rm", ALPINE, "date"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).To(ExitWithError())
-		Expect(session.ErrorToString()).To(ContainSubstring("finding timezone for container"))
-
-		err = os.Remove(tzFile)
-		Expect(err).ToNot(HaveOccurred())
-
-		session = podmanTest.Podman([]string{"run", "--tz", "foo", "--rm", ALPINE, "date"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).To(ExitWithError())
-
-		session = podmanTest.Podman([]string{"run", "--tz", "America", "--rm", ALPINE, "date"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).To(ExitWithError())
+		Expect(session.ErrorToString()).To(
+			Equal("Error: running container create option: finding timezone: time: invalid location name"))
 
 		session = podmanTest.Podman([]string{"run", "--tz", "Pacific/Honolulu", "--rm", ALPINE, "date", "+'%H %Z'"})
 		session.WaitWithDefaultTimeout()

test/system/030-run.bats

@@ -472,6 +472,14 @@ json-file | f
     is "$output" "$expect" "podman run with --tz=local, matches host"
 }
 
+@test "podman run --tz with zoneinfo" {
+    # First make sure that zoneinfo is actually in the image otherwise the test is pointless
+    run_podman run --rm $SYSTEMD_IMAGE ls /usr/share/zoneinfo
+
+    run_podman run --rm --tz Europe/Berlin $SYSTEMD_IMAGE readlink /etc/localtime
+    assert "$output" == "../usr/share/zoneinfo/Europe/Berlin" "localtime is linked correctly"
+}
+
 # run with --runtime should preserve the named runtime
 @test "podman run : full path to --runtime is preserved" {
     skip_if_remote "podman-remote does not support --runtime option"

test/system/060-mount.bats

@@ -50,10 +50,6 @@ load helpers
     run_podman image mount $IMAGE
     mount_path="$output"
 
-    # Make sure that `mount -a` prints a table
-    run_podman image mount -a
-    is "$output" "$IMAGE .*$mount_path"
-
     test -d $mount_path
 
     # Image is custom-built and has a file containing the YMD tag. Check it.
@@ -66,16 +62,23 @@ load helpers
     run_podman image mount
     is "$output" "$IMAGE *$mount_path" "podman image mount with no args"
 
-    # Clean up: -f since we mounted it twice
+    # Clean up, and make sure nothing is mounted any more
     run_podman image umount -f $IMAGE
     is "$output" "$iid" "podman image umount: image ID of what was umounted"
 
     run_podman image umount $IMAGE
     is "$output" "" "podman image umount: does not re-umount"
 
-    run_podman 125 image umount no-such-container
-    is "$output" "Error: no-such-container: image not known" \
-       "error message from image umount no-such-container"
+    run_podman 125 image umount no-such-image
+    is "$output" "Error: no-such-image: image not known" \
+       "error message from image umount no-such-image"
+
+    # Tests for mount -a. This may mount more than one image! (E.g. systemd)
+    run_podman image mount -a
+    is "$output" "$IMAGE .*$mount_path"
+
+    run_podman image umount -a
+    assert "$output" =~ "$iid" "Test image is unmounted"
 
     run_podman image mount
     is "$output" "" "podman image mount, no args, after umount"

test/system/120-load.bats

@@ -63,16 +63,6 @@ verify_iid_and_name() {
     run_podman images $fqin --format '{{.Repository}}:{{.Tag}}'
     is "${lines[0]}" "$fqin" "image preserves name across save/load"
 
-    # Load with a new tag
-    local new_name=x1$(random_string 14 | tr A-Z a-z)
-    local new_tag=t1$(random_string 6 | tr A-Z a-z)
-    run_podman rmi $fqin
-
-    run_podman load -i $archive
-    run_podman images --format '{{.Repository}}:{{.Tag}}' --sort tag
-    is "${lines[0]}" "$IMAGE"     "image is preserved"
-    is "${lines[1]}" "$fqin"      "image is reloaded with old fqin"
-
     # Clean up
     run_podman rmi $fqin
 }

test/system/build-systemd-image

@@ -31,10 +31,11 @@ cd $tmpdir
 echo $YMD >testimage-id
 
 cat >Containerfile <<EOF
-FROM registry.fedoraproject.org/fedora-minimal:37
+FROM registry.fedoraproject.org/fedora-minimal:38
 LABEL created_by="$create_script @ $create_script_rev"
 LABEL created_at=$create_time_z
-RUN microdnf install -y systemd && microdnf clean all && sed -i -e 's/.*LogColor.*/LogColor=no/' /etc/systemd/system.conf
+# Note the reinstall of tzdata is required (https://bugzilla.redhat.com/show_bug.cgi?id=1870814)
+RUN microdnf install -y systemd && microdnf reinstall -y tzdata && microdnf clean all && sed -i -e 's/.*LogColor.*/LogColor=no/' /etc/systemd/system.conf
 ADD testimage-id /home/podman/
 WORKDIR /home/podman
 CMD ["/bin/echo", "This image is intended for podman CI testing"]

test/system/helpers.bash

@@ -14,7 +14,7 @@ PODMAN_TEST_IMAGE_ID=
 
 # Larger image containing systemd tools.
 PODMAN_SYSTEMD_IMAGE_NAME=${PODMAN_SYSTEMD_IMAGE_NAME:-"systemd-image"}
-PODMAN_SYSTEMD_IMAGE_TAG=${PODMAN_SYSTEMD_IMAGE_TAG:-"20230106"}
+PODMAN_SYSTEMD_IMAGE_TAG=${PODMAN_SYSTEMD_IMAGE_TAG:-"20230531"}
 PODMAN_SYSTEMD_IMAGE_FQN="$PODMAN_TEST_IMAGE_REGISTRY/$PODMAN_TEST_IMAGE_USER/$PODMAN_SYSTEMD_IMAGE_NAME:$PODMAN_SYSTEMD_IMAGE_TAG"
 
 # Remote image that we *DO NOT* fetch or keep by default; used for testing pull