add unit tests for bridging

network/add-juju-bridge.py

@@ -13,6 +13,7 @@
 import shutil
 import subprocess
 import sys
+import time
 
 # StringIO: accommodate Python2 & Python3
 
@@ -463,4 +464,7 @@ def main(args):
 # either all active interfaces, or a specific interface.
 
 if __name__ == '__main__':
+    sleep_preamble = os.getenv("ADD_JUJU_BRIDGE_SLEEP_PREAMBLE_FOR_TESTING", 0)
+    if int(sleep_preamble) > 0:
+        time.sleep(int(sleep_preamble))
     main(arg_parser().parse_args())

network/bridge.go

@@ -11,25 +11,21 @@ import (
 
 	"github.com/juju/errors"
 	"github.com/juju/utils/clock"
-	"github.com/juju/utils/exec"
 )
 
+// Bridger creates network bridges to support addressable containers.
 type Bridger interface {
+	// Turns existing devices into bridged devices.
 	Bridge(deviceNames []string) error
 }
 
-type ScriptResult struct {
-	Stdout   []byte
-	Stderr   []byte
-	Code     int
-	TimedOut bool
-}
-
 type etcNetworkInterfacesBridger struct {
-	Clock        clock.Clock
-	Timeout      time.Duration
 	BridgePrefix string
+	Clock        clock.Clock
+	DryRun       bool
+	Environ      []string
 	Filename     string
+	Timeout      time.Duration
 }
 
 var _ Bridger = (*etcNetworkInterfacesBridger)(nil)
