Fix client.Clone() to include the address. (#10077)

CHANGELOG.md

@@ -1,5 +1,9 @@
 ## Next
 
+BUG FIXES:
+
+* core: Fix client.Clone() to include the address [[GH-10077](https://github.com/hashicorp/vault/pull/10077)]
+
 ## 1.6.0 RC
 ### November 4th, 2020
 

api/client.go

@@ -475,6 +475,9 @@ func (c *Client) SetAddress(addr string) error {
 		return errwrap.Wrapf("failed to set address: {{err}}", err)
 	}
 
+	c.config.modifyLock.Lock()
+	c.config.Address = addr
+	c.config.modifyLock.Unlock()
 	c.addr = parsedAddr
 	return nil
 }
@@ -492,57 +495,111 @@ func (c *Client) Address() string {
 // rateLimit and burst are specified according to https://godoc.org/golang.org/x/time/rate#NewLimiter
 func (c *Client) SetLimiter(rateLimit float64, burst int) {
 	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
 	c.config.modifyLock.Lock()
 	defer c.config.modifyLock.Unlock()
-	c.modifyLock.RUnlock()
 
 	c.config.Limiter = rate.NewLimiter(rate.Limit(rateLimit), burst)
 }
 
+func (c *Client) Limiter() *rate.Limiter {
+	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+	c.config.modifyLock.RLock()
+	defer c.config.modifyLock.RUnlock()
+
+	return c.config.Limiter
+}
+
 // SetMaxRetries sets the number of retries that will be used in the case of certain errors
 func (c *Client) SetMaxRetries(retries int) {
 	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
 	c.config.modifyLock.Lock()
 	defer c.config.modifyLock.Unlock()
-	c.modifyLock.RUnlock()
 
 	c.config.MaxRetries = retries
 }
 
+func (c *Client) MaxRetries() int {
+	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+	c.config.modifyLock.RLock()
+	defer c.config.modifyLock.RUnlock()
+
+	return c.config.MaxRetries
+}
+
+func (c *Client) SetSRVLookup(srv bool) {
+	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+	c.config.modifyLock.Lock()
+	defer c.config.modifyLock.Unlock()
+
+	c.config.SRVLookup = srv
+}
+
+func (c *Client) SRVLookup() bool {
+	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+	c.config.modifyLock.RLock()
+	defer c.config.modifyLock.RUnlock()
+
+	return c.config.SRVLookup
+}
+
 // SetCheckRetry sets the CheckRetry function to be used for future requests.
 func (c *Client) SetCheckRetry(checkRetry retryablehttp.CheckRetry) {
 	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
 	c.config.modifyLock.Lock()
 	defer c.config.modifyLock.Unlock()
-	c.modifyLock.RUnlock()
 
 	c.config.CheckRetry = checkRetry
 }
 
+func (c *Client) CheckRetry() retryablehttp.CheckRetry {
+	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+	c.config.modifyLock.RLock()
+	defer c.config.modifyLock.RUnlock()
+
+	return c.config.CheckRetry
+}
+
 // SetClientTimeout sets the client request timeout
 func (c *Client) SetClientTimeout(timeout time.Duration) {
 	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
 	c.config.modifyLock.Lock()
 	defer c.config.modifyLock.Unlock()
-	c.modifyLock.RUnlock()
 
 	c.config.Timeout = timeout
 }
 
-func (c *Client) OutputCurlString() bool {
+func (c *Client) ClientTimeout() time.Duration {
 	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+	c.config.modifyLock.RLock()
+	defer c.config.modifyLock.RUnlock()
+
+	return c.config.Timeout
+}
+
+func (c *Client) OutputCurlString() bool {
+	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
 	c.config.modifyLock.RLock()
 	defer c.config.modifyLock.RUnlock()
-	c.modifyLock.RUnlock()
 
 	return c.config.OutputCurlString
 }
 
 func (c *Client) SetOutputCurlString(curl bool) {
 	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
 	c.config.modifyLock.Lock()
 	defer c.config.modifyLock.Unlock()
-	c.modifyLock.RUnlock()
 
 	c.config.OutputCurlString = curl
 }
@@ -552,7 +609,6 @@ func (c *Client) SetOutputCurlString(curl bool) {
 func (c *Client) CurrentWrappingLookupFunc() WrappingLookupFunc {
 	c.modifyLock.RLock()
 	defer c.modifyLock.RUnlock()
-
 	return c.wrappingLookupFunc
 }
 
@@ -561,7 +617,6 @@ func (c *Client) CurrentWrappingLookupFunc() WrappingLookupFunc {
 func (c *Client) SetWrappingLookupFunc(lookupFunc WrappingLookupFunc) {
 	c.modifyLock.Lock()
 	defer c.modifyLock.Unlock()
-
 	c.wrappingLookupFunc = lookupFunc
 }
 
@@ -570,7 +625,6 @@ func (c *Client) SetWrappingLookupFunc(lookupFunc WrappingLookupFunc) {
 func (c *Client) SetMFACreds(creds []string) {
 	c.modifyLock.Lock()
 	defer c.modifyLock.Unlock()
-
 	c.mfaCreds = creds
 }
 
@@ -595,7 +649,6 @@ func (c *Client) setNamespace(namespace string) {
 func (c *Client) Token() string {
 	c.modifyLock.RLock()
 	defer c.modifyLock.RUnlock()
-
 	return c.token
 }
 
@@ -604,7 +657,6 @@ func (c *Client) Token() string {
 func (c *Client) SetToken(v string) {
 	c.modifyLock.Lock()
 	defer c.modifyLock.Unlock()
-
 	c.token = v
 }
 
@@ -612,7 +664,6 @@ func (c *Client) SetToken(v string) {
 func (c *Client) ClearToken() {
 	c.modifyLock.Lock()
 	defer c.modifyLock.Unlock()
-
 	c.token = ""
 }
 
@@ -655,9 +706,9 @@ func (c *Client) SetHeaders(headers http.Header) {
 // SetBackoff sets the backoff function to be used for future requests.
 func (c *Client) SetBackoff(backoff retryablehttp.Backoff) {
 	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
 	c.config.modifyLock.Lock()
 	defer c.config.modifyLock.Unlock()
-	c.modifyLock.RUnlock()
 
 	c.config.Backoff = backoff
 }
@@ -672,22 +723,30 @@ func (c *Client) SetBackoff(backoff retryablehttp.Backoff) {
 // behavior, must currently then be set as desired on the new client.
 func (c *Client) Clone() (*Client, error) {
 	c.modifyLock.RLock()
-	c.config.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+
 	config := c.config
-	c.modifyLock.RUnlock()
+	config.modifyLock.RLock()
+	defer config.modifyLock.RUnlock()
 
 	newConfig := &Config{
-		Address:    config.Address,
-		HttpClient: config.HttpClient,
-		MaxRetries: config.MaxRetries,
-		Timeout:    config.Timeout,
-		Backoff:    config.Backoff,
-		CheckRetry: config.CheckRetry,
-		Limiter:    config.Limiter,
+		Address:          config.Address,
+		HttpClient:       config.HttpClient,
+		MaxRetries:       config.MaxRetries,
+		Timeout:          config.Timeout,
+		Backoff:          config.Backoff,
+		CheckRetry:       config.CheckRetry,
+		Limiter:          config.Limiter,
+		OutputCurlString: config.OutputCurlString,
+		AgentAddress:     config.AgentAddress,
+		SRVLookup:        config.SRVLookup,
+	}
+	client, err := NewClient(newConfig)
+	if err != nil {
+		return nil, err
 	}
-	config.modifyLock.RUnlock()
 
-	return NewClient(newConfig)
+	return client, nil
 }
 
 // SetPolicyOverride sets whether requests should be sent with the policy
@@ -696,7 +755,6 @@ func (c *Client) Clone() (*Client, error) {
 func (c *Client) SetPolicyOverride(override bool) {
 	c.modifyLock.Lock()
 	defer c.modifyLock.Unlock()
-
 	c.policyOverride = override
 }
 

api/client_test.go

@@ -2,11 +2,13 @@ package api
 
 import (
 	"bytes"
+	"context"
 	"io"
 	"net/http"
 	"os"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/vault/sdk/helper/consts"
 )
@@ -345,16 +347,62 @@ func TestClientNonTransportRoundTripper(t *testing.T) {
 }
 
 func TestClone(t *testing.T) {
-	client1, err1 := NewClient(nil)
-	if err1 != nil {
-		t.Fatalf("NewClient failed: %v", err1)
-	}
-	client2, err2 := client1.Clone()
-	if err2 != nil {
-		t.Fatalf("Clone failed: %v", err2)
+	client1, err := NewClient(DefaultConfig())
+	if err != nil {
+		t.Fatalf("NewClient failed: %v", err)
 	}
 
-	_ = client2
+	// Set all of the things that we provide setter methods for, which modify config values
+	err = client1.SetAddress("http://example.com:8080")
+	if err != nil {
+		t.Fatalf("SetAddress failed: %v", err)
+	}
+
+	clientTimeout := time.Until(time.Now().AddDate(0, 0, 1))
+	client1.SetClientTimeout(clientTimeout)
+
+	checkRetry := func(ctx context.Context, resp *http.Response, err error) (bool, error) {
+		return true, nil
+	}
+	client1.SetCheckRetry(checkRetry)
+
+	client1.SetLimiter(5.0, 10)
+	client1.SetMaxRetries(5)
+	client1.SetOutputCurlString(true)
+	client1.SetSRVLookup(true)
+
+	client2, err := client1.Clone()
+	if err != nil {
+		t.Fatalf("Clone failed: %v", err)
+	}
+
+	if client1.Address() != client2.Address() {
+		t.Fatalf("addresses don't match: %v vs %v", client1.Address(), client2.Address())
+	}
+	if client1.ClientTimeout() != client2.ClientTimeout() {
+		t.Fatalf("timeouts don't match: %v vs %v", client1.ClientTimeout(), client2.ClientTimeout())
+	}
+	if client1.CheckRetry() != nil && client2.CheckRetry() == nil {
+		t.Fatal("checkRetry functions don't match. client2 is nil.")
+	}
+	if (client1.Limiter() != nil && client2.Limiter() == nil) || (client1.Limiter() == nil && client2.Limiter() != nil) {
+		t.Fatalf("limiters don't match: %v vs %v", client1.Limiter(), client2.Limiter())
+	}
+	if client1.Limiter().Limit() != client2.Limiter().Limit() {
+		t.Fatalf("limiter limits don't match: %v vs %v", client1.Limiter().Limit(), client2.Limiter().Limit())
+	}
+	if client1.Limiter().Burst() != client2.Limiter().Burst() {
+		t.Fatalf("limiter bursts don't match: %v vs %v", client1.Limiter().Burst(), client2.Limiter().Burst())
+	}
+	if client1.MaxRetries() != client2.MaxRetries() {
+		t.Fatalf("maxRetries don't match: %v vs %v", client1.MaxRetries(), client2.MaxRetries())
+	}
+	if client1.OutputCurlString() != client2.OutputCurlString() {
+		t.Fatalf("outputCurlString doesn't match: %v vs %v", client1.OutputCurlString(), client2.OutputCurlString())
+	}
+	if client1.SRVLookup() != client2.SRVLookup() {
+		t.Fatalf("SRVLookup doesn't match: %v vs %v", client1.SRVLookup(), client2.SRVLookup())
+	}
 }
 
 func TestSetHeadersRaceSafe(t *testing.T) {