NSG security_rule diff are too big when optional properties are not defined

[sylr]

Terraform (and AzureRM Provider) Version

Terraform v0.12.20
azurerm 1.44.0

Affected Resource(s)

• azurerm_network_security_group

Terraform Configuration Files

resource "azurerm_network_security_group" "test-nsg" {
  name                = "test-nsg"
  resource_group_name = "test-rg"
  location            = "westeurope"

  security_rule {
    name                       = "rule-1"
    priority                   = 1000
    direction                  = "Inbound"
    access                     = "Allow"
    protocol                   = "Tcp"
    source_address_prefixes    = ["8.8.8.8"]
    source_port_range          = "*"
    destination_address_prefix = "*"
    destination_port_range     = "22"
  }

  security_rule {
    name                       = "rule-2"
    priority                   = 1001
    direction                  = "Inbound"
    access                     = "Allow"
    protocol                   = "Tcp"
    source_address_prefix      = "9.9.9.9"
    source_port_ranges         = ["53"]
    destination_address_prefix = "*"
    destination_port_range     = "22"
  }

  security_rule {
    name                       = "rule-3"
    priority                   = 1002
    direction                  = "Inbound"
    access                     = "Allow"
    protocol                   = "Tcp"
    source_address_prefixes    = ["19.19.19.19"]
    destination_address_prefix = "*"
    destination_port_ranges    = ["22", "23"]
  }

  security_rule {
    name                         = "rule-4"
    priority                     = 1003
    direction                    = "Inbound"
    access                       = "Allow"
    protocol                     = "Tcp"
    source_address_prefixes      = ["12.12.12.12"]
    source_port_ranges           = ["10000", "11111"]
    destination_address_prefixes = ["*"]
    destination_port_ranges      = ["22", "23"]
  }
}

Expected Behavior

A change in a security_rule of a NSG should display a diff explaining that only the modified security_rule will be modified

Actual Behavior

The security_rule sets are composed of mutually exclusive properties:

• source_address_prefix (string) / source_address_prefixes ([]string)
• source_port_range (string) / source_port_ranges ([]string)
• destination_address_prefix (string) / destination_address_prefixes ([]string)
• destination_port_range (string) / destination_port_ranges ([]string)

If you modify a NSG which is composed of several rules with ones using the string version and others using []string versions of the properties without explicitly define is counter part as empty, the slight modification of a rule will output a plan that tells you he is about to updates almost all the security_rules.

Steps to Reproduce

1. terraform apply the NSG I gave sooner
2. Modify one of the source/destination property
3. terraform plan
4. Observe the diff

I believe the problem comes from the fact that terraform diff engine treat nil and "" as being different but Azure does not.

The other way to look at it is the terraform azure provider does not instanciate as it should the optional values of the security_rule set.

Regards.

[tombuildsstuff]

@magodo unfortunately that won't work since this response is intentionally unordered in the Azure API

[magodo]

@tombuildsstuff
If it is unordered in API response, then list doesn't fit here. So I wonder why does the un-modified rules be replaced? Does it mean the default SchemaSetFunction is not deterministic?

[sylr]

According to me there are two problems:

• Properties returned empty by Azure vs. Undeclared optional properties in tf files that are considered nil.
• Terraform Set items that are not ordered and do not have uniq keys.

@tombuildsstuff I tried to do this:

https://github.com/sylr/terraform-provider-azurerm/blob/5337d59ae923711f8434222943870cb1c837c1d3/azurerm/internal/services/network/data_source_network_security_group.go#L38-L43

I made that to generate a uniq id based on a tuple of properties that are uniq in 1zure (direction and priority). I hoped terraform mechanism would use that to generate "In set item" property diff instead of removing the set item to replace it by a new one. This does not seem to work.

If you can enlighten me about the purpose of the Set schema property I would really appreciate it.

[sylr]

I also tried to order the security_rules based on the uniqness of direction & priority

