libnetwork/pasta: split out argument parsing

So we can add unit tests for this, the code is not so trivial after all.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

common/libnetwork/pasta/pasta_linux.go

@@ -54,19 +54,83 @@ func Setup(opts *SetupOptions) error {
 // Note that there is no need for any special cleanup logic, the pasta
 // process will automatically exit when the netns path is deleted.
 func Setup2(opts *SetupOptions) (*SetupResult, error) {
+	path, err := opts.Config.FindHelperBinary(BinaryName, true)
+	if err != nil {
+		return nil, fmt.Errorf("could not find pasta, the network namespace can't be configured: %w", err)
+	}
+
+	cmdArgs, dnsForwardIPs, err := createPastaArgs(opts)
+	if err != nil {
+		return nil, err
+	}
+
+	logrus.Debugf("pasta arguments: %s", strings.Join(cmdArgs, " "))
+
+	// pasta forks once ready, and quits once we delete the target namespace
+	out, err := exec.Command(path, cmdArgs...).CombinedOutput()
+	if err != nil {
+		exitErr := &exec.ExitError{}
+		if errors.As(err, &exitErr) {
+			return nil, fmt.Errorf("pasta failed with exit code %d:\n%s",
+				exitErr.ExitCode(), string(out))
+		}
+		return nil, fmt.Errorf("failed to start pasta: %w", err)
+	}
+
+	if len(out) > 0 {
+		// TODO: This should be warning but right now pasta still prints
+		// things with --quiet that we do not care about.
+		// For now info is fine and we can bump it up later, it is only a
+		// nice to have.
+		logrus.Infof("pasta logged warnings: %q", string(out))
+	}
+
+	var ipv4, ipv6 bool
+	result := &SetupResult{}
+	err = ns.WithNetNSPath(opts.Netns, func(_ ns.NetNS) error {
+		addrs, err := net.InterfaceAddrs()
+		if err != nil {
+			return err
+		}
+		for _, addr := range addrs {
+			// make sure to skip localhost and other special addresses
+			if ipnet, ok := addr.(*net.IPNet); ok && ipnet.IP.IsGlobalUnicast() {
+				result.IPAddresses = append(result.IPAddresses, ipnet.IP)
+				if !ipv4 && util.IsIPv4(ipnet.IP) {
+					ipv4 = true
+				}
+				if !ipv6 && util.IsIPv6(ipnet.IP) {
+					ipv6 = true
+				}
+			}
+		}
+		return nil
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	result.IPv6 = ipv6
+	for _, ip := range dnsForwardIPs {
+		ipp := net.ParseIP(ip)
+		// add the namesever ip only if the address family matches
+		if ipv4 && util.IsIPv4(ipp) || ipv6 && util.IsIPv6(ipp) {
+			result.DNSForwardIPs = append(result.DNSForwardIPs, ip)
+		}
+	}
+
+	return result, nil
+}
+
+// createPastaArgs creates the pasta arguments, it returns the args to be passed to pasta(1) and as second arg the dns forward ips used.
+func createPastaArgs(opts *SetupOptions) ([]string, []string, error) {
 	NoTCPInitPorts := true
 	NoUDPInitPorts := true
 	NoTCPNamespacePorts := true
 	NoUDPNamespacePorts := true
 	NoMapGW := true
 
-	path, err := opts.Config.FindHelperBinary(BinaryName, true)
+	cmdArgs := []string{"--config-net"}
-	if err != nil {
-		return nil, fmt.Errorf("could not find pasta, the network namespace can't be configured: %w", err)
-	}
-
-	cmdArgs := []string{}
-	cmdArgs = append(cmdArgs, "--config-net")
 
 	for _, i := range opts.Ports {
 		protocols := strings.Split(i.Protocol, ",")
@@ -83,7 +147,7 @@ func Setup2(opts *SetupOptions) (*SetupResult, error) {
 			case "udp":
 				cmdArgs = append(cmdArgs, "-u")
 			default:
-				return nil, fmt.Errorf("can't forward protocol: %s", protocol)
+				return nil, nil, fmt.Errorf("can't forward protocol: %s", protocol)
 			}
 
 			arg := fmt.Sprintf("%s%d-%d:%d-%d", addr,
@@ -148,60 +212,5 @@ func Setup2(opts *SetupOptions) (*SetupResult, error) {
 	// always pass --quiet to silence the info output from pasta
 	cmdArgs = append(cmdArgs, "--quiet", "--netns", opts.Netns)
 
-	logrus.Debugf("pasta arguments: %s", strings.Join(cmdArgs, " "))
+	return cmdArgs, dnsForwardIPs, nil
-
-	// pasta forks once ready, and quits once we delete the target namespace
-	out, err := exec.Command(path, cmdArgs...).CombinedOutput()
-	if err != nil {
-		exitErr := &exec.ExitError{}
-		if errors.As(err, &exitErr) {
-			return nil, fmt.Errorf("pasta failed with exit code %d:\n%s",
-				exitErr.ExitCode(), string(out))
-		}
-		return nil, fmt.Errorf("failed to start pasta: %w", err)
-	}
-
-	if len(out) > 0 {
-		// TODO: This should be warning but right now pasta still prints
-		// things with --quiet that we do not care about.
-		// For now info is fine and we can bump it up later, it is only a
-		// nice to have.
-		logrus.Infof("pasta logged warnings: %q", string(out))
-	}
-
-	var ipv4, ipv6 bool
-	result := &SetupResult{}
-	err = ns.WithNetNSPath(opts.Netns, func(_ ns.NetNS) error {
-		addrs, err := net.InterfaceAddrs()
-		if err != nil {
-			return err
-		}
-		for _, addr := range addrs {
-			// make sure to skip localhost and other special addresses
-			if ipnet, ok := addr.(*net.IPNet); ok && ipnet.IP.IsGlobalUnicast() {
-				result.IPAddresses = append(result.IPAddresses, ipnet.IP)
-				if !ipv4 && util.IsIPv4(ipnet.IP) {
-					ipv4 = true
-				}
-				if !ipv6 && util.IsIPv6(ipnet.IP) {
-					ipv6 = true
-				}
-			}
-		}
-		return nil
-	})
-	if err != nil {
-		return nil, err
-	}
-
-	result.IPv6 = ipv6
-	for _, ip := range dnsForwardIPs {
-		ipp := net.ParseIP(ip)
-		// add the namesever ip only if the address family matches
-		if ipv4 && util.IsIPv4(ipp) || ipv6 && util.IsIPv6(ipp) {
-			result.DNSForwardIPs = append(result.DNSForwardIPs, ip)
-		}
-	}
-
-	return result, nil
 }

common/libnetwork/pasta/pasta_linux_test.go

@@ -0,0 +1,228 @@
+package pasta
+
+import (
+	"testing"
+
+	"github.com/containers/common/internal/attributedstring"
+	"github.com/containers/common/libnetwork/types"
+	"github.com/containers/common/pkg/config"
+	"github.com/stretchr/testify/assert"
+)
+
+func makeSetupOptions(configArgs, extraArgs []string, ports []types.PortMapping) *SetupOptions {
+	return &SetupOptions{
+		Config:       &config.Config{Network: config.NetworkConfig{PastaOptions: attributedstring.NewSlice(configArgs)}},
+		Netns:        "netns123",
+		ExtraOptions: extraArgs,
+		Ports:        ports,
+	}
+}
+
+func Test_createPastaArgs(t *testing.T) {
+	tests := []struct {
+		name           string
+		input          *SetupOptions
+		wantArgs       []string
+		wantDnsForward []string
+		wantErr        string
+	}{
+		{
+			name: "default options",
+			input: makeSetupOptions(
+				nil,
+				nil,
+				nil,
+			),
+			wantArgs: []string{
+				"--config-net", "--dns-forward", dnsForwardIpv4, "-t", "none", "-u", "none",
+				"-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "basic port",
+			input: makeSetupOptions(
+				nil,
+				nil,
+				[]types.PortMapping{{HostPort: 80, ContainerPort: 80, Protocol: "tcp", Range: 1}},
+			),
+			wantArgs: []string{
+				"--config-net", "-t", "80-80:80-80", "--dns-forward", dnsForwardIpv4, "-u", "none",
+				"-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "port range",
+			input: makeSetupOptions(
+				nil,
+				nil,
+				[]types.PortMapping{{HostPort: 80, ContainerPort: 80, Protocol: "tcp", Range: 3}},
+			),
+			wantArgs: []string{
+				"--config-net", "-t", "80-82:80-82", "--dns-forward", dnsForwardIpv4, "-u", "none",
+				"-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "different host and container port",
+			input: makeSetupOptions(
+				nil,
+				nil,
+				[]types.PortMapping{{HostPort: 80, ContainerPort: 60, Protocol: "tcp", Range: 1}},
+			),
+			wantArgs: []string{
+				"--config-net", "-t", "80-80:60-60", "--dns-forward", dnsForwardIpv4, "-u", "none",
+				"-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "tcp and udp port",
+			input: makeSetupOptions(
+				nil,
+				nil,
+				[]types.PortMapping{
+					{HostPort: 80, ContainerPort: 60, Protocol: "tcp", Range: 1},
+					{HostPort: 100, ContainerPort: 100, Protocol: "udp", Range: 1},
+				},
+			),
+			wantArgs: []string{
+				"--config-net", "-t", "80-80:60-60", "-u", "100-100:100-100", "--dns-forward",
+				dnsForwardIpv4, "-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "two tcp ports",
+			input: makeSetupOptions(
+				nil,
+				nil,
+				[]types.PortMapping{
+					{HostPort: 80, ContainerPort: 60, Protocol: "tcp", Range: 1},
+					{HostPort: 100, ContainerPort: 100, Protocol: "tcp", Range: 1},
+				},
+			),
+			wantArgs: []string{
+				"--config-net", "-t", "80-80:60-60", "-t", "100-100:100-100", "--dns-forward",
+				dnsForwardIpv4, "-u", "none", "-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "invalid port",
+			input: makeSetupOptions(
+				nil,
+				nil,
+				[]types.PortMapping{
+					{HostPort: 80, ContainerPort: 60, Protocol: "sctp", Range: 1},
+				},
+			),
+			wantErr: "can't forward protocol: sctp",
+		},
+		{
+			name: "config options before extra options",
+			input: makeSetupOptions(
+				[]string{"-i", "eth0"},
+				[]string{"-n", "24"},
+				nil,
+			),
+			wantArgs: []string{
+				"--config-net", "-i", "eth0", "-n", "24", "--dns-forward", dnsForwardIpv4,
+				"-t", "none", "-u", "none", "-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "config options before extra options",
+			input: makeSetupOptions(
+				[]string{"-i", "eth0"},
+				[]string{"-n", "24"},
+				nil,
+			),
+			wantArgs: []string{
+				"--config-net", "-i", "eth0", "-n", "24", "--dns-forward", dnsForwardIpv4,
+				"-t", "none", "-u", "none", "-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "-T option",
+			input: makeSetupOptions(
+				nil,
+				[]string{"-T", "80"},
+				nil,
+			),
+			wantArgs: []string{
+				"--config-net", "-T", "80", "--dns-forward", dnsForwardIpv4,
+				"-t", "none", "-u", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "--tcp-ns option",
+			input: makeSetupOptions(
+				nil,
+				[]string{"--tcp-ns", "80"},
+				nil,
+			),
+			wantArgs: []string{
+				"--config-net", "--tcp-ns", "80", "--dns-forward", dnsForwardIpv4,
+				"-t", "none", "-u", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "--map-gw option",
+			input: makeSetupOptions(
+				nil,
+				[]string{"--map-gw"},
+				nil,
+			),
+			wantArgs: []string{
+				"--config-net", "--dns-forward", dnsForwardIpv4, "-t", "none",
+				"-u", "none", "-T", "none", "-U", "none", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{dnsForwardIpv4},
+		},
+		{
+			name: "--dns-forward option",
+			input: makeSetupOptions(
+				nil,
+				[]string{"--dns-forward", "192.168.255.255"},
+				nil,
+			),
+			wantArgs: []string{
+				"--config-net", "--dns-forward", "192.168.255.255", "-t", "none",
+				"-u", "none", "-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{"192.168.255.255"},
+		},
+		{
+			name: "two --dns-forward options",
+			input: makeSetupOptions(
+				nil,
+				[]string{"--dns-forward", "192.168.255.255", "--dns-forward", "::1"},
+				nil,
+			),
+			wantArgs: []string{
+				"--config-net", "--dns-forward", "192.168.255.255", "--dns-forward", "::1", "-t", "none",
+				"-u", "none", "-T", "none", "-U", "none", "--no-map-gw", "--quiet", "--netns", "netns123",
+			},
+			wantDnsForward: []string{"192.168.255.255", "::1"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			args, dnsForward, err := createPastaArgs(tt.input)
+			if tt.wantErr != "" {
+				assert.EqualError(t, err, tt.wantErr, "createPastaArgs error")
+				return
+			}
+			assert.NoError(t, err, "expect no createPastaArgs error")
+			assert.Equal(t, tt.wantArgs, args, "check arguments")
+			assert.Equal(t, tt.wantDnsForward, dnsForward, "check dns forward")
+		})
+	}
+}