Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always evaluate resources in their entirety #22846

Merged
merged 6 commits into from
Oct 8, 2019
Merged

Always evaluate resources in their entirety #22846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
86bf674
change GetResourceInstance to GetResource
jbardin Aug 23, 2019
da8ec6b
change state eval GetResource to return all
jbardin Sep 19, 2019
c49f976
change lang eval to also only lookup resources
jbardin Sep 19, 2019
d403191
parse resource refs correctly
jbardin Sep 19, 2019
fed4e82
check resource config for unknowns during apply
jbardin Sep 19, 2019
bfce780
lang/eval: more evalContext fixups
jbardin Oct 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion addrs/parse_ref.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
20 changes: 8 additions & 12 deletions addrs/parse_ref_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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},
Expand Down
25 changes: 8 additions & 17 deletions configs/configupgrade/analysis_expr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion lang/data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
20 changes: 10 additions & 10 deletions lang/data_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -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) {
Expand Down
94 changes: 40 additions & 54 deletions lang/eval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -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 {
Expand All @@ -226,45 +225,60 @@ 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

if rawSubj == addrs.Self {
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than handling this as a totally special case up here, could we instead get the same result by setting rawSubj to subj.ContainingResource and then just letting this fall out into the switch statement below? If I'm reading this right, it seems like we'd then branch to the case addrs.Resource label and get the same result.

(Alternatively, maybe we could just skip populating managedResources altogether here; populating self should be sufficient I think, because if there were any references to the resource itself as well then we'll catch them on a different iteration of the loop. That would also remove the risk that some future enhancement allows self to refer to data resources and we end up incorrectly putting them in the managedResources map below, just thinking defensively about what ways outside code might violate these assumptions in future.)

Copy link
Member Author

@jbardin jbardin Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your latter suggestion is correct, and I just followed existing code+tests which ended up populating the managed resources.

We need to assign an instance to self in the final context, so I think this section is still required to extract the "self" value from the resource as a whole, whereas the switch below is adding resources.


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
}

continue
}

// This type switch must cover all of the "Referenceable" implementations
// in package addrs.
switch subj := rawSubj.(type) {
// in package addrs, however we are removing the possibility of
// ResourceInstance beforehand.
if addr, ok := rawSubj.(addrs.ResourceInstance); ok {
rawSubj = addr.ContainingResource()
}

case addrs.ResourceInstance:
var into map[string]map[string]map[addrs.InstanceKey]cty.Value
switch subj.Resource.Mode {
switch subj := rawSubj.(type) {
case addrs.Resource:
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.Resource.Mode))
panic(fmt.Errorf("unsupported ResourceMode %s", subj.Mode))
}

val, valDiags := normalizeRefValue(s.Data.GetResourceInstance(subj, rng))
val, valDiags := normalizeRefValue(s.Data.GetResource(subj, rng))
diags = diags.Append(valDiags)

r := subj.Resource
r := subj
if into[r.Type] == nil {
into[r.Type] = make(map[string]map[addrs.InstanceKey]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
if isSelf {
self = val
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))
Expand All @@ -274,9 +288,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))
Expand All @@ -291,57 +302,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
Expand All @@ -367,13 +357,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
Expand Down
Loading