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.

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

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Sep 19, 2019

The primary goal of this PR is to always return entire resources to be evaluated, rather than individual instances. This allows us to push the evaluation of resource indexes down into expression evaluation.

Invalid indexed references to other resources were silently returning unknown values when the instance didn't exist. These were only caught by extra validation in data sources when the config still contained unknown values during ReadDataSource. This now will catch invalid indexes, and adds an IsWhollyKnown validation to the config during apply, preventing config values from being silently dropped.

Only data sources caught this previously, but returned a rather unhelpful error of

configuration for data.null_data_source.foo[0].id still contains unknown values during apply (this is a bug in Terraform; please report it!)

Now that indexes are handled during expression evaluation, we can get a full diagnostic output:

Error: Invalid index

  on main.tf line 7, in resource "null_resource" "b":
   7:     a = null_resource.foo[0].id
    |----------------
    | null_resource.foo is empty tuple

The given key does not identify an element in this collection value.

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.
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.
@jbardin jbardin requested a review from a team September 19, 2019 15:05
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Sep 19, 2019
@jbardin jbardin force-pushed the jbardin/evaluate-resource branch from 8284d38 to 95da635 Compare September 19, 2019 15:36
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.
Now that we only evaluate whole resources, we can parse resource refs
correct as the resource, rather than an unknown instance.
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.
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

I left some questions/feedback inline, but this broadly makes sense to me and I expect my questions may be arising from not having the context fully loaded.

lang/eval.go Outdated
rawSubj = selfAddr
isSelf = true
// self can only be used within a resource instance
subj := selfAddr.(addrs.ResourceInstance)
Copy link
Contributor

Choose a reason for hiding this comment

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

This was originally designed to be generic so that we could in principle support self in other contexts in future, since there are several issues open around that sort of thing. Fully generalizing this isn't necessary right now but since this limitation is not obvious in the Scope API, could we handle this by panicking a more specific error instead?

Relatedly, it seems like the condition right below this can no longer be true because if selfAddr were addrs.Self this type assertion would've panicked already. That branch is also just trying to present a more helpful panic message when the caller uses this API improperly, so perhaps we should invert this to let that one "win" for the more specific case.

// Programming error: the self address cannot alias itself.
panic("scope SelfAddr attempting to alias itself")
}

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

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.

lang/eval.go Outdated
@@ -251,20 +291,15 @@ 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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we remove this case by putting something like the following fixup before the switch here:

if addr, ok := rawSubj.(addrs.ResourceInstance); ok {
    rawSubj = addr.ContainingResource()
}

I think that wouldn't have been possible with how self was handled before, where each of these cases was responsible for detecting if it was self and setting it, but now that self is being handled as special anyway it seems like that removes the remaining need for this clause. I'm probably missing something though, since there seem to be a bunch of different codepaths through there and I'm not sure I'm catching all of the interactions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I think I meant to try and coalesce these cases and forgot

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.
@jbardin
Copy link
Member Author

jbardin commented Oct 8, 2019

Updated with the minor fixes from review.

@Ninir
Copy link
Contributor

Ninir commented Oct 21, 2019

@jbardin It seems this PR is breaking the following use-case:

module "mymodule" {
  source           = "something/something"
  name             = "myname"
  create_resources = var.create_resources

  security_groups = [
    # ...
    aws_security_group.mysg[0].id,
  ]
}

resource "aws_security_group" "mysg" {
  count = var.create_resources ? 1 : 0
  name = "myname"
  # ...
}

Because we can't have count.index on the module (as suggested by #22480 (comment)), the only way I see to fix it is to use the coaslesce function, or add the check manually, as in:

module "mymodule" {
  source           = "something/something"
  name             = "myname"
  create_resources = var.create_resources

  security_groups = [
    # ...
    var.create_resources? aws_security_group.mysg[0].id : null,
  ]
}

# ...

We have a bunch of other places like that in our codebase, and thus I'm wondering if it should really be fixed that way or not.

Let me know if that makes sense!

@jbardin
Copy link
Member Author

jbardin commented Oct 21, 2019

Hi @Ninir,

Yes, if there are no aws_security_group.mysg instances, the expression aws_security_group.mysg[0] should always fail since the index is invalid.

I would use which ever solution leads to the most readable result. Taking a guess from the snippet above, a splat expression should give you want you want

security_groups = aws_security_group.mysg[*].id

Which is equivalent to a list comprehension, which can add additional filtering as an if clause:

[for sg in aws_security_group.mysg : sg.id if ...]

If you don't want to operate on the instances as a complete group however, guarding single references with a conditional would be required.

@Ninir
Copy link
Contributor

Ninir commented Oct 21, 2019

@jbardin ok, so it is up to people to fix their cases got it. Thank you!

appilon pushed a commit to hashicorp/terraform-plugin-sdk that referenced this pull request Oct 29, 2019
appilon added a commit to hashicorp/terraform-plugin-sdk that referenced this pull request Oct 29, 2019
This cherry pick depends on hashicorp/terraform#22846

create_before_destroy across modules
appilon added a commit to hashicorp/terraform-plugin-sdk that referenced this pull request Oct 30, 2019
Cherry pick hashicorp/terraform#22846: Always evaluate resources in their entirety
@appilon appilon removed the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Oct 30, 2019
appilon added a commit to hashicorp/terraform-plugin-sdk that referenced this pull request Oct 30, 2019
This cherry pick depends on hashicorp/terraform#22846

create_before_destroy across modules
appilon added a commit to hashicorp/terraform-plugin-sdk that referenced this pull request Nov 4, 2019
This cherry pick depends on hashicorp/terraform#22846

create_before_destroy across modules
@ghost
Copy link

ghost commented Nov 8, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Nov 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants