Add a hash to random_password with UpgradeState #255
Conversation
internal/provider/diagnostics.go
Outdated
| diags.AddError( | ||
| "Hash Generation Error", | ||
| "While attempting to generate a hash from of the password an error occurred.\n\n"+ | ||
| "Verify that the state contains a populated 'result' field and retry the operation\n\n"+ |
There was a problem hiding this comment.
Might be worth mentioning the terraform state show command. 👍
There was a problem hiding this comment.
Added comment to use terraform state show.
internal/provider/string.go
Outdated
| } | ||
| } | ||
|
|
||
| func importPassword(ctx context.Context, req tfsdk.ImportResourceStateRequest, resp *tfsdk.ImportResourceStateResponse) { |
There was a problem hiding this comment.
Similar to the note on the sdk/v2 version of this, since the new attribute is only handled once on creation, it will also need to be handled during import. It may be desirable to optionally include it in the import ID so folks can have a stable value when doing terraform state rm and terraform import, although that does introduce yet more complexity. Maybe that can be punted on until there is a specific ask.
There was a problem hiding this comment.
Alternatively, could the bcrypt_hash value be just computed during import?
After all what matters about it is not strictly having a specific value, but that it's stable once generated.
I'd leave out the option to import it with the ID only if it comes up again, and hopefully deferred to when "import-from-config" is a thing.
There was a problem hiding this comment.
I've updated importPassword to handle bcrypt_hash during import.
I'm inclined to defer handling of a passed bcrypt_hash during import until/unless it is specifically requested.
internal/provider/string.go
Outdated
| } | ||
| } | ||
|
|
||
| func importPassword(ctx context.Context, req tfsdk.ImportResourceStateRequest, resp *tfsdk.ImportResourceStateResponse) { |
There was a problem hiding this comment.
Alternatively, could the bcrypt_hash value be just computed during import?
After all what matters about it is not strictly having a specific value, but that it's stable once generated.
I'd leave out the option to import it with the ID only if it comes up again, and hopefully deferred to when "import-from-config" is a thing.
| @@ -15,7 +17,17 @@ func (r resourcePasswordType) GetSchema(context.Context) (tfsdk.Schema, diag.Dia | |||
| "data handling in the [Terraform documentation](https://www.terraform.io/docs/language/state/sensitive-data.html).\n" + | |||
| "\n" + | |||
| "This resource *does* use a cryptographic random number generator." | |||
| return getStringSchemaV1(true, description), nil | |||
|
|
|||
| schema := getStringSchemaV1(true, description) | |||
There was a problem hiding this comment.
Nit: the name of the function getStringSchemaV1 makes one thing that the schema that comes out of it is V1.
But it's not right? It's V0. V1 is what you are creating now, by adding bcrypt_hash.
IMHO: I'd try to preserve consistency and have 2 functions able to provide the schema, and the version correctly respected in their name.
There was a problem hiding this comment.
Good point. Have refactored.
| 0: { | ||
| StateUpgrader: migratePasswordStateV0toV1, | ||
| }, |
There was a problem hiding this comment.
Any particular reason to opt for handling RawState in the StateUpgrader versus specifying the version 0 schema here in the PriorSchema field? If you use PriorSchema, you can call req.PriorState.Get() to populate the already defined data type.
There was a problem hiding this comment.
Yep, I went the route of using RawState as req.State is nil in the tfsdk.UpgradeResourceStateRequest and I can't see a PriorState field in req.
There was a problem hiding this comment.
Sorry I meant, UpgradeResourceStateRequest.State. It get's filled in when ResourceStateUpgrader.PriorSchema is defined. It should make the logic a lot easier.
There was a problem hiding this comment.
Thanks for the clarification. I've refactored to use StateUpgrader with PriorSchema.
|
|
||
| err := bcrypt.CompareHashAndPassword([]byte(actual.BcryptHash.Value), []byte(actual.Result.Value)) | ||
| if err != nil { | ||
| t.Error(err) |
There was a problem hiding this comment.
Nit: Might be helpful to add error context here, e.g.
| t.Error(err) | |
| t.Errorf("unexpected bcrypt comparison error: %s", err) |
There was a problem hiding this comment.
Missed this before merging but have added into #249
|
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 UpgradeState once the provider has been upgraded to use Framework.
Further test coverage is required.