Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_key_vault_key - address issue with contacts data plane access when public network access is disabled #25124

Closed
wants to merge 4 commits into from
Closed

azurerm_key_vault_key - address issue with contacts data plane access when public network access is disabled #25124

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f7e907e
Initial Check-in...
WodansSon Mar 3, 2024
9bef028
Tweak logic a bit for data plane access...
WodansSon Mar 3, 2024
306f22c
Wrap 'contact' in FourPointOhBeta feature flag...
WodansSon Mar 7, 2024
c71f688
Address PR comments...
WodansSon Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 85 additions & 15 deletions internal/services/keyvault/key_vault_certificate_contacts_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ func (r KeyVaultCertificateContactsResource) Arguments() map[string]*pluginsdk.S

"contact": {
Type: pluginsdk.TypeSet,
Required: true,
MinItems: 1,
Optional: true,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"email": {
Expand Down Expand Up @@ -104,7 +103,7 @@ func (r KeyVaultCertificateContactsResource) Create() sdk.ResourceFunc {

keyVaultBaseUri, err := vaultClient.BaseUriForKeyVault(ctx, *keyVaultId)
if err != nil {
return fmt.Errorf("looking up Base URI for Key Vault Certificate Contacts from %s: %+v", *keyVaultId, err)
return fmt.Errorf("retrieving Base URI for %s: %+v", *keyVaultId, err)
}

id, err := parse.NewCertificateContactsID(*keyVaultBaseUri)
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, shall we just error out as the user's intent is to create the resource, but lacks of permission?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

// being disabled just ignore the error and set the ID...
if utils.ResponseWasForbidden(existing.Response) {
metadata.SetID(id)
return nil
}

if !utils.ResponseWasNotFound(existing.Response) {
return fmt.Errorf("checking for presence of existing Certificate Contacts (Key Vault %q): %s", *keyVaultBaseUri, err)
return fmt.Errorf("retrieving existing Certificate Contacts for %s from key vault base URI: %q: %+v", *keyVaultId, *keyVaultBaseUri, err)
}
}

Expand All @@ -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...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 len(*contacts.ContactList) != 0 {
if _, err := client.SetCertificateContacts(ctx, *keyVaultBaseUri, contacts); err != nil {
return fmt.Errorf("creating Key Vault Certificate Contacts %s: %+v", id, err)
}
} else {
metadata.Logger.Infof("[CREATE] Contacts are empty - NoOp, but still set the ID since the resource was defined in the configuration file")
}

metadata.SetID(id)
Expand All @@ -158,24 +169,43 @@ func (r KeyVaultCertificateContactsResource) Read() sdk.ResourceFunc {
subscriptionResourceId := commonids.NewSubscriptionID(subscriptionId)
keyVaultIdRaw, err := vaultClient.KeyVaultIDFromBaseUrl(ctx, subscriptionResourceId, id.KeyVaultBaseUrl)
if err != nil {
return fmt.Errorf("retrieving resource ID of the Key Vault at URL %s: %+v", id.KeyVaultBaseUrl, err)
return fmt.Errorf("retrieving resource ID of the Key Vault from Key Vault Base URL %q in %s: %+v", id.KeyVaultBaseUrl, subscriptionResourceId, err)
}

if keyVaultIdRaw == nil {
metadata.Logger.Infof("Unable to determine the Resource ID for the Key Vault at URL %s - removing from state!", id.KeyVaultBaseUrl)
metadata.Logger.Infof("[READ] unable to retrieve Key Vault resource ID from Key Vault Base URL %q in %s - removing from state", id.KeyVaultBaseUrl, subscriptionResourceId)
return metadata.MarkAsGone(id)
}

keyVaultId, err := commonids.ParseKeyVaultID(*keyVaultIdRaw)
if err != nil {
return fmt.Errorf("parsing Key Vault ID: %+v", err)
}

existing, err := client.GetCertificateContacts(ctx, id.KeyVaultBaseUrl)
if err != nil {
// The contacts may or may not have changed but we do not have access to them due
// to the key vault public network access has been disabled... should just exit
// since we have no access to dataplane... This will preserve the contacts
// in the state file so once we do regain access to the dataplane we will be able
// to diff the resources contacts with the key vaults contacts...
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
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated above, that is prolly more consistent with standard provider behavior, updated to return and error instead of swallowing the error.


if utils.ResponseWasNotFound(existing.Response) {
metadata.Logger.Infof("No Certificate Contacts could be found at %s - removing from state!", id.KeyVaultBaseUrl)
return metadata.MarkAsGone(id)
metadata.Logger.Infof("[READ] no certificate contacts were returned from key vault URL %s in %s - set empty contact list to state", id.KeyVaultBaseUrl, subscriptionResourceId)

state := KeyVaultCertificateContactsResourceModel{
KeyVaultId: keyVaultId.ID(),
Contact: make([]Contact, 0),
}

return metadata.Encode(&state)
Comment on lines +202 to +207
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@WodansSon WodansSon Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

return fmt.Errorf("retrieving existing Certificate Contacts from key vault base URL: %q in %s: %+v", id.KeyVaultBaseUrl, subscriptionResourceId, err)
}

state := KeyVaultCertificateContactsResourceModel{
Expand Down Expand Up @@ -209,14 +239,34 @@ func (r KeyVaultCertificateContactsResource) Update() sdk.ResourceFunc {

existing, err := client.GetCertificateContacts(ctx, id.KeyVaultBaseUrl)
if err != nil {
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, now return error base on data plane permissions...

metadata.Logger.Infof("'GetCertificateContacts' error result was 'Forbidden' for key vault URL %s - dataplane access denied, keep current state", id.KeyVaultBaseUrl)
return nil
}

// Since contacts can now be empty Not Found is no longer a reason to return
// an error...
if !utils.ResponseWasNotFound(existing.Response) {
return fmt.Errorf("retrieving existing Certificate Contacts from key vault base URL: %q: %+v", id.KeyVaultBaseUrl, err)
} else {
metadata.Logger.Infof("'GetCertificateContacts' error result was 'NotFound' for key vault URL %s - ignore error", id.KeyVaultBaseUrl)
}
}

if metadata.ResourceData.HasChange("contact") {
existing.ContactList = expandKeyVaultCertificateContactsContact(state.Contact)
}

if _, err := client.SetCertificateContacts(ctx, id.KeyVaultBaseUrl, existing); err != nil {
var updateErr error
if len(*existing.ContactList) == 0 {
_, updateErr = client.DeleteCertificateContacts(ctx, id.KeyVaultBaseUrl)
} else {
_, updateErr = client.SetCertificateContacts(ctx, id.KeyVaultBaseUrl, existing)
}

if updateErr != nil {
return fmt.Errorf("updating Key Vault Certificate Contacts %s: %+v", id, err)
}

Expand All @@ -239,8 +289,28 @@ func (r KeyVaultCertificateContactsResource) Delete() sdk.ResourceFunc {
locks.ByID(id.ID())
defer locks.UnlockByID(id.ID())

if _, err := client.DeleteCertificateContacts(ctx, id.KeyVaultBaseUrl); err != nil {
return fmt.Errorf("deleting %s: %+v", id, err)
// first check to see if contacts exists or not, if they do
// delete them, else just remove them from the state file...
existing, err := client.GetCertificateContacts(ctx, id.KeyVaultBaseUrl)

if err != nil {
// If we don't have access to the dataplane due to the public network access
// being disabled just return error and keep the state unchanged...
if utils.ResponseWasForbidden(existing.Response) {
metadata.Logger.Infof("[DELETE] 'GetCertificateContacts' error result was 'Forbidden' for key vault URL %s - dataplane access denied, keep current state and return error", id.KeyVaultBaseUrl)
return fmt.Errorf("unable to delete %s due to data plane access restrictions: %+v", id, err)
}

// Since contacts can now be empty Not Found is no longer a reason to return an error...
if !utils.ResponseWasNotFound(existing.Response) {
// The GET call found Key Vault contacts, try to delete them...
if _, err := client.DeleteCertificateContacts(ctx, id.KeyVaultBaseUrl); err != nil {
return fmt.Errorf("deleting %s: %+v", id, err)
}
} else {
metadata.Logger.Infof("[DELETE] 'GetCertificateContacts' error result was 'NotFound' for key vault URL %s - removing from state file", id.KeyVaultBaseUrl)
return metadata.MarkAsGone(id)
}
}

return nil
Expand Down
Loading
Loading