Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vendor: Update terraform/consul dependency #23601

Closed
wants to merge 6 commits into from

Conversation

LorbusChris
Copy link

@LorbusChris LorbusChris commented Dec 8, 2019

Updating to the current version of consul. The version currently used and vendored in this repository is outdated.

Some other dependencies were also updated when running go mod tidy && go mod vendor

@hashicorp-cla
Copy link

hashicorp-cla commented Dec 8, 2019

CLA assistant check
All committers have signed the CLA.

@LorbusChris
Copy link
Author

LorbusChris commented Dec 8, 2019

/cc @kayrus @radeksimko

@LorbusChris LorbusChris changed the title vendor: Update tarraform/consul dependency vendor: Update terraform/consul dependency Dec 8, 2019
@LorbusChris
Copy link
Author

It would be cleaner to get #23510 in separately before this

@LorbusChris
Copy link
Author

@danieldreier any news on this?

@LorbusChris
Copy link
Author

@apparentlymart please take a look at this and #23510. The currently vendored consul version is ancient.

@LorbusChris
Copy link
Author

@danieldreier @apparentlymart the vendored Consul version is now 2.5 years old.
Please have a look a #23510 and subsequently this PR here

@LorbusChris LorbusChris force-pushed the update-consul-dep branch 4 times, most recently from 09d02d6 to 63e2a3d Compare April 27, 2020 22:33
@LorbusChris
Copy link
Author

@danieldreier thank you for helping get #23510 in. This PR is now unblocked, and I've rebased it.

@danieldreier
Copy link
Contributor

@LorbusChris thanks for rebasing. backend/consul is now maintained by @hashicorp/consul so I'm going to request their review.

I can't quite tell - does this imply any breaking changes or need for users to update configurations? It looks to me like a pure library version update but I wanted to confirm, e.g. if we lose support for old consul versions or something that we should give users a heads-up on in the release notes.

@danieldreier
Copy link
Contributor

@LorbusChris can you help me understand the purpose of updating to this version? I'm not sure how to prioritize this without other context, if this is a might-as-well, if you are blocked on this in some way, etc.

@LorbusChris
Copy link
Author

With Go 1.13 we hit a version mismatch with a dependency of Consul when importing terraform.
Unfortunately I can't repro this right now as we've switched to a fork of terraform for this, and a number of other reasons.

Regardless of that however, I think keeping dependencies up to date is important.

Most imports here in this repo are from consul/api, and can stay unchanged. The testutil was moved from the main consul package into consul/sdk and the import was changed accordingly: https://github.com/hashicorp/terraform/pull/23601/files#diff-7948751d05c27ab0f28702c12bef2e96R10
Looking at the go.mod file it becomes obvious that now only the api and sdk sub-packages of consul are needed, not the entirety of it.

@LorbusChris
Copy link
Author

Rebased again

@LorbusChris
Copy link
Author

Rebased again

@LorbusChris
Copy link
Author

Rebased. @hashicorp/consul any chance you could have a look at this?

@LorbusChris
Copy link
Author

@remilapeyre I did run that on the code before committing - are you saying it’s needed again? Thank you for the review :)

@LorbusChris
Copy link
Author

rebased

@remilapeyre
Copy link
Contributor

@remilapeyre I did run that on the code before committing - are you saying it’s needed again?

When running go mod tidy && go mod vendor it makes changes to vendor/go.opencensus.io/go.mod but Go modules are a bit like black magic to me so I'm not sure it's actually needed, the change is just:

diff --git a/vendor/go.opencensus.io/go.mod b/vendor/go.opencensus.io/go.mod
index cb4de80f3..51f130a14 100644
--- a/vendor/go.opencensus.io/go.mod
+++ b/vendor/go.opencensus.io/go.mod
@@ -10,3 +10,5 @@ require (
        google.golang.org/genproto v0.0.0-20190425155659-357c62f0e4bb // indirect
        google.golang.org/grpc v1.20.1
 )
+
+go 1.13

@LorbusChris
Copy link
Author

It doesn't yield those changes on my system (go version go1.14.6 linux/amd64). It also doesn't look like a required change to me. I would assume the CI tests verify the vendor contents.

@LorbusChris
Copy link
Author

rebased again. @freddygv @remilapeyre PTAL

@LorbusChris
Copy link
Author

