Repository: grafana/dskit (go)

Issue:
replace fmt.Sprintf("%s:%d") with net.JoinHostPort
Several places in the codebase use fmt.Sprintf to join a host and a port, for example:

```
httpgrpc/server/server.go:118:		address := fmt.Sprintf("kubernetes:///%s%s:%s", service, domain, port)
crypto/tls/test/tls_integration_test.go:109:	httpURL := fmt.Sprintf("https://localhost:%d/hello", httpPort)
crypto/tls/test/tls_integration_test.go:110:	grpcHost := fmt.Sprintf("localhost:%d", grpcPort)
kv/memberlist/memberlist_client_test.go:853:					cfg.JoinMembers = []string{fmt.Sprintf("%slocalhost:%d", testData.queryType, ports[1])}
kv/memberlist/memberlist_client_test.go:933:	cfg.JoinMembers = []string{fmt.Sprintf("127.0.0.1:%d", ports[0])}
kv/memberlist/memberlist_client_test.go:999:				_, err := kv.kv.JoinMembers([]string{fmt.Sprintf("127.0.0.1:%d", portToConnect)})
kv/memberlist/memberlist_client_test.go:1158:	_, err = mkv2.JoinMembers([]string{fmt.Sprintf("127.0.0.1:%d", mkv1.GetListeningPort())})
kv/memberlist/memberlist_client_test.go:1202:	cfg2.JoinMembers = []string{fmt.Sprintf("localhost:%d", ports[0])}
kv/memberlist/memberlist_client_test.go:1433:	cfg.JoinMembers = []string{fmt.Sprintf("127.0.0.1:%d", mkv1.GetListeningPort())}
server/server_tracing_test.go:92:	helloRouteURLRaw := fmt.Sprintf("http://%s:%d/hello", httpAddress, httpPort)
server/server_tracing_test.go:97:	helloPathParamRouteURLRaw := fmt.Sprintf("http://%s:%d/hello/world", httpAddress, httpPort)
server/server.go:251:	httpListener, err := net.Listen(network, fmt.Sprintf("%s:%d", cfg.HTTPListenAddress, cfg.HTTPListenPort))
server/server.go:275:	grpcListener, err := net.Listen(network, fmt.Sprintf("%s:%d", cfg.GRPCListenAddress, cfg.GRPCListenPort))
ring/lifecycler_test.go:515:	assert.Equal(t, fmt.Sprintf("%s:%d", lifecyclerCfg.Addr, lifecyclerCfg.Port), desc.GetAddr())
```

Because fmt.Sprintf is a general purpose parser, it will be slower than invoking the fit-for-purpose net.JoinHostPort. In addition, net.JoinHostPort correctly handles ipv6 addresses, which can include colons. 

I'd be happy to replace all of these just wanted to point this out. 

Pull request:
all: use net.JoinHostPort instead of fmt.Sprintf
This will ensure ipv6 compatibility for provided hosts, and this method should also be faster than the general purpose fmt.Sprintf, which must first parse the input string.

Fixes #380.


**Checklist**
- [x] Tests updated
- [x] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3f43aae..f359ca5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -171,3 +171,4 @@
 * [BUGFIX] `MetricFamiliesPerTenant.SendMinOfGauges` will no longer return 0 if some tenant doesn't have the gauge in their registry, and other tenants have positive values only. #368
 * [BUGFIX] `MetricFamiliesPerTenant.SendMaxOfGauges` will no longer return 0 if some tenant doesn't have the gauge in their registry, and other tenants have negative values only. #368
 * [BUGFIX] `MetricFamiliesPerTenant.SendMaxOfGaugesPerTenant` no longer includes tenants, which do not have specified gauge. #368
+* [BUGFIX] Correctly format `host:port` addresses when using IPv6. #388
diff --git a/httpgrpc/server/server.go b/httpgrpc/server/server.go
index a92761a..b0d808b 100644
--- a/httpgrpc/server/server.go
+++ b/httpgrpc/server/server.go
@@ -80,7 +80,7 @@ type Client struct {
 }
 
 // ParseURL deals with direct:// style URLs, as well as kubernetes:// urls.
