From 52582139d2efb88abc50f4d9744e1528820819a4 Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Fri, 26 Apr 2024 23:52:51 -0600 Subject: [PATCH 01/12] Initial Check-in... --- .../services/keyvault/key_vault_resource.go | 195 ++++++++---------- 1 file changed, 90 insertions(+), 105 deletions(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index 7dffc492d584..24bbc5fb263a 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -21,7 +21,6 @@ import ( "github.com/hashicorp/terraform-provider-azurerm/helpers/tf" commonValidate "github.com/hashicorp/terraform-provider-azurerm/helpers/validate" "github.com/hashicorp/terraform-provider-azurerm/internal/clients" - "github.com/hashicorp/terraform-provider-azurerm/internal/features" "github.com/hashicorp/terraform-provider-azurerm/internal/locks" "github.com/hashicorp/terraform-provider-azurerm/internal/services/keyvault/migration" "github.com/hashicorp/terraform-provider-azurerm/internal/services/keyvault/validate" @@ -37,7 +36,7 @@ import ( var keyVaultResourceName = "azurerm_key_vault" func resourceKeyVault() *pluginsdk.Resource { - resource := pluginsdk.Resource{ + return &pluginsdk.Resource{ Create: resourceKeyVaultCreate, Read: resourceKeyVaultRead, Update: resourceKeyVaultUpdate, @@ -119,6 +118,36 @@ func resourceKeyVault() *pluginsdk.Resource { }, }, + // NOTE: To unblock customers where they had previously deployed a key vault with + // contacts, but cannot re-deploy the key vault. I am adding support for the contact + // field back into the resource for UPDATE ONLY. If this is a new resource and the + // contact field is defined in the configuration file it will now throw an error. + // This will allow legacy key vaults to continue to work as the previously have + // and enforces our new model of seperating out the data plane call into its + // own resource (e.g., contacts)... + "contact": { + Type: pluginsdk.TypeSet, + Optional: true, + Computed: true, + Deprecated: "As the `contact` property requires reaching out to the dataplane, to better support private endpoints and keyvaults with public network access disabled, new key vaults with the `contact` field defined in the configuration file will now be required to use the `azurerm_key_vault_certificate_contacts` resource instead of the exposed `contact` field in the key vault resource itself.", + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "email": { + Type: pluginsdk.TypeString, + Required: true, + }, + "name": { + Type: pluginsdk.TypeString, + Optional: true, + }, + "phone": { + Type: pluginsdk.TypeString, + Optional: true, + }, + }, + }, + }, + "enabled_for_deployment": { Type: pluginsdk.TypeBool, Optional: true, @@ -212,39 +241,11 @@ func resourceKeyVault() *pluginsdk.Resource { }, }, } - - if !features.FourPointOhBeta() { - resource.Schema["contact"] = &pluginsdk.Schema{ - Type: pluginsdk.TypeSet, - Optional: true, - Computed: true, - Deprecated: "As the `contact` property requires reaching out to the dataplane, to better support private endpoints and keyvaults with public network access disabled, `contact` will be removed in favour of the `azurerm_key_vault_certificate_contacts` resource in version 4.0 of the AzureRM Provider.", - Elem: &pluginsdk.Resource{ - Schema: map[string]*pluginsdk.Schema{ - "email": { - Type: pluginsdk.TypeString, - Required: true, - }, - "name": { - Type: pluginsdk.TypeString, - Optional: true, - }, - "phone": { - Type: pluginsdk.TypeString, - Optional: true, - }, - }, - }, - } - } - - return &resource } func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { subscriptionId := meta.(*clients.Client).Account.SubscriptionId client := meta.(*clients.Client).KeyVault.VaultsClient - dataPlaneClient := meta.(*clients.Client).KeyVault.ManagementClient // TODO: Remove in 4.0 ctx, cancel := timeouts.ForCreate(meta.(*clients.Client).StopContext, d) defer cancel() @@ -256,6 +257,15 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { locks.ByName(id.VaultName, keyVaultResourceName) defer locks.UnlockByName(id.VaultName, keyVaultResourceName) + isPublic := d.Get("public_network_access_enabled").(bool) + contactRaw := d.Get("contact").(*pluginsdk.Set).List() + + // NOTE: Block the creation of net new key vault resources where + // the contact field has been defined... + if len(contactRaw) > 0 { + return fmt.Errorf("%s: The `contact` field is not supported for new key vaults", id) + } + // check for the presence of an existing, live one which should be imported into the state existing, err := client.Get(ctx, id) if err != nil { @@ -289,15 +299,6 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { recoverSoftDeletedKeyVault = true } - isPublic := d.Get("public_network_access_enabled").(bool) - contactRaw := d.Get("contact").(*pluginsdk.Set).List() // TODO: Remove in 4.0 - - if !features.FourPointOhBeta() { - if !isPublic && len(contactRaw) > 0 { - return fmt.Errorf("`contact` cannot be specified when `public_network_access_enabled` is set to `false`") - } - } - tenantUUID := d.Get("tenant_id").(string) enabledForDeployment := d.Get("enabled_for_deployment").(bool) enabledForDiskEncryption := d.Get("enabled_for_disk_encryption").(bool) @@ -378,16 +379,20 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { if err != nil { return fmt.Errorf("retrieving %s: %+v", id, err) } + vaultUri := "" if model := read.Model; model != nil { if model.Properties.VaultUri != nil { vaultUri = *model.Properties.VaultUri } } + if vaultUri == "" { return fmt.Errorf("retrieving %s: `properties.VaultUri` was nil", id) } + d.SetId(id.ID()) + meta.(*clients.Client).KeyVault.AddToCache(id, vaultUri) // When Public Network Access is Enabled (i.e. it's Public) we can hit the Data Plane API until @@ -409,6 +414,7 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { if !ok { return fmt.Errorf("internal-error: context had no deadline") } + stateConf := &pluginsdk.StateChangeConf{ Pending: []string{"pending"}, Target: []string{"available"}, @@ -418,26 +424,12 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { ContinuousTargetOccurence: 10, Timeout: time.Until(deadline), } + if _, err := stateConf.WaitForStateContext(ctx); err != nil { return fmt.Errorf("waiting for %s to become available: %s", id, err) } } - if !features.FourPointOhBeta() { - if len(contactRaw) > 0 { - if !isPublic { - return fmt.Errorf("`contact` cannot be specified when `public_network_access_enabled` is set to `false`") - } - - contacts := dataplane.Contacts{ - ContactList: expandKeyVaultCertificateContactList(contactRaw), - } - if _, err := dataPlaneClient.SetCertificateContacts(ctx, vaultUri, contacts); err != nil { - return fmt.Errorf("failed to set Contacts for %s: %+v", id, err) - } - } - } - return resourceKeyVaultRead(d, meta) } @@ -464,13 +456,9 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { if err != nil { return fmt.Errorf("retrieving %s: %+v", *id, err) } - if existing.Model == nil { - return fmt.Errorf("retrieving %s: `properties` was nil", *id) - } - isPublic := true - if model := existing.Model; model != nil && model.Properties.PublicNetworkAccess != nil { - isPublic = strings.EqualFold(*model.Properties.PublicNetworkAccess, "Enabled") + if existing.Model == nil { + return fmt.Errorf("retrieving %s: `Model` was nil", *id) } update := vaults.VaultPatchParameters{} @@ -578,7 +566,7 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { update.Properties = &vaults.VaultPatchProperties{} } - isPublic = d.Get("public_network_access_enabled").(bool) + isPublic := d.Get("public_network_access_enabled").(bool) if isPublic { update.Properties.PublicNetworkAccess = utils.String("Enabled") } else { @@ -635,32 +623,33 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { return fmt.Errorf("updating %s: %+v", *id, err) } - if !features.FourPointOhBeta() { - if d.HasChange("contact") { - if !isPublic { - return fmt.Errorf("`contact` cannot be specified when `public_network_access_enabled` is set to `false`") - } - contacts := dataplane.Contacts{ - ContactList: expandKeyVaultCertificateContactList(d.Get("contact").(*pluginsdk.Set).List()), - } - vaultUri := "" - if existing.Model != nil && existing.Model.Properties.VaultUri != nil { - vaultUri = *existing.Model.Properties.VaultUri - } - if vaultUri == "" { - return fmt.Errorf("failed to get vault base url for %s: %s", *id, err) - } + if d.HasChange("contact") { + contacts := dataplane.Contacts{ + ContactList: expandKeyVaultCertificateContactList(d.Get("contact").(*pluginsdk.Set).List()), + } - var err error - if len(*contacts.ContactList) == 0 { - _, err = managementClient.DeleteCertificateContacts(ctx, vaultUri) - } else { - _, err = managementClient.SetCertificateContacts(ctx, vaultUri, contacts) - } + vaultUri := "" + if existing.Model != nil && existing.Model.Properties.VaultUri != nil { + vaultUri = *existing.Model.Properties.VaultUri + } - if err != nil { - return fmt.Errorf("setting Contacts for %s: %+v", *id, err) - } + if vaultUri == "" { + return fmt.Errorf("failed to get vault base url for %s: %s", *id, err) + } + + var err error + var resp dataplane.Contacts + if len(*contacts.ContactList) == 0 { + resp, err = managementClient.DeleteCertificateContacts(ctx, vaultUri) + } else { + resp, err = managementClient.SetCertificateContacts(ctx, vaultUri, contacts) + } + + // NOTE: For legecy backwards compatibility, we need to ignore the Forbidden response + // here. Many of our customers will have already set up private endpoints so the above + // data plane calls would succeed... + if err != nil && !utils.ResponseWasForbidden(resp.Response) { + return fmt.Errorf("setting Contacts for %s: %+v", *id, err) } } @@ -671,7 +660,7 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { client := meta.(*clients.Client).KeyVault.VaultsClient - dataplaneClient := meta.(*clients.Client).KeyVault.ManagementClient // TODO: Remove in 4.0 + dataplaneClient := meta.(*clients.Client).KeyVault.ManagementClient ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) defer cancel() @@ -691,31 +680,30 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { } vaultUri := "" - isPublic := true if model := resp.Model; model != nil { if model.Properties.VaultUri != nil { vaultUri = *model.Properties.VaultUri } - if model.Properties.PublicNetworkAccess != nil { - isPublic = strings.EqualFold(*model.Properties.PublicNetworkAccess, "Enabled") - } } + if vaultUri != "" { meta.(*clients.Client).KeyVault.AddToCache(*id, vaultUri) } - var contactsResp *dataplane.Contacts // TODO: Remove in 4.0 - if !features.FourPointOhBeta() { - if isPublic { - contacts, err := dataplaneClient.GetCertificateContacts(ctx, vaultUri) - if err != nil { - if !utils.ResponseWasForbidden(contacts.Response) && !utils.ResponseWasNotFound(contacts.Response) { - return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) - } - } - contactsResp = &contacts + // NOTE: We are skipping the isPublic check here because we are enabling support + // for legacy key vaults that were already deployed and are assuming that the + // customer has already created a private link for this key vault so the + // data plane call should succeed if the key vault was configured correctly + // in Azure, which would allow the customer to continue to manage the contacts + // via the exposed contact field in the key vault resource as before... + var contactsResp *dataplane.Contacts + contacts, err := dataplaneClient.GetCertificateContacts(ctx, vaultUri) + if err != nil { + if !utils.ResponseWasForbidden(contacts.Response) && !utils.ResponseWasNotFound(contacts.Response) { + return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) } } + contactsResp = &contacts d.Set("name", id.VaultName) d.Set("resource_group_name", id.ResourceGroupName) @@ -723,7 +711,6 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { if model := resp.Model; model != nil { d.Set("location", location.NormalizeNilable(model.Location)) - d.Set("tenant_id", model.Properties.TenantId) d.Set("enabled_for_deployment", model.Properties.EnabledForDeployment) d.Set("enabled_for_disk_encryption", model.Properties.EnabledForDiskEncryption) @@ -765,10 +752,8 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { return fmt.Errorf("setting `access_policy`: %+v", err) } - if !features.FourPointOhBeta() { - if err := d.Set("contact", flattenKeyVaultCertificateContactList(contactsResp)); err != nil { - return fmt.Errorf("setting `contact` for KeyVault: %+v", err) - } + if err := d.Set("contact", flattenKeyVaultCertificateContactList(contactsResp)); err != nil { + return fmt.Errorf("setting `contact` for KeyVault: %+v", err) } if err := tags.FlattenAndSet(d, model.Tags); err != nil { From 8fe0df6d32efe6b5c908dc6ae1d64252a13f4c7c Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Mon, 29 Apr 2024 15:58:39 -0600 Subject: [PATCH 02/12] Fix lint error... --- internal/services/keyvault/key_vault_resource.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index 24bbc5fb263a..0574fbcfb0e6 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -123,7 +123,7 @@ func resourceKeyVault() *pluginsdk.Resource { // field back into the resource for UPDATE ONLY. If this is a new resource and the // contact field is defined in the configuration file it will now throw an error. // This will allow legacy key vaults to continue to work as the previously have - // and enforces our new model of seperating out the data plane call into its + // and enforces our new model of separating out the data plane call into its // own resource (e.g., contacts)... "contact": { Type: pluginsdk.TypeSet, From eb62b63cc70a7c38fd35c2d62e9eff822d95aff0 Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Fri, 10 May 2024 17:28:38 -0600 Subject: [PATCH 03/12] Move contact state management to update function... --- .../services/keyvault/key_vault_resource.go | 39 +++++++++---------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index 0574fbcfb0e6..dea0edc4ae5c 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -649,7 +649,24 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { // here. Many of our customers will have already set up private endpoints so the above // data plane calls would succeed... if err != nil && !utils.ResponseWasForbidden(resp.Response) { - return fmt.Errorf("setting Contacts for %s: %+v", *id, err) + return fmt.Errorf("updating Contacts for %s: %+v", *id, err) + } + + // Moved state management of the contact field out of the read function + // to the update function because if this is a new resource this would + // cause a context deadline exceeded error during the read call during + // create... + if resp.Response.StatusCode >= 200 && resp.Response.StatusCode < 300 { + contacts, err := managementClient.GetCertificateContacts(ctx, vaultUri) + if err != nil { + if !utils.ResponseWasForbidden(contacts.Response) && !utils.ResponseWasNotFound(contacts.Response) { + return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) + } + } + + if err := d.Set("contact", flattenKeyVaultCertificateContactList(&contacts)); err != nil { + return fmt.Errorf("setting `contact` for KeyVault: %+v", err) + } } } @@ -660,7 +677,6 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { client := meta.(*clients.Client).KeyVault.VaultsClient - dataplaneClient := meta.(*clients.Client).KeyVault.ManagementClient ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) defer cancel() @@ -690,21 +706,6 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { meta.(*clients.Client).KeyVault.AddToCache(*id, vaultUri) } - // NOTE: We are skipping the isPublic check here because we are enabling support - // for legacy key vaults that were already deployed and are assuming that the - // customer has already created a private link for this key vault so the - // data plane call should succeed if the key vault was configured correctly - // in Azure, which would allow the customer to continue to manage the contacts - // via the exposed contact field in the key vault resource as before... - var contactsResp *dataplane.Contacts - contacts, err := dataplaneClient.GetCertificateContacts(ctx, vaultUri) - if err != nil { - if !utils.ResponseWasForbidden(contacts.Response) && !utils.ResponseWasNotFound(contacts.Response) { - return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) - } - } - contactsResp = &contacts - d.Set("name", id.VaultName) d.Set("resource_group_name", id.ResourceGroupName) d.Set("vault_uri", vaultUri) @@ -752,10 +753,6 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { return fmt.Errorf("setting `access_policy`: %+v", err) } - if err := d.Set("contact", flattenKeyVaultCertificateContactList(contactsResp)); err != nil { - return fmt.Errorf("setting `contact` for KeyVault: %+v", err) - } - if err := tags.FlattenAndSet(d, model.Tags); err != nil { return fmt.Errorf("setting `tags`: %+v", err) } From c93d948df7a8e0efdba5e1aa7473aef93182c4dc Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Sun, 12 May 2024 02:47:43 -0600 Subject: [PATCH 04/12] Expose 'contactUpdateSuccessful' flag so the read function will know if it should attempt to retrieve the 'contact' information from the data plane or not... --- .../services/keyvault/key_vault_resource.go | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index dea0edc4ae5c..cc90ddb8640d 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -34,6 +34,7 @@ import ( ) var keyVaultResourceName = "azurerm_key_vault" +var contactUpdateSuccessful bool func resourceKeyVault() *pluginsdk.Resource { return &pluginsdk.Resource{ @@ -645,28 +646,18 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { resp, err = managementClient.SetCertificateContacts(ctx, vaultUri, contacts) } - // NOTE: For legecy backwards compatibility, we need to ignore the Forbidden response - // here. Many of our customers will have already set up private endpoints so the above + // NOTE: For legecy backwards compatibility, we need to ignore the Forbidden/Not Found response + // status codes here. Many of our customers will have already set up private endpoints so the above // data plane calls would succeed... - if err != nil && !utils.ResponseWasForbidden(resp.Response) { + if err != nil && !utils.ResponseWasForbidden(resp.Response) && !utils.ResponseWasNotFound(resp.Response) { return fmt.Errorf("updating Contacts for %s: %+v", *id, err) } - // Moved state management of the contact field out of the read function - // to the update function because if this is a new resource this would - // cause a context deadline exceeded error during the read call during - // create... + // Only set the 'contactUpdateSuccessful' flag if the call to the data plane was successful. + // If it was successfull the read function should call the data plane to get the contact + // information, if it wasn't is should not call the data plane... if resp.Response.StatusCode >= 200 && resp.Response.StatusCode < 300 { - contacts, err := managementClient.GetCertificateContacts(ctx, vaultUri) - if err != nil { - if !utils.ResponseWasForbidden(contacts.Response) && !utils.ResponseWasNotFound(contacts.Response) { - return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) - } - } - - if err := d.Set("contact", flattenKeyVaultCertificateContactList(&contacts)); err != nil { - return fmt.Errorf("setting `contact` for KeyVault: %+v", err) - } + contactUpdateSuccessful = true } } @@ -677,6 +668,7 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { client := meta.(*clients.Client).KeyVault.VaultsClient + managementClient := meta.(*clients.Client).KeyVault.ManagementClient ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) defer cancel() @@ -758,6 +750,19 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { } } + // NOTE: Only call the data plane for the contact information if the update operation + // was successful... + if contactUpdateSuccessful { + contacts, err := managementClient.GetCertificateContacts(ctx, vaultUri) + if err != nil { + return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) + } + + if err := d.Set("contact", flattenKeyVaultCertificateContactList(&contacts)); err != nil { + return fmt.Errorf("setting `contact` for KeyVault: %+v", err) + } + } + return nil } From 9cb81e4ec8acb9f58930d9385fe05c36b3062b93 Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Sun, 12 May 2024 03:17:40 -0600 Subject: [PATCH 05/12] Fix lint error... --- internal/services/keyvault/key_vault_resource.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index cc90ddb8640d..dcc164c359ee 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -654,7 +654,7 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { } // Only set the 'contactUpdateSuccessful' flag if the call to the data plane was successful. - // If it was successfull the read function should call the data plane to get the contact + // If it was successful the read function should call the data plane to get the contact // information, if it wasn't is should not call the data plane... if resp.Response.StatusCode >= 200 && resp.Response.StatusCode < 300 { contactUpdateSuccessful = true From 134680e4395e1d8c640172ddefa485f0333a9815 Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Tue, 14 May 2024 19:28:08 -0600 Subject: [PATCH 06/12] Tweak the read function a bit... --- .../services/keyvault/key_vault_resource.go | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index dcc164c359ee..92dafa8315c3 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -30,11 +30,14 @@ import ( "github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation" "github.com/hashicorp/terraform-provider-azurerm/internal/timeouts" "github.com/hashicorp/terraform-provider-azurerm/utils" - dataplane "github.com/tombuildsstuff/kermit/sdk/keyvault/7.4/keyvault" // TODO: Remove in 4.0 + dataplane "github.com/tombuildsstuff/kermit/sdk/keyvault/7.4/keyvault" ) var keyVaultResourceName = "azurerm_key_vault" -var contactUpdateSuccessful bool + +// NOTE: Need to let the read function know if it can safely call +// the data plane or not... +var privateEndpointEnabled bool func resourceKeyVault() *pluginsdk.Resource { return &pluginsdk.Resource{ @@ -463,6 +466,7 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { } update := vaults.VaultPatchParameters{} + isPublic := d.Get("public_network_access_enabled").(bool) if d.HasChange("access_policy") { if update.Properties == nil { @@ -567,7 +571,6 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { update.Properties = &vaults.VaultPatchProperties{} } - isPublic := d.Get("public_network_access_enabled").(bool) if isPublic { update.Properties.PublicNetworkAccess = utils.String("Enabled") } else { @@ -646,18 +649,17 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { resp, err = managementClient.SetCertificateContacts(ctx, vaultUri, contacts) } - // NOTE: For legecy backwards compatibility, we need to ignore the Forbidden/Not Found response - // status codes here. Many of our customers will have already set up private endpoints so the above - // data plane calls would succeed... - if err != nil && !utils.ResponseWasForbidden(resp.Response) && !utils.ResponseWasNotFound(resp.Response) { - return fmt.Errorf("updating Contacts for %s: %+v", *id, err) + if err != nil { + var extendedErrorMsg string + if !isPublic && (utils.ResponseWasForbidden(resp.Response) || utils.ResponseWasNotFound(resp.Response)) { + extendedErrorMsg = "\n\nWARNING: public network access for this key vault has been disabled, access to the key vault is only allowed through private endpoints" + } + return fmt.Errorf("updating Contacts for %s: %+v %s", *id, err, extendedErrorMsg) } - // Only set the 'contactUpdateSuccessful' flag if the call to the data plane was successful. - // If it was successful the read function should call the data plane to get the contact - // information, if it wasn't is should not call the data plane... + // Only set the 'privateEndpointEnabled' flag if the call to the data plane was successful. if resp.Response.StatusCode >= 200 && resp.Response.StatusCode < 300 { - contactUpdateSuccessful = true + privateEndpointEnabled = true } } @@ -702,6 +704,8 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { d.Set("resource_group_name", id.ResourceGroupName) d.Set("vault_uri", vaultUri) + publicNetworkAccessEnabled := true + if model := resp.Model; model != nil { d.Set("location", location.NormalizeNilable(model.Location)) d.Set("tenant_id", model.Properties.TenantId) @@ -710,7 +714,7 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { d.Set("enabled_for_template_deployment", model.Properties.EnabledForTemplateDeployment) d.Set("enable_rbac_authorization", model.Properties.EnableRbacAuthorization) d.Set("purge_protection_enabled", model.Properties.EnablePurgeProtection) - publicNetworkAccessEnabled := true + if model.Properties.PublicNetworkAccess != nil { publicNetworkAccessEnabled = strings.EqualFold(*model.Properties.PublicNetworkAccess, "Enabled") } @@ -727,7 +731,7 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { d.Set("soft_delete_retention_days", softDeleteRetentionDays) skuName := "" - // the Azure API is inconsistent here, so rewrite this into the casing we expect + // The Azure API is inconsistent here, so rewrite this into the casing we expect // TODO: this can be removed when the new base layer is enabled? for _, v := range vaults.PossibleValuesForSkuName() { if strings.EqualFold(v, string(model.Properties.Sku.Name)) { @@ -750,9 +754,9 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { } } - // NOTE: Only call the data plane for the contact information if the update operation - // was successful... - if contactUpdateSuccessful { + // NOTE: Only call the data plane for the contact information if the key vault + // is public or if the update operation was successful... + if publicNetworkAccessEnabled || privateEndpointEnabled { contacts, err := managementClient.GetCertificateContacts(ctx, vaultUri) if err != nil { return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) From 5ed3c025eab554a16df6d6a9d0762673f3598a9d Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Wed, 15 May 2024 13:35:39 -0600 Subject: [PATCH 07/12] Remove global var... --- .../services/keyvault/key_vault_resource.go | 38 +++++++++---------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index 92dafa8315c3..35f03a7f176e 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -35,10 +35,6 @@ import ( var keyVaultResourceName = "azurerm_key_vault" -// NOTE: Need to let the read function know if it can safely call -// the data plane or not... -var privateEndpointEnabled bool - func resourceKeyVault() *pluginsdk.Resource { return &pluginsdk.Resource{ Create: resourceKeyVaultCreate, @@ -642,25 +638,19 @@ func resourceKeyVaultUpdate(d *pluginsdk.ResourceData, meta interface{}) error { } var err error - var resp dataplane.Contacts if len(*contacts.ContactList) == 0 { - resp, err = managementClient.DeleteCertificateContacts(ctx, vaultUri) + _, err = managementClient.DeleteCertificateContacts(ctx, vaultUri) } else { - resp, err = managementClient.SetCertificateContacts(ctx, vaultUri, contacts) + _, err = managementClient.SetCertificateContacts(ctx, vaultUri, contacts) } if err != nil { var extendedErrorMsg string - if !isPublic && (utils.ResponseWasForbidden(resp.Response) || utils.ResponseWasNotFound(resp.Response)) { + if !isPublic { extendedErrorMsg = "\n\nWARNING: public network access for this key vault has been disabled, access to the key vault is only allowed through private endpoints" } return fmt.Errorf("updating Contacts for %s: %+v %s", *id, err, extendedErrorMsg) } - - // Only set the 'privateEndpointEnabled' flag if the call to the data plane was successful. - if resp.Response.StatusCode >= 200 && resp.Response.StatusCode < 300 { - privateEndpointEnabled = true - } } d.Partial(false) @@ -754,17 +744,23 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { } } - // NOTE: Only call the data plane for the contact information if the key vault - // is public or if the update operation was successful... - if publicNetworkAccessEnabled || privateEndpointEnabled { - contacts, err := managementClient.GetCertificateContacts(ctx, vaultUri) - if err != nil { + // If publicNetworkAccessEnabled is true, the data plane call should succeed. + // If an error is returned from the data plane call we need to return that error. + // + // If publicNetworkAccessEnabled is false, the data plane call should fail unless + // there is a private endpoint connected to the key vault. + // + // We don't know if the private endpoint has been created yet, so we need + // to ignore the error if the data plane call fails. + contacts, err := managementClient.GetCertificateContacts(ctx, vaultUri) + if err != nil { + if publicNetworkAccessEnabled { return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) } + } - if err := d.Set("contact", flattenKeyVaultCertificateContactList(&contacts)); err != nil { - return fmt.Errorf("setting `contact` for KeyVault: %+v", err) - } + if err := d.Set("contact", flattenKeyVaultCertificateContactList(&contacts)); err != nil { + return fmt.Errorf("setting `contact` for KeyVault: %+v", err) } return nil From 28ebdc775951682d198a44b64263fafea180d8a9 Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Wed, 15 May 2024 14:15:32 -0600 Subject: [PATCH 08/12] Add filter to read function... --- internal/services/keyvault/key_vault_resource.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index 35f03a7f176e..974b9bf4897c 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -755,7 +755,9 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { contacts, err := managementClient.GetCertificateContacts(ctx, vaultUri) if err != nil { if publicNetworkAccessEnabled { - return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) + if !utils.ResponseWasForbidden(contacts.Response) && !utils.ResponseWasNotFound(contacts.Response) { + return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) + } } } From 95e63c59e92f072180ec253d3d115649a7d7306a Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Wed, 15 May 2024 18:42:24 -0600 Subject: [PATCH 09/12] Split contact behavior based on provider major version --- .../services/keyvault/key_vault_resource.go | 40 +++++++++++++++---- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index 974b9bf4897c..acbf5885166a 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -21,6 +21,7 @@ import ( "github.com/hashicorp/terraform-provider-azurerm/helpers/tf" commonValidate "github.com/hashicorp/terraform-provider-azurerm/helpers/validate" "github.com/hashicorp/terraform-provider-azurerm/internal/clients" + "github.com/hashicorp/terraform-provider-azurerm/internal/features" "github.com/hashicorp/terraform-provider-azurerm/internal/locks" "github.com/hashicorp/terraform-provider-azurerm/internal/services/keyvault/migration" "github.com/hashicorp/terraform-provider-azurerm/internal/services/keyvault/validate" @@ -245,6 +246,7 @@ func resourceKeyVault() *pluginsdk.Resource { func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { subscriptionId := meta.(*clients.Client).Account.SubscriptionId + managementClient := meta.(*clients.Client).KeyVault.ManagementClient // TODO: Remove in 4.0 client := meta.(*clients.Client).KeyVault.VaultsClient ctx, cancel := timeouts.ForCreate(meta.(*clients.Client).StopContext, d) defer cancel() @@ -259,11 +261,14 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { isPublic := d.Get("public_network_access_enabled").(bool) contactRaw := d.Get("contact").(*pluginsdk.Set).List() + contactCount := len(contactRaw) - // NOTE: Block the creation of net new key vault resources where - // the contact field has been defined... - if len(contactRaw) > 0 { - return fmt.Errorf("%s: The `contact` field is not supported for new key vaults", id) + // In v4.0 providers block creation of all key vaults if the configuration + // file contains a 'contact' field... + if features.FourPointOhBeta() { + if contactCount > 0 { + return fmt.Errorf("%s: The `contact` field is not supported for new key vaults", id) + } } // check for the presence of an existing, live one which should be imported into the state @@ -430,6 +435,24 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { } } + // In v3.x providers block the creation of new key vaults if the + // 'public_network_access_enabled' is set to 'false'... + if !features.FourPointOhBeta() { + if contactCount > 0 { + if !isPublic { + return fmt.Errorf("`contact` cannot be specified when `public_network_access_enabled` is set to `false`") + } + + contacts := dataplane.Contacts{ + ContactList: expandKeyVaultCertificateContactList(contactRaw), + } + + if _, err := managementClient.SetCertificateContacts(ctx, vaultUri, contacts); err != nil { + return fmt.Errorf("failed to set Contacts for %s: %+v", id, err) + } + } + } + return resourceKeyVaultRead(d, meta) } @@ -745,19 +768,20 @@ func resourceKeyVaultRead(d *pluginsdk.ResourceData, meta interface{}) error { } // If publicNetworkAccessEnabled is true, the data plane call should succeed. + // (if the caller has the 'ManageContacts' certificate permissions) + // // If an error is returned from the data plane call we need to return that error. // // If publicNetworkAccessEnabled is false, the data plane call should fail unless // there is a private endpoint connected to the key vault. + // (and the caller has the 'ManageContacts' certificate permissions) // // We don't know if the private endpoint has been created yet, so we need // to ignore the error if the data plane call fails. contacts, err := managementClient.GetCertificateContacts(ctx, vaultUri) if err != nil { - if publicNetworkAccessEnabled { - if !utils.ResponseWasForbidden(contacts.Response) && !utils.ResponseWasNotFound(contacts.Response) { - return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) - } + if publicNetworkAccessEnabled && (!utils.ResponseWasForbidden(contacts.Response) && !utils.ResponseWasNotFound(contacts.Response)) { + return fmt.Errorf("retrieving `contact` for KeyVault: %+v", err) } } From 8b7482e1990a261e705f131508f91ab89acd178a Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Thu, 16 May 2024 11:55:33 -0600 Subject: [PATCH 10/12] Move 3.x contact validation to top of create... --- .../services/keyvault/key_vault_resource.go | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index acbf5885166a..ece8d1b31b09 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -263,11 +263,17 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { contactRaw := d.Get("contact").(*pluginsdk.Set).List() contactCount := len(contactRaw) - // In v4.0 providers block creation of all key vaults if the configuration - // file contains a 'contact' field... - if features.FourPointOhBeta() { - if contactCount > 0 { + if contactCount > 0 { + if features.FourPointOhBeta() { + // In v4.0 providers block creation of all key vaults if the configuration + // file contains a 'contact' field... return fmt.Errorf("%s: The `contact` field is not supported for new key vaults", id) + } else { + // In v3.x providers block creation of key vaults if 'public_network_access_enabled' + // is 'false'... + if !isPublic { + return fmt.Errorf("`contact` cannot be specified when `public_network_access_enabled` is set to `false`") + } } } @@ -435,21 +441,14 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { } } - // In v3.x providers block the creation of new key vaults if the - // 'public_network_access_enabled' is set to 'false'... - if !features.FourPointOhBeta() { - if contactCount > 0 { - if !isPublic { - return fmt.Errorf("`contact` cannot be specified when `public_network_access_enabled` is set to `false`") - } - - contacts := dataplane.Contacts{ - ContactList: expandKeyVaultCertificateContactList(contactRaw), - } + // Only call the data plane if the 'contact' field has been defined... + if contactCount > 0 { + contacts := dataplane.Contacts{ + ContactList: expandKeyVaultCertificateContactList(contactRaw), + } - if _, err := managementClient.SetCertificateContacts(ctx, vaultUri, contacts); err != nil { - return fmt.Errorf("failed to set Contacts for %s: %+v", id, err) - } + if _, err := managementClient.SetCertificateContacts(ctx, vaultUri, contacts); err != nil { + return fmt.Errorf("failed to set Contacts for %s: %+v", id, err) } } From 762773a7a3da24bf728a94a3c60413837b43c541 Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Thu, 16 May 2024 12:17:20 -0600 Subject: [PATCH 11/12] Fix lint error... --- internal/services/keyvault/key_vault_resource.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index ece8d1b31b09..e56b97986d42 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -268,12 +268,10 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { // In v4.0 providers block creation of all key vaults if the configuration // file contains a 'contact' field... return fmt.Errorf("%s: The `contact` field is not supported for new key vaults", id) - } else { + } else if !isPublic { // In v3.x providers block creation of key vaults if 'public_network_access_enabled' // is 'false'... - if !isPublic { - return fmt.Errorf("`contact` cannot be specified when `public_network_access_enabled` is set to `false`") - } + return fmt.Errorf("`contact` cannot be specified when `public_network_access_enabled` is set to `false`") } } From 60c0a5c553cc6ca4c0e3153161159f286bfe5a3c Mon Sep 17 00:00:00 2001 From: Jeffrey Cline <20408400+WodansSon@users.noreply.github.com> Date: Thu, 16 May 2024 12:24:27 -0600 Subject: [PATCH 12/12] Update error msg... --- internal/services/keyvault/key_vault_resource.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/services/keyvault/key_vault_resource.go b/internal/services/keyvault/key_vault_resource.go index e56b97986d42..128a6eb1be7f 100644 --- a/internal/services/keyvault/key_vault_resource.go +++ b/internal/services/keyvault/key_vault_resource.go @@ -267,11 +267,11 @@ func resourceKeyVaultCreate(d *pluginsdk.ResourceData, meta interface{}) error { if features.FourPointOhBeta() { // In v4.0 providers block creation of all key vaults if the configuration // file contains a 'contact' field... - return fmt.Errorf("%s: The `contact` field is not supported for new key vaults", id) + return fmt.Errorf("%s: `contact` field is not supported for new key vaults", id) } else if !isPublic { // In v3.x providers block creation of key vaults if 'public_network_access_enabled' // is 'false'... - return fmt.Errorf("`contact` cannot be specified when `public_network_access_enabled` is set to `false`") + return fmt.Errorf("%s: `contact` cannot be specified when `public_network_access_enabled` is set to `false`", id) } }