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.

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_storage_account - prevent service endpoint error 404 durring re-create #23002

Closed
Closed
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
98a37d4
added loop and timeout for endpoint services
petr-stupka Aug 17, 2023
b9a44a6
update log messages
petr-stupka Aug 17, 2023
dd2f6db
change function to StateChangeConf
petr-stupka Aug 22, 2023
0e0d0a2
code cleanup
petr-stupka Aug 22, 2023
bcb5cc9
change function logs verbose to DEBUG
petr-stupka Aug 22, 2023
2c40e2d
Merge branch 'hashicorp:main' into b/storage-account-error-404
petr-stupka Aug 23, 2023
f100c07
add error handling
petr-stupka Aug 23, 2023
7f3dc7b
Merge branch 'b/storage-account-error-404' of https://github.com/petr…
petr-stupka Aug 23, 2023
ac9422b
change logs to debug level
petr-stupka Aug 23, 2023
829a0bd
update log messages and other code review comments
petr-stupka Aug 25, 2023
b8d5e67
Update internal/services/storage/storage_account_resource.go
petr-stupka Aug 26, 2023
c84b3f4
Update internal/services/storage/storage_account_resource.go
petr-stupka Aug 26, 2023
2a835fd
Update internal/services/storage/storage_account_resource.go
petr-stupka Aug 26, 2023
b49f350
change to `Timeout: d.Timeout(pluginsdk.TimeoutRead),`
petr-stupka Aug 26, 2023
a4d248a
Merge remote-tracking branch 'origin/b/storage-account-error-404' int…
petr-stupka Aug 26, 2023
e247773
add 404 service error in create operation
petr-stupka Aug 29, 2023
5c6de6a
add 404 service error in create operation
petr-stupka Aug 29, 2023
e68458e
Update internal/services/storage/storage_account_resource.go
petr-stupka Sep 3, 2023
8c1d73f
Update internal/services/storage/storage_account_resource.go
petr-stupka Sep 3, 2023
fc34db3
Update internal/services/storage/storage_account_resource.go
petr-stupka Sep 3, 2023
9222c35
Update internal/services/storage/storage_account_resource.go
petr-stupka Sep 3, 2023
76c19b6
change to deadline all pooling functions
petr-stupka Sep 3, 2023
094fbb2
remove polling from read
petr-stupka Sep 3, 2023
2ba08d0
add test
petr-stupka Sep 3, 2023
07acfd6
Merge branch 'hashicorp:main' into b/storage-account-error-404
petr-stupka Sep 3, 2023
a2fb14c
add test
petr-stupka Sep 3, 2023
2db176d
change response handling
petr-stupka Sep 3, 2023
1b90c7e
fix linting
petr-stupka Sep 3, 2023
b536a63
fix tf linting
petr-stupka Sep 3, 2023
eff5176
Update internal/services/storage/storage_account_resource.go
petr-stupka Sep 7, 2023
c69e92c
Merge branch 'hashicorp:main' into b/storage-account-error-404
petr-stupka Sep 8, 2023
bb6b96d
fix tf linting
petr-stupka Sep 9, 2023
c33eed5
Merge remote-tracking branch 'origin/b/storage-account-error-404' int…
petr-stupka Sep 9, 2023
eab7607
Update internal/services/storage/storage_account_resource.go
petr-stupka Oct 11, 2023
abc3d63
change testing - don't destroy the resource group
petr-stupka Oct 11, 2023
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
165 changes: 165 additions & 0 deletions internal/services/storage/storage_account_resource.go
tombuildsstuff marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package storage

import (
"context"
"errors"
"fmt"
"log"
"net/http"
Expand Down Expand Up @@ -1318,6 +1319,153 @@ func resourceStorageAccountCreate(d *pluginsdk.ResourceData, meta interface{}) e

supportLevel := resolveStorageAccountServiceSupportLevel(storage.Kind(accountKind), storage.SkuTier(accountTier))

// Wait for the account services to become available

if supportLevel.supportBlob {
blobClient := meta.(*clients.Client).Storage.BlobServicesClient

// wait for blob service endpoint to become available
log.Printf("[DEBUG] waiting for %s blob service to become available", id.ID())
deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("internal-error: context had no deadline")
}
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"Pending"},
Target: []string{"Completed"},
Refresh: func() (interface{}, string, error) {
_, err = blobClient.GetServiceProperties(ctx, id.ResourceGroupName, id.StorageAccountName)
if err != nil {
return handleStorageServiceError("blob", err)
}
return true, "Completed", nil
},
MinTimeout: 10 * time.Second,
Timeout: time.Until(deadline),
}