https://github.com/sylr/terraform-provider-azurerm/blob/5337d59ae923711f8434222943870cb1c837c1d3/azurerm/internal/services/network/resource_arm_network_security_group.go#L490

https://github.com/sylr/terraform-provider-azurerm/blob/5337d59ae923711f8434222943870cb1c837c1d3/azurerm/internal/services/network/resource_arm_network_security_group.go#L497-L535

It does not reflect in the plan output.

[magodo]

I also tried to define the Set function. It turns out this function doesn't only compute the index of item in set, but also used to tell whether the element has been changed. So if you hash by "direction", then you have to modify "direction" so that terraform can pick up the diff.

Another clue is that terraform support nil value in root level propert, but doesn't for nested property. So if, for instance, "destination_port_range" is not set at all, it will be stored as empty string in state file (the reason is because of resource data implementation for Get method)

[sylr]

I also tried to define the Set function. It turns out this function doesn't only compute the index of item in set, but also used to tell whether the element has been changed. So if you hash by "direction", then you have to modify "direction" so that terraform can pick up the diff.

Absolutely, I think the schema really needs something like a SetItemKey property accepting a function so that the plan can do diffs inside Set items.

[magodo]

Hi @sylr

Properties returned empty by Azure vs. Undeclared optional properties in tf files that are considered nil.

You are right, but only for TypeString.

When generating the plan, terraform will ultimately call diffString to resolve the attribute diff for string fields of security_rule. The old value represents the one in state, which because of nested block will be "auto" fill nil fields as its zero value (as I mentioned in last thread), is empty string ""; while the new value represents the one defined in config, which is omitted, hence nil. So the diffString will resolve as this field is removed. This happens even if you didn't modify anything for a "security_rule".

So sounds like that will happen in any case, even a terraform plan just after a terraform apply, with no change at all. But it is actually not, that's because during the plan, before diff the inner fields inside a set, the diffSet will firstly try to comparing listCode of two sets to see if they are identical. So if you haven't changed any "security_rule" set, then it will remain the same, which resolves to no diff.

Addtionally, this doesn't apply to TypeSet (such as source_port_ranges) itself, since the diffSet is different from diffString.

An easy way to fix this is to set the Default value for each optional string property inside security_rule schema. Since even these fields are absent in cfg, they will ultimately show as "" in state file, so it sounds reasonable to give them a default value "".

[magodo]

There is a related issue in core: hashicorp/terraform#21901

[dsiperek-vendavo]

Hi @sylr

Properties returned empty by Azure vs. Undeclared optional properties in tf files that are considered nil.

You are right, but only for TypeString.

When generating the plan, terraform will ultimately call diffString to resolve the attribute diff for string fields of security_rule. The old value represents the one in state, which because of nested block will be "auto" fill nil fields as its zero value (as I mentioned in last thread), is empty string ""; while the new value represents the one defined in config, which is omitted, hence nil. So the diffString will resolve as this field is removed. This happens even if you didn't modify anything for a "security_rule".

So sounds like that will happen in any case, even a terraform plan just after a terraform apply, with no change at all. But it is actually not, that's because during the plan, before diff the inner fields inside a set, the diffSet will firstly try to comparing listCode of two sets to see if they are identical. So if you haven't changed any "security_rule" set, then it will remain the same, which resolves to no diff.

Addtionally, this doesn't apply to TypeSet (such as source_port_ranges) itself, since the diffSet is different from diffString.

An easy way to fix this is to set the Default value for each optional string property inside security_rule schema. Since even these fields are absent in cfg, they will ultimately show as "" in state file, so it sounds reasonable to give them a default value "".

This work around does work. I was able to only get the actual changes to show by setting the "" or [] values instead of leaving them blank!
Thanks

[favoretti]

Since this issue seems to have been addressed in the latest versions of the provider (or a valid workaround was provided) - I'm going to close it. Please open a new updated bug report if this is still relevant. Thank you.

[github-actions]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.