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

Attributes aren't refreshed if no attributes of that type exist in local state #629

Closed
jonnangle opened this issue Jan 5, 2023 · 7 comments · Fixed by #630
Closed

Attributes aren't refreshed if no attributes of that type exist in local state #629

jonnangle opened this issue Jan 5, 2023 · 7 comments · Fixed by #630

Comments

@jonnangle
Copy link

jonnangle commented Jan 5, 2023

Terraform Version

Terraform v1.3.3
on darwin_arm64

Affected Resource(s)

  • fastly_service_vcl

Terraform Configuration Files

resource "fastly_service_vcl" "cache-setting-test" {
  name = "cache-setting-test"
  domain {
      name = "cache-setting-test.example.com"
  }
  cache_setting {
    action          = "pass"
    cache_condition = "Do not cache non-render responses or non-200"
    name            = "redirect cache setting"
    stale_ttl       = 0
    ttl             = 0
  }
  condition {
    name      = "Do not cache non-render responses or non-200"
    priority  = 100
    statement = "beresp.status != 200 || req.url !~ \"^/render\""
    type      = "CACHE"
  }
}

It appears that most of the attribute handlers intentionally avoid refreshing if the local state has no attributes of that type - for example:

https://github.com/fastly/terraform-provider-fastly/blob/main/fastly/block_fastly_service_cachesetting.go#L90-L93

This can cause problems if the local state has become out of sync - e.g. if the service has been rolled back. Is this intentional?

Expected Behavior

A terraform refresh should update the local state with the complete current state of the Fastly service.

Actual Behavior

A terraform refresh does not pick up attributes from the service if there are no attributes of that type already existing in the local state.

Steps to Reproduce

  1. Create a service as v1 using the terraform config above
  2. Delete the cache_setting attribute block and terraform apply, creating v2 of the service
  3. Rollback the service to v1 via the Fastly UI
  4. Run terraform apply again - it will now report that no changes are needed, and so will not remove the cache_setting attribute. Running terraform refresh does not help.
@Integralist
Copy link
Collaborator

Hi @jonnangle

Thanks for opening this issue. With regards to...

This can cause problems if the local state has become out of sync - e.g. if the service has been rolled back. Is this intentional?

Yes, this logic was intentional, see the following PR for more details:
#593 (released back in October 2022).

If you're reverting to an older service version via the UI, then you're working outside of Terraform and that can cause problems because Terraform expects to manage the entire lifecycle of resources.

One option to get the refresh to work would be to do a reimport of the service after it has been reverted via the UI:

terraform import fastly_service_vcl.cache-setting-test <SERVICE_ID>

As this will cause the || d.Get("imported").(bool) logic fork to execute the relevant API call(s).

@jonnangle
Copy link
Author

Thanks for the quick reply @Integralist!

If you're reverting to an older service version via the UI, then you're working outside of Terraform and that can cause problems because Terraform expects to manage the entire lifecycle of resources.

I see what you mean, but the Terraform docs suggest that refreshing infrastructure state is a pretty core aspect of the tool - there's an implicit refresh before plan and apply for this reason.

It's not unusual to have to make changes to services outside of terraform during incidents, where you may not be able to wait for a CI pipeline to run. We've even had occasions where Fastly support have made changes to services on our behalf.

Also, we've bumped into problems with this even when working completely within terraform. For example, in the example above, if you remove the cache_setting attribute block and add a snippet with an invalid name in the same terraform run:

resource "fastly_service_vcl" "cache-setting-test" {
  name = "cache-setting-test"
  domain {
      name = "cache-setting-test.example.com"
  }
  snippet {
      name     = "Only cache /render responses"
      type     = "fetch"
      priority = 10
      content  = <<-EOT
        if ( beresp.status != 200 || req.url !~ "^/render" ) {
          return(pass);
        }
      EOT
    }
}

then it will fail with this error:

 Error: 400 - Bad Request:
│
│     Title:  Bad request
│     Detail: Name must start with a letter and contain only alphanumeric, underscore, hyphen, period, and space

But you'll find that you have a terraform state that now has an empty cache_setting:

❯ grep cache_setting terraform.tfstate
            "cache_setting": [],

even though the cache setting still exists on the live service on Fastly.

@Integralist
Copy link
Collaborator

Integralist commented Jan 5, 2023

Hi @jonnangle

Thanks for the reply and extra context, I appreciate you taking the time to provide this information.

Re:

It's not unusual to have to make changes to services outside of terraform during incidents, where you may not be able to wait for a CI pipeline to run.

That's understandable.

Re:

Also, we've bumped into problems with this even when working completely within terraform.

Thank you! I'll investigate further 👍🏻

With regards to the main issue reported, I'll have think about how we can handle this situation.

My initial thinking says maybe within the service resource's read function we can make an API call to get the currently 'active' service version. That way if it doesn't match what the currently tracked version is within the provider, then we might be able to update the state with a new internal attribute that ensures the API call for each resource is executed. A similar approach might be that we provide an explicit attribute to force API reads, that would be less implementation code for us and would be a simple extra line of code in your configuration for those times when you want to get a full refresh.

@jonnangle
Copy link
Author

Hi @Integralist - sounds great, thanks! I like the idea of checking the service version, hopefully that would allow the provider to detect all of these situations in a reliable way whilst still keeping the performance optimizations in place on the happy path.

@Integralist
Copy link
Collaborator

@jonnangle just for your reference:
#630

@Integralist
Copy link
Collaborator

Also, I've replicated the issue reported with regard to missing state values when there is an error but it's currently unclear to me how this bug is possible.

When running an apply with TF_LOG=TRACE I can see that we execute the API call to create the snippet first, which subsequently returns an API error and short-circuits the Terraform apply stage.

There's no reported call into the cache_setting resource (nor any API call for it made), so why Terraform deletes the cache_setting resource from the state is unclear to me as the provider shouldn't have reached that step once the error occurred.

I'll keep digging to see if I can make sense of this.

@Integralist
Copy link
Collaborator

Integralist commented Jan 9, 2023

re:

Also, I've replicated the issue reported with regard to missing state values when there is an error but it's currently unclear to me how this bug is possible.

I'm going to open a separate issue for this as it's actually unrelated.

UPDATE: #631

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