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
20 changes: 10 additions & 10 deletions terraform/lib/dependabot/terraform/file_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,11 @@ def parse_terragrunt_files(dependency_set)
modules.each do |details|
next unless details["source"]

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.


dependency_set << build_terragrunt_dependency(file, source)
end
end
end
Expand Down Expand Up @@ -141,15 +145,8 @@ def deprecated_provider?(details)
details.is_a?(String)
end

def build_terragrunt_dependency(file, details)
source = source_from(details)
dep_name =
if Source.from_url(source[:url])
Source.from_url(source[:url]).repo
else
source[:url]
end

def build_terragrunt_dependency(file, source)
dep_name = Source.from_url(source[:url]) ? Source.from_url(source[:url]).repo : source[:url]
version = version_from_ref(source[:ref])

Dependency.new(
Expand Down Expand Up @@ -178,6 +175,8 @@ def source_from(details_hash)
git_source_details_from(bare_source)
when :registry
registry_source_details_from(bare_source)
when :interpolation
return nil
end

source_details[:proxy_url] = raw_source if raw_source != bare_source
Expand Down Expand Up @@ -261,6 +260,7 @@ def version_from_ref(ref)

# rubocop:disable Metrics/PerceivedComplexity
def source_type(source_string)
return :interpolation if source_string.include?("${")
return :path if source_string.start_with?(".")
return :github if source_string.start_with?("github.com/")
return :bitbucket if source_string.start_with?("bitbucket.org/")
Expand Down
86 changes: 86 additions & 0 deletions terraform/spec/dependabot/terraform/file_parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -910,4 +910,90 @@
end
end
end

let(:file_parser) do
described_class.new(
dependency_files: files,
source: source
)
end

let(:files) { project_dependency_files("registry") }
let(:source) { Dependabot::Source.new(provider: "github", repo: "gocardless/bump", directory: "/") }

describe "#source_type" do
subject { file_parser.send(:source_type, source_string) }

context "when the source type is known" do
let(:source_string) { "github.com/org/repo" }

it "returns the correct source type" do
expect(subject).to eq(:github)
end
end

context "when the source type is a registry" do
let(:source_string) { "registry.terraform.io/hashicorp/aws" }

it "returns the correct source type" do
expect(subject).to eq(:registry)
end
end

context "when the source type is an HTTP archive" do
let(:source_string) { "https://example.com/archive.zip?ref=v1.0.0" }

it "returns the correct source type" do
expect(subject).to eq(:http_archive)
end
end

context "when the source type is an interpolation" do
let(:source_string) { "${var.source}" }

it "returns the correct source type" do
expect(subject).to eq(:interpolation)
end
end

context "when the source type is an interpolation at the end" do
let(:source_string) { "git::https://github.com/username/repo.git//path/to/${var.module_name}" }

it "returns the correct source type" do
expect(subject).to eq(:interpolation)
end
end

context "when the source type is an interpolation at the start" do
let(:source_string) { "${var.repo_url}/username/repo.git" }

it "returns the correct source type" do
expect(subject).to eq(:interpolation)
end
end

context "when the source type is an interpolation type with multiple" do
let(:source_string) { "git::https://github.com/${var.username}/${var.repo}//path/to/${var.module_name}" }

it "returns the correct source type" do
expect(subject).to eq(:interpolation)
end
end

context "when the source type is a compound interpolation" do
let(:source_string) { "test/${var.map[${var.key}']" }

it "returns the correct source type" do
expect(subject).to eq(:interpolation)
end
end

context "when the source type is unknown" do
let(:source_string) { "unknown_source" }

it "returns the correct source type" do
expect(subject).to eq(:registry)
end
end
end
end
Loading