Add import support to consul_key_prefix

[remilapeyre]

consul_key_prefix protect operators against unwanted key deletions by
requiring the choosen path to be empty.

While this is a nice default behavior, it can be intrusive to force it.
This change makes add import support to consul_key_prefix so the
operator can manually initialize Terraform state and override the default
behavior.

Close #77

[pearkes]

I'm not sure if this is intended, but the behavior here is a little bit strange to me:

https://gist.github.com/pearkes/2da3d298119b7148d3e216e315d6bd49

I feel like that terraform apply should be a no-op change, and not show a diff. What do you think, is this expected behavior?

[remilapeyre]

I see three possibilities to handle this case:

1. do nothing during the import as I'm doing now which result on a change in the next apply,
2. delete keys that should not be there and add the missing one, a very disruptive import, I don't think this is a good idea,
3. Only authorize the import if all keys correctly defined and the import will result in a no-op change afterward, raise an error if not. In the case you attached, this means the import would fail.

I think 3. is the best solution, the least susceptible to surprise the user. It would still be a very good improvement for #77.

What do you think about it?

[pearkes]

I agree 3 is the right choice there! Is there existing examples of this "import validation" in other providers? If that is a common UX lets do that.

[remilapeyre]

I made some research, none of https://github.com/terraform-providers/terraform-provider-github, https://github.com/terraform-providers/terraform-provider-aws/ or https://github.com/terraform-providers/terraform-provider-google do this.

They either just use the id directly or parse it and sometimes fetch some information like in https://github.com/terraform-providers/terraform-provider-aws/blob/d73361710bf138fd9e7a3e9724668f4c60951b81/aws/resource_aws_codedeploy_app.go#L23-L52 but they never look at the user given configuration to make sure the import will result in a no-op plan.

Here's an example with aws_instance that result in a destructive operation:

$ cat example.tf 
provider "aws" {
        region = "us-east-1"
}

resource "aws_instance" "test" {
  ami           = "ami-0f9e7e8867f55fd8e"
  instance_type = "t2.micro"
}
$ 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:

  + aws_instance.test
      id:                           <computed>
      ami:                          "ami-0f9e7e8867f55fd8e"
      arn:                          <computed>
      associate_public_ip_address:  <computed>
      availability_zone:            <computed>
      cpu_core_count:               <computed>
      cpu_threads_per_core:         <computed>
      ebs_block_device.#:           <computed>
      ephemeral_block_device.#:     <computed>
      get_password_data:            "false"
      host_id:                      <computed>
      instance_state:               <computed>
      instance_type:                "t2.micro"
      ipv6_address_count:           <computed>
      ipv6_addresses.#:             <computed>
      key_name:                     <computed>
      network_interface.#:          <computed>
      network_interface_id:         <computed>
      password_data:                <computed>
      placement_group:              <computed>
      primary_network_interface_id: <computed>
      private_dns:                  <computed>
      private_ip:                   <computed>
      public_dns:                   <computed>
      public_ip:                    <computed>
      root_block_device.#:          <computed>
      security_groups.#:            <computed>
      source_dest_check:            "true"
      subnet_id:                    <computed>
      tenancy:                      <computed>
      volume_tags.%:                <computed>
      vpc_security_group_ids.#:     <computed>


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 

aws_instance.test: Creating...
  ami:                          "" => "ami-0f9e7e8867f55fd8e"
  arn:                          "" => "<computed>"
  associate_public_ip_address:  "" => "<computed>"
  availability_zone:            "" => "<computed>"
  cpu_core_count:               "" => "<computed>"
  cpu_threads_per_core:         "" => "<computed>"
  ebs_block_device.#:           "" => "<computed>"
  ephemeral_block_device.#:     "" => "<computed>"
  get_password_data:            "" => "false"
  host_id:                      "" => "<computed>"
  instance_state:               "" => "<computed>"
  instance_type:                "" => "t2.micro"
  ipv6_address_count:           "" => "<computed>"
  ipv6_addresses.#:             "" => "<computed>"
  key_name:                     "" => "<computed>"
  network_interface.#:          "" => "<computed>"
  network_interface_id:         "" => "<computed>"
  password_data:                "" => "<computed>"
  placement_group:              "" => "<computed>"
  primary_network_interface_id: "" => "<computed>"
  private_dns:                  "" => "<computed>"
  private_ip:                   "" => "<computed>"
  public_dns:                   "" => "<computed>"
  public_ip:                    "" => "<computed>"
  root_block_device.#:          "" => "<computed>"
  security_groups.#:            "" => "<computed>"
  source_dest_check:            "" => "true"
  subnet_id:                    "" => "<computed>"
  tenancy:                      "" => "<computed>"
  volume_tags.%:                "" => "<computed>"
  vpc_security_group_ids.#:     "" => "<computed>"
