Unset dynamic blocks appearing in validations

[morgante]

Terraform Version

Terraform v0.12.17

Terraform Configuration Files

resource "google_compute_firewall" "compute_firewall_rule" {
  name                      = each.key
  description               = each.value.description

  project                   = var.project
  network                   = var.network

  disabled                  = var.disabled
  priority                  = each.value.priority
  direction                 = each.value.direction

  source_ranges             = each.value.direction == "INGRESS" ? each.value.ip_ranges : null
  destination_ranges        = each.value.direction == "EGRESS" ?  each.value.ip_ranges : null
  source_tags               = each.value.direction == "INGRESS" ? each.value.source_tags : null
  target_tags               = each.value.target_tags

  dynamic "allow" {
    for_each = each.value.allow
    content {
      protocol                = allow.value.protocol
      ports                   = allow.value.ports
    }
  }

  dynamic "deny" {
    for_each = []
    content {
      protocol                = deny.value.protocol
      ports                   = deny.value.port
    }
  }
}

Expected Behavior

When dynamic blocks are used, validations should be run against the actual produced blocks.

Actual Behavior

As reported on the Google provider, all dynamic blocks seem to appear at validation time - causing ExactlyOneOf validations to fail, even if at apply-time only one block is actually populated.

The specific error which is raised looks like this:

Error: "allow": only one of `allow,deny` can be specified, but `allow,deny` were specified.

  on modules/gcp/network/firewall/main.tf line 1, in resource "google_compute_firewall" "compute_firewall_rule":
   1: resource "google_compute_firewall" "compute_firewall_rule" {

References

• Version v3.0.0-beta.1 breaks when using alternative dynamic blocks terraform-provider-google#5024

[radeksimko]

Hi @morgante
Thank you for the report and for attaching the config. I managed to reduce it a bit and still reproduce the bug: https://gist.github.com/radeksimko/b0aedba67cdd5039d2c59ef0c7547100

My thought process here is:

1. dynamic is a feature of Terraform Core, not the SDK. Providers get a relevant part of parsed config after any dynamics have already been interpolated, in cty format. That said dynamic provides a way to express something that we may not be able to express through other means. i.e. I can't think of a way to reproduce this without dynamic. The way core produces cty values is by combining parsed HCL + schema, so we're working within the limitations of the two.
2. One detail which first caught my eye is that we're working with sets here and you're passing empty set. Because 0.11 and previous versions had no way of distinguishing between empty ([] in case of a set) and undefined value (null) and the SDK still retains backward compatibility with 0.11 for now, this may play a role too, but that doesn't seem to be the main root cause here actually. I was able to still reproduce the same bug with deny = null.
3. Before ExactlyOneOf validation logic gets any chance to actually validate the config, the 0.12-style values (cty) need to be converted back to 0.11-style ones (flatmap), so this is another place which I probed to see if any bug is there, once I realized that the validation code receives []interface {}{map[string]interface {}{"ports":"74D93920-ED26-11E3-AC10-0800200C9A66", "protocol":"74D93920-ED26-11E3-AC10-0800200C9A66"}} for both allow and deny fields. The weird UUID here is just a representation of an unknown value in 0.11. All values should be known in the config above (after dynamic interpolates), so this is first indication of something going wrong here.
4. To rule out bug in the 0.11/0.12 shims, I also checked what's the original 0.12-style raw value coming from core. This confirmed that unknown values appear to be there, so I think we need to move debugging efforts to core. Below is a snippet of what is received from core.

cty.ObjectVal(map[string]cty.Value{
	"allow": cty.SetVal([]cty.Value{
		cty.ObjectVal(map[string]cty.Value{
			"ports":    cty.UnknownVal(cty.List(cty.String)),
			"protocol": cty.UnknownVal(cty.String),
		}),
	}),
	"creation_timestamp": cty.NullVal(cty.String),
	"deny":cty.SetVal([]cty.Value{
		cty.ObjectVal(map[string]cty.Value{
			"ports":    cty.UnknownVal(cty.List(cty.String)),
			"protocol": cty.UnknownVal(cty.String),
		}),
	}),
...
)})

---

cc @hashicorp/terraform-core TLDR; I suppose we need to look into why dynamic in this particular case produces unknown values and why it's sending the whole object structure, when it should be just null.

[jbardin]

The values you're getting here are what I would expect from a dynamic block. I don't recognize this particular validation, so it may be encountering a condition I had not anticipated, but the other validations do also have to take unknown values into account.

A block cannot be directly "assigned" or "unassigned" at all (which is why ConfigModeAttr was added), so if it exists in the config it needs to be represented in the resource value in some way. This also means we need some way to represent a dynamic block that cannot be evaluated yet, in which case we chose to use a block containing unknown values.

I haven't looked closely yet at the validation code, but there's probably a check missing for "all unknown" values, in which case you cannot know if it will be set, or how many values it will contain. You also need to take into account that blocks may be set multiple times, in which case you will want to see what combinations of static and dynamic blocks will produce through the shims. IIRC most of the more complex validations abort if there are any unkowns, as it's hard to predict what the final shape of he value will be.

[radeksimko]

As @jbardin kindly pointed out to me in a separate conversation we actually run validation twice - for the first time before interpolation and then after. Hence the first validation receives unknowns and fails and we never do the second one due to that failure.

I will check how does ExactlyOneOf logic account for unknown values - I think it may not account for unknowns in complex structures, such as sets here. That alone may not resolve the actual bug since we'll just short-circuit the early validation, but gets us closer as we'll be able to run the validation for the second time with real data.

[radeksimko]

@morgante I submitted a fix in #287

[radeksimko]

Also I just verified that the example above is plannable/appliable with a custom build of google provider 3.0.0-beta.1 with that patch from #287 and it no longer raises validation errors. The latest stable version doesn't raise any errors either, but that's expectable as the affected validation was removed/replaced.

@danawillow hashicorp/terraform-provider-google#5193 should be unblocked once we merge it & release a patched version of the SDK and Google provider upgrades to that version.

[morgante]

@radeksimko Awesome, thanks for the fix!

[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 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.