diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 37cd4688..3d3aa6aa 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -65,6 +65,7 @@ jobs: - '0.15.*' - '1.0.*' - '1.1.*' + - '1.2.*' steps: - name: Setup Go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2543b009..8324648a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## 3.3.0 (Unreleased) + +ENHANCEMENTS: + +* resource/random_password: `number` is now deprecated and `numeric` has been added to align attribute naming. `number` will be removed in the next major release ([#258](https://github.com/hashicorp/terraform-provider-random/pull/258)). +* resource/random_string: `number` is now deprecated and `numeric` has been added to align attribute naming. `number` will be removed in the next major release ([#258](https://github.com/hashicorp/terraform-provider-random/pull/258)). + ## 3.2.0 (May 18, 2022) NEW FEATURES: diff --git a/docs/resources/password.md b/docs/resources/password.md index 2b3a7f78..fec48e08 100644 --- a/docs/resources/password.md +++ b/docs/resources/password.md @@ -46,7 +46,8 @@ resource "aws_db_instance" "example" { - `min_numeric` (Number) Minimum number of numeric characters in the result. Default value is `0`. - `min_special` (Number) Minimum number of special characters in the result. Default value is `0`. - `min_upper` (Number) Minimum number of uppercase alphabet characters in the result. Default value is `0`. -- `number` (Boolean) Include numeric characters in the result. Default value is `true`. +- `number` (Boolean, Deprecated) Include numeric characters in the result. Default value is `true`. **NOTE**: This is deprecated, use `numeric` instead. +- `numeric` (Boolean) Include numeric characters in the result. Default value is `true`. - `override_special` (String) Supply your own list of special characters to use for string generation. This overrides the default character list in the special argument. The `special` argument must still be set to true for any overwritten characters to be used in generation. - `special` (Boolean) Include special characters in the result. These are `!@#$%&*()-_=+[]{}<>:?`. Default value is `true`. - `upper` (Boolean) Include uppercase alphabet characters in the result. Default value is `true`. diff --git a/docs/resources/string.md b/docs/resources/string.md index 31fd7d88..faa1aea5 100644 --- a/docs/resources/string.md +++ b/docs/resources/string.md @@ -41,7 +41,8 @@ resource "random_string" "random" { - `min_numeric` (Number) Minimum number of numeric characters in the result. Default value is `0`. - `min_special` (Number) Minimum number of special characters in the result. Default value is `0`. - `min_upper` (Number) Minimum number of uppercase alphabet characters in the result. Default value is `0`. -- `number` (Boolean) Include numeric characters in the result. Default value is `true`. +- `number` (Boolean, Deprecated) Include numeric characters in the result. Default value is `true`. **NOTE**: This is deprecated, use `numeric` instead. +- `numeric` (Boolean) Include numeric characters in the result. Default value is `true`. - `override_special` (String) Supply your own list of special characters to use for string generation. This overrides the default character list in the special argument. The `special` argument must still be set to true for any overwritten characters to be used in generation. - `special` (Boolean) Include special characters in the result. These are `!@#$%&*()-_=+[]{}<>:?`. Default value is `true`. - `upper` (Boolean) Include uppercase alphabet characters in the result. Default value is `true`. diff --git a/internal/provider/resource_password.go b/internal/provider/resource_password.go index 057ce298..e6452c1a 100644 --- a/internal/provider/resource_password.go +++ b/internal/provider/resource_password.go @@ -5,11 +5,20 @@ import ( "fmt" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "golang.org/x/crypto/bcrypt" ) +// resourcePassword and resourceString both use the same set of CustomizeDiffFunc(s) in order to handle the deprecation +// of the `number` attribute and the simultaneous addition of the `numeric` attribute. planDefaultIfAllNull handles +// ensuring that both `number` and `numeric` default to `true` when they are both absent from config. +// planSyncIfChange handles keeping number and numeric in-sync when either one has been changed. func resourcePassword() *schema.Resource { + customizeDiffFuncs := planDefaultIfAllNull(true, "number", "numeric") + customizeDiffFuncs = append(customizeDiffFuncs, planSyncIfChange("number", "numeric")) + customizeDiffFuncs = append(customizeDiffFuncs, planSyncIfChange("numeric", "number")) + return &schema.Resource{ Description: "Identical to [random_string](string.html) with the exception that the result is " + "treated as sensitive and, thus, _not_ displayed in console output. Read more about sensitive " + @@ -19,18 +28,26 @@ func resourcePassword() *schema.Resource { CreateContext: createPassword, ReadContext: readNil, DeleteContext: RemoveResourceFromState, - Schema: passwordSchemaV1(), + Schema: passwordSchemaV2(), Importer: &schema.ResourceImporter{ StateContext: importPasswordFunc, }, - SchemaVersion: 1, + SchemaVersion: 2, StateUpgraders: []schema.StateUpgrader{ { Version: 0, Type: resourcePasswordV0().CoreConfigSchema().ImpliedType(), Upgrade: resourcePasswordStateUpgradeV0, }, + { + Version: 1, + Type: resourcePasswordV1().CoreConfigSchema().ImpliedType(), + Upgrade: resourcePasswordStringStateUpgradeV1, + }, }, + CustomizeDiff: customdiff.All( + customizeDiffFuncs..., + ), } } @@ -74,6 +91,12 @@ func importPasswordFunc(ctx context.Context, d *schema.ResourceData, meta interf return []*schema.ResourceData{d}, nil } +func resourcePasswordV1() *schema.Resource { + return &schema.Resource{ + Schema: passwordSchemaV1(), + } +} + func resourcePasswordV0() *schema.Resource { return &schema.Resource{ Schema: passwordSchemaV0(), @@ -87,7 +110,7 @@ func resourcePasswordStateUpgradeV0(_ context.Context, rawState map[string]inter result, ok := rawState["result"].(string) if !ok { - return nil, fmt.Errorf("resource password state upgrade failed, result could not be asserted as string: %T", rawState["result"]) + return nil, fmt.Errorf("resource password state upgrade failed, result is not a string: %T", rawState["result"]) } hash, err := generateHash(result) diff --git a/internal/provider/resource_password_test.go b/internal/provider/resource_password_test.go new file mode 100644 index 00000000..7e011a79 --- /dev/null +++ b/internal/provider/resource_password_test.go @@ -0,0 +1,448 @@ +package provider + +import ( + "context" + "errors" + "fmt" + "regexp" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "golang.org/x/crypto/bcrypt" +) + +func TestAccResourcePasswordBasic(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviders, + Steps: []resource.TestStep{ + { + Config: `resource "random_password" "basic" { + length = 12 + }`, + Check: resource.ComposeTestCheckFunc( + testAccResourceStringCheck("random_password.basic", &customLens{ + customLen: 12, + }), + ), + }, + { + ResourceName: "random_password.basic", + // Usage of ImportStateIdFunc is required as the value passed to the `terraform import` command needs + // to be the password itself, as the password resource sets ID to "none" and "result" to the password + // supplied during import. + ImportStateIdFunc: func(s *terraform.State) (string, error) { + id := "random_password.basic" + rs, ok := s.RootModule().Resources[id] + if !ok { + return "", fmt.Errorf("not found: %s", id) + } + if rs.Primary.ID == "" { + return "", fmt.Errorf("no ID is set") + } + + return rs.Primary.Attributes["result"], nil + }, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"bcrypt_hash", "length", "lower", "number", "numeric", "special", "upper", "min_lower", "min_numeric", "min_special", "min_upper", "override_special"}, + }, + }, + }) +} + +func TestAccResourcePasswordOverride(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviders, + Steps: []resource.TestStep{ + { + Config: `resource "random_password" "override" { + length = 4 + override_special = "!" + lower = false + upper = false + number = false + }`, + Check: resource.ComposeTestCheckFunc( + testAccResourceStringCheck("random_password.override", &customLens{ + customLen: 4, + }), + patternMatch("random_password.override", "!!!!"), + ), + }, + }, + }) +} + +func TestAccResourcePasswordMin(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviders, + Steps: []resource.TestStep{ + { + Config: `resource "random_password" "min" { + length = 12 + override_special = "!#@" + min_lower = 2 + min_upper = 3 + min_special = 1 + min_numeric = 4 + }`, + Check: resource.ComposeTestCheckFunc( + testAccResourceStringCheck("random_password.min", &customLens{ + customLen: 12, + }), + regexMatch("random_password.min", regexp.MustCompile(`([a-z])`), 2), + regexMatch("random_password.min", regexp.MustCompile(`([A-Z])`), 3), + regexMatch("random_password.min", regexp.MustCompile(`([0-9])`), 4), + regexMatch("random_password.min", regexp.MustCompile(`([!#@])`), 1), + ), + }, + }, + }) +} + +func TestAccResourcePassword_UpdateNumberAndNumeric(t *testing.T) { + t.Parallel() + resource.UnitTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviders, + Steps: []resource.TestStep{ + { + Config: `resource "random_password" "default" { + length = 12 + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckResourceAttr("random_password.default", "numeric", "true"), + ), + }, + { + Config: `resource "random_password" "default" { + length = 12 + number = false + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_password.default", "number", "false"), + resource.TestCheckResourceAttr("random_password.default", "numeric", "false"), + ), + }, + { + Config: `resource "random_password" "default" { + length = 12 + numeric = true + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckResourceAttr("random_password.default", "numeric", "true"), + ), + }, + { + Config: `resource "random_password" "default" { + length = 12 + numeric = false + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_password.default", "number", "false"), + resource.TestCheckResourceAttr("random_password.default", "numeric", "false"), + ), + }, + { + Config: `resource "random_password" "default" { + length = 12 + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckResourceAttr("random_password.default", "numeric", "true"), + ), + }, + }, + }) +} + +// TestAccResourcePassword_StateUpgraders covers the state upgrades from v0 to V2 and V1 to V2. +// This includes the addition of bcrypt_hash and numeric attributes. +func TestAccResourcePassword_StateUpgraders(t *testing.T) { + t.Parallel() + + v1Cases := []struct { + name string + configBeforeUpgrade string + configDuringUpgrade string + beforeStateUpgrade []resource.TestCheckFunc + afterStateUpgrade []resource.TestCheckFunc + }{ + { + name: "%s number is absent", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_password.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_password.default", "number", "random_password.default", "numeric"), + }, + }, + { + name: "%s number is absent then true", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + }`, + configDuringUpgrade: `resource "random_password" "default" { + length = 12 + number = true + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_password.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_password.default", "number", "random_password.default", "numeric"), + }, + }, + { + name: "%s number is absent then false", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + }`, + configDuringUpgrade: `resource "random_password" "default" { + length = 12 + number = false + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_password.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "false"), + resource.TestCheckResourceAttrPair("random_password.default", "number", "random_password.default", "numeric"), + }, + }, + { + name: "%s number is true", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + number = true + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_password.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_password.default", "number", "random_password.default", "numeric"), + }, + }, + { + name: "%s number is true then absent", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + number = true + }`, + configDuringUpgrade: `resource "random_password" "default" { + length = 12 + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_password.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_password.default", "number", "random_password.default", "numeric"), + }, + }, + { + name: "%s number is true then false", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + number = true + }`, + configDuringUpgrade: `resource "random_password" "default" { + length = 12 + number = false + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_password.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "false"), + resource.TestCheckResourceAttrPair("random_password.default", "number", "random_password.default", "numeric"), + }, + }, + { + name: "%s number is false", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + number = false + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "false"), + resource.TestCheckNoResourceAttr("random_password.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "false"), + resource.TestCheckResourceAttrPair("random_password.default", "number", "random_password.default", "numeric"), + }, + }, + { + name: "%s number is false then absent", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + number = false + }`, + configDuringUpgrade: `resource "random_password" "default" { + length = 12 + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "false"), + resource.TestCheckNoResourceAttr("random_password.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_password.default", "number", "random_password.default", "numeric"), + }, + }, + { + name: "%s number is false then true", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + number = false + }`, + configDuringUpgrade: `resource "random_password" "default" { + length = 12 + number = true + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "false"), + resource.TestCheckNoResourceAttr("random_password.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_password.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_password.default", "number", "random_password.default", "numeric"), + }, + }, + } + + v0Cases := v1Cases + v0Cases = append(v0Cases, struct { + name string + configBeforeUpgrade string + configDuringUpgrade string + beforeStateUpgrade []resource.TestCheckFunc + afterStateUpgrade []resource.TestCheckFunc + }{ + name: "%s bcrypt_hash", + configBeforeUpgrade: `resource "random_password" "default" { + length = 12 + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckNoResourceAttr("random_password.default", "bcrypt_hash"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttrSet("random_password.default", "bcrypt_hash"), + }, + }) + + cases := map[string][]struct { + name string + configBeforeUpgrade string + configDuringUpgrade string + beforeStateUpgrade []resource.TestCheckFunc + afterStateUpgrade []resource.TestCheckFunc + }{ + "3.1.3": v0Cases, + "3.2.0": v1Cases, + } + + for providerVersion, v := range cases { + for _, c := range v { + name := fmt.Sprintf(c.name, providerVersion) + t.Run(name, func(t *testing.T) { + if c.configDuringUpgrade == "" { + c.configDuringUpgrade = c.configBeforeUpgrade + } + + resource.UnitTest(t, resource.TestCase{ + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{"random": { + VersionConstraint: providerVersion, + Source: "hashicorp/random", + }}, + Config: c.configBeforeUpgrade, + Check: resource.ComposeTestCheckFunc(c.beforeStateUpgrade...), + }, + { + ProviderFactories: testAccProviders, + Config: c.configDuringUpgrade, + Check: resource.ComposeTestCheckFunc(c.afterStateUpgrade...), + }, + }, + }) + }) + } + } +} + +func TestResourcePasswordStateUpgradeV0(t *testing.T) { + cases := []struct { + name string + stateV0 map[string]interface{} + err error + expectedStateV1 map[string]interface{} + }{ + { + name: "result is not string", + stateV0: map[string]interface{}{"result": 0}, + err: errors.New("resource password state upgrade failed, result is not a string: int"), + }, + { + name: "success", + stateV0: map[string]interface{}{"result": "abc123"}, + expectedStateV1: map[string]interface{}{"result": "abc123", "bcrypt_hash": "123"}, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + actualStateV1, err := resourcePasswordStateUpgradeV0(context.Background(), c.stateV0, nil) + + if c.err != nil { + // Check error msg + if !cmp.Equal(c.err.Error(), err.Error()) { + t.Errorf("expected: %q, got: %q", c.err.Error(), err) + } + // Check actualStateV1 is nil + if !cmp.Equal(c.expectedStateV1, actualStateV1) { + t.Errorf("expected: %+v, got: %+v", c.expectedStateV1, err) + } + } else { + if err != nil { + t.Errorf("err should be nil, actual: %v", err) + } + + // Compare bcrypt_hash with plaintext equivalent to verify match + bcryptHash := actualStateV1["bcrypt_hash"] + err := bcrypt.CompareHashAndPassword([]byte(bcryptHash.(string)), []byte(c.stateV0["result"].(string))) + if err != nil { + t.Error("hash and password do not match") + } + + // Delete bcrypt_hash from actualStateV1 and expectedStateV1 so can compare + delete(actualStateV1, "bcrypt_hash") + delete(c.expectedStateV1, "bcrypt_hash") + if !cmp.Equal(actualStateV1, c.expectedStateV1) { + t.Errorf("expected: %v, got: %v", c.expectedStateV1, actualStateV1) + } + } + }) + } +} diff --git a/internal/provider/resource_pasword_test.go b/internal/provider/resource_pasword_test.go deleted file mode 100644 index 0e90eb06..00000000 --- a/internal/provider/resource_pasword_test.go +++ /dev/null @@ -1,153 +0,0 @@ -package provider - -import ( - "context" - "fmt" - "regexp" - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" - "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" -) - -func TestAccResourcePasswordBasic(t *testing.T) { - resource.UnitTest(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - ProviderFactories: testAccProviders, - Steps: []resource.TestStep{ - { - Config: `resource "random_password" "basic" { - length = 12 - }`, - Check: resource.ComposeTestCheckFunc( - testAccResourceStringCheck("random_password.basic", &customLens{ - customLen: 12, - }), - ), - }, - { - ResourceName: "random_password.basic", - // Usage of ImportStateIdFunc is required as the value passed to the `terraform import` command needs - // to be the password itself, as the password resource sets ID to "none" and "result" to the password - // supplied during import. - ImportStateIdFunc: func(s *terraform.State) (string, error) { - id := "random_password.basic" - rs, ok := s.RootModule().Resources[id] - if !ok { - return "", fmt.Errorf("not found: %s", id) - } - if rs.Primary.ID == "" { - return "", fmt.Errorf("no ID is set") - } - - return rs.Primary.Attributes["result"], nil - }, - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"bcrypt_hash", "length", "lower", "number", "special", "upper", "min_lower", "min_numeric", "min_special", "min_upper", "override_special"}, - }, - }, - }) -} - -func TestAccResourcePasswordOverride(t *testing.T) { - resource.UnitTest(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - ProviderFactories: testAccProviders, - Steps: []resource.TestStep{ - { - Config: `resource "random_password" "override" { - length = 4 - override_special = "!" - lower = false - upper = false - number = false - }`, - Check: resource.ComposeTestCheckFunc( - testAccResourceStringCheck("random_password.override", &customLens{ - customLen: 4, - }), - patternMatch("random_password.override", "!!!!"), - ), - }, - }, - }) -} - -func TestAccResourcePasswordMin(t *testing.T) { - resource.UnitTest(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - ProviderFactories: testAccProviders, - Steps: []resource.TestStep{ - { - Config: `resource "random_password" "min" { - length = 12 - override_special = "!#@" - min_lower = 2 - min_upper = 3 - min_special = 1 - min_numeric = 4 - }`, - Check: resource.ComposeTestCheckFunc( - testAccResourceStringCheck("random_password.min", &customLens{ - customLen: 12, - }), - regexMatch("random_password.min", regexp.MustCompile(`([a-z])`), 2), - regexMatch("random_password.min", regexp.MustCompile(`([A-Z])`), 3), - regexMatch("random_password.min", regexp.MustCompile(`([0-9])`), 4), - regexMatch("random_password.min", regexp.MustCompile(`([!#@])`), 1), - ), - }, - }, - }) -} - -func TestResourcePasswordStateUpgradeV0(t *testing.T) { - cases := []struct { - name string - stateV0 map[string]interface{} - shouldError bool - errMsg string - expectedStateV1 map[string]interface{} - }{ - { - name: "result is not string", - stateV0: map[string]interface{}{"result": 0}, - shouldError: true, - errMsg: "resource password state upgrade failed, result could not be asserted as string: int", - }, - { - name: "success", - stateV0: map[string]interface{}{"result": "abc123"}, - shouldError: false, - expectedStateV1: map[string]interface{}{"result": "abc123", "bcrypt_hash": "123"}, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - actualStateV1, err := resourcePasswordStateUpgradeV0(context.Background(), c.stateV0, nil) - - if c.shouldError { - if !cmp.Equal(c.errMsg, err.Error()) { - t.Errorf("expected: %q, got: %q", c.errMsg, err) - } - if !cmp.Equal(c.expectedStateV1, actualStateV1) { - t.Errorf("expected: %+v, got: %+v", c.expectedStateV1, err) - } - } else { - if err != nil { - t.Errorf("err should be nil, actual: %v", err) - } - - for k := range c.expectedStateV1 { - _, ok := actualStateV1[k] - if !ok { - t.Errorf("expected key: %s is missing from state", k) - } - } - } - }) - } -} diff --git a/internal/provider/resource_string.go b/internal/provider/resource_string.go index 1a184e31..9b5db706 100644 --- a/internal/provider/resource_string.go +++ b/internal/provider/resource_string.go @@ -4,10 +4,19 @@ import ( "context" "fmt" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) +// resourceString and resourcePassword both use the same set of CustomizeDiffFunc(s) in order to handle the deprecation +// of the `number` attribute and the simultaneous addition of the `numeric` attribute. planDefaultIfAllNull handles +// ensuring that both `number` and `numeric` default to `true` when they are both absent from config. +// planSyncIfChange handles keeping number and numeric in-sync when either one has been changed. func resourceString() *schema.Resource { + customizeDiffFuncs := planDefaultIfAllNull(true, "number", "numeric") + customizeDiffFuncs = append(customizeDiffFuncs, planSyncIfChange("number", "numeric")) + customizeDiffFuncs = append(customizeDiffFuncs, planSyncIfChange("numeric", "number")) + return &schema.Resource{ Description: "The resource `random_string` generates a random permutation of alphanumeric " + "characters and optionally special characters.\n" + @@ -23,11 +32,21 @@ func resourceString() *schema.Resource { // MigrateState is deprecated but the implementation is being left in place as per the // [SDK documentation](https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/resource.go#L91). MigrateState: resourceRandomStringMigrateState, - SchemaVersion: 1, - Schema: stringSchemaV1(), + SchemaVersion: 2, + Schema: stringSchemaV2(), Importer: &schema.ResourceImporter{ StateContext: importStringFunc, }, + StateUpgraders: []schema.StateUpgrader{ + { + Version: 1, + Type: resourceStringV1().CoreConfigSchema().ImpliedType(), + Upgrade: resourcePasswordStringStateUpgradeV1, + }, + }, + CustomizeDiff: customdiff.All( + customizeDiffFuncs..., + ), } } @@ -40,3 +59,9 @@ func importStringFunc(ctx context.Context, d *schema.ResourceData, meta interfac return []*schema.ResourceData{d}, nil } + +func resourceStringV1() *schema.Resource { + return &schema.Resource{ + Schema: stringSchemaV1(), + } +} diff --git a/internal/provider/resource_string_test.go b/internal/provider/resource_string_test.go index 54f2a391..5168525f 100644 --- a/internal/provider/resource_string_test.go +++ b/internal/provider/resource_string_test.go @@ -2,10 +2,11 @@ package provider import ( "fmt" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" - "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" "regexp" "testing" + + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) type customLens struct { @@ -29,7 +30,7 @@ func TestAccResourceString(t *testing.T) { ResourceName: "random_string.basic", ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"length", "lower", "number", "special", "upper", "min_lower", "min_numeric", "min_special", "min_upper", "override_special"}, + ImportStateVerifyIgnore: []string{"length", "lower", "number", "numeric", "special", "upper", "min_lower", "min_numeric", "min_special", "min_upper", "override_special"}, }, }, }) @@ -74,6 +75,272 @@ func TestAccResourceStringMin(t *testing.T) { }) } +func TestAccResourceString_UpdateNumberAndNumeric(t *testing.T) { + t.Parallel() + resource.UnitTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviders, + Steps: []resource.TestStep{ + { + Config: `resource "random_string" "default" { + length = 12 + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckResourceAttr("random_string.default", "numeric", "true"), + ), + }, + { + Config: `resource "random_string" "default" { + length = 12 + number = false + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_string.default", "number", "false"), + resource.TestCheckResourceAttr("random_string.default", "numeric", "false"), + ), + }, + { + Config: `resource "random_string" "default" { + length = 12 + numeric = true + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckResourceAttr("random_string.default", "numeric", "true"), + ), + }, + { + Config: `resource "random_string" "default" { + length = 12 + numeric = false + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_string.default", "number", "false"), + resource.TestCheckResourceAttr("random_string.default", "numeric", "false"), + ), + }, + { + Config: `resource "random_string" "default" { + length = 12 + }`, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckResourceAttr("random_string.default", "numeric", "true"), + ), + }, + }, + }) +} + +// TestAccResourceString_StateUpgraders covers the state upgrade from V1 to V2. +// This includes the addition of numeric attribute. +func TestAccResourceString_StateUpgraders(t *testing.T) { + t.Parallel() + + v1Cases := []struct { + name string + configBeforeUpgrade string + configDuringUpgrade string + beforeStateUpgrade []resource.TestCheckFunc + afterStateUpgrade []resource.TestCheckFunc + }{ + { + name: "%s number is absent", + configBeforeUpgrade: `resource "random_string" "default" { + length = 12 + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_string.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_string.default", "number", "random_string.default", "numeric"), + }, + }, + { + name: "%s number is absent then true", + configBeforeUpgrade: `resource "random_string" "default" { + length = 12 + }`, + configDuringUpgrade: `resource "random_string" "default" { + length = 12 + number = true + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_string.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_string.default", "number", "random_string.default", "numeric"), + }, + }, + { + name: "%s number is absent then false", + configBeforeUpgrade: `resource "random_string" "default" { + length = 12 + }`, + configDuringUpgrade: `resource "random_string" "default" { + length = 12 + number = false + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_string.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "false"), + resource.TestCheckResourceAttrPair("random_string.default", "number", "random_string.default", "numeric"), + }, + }, + { + name: "%s number is true", + configBeforeUpgrade: `resource "random_string" "default" { + length = 12 + number = true + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_string.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_string.default", "number", "random_string.default", "numeric"), + }, + }, + { + name: "%s number is true then absent", + configBeforeUpgrade: `resource "random_string" "default" { + length = 12 + number = true + }`, + configDuringUpgrade: `resource "random_string" "default" { + length = 12 + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_string.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_string.default", "number", "random_string.default", "numeric"), + }, + }, + { + name: "%s number is true then false", + configBeforeUpgrade: `resource "random_string" "default" { + length = 12 + number = true + }`, + configDuringUpgrade: `resource "random_string" "default" { + length = 12 + number = false + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckNoResourceAttr("random_string.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "false"), + resource.TestCheckResourceAttrPair("random_string.default", "number", "random_string.default", "numeric"), + }, + }, + { + name: "%s number is false", + configBeforeUpgrade: `resource "random_string" "default" { + length = 12 + number = false + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "false"), + resource.TestCheckNoResourceAttr("random_string.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "false"), + resource.TestCheckResourceAttrPair("random_string.default", "number", "random_string.default", "numeric"), + }, + }, + { + name: "%s number is false then absent", + configBeforeUpgrade: `resource "random_string" "default" { + length = 12 + number = false + }`, + configDuringUpgrade: `resource "random_string" "default" { + length = 12 + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "false"), + resource.TestCheckNoResourceAttr("random_string.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_string.default", "number", "random_string.default", "numeric"), + }, + }, + { + name: "%s number is false then true", + configBeforeUpgrade: `resource "random_string" "default" { + length = 12 + number = false + }`, + configDuringUpgrade: `resource "random_string" "default" { + length = 12 + number = true + }`, + beforeStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "false"), + resource.TestCheckNoResourceAttr("random_string.default", "numeric"), + }, + afterStateUpgrade: []resource.TestCheckFunc{ + resource.TestCheckResourceAttr("random_string.default", "number", "true"), + resource.TestCheckResourceAttrPair("random_string.default", "number", "random_string.default", "numeric"), + }, + }, + } + + cases := map[string][]struct { + name string + configBeforeUpgrade string + configDuringUpgrade string + beforeStateUpgrade []resource.TestCheckFunc + afterStateUpgrade []resource.TestCheckFunc + }{ + "3.2.0": v1Cases, + } + + for providerVersion, v := range cases { + for _, c := range v { + name := fmt.Sprintf(c.name, providerVersion) + t.Run(name, func(t *testing.T) { + if c.configDuringUpgrade == "" { + c.configDuringUpgrade = c.configBeforeUpgrade + } + + resource.UnitTest(t, resource.TestCase{ + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{"random": { + VersionConstraint: providerVersion, + Source: "hashicorp/random", + }}, + Config: c.configBeforeUpgrade, + Check: resource.ComposeTestCheckFunc(c.beforeStateUpgrade...), + }, + { + ProviderFactories: testAccProviders, + Config: c.configDuringUpgrade, + Check: resource.ComposeTestCheckFunc(c.afterStateUpgrade...), + }, + }, + }) + }) + } + } +} + func TestAccResourceStringErrors(t *testing.T) { resource.UnitTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, diff --git a/internal/provider/string.go b/internal/provider/string.go index 810ec5c8..33330873 100644 --- a/internal/provider/string.go +++ b/internal/provider/string.go @@ -6,15 +6,45 @@ package provider import ( "context" "crypto/rand" + "errors" "fmt" "math/big" "sort" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" ) +// passwordSchemaV2 uses passwordSchemaV1 to obtain the V1 version of the Schema key-value entries but requires that +// the numeric entry be configured and that the number entry be altered to include ConflictsWith. +func passwordSchemaV2() map[string]*schema.Schema { + passwordSchema := passwordSchemaV1() + + passwordSchema["number"] = &schema.Schema{ + Description: "Include numeric characters in the result. Default value is `true`. " + + "**NOTE**: This is deprecated, use `numeric` instead.", + Type: schema.TypeBool, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"numeric"}, + Deprecated: "Use numeric instead.", + } + + passwordSchema["numeric"] = &schema.Schema{ + Description: "Include numeric characters in the result. Default value is `true`.", + Type: schema.TypeBool, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"number"}, + } + + return passwordSchema +} + // passwordSchemaV1 uses passwordSchemaV0 to obtain the V0 version of the Schema key-value entries but requires that // the bcrypt_hash entry be configured. func passwordSchemaV1() map[string]*schema.Schema { @@ -39,6 +69,34 @@ func passwordSchemaV0() map[string]*schema.Schema { return passwordSchema } +// stringSchemaV2 uses stringSchemaV1 to obtain the V1 version of the Schema key-value entries but requires that +// the numeric entry be configured and that the number entry be altered to include ConflictsWith. +func stringSchemaV2() map[string]*schema.Schema { + stringSchema := stringSchemaV1() + + stringSchema["number"] = &schema.Schema{ + Description: "Include numeric characters in the result. Default value is `true`. " + + "**NOTE**: This is deprecated, use `numeric` instead.", + Type: schema.TypeBool, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"numeric"}, + Deprecated: "Use numeric instead.", + } + + stringSchema["numeric"] = &schema.Schema{ + Description: "Include numeric characters in the result. Default value is `true`.", + Type: schema.TypeBool, + Optional: true, + Computed: true, + ForceNew: true, + ConflictsWith: []string{"number"}, + } + + return stringSchema +} + // stringSchemaV1 uses passwordStringSchema to obtain the default Schema key-value entries but requires that the id // description be configured. func stringSchemaV1() map[string]*schema.Schema { @@ -155,8 +213,8 @@ func passwordStringSchema() map[string]*schema.Schema { } } -func createStringFunc(sensitive bool) func(_ context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { - return func(_ context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { +func createStringFunc(sensitive bool) func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics { + return func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics { const numChars = "0123456789" const lowerChars = "abcdefghijklmnopqrstuvwxyz" const upperChars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" @@ -171,6 +229,7 @@ func createStringFunc(sensitive bool) func(_ context.Context, d *schema.Resource lower := d.Get("lower").(bool) minLower := d.Get("min_lower").(int) number := d.Get("number").(bool) + numeric := d.Get("numeric").(bool) minNumeric := d.Get("min_numeric").(int) special := d.Get("special").(bool) minSpecial := d.Get("min_special").(int) @@ -194,7 +253,7 @@ func createStringFunc(sensitive bool) func(_ context.Context, d *schema.Resource if lower { chars += lowerChars } - if number { + if numeric { chars += numChars } if special { @@ -232,6 +291,13 @@ func createStringFunc(sensitive bool) func(_ context.Context, d *schema.Resource return append(diags, diag.Errorf("error setting result: %s", err)...) } + if err := d.Set("number", number); err != nil { + return append(diags, diag.Errorf("error setting number: %s", err)...) + } + if err := d.Set("numeric", numeric); err != nil { + return append(diags, diag.Errorf("error setting numeric: %s", err)...) + } + if sensitive { d.SetId("none") } else { @@ -257,3 +323,76 @@ func generateRandomBytes(charSet *string, length int) ([]byte, error) { func readNil(_ context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { return nil } + +func resourcePasswordStringStateUpgradeV1(_ context.Context, rawState map[string]interface{}, _ interface{}) (map[string]interface{}, error) { + if rawState == nil { + return nil, errors.New("state upgrade failed, state is nil") + } + + number, ok := rawState["number"].(bool) + if !ok { + return nil, fmt.Errorf("state upgrade failed, number is not a boolean: %T", rawState["number"]) + } + + rawState["numeric"] = number + + return rawState, nil +} + +// planDefaultIfAllNull handles ensuring that both `number` and `numeric` attributes default to `true` when neither are set +// in the config and, they had been previously set to `false`. This behaviour mimics setting `Default: true` on the +// attributes. Usage of `Default` is avoided as `Default` cannot be used with CustomizeDiffFunc(s) which are required in +// order to keep `number` and `numeric` in-sync (see planSyncIfChange). +func planDefaultIfAllNull(defaultVal interface{}, keys ...string) []schema.CustomizeDiffFunc { + var result []schema.CustomizeDiffFunc + + for _, key := range keys { + result = append(result, customdiff.IfValue( + key, + func(ctx context.Context, value, meta interface{}) bool { + return !value.(bool) + }, + func(_ context.Context, d *schema.ResourceDiff, _ interface{}) error { + vm := d.GetRawConfig().AsValueMap() + + number, ok := vm["number"] + if !ok { + return errors.New("number is absent from raw config") + } + + numeric, ok := vm["numeric"] + if !ok { + return errors.New("numeric is absent from raw config") + } + + if number.IsNull() && numeric.IsNull() { + err := d.SetNew("number", defaultVal) + if err != nil { + return err + } + err = d.SetNew("numeric", defaultVal) + if err != nil { + return err + } + } + return nil + }, + )) + } + + return result +} + +// planSyncIfChange handles keeping `number` and `numeric` in-sync. If either is changed the value of both is +// set to the new value of the attribute that has changed. +func planSyncIfChange(key, keyToSync string) func(context.Context, *schema.ResourceDiff, interface{}) error { + return customdiff.IfValueChange( + key, + func(ctx context.Context, oldValue, newValue, meta interface{}) bool { + return oldValue != newValue + }, + func(_ context.Context, d *schema.ResourceDiff, _ interface{}) error { + return d.SetNew(keyToSync, d.Get(key)) + }, + ) +} diff --git a/internal/provider/string_test.go b/internal/provider/string_test.go new file mode 100644 index 00000000..e4ecf3b3 --- /dev/null +++ b/internal/provider/string_test.go @@ -0,0 +1,57 @@ +package provider + +import ( + "context" + "errors" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestResourcePasswordStringStateUpgradeV1(t *testing.T) { + cases := []struct { + name string + stateV1 map[string]interface{} + err error + expectedStateV2 map[string]interface{} + }{ + { + name: "raw state is nil", + stateV1: nil, + err: errors.New("state upgrade failed, state is nil"), + }, + { + name: "number is not bool", + stateV1: map[string]interface{}{"number": 0}, + err: errors.New("state upgrade failed, number is not a boolean: int"), + }, + { + name: "success", + stateV1: map[string]interface{}{"number": true}, + expectedStateV2: map[string]interface{}{"number": true, "numeric": true}, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + actualStateV2, err := resourcePasswordStringStateUpgradeV1(context.Background(), c.stateV1, nil) + + if c.err != nil { + if !cmp.Equal(c.err.Error(), err.Error()) { + t.Errorf("expected: %q, got: %q", c.err.Error(), err) + } + if !cmp.Equal(c.expectedStateV2, actualStateV2) { + t.Errorf("expected: %+v, got: %+v", c.expectedStateV2, err) + } + } else { + if err != nil { + t.Errorf("err should be nil, actual: %v", err) + } + + if !cmp.Equal(actualStateV2, c.expectedStateV2) { + t.Errorf("expected: %v, got: %v", c.expectedStateV2, actualStateV2) + } + } + }) + } +}