Closed luispresuelVenafi closed 2 years ago
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:
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:
If you haven't seen already, the v2 Upgrade Guide might have some other helpful tidbits. π
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
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?
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
?
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 output
s 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.
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.
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
If you switch
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 https://github.com/hashicorp/terraform-plugin-log/issues/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
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.
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.
SDK version
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.