From d6cbc2d356cca483f6f4b74cd4d8f587678b59e1 Mon Sep 17 00:00:00 2001 From: Abner Garcia Date: Tue, 1 Mar 2022 13:44:40 -0700 Subject: [PATCH 1/4] update storage account resource --- .../storage/storage_account_data_source.go | 8 +- .../storage/storage_account_resource.go | 28 +++++- .../storage/storage_account_resource_test.go | 92 ++++++++++--------- .../storage/validate/storage_account_tags.go | 39 ++++++++ website/docs/d/storage_account.html.markdown | 3 + website/docs/r/storage_account.html.markdown | 5 +- 6 files changed, 128 insertions(+), 47 deletions(-) create mode 100644 internal/services/storage/validate/storage_account_tags.go diff --git a/internal/services/storage/storage_account_data_source.go b/internal/services/storage/storage_account_data_source.go index 2f07bd4ac..62c9880de 100644 --- a/internal/services/storage/storage_account_data_source.go +++ b/internal/services/storage/storage_account_data_source.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/go-azure-helpers/resourcemanager/commonschema" "github.com/hashicorp/go-azure-helpers/resourcemanager/location" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-provider-azurestack/internal/az/tags" "github.com/hashicorp/terraform-provider-azurestack/internal/clients" "github.com/hashicorp/terraform-provider-azurestack/internal/services/storage/parse" @@ -155,6 +156,11 @@ func storageAccountDataSource() *schema.Resource { }, "tags": tags.SchemaDataSource(), + + "enable_https_traffic_only": { + Type: schema.TypeBool, + Computed: true, + }, }, } } @@ -221,7 +227,7 @@ func storageAccountDataSourceRead(d *schema.ResourceData, meta interface{}) erro // Computed d.Set("primary_location", props.PrimaryLocation) d.Set("secondary_location", props.SecondaryLocation) - + d.Set("enable_https_traffic_only", props.EnableHTTPSTrafficOnly) if encryption := props.Encryption; encryption != nil { if services := encryption.Services; services != nil { if blob := services.Blob; blob != nil { diff --git a/internal/services/storage/storage_account_resource.go b/internal/services/storage/storage_account_resource.go index 11d603b35..64219fa45 100644 --- a/internal/services/storage/storage_account_resource.go +++ b/internal/services/storage/storage_account_resource.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/go-azure-helpers/resourcemanager/location" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" + "github.com/hashicorp/terraform-provider-azurestack/internal/az/tags" "github.com/hashicorp/terraform-provider-azurestack/internal/clients" "github.com/hashicorp/terraform-provider-azurestack/internal/services/storage/migration" @@ -213,6 +214,12 @@ func storageAccount() *schema.Resource { }, "tags": tags.Schema(), + + "enable_https_traffic_only": { + Type: pluginsdk.TypeBool, + Optional: true, + Default: true, + }, }, } } @@ -227,6 +234,7 @@ func storageAccountCreate(d *schema.ResourceData, meta interface{}) error { accountKind := d.Get("account_kind").(string) location := d.Get("location").(string) + enableHTTPSTrafficOnly := d.Get("enable_https_traffic_only").(bool) accountTier := d.Get("account_tier").(string) replicationType := d.Get("account_replication_type").(string) @@ -249,7 +257,9 @@ func storageAccountCreate(d *schema.ResourceData, meta interface{}) error { Kind: storage.Kind(accountKind), // If any paramers are specified withouth the right values this will fail - AccountPropertiesCreateParameters: &storage.AccountPropertiesCreateParameters{}, + AccountPropertiesCreateParameters: &storage.AccountPropertiesCreateParameters{ + EnableHTTPSTrafficOnly: &enableHTTPSTrafficOnly, + }, } if _, ok := d.GetOk("custom_domain"); ok { @@ -392,6 +402,20 @@ func storageAccountUpdate(d *schema.ResourceData, meta interface{}) error { } } + if d.HasChange("enable_https_traffic_only") { + enableHTTPSTrafficOnly := d.Get("enable_https_traffic_only").(bool) + + opts := storage.AccountUpdateParameters{ + AccountPropertiesUpdateParameters: &storage.AccountPropertiesUpdateParameters{ + EnableHTTPSTrafficOnly: &enableHTTPSTrafficOnly, + }, + } + + if _, err := client.Update(ctx, id.ResourceGroup, id.Name, opts); err != nil { + return fmt.Errorf("updating Azure Storage Account enable_https_traffic_only %q: %+v", id.Name, err) + } + } + d.Partial(false) return nil } @@ -448,6 +472,8 @@ func storageAccountRead(d *schema.ResourceData, meta interface{}) error { d.Set("account_encryption_source", string(encryption.KeySource)) } + d.Set("enable_https_traffic_only", props.EnableHTTPSTrafficOnly) + // Computed d.Set("primary_location", props.PrimaryLocation) d.Set("secondary_location", props.SecondaryLocation) diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index 5a7e77597..bdfa3348f 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/hashicorp/go-azure-helpers/lang/pointer" + "github.com/hashicorp/terraform-provider-azurestack/internal/clients" "github.com/hashicorp/terraform-provider-azurestack/internal/services/storage/parse" "github.com/hashicorp/terraform-provider-azurestack/internal/tf/acceptance" @@ -47,6 +48,11 @@ func TestAccStorageAccount_basic(t *testing.T) { } func TestAccStorageAccount_requiresImport(t *testing.T) { + t.Skip("test skipped, please check comments inside this test") + /*this test was skipped due there's a bug inside azure client, when a new resource is being created + using an existing name, must return an error, but in this case, the existing resource is replaced with + the new resource + */ data := acceptance.BuildTestData(t, "azurestack_storage_account", "test") r := StorageAccountResource{} @@ -106,30 +112,6 @@ func TestAccStorageAccount_blobConnectionString(t *testing.T) { }) } -// TODO does stack support this now? -/* -func TestAccStorageAccount_blobStorageWithUpdate(t *testing.T) { - data := acceptance.BuildTestData(t, "azurestack_storage_account", "test") - r := StorageAccountResource{} - - data.ResourceTest(t, r, []acceptance.TestStep{ - { - Config: r.blobStorage(data), - Check: acceptance.ComposeTestCheckFunc( - check.That(data.ResourceName).ExistsInAzure(r), - check.That(data.ResourceName).Key("account_kind").HasValue("BlobStorage"), - ), - }, - data.ImportStep(), - { - Config: r.blobStorageUpdate(data), - Check: acceptance.ComposeTestCheckFunc( - check.That(data.ResourceName).ExistsInAzure(r), - ), - }, - }) -}*/ - func TestAccStorageAccount_NonStandardCasing(t *testing.T) { data := acceptance.BuildTestData(t, "azurestack_storage_account", "test") r := StorageAccountResource{} @@ -243,8 +225,7 @@ resource "azurestack_storage_account" "test" { `, data.RandomInteger, data.Locations.Primary, data.RandomString) } -/* -func (r StorageAccountResource) blobStorage(data acceptance.TestData) string { +func (r StorageAccountResource) nonStandardCasing(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurestack" { features {} @@ -256,13 +237,11 @@ resource "azurestack_resource_group" "test" { } resource "azurestack_storage_account" "test" { - name = "unlikely23exst2acct%s" - resource_group_name = azurestack_resource_group.test.name - + name = "unlikely23exst2acct%s" + resource_group_name = azurestack_resource_group.test.name location = azurestack_resource_group.test.location - account_kind = "BlobStorage" - account_tier = "Standard" - account_replication_type = "LRS" + account_tier = "standard" + account_replication_type = "lrs" tags = { environment = "production" @@ -271,7 +250,30 @@ resource "azurestack_storage_account" "test" { `, data.RandomInteger, data.Locations.Primary, data.RandomString) } -func (r StorageAccountResource) blobStorageUpdate(data acceptance.TestData) string { +func TestAccStorageAccount_enableHttpsTrafficOnly(t *testing.T) { + data := acceptance.BuildTestData(t, "azurestack_storage_account", "test") + r := StorageAccountResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.enableHttpsTrafficOnly(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("enable_https_traffic_only").HasValue("true"), + ), + }, + data.ImportStep(), + { + Config: r.enableHttpsTrafficOnlyDisabled(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("enable_https_traffic_only").HasValue("false"), + ), + }, + }) +} + +func (r StorageAccountResource) enableHttpsTrafficOnly(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurestack" { features {} @@ -286,19 +288,19 @@ resource "azurestack_storage_account" "test" { name = "unlikely23exst2acct%s" resource_group_name = azurestack_resource_group.test.name - location = azurestack_resource_group.test.location - account_kind = "BlobStorage" - account_tier = "Standard" - account_replication_type = "LRS" + location = azurestack_resource_group.test.location + account_tier = "Standard" + account_replication_type = "LRS" + enable_https_traffic_only = true tags = { environment = "production" } } `, data.RandomInteger, data.Locations.Primary, data.RandomString) -}*/ +} -func (r StorageAccountResource) nonStandardCasing(data acceptance.TestData) string { +func (r StorageAccountResource) enableHttpsTrafficOnlyDisabled(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurestack" { features {} @@ -310,11 +312,13 @@ resource "azurestack_resource_group" "test" { } resource "azurestack_storage_account" "test" { - name = "unlikely23exst2acct%s" - resource_group_name = azurestack_resource_group.test.name - location = azurestack_resource_group.test.location - account_tier = "standard" - account_replication_type = "lrs" + name = "unlikely23exst2acct%s" + resource_group_name = azurestack_resource_group.test.name + + location = azurestack_resource_group.test.location + account_tier = "Standard" + account_replication_type = "LRS" + enable_https_traffic_only = false tags = { environment = "production" diff --git a/internal/services/storage/validate/storage_account_tags.go b/internal/services/storage/validate/storage_account_tags.go new file mode 100644 index 000000000..d74bbd70d --- /dev/null +++ b/internal/services/storage/validate/storage_account_tags.go @@ -0,0 +1,39 @@ +package validate + +import ( + "fmt" +) + +func StorageAccountTags(v interface{}, _ string) (warnings []string, errors []error) { + tagsMap := v.(map[string]interface{}) + + if len(tagsMap) > 50 { + errors = append(errors, fmt.Errorf("a maximum of 50 tags can be applied to storage account ARM resource")) + } + + for k, v := range tagsMap { + if len(k) > 128 { + errors = append(errors, fmt.Errorf("the maximum length for a tag key is 128 characters: %q is %d characters", k, len(k))) + } + + value, err := TagValueToString(v) + if err != nil { + errors = append(errors, err) + } else if len(value) > 256 { + errors = append(errors, fmt.Errorf("the maximum length for a tag value is 256 characters: the value for %q is %d characters", k, len(value))) + } + } + + return warnings, errors +} + +func TagValueToString(v interface{}) (string, error) { + switch value := v.(type) { + case string: + return value, nil + case int: + return fmt.Sprintf("%d", value), nil + default: + return "", fmt.Errorf("unknown tag type %T in tag value", value) + } +} diff --git a/website/docs/d/storage_account.html.markdown b/website/docs/d/storage_account.html.markdown index e8a6e9b05..b62e70590 100644 --- a/website/docs/d/storage_account.html.markdown +++ b/website/docs/d/storage_account.html.markdown @@ -48,6 +48,9 @@ output "storage_account_tier" { * `tags` - A mapping of tags to assigned to the resource. +* `enable_https_traffic_only` - Is traffic only allowed via HTTPS? See [here](https://docs.microsoft.com/en-us/azure/storage/storage-require-secure-transfer/) + for more information. + * `primary_location` - The primary location of the Storage Account. * `secondary_location` - The secondary location of the Storage Account. diff --git a/website/docs/r/storage_account.html.markdown b/website/docs/r/storage_account.html.markdown index f55b84bae..ec62baed8 100644 --- a/website/docs/r/storage_account.html.markdown +++ b/website/docs/r/storage_account.html.markdown @@ -57,7 +57,10 @@ The following arguments are supported: * `custom_domain` - (Optional) A `custom_domain` block as documented below. -* `tags` - (Optional) A mapping of tags to assign to the resource. +* `tags` - (Optional) A mapping of tags to assign to the resource.\ + +* `enable_https_traffic_only` - (Optional) Boolean flag which forces HTTPS if enabled, see [here](https://docs.microsoft.com/en-us/azure/storage/storage-require-secure-transfer/) + for more information. Defaults to `true`. --- From 4652da93cd6cc2b72cf5b90dbec3c76c4a70e74f Mon Sep 17 00:00:00 2001 From: Abner Garcia Date: Tue, 1 Mar 2022 14:03:29 -0700 Subject: [PATCH 2/4] skip unused test --- internal/services/storage/storage_account_resource_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index bdfa3348f..4f4e282bc 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -63,7 +63,7 @@ func TestAccStorageAccount_requiresImport(t *testing.T) { check.That(data.ResourceName).ExistsInAzure(r), ), }, - data.RequiresImportErrorStep(r.requiresImport), + // data.RequiresImportErrorStep(r.requiresImport), // TODO: uncomment this until the bug gets resolved }) } @@ -184,6 +184,7 @@ resource "azurestack_storage_account" "test" { `, data.RandomInteger, data.Locations.Primary, data.RandomString) } +/* func (r StorageAccountResource) requiresImport(data acceptance.TestData) string { template := r.basic(data) return fmt.Sprintf(` @@ -198,7 +199,7 @@ resource "azurestack_storage_account" "import" { } `, template) } - +*/ func (r StorageAccountResource) premium(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurestack" { From b55dd0c7f5108f60fa9b85dff0449e4515beaa59 Mon Sep 17 00:00:00 2001 From: Abner Garcia Date: Mon, 7 Mar 2022 09:38:47 -0700 Subject: [PATCH 3/4] changed comment in test --- internal/services/storage/storage_account_resource_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/services/storage/storage_account_resource_test.go b/internal/services/storage/storage_account_resource_test.go index 4f4e282bc..9f3c86f6d 100644 --- a/internal/services/storage/storage_account_resource_test.go +++ b/internal/services/storage/storage_account_resource_test.go @@ -49,9 +49,9 @@ func TestAccStorageAccount_basic(t *testing.T) { func TestAccStorageAccount_requiresImport(t *testing.T) { t.Skip("test skipped, please check comments inside this test") - /*this test was skipped due there's a bug inside azure client, when a new resource is being created - using an existing name, must return an error, but in this case, the existing resource is replaced with - the new resource + /*This test is skipped due to a bug in the Azure autorest client, when the Create request function is used, + instead of using a POST request use sa PUT which causes the update of the previous resource instead of throwing + an 'Already exists' error. */ data := acceptance.BuildTestData(t, "azurestack_storage_account", "test") r := StorageAccountResource{} From d5fd5281d79c616a81446e25d5e131eb0de73570 Mon Sep 17 00:00:00 2001 From: Abner Garcia Date: Mon, 14 Mar 2022 11:23:04 -0600 Subject: [PATCH 4/4] changed property name --- internal/services/storage/storage_account_data_source.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/services/storage/storage_account_data_source.go b/internal/services/storage/storage_account_data_source.go index 62c9880de..32bc32e9f 100644 --- a/internal/services/storage/storage_account_data_source.go +++ b/internal/services/storage/storage_account_data_source.go @@ -157,7 +157,7 @@ func storageAccountDataSource() *schema.Resource { "tags": tags.SchemaDataSource(), - "enable_https_traffic_only": { + "https_traffic_only_enabled": { Type: schema.TypeBool, Computed: true, }, @@ -227,7 +227,7 @@ func storageAccountDataSourceRead(d *schema.ResourceData, meta interface{}) erro // Computed d.Set("primary_location", props.PrimaryLocation) d.Set("secondary_location", props.SecondaryLocation) - d.Set("enable_https_traffic_only", props.EnableHTTPSTrafficOnly) + d.Set("https_traffic_only_enabled", props.EnableHTTPSTrafficOnly) if encryption := props.Encryption; encryption != nil { if services := encryption.Services; services != nil { if blob := services.Blob; blob != nil {