From bdcac8792dfc324f26e1b3246862d303e3e1ec59 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 1 Feb 2019 14:58:52 -0800 Subject: [PATCH] plugin: Use correct schema when marshaling imported resource objects Previously we were using the type name requested in the import to select the schema, but a provider is free to return additional objects of other types as part of an import result, and so it's important that we perform schema selection separately for each returned object. If we don't do this, we get confusing downstream errors where the resulting object decodes to the wrong type and breaks various invariants expected by Terraform Core. The testResourceImportOther test in the test provider didn't catch this previously because it happened to have an identical schema to the other resource type being imported. Now the schema is changed and also there's a computed attribute we can set as part of the refresh phase to make sure we're completing the Read call properly during import. Refresh was working correctly, but we didn't have any tests for it as part of the import flow. --- .../providers/test/resource_import_other.go | 27 +++++++------------ .../test/resource_import_other_test.go | 2 ++ helper/plugin/grpc_provider.go | 13 +++++---- plugin/grpc_provider.go | 3 +-- 4 files changed, 18 insertions(+), 27 deletions(-) diff --git a/builtin/providers/test/resource_import_other.go b/builtin/providers/test/resource_import_other.go index 7289cc81e..4ee452df7 100644 --- a/builtin/providers/test/resource_import_other.go +++ b/builtin/providers/test/resource_import_other.go @@ -1,6 +1,8 @@ package test import ( + "fmt" + "github.com/hashicorp/terraform/helper/schema" ) @@ -26,23 +28,9 @@ func testResourceImportOther() *schema.Resource { Optional: true, Default: true, }, - "nested": { - Type: schema.TypeSet, - Optional: true, - ForceNew: true, - Elem: &schema.Resource{ - Schema: map[string]*schema.Schema{ - "string": { - Type: schema.TypeString, - Optional: true, - Default: "default nested", - }, - "optional": { - Type: schema.TypeString, - Optional: true, - }, - }, - }, + "computed": { + Type: schema.TypeString, + Computed: true, }, }, } @@ -74,10 +62,13 @@ func testResourceImportOtherUpdate(d *schema.ResourceData, meta interface{}) err } func testResourceImportOtherRead(d *schema.ResourceData, meta interface{}) error { + err := d.Set("computed", "hello!") + if err != nil { + return fmt.Errorf("failed to set 'computed' attribute: %s", err) + } return nil } func testResourceImportOtherDelete(d *schema.ResourceData, meta interface{}) error { - d.SetId("") return nil } diff --git a/builtin/providers/test/resource_import_other_test.go b/builtin/providers/test/resource_import_other_test.go index a892ed71e..1965d9e66 100644 --- a/builtin/providers/test/resource_import_other_test.go +++ b/builtin/providers/test/resource_import_other_test.go @@ -38,6 +38,8 @@ resource "test_resource_import_other" "foo" { return fmt.Errorf("no instance with id import_other_main in state") } else if got, want := is.Ephemeral.Type, "test_resource_import_other"; got != want { return fmt.Errorf("import_other_main has wrong type %q; want %q", got, want) + } else if got, want := is.Attributes["computed"], "hello!"; got != want { + return fmt.Errorf("import_other_main has wrong value %q for its computed attribute; want %q", got, want) } if is, ok := byID["import_other_other"]; !ok { return fmt.Errorf("no instance with id import_other_other in state") diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index da048489c..5b58f1dd1 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -794,8 +794,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A func (s *GRPCProviderServer) ImportResourceState(_ context.Context, req *proto.ImportResourceState_Request) (*proto.ImportResourceState_Response, error) { resp := &proto.ImportResourceState_Response{} - block := s.getResourceSchemaBlock(req.TypeName) - info := &terraform.InstanceInfo{ Type: req.TypeName, } @@ -810,6 +808,12 @@ func (s *GRPCProviderServer) ImportResourceState(_ context.Context, req *proto.I // copy the ID again just to be sure it wasn't missed is.Attributes["id"] = is.ID + resourceType := is.Ephemeral.Type + if resourceType == "" { + resourceType = req.TypeName + } + + block := s.getResourceSchemaBlock(resourceType) newStateVal, err := hcl2shim.HCL2ValueFromFlatmap(is.Attributes, block.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -828,11 +832,6 @@ func (s *GRPCProviderServer) ImportResourceState(_ context.Context, req *proto.I return resp, nil } - resourceType := is.Ephemeral.Type - if resourceType == "" { - resourceType = req.TypeName - } - importedResource := &proto.ImportResourceState_ImportedResource{ TypeName: resourceType, State: &proto.DynamicValue{ diff --git a/plugin/grpc_provider.go b/plugin/grpc_provider.go index 373e07394..c8ca4edbf 100644 --- a/plugin/grpc_provider.go +++ b/plugin/grpc_provider.go @@ -459,8 +459,6 @@ func (p *GRPCProvider) ApplyResourceChange(r providers.ApplyResourceChangeReques func (p *GRPCProvider) ImportResourceState(r providers.ImportResourceStateRequest) (resp providers.ImportResourceStateResponse) { log.Printf("[TRACE] GRPCProvider: ImportResourceState") - resSchema := p.getResourceSchema(r.TypeName) - protoReq := &proto.ImportResourceState_Request{ TypeName: r.TypeName, Id: r.ID, @@ -479,6 +477,7 @@ func (p *GRPCProvider) ImportResourceState(r providers.ImportResourceStateReques Private: imported.Private, } + resSchema := p.getResourceSchema(resource.TypeName) state := cty.NullVal(resSchema.Block.ImpliedType()) if imported.State != nil { state, err = msgpack.Unmarshal(imported.State.Msgpack, resSchema.Block.ImpliedType())