hashicorp / terraform-plugin-framework

A next-generation framework for building Terraform providers.
https://developer.hashicorp.com/terraform/plugin/framework
Mozilla Public License 2.0
305 stars 93 forks source link

Cannot write state to interface in generic func #1035

Open JacobPotter opened 2 months ago

JacobPotter commented 2 months ago

Module version

github.com/hashicorp/terraform-plugin-framework v1.11.0

Relevant provider source code

SEE ACTUAL BEHAVIOR FOR EXAMPLES

Terraform Configuration Files

...

Debug Output

Expected Behavior

Actual Behavior

We are using an interface for our resource models that stores the state of a provider resource. This interface defines methods for converting data from API to TF and vice versa. For example:

We have an interface here:

type ResourceTransform[M any] interface {
    GetApiModelFromTfModel(context.Context) (M, diag.Diagnostics)
    GetTfModelFromApiModel(context.Context, M) diag.Diagnostics
}
type ResourceTransformWithID[M any] interface {
    GetID() int64
    ResourceTransform[M]
}

Which is implemented by the model below

type DynamicContentVariantModel struct {
    ID       types.Int64  `tfsdk:"id"`
    Content  types.String `tfsdk:"content"`
    LocaleID types.Int64  `tfsdk:"locale_id"`
    Active   types.Bool   `tfsdk:"active"`
    Default  types.Bool   `tfsdk:"default"`
}

type DynamicContentItemResourceModel struct {
    ID              types.Int64                  `tfsdk:"id"`
    Name            types.String                 `tfsdk:"name"`
    Placeholder     types.String                 `tfsdk:"placeholder"`
    DefaultLocaleID types.Int64                  `tfsdk:"default_locale_id"`
    Variants        []DynamicContentVariantModel `tfsdk:"variants"`
}

func (d *DynamicContentItemResourceModel) GetID() int64 {
    ...
}

func (d *DynamicContentItemResourceModel) GetApiModelFromTfModel(_ context.Context) (dci zendesk.DynamicContentItem, diags diag.Diagnostics) {
      ...
}

func (d *DynamicContentItemResourceModel) GetTfModelFromApiModel(_ context.Context, dci zendesk.DynamicContentItem) (diags diag.Diagnostics) {
    ...
}

If I try to use a generic function like this:

func CreateResource[M any](ctx context.Context, request resource.CreateRequest, response *resource.CreateResponse, resourceModel ResourceTransformWithID[M], createFunc func(ctx context.Context, newResource M) (M, error)) {
    response.Diagnostics.Append(request.Plan.Get(ctx, &resourceModel)...)

    if response.Diagnostics.HasError() {
        return
    }
    newResource, diags := resourceModel.GetApiModelFromTfModel(ctx)

    response.Diagnostics.Append(diags...)

    if response.Diagnostics.HasError() {
        return
    }

    resp, err := createFunc(ctx, newResource)

    if err != nil {
        response.Diagnostics.AddError("Error creating resource", fmt.Sprintf("Error: %s", err))
        return
    }

    response.Diagnostics.Append(resourceModel.GetTfModelFromApiModel(ctx, resp)...)

    if response.Diagnostics.HasError() {
        return
    }

    response.Diagnostics.Append(response.State.Set(ctx, resourceModel)...)
}

I get the following error when trying to apply:

 Error: Value Conversion Error

          with zendesk_dynamic_content.test,
        An unexpected error was encountered trying to build a value. This is always
        an error in the provider. Please report the following to the provider
        developer:

        don't know how to reflect tftypes.Object["default_locale_id":tftypes.Number,
        "id":tftypes.Number, "name":tftypes.String, "placeholder":tftypes.String,
        "variants":tftypes.List[tftypes.Object["active":tftypes.Bool,
        "content":tftypes.String, "default":tftypes.Bool, "id":tftypes.Number,
        "locale_id":tftypes.Number]]] into
        models.ResourceTransformWithID[github.com/JacobPotter/go-zendesk/zendesk.DynamicContentItem]

It looks like request.Plan.Get is having an issue writing the state data into the model when using an interface.

Steps to Reproduce

  1. Write terraform plan/state data into model via interface

References

SBGoods commented 2 months ago

Hi @JacobPotter 👋🏾,

Sorry that you're running into trouble here. The (request).Plan.Get() function uses reflection to build the plan into the target type. We intentionally don't support interfaces as a target type, so this would be a new feature request rather than a bug. To set expectations, I'm not entirely sure if we can support interfaces in the reflection logic without digging deeper.

