From 86bf674246540cec5d9b82271ae14c16e745c1b8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 23 Aug 2019 19:20:47 -0400 Subject: [PATCH 1/6] change GetResourceInstance to GetResource In order to allow lazy evaluation of resource indexes, we can't index resources immediately via GetResourceInstance. Change the evaluation to always return whole Resources via GetResource, and index individual instances during expression evaluation. This will allow us to always check for invalid index errors rather than returning an unknown value and ignoring it during apply. --- configs/configupgrade/analysis_expr.go | 25 ++--- lang/data.go | 2 +- lang/eval.go | 23 ++-- terraform/context_apply_test.go | 41 +++++++ terraform/evaluate.go | 101 ++++-------------- .../testdata/apply-invalid-index/main.tf | 7 ++ 6 files changed, 83 insertions(+), 116 deletions(-) create mode 100644 terraform/testdata/apply-invalid-index/main.tf diff --git a/configs/configupgrade/analysis_expr.go b/configs/configupgrade/analysis_expr.go index 4b69714e25c9..64e5ea1c9a4c 100644 --- a/configs/configupgrade/analysis_expr.go +++ b/configs/configupgrade/analysis_expr.go @@ -75,28 +75,27 @@ func (d analysisData) GetForEachAttr(addr addrs.ForEachAttr, rng tfdiags.SourceR return cty.DynamicVal, nil } -func (d analysisData) GetResourceInstance(instAddr addrs.ResourceInstance, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { - log.Printf("[TRACE] configupgrade: Determining type for %s", instAddr) - addr := instAddr.Resource +func (d analysisData) GetResource(addr addrs.Resource, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { + log.Printf("[TRACE] configupgrade: Determining type for %s", addr) // Our analysis pass should've found a suitable schema for every resource // type in the module. providerType, ok := d.an.ResourceProviderType[addr] if !ok { // Should not be possible, since analysis visits every resource block. - log.Printf("[TRACE] configupgrade: analysis.GetResourceInstance doesn't have a provider type for %s", addr) + log.Printf("[TRACE] configupgrade: analysis.GetResource doesn't have a provider type for %s", addr) return cty.DynamicVal, nil } providerSchema, ok := d.an.ProviderSchemas[providerType] if !ok { // Should not be possible, since analysis loads schema for every provider. - log.Printf("[TRACE] configupgrade: analysis.GetResourceInstance doesn't have a provider schema for for %q", providerType) + log.Printf("[TRACE] configupgrade: analysis.GetResource doesn't have a provider schema for for %q", providerType) return cty.DynamicVal, nil } schema, _ := providerSchema.SchemaForResourceAddr(addr) if schema == nil { // Should not be possible, since analysis loads schema for every provider. - log.Printf("[TRACE] configupgrade: analysis.GetResourceInstance doesn't have a schema for for %s", addr) + log.Printf("[TRACE] configupgrade: analysis.GetResource doesn't have a schema for for %s", addr) return cty.DynamicVal, nil } @@ -106,19 +105,11 @@ func (d analysisData) GetResourceInstance(instAddr addrs.ResourceInstance, rng t // return a list or a single object type depending on whether count is // set and whether an instance key is given in the address. if d.an.ResourceHasCount[addr] { - if instAddr.Key == addrs.NoKey { - log.Printf("[TRACE] configupgrade: %s refers to counted instance without a key, so result is a list of %#v", instAddr, objTy) - return cty.UnknownVal(cty.List(objTy)), nil - } - log.Printf("[TRACE] configupgrade: %s refers to counted instance with a key, so result is single object", instAddr) - return cty.UnknownVal(objTy), nil + log.Printf("[TRACE] configupgrade: %s refers to counted instance, so result is a list of %#v", addr, objTy) + return cty.UnknownVal(cty.List(objTy)), nil } - if instAddr.Key != addrs.NoKey { - log.Printf("[TRACE] configupgrade: %s refers to non-counted instance with a key, which is invalid", instAddr) - return cty.DynamicVal, nil - } - log.Printf("[TRACE] configupgrade: %s refers to non-counted instance without a key, so result is single object", instAddr) + log.Printf("[TRACE] configupgrade: %s refers to non-counted instance, so result is single object", addr) return cty.UnknownVal(objTy), nil } diff --git a/lang/data.go b/lang/data.go index eca588ecba04..ebc008e35d27 100644 --- a/lang/data.go +++ b/lang/data.go @@ -24,7 +24,7 @@ type Data interface { GetCountAttr(addrs.CountAttr, tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) GetForEachAttr(addrs.ForEachAttr, tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) - GetResourceInstance(addrs.ResourceInstance, tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) + GetResource(addrs.Resource, tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) GetLocalValue(addrs.LocalValue, tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) GetModuleInstance(addrs.ModuleCallInstance, tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) GetModuleInstanceOutput(addrs.ModuleCallOutput, tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) diff --git a/lang/eval.go b/lang/eval.go index a8fe8b662de3..d4c24e7d6231 100644 --- a/lang/eval.go +++ b/lang/eval.go @@ -194,8 +194,8 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl // it, since that allows us to gather a full set of any errors and // warnings, but once we've gathered all the data we'll then skip anything // that's redundant in the process of populating our values map. - dataResources := map[string]map[string]map[addrs.InstanceKey]cty.Value{} - managedResources := map[string]map[string]map[addrs.InstanceKey]cty.Value{} + dataResources := map[string]map[string]cty.Value{} + managedResources := map[string]map[string]cty.Value{} wholeModules := map[string]map[addrs.InstanceKey]cty.Value{} moduleOutputs := map[string]map[addrs.InstanceKey]map[string]cty.Value{} inputVariables := map[string]cty.Value{} @@ -241,7 +241,7 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl switch subj := rawSubj.(type) { case addrs.ResourceInstance: - var into map[string]map[string]map[addrs.InstanceKey]cty.Value + var into map[string]map[string]cty.Value switch subj.Resource.Mode { case addrs.ManagedResourceMode: into = managedResources @@ -251,17 +251,14 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl panic(fmt.Errorf("unsupported ResourceMode %s", subj.Resource.Mode)) } - val, valDiags := normalizeRefValue(s.Data.GetResourceInstance(subj, rng)) + val, valDiags := normalizeRefValue(s.Data.GetResource(subj.ContainingResource(), rng)) diags = diags.Append(valDiags) r := subj.Resource if into[r.Type] == nil { - into[r.Type] = make(map[string]map[addrs.InstanceKey]cty.Value) + into[r.Type] = make(map[string]cty.Value) } - if into[r.Type][r.Name] == nil { - into[r.Type][r.Name] = make(map[addrs.InstanceKey]cty.Value) - } - into[r.Type][r.Name][subj.Key] = val + into[r.Type][r.Name] = val if isSelf { self = val } @@ -367,13 +364,9 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl return ctx, diags } -func buildResourceObjects(resources map[string]map[string]map[addrs.InstanceKey]cty.Value) map[string]cty.Value { +func buildResourceObjects(resources map[string]map[string]cty.Value) map[string]cty.Value { vals := make(map[string]cty.Value) - for typeName, names := range resources { - nameVals := make(map[string]cty.Value) - for name, keys := range names { - nameVals[name] = buildInstanceObjects(keys) - } + for typeName, nameVals := range resources { vals[typeName] = cty.ObjectVal(nameVals) } return vals diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 39e4ef175230..6f8cf50bbec2 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -10527,3 +10527,44 @@ func TestContext2Apply_issue19908(t *testing.T) { t.Errorf("test.foo attributes JSON doesn't contain %s after apply\ngot: %s", want, got) } } + +func TestContext2Apply_invalidIndexRef(t *testing.T) { + p := testProvider("test") + p.GetSchemaReturn = &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_instance": { + Attributes: map[string]*configschema.Attribute{ + "value": {Type: cty.String, Optional: true, Computed: true}, + }, + }, + }, + } + p.DiffFn = testDiffFn + + m := testModule(t, "apply-invalid-index") + c := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "test": testProviderFuncFixed(p), + }, + ), + }) + + diags := c.Validate() + if diags.HasErrors() { + t.Fatalf("unexpected validation failure: %s", diags.Err()) + } + + wantErr := `The given key does not identify an element in this collection value` + _, diags = c.Plan() + + if !diags.HasErrors() { + t.Fatalf("plan succeeded; want error") + } + gotErr := diags.Err().Error() + + if !strings.Contains(gotErr, wantErr) { + t.Fatalf("missing expected error\ngot: %s\n\nwant: error containing %q", gotErr, wantErr) + } +} diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 9bb600945f97..9dae0a43376c 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -507,14 +507,8 @@ func (d *evaluationStateData) GetPathAttr(addr addrs.PathAttr, rng tfdiags.Sourc } } -func (d *evaluationStateData) GetResourceInstance(addr addrs.ResourceInstance, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { +func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - - // Although we are giving a ResourceInstance address here, if it has - // a key of addrs.NoKey then it might actually be a request for all of - // the instances of a particular resource. The reference resolver can't - // resolve the ambiguity itself, so we must do it in here. - // First we'll consult the configuration to see if an resource of this // name is declared at all. moduleAddr := d.ModulePath @@ -525,36 +519,21 @@ func (d *evaluationStateData) GetResourceInstance(addr addrs.ResourceInstance, r panic(fmt.Sprintf("resource value read from %s, which has no configuration", moduleAddr)) } - config := moduleConfig.Module.ResourceByAddr(addr.ContainingResource()) + config := moduleConfig.Module.ResourceByAddr(addr) if config == nil { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: `Reference to undeclared resource`, - Detail: fmt.Sprintf(`A resource %q %q has not been declared in %s`, addr.Resource.Type, addr.Resource.Name, moduleDisplayAddr(moduleAddr)), + Detail: fmt.Sprintf(`A resource %q %q has not been declared in %s`, addr.Type, addr.Name, moduleDisplayAddr(moduleAddr)), Subject: rng.ToHCL().Ptr(), }) return cty.DynamicVal, diags } - // First we'll find the state for the resource as a whole, and decide - // from there whether we're going to interpret the given address as a - // resource or a resource instance address. - rs := d.Evaluator.State.Resource(addr.ContainingResource().Absolute(d.ModulePath)) + rs := d.Evaluator.State.Resource(addr.Absolute(d.ModulePath)) if rs == nil { - schema := d.getResourceSchema(addr.ContainingResource(), config.ProviderConfigAddr().Absolute(d.ModulePath)) - - // If it doesn't exist at all then we can't reliably determine whether - // single-instance or whole-resource interpretation was intended, but - // we can decide this partially... - if addr.Key != addrs.NoKey { - // If there's an instance key then the user must be intending - // single-instance interpretation, and so we can return a - // properly-typed unknown value to help with type checking. - return cty.UnknownVal(schema.ImpliedType()), diags - } - - // otherwise we must return DynamicVal so that both interpretations + // we must return DynamicVal so that both interpretations // can proceed without generating errors, and we'll deal with this // in a later step where more information is gathered. // (In practice we should only end up here during the validate walk, @@ -569,69 +548,25 @@ func (d *evaluationStateData) GetResourceInstance(addr addrs.ResourceInstance, r return cty.DynamicVal, diags } - schema := d.getResourceSchema(addr.ContainingResource(), rs.ProviderConfig) - - // If we are able to automatically convert to the "right" type of instance - // key for this each mode then we'll do so, to match with how we generally - // treat values elsewhere in the language. This allows code below to - // assume that any possible conversions have already been dealt with and - // just worry about validation. - key := d.coerceInstanceKey(addr.Key, rs.EachMode) - - multi := false - switch rs.EachMode { case states.NoEach: - if key != addrs.NoKey { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid resource index", - Detail: fmt.Sprintf("Resource %s does not have either \"count\" or \"for_each\" set, so it cannot be indexed.", addr.ContainingResource()), - Subject: rng.ToHCL().Ptr(), - }) - return cty.DynamicVal, diags - } - case states.EachList: - multi = key == addrs.NoKey - if _, ok := addr.Key.(addrs.IntKey); !multi && !ok { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid resource index", - Detail: fmt.Sprintf("Resource %s must be indexed with a number value.", addr.ContainingResource()), - Subject: rng.ToHCL().Ptr(), - }) - return cty.DynamicVal, diags - } - case states.EachMap: - multi = key == addrs.NoKey - if _, ok := addr.Key.(addrs.StringKey); !multi && !ok { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid resource index", - Detail: fmt.Sprintf("Resource %s must be indexed with a string value.", addr.ContainingResource()), - Subject: rng.ToHCL().Ptr(), - }) - return cty.DynamicVal, diags - } - } - - if !multi { - log.Printf("[TRACE] GetResourceInstance: %s is a single instance", addr) - is := rs.Instance(key) - if is == nil { - return cty.UnknownVal(schema.ImpliedType()), diags - } - return d.getResourceInstanceSingle(addr, rng, is, config, rs.ProviderConfig) + log.Printf("[TRACE] GetResource: %s is a single instance", addr) + return d.getResourceInstanceSingle(addr, rng, rs, config, rs.ProviderConfig) + case states.EachList, states.EachMap: + log.Printf("[TRACE] GetResource: %s has multiple keyed instances", addr) + return d.getResourceInstancesAll(addr, rng, config, rs, rs.ProviderConfig) + default: + panic("invalid EachMode") } - - log.Printf("[TRACE] GetResourceInstance: %s has multiple keyed instances", addr) - return d.getResourceInstancesAll(addr.ContainingResource(), rng, config, rs, rs.ProviderConfig) } -func (d *evaluationStateData) getResourceInstanceSingle(addr addrs.ResourceInstance, rng tfdiags.SourceRange, is *states.ResourceInstance, config *configs.Resource, providerAddr addrs.AbsProviderConfig) (cty.Value, tfdiags.Diagnostics) { +func (d *evaluationStateData) getResourceInstanceSingle(addr addrs.Resource, rng tfdiags.SourceRange, rs *states.Resource, config *configs.Resource, providerAddr addrs.AbsProviderConfig) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - schema := d.getResourceSchema(addr.ContainingResource(), providerAddr) + instAddr := addrs.ResourceInstance{Resource: addr, Key: addrs.NoKey} + is := rs.Instances[addrs.NoKey] + + schema := d.getResourceSchema(addr, providerAddr) if schema == nil { // This shouldn't happen, since validation before we get here should've // taken care of it, but we'll show a reasonable error message anyway. @@ -654,7 +589,7 @@ func (d *evaluationStateData) getResourceInstanceSingle(addr addrs.ResourceInsta // If there's a pending change for this instance in our plan, we'll prefer // that. This is important because the state can't represent unknown values // and so its data is inaccurate when changes are pending. - if change := d.Evaluator.Changes.GetResourceInstanceChange(addr.Absolute(d.ModulePath), states.CurrentGen); change != nil { + if change := d.Evaluator.Changes.GetResourceInstanceChange(instAddr.Absolute(d.ModulePath), states.CurrentGen); change != nil { val, err := change.After.Decode(ty) if err != nil { diags = diags.Append(&hcl.Diagnostic{ diff --git a/terraform/testdata/apply-invalid-index/main.tf b/terraform/testdata/apply-invalid-index/main.tf new file mode 100644 index 000000000000..8ea02d77384e --- /dev/null +++ b/terraform/testdata/apply-invalid-index/main.tf @@ -0,0 +1,7 @@ +resource "test_instance" "a" { + count = 0 +} + +resource "test_instance" "b" { + value = test_instance.a[0].value +} From da8ec6b483ce25cc543d59b7bc7a1e9d94a91846 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 19 Sep 2019 09:08:17 -0400 Subject: [PATCH 2/6] change state eval GetResource to return all Always return the entire resource object from evaluationStateData.GetResource, rather than parsing the references for individual instances. This allows for later evaluation of resource indexes so we can return errors when they don't exist, and prevent errors when short-circuiting around invalid indexes in conditionals. --- terraform/evaluate.go | 109 ++++++++++++++++-------------------------- 1 file changed, 41 insertions(+), 68 deletions(-) diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 9dae0a43376c..18b5dce99f6e 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -2,7 +2,6 @@ package terraform import ( "fmt" - "log" "os" "path/filepath" "strconv" @@ -548,23 +547,13 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc return cty.DynamicVal, diags } - switch rs.EachMode { - case states.NoEach: - log.Printf("[TRACE] GetResource: %s is a single instance", addr) - return d.getResourceInstanceSingle(addr, rng, rs, config, rs.ProviderConfig) - case states.EachList, states.EachMap: - log.Printf("[TRACE] GetResource: %s has multiple keyed instances", addr) - return d.getResourceInstancesAll(addr, rng, config, rs, rs.ProviderConfig) - default: - panic("invalid EachMode") - } + return d.getResourceInstancesAll(addr, rng, config, rs, rs.ProviderConfig) } -func (d *evaluationStateData) getResourceInstanceSingle(addr addrs.Resource, rng tfdiags.SourceRange, rs *states.Resource, config *configs.Resource, providerAddr addrs.AbsProviderConfig) (cty.Value, tfdiags.Diagnostics) { +func (d *evaluationStateData) getResourceInstancesAll(addr addrs.Resource, rng tfdiags.SourceRange, config *configs.Resource, rs *states.Resource, providerAddr addrs.AbsProviderConfig) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics instAddr := addrs.ResourceInstance{Resource: addr, Key: addrs.NoKey} - is := rs.Instances[addrs.NoKey] schema := d.getResourceSchema(addr, providerAddr) if schema == nil { @@ -579,75 +568,59 @@ func (d *evaluationStateData) getResourceInstanceSingle(addr addrs.Resource, rng return cty.DynamicVal, diags } - ty := schema.ImpliedType() - if is == nil || is.Current == nil { - // Assume we're dealing with an instance that hasn't been created yet. - return cty.UnknownVal(ty), diags - } + switch rs.EachMode { + case states.NoEach: + ty := schema.ImpliedType() + is := rs.Instances[addrs.NoKey] + if is == nil || is.Current == nil { + // Assume we're dealing with an instance that hasn't been created yet. + return cty.UnknownVal(ty), diags + } - if is.Current.Status == states.ObjectPlanned { - // If there's a pending change for this instance in our plan, we'll prefer - // that. This is important because the state can't represent unknown values - // and so its data is inaccurate when changes are pending. - if change := d.Evaluator.Changes.GetResourceInstanceChange(instAddr.Absolute(d.ModulePath), states.CurrentGen); change != nil { - val, err := change.After.Decode(ty) - if err != nil { + if is.Current.Status == states.ObjectPlanned { + // If there's a pending change for this instance in our plan, we'll prefer + // that. This is important because the state can't represent unknown values + // and so its data is inaccurate when changes are pending. + if change := d.Evaluator.Changes.GetResourceInstanceChange(instAddr.Absolute(d.ModulePath), states.CurrentGen); change != nil { + val, err := change.After.Decode(ty) + if err != nil { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid resource instance data in plan", + Detail: fmt.Sprintf("Instance %s data could not be decoded from the plan: %s.", addr.Absolute(d.ModulePath), err), + Subject: &config.DeclRange, + }) + return cty.UnknownVal(ty), diags + } + return val, diags + } else { + // If the object is in planned status then we should not + // get here, since we should've found a pending value + // in the plan above instead. diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: "Invalid resource instance data in plan", - Detail: fmt.Sprintf("Instance %s data could not be decoded from the plan: %s.", addr.Absolute(d.ModulePath), err), + Summary: "Missing pending object in plan", + Detail: fmt.Sprintf("Instance %s is marked as having a change pending but that change is not recorded in the plan. This is a bug in Terraform; please report it.", addr), Subject: &config.DeclRange, }) return cty.UnknownVal(ty), diags } - return val, diags - } else { - // If the object is in planned status then we should not - // get here, since we should've found a pending value - // in the plan above instead. + } + + ios, err := is.Current.Decode(ty) + if err != nil { + // This shouldn't happen, since by the time we get here + // we should've upgraded the state data already. diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: "Missing pending object in plan", - Detail: fmt.Sprintf("Instance %s is marked as having a change pending but that change is not recorded in the plan. This is a bug in Terraform; please report it.", addr), + Summary: "Invalid resource instance data in state", + Detail: fmt.Sprintf("Instance %s data could not be decoded from the state: %s.", addr.Absolute(d.ModulePath), err), Subject: &config.DeclRange, }) return cty.UnknownVal(ty), diags } - } - - ios, err := is.Current.Decode(ty) - if err != nil { - // This shouldn't happen, since by the time we get here - // we should've upgraded the state data already. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid resource instance data in state", - Detail: fmt.Sprintf("Instance %s data could not be decoded from the state: %s.", addr.Absolute(d.ModulePath), err), - Subject: &config.DeclRange, - }) - return cty.UnknownVal(ty), diags - } - return ios.Value, diags -} - -func (d *evaluationStateData) getResourceInstancesAll(addr addrs.Resource, rng tfdiags.SourceRange, config *configs.Resource, rs *states.Resource, providerAddr addrs.AbsProviderConfig) (cty.Value, tfdiags.Diagnostics) { - var diags tfdiags.Diagnostics - - schema := d.getResourceSchema(addr, providerAddr) - if schema == nil { - // This shouldn't happen, since validation before we get here should've - // taken care of it, but we'll show a reasonable error message anyway. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: `Missing resource type schema`, - Detail: fmt.Sprintf("No schema is available for %s in %s. This is a bug in Terraform and should be reported.", addr, providerAddr), - Subject: rng.ToHCL().Ptr(), - }) - return cty.DynamicVal, diags - } - - switch rs.EachMode { + return ios.Value, diags case states.EachList: // We need to infer the length of our resulting tuple by searching From c49f9763087e0d3d69f531ee8fdb672779937e5d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 19 Sep 2019 09:13:47 -0400 Subject: [PATCH 3/6] change lang eval to also only lookup resources Continue only evaluating resource at a whole and push the indexing of the resource down into the expression evaluation. The exception here is that `self` must be an instance which must be extracted from the resource. We now also add the entire resource to the context, which was previously only partially populated with the self referenced instance. --- lang/data_test.go | 20 ++++++------ lang/eval.go | 80 ++++++++++++++++++++++++++++------------------- lang/eval_test.go | 35 +++++++++++++++++++-- 3 files changed, 90 insertions(+), 45 deletions(-) diff --git a/lang/data_test.go b/lang/data_test.go index dba68295800e..fd87861fc93f 100644 --- a/lang/data_test.go +++ b/lang/data_test.go @@ -7,14 +7,14 @@ import ( ) type dataForTests struct { - CountAttrs map[string]cty.Value - ForEachAttrs map[string]cty.Value - ResourceInstances map[string]cty.Value - LocalValues map[string]cty.Value - Modules map[string]cty.Value - PathAttrs map[string]cty.Value - TerraformAttrs map[string]cty.Value - InputVariables map[string]cty.Value + CountAttrs map[string]cty.Value + ForEachAttrs map[string]cty.Value + Resources map[string]cty.Value + LocalValues map[string]cty.Value + Modules map[string]cty.Value + PathAttrs map[string]cty.Value + TerraformAttrs map[string]cty.Value + InputVariables map[string]cty.Value } var _ Data = &dataForTests{} @@ -31,8 +31,8 @@ func (d *dataForTests) GetForEachAttr(addr addrs.ForEachAttr, rng tfdiags.Source return d.ForEachAttrs[addr.Name], nil } -func (d *dataForTests) GetResourceInstance(addr addrs.ResourceInstance, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { - return d.ResourceInstances[addr.String()], nil +func (d *dataForTests) GetResource(addr addrs.Resource, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { + return d.Resources[addr.String()], nil } func (d *dataForTests) GetInputVariable(addr addrs.InputVariable, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { diff --git a/lang/eval.go b/lang/eval.go index d4c24e7d6231..46baef7c0c8d 100644 --- a/lang/eval.go +++ b/lang/eval.go @@ -208,7 +208,6 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl for _, ref := range refs { rng := ref.SourceRange - isSelf := false rawSubj := ref.Subject if rawSubj == addrs.Self { @@ -226,19 +225,62 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl continue } - // Treat "self" as an alias for the configured self address. - rawSubj = selfAddr - isSelf = true + // self can only be used within a resource instance + subj := selfAddr.(addrs.ResourceInstance) - if rawSubj == addrs.Self { + if selfAddr == addrs.Self { // Programming error: the self address cannot alias itself. panic("scope SelfAddr attempting to alias itself") } + + val, valDiags := normalizeRefValue(s.Data.GetResource(subj.ContainingResource(), rng)) + + diags = diags.Append(valDiags) + + // Self is an exception in that it must always resolve to a + // particular instance. We will still insert the full resource into + // the context below. + switch k := subj.Key.(type) { + case addrs.IntKey: + self = val.Index(cty.NumberIntVal(int64(k))) + case addrs.StringKey: + self = val.Index(cty.StringVal(string(k))) + default: + self = val + } + + r := subj.Resource + if managedResources[r.Type] == nil { + managedResources[r.Type] = make(map[string]cty.Value) + } + managedResources[r.Type][r.Name] = val + continue } // This type switch must cover all of the "Referenceable" implementations // in package addrs. switch subj := rawSubj.(type) { + case addrs.Resource: + panic("RESOURCE REFERENCES DON'T HIT THIS") + + var into map[string]map[string]cty.Value + switch subj.Mode { + case addrs.ManagedResourceMode: + into = managedResources + case addrs.DataResourceMode: + into = dataResources + default: + panic(fmt.Errorf("unsupported ResourceMode %s", subj.Mode)) + } + + val, valDiags := normalizeRefValue(s.Data.GetResource(subj, rng)) + diags = diags.Append(valDiags) + + r := subj + if into[r.Type] == nil { + into[r.Type] = make(map[string]cty.Value) + } + into[r.Type][r.Name] = val case addrs.ResourceInstance: var into map[string]map[string]cty.Value @@ -252,6 +294,7 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl } val, valDiags := normalizeRefValue(s.Data.GetResource(subj.ContainingResource(), rng)) + diags = diags.Append(valDiags) r := subj.Resource @@ -259,9 +302,6 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl into[r.Type] = make(map[string]cty.Value) } into[r.Type][r.Name] = val - if isSelf { - self = val - } case addrs.ModuleCallInstance: val, valDiags := normalizeRefValue(s.Data.GetModuleInstance(subj, rng)) @@ -271,9 +311,6 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl wholeModules[subj.Call.Name] = make(map[addrs.InstanceKey]cty.Value) } wholeModules[subj.Call.Name][subj.Key] = val - if isSelf { - self = val - } case addrs.ModuleCallOutput: val, valDiags := normalizeRefValue(s.Data.GetModuleInstanceOutput(subj, rng)) @@ -288,57 +325,36 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl moduleOutputs[callName][callKey] = make(map[string]cty.Value) } moduleOutputs[callName][callKey][subj.Name] = val - if isSelf { - self = val - } case addrs.InputVariable: val, valDiags := normalizeRefValue(s.Data.GetInputVariable(subj, rng)) diags = diags.Append(valDiags) inputVariables[subj.Name] = val - if isSelf { - self = val - } case addrs.LocalValue: val, valDiags := normalizeRefValue(s.Data.GetLocalValue(subj, rng)) diags = diags.Append(valDiags) localValues[subj.Name] = val - if isSelf { - self = val - } case addrs.PathAttr: val, valDiags := normalizeRefValue(s.Data.GetPathAttr(subj, rng)) diags = diags.Append(valDiags) pathAttrs[subj.Name] = val - if isSelf { - self = val - } case addrs.TerraformAttr: val, valDiags := normalizeRefValue(s.Data.GetTerraformAttr(subj, rng)) diags = diags.Append(valDiags) terraformAttrs[subj.Name] = val - if isSelf { - self = val - } case addrs.CountAttr: val, valDiags := normalizeRefValue(s.Data.GetCountAttr(subj, rng)) diags = diags.Append(valDiags) countAttrs[subj.Name] = val - if isSelf { - self = val - } case addrs.ForEachAttr: val, valDiags := normalizeRefValue(s.Data.GetForEachAttr(subj, rng)) diags = diags.Append(valDiags) forEachAttrs[subj.Name] = val - if isSelf { - self = val - } default: // Should never happen diff --git a/lang/eval_test.go b/lang/eval_test.go index 67feb30d0163..57763770e21b 100644 --- a/lang/eval_test.go +++ b/lang/eval_test.go @@ -24,7 +24,7 @@ func TestScopeEvalContext(t *testing.T) { "key": cty.StringVal("a"), "value": cty.NumberIntVal(1), }, - ResourceInstances: map[string]cty.Value{ + Resources: map[string]cty.Value{ "null_resource.foo": cty.ObjectVal(map[string]cty.Value{ "attr": cty.StringVal("bar"), }), @@ -39,6 +39,14 @@ func TestScopeEvalContext(t *testing.T) { "attr": cty.StringVal("multi1"), }), }), + "null_resource.each": cty.ObjectVal(map[string]cty.Value{ + "each0": cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("each0"), + }), + "each1": cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("each1"), + }), + }), "null_resource.multi[1]": cty.ObjectVal(map[string]cty.Value{ "attr": cty.StringVal("multi1"), }), @@ -139,11 +147,14 @@ func TestScopeEvalContext(t *testing.T) { }, }, { + // at this level, all instance references return the entire resource `null_resource.multi[1]`, map[string]cty.Value{ "null_resource": cty.ObjectVal(map[string]cty.Value{ "multi": cty.TupleVal([]cty.Value{ - cty.DynamicVal, + cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("multi0"), + }), cty.ObjectVal(map[string]cty.Value{ "attr": cty.StringVal("multi1"), }), @@ -151,6 +162,22 @@ func TestScopeEvalContext(t *testing.T) { }), }, }, + { + // at this level, all instance references return the entire resource + `null_resource.each["each1"]`, + map[string]cty.Value{ + "null_resource": cty.ObjectVal(map[string]cty.Value{ + "each": cty.ObjectVal(map[string]cty.Value{ + "each0": cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("each0"), + }), + "each1": cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("each1"), + }), + }), + }), + }, + }, { `foo(null_resource.multi, null_resource.multi[1])`, map[string]cty.Value{ @@ -215,7 +242,9 @@ func TestScopeEvalContext(t *testing.T) { // expanded here and then copied into "self". "null_resource": cty.ObjectVal(map[string]cty.Value{ "multi": cty.TupleVal([]cty.Value{ - cty.DynamicVal, + cty.ObjectVal(map[string]cty.Value{ + "attr": cty.StringVal("multi0"), + }), cty.ObjectVal(map[string]cty.Value{ "attr": cty.StringVal("multi1"), }), From d4031918d11351cba3b8e3f7ad68a3e11232d56f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 19 Sep 2019 09:53:23 -0400 Subject: [PATCH 4/6] parse resource refs correctly Now that we only evaluate whole resources, we can parse resource refs correct as the resource, rather than an unknown instance. --- addrs/parse_ref.go | 2 +- addrs/parse_ref_test.go | 20 ++++++++------------ lang/eval.go | 2 -- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/addrs/parse_ref.go b/addrs/parse_ref.go index a230d0cd1d8e..4a614102cfc8 100644 --- a/addrs/parse_ref.go +++ b/addrs/parse_ref.go @@ -290,7 +290,7 @@ func parseResourceRef(mode ResourceMode, startRange hcl.Range, traversal hcl.Tra // of the resource, but we don't have enough context here to decide // so we'll let the caller resolve that ambiguity. return &Reference{ - Subject: resourceInstAddr, + Subject: resourceAddr, SourceRange: tfdiags.SourceRangeFromHCL(rng), }, diags } diff --git a/addrs/parse_ref_test.go b/addrs/parse_ref_test.go index f42799f50d78..792179cefb28 100644 --- a/addrs/parse_ref_test.go +++ b/addrs/parse_ref_test.go @@ -114,12 +114,10 @@ func TestParseRef(t *testing.T) { { `data.external.foo`, &Reference{ - Subject: ResourceInstance{ - Resource: Resource{ - Mode: DataResourceMode, - Type: "external", - Name: "foo", - }, + Subject: Resource{ + Mode: DataResourceMode, + Type: "external", + Name: "foo", }, SourceRange: tfdiags.SourceRange{ Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, @@ -592,12 +590,10 @@ func TestParseRef(t *testing.T) { { `boop_instance.foo`, &Reference{ - Subject: ResourceInstance{ - Resource: Resource{ - Mode: ManagedResourceMode, - Type: "boop_instance", - Name: "foo", - }, + Subject: Resource{ + Mode: ManagedResourceMode, + Type: "boop_instance", + Name: "foo", }, SourceRange: tfdiags.SourceRange{ Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, diff --git a/lang/eval.go b/lang/eval.go index 46baef7c0c8d..a2408f1501a5 100644 --- a/lang/eval.go +++ b/lang/eval.go @@ -261,8 +261,6 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl // in package addrs. switch subj := rawSubj.(type) { case addrs.Resource: - panic("RESOURCE REFERENCES DON'T HIT THIS") - var into map[string]map[string]cty.Value switch subj.Mode { case addrs.ManagedResourceMode: From fed4e82c252930787a50c9f957fe47900d222e17 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 19 Sep 2019 10:56:21 -0400 Subject: [PATCH 5/6] check resource config for unknowns during apply Now that the most common cause of unknowns (invalid resource indexes) is caught earlier, we can validate that the final apply config is wholly known before attempting to apply it. This ensures that we're applying the configuration we intend, and not silently dropping values. --- terraform/eval_apply.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 7bf786654f2d..84f3bc1d9a0c 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -70,6 +70,13 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { } } + if !configVal.IsWhollyKnown() { + return nil, fmt.Errorf( + "configuration for %s still contains unknown values during apply (this is a bug in Terraform; please report it!)", + absAddr, + ) + } + log.Printf("[DEBUG] %s: applying the planned %s change", n.Addr.Absolute(ctx.Path()), change.Action) resp := provider.ApplyResourceChange(providers.ApplyResourceChangeRequest{ TypeName: n.Addr.Resource.Type, From bfce78064b959a469d7dbf4bc6998561d5c890b3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 7 Oct 2019 18:13:20 -0400 Subject: [PATCH 6/6] lang/eval: more evalContext fixups self references do not need to be added to `managedResource`, and in fact that could cause issues later if self is allowed in contexts other than managed resources. Coalesce 2 cases in the Referenceable switch, be take the ContainingResource address of an instance beforehand. --- lang/eval.go | 39 +++++++++------------------------------ lang/eval_test.go | 13 ------------- 2 files changed, 9 insertions(+), 43 deletions(-) diff --git a/lang/eval.go b/lang/eval.go index a2408f1501a5..d3cb5dbb1da5 100644 --- a/lang/eval.go +++ b/lang/eval.go @@ -225,14 +225,14 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl continue } - // self can only be used within a resource instance - subj := selfAddr.(addrs.ResourceInstance) - if selfAddr == addrs.Self { // Programming error: the self address cannot alias itself. panic("scope SelfAddr attempting to alias itself") } + // self can only be used within a resource instance + subj := selfAddr.(addrs.ResourceInstance) + val, valDiags := normalizeRefValue(s.Data.GetResource(subj.ContainingResource(), rng)) diags = diags.Append(valDiags) @@ -249,16 +249,16 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl self = val } - r := subj.Resource - if managedResources[r.Type] == nil { - managedResources[r.Type] = make(map[string]cty.Value) - } - managedResources[r.Type][r.Name] = val continue } // This type switch must cover all of the "Referenceable" implementations - // in package addrs. + // in package addrs, however we are removing the possibility of + // ResourceInstance beforehand. + if addr, ok := rawSubj.(addrs.ResourceInstance); ok { + rawSubj = addr.ContainingResource() + } + switch subj := rawSubj.(type) { case addrs.Resource: var into map[string]map[string]cty.Value @@ -280,27 +280,6 @@ func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceabl } into[r.Type][r.Name] = val - case addrs.ResourceInstance: - var into map[string]map[string]cty.Value - switch subj.Resource.Mode { - case addrs.ManagedResourceMode: - into = managedResources - case addrs.DataResourceMode: - into = dataResources - default: - panic(fmt.Errorf("unsupported ResourceMode %s", subj.Resource.Mode)) - } - - val, valDiags := normalizeRefValue(s.Data.GetResource(subj.ContainingResource(), rng)) - - diags = diags.Append(valDiags) - - r := subj.Resource - if into[r.Type] == nil { - into[r.Type] = make(map[string]cty.Value) - } - into[r.Type][r.Name] = val - case addrs.ModuleCallInstance: val, valDiags := normalizeRefValue(s.Data.GetModuleInstance(subj, rng)) diags = diags.Append(valDiags) diff --git a/lang/eval_test.go b/lang/eval_test.go index 57763770e21b..64c1e4542904 100644 --- a/lang/eval_test.go +++ b/lang/eval_test.go @@ -237,19 +237,6 @@ func TestScopeEvalContext(t *testing.T) { { `self.baz`, map[string]cty.Value{ - // In the test function below we set "SelfAddr" to be - // one of the resources in our dataset, causing it to get - // expanded here and then copied into "self". - "null_resource": cty.ObjectVal(map[string]cty.Value{ - "multi": cty.TupleVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "attr": cty.StringVal("multi0"), - }), - cty.ObjectVal(map[string]cty.Value{ - "attr": cty.StringVal("multi1"), - }), - }), - }), "self": cty.ObjectVal(map[string]cty.Value{ "attr": cty.StringVal("multi1"), }),