-// For backwards compatibility it treats URLs without schems as kubernetes://.
+// For backwards compatibility it treats URLs without schemes as kubernetes://.
 func ParseURL(unparsed string) (string, error) {
 	// if it has :///, this is the kuberesolver v2 URL. Return it as it is.
 	if strings.Contains(unparsed, ":///") {
@@ -115,7 +115,7 @@ func ParseURL(unparsed string) (string, error) {
 		if len(parts) > 2 {
 			domain = domain + "." + parts[2]
 		}
-		address := fmt.Sprintf("kubernetes:///%s%s:%s", service, domain, port)
+		address := fmt.Sprintf("kubernetes:///%s", net.JoinHostPort(service+domain, port))
 		return address, nil
 
 	default:
diff --git a/ring/example/local/local.go b/ring/example/local/local.go
index 7999304..4a00891 100644
--- a/ring/example/local/local.go
+++ b/ring/example/local/local.go
@@ -7,6 +7,7 @@ import (
 	"net"
 	"net/http"
 	"os"
+	"strconv"
 	"time"
 
 	"github.com/go-kit/log"
@@ -87,7 +88,7 @@ func runCluster() {
 	}
 	defer services.StopAndAwaitTerminated(ctx, lfc)
 
-	listener, err := net.Listen("tcp", bindaddr+":8100")
+	listener, err := net.Listen("tcp", net.JoinHostPort(bindaddr, "8100"))
 	if err != nil {
 		panic(err)
 	}
@@ -211,7 +212,7 @@ func SimpleRing(store kv.Client) (*ring.Ring, error) {
 func SimpleRingLifecycler(store kv.Client, bindaddr string, bindport int) (*ring.BasicLifecycler, error) {
 	var config ring.BasicLifecyclerConfig
 	config.ID = bindaddr
-	config.Addr = fmt.Sprintf("%s:%d", bindaddr, bindport)
+	config.Addr = net.JoinHostPort(bindaddr, strconv.Itoa(bindport))
 
 	var delegate ring.BasicLifecyclerDelegate
 
diff --git a/server/server.go b/server/server.go
index eb238d4..25f08e8 100644
--- a/server/server.go
+++ b/server/server.go
@@ -13,6 +13,7 @@ import (
 	"net"
 	"net/http"
 	_ "net/http/pprof" // anonymous import to get the pprof handler registered
+	"strconv"
 	"strings"
 	"time"
 
@@ -251,7 +252,7 @@ func newServer(cfg Config, metrics *Metrics) (*Server, error) {
 		network = DefaultNetwork
 	}
 	// Setup listeners first, so we can fail early if the port is in use.
-	httpListener, err := net.Listen(network, fmt.Sprintf("%s:%d", cfg.HTTPListenAddress, cfg.HTTPListenPort))
+	httpListener, err := net.Listen(network, net.JoinHostPort(cfg.HTTPListenAddress, strconv.Itoa(cfg.HTTPListenPort)))
 	if err != nil {
 		return nil, err
 	}
@@ -275,7 +276,7 @@ func newServer(cfg Config, metrics *Metrics) (*Server, error) {
 	if network == "" {
 		network = DefaultNetwork
 	}
-	grpcListener, err := net.Listen(network, fmt.Sprintf("%s:%d", cfg.GRPCListenAddress, cfg.GRPCListenPort))
+	grpcListener, err := net.Listen(network, net.JoinHostPort(cfg.GRPCListenAddress, strconv.Itoa(cfg.GRPCListenPort)))
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/crypto/tls/test/tls_integration_test.go b/crypto/tls/test/tls_integration_test.go
index bcd89df..e1f6b5f 100644
--- a/crypto/tls/test/tls_integration_test.go
+++ b/crypto/tls/test/tls_integration_test.go
@@ -12,6 +12,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"strconv"
 	"testing"
 	"time"
 
@@ -107,7 +108,7 @@ func newIntegrationClientServer(
 	}()
 
 	httpURL := fmt.Sprintf("https://localhost:%d/hello", httpPort)
-	grpcHost := fmt.Sprintf("localhost:%d", grpcPort)
+	grpcHost := net.JoinHostPort("localhost", strconv.Itoa(grpcPort))
 
 	for _, tc := range tcs {
 		tlsClientConfig, err := tc.tlsConfig.GetTLSConfig()
diff --git a/kv/memberlist/memberlist_client_test.go b/kv/memberlist/memberlist_client_test.go
index a48b86c..500c397 100644
--- a/kv/memberlist/memberlist_client_test.go
+++ b/kv/memberlist/memberlist_client_test.go
@@ -11,6 +11,7 @@ import (
 	"net"
 	"reflect"
 	"sort"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -930,7 +931,7 @@ func TestMemberlistFailsToJoin(t *testing.T) {
 		BindPort:  0,
 	}
 
-	cfg.JoinMembers = []string{fmt.Sprintf("127.0.0.1:%d", ports[0])}
+	cfg.JoinMembers = []string{net.JoinHostPort("127.0.0.1", strconv.Itoa(ports[0]))}
 
 	cfg.Codecs = []codec.Codec{c}
 
@@ -996,7 +997,7 @@ func runClient(kv *Client, name string, ringKey string, portToConnect int, casIn
 
 			// let's join the first member
 			if portToConnect > 0 {
-				_, err := kv.kv.JoinMembers([]string{fmt.Sprintf("127.0.0.1:%d", portToConnect)})
+				_, err := kv.kv.JoinMembers([]string{net.JoinHostPort("127.0.0.1", strconv.Itoa(portToConnect))})
 				if err != nil {
 					return fmt.Errorf("%s failed to join the cluster: %f", name, err)
 				}
@@ -1155,7 +1156,7 @@ func TestMultipleCodecs(t *testing.T) {
 	defer services.StopAndAwaitTerminated(context.Background(), mkv2) //nolint:errcheck
 
 	// Join second KV to first one. That will also trigger state transfer.
-	_, err = mkv2.JoinMembers([]string{fmt.Sprintf("127.0.0.1:%d", mkv1.GetListeningPort())})
+	_, err = mkv2.JoinMembers([]string{net.JoinHostPort("127.0.0.1", strconv.Itoa(mkv1.GetListeningPort()))})
 	require.NoError(t, err)
 
 	// Now read both values from second KV. It should have both values by now.
@@ -1199,7 +1200,7 @@ func TestRejoin(t *testing.T) {
 
 	cfg2 := cfg1
 	cfg2.TCPTransport.BindPort = ports[1]
-	cfg2.JoinMembers = []string{fmt.Sprintf("localhost:%d", ports[0])}
+	cfg2.JoinMembers = []string{net.JoinHostPort("localhost", strconv.Itoa(ports[0]))}
 	cfg2.RejoinInterval = 1 * time.Second
 
 	mkv1 := NewKV(cfg1, log.NewNopLogger(), &dnsProviderMock{}, prometheus.NewPedanticRegistry())
@@ -1430,7 +1431,7 @@ func TestFastJoin(t *testing.T) {
 	require.NoError(t, err)
 
 	// We will read values from second KV, which will join the first one
-	cfg.JoinMembers = []string{fmt.Sprintf("127.0.0.1:%d", mkv1.GetListeningPort())}
+	cfg.JoinMembers = []string{net.JoinHostPort("127.0.0.1", strconv.Itoa(mkv1.GetListeningPort()))}
 
 	mkv2 := NewKV(cfg, log.NewNopLogger(), &dnsProviderMock{}, prometheus.NewPedanticRegistry())
 	go func() {
diff --git a/ring/lifecycler_test.go b/ring/lifecycler_test.go
index a50e621..0d53ac3 100644
--- a/ring/lifecycler_test.go
+++ b/ring/lifecycler_test.go
@@ -3,8 +3,10 @@ package ring
 import (
 	"context"
 	"fmt"
+	"net"
 	"os"
 	"sort"
+	"strconv"
 	"sync"
 	"testing"
 	"time"
@@ -513,7 +515,7 @@ func TestLifecycler_HeartbeatAfterBackendReset(t *testing.T) {
 	assert.Greater(t, desc.GetTimestamp(), int64(0))
 	assert.Equal(t, testInstanceID, desc.GetId())
 	assert.Equal(t, ACTIVE, desc.GetState())
-	assert.Equal(t, fmt.Sprintf("%s:%d", lifecyclerCfg.Addr, lifecyclerCfg.Port), desc.GetAddr())
+	assert.Equal(t, net.JoinHostPort(lifecyclerCfg.Addr, strconv.Itoa(lifecyclerCfg.Port)), desc.GetAddr())
 	assert.Equal(t, lifecyclerCfg.Zone, desc.Zone)
 	assert.Equal(t, prevTokens, Tokens(desc.GetTokens()))
 }
diff --git a/server/server_tracing_test.go b/server/server_tracing_test.go
index 2a8e204..b3a1ec9 100644
--- a/server/server_tracing_test.go
+++ b/server/server_tracing_test.go
@@ -3,8 +3,10 @@ package server
 import (
 	"bytes"
 	"fmt"
+	"net"
 	"net/http"
 	"net/url"
+	"strconv"
 	"testing"
 
 	"github.com/gorilla/mux"
@@ -89,12 +91,13 @@ func TestHTTPGRPCTracing(t *testing.T) {
 
 	helloRouteName := "hello"
 	helloRouteTmpl := "/hello"
-	helloRouteURLRaw := fmt.Sprintf("http://%s:%d/hello", httpAddress, httpPort)
+	hostport := net.JoinHostPort(httpAddress, strconv.Itoa(httpPort))
+	helloRouteURLRaw := fmt.Sprintf("http://%s/hello", hostport)
 	helloRouteURL, err := url.Parse(helloRouteURLRaw)
 	require.NoError(t, err)
 
 	helloPathParamRouteTmpl := "/hello/{pathParam}"
-	helloPathParamRouteURLRaw := fmt.Sprintf("http://%s:%d/hello/world", httpAddress, httpPort)
+	helloPathParamRouteURLRaw := fmt.Sprintf("http://%s/hello/world", hostport)
 	helloPathParamRouteURL, err := url.Parse(helloPathParamRouteURLRaw)
 	require.NoError(t, err)