Skip to content

Commit 4f9845a

Browse files
author
Justin Reagor
authored
Upgrade ContainerPilot to Go 1.9.x and Consul 1.0.0 (#527)
* Update dependencies and peg Consul at v1.0.0 * tools: Switch builds and tooling to use Go 1.9.x and Consul 1.0.0 * testing: Don't cause NPEs by ignoring errors when configuring Consul * testing: Custom TestServer under `discovery` for exec'ing `consul` * discovery: Deprecate usage of Consul's internal `testutil` package This commit upgrades ContainerPilot to be built with Go 1.9 and tested against Consul 1.0.0. While upgrading Consul to version 1.0.0 I found that our tests caused a null pointer exception if there's an error configuring a Consul process to test against. This was due to the fact that we were ignoring errors when configuring Consul and continuing forward as if Consul was functional. This commit properly causes a fatal return from the test and includes the actual error returned by Consul's own internal testing framework (which we use for bootstrapping our own testing). Finally, this commit removes the use case within the discovery package's tests that depended on an internal Consul test package, `testutil`. We replace this with our own TestServer object which is responsible for executing a locally installed `consul` binary. Consul is installed by our Makefile target `tools` for local development use. Fixes: #528
1 parent bda8eba commit 4f9845a

6 files changed

Lines changed: 210 additions & 112 deletions

File tree

‎Dockerfile‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
1-
FROM golang:1.8
1+
FROM golang:1.9
2+
3+
ENV CONSUL_VERSION=1.0.0
4+
ENV GLIDE_VERSION=0.12.3
25

36
RUN apt-get update \
47
&& apt-get install -y unzip \
58
&& go get github.com/golang/lint/golint \
6-
&& curl -Lo /tmp/glide.tgz "https://github.com/Masterminds/glide/releases/download/v0.12.3/glide-v0.12.3-linux-amd64.tar.gz" \
9+
&& curl -Lo /tmp/glide.tgz "https://github.com/Masterminds/glide/releases/download/v${GLIDE_VERSION}/glide-v${GLIDE_VERSION}-linux-amd64.tar.gz" \
710
&& tar -C /usr/bin -xzf /tmp/glide.tgz --strip=1 linux-amd64/glide \
8-
&& curl --fail -Lso consul.zip "https://releases.hashicorp.com/consul/0.7.5/consul_0.7.5_linux_amd64.zip" \
11+
&& curl --fail -Lso consul.zip "https://releases.hashicorp.com/consul/${CONSUL_VERSION}/consul_${CONSUL_VERSION}_linux_amd64.zip" \
912
&& unzip consul.zip -d /usr/bin
1013

1114
ENV CGO_ENABLED 0

‎discovery/consul_test.go‎

Lines changed: 94 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"testing"
66

77
consul "github.com/hashicorp/consul/api"
8-
"github.com/hashicorp/consul/testutil"
98
"github.com/stretchr/testify/assert"
109
)
1110

@@ -71,113 +70,117 @@ func TestCheckForChanges(t *testing.T) {
7170
assert.True(t, didChange, "value for 'didChange' after t3")
7271
}
7372

74-
/*
75-
The TestWithConsul suite of tests uses Hashicorp's own testutil for managing
76-
a Consul server for testing. The 'consul' binary must be in the $PATH
77-
ref https://github.com/hashicorp/consul/tree/master/testutil
78-
*/
79-
80-
var testServer *testutil.TestServer
81-
8273
func TestWithConsul(t *testing.T) {
83-
testServer, _ = testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) {
84-
c.LogLevel = "err"
85-
})
74+
testServer, err := NewTestServer(8500)
75+
if err != nil {
76+
t.Fatal(err)
77+
}
8678
defer testServer.Stop()
87-
t.Run("TestConsulTTLPass", testConsulTTLPass)
88-
t.Run("TestConsulReregister", testConsulReregister)
89-
t.Run("TestConsulCheckForChanges", testConsulCheckForChanges)
90-
t.Run("TestConsulEnableTagOverride", testConsulEnableTagOverride)
79+
80+
testServer.WaitForAPI()
81+
82+
t.Run("TestConsulTTLPass", testConsulTTLPass(testServer))
83+
t.Run("TestConsulReregister", testConsulReregister(testServer))
84+
t.Run("TestConsulCheckForChanges", testConsulCheckForChanges(testServer))
85+
t.Run("TestConsulEnableTagOverride", testConsulEnableTagOverride(testServer))
9186
}
9287

