#2517 fix parsing cache key construction

[untcha]

Description

Preface

With v0.38.9 our HCL parsing cache contribution from issue #2203 (PR #2204) was added to terragrunt which improved the parsing speed of HCL files by introducing an in-memory cache:
https://github.com/gruntwork-io/terragrunt/releases/tag/v0.38.9

For detailed information, please refer to #2203 and #2204

Motivation for this issue

During our daily work, we encountered that the new parsing cache key has a bug which occurs only in certain, rare conditions.
Specifically, it only occurs if a Terragrunt module .hcl file depends on itself in a different folder, which was previously not the case and therefor couldn't been tested in our environments.

Solution

Adding a missing parameter (filename) in the cache key construction of TerragruntConfigFromPartialConfigString

Documentation

n/a

TODOs

Please ensure all of these TODOs are completed before asking for a review.

• Ensure the branch is named correctly with the issue number.
• Keep the changes backward compatible where possible.
• Run the pre-commit checks successfully.
• Run the relevant tests successfully.

Related Issues

Fixes #2517

Related to #2203 and #2204

[maunzCache]

I don't want to vote against this MR in general, but adding the filename will have an major effect on the caching implementation. The logic was to cache duplicate blocks of code and not cache individual files. Adding the filename will break the initial idea of that because you will only cache files. While this will still boost reoccurring dependency parsing, it does not have the benefit of different files containing the same code logic.

In addition, i want to point out that there was no consideration in changing the test suite for the implementation to confirm that the changes have an impact or not. Also missing some reproduction guide for the bug.

I'd recommend in understanding this MR as feature request and not a (bug) fix.

[denis256]

Hi,
I agree with the previous note - will be helpful to have test that will show improvements added by this PR

[maunzCache]

Hi, I agree with the previous note - will be helpful to have test that will show improvements added by this PR

This is also a nice case for the benchmarks that we included in the initial MR ;)

[untcha]

Hi Kevin (@maunzCache), hi Denis (@denis256),

in general I agree with Kevin. Adding the filename is against the initial idea.

But in our case the following issue occurs. I need to explain a bit about the environment.
Kevin should know it (since he worked for us in the past), but I guess not the latest changes.

One remark: performance is not an issue. The performance decreased only slightly (tested by Merlin)

Explaining our environment and dependencies

In the following picture you can see a snippet which shows a simple representation of our terragrunt environment:

The important part is the multi-region config in this example account: aws-account-001
We have eu-central-1 and eu-west-3 both using a so called baseline-common module.
Both terragrunt.hcl files are Symlinks to the one and only baseline-common.hcl in the default-hcls folder.

In there we have a dependency block to a kms key.

The config_path is built by checking if we are in eu-central-1 if yes then the multiregion key
is generated and later used in eu-central-1 by using this path "../../global/baseline-common-global"

If not, then we are in eu-west-3 and then this path is used "../../eu-central-1/baseline-common” and in eu-west-3 the replica kms key.

dependency "baseline_common_kms_key" {
  config_path = local.region == "eu-central-1" ? "../../global/baseline-common-global" : "../../eu-central-1/baseline-common"
  mock_outputs = {
    default_cmk_arn    = "arn:aws:kms:eu-central-1:..."
    cloudtrail_cmk_arn = "arn:aws:kms:eu-central-1:..."
  }
}

Reproducing the issue without and with the filename parameter in the cacheKey

Test 1:

• terragrunt version v0.45.2 (NO FORK!)
• TERRAGRUNT_USE_PARTIAL_PARSE_CONFIG_CACHE=true
• terragrunt run-all init in /terragrunt-environment/dev3/spoke/aws-spoke-dev3-a/eu-west-3/baseline-common

Test 1 result: init NOT successful

time=2023-04-12T16:24:29+02:00 Executing: terragrunt run-all init --terragrunt-ignore-external-dependencies  --terragrunt-parallelism 20

time=2023-04-12T16:24:31+02:00 level=error msg=Found a dependency cycle between modules: /Users/untcha/.../terragrunt-environment/dev3/spoke/aws-spoke-dev3-a/eu-central-1/baseline-common -> /Users/untcha/.../terragrunt-environment/dev3/spoke/aws-spoke-dev3-a/eu-central-1/baseline-common
time=2023-04-12T16:24:31+02:00 level=error msg=Unable to determine underlying exit code, so Terragrunt will exit with error code 1

…/.../terragrunt-environment/dev3/spoke/aws-spoke-dev3-a/eu-west-3/baseline-common [test] terragrunt run-all init —terragrunt-ignore-external-dependencies  --terragrunt-parallelism 20

Test 2:

• terragrunt version v0.45.2 (NO FORK!)
• TERRAGRUNT_USE_PARTIAL_PARSE_CONFIG_CACHE=false
• terragrunt run-all init in /terragrunt-environment/dev3/spoke/aws-spoke-dev3-a/eu-west-3/baseline-common

Test 2 result: init successful

Test 3:

• terragrunt version v0.45.2.1 (forked version with filename param)
• TERRAGRUNT_USE_PARTIAL_PARSE_CONFIG_CACHE=true
• terragrunt run-all init in /terragrunt-environment/dev3/spoke/aws-spoke-dev3-a/eu-west-3/baseline-common

Test 3 result: init successful

[denis256]

Hello,
will be very helpful to provide an example repository where this issue occurs

I tried to reproduce a similar setup in https://github.com/denis256/terragrunt-tests/tree/master/issue-2518 but still can't get Found a dependency cycle between modules errors during Terragrunt execution

[untcha]

Hi @denis256,

thanks a lot for your effort. We will try to provide a repo with an environment to reproduce.
This will take us a bit of time.

Thanks a lot!
Alex

[untcha]

Hi @denis256,

we spent some time on stripping down one of our environments to make it as small as possible with a good use case to show the error. Since this was still too much. One of our developers (@jkarkoszka) tried the other way round and created a very small example repository to show exactly our error.

Here you can find this sandbox repo: https://github.com/jkarkoszka/terragrunt-environment-for-pr-2518

The repository also includes two terragrunt binaries. The original v0.44.5 and the forked one from us, which includes the change from this PR. So it should be very easy without configuration to clone the repo and follow the instructions to reproduce the error.

Our current approach to get the region is to retrieve it from the directory path. We thought that this is maybe not the way gruntwork would propose it to do. @sdahlen-reply tried the approach to retrieve the region from a region.hcl file. The result is the same dependency cycle with that approach.
You can find this in the branch region-from-file

Thanks a lot!

Thanks to Jakub (@jkarkoszka) and Sebastian (@sdahlen-reply)

[denis256]

The original issue is that the cached value of TerragruntConfig already has an evaluated value of config_path which leads to a cycle error between dependencies

https://github.com/jkarkoszka/terragrunt-environment-for-pr-2518/blob/main/default-hcls/baseline-common.hcl#L18

[denis256]

Raised separated issue #2643 to implement tracking of HCL cache performance