Fix failure to start CNS when setting WireserverIP (#1707)

There was insufficient coverage over cases involving different
permutations of the "WireserverIP" configuration option. Consequently,
there were instances where reasonable values for this option caused CNS
to fail to start.

This moves the logic for transforming the CNS configuration into
configuration suitable for the NMAgent client into a method off the
CNSConfig. It also permits adding coverage over different scenarios that
are likely to emerge.

cns/configuration/configuration.go

@@ -3,8 +3,12 @@ package configuration
 
 import (
 	"encoding/json"
+	"net"
+	"net/url"
 	"os"
 	"path/filepath"
+	"strconv"
+	"strings"
 
 	"github.com/Azure/azure-container-networking/cns"
 	"github.com/Azure/azure-container-networking/cns/logger"
@@ -105,6 +109,53 @@ func getConfigFilePath(cmdLineConfigPath string) (string, error) {
 	return configpath, nil
 }
 
+type NMAgentConfig struct {
+	Host   string
+	Port   uint16
+	UseTLS bool
+}
+
+func (c *CNSConfig) NMAgentConfig() (NMAgentConfig, error) {
+	host := "168.63.129.16" // wireserver's IP
+
+	if c.WireserverIP != "" {
+		host = c.WireserverIP
+	}
+
+	if strings.Contains(host, "http") {
+		parts, err := url.Parse(host)
+		if err != nil {
+			return NMAgentConfig{}, errors.Wrap(err, "parsing WireserverIP as URL")
+		}
+		host = parts.Host
+	}
+
+	// create an NMAgent Client based on provided configuration
+	if strings.Contains(host, ":") {
+		host, prt, err := net.SplitHostPort(host) //nolint:govet // it's fine to shadow err here
+		if err != nil {
+			return NMAgentConfig{}, errors.Wrap(err, "splitting wireserver IP into host port")
+		}
+
+		port, err := strconv.ParseUint(prt, 10, 16) //nolint:gomnd // obvious from ParseUint docs
+		if err != nil {
+			return NMAgentConfig{}, errors.Wrap(err, "parsing wireserver port value as uint16")
+		}
+
+		return NMAgentConfig{
+			Host: host,
+			Port: uint16(port),
+		}, nil
+	}
+
+	return NMAgentConfig{
+		Host: host,
+
+		// nolint:gomnd // there's no benefit to constantizing a well-known port
+		Port: 80,
+	}, nil
+}
+
 // ReadConfig returns a CNS config from file or an error.
 func ReadConfig(cmdLineConfigPath string) (*CNSConfig, error) {
 	configpath, err := getConfigFilePath(cmdLineConfigPath)

cns/configuration/configuration_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/Azure/azure-container-networking/common"
+	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -258,3 +259,99 @@ func TestSetCNSConfigDefaults(t *testing.T) {
 		})
 	}
 }
+
+func TestNMAgentConfig(t *testing.T) {
+	tests := []struct {
+		name      string
+		conf      CNSConfig
+		exp       NMAgentConfig
+		shouldErr bool
+	}{
+		{
+			"empty",
+			CNSConfig{
+				WireserverIP: "",
+			},
+			NMAgentConfig{
+				Host: "168.63.129.16",
+				Port: 80,
+			},
+			false,
+		},
+		{
+			"ip",
+			CNSConfig{
+				WireserverIP: "127.0.0.1",
+			},
+			NMAgentConfig{
+				Host: "127.0.0.1",
+				Port: 80,
+			},
+			false,
+		},
+		{
+			"ipport",
+			CNSConfig{
+				WireserverIP: "127.0.0.1:8080",
+			},
+			NMAgentConfig{
+				Host: "127.0.0.1",
+				Port: 8080,
+			},
+			false,
+		},
+		{
+			"scheme",
+			CNSConfig{
+				WireserverIP: "http://127.0.0.1:8080",
+			},
+			NMAgentConfig{
+				Host: "127.0.0.1",
+				Port: 8080,
+			},
+			false,
+		},
+		{
+			"invalid URL",
+			CNSConfig{
+				WireserverIP: "a string containing \"http\" with an invalid character: \x7F",
+			},
+			NMAgentConfig{},
+			true,
+		},
+		{
+			"invalid host:port",
+			CNSConfig{
+				WireserverIP: "way:too:many:colons",
+			},
+			NMAgentConfig{},
+			true,
+		},
+		{
+			"too big for a uint16 port",
+			CNSConfig{
+				WireserverIP: "127.0.0.1:4815162342",
+			},
+			NMAgentConfig{},
+			true,
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			got, err := test.conf.NMAgentConfig()
+			if err != nil && !test.shouldErr {
+				t.Fatal("unexpected error fetching nmagent config: err:", err)
+			}
+
+			if err == nil && test.shouldErr {
+				t.Fatal("expected error fetching nmagent config but received none")
+			}
+
+			if !cmp.Equal(got, test.exp) {
+				t.Error("received config differs from expectation: diff:", cmp.Diff(got, test.exp))
+			}
+		})
+	}
+}

cns/service/main.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"net"
 	"net/http"
 	"net/http/pprof"
 	"os"
@@ -512,32 +511,13 @@ func main() {
 	z, _ := zap.NewProduction()
 	go healthserver.Start(z, cnsconfig.MetricsBindAddress)
 
-	nmaConfig := nmagent.Config{
-		Host: "168.63.129.16", // wireserver
-
-		// nolint:gomnd // there's no benefit to constantizing a well-known port
-		Port: 80,
+	nmaConfig, err := cnsconfig.NMAgentConfig()
+	if err != nil {
+		logger.Errorf("[Azure CNS] Failed to produce NMAgent config from supplied configuration: %v", err)
+		return
 	}
 
-	// create an NMAgent Client based on provided configuration
-	if cnsconfig.WireserverIP != "" {
-		host, prt, err := net.SplitHostPort(cnsconfig.WireserverIP) //nolint:govet // it's fine to shadow err here
-		if err != nil {
-			logger.Errorf("[Azure CNS] Invalid IP for Wireserver: %q: %s", cnsconfig.WireserverIP, err.Error())
-			return
-		}
-
-		port, err := strconv.ParseUint(prt, 10, 16) //nolint:gomnd // obvious from ParseUint docs
-		if err != nil {
-			logger.Errorf("[Azure CNS] Invalid port value for Wireserver: %q: %s", cnsconfig.WireserverIP, err.Error())
-			return
-		}
-
-		nmaConfig.Host = host
-		nmaConfig.Port = uint16(port)
-	}
-
-	nmaClient, err := nmagent.NewClient(nmaConfig)
+	nmaClient, err := nmagent.NewClient(nmagent.Config(nmaConfig))
 	if err != nil {
 		logger.Errorf("[Azure CNS] Failed to start nmagent client due to error: %v", err)
 		return