aws_instance.test: Still creating... (10s elapsed)
aws_instance.test: Still creating... (20s elapsed)
aws_instance.test: Still creating... (30s elapsed)
aws_instance.test: Creation complete after 31s (ID: i-0fdf2fc86f576fe1e)

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
$ sed -e s/ami-0f9e7e8867f55fd8e/ami-4bf3d731/ -i '' example.tf 
$ rm terraform.tfstate
$ terraform import aws_instance.test i-0fdf2fc86f576fe1e
aws_instance.test: Importing from ID "i-0fdf2fc86f576fe1e"...
aws_instance.test: Import complete!
  Imported aws_instance (ID: i-0fdf2fc86f576fe1e)
aws_instance.test: Refreshing state... (ID: i-0fdf2fc86f576fe1e)

Import successful!

The resources that were imported are shown above. These resources are now in
your Terraform state and will henceforth be managed by Terraform.

$ terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.

aws_instance.test: Refreshing state... (ID: i-0fdf2fc86f576fe1e)

------------------------------------------------------------------------

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

Terraform will perform the following actions:

-/+ aws_instance.test (new resource required)
      id:                           "i-0fdf2fc86f576fe1e" => <computed> (forces new resource)
      ami:                          "ami-0f9e7e8867f55fd8e" => "ami-4bf3d731" (forces new resource)
      arn:                          "arn:aws:ec2:us-east-1:129757167202:instance/i-0fdf2fc86f576fe1e" => <computed>
      associate_public_ip_address:  "true" => <computed>
      availability_zone:            "us-east-1b" => <computed>
      cpu_core_count:               "1" => <computed>
      cpu_threads_per_core:         "1" => <computed>
      ebs_block_device.#:           "0" => <computed>
      ephemeral_block_device.#:     "0" => <computed>
      get_password_data:            "false" => "false"
      host_id:                      "" => <computed>
      instance_state:               "running" => <computed>
      instance_type:                "t2.micro" => "t2.micro"
      ipv6_address_count:           "" => <computed>
      ipv6_addresses.#:             "0" => <computed>
      key_name:                     "" => <computed>
      network_interface.#:          "0" => <computed>
      network_interface_id:         "eni-0d2c3aa6c2ea791f0" => <computed>
      password_data:                "" => <computed>
      placement_group:              "" => <computed>
      primary_network_interface_id: "eni-0d2c3aa6c2ea791f0" => <computed>
      private_dns:                  "ip-172-31-81-93.ec2.internal" => <computed>
      private_ip:                   "172.31.81.93" => <computed>
      public_dns:                   "ec2-54-80-47-180.compute-1.amazonaws.com" => <computed>
      public_ip:                    "54.80.47.180" => <computed>
      root_block_device.#:          "1" => <computed>
      security_groups.#:            "1" => <computed>
      source_dest_check:            "true" => "true"
      subnet_id:                    "subnet-d754c3f9" => <computed>
      tenancy:                      "default" => <computed>
      volume_tags.%:                "0" => <computed>
      vpc_security_group_ids.#:     "1" => <computed>


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

------------------------------------------------------------------------

Note: You didn't specify an "-out" parameter to save this plan, so Terraform
can't guarantee that exactly these actions will be performed if
"terraform apply" is subsequently run.

I still think it may be good idea though, Consul often store configuration that services will auto-reload on changes, a mistake can be costly, especially if those keys were not terraformed before as it may require to fetch them from backup.

[pearkes]

Interesting wonderful research @remilapeyre. With that being said I'm okay with 1 which I believe is the current approach, but also if you want to pursue 3 I'd also be okay with it!

[remilapeyre]

I suspect that one of the reasons that this not currently checked by any provider may be that they are not usually to diff and check states by hand.

I think they may do it more if it were automatic, a new boolean attribute ForceNoChange could be added to ResourceImporter (https://github.com/hashicorp/terraform/blob/d4ac68423c4998279f33404db46809d27a5c2362/helper/schema/resource_importer.go#L30).

This boolean could be checked on https://github.com/hashicorp/terraform/blob/50e2b1856e211dc9dfd80a51d4f7e4af0bdd0f28/helper/schema/provider.go#L358 to abort the import if it would not result on a no-op change.

If provider designers need a more customizable, two booleans could be used ForceNoChange and ForceNoDestroy, the first one making sure that the import will result in a no-op plan, the second that the changes, if there are some, will be in place and that the resource won't be destroyed.

This could be useful for stateful resources, if I want to import a RDS database it's not to get it destroyed right away, having an import resulting in a destroy-recreate for such resources is probably a user error. If we want to let the user override ForceNoChange and ForceNoDestroy, a new --force could be added to terraform import.

The best way to know whether this would be useful would be to ask other provider writers, what do you think?

[paultyng]

If you wanted to prevent destruction via the config to be safe you can code it that way:

resource "aws_instance" "test" {
  lifecycle {
    prevent_destroy = true
  }

  ami           = "ami-0f9e7e8867f55fd8e"
  instance_type = "t2.micro"
}