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

HasChange always return true for type map[string] Set #617

Closed
yupwei68 opened this issue Oct 20, 2020 · 5 comments
Closed

HasChange always return true for type map[string] Set #617

yupwei68 opened this issue Oct 20, 2020 · 5 comments
Labels
bug Something isn't working subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.
Milestone

Comments

@yupwei68
Copy link

yupwei68 commented Oct 20, 2020

SDK version

v1.13.1

Relevant provider source code

"network_rules": {
				Type:     schema.TypeList,
				Optional: true,
				Computed: true,
				MaxItems: 1,
				Elem: &schema.Resource{
					Schema: map[string]*schema.Schema{
						"bypass": {
							Type:     schema.TypeSet,
							Optional: true,
							Computed: true,
							Elem: &schema.Schema{
								Type: schema.TypeString,
								ValidateFunc: validation.StringInSlice([]string{
									string(storage.AzureServices),
									string(storage.Logging),
									string(storage.Metrics),
									string(storage.None),
								}, true),
							},
							Set: schema.HashString,
						},

						"ip_rules": {
							Type:     schema.TypeSet,
							Optional: true,
							Computed: true,
							Elem: &schema.Schema{
								Type:         schema.TypeString,
								ValidateFunc: validate.StorageAccountIPRule,
							},
							Set: schema.HashString,
						},

						"virtual_network_subnet_ids": {
							Type:     schema.TypeSet,
							Optional: true,
							Computed: true,
							Elem:     &schema.Schema{Type: schema.TypeString},
							Set:      schema.HashString,
						},

						"default_action": {
							Type:     schema.TypeString,
							Required: true,
							ValidateFunc: validation.StringInSlice([]string{
								string(storage.DefaultActionAllow),
								string(storage.DefaultActionDeny),
							}, false),
						},
					},
				},
			},
...
func flattenStorageAccountNetworkRules(input *storage.NetworkRuleSet) []interface{} {
	if input == nil {
		return []interface{}{}
	}

	networkRules := make(map[string]interface{})

	networkRules["ip_rules"] = schema.NewSet(schema.HashString, flattenStorageAccountIPRules(input.IPRules))
	networkRules["virtual_network_subnet_ids"] = schema.NewSet(schema.HashString, flattenStorageAccountVirtualNetworks(input.VirtualNetworkRules))
	networkRules["bypass"] = schema.NewSet(schema.HashString, flattenStorageAccountBypass(input.Bypass))
	networkRules["default_action"] = string(input.DefaultAction)

	return []interface{}{networkRules}
}
...
if d.HasChange("network_rules") {
...
}

Terraform Configuration Files

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"

  tags = {
    environment = "production"
  }
}

Debug Output

I can see d.HasChange has used Set's Equal() before reflect.DeepEqual(o, n). But it works only for Set. It doesn't work for complicated struct including type Set.

Expected Behavior

If no update in "network_rules", d.HasChange("network_rules") should return false

Actual Behavior

d.HasChange("network_rules") always return true

Steps to Reproduce

Please list the full steps required to reproduce the issue, for example:

  1. terraform init
  2. terraform apply
  3. terraform apply

References

@yupwei68 yupwei68 added the bug Something isn't working label Oct 20, 2020
@gdavison
Copy link
Contributor

This is still occurring in v2.2.0. I will provide some test cases that demonstrate the error.

@gdavison
Copy link
Contributor

Here are two test cases that can be added to TestResourceDataHasChange() in helper/schema/resource_data_test.go

This test case should not indicate that there is a change, but currently does:

{
	Schema: map[string]*Schema{
		"vpc_config": {
			Type:     TypeList,
			Optional: true,
			Elem: &Resource{
				Schema: map[string]*Schema{
					"subnet_ids": {
						Type:     TypeSet,
						Required: true,
						Elem:     &Schema{Type: TypeString},
						Set:      HashString,
					},
				},
			},
		},
	},

	State: &terraform.InstanceState{
		Attributes: map[string]string{
			"vpc_config.#":              "1",
			"vpc_config.0.subnet_ids.#": "1",
			fmt.Sprintf("vpc_config.0.subnet_ids.%d", HashString("abc-123")): "abc-123",
		},
	},

	Diff: &terraform.InstanceDiff{
		Attributes: map[string]*terraform.ResourceAttrDiff{
			"tags.foo": {
				Old: "",
				New: "bar",
			},
		},
	},

	Key: "vpc_config",

	Change: false,
},

This test case should indicate a change, and currently does:

{
	Schema: map[string]*Schema{
		"vpc_config": {
			Type:     TypeList,
			Optional: true,
			Elem: &Resource{
				Schema: map[string]*Schema{
					"subnet_ids": {
						Type:     TypeSet,
						Required: true,
						Elem:     &Schema{Type: TypeString},
						Set:      HashString,
					},
				},
			},
		},
	},

	State: &terraform.InstanceState{
		Attributes: map[string]string{
			"vpc_config.#":              "1",
			"vpc_config.0.subnet_ids.#": "1",
			fmt.Sprintf("vpc_config.0.subnet_ids.%d", HashString("abc-123")): "abc-123",
		},
	},

	Diff: &terraform.InstanceDiff{
		Attributes: map[string]*terraform.ResourceAttrDiff{
			"vpc_config.0.subnet_ids.#": {
				Old: "1",
				New: "0",
			},
		},
	},

	Key: "vpc_config",

	Change: true,
},

@paddycarver paddycarver added the subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it. label Jan 6, 2021
@IvDoorn
Copy link

IvDoorn commented Feb 11, 2021

Hi, there is a PR related to this issue opened in November. Any chance that this PR can be looked at by the maintainers?
This issue is high impacting users of the terraform-provider-aws plugin, which either prevents people from upgrading the plugin or if they do upgrade, the output of terraform plan will become too cluttered with non-actionable changes that action important changes are overlooked.

gdavison pushed a commit to hashicorp/terraform-provider-aws that referenced this issue Jun 4, 2021
As reported in #17385
a change i  detected in vpc_config when there are no changes, this is
caused by an issue in hashicorp/terraform-plugin-sdk#617
The PR to provide a fix is not getting any traction.

On further debuging the issue is caused by the nested elements being of
type set which needs the use of Equal rather than reflect.DeepEqual to
test for differences.

We can work around this bug by testing for changes in the two fields
within vpc_config independantly as when the item passed to HasChanges is
a Set it is tested correctly.
@bflad bflad added this to the v2.8.1 milestone Oct 14, 2021
@bflad
Copy link
Contributor

bflad commented Oct 14, 2021

The fix for this (#711) has been merged and will release with the next Terraform Plugin SDK version. 👍

@bflad bflad closed this as completed Oct 14, 2021
@bflad bflad modified the milestones: v2.8.1, v2.9.0 Oct 14, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants