[WIP] Use a DAG to resolve locals, globals, and include #858

apottere · 2019-09-09T22:47:53Z

This is a possible solution to #814.

This PR uses the same DAG that terraform uses to perform dependency resolution. All locals, globals, and include block attributes are added to the graph, and then edges are generated between nodes by extracting variable references in the attribute's expression (this is supported natively in HCL2).

The graph is then evaluated and updated in a few stages:

Child locals/include are evaluated, to see if an include exists.
Parent locals and globals are added to the graph if a parent is included
All variables are evaluated

This allows any variable to depend on any other variable, with a few exceptions:

No dependency loops
Nothing used in an include expression can depend on a global variable, necessarily

Docs from the code:

// Evaluation Steps:
// 1. Parse child HCL, extract locals, globals, and include
// 2. Add vertices for child locals, globals, and include
// 3. Add edges for child variables based on interpolations used
//     a. When encountering globals that aren't defined in this config, create a vertex for them with an empty expression
// 4. Verify DAG and reduce graph
//     a. Verify no globals are used in path to include (if exists)
// 5. Evaluate everything except globals
// 6. If include exists, find parent HCL, parse, and extract locals and globals
// 7. Add vertices for parent locals
// 8. Add vertices for parent globals that don't already exist, or add expressions to empty globals
// 9. Verify and reduce graph
//     a. Verify that there are no globals that are empty.
// 10. Evaluate everything, skipping things that were evaluated in (5)

There's more cleanup/validation/documentation that needs to be done, but a POC can be found in config/config_graph_test.go and test/fixture-config-graph that shows how a config might be structured with this PR.

Note: include now has to be a variable instead of a function because AFAIK there's no way to extract function references from an HCL2 expression.

apottere · 2019-09-09T23:41:17Z

Obviously it hasn't been hooked into any of the actual config parsing yet, either.

brikis98 · 2019-09-12T10:23:47Z

@apottere Could you update the README so we have a better idea of how you expect these config features to be used?

@yorinasub17 I think you were driving these discussions and did a lot of the recent work in this area. Could you take a look and share your thoughts?

yorinasub17 · 2019-09-12T14:25:49Z

Yup I plan on taking a look, but have been buried with my other tasks. I took a quick look earlier in the week and realized this is pretty big and requires some deeper thinking so I decided to wait until I have a bit of time to sit down and dive deep.

I expect to have some time early next week to review this.

apottere · 2019-09-13T14:31:55Z

@brikis98 I'd rather chat about functionality here (instead of updating the readme) if that's ok, since there's a very good chance what I put in the readme now will have to change. This was my desired solution to #814, which allows a developer to have global variables that are shared across included terragrunt.hcl files - with child terragrunt files having the option to override the value of a global in the parent.

This approach also allows you to reference global variables and include information (like relative path to include) in local variables, as long as you don't create a dependency loop. This means you don't need to expose variables from the parent that shouldn't be overridden simply because they need global or include data.

The test in the PR has an example of how this could be used, but I'll explain in more detail here.
Given the following parent terragrunt.hcl:

locals {
  source-prefix = "src-"
}
globals {
  region = "us-west-2"
  source-postfix = null
}
terraform {
  source = "${local.source-prefix}${global.source-postfix}"
}

And a child one/two/three/terragrunt.hcl:

locals {
  full-name = "${local.name}-${local.region}"
  name = "test"
  region = "us-east-1"
  parent = "${local.parent-dir}/terragrunt.hcl"
  parent-dir = "../../.."
}
globals {
  region = local.region
  source-postfix = "${local.parent}-${include.relative}"
}
include {
  path = "${local.parent}"
}
input = {
  region = global.region
}

Terragrunt can figure out which variables depend on other variables using builtin HCL2 methods, and then graph them out to determine a resolution order. It can figure out that the child's include block depends on local.parent, figure out local.parent depends on local.parent-dir, etc. For this particular config, include.path would evaluate to ../../../terragrunt.hcl.

Once terragrunt has resolved the include path (and only the locals that are required for that), it can go find the parent and merge the parent's globals and locals into the graph. At that point it has everything it needs to evaluate all of the variables and create a map that can be added to the EvaluationContext for decoding the rest of the HCL.

In the example above, the parent has global.source-postfix default to null. The child overrides it to ${local.parent}-${include.relative}, which evaluates to ../../../terragrunt.hcl-one/two/three. Then when the parent's terraform.source is evaluated, it'll use the value from the child, evaluating to src-../../../terragrunt.hcl-one/two/three.

For the parent's global.region, the child overrides the default value of us-west-2 to us-east-1. Now wherever that variable is referenced (in the parent or child), global.region will evaluate to us-east-1.

Another thing I forgot to mention in my first comment is that this approach is fairly trivial to modify to support multiple includes. You basically just repeat the local -> include resolution until a config doesn't have an include block, and then you evaluate everything.

apottere · 2019-09-13T14:33:24Z

Also another reminder that this is a POC and not finished code, e.g. it's not actually hooked up to the real evaluation pipeline yet and has a bunch of TODOs.

apottere · 2019-09-13T14:46:57Z

Also, I'm going to be out of town for 2 weeks, so no rush getting to this.

yorinasub17

I finally got the chance to sit down and read through this. Thanks for your contribution! I've been thinking about the graph approach, but wanted to start simple so it is good to see an iteration from the community that explores this.

