From 2b7e0914241af0a9f7e92900fda069367ddd2e69 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 20 Feb 2017 11:12:34 -0800 Subject: [PATCH] Fix API panic and bad missing port check The format of the missing port error message changed from Go 1.7 to 1.8. The fix is to just use strings.Contains instead of strings.HasPrefix when looking for the "missing port" part. Also add an error return to Client.newRequest as parsing the path processes arbitrary user input and would panic if given an invalid URL. See: https://groups.google.com/d/topic/nomad-tool/gi3-CTE7oXo/discussion --- api/api.go | 31 +++++++++++++++++++++++-------- api/api_test.go | 8 ++++---- api/operator.go | 10 ++++++++-- command/agent/config.go | 2 +- 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/api/api.go b/api/api.go index 5eec8f5be..d2309c32f 100644 --- a/api/api.go +++ b/api/api.go @@ -374,9 +374,12 @@ func (r *request) toHTTP() (*http.Request, error) { } // newRequest is used to create a new request -func (c *Client) newRequest(method, path string) *request { +func (c *Client) newRequest(method, path string) (*request, error) { base, _ := url.Parse(c.config.Address) - u, _ := url.Parse(path) + u, err := url.Parse(path) + if err != nil { + return nil, err + } r := &request{ config: &c.config, method: method, @@ -402,7 +405,7 @@ func (c *Client) newRequest(method, path string) *request { } } - return r + return r, nil } // multiCloser is to wrap a ReadCloser such that when close is called, multiple @@ -463,7 +466,10 @@ func (c *Client) doRequest(r *request) (time.Duration, *http.Response, error) { // rawQuery makes a GET request to the specified endpoint but returns just the // response body. func (c *Client) rawQuery(endpoint string, q *QueryOptions) (io.ReadCloser, error) { - r := c.newRequest("GET", endpoint) + r, err := c.newRequest("GET", endpoint) + if err != nil { + return nil, err + } r.setQueryOptions(q) _, resp, err := requireOK(c.doRequest(r)) if err != nil { @@ -477,7 +483,10 @@ func (c *Client) rawQuery(endpoint string, q *QueryOptions) (io.ReadCloser, erro // and deserialize the response into an interface using // standard Nomad conventions. func (c *Client) query(endpoint string, out interface{}, q *QueryOptions) (*QueryMeta, error) { - r := c.newRequest("GET", endpoint) + r, err := c.newRequest("GET", endpoint) + if err != nil { + return nil, err + } r.setQueryOptions(q) rtt, resp, err := requireOK(c.doRequest(r)) if err != nil { @@ -498,7 +507,10 @@ func (c *Client) query(endpoint string, out interface{}, q *QueryOptions) (*Quer // write is used to do a PUT request against an endpoint // and serialize/deserialized using the standard Nomad conventions. func (c *Client) write(endpoint string, in, out interface{}, q *WriteOptions) (*WriteMeta, error) { - r := c.newRequest("PUT", endpoint) + r, err := c.newRequest("PUT", endpoint) + if err != nil { + return nil, err + } r.setWriteOptions(q) r.obj = in rtt, resp, err := requireOK(c.doRequest(r)) @@ -518,10 +530,13 @@ func (c *Client) write(endpoint string, in, out interface{}, q *WriteOptions) (* return wm, nil } -// write is used to do a PUT request against an endpoint +// delete is used to do a DELETE request against an endpoint // and serialize/deserialized using the standard Nomad conventions. func (c *Client) delete(endpoint string, out interface{}, q *WriteOptions) (*WriteMeta, error) { - r := c.newRequest("DELETE", endpoint) + r, err := c.newRequest("DELETE", endpoint) + if err != nil { + return nil, err + } r.setWriteOptions(q) rtt, resp, err := requireOK(c.doRequest(r)) if err != nil { diff --git a/api/api_test.go b/api/api_test.go index 1caf9db03..5c6ea9e90 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -118,7 +118,7 @@ func TestSetQueryOptions(t *testing.T) { c, s := makeClient(t, nil, nil) defer s.Stop() - r := c.newRequest("GET", "/v1/jobs") + r, _ := c.newRequest("GET", "/v1/jobs") q := &QueryOptions{ Region: "foo", AllowStale: true, @@ -145,7 +145,7 @@ func TestSetWriteOptions(t *testing.T) { c, s := makeClient(t, nil, nil) defer s.Stop() - r := c.newRequest("GET", "/v1/jobs") + r, _ := c.newRequest("GET", "/v1/jobs") q := &WriteOptions{ Region: "foo", } @@ -160,7 +160,7 @@ func TestRequestToHTTP(t *testing.T) { c, s := makeClient(t, nil, nil) defer s.Stop() - r := c.newRequest("DELETE", "/v1/jobs/foo") + r, _ := c.newRequest("DELETE", "/v1/jobs/foo") q := &QueryOptions{ Region: "foo", } @@ -222,7 +222,7 @@ func TestQueryString(t *testing.T) { c, s := makeClient(t, nil, nil) defer s.Stop() - r := c.newRequest("PUT", "/v1/abc?foo=bar&baz=zip") + r, _ := c.newRequest("PUT", "/v1/abc?foo=bar&baz=zip") q := &WriteOptions{Region: "foo"} r.setWriteOptions(q) diff --git a/api/operator.go b/api/operator.go index dcf65b332..a10648a29 100644 --- a/api/operator.go +++ b/api/operator.go @@ -45,7 +45,10 @@ type RaftConfiguration struct { // RaftGetConfiguration is used to query the current Raft peer set. func (op *Operator) RaftGetConfiguration(q *QueryOptions) (*RaftConfiguration, error) { - r := op.c.newRequest("GET", "/v1/operator/raft/configuration") + r, err := op.c.newRequest("GET", "/v1/operator/raft/configuration") + if err != nil { + return nil, err + } r.setQueryOptions(q) _, resp, err := requireOK(op.c.doRequest(r)) if err != nil { @@ -64,7 +67,10 @@ func (op *Operator) RaftGetConfiguration(q *QueryOptions) (*RaftConfiguration, e // quorum but no longer known to Serf or the catalog) by address in the form of // "IP:port". func (op *Operator) RaftRemovePeerByAddress(address string, q *WriteOptions) error { - r := op.c.newRequest("DELETE", "/v1/operator/raft/peer") + r, err := op.c.newRequest("DELETE", "/v1/operator/raft/peer") + if err != nil { + return err + } r.setWriteOptions(q) // TODO (alexdadgar) Currently we made address a query parameter. Once diff --git a/command/agent/config.go b/command/agent/config.go index dc90c740f..9efac4b4d 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -809,7 +809,7 @@ func normalizeAdvertise(addr string, bind string, defport int, dev bool) (string func isMissingPort(err error) bool { // matches error const in net/ipsock.go const missingPort = "missing port in address" - return err != nil && strings.HasPrefix(err.Error(), missingPort) + return err != nil && strings.Contains(err.Error(), missingPort) } // Merge is used to merge two server configs together