#667 iam_role parsing before evaluation of other HCL blocks #1807

denis256 · 2021-09-14T13:41:18Z

Updated HCL parsing first to extract iam_role and use obtained value in the evaluation of HCL blocks

Example:

$ aws sts get-caller-identity
{
    "UserId": "AAA",
    "Account": "BBB",
    "Arn": "arn:aws:iam::YYYY:/app0"
}

$ cat terragrunt.hcl
iam_role = "arn:aws:iam::ZZZZ:user/app666"

locals {
  var = run_cmd("echo", "get_aws_account_id", "${get_aws_account_id()}")
}

Before:

$ terragrunt init
....
get_aws_account_id YYYY
....

After this PR:

$ terragrunt init
...
get_aws_account_id ZZZZ
...

Fix for issue: #667

yorinasub17

This looks like a good start! I think the general idea of parsing iam_role early in the process before others makes sense, but there are a few gotchas/edge cases that this implementation doesn't handle. See the comments below for more details.

yorinasub17 · 2021-09-14T17:34:07Z

config/config_partial.go

+	if flags.IamRole != nil {
+		terragruntOptions.IamRole = *flags.IamRole
+	}


I think this should check if terragruntOptions.IamRole is already set or not, since we should prefer to use the CLI passed in option over the local configuration.

yorinasub17 · 2021-09-14T17:39:09Z

config/config_partial.go

+	contextExtensions := EvalContextExtensions{}
+	flags := terragruntFlags{}
+	err := decodeHcl(hclFile, filename, &flags, terragruntOptions, contextExtensions)
+	if err != nil {
+		return nil, nil, TrackInclude{}, err
+	}


Doing this parsing here means that we won't honor iam_role set on parent configurations that are included. You'll need to implement a special version of PartialParseConfigFile that bypasses locals and only parses include + iam_role to handle this.

Make sense, will look in this dirrection

Simplified test for get_aws_account_id

denis256 · 2021-09-15T08:24:48Z

Behaviour with current changes:

$ cat terragrunt.hcl
remote_state {
  backend = "local"
  generate = {
    path      = "${get_aws_account_id()}.txt"
    if_exists = "overwrite"
  }
  config = {
    path = "terraform.tfstate"
  }
}

include "issue667" {
  path = "../issue-667-include/vars.hcl"
}
$ cat ../issue-667-include/vars.hcl
iam_role = "arn:aws:iam::AAAA:role/terragrunttest"

$ aws sts get-caller-identity
{
    "UserId": "X",
    "Account": "Y",
    "Arn": "arn:aws:iam::BBBB:user/terragrunttest2"
}

$ terragrunt init
....

$ ls AAAA.txt 
AAAA.txt

So get_aws_account_id() resolved with value from iam_role included from other file, without this change will be created BBBB.txt

config/config.go

brikis98 · 2021-09-15T09:53:28Z

config/config.go

@@ -545,6 +545,17 @@ func ParseConfigFile(filename string, terragruntOptions *options.TerragruntOptio
 		return nil, err
 	}

