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

resource/random_password+random_string: Prevent override_special differences upgrading from 3.3.2 #312

Merged
merged 7 commits into from
Sep 6, 2022

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Sep 5, 2022

Closes #306
Builds on #308

Note: This was almost completed work from Friday, but I didn't have time to submit it. We should compare and contrast this with #310, which I'll take a look at now.

This fixes the override_special attribute issues by removing Computed: true and the empty string default value plan modifier (this attribute's value should always come from the practitioner) and resolves the underlying issue of always reading plan.OverrideSpecial.Value into the state, which will never be correct for null configurations.

For any environment that happened to upgrade to 3.4.2 and save the empty string into state, the replacement plan modifier will allow in-place modification from empty string ("") to null.

Previously after removing Computed: true and default value plan modifier:

=== CONT  TestAccResourcePassword_OverrideSpecial_FromVersion3_3_2
    resource_password_test.go:221: Step 2/2 error: Error running apply: exit status 1

        Error: Provider produced inconsistent result after apply

        When applying changes to random_password.test, provider
        "provider[\"registry.terraform.io/hashicorp/random\"]" produced an unexpected
        new value: .override_special: was null, but now cty.StringVal("").

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
=== CONT  TestAccResourcePassword_OverrideSpecial_FromVersion3_4_2
    resource_password_test.go:255: Step 2/2 error: Error running apply: exit status 1

        Error: Provider produced inconsistent result after apply

        When applying changes to random_password.test, provider
        "provider[\"registry.terraform.io/hashicorp/random\"]" produced an unexpected
        new value: .override_special: was null, but now cty.StringVal("").

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

Previously before adjusting requires replace plan modifier:

=== CONT  TestAccResourcePassword_OverrideSpecial_FromVersion3_3_2
    resource_password_test.go:221: Step 2/2 error: Check failed: Check 3/3 error: attribute values are different, got :i$(*K3_Vinv and FSw{$8sB#Uc3
=== CONT  TestAccResourcePassword_OverrideSpecial_FromVersion3_4_2
    resource_password_test.go:255: Step 2/2 error: Check failed: Check 3/3 error: attribute values are different, got VrNYb]{#Z2Xu and XZD#_<BjDhkH

Reference: #307

This change fixes the source of the `bcrypt_hash` generation to being the result of the random password generation. The issue was introduced in v3.4.0.

Previously:

```
--- FAIL: TestAccResourcePassword_BcryptHash (0.63s)
    /Users/bflad/src/github.com/hashicorp/terraform-provider-random/internal/provider/resource_password_test.go:107: Step 1/1 error: Check failed: Check 3/3 error: crypto/bcrypt: hashedPassword is not the hash of the given password
```

Suggested CHANGELOG:

```
NOTES:

* resource/random_password: If the resource was created between versions 3.4.0 and 3.4.2, the `bcrypt_hash` value will not correctly verify against the `result` value. Use `terraform taint` or `terraform apply -replace` to trigger resource recreation with this version.

BUG FIXES:

* resource/random_password: Fixed incorrect `bcrypt_hash` generation since version 3.4.0
```
… from 3.3.2

Reference: #306

This fixes the `override_special` attribute issues by removing `Computed: true` and the empty string default value plan modifier (this attribute's value should always come from the practitioner) and resolves the underlying issue of always reading `plan.OverrideSpecial.Value` into the state, which will never be correct for null configurations.

For any environment that happened to upgrade to 3.4.2 and save the empty string into state, the replacement plan modifier will allow in-place modification from empty string (`""`) to `null`.

Previously after removing `Computed: true` and default value plan modifier:

```
=== CONT  TestAccResourcePassword_OverrideSpecial_FromVersion3_3_2
    resource_password_test.go:221: Step 2/2 error: Error running apply: exit status 1

        Error: Provider produced inconsistent result after apply

        When applying changes to random_password.test, provider
        "provider[\"registry.terraform.io/hashicorp/random\"]" produced an unexpected
        new value: .override_special: was null, but now cty.StringVal("").

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
=== CONT  TestAccResourcePassword_OverrideSpecial_FromVersion3_4_2
    resource_password_test.go:255: Step 2/2 error: Error running apply: exit status 1

        Error: Provider produced inconsistent result after apply

        When applying changes to random_password.test, provider
        "provider[\"registry.terraform.io/hashicorp/random\"]" produced an unexpected
        new value: .override_special: was null, but now cty.StringVal("").

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
```

Previously before adjusting requires replace plan modifier:

```
=== CONT  TestAccResourcePassword_OverrideSpecial_FromVersion3_3_2
    resource_password_test.go:221: Step 2/2 error: Check failed: Check 3/3 error: attribute values are different, got :i$(*K3_Vinv and FSw{$8sB#Uc3
=== CONT  TestAccResourcePassword_OverrideSpecial_FromVersion3_4_2
    resource_password_test.go:255: Step 2/2 error: Check failed: Check 3/3 error: attribute values are different, got VrNYb]{#Z2Xu and XZD#_<BjDhkH
```
@bflad bflad added the bug label Sep 5, 2022
@bflad bflad requested a review from a team as a code owner September 5, 2022 18:19
@github-actions github-actions bot added the size/L label Sep 5, 2022
Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Couple of minor updates to the tests for v3.3.2.

internal/provider/resource_password_test.go Outdated Show resolved Hide resolved
internal/provider/resource_password_test.go Outdated Show resolved Hide resolved
internal/provider/resource_string_test.go Outdated Show resolved Hide resolved
internal/provider/resource_string_test.go Outdated Show resolved Hide resolved
Base automatically changed from bflad/password-bcrypt-fix to main September 6, 2022 12:48
@bflad bflad added this to the v3.4.3 milestone Sep 6, 2022
Co-authored-by: Benjamin Bennett <ben.bennett@hashicorp.com>
@github-actions github-actions bot added size/XL and removed size/L labels Sep 6, 2022
@github-actions github-actions bot added size/L and removed size/XL labels Sep 6, 2022
…ial explicitly to null during import

Previously:

```
=== RUN   TestAccResourcePassword
    resource_password_test.go:67: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) (len=1) {
         (string) (len=16) "override_special": (string) ""
        }

        (map[string]string) {
        }
--- FAIL: TestAccResourcePassword (0.79s)
```
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, 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 May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

random_password Showing Unexpected Plan 3.3.2 to 3.4.2
2 participants