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

corecfg: add "system.timezone" setting to the system settings #9102

Merged
merged 8 commits into from Aug 18, 2020
3 changes: 3 additions & 0 deletions overlord/configstate/configcore/handlers.go
Expand Up @@ -85,6 +85,9 @@ func init() {
// journal.persistent
addFSOnlyHandler(validateJournalSettings, handleJournalConfiguration, coreOnly)

// system.timezone
addFSOnlyHandler(validateTimezoneSettings, handleTimezoneConfiguration, coreOnly)

sysconfig.ApplyFilesystemOnlyDefaultsImpl = func(rootDir string, defaults map[string]interface{}, options *sysconfig.FilesystemOnlyApplyOptions) error {
return filesystemOnlyApply(rootDir, plainCoreConfig(defaults), options)
}
Expand Down
86 changes: 86 additions & 0 deletions overlord/configstate/configcore/timezone.go
@@ -0,0 +1,86 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2020 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package configcore

import (
"fmt"
"os"
"os/exec"
"path/filepath"
"regexp"

"github.com/snapcore/snapd/osutil"
"github.com/snapcore/snapd/overlord/configstate/config"
)

func init() {
// add supported configuration of this module
supportedConfigurations["core.system.timezone"] = true
}

var validTimezone = regexp.MustCompile(`^[a-zA-Z0-9+_-]+(/[a-zA-Z0-9+_-]+)?(/[a-zA-Z0-9+_-]+)?$`).MatchString

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I validated the regexp using the timezones available in my system and it's ok now 👍 (assuming that the "right" and "posix" timezones are not valid or not meant to be used directly)

Copy link
Collaborator

@zyga zyga Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about that myself. Perhaps we should just use: ^[a-zA-Z0-9+_-]+(/[a-zA-Z0-9+_-]+)?(/[a-zA-Z0-9+_-]+)*$ - this would allow any number of slashes.

func validateTimezoneSettings(tr config.ConfGetter) error {
timezone, err := coreCfg(tr, "system.timezone")
if err != nil {
return err
}
if timezone == "" {
return nil
}
if !validTimezone(timezone) {
return fmt.Errorf("cannot set timezone %q: name not valid", timezone)
Comment on lines +48 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go's time module has time.LoadLocation function, not sure if it's worth it but could be used as well.

location, err := time.LoadLocation("America/Los_Angeles")
if err != nil {
   ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe not a good idea, it probably depends on runtime stuff that we might want to avoid e.g. with early config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not sure about this for exactly the reason you outlined - build-host != runtime system and all that.

}

return nil
}

func handleTimezoneConfiguration(tr config.ConfGetter, opts *fsOnlyContext) error {
output, err := coreCfg(tr, "system.timezone")
mvo5 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil
}
// nothing to do
if output == "" {
return nil
}
// runtime system
if opts == nil {
output, err := exec.Command("timedatectl", "set-timezone", output).CombinedOutput()
mvo5 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return fmt.Errorf("cannot set timezone: %v", osutil.OutputErr(output, err))
}
} else {
// important to use /etc/writable/
mvo5 marked this conversation as resolved.
Show resolved Hide resolved
localtimePath := filepath.Join(opts.RootDir, "/etc/writable/localtime")
if err := os.MkdirAll(filepath.Dir(localtimePath), 0755); err != nil {
return err
}
if err := os.Symlink(filepath.Join("/usr/share/zoneinfo", output), localtimePath); err != nil {
return err
}
timezonePath := filepath.Join(opts.RootDir, "/etc/writable/timezone")
if err := osutil.AtomicWriteFile(timezonePath, []byte(output+"\n"), 0644, 0); err != nil {
return fmt.Errorf("cannot write timezone: %v", err)
}
}

return nil
}
102 changes: 102 additions & 0 deletions overlord/configstate/configcore/timezone_test.go
@@ -0,0 +1,102 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2020 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package configcore_test

import (
"os"
"path/filepath"

. "gopkg.in/check.v1"

"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/overlord/configstate/configcore"
"github.com/snapcore/snapd/release"
"github.com/snapcore/snapd/testutil"
)

type timezoneSuite struct {
configcoreSuite
}

var _ = Suite(&timezoneSuite{})

