From b351745c1c128e8eda4a2e4f456dc2fe6141a3c2 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 Apr 2022 18:16:02 -0400 Subject: [PATCH] Replace use of Sprintf with net.JoinHostPort On IPv6 clusters, one of the most frequent problems I encounter is assumptions that one can build a URL with a host and port simply by using Sprintf, like this: ```go fmt.Sprintf("http://%s:%d/foo", host, port) ``` When `host` is an IPv6 address, this produces an invalid URL as it must be bracketed, like this: ``` http://[2001:4860:4860::8888]:9443 ``` This change fixes the occurences of joining a host and port with the purpose built `net.JoinHostPort` function. I encounter this problem often enough that I started to [write a linter for it](https://github.com/stbenjam/go-sprintf-host-port). I don't think the linter is quite ready for wide use yet, but I did run it against the Kube codebase and found these. While the host portion in some of these changes may always be an FQDN or IPv4 IP today, it's an easy thing that can break later on. --- pkg/volume/portworx/portworx.go | 4 +++- pkg/volume/portworx/portworx_util.go | 5 ++++- test/e2e/storage/vsphere/connection.go | 3 ++- test/e2e/upgrades/apps/cassandra.go | 6 ++++-- test/e2e/upgrades/apps/etcd.go | 6 ++++-- test/e2e/upgrades/apps/mysql.go | 6 ++++-- test/e2e/windows/service.go | 5 ++++- test/e2e_node/util.go | 4 +++- 8 files changed, 28 insertions(+), 11 deletions(-) diff --git a/pkg/volume/portworx/portworx.go b/pkg/volume/portworx/portworx.go index 927c2e471c39..33baef06f229 100644 --- a/pkg/volume/portworx/portworx.go +++ b/pkg/volume/portworx/portworx.go @@ -18,7 +18,9 @@ package portworx import ( "fmt" + "net" "os" + "strconv" "k8s.io/klog/v2" "k8s.io/mount-utils" @@ -71,7 +73,7 @@ func (plugin *portworxVolumePlugin) IsMigratedToCSI() bool { func (plugin *portworxVolumePlugin) Init(host volume.VolumeHost) error { client, err := volumeclient.NewDriverClient( - fmt.Sprintf("http://%s:%d", host.GetHostName(), osdMgmtDefaultPort), + fmt.Sprintf("http://%s", net.JoinHostPort(host.GetHostName(), strconv.Itoa(osdMgmtDefaultPort))), pxdDriverName, osdDriverVersion, pxDriverName) if err != nil { return err diff --git a/pkg/volume/portworx/portworx_util.go b/pkg/volume/portworx/portworx_util.go index 5ff253a74944..58c62e7f1fee 100644 --- a/pkg/volume/portworx/portworx_util.go +++ b/pkg/volume/portworx/portworx_util.go @@ -19,6 +19,8 @@ package portworx import ( "context" "fmt" + "net" + "strconv" osdapi "github.com/libopenstorage/openstorage/api" osdclient "github.com/libopenstorage/openstorage/api/client" @@ -30,6 +32,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" volumehelpers "k8s.io/cloud-provider/volume/helpers" "k8s.io/klog/v2" + api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/volume" ) @@ -265,7 +268,7 @@ func isClientValid(client *osdclient.Client) (bool, error) { } func createDriverClient(hostname string, port int32) (*osdclient.Client, error) { - client, err := volumeclient.NewDriverClient(fmt.Sprintf("http://%s:%d", hostname, port), + client, err := volumeclient.NewDriverClient(fmt.Sprintf("http://%s", net.JoinHostPort(hostname, strconv.Itoa(int(port)))), pxdDriverName, osdDriverVersion, pxDriverName) if err != nil { return nil, err diff --git a/test/e2e/storage/vsphere/connection.go b/test/e2e/storage/vsphere/connection.go index 8257d0dce5ec..cbcf32188d60 100644 --- a/test/e2e/storage/vsphere/connection.go +++ b/test/e2e/storage/vsphere/connection.go @@ -19,6 +19,7 @@ package vsphere import ( "context" "fmt" + "net" neturl "net/url" "sync" @@ -72,7 +73,7 @@ func Connect(ctx context.Context, vs *VSphere) error { // NewClient creates a new client for vSphere connection func NewClient(ctx context.Context, vs *VSphere) (*govmomi.Client, error) { - url, err := neturl.Parse(fmt.Sprintf("https://%s:%s/sdk", vs.Config.Hostname, vs.Config.Port)) + url, err := neturl.Parse(fmt.Sprintf("https://%s/sdk", net.JoinHostPort(vs.Config.Hostname, vs.Config.Port))) if err != nil { klog.Errorf("Failed to parse URL: %s. err: %+v", url, err) return nil, err diff --git a/test/e2e/upgrades/apps/cassandra.go b/test/e2e/upgrades/apps/cassandra.go index cc0e366c010c..144c2827d5eb 100644 --- a/test/e2e/upgrades/apps/cassandra.go +++ b/test/e2e/upgrades/apps/cassandra.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" "path/filepath" "sync" @@ -32,6 +33,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/version" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/kubernetes/test/e2e/framework" e2estatefulset "k8s.io/kubernetes/test/e2e/framework/statefulset" e2etestfiles "k8s.io/kubernetes/test/e2e/framework/testfiles" @@ -118,7 +120,7 @@ func (t *CassandraUpgradeTest) Setup(f *framework.Framework) { // listUsers gets a list of users from the db via the tester service. func (t *CassandraUpgradeTest) listUsers() ([]string, error) { - r, err := http.Get(fmt.Sprintf("http://%s:8080/list", t.ip)) + r, err := http.Get(fmt.Sprintf("http://%s/list", net.JoinHostPort(t.ip, "8080"))) if err != nil { return nil, err } @@ -140,7 +142,7 @@ func (t *CassandraUpgradeTest) listUsers() ([]string, error) { // addUser adds a user to the db via the tester services. func (t *CassandraUpgradeTest) addUser(name string) error { val := map[string][]string{"name": {name}} - r, err := http.PostForm(fmt.Sprintf("http://%s:8080/add", t.ip), val) + r, err := http.PostForm(fmt.Sprintf("http://%s/add", net.JoinHostPort(t.ip, "8080")), val) if err != nil { return err } diff --git a/test/e2e/upgrades/apps/etcd.go b/test/e2e/upgrades/apps/etcd.go index a8f3a43d2a7d..1a95466fb244 100644 --- a/test/e2e/upgrades/apps/etcd.go +++ b/test/e2e/upgrades/apps/etcd.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" "path/filepath" "sync" @@ -32,6 +33,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/version" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/kubernetes/test/e2e/framework" e2estatefulset "k8s.io/kubernetes/test/e2e/framework/statefulset" e2etestfiles "k8s.io/kubernetes/test/e2e/framework/testfiles" @@ -112,7 +114,7 @@ func (t *EtcdUpgradeTest) Setup(f *framework.Framework) { } func (t *EtcdUpgradeTest) listUsers() ([]string, error) { - r, err := http.Get(fmt.Sprintf("http://%s:8080/list", t.ip)) + r, err := http.Get(fmt.Sprintf("http://%s/list", net.JoinHostPort(t.ip, "8080"))) if err != nil { return nil, err } @@ -133,7 +135,7 @@ func (t *EtcdUpgradeTest) listUsers() ([]string, error) { func (t *EtcdUpgradeTest) addUser(name string) error { val := map[string][]string{"name": {name}} - r, err := http.PostForm(fmt.Sprintf("http://%s:8080/add", t.ip), val) + r, err := http.PostForm(fmt.Sprintf("http://%s/add", net.JoinHostPort(t.ip, "8080")), val) if err != nil { return err } diff --git a/test/e2e/upgrades/apps/mysql.go b/test/e2e/upgrades/apps/mysql.go index a4600f393056..1156d086e2ac 100644 --- a/test/e2e/upgrades/apps/mysql.go +++ b/test/e2e/upgrades/apps/mysql.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" "path/filepath" "strconv" @@ -32,6 +33,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/version" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/kubernetes/test/e2e/framework" e2estatefulset "k8s.io/kubernetes/test/e2e/framework/statefulset" e2etestfiles "k8s.io/kubernetes/test/e2e/framework/testfiles" @@ -181,7 +183,7 @@ func (t *MySQLUpgradeTest) Teardown(f *framework.Framework) { func (t *MySQLUpgradeTest) addName(name string) error { val := map[string][]string{"name": {name}} t.nextWrite++ - r, err := http.PostForm(fmt.Sprintf("http://%s:8080/addName", t.ip), val) + r, err := http.PostForm(fmt.Sprintf("http://%s/addName", net.JoinHostPort(t.ip, "8080")), val) if err != nil { return err } @@ -199,7 +201,7 @@ func (t *MySQLUpgradeTest) addName(name string) error { // countNames checks to make sure the values in testing.users are available, and returns // the count of them. func (t *MySQLUpgradeTest) countNames() (int, error) { - r, err := http.Get(fmt.Sprintf("http://%s:8080/countNames", t.ip)) + r, err := http.Get(fmt.Sprintf("http://%s/countNames", net.JoinHostPort(t.ip, "8080"))) if err != nil { return 0, err } diff --git a/test/e2e/windows/service.go b/test/e2e/windows/service.go index 61cb0b95f406..1a5553791383 100644 --- a/test/e2e/windows/service.go +++ b/test/e2e/windows/service.go @@ -18,9 +18,12 @@ package windows import ( "fmt" + "net" + "strconv" v1 "k8s.io/api/core/v1" clientset "k8s.io/client-go/kubernetes" + "k8s.io/kubernetes/test/e2e/framework" e2enode "k8s.io/kubernetes/test/e2e/framework/node" e2eservice "k8s.io/kubernetes/test/e2e/framework/service" @@ -77,7 +80,7 @@ var _ = SIGDescribe("Services", func() { framework.ExpectEqual(testPod.Spec.NodeSelector["kubernetes.io/os"], "windows") ginkgo.By(fmt.Sprintf("checking connectivity Pod to curl http://%s:%d", nodeIP, nodePort)) - assertConsistentConnectivity(f, testPod.ObjectMeta.Name, windowsOS, windowsCheck(fmt.Sprintf("http://%s:%d", nodeIP, nodePort))) + assertConsistentConnectivity(f, testPod.ObjectMeta.Name, windowsOS, windowsCheck(fmt.Sprintf("http://%s", net.JoinHostPort(nodeIP, strconv.Itoa(nodePort))))) }) diff --git a/test/e2e_node/util.go b/test/e2e_node/util.go index f0afa1220505..25185255c94b 100644 --- a/test/e2e_node/util.go +++ b/test/e2e_node/util.go @@ -23,9 +23,11 @@ import ( "flag" "fmt" "io" + "net" "net/http" "os/exec" "regexp" + "strconv" "strings" "time" @@ -83,7 +85,7 @@ func getNodeSummary() (*stats.Summary, error) { if err != nil { return nil, fmt.Errorf("failed to get current kubelet config") } - req, err := http.NewRequest("GET", fmt.Sprintf("http://%s:%d/stats/summary", kubeletConfig.Address, kubeletConfig.ReadOnlyPort), nil) + req, err := http.NewRequest("GET", fmt.Sprintf("http://%s/stats/summary", net.JoinHostPort(kubeletConfig.Address, strconv.Itoa(int(kubeletConfig.ReadOnlyPort)))), nil) if err != nil { return nil, fmt.Errorf("failed to build http request: %v", err) }