cleanup(shellx): do not directly depend on apex/log (#357)

CONTRIBUTING.md

@@ -59,9 +59,9 @@ run `go mod tidy` to minimize such changes.
 
 - use `./internal/atomicx` rather than `atomic/sync`
 
-- use `./internal/fsx.OpenFile` when you need to open a file
+- do not use `os/exec`, use `x/sys/execabs` or `./internal/shellx`
 
-Do now use `os/exec`, use `x/sys/execabs`.
+- use `./internal/fsx.OpenFile` when you need to open a file
 
 ## Code testing requirements
 

cmd/ooniprobe/internal/autorun/autorun_darwin.go

@@ -80,12 +80,12 @@ func (managerDarwin) LogShow() error {
 	if major < 20 /* macOS 11.0 Big Sur */ {
 		return errNotImplemented
 	}
-	return shellx.Run("log", "show", "--info", "--debug",
+	return shellx.Run(log.Log, "log", "show", "--info", "--debug",
 		"--process", "ooniprobe", "--style", "compact")
 }
 
 func (managerDarwin) LogStream() error {
-	return shellx.Run("log", "stream", "--style", "compact", "--level",
+	return shellx.Run(log.Log, "log", "stream", "--style", "compact", "--level",
 		"debug", "--process", "ooniprobe")
 }
 

internal/cmd/jafar/iptables/iptables_integration_test.go

@@ -291,7 +291,7 @@ func TestHijackHTTP(t *testing.T) {
 	if err := policy.Apply(); err != nil {
 		t.Fatal(err)
 	}
-	err = shellx.Run("sudo", "-u", "nobody", "--",
+	err = shellx.Run(log.Log, "sudo", "-u", "nobody", "--",
 		"curl", "-sf", "http://example.com")
 	if err == nil {
 		t.Fatal("expected an error here")
@@ -330,7 +330,7 @@ func TestHijackHTTPS(t *testing.T) {
 	if err := policy.Apply(); err != nil {
 		t.Fatal(err)
 	}
-	err = shellx.Run("sudo", "-u", "nobody", "--",
+	err = shellx.Run(log.Log, "sudo", "-u", "nobody", "--",
 		"curl", "-sf", "https://example.com")
 	if err == nil {
 		t.Fatal("expected an error here")

internal/cmd/jafar/iptables/iptables_linux.go

@@ -3,6 +3,7 @@
 package iptables
 
 import (
+	"github.com/apex/log"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
 	"github.com/ooni/probe-cli/v3/internal/shellx"
 )
@@ -15,55 +16,56 @@ func (s *linuxShell) createChains() (err error) {
 			// JUST KNOW WE'VE BEEN HERE
 		}
 	}()
-	err = shellx.Run("sudo", "iptables", "-N", "JAFAR_INPUT")
+	err = shellx.Run(log.Log, "sudo", "iptables", "-N", "JAFAR_INPUT")
 	runtimex.PanicOnError(err, "cannot create JAFAR_INPUT chain")
-	err = shellx.Run("sudo", "iptables", "-N", "JAFAR_OUTPUT")
+	err = shellx.Run(log.Log, "sudo", "iptables", "-N", "JAFAR_OUTPUT")
 	runtimex.PanicOnError(err, "cannot create JAFAR_OUTPUT chain")
-	err = shellx.Run("sudo", "iptables", "-t", "nat", "-N", "JAFAR_NAT_OUTPUT")
+	err = shellx.Run(log.Log, "sudo", "iptables", "-t", "nat", "-N", "JAFAR_NAT_OUTPUT")
 	runtimex.PanicOnError(err, "cannot create JAFAR_NAT_OUTPUT chain")
-	err = shellx.Run("sudo", "iptables", "-I", "OUTPUT", "-j", "JAFAR_OUTPUT")
+	err = shellx.Run(log.Log, "sudo", "iptables", "-I", "OUTPUT", "-j", "JAFAR_OUTPUT")
 	runtimex.PanicOnError(err, "cannot insert jump to JAFAR_OUTPUT")
-	err = shellx.Run("sudo", "iptables", "-I", "INPUT", "-j", "JAFAR_INPUT")
+	err = shellx.Run(log.Log, "sudo", "iptables", "-I", "INPUT", "-j", "JAFAR_INPUT")
 	runtimex.PanicOnError(err, "cannot insert jump to JAFAR_INPUT")
-	err = shellx.Run("sudo", "iptables", "-t", "nat", "-I", "OUTPUT", "-j", "JAFAR_NAT_OUTPUT")
+	err = shellx.Run(log.Log, "sudo", "iptables", "-t", "nat", "-I", "OUTPUT", "-j", "JAFAR_NAT_OUTPUT")
 	runtimex.PanicOnError(err, "cannot insert jump to JAFAR_NAT_OUTPUT")
 	return nil
 }
 
 func (s *linuxShell) dropIfDestinationEquals(ip string) error {
-	return shellx.Run("sudo", "iptables", "-A", "JAFAR_OUTPUT", "-d", ip, "-j", "DROP")
+	return shellx.Run(log.Log,
+		"sudo", "iptables", "-A", "JAFAR_OUTPUT", "-d", ip, "-j", "DROP")
 }
 
 func (s *linuxShell) rstIfDestinationEqualsAndIsTCP(ip string) error {
-	return shellx.Run(
+	return shellx.Run(log.Log,
 		"sudo", "iptables", "-A", "JAFAR_OUTPUT", "--proto", "tcp", "-d", ip,
 		"-j", "REJECT", "--reject-with", "tcp-reset",
 	)
 }
 
 func (s *linuxShell) dropIfContainsKeywordHex(keyword string) error {
-	return shellx.Run(
+	return shellx.Run(log.Log,
 		"sudo", "iptables", "-A", "JAFAR_OUTPUT", "-m", "string", "--algo", "kmp",
 		"--hex-string", keyword, "-j", "DROP",
 	)
 }
 
 func (s *linuxShell) dropIfContainsKeyword(keyword string) error {
-	return shellx.Run(
+	return shellx.Run(log.Log,
 		"sudo", "iptables", "-A", "JAFAR_OUTPUT", "-m", "string", "--algo", "kmp",
 		"--string", keyword, "-j", "DROP",
 	)
 }
 
 func (s *linuxShell) rstIfContainsKeywordHexAndIsTCP(keyword string) error {
-	return shellx.Run(
+	return shellx.Run(log.Log,
 		"sudo", "iptables", "-A", "JAFAR_OUTPUT", "-m", "string", "--proto", "tcp", "--algo",
 		"kmp", "--hex-string", keyword, "-j", "REJECT", "--reject-with", "tcp-reset",
 	)
 }
 
 func (s *linuxShell) rstIfContainsKeywordAndIsTCP(keyword string) error {
-	return shellx.Run(
+	return shellx.Run(log.Log,
 		"sudo", "iptables", "-A", "JAFAR_OUTPUT", "-m", "string", "--proto", "tcp", "--algo",
 		"kmp", "--string", keyword, "-j", "REJECT", "--reject-with", "tcp-reset",
 	)
@@ -73,7 +75,7 @@ func (s *linuxShell) hijackDNS(address string) error {
 	// Hijack any DNS query, like the Vodafone station does when using the
 	// secure network feature. Our transparent proxies will use DoT, in order
 	// to bypass this restriction and avoid routing loop.
-	return shellx.Run(
+	return shellx.Run(log.Log,
 		"sudo", "iptables", "-t", "nat", "-A", "JAFAR_NAT_OUTPUT", "-p", "udp",
 		"--dport", "53", "-j", "DNAT", "--to", address,
 	)
@@ -82,7 +84,7 @@ func (s *linuxShell) hijackDNS(address string) error {
 func (s *linuxShell) hijackHTTPS(address string) error {
 	// We need to whitelist root otherwise the traffic sent by Jafar
 	// itself will match the rule and loop.
-	return shellx.Run(
+	return shellx.Run(log.Log,
 		"sudo", "iptables", "-t", "nat", "-A", "JAFAR_NAT_OUTPUT", "-p", "tcp",
 		"--dport", "443", "-m", "owner", "!", "--uid-owner", "0",
 		"-j", "DNAT", "--to", address,
@@ -92,7 +94,7 @@ func (s *linuxShell) hijackHTTPS(address string) error {
 func (s *linuxShell) hijackHTTP(address string) error {
 	// We need to whitelist root otherwise the traffic sent by Jafar
 	// itself will match the rule and loop.
-	return shellx.Run(
+	return shellx.Run(log.Log,
 		"sudo", "iptables", "-t", "nat", "-A", "JAFAR_NAT_OUTPUT", "-p", "tcp",
 		"--dport", "80", "-m", "owner", "!", "--uid-owner", "0",
 		"-j", "DNAT", "--to", address,

internal/cmd/jafar/main.go

@@ -276,7 +276,7 @@ func main() {
 	policy := iptablesStart()
 	var err error
 	if *mainCommand != "" {
-		err = shellx.RunCommandline(fmt.Sprintf(
+		err = shellx.RunCommandline(log.Log, fmt.Sprintf(
 			"sudo -u '%s' -- %s", *mainUser, *mainCommand,
 		))
 	} else {

internal/cmd/jafar/main_test.go

@@ -6,6 +6,7 @@ import (
 	"runtime"
 	"testing"
 
+	"github.com/apex/log"
 	"github.com/ooni/probe-cli/v3/internal/cmd/jafar/iptables"
 	"github.com/ooni/probe-cli/v3/internal/shellx"
 )
@@ -74,7 +75,7 @@ func TestMustx(t *testing.T) {
 			called   int
 			exitcode int
 		)
-		err := shellx.Run("curl", "-sf", "") // cause exitcode == 3
+		err := shellx.Run(log.Log, "curl", "-sf", "") // cause exitcode == 3
 		mustx(err, "", func(ec int) {
 			called++
 			exitcode = ec

internal/shellx/shellx.go

@@ -6,11 +6,15 @@ import (
 	"os"
 	"strings"
 
-	"github.com/apex/log"
 	"github.com/google/shlex"
 	"golang.org/x/sys/execabs"
 )
 
+// Logger is the logger expected by this package.
+type Logger interface {
+	Infof(format string, v ...interface{})
+}
+
 // runconfig is the configuration for run.
 type runconfig struct {
 	// args contains the command line arguments.
@@ -43,12 +47,12 @@ func run(config runconfig) error {
 	return err
 }
 
-// Run executes the specified command with the specified args
+// Run executes the specified command with the specified args.
-func Run(name string, arg ...string) error {
+func Run(logger Logger, name string, arg ...string) error {
 	return run(runconfig{
 		args:     arg,
 		command:  name,
-		loginfof: log.Log.Infof,
+		loginfof: logger.Infof,
 		stdout:   os.Stdout,
 		stderr:   os.Stderr,
 	})
@@ -68,15 +72,17 @@ func RunQuiet(name string, arg ...string) error {
 	})
 }
 
-// RunCommandline is like Run but its only argument is a command
+// ErrNoCommandToExecute means that the command line is empty.
-// line that will be splitted using the google/shlex package.
+var ErrNoCommandToExecute = errors.New("shellx: no command to execute")
-func RunCommandline(cmdline string) error {
+
+// RunCommandline executes the given command line.
+func RunCommandline(logger Logger, cmdline string) error {
 	args, err := shlex.Split(cmdline)
 	if err != nil {
 		return err
 	}
 	if len(args) < 1 {
-		return errors.New("shellx: no command to execute")
+		return ErrNoCommandToExecute
 	}
-	return Run(args[0], args[1:]...)
+	return Run(logger, args[0], args[1:]...)
 }

internal/shellx/shellx_test.go

@@ -1,12 +1,16 @@
 package shellx
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/apex/log"
+)
 
 func TestRun(t *testing.T) {
-	if err := Run("whoami"); err != nil {
+	if err := Run(log.Log, "whoami"); err != nil {
 		t.Fatal(err)
 	}
-	if err := Run("./nonexistent/command"); err == nil {
+	if err := Run(log.Log, "./nonexistent/command"); err == nil {
 		t.Fatal("expected an error here")
 	}
 }
@@ -22,22 +26,22 @@ func TestRunQuiet(t *testing.T) {
 
 func TestRunCommandline(t *testing.T) {
 	t.Run("when the command does not parse", func(t *testing.T) {
-		if err := RunCommandline(`"foobar`); err == nil {
+		if err := RunCommandline(log.Log, `"foobar`); err == nil {
 			t.Fatal("expected an error here")
 		}
 	})
 	t.Run("when we have no arguments", func(t *testing.T) {
-		if err := RunCommandline(""); err == nil {
+		if err := RunCommandline(log.Log, ""); err == nil {
 			t.Fatal("expected an error here")
 		}
 	})
 	t.Run("when we have a single argument", func(t *testing.T) {
-		if err := RunCommandline("ls"); err != nil {
+		if err := RunCommandline(log.Log, "ls"); err != nil {
 			t.Fatal(err)
 		}
 	})
 	t.Run("when we have more than one argument", func(t *testing.T) {
-		if err := RunCommandline("ls ."); err != nil {
+		if err := RunCommandline(log.Log, "ls ."); err != nil {
 			t.Fatal(err)
 		}
 	})