From bf09edc6429ad41ba6624633bb49451b0d836d30 Mon Sep 17 00:00:00 2001 From: Nashwan Azhari Date: Thu, 29 Oct 2015 22:37:08 +0200 Subject: [PATCH] provider/Azure: fixes: added wait on instance deletion for associated blob deletion. added guarding Mutex for secgroup-rule-related concurrent operations. added usage warning on secgroup rules. --- builtin/providers/azure/config.go | 14 +++-- .../azure/resource_azure_dns_server.go | 12 ++-- .../azure/resource_azure_instance.go | 56 +++++++++++++++++-- .../azure/resource_azure_local_network.go | 12 ++-- .../resource_azure_security_group_rule.go | 9 +++ .../azure/resource_azure_virtual_network.go | 12 ++-- .../azure/r/security_group_rule.html.markdown | 14 ++++- 7 files changed, 99 insertions(+), 30 deletions(-) diff --git a/builtin/providers/azure/config.go b/builtin/providers/azure/config.go index b096a10c4..0dd7b142e 100644 --- a/builtin/providers/azure/config.go +++ b/builtin/providers/azure/config.go @@ -35,8 +35,6 @@ type Client struct { hostedServiceClient hostedservice.HostedServiceClient - secGroupClient networksecuritygroup.SecurityGroupClient - osImageClient osimage.OSImageClient sqlClient sql.SQLDatabaseClient @@ -52,7 +50,11 @@ type Client struct { // unfortunately; because of how Azure's network API works; doing networking operations // concurrently is very hazardous, and we need a mutex to guard the VirtualNetworkClient. vnetClient virtualnetwork.VirtualNetworkClient - mutex *sync.Mutex + vnetMutex *sync.Mutex + + // same as the above for security group rule operations: + secGroupClient networksecuritygroup.SecurityGroupClient + secGroupMutex *sync.Mutex } // getStorageClientForStorageService is helper method which returns the @@ -106,6 +108,7 @@ func (c *Config) NewClientFromSettingsData() (*Client, error) { affinityGroupClient: affinitygroup.NewClient(mc), hostedServiceClient: hostedservice.NewClient(mc), secGroupClient: networksecuritygroup.NewClient(mc), + secGroupMutex: &sync.Mutex{}, osImageClient: osimage.NewClient(mc), sqlClient: sql.NewClient(mc), storageServiceClient: storageservice.NewClient(mc), @@ -113,7 +116,7 @@ func (c *Config) NewClientFromSettingsData() (*Client, error) { vmDiskClient: virtualmachinedisk.NewClient(mc), vmImageClient: virtualmachineimage.NewClient(mc), vnetClient: virtualnetwork.NewClient(mc), - mutex: &sync.Mutex{}, + vnetMutex: &sync.Mutex{}, }, nil } @@ -130,6 +133,7 @@ func (c *Config) NewClient() (*Client, error) { affinityGroupClient: affinitygroup.NewClient(mc), hostedServiceClient: hostedservice.NewClient(mc), secGroupClient: networksecuritygroup.NewClient(mc), + secGroupMutex: &sync.Mutex{}, osImageClient: osimage.NewClient(mc), sqlClient: sql.NewClient(mc), storageServiceClient: storageservice.NewClient(mc), @@ -137,6 +141,6 @@ func (c *Config) NewClient() (*Client, error) { vmDiskClient: virtualmachinedisk.NewClient(mc), vmImageClient: virtualmachineimage.NewClient(mc), vnetClient: virtualnetwork.NewClient(mc), - mutex: &sync.Mutex{}, + vnetMutex: &sync.Mutex{}, }, nil } diff --git a/builtin/providers/azure/resource_azure_dns_server.go b/builtin/providers/azure/resource_azure_dns_server.go index f68eaff10..ffd306f7b 100644 --- a/builtin/providers/azure/resource_azure_dns_server.go +++ b/builtin/providers/azure/resource_azure_dns_server.go @@ -43,8 +43,8 @@ func resourceAzureDnsServerCreate(d *schema.ResourceData, meta interface{}) erro vnetClient := azureClient.vnetClient log.Println("[INFO] Fetching current network configuration from Azure.") - azureClient.mutex.Lock() - defer azureClient.mutex.Unlock() + azureClient.vnetMutex.Lock() + defer azureClient.vnetMutex.Unlock() netConf, err := vnetClient.GetVirtualNetworkConfiguration() if err != nil { if management.IsResourceNotFoundError(err) { @@ -124,8 +124,8 @@ func resourceAzureDnsServerUpdate(d *schema.ResourceData, meta interface{}) erro if d.HasChange("dns_address") { log.Println("[DEBUG] DNS server address has changes; updating it on Azure.") log.Println("[INFO] Fetching current network configuration from Azure.") - azureClient.mutex.Lock() - defer azureClient.mutex.Unlock() + azureClient.vnetMutex.Lock() + defer azureClient.vnetMutex.Unlock() netConf, err := vnetClient.GetVirtualNetworkConfiguration() if err != nil { return fmt.Errorf("Failed to get the current network configuration from Azure: %s", err) @@ -198,8 +198,8 @@ func resourceAzureDnsServerDelete(d *schema.ResourceData, meta interface{}) erro vnetClient := azureClient.vnetClient log.Println("[INFO] Fetching current network configuration from Azure.") - azureClient.mutex.Lock() - defer azureClient.mutex.Unlock() + azureClient.vnetMutex.Lock() + defer azureClient.vnetMutex.Unlock() netConf, err := vnetClient.GetVirtualNetworkConfiguration() if err != nil { return fmt.Errorf("Failed to get the current network configuration from Azure: %s", err) diff --git a/builtin/providers/azure/resource_azure_instance.go b/builtin/providers/azure/resource_azure_instance.go index c95285ec2..8a643931c 100644 --- a/builtin/providers/azure/resource_azure_instance.go +++ b/builtin/providers/azure/resource_azure_instance.go @@ -6,6 +6,7 @@ import ( "fmt" "log" "strings" + "time" "github.com/Azure/azure-sdk-for-go/management" "github.com/Azure/azure-sdk-for-go/management/hostedservice" @@ -14,13 +15,16 @@ import ( "github.com/Azure/azure-sdk-for-go/management/virtualmachineimage" "github.com/Azure/azure-sdk-for-go/management/vmutils" "github.com/hashicorp/terraform/helper/hashcode" + "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" ) const ( - linux = "Linux" - windows = "Windows" - osDiskBlobStorageURL = "http://%s.blob.core.windows.net/vhds/%s.vhd" + linux = "Linux" + windows = "Windows" + storageContainterName = "vhds" + osDiskBlobNameFormat = "%s.vhd" + osDiskBlobStorageURL = "http://%s.blob.core.windows.net/" + storageContainterName + "/" + osDiskBlobNameFormat ) func resourceAzureInstance() *schema.Resource { @@ -44,6 +48,16 @@ func resourceAzureInstance() *schema.Resource { ForceNew: true, }, + // in order to prevent an unintentional delete of a containing + // hosted service in the case the same name are given to both the + // service and the instance despite their being created separately, + // we must maintain a flag to definitively denote whether this + // instance had a hosted service created for it or not: + "has_dedicated_service": &schema.Schema{ + Type: schema.TypeBool, + Computed: true, + }, + "description": &schema.Schema{ Type: schema.TypeString, Optional: true, @@ -234,6 +248,7 @@ func resourceAzureInstanceCreate(d *schema.ResourceData, meta interface{}) (err // if not provided; just use the name of the instance to create a new one: hostedServiceName = name d.Set("hosted_service_name", hostedServiceName) + d.Set("has_dedicated_service", true) p := hostedservice.CreateHostedServiceParameters{ ServiceName: hostedServiceName, @@ -560,7 +575,6 @@ func resourceAzureInstanceDelete(d *schema.ResourceData, meta interface{}) error azureClient := meta.(*Client) mc := azureClient.mgmtClient vmClient := azureClient.vmClient - hostedServiceClient := azureClient.hostedServiceClient name := d.Get("name").(string) hostedServiceName := d.Get("hosted_service_name").(string) @@ -568,8 +582,9 @@ func resourceAzureInstanceDelete(d *schema.ResourceData, meta interface{}) error log.Printf("[DEBUG] Deleting instance: %s", name) // check if the instance had a hosted service created especially for it: - if name == hostedServiceName { + if d.Get("has_dedicated_service").(bool) { // if so; we must delete the associated hosted service as well: + hostedServiceClient := azureClient.hostedServiceClient req, err := hostedServiceClient.DeleteHostedService(name, true) if err != nil { return fmt.Errorf("Error deleting instance and hosted service %s: %s", name, err) @@ -594,7 +609,35 @@ func resourceAzureInstanceDelete(d *schema.ResourceData, meta interface{}) error } } - return nil + log.Printf("[INFO] Waiting for the deletion of instance '%s''s disk blob.", name) + + // in order to avoid `terraform taint`-like scenarios in which the instance + // is deleted and re-created so fast the previous storage blob which held + // the image doesn't manage to get deleted (despite it being in a + // 'deleting' state) and a lease conflict occurs over it, we must ensure + // the blob got completely deleted as well: + storName := d.Get("storage_service_name").(string) + blobClient, err := azureClient.getStorageServiceBlobClient(storName) + if err != nil { + return err + } + + err = resource.Retry(5*time.Minute, func() error { + exists, err := blobClient.BlobExists( + storageContainterName, fmt.Sprintf(osDiskBlobNameFormat, name), + ) + if err != nil { + return resource.RetryError{Err: err} + } + + if exists { + return fmt.Errorf("Instance '%s''s disk storage blob still exists.", name) + } + + return nil + }) + + return err } func resourceAzureEndpointHash(v interface{}) int { @@ -674,6 +717,7 @@ func retrieveOSImageDetails( label string, name string, storage string) (func(*virtualmachine.Role) error, string, []string, error) { + imgs, err := osImageClient.ListOSImages() if err != nil { return nil, "", nil, fmt.Errorf("Error retrieving image details: %s", err) diff --git a/builtin/providers/azure/resource_azure_local_network.go b/builtin/providers/azure/resource_azure_local_network.go index 5c2f4f3d8..82b451739 100644 --- a/builtin/providers/azure/resource_azure_local_network.go +++ b/builtin/providers/azure/resource_azure_local_network.go @@ -51,8 +51,8 @@ func resourceAzureLocalNetworkConnectionCreate(d *schema.ResourceData, meta inte vnetClient := azureClient.vnetClient log.Println("[INFO] Fetching current network configuration from Azure.") - azureClient.mutex.Lock() - defer azureClient.mutex.Unlock() + azureClient.vnetMutex.Lock() + defer azureClient.vnetMutex.Unlock() netConf, err := vnetClient.GetVirtualNetworkConfiguration() if err != nil { if management.IsResourceNotFoundError(err) { @@ -138,8 +138,8 @@ func resourceAzureLocalNetworkConnectionUpdate(d *schema.ResourceData, meta inte vnetClient := azureClient.vnetClient log.Println("[INFO] Fetching current network configuration from Azure.") - azureClient.mutex.Lock() - defer azureClient.mutex.Unlock() + azureClient.vnetMutex.Lock() + defer azureClient.vnetMutex.Unlock() netConf, err := vnetClient.GetVirtualNetworkConfiguration() if err != nil { return fmt.Errorf("Failed to get the current network configuration from Azure: %s", err) @@ -217,8 +217,8 @@ func resourceAzureLocalNetworkConnectionDelete(d *schema.ResourceData, meta inte vnetClient := azureClient.vnetClient log.Println("[INFO] Fetching current network configuration from Azure.") - azureClient.mutex.Lock() - defer azureClient.mutex.Unlock() + azureClient.vnetMutex.Lock() + defer azureClient.vnetMutex.Unlock() netConf, err := vnetClient.GetVirtualNetworkConfiguration() if err != nil { return fmt.Errorf("Failed to get the current network configuration from Azure: %s", err) diff --git a/builtin/providers/azure/resource_azure_security_group_rule.go b/builtin/providers/azure/resource_azure_security_group_rule.go index 868edb150..d404d020b 100644 --- a/builtin/providers/azure/resource_azure_security_group_rule.go +++ b/builtin/providers/azure/resource_azure_security_group_rule.go @@ -86,6 +86,9 @@ func resourceAzureSecurityGroupRuleCreate(d *schema.ResourceData, meta interface mgmtClient := azureClient.mgmtClient secGroupClient := azureClient.secGroupClient + azureClient.secGroupMutex.Lock() + defer azureClient.secGroupMutex.Unlock() + // create and configure the RuleResponse: name := d.Get("name").(string) rule := netsecgroup.RuleRequest{ @@ -183,6 +186,9 @@ func resourceAzureSecurityGroupRuleUpdate(d *schema.ResourceData, meta interface mgmtClient := azureClient.mgmtClient secGroupClient := azureClient.secGroupClient + azureClient.secGroupMutex.Lock() + defer azureClient.secGroupMutex.Unlock() + var found bool name := d.Get("name").(string) newRule := netsecgroup.RuleRequest{ @@ -262,6 +268,9 @@ func resourceAzureSecurityGroupRuleDelete(d *schema.ResourceData, meta interface mgmtClient := azureClient.mgmtClient secGroupClient := azureClient.secGroupClient + azureClient.secGroupMutex.Lock() + defer azureClient.secGroupMutex.Unlock() + name := d.Get("name").(string) secGroupNames := d.Get("security_group_names").(*schema.Set).List() for _, sg := range secGroupNames { diff --git a/builtin/providers/azure/resource_azure_virtual_network.go b/builtin/providers/azure/resource_azure_virtual_network.go index dcb008f07..f41f9955e 100644 --- a/builtin/providers/azure/resource_azure_virtual_network.go +++ b/builtin/providers/azure/resource_azure_virtual_network.go @@ -82,8 +82,8 @@ func resourceAzureVirtualNetworkCreate(d *schema.ResourceData, meta interface{}) // Lock the client just before we get the virtual network configuration and immediately // set an defer to unlock the client again whenever this function exits - ac.mutex.Lock() - defer ac.mutex.Unlock() + ac.vnetMutex.Lock() + defer ac.vnetMutex.Unlock() nc, err := vnetClient.GetVirtualNetworkConfiguration() if err != nil { @@ -181,8 +181,8 @@ func resourceAzureVirtualNetworkUpdate(d *schema.ResourceData, meta interface{}) // Lock the client just before we get the virtual network configuration and immediately // set an defer to unlock the client again whenever this function exits - ac.mutex.Lock() - defer ac.mutex.Unlock() + ac.vnetMutex.Lock() + defer ac.vnetMutex.Unlock() nc, err := vnetClient.GetVirtualNetworkConfiguration() if err != nil { @@ -227,8 +227,8 @@ func resourceAzureVirtualNetworkDelete(d *schema.ResourceData, meta interface{}) // Lock the client just before we get the virtual network configuration and immediately // set an defer to unlock the client again whenever this function exits - ac.mutex.Lock() - defer ac.mutex.Unlock() + ac.vnetMutex.Lock() + defer ac.vnetMutex.Unlock() nc, err := vnetClient.GetVirtualNetworkConfiguration() if err != nil { diff --git a/website/source/docs/providers/azure/r/security_group_rule.html.markdown b/website/source/docs/providers/azure/r/security_group_rule.html.markdown index 20a28ce9d..5ba85bbe3 100644 --- a/website/source/docs/providers/azure/r/security_group_rule.html.markdown +++ b/website/source/docs/providers/azure/r/security_group_rule.html.markdown @@ -8,7 +8,19 @@ description: |- # azure\_security\_group\_rule -Creates a new network security rule to be associated with a given security group. +Creates a new network Security Group Rule to be associated with a number of +given Security Groups. + +~> **NOTE on Security Group Rules**: for usability purposes; Terraform allows the +addition of a single Security Group Rule to multiple Security Groups, despite +it having to define each rule individually per Security Group on Azure. As a +result; in the event that one of the Rules on one of the Groups is modified by +external factors, Terraform cannot reason as to whether or not that change +should be propagated to the others; let alone choose one changed Rule +configuration over another in case of a conflic. As such; `terraform refresh` +only checks that the rule is still defined for each of the specified +`security_group_names`; ignoring the actual parameters of the Rule and **not** +updating the state with regards to them. ## Example Usage