I think this is a good start. I took a pass through and highlighted a few of the glaring things that stood out, but unfortunately I must admit that I didn't do a complete thorough review and add all my comments. This is primarily because the lack of comments on all the functions made it hard to follow and understand what you were going for in each of the implementation. E.g why does addVertices need a side effect function to add vertices to the callers' array? Why does evaluateVariable return a boolean, and seems to ignore the errors? You don't need to answer these as I realized this after reading the code a few times, but definitely could have used some comments to assist in this.

So as a next step, can you:

Add comments to each function and struct with a description of the intended action, and expected return items, especially those functions that are unconventional like evaluateVariable.
I could use some help understanding the intended usage of globals in relation to locals, especially after all this graph logic. E.g when can globals reference locals? Can a child local reference the parent local? Can a child reference the parent local in the inputs? What about include? And what is supposed to happen if both the child and the parent has global with the same name? Does the child win or the parent? Ideally, you would capture all this in the README.

Speaking of that...

I'd rather chat about functionality here (instead of updating the readme) if that's ok, since there's a very good chance what I put in the readme now will have to change.

We follow RDD here and it is actually important to us that our contributors follow it as much as possible. We mention this practice in our contribution guideline. For example, if you had updated the README first for feedback with your intentions of supporting globals referencing locals in a loop, it would have made it a lot easier to understand what you were going for in the graph approach. We also could have discussed there if the graph was necessary, or if we should go for the simpler approach of reorganizing the order of evaluation. Also as a reviewer, it is much easier to understand and discuss a README change than it is to discuss the code update.

yorinasub17 · 2019-09-27T19:31:12Z

config/config_graph.go

+
+const local = "local"
+const global = "global"
+const include = "include"


const namespace is shared with the package. Since this can be used for other files in this package, I think it better to put this in a constants.go file.

yorinasub17 · 2019-09-27T19:39:19Z

config/config_graph.go

+// 4. Verify DAG and reduce graph
+//     a. Verify no globals are used in path to include (if exists)
+// 5. Evaluate everything except globals
+// 6. If include exists, find parent HCL, parse, and extract locals and globals


Does this mean the child can refer to parent locals? I am not sure I want to support that. The whole point of separating globals (which we intend to support all the looping logic) and locals was to avoid all the messy logic in merging variable references. What happens if both the child and parent define the same name? Does the child win? Or the parent? What about when referring to locals in the parent? Is that referring to the childs local?

For this reason, the idea was to keep locals as variables that are only referencable in the current config, and globals are variables that are in the shared namespace. Having this split allows us to recommend locals for most use cases, and for the cases where you need to reference other config or for resolving variables that depend on include, you can use globals but the usage should be limited since it adds complexity to your terragrunt config.

No, locals are always local to the file where they're defined. globals can be shared between files, and their values can be based off locals in the file they're declared in.

If a child config overrides a global variable, the child config's expression will be evaluated based on the child config's locals, and the value of the global will be used in the parent and the child for that global.

yorinasub17 · 2019-09-27T19:40:22Z

config/config_graph.go