If it makes things easier to review, I can split the vendor dir changes into a separate commit. Please let me know.

@LorbusChris
Copy link
Author

rebased

@alisdair
Copy link
Contributor

Thanks for opening and rebasing this PR, @LorbusChris! Sorry it's taking so long to get to, but we're looking at it now.

I'm a little concerned by the number of core dependency upgrades that bumping Consul is forcing on us. My plan is to pull some of those out into separate PRs so that they can be tested and reviewed separately. If those PRs end up merged, I expect there might be conflicts here—but please don't feel that you have rebase until that process is complete.

@alisdair alisdair self-assigned this Sep 10, 2020
Updates github.com/mattn/go-colorable to v0.1.7,
and github.com/mattn/go-isatty to v0.0.12.
Updates consul/api to v1.7.0 and consul/sdk to v0.6.0

Fixes TestMain in `backend/remote-state/consul/backend.test` to work with new version of testutil imported from consul/sdk.
@LorbusChris
Copy link
Author

@alisdair thank you, that's fair. I've split up the big commit into smaller ones that update one dep each. Feel free to take them as a reference.

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LorbusChris Thank you for doing that, it's extremely helpful!

I've now reviewed all of the extra dependency updates and I'm satisfied that they should not introduce any problems. I need to manually test the Windows build to verify some changes to terminal output colourization, but that shouldn't take long.

However, I noticed when reviewing this and other Consul backend PRs that our CI system was not running the backend tests. I fixed that in #26221 (just merged now), and it looks like tests are failing on this branch. I think if you rebase you'll see the failures here, too. You should also be able to verify this locally:

TF_CONSUL_TEST=1 go test ./backend/remote-state/consul/

I think the issue is in the change to newConsulTestServer, where we're passing a nil pointer to the testing.TB interface to satisfy the type signature of testutil.NewTestServerConfigT. This results in a panic when later code attempts to use the callbacks on this value.

testutil.NewTestServerConfigT is intended to be used with a *testing.T value, which means that it has to be called from within a test. Unfortunately, naively changing the newConsulTestServer helper to spawn a new server, calling it from within each test, and defer srv.Stop() results in pipe: too many open files errors on my workstation.

We can't implement testing.TB ourselves because of a private field, and I'm not aware of any way of constructing a *testing.T from inside TestMain. I'm currently out of ideas. Hopefully you have an idea of how to move forward!

We can't merge this until the tests pass, so I'm marking this PR as "request changes" to make sure we don't accidentally merge it.

@rboyer
Copy link
Member

rboyer commented Sep 11, 2020

Hi, I'm on the Consul team so I figured I'd pop in and help out.

I took at stab at the changes @alisdair described and I didn't have a file handle issue running tests locally. Perhaps the way I patched it up differed from your approach somehow?

(This omits go.mod, go.sum, and vendor updates).

diff --git a/backend/remote-state/consul/backend_test.go b/backend/remote-state/consul/backend_test.go
index 394bf428e..c92eeb01a 100644
--- a/backend/remote-state/consul/backend_test.go
+++ b/backend/remote-state/consul/backend_test.go
@@ -8,7 +8,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/hashicorp/consul/testutil"
+	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/terraform/backend"
 )
 
@@ -16,28 +16,18 @@ func TestBackend_impl(t *testing.T) {
 	var _ backend.Backend = new(Backend)
 }
 
-var srv *testutil.TestServer
-
 func TestMain(m *testing.M) {
 	if os.Getenv("TF_ACC") == "" && os.Getenv("TF_CONSUL_TEST") == "" {
 		fmt.Println("consul server tests require setting TF_ACC or TF_CONSUL_TEST")
 		return
 	}
 
-	var err error
-	srv, err = newConsulTestServer()
-	if err != nil {
-		fmt.Println(err)
-		os.Exit(1)
-	}
-
 	rc := m.Run()
-	srv.Stop()
 	os.Exit(rc)
 }
 
