Add a hash to random_password with StateUpgrader #254
Conversation
| return diags | ||
| } | ||
|
|
||
| if d.Get("bcrypt_hash") == "" { |
There was a problem hiding this comment.
This attribute will always be empty on creation, so the conditional is unnecessary. 👍
There was a problem hiding this comment.
Removed conditional.
|
|
||
| hash, err := generateHash(result) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("resource password state upgrade failed, generate hash error: %v", err) |
There was a problem hiding this comment.
Conventionally after Go 1.13, it is recommended to use the %w (wrapped error) formatter for errors.
This does leave an interesting question though, what errors can be generated and are they recoverable? If they are not recoverable (e.g. cannot be fixed by retrying), practitioners will be forever stuck with an unusable resource unless they pin to an older provider version or the state upgrade code is "fixed" or updated to skip on the generation error.
There was a problem hiding this comment.
Have wrapped the error.
The errors that can be generated relate to:
- checkCost - this should never return an error as we're using DefaultCost.
- io.ReadFull - this will return an error if maxSaltSize bytes cannot be read from rand.Reader.
- bcrypt - can return error if underlying cipher funcs return error.
| ) | ||
|
|
||
| func resourcePassword() *schema.Resource { | ||
| passwordSchema := stringSchemaV1(true) | ||
| passwordSchema["bcrypt_hash"] = &schema.Schema{ |
There was a problem hiding this comment.
Since this new attribute is only handled on Create, e.g. not during Read, it will also need to be handled during Import.
There was a problem hiding this comment.
Have added handling of bcrypt_hash to import.
| ) | ||
|
|
||
| func resourcePassword() *schema.Resource { | ||
| create := func(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { |
There was a problem hiding this comment.
Nitpick: Why is this function defined inside inside the other function (lamba)?
I can't spot anything that would require this approach ❓
| } | ||
| } | ||
|
|
||
| func importPasswordFunc() schema.StateContextFunc { |
There was a problem hiding this comment.
Why do you need a function that returns a function here?
internal/provider/resource_string.go
Outdated
| }, | ||
| } | ||
| } | ||
|
|
||
| func importStringFunc() schema.StateContextFunc { |
There was a problem hiding this comment.
Same as above: having this function returning another function seem not necessary.
But I might be missing something.
| // 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 { | ||
| passwordSchema := passwordSchemaV0() | ||
| passwordSchema["bcrypt_hash"] = &schema.Schema{ | ||
| Description: "A bcrypt hash of the generated random string.", | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| Sensitive: true, | ||
| } | ||
|
|
||
| return passwordSchema | ||
| } | ||
|
|
||
| // passwordSchemaV0 uses passwordStringSchema to obtain the default Schema key-value entries but requires that the id | ||
| // description, result sensitive and bcrypt_hash entries be configured. | ||
| func passwordSchemaV0() map[string]*schema.Schema { | ||
| passwordSchema := passwordStringSchema() | ||
| passwordSchema["id"].Description = "A static value used internally by Terraform, this should not be referenced in configurations." | ||
| passwordSchema["result"].Sensitive = true | ||
|
|
||
| return passwordSchema | ||
| } | ||
|
|
||
| // 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 { | ||
| stringSchema := passwordStringSchema() | ||
| stringSchema["id"].Description = "The generated random string." | ||
|
|
||
| return stringSchema | ||
| } | ||
|
|
||
| // passwordStringSchema returns map[string]*schema.Schema with all keys and values that are common to both the | ||
| // password and string resources. |
There was a problem hiding this comment.
I like that all the schemas are now next to each other.
Co-authored-by: Ramon Rüttimann <[email protected]>
|
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. |
This is evaluating whether the changes proposed in #73 can be combined with a StateUpgrader to avoid the situation where including the
bcrypt_hashfield generates an empty value in state on first use.Closes #73
Closes #102