identity/group-alias: add api client lock - #2140
Conversation
| * Add support for configuration of plugin WIF to the AWS Secret Backend. Requires Vault 1.16+ ([#2138](https://github.com/hashicorp/terraform-provider-vault/pull/2138)). | ||
| * Add support for Oracle database plugin configuration options `split_statements` and `disconnect_sessions`: ([#2085](https://github.com/hashicorp/terraform-provider-vault/pull/2085)) | ||
|
|
||
| IMPROVEMENTS: |
There was a problem hiding this comment.
Or BUG? I went with improvement since I have not been able to verify that this fixes the reported error.
|
Looking into the test timeout failures: https://github.com/hashicorp/terraform-provider-vault/actions/runs/7890026265/job/21531339605 |
vinay-gopalan
left a comment
vinay-gopalan
left a comment
There was a problem hiding this comment.
Thanks for looking into this! Though it is hard to come up with a testcase using just the TF testing framework that can validate the exact fix for the issue, I do think this is a valid addition to the code, as it matches the Group Alias resource behavior to the Entity Alias resource (both of which support performance secondaries and standbys). Had 1 nit but looks good to me otherwise 👍🏼
| } | ||
|
|
||
| func identityGroupAliasCreate(d *schema.ResourceData, meta interface{}) error { | ||
| lock, unlock := getEntityLockFuncs(d, identityGroupAliasIDPath) |
There was a problem hiding this comment.
nit: is it worth updating the function name to something more generic, like getLockFuncs since it is not specific to entities?
There was a problem hiding this comment.
I think I will keep it as-is since an Identity Group is a collection of Entities and the lock keys that we create are specific to entities in that the key is tied to the mount accessor. I don't see this function being used much or at all outside of entities use cases.
2 participants
Description
Relates #1695
I haven't been able to reproduce the issue from #1695 yet, but the lack of locking here is quite suspicious.
Checklist