As a workaround, you can try implementing the tftypes.ValueConverter interface on ResourceTransform[M], which should allow the value to be built without using reflection.

JacobPotter commented 1 month ago

@SBGoods Can you show an example of how ValueConverter would be implemented?

EDIT: I am trying to implement it, but I am unsure how the ValueConverter method would interact with the model and what it would return

JacobPotter commented 1 month ago

@SBGoods I have updated my comment above, bumping for visibility.

austinvalle commented 1 month ago

Hey there @JacobPotter, unfortunately there aren't a ton of examples of implementing that interface since most provider implementations just use the reflection as-is.

There is a dynamic resource that Terraform core uses for it's own testing that implements it: https://github.com/hashicorp/terraform-provider-tfcoremock/blob/b8029786d035fdc5d5003511b3299da3a17ef74e/internal/data/resource.go#L67

It also implements the tftypes.ValueCreator interface, which you'll also need if you're attempting to use (tfsdk.State).Set: https://github.com/hashicorp/terraform-provider-tfcoremock/blob/b8029786d035fdc5d5003511b3299da3a17ef74e/internal/data/resource.go#L60

JacobPotter commented 1 month ago

@austinvalle

So I attempted to implement both interfaces, and I am running into an issue with reflect.

Here is my implementation of FromTerraform5Value

func (g *GroupResourceModel) FromTerraform5Value(value tftypes.Value) error {

    // It has to be an object we are converting from.
    if !value.Type().Is(tftypes.Object{}) {
        return errors.New("can only convert between object types")
    }

    values, err := fromTerraform5Value(value)
    if err != nil {
        return err
    }

    // We know these kinds of conversions are safe now, as we checked the type
    // at the beginning.

    if v, ok := values.(map[string]any); ok {
        switch v["id"].(type) {
        case big.Float, *big.Float:
            i, _ := v["id"].(*big.Float).Int64()
            g.ID = types.Int64Value(i)
        case int, int64:
            g.ID = types.Int64Value(v["id"].(int64))
        default:
            return fmt.Errorf("bad id value type: %v", reflect.TypeOf(v))
        }

        g.URL = types.StringValue(v["url"].(string))
        g.Name = types.StringValue(v["name"].(string))
        g.Default = types.BoolValue(v["default"].(bool))
        g.Deleted = types.BoolValue(v["deleted"].(bool))
        g.IsPublic = types.BoolValue(v["is_public"].(bool))
        g.Description = types.StringValue(v["description"].(string))
        g.CreatedAt = types.StringValue(v["created_at"].(string))
        g.UpdatedAt = types.StringValue(v["updated_at"].(string))

    } else {
        return errors.New("can only convert between object types")
    }

    return nil
}

When trying to run, am getting a panic:


goroutine 158 [running]:
reflect.Value.Method({0x10107a1a0, 0x1018d82a0, 0x94}, 0x0)
    /Users/jacob.potter/sdk/go1.22.5/src/reflect/value.go:2082 +0x19c
reflect.Value.MethodByName({0x10107a1a0, 0x1018d82a0, 0x94}, {0x100de841a, 0x13})
    /Users/jacob.potter/sdk/go1.22.5/src/reflect/value.go:2121 +0x174
