Create node and service if deleted outside of terraform #69

Gufran · 2018-10-03T14:40:45Z

This changeset fixes an edge case of #33 where if terraform state has consul catalog resource but consul catalog is actually empty the provider will raise an error rather than creating the node or service.

Acceptance Tests:

→ make testacc TEST=./consul TESTARGS="-run=TestAccConsulNode_basic"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./consul -v -run=TestAccConsulNode_basic -timeout 120m
=== RUN   TestAccConsulNode_basic
--- PASS: TestAccConsulNode_basic (0.06s)
PASS
ok      github.com/terraform-providers/terraform-provider-consul/consul 0.107s

→ make testacc TEST=./consul TESTARGS="-run=TestAccConsulService_basic"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./consul -v -run=TestAccConsulService_basic -timeout 120m
=== RUN   TestAccConsulService_basic
--- PASS: TestAccConsulService_basic (0.07s)
=== RUN   TestAccConsulService_basicModify
--- PASS: TestAccConsulService_basicModify (0.08s)
PASS
ok      github.com/terraform-providers/terraform-provider-consul/consul 0.167s

→ make testacc TEST=./consul TESTARGS="-run=TestAccConsulIntention_basic"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./consul -v -run=TestAccConsulIntention_basic -timeout 120m
=== RUN   TestAccConsulIntention_basic
--- PASS: TestAccConsulIntention_basic (0.08s)
PASS
ok      github.com/terraform-providers/terraform-provider-consul/consul (cached)

pearkes · 2018-10-03T14:45:54Z

That is definitely problematic behavior! Do you mind adding tests for this case? There are some test instructions in the bottom section of the README.

To my understanding this is a relatively common case in larger providers if you're looking for an example of how to write a test for it.

Gufran · 2018-10-03T15:12:48Z

@pearkes I'd love to add tests but I couldn't find an example which deal with pre-populated state. can you help me please?

kumarmunish · 2018-10-09T10:15:46Z

I'm also facing the same issue, is there any update about it's fix.

remilapeyre · 2018-12-27T01:46:59Z

Hi @Gufran, thanks for the PR! I'm pretty sure I got bite by this bug but I add no time to find the cause when it happened to me.

For the tests, you can execute arbitrary code in TestStep.PreConfig, here's an example:

commit 275a30d8e8e7dae825d5f23484932586a8f827c4
Author: Rémi Lapeyre <remi.lapeyre@henki.fr>
Date:   Thu Dec 27 02:43:06 2018 +0100

    Add test to recreate node and services when deleted outside Terraform

diff --git a/consul/resource_consul_catalog_entry_test.go b/consul/resource_consul_catalog_entry_test.go
index f967845..3677afe 100644
--- a/consul/resource_consul_catalog_entry_test.go
+++ b/consul/resource_consul_catalog_entry_test.go
@@ -18,7 +18,7 @@ func TestAccConsulCatalogEntry_basic(t *testing.T) {
 			resource.TestStep{
 				Config: testAccConsulCatalogEntryConfig,
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheckConsulCatalogEntryExists(),
+					testAccCheckConsulCatalogEntryExists("google"),
 					testAccCheckConsulCatalogEntryValue("consul_catalog_entry.app", "address", "127.0.0.1"),
 					testAccCheckConsulCatalogEntryValue("consul_catalog_entry.app", "node", "bastion"),
 					testAccCheckConsulCatalogEntryValue("consul_catalog_entry.app", "service.#", "1"),
@@ -45,7 +45,7 @@ func TestAccConsulCatalogEntry_extremove(t *testing.T) {
 				Config:             testAccConsulCatalogEntryConfig,
 				ExpectNonEmptyPlan: true,
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheckConsulCatalogEntryExists(),
+					testAccCheckConsulCatalogEntryExists("google"),
 					testAccCheckConsulCatalogEntryValue("consul_catalog_entry.app", "address", "127.0.0.1"),
 					testAccCheckConsulCatalogEntryValue("consul_catalog_entry.app", "node", "bastion"),
 					testAccCheckConsulCatalogEntryValue("consul_catalog_entry.app", "service.#", "1"),
@@ -103,7 +103,7 @@ func testAccCheckConsulCatalogEntryDeregister(node string) resource.TestCheckFun
 	}
 }
 
-func testAccCheckConsulCatalogEntryExists() resource.TestCheckFunc {
+func testAccCheckConsulCatalogEntryExists(service string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		catalog := testAccProvider.Meta().(*consulapi.Client).Catalog()
 		qOpts := consulapi.QueryOptions{}
@@ -111,9 +111,9 @@ func testAccCheckConsulCatalogEntryExists() resource.TestCheckFunc {
 		if err != nil {
 			return err
 		}
-		_, ok := services["google"]
+		_, ok := services[service]
 		if !ok {
-			return fmt.Errorf("Service does not exist: %#v", "google")
+			return fmt.Errorf("Service does not exist: %#v", service)
 		}
 		return nil
 	}
diff --git a/consul/resource_consul_node_test.go b/consul/resource_consul_node_test.go
index b2ddfe7..e553d87 100644
--- a/consul/resource_consul_node_test.go
+++ b/consul/resource_consul_node_test.go
@@ -23,13 +23,28 @@ func TestAccConsulNode_basic(t *testing.T) {
 					testAccCheckConsulNodeValue("consul_node.foo", "name", "foo"),
 				),
 			},
+			// Removing the node from the catalog should make Terraform recreate it
+			resource.TestStep{
+				PreConfig: func() {
+					catalog := testAccProvider.Meta().(*consulapi.Client).Catalog()
+					wOpts := &consulapi.WriteOptions{}
+					dereg := &consulapi.CatalogDeregistration{Node: "foo"}
+					_, err := catalog.Deregister(dereg, wOpts)
+					if err != nil {
+						t.Errorf("err: %v", err)
+					}
+				},
+				Config: testAccConsulNodeConfigBasic,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckConsulNodeExists(),
+				),
+			},
 		},
 	})
 }
 
 func TestAccConsulNode_nodeMeta(t *testing.T) {
 	resource.Test(t, resource.TestCase{
-		PreCheck:     func() {},
 		Providers:    testAccProviders,
 		CheckDestroy: testAccCheckConsulNodeDestroy,
 		Steps: []resource.TestStep{
@@ -89,7 +104,7 @@ func testAccCheckConsulNodeExists() resource.TestCheckFunc {
 				return nil
 			}
 		}
-		return fmt.Errorf("Service does not exist: %#v", "google")
+		return fmt.Errorf("Node does not exist: foo")
 	}
 }
 
diff --git a/consul/resource_consul_service_test.go b/consul/resource_consul_service_test.go
index b758f94..f6cd8a3 100644
--- a/consul/resource_consul_service_test.go
+++ b/consul/resource_consul_service_test.go
@@ -28,6 +28,25 @@ func TestAccConsulService_basic(t *testing.T) {
 					resource.TestCheckResourceAttr("consul_service.example", "tags.0", "tag0"),
 				),
 			},
+			// Removing the service from Consul should make Terraform recreate it
+			resource.TestStep{
+				PreConfig: func() {
+					catalog := testAccProvider.Meta().(*consulapi.Client).Catalog()
+					wOpts := &consulapi.WriteOptions{}
+					dereg := &consulapi.CatalogDeregistration{
+						Node:      "compute-example",
+						ServiceID: "example",
+					}
+					_, err := catalog.Deregister(dereg, wOpts)
+					if err != nil {
+						t.Errorf("err: %v", err)
+					}
+				},
+				Config: testAccConsulServiceConfigBasic,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckConsulCatalogEntryExists("example"),
+				),
+			},
 		},
 	})
 }

remilapeyre · 2018-12-27T15:04:17Z

I thought other resources could have the same issue, tried a random one and sure enough, consul_intention is subject to the same kind of bug:

➜  terraform-provider-consul git:(catalog-fix) cat example.tf
resource "consul_intention" "database" {
  source_name      = "api"
  destination_name = "db"
  action           = "allow"
}
➜  terraform-provider-consul git:(catalog-fix) terraform apply

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  + consul_intention.database
      id:               <computed>
      action:           "allow"
      destination_name: "db"
      source_name:      "api"


Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

consul_intention.database: Creating...
  action:           "" => "allow"
  destination_name: "" => "db"
  source_name:      "" => "api"
consul_intention.database: Creation complete after 0s (ID: 2056a436-8578-9dc4-3a9d-eaa257bfd628)

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
➜  terraform-provider-consul git:(catalog-fix) consul intention delete api db
Intention deleted.
➜  terraform-provider-consul git:(catalog-fix) terraform apply
consul_intention.database: Refreshing state... (ID: 2056a436-8578-9dc4-3a9d-eaa257bfd628)

