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

Reject or warn about additional attributes during object type conversion #33570

Open
asaba-hashi opened this issue Jul 24, 2023 · 6 comments
Open

Comments

@asaba-hashi
Copy link

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

  1. Include the above model and schema in a provider.
  2. 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: hashicorp/terraform-plugin-framework#805 (comment)

References

There appear to be a number of related issues already open:

@asaba-hashi asaba-hashi added bug new new issue not yet triaged labels Jul 24, 2023
@apparentlymart
Copy link
Contributor

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.

@apparentlymart apparentlymart added enhancement config and removed bug new new issue not yet triaged labels Jul 25, 2023
@apparentlymart apparentlymart changed the title Extra keys SingleNestedAttribute do not fail terraform validate or warn Reject or warn about additional attributes during object type conversion Jul 25, 2023
@asaba-hashi
Copy link
Author

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.

@bflad
Copy link
Contributor

bflad commented Jul 27, 2023

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.

@apparentlymart
Copy link
Contributor

apparentlymart commented Jul 27, 2023

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.

@adriansuarez
Copy link

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?

@shufanhao

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants