2.3.1: Breaking change in that a null value is converted to "" instead of simply not present

[lorengordon]

          This is actually still failing as a behavioral change, because it converts `null` to `""` instead of simply passing nothing.

Originally posted by @lorengordon in #193 (comment)

We're still seeing an issue where 2.3.1 is converting null to "" instead of simply passing nothing. Below is an example where this is failing.

Call the program:
https://github.com/plus3it/terraform-external-file-cache/blob/master/main.tf#L4

Read the args and run:
https://github.com/plus3it/terraform-external-file-cache/blob/80307f75db09d9549c6d42ebaf9cf2f43c0178d0/file_cache.py#L10-L11

This ends up failing because s3_endpoint_url is passed as "" instead of either absent or None. And boto3/botocore actually test separately for None (valid) and "" (invalid). Which is how we discovered the breakage.

[bflad]

Thanks so much for filing this, @lorengordon, and sorry its giving you trouble. I believe the answer here will be yes, but just to confirm, was this working previously with version 2.2.3?

[lorengordon]

Yes, this was working in 2.2.3. We also believe that particular module (terraform-external-file-cache) was working fine with 2.3.0, and the fix in 2.3.1 is what broke it.

[bflad]

Hey again 👋 I'm trying to reproduce this, even using the module instead of creating a standalone Terraform configuration. Are you able to modify this so it fails? It works locally for me with s3_endpoint_url not set.

terraform {
  required_providers {
    external = {
      source  = "hashicorp/external"
      version = "2.3.1"
    }
  }

  required_version = "1.4.4"
}

module "file_cache" {
  source = "git::https://github.com/plus3it/terraform-external-file-cache"

  python_cmd = ["python3"]
  uris       = ["https://registry.terraform.io/.well-known/terraform.json"]
}

output "filepaths" {
  value = module.file_cache.filepaths
}

$ pip3 install --user -r requirements.txt # copied from repo
$ terraform init
...
$ terraform apply
module.file_cache.data.external.this["https://registry.terraform.io/.well-known/terraform.json"]: Reading...
module.file_cache.data.external.this["https://registry.terraform.io/.well-known/terraform.json"]: Read complete after 2s [id=-]

Changes to Outputs:
  + filepaths = {
      + "https://registry.terraform.io/.well-known/terraform.json" = ".filecache/cc1a2a4c3f20d88ce8d8276551df1556fec11a65539d163690072af67047befd/terraform.json"
    }

You can apply this plan to save these new output values to the Terraform state, without changing any real infrastructure.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes


Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

filepaths = {
  "https://registry.terraform.io/.well-known/terraform.json" = ".filecache/cc1a2a4c3f20d88ce8d8276551df1556fec11a65539d163690072af67047befd/terraform.json"
}

[lorengordon]

We literally just patched it so pointing at latest master works works either way. 😁 Change your reference to point at the immediately prior version of the module:

module "file_cache" {
  source = "git::https://github.com/plus3it/terraform-external-file-cache?ref=2.1.1"

  python_cmd = ["python3"]
  uris       = ["https://registry.terraform.io/.well-known/terraform.json"]
}

[bflad]

Oh! That makes sense. 😅 Perfect, was able to reproduce locally so I could use custom provider builds for debugging. Here is the data that is visible to the provider code after each of the provider SDK has done its data handling:

terraform-plugin-sdk apparently omitted map keys completely when the map value was null:

map[string]interface {}{"path":".filecache/cc1a2a4c3f20d88ce8d8276551df1556fec11a65539d163690072af67047befd", "refresh":"false", "uri":"https://registry.terraform.io/.well-known/terraform.json"}

terraform-plugin-framework meanwhile is able to properly set the map value when it is null, since it almost fully implements the Terraform type system:

basetypes.MapValue{elements:map[string]attr.Value{"path":basetypes.StringValue{state:0x2, value:".filecache/cc1a2a4c3f20d88ce8d8276551df1556fec11a65539d163690072af67047befd"}, "refresh":basetypes.StringValue{state:0x2, value:"false"}, "s3_endpoint_url":basetypes.StringValue{state:0x0, value:""}, "uri":basetypes.StringValue{state:0x2, value:"https://registry.terraform.io/.well-known/terraform.json"}}, elementType:basetypes.StringType{}, state:0x2}

The logic in the data source then needs to create a JSON object out of this information, which currently boils down to:

// v2.3.1 logic
json.Marshal(map[string]string{
	"path": ".filecache/cc1a2a4c3f20d88ce8d8276551df1556fec11a65539d163690072af67047befd",
	"refresh": "false",
	"s3_endpoint_url": "",
	"uri": "https://registry.terraform.io/.well-known/terraform.json",
})
// {
//	"path": ".filecache/cc1a2a4c3f20d88ce8d8276551df1556fec11a65539d163690072af67047befd",
//	"refresh": "false",
//	"s3_endpoint_url": "",
//	"uri": "https://registry.terraform.io/.well-known/terraform.json",
// }

One forward-looking option would be switching to string pointers so its fully representative of the query data being passed in:

json.Marshal(map[string]*string{
	"path": pointer(".filecache/cc1a2a4c3f20d88ce8d8276551df1556fec11a65539d163690072af67047befd"),
	"refresh": pointer("false"),
	"s3_endpoint_url": nil,
	"uri": pointer("https://registry.terraform.io/.well-known/terraform.json"),
})
// {
//	"path": ".filecache/cc1a2a4c3f20d88ce8d8276551df1556fec11a65539d163690072af67047befd",
//	"refresh": "false",
//	"s3_endpoint_url": null,
//	"uri": "https://registry.terraform.io/.well-known/terraform.json",
// }

However, that also is different than it originally was in v2.2.3 where the map keys were omitted completely if the map value was null. I'm guessing the correct behavior here is to skip map keys completely if null, to preserve the prior behavior, and raise an issue to switch this to the last behavior I mention above in a future major version.

[lorengordon]

Tracking! Makes sense @bflad, thanks for investigating so quickly!

[bflad]

#209 is the followup enhancement proposal, although in all honesty, we will probably be introducing a new data source and resource based off a generic string for input and dynamic typing for the output to remove all the oddities and restrictions with the current data source.

In the meantime, I'll submit a quick bug fix for this to revert the behavior back to how it was with v2.2.3, since deviating from that is considered a regression with our migration to terraform-plugin-framework.

[bryan-bar]

#209 is the followup enhancement proposal, although in all honesty, we will probably be introducing a new data source and resource based off a generic string for input and dynamic typing for the output to remove all the oddities and restrictions with the current data source.

In the meantime, I'll submit a quick bug fix for this to revert the behavior back to how it was with v2.2.3, since deviating from that is considered a regression with our migration to terraform-plugin-framework.

@bflad
Is there any updates or somewhere to track this publicly?
I see a PR was submitted which wanted to use resources but awaiting review. I assume it is now stale since the provider is moving to the plugin framework and will require changes: #49

There is also an issue here since 2017 with a request for a resource but no recent movement: #5

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.