Error: Error refreshing state: 1 error(s) occurred:

* consul_intention.database: 1 error(s) occurred:

* consul_intention.database: consul_intention.database: unexpected EOF


panic: runtime error: invalid memory address or nil pointer dereference
2018-12-27T16:02:34.913+0100 [DEBUG] plugin.terraform-provider-consul: [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1822e02]
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul:
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul: goroutine 8 [running]:
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul: github.com/terraform-providers/terraform-provider-consul/consul.resourceConsulIntentionRead(0xc000201960, 0x1a04cc0, 0xc0001ec3c0, 0xc000201960, 0x0)
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul:  /Users/remi/go/src/github.com/terraform-providers/terraform-provider-consul/consul/resource_consul_intention.go:175 +0x242
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul: github.com/terraform-providers/terraform-provider-consul/vendor/github.com/hashicorp/terraform/helper/schema.(*Resource).Refresh(0xc0002677a0, 0xc0003bef50, 0x1a04cc0, 0xc0001ec3c0, 0xc0003ab510, 0x10bf501, 0x189b1e0)
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul:  /Users/remi/go/src/github.com/terraform-providers/terraform-provider-consul/vendor/github.com/hashicorp/terraform/helper/schema/resource.go:352 +0x160
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul: github.com/terraform-providers/terraform-provider-consul/vendor/github.com/hashicorp/terraform/helper/schema.(*Provider).Refresh(0xc000267810, 0xc0003bef00, 0xc0003bef50, 0xc0003cc400, 0x18, 0x2539000)
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul:  /Users/remi/go/src/github.com/terraform-providers/terraform-provider-consul/vendor/github.com/hashicorp/terraform/helper/schema/provider.go:308 +0x92
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul: github.com/terraform-providers/terraform-provider-consul/vendor/github.com/hashicorp/terraform/plugin.(*ResourceProviderServer).Refresh(0xc0001a4140, 0xc00052ecb0, 0xc00052ef00, 0x0, 0x0)
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul:  /Users/remi/go/src/github.com/terraform-providers/terraform-provider-consul/vendor/github.com/hashicorp/terraform/plugin/resource_provider.go:549 +0x4e
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul: reflect.Value.call(0xc0000a35c0, 0xc0003c00e0, 0x13, 0x1a312c0, 0x4, 0xc000079f18, 0x3, 0x3, 0xc0000c1700, 0x0, ...)
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul:  /usr/local/Cellar/go/1.11.4/libexec/src/reflect/value.go:447 +0x454
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul: reflect.Value.Call(0xc0000a35c0, 0xc0003c00e0, 0x13, 0xc0002df718, 0x3, 0x3, 0x0, 0x0, 0x0)
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul:  /usr/local/Cellar/go/1.11.4/libexec/src/reflect/value.go:308 +0xa4
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul: net/rpc.(*service).call(0xc0003e6a40, 0xc0000d6730, 0xc0000be700, 0xc0000be870, 0xc000198f00, 0xc000161e00, 0x189b1a0, 0xc00052ecb0, 0x16, 0x189b1e0, ...)
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul:  /usr/local/Cellar/go/1.11.4/libexec/src/net/rpc/server.go:384 +0x14e
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul: created by net/rpc.(*Server).ServeCodec
2018-12-27T16:02:34.914+0100 [DEBUG] plugin.terraform-provider-consul:  /usr/local/Cellar/go/1.11.4/libexec/src/net/rpc/server.go:481 +0x47e
2018-12-27T16:02:34.916+0100 [DEBUG] plugin: plugin process exited: path=/Users/remi/.terraform.d/plugins/terraform-provider-consul
2018/12/27 16:02:34 [ERROR] root: eval: *terraform.EvalRefresh, err: consul_intention.database: unexpected EOF
2018/12/27 16:02:34 [ERROR] root: eval: *terraform.EvalSequence, err: consul_intention.database: unexpected EOF
2018/12/27 16:02:34 [TRACE] [walkRefresh] Exiting eval tree: consul_intention.database
2018/12/27 16:02:34 [TRACE] dag/walk: upstream errored, not walking "provider.consul (close)"
2018/12/27 16:02:34 [DEBUG] plugin: waiting for all plugin processes to complete...
2018-12-27T16:02:34.916+0100 [WARN ] plugin: error closing client during Kill: err="connection is shut down"



