From 2cc94b4e89ccd47475c2b894e7cb9bd758ae73fc Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 1 Sep 2023 15:52:31 -0700 Subject: [PATCH] core: terraform.Context with preloaded provider schemas Loading schemas from some providers can be particularly expensive, since providers for large remote platforms tend to have very large schemas. Since provider schemas are needed for many operations in Terraform, callers sometimes end up loading schemas themselves anyway. Earlier work tried to mitigate this by introducing a global schema cache for all plugin-based providers, but that's problematic because it forces only a single implementation of each distinct provider source address across the entire lifetime of a process importing package providers. This does not remove that global cache yet, but does add a new capability that will hopefully eventually supplant it: callers of terraform.NewContext can provide a set of preloaded provider schemas which they must ensure would match what Terraform Core would find if it loaded the schemas from an instance of the same provider instantiated through the corresponding factory function given alongside. A caller that wishes to avoid the potential cost of multiple schema lookups can now therefore go look up the schemas itself before calling terraform.NewContext, and provide frozen schemas that we'll use instead of fetching from the associated plugins. As of this commit no callers are actually using this mechanism. The first caller will be the "stackeval" package, which already loads provider schemas in order to evaluate provider configuration blocks anyway and so should always be able to provide a full complement of preloaded schemas to avoid Terraform Core needing to do any further lookups itself. --- internal/plugin/grpc_provider.go | 6 +++ internal/plugin6/grpc_provider.go | 6 +++ internal/providers/schema_cache.go | 7 +++ internal/terraform/context.go | 29 +++++++++++- internal/terraform/context_plugins.go | 30 ++++++++++-- internal/terraform/context_test.go | 47 +++++++++++++++++++ .../terraform/eval_context_builtin_test.go | 2 +- .../terraform/graph_builder_apply_test.go | 2 +- internal/terraform/graph_builder_plan_test.go | 8 ++-- internal/terraform/schemas_test.go | 2 +- 10 files changed, 128 insertions(+), 11 deletions(-) diff --git a/internal/plugin/grpc_provider.go b/internal/plugin/grpc_provider.go index 254ecd2fea..89b66f48df 100644 --- a/internal/plugin/grpc_provider.go +++ b/internal/plugin/grpc_provider.go @@ -79,6 +79,9 @@ func (p *GRPCProvider) GetProviderSchema() providers.GetProviderSchemaResponse { defer p.mu.Unlock() // check the global cache if we can + // FIXME: A global cache is inappropriate when Terraform Core is being + // used in a non-Terraform-CLI mode where we shouldn't assume that all + // calls share the same provider implementations. if !p.Addr.IsZero() { if resp, ok := providers.SchemaCache.Get(p.Addr); ok && resp.ServerCapabilities.GetProviderSchemaOptional { logger.Trace("GRPCProvider: returning cached schema", p.Addr.String()) @@ -144,6 +147,9 @@ func (p *GRPCProvider) GetProviderSchema() providers.GetProviderSchemaResponse { } // set the global cache if we can + // FIXME: A global cache is inappropriate when Terraform Core is being + // used in a non-Terraform-CLI mode where we shouldn't assume that all + // calls share the same provider implementations. if !p.Addr.IsZero() { providers.SchemaCache.Set(p.Addr, resp) } diff --git a/internal/plugin6/grpc_provider.go b/internal/plugin6/grpc_provider.go index 88112aecf1..70e65cd7f9 100644 --- a/internal/plugin6/grpc_provider.go +++ b/internal/plugin6/grpc_provider.go @@ -79,6 +79,9 @@ func (p *GRPCProvider) GetProviderSchema() providers.GetProviderSchemaResponse { defer p.mu.Unlock() // check the global cache if we can + // FIXME: A global cache is inappropriate when Terraform Core is being + // used in a non-Terraform-CLI mode where we shouldn't assume that all + // calls share the same provider implementations. if !p.Addr.IsZero() { if resp, ok := providers.SchemaCache.Get(p.Addr); ok && resp.ServerCapabilities.GetProviderSchemaOptional { logger.Trace("GRPCProvider.v6: returning cached schema", p.Addr.String()) @@ -144,6 +147,9 @@ func (p *GRPCProvider) GetProviderSchema() providers.GetProviderSchemaResponse { } // set the global cache if we can + // FIXME: A global cache is inappropriate when Terraform Core is being + // used in a non-Terraform-CLI mode where we shouldn't assume that all + // calls share the same provider implementations. if !p.Addr.IsZero() { providers.SchemaCache.Set(p.Addr, resp) } diff --git a/internal/providers/schema_cache.go b/internal/providers/schema_cache.go index 2b851335e3..670a0ecd21 100644 --- a/internal/providers/schema_cache.go +++ b/internal/providers/schema_cache.go @@ -12,6 +12,13 @@ import ( // SchemaCache is a global cache of Schemas. // This will be accessed by both core and the provider clients to ensure that // large schemas are stored in a single location. +// +// FIXME: A global cache is inappropriate when Terraform Core is being +// used in a non-Terraform-CLI mode where we shouldn't assume that all +// calls share the same provider implementations. This would be better +// as a per-terraform.Context cache instead, or to have callers preload +// the schemas for the providers they intend to use and pass them in +// to terraform.NewContext so we don't need to load them at runtime. var SchemaCache = &schemaCache{ m: make(map[addrs.Provider]ProviderSchema), } diff --git a/internal/terraform/context.go b/internal/terraform/context.go index b59a354e33..98176087c9 100644 --- a/internal/terraform/context.go +++ b/internal/terraform/context.go @@ -42,6 +42,23 @@ type ContextOpts struct { Providers map[addrs.Provider]providers.Factory Provisioners map[string]provisioners.Factory + // PreloadedProviderSchemas is an optional map of provider schemas that + // were already loaded from providers by the caller. This is intended + // to avoid redundant re-fetching of schemas when the caller has already + // loaded them for some other reason. + // + // The preloaded schemas do not need to be exhaustive. Terraform will + // use a preloaded schema if available, or will load a schema directly from + // a provider if no preloaded schema is available. + // + // The caller MUST ensure that the given schemas exactly match those that + // would be returned from a running provider of the given type or else the + // runtime behavior is likely to be erratic. + // + // Callers must not access (read or write) the given map once it has + // been passed to Terraform Core using this field. + PreloadedProviderSchemas map[addrs.Provider]providers.ProviderSchema + UIInput UIInput } @@ -128,7 +145,7 @@ func NewContext(opts *ContextOpts) (*Context, tfdiags.Diagnostics) { par = 10 } - plugins := newContextPlugins(opts.Providers, opts.Provisioners) + plugins := newContextPlugins(opts.Providers, opts.Provisioners, opts.PreloadedProviderSchemas) log.Printf("[TRACE] terraform.NewContext: complete") @@ -354,6 +371,16 @@ func (c *Context) checkConfigDependencies(config *configs.Config) tfdiags.Diagno } for providerAddr := range providerReqs { if !c.plugins.HasProvider(providerAddr) { + if c.plugins.HasPreloadedSchemaForProvider(providerAddr) { + // If the caller provided a preloaded schema for this provider + // then we'll take that as a hint that the caller is intending + // to handle some of these pre-validation tasks itself and + // so we'll just optimistically assume that the caller + // has arranged for this to work some other way, or will + // return its own version of this error before calling + // into here if not. + continue + } if !providerAddr.IsBuiltIn() { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, diff --git a/internal/terraform/context_plugins.go b/internal/terraform/context_plugins.go index bea67dcc38..eeda054086 100644 --- a/internal/terraform/context_plugins.go +++ b/internal/terraform/context_plugins.go @@ -20,12 +20,19 @@ import ( type contextPlugins struct { providerFactories map[addrs.Provider]providers.Factory provisionerFactories map[string]provisioners.Factory + + preloadedProviderSchemas map[addrs.Provider]providers.ProviderSchema } -func newContextPlugins(providerFactories map[addrs.Provider]providers.Factory, provisionerFactories map[string]provisioners.Factory) *contextPlugins { +func newContextPlugins( + providerFactories map[addrs.Provider]providers.Factory, + provisionerFactories map[string]provisioners.Factory, + preloadedProviderSchemas map[addrs.Provider]providers.ProviderSchema, +) *contextPlugins { ret := &contextPlugins{ - providerFactories: providerFactories, - provisionerFactories: provisionerFactories, + providerFactories: providerFactories, + provisionerFactories: provisionerFactories, + preloadedProviderSchemas: preloadedProviderSchemas, } return ret } @@ -35,6 +42,11 @@ func (cp *contextPlugins) HasProvider(addr addrs.Provider) bool { return ok } +func (cp *contextPlugins) HasPreloadedSchemaForProvider(addr addrs.Provider) bool { + _, ok := cp.preloadedProviderSchemas[addr] + return ok +} + func (cp *contextPlugins) NewProviderInstance(addr addrs.Provider) (providers.Interface, error) { f, ok := cp.providerFactories[addr] if !ok { @@ -72,11 +84,23 @@ func (cp *contextPlugins) ProviderSchema(addr addrs.Provider) (providers.Provide // This cache is only written by the provider client, and transparently // used by GetProviderSchema, but we check it here because at this point we // may be able to avoid spinning up the provider instance at all. + // We skip this if we have preloaded schemas because that suggests that + // our caller is not Terraform CLI and therefore it's probably inappropriate + // to assume that provider schemas are unique process-wide. + // + // FIXME: A global cache is inappropriate when Terraform Core is being + // used in a non-Terraform-CLI mode where we shouldn't assume that all + // calls share the same provider implementations. schemas, ok := providers.SchemaCache.Get(addr) if ok { return schemas, nil } + // We might have a non-global preloaded copy of this provider's schema. + if schema, ok := cp.preloadedProviderSchemas[addr]; ok { + return schema, nil + } + provider, err := cp.NewProviderInstance(addr) if err != nil { return schemas, fmt.Errorf("failed to instantiate provider %q to obtain schema: %s", addr, err) diff --git a/internal/terraform/context_test.go b/internal/terraform/context_test.go index 7767fe4f3c..73afc056a0 100644 --- a/internal/terraform/context_test.go +++ b/internal/terraform/context_test.go @@ -16,6 +16,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configload" "github.com/hashicorp/terraform/internal/configs/configschema" @@ -250,6 +251,52 @@ resource "implicit_thing" "b" { } } +func TestContext_preloadedProviderSchemas(t *testing.T) { + var provider *MockProvider + { + var diags tfdiags.Diagnostics + diags = diags.Append(fmt.Errorf("mustn't really call GetProviderSchema")) + provider = &MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + Diagnostics: diags, + }, + } + } + + tfCore, err := NewContext(&ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewBuiltInProvider("blep"): func() (providers.Interface, error) { + return provider, nil + }, + }, + PreloadedProviderSchemas: map[addrs.Provider]providers.ProviderSchema{ + addrs.NewBuiltInProvider("blep"): providers.ProviderSchema{}, + }, + }) + if err != nil { + t.Fatal(err) + } + + cfg := testModuleInline(t, map[string]string{ + "main.tf": ` + terraform { + required_providers { + blep = { + source = "terraform.io/builtin/blep" + } + } + } + provider "blep" {} + `, + }) + _, diags := tfCore.Schemas(cfg, states.NewState()) + assertNoDiagnostics(t, diags) + + if provider.GetProviderSchemaCalled { + t.Error("called GetProviderSchema even though a preloaded schema was provided") + } +} + func testContext2(t *testing.T, opts *ContextOpts) *Context { t.Helper() diff --git a/internal/terraform/eval_context_builtin_test.go b/internal/terraform/eval_context_builtin_test.go index 47199ae78b..99ae1b8dd0 100644 --- a/internal/terraform/eval_context_builtin_test.go +++ b/internal/terraform/eval_context_builtin_test.go @@ -66,7 +66,7 @@ func TestBuildingEvalContextInitProvider(t *testing.T) { ctx.ProviderCache = make(map[string]providers.Interface) ctx.Plugins = newContextPlugins(map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("test"): providers.FactoryFixed(testP), - }, nil) + }, nil, nil) providerAddrDefault := addrs.AbsProviderConfig{ Module: addrs.RootModule, diff --git a/internal/terraform/graph_builder_apply_test.go b/internal/terraform/graph_builder_apply_test.go index 4067b10b05..861031455c 100644 --- a/internal/terraform/graph_builder_apply_test.go +++ b/internal/terraform/graph_builder_apply_test.go @@ -732,7 +732,7 @@ func TestApplyGraphBuilder_withChecks(t *testing.T) { plugins := newContextPlugins(map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("aws"): providers.FactoryFixed(awsProvider), - }, nil) + }, nil, nil) b := &ApplyGraphBuilder{ Config: testModule(t, "apply-with-checks"), diff --git a/internal/terraform/graph_builder_plan_test.go b/internal/terraform/graph_builder_plan_test.go index 1138ea6294..14dd9af597 100644 --- a/internal/terraform/graph_builder_plan_test.go +++ b/internal/terraform/graph_builder_plan_test.go @@ -34,7 +34,7 @@ func TestPlanGraphBuilder(t *testing.T) { plugins := newContextPlugins(map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("aws"): providers.FactoryFixed(awsProvider), addrs.NewDefaultProvider("openstack"): providers.FactoryFixed(openstackProvider), - }, nil) + }, nil, nil) b := &PlanGraphBuilder{ Config: testModule(t, "graph-builder-plan-basic"), @@ -77,7 +77,7 @@ func TestPlanGraphBuilder_dynamicBlock(t *testing.T) { }) plugins := newContextPlugins(map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("test"): providers.FactoryFixed(provider), - }, nil) + }, nil, nil) b := &PlanGraphBuilder{ Config: testModule(t, "graph-builder-plan-dynblock"), @@ -133,7 +133,7 @@ func TestPlanGraphBuilder_attrAsBlocks(t *testing.T) { }) plugins := newContextPlugins(map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("test"): providers.FactoryFixed(provider), - }, nil) + }, nil, nil) b := &PlanGraphBuilder{ Config: testModule(t, "graph-builder-plan-attr-as-blocks"), @@ -198,7 +198,7 @@ func TestPlanGraphBuilder_forEach(t *testing.T) { plugins := newContextPlugins(map[addrs.Provider]providers.Factory{ addrs.NewDefaultProvider("aws"): providers.FactoryFixed(awsProvider), - }, nil) + }, nil, nil) b := &PlanGraphBuilder{ Config: testModule(t, "plan-for-each"), diff --git a/internal/terraform/schemas_test.go b/internal/terraform/schemas_test.go index cf1c27fdb9..02a4046245 100644 --- a/internal/terraform/schemas_test.go +++ b/internal/terraform/schemas_test.go @@ -46,5 +46,5 @@ func schemaOnlyProvidersForTesting(schemas map[addrs.Provider]providers.Provider } } - return newContextPlugins(factories, nil) + return newContextPlugins(factories, nil, nil) }