Maint: rework OpenVPN custom configuration code

- Refactor code and errors returned
- Add unit tests
- Make custom config code independent from loop

internal/openvpn/custom.go

@@ -15,23 +15,25 @@ import (
 	"github.com/qdm12/gluetun/internal/provider/utils"
 )
 
-var errProcessCustomConfig = errors.New("cannot process custom config")
+var (
+	errReadCustomConfig  = errors.New("cannot read custom configuration file")
+	errExtractConnection = errors.New("cannot extract connection from custom configuration file")
+)
 
-func (l *Loop) processCustomConfig(settings configuration.OpenVPN) (
+func processCustomConfig(settings configuration.OpenVPN) (
 	lines []string, connection models.OpenVPNConnection, err error) {
 	lines, err = readCustomConfigLines(settings.Config)
 	if err != nil {
-		return nil, connection, fmt.Errorf("%w: %s", errProcessCustomConfig, err)
+		return nil, connection, fmt.Errorf("%w: %s", errReadCustomConfig, err)
 	}
 
-	lines = modifyCustomConfig(lines, settings)
-
 	connection, err = extractConnectionFromLines(lines)
 	if err != nil {
-		return nil, connection, fmt.Errorf("%w: %s", errProcessCustomConfig, err)
+		return nil, connection, fmt.Errorf("%w: %s", errExtractConnection, err)
 	}
 
-	lines = setConnectionToLines(lines, connection)
+	lines = modifyCustomConfig(lines, settings, connection)
+
 	return lines, connection, nil
 }
 
@@ -41,10 +43,10 @@ func readCustomConfigLines(filepath string) (
 	if err != nil {
 		return nil, err
 	}
-	defer file.Close()
 
 	b, err := io.ReadAll(file)
 	if err != nil {
+		_ = file.Close()
 		return nil, err
 	}
 
@@ -55,8 +57,8 @@ func readCustomConfigLines(filepath string) (
 	return strings.Split(string(b), "\n"), nil
 }
 
-func modifyCustomConfig(lines []string,
-	settings configuration.OpenVPN) (modified []string) {
+func modifyCustomConfig(lines []string, settings configuration.OpenVPN,
+	connection models.OpenVPNConnection) (modified []string) {
 	// Remove some lines
 	for _, line := range lines {
 		switch {
@@ -65,9 +67,11 @@ func modifyCustomConfig(lines []string,
 			strings.HasPrefix(line, "verb "),
 			strings.HasPrefix(line, "auth-user-pass "),
 			strings.HasPrefix(line, "user "),
-			len(settings.Cipher) > 0 && strings.HasPrefix(line, "cipher "),
-			len(settings.Cipher) > 0 && strings.HasPrefix(line, "data-ciphers"),
-			len(settings.Auth) > 0 && strings.HasPrefix(line, "auth "),
+			strings.HasPrefix(line, "proto "),
+			strings.HasPrefix(line, "remote "),
+			settings.Cipher != "" && strings.HasPrefix(line, "cipher "),
+			settings.Cipher != "" && strings.HasPrefix(line, "data-ciphers "),
+			settings.Auth != "" && strings.HasPrefix(line, "auth "),
 			settings.MSSFix > 0 && strings.HasPrefix(line, "mssfix "),
 			!settings.IPv6 && strings.HasPrefix(line, "tun-ipv6"):
 		default:
@@ -76,6 +80,8 @@ func modifyCustomConfig(lines []string,
 	}
 
 	// Add values
+	modified = append(modified, connection.ProtoLine())
+	modified = append(modified, connection.RemoteLine())
 	modified = append(modified, "mute-replay-warnings")
 	modified = append(modified, "auth-nocache")
 	modified = append(modified, "pull-filter ignore \"auth-token\"") // prevent auth failed loop
@@ -85,10 +91,10 @@ func modifyCustomConfig(lines []string,
 		modified = append(modified, "auth-user-pass "+constants.OpenVPNAuthConf)
 	}
 	modified = append(modified, "verb "+strconv.Itoa(settings.Verbosity))
-	if len(settings.Cipher) > 0 {
+	if settings.Cipher != "" {
 		modified = append(modified, utils.CipherLines(settings.Cipher, settings.Version)...)
 	}
-	if len(settings.Auth) > 0 {
+	if settings.Auth != "" {
 		modified = append(modified, "auth "+settings.Auth)
 	}
 	if settings.MSSFix > 0 {
@@ -105,64 +111,19 @@ func modifyCustomConfig(lines []string,
 	return modified
 }
 
-var errExtractConnection = errors.New("cannot extract connection")
+var (
+	errRemoteLineNotFound = errors.New("remote line not found")
+)
 
 // extractConnectionFromLines always takes the first remote line only.
-func extractConnectionFromLines(lines []string) ( //nolint:gocognit
+func extractConnectionFromLines(lines []string) (
 	connection models.OpenVPNConnection, err error) {
-	for _, line := range lines {
-		switch {
-		case strings.HasPrefix(line, "proto "):
-			fields := strings.Fields(line)
-			if n := len(fields); n != 2 { //nolint:gomnd
-				return connection, fmt.Errorf(
-					"%w: proto line has %d fields instead of 2: %s",
-					errExtractConnection, n, line)
-			}
-			connection.Protocol = fields[1]
-
-		// only take the first remote line
-		case strings.HasPrefix(line, "remote ") && connection.IP == nil:
-			fields := strings.Fields(line)
-			n := len(fields)
-			//nolint:gomnd
-			if n < 2 {
-				return connection, fmt.Errorf(
-					"%w: remote line has not enough fields: %s",
-					errExtractConnection, line)
-			}
-
-			host := fields[1]
-			if ip := net.ParseIP(host); ip != nil {
-				connection.IP = ip
-			} else {
-				return connection, fmt.Errorf(
-					"%w: for now, the remote line must contain an IP adddress: %s",
-					errExtractConnection, line)
-				// TODO resolve hostname once there is an option to allow it through
-				// the firewall before the VPN is up.
-			}
-
-			if n > 2 { //nolint:gomnd
-				port, err := strconv.Atoi(fields[2])
-				if err != nil {
-					return connection, fmt.Errorf(
-						"%w: remote line has an invalid port: %s",
-						errExtractConnection, line)
-				}
-				connection.Port = uint16(port)
-			}
-
-			if n > 3 { //nolint:gomnd
-				connection.Protocol = strings.ToLower(fields[3])
-			}
-
-			if n > 4 { //nolint:gomnd
-				return connection, fmt.Errorf(
-					"%w: remote line has too many fields: %s",
-					errExtractConnection, line)
-			}
+	for i, line := range lines {
+		newConnectionData, err := extractConnectionFromLine(line)
+		if err != nil {
+			return connection, fmt.Errorf("on line %d: %w", i+1, err)
 		}
+		connection.UpdateEmptyWith(newConnectionData)
 
 		if connection.Protocol != "" && connection.IP != nil {
 			break
@@ -170,40 +131,109 @@ func extractConnectionFromLines(lines []string) ( //nolint:gocognit
 	}
 
 	if connection.IP == nil {
-		return connection, fmt.Errorf("%w: remote line not found", errExtractConnection)
+		return connection, errRemoteLineNotFound
 	}
 
-	switch connection.Protocol {
-	case "":
-		connection.Protocol = "udp"
-	case "tcp", "udp":
-	default:
-		return connection, fmt.Errorf("%w: network protocol not supported: %s", errExtractConnection, connection.Protocol)
+	if connection.Protocol == "" {
+		connection.Protocol = constants.UDP
 	}
 
 	if connection.Port == 0 {
-		if connection.Protocol == "tcp" {
-			const defaultPort uint16 = 443
-			connection.Port = defaultPort
-		} else {
-			const defaultPort uint16 = 1194
-			connection.Port = defaultPort
+		connection.Port = 1194
+		if connection.Protocol == constants.TCP {
+			connection.Port = 443
 		}
 	}
 
 	return connection, nil
 }
 
-func setConnectionToLines(lines []string, connection models.OpenVPNConnection) (modified []string) {
-	for i, line := range lines {
-		switch {
-		case strings.HasPrefix(line, "proto "):
-			lines[i] = connection.ProtoLine()
+var (
+	errExtractProto  = errors.New("failed extracting protocol from proto line")
+	errExtractRemote = errors.New("failed extracting protocol from remote line")
+)
 
-		case strings.HasPrefix(line, "remote "):
-			lines[i] = connection.RemoteLine()
+func extractConnectionFromLine(line string) (
+	connection models.OpenVPNConnection, err error) {
+	switch {
+	case strings.HasPrefix(line, "proto "):
+		connection.Protocol, err = extractProto(line)
+		if err != nil {
+			return connection, fmt.Errorf("%w: %s", errExtractProto, err)
+		}
+
+	// only take the first remote line
+	case strings.HasPrefix(line, "remote ") && connection.IP == nil:
+		connection.IP, connection.Port, connection.Protocol, err = extractRemote(line)
+		if err != nil {
+			return connection, fmt.Errorf("%w: %s", errExtractRemote, err)
 		}
 	}
 
-	return lines
+	return connection, nil
+}
+
+var (
+	errProtoLineFieldsCount = errors.New("proto line has not 2 fields as expected")
+	errProtocolNotSupported = errors.New("network protocol not supported")
+)
+
+func extractProto(line string) (protocol string, err error) {
+	fields := strings.Fields(line)
+	if len(fields) != 2 { //nolint:gomnd
+		return "", fmt.Errorf("%w: %s", errProtoLineFieldsCount, line)
+	}
+
+	switch fields[1] {
+	case "tcp", "udp":
+	default:
+		return "", fmt.Errorf("%w: %s", errProtocolNotSupported, fields[1])
+	}
+
+	return fields[1], nil
+}
+
+var (
+	errRemoteLineFieldsCount = errors.New("remote line has not 2 fields as expected")
+	errHostNotIP             = errors.New("host is not an an IP address")
+	errPortNotValid          = errors.New("port is not valid")
+)
+
+func extractRemote(line string) (ip net.IP, port uint16,
+	protocol string, err error) {
+	fields := strings.Fields(line)
+	n := len(fields)
+
+	if n < 2 || n > 4 {
+		return nil, 0, "", fmt.Errorf("%w: %s", errRemoteLineFieldsCount, line)
+	}
+
+	host := fields[1]
+	ip = net.ParseIP(host)
+	if ip == nil {
+		return nil, 0, "", fmt.Errorf("%w: %s", errHostNotIP, host)
+		// TODO resolve hostname once there is an option to allow it through
+		// the firewall before the VPN is up.
+	}
+
+	if n > 2 { //nolint:gomnd
+		portInt, err := strconv.Atoi(fields[2])
+		if err != nil {
+			return nil, 0, "", fmt.Errorf("%w: %s", errPortNotValid, line)
+		} else if portInt < 1 || portInt > 65535 {
+			return nil, 0, "", fmt.Errorf("%w: not between 1 and 65535: %d", errPortNotValid, portInt)
+		}
+		port = uint16(portInt)
+	}
+
+	if n > 3 { //nolint:gomnd
+		switch fields[3] {
+		case "tcp", "udp":
+			protocol = fields[3]
+		default:
+			return nil, 0, "", fmt.Errorf("%w: %s", errProtocolNotSupported, fields[3])
+		}
+	}
+
+	return ip, port, protocol, nil
 }