!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

Terraform crashed! This is always indicative of a bug within Terraform.
A crash log has been placed at "crash.log" relative to your current
working directory. It would be immensely helpful if you could please
report the crash with Terraform[1] so that we can fix this.

When reporting bugs, please include your terraform version. That
information is available on the first line of crash.log. You can also
get it by running 'terraform --version' on the command line.

[1]: https://github.com/hashicorp/terraform/issues

!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

We should add tests for this on the others resources.

This changeset fixes an edge case of hashicorp#33 where if terraform state has consul catalog resource but consul catalog is actually empty the provider will raise an error rather than creating the node or service.

Gufran · 2019-01-05T06:18:37Z

Thanks @remilapeyre for the help with tests. I've also patched the intention provider.

remilapeyre

Thanks for adding the tests, this look good to me, with the small change I indicated.

Can you just push a new commit on top of your change though rather than amending your commit and force push next time? It makes review easier and keep the discussion during previous reviews meaningfull.

consul/resource_consul_node_test.go

consul/resource_consul_service_test.go

Gufran · 2019-01-07T16:54:31Z

@remilapeyre Done!

remilapeyre

Thanks!

Gufran · 2019-01-29T09:10:51Z

@remilapeyre any idea on when will this be merged?

remilapeyre · 2019-01-29T09:14:20Z

Hi @Gufran, I will make a final review and merge tonight.

remilapeyre · 2019-01-29T12:48:13Z

Thanks @Gufran for you help 🚀

ghost added the size/XS label Oct 3, 2018

remilapeyre mentioned this pull request Dec 27, 2018

[Bug] consul_intention makes Terraform panics if the resource has been deleted #81

Closed

Create node and service if deleted outside of terraform

ddbefd3

This changeset fixes an edge case of hashicorp#33 where if terraform state has consul catalog resource but consul catalog is actually empty the provider will raise an error rather than creating the node or service.

Gufran force-pushed the catalog-fix branch from 5c7bfa4 to ddbefd3 Compare January 5, 2019 06:15

ghost added size/M and removed size/XS labels Jan 5, 2019

remilapeyre reviewed Jan 7, 2019

View reviewed changes

consul/resource_consul_node_test.go Outdated Show resolved Hide resolved

consul/resource_consul_service_test.go Outdated Show resolved Hide resolved

remilapeyre mentioned this pull request Jan 7, 2019

Create consul_intention when deleted outside Terraform #82

Closed

Write PreConfig logic in separate functions

67a8879

remilapeyre approved these changes Jan 7, 2019

View reviewed changes

remilapeyre added the bug label Jan 29, 2019

Update changelog

1b3799b

remilapeyre merged commit 194fff3 into hashicorp:master Jan 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create node and service if deleted outside of terraform #69

Create node and service if deleted outside of terraform #69

Gufran commented Oct 3, 2018 •

edited

pearkes commented Oct 3, 2018 •

edited

Gufran commented Oct 3, 2018

kumarmunish commented Oct 9, 2018

remilapeyre commented Dec 27, 2018

remilapeyre commented Dec 27, 2018

Gufran commented Jan 5, 2019

remilapeyre left a comment

Gufran commented Jan 7, 2019

remilapeyre left a comment

Gufran commented Jan 29, 2019

remilapeyre commented Jan 29, 2019

remilapeyre commented Jan 29, 2019

Create node and service if deleted outside of terraform #69

Create node and service if deleted outside of terraform #69

Conversation

Gufran commented Oct 3, 2018 • edited

pearkes commented Oct 3, 2018 • edited

Gufran commented Oct 3, 2018

kumarmunish commented Oct 9, 2018

remilapeyre commented Dec 27, 2018

remilapeyre commented Dec 27, 2018

Gufran commented Jan 5, 2019

remilapeyre left a comment

Choose a reason for hiding this comment

Gufran commented Jan 7, 2019

remilapeyre left a comment

Choose a reason for hiding this comment

Gufran commented Jan 29, 2019

remilapeyre commented Jan 29, 2019

remilapeyre commented Jan 29, 2019

Gufran commented Oct 3, 2018 •

edited

pearkes commented Oct 3, 2018 •

edited