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.

Sign up for GitHub

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 - fix crash in multichannel checking #19298

Merged
merged 3 commits into from
Nov 24, 2022
Merged

azurerm_storage_account - fix crash in multichannel checking #19298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d08c549
fix crash in multichannel checking
jackofallops Nov 15, 2022
42d1db1
correct naming on tests
jackofallops Nov 15, 2022
7222321
review feedback changes
jackofallops Nov 16, 2022
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
17 changes: 11 additions & 6 deletions internal/services/storage/storage_account_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1336,17 +1336,22 @@ func resourceStorageAccountCreate(d *pluginsdk.ResourceData, meta interface{}) e
fileServiceClient := meta.(*clients.Client).Storage.FileServicesClient

shareProperties := expandShareProperties(val.([]interface{}))

// The API complains if any multichannel info is sent on non premium fileshares. Even if multichannel is set to false
if accountTier != string(storage.SkuTierPremium) {
if accountTier != string(storage.SkuTierPremium) && shareProperties.FileServicePropertiesProperties.ProtocolSettings != nil {
jackofallops marked this conversation as resolved.
Show resolved Hide resolved

// Error if the user has tried to enable multichannel on a standard tier storage account
if shareProperties.FileServicePropertiesProperties.ProtocolSettings.Smb.Multichannel != nil && shareProperties.FileServicePropertiesProperties.ProtocolSettings.Smb.Multichannel.Enabled != nil {
if *shareProperties.FileServicePropertiesProperties.ProtocolSettings.Smb.Multichannel.Enabled {
return fmt.Errorf("`multichannel_enabled` isn't supported for Standard tier Storage accounts")
smb := shareProperties.FileServicePropertiesProperties.ProtocolSettings.Smb
if smb != nil && smb.Multichannel != nil {

if smb.Multichannel.Enabled != nil {
if *shareProperties.FileServicePropertiesProperties.ProtocolSettings.Smb.Multichannel.Enabled {
return fmt.Errorf("`multichannel_enabled` isn't supported for Standard tier Storage accounts")
}
}
Comment on lines +1346 to 1352
Copy link
Contributor

Choose a reason for hiding this comment

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

probably could simplify this to:

Suggested change
if smb != nil && smb.Multichannel != nil {
if smb.Multichannel.Enabled != nil {
if *shareProperties.FileServicePropertiesProperties.ProtocolSettings.Smb.Multichannel.Enabled {
return fmt.Errorf("`multichannel_enabled` isn't supported for Standard tier Storage accounts")
}
}
if smb != nil && smb.Multichannel != nil && smb.Multichannel.Enabled != nil && * smb.Multichannel.Enabled {
return fmt.Errorf("`multichannel_enabled` isn't supported for Standard tier Storage accounts")
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is dual purpose here, we need to error on *smb.Multichannel.Enabled == true and zero the *smb.Multichannel regardless, hence the split of if conditions, so I think this is correct as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry missed this notification - sounds good/works for me

}

shareProperties.FileServicePropertiesProperties.ProtocolSettings.Smb.Multichannel = nil
shareProperties.FileServicePropertiesProperties.ProtocolSettings.Smb.Multichannel = nil
}
}

if _, err = fileServiceClient.SetServiceProperties(ctx, id.ResourceGroup, id.Name, shareProperties); err != nil {
Expand Down
40 changes: 40 additions & 0 deletions internal/services/storage/storage_account_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1350,6 +1350,21 @@ func TestAccStorageAccount_sasPolicy(t *testing.T) {
})
}

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

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.emptyStorageProperties(data),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
})
}

func (r StorageAccountResource) Exists(ctx context.Context, client *clients.Client, state *pluginsdk.InstanceState) (*bool, error) {
id, err := parse.StorageAccountID(state.ID)
if err != nil {
Expand Down Expand Up @@ -4107,3 +4122,28 @@ resource "azurerm_storage_account" "test" {
}
`, data.RandomInteger, data.Locations.Primary, data.RandomString)
}

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

resource "azurerm_resource_group" "test" {
name = "acctestRG-storage-%[1]d"
location = "%[2]s"
}

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

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

share_properties {}
jackofallops marked this conversation as resolved.
Show resolved Hide resolved
}
`, data.RandomInteger, data.Locations.Primary, data.RandomString)
}