From fc518448103bf90d57a7279423f2855aaf09c898 Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Tue, 1 Oct 2019 15:01:26 +0200 Subject: [PATCH 1/7] provider: adding a feature toggle for custom timeouts --- azurerm/internal/features/custom_timeouts.go | 22 ++++++++ .../internal/features/custom_timeouts_test.go | 55 +++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 azurerm/internal/features/custom_timeouts.go create mode 100644 azurerm/internal/features/custom_timeouts_test.go diff --git a/azurerm/internal/features/custom_timeouts.go b/azurerm/internal/features/custom_timeouts.go new file mode 100644 index 000000000000..a7743fc3ef51 --- /dev/null +++ b/azurerm/internal/features/custom_timeouts.go @@ -0,0 +1,22 @@ +package features + +import ( + "os" + "strings" +) + +// SupportsCustomTimeouts returns whether Custom Timeouts are supported +// +// This feature allows Resources to define Custom Timeouts for Creation, Updating and Deletion +// which helps work with Azure resources that take longer to provision/delete. +// When this feature is disabled, all resources have a hard-coded timeout of 3 hours. +// +// This feature-toggle defaults to off in 1.x versions of the Azure Provider, however this will +// become the default behaviour in version 2.0 of the Azure Provider. As outlined in the announcement +// for v2.0 of the Azure Provider: https://github.com/terraform-providers/terraform-provider-azurerm/issues/2807 +// +// Operators wishing to adopt this behaviour can opt-into this behaviour in 1.x versions of the +// Azure Provider by setting the Environment Variable 'ARM_PROVIDER_CUSTOM_TIMEOUTS' to 'true' +func SupportsCustomTimeouts() bool { + return strings.EqualFold(os.Getenv("ARM_PROVIDER_CUSTOM_TIMEOUTS"), "true") +} diff --git a/azurerm/internal/features/custom_timeouts_test.go b/azurerm/internal/features/custom_timeouts_test.go new file mode 100644 index 000000000000..cd260cf573b4 --- /dev/null +++ b/azurerm/internal/features/custom_timeouts_test.go @@ -0,0 +1,55 @@ +package features + +import ( + "os" + "testing" +) + +func TestCustomTimeouts(t *testing.T) { + testData := []struct { + name string + value string + expected bool + }{ + { + name: "unset", + value: "", + expected: false, + }, + { + name: "disabled lower-case", + value: "false", + expected: false, + }, + { + name: "disabled upper-case", + value: "FALSE", + expected: false, + }, + { + name: "enabled lower-case", + value: "true", + expected: true, + }, + { + name: "enabled upper-case", + value: "TRUE", + expected: true, + }, + { + name: "invalid", + value: "pandas", + expected: false, + }, + } + + for _, v := range testData { + t.Logf("[DEBUG] Test %q..", v.name) + + os.Setenv("ARM_PROVIDER_CUSTOM_TIMEOUTS", v.value) + actual := SupportsCustomTimeouts() + if v.expected != actual { + t.Fatalf("Expected %t but got %t", v.expected, actual) + } + } +} From e8c122e3b766ea95bdc9d2aad65e8f6174e5ebbc Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Tue, 1 Oct 2019 15:07:12 +0200 Subject: [PATCH 2/7] provider: adding/removing timeouts if the features enabled --- azurerm/provider.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/azurerm/provider.go b/azurerm/provider.go index 0138b9fbc463..6023a7b26e8d 100644 --- a/azurerm/provider.go +++ b/azurerm/provider.go @@ -5,11 +5,13 @@ import ( "log" "os" "strings" + "time" "github.com/hashicorp/go-azure-helpers/authentication" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/terraform" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/validate" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/features" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/common" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/compute" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" @@ -476,6 +478,29 @@ func Provider() terraform.ResourceProvider { } } + // TODO: remove all of this in 2.0 once Custom Timeouts are supported + if features.SupportsCustomTimeouts() { + // default everything to 3 hours for now + for _, v := range resources { + if v.Timeouts == nil { + v.Timeouts = &schema.ResourceTimeout{ + Create: schema.DefaultTimeout(3 * time.Hour), + Update: schema.DefaultTimeout(3 * time.Hour), + Delete: schema.DefaultTimeout(3 * time.Hour), + Default: schema.DefaultTimeout(3 * time.Hour), + + // Read is the only exception, since if it's taken more than 5 minutes something's seriously wrong + Read: schema.DefaultTimeout(5 * time.Minute), + } + } + } + } else { + // ensure any timeouts configured on the resources are removed until 2.0 + for _, v := range resources { + v.Timeouts = nil + } + } + p := &schema.Provider{ Schema: map[string]*schema.Schema{ "subscription_id": { From 19097ade5d7e8e1f9dd837fa5d1e7dc2ccd3ee91 Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Tue, 1 Oct 2019 15:24:39 +0200 Subject: [PATCH 3/7] provider: adding a timeouts package to allow to conditionally build timeouts --- azurerm/internal/timeouts/determine.go | 54 ++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 azurerm/internal/timeouts/determine.go diff --git a/azurerm/internal/timeouts/determine.go b/azurerm/internal/timeouts/determine.go new file mode 100644 index 000000000000..7ddce1a0c4de --- /dev/null +++ b/azurerm/internal/timeouts/determine.go @@ -0,0 +1,54 @@ +package timeouts + +import ( + "context" + "time" + + "github.com/hashicorp/terraform/helper/schema" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/features" +) + +// TODO: tests for this + +// ForCreate returns the context wrapped with the timeout for an Create operation +// +// If the 'SupportsCustomTimeouts' feature toggle is enabled - this is wrapped with a context +// Otherwise this returns the default context +func ForCreate(ctx context.Context, d *schema.ResourceData) (context.Context, context.CancelFunc) { + return buildWithTimeout(ctx, d.Timeout(schema.TimeoutCreate)) +} + +// ForDelete returns the context wrapped with the timeout for an Delete operation +// +// If the 'SupportsCustomTimeouts' feature toggle is enabled - this is wrapped with a context +// Otherwise this returns the default context +func ForDelete(ctx context.Context, d *schema.ResourceData) (context.Context, context.CancelFunc) { + return buildWithTimeout(ctx, d.Timeout(schema.TimeoutDelete)) +} + +// ForRead returns the context wrapped with the timeout for an Read operation +// +// If the 'SupportsCustomTimeouts' feature toggle is enabled - this is wrapped with a context +// Otherwise this returns the default context +func ForRead(ctx context.Context, d *schema.ResourceData) (context.Context, context.CancelFunc) { + return buildWithTimeout(ctx, d.Timeout(schema.TimeoutRead)) +} + +// ForUpdate returns the context wrapped with the timeout for an Update operation +// +// If the 'SupportsCustomTimeouts' feature toggle is enabled - this is wrapped with a context +// Otherwise this returns the default context +func ForUpdate(ctx context.Context, d *schema.ResourceData) (context.Context, context.CancelFunc) { + return buildWithTimeout(ctx, d.Timeout(schema.TimeoutUpdate)) +} + +func buildWithTimeout(ctx context.Context, timeout time.Duration) (context.Context, context.CancelFunc) { + if features.SupportsCustomTimeouts() { + return context.WithTimeout(ctx, timeout) + } + + nullFunc := func() { + // do nothing on cancel since timeouts aren't enabled + } + return ctx, nullFunc +} From 71c615c244361bea0ab5d070d94561aa7900b90e Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Tue, 1 Oct 2019 15:25:03 +0200 Subject: [PATCH 4/7] provider: making the timeout nilable to ensure we catch when this feature's disabled --- azurerm/config.go | 4 +++- azurerm/internal/common/client_options.go | 11 +++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/azurerm/config.go b/azurerm/config.go index 76da3695e470..957d3b0bfe1e 100644 --- a/azurerm/config.go +++ b/azurerm/config.go @@ -197,6 +197,8 @@ func getArmClient(authConfig *authentication.Config, skipProviderRegistration bo // Key Vault Endpoints keyVaultAuth := authConfig.BearerAuthorizerCallback(sender, oauthConfig) + timeout := 180 * time.Minute + o := &common.ClientOptions{ SubscriptionId: authConfig.SubscriptionID, TenantID: authConfig.TenantID, @@ -208,7 +210,7 @@ func getArmClient(authConfig *authentication.Config, skipProviderRegistration bo ResourceManagerAuthorizer: auth, ResourceManagerEndpoint: endpoint, StorageAuthorizer: storageAuth, - PollingDuration: 180 * time.Minute, + PollingDuration: &timeout, SkipProviderReg: skipProviderRegistration, DisableCorrelationRequestID: disableCorrelationRequestID, Environment: *env, diff --git a/azurerm/internal/common/client_options.go b/azurerm/internal/common/client_options.go index abe91cd852fd..fe4ab03443b7 100644 --- a/azurerm/internal/common/client_options.go +++ b/azurerm/internal/common/client_options.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/go-autorest/autorest/azure" "github.com/hashicorp/go-azure-helpers/sender" "github.com/hashicorp/terraform/httpclient" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/features" "github.com/terraform-providers/terraform-provider-azurerm/version" ) @@ -27,10 +28,11 @@ type ClientOptions struct { ResourceManagerEndpoint string StorageAuthorizer autorest.Authorizer - PollingDuration time.Duration SkipProviderReg bool DisableCorrelationRequestID bool Environment azure.Environment + + PollingDuration *time.Duration } func (o ClientOptions) ConfigureClient(c *autorest.Client, authorizer autorest.Authorizer) { @@ -38,11 +40,16 @@ func (o ClientOptions) ConfigureClient(c *autorest.Client, authorizer autorest.A c.Authorizer = authorizer c.Sender = sender.BuildSender("AzureRM") - c.PollingDuration = o.PollingDuration c.SkipResourceProviderRegistration = o.SkipProviderReg if !o.DisableCorrelationRequestID { c.RequestInspector = WithCorrelationRequestID(CorrelationRequestID()) } + + // TODO: remove in 2.0 + if !features.SupportsCustomTimeouts() { + // this is intentionally de-referencing the timeout since this should be nil when timeouts are enabled by default + c.PollingDuration = *o.PollingDuration + } } func setUserAgent(client *autorest.Client, tfVersion, partnerID string) { From 724f8d849507a25d13f58d6ab722708811bd58be Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Tue, 1 Oct 2019 15:25:19 +0200 Subject: [PATCH 5/7] r/resource_group: conditionally switching to use timeouts --- azurerm/resource_arm_resource_group.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/azurerm/resource_arm_resource_group.go b/azurerm/resource_arm_resource_group.go index b72ed31b2aa8..cd0bb80c100e 100644 --- a/azurerm/resource_arm_resource_group.go +++ b/azurerm/resource_arm_resource_group.go @@ -8,6 +8,7 @@ import ( "github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/tf" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/features" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/tags" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/timeouts" "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2018-05-01/resources" "github.com/hashicorp/terraform/helper/schema" @@ -37,7 +38,8 @@ func resourceArmResourceGroup() *schema.Resource { func resourceArmResourceGroupCreateUpdate(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).resource.GroupsClient - ctx := meta.(*ArmClient).StopContext + ctx, cancel := timeouts.ForCreate(meta.(*ArmClient).StopContext, d) + defer cancel() name := d.Get("name").(string) location := azure.NormalizeLocation(d.Get("location").(string)) @@ -77,7 +79,8 @@ func resourceArmResourceGroupCreateUpdate(d *schema.ResourceData, meta interface func resourceArmResourceGroupRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).resource.GroupsClient - ctx := meta.(*ArmClient).StopContext + ctx, cancel := timeouts.ForRead(meta.(*ArmClient).StopContext, d) + defer cancel() id, err := azure.ParseAzureResourceID(d.Id()) if err != nil { @@ -106,7 +109,8 @@ func resourceArmResourceGroupRead(d *schema.ResourceData, meta interface{}) erro func resourceArmResourceGroupDelete(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).resource.GroupsClient - ctx := meta.(*ArmClient).StopContext + ctx, cancel := timeouts.ForDelete(meta.(*ArmClient).StopContext, d) + defer cancel() id, err := azure.ParseAzureResourceID(d.Id()) if err != nil { From d4c97a22daf49d35c2f525b67f6cc50a524c658a Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Tue, 1 Oct 2019 15:26:34 +0200 Subject: [PATCH 6/7] provider: support for combined create/update methods --- azurerm/internal/timeouts/determine.go | 12 ++++++++++++ azurerm/resource_arm_resource_group.go | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/azurerm/internal/timeouts/determine.go b/azurerm/internal/timeouts/determine.go index 7ddce1a0c4de..7928459c7701 100644 --- a/azurerm/internal/timeouts/determine.go +++ b/azurerm/internal/timeouts/determine.go @@ -18,6 +18,18 @@ func ForCreate(ctx context.Context, d *schema.ResourceData) (context.Context, co return buildWithTimeout(ctx, d.Timeout(schema.TimeoutCreate)) } +// ForCreateUpdate returns the context wrapped with the timeout for an combined Create/Update operation +// +// If the 'SupportsCustomTimeouts' feature toggle is enabled - this is wrapped with a context +// Otherwise this returns the default context +func ForCreateUpdate(ctx context.Context, d *schema.ResourceData) (context.Context, context.CancelFunc) { + if d.IsNewResource() { + return ForCreate(ctx, d) + } + + return ForUpdate(ctx, d) +} + // ForDelete returns the context wrapped with the timeout for an Delete operation // // If the 'SupportsCustomTimeouts' feature toggle is enabled - this is wrapped with a context diff --git a/azurerm/resource_arm_resource_group.go b/azurerm/resource_arm_resource_group.go index cd0bb80c100e..837fcc147bd2 100644 --- a/azurerm/resource_arm_resource_group.go +++ b/azurerm/resource_arm_resource_group.go @@ -38,7 +38,7 @@ func resourceArmResourceGroup() *schema.Resource { func resourceArmResourceGroupCreateUpdate(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).resource.GroupsClient - ctx, cancel := timeouts.ForCreate(meta.(*ArmClient).StopContext, d) + ctx, cancel := timeouts.ForCreateUpdate(meta.(*ArmClient).StopContext, d) defer cancel() name := d.Get("name").(string) From 7fda6155b8736fc01c0bceb613613c638b37681c Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Tue, 1 Oct 2019 15:36:28 +0200 Subject: [PATCH 7/7] provider: making the timeout non-nilable --- azurerm/config.go | 4 +--- azurerm/internal/common/client_options.go | 6 +++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/azurerm/config.go b/azurerm/config.go index 957d3b0bfe1e..76da3695e470 100644 --- a/azurerm/config.go +++ b/azurerm/config.go @@ -197,8 +197,6 @@ func getArmClient(authConfig *authentication.Config, skipProviderRegistration bo // Key Vault Endpoints keyVaultAuth := authConfig.BearerAuthorizerCallback(sender, oauthConfig) - timeout := 180 * time.Minute - o := &common.ClientOptions{ SubscriptionId: authConfig.SubscriptionID, TenantID: authConfig.TenantID, @@ -210,7 +208,7 @@ func getArmClient(authConfig *authentication.Config, skipProviderRegistration bo ResourceManagerAuthorizer: auth, ResourceManagerEndpoint: endpoint, StorageAuthorizer: storageAuth, - PollingDuration: &timeout, + PollingDuration: 180 * time.Minute, SkipProviderReg: skipProviderRegistration, DisableCorrelationRequestID: disableCorrelationRequestID, Environment: *env, diff --git a/azurerm/internal/common/client_options.go b/azurerm/internal/common/client_options.go index fe4ab03443b7..8bd3552adaac 100644 --- a/azurerm/internal/common/client_options.go +++ b/azurerm/internal/common/client_options.go @@ -32,7 +32,8 @@ type ClientOptions struct { DisableCorrelationRequestID bool Environment azure.Environment - PollingDuration *time.Duration + // TODO: remove me in 2.0 + PollingDuration time.Duration } func (o ClientOptions) ConfigureClient(c *autorest.Client, authorizer autorest.Authorizer) { @@ -47,8 +48,7 @@ func (o ClientOptions) ConfigureClient(c *autorest.Client, authorizer autorest.A // TODO: remove in 2.0 if !features.SupportsCustomTimeouts() { - // this is intentionally de-referencing the timeout since this should be nil when timeouts are enabled by default - c.PollingDuration = *o.PollingDuration + c.PollingDuration = o.PollingDuration } }