Refactor GetNodeClient weirdness

- No need to for a pointer to a pointer
- Properly set and use QueryOptions.Region

api/allocations.go

@@ -48,7 +48,7 @@ func (a *Allocations) Info(allocID string, q *QueryOptions) (*Allocation, *Query
 }
 
 func (a *Allocations) Stats(alloc *Allocation, q *QueryOptions) (*AllocResourceUsage, error) {
-	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q)
+	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, q)
 	if err != nil {
 		return nil, err
 	}
@@ -59,7 +59,7 @@ func (a *Allocations) Stats(alloc *Allocation, q *QueryOptions) (*AllocResourceU
 }
 
 func (a *Allocations) GC(alloc *Allocation, q *QueryOptions) error {
-	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q)
+	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, q)
 	if err != nil {
 		return err
 	}

api/api.go

@@ -111,19 +111,20 @@ type Config struct {
 }
 
 // CopyConfig copies the configuration with a new address
-func (c *Config) CopyConfig(address string, tlsEnabled bool) *Config {
+func (c *Config) CopyConfig(region, address string, tlsEnabled bool) *Config {
 	scheme := "http"
 	if tlsEnabled {
 		scheme = "https"
 	}
 	config := &Config{
 		Address:    fmt.Sprintf("%s://%s", scheme, address),
-		Region:     c.Region,
+		Region:     region,
 		HttpClient: c.HttpClient,
 		HttpAuth:   c.HttpAuth,
 		WaitTime:   c.WaitTime,
-		TLSConfig:  c.TLSConfig,
+		TLSConfig:  c.TLSConfig.Copy(),
 	}
+	config.TLSConfig.TLSServerName = fmt.Sprintf("client.%s.nomad", c.Region)
 
 	return config
 }
@@ -153,6 +154,16 @@ type TLSConfig struct {
 	Insecure bool
 }
 
+func (t *TLSConfig) Copy() *TLSConfig {
+	if t == nil {
+		return nil
+	}
+
+	nt := new(TLSConfig)
+	*nt = *t
+	return nt
+}
+
 // DefaultConfig returns a default configuration for the client
 func DefaultConfig() *Config {
 	config := &Config{
@@ -286,10 +297,28 @@ func (c *Client) SetRegion(region string) {
 }
 
 // GetNodeClient returns a new Client that will dial the specified node. If the
-// QueryOptions is set, the function will ensure that it is initialized and
+// QueryOptions is set, its region will be used.
-// that the Params field is valid.
+func (c *Client) GetNodeClient(nodeID string, q *QueryOptions) (*Client, error) {
-func (c *Client) GetNodeClient(nodeID string, q **QueryOptions) (*Client, error) {
+	if q == nil {
-	node, _, err := c.Nodes().Info(nodeID, &QueryOptions{})
+		q = &QueryOptions{}
+	}
+
+	// If region is unset, set from config or agent if available
+	if q.Region == "" {
+		if c.config.Region != "" {
+			// Use the region from the client
+			q.Region = c.config.Region
+		} else {
+			// Use the region from the agent
+			agentRegion, err := c.Agent().Region()
+			if err != nil {
+				return nil, err
+			}
+			q.Region = agentRegion
+		}
+	}
+
+	node, _, err := c.Nodes().Info(nodeID, q)
 	if err != nil {
 		return nil, err
 	}
@@ -300,41 +329,9 @@ func (c *Client) GetNodeClient(nodeID string, q **QueryOptions) (*Client, error)
 		return nil, fmt.Errorf("http addr of node %q (%s) is not advertised", node.Name, nodeID)
 	}
 
-	region := ""
-	if q != nil && *q != nil && (*q).Region != "" {
-		region = (*q).Region
-	} else if c.config.Region != "" {
-		// Use the region from the client
-		region = c.config.Region
-	} else {
-		// Use the region from the agent
-		agentRegion, err := c.Agent().Region()
-		if err != nil {
-			return nil, err
-		}
-		region = agentRegion
-	}
-
 	// Get an API client for the node
-	conf := c.config.CopyConfig(node.HTTPAddr, node.TLSEnabled)
+	conf := c.config.CopyConfig(q.Region, node.HTTPAddr, node.TLSEnabled)
-	conf.TLSConfig.TLSServerName = fmt.Sprintf("client.%s.nomad", region)
+	return NewClient(conf)
-	nodeClient, err := NewClient(conf)
-	if err != nil {
-		return nil, err
-	}
-
-	// Set the query params
-	if q == nil {
-		return nodeClient, nil
-	}
-
-	if *q == nil {
-		*q = &QueryOptions{}
-	}
-	if actQ := *q; actQ.Params == nil {
-		actQ.Params = make(map[string]string)
-	}
-	return nodeClient, nil
 }
 
 // request is used to help build up a request

api/fs.go

@@ -51,7 +51,7 @@ func (c *Client) AllocFS() *AllocFS {
 
 // List is used to list the files at a given path of an allocation directory
 func (a *AllocFS) List(alloc *Allocation, path string, q *QueryOptions) ([]*AllocFileInfo, *QueryMeta, error) {
-	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q)
+	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, q)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -68,7 +68,7 @@ func (a *AllocFS) List(alloc *Allocation, path string, q *QueryOptions) ([]*Allo
 
 // Stat is used to stat a file at a given path of an allocation directory
 func (a *AllocFS) Stat(alloc *Allocation, path string, q *QueryOptions) (*AllocFileInfo, *QueryMeta, error) {
-	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q)
+	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, q)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -85,7 +85,7 @@ func (a *AllocFS) Stat(alloc *Allocation, path string, q *QueryOptions) (*AllocF
 // ReadAt is used to read bytes at a given offset until limit at the given path
 // in an allocation directory. If limit is <= 0, there is no limit.
 func (a *AllocFS) ReadAt(alloc *Allocation, path string, offset int64, limit int64, q *QueryOptions) (io.ReadCloser, error) {
-	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q)
+	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, q)
 	if err != nil {
 		return nil, err
 	}
@@ -103,7 +103,7 @@ func (a *AllocFS) ReadAt(alloc *Allocation, path string, offset int64, limit int
 // Cat is used to read contents of a file at the given path in an allocation
 // directory
 func (a *AllocFS) Cat(alloc *Allocation, path string, q *QueryOptions) (io.ReadCloser, error) {
-	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q)
+	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, q)
 	if err != nil {
 		return nil, err
 	}
@@ -127,7 +127,7 @@ func (a *AllocFS) Cat(alloc *Allocation, path string, q *QueryOptions) (io.ReadC
 func (a *AllocFS) Stream(alloc *Allocation, path, origin string, offset int64,
 	cancel <-chan struct{}, q *QueryOptions) (<-chan *StreamFrame, error) {
 
-	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q)
+	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, q)
 	if err != nil {
 		return nil, err
 	}
@@ -191,10 +191,13 @@ func (a *AllocFS) Stream(alloc *Allocation, path, origin string, offset int64,
 func (a *AllocFS) Logs(alloc *Allocation, follow bool, task, logType, origin string,
 	offset int64, cancel <-chan struct{}, q *QueryOptions) (<-chan *StreamFrame, error) {
 
-	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q)
+	nodeClient, err := a.client.GetNodeClient(alloc.NodeID, q)
 	if err != nil {
 		return nil, err
 	}
+	if q == nil {
+		q = &QueryOptions{}
+	}
 	q.Params["follow"] = strconv.FormatBool(follow)
 	q.Params["task"] = task
 	q.Params["type"] = logType

api/nodes.go

@@ -1,7 +1,6 @@
 package api
 
 import (
-	"fmt"
 	"sort"
 	"strconv"
 )
@@ -73,26 +72,19 @@ func (n *Nodes) ForceEvaluate(nodeID string, q *WriteOptions) (string, *WriteMet
 }
 
 func (n *Nodes) Stats(nodeID string, q *QueryOptions) (*HostStats, error) {
-	node, _, err := n.client.Nodes().Info(nodeID, q)
+	nodeClient, err := n.client.GetNodeClient(nodeID, q)
-	if err != nil {
-		return nil, err
-	}
-	if node.HTTPAddr == "" {
-		return nil, fmt.Errorf("http addr of the node %q is running is not advertised", nodeID)
-	}
-	client, err := NewClient(n.client.config.CopyConfig(node.HTTPAddr, node.TLSEnabled))
 	if err != nil {
 		return nil, err
 	}
 	var resp HostStats
-	if _, err := client.query("/v1/client/stats", &resp, nil); err != nil {
+	if _, err := nodeClient.query("/v1/client/stats", &resp, nil); err != nil {
 		return nil, err
 	}
 	return &resp, nil
 }
 
 func (n *Nodes) GC(nodeID string, q *QueryOptions) error {
-	nodeClient, err := n.client.GetNodeClient(nodeID, &q)
+	nodeClient, err := n.client.GetNodeClient(nodeID, q)
 	if err != nil {
 		return err
 	}