func (s *timezoneSuite) SetUpTest(c *C) {
s.configcoreSuite.SetUpTest(c)

err := os.MkdirAll(filepath.Join(dirs.GlobalRootDir, "/etc/"), 0755)
c.Assert(err, IsNil)
}

func (s *timezoneSuite) TestConfigureTimezoneInvalid(c *C) {
invalidTimezones := []string{
"no-#", "no-ä", "no/tripple/slash/",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I believe this should be "triple" in this context?

}

for _, tz := range invalidTimezones {
err := configcore.Run(&mockConf{
state: s.state,
conf: map[string]interface{}{
"system.timezone": tz,
},
})
c.Assert(err, ErrorMatches, `cannot set timezone.*`)
}
}

func (s *timezoneSuite) TestConfigureTimezoneIntegration(c *C) {
restore := release.MockOnClassic(false)
defer restore()

mockedTimedatectl := testutil.MockCommand(c, "timedatectl", "")
defer mockedTimedatectl.Restore()

validTimezones := []string{
mvo5 marked this conversation as resolved.
Show resolved Hide resolved
"UTC", "Europe/Malta", "US/Indiana-Starke", "Africa/Sao_Tome",
"America/Argentina/Cordoba", "America/Argentina/La_Rioja",
"Etc/GMT+1", "CST6CDT", "GMT0", "GMT-0", "PST8PDT",
}

for _, tz := range validTimezones {
err := configcore.Run(&mockConf{
state: s.state,
conf: map[string]interface{}{
"system.timezone": tz,
},
})
c.Assert(err, IsNil)
c.Check(mockedTimedatectl.Calls(), DeepEquals, [][]string{
{"timedatectl", "set-timezone", tz},
})
mockedTimedatectl.ForgetCalls()
}
}

func (s *timezoneSuite) TestFilesystemOnlyApply(c *C) {
conf := configcore.PlainCoreConfig(map[string]interface{}{
"system.timezone": "Europe/Berlin",
})
tmpDir := c.MkDir()
c.Assert(configcore.FilesystemOnlyApply(tmpDir, conf, nil), IsNil)

c.Check(filepath.Join(tmpDir, "/etc/writable/timezone"), testutil.FileEquals, "Europe/Berlin\n")
p, err := os.Readlink(filepath.Join(tmpDir, "/etc/writable/localtime"))
c.Assert(err, IsNil)
c.Check(p, Equals, "/usr/share/zoneinfo/Europe/Berlin")
}
11 changes: 10 additions & 1 deletion tests/core/snap-set-core-config/task.yaml
Expand Up @@ -29,7 +29,10 @@ restore: |
systemctl enable rsyslog.service
systemctl start rsyslog.service
fi
if [ -e current-timezone ]; then
timedatectl set-timezone "$(cat current-timezone)"
fi
rm -f /etc/systemd/login.conf.d/00-snap-core.conf
execute: |
Expand Down Expand Up @@ -134,3 +137,9 @@ execute: |
snap unset system system.kernel.printk.console-loglevel
not test -e /etc/sysctl.d/99-snapd.conf
sysctl -n kernel.printk|MATCH "^4\s+"
echo "setting the timezone works"
cat /etc/timezone > current-timezone
snap set system system.timezone=Europe/Malta
MATCH "Europe/Malta" /etc/timezone
test "$(readlink -f /etc/localtime)" = "/usr/share/zoneinfo/Europe/Malta"
2 changes: 2 additions & 0 deletions tests/nested/manual/core-early-config/defaults.yaml
Expand Up @@ -3,3 +3,5 @@ defaults:
service:
rsyslog:
disable: true
system:
timezone: Europe/Malta
4 changes: 4 additions & 0 deletions tests/nested/manual/core-early-config/task.yaml
Expand Up @@ -61,3 +61,7 @@ execute: |
# early config is witnessed by install hook of the pc gadget
execute_remote "cat /var/snap/pc/common/debug.txt" | MATCH "/dev/null"
execute_remote "test -L /etc/systemd/system/rsyslog.service"
# timezone is set
execute_remote "cat /etc/timezone" | MATCH "Europe/Malta"
execute_remote "readlink -f /etc/localtime" | MATCH "Europe/Malta"
Comment on lines +65 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better check would be to use install hook of this test to inspect /etc/localtime symlink, similiar to what it does for rsyslog above. This will ensure it was applied early.