From beeb27810c0f5d6259be625e299f2ab8bfb836de Mon Sep 17 00:00:00 2001 From: gotjosh Date: Thu, 1 Nov 2018 20:17:03 +0000 Subject: [PATCH 1/3] Add tests for probe/cri/registry Unhappy path tests try to cover three scenarios: - When the endpoint URL scheme is not explicitly supported e.g. HTTP - When the endpoint URL scheme is TCP which is also not supported - When the fail to parse the given URL (to extract the scheme) The happy path covers two scenarios: - When we specify the supported scheme in the URL which is an unix socket e.g. unix///var/run/dockershim.sock - When we pass a socket address but fail to specify the scheme but our registry attempts to use the fallback protocol e.g. var/run/dockershim.sock --- probe/cri/registry_test.go | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/probe/cri/registry_test.go b/probe/cri/registry_test.go index 916f06e4fd..2c91a16842 100644 --- a/probe/cri/registry_test.go +++ b/probe/cri/registry_test.go @@ -7,15 +7,36 @@ import ( "github.com/weaveworks/scope/probe/cri" ) -func TestParseHttpEndpointUrl(t *testing.T) { - _, err := cri.NewCRIClient("http://xyz.com") +var nonUnixSocketsTest = []struct { + endpoint string + errorMessage string +}{ + {"http://xyz.com", "protocol \"http\" not supported"}, + {"tcp://var/unix.sock", "endpoint was not unix socket tcp"}, + {"http://[fe80::%31]/", "parse http://[fe80::%31]/: invalid URL escape \"%31\""}, +} + +func TestParseNonUnixEndpointUrl(t *testing.T) { + for _, tt := range nonUnixSocketsTest { + _, err := cri.NewCRIClient(tt.endpoint) - assert.Equal(t, "protocol \"http\" not supported", err.Error()) + assert.Equal(t, tt.errorMessage, err.Error()) + } } -func TestParseTcpEndpointUrl(t *testing.T) { - client, err := cri.NewCRIClient("127.0.0.1") +var unixSocketsTest = []string{ + "127.0.0.1", // tests the fallback endpoint + "unix://127.0.0.1", + "unix///var/run/dockershim.sock", + "var/run/dockershim.sock", +} + +func TestParseUnixEndpointUrl(t *testing.T) { + for _, tt := range unixSocketsTest { + client, err := cri.NewCRIClient(tt) + + assert.Equal(t, nil, err) + assert.NotEqual(t, nil, client) + } - assert.Equal(t, nil, err) - assert.NotEqual(t, nil, client) } From 3faf109b8fc31e6535f580bba127a40e6fb6d5e2 Mon Sep 17 00:00:00 2001 From: gotjosh Date: Thu, 1 Nov 2018 20:21:54 +0000 Subject: [PATCH 2/3] Fix intention of support for endpoints without protocols When we receive an endpoint address without a protocol, our code states we don't support them and that the format is deprecated. In reality it was not the case, e.g: When we received an address in the form of `127.0.0.1`, we'd attempt to parse the scheme from it, we'd realise is does have one (would be equivalent to "") and our function `parseEndpoint` would return `"", "", fmt.Error`. Then, our `parseEndpointWithFallbackProtocol` would use the fallback protocol (unix) and attempt to connect to `unix://127.0.0.1`. This meant two things: 1. The error returned from `parseEndpoint` would never be thrown 2. We would connect anyways since the address is valid This commit changes the assertion logic to match the intention of using a fallback protocol when one is not supplied. --- probe/cri/registry.go | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/probe/cri/registry.go b/probe/cri/registry.go index c346ec59b3..c09cb81b39 100644 --- a/probe/cri/registry.go +++ b/probe/cri/registry.go @@ -18,26 +18,32 @@ func dial(addr string, timeout time.Duration) (net.Conn, error) { } func getAddressAndDialer(endpoint string) (string, func(addr string, timeout time.Duration) (net.Conn, error), error) { - protocol, addr, err := parseEndpointWithFallbackProtocol(endpoint, unixProtocol) + addr, err := parseEndpointWithFallbackProtocol(endpoint, unixProtocol) if err != nil { return "", nil, err } - if protocol != unixProtocol { - return "", nil, fmt.Errorf("endpoint was not unix socket %v", protocol) - } return addr, dial, nil } -func parseEndpointWithFallbackProtocol(endpoint string, fallbackProtocol string) (protocol string, addr string, err error) { - if protocol, addr, err = parseEndpoint(endpoint); err != nil && protocol == "" { +func parseEndpointWithFallbackProtocol(endpoint string, fallbackProtocol string) (addr string, err error) { + var protocol string + + protocol, addr, err = parseEndpoint(endpoint) + + if err != nil { + return "", err + } + + if protocol == "" { fallbackEndpoint := fallbackProtocol + "://" + endpoint - protocol, addr, err = parseEndpoint(fallbackEndpoint) + _, addr, err = parseEndpoint(fallbackEndpoint) + if err != nil { - return "", "", err + return "", err } } - return + return addr, err } func parseEndpoint(endpoint string) (string, string, error) { @@ -47,11 +53,11 @@ func parseEndpoint(endpoint string) (string, string, error) { } if u.Scheme == "tcp" { - return "tcp", u.Host, nil + return "tcp", u.Host, fmt.Errorf("endpoint was not unix socket %v", u.Scheme) } else if u.Scheme == "unix" { return "unix", u.Path, nil } else if u.Scheme == "" { - return "", "", fmt.Errorf("Using %q as endpoint is deprecated, please consider using full url format", endpoint) + return "", "", nil } else { return u.Scheme, "", fmt.Errorf("protocol %q not supported", u.Scheme) } From fbb027752f885769e8fd535758d180f8d6f1c85a Mon Sep 17 00:00:00 2001 From: gotjosh Date: Mon, 15 Oct 2018 21:30:45 +0100 Subject: [PATCH 3/3] Refactor if/else into a switch statement Also, extracts strings into constants --- probe/cri/registry.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/probe/cri/registry.go b/probe/cri/registry.go index c09cb81b39..a069103e24 100644 --- a/probe/cri/registry.go +++ b/probe/cri/registry.go @@ -11,7 +11,10 @@ import ( client "github.com/weaveworks/scope/cri/runtime" ) -const unixProtocol = "unix" +const ( + unixProtocol = "unix" + tcpProtocol = "tcp" +) func dial(addr string, timeout time.Duration) (net.Conn, error) { return net.DialTimeout(unixProtocol, addr, timeout) @@ -48,17 +51,19 @@ func parseEndpointWithFallbackProtocol(endpoint string, fallbackProtocol string) func parseEndpoint(endpoint string) (string, string, error) { u, err := url.Parse(endpoint) + if err != nil { return "", "", err } - if u.Scheme == "tcp" { - return "tcp", u.Host, fmt.Errorf("endpoint was not unix socket %v", u.Scheme) - } else if u.Scheme == "unix" { - return "unix", u.Path, nil - } else if u.Scheme == "" { + switch u.Scheme { + case tcpProtocol: + return tcpProtocol, u.Host, fmt.Errorf("endpoint was not unix socket %v", u.Scheme) + case unixProtocol: + return unixProtocol, u.Path, nil + case "": return "", "", nil - } else { + default: return u.Scheme, "", fmt.Errorf("protocol %q not supported", u.Scheme) } }