github.com/hashicorp/terraform-plugin-framework/internal/reflect.NewValueConverter({0x10119dd90, 0x140006a4390}, {0x10119fed0, 0x140006a5ef0}, {{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}, {0x10107a1a0, 0x140006a6930, ...}, ...)
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-framework@v1.12.0/internal/reflect/interfaces.go:296 +0xac
github.com/hashicorp/terraform-plugin-framework/internal/reflect.BuildValue({0x10119dd90, 0x140006a4390}, {0x10119fed0, 0x140006a5ef0}, {{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}, {0x10107a1a0, 0x140006a6930, ...}, ...)
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-framework@v1.12.0/internal/reflect/into.go:77 +0x2d0
github.com/hashicorp/terraform-plugin-framework/internal/reflect.Into({0x10119dd90, 0x140006a4390}, {0x10119fed0, 0x140006a5ef0}, {{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}, {0x100fb5760, 0x140006a6930}, ...)
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-framework@v1.12.0/internal/reflect/into.go:42 +0x358
github.com/hashicorp/terraform-plugin-framework/internal/fwschemadata.Data.Get({{0x100dd92e1, 0x4}, {0x1011a4608, 0x14000032d20}, {{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}}, {0x10119dd90, 0x140006a4390}, ...)
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-framework@v1.12.0/internal/fwschemadata/data_get.go:16 +0xc8
github.com/hashicorp/terraform-plugin-framework/tfsdk.Plan.Get({{{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}, {0x1011a4608, 0x14000032d20}}, {0x10119dd90, 0x140006a4390}, {0x100fb5760, 0x140006a6930})
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-framework@v1.12.0/tfsdk/plan.go:24 +0xc4
github.com/Dynatrace/terraform-provider-zendesk/internal/provider/models.CreateResource[...]({0x10119dd90, 0x140006a4390}, {{{{0x1011a28a0, 0x140006a4d80}, {0x101023740, 0x140006a4c90}}, {0x1011a4608, 0x14000032d20}}, {{{0x1011a28a0, 0x140006a5620}, ...}, ...}, ...}, ...)
    /Users/jacob.potter/Bitbucket/terraform-provider-zendesk/internal/provider/models/shared.go:30 +0xd0
github.com/Dynatrace/terraform-provider-zendesk/internal/provider.(*GroupResource).Create(0x140004ac0f8, {0x10119dd90, 0x140006a4390}, {{{{0x1011a28a0, 0x140006a4d80}, {0x101023740, 0x140006a4c90}}, {0x1011a4608, 0x14000032d20}}, {{{0x1011a28a0, ...}, ...}, ...}, ...}, ...)
    /Users/jacob.potter/Bitbucket/terraform-provider-zendesk/internal/provider/group_resource.go:53 +0x128
github.com/hashicorp/terraform-plugin-framework/internal/fwserver.(*Server).CreateResource(0x140004bc000, {0x10119dd90, 0x140006a4390}, 0x140006c4760, 0x140006c4730)
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-framework@v1.12.0/internal/fwserver/server_createresource.go:101 +0x514
github.com/hashicorp/terraform-plugin-framework/internal/fwserver.(*Server).ApplyResourceChange(0x140004bc000, {0x10119dd90, 0x140006a4390}, 0x14000622910, 0x140006c4980)
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-framework@v1.12.0/internal/fwserver/server_applyresourcechange.go:57 +0x2d8
github.com/hashicorp/terraform-plugin-framework/internal/proto6server.(*Server).ApplyResourceChange(0x140004bc000, {0x10119dd90, 0x140006a4390}, 0x14000622870)
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-framework@v1.12.0/internal/proto6server/server_applyresourcechange.go:55 +0x538
github.com/hashicorp/terraform-plugin-go/tfprotov6/tf6server.(*server).ApplyResourceChange(0x140004da280, {0x10119dd90, 0x140006a4240}, 0x140004d23f0)
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-go@v0.24.0/tfprotov6/tf6server/server.go:865 +0x3cc
github.com/hashicorp/terraform-plugin-go/tfprotov6/internal/tfplugin6._Provider_ApplyResourceChange_Handler({0x10115ca40, 0x140004da280}, {0x10119dd90, 0x14000323530}, 0x14000511280, 0x0)
    /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/terraform-plugin-go@v0.24.0/tfprotov6/internal/tfplugin6/tfplugin6_grpc.pb.go:545 +0x2a4
google.golang.org/grpc.(*Server).processUnaryRPC(0x140000f6400, {0x10119dd90, 0x140003234a0}, {0x1011a3440, 0x140001371e0}, 0x14000316fc0, 0x14000322d50, 0x10185cdd8, 0x0)
    /Users/jacob.potter/go/pkg/mod/google.golang.org/grpc@v1.66.2/server.go:1394 +0x1480
google.golang.org/grpc.(*Server).handleStream(0x140000f6400, {0x1011a3440, 0x140001371e0}, 0x14000316fc0)
    /Users/jacob.potter/go/pkg/mod/google.golang.org/grpc@v1.66.2/server.go:1805 +0xd0c
google.golang.org/grpc.(*Server).serveStreams.func2.1()
    /Users/jacob.potter/go/pkg/mod/google.golang.org/grpc@v1.66.2/server.go:1029 +0x144
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 184
    /Users/jacob.potter/go/pkg/mod/google.golang.org/grpc@v1.66.2/server.go:1040 +0x1c8

Looking at a debugger when this happens, it seems to be failing when trying to create a NewValueConverter: image

So my implementation of FromTerraform5Value does not even appear to be run; it is getting caught up in making sure it exists I think.

austinvalle commented 1 month ago

Hmm, very interesting! That looks like there's a potential missing case in our reflection utility before trying to retrieve that method. A lot of the reflection logic was written pre-generics, so it wouldn't be surprising 🤔. I'll try to recreate that just in the framework logic and report back.

JacobPotter commented 1 month ago

Yeah, it would be nice if there was cleaner support for generics in general. Needing to implement ValueCreator and ValueConverter almost brings back the overhead that was reduced by using generics in the first place. I'm just trying to avoid the repetitive code that we currently have in our provider.

austinvalle commented 1 month ago

Just spent some time looking at your examples and I think I may have skipped by a detail in there:

func CreateResource[M any](ctx context.Context, request resource.CreateRequest, response *resource.CreateResponse, resourceModel ResourceTransformWithID[M], createFunc func(ctx context.Context, newResource M) (M, error)) {
    response.Diagnostics.Append(request.Plan.Get(ctx, &resourceModel)...)

In your provider, for parameter resourceModel, I'm assuming this is populated by a struct pointer? Like of type *GroupResourceModel?

If that's the case I think you'd want to pass that in directly, rather than passing a pointer of resourceModel which I believe would be the interface (ResourceTransformWithID[M]) with a nil value?

func CreateResource[M any](ctx context.Context, request resource.CreateRequest, response *resource.CreateResponse, resourceModel ResourceTransformWithID[M], createFunc func(ctx context.Context, newResource M) (M, error)) {
    // resourceModel is already a pointer to a struct
    response.Diagnostics.Append(request.Plan.Get(ctx, resourceModel)...)

If you're able to do that, I don't believe you'll need to implement ValueConverter because you're passing our reflection logic a pointer to the struct which has the correct tfsdk tags on it.


Side note: I notice your provider is private, but are you able to share some of the code that instantiates/calls your CreateResource[M any] method? That'd make it a little easier for me to help debug further and make sure we're on the same page.

austinvalle commented 1 month ago

Not related to my comment above, but semi-related to the overall conversation

While attempting to recreate the behavior, I think I stumbled across a different bug, top-level structs that implement tftypes.ValueConverter with pointer receiver methods aren't being detected properly because our reflection logic grabs the element of the pointer during the Into function, which is our entry point underneath request.Plan.Get. If you implement tftypes.ValueConverter with value receiver methods then it'd work fine, although that's not how they are typically implemented 🙃

Fields on a struct that implement tftypes. ValueConverter would still work however, which seems to be the majority of test cases, so likely just an oversight we could fix with some refactoring.


With that aside, I'm still thinking we should be able to resolve your problem by passing in the variable that contains the pointer struct (resourceModel => *GroupResourceModel) rather than the pointer of the generic interface (&resourceModel => ResourceTransformWithID[M](nil))

JacobPotter commented 2 weeks ago

@austinvalle Sorry for the late response, but this exercise's whole point was to create some generic functions to handle different resource models. If we pass in the actual struct instead of the interface, doesn't that leave me where I was in the first place, where I have repeated logic for different models?

austinvalle commented 2 weeks ago

I might just be doing a poor job of explaining what I mean 🤔

My comment above was suggesting to just modify your generic CreateResource[M any] function to utilize the value coming from resourceModel directly, which should already be a pointer to a struct (in my example below, *DynamicContentItemResourceModel). If you take the address of &resourceModel, it will always be a nil interface of ResourceTransformWithID[M], which the framework reflection logic cannot create because it doesn't know what struct value to create.


I wrote a quick compilable playground example to try and illustrate what I'm talking about: https://go.dev/play/p/ehnEgBpxEWk

You may run into a build timeout running this in the Go playground, but you can always just copy it locally into a go file and run it. Output should show all the data flowing through properly:

New State will be: tftypes.Object["id":tftypes.Number, "name":tftypes.String]<"id":tftypes.Number<"1234">, "name":tftypes.String<"test-name">>

Notice in that example, if you uncomment line 87, you'll get the same error you're referencing:

panic: [{{An unexpected error was encountered trying to build a value. This is always an error in the provider. Please report the following to the provider developer:

    don't know how to reflect tftypes.Object["id":tftypes.Number, "name":tftypes.String] into main.ResourceTransformWithID[main.ZenDeskDynamicContentItem] Value Conversion Error} {[]}}]

goroutine 1 [running]:
main.main()
    /tmp/sandbox2824779807/prog.go:75 +0x874

Here is the entire example in-case that link ever dies ```go package main import ( "context" "fmt" "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/tfsdk" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-go/tftypes" ) func main() { // ----------------- // All of this logic is just here to simulate a request/response that comes from terraform-plugin-framework. // This is just simulating creating a plan with an unknown id and a configured "test-name". // ----------------- rawValue := tftypes.NewValue( tftypes.Object{ AttributeTypes: map[string]tftypes.Type{ "id": tftypes.Number, "name": tftypes.String, }, }, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.Number, tftypes.UnknownValue), "name": tftypes.NewValue(tftypes.String, "test-name"), }, ) fwSchema := schema.Schema{ Attributes: map[string]schema.Attribute{ "id": schema.Int64Attribute{ Computed: true, }, "name": schema.StringAttribute{ Required: true, }, }, } fwReq := resource.CreateRequest{ Config: tfsdk.Config{ Raw: rawValue, Schema: fwSchema, }, Plan: tfsdk.Plan{ Raw: rawValue, Schema: fwSchema, }, } fwResp := resource.CreateResponse{ State: tfsdk.State{ Raw: rawValue, Schema: fwSchema, }, } // ----------------- // Creating a pointer to a struct, this struct satisfies the ResourceTransformWithID[M] interface and can be reflected on by Plan.Get dynamicContentItem := &DynamicContentItemResourceModel{} CreateResource(context.Background(), fwReq, &fwResp, dynamicContentItem, func(ctx context.Context, newResource ZenDeskDynamicContentItem) (ZenDeskDynamicContentItem, error) { // Simulating an API call return ZenDeskDynamicContentItem{ ID: 1234, Name: "test-name", }, nil }) // If we get an error just print it and exit if fwResp.Diagnostics.HasError() { panic(fmt.Sprintf("%s", fwResp.Diagnostics)) } // If no error, print what the new state will be (populated by the createFunc) fmt.Printf("New State will be: %s", fwResp.State.Raw) } func CreateResource[M any](ctx context.Context, request resource.CreateRequest, response *resource.CreateResponse, resourceModel ResourceTransformWithID[M], createFunc func(ctx context.Context, newResource M) (M, error)) { // This was the original line from: https://github.com/hashicorp/terraform-plugin-framework/issues/1035 // Plan.Get will eventually grab the type of => ResourceTransformWithID[M], which is an interface and cannot be created. // ---- Uncomment the line below to see the original error message ----- // // response.Diagnostics.Append(request.Plan.Get(ctx, &resourceModel)...) // This is the suggested line from: https://github.com/hashicorp/terraform-plugin-framework/issues/1035#issuecomment-2403459626 // Plan.Get will eventually grab the type of => DynamicContentItem, which is a struct that can be created. // response.Diagnostics.Append(request.Plan.Get(ctx, resourceModel)...) if response.Diagnostics.HasError() { return } newResource, diags := resourceModel.GetApiModelFromTfModel(ctx) response.Diagnostics.Append(diags...) if response.Diagnostics.HasError() { return } resp, err := createFunc(ctx, newResource) if err != nil { response.Diagnostics.AddError("Error creating resource", fmt.Sprintf("Error: %s", err)) return } response.Diagnostics.Append(resourceModel.GetTfModelFromApiModel(ctx, resp)...) if response.Diagnostics.HasError() { return } response.Diagnostics.Append(response.State.Set(ctx, resourceModel)...) } type ResourceTransformWithID[M any] interface { GetID() int64 ResourceTransform[M] } type ResourceTransform[M any] interface { GetApiModelFromTfModel(context.Context) (M, diag.Diagnostics) GetTfModelFromApiModel(context.Context, M) diag.Diagnostics } type DynamicContentItemResourceModel struct { ID types.Int64 `tfsdk:"id"` Name types.String `tfsdk:"name"` } // => zendesk.DynamicContentItem type ZenDeskDynamicContentItem struct { ID int64 Name string } func (d *DynamicContentItemResourceModel) GetID() int64 { return d.ID.ValueInt64() } func (d *DynamicContentItemResourceModel) GetApiModelFromTfModel(_ context.Context) (dci ZenDeskDynamicContentItem, diags diag.Diagnostics) { return ZenDeskDynamicContentItem{ ID: d.ID.ValueInt64(), Name: d.Name.ValueString(), }, nil } func (d *DynamicContentItemResourceModel) GetTfModelFromApiModel(_ context.Context, dci ZenDeskDynamicContentItem) (diags diag.Diagnostics) { d.ID = types.Int64Value(dci.ID) d.Name = types.StringValue(dci.Name) return nil } ```