From 4f9845a50eb265cbd1a36ebe0b7da5960f06bc0d Mon Sep 17 00:00:00 2001 From: Justin Reagor Date: Tue, 14 Nov 2017 17:09:43 -0500 Subject: [PATCH] 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 --- Dockerfile | 9 +- discovery/consul_test.go | 185 ++++++++++++++++++++------------------- discovery/test_server.go | 90 +++++++++++++++++++ glide.lock | 27 +++--- glide.yaml | 2 +- makefile | 9 +- 6 files changed, 210 insertions(+), 112 deletions(-) create mode 100644 discovery/test_server.go diff --git a/Dockerfile b/Dockerfile index 127ad282..fa856d66 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,11 +1,14 @@ -FROM golang:1.8 +FROM golang:1.9 + +ENV CONSUL_VERSION=1.0.0 +ENV GLIDE_VERSION=0.12.3 RUN apt-get update \ && apt-get install -y unzip \ && go get github.com/golang/lint/golint \ - && curl -Lo /tmp/glide.tgz "https://github.com/Masterminds/glide/releases/download/v0.12.3/glide-v0.12.3-linux-amd64.tar.gz" \ + && curl -Lo /tmp/glide.tgz "https://github.com/Masterminds/glide/releases/download/v${GLIDE_VERSION}/glide-v${GLIDE_VERSION}-linux-amd64.tar.gz" \ && tar -C /usr/bin -xzf /tmp/glide.tgz --strip=1 linux-amd64/glide \ - && curl --fail -Lso consul.zip "https://releases.hashicorp.com/consul/0.7.5/consul_0.7.5_linux_amd64.zip" \ + && curl --fail -Lso consul.zip "https://releases.hashicorp.com/consul/${CONSUL_VERSION}/consul_${CONSUL_VERSION}_linux_amd64.zip" \ && unzip consul.zip -d /usr/bin ENV CGO_ENABLED 0 diff --git a/discovery/consul_test.go b/discovery/consul_test.go index 44847c7c..385e8ca5 100644 --- a/discovery/consul_test.go +++ b/discovery/consul_test.go @@ -5,7 +5,6 @@ import ( "testing" consul "github.com/hashicorp/consul/api" - "github.com/hashicorp/consul/testutil" "github.com/stretchr/testify/assert" ) @@ -71,113 +70,117 @@ func TestCheckForChanges(t *testing.T) { assert.True(t, didChange, "value for 'didChange' after t3") } -/* -The TestWithConsul suite of tests uses Hashicorp's own testutil for managing -a Consul server for testing. The 'consul' binary must be in the $PATH -ref https://github.com/hashicorp/consul/tree/master/testutil -*/ - -var testServer *testutil.TestServer - func TestWithConsul(t *testing.T) { - testServer, _ = testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { - c.LogLevel = "err" - }) + testServer, err := NewTestServer(8500) + if err != nil { + t.Fatal(err) + } defer testServer.Stop() - t.Run("TestConsulTTLPass", testConsulTTLPass) - t.Run("TestConsulReregister", testConsulReregister) - t.Run("TestConsulCheckForChanges", testConsulCheckForChanges) - t.Run("TestConsulEnableTagOverride", testConsulEnableTagOverride) + + testServer.WaitForAPI() + + t.Run("TestConsulTTLPass", testConsulTTLPass(testServer)) + t.Run("TestConsulReregister", testConsulReregister(testServer)) + t.Run("TestConsulCheckForChanges", testConsulCheckForChanges(testServer)) + t.Run("TestConsulEnableTagOverride", testConsulEnableTagOverride(testServer)) } -func testConsulTTLPass(t *testing.T) { - consul, _ := NewConsul(testServer.HTTPAddr) - name := fmt.Sprintf("TestConsulTTLPass") - service := generateServiceDefinition(name, consul) - checkID := fmt.Sprintf("service:%s", service.ID) - - service.SendHeartbeat() // force registration and 1st heartbeat - checks, _ := consul.Agent().Checks() - check := checks[checkID] - if check.Status != "passing" { - t.Fatalf("status of check %s should be 'passing' but is %s", checkID, check.Status) +func testConsulTTLPass(testServer *TestServer) func(*testing.T) { + return func(t *testing.T) { + consul, _ := NewConsul(testServer.HTTPAddr) + name := fmt.Sprintf("TestConsulTTLPass") + service := generateServiceDefinition(name, consul) + checkID := fmt.Sprintf("service:%s", service.ID) + + service.SendHeartbeat() // force registration and 1st heartbeat + checks, _ := consul.Agent().Checks() + check := checks[checkID] + if check.Status != "passing" { + t.Fatalf("status of check %s should be 'passing' but is %s", checkID, check.Status) + } } } -func testConsulReregister(t *testing.T) { - consul, _ := NewConsul(testServer.HTTPAddr) - name := fmt.Sprintf("TestConsulReregister") - service := generateServiceDefinition(name, consul) - id := service.ID - - service.SendHeartbeat() // force registration and 1st heartbeat - services, _ := consul.Agent().Services() - svc := services[id] - if svc.Address != "192.168.1.1" { - t.Fatalf("service address should be '192.168.1.1' but is %s", svc.Address) - } +func testConsulReregister(testServer *TestServer) func(*testing.T) { + return func(t *testing.T) { + consul, _ := NewConsul(testServer.HTTPAddr) + name := fmt.Sprintf("TestConsulReregister") + service := generateServiceDefinition(name, consul) + id := service.ID + + service.SendHeartbeat() // force registration and 1st heartbeat + services, _ := consul.Agent().Services() + svc := services[id] + if svc.Address != "192.168.1.1" { + t.Fatalf("service address should be '192.168.1.1' but is %s", svc.Address) + } - // new Consul client (as though we've restarted) - consul, _ = NewConsul(testServer.HTTPAddr) - service = generateServiceDefinition(name, consul) - service.IPAddress = "192.168.1.2" - service.SendHeartbeat() // force re-registration and 1st heartbeat + // new Consul client (as though we've restarted) + consul, _ = NewConsul(testServer.HTTPAddr) + service = generateServiceDefinition(name, consul) + service.IPAddress = "192.168.1.2" + service.SendHeartbeat() // force re-registration and 1st heartbeat - services, _ = consul.Agent().Services() - svc = services[id] - if svc.Address != "192.168.1.2" { - t.Fatalf("service address should be '192.168.1.2' but is %s", svc.Address) + services, _ = consul.Agent().Services() + svc = services[id] + if svc.Address != "192.168.1.2" { + t.Fatalf("service address should be '192.168.1.2' but is %s", svc.Address) + } } } -func testConsulCheckForChanges(t *testing.T) { - backend := fmt.Sprintf("TestConsulCheckForChanges") - consul, _ := NewConsul(testServer.HTTPAddr) - service := generateServiceDefinition(backend, consul) - id := service.ID - if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed { - t.Fatalf("First read of %s should show `false` for change", id) - } - service.SendHeartbeat() // force registration and 1st heartbeat +func testConsulCheckForChanges(testServer *TestServer) func(*testing.T) { + return func(t *testing.T) { + backend := fmt.Sprintf("TestConsulCheckForChanges") + consul, _ := NewConsul(testServer.HTTPAddr) + service := generateServiceDefinition(backend, consul) + id := service.ID + if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed { + t.Fatalf("First read of %s should show `false` for change", id) + } + service.SendHeartbeat() // force registration and 1st heartbeat - if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); !changed { - t.Errorf("%v should have changed after first health check TTL", id) - } - if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed { - t.Errorf("%v should not have changed without TTL expiring", id) - } - check := fmt.Sprintf("service:TestConsulCheckForChanges") - consul.Agent().UpdateTTL(check, "expired", "critical") - if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); !changed { - t.Errorf("%v should have changed after TTL expired.", id) + if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); !changed { + t.Errorf("%v should have changed after first health check TTL", id) + } + if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed { + t.Errorf("%v should not have changed without TTL expiring", id) + } + check := fmt.Sprintf("service:TestConsulCheckForChanges") + consul.Agent().UpdateTTL(check, "expired", "critical") + if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); !changed { + t.Errorf("%v should have changed after TTL expired.", id) + } } } -func testConsulEnableTagOverride(t *testing.T) { - backend := fmt.Sprintf("TestConsulEnableTagOverride") - consul, _ := NewConsul(testServer.HTTPAddr) - service := &ServiceDefinition{ - ID: backend, - Name: backend, - IPAddress: "192.168.1.1", - TTL: 1, - Port: 9000, - EnableTagOverride: true, - Consul: consul, - } - id := service.ID - if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed { - t.Fatalf("First read of %s should show `false` for change", id) - } - service.SendHeartbeat() // force registration - catalogService, _, err := consul.Catalog().Service(id, "", nil) - if err != nil { - t.Fatalf("error finding service: %v", err) - } +func testConsulEnableTagOverride(testServer *TestServer) func(*testing.T) { + return func(t *testing.T) { + backend := fmt.Sprintf("TestConsulEnableTagOverride") + consul, _ := NewConsul(testServer.HTTPAddr) + service := &ServiceDefinition{ + ID: backend, + Name: backend, + IPAddress: "192.168.1.1", + TTL: 1, + Port: 9000, + EnableTagOverride: true, + Consul: consul, + } + id := service.ID + if changed, _ := consul.CheckForUpstreamChanges(backend, "", ""); changed { + t.Fatalf("First read of %s should show `false` for change", id) + } + service.SendHeartbeat() // force registration + catalogService, _, err := consul.Catalog().Service(id, "", nil) + if err != nil { + t.Fatalf("error finding service: %v", err) + } - for _, service := range catalogService { - if service.ServiceEnableTagOverride != true { - t.Errorf("%v should have had EnableTagOverride set to true", id) + for _, service := range catalogService { + if service.ServiceEnableTagOverride != true { + t.Errorf("%v should have had EnableTagOverride set to true", id) + } } } } diff --git a/discovery/test_server.go b/discovery/test_server.go new file mode 100644 index 00000000..cc32a8df --- /dev/null +++ b/discovery/test_server.go @@ -0,0 +1,90 @@ +package discovery + +import ( + "errors" + "fmt" + "io" + "net/http" + "os" + "os/exec" + "strconv" + + "github.com/hashicorp/consul/testutil/retry" + cleanhttp "github.com/hashicorp/go-cleanhttp" +) + +// TestServer represents a Consul server we can run our tests against. Depends +// on a local `consul` binary installed into our environ's PATH. +type TestServer struct { + HTTPAddr string + cmd *exec.Cmd + client *http.Client +} + +// NewTestServer constructs a new TestServer by including the httpPort as well. +func NewTestServer(httpPort int) (*TestServer, error) { + path, err := exec.LookPath("consul") + if err != nil || path == "" { + return nil, fmt.Errorf("consul not found on $PATH - download and install " + + "consul or skip this test") + } + + args := []string{"agent", "-dev", "-http-port", strconv.Itoa(httpPort)} + cmd := exec.Command("consul", args...) + cmd.Stdout = io.Writer(os.Stdout) + cmd.Stderr = io.Writer(os.Stderr) + if err := cmd.Start(); err != nil { + return nil, errors.New("failed starting command") + } + + httpAddr := fmt.Sprintf("127.0.0.1:%d", httpPort) + + client := cleanhttp.DefaultClient() + + return &TestServer{httpAddr, cmd, client}, nil +} + +// Stop stops a TestServer +func (s *TestServer) Stop() error { + if s.cmd == nil { + return nil + } + + if s.cmd.Process != nil { + if err := s.cmd.Process.Signal(os.Interrupt); err != nil { + return errors.New("failed to kill consul server") + } + } + + return s.cmd.Wait() +} + +// failer implements the retry.Failer interface +type failer struct { + failed bool +} + +func (f *failer) Log(args ...interface{}) { fmt.Println(args) } +func (f *failer) FailNow() { f.failed = true } + +// WaitForAPI waits for only the agent HTTP endpoint to start responding. This +// is an indication that the agent has started, but will likely return before a +// leader is elected. +func (s *TestServer) WaitForAPI() error { + f := &failer{} + retry.Run(f, func(r *retry.R) { + resp, err := s.client.Get(s.HTTPAddr + "/v1/agent/self") + if err != nil { + r.Fatal(err) + } + defer resp.Body.Close() + + if resp.StatusCode != 200 { + r.Fatalf("bad status code %d", resp.StatusCode) + } + }) + if f.failed { + return errors.New("failed waiting for API") + } + return nil +} diff --git a/glide.lock b/glide.lock index fe7c2e72..2bd3f8e9 100644 --- a/glide.lock +++ b/glide.lock @@ -1,12 +1,12 @@ -hash: a730729c91753fec77a0c9b4084e37a07ab19dceb8f5c4f8b134eee0d7980df9 -updated: 2017-07-05T15:36:19.413681678-04:00 +hash: c13286385a0d957eb3b4e47af1f85b6cd8d8b084c846737477f46ac04d2c44f2 +updated: 2017-11-13T21:22:43.229954534-05:00 imports: - name: github.com/beorn7/perks version: 4c0e84591b9aa9e6dcfdf3e020114cd81f89d5f9 subpackages: - quantile -- name: github.com/docker/go-units - version: 0dadbb0345b35ec7ef35e228dabb8de89a65bf52 +- name: github.com/client9/reopen + version: 1a6ccbeaae3f56aa0058f5491382cb21726e214e - name: github.com/flynn/json5 version: 7620272ed63390e979cf5882d2fa0506fe2a8db5 - name: github.com/golang/protobuf @@ -14,17 +14,14 @@ imports: subpackages: - proto - name: github.com/hashicorp/consul - version: 2c7715154d8d4568524b76d2d4deb7ca6fd1b285 + version: 783a405d781ffc5edaf2d6b5eff41e22df96644f subpackages: - api - - testutil - testutil/retry - name: github.com/hashicorp/go-cleanhttp version: 3573b8b52aa7b37b9358d966a898feb387f62437 - name: github.com/hashicorp/go-rootcerts version: 6bb64b370b90e7ef1fa532be9e591a81c3493e00 -- name: github.com/hashicorp/go-uuid - version: 64130c7a86d732268a38cb04cfbaf0cc987fda98 - name: github.com/hashicorp/serf version: 91fd53b1d3e624389ed9a295a3fa380e5c7b9dfc subpackages: @@ -37,8 +34,6 @@ imports: version: b8bc1bf767474819792c23f32d8286a45736f1c6 - name: github.com/mitchellh/mapstructure version: d2dd0262208475919e1a362f675cfc0e7c10e905 -- name: github.com/pkg/errors - version: c605e284fe17294bda444b34710735b29d1a9d90 - name: github.com/prometheus/client_golang version: c5b7fccd204277076155f10851dad72b76a49317 subpackages: @@ -57,8 +52,6 @@ imports: version: e645f4e5aaa8506fc71d6edbc5c4ff02c04c46f2 subpackages: - xfs -- name: github.com/samalba/dockerclient - version: a3036261847103270e9f732509f43b5f98710ace - name: github.com/sirupsen/logrus version: 202f25545ea4cf9b191ff7f846df5d87c9382c2b - name: golang.org/x/sys @@ -66,9 +59,15 @@ imports: subpackages: - unix testImports: +- name: github.com/davecgh/go-spew + version: 6d212800a42e8ab5c146b8ace3490ee17e5225f9 + subpackages: + - spew +- name: github.com/pmezard/go-difflib + version: d8ed2627bdf02c080bf22230dbb337003b7aba2d + subpackages: + - difflib - name: github.com/stretchr/testify version: 69483b4bd14f5845b5a1e55bca19e954e827f1d0 subpackages: - assert -- name: github.com/client9/reopen - version: 1a6ccbeaae3f56aa0058f5491382cb21726e214e diff --git a/glide.yaml b/glide.yaml index 22bfab67..4c0c1053 100644 --- a/glide.yaml +++ b/glide.yaml @@ -5,7 +5,7 @@ import: - package: github.com/sirupsen/logrus version: 1.0.0 - package: github.com/hashicorp/consul - version: ~0.8.4 + version: ~1.0.0 subpackages: - api - package: github.com/mitchellh/mapstructure diff --git a/makefile b/makefile index 5bcccdb4..b95baf74 100644 --- a/makefile +++ b/makefile @@ -21,6 +21,9 @@ GOARCH ?= $(shell go env GOARCH) CGO_ENABLED := 0 GOEXPERIMENT := framepointer +CONSUL_VERSION := 1.0.0 +GLIDE_VERSION := 0.12.3 + ## display this help message help: @echo -e "\033[32m" @@ -94,13 +97,13 @@ dep-add: build/containerpilot_build # run 'GOOS=darwin make tools' if you're installing on MacOS ## set up local dev environment tools: - @go version | grep 1.[8,9] || (echo 'go1.8 or go1.9 should be installed') + @go version | grep 1.9 || (echo 'WARNING: go1.9 should be installed!') @$(if $(value GOPATH),, $(error 'GOPATH not set')) go get github.com/golang/lint/golint - curl --fail -Lso glide.tgz "https://github.com/Masterminds/glide/releases/download/v0.12.3/glide-v0.12.3-$(GOOS)-$(GOARCH).tar.gz" + curl --fail -Lso glide.tgz "https://github.com/Masterminds/glide/releases/download/v$(GLIDE_VERSION)/glide-v$(GLIDE_VERSION)-$(GOOS)-$(GOARCH).tar.gz" tar -C "$(GOPATH)/bin" -xzf glide.tgz --strip=1 $(GOOS)-$(GOARCH)/glide rm glide.tgz - curl --fail -Lso consul.zip "https://releases.hashicorp.com/consul/0.7.5/consul_0.7.5_$(GOOS)_$(GOARCH).zip" + curl --fail -Lso consul.zip "https://releases.hashicorp.com/consul/$(CONSUL_VERSION)/consul_$(CONSUL_VERSION)_$(GOOS)_$(GOARCH).zip" unzip consul.zip -d "$(GOPATH)/bin" rm consul.zip