/juju

Permalink
Browse files

Use a script that identifies the init system.

  • Loading branch information...
1 parent 772cb76 commit 2ae93fe45df4b95bedf493adc07ee7049859fb2d @ericsnowcurrently ericsnowcurrently committed Mar 16, 2015
Unified Split
Showing with 111 additions and 74 deletions.
  1. +1 −1 environs/manual/init.go
  2. +73 −26 service/discovery.go
  3. +15 −5 service/service.go
  4. +22 −42 service/service_test.go
View
2 environs/manual/init.go
@@ -34,7 +34,7 @@ var CheckProvisioned = checkProvisioned
func checkProvisioned(host string) (bool, error) {
logger.Infof("Checking if %s is already provisioned", host)
- script := service.ListServicesCommand()
+ script := strings.Join(service.ListServicesCommand(), "\n")
cmd := ssh.Command("ubuntu@"+host, []string{"/bin/bash"}, nil)
var stdout, stderr bytes.Buffer
View
99 service/discovery.go
@@ -166,48 +166,95 @@ func identifyExecutable(executable string) (string, bool) {
}
}
-// TODO(ericsnow) Synchronize newShellSelectCommand with discoverLocalInitSystem.
+// TODO(ericsnow) Build this script more dynamically (using shell.Renderer).
+// TODO(ericsnow) Use a case statement in the script?
-type initSystem struct {
- executable string
- name string
-}
+// DiscoverInitSystemScript is the shell script to use when
+// discovering the local init system.
+const DiscoverInitSystemScript = `#!/usr/bin/env bash
-var linuxExecutables = []initSystem{
- // Note that some systems link /sbin/init to whatever init system
- // is supported, so in the future we may need some other way to
- // identify upstart uniquely.
- {"/sbin/init", InitSystemUpstart},
- {"/sbin/upstart", InitSystemUpstart},
- {"/sbin/systemd", InitSystemSystemd},
- {"/bin/systemd", InitSystemSystemd},
- {"/lib/systemd/systemd", InitSystemSystemd},
-}
+# Find the executable.
+executable=$(cat /proc/1/cmdline | awk -F"\0" '{print $1}')
+if [[ $? ]]; then
+ exit 1
+fi
+
@xnox

xnox Mar 31, 2015

Contributor

I cringe reading this =)

[ -d /run/systemd/systemd ] -> system is running PID 1 as systemd. You can check that in go code natively, without forking out to a shell.

check that system is running upstart can be done with

  • "initctl version"
  • or check that unix:abstract=/com/ubuntu/upstart socket exists
  • or as a private DBus socket call to Version api over unix:abstract=/com/ubuntu/upstart
@martinpitt

martinpitt Mar 31, 2015

This is also wrong -- /sbin/init is different depending on whether you have systemd-sysv or upstart-sysv installed, so checking /proc/cmdline for /sbin/init doesn't tell you anything. Checking for /run/systemd/system is the correct, reliable, and fast thing to do here indeed.

@ericsnowcurrently

ericsnowcurrently Mar 31, 2015

Contributor

@xnox As noted in my last comment, we have to be able to run this on arbitrary remote systems, both via ssh and a cloud-init script. So using a bash script was our best option. We also needed to be confident about the running init system, which is what checking PID 1 accomplishes perfectly.

@martinpitt We no longer assume /sbin/init implies upstart.

@martinpitt

martinpitt Mar 31, 2015

You can run ssh remote.server test -d /run/systemd/system through ssh just fine, though.
+# Check the executable.
+if [[ $executable == *"systemd"* ]]; then
+ echo -n systemd
+ exit $?
+elif [[ $executable == *"upstart"* ]]; then
+ echo -n upstart
+ exit $?
+fi
+
+# First fall back to following symlinks.
+if [[ -L $executable ]]; then
+ linked=$(readlink "$(executable)")
+ if [[ ! $? ]]; then
+ executable=$linked
+ fi
+
+ # Check the linked executable.
+ if [[ $executable == *"systemd"* ]]; then
+ echo -n systemd
+ exit $?
+ elif [[ $executable == *"upstart"* ]]; then
+ echo -n upstart
+ exit $?
+ fi
+fi
-// TODO(ericsnow) Is it too much to cat once for each executable?
-const initSystemTest = `[[ "$(cat ` + pid1 + ` | awk '{print $1}')" == "%s" ]]`
+# Fall back to checking the "version" text.
+verText=$("${executable}" --version)
+if [[ $? ]]; then
+ exit 1
+else
+ if [[ $verText == *"systemd"* ]]; then
+ echo -n systemd
+ exit $?
+ elif [[ $verText == *"upstart"* ]]; then
+ echo -n upstart
+ exit $?
+ fi
+fi
+
+# uh-oh
+exit 1
+`
+
+func writeDiscoverInitSystemScript(filename string) []string {
+ // TODO(ericsnow) Use utils.shell.Renderer.WriteScript.
+ return []string{
+ fmt.Sprintf(`
+cat > %s << 'EOF'
+%s
+EOF`[1:], filename, DiscoverInitSystemScript),
+ "chmod 0755 " + filename,
+ }
+}
// newShellSelectCommand creates a bash if statement with an if
// (or elif) clause for each of the executables in linuxExecutables.
// The body of each clause comes from calling the provided handler with
// the init system name. If the handler does not support the args then
// it returns a false "ok" value.
-func newShellSelectCommand(handler func(string) (string, bool)) string {
- // TODO(ericsnow) Allow passing in "initSystems ...string".
- executables := linuxExecutables
-
- // TODO(ericsnow) build the command in a better way?
+func newShellSelectCommand(discoverScript string, handler func(string) (string, bool)) string {
+ // TODO(ericsnow) Build the command in a better way?
+ // TODO(ericsnow) Use a case statement?
cmdAll := ""
- for _, initSystem := range executables {
- cmd, ok := handler(initSystem.name)
+ for _, initSystem := range linuxInitSystems {
+ cmd, ok := handler(initSystem)
if !ok {
continue
}
- test := fmt.Sprintf(initSystemTest, initSystem.executable)
+ test := fmt.Sprintf("[[ $init_system == %q ]]", initSystem)
cmd = fmt.Sprintf("if %s; then %s\n", test, cmd)
- if cmdAll != "" {
+ if cmdAll == "" {
+ cmd = "init_system=$(" + discoverScript + ") " + cmd
+ } else {
cmd = "el" + cmd
}
cmdAll += cmd
View
20 service/service.go
@@ -20,11 +20,18 @@ var (
// These are the names of the init systems regognized by juju.
const (
- InitSystemWindows = "windows"
- InitSystemUpstart = "upstart"
InitSystemSystemd = "systemd"
+ InitSystemUpstart = "upstart"
+ InitSystemWindows = "windows"
)
+// linuxInitSystems lists the names of the init systems that juju might
+// find on a linux host.
+var linuxInitSystems = []string{
+ InitSystemSystemd,
+ InitSystemUpstart,
+}
+
// ServiceActions represents the actions that may be requested for
// an init system service.
type ServiceActions interface {
@@ -141,10 +148,13 @@ func ListServices() ([]string, error) {
}
}
-// ListServicesCommand returns the command that should be run to get
+// ListServicesScript returns the commands that should be run to get
// a list of service names on a host.
-func ListServicesCommand() string {
- return newShellSelectCommand(listServicesCommand)
+func ListServicesScript() []string {
+ filename := "/tmp/discover_init_system.sh"
@xnox

xnox Mar 31, 2015

Contributor

This is a security vulnerability, no? symlink race.

@ericsnowcurrently

ericsnowcurrently Mar 31, 2015

Contributor

If it's a security issue then we should fix it. Could you elaborate? What would you suggest to fix it?

@martinpitt

martinpitt Mar 31, 2015

It's a predictable file name in a world-writable directory. So any user can destroy any file in the system (also from other users, or from root) by e. g. ln -s /etc/shadow /tmp/discover_init_system.sh and then wait for juju to run. (google for /tmp overwrite race, there have been plenty of CVEs in the past). So if you create/call a shell script, please use mktemp.

But I'm still convinced that this is overkill. You can check what's running on a remote system with a single shell command (testing for /run/systemd/system and/or for initctl --system list).

@ericsnowcurrently

ericsnowcurrently Mar 31, 2015

Contributor

Thanks for the explanation. I'll open a bug for this.
+ commands := writeDiscoverInitSystemScript(filename)
+ commands = append(commands, newShellSelectCommand(filename, listServicesCommand))
+ return commands
}
func listServicesCommand(initSystem string) (string, bool) {
View
64 service/service_test.go
@@ -4,7 +4,6 @@
package service_test
import (
- "fmt"
"strings"
"github.com/juju/errors"
@@ -66,47 +65,28 @@ func (s *serviceSuite) TestListServices(c *gc.C) {
c.Check(err, jc.ErrorIsNil)
}
-// checkShellSwitch examines the contents a fragment of shell script that implements a switch
-// using an if, elif, else chain. It tests that each command in expectedCommands is used once
-// and that the whole script fragment ends with "else exit 1". The order of commands in
-// script doesn't matter.
-func checkShellSwitch(c *gc.C, script string, expectedCommands []string) {
- cmds := strings.Split(script, "\n")
-
- // Ensure that we terminate the if, elif, else chain correctly
- last := len(cmds) - 1
- c.Check(cmds[last-1], gc.Equals, "else exit 1")
- c.Check(cmds[last], gc.Equals, "fi")
-
- // First line must start with if
- c.Check(cmds[0][0:3], gc.Equals, "if ")
-
- // Further lines must start with elif. Convert them to if <statement>
- for i := 1; i < last-1; i++ {
- c.Check(cmds[i][0:5], gc.Equals, "elif ")
- cmds[i] = cmds[i][2:]
- }
-
- c.Check(cmds[0:last-1], jc.SameContents, expectedCommands)
-}
-
-func (*serviceSuite) TestListServicesCommand(c *gc.C) {
- cmd := service.ListServicesCommand()
-
- line := `if [[ "$(cat /proc/1/cmdline | awk '{print $1}')" == "%s" ]]; then %s`
- upstart := `sudo initctl list | awk '{print $1}' | sort | uniq`
- systemd := `/bin/systemctl list-unit-files --no-legend --no-page -t service` +
- ` | grep -o -P '^\w[\S]*(?=\.service)'`
-
- lines := []string{
- fmt.Sprintf(line, "/sbin/init", upstart),
- fmt.Sprintf(line, "/sbin/upstart", upstart),
- fmt.Sprintf(line, "/sbin/systemd", systemd),
- fmt.Sprintf(line, "/bin/systemd", systemd),
- fmt.Sprintf(line, "/lib/systemd/systemd", systemd),
- }
-
- checkShellSwitch(c, cmd, lines)
+func (*serviceSuite) TestListServicesScript(c *gc.C) {
+ commands := service.ListServicesScript()
+
+ writeLines := "cat > /tmp/discover_init_system.sh << 'EOF'\n" +
+ service.DiscoverInitSystemScript + "\n" +
+ "EOF"
+ switchLines := "" +
+ "init_system=$(/tmp/discover_init_system.sh) " +
+ `if [[ $init_system == "systemd" ]]; then ` +
+ `/bin/systemctl list-unit-files --no-legend --no-page -t service` +
+ ` | grep -o -P '^\w[\S]*(?=\.service)'` + "\n" +
+ `elif [[ $init_system == "upstart" ]]; then ` +
+ `sudo initctl list | awk '{print $1}' | sort | uniq` + "\n" +
+ `else exit 1` + "\n" +
+ `fi`
+ c.Check(commands, jc.DeepEquals, []string{
+ writeLines,
+ "chmod 0755 /tmp/discover_init_system.sh",
+ switchLines,
+ })
+ c.Check(strings.Split(commands[0], "\n"), jc.DeepEquals, strings.Split(writeLines, "\n"))
+ c.Check(strings.Split(commands[2], "\n"), jc.DeepEquals, strings.Split(switchLines, "\n"))
}
func (s *serviceSuite) TestInstallAndStartOkay(c *gc.C) {

2 comments on commit 2ae93fe

@xnox
Contributor

xnox replied Mar 31, 2015

Please drop all of this crazy shell scripts forking with simply:

if _, err := os.Stat("/run/systemd/system"); err == nil {
    // pid 1 is systemd
} else {
    // pid 1 is upstart
}
@ericsnowcurrently
Contributor

ericsnowcurrently replied Mar 31, 2015

While that works for the local host, for interacting with remote hosts it isn't an option. We went with a bash script in that case for the confidence that it will work effectively universally. If you know of a better approach that can be used over ssh on arbitrary hosts (as well as in a cloud-init script) then please let us know.

As to the commands for deciding the init system, we went with using the executable running in PID 1 because the approach gives better information in the case that we don't recognize the init system. Furthermore, the only way to be absolutely sure about the running init system is by checking PID 1. Beyond that, we wanted to bake into juju as little knowledge of each init system as possible. The directory that the init system uses is an example of that.
Please sign in to comment.