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

Detect interpolation in terragrunt sources and skip if present #7502

Merged

Conversation

dwc0011
Copy link
Contributor

@dwc0011 dwc0011 commented Jun 30, 2023

Fixes #5841 by using a regex to determine if interpolation syntax is used anywhere in a source.

@dwc0011 dwc0011 requested a review from a team as a code owner June 30, 2023 21:27
@github-actions github-actions bot added the L: terraform Terraform packages label Jun 30, 2023
@dwc0011
Copy link
Contributor Author

dwc0011 commented Jun 30, 2023

Changed to do a simple source_string.include?("${") instead of regex, avoiding the Polynomial regular expression used on uncontrolled data would allow invalid nested interpolation through resulting in the same error this fixes.

include will catch any interpolation passed in and not process it.

@dwc0011
Copy link
Contributor Author

dwc0011 commented Jul 7, 2023

@deivid-rodriguez @jeffwidman @jurre Any estimate on when you all will be able to look at this?

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for all the helpful context in #5841.

@jeffwidman jeffwidman force-pushed the dwc0011-terragrunt-interpolation-error branch from b3aab97 to 7dd3187 Compare July 15, 2023 19:20
@jeffwidman jeffwidman enabled auto-merge (squash) July 15, 2023 19:21
@jeffwidman jeffwidman merged commit 7fbd771 into dependabot:main Jul 15, 2023
86 checks passed
dependency_set << build_terragrunt_dependency(file, details)
source = source_from(details)
# Cannot update nil (interpolation sources) or local path modules, skip
next if source.nil? || source[:type] == "path"
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how this code is used, and don't know Terragrunt/Terraform very well.

Obviously we can't update local path modules as deps to the current terragrunt files, but is there any risk that this prevents updating a local path module that's sitting in the same monorepo? Ie, because this stops :dependabot: from parsing the local module, it also prevent Dependabot from updating any remote deps listed in that local module?

I highly doubt that's a risk, so I'm going to merge, but wanted to doublecheck with you @dwc0011 since you're the expert.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it would prevent it, exactly... There simply is no way for Dependabot to know how the Terragrunt interpolation resolves. The resolution depends on the Terragrunt config at runtime, and may not even be consistent. So when the user is using Terragrunt interpolation this way, Dependabot simply can't use its recursive feature to update anything in that other path anyway.

The workaround for the user would be just to specify both paths in the Dependabot config, so then Dependabot processes both. Which honestly is my preference anyway; the recursive feature in Dependabot has been a bit of a pain and I'd rather just disable it. (Though now with grouping it is becoming of more interest...)

Copy link
Contributor Author

@dwc0011 dwc0011 Jul 17, 2023

Choose a reason for hiding this comment

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

@jeffwidman For terraform local path modules, dependabot does not support updating local path modules, so it makes sense that terragrunt local path modules would not be updated as well.
Local Path Module Files are found and traversed by the file fetcher, not the file parser, all files including those where local path modules are found are then passed to the file parser. That said Terragrunt files do not have a fetch local path module at this time. (This could be added in a separate PR)

As mentioned, the file fetcher is responsible for gathering all the files to process including local source module files.
For terraform files see: fetch local path modules.

Since the process is fetch the files, parse, check updatable, and then update; not returning terragrunt local path dependency in the file_parser makes the most sense. We do this for other types that are not supported as well. i.e. local path modules for terraform, interpolation for terragrunt (added by this PR),

Even if the dependency was added in the file parser, it would be removed later in the update checker

Copy link
Member

Choose a reason for hiding this comment

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

Thanks both of you. That's very helpful context, and I appreciate the extra details... Will be useful for any future git blame spelunkers.

brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
…dabot#7502)

Use a regex to determine if interpolation syntax is used anywhere in a source. If so, return `nil` and skip.

Also skip local path dependencies, as they cannot be updated, so no point in processing them.

---------

Co-authored-by: Dennis Carey <dwc0011@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: terraform Terraform packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terragrunt source results in error
3 participants