+//     a. Verify that there are no globals that are empty.
+// 10. Evaluate everything, skipping things that were evaluated in (5)
+func ParseConfigVariables(filename string, terragruntOptions *options.TerragruntOptions) (*EvaluationResult, *EvaluationResult, error) {
+	globals := evaluatorGlobals{


Suggested change

globals := evaluatorGlobals{

globalEvaluatorConfig := evaluatorGlobals{

or globalEvaluatorOptions? Using a name like globals makes it easy to confuse with the globals block later on in the code.

yorinasub17 · 2019-09-27T19:40:50Z

config/config_graph.go

+	// Add root of graph
+	globals.graph.Add(globals.root)
+
+	child := *newConfigEvaluator(filename, globals, nil)


Why dereference the pointer here?

Ignorance, mostly :D I really don't have a good grasp of pointer semantics in go, so that's something I'll read up on before I clean up the code.

yorinasub17 · 2019-09-27T19:41:38Z

config/config_graph.go

+
+	// 5
+	diags := globals.evaluateVariables(false)
+	if diags != nil {


Suggested change

if diags != nil {

if diags != nil && diags.HasErrors() {

yorinasub17 · 2019-09-27T19:44:13Z

config/config_graph.go

+
+	// 10
+	diags = globals.evaluateVariables(true)
+	if diags != nil {


Suggested change

if diags != nil {

if diags != nil && diags.HasErrors() {

yorinasub17 · 2019-09-27T19:49:56Z

config/config_graph.go

+		return blocksByType[locals][0].Body, blocksByType[globals][0].Body, blocksByType[include][0].Body, diags
+	} else {
+		return blocksByType[locals][0].Body, blocksByType[globals][0].Body, nil, diags
+	}


Perhaps this should return a struct to simplify this logic? This will also make it clear what each positional return type is.

Yeah, this code will change significantly to be less brittle. Right now it's just happy-path.

yorinasub17 · 2019-09-27T19:51:08Z

config/config_graph.go

+	return nil
+}
+
+func (eval *configEvaluator) evaluateVariable(vertex variableVertex, diags hcl.Diagnostics, evaluateGlobals bool) bool {


I think it would be easier to understand if the evaluateGlobals bool was replaced with a list of variable references to evaluate.

yorinasub17 · 2019-09-27T20:02:40Z

config/config_graph.go

+	}
+}
+
+func (eval *configEvaluator) addVertices(vertexType string, block hcl.Body, consumer func(vertex variableVertex) error) error {


I'm having trouble understanding what this function is doing. I can tell that it is a routine to add vertices to the graph, but what is difficult to understand are:

Each of the conditions for when a vertex is added.

The purpose of the consumer function. In general, side effect based functions make it hard to parse and understand, which makes it harder to maintain the code. If you can come up with a way to avoid that, it would be great. E.g you can make this function return all the vertices that were added to the graph, and then have the caller do what it needs to do by looping through those vertices. That is easier to parse than having to understand that this is a callback that is called on each vertex that was added to the graph.

yorinasub17 · 2019-09-27T20:13:38Z

config/config_graph.go

+
+	variables := target.Expr.Variables()
+
+	if variables == nil || len(variables) <= 0 {


Suggested change

if variables == nil || len(variables) <= 0 {

if variables == nil || len(variables) == 0 {

apottere · 2019-10-08T20:53:02Z

@yorinasub17 I just got back and settled from vacation, so I should be more responsive from here on out. Thanks for taking a look! I'll update the README shortly with some examples of what I'd expect to be possible.

I'll also take a look at all the comments in the code, but I tried to stress in my original comment that is is not releasable code in any way, shape, or form. I'd really like to just discuss the entire strategy before I spend time perfecting the implementation, which might just be thrown away. If/when I get the go-ahead, I'll get the code to a place where I think it's ready for review and it should be a lot easier to follow then - till then, it's 100% a POC.

Ideally, you should only have to look at the README (coming), PR, and example test for now. I'll add another comment once the README changes are done.

apottere · 2019-10-15T19:27:21Z

@yorinasub17 Made a first pass of updates to the readme, mainly in the #values section. A lot of the readme updates I left out are simply find/replace with the include values instead of functions.

yorinasub17

Thanks for taking the time to write the README! It was very useful in understanding how each of the new functionality can help.

Other than the naming changes, I think what you have makes sense. I am not entirely convinced that we should support the cross calls between globals and locals, but I think there is a way to make the graph parsing logic easier to follow so as long as we can get it simplified and easier to read, I think I am ok moving forward with the graph based approach.

However, given that it is a pretty big change, I would like a second opinion from @brikis98 .

yorinasub17 · 2019-10-18T01:18:22Z

README.md

+what you want.
+
+
+##### `include.relative`


This should be named path_relative_to_include, so the direction is clear in the name, and it parallels the existing function.

yorinasub17 · 2019-10-18T01:18:40Z

README.md

-`locals` defined in the parent config of an `include` block into the current context. If you wish to reuse variables
-globally, consider using `yaml` or `json` files that are included and merged using the `terraform` built in functions
-available to `terragrunt`.
+##### `include.relative_reverse`


Similar to above, this should be named path_relative_from_include.

apottere · 2019-10-30T18:06:59Z

@brikis98 did you get a chance to take a look at this yet?

brikis98 · 2019-10-30T18:10:46Z

Not yet. Really buried this week. Will try to get to it as soon as I can.

brikis98 · 2019-12-02T12:04:13Z

Hey folks, apologies for the very long delay. I kept snoozing this in favor of other priorities... And somehow more than a month went by before I got to it. I appreciate your patience.

I looked through the README updates in this PR and here are some thoughts:

On the one hand, I like the changes. local.xxx, global.xx, include.xxx, and the ability to do cross-references all make sense and seem to behave intuitively. So in the short term, it's clear this is an improvement over what we have today.
On the other hand, I'm a little worried that the config language in Terragrunt is starting to grow into more and more of a full language (see also Greenspun's tenth rule). I feel like we're slowly following in the tracks of all he features Terraform had to add (functions, locals, loops, conditionals, etc), one at a time, and without taking a step back and carefully considering the design—and what problem we're trying to solve—this could lead to a messy config language and hard-to-maintain code. The prospect of supporting multiple includes in the future and trying to mentally sort through how all the globals may override each other is... worrying.
So before diving into the details of the code, tests, etc, I'd like to pause for a minute and ask if there's a simpler way to approach this problem?
- The goal of locals is to assign values to names in the current configuration to keep things DRY and readable. This makes sense to me and it's the approached used in all languages for decades.
- The goal of include and globals is to be able to reuse values from other configurations. But is an inheritance model the best approach here? For example, should we consider using the dependency concept instead? That is, instead of including a configuration and magically inheriting all of its values, perhaps we should be specifying it as a dependency and explicitly extracting just the values we need from it? For example, let's say you had a terragrunt-prod-common.hcl with these contents:
```
remote_state {
  backend = "s3"
  config = {
    encrypt        = true
    bucket         = "my-bucket"
    key            = "${path_relative_to_include()}/terraform.tfstate"
    region         = "us-east-1"
    dynamodb_table = "terraform-locks"
  }
}        

terraform {
  extra_arguments "retry_lock" {
    commands  = get_terraform_commands_that_need_locking()
    arguments = ["-lock-timeout=20m"]
  }          
} 
```
  Then perhaps you could have one terragrunt.hcl that reuses all of this config as follows:
```
dependency "prod_common" {
  config_path = "../terragrunt-prod-common.hcl"
}

remote_state = dependency.prod_common.remote_state
terraform    = dependency.prod_common.terraform
```
  And another terragrunt.hcl that only reuses some pieces of it:
```
dependency "prod_common" {
  config_path = "../terragrunt-prod-common.hcl"
}

remote_state {
  backend = "s3"
  config = {
    encrypt        = false
    bucket         = dependency.prod_common.remote_state.config.bucket
    key            = "/some/custom/path/terraform.tfstate"
    region         = dependency.prod_common.remote_state.config.region
    dynamodb_table = dependency.prod_common.remote_state.config.dynamodb_table
  }
}  
```
  It seems like this approach might be simpler, easier to follow (since everything is explicit), and require fewer language features (in fact, we could deprecate include and reduce the number of language features!).
  
  Or perhaps that's not the right direction, but my point is: let's take a step back, figure out what we're trying to solve, and find the simplest approach, rather than adding more and more language features.

apottere · 2019-12-02T17:23:36Z

No worries, thanks for taking a look @brikis98!

On the other hand, I'm a little worried that the config language in Terragrunt is starting to grow into more and more of a full language (see also Greenspun's tenth rule). I feel like we're slowly following in the tracks of all he features Terraform had to add (functions, locals, loops, conditionals, etc), one at a time, and without taking a step back and carefully considering the design—and what problem we're trying to solve—this could lead to a messy config language and hard-to-maintain code. The prospect of supporting multiple includes in the future and trying to mentally sort through how all the globals may override each other is... worrying.

I agree - I've been mulling over this in the back of my head for a while now and I think what I would really like from a project like terragrunt is basically terraform, but where terraform projects (with one remote state each) are the resources. This is already partially realized with the current "dependency" block and local variables.

The goal of include and globals is to be able to reuse values from other configurations. But is an inheritance model the best approach here?

I'm also starting to think that the answer to this might be no.

For example, should we consider using the dependency concept instead?

I really like this idea, but:

It might make sense to have a distinction between a terraform dependency and a terragrunt dependency, since the dependency block already exists and automatically maps terraform outputs. Maybe we should have a dependency block for depending on entire other configuration trees and grabbing their outputs, and an import block witch evaluates another terragrunt configuration but doesn't actually run the corresponding terraform, allowing us to just re-use the configuration values? Ex:
```
import "common" {
  config_path = "../.."
}

dependency "s3-bucket" {
  config_path = "../s3"
}

remote_state = import.common.remote_state
terraform    = import.common.terraform
inputs = {
  arn = dependency.s3-bucket.bucket-arn
}
```
It might be helpful to have a flag in the dependency/import (or whatever the name is) block that automatically grabs the configuration from the included terragrunt, to simulate what we have today. Ex:
```
import "common" {
  auto_import = true
  config_path = "../.."
}

// no need to specify remote_state, terraform, inputs, etc: automatically grabbed from import
```

It seems like this approach might be simpler, easier to follow (since everything is explicit), and require fewer language features (in fact, we could deprecate include and reduce the number of language features!).

I definitely agree. I'm also open to other suggestions, and I'd like to hear @yorinasub17's thoughts on it as well.

tomalok · 2019-12-03T01:04:01Z

I've been getting around the lack of inherited "globals" by using some smarts in my leaf terragrunt.hcl's locals { ... } and doing an inputs = merge( ... )...

terraform {
  source = "..."
}

include {
  # toplevel terragrunt.hcl sets remote_state { ... } and terraform { extra_arguments { ... } }
  path = find_in_parent_folders()
}

dependency "nodes" {
  config_path = "../../nodes"
}
dependency "networks" {
  config_path = "../../networks"
}

locals {
  # where are we now?
  dir = get_terragrunt_dir()
  # ___.yaml files to look for, and order that they get merged
  yaml_vars = [ "account", "region", "swarm", "service" ]
  # magic...
  yaml_merged = merge(
    [ for p in
      [ for f in local.yaml_vars:
        fileexists("${local.dir}/${f}.yaml")
          ? "${local.dir}/${f}.yaml"
          : "${local.dir}/${find_in_parent_folders("${f}.yaml", "NONE")}"
      ]: yamldecode(fileexists(p) ? file(p) : "{}")
    ]...
  )

  # need to load in more things based on merged yaml...
  configs = { for d in local.yaml_merged.configs:
    d => { for f in fileset("${local.dir}/configs/${d}", "*"):
      f => base64encode(file("configs/${d}/${f}"))
    }
  }
}

inputs = merge(
  local.yaml_merged, {
    # additional local inputs
    configs = local.configs

    # additional dependency inputs
    swarm_fqdn  = dependency.nodes.outputs.swarm_fqdn
    networks    = dependency.networks.outputs.these
  }
)

Of course if some terragrunt function did all/more of the magic automagically, i'd find that very useful.

yorinasub17 · 2019-12-03T01:44:15Z

For example, should we consider using the dependency concept instead?

I like this approach a lot! This makes sense and agree that reduces the complexity of the language.

It might make sense to have a distinction between a terraform dependency and a terragrunt dependency, since the dependency block already exists and automatically maps terraform outputs. Maybe we should have a dependency block for depending on entire other configuration trees and grabbing their outputs, and an import block witch evaluates another terragrunt configuration but doesn't actually run the corresponding terraform, allowing us to just re-use the configuration values?

This distinction already exists in the skip_outputs property for dependency blocks. In fact, this use case is precisely the reason why skip_outputs currently exists.

It might be helpful to have a flag in the dependency/import (or whatever the name is) block that automatically grabs the configuration from the included terragrunt, to simulate what we have today.

From a design principle perspective, we've been leaning towards being explicit as opposed to implicit, and generally want to avoid adding implicit features if we can. I think if we were implementing our configs ourselves, we would always err on the side of explicitly specifying the imports. This is because it avoids ambiguities where you may have multiple dependency blocks that have the auto_import flag set to true. In this case, it will not be 100% clear the merge order of each config and thus how overrides work. By being explicit, not only do you know exactly which imports are being used, but you also have control over the merge order by adjusting how the properties are passed to the merge function.

brikis98 · 2019-12-03T09:48:01Z

Alright, so is anyone up for opening a new PR that updates the dependency feature to support parsing the terragrunt.hcl of other modules and allowing you to reuse it?

apottere · 2019-12-03T18:03:36Z

This distinction already exists in the skip_outputs property for dependency blocks. In fact, this use case is precisely the reason why skip_outputs currently exists.

As a user, this sort of feels like shoehorning two use-cases into a single block for the sole purpose of not having to support a second block. I think it's fair to say that:

If you're including another terragrunt.hcl, the purpose is to keep your current terragrunt.hcl DRY and re-use terragrunt configuration. I can't think of a use-case where you would also want to treat whatever directory that file is in as a terraform module as well.
If you're including another terragrunt module as a dependency, you most likely don't care about how that module is configured, and you are only concerned with the outputs - essentially treating it as a smarter terraform module. While I can imagine a case where you create a dependency on a module and use both its outputs and configuration, it seems like a more appropriate solution would be to instead have both terragrunt modules depend on a shared terragrunt.hcl file with the real values.

This is because it avoids ambiguities where you may have multiple dependency blocks that have the auto_import flag set to true.

This could also be solved by only allowing a single dependency to auto-import, and it would be opt-in. If you want fine-grained control over merging, just don't add that flag. If you want auto-merging, it would merge your configurations exactly as it does today.

--

One thing I didn't think of in my original comment is that using dependency blocks doesn't solve the use-case of letting child configs affect the behavior of parent configs. We would need to have some concept of variables or inputs for a terragrunt.hcl, and allow you to specify values for them in the dependency block.

apottere · 2019-12-03T18:07:44Z

Another issue we need to figure out is whether or not to allow dependencies in included configs, and if allowed, how to also include those dependencies in your own config.

apottere · 2019-12-03T18:14:51Z

Also, would things like path_relative_to_include still work, or be scrapped?

apottere · 2019-12-03T18:24:26Z

Sorry for all of the individual comments, I just keep thinking of things after the fact.

Another thought: does it make sense for the importer to get access to all of the imported config's locals? It seems like it would be useful to have a way to get some intermediate values from a config you import, but that would kind of violate the concept of those variables being local. Should there be a different way to get values from included config, or maybe just use inputs (even though that has different issues)?

yorinasub17 · 2019-12-03T19:35:36Z

As a user, this sort of feels like shoehorning two use-cases into a single block for the sole purpose of not having to support a second block.

Those are fair points. You are right about the dependency block having an implication that is not desirable for the import use case, where it could be included in the stack tree for xxx-all. For that reason alone, it does make sense to have a different construct for importing the configs than dependency.

I am actually now convinced that we should have an import block that reads in the config and exports the config values, as opposed to dependency which is used for reading in the outputs. @brikis98 What do you think about this?

This could also be solved by only allowing a single dependency to auto-import, and it would be opt-in.

I think adding this in with the limitation would frustrate more users. Psychologically, it is much easier to argue for 1+N imports when we already support 1 import.

doesn't solve the use-case of letting child configs affect the behavior of parent configs.

This was always awkward to me because there is logic in the parent config that depends on who is importing. This resembles monkey patching, which is powerful but usually frowned upon.

The nice thing about the new model is that this is handled by being more explicit in the config. E.g in the current model, we rely on path_relative_to_include to set the remote state key so that we get a different state file for each module. In the new model, you can explicitly override the values in the child:

import "prod_common" {
  config_path = "../terragrunt-prod-common.hcl"
}

remote_state = merge(
  import.prod_common.remote_state,
  {
    config = {
       key = "${get_terragrunt_dir()}/terraform.tfstate"
    }
  },
)

(Note that this assumes merge does a nested merge, which I am not 100% sure it does. But we can probably implement a helper function in terragrunt that allows it.)

Now I know this is slowly getting into flame war territory (explicit vs implicit; convention vs configuration), but I really like the fact that everything is now one way and thus much easier to mentally parse. This also provides a workaround for cyclic imports.

Another example: currently we rely on find_in_parent_folders to look for common variables to include in the root config that is included. This is again, awkward because the result depends on who is importing. We can replace this with a much more explicit construct. Consider the following:

prod
├── region
│   ├── env
│   │   ├── terragrunt.hcl
│   │   └── vpc
│   │       └── terragrunt.hcl
│   └── terragrunt.hcl
└── terragrunt.hcl

prod/terragrunt.hcl

inputs = {
  account_id = 0000000
}

prod/region/terragrunt.hcl

import "account" {
  config_path = "../terragrunt.hcl"
}

inputs = merge(
  import.account.inputs,
  {
    region = "us-east-1"
  },
)

prod/region/env/terragrunt.hcl

import "region" {
  config_path = "../terragrunt.hcl"
}

inputs = merge(
  import.region.inputs,
  {
    env = "prod"
  },
)

prod/region/env/vpc/terragrunt.hcl

import "env" {
  config_path = "../terragrunt.hcl"
}

inputs = merge(
  import.env.inputs,
  {
    # args to module
  },
)

dependencies in included configs, and if allowed, how to also include those dependencies in your own config.

If everything can only be imported one way, then I think we can and should allow dependencies. Having a different construct for import use case also avoids the complexities here of when to resolve those dependencies.

does it make sense for the importer to get access to all of the imported config's locals?

If we assume that imports are only one way, then I think this makes sense and is not much mental overhead to process. The only reason why I was against auto merging/making available locals across includes was because the way include and locals was implemented now meant that it was hard to enforce locals to be available one way, so we would have to make child locals available in parents, which adds to the mental overhead to track that. If you can only access locals one way, through an explicit import, then I think there is not much mental overhead to parse that, and thus does not seem too dangerous to allow.

yorinasub17 · 2019-12-03T19:42:26Z

Another reason why allowing locals across imports in the new model feels better than the old model: the parent locals are namespaced by the import name, so the parent locals are explicitly referenced vs implicitly referenced. A similar version that probably would feel just as good in the old model (that I didn't think of before) is if you can reference the parent locals on the include block; e.g include.locals.region.

apottere · 2019-12-03T21:34:13Z

I'm definitely a fan of explicit vs implicit, as long as it doesn't get in the way. For context, a lot of our terragrunt configs have ended up looking something like this:

.
├── account-a
│   ├── region-a
│   │   ├── env-a
│   │   │   ├── ecs
│   │   │   │   └── terragrunt.hcl
│   │   │   └── vpc
│   │   │       └── terragrunt.hcl
│   │   └── env-b
│   │       ├── ecs
│   │       │   └── terragrunt.hcl
│   │       └── vpc
│   │           └── terragrunt.hcl
│   └── region-b
│       ├── env-a
│       │   ├── ecs
│       │   │   └── terragrunt.hcl
│       │   └── vpc
│       │       └── terragrunt.hcl
│       └── env-b
│           ├── ecs
│           │   └── terragrunt.hcl
│           └── vpc
│               └── terragrunt.hcl
├── account-b
│   ├── region-a
│   │   ├── env-a
│   │   │   ├── ecs
│   │   │   │   └── terragrunt.hcl
│   │   │   └── vpc
│   │   │       └── terragrunt.hcl
│   │   └── env-b
│   │       ├── ecs
│   │       │   └── terragrunt.hcl
│   │       └── vpc
│   │           └── terragrunt.hcl
│   └── region-b
│       ├── env-a
│       │   ├── ecs
│       │   │   └── terragrunt.hcl
│       │   └── vpc
│       │       └── terragrunt.hcl
│       └── env-b
│           ├── ecs
│           │   └── terragrunt.hcl
│           └── vpc
│               └── terragrunt.hcl
├── src
│   ├── ecs
│   └── vpc
└── terragrunt.hcl

The leaf terragrunt.hcl files are no more than a "marker" file, and this is the exact contents (in the simplest case):

include {
  path = find_in_parent_folders()
}

All of the heavy lifting is done in the parent terragrunt.hcl, with the information gained from path_relative_to_include. This setup enables us to have identical infrastructure in all accounts/regions/environments when desired, and we can drop other variable files elsewhere in the tree to change the behavior of a subtree (like a region or account), and allows us to run xxx-all in the account directories to apply all of the infrastructure in the account.

While auto-import would help keep our "marker" files simple, I'm definitely open to other solutions too. One that I can think of off the top of my head is moving everything that affects terraform (remote_state, inputs, etc) into a single block so it's easy to pass along. Then, our parent config could do a bunch of fancy logic:

locals {
    ...
}

terraform {
    source = ...
    remote_state {
        ...
    }
    inputs = {
        ...
    }
}

And our child configs could look something like this:

import "parent" {
    config_path = find_in_parent_folders()
}

terraform = import.parent.terraform

With a deep-merge helper function, it gets much easier to include all of the parent config except a certain part as well.

This approach also relies heavily on being able to affect the behavior of the config based on where it's imported from. I'm all for making that behavior change explicit, though. Just like terraform modules, we could have variables specified in the config that must be supplied (or have defaults) in order to import it. This would remove a lot of the "magic" we have right now while still supporting the use-case.

Consider the following example:

# terraform/us-east-1/prod/vpc/terragrunt.hcl
import "parent" {
    config_path = find_in_parent_folders()
    imported_from = get_terragrunt_dir()
}

# terraform/terragrunt.hcl
vars {
    imported_from
}

locals {
    relative_path = get_relative_path(get_terragrunt_dir(), var.imported_from)
}

Obviously how the variables are defined and used is up for debate, but something like this would allow you to explicitly change the behavior of an imported config with expected results.

Another reason why allowing locals across imports in the new model feels better than the old model: the parent locals are namespaced by the import name, so the parent locals are explicitly referenced vs implicitly referenced.

I like that as well, but it still feels weird that you could break another config that includes the current config by changing the name or value of a local. Would it make sense to have a concept like outputs for a terragrunt config so you can refactor the locals while keeping the external contract the same? That would make it explicit what you do and don't expect another config to be able to get from this config.

After typing this all out, its starting to feel like I'm just trying to re-create all of the features of a terraform module in terragrunt... not sure if that's a good or bad thing.

brikis98 · 2019-12-04T12:21:19Z

I am actually now convinced that we should have an import block that reads in the config and exports the config values, as opposed to dependency which is used for reading in the outputs. @brikis98 What do you think about this?

Makes sense to me.

This approach also relies heavily on being able to affect the behavior of the config based on where it's imported from. I'm all for making that behavior change explicit, though. Just like terraform modules, we could have variables specified in the config that must be supplied (or have defaults) in order to import it.

What you're really defining then is not a config file (something relatively static), but a function that can be called from other places. Functions have inputs, which affect their behavior, and outputs, which are the data they return to the rest of the world.

So to implement the usage pattern where the parent config has almost all the logic, the pattern you're describing might look like this:

import "common" {
  path = "../../common/terragrunt.hcl"
}

terraform = common.some_function(input1, input2)

That's the explicit version of what you're describing... But I'm not sure we want to go down that path?

After typing this all out, its starting to feel like I'm just trying to re-create all of the features of a terraform module in terragrunt... not sure if that's a good or bad thing.

Yup. The reality is that Terragrunt exists only to work around weaknesses in Terraform... And it would be far better if Terraform didn't have those weaknesses in the first place. I wrote up a mini RFC before that basically gets Terragrunt to do what we want from Terraform: #759.

IMO, this approach is DRY, explicit, and easy to understand and maintain. People didn't seem too enthused about it though 😁

apottere · 2019-12-04T15:37:03Z

What you're really defining then is not a config file (something relatively static), but a function that can be called from other places. Functions have inputs, which affect their behavior, and outputs, which are the data they return to the rest of the world.

So to implement the usage pattern where the parent config has almost all the logic, the pattern you're describing might look like this:

Yeah, what I'm looking for is a function in the theoretical sense - inputs get computed and make an output. However, what I really think I want is not an HCL function, but something like a data source from terraform. Since "imports" would no longer be "parents", I think it's reasonable for them to be parameterized - and maybe "imports" is the wrong terminology at that point. Data sources are a good analogy because they're essentially pure functions - you put in inputs, and you can use the resulting outputs without side-effects.

An un-parameterized import is very simple to understand, but without parameters its extremely limited on how DRY it can really keep your code. At some point you'll have to make the last-mile adjustments to the config, and you'll most likely do that by using copy and paste in the files that import it.

Yup. The reality is that Terragrunt exists only to work around weaknesses in Terraform... And it would be far better if Terraform didn't have those weaknesses in the first place. I wrote up a mini RFC before that basically gets Terragrunt to do what we want from Terraform: #759.

That's pretty much exactly what I'm looking for, it's unfortunate it didn't gain much traction. Having a module syntax for actual terraform modules, and something like a data source for creating DRY remote_state blocks, while still retaining the dependency block for including other terragrunt modules would be amazing. I can't really think of a use-case that isn't covered by a setup like that.

brikis98 · 2019-12-05T22:24:41Z

However, what I really think I want is not an HCL function, but something like a data source from terraform.

Yup, but the data sources in Terraform are implemented as functions in Go, not in Terraform 😁

That's pretty much exactly what I'm looking for, it's unfortunate it didn't gain much traction.

It seems like the main complaints were:

A single .hcl files per env (e.g., staging.hcl, prod.hcl) would be too huge and unwieldy. This is true, which is why I suggested that Terragrunt could load all *.hcl files in a folder, so instead of a single staging.hcl, you could have staging-vpc.hcl, staging-data-stores.hcl, staging-services.hcl, etc. Of course, all the modules in all of those files could still reference each other as if they were all in one file, just like references in *.tf files with normal Terraform.
Having separate environments and components in separate folders makes it harder to deploy the wrong thing. E.g., It's very easy to hit up-arrow or CTRL+R and enter, and run apply or destroy on the wrong thing. Whereas with folders, that's less likely to happen. I believe @yorinasub17 brought this one up. I agree with it, but it seems like a small cost to pay for some large gains. Also, if your terminal commands include cd commands (e.g., cd ../foo && terraform apply), then CTRL+R is just as dangerous. We could even offer a nice CLI UX where, when you run terragrunt apply, it shows you a list of the modules it found in the .hcl files and lets you use your keyboard to select the one (or ones) to apply.

apottere · 2019-12-06T16:03:59Z

Yup, but the data sources in Terraform are implemented as functions in Go, not in Terraform 😁

I was talking more about the UX, and less about the implementation. Obviously data sources are functions somehwere 😛

It seems like the main complaints were:

Yeah, both of those issues make sense. I definitely think that separate files would be necessary to keep things clean, but it would be really nice for them to have access to the other modules in the directory without explicitly specifying dependency locations - that way dependencies in-environment can be discovered naturally by references the way terraform does it.

As far as "hitting up + enter destroying things accidentally" goes, why not require the non xxx-all commands to specify a single module (edit: if there's more than just the default terragrunt.hcl in the directory)? Then it could have the same behavior as today where it prompts for running the command on dependencies as well. e.g.:

$ terraform apply vpc

jfunnell · 2020-01-29T22:13:47Z

FYI I was a huge fan of the single folder approach, and still am. I think it's worth trying and would definitely use it for all of my projects.
It almost feels like vanilla terraform except without all the drawbacks of using --target + a unified state file (which our team tried, and it sucks)

yorinasub17 · 2020-01-30T01:04:52Z

I wrote up an RFC for the idea proposed in #858 (comment). Would appreciate feedback from those following this PR as it is an alternative approach to addressing the problem that globals tries to address.

michelzanini · 2020-09-09T13:01:43Z

I believe that the RFC is the way to go. With imports more problems are solved than when using just globals.

nediamond · 2021-01-14T20:24:06Z

test/fixture-config-graph/terragrunt.hcl

+  source-prefix = "src-"
+}
+globals {
+  region = "us-west-2"


7nwwrkdnakht3 · 2021-12-29T17:11:35Z

I know it's been a while, is this still something that is active?

dudicoco · 2022-01-03T10:16:02Z

@7nwwrkdnakht3 try the new deep merge option within the include block: https://terragrunt.gruntwork.io/docs/reference/config-blocks-and-attributes/#include

apottere added 5 commits September 6, 2019 18:25

Add POC for resolving variables with the terraform DAG

53571f7

Add comment explaining steps

826aeed

Update resolution to support include

7f8c5a7

Merge branch 'master' into graph-variable-resolution

bddabc6

Fix import

13d8626

apottere requested review from autero1, brikis98 and eak12913 as code owners September 9, 2019 22:47

apottere changed the title ~~[WIP] Use a DAG to resolve locals, globals, and include~~ [WIP] Use a DAG to resolve locals, globals, and include Sep 9, 2019

apottere mentioned this pull request Sep 9, 2019

Feature Request: Shared and overridable variables (globals?) #814

Open

jakauppila mentioned this pull request Sep 12, 2019

Terraform 0.12 map merging #744

Closed

yorinasub17 reviewed Sep 27, 2019

View reviewed changes

First pass on README

6da35be

yorinasub17 reviewed Oct 18, 2019

View reviewed changes

IMBurbank mentioned this pull request Nov 16, 2019

Only one level of includes is allowed #303

Open

yorinasub17 mentioned this pull request Dec 3, 2019

[Question] - Referencing inputs in child terragrunt.hcl #967

Closed

nediamond reviewed Jan 14, 2021

View reviewed changes

test/fixture-config-graph/terragrunt.hcl

source-prefix = "src-"

}

globals {

region = "us-west-2"

Copy link

nediamond Jan 14, 2021

Choose a reason for hiding this comment

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

west?

	globals := evaluatorGlobals{
	globalEvaluatorConfig := evaluatorGlobals{


		variables := target.Expr.Variables()

		if variables == nil \|\| len(variables) <= 0 {

	if variables == nil \|\| len(variables) <= 0 {
	if variables == nil \|\| len(variables) == 0 {

[WIP] Use a DAG to resolve locals, globals, and include #858

Are you sure you want to change the base?

[WIP] Use a DAG to resolve locals, globals, and include #858

Conversation

apottere commented Sep 9, 2019

apottere commented Sep 9, 2019

brikis98 commented Sep 12, 2019

yorinasub17 commented Sep 12, 2019

apottere commented Sep 13, 2019

apottere commented Sep 13, 2019

apottere commented Sep 13, 2019

yorinasub17 left a comment

Choose a reason for hiding this comment

yorinasub17 Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apottere commented Oct 8, 2019

apottere commented Oct 15, 2019

yorinasub17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apottere commented Oct 30, 2019

brikis98 commented Oct 30, 2019

brikis98 commented Dec 2, 2019

apottere commented Dec 2, 2019 • edited Loading

tomalok commented Dec 3, 2019 • edited Loading

yorinasub17 commented Dec 3, 2019

brikis98 commented Dec 3, 2019

apottere commented Dec 3, 2019 • edited Loading

apottere commented Dec 3, 2019

apottere commented Dec 3, 2019

apottere commented Dec 3, 2019

yorinasub17 commented Dec 3, 2019

yorinasub17 commented Dec 3, 2019

apottere commented Dec 3, 2019

brikis98 commented Dec 4, 2019 • edited Loading

apottere commented Dec 4, 2019

brikis98 commented Dec 5, 2019

apottere commented Dec 6, 2019 • edited Loading

jfunnell commented Jan 29, 2020

yorinasub17 commented Jan 30, 2020

michelzanini commented Sep 9, 2020

Choose a reason for hiding this comment

7nwwrkdnakht3 commented Dec 29, 2021

dudicoco commented Jan 3, 2022

yorinasub17 Sep 27, 2019 •

edited

Loading

apottere commented Dec 2, 2019 •

edited

Loading

tomalok commented Dec 3, 2019 •

edited

Loading

apottere commented Dec 3, 2019 •

edited

Loading

brikis98 commented Dec 4, 2019 •

edited

Loading

apottere commented Dec 6, 2019 •

edited

Loading