From 2faebff0c94c36659a19adf5f080cb89f619b3a7 Mon Sep 17 00:00:00 2001 From: Jurre Stender Date: Wed, 16 Dec 2020 13:53:14 +0100 Subject: [PATCH 1/5] Stop parsing unrequired indirect go packages Previously we would run `go list -m -json all` to generate a list of dependencies to attempt to update. The way that this is implemented, it causes _every_ transitive dependency to be listed, even if they are not required by this project. This can happen when a module defines several packages, but only a subset of those packages is used by the current repository. It will also verify that each of those transitive dependencies is available, and this causes issues when we rely on a dependency with private transitive dependencies. We run into the scenario where we have access to any of the transitive dependencies that are used, but since we do not have access to an _unused_ transitive dependency, we run into a `dependency_file_not_resolvable` error. This is especially unfortunate, since we won't attempt to update _any_ indirect dependencies for go_modules anyway. By using `go mod edit -json` instead, we get an easier to parse list of packages that are _actually_ used in this repo. In order to replicate the previous behavior of raising a `DependencyFileNotResolvable` error when some of the packages are not available, we now also run a `go get`, but it's debatable if we even need to do this, since this error will be raised in subsequent steps (in the `FileUpdater`), and not raising it here means that we can consolidate our error handling for this scenario in one place. Co-authored-by: Fatih Arslan --- .../lib/dependabot/go_modules/file_parser.rb | 24 +++++++------------ .../dependabot/go_modules/file_parser_spec.rb | 2 +- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/go_modules/lib/dependabot/go_modules/file_parser.rb b/go_modules/lib/dependabot/go_modules/file_parser.rb index 58e7189308..c2f02dde16 100644 --- a/go_modules/lib/dependabot/go_modules/file_parser.rb +++ b/go_modules/lib/dependabot/go_modules/file_parser.rb @@ -16,17 +16,8 @@ class FileParser < Dependabot::FileParsers::Base def parse dependency_set = Dependabot::FileParsers::Base::DependencySet.new - i = 0 - chunks = module_info.lines. - group_by { |line| line == "{\n" ? i += 1 : i } - deps = chunks.values.map { |chunk| JSON.parse(chunk.join) } - - deps.each do |dep| - # The project itself appears in this list as "Main" - next if dep["Main"] - - dependency = dependency_from_details(dep) - dependency_set << dependency if dependency + required_packages.each do |dep| + dependency_set << dependency_from_details(dep) end dependency_set.dependencies @@ -65,8 +56,8 @@ def dependency_from_details(details) ) end - def module_info - @module_info ||= + def required_packages + @required_packages ||= SharedHelpers.in_a_temporary_directory do |path| SharedHelpers.with_git_configured(credentials: credentials) do # Create a fake empty module for each local module so that @@ -78,9 +69,10 @@ def module_info end File.write("go.mod", go_mod_content) + File.write("main.go", "package dummypkg\n func main() {}\n") - command = "go mod edit -print > /dev/null" - command += " && go list -m -json all" + command = "go get > /dev/null" # Verify the packages are installable + command += " && go mod edit -json" # Turn off the module proxy for now, as it's causing issues with # private git dependencies @@ -88,7 +80,7 @@ def module_info stdout, stderr, status = Open3.capture3(env, command) handle_parser_error(path, stderr) unless status.success? - stdout + JSON.parse(stdout)["Require"] rescue Dependabot::DependencyFileNotResolvable # We sometimes see this error if a host times out. # In such cases, retrying (a maximum of 3 times) may fix it. diff --git a/go_modules/spec/dependabot/go_modules/file_parser_spec.rb b/go_modules/spec/dependabot/go_modules/file_parser_spec.rb index 251931c04a..9a175f22af 100644 --- a/go_modules/spec/dependabot/go_modules/file_parser_spec.rb +++ b/go_modules/spec/dependabot/go_modules/file_parser_spec.rb @@ -37,7 +37,7 @@ describe "parse" do subject(:dependencies) { parser.parse } - its(:length) { is_expected.to eq(6) } + its(:length) { is_expected.to eq(5) } describe "top level dependencies" do subject(:dependencies) do From 65b4ab1200d96ecfad9085bd2bfeb614df187836 Mon Sep 17 00:00:00 2001 From: Jurre Stender Date: Fri, 18 Dec 2020 11:54:47 +0100 Subject: [PATCH 2/5] Do not parse indirect go mod dependencies as we cannot update them --- go_modules/lib/dependabot/go_modules/file_parser.rb | 2 +- go_modules/spec/dependabot/go_modules/file_parser_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go_modules/lib/dependabot/go_modules/file_parser.rb b/go_modules/lib/dependabot/go_modules/file_parser.rb index c2f02dde16..e1f010f37a 100644 --- a/go_modules/lib/dependabot/go_modules/file_parser.rb +++ b/go_modules/lib/dependabot/go_modules/file_parser.rb @@ -17,7 +17,7 @@ def parse dependency_set = Dependabot::FileParsers::Base::DependencySet.new required_packages.each do |dep| - dependency_set << dependency_from_details(dep) + dependency_set << dependency_from_details(dep) unless dep["Indirect"] end dependency_set.dependencies diff --git a/go_modules/spec/dependabot/go_modules/file_parser_spec.rb b/go_modules/spec/dependabot/go_modules/file_parser_spec.rb index 9a175f22af..b92f53e500 100644 --- a/go_modules/spec/dependabot/go_modules/file_parser_spec.rb +++ b/go_modules/spec/dependabot/go_modules/file_parser_spec.rb @@ -37,7 +37,7 @@ describe "parse" do subject(:dependencies) { parser.parse } - its(:length) { is_expected.to eq(5) } + its(:length) { is_expected.to eq(3) } describe "top level dependencies" do subject(:dependencies) do From 18d48b4f4c97764ede719f2a975756267da29b1d Mon Sep 17 00:00:00 2001 From: Jurre Stender Date: Tue, 22 Dec 2020 11:55:24 +0100 Subject: [PATCH 3/5] Do not attempt to go get modules when parsing go.mod file Also cleans up any error handling that can only occur from running `go get`, or any other command that actually downloads the modules. --- .../lib/dependabot/go_modules/file_parser.rb | 50 ++----------------- .../dependabot/go_modules/file_parser_spec.rb | 41 +++++---------- 2 files changed, 16 insertions(+), 75 deletions(-) diff --git a/go_modules/lib/dependabot/go_modules/file_parser.rb b/go_modules/lib/dependabot/go_modules/file_parser.rb index e1f010f37a..da4b03f4b5 100644 --- a/go_modules/lib/dependabot/go_modules/file_parser.rb +++ b/go_modules/lib/dependabot/go_modules/file_parser.rb @@ -71,8 +71,7 @@ def required_packages File.write("go.mod", go_mod_content) File.write("main.go", "package dummypkg\n func main() {}\n") - command = "go get > /dev/null" # Verify the packages are installable - command += " && go mod edit -json" + command = "go mod edit -json" # Turn off the module proxy for now, as it's causing issues with # private git dependencies @@ -127,52 +126,9 @@ def go_mod_content end end - GIT_ERROR_REGEX = /go: .*: git fetch .*: exit status 128/m.freeze def handle_parser_error(path, stderr) - case stderr - when /go: .*: unknown revision/m - line = stderr.lines.grep(/unknown revision/).first.strip - handle_github_unknown_revision(line) if line.start_with?("go: github.com/") - raise Dependabot::DependencyFileNotResolvable, line - when /go: .*: unrecognized import path/m - line = stderr.lines.grep(/unrecognized import/).first - raise Dependabot::DependencyFileNotResolvable, line.strip - when /go: errors parsing go.mod/m - msg = stderr.gsub(path.to_s, "").strip - raise Dependabot::DependencyFileNotParseable.new(go_mod.path, msg) - when GIT_ERROR_REGEX - lines = stderr.lines.drop_while { |l| GIT_ERROR_REGEX !~ l } - raise Dependabot::DependencyFileNotResolvable.new, lines.join - else - msg = stderr.gsub(path.to_s, "").strip - raise Dependabot::DependencyFileNotParseable.new(go_mod.path, msg) - end - end - - GITHUB_REPO_REGEX = %r{github.com/[^@]*}.freeze - def handle_github_unknown_revision(line) - mod_path = line.scan(GITHUB_REPO_REGEX).first - return unless mod_path - - # Query for _any_ version of this module, to know if it doesn't exist (or is private) - # or we were just given a bad revision by this manifest - SharedHelpers.in_a_temporary_directory do - SharedHelpers.with_git_configured(credentials: credentials) do - File.write("go.mod", "module dummy\n") - - env = { "GOPRIVATE" => "*" } - _, _, status = Open3.capture3(env, SharedHelpers.escape_command("go get #{mod_path}")) - raise Dependabot::DependencyFileNotResolvable, line if status.success? - - mod_split = mod_path.split("/") - repo_path = if mod_split.size > 3 - mod_split[0..2].join("/") - else - mod_path - end - raise Dependabot::GitDependenciesNotReachable, [repo_path] - end - end + msg = stderr.gsub(path.to_s, "").strip + raise Dependabot::DependencyFileNotParseable.new(go_mod.path, msg) end def rev_identifier?(dep) diff --git a/go_modules/spec/dependabot/go_modules/file_parser_spec.rb b/go_modules/spec/dependabot/go_modules/file_parser_spec.rb index b92f53e500..fef5e79b5a 100644 --- a/go_modules/spec/dependabot/go_modules/file_parser_spec.rb +++ b/go_modules/spec/dependabot/go_modules/file_parser_spec.rb @@ -144,9 +144,8 @@ go_mod.sub("rsc.io/quote", "example.com/not-a-repo") end - it "raises the correct error" do - expect { parser.parse }. - to raise_error(Dependabot::DependencyFileNotResolvable) + it "does not raise an error" do + expect { parser.parse }.not_to raise_error end end @@ -157,11 +156,8 @@ go_mod.sub("rsc.io/quote", invalid_repo) end - it "raises the correct error" do - expect { parser.parse }. - to raise_error(Dependabot::GitDependenciesNotReachable) do |error| - expect(error.dependency_urls).to contain_exactly(invalid_repo) - end + it "does not raise an error" do + expect { parser.parse }.not_to raise_error end end @@ -172,11 +168,8 @@ go_mod.sub("rsc.io/quote v1.4.0", "#{invalid_repo}/v2 v2.0.0") end - it "raises the correct error" do - expect { parser.parse }. - to raise_error(Dependabot::GitDependenciesNotReachable) do |error| - expect(error.dependency_urls).to contain_exactly(invalid_repo) - end + it "does not raise an error" do + expect { parser.parse }.not_to raise_error end end @@ -186,9 +179,8 @@ go_mod.sub("github.com/mattn/go-colorable v0.0.9", "github.com/mattn/go-colorable v0.1234.4321") end - it "raises the correct error" do - expect { parser.parse }. - to raise_error(Dependabot::DependencyFileNotResolvable) + it "does not raise an error" do + expect { parser.parse }.not_to raise_error end end @@ -197,10 +189,7 @@ let(:go_mod_fixture_name) { "parent_module.mod" } it "raises the correct error" do - expect { parser.parse }. - to raise_error(Dependabot::DependencyFileNotResolvable) do |error| - expect(error.message).to include("hmarr/404") - end + expect { parser.parse }.not_to raise_error end end @@ -210,9 +199,8 @@ go_mod.sub("rsc.io/quote v1.4.0", "rsc.io/quote v1.321.0") end - it "raises the correct error" do - expect { parser.parse }. - to raise_error(Dependabot::DependencyFileNotResolvable) + it "does not raise an error" do + expect { parser.parse }.not_to raise_error end end @@ -223,11 +211,8 @@ "github.com/hmarr/404 v0.0.0-20181216014959-b89dc648a159") end - it "raises the correct error" do - expect { parser.parse }. - to raise_error(Dependabot::DependencyFileNotResolvable) do |error| - expect(error.message).to include("hmarr/404") - end + it "does not raise an error" do + expect { parser.parse }.not_to raise_error end end From aabc3e3ebed3ba94f4da4689cb6b6b1b16a12a69 Mon Sep 17 00:00:00 2001 From: Jurre Stender Date: Tue, 22 Dec 2020 14:07:02 +0100 Subject: [PATCH 4/5] Remove unnecessary git credentials and dummypkg --- .../lib/dependabot/go_modules/file_parser.rb | 55 +++++++++---------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/go_modules/lib/dependabot/go_modules/file_parser.rb b/go_modules/lib/dependabot/go_modules/file_parser.rb index da4b03f4b5..215bdda8ed 100644 --- a/go_modules/lib/dependabot/go_modules/file_parser.rb +++ b/go_modules/lib/dependabot/go_modules/file_parser.rb @@ -59,36 +59,33 @@ def dependency_from_details(details) def required_packages @required_packages ||= SharedHelpers.in_a_temporary_directory do |path| - SharedHelpers.with_git_configured(credentials: credentials) do - # Create a fake empty module for each local module so that - # `go list` works, even if some modules have been `replace`d with - # a local module that we don't have access to. - local_replacements.each do |_, stub_path| - Dir.mkdir(stub_path) unless Dir.exist?(stub_path) - FileUtils.touch(File.join(stub_path, "go.mod")) - end - - File.write("go.mod", go_mod_content) - File.write("main.go", "package dummypkg\n func main() {}\n") - - command = "go mod edit -json" - - # Turn off the module proxy for now, as it's causing issues with - # private git dependencies - env = { "GOPRIVATE" => "*" } - - stdout, stderr, status = Open3.capture3(env, command) - handle_parser_error(path, stderr) unless status.success? - JSON.parse(stdout)["Require"] - rescue Dependabot::DependencyFileNotResolvable - # We sometimes see this error if a host times out. - # In such cases, retrying (a maximum of 3 times) may fix it. - retry_count ||= 0 - raise if retry_count >= 3 - - retry_count += 1 - retry + # Create a fake empty module for each local module so that + # `go list` works, even if some modules have been `replace`d with + # a local module that we don't have access to. + local_replacements.each do |_, stub_path| + Dir.mkdir(stub_path) unless Dir.exist?(stub_path) + FileUtils.touch(File.join(stub_path, "go.mod")) end + + File.write("go.mod", go_mod_content) + + command = "go mod edit -json" + + # Turn off the module proxy for now, as it's causing issues with + # private git dependencies + env = { "GOPRIVATE" => "*" } + + stdout, stderr, status = Open3.capture3(env, command) + handle_parser_error(path, stderr) unless status.success? + JSON.parse(stdout)["Require"] + rescue Dependabot::DependencyFileNotResolvable + # We sometimes see this error if a host times out. + # In such cases, retrying (a maximum of 3 times) may fix it. + retry_count ||= 0 + raise if retry_count >= 3 + + retry_count += 1 + retry end end From cdf0fdd2de8a2adb7d959764fb8e434681db46d5 Mon Sep 17 00:00:00 2001 From: Jurre Stender Date: Tue, 22 Dec 2020 16:02:35 +0100 Subject: [PATCH 5/5] Update comment to match new implementation --- go_modules/lib/dependabot/go_modules/file_parser.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go_modules/lib/dependabot/go_modules/file_parser.rb b/go_modules/lib/dependabot/go_modules/file_parser.rb index 215bdda8ed..10b050f56e 100644 --- a/go_modules/lib/dependabot/go_modules/file_parser.rb +++ b/go_modules/lib/dependabot/go_modules/file_parser.rb @@ -60,7 +60,7 @@ def required_packages @required_packages ||= SharedHelpers.in_a_temporary_directory do |path| # Create a fake empty module for each local module so that - # `go list` works, even if some modules have been `replace`d with + # `go mod edit` works, even if some modules have been `replace`d with # a local module that we don't have access to. local_replacements.each do |_, stub_path| Dir.mkdir(stub_path) unless Dir.exist?(stub_path)