if _, err = stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("fetching %s blob service properties: %+v", id.ID(), err)
}
}

if supportLevel.supportQueue {
storageClient := meta.(*clients.Client).Storage
account, err := storageClient.FindAccount(ctx, id.StorageAccountName)
if err != nil {
return fmt.Errorf("retrieving %s: %+v", id, err)
}
if account == nil {
return fmt.Errorf("unable to locate %q", id)
}

queueClient, err := storageClient.QueuesClient(ctx, *account)
if err != nil {
return fmt.Errorf("building Queues Client: %s", err)
}

// wait for queue service endpoint to become available
log.Printf("[DEBUG] waiting for %s queue service to become available", id.ID())
deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("internal-error: context had no deadline")
}
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"Pending"},
Target: []string{"Completed"},
Refresh: func() (interface{}, string, error) {
properties, err := queueClient.GetServiceProperties(ctx, id.ResourceGroupName, id.StorageAccountName)
if err != nil {
return handleStorageServiceError("queue", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simplify this to:

Suggested change
return handleStorageServiceError("queue", err)
return "Error", "Error", fmt.Errorf("waiting for the Blob %s to become available: %+v", id, err")

Since the handleStorageServiceError function below assumes that this is Completed if it's not a 404 (which would mean we mark this as Completed when it's an error?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the queueClient.GetServiceProperties function returns error (this may be the 404 error or any other error storage API may return)

This error (generic error type) is handled by handleStorageServiceError function. It return Pending, in case of 404, however any other error is returned imidiately with Completed to brake the loop.

This has been requested by @magodo to avoid consuming all errors.

If i understand the logic correctly, in your example the loop will continue also in case of non 404 error?

And one exception here for queue endpoint > there is no error in case of 404 and therefore i put there the workaround below if properties == nil

You the owners, you can say the final word how the code should looks like. From my point of view the function handleStorageServiceError make sense and therefore asking for confirmation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tombuildsstuff Could you please review above comment?

}
// workaround for queueClient.GetServiceProperties() not returning 404 error
if properties == nil {
customErr := azautorest.DetailedError{
StatusCode: http.StatusNotFound,
Message: "Failure responding to request",
}
return handleStorageServiceError("queue", customErr)
petr-stupka marked this conversation as resolved.
Show resolved Hide resolved
}
return true, "Completed", nil
},
MinTimeout: 10 * time.Second,
Timeout: time.Until(deadline),
}

if _, err = stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("fetching %s queue service properties: %+v", id.ID(), err)
}
}

if supportLevel.supportShare {
fileServiceClient := meta.(*clients.Client).Storage.FileServicesClient

// wait for file service endpoint to become available
log.Printf("[DEBUG] waiting for %s file service to become available", id.ID())
deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("internal-error: context had no deadline")
}
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"Pending"},
Target: []string{"Completed"},
Refresh: func() (interface{}, string, error) {
_, err = fileServiceClient.GetServiceProperties(ctx, id.ResourceGroupName, id.StorageAccountName)
if err != nil {
return handleStorageServiceError("file", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above) we should be able to just return Pending here instead?

}
return true, "Completed", nil
},
MinTimeout: 10 * time.Second,
Timeout: time.Until(deadline),
}

if _, err = stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("fetching %s file service properties: %+v", id.ID(), err)
}
}

if supportLevel.supportStaticWebsite {
storageClient := meta.(*clients.Client).Storage

account, err := storageClient.FindAccount(ctx, id.StorageAccountName)
if err != nil {
return fmt.Errorf("retrieving %s: %+v", id, err)
}
if account == nil {
return fmt.Errorf("unable to locate %s", id)
}

accountsClient, err := storageClient.AccountsDataPlaneClient(ctx, *account)
if err != nil {
return fmt.Errorf("building Accounts Data Plane Client: %s", err)
}

// wait for static website endpoint to become available
log.Printf("[DEBUG] waiting for %s static website service to become available", id.ID())
deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("internal-error: context had no deadline")
}
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{"Pending"},
Target: []string{"Completed"},
Refresh: func() (interface{}, string, error) {
_, err = accountsClient.GetServiceProperties(ctx, id.StorageAccountName)
if err != nil {
return handleStorageServiceError("static website", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above) we should be able to just return Pending here instead?

}
return true, "Completed", nil
},
MinTimeout: 10 * time.Second,
Timeout: time.Until(deadline),
}

if _, err = stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("fetching %s static website properties: %+v", id.ID(), err)
}
}

