Storage Account: Enable DefaultAction in Network rules.

[thatInfrastructureGuy]

Introduction: DefaultAction is the toggle which enables/disables network rules in azure storage account.
Setting it to Deny enables storage account firewall settings and Allow disables the firewalls.

Problem: When using azure storage account as module with network_rules block, there is no good way to disable network security apart from removing the network_rules block entirely from the root module.

PR objective: This PR aims to solve this problem by implementing DefaultAction variable.

Storage Account - Module with network_rules block:

 network_rules {
    default_action             = "${var.default_action}"
    virtual_network_subnet_ids = ["${var.virtual_network_subnet_ids}"]
    bypass                     = ["${var.bypass}"]
    ip_rules                   = ["${var.ip_rules}"]
  }

Storage Account - Deployment 1 with no firewall:

 network_rules {
    default_action             = "Allow"
    virtual_network_subnet_ids = ""
    bypass                     = ""
    ip_rules                   = ""
  }

Storage Account - Deployment 2 with firewall:

 network_rules {
    default_action             = "Deny"
    virtual_network_subnet_ids = "${azurerm_subnet.test.id"
    bypass                     = ["Logging", "Metrics"]
    ip_rules                   = ["127.0.0.1", "127.0.0.2"]
  }

[tombuildsstuff]

hey @thatInfrastructureGuy

Thanks for this PR :)

I've taken a look through and left some comments inline but this is off to a good start - if we can fix up the comments we should be able to run the tests and get this merged 👍

Thanks!

[thatInfrastructureGuy]

@tombuildsstuff Thanks for quick review. :) I have made the requested changes. I do agree default_action should be required field and having to let users decide on the value of it.

[thatInfrastructureGuy]

        FAIL: TestAccAzureRMStorageAccount_networkRulesDeleted (150.14s)
    testing.go:538: Step 1 error: After applying this step, the plan was not empty:

        DIFF:

        UPDATE: azurerm_storage_account.testsa
          network_rules.#:                                       "1" => "0"
          network_rules.0.default_action:                        "Allow" => ""
          network_rules.0.ip_rules.#:                            "1" => "0"
          network_rules.0.ip_rules.3619153832:                   "127.0.0.1" => ""
          network_rules.0.virtual_network_subnet_ids.#:          "1" => "0"
          network_rules.0.virtual_network_subnet_ids.1226267014: "/subscriptions/xxx/r
esourceGroups/acctestAzureRMSA-190416064647690939/providers/Microsoft.Network/virtualNetworks/acctestvirtnet19041606464
7690939/subnets/acctestsubnet190416064647690939" => ""
                                                                                                      
        STATE:

        azurerm_resource_group.testrg:
          network_rules.# = 1
          network_rules.0.bypass.# = 1
          network_rules.0.bypass.625562768 = AzureServices
          network_rules.0.default_action = Allow
          network_rules.0.ip_rules.# = 1
          network_rules.0.ip_rules.3619153832 = 127.0.0.1
          network_rules.0.virtual_network_subnet_ids.# = 1
          network_rules.0.virtual_network_subnet_ids.1226267014 = /subscriptions/xxx/r
esourceGroups/acctestAzureRMSA-190416064647690939/providers/Microsoft.Network/virtualNetworks/acctestvirtnet19041606464
7690939/subnets/acctestsubnet190416064647690939

I believe this happens because the rules itself are not deleted. Only the flag "defaultAction": "Allow" is set on the backend.

[tombuildsstuff]

@thatInfrastructureGuy

I believe this happens because the rules itself are not deleted. Only the flag "defaultAction": "Allow" is set on the backend.

Indeed - because the NetworkRuleSet created when Allow is set doesn't contain any IP Rules/Virtual Network Rules, these aren't being overridden. If an empty list is specified for those then this should pass 👍

[thatInfrastructureGuy]

@tombuildsstuff Thanks for the pointer. However, I am not able to override those IP Rules/Virtual Network Rules even after specifying empty list. I tried the below snippet:

return &storage.NetworkRuleSet{
			DefaultAction:       storage.DefaultActionAllow,
			Bypass:              storage.Bypass(""),
			IPRules:             &[]storage.IPRule{},
			VirtualNetworkRules: &[]storage.VirtualNetworkRule{},
		}

I dont know what I am missing. Can you please guide me here?

[katbyte]

Hi @thatInfrastructureGuy,

Thank you for the PR, this LGTM, however i am curious as to why you explicitly don't send the network rules when it is allow. Will the API not be happy?

[ashishkulk]

Added response in the comments.

[katbyte]

@thatInfrastructureGuy,

See my comment left inline. I am still wondering why you are conditionally setting the properties on create/update. Does the API throw an error if you set them?

[katbyte]

👋 @thatInfrastructureGuy,

I hope you don't mind but i've pushed a commit demonstrating what I was referring to. I also changed the property to optional as a new required property in an existing block could potentially break peoples configuration.

[thatInfrastructureGuy]

@katbyte Sorry for late response. I was on vacation for a bit and never really got back to this PR. Thanks so much for review and the commit. I am totally good with it.

[katbyte]

LGTM now 👍

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!