d.Set doesn't work on not computed values

[luispresuelVenafi]

SDK version

latest 2.14

As part migration from SDKv1 to SDKv2 we noticed of our provider that the d.Set doesn't work anymore for attributes of a resource that are not computed. When trying to "correct" the value from our provider of the expiration_window attribute during this section of the code our resource: https://github.com/Venafi/terraform-provider-venafi/blob/master/venafi/resource_venafi_certificate.go#L528
it will no longer take effect (we want this behavior because we do a validation after our certificate resource having created that may return a new value for the expiration_window that we want to correct). Is that now intended behavior for the SDKv2? If so, what could be the better approach to reach our desired behavior for our use case. Not sure if is this a bug and there's plan to fix it in the future.

Expected Behavior

d.Set should also update the value from the resource attribute even if the resource is not computed.

Actual Behavior

d.Set doesn't update the value if the resource is not computed.

[bflad]

Hi @luispresuelVenafi 👋 Thanks for raising this and sorry you ran into trouble here.

Looking real quickly at that resource, it looks like its taking advantage of the Exists functionality that was deprecated with v2:

https://github.com/Venafi/terraform-provider-venafi/blob/a157e1ddb60bf4f730b47b02db399ecb1c25d821/venafi/resource_venafi_certificate.go#L226-L230

Does it work if you move that logic into the Read function (substituting any return false, nil with d.SetId("") and return nil to signal to Terraform CLI that the resource no longer exists)?

I'm not sure if it was intentional that Exists in v1 supported modifying the ResourceData, but the logic around the functionality in v2 certainly does not now:

terraform-plugin-sdk/helper/schema/resource.go

Lines 978 to 1002 in bb44c87

	if r.Exists != nil {
	// Make a copy of data so that if it is modified it doesn't
	// affect our Read later.
	data, err := schemaMap(r.Schema).Data(s, nil)
	if err != nil {
	return s, diag.FromErr(err)
	}
	data.timeouts = &rt
	if s != nil {
	data.providerMeta = s.ProviderMeta
	}
	logging.HelperSchemaTrace(ctx, "Calling downstream")
	exists, err := r.Exists(data, meta)
	logging.HelperSchemaTrace(ctx, "Called downstream")
	if err != nil {
	return s, diag.FromErr(err)
	}
	if !exists {
	return nil, nil
	}
	}

If you haven't seen already, the v2 Upgrade Guide might have some other helpful tidbits. 👍

[luispresuelVenafi]

Thank you for the quick response @bflad .

The behavior you mention in the Read function, does work to remove the resource from state (e.g. substituting any return false, nil with d.SetId("") ), but that is not the behavior I'm referring to. I'm referring that calling err = d.Set("expiration_window", <integer value>) won't take effect if the resource is a not computed value and that's true for any CRUD operation (I already checked that it won't take effect in any Create, Read, Update functions).

Unfortunately, I couldn't anything related to our use case in the upgrade guide regarding d.Set("key_value", value) function not working during the CRUD operation for SDKv2

Edit: I need to add, this does work during real testing using a binary. This only fails during tests scenarios

[bflad]

I would certainly expect any calls to d.Set() with error checking to return some form of error if the value was somehow incorrect for the situation, such as an incorrect type. So that doesn't seem to be the case.

If you've already tried using Read instead of Exists (which, at least in v2, is only providing a copy of ResourceData to provider logic so any d.Set() calls would not effect the state), then my next recommendation would be to try running Terraform CLI with logging enabled, e.g. TF_LOG=TRACE terraform apply. If the SDK is doing something that Terraform CLI shouldn't allow, but does for legacy reasons, it will output a WARN log.

---

I see you amended your above comment to mention this is only during acceptance testing. Can you show your testing code and the test failure output?

[luispresuelVenafi]

Sure

in the new resource_test file we'd have the following:

These imports:

package venafi

import (
	"crypto/x509"
	"encoding/pem"
	"fmt"
	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
	"os"
	"regexp"
	"strconv"
	"testing"
)

Then the following strings for the provider and resource

var (
	environmentVariables = fmt.Sprintf(`
variable "TPP_USER" {default = "%s"}
variable "TPP_PASSWORD" {default = "%s"}
variable "TPP_URL" {default = "%s"}
variable "TPP_ZONE" {default = "%s"}
variable "TPP_ZONE_ECDSA" {default = "%s"}
variable "TRUST_BUNDLE" {default = "%s"}
variable "CLOUD_URL" {default = "%s"}
variable "CLOUD_APIKEY" {default = "%s"}
variable "CLOUD_ZONE" {default = "%s"}
variable "TPP_ACCESS_TOKEN" {default = "%s"}
`,
...
vaasProvider = environmentVariables + `
provider "venafi" {
	alias = "vaas"
	url = "${var.CLOUD_URL}"
	api_key = "${var.CLOUD_APIKEY}"
	zone = "${var.CLOUD_ZONE}"
}
`
...
vaasConfig = `
%s
resource "venafi_certificate" "vaas_certificate" {
	provider = "venafi.vaas"
	common_name = "%s"
	%s
	key_password = "%s"
	expiration_window = %d
}
output "certificate" {
	value = "${venafi_certificate.vaas_certificate.certificate}"
}
output "private_key" {
	value = "${venafi_certificate.vaas_certificate.private_key_pem}"
	sensitive = true
}
output "expiration_window" {
	value = "${venafi_certificate.vaas_certificate.expiration_window}"
}`
...

Then the following test:

func TestVaasSignedCertUpdateWithCertDurationFromZoneWithGreaterExpWindow(t *testing.T) {
	/*
		We test to create a certificate on first step that has duration less from zone (without setting valid_days)
		than the expiration_window: It should create a Terraform state with an expiration_window as same as the cert duration.
		We expect a not empty plan due to the expiration_window being equal to cert duration
	*/
	data := testData{}
	rand := randSeq(9)
	domain := "venafi.example.com"
	data.cn = rand + "." + domain
	data.private_key_password = "123xxx"
	data.key_algo = rsa2048
	data.expiration_window = 170
	currentExpirationWindow := data.expiration_window
	config := fmt.Sprintf(vaasConfig, vaasProvider, data.cn, data.key_algo, data.private_key_password, data.expiration_window)
	t.Logf("Testing Cloud certificate with config:\n %s", config)
	resource.Test(t, resource.TestCase{
		ProviderFactories: testAccProviderFactories,
		Steps: []resource.TestStep{
			resource.TestStep{
				Config: config,
				Check: func(s *terraform.State) error {

					err := checkStandardCert(t, &data, s)
					if err != nil {
						return err
					}
					err = checkCertExpirationWindow(t, &data, s)
					if err != nil {
						return err
					}
					if currentExpirationWindow == data.expiration_window {
						return fmt.Errorf(fmt.Sprintf("expiration window should have changed during enroll. current: %s got from zone: %s", strconv.Itoa(currentExpirationWindow), strconv.Itoa(data.expiration_window)))
					}
					return nil

				},
				ExpectNonEmptyPlan: true,
			},
			resource.TestStep{
				Config:             config,
				ExpectNonEmptyPlan: true,
				Check: func(s *terraform.State) error {
					t.Log("Testing TPP certificate update")
					gotSerial := data.serial
					err := checkStandardCert(t, &data, s)
					if err != nil {
						return err
					} else {
						t.Logf("Compare updated original certificate serial %s with updated %s", gotSerial, data.serial)
						if gotSerial == data.serial {
							return fmt.Errorf("serial number from updated certificate %s is the same as "+
								"in original number %s", data.serial, gotSerial)
						} else {
							return nil
						}
					}
				},
			},
		},
	})
}

And then the check function for the expiration were we gather the value from the outputs:

func checkCertExpirationWindow(t *testing.T, data *testData, s *terraform.State) error {
	t.Log("Getting expiration_window from terraform state", data.cn)
	expirationWindowUntyped := s.RootModule().Outputs["expiration_window"].Value
	expirationWindowString, ok := expirationWindowUntyped.(string)
	if !ok {
		return fmt.Errorf("output for \"expiration_window\" was not defined")
	}
	expirationWindow, err := strconv.Atoi(expirationWindowString)
	if err != nil {
		return fmt.Errorf("output for \"expiration_window\" is not a valid integer")
	}
	data.expiration_window = expirationWindow
	return nil
}

One thing to notice here is that before we have to cast as integer as in here: https://github.com/Venafi/terraform-provider-venafi/blob/master/venafi/resource_venafi_certificate_test.go#L1245 and now we expect an string from the s.RootModule().Outputs["expiration_window"].Value

This is error I get if I run the test:

   test_util.go:191: Getting expiration_window from terraform state a5jmkhjsf.venafi.example.com
    resource_venafi_certificate_test.go:524: Step 1/2 error: Check failed: expiration window should have changed during enroll. current: 170 got from zone: 170
--- FAIL: TestVaasSignedCertUpdateWithCertDurationFromZoneWithGreaterExpWindow (13.74s)

Which makes sence, since the d.Set didn't effect during the test or could it berelated to the s.RootModule ?

[bflad]

Ah ha -- this information is super helpful. One of the big changes from v1 to v2 was that the acceptance testing framework was changed from using (or rather, duplicating) Terraform code to handle its internals to actually calling Terraform CLI commands. It's entirely possible that certain internals of terraform.State passed to provider-defined TestCheckFunc could've changed in the process.

If you haven't seen, the SDK does provide some helpers that could simplify this testing code (and potentially fix it in this case), e.g. resource.TestCheckResourceAttr. In general, you can also avoid having outputs in the testing configuration and check the resource state directly.

I'm wondering if swapping out the custom TestCheckFunc for some of the standard helper/resource checks might help alleviate any testing framework oddities rather than trying to access some of its internal data directly.

[luispresuelVenafi]

Hi @bflad, I've indeed confirmed that it was a flaw within our code since it still was using the old test driver (it seems like you mention, the internal of terraform.State were not the expected one). The code got cleaner as I updated code with required new helprs and the TestChecFunc with new test driver. Thank you very much for your help. We can re-label this as a question and remove the "bug" tag since indeed is expected behavior.

[luispresuelVenafi]

I was wondering if do you have a suggestion about using tflog instead of log.Printf right here: https://github.com/Venafi/terraform-provider-venafi/blob/master/venafi/provider.go#L187

It would require to add context to the function normalizeZone in order to use tflog there, but we don't have context by this point in our provider_test.go:

Any suggestion to add it here, during the test function?
https://github.com/Venafi/terraform-provider-venafi/blob/master/venafi/provider_test.go#L47

[bflad]

If you switch

https://github.com/Venafi/terraform-provider-venafi/blob/a157e1ddb60bf4f730b47b02db399ecb1c25d821/venafi/provider.go#L92

To using the context-aware field ConfigureContextFunc, then you can get access to the context. 😄

In unit testing, if you don't mind the logs not showing up (until hashicorp/terraform-plugin-log#52 is resolved) you can use context.Background(). Otherwise, we did just release an initial tflogtest package which would allow you to setup the logging context correctly in unit testing. e.g. to log to stderr in a unit test

ctx := tflogtest.RootLogger(context.Background(), os.Stderr)
// now calls to tflog.XXX(ctx, ...) will show up in go test output with the verbose (-v) flag set

[bflad]

By the way, I'm happy to keep answering questions here, but I'm going to close out the issue since there's nothing actionable (in a bug report or enhancement sense) for the maintainers. You can also submit questions to the HashiCorp Discuss Forums, where there are more folks including other provider developers who might be able to answer things as well.

[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.