From 343279110a461ce455b30e291768b2522d39bbf8 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 31 Aug 2021 17:53:03 -0700 Subject: [PATCH] core: Graph walk loads plugin schemas opportunistically Previously our graph walker expected to recieve a data structure containing schemas for all of the provider and provisioner plugins used in the configuration and state. That made sense back when terraform.NewContext was responsible for loading all of the schemas before taking any other action, but it no longer has that responsiblity. Instead, we'll now make sure that the "contextPlugins" object reaches all of the locations where we need schema -- many of which already had access to that object anyway -- and then load the needed schemas just in time. The contextPlugins object memoizes schema lookups, so we can safely call it many times with the same provider address or provisioner type name and know that it'll still only load each distinct plugin once per Context object. As of this commit, the Context.Schemas method is now a public interface only and not used by logic in the "terraform" package at all. However, that does leave us in a rather tenuous situation of relying on the fact that all practical users of terraform.Context end up calling "Schemas" at some point in order to verify that we have all of the expected versions of plugins. That's a non-obvious implicit dependency, and so in subsequent commits we'll gradually move all responsibility for verifying plugin versions into the caller of terraform.NewContext, which'll heal a long-standing architectural wart whereby the caller is responsible for installing and locating the plugin executables but not for verifying that what's installed is conforming to the current configuration and dependency lock file. --- internal/backend/local/backend_plan_test.go | 2 +- internal/command/plan_test.go | 2 +- internal/terraform/context_apply.go | 7 -- internal/terraform/context_eval.go | 7 -- internal/terraform/context_import.go | 7 -- internal/terraform/context_plan.go | 19 +--- internal/terraform/context_test.go | 4 + internal/terraform/context_validate.go | 7 -- internal/terraform/context_validate_test.go | 7 +- internal/terraform/context_walk.go | 8 -- internal/terraform/evaluate.go | 21 ++-- internal/terraform/evaluate_test.go | 108 ++++++++++---------- internal/terraform/evaluate_valid.go | 13 ++- internal/terraform/evaluate_valid_test.go | 24 ++--- internal/terraform/graph_walk_context.go | 3 +- 15 files changed, 100 insertions(+), 139 deletions(-) diff --git a/internal/backend/local/backend_plan_test.go b/internal/backend/local/backend_plan_test.go index 5866048f9..73bd78df4 100644 --- a/internal/backend/local/backend_plan_test.go +++ b/internal/backend/local/backend_plan_test.go @@ -136,7 +136,7 @@ func TestLocal_plan_context_error(t *testing.T) { // the backend should be unlocked after a run assertBackendStateUnlocked(t, b) - if got, want := done(t).Stderr(), "Error: Failed to load plugin schemas"; !strings.Contains(got, want) { + if got, want := done(t).Stderr(), "failed to read schema for test_instance.foo in registry.terraform.io/hashicorp/test"; !strings.Contains(got, want) { t.Fatalf("unexpected error output:\n%s\nwant: %s", got, want) } } diff --git a/internal/command/plan_test.go b/internal/command/plan_test.go index 5638a9abf..880f2e971 100644 --- a/internal/command/plan_test.go +++ b/internal/command/plan_test.go @@ -1051,7 +1051,7 @@ func TestPlan_init_required(t *testing.T) { t.Fatalf("expected error, got success") } got := output.Stderr() - if !strings.Contains(got, `Please run "terraform init".`) { + if !strings.Contains(got, `failed to read schema for test_instance.foo in registry.terraform.io/hashicorp/test`) { t.Fatal("wrong error message in output:", got) } } diff --git a/internal/terraform/context_apply.go b/internal/terraform/context_apply.go index ff32074ec..e94449b72 100644 --- a/internal/terraform/context_apply.go +++ b/internal/terraform/context_apply.go @@ -24,12 +24,6 @@ func (c *Context) Apply(plan *plans.Plan, config *configs.Config) (*states.State defer c.acquireRun("apply")() var diags tfdiags.Diagnostics - schemas, moreDiags := c.Schemas(config, plan.PriorState) - diags = diags.Append(moreDiags) - if moreDiags.HasErrors() { - return nil, diags - } - log.Printf("[DEBUG] Building and walking apply graph for %s plan", plan.UIMode) graph, operation, moreDiags := c.applyGraph(plan, config, true) @@ -58,7 +52,6 @@ func (c *Context) Apply(plan *plans.Plan, config *configs.Config) (*states.State workingState := plan.PriorState.DeepCopy() walker, walkDiags := c.walk(graph, operation, &graphWalkOpts{ Config: config, - Schemas: schemas, InputState: workingState, Changes: plan.Changes, RootVariableValues: variables, diff --git a/internal/terraform/context_eval.go b/internal/terraform/context_eval.go index 8201db4d6..efc24767c 100644 --- a/internal/terraform/context_eval.go +++ b/internal/terraform/context_eval.go @@ -40,12 +40,6 @@ func (c *Context) Eval(config *configs.Config, state *states.State, moduleAddr a var diags tfdiags.Diagnostics defer c.acquireRun("eval")() - schemas, moreDiags := c.Schemas(config, state) - diags = diags.Append(moreDiags) - if moreDiags.HasErrors() { - return nil, diags - } - // Start with a copy of state so that we don't affect the instance that // the caller is holding. state = state.DeepCopy() @@ -78,7 +72,6 @@ func (c *Context) Eval(config *configs.Config, state *states.State, moduleAddr a walkOpts := &graphWalkOpts{ InputState: state, Config: config, - Schemas: schemas, RootVariableValues: variables, } diff --git a/internal/terraform/context_import.go b/internal/terraform/context_import.go index f4bc6d809..af17cbd62 100644 --- a/internal/terraform/context_import.go +++ b/internal/terraform/context_import.go @@ -48,12 +48,6 @@ func (c *Context) Import(config *configs.Config, prevRunState *states.State, opt // Hold a lock since we can modify our own state here defer c.acquireRun("import")() - schemas, moreDiags := c.Schemas(config, prevRunState) - diags = diags.Append(moreDiags) - if moreDiags.HasErrors() { - return nil, diags - } - // Don't modify our caller's state state := prevRunState.DeepCopy() @@ -78,7 +72,6 @@ func (c *Context) Import(config *configs.Config, prevRunState *states.State, opt // Walk it walker, walkDiags := c.walk(graph, walkImport, &graphWalkOpts{ Config: config, - Schemas: schemas, InputState: state, RootVariableValues: variables, }) diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 208ee771c..248673254 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -324,16 +324,10 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, r var diags tfdiags.Diagnostics log.Printf("[DEBUG] Building and walking plan graph for %s", opts.Mode) - schemas, moreDiags := c.Schemas(config, prevRunState) - diags = diags.Append(moreDiags) - if diags.HasErrors() { - return nil, diags - } - prevRunState = prevRunState.DeepCopy() // don't modify the caller's object when we process the moves moveStmts, moveResults := c.prePlanFindAndApplyMoves(config, prevRunState, opts.Targets) - graph, walkOp, moreDiags := c.planGraph(config, prevRunState, opts, schemas, true) + graph, walkOp, moreDiags := c.planGraph(config, prevRunState, opts, true) diags = diags.Append(moreDiags) if diags.HasErrors() { return nil, diags @@ -344,7 +338,6 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, r changes := plans.NewChanges() walker, walkDiags := c.walk(graph, walkOp, &graphWalkOpts{ Config: config, - Schemas: schemas, InputState: prevRunState, Changes: changes, MoveResults: moveResults, @@ -365,7 +358,7 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, r return plan, diags } -func (c *Context) planGraph(config *configs.Config, prevRunState *states.State, opts *PlanOpts, schemas *Schemas, validate bool) (*Graph, walkOperation, tfdiags.Diagnostics) { +func (c *Context) planGraph(config *configs.Config, prevRunState *states.State, opts *PlanOpts, validate bool) (*Graph, walkOperation, tfdiags.Diagnostics) { switch mode := opts.Mode; mode { case plans.NormalMode: graph, diags := (&PlanGraphBuilder{ @@ -421,13 +414,7 @@ func (c *Context) PlanGraphForUI(config *configs.Config, prevRunState *states.St opts := &PlanOpts{Mode: mode} - schemas, moreDiags := c.Schemas(config, prevRunState) - diags = diags.Append(moreDiags) - if diags.HasErrors() { - return nil, diags - } - - graph, _, moreDiags := c.planGraph(config, prevRunState, opts, schemas, false) + graph, _, moreDiags := c.planGraph(config, prevRunState, opts, false) diags = diags.Append(moreDiags) return graph, diags } diff --git a/internal/terraform/context_test.go b/internal/terraform/context_test.go index 47fd32f4b..02addb4db 100644 --- a/internal/terraform/context_test.go +++ b/internal/terraform/context_test.go @@ -123,6 +123,10 @@ func TestNewContextRequiredVersion(t *testing.T) { } func TestNewContext_lockedDependencies(t *testing.T) { + // TODO: Remove this test altogether once we've factored out the version + // and checksum verification to be exclusively the caller's responsibility. + t.Skip("only one step away from locked dependencies being the caller's responsibility") + configBeepGreaterThanOne := ` terraform { required_providers { diff --git a/internal/terraform/context_validate.go b/internal/terraform/context_validate.go index f5aa8540c..b079477ba 100644 --- a/internal/terraform/context_validate.go +++ b/internal/terraform/context_validate.go @@ -35,12 +35,6 @@ func (c *Context) Validate(config *configs.Config) tfdiags.Diagnostics { return diags } - schemas, moreDiags := c.Schemas(config, nil) - diags = diags.Append(moreDiags) - if moreDiags.HasErrors() { - return diags - } - log.Printf("[DEBUG] Building and walking validate graph") graph, moreDiags := ValidateGraphBuilder(&PlanGraphBuilder{ @@ -74,7 +68,6 @@ func (c *Context) Validate(config *configs.Config) tfdiags.Diagnostics { walker, walkDiags := c.walk(graph, walkValidate, &graphWalkOpts{ Config: config, - Schemas: schemas, RootVariableValues: varValues, }) diags = diags.Append(walker.NonFatalDiagnostics) diff --git a/internal/terraform/context_validate_test.go b/internal/terraform/context_validate_test.go index 0c5a2a3a5..4f628945e 100644 --- a/internal/terraform/context_validate_test.go +++ b/internal/terraform/context_validate_test.go @@ -1193,10 +1193,6 @@ func TestContext2Validate_PlanGraphBuilder(t *testing.T) { opts := fixture.ContextOpts() c := testContext2(t, opts) - state := states.NewState() - schemas, diags := c.Schemas(fixture.Config, state) - assertNoDiagnostics(t, diags) - graph, diags := ValidateGraphBuilder(&PlanGraphBuilder{ Config: fixture.Config, State: states.NewState(), @@ -1207,8 +1203,7 @@ func TestContext2Validate_PlanGraphBuilder(t *testing.T) { } defer c.acquireRun("validate-test")() walker, diags := c.walk(graph, walkValidate, &graphWalkOpts{ - Config: fixture.Config, - Schemas: schemas, + Config: fixture.Config, }) if diags.HasErrors() { t.Fatal(diags.Err()) diff --git a/internal/terraform/context_walk.go b/internal/terraform/context_walk.go index b44a5910c..e041f80b2 100644 --- a/internal/terraform/context_walk.go +++ b/internal/terraform/context_walk.go @@ -23,7 +23,6 @@ type graphWalkOpts struct { InputState *states.State Changes *plans.Changes Config *configs.Config - Schemas *Schemas RootVariableValues InputValues MoveResults map[addrs.UniqueKey]refactoring.MoveResult @@ -95,12 +94,6 @@ func (c *Context) graphWalker(operation walkOperation, opts *graphWalkOpts) *Con changes = plans.NewChanges() } - if opts.Schemas == nil { - // Should never happen: caller must always set this one. - // (We catch this here, rather than later, to get a more intelligible - // stack trace when it _does_ panic.) - panic("Context.graphWalker call without Schemas") - } if opts.Config == nil { panic("Context.graphWalker call without Config") } @@ -109,7 +102,6 @@ func (c *Context) graphWalker(operation walkOperation, opts *graphWalkOpts) *Con Context: c, State: state, Config: opts.Config, - Schemas: opts.Schemas, RefreshState: refreshState, PrevRunState: prevRunState, Changes: changes.SyncWrapper(), diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index efcc9c1f4..47889a9a4 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -46,13 +46,13 @@ type Evaluator struct { VariableValues map[string]map[string]cty.Value VariableValuesLock *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. + // Plugins is the library of available plugin components (providers and + // provisioners) that we have available to help us evaluate expressions + // that interact with plugin-provided objects. // - // This must not be mutated during evaluation. - Schemas *Schemas + // From this we only access the schemas of the plugins, and don't otherwise + // interact with plugin instances. + Plugins *contextPlugins // State is the current state, embedded in a wrapper that ensures that // it can be safely accessed and modified concurrently. @@ -892,8 +892,13 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc } func (d *evaluationStateData) getResourceSchema(addr addrs.Resource, providerAddr addrs.AbsProviderConfig) *configschema.Block { - schemas := d.Evaluator.Schemas - schema, _ := schemas.ResourceTypeConfig(providerAddr.Provider, addr.Mode, addr.Type) + schema, _, err := d.Evaluator.Plugins.ResourceTypeSchema(providerAddr.Provider, addr.Mode, addr.Type) + if err != nil { + // We have plently other codepaths that will detect and report + // schema lookup errors before we'd reach this point, so we'll just + // treat a failure here the same as having no schema. + return nil + } return schema } diff --git a/internal/terraform/evaluate_test.go b/internal/terraform/evaluate_test.go index f8a46d4fc..ffd067f3f 100644 --- a/internal/terraform/evaluate_test.go +++ b/internal/terraform/evaluate_test.go @@ -193,79 +193,77 @@ func TestEvaluatorGetResource(t *testing.T) { }, }, State: stateSync, - Schemas: &Schemas{ - Providers: map[addrs.Provider]*ProviderSchema{ - addrs.NewDefaultProvider("test"): { - Provider: &configschema.Block{}, - ResourceTypes: map[string]*configschema.Block{ - "test_resource": { - Attributes: map[string]*configschema.Attribute{ - "id": { - Type: cty.String, - Computed: true, - }, - "value": { - Type: cty.String, - Computed: true, - Sensitive: true, - }, + Plugins: schemaOnlyProvidersForTesting(map[addrs.Provider]*ProviderSchema{ + addrs.NewDefaultProvider("test"): { + Provider: &configschema.Block{}, + ResourceTypes: map[string]*configschema.Block{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, }, - BlockTypes: map[string]*configschema.NestedBlock{ - "nesting_list": { - Block: configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "value": {Type: cty.String, Optional: true}, - "sensitive_value": {Type: cty.String, Optional: true, Sensitive: true}, - }, + "value": { + Type: cty.String, + Computed: true, + Sensitive: true, + }, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "nesting_list": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": {Type: cty.String, Optional: true}, + "sensitive_value": {Type: cty.String, Optional: true, Sensitive: true}, }, - Nesting: configschema.NestingList, }, - "nesting_map": { - Block: configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "foo": {Type: cty.String, Optional: true, Sensitive: true}, - }, + Nesting: configschema.NestingList, + }, + "nesting_map": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "foo": {Type: cty.String, Optional: true, Sensitive: true}, }, - Nesting: configschema.NestingMap, }, - "nesting_set": { - Block: configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "baz": {Type: cty.String, Optional: true, Sensitive: true}, - }, + Nesting: configschema.NestingMap, + }, + "nesting_set": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "baz": {Type: cty.String, Optional: true, Sensitive: true}, }, - Nesting: configschema.NestingSet, }, - "nesting_single": { - Block: configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "boop": {Type: cty.String, Optional: true, Sensitive: true}, - }, + Nesting: configschema.NestingSet, + }, + "nesting_single": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "boop": {Type: cty.String, Optional: true, Sensitive: true}, }, - Nesting: configschema.NestingSingle, }, - "nesting_nesting": { - Block: configschema.Block{ - BlockTypes: map[string]*configschema.NestedBlock{ - "nesting_list": { - Block: configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "value": {Type: cty.String, Optional: true}, - "sensitive_value": {Type: cty.String, Optional: true, Sensitive: true}, - }, + Nesting: configschema.NestingSingle, + }, + "nesting_nesting": { + Block: configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "nesting_list": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": {Type: cty.String, Optional: true}, + "sensitive_value": {Type: cty.String, Optional: true, Sensitive: true}, }, - Nesting: configschema.NestingList, }, + Nesting: configschema.NestingList, }, }, - Nesting: configschema.NestingSingle, }, + Nesting: configschema.NestingSingle, }, }, }, }, }, - }, + }), } data := &evaluationStateData{ @@ -430,7 +428,7 @@ func TestEvaluatorGetResource_changes(t *testing.T) { }, }, State: stateSync, - Schemas: schemas, + Plugins: schemaOnlyProvidersForTesting(schemas.Providers), } data := &evaluationStateData{ diff --git a/internal/terraform/evaluate_valid.go b/internal/terraform/evaluate_valid.go index 68943a842..232f6913d 100644 --- a/internal/terraform/evaluate_valid.go +++ b/internal/terraform/evaluate_valid.go @@ -224,7 +224,18 @@ func (d *evaluationStateData) staticValidateResourceReference(modCfg *configs.Co } providerFqn := modCfg.Module.ProviderForLocalConfig(cfg.ProviderConfigAddr()) - schema, _ := d.Evaluator.Schemas.ResourceTypeConfig(providerFqn, addr.Mode, addr.Type) + schema, _, err := d.Evaluator.Plugins.ResourceTypeSchema(providerFqn, addr.Mode, addr.Type) + if err != nil { + // Prior validation should've taken care of a schema lookup error, + // so we should never get here but we'll handle it here anyway for + // robustness. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Failed provider schema lookup`, + Detail: fmt.Sprintf(`Couldn't load schema for %s resource type %q in %s: %s.`, modeAdjective, addr.Type, providerFqn.String(), err), + Subject: rng.ToHCL().Ptr(), + }) + } if schema == nil { // Prior validation should've taken care of a resource block with an diff --git a/internal/terraform/evaluate_valid_test.go b/internal/terraform/evaluate_valid_test.go index ff8ca4397..cfdfdea1f 100644 --- a/internal/terraform/evaluate_valid_test.go +++ b/internal/terraform/evaluate_valid_test.go @@ -69,21 +69,19 @@ For example, to correlate with indices of a referring resource, use: cfg := testModule(t, "static-validate-refs") evaluator := &Evaluator{ Config: cfg, - Schemas: &Schemas{ - Providers: map[addrs.Provider]*ProviderSchema{ - addrs.NewDefaultProvider("aws"): { - ResourceTypes: map[string]*configschema.Block{ - "aws_instance": {}, - }, - }, - addrs.MustParseProviderSourceString("foobar/beep"): { - ResourceTypes: map[string]*configschema.Block{ - // intentional mismatch between resource type prefix and provider type - "boop_instance": {}, - }, + Plugins: schemaOnlyProvidersForTesting(map[addrs.Provider]*ProviderSchema{ + addrs.NewDefaultProvider("aws"): { + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": {}, }, }, - }, + addrs.MustParseProviderSourceString("foobar/beep"): { + ResourceTypes: map[string]*configschema.Block{ + // intentional mismatch between resource type prefix and provider type + "boop_instance": {}, + }, + }, + }), } for _, test := range tests { diff --git a/internal/terraform/graph_walk_context.go b/internal/terraform/graph_walk_context.go index 6fd790e92..39a97032c 100644 --- a/internal/terraform/graph_walk_context.go +++ b/internal/terraform/graph_walk_context.go @@ -34,7 +34,6 @@ type ContextGraphWalker struct { Operation walkOperation StopContext context.Context RootVariableValues InputValues - Schemas *Schemas Config *configs.Config // This is an output. Do not set this, nor read it while a graph walk @@ -81,7 +80,7 @@ func (w *ContextGraphWalker) EvalContext() EvalContext { Operation: w.Operation, State: w.State, Changes: w.Changes, - Schemas: w.Schemas, + Plugins: w.Context.plugins, VariableValues: w.variableValues, VariableValuesLock: &w.variableValuesLock, }