Skip to content

Commit

Permalink
api: only set url field in config if previously unset (#23785)
Browse files Browse the repository at this point in the history
In #16872 we added support for configuring the API client with a unix domain
socket. In order to set the host correctly, we parse the address before mutating
the Address field in the configuration. But this prevents the configuration from
being reused across multiple clients, as the next time we parse the address it
will no longer be pointing to the socket. This breaks consumers like the
autoscaler, which reuse the API config between plugins.

Update the `NewClient` constructor to only override the `url` field if it hasn't
already been parsed. Include a test demonstrating safe reuse with a unix domain
socket.

Ref: hashicorp/nomad-autoscaler#944
Ref: hashicorp/nomad-autoscaler#945
  • Loading branch information
tgross authored Aug 9, 2024
1 parent 920f470 commit b7419bc
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 3 deletions.
3 changes: 3 additions & 0 deletions .changelog/23785.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
api: Fixed a bug where an `api.Config` targeting a unix domain socket could not be reused between clients
```
10 changes: 7 additions & 3 deletions api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -509,9 +509,13 @@ func NewClient(config *Config) (*Client, error) {
}

// we have to test the address that comes from DefaultConfig, because it
// could be the value of NOMAD_ADDR which is applied without testing
if config.url, err = url.Parse(config.Address); err != nil {
return nil, fmt.Errorf("invalid address '%s': %v", config.Address, err)
// could be the value of NOMAD_ADDR which is applied without testing. But
// only on the first use of this Config, otherwise we'll have mutated the
// address
if config.url == nil {
if config.url, err = url.Parse(config.Address); err != nil {
return nil, fmt.Errorf("invalid address '%s': %v", config.Address, err)
}
}

httpClient := config.HttpClient
Expand Down
44 changes: 44 additions & 0 deletions api/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,12 @@ import (
"errors"
"fmt"
"io"
"net"
"net/http"
"net/http/httptest"
"net/url"
"os"
"path"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -595,3 +598,44 @@ func TestClient_autoUnzip(t *testing.T) {
Body: io.NopCloser(&b),
}, nil)
}

func TestUnixSocketConfig(t *testing.T) {

td := os.TempDir() // testing.TempDir() on macOS makes paths that are too long
socketPath := path.Join(td, t.Name()+".sock")
os.Remove(socketPath) // git rid of stale ones now.

t.Cleanup(func() { os.Remove(socketPath) })

ts := httptest.NewUnstartedServer(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(`"10.1.1.1"`))
}))

// Create a Unix domain socket and listen for incoming connections.
socket, err := net.Listen("unix", socketPath)
must.NoError(t, err)
t.Cleanup(func() { socket.Close() })

ts.Listener = socket
ts.Start()
defer ts.Close()

cfg := &Config{Address: "unix://" + socketPath}

client1, err := NewClient(cfg)
must.NoError(t, err)
t.Cleanup(client1.Close)

resp, err := client1.Status().Leader()
must.NoError(t, err)
must.Eq(t, "10.1.1.1", resp)

client2, err := NewClient(cfg)
must.NoError(t, err)
t.Cleanup(client2.Close)

_, err = client2.Status().Leader()
must.NoError(t, err)
must.Eq(t, "10.1.1.1", resp)
}

0 comments on commit b7419bc

Please sign in to comment.