From 1bb8a54ffb7b80da0644a95bf7dc39bff317d1f2 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 28 Aug 2017 11:32:52 -0700 Subject: [PATCH 1/3] Fix TLS support in api pkg / cli Fixes #3013 It's a little weird that Client now has a method for returning a NewClient, but it's a convenient way to dedupe the logic to connect-directly-to-a-node which is nontrivial and had sutble differences between locations. --- api/allocations.go | 29 +++------------- api/api.go | 52 ++++++++++++++++++++++++++++ api/fs.go | 85 ++++------------------------------------------ api/nodes.go | 12 ++----- 4 files changed, 66 insertions(+), 112 deletions(-) diff --git a/api/allocations.go b/api/allocations.go index 203fdc0646f1..42ac087f6bd9 100644 --- a/api/allocations.go +++ b/api/allocations.go @@ -48,43 +48,24 @@ func (a *Allocations) Info(allocID string, q *QueryOptions) (*Allocation, *Query } func (a *Allocations) Stats(alloc *Allocation, q *QueryOptions) (*AllocResourceUsage, error) { - node, _, err := a.client.Nodes().Info(alloc.NodeID, q) - if err != nil { - return nil, err - } - if node.Status == "down" { - return nil, NodeDownErr - } - if node.HTTPAddr == "" { - return nil, fmt.Errorf("http addr of the node where alloc %q is running is not advertised", alloc.ID) - } - client, err := NewClient(a.client.config.CopyConfig(node.HTTPAddr, node.TLSEnabled)) + nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q) if err != nil { return nil, err } + var resp AllocResourceUsage - _, err = client.query("/v1/client/allocation/"+alloc.ID+"/stats", &resp, nil) + _, err = nodeClient.query("/v1/client/allocation/"+alloc.ID+"/stats", &resp, nil) return &resp, err } func (a *Allocations) GC(alloc *Allocation, q *QueryOptions) error { - node, _, err := a.client.Nodes().Info(alloc.NodeID, q) - if err != nil { - return err - } - if node.Status == "down" { - return NodeDownErr - } - if node.HTTPAddr == "" { - return fmt.Errorf("http addr of the node where alloc %q is running is not advertised", alloc.ID) - } - client, err := NewClient(a.client.config.CopyConfig(node.HTTPAddr, node.TLSEnabled)) + nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q) if err != nil { return err } var resp struct{} - _, err = client.query("/v1/client/allocation"+alloc.ID+"/gc", &resp, nil) + _, err = nodeClient.query("/v1/client/allocation/"+alloc.ID+"/gc", &resp, nil) return err } diff --git a/api/api.go b/api/api.go index 582dd6667ca4..d5e56a07dd43 100644 --- a/api/api.go +++ b/api/api.go @@ -285,6 +285,58 @@ func (c *Client) SetRegion(region string) { c.config.Region = region } +// 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 +// that the Params field is valid. +func (c *Client) GetNodeClient(nodeID string, q **QueryOptions) (*Client, error) { + node, _, err := c.Nodes().Info(nodeID, &QueryOptions{}) + if err != nil { + return nil, err + } + if node.Status == "down" { + return nil, NodeDownErr + } + if node.HTTPAddr == "" { + 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.TLSConfig.TLSServerName = fmt.Sprintf("client.%s.nomad", region) + 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 type request struct { config *Config diff --git a/api/fs.go b/api/fs.go index 73ae71c97b2e..efe65650ee71 100644 --- a/api/fs.go +++ b/api/fs.go @@ -49,58 +49,9 @@ func (c *Client) AllocFS() *AllocFS { return &AllocFS{client: c} } -// getNodeClient returns a Client that will dial the node. If the QueryOptions -// is set, the function will ensure that it is initialized and that the Params -// field is valid. -func (a *AllocFS) getNodeClient(node *Node, allocID string, q **QueryOptions) (*Client, error) { - if node.HTTPAddr == "" { - return nil, fmt.Errorf("http addr of the node where alloc %q is running is not advertised", allocID) - } - - region := "" - if q != nil && *q != nil && (*q).Region != "" { - region = (*q).Region - } else if a.client.config.Region != "" { - // Use the region from the client - region = a.client.config.Region - } else { - // Use the region from the agent - agentRegion, err := a.client.Agent().Region() - if err != nil { - return nil, err - } - region = agentRegion - } - - // Get an API client for the node - conf := a.client.config.CopyConfig(node.HTTPAddr, node.TLSEnabled) - conf.TLSConfig.TLSServerName = fmt.Sprintf("client.%s.nomad", region) - 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 -} - // 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) { - node, _, err := a.client.Nodes().Info(alloc.NodeID, &QueryOptions{}) - if err != nil { - return nil, nil, err - } - nodeClient, err := a.getNodeClient(node, alloc.ID, &q) + nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q) if err != nil { return nil, nil, err } @@ -117,11 +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) { - node, _, err := a.client.Nodes().Info(alloc.NodeID, &QueryOptions{}) - if err != nil { - return nil, nil, err - } - nodeClient, err := a.getNodeClient(node, alloc.ID, &q) + nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q) if err != nil { return nil, nil, err } @@ -138,12 +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) { - node, _, err := a.client.Nodes().Info(alloc.NodeID, &QueryOptions{}) - if err != nil { - return nil, err - } - - nodeClient, err := a.getNodeClient(node, alloc.ID, &q) + nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q) if err != nil { return nil, err } @@ -161,12 +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) { - node, _, err := a.client.Nodes().Info(alloc.NodeID, &QueryOptions{}) - if err != nil { - return nil, err - } - - nodeClient, err := a.getNodeClient(node, alloc.ID, &q) + nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q) if err != nil { return nil, err } @@ -190,12 +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) { - node, _, err := a.client.Nodes().Info(alloc.NodeID, q) - if err != nil { - return nil, err - } - - nodeClient, err := a.getNodeClient(node, alloc.ID, &q) + nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q) if err != nil { return nil, err } @@ -259,12 +191,7 @@ 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) { - node, _, err := a.client.Nodes().Info(alloc.NodeID, q) - if err != nil { - return nil, err - } - - nodeClient, err := a.getNodeClient(node, alloc.ID, &q) + nodeClient, err := a.client.GetNodeClient(alloc.NodeID, &q) if err != nil { return nil, err } diff --git a/api/nodes.go b/api/nodes.go index 8ae8cd6dc384..b1bdb5054acd 100644 --- a/api/nodes.go +++ b/api/nodes.go @@ -92,19 +92,13 @@ func (n *Nodes) Stats(nodeID string, q *QueryOptions) (*HostStats, error) { } func (n *Nodes) GC(nodeID string, q *QueryOptions) error { - node, _, err := n.client.Nodes().Info(nodeID, q) - if err != nil { - return err - } - if node.HTTPAddr == "" { - return 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)) + nodeClient, err := n.client.GetNodeClient(nodeID, &q) if err != nil { return err } + var resp struct{} - _, err = client.query("/v1/client/gc", &resp, nil) + _, err = nodeClient.query("/v1/client/gc", &resp, nil) return err } From 2f308272058f7ee8a78587f26198fcc98b9ec768 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 28 Aug 2017 14:41:21 -0700 Subject: [PATCH 2/3] Refactor GetNodeClient weirdness - No need to for a pointer to a pointer - Properly set and use QueryOptions.Region --- api/allocations.go | 4 +-- api/api.go | 79 ++++++++++++++++++++++------------------------ api/fs.go | 15 +++++---- api/nodes.go | 14 ++------ 4 files changed, 52 insertions(+), 60 deletions(-) diff --git a/api/allocations.go b/api/allocations.go index 42ac087f6bd9..1af3f2533b1a 100644 --- a/api/allocations.go +++ b/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 } diff --git a/api/api.go b/api/api.go index d5e56a07dd43..5f46383d3491 100644 --- a/api/api.go +++ b/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 -// that the Params field is valid. -func (c *Client) GetNodeClient(nodeID string, q **QueryOptions) (*Client, error) { - node, _, err := c.Nodes().Info(nodeID, &QueryOptions{}) +// QueryOptions is set, its region will be used. +func (c *Client) GetNodeClient(nodeID string, q *QueryOptions) (*Client, error) { + if q == nil { + 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.TLSConfig.TLSServerName = fmt.Sprintf("client.%s.nomad", region) - 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 + conf := c.config.CopyConfig(q.Region, node.HTTPAddr, node.TLSEnabled) + return NewClient(conf) } // request is used to help build up a request diff --git a/api/fs.go b/api/fs.go index efe65650ee71..e2fbf8181b02 100644 --- a/api/fs.go +++ b/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 diff --git a/api/nodes.go b/api/nodes.go index b1bdb5054acd..50a159628a33 100644 --- a/api/nodes.go +++ b/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) - 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)) + nodeClient, err := n.client.GetNodeClient(nodeID, q) 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 } From 52857a6a9d1aa6838fbae00f647ad4dc2087e3da Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 28 Aug 2017 14:58:15 -0700 Subject: [PATCH 3/3] Simplify region handling --- api/api.go | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/api/api.go b/api/api.go index 5f46383d3491..ce6d910fe170 100644 --- a/api/api.go +++ b/api/api.go @@ -110,8 +110,9 @@ type Config struct { TLSConfig *TLSConfig } -// CopyConfig copies the configuration with a new address -func (c *Config) CopyConfig(region, address string, tlsEnabled bool) *Config { +// ClientConfig copies the configuration with a new client address, region, and +// whether the client has TLS enabled. +func (c *Config) ClientConfig(region, address string, tlsEnabled bool) *Config { scheme := "http" if tlsEnabled { scheme = "https" @@ -299,25 +300,6 @@ func (c *Client) SetRegion(region string) { // GetNodeClient returns a new Client that will dial the specified node. If the // QueryOptions is set, its region will be used. func (c *Client) GetNodeClient(nodeID string, q *QueryOptions) (*Client, error) { - if q == nil { - 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 @@ -329,8 +311,13 @@ 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 := c.config.Region + if q != nil && q.Region != "" { + region = q.Region + } + // Get an API client for the node - conf := c.config.CopyConfig(q.Region, node.HTTPAddr, node.TLSEnabled) + conf := c.config.ClientConfig(region, node.HTTPAddr, node.TLSEnabled) return NewClient(conf) }