@@ -64,72 +60,61 @@ func bestPythonVersion() string {
 }
 
 func (b *etcNetworkInterfacesBridger) Bridge(deviceNames []string) error {
-	prefix := ""
-	if b.BridgePrefix != "" {
-		prefix = fmt.Sprintf("--bridge-prefix=%s", b.BridgePrefix)
+	cmd := bridgeCmd(deviceNames, b.BridgePrefix, b.Filename, BridgeScriptPythonContent, b.DryRun)
+	logger.Debugf("bridgescript command=%s", cmd)
+	result, err := runCommand(cmd, b.Environ, b.Clock, b.Timeout)
+	if err != nil {
+		return errors.Errorf("script invocation error: %s", err)
 	}
-	cmd := fmt.Sprintf(`%s - --interfaces-to-bridge=%q --activate %s %s <<'EOF'
-%s
-EOF
-`,
-		bestPythonVersion(),
-		strings.Join(deviceNames, " "),
-		prefix,
-		b.Filename,
-		BridgeScriptPythonContent)
-
-	result, err := RunCommand(cmd, os.Environ(), b.Clock, b.Timeout)
-	logger.Infof("bridgescript command=%s", cmd)
 	logger.Infof("bridgescript result=%v, timeout=%v", result.Code, result.TimedOut)
+	if result.TimedOut {
+		return errors.Errorf("bridgescript timed out after %v", b.Timeout)
+	}
 	if result.Code != 0 {
 		logger.Errorf("bridgescript stdout\n%s\n", result.Stdout)
 		logger.Errorf("bridgescript stderr\n%s\n", result.Stderr)
+		return errors.Errorf("bridgescript failed: %s", string(result.Stderr))
 	}
-	if result.TimedOut {
-		return errors.Errorf("bridgescript timed out after %v", b.Timeout)
-	}
-	return err
+	return nil
 }
 
-func NewEtcNetworkInterfacesBridger(clock clock.Clock, timeout time.Duration, bridgePrefix, filename string) Bridger {
-	return &etcNetworkInterfacesBridger{
-		Clock:        clock,
-		Timeout:      timeout,
-		BridgePrefix: bridgePrefix,
-		Filename:     filename,
-	}
-}
+func bridgeCmd(deviceNames []string, bridgePrefix, filename, pythonScript string, dryRun bool) string {
+	dryRunOption := ""
 
-func RunCommand(command string, environ []string, clock clock.Clock, timeout time.Duration) (*ScriptResult, error) {
-	cmd := exec.RunParams{
-		Commands:    command,
-		Environment: environ,
-		Clock:       clock,
+	if bridgePrefix != "" {
+		bridgePrefix = fmt.Sprintf("--bridge-prefix=%s", bridgePrefix)
 	}
 
-	err := cmd.Run()
-	if err != nil {
-		return nil, errors.Trace(err)
+	if dryRun {
+		dryRunOption = "--dry-run"
 	}
 
-	var cancel chan struct{}
-	timedOut := false
+	return fmt.Sprintf(`
+%s - --interfaces-to-bridge=%q --activate %s %s %s <<'EOF'
+%s
+EOF
+`[1:],
+		bestPythonVersion(),
+		strings.Join(deviceNames, " "),
+		bridgePrefix,
+		dryRunOption,
+		filename,
+		pythonScript)
+}
 
-	if timeout != 0 {
-		cancel = make(chan struct{})
-		go func() {
-			<-clock.After(timeout)
-			timedOut = true
-			close(cancel)
-		}()
+// NewEtcNetworkInterfacesBridger returns a Bridger that can parse
+// /etc/network/interfaces and create new stanzas to bridge existing
+// interfaces.
+//
+// TODO(frobware): We shouldn't expose DryRun; once we implement the
+// Python-based bridge script in Go this interface can change.
+func NewEtcNetworkInterfacesBridger(environ []string, clock clock.Clock, timeout time.Duration, bridgePrefix, filename string, dryRun bool) Bridger {
+	return &etcNetworkInterfacesBridger{
+		BridgePrefix: bridgePrefix,
+		Clock:        clock,
+		DryRun:       dryRun,
+		Environ:      environ,
+		Filename:     filename,
+		Timeout:      timeout,
 	}
-
-	result, err := cmd.WaitWithCancel(cancel)
-
-	return &ScriptResult{
-		Stdout:   result.Stdout,
-		Stderr:   result.Stderr,
-		Code:     result.Code,
-		TimedOut: timedOut,
-	}, nil
 }

network/bridge_test.go

@@ -4,138 +4,119 @@
 package network_test
 
 import (
-	"fmt"
 	"os"
 	"runtime"
-	"strings"
 	"time"
 
 	"github.com/juju/juju/network"
-	coretesting "github.com/juju/juju/testing"
 	"github.com/juju/testing"
-	jc "github.com/juju/testing/checkers"
 	"github.com/juju/utils/clock"
 	gc "gopkg.in/check.v1"
 )
 
-type ScriptRunnerSuite struct {
+type BridgeSuite struct {
 	testing.IsolationSuite
 }
 
-var _ = gc.Suite(&ScriptRunnerSuite{})
+var _ = gc.Suite(&BridgeSuite{})
 
-func (s *ScriptRunnerSuite) SetUpSuite(c *gc.C) {
+const echoArgsScript = `
+import sys
+for arg in sys.argv[1:]: print(arg)
+`
+
+func (s *BridgeSuite) SetUpSuite(c *gc.C) {
 	if runtime.GOOS == "windows" {
-		c.Skip("skipping ScriptRunnerSuite tests on windows")
+		c.Skip("skipping BridgeSuite tests on windows")
 	}
 	s.IsolationSuite.SetUpSuite(c)
 }
 
-func (*ScriptRunnerSuite) TestScriptRunnerFails(c *gc.C) {
-	clock := testing.NewClock(coretesting.ZeroTime())
-	result, err := network.RunCommand("exit 1", os.Environ(), clock, 0)
-	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(result.TimedOut, gc.Equals, false)
-	c.Assert(result.Code, gc.Equals, 1)
-}
-
-func (*ScriptRunnerSuite) TestScriptRunnerSucceeds(c *gc.C) {
-	clock := testing.NewClock(coretesting.ZeroTime())
-	result, err := network.RunCommand("exit 0", os.Environ(), clock, 0)
-	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(result.TimedOut, gc.Equals, false)
+func assertCmdResult(c *gc.C, cmd, expected string) {
+	result, err := network.RunCommand(cmd, os.Environ(), clock.WallClock, 0)
+	c.Assert(err, gc.IsNil)
 	c.Assert(result.Code, gc.Equals, 0)
+	c.Assert(string(result.Stdout), gc.Equals, expected)
+	c.Assert(string(result.Stderr), gc.Equals, "")
 }
 
-func (*ScriptRunnerSuite) TestScriptRunnerCheckStdout(c *gc.C) {
-	clock := testing.NewClock(coretesting.ZeroTime())
-	result, err := network.RunCommand("echo -n 42", os.Environ(), clock, 0)
-	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(result.TimedOut, gc.Equals, false)
-	c.Assert(result.Code, gc.Equals, 0)
-	c.Check(string(result.Stdout), gc.Equals, "42")
-	c.Check(string(result.Stderr), gc.Equals, "")
+func (*BridgeSuite) TestBridgeCmdArgumentsNoBridgePrefixAndDryRun(c *gc.C) {
+	deviceNames := []string{"ens3", "ens4", "bond0"}
+	cmd := network.BridgeCmd(deviceNames, "", "/etc/network/interfaces", echoArgsScript, true)
+	assertCmdResult(c, cmd, `
+--interfaces-to-bridge=ens3 ens4 bond0
+--activate
+--dry-run
+/etc/network/interfaces
+`[1:])
 }
 
-func (*ScriptRunnerSuite) TestScriptRunnerCheckStderr(c *gc.C) {
-	clock := testing.NewClock(coretesting.ZeroTime())
-	result, err := network.RunCommand(">&2 echo -n 3.141", os.Environ(), clock, 0)
-	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(result.TimedOut, gc.Equals, false)
-	c.Assert(result.Code, gc.Equals, 0)
-	c.Check(string(result.Stdout), gc.Equals, "")
-	c.Check(string(result.Stderr), gc.Equals, "3.141")
+func (*BridgeSuite) TestBridgeCmdArgumentsWithBridgePrefixAndDryRun(c *gc.C) {
+	deviceNames := []string{"ens3", "ens4", "bond0"}
+	cmd := network.BridgeCmd(deviceNames, "foo-", "/etc/network/interfaces", echoArgsScript, true)
+	assertCmdResult(c, cmd, `
+--interfaces-to-bridge=ens3 ens4 bond0
+--activate
+--bridge-prefix=foo-
+--dry-run
+/etc/network/interfaces
+`[1:])
 }
 
-func (*ScriptRunnerSuite) TestScriptRunnerTimeout(c *gc.C) {
-	result, err := network.RunCommand("sleep 60", os.Environ(), clock.WallClock, 500*time.Microsecond)
-	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(result.TimedOut, gc.Equals, true)
-	c.Assert(result.Code, gc.Equals, 0)
+func (*BridgeSuite) TestBridgeCmdArgumentsWithBridgePrefixWithoutDryRun(c *gc.C) {
+	deviceNames := []string{"ens3", "ens4", "bond0"}
+	cmd := network.BridgeCmd(deviceNames, "foo-", "/etc/network/interfaces", echoArgsScript, false)
+	assertCmdResult(c, cmd, `
+--interfaces-to-bridge=ens3 ens4 bond0
+--activate
+--bridge-prefix=foo-
+/etc/network/interfaces
+`[1:])
 }
 
-func (*ScriptRunnerSuite) TestBridgeScriptInvocationWithBadArg(c *gc.C) {
-	args := []string{"--big-bad-bogus-arg"}
-
-	cmd := fmt.Sprintf(`
-if [ -x "$(command -v python2)" ]; then
-    PREFERRED_PYTHON_BINARY=/usr/bin/python2
-elif [ -x "$(command -v python3)" ]; then
-    PREFERRED_PYTHON_BINARY=/usr/bin/python3
-elif [ -x "$(command -v python)" ]; then
-    PREFERRED_PYTHON_BINARY=/usr/bin/python
-fi
-
-if ! [ -x "$(command -v $PREFERRED_PYTHON_BINARY)" ]; then
-    echo "error: $PREFERRED_PYTHON_BINARY not executable, or not a command" >&2
-    exit 1
-fi
-
-${PREFERRED_PYTHON_BINARY} - %s <<'EOF'
-%s
-EOF
-`,
-		strings.Join(args, " "),
-		network.BridgeScriptPythonContent)
-
-	result, err := network.RunCommand(cmd, os.Environ(), clock.WallClock, 0)
+func (*BridgeSuite) TestBridgeCmdArgumentsWithoutBridgePrefixAndWithoutDryRun(c *gc.C) {
+	deviceNames := []string{"ens3", "ens4", "bond0"}
+	cmd := network.BridgeCmd(deviceNames, "", "/etc/network/interfaces", echoArgsScript, false)
+	assertCmdResult(c, cmd, `
+--interfaces-to-bridge=ens3 ens4 bond0
+--activate
+/etc/network/interfaces
+`[1:])
+}
 
-	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(result.TimedOut, gc.Equals, false)
-	c.Assert(result.Code, gc.Equals, 2) // Python argparse error
+func (*BridgeSuite) TestENIBridgerWithMissingFilenameArgument(c *gc.C) {
+	deviceNames := []string{"ens3", "ens4", "bond0"}
+	bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, 0, "", "", true)
+	err := bridger.Bridge(deviceNames)
+	c.Assert(err, gc.ErrorMatches, `(?s)bridgescript failed:.*too few arguments\n`)
 }
 
-func (*ScriptRunnerSuite) TestBridgeScriptInvocationWithDryRun(c *gc.C) {
-	args := []string{
-		"--interfaces-to-bridge=non-existent",
-		"--dry-run",
-		"/dev/null",
-	}
+func (*BridgeSuite) TestENIBridgerWithEmptyDeviceNamesArgument(c *gc.C) {
+	bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, 0, "", "", true)
+	err := bridger.Bridge([]string{})
+	c.Assert(err, gc.ErrorMatches, `(?s)bridgescript failed:.*too few arguments\n`)
+}
 
-	cmd := fmt.Sprintf(`
-if [ -x "$(command -v python2)" ]; then
-    PREFERRED_PYTHON_BINARY=/usr/bin/python2
-elif [ -x "$(command -v python3)" ]; then
-    PREFERRED_PYTHON_BINARY=/usr/bin/python3
-elif [ -x "$(command -v python)" ]; then
-    PREFERRED_PYTHON_BINARY=/usr/bin/python
-fi
-
-if ! [ -x "$(command -v $PREFERRED_PYTHON_BINARY)" ]; then
-    echo "error: $PREFERRED_PYTHON_BINARY not executable, or not a command" >&2
-    exit 1
-fi
-
-${PREFERRED_PYTHON_BINARY} - %s <<'EOF'
-%s
-EOF
-`,
-		strings.Join(args, " "),
-		network.BridgeScriptPythonContent)
+func (*BridgeSuite) TestENIBridgerWithNonExistentFile(c *gc.C) {
+	deviceNames := []string{"ens3", "ens4", "bond0"}
+	bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, 0, "", "testdata/non-existent-file", true)
+	err := bridger.Bridge(deviceNames)
+	c.Assert(err, gc.NotNil)
+	c.Check(err, gc.ErrorMatches, `(?s).*IOError:.*No such file or directory: 'testdata/non-existent-file'\n`)
+}
 
-	result, err := network.RunCommand(cmd, os.Environ(), clock.WallClock, 0)
+func (*BridgeSuite) TestENIBridgerWithTimeout(c *gc.C) {
+	environ := os.Environ()
+	environ = append(environ, "ADD_JUJU_BRIDGE_SLEEP_PREAMBLE_FOR_TESTING=10")
+	deviceNames := []string{"ens3", "ens4", "bond0"}
+	bridger := network.NewEtcNetworkInterfacesBridger(environ, clock.WallClock, 1*time.Second, "", "testdata/non-existent-file", true)
+	err := bridger.Bridge(deviceNames)
+	c.Assert(err, gc.NotNil)
+	c.Check(err, gc.ErrorMatches, `bridgescript timed out after 1s`)
+}
 
-	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(result.TimedOut, gc.Equals, false)
-	c.Assert(result.Code, gc.Equals, 0)
+func (*BridgeSuite) TestENIBridgerWithDryRun(c *gc.C) {
+	bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, 1*time.Second, "", "testdata/interfaces", true)
+	err := bridger.Bridge([]string{"ens123"})
+	c.Assert(err, gc.IsNil)
 }

network/bridgescript.go

@@ -17,6 +17,7 @@ import re
 import shutil
 import subprocess
 import sys
+import time
 
 # StringIO: accommodate Python2 & Python3
 
@@ -467,5 +468,8 @@ def main(args):
 # either all active interfaces, or a specific interface.
 
 if __name__ == '__main__':
+    sleep_preamble = os.getenv("ADD_JUJU_BRIDGE_SLEEP_PREAMBLE_FOR_TESTING", 0)
+    if int(sleep_preamble) > 0:
+        time.sleep(int(sleep_preamble))
     main(arg_parser().parse_args())
 `