azurerm_key_vault_key - address issue with contacts data plane access when public network access is disabled
#25124
Conversation
There was a problem hiding this comment.
Thanks for this PR!
I've taken a look through and left some comments inline. The main point from me is that we shall reach out to the KV team to ask them to support an empty certificate contact resource (i.e. 0 contact defined), which will then makes things much easier.
| @@ -117,8 +116,15 @@ func (r KeyVaultCertificateContactsResource) Create() sdk.ResourceFunc { | |||
|
|
|||
| existing, err := client.GetCertificateContacts(ctx, *keyVaultBaseUri) | |||
| if err != nil { | |||
| // If we don't have access to the dataplane due to the public network access | |||
There was a problem hiding this comment.
In this case, shall we just error out as the user's intent is to create the resource, but lacks of permission?
There was a problem hiding this comment.
I am not exactly sure what the behavior should be here tbh... but returning an error is prolly more consistent with the provider so that is prolly a good call...
| @@ -132,8 +138,13 @@ func (r KeyVaultCertificateContactsResource) Create() sdk.ResourceFunc { | |||
| ContactList: expandKeyVaultCertificateContactsContact(state.Contact), | |||
| } | |||
|
|
|||
| if _, err := client.SetCertificateContacts(ctx, *keyVaultBaseUri, contacts); err != nil { | |||
| return fmt.Errorf("creating Key Vault Certificate Contacts %s: %+v", id, err) | |||
| // Don't set the contacts unless there are contacts defined... | |||
There was a problem hiding this comment.
Can we put more texts here to explain why an empty contact resource is useful here (per my understanding, it is used as a guard to ensure any out of band updates to this resource will be tracked)?
Whilst one caveat is that it then allows users to define multiple such resources for a single key vault, which violates the fact that the contact should be a singleton.
There was a problem hiding this comment.
Agreed, added more inline comments to explain why an empty resource will be created if the contact field is empty. As for the singleton, I am not currently aware of a way within the provider code to enforce singleton resource creation. I suspect that would be handled by the RP returning the conflict error, stating that there are multiple concurrent updates to the resource. The first one should succeed while the additional updates should fail with the conflict error?
There was a problem hiding this comment.
The timing of the updates of these two resources might happen to be in sequence, in which case even the RP has the concurrent update check, it still won't help.
| if utils.ResponseWasForbidden(existing.Response) { | ||
| metadata.Logger.Infof("[READ] unable to enumerate key vault certificate contacts at URL %s in %s - ignore error", id.KeyVaultBaseUrl, subscriptionResourceId) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I still think in this case we shall just error out, as it is similar to a mgmt plane resource that exists but you don't have permission to access it?
There was a problem hiding this comment.
As stated above, that is prolly more consistent with standard provider behavior, updated to return and error instead of swallowing the error.
| state := KeyVaultCertificateContactsResourceModel{ | ||
| KeyVaultId: keyVaultId.ID(), | ||
| Contact: make([]Contact, 0), | ||
| } | ||
|
|
||
| return metadata.Encode(&state) |
There was a problem hiding this comment.
This then disables one common Terraform behavior that removes non-exist resource from state. Instead, I think we shall reach to the KV service team to ask them to allow creating an empty contact resource (with 0 contact), then things will be much easier.
There was a problem hiding this comment.
I understand what you are saying, however the core runtime requires all fields to have a value (even if empty). I added this code as a stop gap measure, between the RP and Terraform, that said if we can get the service team to agree to allow empty key vault contacts that would be ideal, however I am not sure how successful we will be in convincing them to do so, which is why I introduced this wrapper code as it is sort of equivalent.
| return fmt.Errorf("checking for presence of existing Certificate Contacts (Key Vault %q): %s", id.KeyVaultBaseUrl, err) | ||
| // If we don't have access to the dataplane due to the public network access | ||
| // being disabled just return and keep the state unchanged... | ||
| if utils.ResponseWasForbidden(existing.Response) { |
There was a problem hiding this comment.
As above, now return error base on data plane permissions...
|
This PR is being labeled as "stale" because it has not been updated for 30 or more days. If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone. If you need some help completing this PR, please leave a comment letting us know. Thank you! |
|
Glad to see this PR as we are currently affected from the same issue. Is this really stale at the moment? |
|
This PR is being labeled as "stale" because it has not been updated for 30 or more days. If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone. If you need some help completing this PR, please leave a comment letting us know. Thank you! |
|
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. |
(Fixes #9738)
This is just a POC that I am working on, while it fixes the issues I am specifically working on at the moment it may not address all issue related to #9738, but I think it will solve a majority of them.