From bcf2aa06dddf463fc3f3a4f18d6ea13a362e65c4 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 10 May 2019 09:25:02 -0700 Subject: [PATCH 1/6] core: Call providers' UpgradeResourceState every time Previously we tried to short-circuit this if the schema version hadn't changed and we were already using JSON serialization. However, if we instead call into UpgradeResourceState every time we can let the provider or the SDK do some general, systematic normalization and cleanup steps without always requiring a schema version increase. What exactly would be fixed up this way is for the SDK to decide, but for example the SDK might choose to automatically delete from the state anything that is no longer present in the schema, avoiding the need to write explicit state migration functions for that common case where the remedy is always the same. The actual update logic is delegated to the provider/SDK intentionally so that it can evolve over time and potentially differ depending on how each SDK thinks about schema. --- terraform/eval_state_upgrade.go | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/terraform/eval_state_upgrade.go b/terraform/eval_state_upgrade.go index b7720d71b..e1940005e 100644 --- a/terraform/eval_state_upgrade.go +++ b/terraform/eval_state_upgrade.go @@ -19,26 +19,13 @@ import ( // If any errors occur during upgrade, error diagnostics are returned. In that // case it is not safe to proceed with using the original state object. func UpgradeResourceState(addr addrs.AbsResourceInstance, provider providers.Interface, src *states.ResourceInstanceObjectSrc, currentSchema *configschema.Block, currentVersion uint64) (*states.ResourceInstanceObjectSrc, tfdiags.Diagnostics) { - currentTy := currentSchema.ImpliedType() - - // If the state is currently in flatmap format and the current schema - // contains DynamicPseudoType attributes then we won't be able to convert - // it to JSON without the provider's help even if the schema version matches, - // since only the provider knows how to interpret the dynamic attribute - // value in flatmap format to convert it to JSON. - schemaHasDynamic := currentTy.HasDynamicTypes() - stateIsFlatmap := len(src.AttrsJSON) == 0 - forceProviderUpgrade := schemaHasDynamic && stateIsFlatmap - - if src.SchemaVersion == currentVersion && !forceProviderUpgrade { - // No upgrading required, then. - return src, nil - } if addr.Resource.Resource.Mode != addrs.ManagedResourceMode { // We only do state upgrading for managed resources. return src, nil } + stateIsFlatmap := len(src.AttrsJSON) == 0 + providerType := addr.Resource.Resource.DefaultProviderConfig().Type if src.SchemaVersion > currentVersion { log.Printf("[TRACE] UpgradeResourceState: can't downgrade state for %s from version %d to %d", addr, src.SchemaVersion, currentVersion) @@ -60,7 +47,11 @@ func UpgradeResourceState(addr addrs.AbsResourceInstance, provider providers.Int // v0.12, this also includes translating from legacy flatmap to new-style // representation, since only the provider has enough information to // understand a flatmap built against an older schema. - log.Printf("[TRACE] UpgradeResourceState: upgrading state for %s from version %d to %d using provider %q", addr, src.SchemaVersion, currentVersion, providerType) + if src.SchemaVersion != currentVersion { + log.Printf("[TRACE] UpgradeResourceState: upgrading state for %s from version %d to %d using provider %q", addr, src.SchemaVersion, currentVersion, providerType) + } else { + log.Printf("[TRACE] UpgradeResourceState: schema version of %s is still %d; calling provider %q for any other minor fixups", addr, currentVersion, providerType) + } req := providers.UpgradeResourceStateRequest{ TypeName: addr.Resource.Resource.Type, From ec65fb960def9df479800f297ee07e916be437c7 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 May 2019 15:43:47 -0400 Subject: [PATCH 2/6] sdk: use core schema for json state upgrade When handling the json state in UpgradeResourceState, the schema must be what core uses, because that is the schema used for encoding/decoding the json state. When converting from flatmap to json state, the legacy schema will be used to decode the flatmap to a cty value, but the resulting json will be encoded using the CoreConfigSchema to match what core expects. --- helper/plugin/grpc_provider.go | 34 +++++++++++++++++------------ helper/plugin/grpc_provider_test.go | 7 +++--- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index b9b420995..c907bcb72 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -261,30 +261,31 @@ func (s *GRPCProviderServer) UpgradeResourceState(_ context.Context, req *proto. res := s.provider.ResourcesMap[req.TypeName] blockForCore := s.getResourceSchemaBlockForCore(req.TypeName) - blockForShimming := s.getResourceSchemaBlockForShimming(req.TypeName) version := int(req.Version) - var jsonMap map[string]interface{} + jsonMap := map[string]interface{}{} var err error - // if there's a JSON state, we need to decode it. - if len(req.RawState.Json) > 0 { - err = json.Unmarshal(req.RawState.Json, &jsonMap) - if err != nil { - resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) - return resp, nil - } - } - + switch { // We first need to upgrade a flatmap state if it exists. // There should never be both a JSON and Flatmap state in the request. - if req.RawState.Flatmap != nil { + case len(req.RawState.Flatmap) > 0: jsonMap, version, err = s.upgradeFlatmapState(version, req.RawState.Flatmap, res) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } + // if there's a JSON state, we need to decode it. + case len(req.RawState.Json) > 0: + err = json.Unmarshal(req.RawState.Json, &jsonMap) + if err != nil { + resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) + return resp, nil + } + default: + log.Println("[DEBUG] no state provided to upgrade") + return resp, nil } // complete the upgrade of the JSON states @@ -295,11 +296,11 @@ func (s *GRPCProviderServer) UpgradeResourceState(_ context.Context, req *proto. } // The provider isn't required to clean out removed fields - s.removeAttributes(jsonMap, blockForShimming.ImpliedType()) + s.removeAttributes(jsonMap, blockForCore.ImpliedType()) // now we need to turn the state into the default json representation, so // that it can be re-decoded using the actual schema. - val, err := schema.JSONMapToStateValue(jsonMap, blockForShimming) + val, err := schema.JSONMapToStateValue(jsonMap, blockForCore) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -433,6 +434,11 @@ func (s *GRPCProviderServer) removeAttributes(v interface{}, ty cty.Type) { return } + if ty == cty.DynamicPseudoType { + log.Printf("[DEBUG] ignoring dynamic block: %#v\n", v) + return + } + if !ty.IsObjectType() { // This shouldn't happen, and will fail to decode further on, so // there's no need to handle it here. diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go index f58778c47..6186cd411 100644 --- a/helper/plugin/grpc_provider_test.go +++ b/helper/plugin/grpc_provider_test.go @@ -154,10 +154,9 @@ func TestUpgradeState_removedAttr(t *testing.T) { r3 := &schema.Resource{ Schema: map[string]*schema.Schema{ "config_mode_attr": { - Type: schema.TypeList, - ConfigMode: schema.SchemaConfigModeAttr, - SkipCoreTypeCheck: true, - Optional: true, + Type: schema.TypeList, + ConfigMode: schema.SchemaConfigModeAttr, + Optional: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "foo": { From 059dcf1e252427ccfa1df5c3d94bfbeed0e8938d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 May 2019 18:00:01 -0400 Subject: [PATCH 3/6] implement UpgradeResourceState in the mock provider The default is a Noop that simply encodes the provided state --- terraform/provider_mock.go | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/terraform/provider_mock.go b/terraform/provider_mock.go index 8c5824089..4ae346d7d 100644 --- a/terraform/provider_mock.go +++ b/terraform/provider_mock.go @@ -6,6 +6,7 @@ import ( "sync" "github.com/zclconf/go-cty/cty" + ctyjson "github.com/zclconf/go-cty/cty/json" "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/config/hcl2shim" @@ -191,6 +192,10 @@ func (p *MockProvider) UpgradeResourceState(r providers.UpgradeResourceStateRequ p.Lock() defer p.Unlock() + schemas := p.getSchema() + schema := schemas.ResourceTypes[r.TypeName] + schemaType := schema.Block.ImpliedType() + p.UpgradeResourceStateCalled = true p.UpgradeResourceStateRequest = r @@ -198,7 +203,28 @@ func (p *MockProvider) UpgradeResourceState(r providers.UpgradeResourceStateRequ return p.UpgradeResourceStateFn(r) } - return p.UpgradeResourceStateResponse + resp := p.UpgradeResourceStateResponse + + if resp.UpgradedState == cty.NilVal { + switch { + case r.RawStateFlatmap != nil: + v, err := hcl2shim.HCL2ValueFromFlatmap(r.RawStateFlatmap, schemaType) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp + } + resp.UpgradedState = v + case len(r.RawStateJSON) > 0: + v, err := ctyjson.Unmarshal(r.RawStateJSON, schemaType) + + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + return resp + } + resp.UpgradedState = v + } + } + return resp } func (p *MockProvider) Configure(r providers.ConfigureRequest) providers.ConfigureResponse { From c8a2f3840b4a548cb6639051cfde424a4f5fa940 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 May 2019 18:05:30 -0400 Subject: [PATCH 4/6] remove SkipCoreTypeCheck This experiment is no longer needed for handling computed blocks, since the legacy SDK can't reasonably handle Dynamic types, we need to remove this before the final release. Remove LegacySchema functions as well, since handling SkipCoreTypeCheck was the only thing left they were handling. --- helper/schema/core_schema.go | 8 ------ helper/schema/core_schema_test.go | 22 ---------------- helper/schema/schema.go | 36 -------------------------- helper/schema/shims.go | 42 ------------------------------- 4 files changed, 108 deletions(-) diff --git a/helper/schema/core_schema.go b/helper/schema/core_schema.go index d563ab8ca..875677eb3 100644 --- a/helper/schema/core_schema.go +++ b/helper/schema/core_schema.go @@ -174,14 +174,6 @@ func (s *Schema) coreConfigSchemaBlock() *configschema.NestedBlock { // coreConfigSchemaType determines the core config schema type that corresponds // to a particular schema's type. func (s *Schema) coreConfigSchemaType() cty.Type { - if s.SkipCoreTypeCheck { - // If we're preparing a schema for Terraform Core and the schema is - // asking us to skip the Core type-check then we'll tell core that this - // attribute is dynamically-typed, so it'll just pass through anything - // and let us validate it on the plugin side. - return cty.DynamicPseudoType - } - switch s.Type { case TypeString: return cty.String diff --git a/helper/schema/core_schema_test.go b/helper/schema/core_schema_test.go index 92fe194af..7d4b32e01 100644 --- a/helper/schema/core_schema_test.go +++ b/helper/schema/core_schema_test.go @@ -445,28 +445,6 @@ func TestSchemaMapCoreConfigSchema(t *testing.T) { BlockTypes: map[string]*configschema.NestedBlock{}, }), }, - "skip core type check": { - map[string]*Schema{ - "list": { - Type: TypeList, - ConfigMode: SchemaConfigModeAttr, - SkipCoreTypeCheck: true, - Optional: true, - Elem: &Resource{ - Schema: map[string]*Schema{}, - }, - }, - }, - testResource(&configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "list": { - Type: cty.DynamicPseudoType, - Optional: true, // Just so we can progress to provider-driven validation and return the error there - }, - }, - BlockTypes: map[string]*configschema.NestedBlock{}, - }), - }, } for name, test := range tests { diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 140cdff22..6a3c15a64 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -95,34 +95,6 @@ type Schema struct { // behavior, and SchemaConfigModeBlock is not permitted. ConfigMode SchemaConfigMode - // SkipCoreTypeCheck, if set, will advertise this attribute to Terraform Core - // has being dynamically-typed rather than deriving a type from the schema. - // This has the effect of making Terraform Core skip all type-checking of - // the value, and thus leaves all type checking up to a combination of this - // SDK and the provider's own code. - // - // This flag does nothing for Terraform versions prior to v0.12, because - // in prior versions there was no Core-side typecheck anyway. - // - // The most practical effect of this flag is to allow object-typed schemas - // (specified with Elem: schema.Resource) to pass through Terraform Core - // even without all of the object type attributes specified, which may be - // useful when using ConfigMode: SchemaConfigModeAttr to achieve - // nested-block-like behaviors while using attribute syntax. - // - // However, by doing so we require type information to be sent and stored - // per-object rather than just once statically in the schema, and so this - // will change the wire serialization of a resource type in state. Changing - // the value of SkipCoreTypeCheck will therefore require a state migration - // if there has previously been any release of the provider compatible with - // Terraform v0.12. - // - // SkipCoreTypeCheck can only be set when ConfigMode is SchemaConfigModeAttr, - // because nested blocks cannot be decoded by Terraform Core without at - // least shallow information about the next level of nested attributes and - // blocks. - SkipCoreTypeCheck bool - // If one of these is set, then this item can come from the configuration. // Both cannot be set. If Optional is set, the value is optional. If // Required is set, the value is required. @@ -735,8 +707,6 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro computedOnly := v.Computed && !v.Optional - isBlock := false - switch v.ConfigMode { case SchemaConfigModeBlock: if _, ok := v.Elem.(*Resource); !ok { @@ -748,7 +718,6 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro if computedOnly { return fmt.Errorf("%s: ConfigMode of block cannot be used for computed schema", k) } - isBlock = true case SchemaConfigModeAttr: // anything goes case SchemaConfigModeAuto: @@ -756,7 +725,6 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro // and that's impossible inside an attribute, we require it to be // explicitly overridden as mode "Attr" for clarity. if _, ok := v.Elem.(*Resource); ok { - isBlock = true if attrsOnly { return fmt.Errorf("%s: in *schema.Resource with ConfigMode of attribute, so must also have ConfigMode of attribute", k) } @@ -765,10 +733,6 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro return fmt.Errorf("%s: invalid ConfigMode value", k) } - if isBlock && v.SkipCoreTypeCheck { - return fmt.Errorf("%s: SkipCoreTypeCheck must be false unless ConfigMode is attribute", k) - } - if v.Computed && v.Default != nil { return fmt.Errorf("%s: Default must be nil if computed", k) } diff --git a/helper/schema/shims.go b/helper/schema/shims.go index d43028e58..203d01704 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -113,45 +113,3 @@ func JSONMapToStateValue(m map[string]interface{}, block *configschema.Block) (c func StateValueFromInstanceState(is *terraform.InstanceState, ty cty.Type) (cty.Value, error) { return is.AttrsAsObjectValue(ty) } - -// LegacyResourceSchema takes a *Resource and returns a deep copy with 0.12 specific -// features removed. This is used by the shims to get a configschema that -// directly matches the structure of the schema.Resource. -func LegacyResourceSchema(r *Resource) *Resource { - if r == nil { - return nil - } - // start with a shallow copy - newResource := new(Resource) - *newResource = *r - newResource.Schema = map[string]*Schema{} - - for k, s := range r.Schema { - newResource.Schema[k] = LegacySchema(s) - } - - return newResource -} - -// LegacySchema takes a *Schema and returns a deep copy with some 0.12-specific -// features disabled. This is used by the shims to get a configschema that -// better reflects the given schema.Resource, without any adjustments we -// make for when sending a schema to Terraform Core. -func LegacySchema(s *Schema) *Schema { - if s == nil { - return nil - } - // start with a shallow copy - newSchema := new(Schema) - *newSchema = *s - newSchema.SkipCoreTypeCheck = false - - switch e := newSchema.Elem.(type) { - case *Schema: - newSchema.Elem = LegacySchema(e) - case *Resource: - newSchema.Elem = LegacyResourceSchema(e) - } - - return newSchema -} From c9e1d26c25faf056b09d705b733ef6adc8d1a13a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 May 2019 18:12:57 -0400 Subject: [PATCH 5/6] remove the legacy schema access Having removed the methods, it is straightforward to mechanically update this file to get rid of all references to the "legacy schema". There is now only one config schema type to deal with in the sdk. --- helper/plugin/grpc_provider.go | 134 +++++++++++++-------------------- 1 file changed, 53 insertions(+), 81 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index c907bcb72..510f47f35 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -52,7 +52,7 @@ func (s *GRPCProviderServer) GetSchema(_ context.Context, req *proto.GetProvider } resp.Provider = &proto.Schema{ - Block: convert.ConfigSchemaToProto(s.getProviderSchemaBlockForCore()), + Block: convert.ConfigSchemaToProto(s.getProviderSchemaBlock()), } for typ, res := range s.provider.ResourcesMap { @@ -72,46 +72,26 @@ func (s *GRPCProviderServer) GetSchema(_ context.Context, req *proto.GetProvider return resp, nil } -func (s *GRPCProviderServer) getProviderSchemaBlockForCore() *configschema.Block { +func (s *GRPCProviderServer) getProviderSchemaBlock() *configschema.Block { return schema.InternalMap(s.provider.Schema).CoreConfigSchema() } -func (s *GRPCProviderServer) getResourceSchemaBlockForCore(name string) *configschema.Block { +func (s *GRPCProviderServer) getResourceSchemaBlock(name string) *configschema.Block { res := s.provider.ResourcesMap[name] return res.CoreConfigSchema() } -func (s *GRPCProviderServer) getDatasourceSchemaBlockForCore(name string) *configschema.Block { +func (s *GRPCProviderServer) getDatasourceSchemaBlock(name string) *configschema.Block { dat := s.provider.DataSourcesMap[name] return dat.CoreConfigSchema() } -func (s *GRPCProviderServer) getProviderSchemaBlockForShimming() *configschema.Block { - newSchema := map[string]*schema.Schema{} - for attr, s := range s.provider.Schema { - newSchema[attr] = schema.LegacySchema(s) - } - - return schema.InternalMap(newSchema).CoreConfigSchema() -} - -func (s *GRPCProviderServer) getResourceSchemaBlockForShimming(name string) *configschema.Block { - res := s.provider.ResourcesMap[name] - return schema.LegacyResourceSchema(res).CoreConfigSchema() -} - -func (s *GRPCProviderServer) getDatasourceSchemaBlockForShimming(name string) *configschema.Block { - dat := s.provider.DataSourcesMap[name] - return schema.LegacyResourceSchema(dat).CoreConfigSchema() -} - func (s *GRPCProviderServer) PrepareProviderConfig(_ context.Context, req *proto.PrepareProviderConfig_Request) (*proto.PrepareProviderConfig_Response, error) { resp := &proto.PrepareProviderConfig_Response{} - blockForCore := s.getProviderSchemaBlockForCore() - blockForShimming := s.getProviderSchemaBlockForShimming() + schemaBlock := s.getProviderSchemaBlock() - configVal, err := msgpack.Unmarshal(req.Config.Msgpack, blockForCore.ImpliedType()) + configVal, err := msgpack.Unmarshal(req.Config.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -182,7 +162,7 @@ func (s *GRPCProviderServer) PrepareProviderConfig(_ context.Context, req *proto return resp, nil } - configVal, err = blockForShimming.CoerceValue(configVal) + configVal, err = schemaBlock.CoerceValue(configVal) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -194,12 +174,12 @@ func (s *GRPCProviderServer) PrepareProviderConfig(_ context.Context, req *proto return resp, nil } - config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) + config := terraform.NewResourceConfigShimmed(configVal, schemaBlock) warns, errs := s.provider.Validate(config) resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, convert.WarnsAndErrsToProto(warns, errs)) - preparedConfigMP, err := msgpack.Marshal(configVal, blockForCore.ImpliedType()) + preparedConfigMP, err := msgpack.Marshal(configVal, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -213,16 +193,15 @@ func (s *GRPCProviderServer) PrepareProviderConfig(_ context.Context, req *proto func (s *GRPCProviderServer) ValidateResourceTypeConfig(_ context.Context, req *proto.ValidateResourceTypeConfig_Request) (*proto.ValidateResourceTypeConfig_Response, error) { resp := &proto.ValidateResourceTypeConfig_Response{} - blockForCore := s.getResourceSchemaBlockForCore(req.TypeName) - blockForShimming := s.getResourceSchemaBlockForShimming(req.TypeName) + schemaBlock := s.getResourceSchemaBlock(req.TypeName) - configVal, err := msgpack.Unmarshal(req.Config.Msgpack, blockForCore.ImpliedType()) + configVal, err := msgpack.Unmarshal(req.Config.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } - config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) + config := terraform.NewResourceConfigShimmed(configVal, schemaBlock) warns, errs := s.provider.ValidateResource(req.TypeName, config) resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, convert.WarnsAndErrsToProto(warns, errs)) @@ -233,10 +212,9 @@ func (s *GRPCProviderServer) ValidateResourceTypeConfig(_ context.Context, req * func (s *GRPCProviderServer) ValidateDataSourceConfig(_ context.Context, req *proto.ValidateDataSourceConfig_Request) (*proto.ValidateDataSourceConfig_Response, error) { resp := &proto.ValidateDataSourceConfig_Response{} - blockForCore := s.getDatasourceSchemaBlockForCore(req.TypeName) - blockForShimming := s.getDatasourceSchemaBlockForShimming(req.TypeName) + schemaBlock := s.getDatasourceSchemaBlock(req.TypeName) - configVal, err := msgpack.Unmarshal(req.Config.Msgpack, blockForCore.ImpliedType()) + configVal, err := msgpack.Unmarshal(req.Config.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -248,7 +226,7 @@ func (s *GRPCProviderServer) ValidateDataSourceConfig(_ context.Context, req *pr return resp, nil } - config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) + config := terraform.NewResourceConfigShimmed(configVal, schemaBlock) warns, errs := s.provider.ValidateDataSource(req.TypeName, config) resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, convert.WarnsAndErrsToProto(warns, errs)) @@ -260,7 +238,7 @@ func (s *GRPCProviderServer) UpgradeResourceState(_ context.Context, req *proto. resp := &proto.UpgradeResourceState_Response{} res := s.provider.ResourcesMap[req.TypeName] - blockForCore := s.getResourceSchemaBlockForCore(req.TypeName) + schemaBlock := s.getResourceSchemaBlock(req.TypeName) version := int(req.Version) @@ -296,18 +274,18 @@ func (s *GRPCProviderServer) UpgradeResourceState(_ context.Context, req *proto. } // The provider isn't required to clean out removed fields - s.removeAttributes(jsonMap, blockForCore.ImpliedType()) + s.removeAttributes(jsonMap, schemaBlock.ImpliedType()) // now we need to turn the state into the default json representation, so // that it can be re-decoded using the actual schema. - val, err := schema.JSONMapToStateValue(jsonMap, blockForCore) + val, err := schema.JSONMapToStateValue(jsonMap, schemaBlock) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } // encode the final state to the expected msgpack format - newStateMP, err := msgpack.Marshal(val, blockForCore.ImpliedType()) + newStateMP, err := msgpack.Marshal(val, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -330,7 +308,7 @@ func (s *GRPCProviderServer) upgradeFlatmapState(version int, m map[string]strin // first determine if we need to call the legacy MigrateState func requiresMigrate := version < res.SchemaVersion - schemaType := schema.LegacyResourceSchema(res).CoreConfigSchema().ImpliedType() + schemaType := res.CoreConfigSchema().ImpliedType() // if there are any StateUpgraders, then we need to only compare // against the first version there @@ -474,10 +452,9 @@ func (s *GRPCProviderServer) Stop(_ context.Context, _ *proto.Stop_Request) (*pr func (s *GRPCProviderServer) Configure(_ context.Context, req *proto.Configure_Request) (*proto.Configure_Response, error) { resp := &proto.Configure_Response{} - blockForCore := s.getProviderSchemaBlockForCore() - blockForShimming := s.getProviderSchemaBlockForShimming() + schemaBlock := s.getProviderSchemaBlock() - configVal, err := msgpack.Unmarshal(req.Config.Msgpack, blockForCore.ImpliedType()) + configVal, err := msgpack.Unmarshal(req.Config.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -491,7 +468,7 @@ func (s *GRPCProviderServer) Configure(_ context.Context, req *proto.Configure_R return resp, nil } - config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) + config := terraform.NewResourceConfigShimmed(configVal, schemaBlock) err = s.provider.Configure(config) resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) @@ -502,10 +479,9 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso resp := &proto.ReadResource_Response{} res := s.provider.ResourcesMap[req.TypeName] - blockForCore := s.getResourceSchemaBlockForCore(req.TypeName) - blockForShimming := s.getResourceSchemaBlockForShimming(req.TypeName) + schemaBlock := s.getResourceSchemaBlock(req.TypeName) - stateVal, err := msgpack.Unmarshal(req.CurrentState.Msgpack, blockForCore.ImpliedType()) + stateVal, err := msgpack.Unmarshal(req.CurrentState.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -527,7 +503,7 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso // The old provider API used an empty id to signal that the remote // object appears to have been deleted, but our new protocol expects // to see a null value (in the cty sense) in that case. - newStateMP, err := msgpack.Marshal(cty.NullVal(blockForCore.ImpliedType()), blockForCore.ImpliedType()) + newStateMP, err := msgpack.Marshal(cty.NullVal(schemaBlock.ImpliedType()), schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) } @@ -540,7 +516,7 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso // helper/schema should always copy the ID over, but do it again just to be safe newInstanceState.Attributes["id"] = newInstanceState.ID - newStateVal, err := hcl2shim.HCL2ValueFromFlatmap(newInstanceState.Attributes, blockForShimming.ImpliedType()) + newStateVal, err := hcl2shim.HCL2ValueFromFlatmap(newInstanceState.Attributes, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -549,7 +525,7 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso newStateVal = normalizeNullValues(newStateVal, stateVal, false) newStateVal = copyTimeoutValues(newStateVal, stateVal) - newStateMP, err := msgpack.Marshal(newStateVal, blockForCore.ImpliedType()) + newStateMP, err := msgpack.Marshal(newStateVal, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -574,10 +550,9 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl resp.LegacyTypeSystem = true res := s.provider.ResourcesMap[req.TypeName] - blockForCore := s.getResourceSchemaBlockForCore(req.TypeName) - blockForShimming := s.getResourceSchemaBlockForShimming(req.TypeName) + schemaBlock := s.getResourceSchemaBlock(req.TypeName) - priorStateVal, err := msgpack.Unmarshal(req.PriorState.Msgpack, blockForCore.ImpliedType()) + priorStateVal, err := msgpack.Unmarshal(req.PriorState.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -585,7 +560,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl create := priorStateVal.IsNull() - proposedNewStateVal, err := msgpack.Unmarshal(req.ProposedNewState.Msgpack, blockForCore.ImpliedType()) + proposedNewStateVal, err := msgpack.Unmarshal(req.ProposedNewState.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -623,7 +598,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl } // turn the proposed state into a legacy configuration - cfg := terraform.NewResourceConfigShimmed(proposedNewStateVal, blockForShimming) + cfg := terraform.NewResourceConfigShimmed(proposedNewStateVal, schemaBlock) diff, err := s.provider.SimpleDiff(info, priorState, cfg) if err != nil { @@ -656,15 +631,15 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl } // now we need to apply the diff to the prior state, so get the planned state - plannedAttrs, err := diff.Apply(priorState.Attributes, blockForShimming) + plannedAttrs, err := diff.Apply(priorState.Attributes, schemaBlock) - plannedStateVal, err := hcl2shim.HCL2ValueFromFlatmap(plannedAttrs, blockForShimming.ImpliedType()) + plannedStateVal, err := hcl2shim.HCL2ValueFromFlatmap(plannedAttrs, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } - plannedStateVal, err = blockForShimming.CoerceValue(plannedStateVal) + plannedStateVal, err = schemaBlock.CoerceValue(plannedStateVal) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -696,10 +671,10 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl // if this was creating the resource, we need to set any remaining computed // fields if create { - plannedStateVal = SetUnknowns(plannedStateVal, blockForShimming) + plannedStateVal = SetUnknowns(plannedStateVal, schemaBlock) } - plannedMP, err := msgpack.Marshal(plannedStateVal, blockForCore.ImpliedType()) + plannedMP, err := msgpack.Marshal(plannedStateVal, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -752,7 +727,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl requiresNew = append(requiresNew, "id") } - requiresReplace, err := hcl2shim.RequiresReplace(requiresNew, blockForShimming.ImpliedType()) + requiresReplace, err := hcl2shim.RequiresReplace(requiresNew, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -773,16 +748,15 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A } res := s.provider.ResourcesMap[req.TypeName] - blockForCore := s.getResourceSchemaBlockForCore(req.TypeName) - blockForShimming := s.getResourceSchemaBlockForShimming(req.TypeName) + schemaBlock := s.getResourceSchemaBlock(req.TypeName) - priorStateVal, err := msgpack.Unmarshal(req.PriorState.Msgpack, blockForCore.ImpliedType()) + priorStateVal, err := msgpack.Unmarshal(req.PriorState.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } - plannedStateVal, err := msgpack.Unmarshal(req.PlannedState.Msgpack, blockForCore.ImpliedType()) + plannedStateVal, err := msgpack.Unmarshal(req.PlannedState.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -870,13 +844,13 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) } - newStateVal := cty.NullVal(blockForShimming.ImpliedType()) + newStateVal := cty.NullVal(schemaBlock.ImpliedType()) // Always return a null value for destroy. // While this is usually indicated by a nil state, check for missing ID or // attributes in the case of a provider failure. if destroy || newInstanceState == nil || newInstanceState.Attributes == nil || newInstanceState.ID == "" { - newStateMP, err := msgpack.Marshal(newStateVal, blockForCore.ImpliedType()) + newStateMP, err := msgpack.Marshal(newStateVal, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -889,7 +863,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A // We keep the null val if we destroyed the resource, otherwise build the // entire object, even if the new state was nil. - newStateVal, err = schema.StateValueFromInstanceState(newInstanceState, blockForShimming.ImpliedType()) + newStateVal, err = schema.StateValueFromInstanceState(newInstanceState, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -899,7 +873,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A newStateVal = copyTimeoutValues(newStateVal, plannedStateVal) - newStateMP, err := msgpack.Marshal(newStateVal, blockForCore.ImpliedType()) + newStateMP, err := msgpack.Marshal(newStateVal, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -948,15 +922,14 @@ func (s *GRPCProviderServer) ImportResourceState(_ context.Context, req *proto.I resourceType = req.TypeName } - blockForCore := s.getResourceSchemaBlockForCore(resourceType) - blockForShimming := s.getResourceSchemaBlockForShimming(resourceType) - newStateVal, err := hcl2shim.HCL2ValueFromFlatmap(is.Attributes, blockForShimming.ImpliedType()) + schemaBlock := s.getResourceSchemaBlock(resourceType) + newStateVal, err := hcl2shim.HCL2ValueFromFlatmap(is.Attributes, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil } - newStateMP, err := msgpack.Marshal(newStateVal, blockForCore.ImpliedType()) + newStateMP, err := msgpack.Marshal(newStateVal, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -985,10 +958,9 @@ func (s *GRPCProviderServer) ImportResourceState(_ context.Context, req *proto.I func (s *GRPCProviderServer) ReadDataSource(_ context.Context, req *proto.ReadDataSource_Request) (*proto.ReadDataSource_Response, error) { resp := &proto.ReadDataSource_Response{} - blockForCore := s.getDatasourceSchemaBlockForCore(req.TypeName) - blockForShimming := s.getDatasourceSchemaBlockForShimming(req.TypeName) + schemaBlock := s.getDatasourceSchemaBlock(req.TypeName) - configVal, err := msgpack.Unmarshal(req.Config.Msgpack, blockForCore.ImpliedType()) + configVal, err := msgpack.Unmarshal(req.Config.Msgpack, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -1004,7 +976,7 @@ func (s *GRPCProviderServer) ReadDataSource(_ context.Context, req *proto.ReadDa return resp, nil } - config := terraform.NewResourceConfigShimmed(configVal, blockForShimming) + config := terraform.NewResourceConfigShimmed(configVal, schemaBlock) // we need to still build the diff separately with the Read method to match // the old behavior @@ -1021,7 +993,7 @@ func (s *GRPCProviderServer) ReadDataSource(_ context.Context, req *proto.ReadDa return resp, nil } - newStateVal, err := schema.StateValueFromInstanceState(newInstanceState, blockForShimming.ImpliedType()) + newStateVal, err := schema.StateValueFromInstanceState(newInstanceState, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil @@ -1029,7 +1001,7 @@ func (s *GRPCProviderServer) ReadDataSource(_ context.Context, req *proto.ReadDa newStateVal = copyTimeoutValues(newStateVal, configVal) - newStateMP, err := msgpack.Marshal(newStateVal, blockForCore.ImpliedType()) + newStateMP, err := msgpack.Marshal(newStateVal, schemaBlock.ImpliedType()) if err != nil { resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err) return resp, nil From b9e5745b3c8eb26ed661c850a0e339a57c8fd59a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 May 2019 18:19:18 -0400 Subject: [PATCH 6/6] remove SkipCoreTypeCheck and LegacySchema Update the test provider to match the sdk. --- builtin/providers/test/diff_apply_test.go | 2 +- .../providers/test/resource_config_mode.go | 29 +++---------- .../test/resource_config_mode_test.go | 43 ------------------- 3 files changed, 7 insertions(+), 67 deletions(-) diff --git a/builtin/providers/test/diff_apply_test.go b/builtin/providers/test/diff_apply_test.go index 63a186d36..144e70624 100644 --- a/builtin/providers/test/diff_apply_test.go +++ b/builtin/providers/test/diff_apply_test.go @@ -131,7 +131,7 @@ func TestDiffApply_set(t *testing.T) { "id": "testID", } - attrs, err := diff.Apply(priorAttrs, schema.LegacyResourceSchema(&schema.Resource{Schema: resSchema}).CoreConfigSchema()) + attrs, err := diff.Apply(priorAttrs, (&schema.Resource{Schema: resSchema}).CoreConfigSchema()) if err != nil { t.Fatal(err) } diff --git a/builtin/providers/test/resource_config_mode.go b/builtin/providers/test/resource_config_mode.go index 82e8ff214..da0896e33 100644 --- a/builtin/providers/test/resource_config_mode.go +++ b/builtin/providers/test/resource_config_mode.go @@ -28,21 +28,6 @@ func testResourceConfigMode() *schema.Resource { }, }, }, - "resource_as_attr_dynamic": { - Type: schema.TypeList, - ConfigMode: schema.SchemaConfigModeAttr, - SkipCoreTypeCheck: true, - Optional: true, - Elem: &schema.Resource{ - Schema: map[string]*schema.Schema{ - "foo": { - Type: schema.TypeString, - Optional: true, - Default: "default", - }, - }, - }, - }, }, } } @@ -53,14 +38,12 @@ func testResourceConfigModeCreate(d *schema.ResourceData, meta interface{}) erro } func testResourceConfigModeRead(d *schema.ResourceData, meta interface{}) error { - for _, k := range []string{"resource_as_attr", "resource_as_attr_dynamic"} { - if l, ok := d.Get(k).([]interface{}); !ok { - return fmt.Errorf("%s should appear as []interface{}, not %T", k, l) - } else { - for i, item := range l { - if _, ok := item.(map[string]interface{}); !ok { - return fmt.Errorf("%s[%d] should appear as map[string]interface{}, not %T", k, i, item) - } + if l, ok := d.Get("resource_as_attr").([]interface{}); !ok { + return fmt.Errorf("resource_as_attr should appear as []interface{}, not %T", l) + } else { + for i, item := range l { + if _, ok := item.(map[string]interface{}); !ok { + return fmt.Errorf("resource_as_attr[%d] should appear as map[string]interface{}, not %T", i, item) } } } diff --git a/builtin/providers/test/resource_config_mode_test.go b/builtin/providers/test/resource_config_mode_test.go index 02d0c575a..46445bed5 100644 --- a/builtin/providers/test/resource_config_mode_test.go +++ b/builtin/providers/test/resource_config_mode_test.go @@ -23,22 +23,12 @@ resource "test_resource_config_mode" "foo" { foo = "resource_as_attr 1" }, ] - resource_as_attr_dynamic = [ - { - foo = "resource_as_attr_dynamic 0" - }, - { - }, - ] } `), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.#", "2"), resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.0.foo", "resource_as_attr 0"), resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.1.foo", "resource_as_attr 1"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.#", "2"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.0.foo", "resource_as_attr_dynamic 0"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.1.foo", "default"), ), }, resource.TestStep{ @@ -58,22 +48,12 @@ resource "test_resource_config_mode" "foo" { resource_as_attr { foo = "resource_as_attr 1" } - resource_as_attr_dynamic = [ - { - foo = "resource_as_attr_dynamic 0" - }, - { - }, - ] } `), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.#", "2"), resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.0.foo", "resource_as_attr 0"), resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.1.foo", "resource_as_attr 1"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.#", "2"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.0.foo", "resource_as_attr_dynamic 0"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.1.foo", "default"), ), }, resource.TestStep{ @@ -84,43 +64,21 @@ resource "test_resource_config_mode" "foo" { foo = "resource_as_attr 0 updated" }, ] - resource_as_attr_dynamic = [ - { - }, - ] } `), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.#", "1"), resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.0.foo", "resource_as_attr 0 updated"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.#", "1"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.0.foo", "default"), - ), - }, - resource.TestStep{ - Config: strings.TrimSpace(` -resource "test_resource_config_mode" "foo" { - resource_as_attr_dynamic = [ - { - }, - ] -} - `), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.#", "1"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.#", "1"), ), }, resource.TestStep{ Config: strings.TrimSpace(` resource "test_resource_config_mode" "foo" { resource_as_attr = [] - resource_as_attr_dynamic = [] } `), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.#", "0"), - resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.#", "0"), ), }, resource.TestStep{ @@ -130,7 +88,6 @@ resource "test_resource_config_mode" "foo" { `), Check: resource.ComposeTestCheckFunc( resource.TestCheckNoResourceAttr("test_resource_config_mode.foo", "resource_as_attr.#"), - resource.TestCheckNoResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.#"), ), }, },