if val, ok := d.GetOk("blob_properties"); ok {
if !supportLevel.supportBlob {
return fmt.Errorf("`blob_properties` aren't supported for account kind %q in sku tier %q", accountKind, accountTier)
Expand Down Expand Up @@ -2275,6 +2423,23 @@ func resourceStorageAccountDelete(d *pluginsdk.ResourceData, meta interface{}) e
return nil
}

func handleStorageServiceError(service string, err error) (interface{}, string, error) {
// if the error is a DetailedError type, check the status code and return the appropriate state
var respErr azautorest.DetailedError
if errors.As(err, &respErr) {
log.Printf("[DEBUG] error fetching %s service properties: statusCode=%d, message=%s, error=%s\n", service, respErr.StatusCode.(int), respErr.Message, err)
// if the status code is 404 (not found), retry the request
if respErr.StatusCode.(int) == http.StatusNotFound {
// if the status code is 404 (not found), retry the request
log.Printf("[DEBUG] %s service is not available, retrying...\n", service)
return false, "Pending", nil
}
}
// if the error is unhandled type or status, log the error and return the error state
log.Printf("[DEBUG] unexpected error while fetching %s service properties: %v\n", service, err)
return nil, "Completed", err
}
Comment on lines +2422 to +2437
Copy link
Contributor

Choose a reason for hiding this comment

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

as such I think this function can be removed?


func expandStorageAccountCustomDomain(d *pluginsdk.ResourceData) *storage.CustomDomain {
domains := d.Get("custom_domain").([]interface{})
if len(domains) == 0 {
Expand Down
114 changes: 114 additions & 0 deletions internal/services/storage/storage_account_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,70 @@ func TestAccStorageAccount_requiresImport(t *testing.T) {
})
}

func TestAccStorageAccount_recreateBasic(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_storage_account", "test")
r := StorageAccountResource{}

// fix location to test recreation
data.Locations.Primary = "westeurope"

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.basic(data),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
{
// destroy the account and recreate it
Config: r.recreate(data),
Destroy: true,
},
{
Config: r.basic(data),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
})
}

func TestAccStorageAccount_recreateWithProperties(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_storage_account", "test")
r := StorageAccountResource{}

// fix location to test recreation
data.Locations.Primary = "westeurope"

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.recreate(data),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
{
// destroy the account and recreate it
Config: r.recreate(data),
Destroy: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

this would destroy the resource group too, instead we can delete just the account via:

Suggested change
{
// destroy the account and recreate it
Config: r.recreate(data),
Destroy: true,
{
// destroy the account so that we can recreate it below
Config: r.template(data),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. also for basic account

},
{
Config: r.recreate(data),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
check.That(data.ResourceName).Key("blob_properties.0.versioning_enabled").HasValue("true"),
check.That(data.ResourceName).Key("share_properties.0.retention_policy.0.days").HasValue("14"),
check.That(data.ResourceName).Key("queue_properties.0.cors_rule.0.max_age_in_seconds").HasValue("3600"),
check.That(data.ResourceName).Key("static_website.0.index_document").HasValue("test.html"),
),
},
data.ImportStep(),
})
}

func TestAccStorageAccount_noCrossTenantReplication(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_storage_account", "test")
r := StorageAccountResource{}
Expand Down Expand Up @@ -1537,6 +1601,56 @@ resource "azurerm_storage_account" "test" {
`, data.RandomInteger, data.Locations.Primary, data.RandomString)
}

func (r StorageAccountResource) recreate(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azurerm" {
features {}
}

resource "azurerm_resource_group" "test" {
name = "acctestRG-storage-%d"
location = "%s"
}

resource "azurerm_storage_account" "test" {
name = "unlikely23exst2acct%s"
resource_group_name = azurerm_resource_group.test.name

location = azurerm_resource_group.test.location
account_tier = "Standard"
account_replication_type = "LRS"

blob_properties {
versioning_enabled = true
}

share_properties {
retention_policy {
days = 14
}
}

queue_properties {
cors_rule {
allowed_headers = ["*"]
allowed_methods = ["GET", "POST", "OPTIONS"]
allowed_origins = ["*"]
exposed_headers = ["*"]
max_age_in_seconds = 3600
}
}

static_website {
index_document = "test.html"
}

tags = {
environment = "production"
}
}
`, data.RandomInteger, data.Locations.Primary, data.RandomString)
}

func (r StorageAccountResource) publicNetworkAccess(data acceptance.TestData, enabled bool) string {
return fmt.Sprintf(`
provider "azurerm" {
Expand Down