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

SetNew in CustomizeDiff does not trigger diff on computed field when map value is set to empty string #371

Open
invidian opened this issue Mar 28, 2020 · 3 comments
Labels
bug Something isn't working subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.

Comments

@invidian
Copy link

SDK version

{
  "Path": "github.com/hashicorp/terraform-plugin-sdk",
  "Version": "v1.9.0"
}

Relevant provider source code

package main

import (
	"log"

	"github.com/google/go-cmp/cmp"
	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
	"github.com/hashicorp/terraform-plugin-sdk/plugin"
	"github.com/hashicorp/terraform-plugin-sdk/terraform"
)

func main() {
	plugin.Serve(&plugin.ServeOpts{
		ProviderFunc: Provider})
}

func Provider() terraform.ResourceProvider {
	return &schema.Provider{
		ResourcesMap: map[string]*schema.Resource{
			"mapdiff_test": resourceTest(),
		},
	}
}

func resourceTest() *schema.Resource {
	return &schema.Resource{
		Create:        resourceTestCreate,
		Read:          resourceTestRead,
		Delete:        resourceTestDelete,
		Update:        resourceTestCreate,
		CustomizeDiff: resourceTestDiff,
		Schema: map[string]*schema.Schema{
			"input": &schema.Schema{
				Type:     schema.TypeMap,
				Required: true,
				Elem: &schema.Schema{
					Type: schema.TypeString,
				},
			},
			"result": &schema.Schema{
				Type:     schema.TypeMap,
				Computed: true,
				Elem: &schema.Schema{
					Type: schema.TypeString,
				},
			},
		},
	}
}

func resourceTestCreate(d *schema.ResourceData, m interface{}) error {
	d.SetId("dooo")

	return resourceTestRead(d, m)
}

func resourceTestRead(d *schema.ResourceData, m interface{}) error {
	return nil
}

func resourceTestDelete(d *schema.ResourceData, m interface{}) error {
	return nil
}

func resourceTestDiff(d *schema.ResourceDiff, m interface{}) error {
	d.SetNew("result", d.Get("input"))

	o, n := d.GetChange("result")

	log.Printf(cmp.Diff(o, n))

	return nil
}

Terraform Configuration Files

// Example to reproduce.
resource "mapdiff_test" "foo" {
  input = { 
    "baz" = "hahn"
    "a"   = ""
  } 
}

// Real world example.
provider "tls" {
  version = "~> 2.1"
} 

resource "tls_private_key" "example" {
  algorithm   = "ECDSA"
  ecdsa_curve = "P384"
}

resource "mapdiff_test" "foo" {
  input = { 
    "foo" = tls_private_key.example.private_key_pem
    "baz" = "hahn"
  } 
}

Debug Output

https://gist.github.com/invidian/4f1d9d0b6f74f352aad892dab857a8fa

Expected Behavior

When using SetNew from schema.ResourceDiff on fields which are TypeMap and Computed, Terraform should properly write them and show them in diff, not ignore them.

Actual Behavior

Field input, containing user input properly shows diff and gets updated in the state, but field result gets ignored and it's never corrected. This might be problematic, when one needs a Map, where the value can actually be an empty string.

Related upstream issue flexkube/libflexkube#48 (comment).

Steps to Reproduce

  1. Save provider code to main.go file.
  2. Build using go build -o terraform-provider-diffmap.
  3. Create main.tf file with following content:
resource "mapdiff_test" "foo" {
  input = { 
    "baz" = "hahn"
    "a"   = ""
  } 
}
  1. Run terraform init.
  2. Run terraform apply -auto-approve.
  3. Inspect created Terraform state with cat terraform.tfstate. Example output:
{
  "version": 4,
  "terraform_version": "0.12.24",
  "serial": 38,
  "lineage": "9f56ba63-345b-ca92-75c4-7e9cbd2d5da1",
  "outputs": {},
  "resources": [
    {
      "mode": "managed",
      "type": "mapdiff_test",
      "name": "foo",
      "provider": "provider.mapdiff",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "id": "dooo",
            "input": {
              "a": "",
              "baz": "hahn"
            },
            "result": {
              "baz": "hahn"
            }
          },
          "private": "bnVsbA=="
        }
      ]
    }
  ]
}
  1. What actual output should be:
{
  "version": 4,
  "terraform_version": "0.12.24",
  "serial": 38,
  "lineage": "9f56ba63-345b-ca92-75c4-7e9cbd2d5da1",
  "outputs": {},
  "resources": [
    {
      "mode": "managed",
      "type": "mapdiff_test",
      "name": "foo",
      "provider": "provider.mapdiff",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "id": "dooo",
            "input": {
              "a": "",
              "baz": "hahn"
            },
            "result": {
              "a": "",
              "baz": "hahn"
            }
          },
          "private": "bnVsbA=="
        }
      ]
    }
  ]
}

Context

With terraform-provider-flexkube, I take user configuration, including TLS certificates and then transpile it to more complex structure, which is written to Terraform state to provide user friendly diff. Unfortunately, currently re-generating the certificates triggers inconsistent state, as certificate content is not known while planning. I would expect computed fields which has value set to empty string ("") to be seen in diff as "Known after apply", instead of just being ignored. Using SetNewComputed on sub-field produces error too.

With "real world example", running Terraform twice or with proper -target serves as a workaround. To trigger inconsistent plan issue, one needs to run terraform taint tls_private_key.example and then terraform apply.

If this is like this by design, how can I avoid getting inconsistent plan in such case?

References

@invidian invidian added the bug Something isn't working label Mar 28, 2020
@invidian invidian changed the title SetNew in CuostmizeDiff does not trigger diff on computed field when map value is set to empty string SetNew in CustomizeDiff does not trigger diff on computed field when map value is set to empty string Mar 28, 2020
@invidian
Copy link
Author

I'm also doing some testing now with NewValueKnown and it seems, that this function incorrectly returns true for input field in the example I provided:

func resourceTestDiff(d *schema.ResourceDiff, m interface{}) error {
  if d.NewValueKnown("input") {
    log.Printf("new value known")
  }

And if I run terraform plan, I get the message logged every time. That doesn't seem like the right behavior either.

@mingfang
Copy link

@invidian I'm not entirely sure but I was seeing something similar and this is my hackaround
https://github.com/mingfang/terraform-provider-k8s/blob/master/k8s/k8s.go#L99

@invidian
Copy link
Author

@invidian I'm not entirely sure but I was seeing something similar and this is my hackaround
https://github.com/mingfang/terraform-provider-k8s/blob/master/k8s/k8s.go#L99

Thanks for the hint @mingfang. Unfortunately, from my testing, it seems that what you're doing is to remove the extra parameters from diff, and what I need is to trigger additional diff on map field. I've tried injecting some attributes, rather than removing them, but I don't know Terraform internals very well, so I still didn't figure out how to do that.

I think for the workaround I'll just set top field as NewComputed, to at least avoid Terraform giving the error to the user. The downside is not so accurate diff provided to the user.

@paddycarver paddycarver added the subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it. label Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.
Projects
None yet
Development

No branches or pull requests

3 participants