datadog_integration_pagerduty breaks expected terraform contract, allows for creating same resource twice

[geota]

Terraform Version

$ terraform -v
terraform v0.12.26

Affected Resource(s)

Please list the resources as a list, for example:

• datadog_integration_pagerduty

Terraform Configuration Files

test/module_a/main.tf

resource "datadog_integration_pagerduty" "pd" {
  individual_services = true
  subdomain           = "test-geota"
}

test/module_b/main.tf

resource "datadog_integration_pagerduty" "pd" {
  individual_services = true
  subdomain           = "test-geota"
}

Debug Output

Module a

❯ 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:

  # datadog_integration_pagerduty.pd will be created
  + resource "datadog_integration_pagerduty" "pd" {
      + id                  = (known after apply)
      + individual_services = true
      + subdomain           = "test-geota"
    }

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

datadog_integration_pagerduty.pd: Creating...
datadog_integration_pagerduty.pd: Still creating... [10s elapsed]
datadog_integration_pagerduty.pd: Creation complete after 11s [id=test-geota]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Module b

❯ 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:

  # datadog_integration_pagerduty.pd will be created
  + resource "datadog_integration_pagerduty" "pd" {
      + id                  = (known after apply)
      + individual_services = true
      + subdomain           = "test-geota"
    }

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

datadog_integration_pagerduty.pd: Creating...
datadog_integration_pagerduty.pd: Creation complete after 7s [id=test-geota]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Expected Behavior

Terraform datadog provider should fail to create the resource a second time. Terraform datadog provider should not implicitly adopt or recreate the existing datadog_inteegration_pagerduty resource.

Actual Behavior

Terraform silently succeeds to create and/or implicitly imports the resource even though it already exists. This leads to a shared resource with no implicit action taken by the user to acknowledge it is a shared resource. This then leads to unexpected behavior when the resource is deleted by one of the terraform modules causing the shared resource gets deleted across all of the dependencies which can have wide-ranging impact in a large organization (it will delete all the PD / Datadog integration service entries and leads to ops team flying blind).

Steps to Reproduce

Please list the steps required to reproduce the issue, for example:

1. Create two distinct modules (tf provided above) that create datadog_integration_pagerduty resources pointed to the same subdomain
2. Run terraform apply in both. Watch them both succeed.
3. Run terraform destroy in one. Watch it delete the resource from both.

[therve]

Thanks for the great explanation. The integration resource is effectively a singleton, so the fit into terraform is tricky indeed. I'm thinking that we could disable deletion of the resource anyway. The main point is to delete the services behind, but if individual_services is set then they will deleted with the associated resources anyway.

[skang0601]

I've opened up a MR to address the above issue: https://github.com/terraform-providers/terraform-provider-datadog/pull/565

My thought process was to include an optional flag to enforce that the integration wouldn't be deleted if it already exists for backward compatibility. It feels like that should be the default option but wanted to ease folks in.

[therve]

Doesn't your PR raise an error if the integration exists? It seems that 1) It's prone to race condition 2) It will fail to update if the subdomain changes. I think I'd favor a flag that ignore deletion.

[therve]

I talked to the team responsible for that endpoint, and it looks like this is some legacy resource. Apparently you shouldn't need that resource at all, as the integration is now setup via web hooks in PagerDuty. You can use the service resource to manage services, but the main resource isn't required. We need to update the documentation to cover that, but does that makes sense @geota?

[skang0601]

@therve Yes it'll currently throw an error if the integration exist but just ignoring deletion makes sense. That seems to be an easy enough to accommodate in the PR.
But in regards to your other response, would just a documentation update suffice in this case since users of the provider could still accidentally destroy the integration if they use this resource. Curious to know your thoughts on next steps and whether updating the PR is worthwhile.

[therve]

I think the first step is to remove the deletion, and to deprecate the resource. Then update documentation to remove the reference to that resource.

[geota]

@therve
I apologize for the delay in responding.

Removing the deletion, deprecating the resource, and clearly updating the docs seems sufficient imho.