Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

redirectpizza_redirect cannot round-trip the destination.monitoring field default value for "Single destination" redirects #56

Closed
michael-sterling opened this issue May 20, 2024 · 2 comments · Fixed by #57

Comments

@michael-sterling
Copy link

michael-sterling commented May 20, 2024

Summary

For a "Single destination" redirect, the redirect.pizza API represents the destination field as a string, but the Terraform provider state represents it as an array of { url, expression, monitoring } objects.

The default value of the monitoring property for a destination object in Terraform is specified here to be inherit. But when read from the API, the value of "monitoring" is empty here -- which will cause the terraform plan to try to change the empty value to inherit, which the API will ignore because there is only one destination, which will cause Terraform to read the "empty" value from the API again, etc.

To Reproduce

Terraform

terraform {
  required_providers {
    redirectpizza = {
      source  = "enflow/redirectpizza"
      version = "0.2.1"
    }
  }
}

provider "redirectpizza" {
  token = [token]
}

resource "redirectpizza_redirect" "test" {
  sources = ["example.com"]
  destination {
    url = "https://www.example.com/"
  }
}

Steps to Reproduce

  1. terraform init
  2. terraform apply
    i. Observe the monitoring = "inherit" in the destination block in the plan
  3. terraform apply again

Expected

No Changes

Actual

  ~ resource "redirectpizza_redirect" "test" {
        id                = "[id]"
        tags              = []
        # (5 unchanged attributes hidden)

      ~ destination {
          + monitoring = "inherit"
            # (2 unchanged attributes hidden)
        }
    }

It was not able to set the value of monitoring to inherit in the original apply, so it tries to do so again, ad infinitum.

Workaround

This does not work either, as Terraform interprets null as "use the default from the provider":

resource "redirectpizza_redirect" "test" {
  sources = ["example.com"]
  destination {
    url = "https://www.example.com/"
    monitoring = null
  }
}

However, this will round-trip properly:

resource "redirectpizza_redirect" "test" {
  sources = ["example.com"]
  destination {
    url = "https://www.example.com/"
    monitoring = ""
  }
}

because there is no terraform-side validation that monitoring be one of the valid values, and it successfully considers the un-set value from the API equivalent to the provided empty string.

Potential Fixes

When converting a "Single" destination from a string to a list of objects, we could treat it as if the monitoring property were the default value of inherit (matching the API to the Terraform state).

Alternately, we could disallow setting the monitoring property (or the expression property) when there is only one destination block and store the value as empty (matching Terraform state to the API) in the Terraform state, better matching what one can express in Terraform configuration to what the API expects.

@Freeaqingme
Copy link
Contributor

Thank you for reporting this issue! I must have seen it before, but while focusing on the big things, my brain must have kinda ignored it. I've prepared a fix in #57 , which my colleague will probably deploy as a new version this week still.

mbardelmeijer added a commit that referenced this issue May 30, 2024
Parse default monitoring value from API as 'inherit' #56
@mbardelmeijer
Copy link
Member

mbardelmeijer commented May 30, 2024

Thanks @Freeaqingme for the fix! Thanks @michael-sterling for the clean issue report 👍

Tagged under v0.2.2

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
3 participants