From 57fce171b6c4816e8dac18d984d751adb007a752 Mon Sep 17 00:00:00 2001 From: Andrew McDermott Date: Wed, 4 Jan 2017 11:03:03 +0000 Subject: [PATCH] add unit tests for bridging --- network/add-juju-bridge.py | 4 + network/bridge.go | 111 +++++------ network/bridge_test.go | 181 ++++++++---------- network/bridgescript.go | 4 + network/export_test.go | 2 + network/scriptrunner.go | 53 +++++ network/scriptrunner_test.go | 73 +++++++ network/testdata/interfaces | 2 + .../provisioner/container_initialisation.go | 3 +- 9 files changed, 269 insertions(+), 164 deletions(-) create mode 100644 network/scriptrunner.go create mode 100644 network/scriptrunner_test.go create mode 100644 network/testdata/interfaces diff --git a/network/add-juju-bridge.py b/network/add-juju-bridge.py index fac9ffb46a6..9a133cc5abe 100644 --- a/network/add-juju-bridge.py +++ b/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()) diff --git a/network/bridge.go b/network/bridge.go index 8afd79e8b7c..ac21527c087 100644 --- a/network/bridge.go +++ b/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 } diff --git a/network/bridge_test.go b/network/bridge_test.go index 221bfa38e47..635304ded7f 100644 --- a/network/bridge_test.go +++ b/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) } diff --git a/network/bridgescript.go b/network/bridgescript.go index 4e3fb0f4203..55664b10bb0 100644 --- a/network/bridgescript.go +++ b/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()) ` diff --git a/network/export_test.go b/network/export_test.go index d7652b19a65..2db754806f9 100644 --- a/network/export_test.go +++ b/network/export_test.go @@ -6,4 +6,6 @@ package network var ( NetLookupIP = &netLookupIP NetListen = &netListen + RunCommand = runCommand + BridgeCmd = bridgeCmd ) diff --git a/network/scriptrunner.go b/network/scriptrunner.go new file mode 100644 index 00000000000..bada3370507 --- /dev/null +++ b/network/scriptrunner.go @@ -0,0 +1,53 @@ +// Copyright 2017 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package network + +import ( + "time" + + "github.com/juju/errors" + "github.com/juju/utils/clock" + "github.com/juju/utils/exec" +) + +type ScriptResult struct { + Stdout []byte + Stderr []byte + Code int + TimedOut bool +} + +func runCommand(command string, environ []string, clock clock.Clock, timeout time.Duration) (*ScriptResult, error) { + cmd := exec.RunParams{ + Commands: command, + Environment: environ, + Clock: clock, + } + + err := cmd.Run() + if err != nil { + return nil, errors.Trace(err) + } + + var cancel chan struct{} + timedOut := false + + if timeout != 0 { + cancel = make(chan struct{}) + go func() { + <-clock.After(timeout) + timedOut = true + close(cancel) + }() + } + + result, err := cmd.WaitWithCancel(cancel) + + return &ScriptResult{ + Stdout: result.Stdout, + Stderr: result.Stderr, + Code: result.Code, + TimedOut: timedOut, + }, nil +} diff --git a/network/scriptrunner_test.go b/network/scriptrunner_test.go new file mode 100644 index 00000000000..3135f1a1be2 --- /dev/null +++ b/network/scriptrunner_test.go @@ -0,0 +1,73 @@ +// Copyright 2016 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package network_test + +import ( + "os" + "runtime" + "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 { + testing.IsolationSuite +} + +var _ = gc.Suite(&ScriptRunnerSuite{}) + +func (s *ScriptRunnerSuite) SetUpSuite(c *gc.C) { + if runtime.GOOS == "windows" { + c.Skip("skipping ScriptRunnerSuite 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) + c.Assert(result.Code, gc.Equals, 0) +} + +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 (*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 (*ScriptRunnerSuite) TestScriptRunnerTimeout(c *gc.C) { + result, err := network.RunCommand("sleep 6", 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) +} diff --git a/network/testdata/interfaces b/network/testdata/interfaces new file mode 100644 index 00000000000..05c54c5cb9f --- /dev/null +++ b/network/testdata/interfaces @@ -0,0 +1,2 @@ +auto ens123 +iface ens123 inet dhcp diff --git a/worker/provisioner/container_initialisation.go b/worker/provisioner/container_initialisation.go index 349b60a3ce1..ac0efbffd46 100644 --- a/worker/provisioner/container_initialisation.go +++ b/worker/provisioner/container_initialisation.go @@ -5,6 +5,7 @@ package provisioner import ( "fmt" + "os" "sync/atomic" "time" @@ -207,7 +208,7 @@ func (cs *ContainerSetup) getContainerArtifacts( return nil, nil, nil, err } - bridger := network.NewEtcNetworkInterfacesBridger(clock.WallClock, activateBridgesTimeout, instancecfg.DefaultBridgePrefix, systemNetworkInterfacesFile) + bridger := network.NewEtcNetworkInterfacesBridger(os.Environ(), clock.WallClock, activateBridgesTimeout, instancecfg.DefaultBridgePrefix, systemNetworkInterfacesFile, false) switch containerType { case instance.KVM: