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

ImportStateCheck func receives states other than the imported resource #1060

Closed
SarahFrench opened this issue Sep 15, 2022 · 8 comments · Fixed by #1089
Closed

ImportStateCheck func receives states other than the imported resource #1060

SarahFrench opened this issue Sep 15, 2022 · 8 comments · Fixed by #1089
Assignees
Labels
bug Something isn't working subsystem/tests Issues and feature requests related to the testing framework.
Milestone

Comments

@SarahFrench
Copy link
Member

SDK version

v2.18.0

Relevant provider source code

func TestAccPrivatecaCertificateTemplateIamPolicyGenerated_withCondition(t *testing.T) {
    t.Parallel()

    context := map[string]interface{}{
        "random_suffix":           randString(t, 10),
        "role":                    "roles/privateca.templateUser",
        "condition_title":         "expires_after_2019_12_31",
        "condition_expr":          `request.time < timestamp(\"2020-01-01T00:00:00Z\")`,
        "condition_desc":          "Expiring at midnight of 2019-12-31",
        "condition_title_no_desc": "expires_after_2019_12_31-no-description",
        "condition_expr_no_desc":  `request.time < timestamp(\"2020-01-01T00:00:00Z\")`,
    }

    // Test should have 2 bindings: one with a description and one without. Any < chars are converted to a unicode character by the API.
    expectedPolicyData := Nprintf(`{"bindings":[{"condition":{"description":"%{condition_desc}","expression":"%{condition_expr}","title":"%{condition_title}"},"members":["user:admin@hashicorptest.com"],"role":"%{role}"},{"condition":{"expression":"%{condition_expr}","title":"%{condition_title}-no-description"},"members":["user:admin@hashicorptest.com"],"role":"%{role}"}]}`, context)
    expectedPolicyData = strings.Replace(expectedPolicyData, "<", "\\u003c", -1)

    vcrTest(t, resource.TestCase{
        PreCheck:  func() { testAccPreCheck(t) },
        Providers: testAccProviders,
        Steps: []resource.TestStep{
            {
                Config: testAccPrivatecaCertificateTemplateIamPolicy_withConditionGenerated(context),
            },
            {
                ResourceName:  "google_privateca_certificate_template_iam_policy.foo",
                ImportStateId: fmt.Sprintf("projects/%s/locations/%s/certificateTemplates/%s", getTestProjectFromEnv(), getTestRegionFromEnv(), fmt.Sprintf("tf-test-my-template%s", context["random_suffix"])),
                ImportState:   true,
                // We cannot verify state with ImportStateVerify because the data.google_iam_policy.foo data source causes an issue,
                // which results in an error referencing the google_iam_policy data source's ID
                // e.g. Failed state verification, resource with ID 2561318194 not found
                // ImportStateVerify: true,
                ImportStateCheck: func(s []*terraform.InstanceState) error {
                    var policy *terraform.InstanceState
                    if len(s) != 1 {
                        return fmt.Errorf("expected 1 state: %#v", s)
                    }
                    policy = s[0]
                    if policy.Attributes["policy_data"] != expectedPolicyData {
                        return fmt.Errorf("expected google_privateca_certificate_template_iam_policy `policy_data` attribute to be %s, got: %s", expectedPolicyData, policy.Attributes["policy_data"])
                    }
                    return nil
                },
            },
        },
    })
}

func testAccPrivatecaCertificateTemplateIamPolicy_withConditionGenerated(context map[string]interface{}) string {
	return Nprintf(`
resource "google_privateca_certificate_template" "default" {
  name = "tf-test-my-template%{random_suffix}"
  location = "us-central1"

  identity_constraints {
    allow_subject_alt_names_passthrough = true
    allow_subject_passthrough           = true

    cel_expression {
      description = "Always true"
      expression  = "true"
      location    = "any.file.anywhere"
      title       = "Sample expression"
    }
  }
}

data "google_iam_policy" "foo" {
  binding {
    role = "%{role}"
    members = ["user:admin@hashicorptest.com"]
    condition {
      // Check that lack of description doesn't have non-empty plan after
      title       = "%{condition_title_no_desc}"
      expression  = "%{condition_expr_no_desc}"
    }
  }
  binding {
    role = "%{role}"
    members = ["user:admin@hashicorptest.com"]
    condition {
      title       = "%{condition_title}"
      description = "%{condition_desc}"
      expression  = "%{condition_expr}"
    }
  }
}

resource "google_privateca_certificate_template_iam_policy" "foo" {
  certificate_template = google_privateca_certificate_template.default.id
  policy_data = data.google_iam_policy.foo.policy_data
}
`, context)
}

// This is a Printf sibling (Nprintf; Named Printf), which handles strings like
// Nprintf("Hello %{target}!", map[string]interface{}{"target":"world"}) == "Hello world!".
// This is particularly useful for generated tests, where we don't want to use Printf,
// since that would require us to generate a very particular ordering of arguments.
func Nprintf(format string, params map[string]interface{}) string {
	for key, val := range params {
		format = strings.Replace(format, "%{"+key+"}", fmt.Sprintf("%v", val), -1)
	}
	return format
}

Terraform Configuration Files

See above

Debug Output

Expected Behavior

As 1 resource is being imported I'd expect the argument passed to the ImportStateCheck func to contain state for the google_privateca_certificate_template_iam_policy.foo resource only. Instead, there is state for both the google_privateca_certificate_template_iam_policy.foo resource and data.google_iam_policy.foo data source.

Actual Behavior

The test fails and in the output is:

    provider_test.go:307: expected 1 state: []*terraform.InstanceState{(*terraform.InstanceState)(0x14000fa4270), (*terraform.InstanceState)(0x14000fa4410)}

The file line being referred to is here in the google provider.

👉 Click to expand and see the 2 items in the state array above

# google_privateca_ca_pool_iam_policy resource state

&terraform.InstanceState{ID:"projects/[PROJECT_ID]/locations/us-central1/caPools/tf-test-my-poolom8j1hjruo", Attributes:map[string]string{"%":"6", "ca_pool":"projects/[PROJECT_ID]/locations/us-central1/caPools/tf-test-my-poolom8j1hjruo", "etag":"BwXouFsomjI=", "id":"projects/[PROJECT_ID]/locations/us-central1/caPools/tf-test-my-poolom8j1hjruo", "location":"us-central1", "policy_data":"{\"bindings\":[{\"condition\":{\"expression\":\"request.time \\u003c timestamp(\\\"2020-01-01T00:00:00Z\\\")\",\"title\":\"expires_after_2019_12_31\"},\"members\":[\"user:admin@hashicorptest.com\"],\"role\":\"roles/privateca.certificateManager\"},{\"condition\":{\"description\":\"Expiring at midnight of 2019-12-31\",\"expression\":\"request.time \\u003c timestamp(\\\"2020-01-01T00:00:00Z\\\")\",\"title\":\"expires_after_2019_12_31\"},\"members\":[\"user:admin@hashicorptest.com\"],\"role\":\"roles/privateca.certificateManager\"}]}", "project":"[PROJECT_ID]"}, Ephemeral:terraform.EphemeralState{ConnInfo:map[string]string(nil), Type:"google_privateca_ca_pool_iam_policy"}, Meta:map[string]interface {}{"schema_version":0}, ProviderMeta:cty.NilVal, RawConfig:cty.NilVal, RawState:cty.NilVal, RawPlan:cty.NilVal, Tainted:false, mu:sync.Mutex{state:0, sema:0x0}}

# google_iam_policy data source state

&terraform.InstanceState{ID:"2561318194", Attributes:map[string]string{"%":"4", "binding.#":"2", "binding.0.%":"3", "binding.0.condition.#":"1", "binding.0.condition.0.%":"3", "binding.0.condition.0.description":"", "binding.0.condition.0.expression":"request.time < timestamp(\"2020-01-01T00:00:00Z\")", "binding.0.condition.0.title":"expires_after_2019_12_31", "binding.0.members.#":"1", "binding.0.members.0":"user:admin@hashicorptest.com", "binding.0.role":"roles/privateca.certificateManager", "binding.1.%":"3", "binding.1.condition.#":"1", "binding.1.condition.0.%":"3", "binding.1.condition.0.description":"Expiring at midnight of 2019-12-31", "binding.1.condition.0.expression":"request.time < timestamp(\"2020-01-01T00:00:00Z\")", "binding.1.condition.0.title":"expires_after_2019_12_31", "binding.1.members.#":"1", "binding.1.members.0":"user:admin@hashicorptest.com", "binding.1.role":"roles/privateca.certificateManager", "id":"2561318194", "policy_data":"{\"bindings\":[{\"condition\":{\"description\":\"Expiring at midnight of 2019-12-31\",\"expression\":\"request.time \\u003c timestamp(\\\"2020-01-01T00:00:00Z\\\")\",\"title\":\"expires_after_2019_12_31\"},\"members\":[\"user:admin@hashicorptest.com\"],\"role\":\"roles/privateca.certificateManager\"},{\"condition\":{\"expression\":\"request.time \\u003c timestamp(\\\"2020-01-01T00:00:00Z\\\")\",\"title\":\"expires_after_2019_12_31\"},\"members\":[\"user:admin@hashicorptest.com\"],\"role\":\"roles/privateca.certificateManager\"}]}"}, Ephemeral:terraform.EphemeralState{ConnInfo:map[string]string(nil), Type:"google_iam_policy"}, Meta:map[string]interface {}{"schema_version":0}, ProviderMeta:cty.NilVal, RawConfig:cty.NilVal, RawState:cty.NilVal, RawPlan:cty.NilVal, Tainted:false, mu:sync.Mutex{state:0, sema:0x0}}

Steps to Reproduce

  • You can get a version of the google provider containing these code changes by checking out this commit in a fork of the provider: modular-magician/terraform-provider-google@d38cd23
  • In the root of the repo, run make testacc TEST=./google TESTARGS="-run=^TestAccComputeSubnetworkIamPolicyGenerated_withCondition$"
    • You would need these ENVs set for acceptance tests to work:
      • GOOGLE_PROJECT=<project ID>
      • GOOGLE_REGION=us-central1
      • GOOGLE_ZONE=us-central1-a
      • GOOGLE_CREDENTIALS=<path to a .json key file for a service account> - this is the identity use by Terraform
      • GOOGLE_SERVICE_ACCOUNT=<email of the service account above>

References

@SarahFrench SarahFrench added the bug Something isn't working label Sep 15, 2022
@SarahFrench
Copy link
Member Author

To help it stand out from the description above: I found this very similar issue (#531) that was closed in 2020.

@SarahFrench
Copy link
Member Author

The tests I've experienced failures with have passed in the the automated tests on PRs in the terraform-provider-google repo (link to tests on the relevant PR here), which is interesting

@Serpentiel
Copy link

hey 👋

we are facing the same problem in our Terraform provider, please see: https://github.com/aiven/terraform-provider-aiven/actions/runs/3386997636/jobs/5627140403#step:6:60

there are, indeed, multiple states passed to the ImportStateCheck function, and this behavior has only been introduced recently, so we believe this is a bug

@bflad
Copy link
Member

bflad commented Nov 3, 2022

Hi @Serpentiel 👋 Can you confirm if this is only an issue when using Terraform 1.3 and later?

@bflad bflad added the subsystem/tests Issues and feature requests related to the testing framework. label Nov 3, 2022
@bflad bflad self-assigned this Nov 3, 2022
@bflad bflad added this to the v2.24.1 milestone Nov 3, 2022
bflad added a commit that referenced this issue Nov 3, 2022
Reference: #1060

The `TestStep` type `ImportStateCheck` functionality is for verifying two specific scenarios over `ImportStateVerify`:

- Resources which create multiple resources during import (an implementation detail which is an legacy anti-pattern that should no longer be present in resources).
- Resources where importing may cause attributes to have syntactically different but semantically/functionally equivalent values that requires special logic to check.

Terraform 1.3 and later can include data source states when importing resources. Rather than forcing provider developers to account for this Terraform version-specific behavior, the `ImportStateCheck` logic will now only receive managed resource states to match the intended usage.

Previously with Terraform 1.2.9 and earlier:

```
--- PASS: TestTest_TestStep_ImportStateCheck_SkipDataSourceState (1.58s)
```

Previously with Terraform 1.3.4:

```
--- FAIL: TestTest_TestStep_ImportStateCheck_SkipDataSourceState (0.63s)
    /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/resource/testing_new_import_state_test.go:16: expected 1 state, got: 2
```

Now with Terraform 1.3.4:

```
--- PASS: TestTest_TestStep_ImportStateCheck_SkipDataSourceState (0.91s)
```
@bflad
Copy link
Member

bflad commented Nov 3, 2022

I have submitted #1089 which will strip data source states (seemingly present after import with Terraform 1.3 and later) from ImportStateCheck. Please note as mentioned in the PR description and the updated Go documentation that ImportStateVerify is generally preferable over ImportStateCheck, as it can ensure that the entire resource state for all attributes match between created and imported resources.

@Serpentiel
Copy link

Serpentiel commented Nov 4, 2022

Hi @Serpentiel 👋 Can you confirm if this is only an issue when using Terraform 1.3 and later?

hey, thank you for your reply! 👋

I can confirm that this issue appeared in recent releases of the Terraform

bflad added a commit that referenced this issue Nov 4, 2022
…ck (#1089)

* helper/resource: Skip data source states with TestStep.ImportStateCheck

Reference: #1060

The `TestStep` type `ImportStateCheck` functionality is for verifying two specific scenarios over `ImportStateVerify`:

- Resources which create multiple resources during import (an implementation detail which is an legacy anti-pattern that should no longer be present in resources).
- Resources where importing may cause attributes to have syntactically different but semantically/functionally equivalent values that requires special logic to check.

Terraform 1.3 and later can include data source states when importing resources. Rather than forcing provider developers to account for this Terraform version-specific behavior, the `ImportStateCheck` logic will now only receive managed resource states to match the intended usage.

Previously with Terraform 1.2.9 and earlier:

```
--- PASS: TestTest_TestStep_ImportStateCheck_SkipDataSourceState (1.58s)
```

Previously with Terraform 1.3.4:

```
--- FAIL: TestTest_TestStep_ImportStateCheck_SkipDataSourceState (0.63s)
    /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/resource/testing_new_import_state_test.go:16: expected 1 state, got: 2
```

Now with Terraform 1.3.4:

```
--- PASS: TestTest_TestStep_ImportStateCheck_SkipDataSourceState (0.91s)
```

* Update CHANGELOG for #1089
@SarahFrench
Copy link
Member Author

Thanks for addressing this! In my case I must have been using a newer version of TF locally versus the version used in terraform-provider-google's automated tests.

I'll revisit the affected tests and the possibility of using ImportStateVerify

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working subsystem/tests Issues and feature requests related to the testing framework.
Projects
None yet
3 participants