Merge pull request #1049 from mysteriumnetwork/fix-mac-ipforward

initialize commands on demand for mac/linux ipforward

nat/factory_darwin.go

@@ -23,9 +23,12 @@ import "os/exec"
 func NewService() NATService {
 	return &servicePFCtl{
 		ipForward: serviceIPForward{
-			CommandEnable:  exec.Command("/usr/sbin/sysctl", "-w", "net.inet.ip.forwarding=1"),
-			CommandDisable: exec.Command("/usr/sbin/sysctl", "-w", "net.inet.ip.forwarding=0"),
-			CommandRead:    exec.Command("/usr/sbin/sysctl", "-n", "net.inet.ip.forwarding"),
+			CommandFactory: func(name string, arg ...string) Command {
+				return exec.Command(name, arg...)
+			},
+			CommandEnable:  []string{"/usr/sbin/sysctl", "-w", "net.inet.ip.forwarding=1"},
+			CommandDisable: []string{"/usr/sbin/sysctl", "-w", "net.inet.ip.forwarding=0"},
+			CommandRead:    []string{"/usr/sbin/sysctl", "-n", "net.inet.ip.forwarding"},
 		},
 		rules: make(map[RuleForwarding]struct{}),
 	}

nat/factory_linux.go

@@ -23,9 +23,12 @@ import "os/exec"
 func NewService() NATService {
 	return &serviceIPTables{
 		ipForward: serviceIPForward{
-			CommandEnable:  exec.Command("sudo", "/sbin/sysctl", "-w", "net.ipv4.ip_forward=1"),
-			CommandDisable: exec.Command("sudo", "/sbin/sysctl", "-w", "net.ipv4.ip_forward=0"),
-			CommandRead:    exec.Command("/sbin/sysctl", "-n", "net.ipv4.ip_forward"),
+			CommandFactory: func(name string, arg ...string) Command {
+				return exec.Command(name, arg...)
+			},
+			CommandEnable:  []string{"sudo", "/sbin/sysctl", "-w", "net.ipv4.ip_forward=1"},
+			CommandDisable: []string{"sudo", "/sbin/sysctl", "-w", "net.ipv4.ip_forward=0"},
+			CommandRead:    []string{"/sbin/sysctl", "-n", "net.ipv4.ip_forward"},
 		},
 		rules: make(map[RuleForwarding]struct{}),
 	}

nat/service_ipforward.go

@@ -18,28 +18,37 @@
 package nat
 
 import (
-	"os/exec"
 	"strings"
 
 	log "github.com/cihub/seelog"
 )
 
 type serviceIPForward struct {
-	CommandEnable  *exec.Cmd
-	CommandDisable *exec.Cmd
-	CommandRead    *exec.Cmd
+	CommandEnable  []string
+	CommandDisable []string
+	CommandRead    []string
+	CommandFactory CommandFactory
 	forward        bool
 }
 
+// CommandFactory is responsible for creating new instances of command
+type CommandFactory func(name string, arg ...string) Command
+
+// Command allows us to run commands
+type Command interface {
+	CombinedOutput() ([]byte, error)
+	Output() ([]byte, error)
+}
+
 func (service *serviceIPForward) Enable() error {
 	if service.Enabled() {
 		service.forward = true
 		log.Info(natLogPrefix, "IP forwarding already enabled")
 		return nil
 	}
 
-	if output, err := service.CommandEnable.CombinedOutput(); err != nil {
-		log.Warn("Failed to enable IP forwarding: ", service.CommandEnable.Args, " Returned exit error: ", err.Error(), " Cmd output: ", string(output))
+	if output, err := service.CommandFactory(service.CommandEnable[0], service.CommandEnable[1:]...).CombinedOutput(); err != nil {
+		log.Warn("Failed to enable IP forwarding: ", service.CommandEnable[1:], " Returned exit error: ", err.Error(), " Cmd output: ", string(output))
 		return err
 	}
 
@@ -52,17 +61,17 @@ func (service *serviceIPForward) Disable() {
 		return
 	}
 
-	if output, err := service.CommandDisable.CombinedOutput(); err != nil {
-		log.Warn("Failed to disable IP forwarding: ", service.CommandDisable.Args, " Returned exit error: ", err.Error(), " Cmd output: ", string(output))
+	if output, err := service.CommandFactory(service.CommandDisable[0], service.CommandDisable[1:]...).CombinedOutput(); err != nil {
+		log.Warn("Failed to disable IP forwarding: ", service.CommandDisable[1:], " Returned exit error: ", err.Error(), " Cmd output: ", string(output))
 	}
 
 	log.Info(natLogPrefix, "IP forwarding disabled")
 }
 
 func (service *serviceIPForward) Enabled() bool {
-	output, err := service.CommandEnable.Output()
+	output, err := service.CommandFactory(service.CommandRead[0], service.CommandRead[1:]...).Output()
 	if err != nil {
-		log.Warn("Failed to check IP forwarding status: ", service.CommandRead.Args, " Returned exit error: ", err.Error(), " Cmd output: ", string(output))
+		log.Warn("Failed to check IP forwarding status: ", service.CommandRead[1:], " Returned exit error: ", err.Error(), " Cmd output: ", string(output))
 	}
 
 	return strings.TrimSpace(string(output)) == "1"

nat/service_ipforward_test.go

@@ -0,0 +1,116 @@
+/*
+ * Copyright (C) 2018 The "MysteriumNetwork/node" Authors.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 nat
+
+import (
+	"testing"
+
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"
+)
+
+type mockCommand struct {
+	CombinedOutputRes   []byte
+	CombinedOutputError error
+	OutputRes           []byte
+	OutputError         error
+}
+
+func (mc *mockCommand) CombinedOutput() ([]byte, error) {
+	return mc.CombinedOutputRes, mc.CombinedOutputError
+}
+
+func (mc *mockCommand) Output() ([]byte, error) {
+	return mc.OutputRes, mc.OutputError
+}
+
+type mockCommandFactory struct {
+	MockCommand Command
+}
+
+func (mcf *mockCommandFactory) Create(name string, arg ...string) Command {
+	return mcf.MockCommand
+}
+
+func Test_ServiceIPForward_Enabled(t *testing.T) {
+	mc := &mockCommand{
+		OutputRes: []byte("1"),
+	}
+	mf := &mockCommandFactory{
+		MockCommand: mc,
+	}
+	service := &serviceIPForward{
+		CommandFactory: mf.Create,
+		CommandRead:    []string{"doesnt", "matter"},
+	}
+
+	assert.True(t, service.Enabled())
+
+	mc.OutputRes = []byte("calm waters")
+	assert.False(t, service.Enabled())
+
+	mc.OutputError = errors.New("mass panic")
+	mc.OutputRes = []byte("1")
+	assert.True(t, service.Enabled())
+}
+
+func Test_ServiceIPForward_Enable(t *testing.T) {
+	mc := &mockCommand{
+		CombinedOutputRes: []byte("1"),
+		OutputRes:         []byte("1"),
+	}
+	mf := &mockCommandFactory{
+		MockCommand: mc,
+	}
+	service := &serviceIPForward{
+		CommandFactory: mf.Create,
+		CommandRead:    []string{"doesnt", "matter"},
+		CommandEnable:  []string{"doesnt", "matter"},
+	}
+
+	assert.Nil(t, service.Enable())
+	assert.True(t, service.forward)
+	service.forward = false
+
+	mc.OutputRes = []byte("people screaming")
+	mc.CombinedOutputError = errors.New("explosions everywhere")
+
+	assert.Equal(t, mc.CombinedOutputError, service.Enable())
+
+	mc.CombinedOutputError = nil
+
+	assert.Nil(t, mc.CombinedOutputError, service.Enable())
+}
+
+func Test_ServiceIPForward_Disable(t *testing.T) {
+	mc := &mockCommand{
+		CombinedOutputRes:   []byte("1"),
+		CombinedOutputError: errors.New("explosions everywhere"),
+	}
+	mf := &mockCommandFactory{
+		MockCommand: mc,
+	}
+	service := &serviceIPForward{
+		CommandFactory: mf.Create,
+		CommandDisable: []string{"doesnt", "matter"},
+	}
+	service.Disable()
+
+	service.forward = true
+	service.Disable()
+}