-func newConsulTestServer() (*testutil.TestServer, error) {
-	srv, err := testutil.NewTestServerConfig(func(c *testutil.TestServerConfig) {
+func newConsulTestServer(t *testing.T) (*testutil.TestServer, error) {
+	srv, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) {
 		c.LogLevel = "warn"
 
 		if !flag.Parsed() {
@@ -54,6 +44,12 @@ func newConsulTestServer() (*testutil.TestServer, error) {
 }
 
 func TestBackend(t *testing.T) {
+	srv, err := newConsulTestServer(t)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	defer srv.Stop()
+
 	path := fmt.Sprintf("tf-unit/%s", time.Now().String())
 
 	// Get the backend. We need two to test locking.
@@ -73,6 +69,12 @@ func TestBackend(t *testing.T) {
 }
 
 func TestBackend_lockDisabled(t *testing.T) {
+	srv, err := newConsulTestServer(t)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	defer srv.Stop()
+
 	path := fmt.Sprintf("tf-unit/%s", time.Now().String())
 
 	// Get the backend. We need two to test locking.
@@ -94,6 +96,12 @@ func TestBackend_lockDisabled(t *testing.T) {
 }
 
 func TestBackend_gzip(t *testing.T) {
+	srv, err := newConsulTestServer(t)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	defer srv.Stop()
+
 	// Get the backend
 	b := backend.TestBackendConfig(t, New(), backend.TestWrapConfig(map[string]interface{}{
 		"address": srv.HTTPAddr,
diff --git a/backend/remote-state/consul/client_test.go b/backend/remote-state/consul/client_test.go
index ff38f0120..24e8de8b9 100644
--- a/backend/remote-state/consul/client_test.go
+++ b/backend/remote-state/consul/client_test.go
@@ -19,6 +19,12 @@ func TestRemoteClient_impl(t *testing.T) {
 }
 
 func TestRemoteClient(t *testing.T) {
+	srv, err := newConsulTestServer(t)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	defer srv.Stop()
+
 	testCases := []string{
 		fmt.Sprintf("tf-unit/%s", time.Now().String()),
 		fmt.Sprintf("tf-unit/%s/", time.Now().String()),
@@ -46,6 +52,12 @@ func TestRemoteClient(t *testing.T) {
 
 // test the gzip functionality of the client
 func TestRemoteClient_gzipUpgrade(t *testing.T) {
+	srv, err := newConsulTestServer(t)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	defer srv.Stop()
+
 	statePath := fmt.Sprintf("tf-unit/%s", time.Now().String())
 
 	// Get the backend
@@ -81,6 +93,12 @@ func TestRemoteClient_gzipUpgrade(t *testing.T) {
 }
 
 func TestConsul_stateLock(t *testing.T) {
+	srv, err := newConsulTestServer(t)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	defer srv.Stop()
+
 	testCases := []string{
 		fmt.Sprintf("tf-unit/%s", time.Now().String()),
 		fmt.Sprintf("tf-unit/%s/", time.Now().String()),
@@ -111,6 +129,12 @@ func TestConsul_stateLock(t *testing.T) {
 }
 
 func TestConsul_destroyLock(t *testing.T) {
+	srv, err := newConsulTestServer(t)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	defer srv.Stop()
+
 	testCases := []string{
 		fmt.Sprintf("tf-unit/%s", time.Now().String()),
 		fmt.Sprintf("tf-unit/%s/", time.Now().String()),
@@ -157,6 +181,12 @@ func TestConsul_destroyLock(t *testing.T) {
 }
 
 func TestConsul_lostLock(t *testing.T) {
+	srv, err := newConsulTestServer(t)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	defer srv.Stop()
+
 	path := fmt.Sprintf("tf-unit/%s", time.Now().String())
 
 	// create 2 instances to get 2 remote.Clients
@@ -204,6 +234,12 @@ func TestConsul_lostLock(t *testing.T) {
 }
 
 func TestConsul_lostLockConnection(t *testing.T) {
+	srv, err := newConsulTestServer(t)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	defer srv.Stop()
+
 	// create an "unreliable" network by closing all the consul client's
 	// network connections
 	conns := &unreliableConns{}

@alisdair
Copy link
Contributor

@rboyer Thanks, I appreciate it! That's fundamentally the same patch I tried (I had just moved the error checking inside the helper). Trying your exact patch has the same result, although I just noticed that the first error is not "too many open files" but instead "api unavailable":

$ TF_CONSUL_TEST=1 go test ./backend/remote-state/consul/
[INFO] freeport: blockSize 1500 too big for system limit 256. Adjusting...
[INFO] freeport: detected ephemeral port range of [49152, 65535]
2020/09/11 17:12:25 [INFO] created consul lock session 1791e017-8508-f989-7c57-7d678e6b485f
2020/09/11 17:12:25 [INFO] created consul lock session ec77b90d-09e7-9f57-a94c-aa53fdbec0e6
2020/09/11 17:12:25 [INFO] created consul lock session f5843fac-5347-1c74-9794-bf3624e3a2d2
2020/09/11 17:12:25 [INFO] created consul lock session 7f1453a8-709d-366e-5558-0bd6759ab99a
2020/09/11 17:12:25 [INFO] created consul lock session 40959846-5c2d-a0c8-5238-01e766f9b4e7
2020/09/11 17:12:26 [INFO] created consul lock session c44b6d66-b170-6af7-7845-c6ea8f1d2e65
2020/09/11 17:12:26 [INFO] created consul lock session c7aeef9b-389e-3df9-3869-527cd601e0f4
2020/09/11 17:12:27 [INFO] created consul lock session 79bf5a1b-6697-8cce-fe3b-72686be1e4cc
2020/09/11 17:12:31 [INFO] created consul lock session e35fad1c-0cd6-6347-7c23-2e976483cf76
2020/09/11 17:12:31 [INFO] created consul lock session c1901d0f-6142-80a8-bacc-58d5a48f1c18
2020/09/11 17:12:31 [INFO] created consul lock session f2091d39-d350-bcf9-2952-480e00305e81
2020/09/11 17:12:31 [INFO] created consul lock session d311ea65-f45a-25b3-f6b9-a9699ebf229c
2020/09/11 17:12:31 [INFO] created consul lock session 6c6ea870-96f8-9810-6d69-cc319e6d2135
--- FAIL: TestRemoteClient (2.03s)
    server.go:252: CONFIG JSON: {"node_name":"node-05d8a142-cd02-931e-0c0b-8d29d8928cd9","node_id":"05d8a142-cd02-931e-0c0b-8d29d8928cd9","performance":{"raft_multiplier":1},"bootstrap":true,"server":true,"data_dir":"/var/folders/d6/3gfl83ks03sf7j34c0c9125r0000gn/T/TestRemoteClient181070277/data","segments":null,"disable_update_check":true,"log_level":"warn","bind_addr":"127.0.0.1","addresses":{},"ports":{"dns":11790,"http":11791,"https":11792,"serf_lan":11793,"serf_wan":11794,"server":11795},"acl":{"tokens":{}},"connect":{"ca_config":{"cluster_id":"11111111-2222-3333-4444-555555555555"},"enabled":true}}
    server.go:300: server stop failed with: exit status 1
    client_test.go:24: err: api unavailable
--- FAIL: TestRemoteClient_gzipUpgrade (0.00s)
    server.go:252: CONFIG JSON: {"node_name":"node-421dbe93-95b5-6f63-0141-d0bf1938a241","node_id":"421dbe93-95b5-6f63-0141-d0bf1938a241","performance":{"raft_multiplier":1},"bootstrap":true,"server":true,"data_dir":"/var/folders/d6/3gfl83ks03sf7j34c0c9125r0000gn/T/TestRemoteClient_gzipUpgrade981698144/data","segments":null,"disable_update_check":true,"log_level":"warn","bind_addr":"127.0.0.1","addresses":{},"ports":{"dns":11796,"http":11797,"https":11798,"serf_lan":11799,"serf_wan":11800,"server":11801},"acl":{"tokens":{}},"connect":{"ca_config":{"cluster_id":"11111111-2222-3333-4444-555555555555"},"enabled":true}}
    client_test.go:48: err: failed starting command: pipe: too many open files
--- FAIL: TestConsul_stateLock (0.00s)
    server.go:252: CONFIG JSON: {"node_name":"node-ea858a21-cc04-a3a0-9096-ec903b47f211","node_id":"ea858a21-cc04-a3a0-9096-ec903b47f211","performance":{"raft_multiplier":1},"bootstrap":true,"server":true,"data_dir":"/var/folders/d6/3gfl83ks03sf7j34c0c9125r0000gn/T/TestConsul_stateLock694110271/data","segments":null,"disable_update_check":true,"log_level":"warn","bind_addr":"127.0.0.1","addresses":{},"ports":{"dns":11802,"http":11803,"https":11804,"serf_lan":11805,"serf_wan":11806,"server":11807},"acl":{"tokens":{}},"connect":{"ca_config":{"cluster_id":"11111111-2222-3333-4444-555555555555"},"enabled":true}}
    client_test.go:89: err: failed starting command: pipe: too many open files
--- FAIL: TestConsul_destroyLock (0.00s)
    server.go:252: CONFIG JSON: {"node_name":"node-360a0b1d-ef92-8742-0dfb-11ffd1596483","node_id":"360a0b1d-ef92-8742-0dfb-11ffd1596483","performance":{"raft_multiplier":1},"bootstrap":true,"server":true,"data_dir":"/var/folders/d6/3gfl83ks03sf7j34c0c9125r0000gn/T/TestConsul_destroyLock530247314/data","segments":null,"disable_update_check":true,"log_level":"warn","bind_addr":"127.0.0.1","addresses":{},"ports":{"dns":11808,"http":11809,"https":11810,"serf_lan":11811,"serf_wan":11812,"server":11813},"acl":{"tokens":{}},"connect":{"ca_config":{"cluster_id":"11111111-2222-3333-4444-555555555555"},"enabled":true}}
    client_test.go:118: err: failed starting command: pipe: too many open files
--- FAIL: TestConsul_lostLock (0.00s)
    server.go:252: CONFIG JSON: {"node_name":"node-3c6c2ad5-3ef8-7b98-1cd7-e0f8e226ae37","node_id":"3c6c2ad5-3ef8-7b98-1cd7-e0f8e226ae37","performance":{"raft_multiplier":1},"bootstrap":true,"server":true,"data_dir":"/var/folders/d6/3gfl83ks03sf7j34c0c9125r0000gn/T/TestConsul_lostLock274379977/data","segments":null,"disable_update_check":true,"log_level":"warn","bind_addr":"127.0.0.1","addresses":{},"ports":{"dns":11814,"http":11815,"https":11816,"serf_lan":11817,"serf_wan":11818,"server":11819},"acl":{"tokens":{}},"connect":{"ca_config":{"cluster_id":"11111111-2222-3333-4444-555555555555"},"enabled":true}}
    client_test.go:161: err: failed starting command: pipe: too many open files
--- FAIL: TestConsul_lostLockConnection (0.00s)
    server.go:252: CONFIG JSON: {"node_name":"node-f91a0030-9f66-3d89-2fe2-2934c304601c","node_id":"f91a0030-9f66-3d89-2fe2-2934c304601c","performance":{"raft_multiplier":1},"bootstrap":true,"server":true,"data_dir":"/var/folders/d6/3gfl83ks03sf7j34c0c9125r0000gn/T/TestConsul_lostLockConnection242266004/data","segments":null,"disable_update_check":true,"log_level":"warn","bind_addr":"127.0.0.1","addresses":{},"ports":{"dns":11820,"http":11821,"https":11822,"serf_lan":11823,"serf_wan":11824,"server":11825},"acl":{"tokens":{}},"connect":{"ca_config":{"cluster_id":"11111111-2222-3333-4444-555555555555"},"enabled":true}}
    client_test.go:214: err: failed starting command: pipe: too many open files
FAIL
FAIL	github.com/hashicorp/terraform/backend/remote-state/consul	11.543s
FAIL
$

This is on macOS with Consul 1.7.4., in case that matters.

@rboyer
Copy link
Member

rboyer commented Sep 11, 2020

You might want to check out what your system max file handle value is set to and possibly raise it. Usually on my linux machines I bump it to a huge number so these artificial limits don't get awkwardly in the way across the board.

# /etc/security/limits.conf
myusername soft nofile 500000
myusername hard nofile 500000

@dnephin
Copy link

dnephin commented Sep 11, 2020

We recently merged hashicorp/consul#8647 which should address the testing.T problem. As of that PR testutil.NewTestServerConfigT accepts an interface with 4 methods. It should be possible to implement a struct that handles those 4 methods so that the test server can be called from TestMain.

@LorbusChris
Copy link
Author

I'm cleaning out my backlog and this has no priority for me any longer. I'll leave the branch up, feel free to take the code as a reference.

@ghost
Copy link

ghost commented Oct 24, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants