Add support for worker text bindings

[blakeembrey]

Closes https://github.com/terraform-providers/terraform-provider-cloudflare/issues/615, closes https://github.com/terraform-providers/terraform-provider-cloudflare/issues/164. Not entirely sure on the terraform idioms so feel free to correct anything I'm doing incorrectly.

I wanted to use separate keys in binding because they have different requirements for things like sensitive and possibly values in the future. The key in the schema determines the "type" of the binding, e.g. secret_text, plain_text or kv_namespace_id. Roughly follows https://github.com/terraform-providers/terraform-provider-cloudflare/issues/615#issuecomment-598801105 but didn't see the need for specifying type since every binding is different enough so far they can be differentiated easily by the key set.

We ended up using separate binding groups for each type of binding since it makes validation in Terraform a lot easier to do and maintains backward compatibility with the existing kv_namespace_binding.

[jacobbednarz]

I've kicked off an integration test run for this one and will report the findings back shortly.

[jacobbednarz]

Looks like we've got a new failure.

------- Stdout: -------
=== RUN   TestAccCloudflareWorkerScript_MultiScriptEnt
=== PAUSE TestAccCloudflareWorkerScript_MultiScriptEnt
=== CONT  TestAccCloudflareWorkerScript_MultiScriptEnt
--- FAIL: TestAccCloudflareWorkerScript_MultiScriptEnt (7.54s)
testing.go:684: Step 2 error: errors during apply:
Error: error updating worker script: NamespaceID for binding "PLAIN_TEXT" cannot be empty
on /opt/teamcity-agent/temp/buildTmp/tf-test036547402/main.tf line 6:
(source code not available)
FAIL

[blakeembrey]

@jacobbednarz Thanks, I think I might be misunderstanding the map provided by terraform from the TypeSet list method. I'll take a look now.

[blakeembrey]

@jacobbednarz can you try it again? If it doesn’t pass this time I’ll put some more work into figuring out how to test it myself properly.

[blakeembrey]

I have things working locally but appear to have run into hashicorp/terraform#21901 on binding. Doesn't seem to be a major blocker but trying to figure out how to avoid having secret_text always attempt to change.

[blakeembrey]

@jacobbednarz I've resolved the secret text issue by using the text value from the existing resource during read. This seems to be how other providers work, i.e. AWS resources with passwords. The bindings only cause a change in two cases now:

1. When the value of the binding changes (from the read or previous state in the case of secret_text)
2. When a secret text binding has been removed from terraform but not Cloudflare

For 2, I didn't see the APIs available to handle it within the cloudflare package or documented on their site. Having the diff show up each time is probably enough to prompt that it's not being deleted properly, but happy to document this behavior and how to delete it from the cloudflare UI.

[jacobbednarz]

@blakeembrey That seems like a reasonable fix. We do have other cases of write-once-never-seen-again APIs in Cloudflare however their values are just masked so we write the masked value to state instead of pulling it from the schema.

Your second point is also fine for now 👍 If it becomes a bigger issue than we can poke Cloudflare to improve the UX somehow.

[blakeembrey]

Sweet, I ported all my local code to use this and it's been working great so far. By masked value do you mean an empty string or a value like xxx?

[jacobbednarz]

Masked value of asterisks

https://github.com/terraform-providers/terraform-provider-cloudflare/blob/dc74b3b0d164d212249d2f4678bf0d1e0533ce62/cloudflare/resource_cloudflare_access_identity_provider.go#L13

https://github.com/terraform-providers/terraform-provider-cloudflare/blob/dc74b3b0d164d212249d2f4678bf0d1e0533ce62/cloudflare/resource_cloudflare_access_identity_provider.go#L76-L84

I'll kick off an integration test run and 🤞 we're ready to roll!

[jacobbednarz]

Integration is looking good! I'm going to leave this for today and make sure that no one else has any thoughts before we land this one.

Thanks for the PR so far 🙇

[blakeembrey]

@jacobbednarz Any opinion on using the singular binding versus many bindings? For example:

text_binding {
  name = "TEXT"
  value = "TEXT"
}

secret_binding {
  name = "SECRET"
  value = "SECRET"
}

This would make the change backward compatible (no need to deprecate the existing binding) and makes something like the concealed StateFunc easier to implement.

[jacobbednarz]

I personally prefer the individual blocks as it is more explicit and gives us a little more flexibility with changes to individual resources. In turn, you end up with less complex code branches and more readable code.

If you want to swap it over to help with backwards compatibility, I'd be 👍 for it.

[blakeembrey]

@jacobbednarz Done, though I couldn't get the concealed text behavior to easily work within the set so I fell back on the existing logic of looking up the previous value to diff.

[jacobbednarz]

this is really great, thanks @blakeembrey! i've just kicked off a final integration test run and we'll land this as soon as that is green.

[blakeembrey]

Awesome! Really excited to see it released 😄