terraform
terraform copied to clipboard
hashicorp
Reject or warn about additional attributes during object type conversion
Terraform Version
1.5.1
Terraform Configuration Files
type Things struct {
A types.Bool `tfsdk:"A"`
B types.Bool `tfsdk:"B"`
}
type SomeResource struct {
Things things
}
<snip>
"things": schema.SingleNestedAttribute{
Attributes: map[string]schema.Attribute{
"B": schema.BoolAttribute{
Default: booldefault.StaticBool(false),
Optional: true,
Computed: true,
},
"B": schema.BoolAttribute{
Default: booldefault.StaticBool(false),
Optional: true,
Computed: true,
},
}
required: true,
}
resource "some_resource" "some_resource_name" {
things = {
A = true
should_fail_to_validate = "but doesn't"
}
}
Debug Output
n/a
Expected Behavior
terraform validate should fail with a message that an unknown key is present.
Actual Behavior
terraform validate/plan/apply all ran succesfully without errors or warning.
Steps to Reproduce
- Include the above model and schema in a provider.
terraform validate.
Additional Context
It was suggested by @bflad to open an issue in this tracker instead, including some notes about what is being passed: https://github.com/hashicorp/terraform-plugin-framework/issues/805#issuecomment-1648521484
References
There appear to be a number of related issues already open:
- #28696
- #28727
- #29204
- #32015
Hi @asaba-hashi,
In the Terraform language an object type constraint accepts any value that has at least the attributes specified. Any additional attributes are discarded during type conversion.
Therefore this seems like Terraform working as designed, similar to what would happen if a module author declared an input variable of an object type. This is a design tradeoff of the type system that is intended to allow the producer and consumer of an argument to evolve independently:
- the consumer can accept more in later releases by adding new optional attributes
- the producer can provide more in later releases as long as it preserves all of the attributes that were previously provided
Although the ability to "provide more" is not important when the object is written out literally using an object constructor as you showed here, it is important to reduce coupling when the entire object is being provided from further away, such as in another module or as an output attribute of another resource type.
If you would prefer a more rigid interpretation then I think you will need to declare a nested block type called thing. Nested blocks can be more rigid because all of the arguments must be written out literally; it's not syntactically possible to just assign an entire object value in that case, so the author of the block decides explicitly which arguments to include and thus Terraform can safely reject unexpected arguments without it causing problems when the source object grows to include new attributes later.
If this is a request to change Terraform's type system to reject attributes that are not declared rather than just discarding them during conversion then unfortunately I don't see a path to get there within the Terraform v1.x Compatibility Promises, even if we were to decide that the compatibility considerations I described above were not important.
If you would prefer a more rigid interpretation then I think you will need to declare a nested block type called
thing.
...unfortunately I don't see a path to get there within the Terraform v1.x Compatibility Promises...
Thanks for the thorough explanation!
As a new framework based provider, I only chose a nested attributes because of the following note in the docs, so I've opened up a doc issue for the framework in the meantime:
Use nested attributes for new schema implementations. Block support is mainly for migrating prior SDK-based providers.
It sounds like there is some potential conflation of concerns because any particular schema definition recommendations will cause provider developers to try to choose an appropriate practitioner experience for them. When is it more "correct" for a provider developer to choose blocks over attributes? Nested blocks, while having the strict attribute name validation, are inherently more difficult to work with as a practitioner since they require explicit per attribute re-mapping and potential usage of dynamic block expressions. My understanding of the intention of adding nested attributes into schema definitions was to simplify the practitioner experience in this regard, where the behaviors mentioned by @apparentlymart are actually a benefit in many cases.
Indeed, unfortunately it is the fact that blocks are "more difficult to work with" that makes it reasonable to do the stricter validation we've historically done for them: the arguments inside a block must always be written out individually and explicitly, which means we know that any extraneous arguments were put there directly by the author of the block. In return for the more onerous requirements at the definition site we get more information about author intent and so can justify stricter error checking.
On the other hand, arguments of an object type add some more flexibility because now it's possible to dynamically generate an entire object, or who wholesale assign an object from one resource to another, and various other situations where the exact set of attributes is not explicitly specified in the source code immediately at the assignment site. That means we take this different approach so that the source of the object and the location that it's assigned to will not be so tightly coupled. Without this compromise, it would be a potential breaking change to add an attribute to any existing module output value or resource type schema, which would make it challenging to evolve providers and modules to meet new requirements. (The idea here is similar to how in JSON-based API design it's common to ask the recipient of a JSON document to ignore properties they don't recognize, rather than immediately returning an error, so that there is a clear path to backward-compatible API evolution.)
These two design approaches have different tradeoffs, and so I don't think it's really possible to rule that one is unambiguously "better" than the other. Instead, like most API design decisions, one needs to understand the benefits and consequences of each approach and decide which one best suits the problem. I think it's fair to say that the value-based approach offers more flexibility and is therefore probably a reasonable default choice where the answer is unclear, but I wouldn't assert that it's universally better to use that over blocks.
For example, in my old experimental testing provider I decided to use nested blocks to describe each assertion because there is little reason to be generating a set of test assertions dynamically, and the block structure allows Terraform to give better feedback for incorrect usage in return for the relative rigidity. But I would not claim that tradeoff as universal; for most real infrastructure objects (as opposed to utility providers) it seems more important to be flexible in allowing dynamic definitions, even though that generates slightly less helpful feedback for incorrect usage.
I'm confused about the reasoning for allowing unknown nested attributes. Is it to enable forward compatibility with configs that target newer versions of the provider?
If that is the case, then why are resources with unknown non-nested attributes rejected?
I stumbled on this issue, trying to figure out why a misspelled variable didn't get reported by TF.
Just as a side note, tried to add a validation to check that all the objects are valid. (Did not work, seems the validation only has access to the expected objects and ignores the rest so no way to validate ??)
e.g. this validation is happy with unknown object's
validation {
condition = alltrue(
[for k, conf in var.config : alltrue(
[for k2, conf2 in conf : contains(["listener_urls", "rule_priority", "backend_http_settings"], k2)]
)]
)
error_message = "ERR: appgw config: only listener_urls, rule_priority, backend_http_settings are allowed."
}
Just as a side note, tried to add a validation to check that all the objects are valid. (Did not work, seems the validation only has access to the expected objects and ignores the rest so no way to validate ??)
Yes, that is what makes this issue so frustrating. The deserialization does not even collect the unknown/unexpected attributes into an opaque form so that validation can be done manually.
I haven't read these extremely long comments that are trying to justify the current behavior, but I suspect it is some kind of fundamental problem with the way they do deserialization that is preventing them from fixing this.
Would it not be possible to allow the user to determine if direct inputs with nested objects are strictly validated? Just allow specifying a configuration such as treat_unrecognized_nested_object_attributes_as_errors (with better naming than I have provided, of course :))?
It's tedious to maintain the type signature in two places, and the error messages are a lot less helpful than they would be if this were a first-class language feature (it's left to the user to figure out which keys aren't matching) but this kind of validation is possible for module variables using a custom validation condition that uses setsubtract to subtract known keys and see if the remaining set of object keys has a non-zero length.
Here's an example from one of our modules:
validation {
error_message = <<-DOCS
Firewall rule specs cannot contain unknown fields.
DOCS
condition = alltrue([
for _, spec in var.firewall_rules : 0 == length(setsubtract(keys(spec), [
"description",
"priority",
"source_subnets",
"source_ranges",
"source_tags",
"source_service_accounts",
"source_all",
"target_tags",
"target_service_accounts",
"allow_tcp",
"allow_udp",
"allow_icmp"
]))
])
}
Would be great to have this as a builtin feature of the language.
I understand the concept of structural typing and the virtues of it, and I think Terraform can/should continue to have structural typing as it currently does, but add additional validation of object literals, like TypeScript has.
In Typescript:
- subtype checks uses structural typing, and the subtype is allowed to have extra fields while still passing the check
- object literals are subject to an additional check, which compares the set of object literal keys to the set of expected keys
In other words, Terraform should be doing structural checking on the argument receiver side, but doing known/unknown object key checking on the sending side. This is based on the insight that objects being passed around as values should be allowed to evolve freely (as described by @apparentlymart above), but an object literal that specifies keys which will be ignored by the receiver is always a mistake and should be corrected by the programmer before being allowed to continue with the plan/apply. Therefore extra validation is needed at the site of each object literal.
Terraform should have made structural typing opt-in on a per-object basis. IOW it should have required rigor by default, and for those situations where we need it (typically passing in a subset of an object to a submodule), one could have specified an additional attribute or function (like nonsensitive()).
But this is moot since doing this now would break too many things. Although a deprecation mechanism could be used (warn users that at some version the feature will be opt-in, this giving people time to convert - the conversion would be trivial and probably expose bugs!).
In another ticket (#37627) I proposed to add a flag to validate that would at least show a warning, as this does not break backwards compatibility.
(I no longer work on Terraform at HashiCorp, so the following is just me posting as an individual and not in the voice of the Terraform team.)
For what it's worth, my earliest versions of what are now the type conversion rules in Terraform (which originated in my side-project go-cty) were stricter about object types, until that work was used in building what became Terraform v0.12. Prior versions of Terraform had no concept of "object types" at all and so existing modules had been using map types to fake it. Passing a map with excess keys to a child module had never caused any sort of error, and so there were a non-trivial amount of existing modules relying on those patterns, doing things like I've previously described about passing an entire "object" (really: map) at once into a variable and expecting that the child module would only use the subset of keys it cared about.
The current behavior was therefore a compromise to keep that working while also encouraging module authors to be explicit about what keys they were expecting by making the unspecified keys unavailable to the child module. It made object types be more "map-like" in their treatment while still getting a subset of the benefits of strict type checking and conversion.
The fact that it was also useful for allowing modules to evolve separately without breaking one another was originally a secondary benefit, but now that many years have passed and module authors are more commonly using object types instead of map types that has now become the primary benefit rather than a secondary one.
If it were up to me (which is very much isn't) I'd compromise here by retroactively adding a warning for when there's an object constructor expression in particular (HCL's syntax for "literal" object values) that specifies an attribute that would definitely be discarded during type conversion, but to still allow assigning object values in other non-object-constructor ways without generating warnings.
My justification for that compromise is that unused additional attributes in an object constructor written directly inline in the input variable assignment cannot possibly be useful in any way: there is no expression anywhere else in the language that could possibly access that attribute, because by the time the value is exposed anywhere that attribute would already have been discarded by type conversion.
However, it is potentially useful to write a single object with a superset of the attributes used in different module calls and have each call discard a different subset of them, and so this compromise still allows that to happen while still giving good feedback in the most common case where the object is written inline.
It also means that users of all existing modules would immediately benefit from the added warning, rather than having to wait for module authors to adopt a new language feature that some probably wouldn't want to adopt until all currently-supported versions of Terraform would accept it. The warning would be triggered by a decision made by the caller of the module, rather than by the author of the module that is being called.
If I were doing this over again I probably would make it an error by default and have the caller opt out of it by using some special syntax, but it's far too late for that now and so I think a warning like this would be a good way to catch the common mistake without removing anything that already works and without complicating the language with yet another variation of object type conversion.
I share this only in the hope that it's useful.
@schollii, I'm kinda in favor of the terraform validate --strict solution at this point. Everything else seems to have stalled out and at least that would honor the compatibility guarantee without having to make code changes.
But at this point I'd kill for ANYTHING. With the validation solutions being no good unless I retype to any I'm kinda at wit's end on what even I begin to do. Plus tflint looked at it and pointed out all sorts of reason them addressing it wouldn't work super well without an API for variables complicated by remote modules.
My hope is that something can get introduced within terraform core to do this. Sadly, the warning options probably doesn't solve my problem as I really really want to have it throw a non-zero exit code so I can bail out and prevent PRs that have this pattern from being mergeable. And the issues for having warnings treated as errors in Terraform are also still open.
My vote at this point... validation flag, so that I can at least get some sort of error code back on the CLI.
This comment from @apparentlymart seems to be in line with my comment above about the behavior of TypeScript for object literals.
If it were up to me (which is very much isn't) I'd compromise here by retroactively adding a warning for when there's an object constructor expression in particular (HCL's syntax for "literal" object values) that specifies an attribute that would definitely be discarded during type conversion, but to still allow assigning object values in other non-object-constructor ways without generating warnings.
Seems to be a good path forward that won't break valid use cases and is tolerant to general cases of schema evolution, while still meeting the core need of this thread.
EDIT: But I'd recommend doing an error instead of a warning for this case, for the same reason @ebachle mentioned:
Sadly, the warning options probably doesn't solve my problem as I really really want to have it throw a non-zero exit code so I can bail out and prevent PRs that have this pattern from being mergeable
We want to catch it in CI before merging a PR.