From d961b1de1bbdfb59e38f48d50de9208a5b092d2f Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 1 Jun 2018 12:36:55 -0700 Subject: [PATCH] core: Stop loading provider schema during graph walk We now fetch all of the necessary schemas during context creation, so we can just thread that repository of schemas through into EvalContext and Evaluator and access the schemas as needed without any further fetching. This requires updating a few tests to have a valid Provider address in their state objects, because we need that in order to trigger the loading of the relevant schema. --- terraform/context_apply_test.go | 9 ++++ terraform/context_refresh_test.go | 4 ++ terraform/eval_context_builtin.go | 59 +++++--------------------- terraform/eval_context_builtin_test.go | 38 ----------------- terraform/evaluate.go | 33 +++++++------- terraform/graph_walk_context.go | 5 +-- terraform/schemas.go | 33 ++++++++------ 7 files changed, 60 insertions(+), 121 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index edc908ac7..17af4d55c 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -285,6 +285,7 @@ func TestContext2Apply_resourceDependsOnModuleStateOnly(t *testing.T) { ID: "bar", }, Dependencies: []string{"module.child"}, + Provider: "provider.aws", }, }, }, @@ -296,6 +297,7 @@ func TestContext2Apply_resourceDependsOnModuleStateOnly(t *testing.T) { Primary: &InstanceState{ ID: "bar", }, + Provider: "provider.aws", }, }, }, @@ -1330,6 +1332,7 @@ func testContext2Apply_destroyDependsOnStateOnly(t *testing.T) { ID: "foo", Attributes: map[string]string{}, }, + Provider: "provider.aws", }, "aws_instance.bar": &ResourceState{ @@ -1339,6 +1342,7 @@ func testContext2Apply_destroyDependsOnStateOnly(t *testing.T) { Attributes: map[string]string{}, }, Dependencies: []string{"aws_instance.foo"}, + Provider: "provider.aws", }, }, }, @@ -1408,6 +1412,7 @@ func testContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { ID: "foo", Attributes: map[string]string{}, }, + Provider: "provider.aws", }, "aws_instance.bar": &ResourceState{ @@ -1417,6 +1422,7 @@ func testContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { Attributes: map[string]string{}, }, Dependencies: []string{"aws_instance.foo"}, + Provider: "provider.aws", }, }, }, @@ -6158,6 +6164,7 @@ func TestContext2Apply_destroyNestedModule(t *testing.T) { Primary: &InstanceState{ ID: "bar", }, + Provider: "provider.aws", }, }, }, @@ -6207,6 +6214,7 @@ func TestContext2Apply_destroyDeeplyNestedModule(t *testing.T) { Primary: &InstanceState{ ID: "bar", }, + Provider: "provider.aws", }, }, }, @@ -6977,6 +6985,7 @@ func TestContext2Apply_hookOrphan(t *testing.T) { Primary: &InstanceState{ ID: "bar", }, + Provider: "provider.aws", }, }, }, diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index f0373d85f..75b947c4c 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -853,6 +853,7 @@ func TestContext2Refresh_dataOrphan(t *testing.T) { Primary: &InstanceState{ ID: "foo", }, + Provider: "provider.null", }, }, }, @@ -1224,6 +1225,7 @@ func TestContext2Refresh_orphanModule(t *testing.T) { "module.child", "module.child", }, + Provider: "provider.aws", }, }, }, @@ -1241,6 +1243,7 @@ func TestContext2Refresh_orphanModule(t *testing.T) { Dependencies: []string{ "module.grandchild", }, + Provider: "provider.aws", }, }, Outputs: map[string]*OutputState{ @@ -1262,6 +1265,7 @@ func TestContext2Refresh_orphanModule(t *testing.T) { Primary: &InstanceState{ ID: "i-cde345", }, + Provider: "provider.aws", }, }, Outputs: map[string]*OutputState{ diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 7608be3c5..bda5676d0 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -28,6 +28,15 @@ type BuiltinEvalContext struct { // eval context. Evaluator *Evaluator + // Schemas is a repository of all of the schemas we should need to + // decode configuration blocks and expressions. This must be constructed by + // the caller to include schemas for all of the providers, resource types, + // data sources and provisioners used by the given configuration and + // state. + // + // This must not be mutated during evaluation. + Schemas *Schemas + // VariableValues contains the variable values across all modules. This // structure is shared across the entire containing context, and so it // may be accessed only when holding VariableValuesLock. @@ -41,11 +50,9 @@ type BuiltinEvalContext struct { Hooks []Hook InputValue UIInput ProviderCache map[string]ResourceProvider - ProviderSchemas map[string]*ProviderSchema ProviderInputConfig map[string]map[string]cty.Value ProviderLock *sync.Mutex ProvisionerCache map[string]ResourceProvisioner - ProvisionerSchemas map[string]*configschema.Block ProvisionerLock *sync.Mutex DiffValue *Diff DiffLock *sync.RWMutex @@ -115,34 +122,6 @@ func (ctx *BuiltinEvalContext) InitProvider(typeName string, addr addrs.Provider log.Printf("[TRACE] BuiltinEvalContext: Initialized %q provider for %s", typeName, absAddr) ctx.ProviderCache[key] = p - // Also fetch and cache the provider's schema. - // FIXME: This is using a non-ideal provider API that requires us to - // request specific resource types, but we actually just want _all_ the - // resource types, so we'll list these first. Once the provider API is - // updated we'll get enough data to populate this whole structure in - // a single call. - resourceTypes := p.Resources() - dataSources := p.DataSources() - resourceTypeNames := make([]string, len(resourceTypes)) - for i, t := range resourceTypes { - resourceTypeNames[i] = t.Name - } - dataSourceNames := make([]string, len(dataSources)) - for i, t := range dataSources { - dataSourceNames[i] = t.Name - } - schema, err := p.GetSchema(&ProviderSchemaRequest{ - DataSources: dataSourceNames, - ResourceTypes: resourceTypeNames, - }) - if err != nil { - return nil, fmt.Errorf("error fetching schema for %s: %s", key, err) - } - if ctx.ProviderSchemas == nil { - ctx.ProviderSchemas = make(map[string]*ProviderSchema) - } - ctx.ProviderSchemas[typeName] = schema - return p, nil } @@ -158,10 +137,7 @@ func (ctx *BuiltinEvalContext) Provider(addr addrs.AbsProviderConfig) ResourcePr func (ctx *BuiltinEvalContext) ProviderSchema(addr addrs.AbsProviderConfig) *ProviderSchema { ctx.once.Do(ctx.init) - ctx.ProviderLock.Lock() - defer ctx.ProviderLock.Unlock() - - return ctx.ProviderSchemas[addr.ProviderConfig.Type] + return ctx.Schemas.ProviderSchema(addr.ProviderConfig.Type) } func (ctx *BuiltinEvalContext) CloseProvider(addr addrs.ProviderConfig) error { @@ -250,16 +226,6 @@ func (ctx *BuiltinEvalContext) InitProvisioner(n string) (ResourceProvisioner, e ctx.ProvisionerCache[key] = p - // Also fetch the provisioner's schema - schema, err := p.GetConfigSchema() - if err != nil { - return nil, fmt.Errorf("error getting schema for provisioner %q: %s", n, err) - } - if ctx.ProvisionerSchemas == nil { - ctx.ProvisionerSchemas = make(map[string]*configschema.Block) - } - ctx.ProvisionerSchemas[n] = schema - return p, nil } @@ -276,10 +242,7 @@ func (ctx *BuiltinEvalContext) Provisioner(n string) ResourceProvisioner { func (ctx *BuiltinEvalContext) ProvisionerSchema(n string) *configschema.Block { ctx.once.Do(ctx.init) - ctx.ProvisionerLock.Lock() - defer ctx.ProvisionerLock.Unlock() - - return ctx.ProvisionerSchemas[n] + return ctx.Schemas.ProvisionerConfig(n) } func (ctx *BuiltinEvalContext) CloseProvisioner(n string) error { diff --git a/terraform/eval_context_builtin_test.go b/terraform/eval_context_builtin_test.go index 2cb130511..2e2081a0c 100644 --- a/terraform/eval_context_builtin_test.go +++ b/terraform/eval_context_builtin_test.go @@ -7,9 +7,6 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/zclconf/go-cty/cty" - - "github.com/davecgh/go-spew/spew" - "github.com/hashicorp/terraform/config/configschema" ) func TestBuiltinEvalContextProviderInput(t *testing.T) { @@ -61,15 +58,6 @@ func TestBuildingEvalContextInitProvider(t *testing.T) { SchemaAvailable: true, }, }, - GetSchemaReturn: &ProviderSchema{ - Provider: &configschema.Block{}, - ResourceTypes: map[string]*configschema.Block{ - "test_thing": &configschema.Block{}, - }, - DataSources: map[string]*configschema.Block{ - "test_thing": &configschema.Block{}, - }, - }, } ctx := testBuiltinEvalContext(t) @@ -92,32 +80,6 @@ func TestBuildingEvalContextInitProvider(t *testing.T) { if err != nil { t.Fatalf("error initializing provider test.foo: %s", err) } - - { - got := testP.GetSchemaRequest - want := &ProviderSchemaRequest{ - DataSources: []string{"test_thing"}, - ResourceTypes: []string{"test_thing"}, - } - - if !reflect.DeepEqual(got, want) { - t.Errorf("wrong schema request\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(want)) - } - } - - { - schema := ctx.ProviderSchema(providerAddrDefault.Absolute(addrs.RootModuleInstance)) - if got, want := schema, testP.GetSchemaReturn; !reflect.DeepEqual(got, want) { - t.Errorf("wrong schema\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(want)) - } - } - - { - schema := ctx.ProviderSchema(providerAddrAlias.Absolute(addrs.RootModuleInstance)) - if got, want := schema, testP.GetSchemaReturn; !reflect.DeepEqual(got, want) { - t.Errorf("wrong schema\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(want)) - } - } } func testBuiltinEvalContext(t *testing.T) *BuiltinEvalContext { diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 342e695b7..ce622b53c 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -44,11 +44,13 @@ type Evaluator struct { VariableValues map[string]map[string]cty.Value VariableValuesLock *sync.Mutex - // ProviderSchemas is a map of schemas for all provider configurations - // that have been initialized so far. This is mutated concurrently, so - // it must be accessed only while holding ProvidersLock. - ProviderSchemas map[string]*ProviderSchema - ProvidersLock *sync.Mutex + // Schemas is a repository of all of the schemas we should need to + // evaluate expressions. This must be constructed by the caller to + // include schemas for all of the providers, resource types, data sources + // and provisioners used by the given configuration and state. + // + // This must not be mutated during evaluation. + Schemas *Schemas // State is the current state. During some operations this structure // is mutated concurrently, and so it must be accessed only while holding @@ -694,23 +696,18 @@ func (d *evaluationStateData) getResourceInstancePending(addr addrs.ResourceInst } func (d *evaluationStateData) getResourceSchema(addr addrs.Resource, providerAddr addrs.AbsProviderConfig) *configschema.Block { - d.Evaluator.ProvidersLock.Lock() - defer d.Evaluator.ProvidersLock.Unlock() - - log.Printf("[TRACE] Need provider schema for %s", providerAddr) - providerSchema := d.Evaluator.ProviderSchemas[providerAddr.ProviderConfig.Type] - if providerSchema == nil { - return nil - } - - var schema *configschema.Block + providerType := providerAddr.ProviderConfig.Type + typeName := addr.Type + schemas := d.Evaluator.Schemas switch addr.Mode { case addrs.ManagedResourceMode: - schema = providerSchema.ResourceTypes[addr.Type] + return schemas.ResourceTypeConfig(providerType, typeName) case addrs.DataResourceMode: - schema = providerSchema.DataSources[addr.Type] + return schemas.DataSourceConfig(providerType, typeName) + default: + log.Printf("[WARN] Don't know how to fetch schema for resource %s", providerAddr) + return nil } - return schema } func (d *evaluationStateData) GetTerraformAttr(addr addrs.TerraformAttr, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { diff --git a/terraform/graph_walk_context.go b/terraform/graph_walk_context.go index 454202f5c..4ecfbbdd2 100644 --- a/terraform/graph_walk_context.go +++ b/terraform/graph_walk_context.go @@ -65,8 +65,7 @@ func (w *ContextGraphWalker) EnterPath(path addrs.ModuleInstance) EvalContext { Operation: w.Operation, State: w.Context.state, StateLock: &w.Context.stateLock, - ProviderSchemas: w.providerSchemas, - ProvidersLock: &w.providerLock, + Schemas: w.Context.schemas, VariableValues: w.variableValues, VariableValuesLock: &w.variableValuesLock, } @@ -77,9 +76,9 @@ func (w *ContextGraphWalker) EnterPath(path addrs.ModuleInstance) EvalContext { Hooks: w.Context.hooks, InputValue: w.Context.uiInput, Components: w.Context.components, + Schemas: w.Context.schemas, ProviderCache: w.providerCache, ProviderInputConfig: w.Context.providerInputConfig, - ProviderSchemas: w.providerSchemas, ProviderLock: &w.providerLock, ProvisionerCache: w.provisionerCache, ProvisionerLock: &w.provisionerLock, diff --git a/terraform/schemas.go b/terraform/schemas.go index d7294d110..8b8f3f338 100644 --- a/terraform/schemas.go +++ b/terraform/schemas.go @@ -17,11 +17,23 @@ type Schemas struct { provisioners map[string]*configschema.Block } +// ProviderSchema returns the entire ProviderSchema object that was produced +// by the plugin for the given provider, or nil if no such schema is available. +// +// It's usually better to go use the more precise methods offered by type +// Schemas to handle this detail automatically. +func (ss *Schemas) ProviderSchema(typeName string) *ProviderSchema { + if ss.providers == nil { + return nil + } + return ss.providers[typeName] +} + // ProviderConfig returns the schema for the provider configuration of the // given provider type, or nil if no such schema is available. func (ss *Schemas) ProviderConfig(typeName string) *configschema.Block { - ps, exists := ss.providers[typeName] - if !exists { + ps := ss.ProviderSchema(typeName) + if ps == nil { return nil } return ps.Provider @@ -37,12 +49,8 @@ func (ss *Schemas) ProviderConfig(typeName string) *configschema.Block { // always pass the correct provider name, even though it many cases it feels // redundant. func (ss *Schemas) ResourceTypeConfig(providerType string, resourceType string) *configschema.Block { - ps, exists := ss.providers[providerType] - if !exists { - return nil - } - - if ps.ResourceTypes == nil { + ps := ss.ProviderSchema(providerType) + if ps == nil || ps.ResourceTypes == nil { return nil } @@ -59,12 +67,8 @@ func (ss *Schemas) ResourceTypeConfig(providerType string, resourceType string) // always pass the correct provider name, even though it many cases it feels // redundant. func (ss *Schemas) DataSourceConfig(providerType string, dataSource string) *configschema.Block { - ps, exists := ss.providers[providerType] - if !exists { - return nil - } - - if ps.DataSources == nil { + ps := ss.ProviderSchema(providerType) + if ps == nil || ps.DataSources == nil { return nil } @@ -197,6 +201,7 @@ func loadProviderSchemas(schemas map[string]*ProviderSchema, config *configs.Con // There's a check deeper in Terraform that makes this a // failure when an empty/invalid provider string is present // in practice. + log.Printf("[WARN] LoadSchemas: Resource %s in %s has invalid provider address %q in its state", rsKey, moduleAddrStr, providerAddrStr) diags = diags.Append( tfdiags.SimpleWarning(fmt.Sprintf("Resource %s in %s has invalid provider address %q in its state", rsKey, moduleAddrStr, providerAddrStr)), )