fastly / terraform-provider-fastly

Terraform Fastly provider
https://www.terraform.io/docs/providers/fastly/
Mozilla Public License 2.0
119 stars 138 forks source link

Conflict when recreating a snippet #628

Open sjparkinson opened 1 year ago

sjparkinson commented 1 year ago

Terraform Version

Terraform v1.3.6
on darwin_arm64

Affected Resource(s)

Terraform Configuration Files

From

resource "fastly_service_vcl" "demo" {
  name = "demofastly"

  ...

  dynamicsnippet {
    name = "Waf_Prefetch"
    ...
  }
}

To

resource "fastly_service_vcl" "demo" {
  name = "demofastly"

  ...

  snippet {
    name = "WAF_Prefetch"
    ...
  }
}

Debug Output

╷
│ Error: 409 - Conflict:
│ 
│     Title:  Duplicate record
│     Detail: Duplicate snippet: 'WAF_Snippet'
│ 
│   with module.service.fastly_service_vcl.this,
│   on .terraform/modules/service/terraform/modules/services/single-origin-service/main.tf line 150, in resource "fastly_service_vcl" "this":
│  150: resource "fastly_service_vcl" "this" {
│ 
╵
ERRO[0011] Terraform invocation failed in /home/circleci/project/terraform/services/service 
ERRO[0011] 1 error occurred:
    * exit status 1

Exited with code exit status 1

Expected Behavior

In order to avoid a conflict when converting a dynamic snippet into a standard snippet with the same name:

  1. First delete the dynamic snippet
  2. Then create the standard snippet

Or vice versa if going in the other direction, the key being for any snippet deletions to occur before snippet creations to avoid conflicts if the name is identical.

Actual Behavior

The API request to create the standard snippet occurs before the request to delete the dynamic snippet.

Important Factoids

We're seeing this in relation to converting the WAF_Prefetch Legacy WAF dynamic snippet into a standard snippet.

Integralist commented 1 year ago

Hi @sjparkinson

Thanks for opening this issue. I'll investigate this further as it looks like we should 'delete', then 'add', then 'update': https://github.com/fastly/terraform-provider-fastly/blob/072d14c0d0926c047e67a7a3b46331e294ac37d2/fastly/service_crud_attribute_definition.go#L102-L127

Integralist commented 1 year ago

OK, so the problem is that we currently process versioned snippets first before dynamic snippets...

https://github.com/fastly/terraform-provider-fastly/blob/072d14c0d0926c047e67a7a3b46331e294ac37d2/fastly/resource_fastly_service_vcl.go#L54-L55

So if you were to define a versioned snippet and then switched to a dynamic snippet, the order works. This is because the provider will delete the versioned snippet first, and then move to the creation of the dynamic snippet. If (as you have in your example) start with a dynamic snippet and switch to a versioned snippet, then the order breaks because the provider will end up creating the versioned snippet first, which causes the API conflict to be raised.

We can't just move the NewServiceDynamicSnippet call above NewServiceSnippet because ultimately the same problem exists in reverse. So for example, if we did switch the calls in the provider, and a user started with a versioned snippet and wants to migrate to a dynamic snippet, then the order fails again.

I don't know of a way to resolve this without a significant rewrite of the snippets architecture inside of the provider. I'll leave this issue open because it is a bug in the provider, and in the meantime, I'll consult internally to see if anyone has any other suggestions.

For now, you'll need to do this as two separate plan/apply steps to avoid the conflict.

sjparkinson commented 1 year ago

For now, you'll need to do this as two separate plan/apply steps to avoid the conflict.

That's helpful, and sort of how we've unpicked ourselves so far. Thank you.

I wouldn't mind seeing non-unique snippet names if you want to pitch for another sort of unique ID for them in the API!

Integralist commented 1 year ago

@sjparkinson Well, there is a new Terraform plugin framework (which replaces the current v2 SDK).

For us to support the new framework will be a significant piece of work, and definitely a new major release (due to various breaking changes), and so as part of that work (no ETA on that of course, we have not scheduled anything yet) I would expect to consolidate dynamicsnippet and snippet into a single snippet block type.

Then we would be able to expose a dynamic boolean attribute to toggle between dynamic and versioned snippets.

The underlying API for both types is the same and all the API client does is pass a flag to toggle between dynamic and versioned: see the API documentation.

Integralist commented 1 year ago

👋🏻 @sjparkinson I forgot to mention if you wanted to follow along with my (personal) progress on a Fastly provider that supports the new HashiCorp 'framework', then I can refer you to https://github.com/Integralist/terraform-provider-fastly-framework (in that project see the CHANGELOG and DEVELOPMENT files for extra context/info).