93-
func testConsulTTLPass(t *testing.T) {
94-
consul, _ := NewConsul(testServer.HTTPAddr)
95-
name := fmt.Sprintf("TestConsulTTLPass")
96-
service := generateServiceDefinition(name, consul)
97-
checkID := fmt.Sprintf("service:%s", service.ID)
98-
99-
service.SendHeartbeat() // force registration and 1st heartbeat
100-
checks, _ := consul.Agent().Checks()
101-
check := checks[checkID]
102-
if check.Status != "passing" {
103-
t.Fatalf("status of check %s should be 'passing' but is %s", checkID, check.Status)
88+
func testConsulTTLPass(testServer *TestServer) func(*testing.T) {
89+
return func(t *testing.T) {
90+
consul, _ := NewConsul(testServer.HTTPAddr)
91+
name := fmt.Sprintf("TestConsulTTLPass")
92+
service := generateServiceDefinition(name, consul)
93+
checkID := fmt.Sprintf("service:%s", service.ID)
94+
95+
service.SendHeartbeat() // force registration and 1st heartbeat
96+
checks, _ := consul.Agent().Checks()
97+
check := checks[checkID]
98+
if check.Status != "passing" {
99+
t.Fatalf("status of check %s should be 'passing' but is %s", checkID, check.Status)
100+
}
104101
}
105102
}
106103

107-
func testConsulReregister(t *testing.T) {
108-
consul, _ := NewConsul(testServer.HTTPAddr)
109-
name := fmt.Sprintf("TestConsulReregister")
110-
service := generateServiceDefinition(name, consul)
111-
id := service.ID
112-
113-
service.SendHeartbeat() // force registration and 1st heartbeat
114-
services, _ := consul.Agent().Services()
115-
svc := services[id]
116-
if svc.Address != "192.168.1.1" {
117-
t.Fatalf("service address should be '192.168.1.1' but is %s", svc.Address)
118-
}
104+
func testConsulReregister(testServer *TestServer) func(*testing.T) {
105+
return func(t *testing.T) {
106+
consul, _ := NewConsul(testServer.HTTPAddr)
107+
name := fmt.Sprintf("TestConsulReregister")
108+
service := generateServiceDefinition(name, consul)
109+
id := service.ID
110+
111+
service.SendHeartbeat() // force registration and 1st heartbeat
112+
services, _ := consul.Agent().Services()
113+
svc := services[id]
114+
if svc.Address != "192.168.1.1" {
115+
t.Fatalf("service address should be '192.168.1.1' but is %s", svc.Address)
116+
}
119117

120-
// new Consul client (as though we've restarted)
121-
consul, _ = NewConsul(testServer.HTTPAddr)
122-
service = generateServiceDefinition(name, consul)
123-
service.IPAddress = "192.168.1.2"
124-
service.SendHeartbeat() // force re-registration and 1st heartbeat
118+
// new Consul client (as though we've restarted)
119+
consul, _ = NewConsul(testServer.HTTPAddr)
120+
service = generateServiceDefinition(name, consul)
121+
service.IPAddress = "192.168.1.2"
122+
service.SendHeartbeat() // force re-registration and 1st heartbeat
125123

126-
services, _ = consul.Agent().Services()
127-
svc = services[id]
128-
if svc.Address != "192.168.1.2" {
129-
t.Fatalf("service address should be '192.168.1.2' but is %s", svc.Address)
124+
services, _ = consul.Agent().Services()
125+
svc = services[id]
126+
if svc.Address != "192.168.1.2" {
127+
t.Fatalf("service address should be '192.168.1.2' but is %s", svc.Address)
128+
}
130129
}
131130
}
132131

133-
func testConsulCheckForChanges(t *testing.T) {
134-
backend := fmt.Sprintf("TestConsulCheckForChanges")
135-
consul, _ := NewConsul(testServer.HTTPAddr)
136-
service := generateServiceDefinition(backend, consul)
137-
id := service.ID
138-
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed {
139-
t.Fatalf("First read of %s should show `false` for change", id)
140-
}
141-
service.SendHeartbeat() // force registration and 1st heartbeat
132+
func testConsulCheckForChanges(testServer *TestServer) func(*testing.T) {
133+
return func(t *testing.T) {
134+
backend := fmt.Sprintf("TestConsulCheckForChanges")
135+
consul, _ := NewConsul(testServer.HTTPAddr)
136+
service := generateServiceDefinition(backend, consul)
137+
id := service.ID
138+
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed {
139+
t.Fatalf("First read of %s should show `false` for change", id)
140+
}
141+
service.SendHeartbeat() // force registration and 1st heartbeat
142142

143-
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); !changed {
144-
t.Errorf("%v should have changed after first health check TTL", id)
145-
}
146-
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed {
147-
t.Errorf("%v should not have changed without TTL expiring", id)
148-
}
149-
check := fmt.Sprintf("service:TestConsulCheckForChanges")
150-
consul.Agent().UpdateTTL(check, "expired", "critical")
151-
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); !changed {
152-
t.Errorf("%v should have changed after TTL expired.", id)
143+
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); !changed {
144+
t.Errorf("%v should have changed after first health check TTL", id)
145+
}
146+
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed {
147+
t.Errorf("%v should not have changed without TTL expiring", id)
148+
}
149+
check := fmt.Sprintf("service:TestConsulCheckForChanges")
150+
consul.Agent().UpdateTTL(check, "expired", "critical")
151+
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); !changed {
152+
t.Errorf("%v should have changed after TTL expired.", id)
153+
}
153154
}
154155
}
155156

156-
func testConsulEnableTagOverride(t *testing.T) {
157-
backend := fmt.Sprintf("TestConsulEnableTagOverride")
158-
consul, _ := NewConsul(testServer.HTTPAddr)
159-
service := &ServiceDefinition{
160-
ID: backend,
161-
Name: backend,
162-
IPAddress: "192.168.1.1",
163-
TTL: 1,
164-
Port: 9000,
165-
EnableTagOverride: true,
166-
Consul: consul,
167-
}
168-
id := service.ID
169-
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed {
170-
t.Fatalf("First read of %s should show `false` for change", id)
171-
}
172-
service.SendHeartbeat() // force registration
173-
catalogService, _, err := consul.Catalog().Service(id, "", nil)
174-
if err != nil {
175-
t.Fatalf("error finding service: %v", err)
176-
}
157+
func testConsulEnableTagOverride(testServer *TestServer) func(*testing.T) {
158+
return func(t *testing.T) {
159+
backend := fmt.Sprintf("TestConsulEnableTagOverride")
160+
consul, _ := NewConsul(testServer.HTTPAddr)
161+
service := &ServiceDefinition{
162+
ID: backend,
163+
Name: backend,
164+
IPAddress: "192.168.1.1",
165+
TTL: 1,
166+
Port: 9000,
167+
EnableTagOverride: true,
168+
Consul: consul,
169+
}
170+
id := service.ID
171+
if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed {
172+
t.Fatalf("First read of %s should show `false` for change", id)
173+
}
174+
service.SendHeartbeat() // force registration
175+
catalogService, _, err := consul.Catalog().Service(id, "", nil)
176+
if err != nil {
177+
t.Fatalf("error finding service: %v", err)
178+
}
177179

178-
for _, service := range catalogService {
179-
if service.ServiceEnableTagOverride != true {
180-
t.Errorf("%v should have had EnableTagOverride set to true", id)
180+
for _, service := range catalogService {
181+
if service.ServiceEnableTagOverride != true {
182+
t.Errorf("%v should have had EnableTagOverride set to true", id)
183+
}
181184
}
182185
}
183186
}

‎discovery/test_server.go‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
package discovery
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"io"
7+
"net/http"
8+
"os"
9+
"os/exec"
10+
"strconv"
11+
12+
"github.com/hashicorp/consul/testutil/retry"
13+
cleanhttp "github.com/hashicorp/go-cleanhttp"
14+
)
15+
16+
// TestServer represents a Consul server we can run our tests against. Depends
17+
// on a local `consul` binary installed into our environ's PATH.
18+
type TestServer struct {
19+
HTTPAddr string
20+
cmd *exec.Cmd
21+
client *http.Client
22+
}
23+
24+
// NewTestServer constructs a new TestServer by including the httpPort as well.
25+
func NewTestServer(httpPort int) (*TestServer, error) {
26+
path, err := exec.LookPath("consul")
27+
if err != nil || path == "" {
28+
return nil, fmt.Errorf("consul not found on $PATH - download and install " +
29+
"consul or skip this test")
30+
}
31+
32+
args := []string{"agent", "-dev", "-http-port", strconv.Itoa(httpPort)}
33+
cmd := exec.Command("consul", args...)
34+
cmd.Stdout = io.Writer(os.Stdout)
35+
cmd.Stderr = io.Writer(os.Stderr)
36+
if err := cmd.Start(); err != nil {
37+
return nil, errors.New("failed starting command")
38+
}
39+
40+
httpAddr := fmt.Sprintf("127.0.0.1:%d", httpPort)
41+
42+
client := cleanhttp.DefaultClient()
43+
44+
return &TestServer{httpAddr, cmd, client}, nil
45+
}
46+
47+
// Stop stops a TestServer
48+
func (s *TestServer) Stop() error {
49+
if s.cmd == nil {
50+
return nil
51+
}
52+
53+
if s.cmd.Process != nil {
54+
if err := s.cmd.Process.Signal(os.Interrupt); err != nil {
55+
return errors.New("failed to kill consul server")
56+
}
57+
}
58+
59+
return s.cmd.Wait()
60+
}
61+
62+
// failer implements the retry.Failer interface
63+
type failer struct {
64+
failed bool
65+
}
66+
67+
func (f *failer) Log(args ...interface{}) { fmt.Println(args) }
68+
func (f *failer) FailNow() { f.failed = true }
69+
70+
// WaitForAPI waits for only the agent HTTP endpoint to start responding. This
71+
// is an indication that the agent has started, but will likely return before a
72+
// leader is elected.
73+
func (s *TestServer) WaitForAPI() error {
74+
f := &failer{}
75+
retry.Run(f, func(r *retry.R) {
76+
resp, err := s.client.Get(s.HTTPAddr + "/v1/agent/self")
77+
if err != nil {
78+
r.Fatal(err)
79+
}
80+
defer resp.Body.Close()
81+
82+
if resp.StatusCode != 200 {
83+
r.Fatalf("bad status code %d", resp.StatusCode)
84+
}
85+
})
86+
if f.failed {
87+
return errors.New("failed waiting for API")
88+
}
89+
return nil
90+
}

‎glide.lock‎

Lines changed: 13 additions & 14 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)