+	// If IamRole wasn't provided, and if it is config file, try first to decode IamRole
+	if include == nil && terragruntOptions.IamRole == "" {
+		iamConfig, err := PartialParseConfigString(configString, terragruntOptions, include, filename, []PartialDecodeSectionType{TerragruntFlags})


What happens if the iam_role param depends on a local var? Or a dependency block? Or anything else that hasn't been parsed yet? In other words, do we need to update the docs on parse order to call out any new limitations here?

PartialParseConfigString handles local and include, but not dependency, so yes indeed we need to update the parsing order docs to indicate this.

brikis98 · 2021-09-15T09:54:18Z

test/integration_test.go

+	stderr := bytes.Buffer{}
+
+	// Invoke terragrunt and verify used IAM role
+	err = runTerragruntCommand(t, fmt.Sprintf("terragrunt init --terragrunt-working-dir %s", TEST_FIXTURE_READ_IAM_ROLE), &stdout, &stderr)


Will this actually try to assume the fake IAM role and lead to a permissions error?

Yes, and it will fail to create state file with IAM role ID

denis256 · 2021-09-15T10:17:20Z

Moved to draft since this implementation caused more integration tests to fail

denis256 · 2021-09-15T15:31:52Z

My local issue, the same number of tests don't pass with or without this change

denis256 · 2021-09-16T07:28:01Z

In the current implementation, it adds a +1 evaluation of HCL code at the beginning, fetching from result TerragruntFlags / IamRole and doing another revaluation.

It will work in cases of remote state definition but may generate some unexpected output in cases like:

iam_role = "arn:aws:iam::${local.id}:user/terragrunt"
locals {
  id = "6666"
  var = run_cmd("echo", "get_aws_account_id", "${get_aws_account_id()}")
}

It will print:

get_aws_account_id AAAA
get_aws_account_id 6666

AAAA from the first evaluation when iam_role was empty and was loaded default value, and 6666 from the second evaluation when iam_role was defined

brikis98 · 2021-09-28T13:37:46Z

@yorinasub17 If you're still digging into Terragrunt issues, could you review this one?

yorinasub17

Updates LGTM! I think we are very close. Just a few more nits to address and I think we can get this going.

The side effect of additional parsing of locals is a potential cause for concern, but I think it would actually work out given that I think most configurations would limit the amount of side effects in locals. What is more of a concern is the backward incompatible nature of this feature.

Currently, before this PR, get_aws_account_id will always return the value based on the original credentials. Now, this will change to the value after iam_role is assumed. In most use cases, this is probably not a huge issue and could be worked around.

I think where this gets super tricky is if users had a config like the following:

iam_role = "arn:aws:iam::${get_aws_account_id()}:role/terragrunttest"

Currently as implemented, what would happen is that in the initial parse for iam_role, iam_role would resolve get_account_id based on the original credentials, but when it goes through to do the full parse, get_account_id would now be based on the assumed iam_role. That means that potentially, the iam_role could change depending on which stage of parsing it is in.

This probably isn't a big deal for functions like get_account_id, where the value doesn't change even after assuming the role, but what about the following config?

# Where 333333333 is another account
iam_role = "arn:aws:iam::333333333:role/from-${get_aws_account_id()}"

In this case, get_aws_account_id() will return 333333333 in the full parse, which means it will assume a different IAM role when it runs terraform.

That said, I think this is fine for now because I feel like the above case will be rare in practice. But it makes me wonder if this is a fundamentally flawed feature, and whether we should consider separating configuration of cloud authentication out of the main terragrunt.hcl so that it can be parsed without side effects.

@brikis98 any thoughts on the above analysis of the implications here?

yorinasub17 · 2021-09-28T15:00:31Z

config/config.go

@@ -545,6 +545,17 @@ func ParseConfigFile(filename string, terragruntOptions *options.TerragruntOptio
 		return nil, err
 	}

+	// If IamRole wasn't provided, and if it is config file, try first to decode IamRole
+	if include == nil && terragruntOptions.IamRole == "" {
+		iamConfig, err := PartialParseConfigString(configString, terragruntOptions, include, filename, []PartialDecodeSectionType{TerragruntFlags})


PartialParseConfigString handles local and include, but not dependency, so yes indeed we need to update the parsing order docs to indicate this.

yorinasub17 · 2021-09-28T15:01:32Z

config/config.go

+	// Initial evaluation of configuration to load flags like IamRole which will be used for final parsing
+	// https://github.com/gruntwork-io/terragrunt/issues/667
+	if include == nil && terragruntOptions.IamRole == "" {
+		iamConfig, err := PartialParseConfigString(configString, terragruntOptions, include, filename, []PartialDecodeSectionType{TerragruntFlags})
+		if err != nil {
+			return nil, err
+		}
+		if iamConfig.IamRole != "" {
+			terragruntOptions.IamRole = iamConfig.IamRole
+		}
+	}


We probably want this logic in ParseConfigString, since there are more entrypoints to that function than ParseConfigFile.

Also, put this in a separate function to make it easier to extend in the future.

Oh do we also need to parse and set iam_assume_role_duration here?

yorinasub17 · 2021-09-28T15:05:36Z

config/config.go

@@ -545,6 +545,18 @@ func ParseConfigFile(filename string, terragruntOptions *options.TerragruntOptio
 		return nil, err
 	}

+	// Initial evaluation of configuration to load flags like IamRole which will be used for final parsing
+	// https://github.com/gruntwork-io/terragrunt/issues/667
+	if include == nil && terragruntOptions.IamRole == "" {


I think you need to omit the include == nil check here, as this means it won't handle a config where the iam_role is defined on the child config that has an include defined.

…ssue-667

yorinasub17

Updates LGTM! Just had one nit on the function name. I'll kick off a regression build.

Note that before merge, I'd like to make sure @brikis98 has a chance to sanity check the backward incompatible implications.

yorinasub17 · 2021-09-29T18:21:35Z

config/config.go

+// extractIamRole - extract IAM role details from Terragrunt flags block
+func extractIamRole(configString string, terragruntOptions *options.TerragruntOptions, filename string) error {


NIT: Rename to setIAMRole, to indicate that this function has side effects (modifying attribute on terragruntOptions.

yorinasub17

Build passed and last update LGTM, so only thing remaining is sanity check from Jim!

brikis98 · 2021-09-30T10:24:31Z

docs/_docs/04_reference/built-in-functions.md

+  * Output contains multiple times `uuid1` and `uuid2` because during HCL evaluation each `run_cmd` in `locals` is evaluated multiple times and random argument generated from `uuid()` save cached value under different key each time
+  * Output contains multiple times `uuid3`, +1 more output comparing to `uuid1` and `uuid2` - because `uuid3` is declared in locals and inputs which add one more evaluation 


NIT: are these changes related to this PR?

Yes. It's the result of the change in the parsing order.

brikis98 · 2021-09-30T10:38:40Z

The side effect of additional parsing of locals is a potential cause for concern, but I think it would actually work out given that I think most configurations would limit the amount of side effects in locals. What is more of a concern is the backward incompatible nature of this feature.

Currently, before this PR, get_aws_account_id will always return the value based on the original credentials. Now, this will change to the value after iam_role is assumed. In most use cases, this is probably not a huge issue and could be worked around.

I think where this gets super tricky is if users had a config like the following:
iam_role = "arn:aws:iam::${get_aws_account_id()}:role/terragrunttest"
Currently as implemented, what would happen is that in the initial parse for iam_role, iam_role would resolve get_account_id based on the original credentials, but when it goes through to do the full parse, get_account_id would now be based on the assumed iam_role. That means that potentially, the iam_role could change depending on which stage of parsing it is in.

This probably isn't a big deal for functions like get_account_id, where the value doesn't change even after assuming the role, but what about the following config?
# Where 333333333 is another account
iam_role = "arn:aws:iam::333333333:role/from-${get_aws_account_id()}"
In this case, get_aws_account_id() will return 333333333 in the full parse, which means it will assume a different IAM role when it runs terraform.

Oof, you're right, this does feel messy.

But it makes me wonder if this is a fundamentally flawed feature, and whether we should consider separating configuration of cloud authentication out of the main terragrunt.hcl so that it can be parsed without side effects.

Yea, I've been having lots of second thoughts about:

iam_role
iam_assume_role_duration
get_aws_account_id()
get_aws_caller_identity_arn()
get_aws_caller_identity_user_id()

And for that matter, the way we've implemented Auto Init for S3 buckets, DynamoDB tables, etc.

They all feel like partial implementations that don't fully solve the problem; they also feel too cloud-specific and are a pain to maintain.

I wonder if what we really need is:

Either remove all auth from Terragrunt and require that it's handled externally. Or, alternatively, have first-class auth functionality, but handled separately than the rest of the config. This would need a design.
Either remove all "data fetching" from Terragrunt and require that it's handled externally. Or, alternatively, have first-class data source support: conceptually, I imagine a separate Terraform module with a bunch of data sources and output variables, we run apply on that first, and then expose it to your config similar to a dependency block; you can then use all that fetched data in your config, and we run apply on the module you have in source.
Either remove all "Auto Init" for remote state for Terragrunt and require that it's handled externally. Or, alternatively, have first-class remote state support: conceptually, I imagine a separate Terraform module that creates your remote state resources (e.g., S3 bucket, DynamoDB table) and provides info about them via output variables, we run apply on that first, and then expose it to your config similar to a dependency block; you can then use that data in your config, and we run apply on the module you have in source.

But these are all larger changes, prob not super relevant to this PR.

For this PR, the key question, I suppose, is how many users combine the iam_role config with the various data functions:

get_aws_account_id()
get_aws_caller_identity_arn()
get_aws_caller_identity_user_id()

My guess is that 99% of the time, they are used separately, and the intention is (a) assume the specified IAM role and (b) fetch the requested data while authenticated as that IAM role. In the <1% of use cases where the two are combined together, the result will likely be unpredictable. Not sure if our partial parse can detect that and show a warning? If not, then perhaps we document it, merge this PR, and file issues to do the larger clean up mentioned above?

yorinasub17 · 2021-09-30T16:02:51Z

That makes sense.

If not, then perhaps we document it, merge this PR, and file issues to do the larger clean up mentioned above?

Let's move forward with documenting this. @denis256 can you update the docs for the following functions and mention the caveat that the value will change depending on the parsing stage?

get_aws_account_id
get_aws_caller_identity_arn
get_aws_caller_identity_user_id

The source is here: https://github.com/gruntwork-io/terragrunt/blob/master/docs/_docs/04_reference/built-in-functions.md

Separately, I'll go ahead and file an issue capturing the above point: #1840

yorinasub17

Docs updates LGTM! I'm going to merge this in now given our discussion, and release as backward incompatible so we can better highlight the behavior change.

denis256 added 4 commits September 14, 2021 16:00

IamRole decode in base blocks

49121c6

Cleanup

cd9c5ef

Test for loading IAM role from file

4566149

Integration tests cleanup

60b8f91

denis256 marked this pull request as ready for review September 14, 2021 15:09

denis256 requested review from brikis98 and yorinasub17 as code owners September 14, 2021 15:09

Updated description

870009c

yorinasub17 reviewed Sep 14, 2021

View reviewed changes

denis256 added 4 commits September 15, 2021 09:36

Merge remote-tracking branch 'origin/master' into bugfix/issue-667

ef63daa

Upstream merge

fcbe185

Simplified test for get_aws_account_id

IamRole loading during parsing of config files

61f9bf6

Updated description

88d2f58

Updated test assertions since locals +1 time

756cce8

denis256 marked this pull request as draft September 15, 2021 09:14

brikis98 reviewed Sep 15, 2021

View reviewed changes

Added reference to issue name

606612f

denis256 marked this pull request as ready for review September 16, 2021 06:30

denis256 added 2 commits September 16, 2021 10:07

Description update

0eb1cb5

Documentation update

b57bf54

yorinasub17 reviewed Sep 28, 2021

View reviewed changes

denis256 added 4 commits September 28, 2021 20:24

Merge branch 'master' of github.com:denis256/terragrunt into bugfix/i…

8ad9b93

…ssue-667

Merge branch 'master' of github.com:denis256/terragrunt into bugfix/i…

5485272

…ssue-667

IamConfig parsing updates

728a7d2

Extracted IAM parsing as separated function

1a95205

Documentation update

118ef94

denis256 requested review from yorinasub17 and brikis98 September 29, 2021 16:29

yorinasub17 reviewed Sep 29, 2021

View reviewed changes

Renamed extractIamRole to setIAMRole

2db4e5b

yorinasub17 previously approved these changes Sep 29, 2021

View reviewed changes

brikis98 reviewed Sep 30, 2021

View reviewed changes

yorinasub17 mentioned this pull request Sep 30, 2021

Better way to handle cloud authentication and cloud helpers #1840

Open

Documentation update

3efe7f4

denis256 dismissed yorinasub17’s stale review via 3efe7f4 September 30, 2021 17:08

Updated formatting

3129a74

denis256 requested a review from yorinasub17 September 30, 2021 19:44

yorinasub17 approved these changes Oct 1, 2021

View reviewed changes

yorinasub17 merged commit 52ecbbb into gruntwork-io:master Oct 1, 2021

denis256 mentioned this pull request Oct 4, 2021

path_relative_to_include() used with run_cmd() in locals broken in 0.34.0 #1845

Closed

awiesner4 mentioned this pull request Oct 14, 2021

iam_role arn pulled from dependency broken #1870

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#667 iam_role parsing before evaluation of other HCL blocks #1807

#667 iam_role parsing before evaluation of other HCL blocks #1807

denis256 commented Sep 14, 2021 •

edited

Loading

yorinasub17 left a comment

yorinasub17 Sep 14, 2021

yorinasub17 Sep 14, 2021

denis256 Sep 15, 2021

denis256 commented Sep 15, 2021 •

edited

Loading

brikis98 Sep 15, 2021

yorinasub17 Sep 28, 2021

brikis98 Sep 15, 2021

denis256 Sep 15, 2021

denis256 commented Sep 15, 2021

denis256 commented Sep 15, 2021

denis256 commented Sep 16, 2021 •

edited

Loading

brikis98 commented Sep 28, 2021

yorinasub17 left a comment

yorinasub17 Sep 28, 2021

yorinasub17 Sep 28, 2021

yorinasub17 Sep 28, 2021

yorinasub17 Sep 28, 2021

yorinasub17 Sep 28, 2021

yorinasub17 left a comment

yorinasub17 Sep 29, 2021

yorinasub17 left a comment

brikis98 Sep 30, 2021

yorinasub17 Sep 30, 2021

brikis98 commented Sep 30, 2021

yorinasub17 commented Sep 30, 2021 •

edited

Loading

yorinasub17 left a comment

		// extractIamRole - extract IAM role details from Terragrunt flags block
		func extractIamRole(configString string, terragruntOptions *options.TerragruntOptions, filename string) error {

		* Output contains multiple times `uuid1` and `uuid2` because during HCL evaluation each `run_cmd` in `locals` is evaluated multiple times and random argument generated from `uuid()` save cached value under different key each time
		* Output contains multiple times `uuid3`, +1 more output comparing to `uuid1` and `uuid2` - because `uuid3` is declared in locals and inputs which add one more evaluation

#667 iam_role parsing before evaluation of other HCL blocks #1807

#667 iam_role parsing before evaluation of other HCL blocks #1807

Conversation

denis256 commented Sep 14, 2021 • edited Loading

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

Choose a reason for hiding this comment

denis256 commented Sep 15, 2021 • 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

denis256 commented Sep 15, 2021

denis256 commented Sep 15, 2021

denis256 commented Sep 16, 2021 • edited Loading

brikis98 commented Sep 28, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorinasub17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

brikis98 commented Sep 30, 2021

yorinasub17 commented Sep 30, 2021 • edited Loading

yorinasub17 left a comment

Choose a reason for hiding this comment

denis256 commented Sep 14, 2021 •

edited

Loading

denis256 commented Sep 15, 2021 •

edited

Loading

denis256 commented Sep 16, 2021 •

edited

Loading

yorinasub17 commented Sep 30, 2021 •

edited

Loading