hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.4k stars 9.5k forks source link

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

Open asaba-hashi opened 1 year ago

asaba-hashi commented 1 year ago

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: https://github.com/hashicorp/terraform-plugin-framework/issues/805#issuecomment-1648521484

References

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

apparentlymart commented 1 year ago

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:

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.

asaba-hashi commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 4 months ago

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 commented 3 months ago

Also hit the same issue.