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

Conflict when recreating a snippet #628

Open
sjparkinson opened this issue Dec 21, 2022 · 5 comments
Open

Conflict when recreating a snippet #628

sjparkinson opened this issue Dec 21, 2022 · 5 comments

Comments

@sjparkinson
Copy link
Contributor

Terraform Version

Terraform v1.3.6
on darwin_arm64

Affected Resource(s)

  • fastly_service_vcl

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
Copy link
Collaborator

Hi @sjparkinson

Thanks for opening this issue. I'll investigate this further as it looks like we should 'delete', then 'add', then 'update':

for _, resource := range diffResult.Deleted {
resource := resource.(map[string]any)
err := h.handler.Delete(ctx, d, resource, serviceVersion, conn)
if err != nil {
return err
}
}
for _, resource := range diffResult.Added {
resource := resource.(map[string]any)
err := h.handler.Create(ctx, d, resource, serviceVersion, conn)
if err != nil {
return err
}
}
for _, resource := range diffResult.Modified {
resource := resource.(map[string]any)
modified := setDiff.Filter(resource, oldSet)
err := h.handler.Update(ctx, d, resource, modified, serviceVersion, conn)
if err != nil {
return err
}
}

@Integralist
Copy link
Collaborator

Integralist commented Jan 5, 2023

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

NewServiceSnippet(vclAttributes),
NewServiceDynamicSnippet(vclAttributes),

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
Copy link
Contributor Author

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
Copy link
Collaborator

@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
Copy link
Collaborator

👋🏻 @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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants