Allow configuration of GOPRIVATE #4568

mctofu · 2021-12-28T19:11:31Z

Previously we've hardcoded setting GOPRIVATE=* to prevent failures if private go modules are validated against the public sumdb. That works but incurs a performance penalty when public go modules are updated. This change allows a custom GOPRIVATE to be specified. This could be leveraged in the future to:

Use an unrestricted GOPRIVATE= when checking updates for a public repo
Use a more specific GOPRIVATE=github.com/owner/* when checking updates for a private repo
Use a more customized GOPRIVATE when private registries are defined

Those changes would allow updates to perform more efficiently (less time and less disk usage).

In the first 2 commits I made some minor cleanup to the FileParser. In the past FileParser ran go get and required GOPRIVATE and resolvability error handling. That was later removed so I was able to remove the remaining parts that were no longer required.

I've added some tests for this as well but they aren't particularly effective as they just check that different variations of GOPRIVATE don't have any effect on the output. They might be useful in detecting behavior changes in future go updates but I'd also be OK to remove them if preferred. A better test would require a test case that used private credentials but that would only work in CI. For now I tested manually:

`GOPRIVATE=` fails with private deps

dependabot-core-dev] ~/dependabot-core $ go clean -modcache && bin/dry-run.rb go_modules dependabot-fixtures/go-modules-private-dependency --updater-options goprivate=
=> cloning into /home/dependabot/dependabot-core/tmp/dependabot-fixtures/go-modules-private-dependency
=> parsing dependency files
=> updating 1 dependencies: github.com/dependabot-fixtures/go-modules-private

=== github.com/dependabot-fixtures/go-modules-private (0.0.1)
 => checking for updates 1/1
 => handled error whilst updating github.com/dependabot-fixtures/go-modules-private: git_dependencies_not_reachable {:"dependency-urls"=>["github.com/dependabot-fixtures/go-modules-private"]}

`GOPRIVATE=github.com/dependabot-fixtures/*` succeeds with private deps

[dependabot-core-dev] ~/dependabot-core $ go clean -modcache && bin/dry-run.rb go_modules dependabot-fixtures/go-modules-private-dependency --updater-options goprivate=github.com/dependabot-fixtures/*
=> cloning into /home/dependabot/dependabot-core/tmp/dependabot-fixtures/go-modules-private-dependency
=> parsing dependency files
=> updating 1 dependencies: github.com/dependabot-fixtures/go-modules-private

=== github.com/dependabot-fixtures/go-modules-private (0.0.1)
 => checking for updates 1/1
 => latest available version is 0.0.1
 => latest allowed version is 0.0.1
    (no update needed as it's already up-to-date)

default `GOPRIVATE` succeeds with private deps

[dependabot-core-dev] ~/dependabot-core $ go clean -modcache && bin/dry-run.rb go_modules dependabot-fixtures/go-modules-private-dependency=> cloning into /home/dependabot/dependabot-core/tmp/dependabot-fixtures/go-modules-private-dependency
=> parsing dependency files
=> updating 1 dependencies: github.com/dependabot-fixtures/go-modules-private

=== github.com/dependabot-fixtures/go-modules-private (0.0.1)
 => checking for updates 1/1
 => latest available version is 0.0.1
 => latest allowed version is 0.0.1
    (no update needed as it's already up-to-date)

It only parses the local go.mod and doesn't reach out over the network.

Possibility of this error occurring was removed in 18d48b4

jurre · 2021-12-29T09:18:27Z

How will we determine what value to pass in? I'm wondering if we could instead ask the dependency being updated if it's private somehow, by implementing a Dependabot::Dependency#private? or something, and use the correct value for the GOPRIVATE flag based on that, the benefit would be that the logic is all contained in dependabot-core, instead of having to figure out and pass in the correct value from the outside.

I'm not sure if there's a way to determine which dependency is private in the file parsing step though

mctofu · 2021-12-29T17:08:34Z

How will we determine what value to pass in?

I was planning to determine this based on the repo being updated. If it's a public repo it shouldn't have any private dependencies so we don't need to set GOPRIVATE. It's a private repo we can set it with that knowledge plus any known settings for Dependabot private repo & private registry access.

Checking per dependency would be more time consuming. The go tooling will need to resolve all of them during an update so we'd need to look them all up. I'm also not sure of a good way to tell if a particular dependency is public or private. For a github hosted dependency we might have to reach out to the API to tell if it's public or private.

jurre · 2021-12-31T09:05:10Z

I was planning to determine this based on the repo being updated. If it's a public repo it shouldn't have any private dependencies

Not entirely sure if that's always true, unless it's something that's enforced by go modules that I don't know about? But I agree it is a good assumption to start with 👍

mctofu · 2021-12-31T18:21:54Z

Not entirely sure if that's always true, unless it's something that's enforced by go modules that I don't know about?

It's not enforced by go modules but in terms of Dependabot being able to successfully make updates to the repo it needs to be true. If a public repo did use private dependencies it would need to configure private registry support for Dependabot to work and we could adjust GOPRIVATE based on that.

landongrindheim · 2022-01-04T14:47:09Z

bin/dry-run.rb

+      if o.include?("=")
+        o.split("=", 2).map.with_index do |v, i|
+          if i == 0
+            v.strip.downcase.to_sym


I'm able to reason about this given the context I'm currently working with (reviewing a PR with a description in front of me), but I wonder what the experience will be like for future me.

Is there a way we could provide more context inline (e.g. o.include?("goprivate="))? Or, if this is more generic and meant for parsing several arguments, could we extract this logic into a named method? I'd be hesitant to add yet more methods to this script, but I think the trade-off would be worth it. What do you think?

Added some code comments to describe what each case is expected to handle. This is meant to be generic as each package manager can have its own set of supported options although goprivate is the first to allow setting both the key and value.

I'm holding off on introducing any new methods for now since they'd live pretty far away from this code. We could revisit this if there's an opportunity for reusing the code to handle other args.

landongrindheim · 2022-01-04T14:51:10Z

go_modules/lib/dependabot/go_modules/file_updater.rb

@@ -14,6 +14,7 @@ def initialize(dependencies:, dependency_files:, repo_contents_path: nil,
                     credentials:, options: {})
        super

+        @goprivate = options.fetch(:goprivate, "*")


This PR's description states

Previously we've defaulted to setting GOPRIVATE=* to prevent failures if private go modules are validated against the public sumdb. That works but incurs a performance penalty when public go modules are updated.

As I understand this line, it seems that we're still defaulting to *. Is that what we want?

I can certainly see the case for continuing to do so. At the very least, we're not breaking functionality for users, which seems desirable now. Will we change this default in the future?

Yes, I could have stated that better. Previously we hardcoded GOPRIVATE=* everywhere and now we are providing an option for dependabot-core integrators to use a different value. We'll (GitHub) start setting this to more performant values based on the repo being updated in the near future but in the meantime we still need to preserve the GOPRIVATE=* behavior.

noseglid · 2022-02-02T10:29:53Z

When can this change be expected to land in the Github dependabot?

jurre · 2022-02-02T19:46:56Z

It's been live for about a month now, is something not working for you?

noseglid · 2022-02-03T07:02:33Z

I'm probably misunderstanding something. I can't find it in the configuration options docs.

Looking at this PR, I tried to decipher where to put it, and my dependabot.yml looks like this

version: 2
updates:
- package-ecosystem: gomod
  goprivate: "*"
  directory: "/"
  schedule:
    interval: daily
    time: "08:00"

But then dependabot say;

The property '#/updates/0/' contains additional properties ["goprivate"] outside of the schema when none are allowed

landongrindheim · 2022-02-04T16:09:07Z

👋 Hi @noseglid You shouldn't need to do anything to take advantage of this feature on your Go repos on GitHub 😄 GOPRIVATE is an argument that can be used by integrators when running the go modules parts of Dependabot. It's not a configuration option at this time.

As @jurre said, GitHub's Dependabot service (one of the aforementioned integrators) has been using this for a month now. Please do let us know if you see any issues related to this feature 🙇

mctofu added 5 commits December 27, 2021 16:45

GOPRIVATE not needed for 'go mod edit -json'

e69536c

It only parses the local go.mod and doesn't reach out over the network.

Remove unreachable error handling

4957647

Possibility of this error occurring was removed in 18d48b4

Allow configuration of GOPRIVATE

c365975

Tests to check for GOPRIVATE affecting update behavior

b7cffaf

Allow goprivate config via dry-run

f520d2c

mctofu added the L: go:modules Golang modules label Dec 28, 2021

mctofu marked this pull request as ready for review December 28, 2021 19:23

mctofu requested a review from a team as a code owner December 28, 2021 19:23

mctofu mentioned this pull request Dec 30, 2021

git.apache.org/thrift.git@...: invalid version: unknown revision due to connect: connection timed out (but the repo server isn't down) #4530

Closed

landongrindheim reviewed Jan 4, 2022

View reviewed changes

Add description of updater_options formats

0e7aaed

landongrindheim approved these changes Jan 5, 2022

View reviewed changes

jurre approved these changes Jan 5, 2022

View reviewed changes

mctofu merged commit 8c8dd9d into main Jan 5, 2022

mctofu deleted the mctofu/configure-goprivate branch January 5, 2022 19:25

This was referenced Jan 5, 2022

v0.170.0 #4590

Merged

Incorrect parsing of version into url hostname #4625

Closed

Start using the Go module proxy #1437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration of GOPRIVATE #4568

Allow configuration of GOPRIVATE #4568

mctofu commented Dec 28, 2021 •

edited

Loading

jurre commented Dec 29, 2021 •

edited

Loading

mctofu commented Dec 29, 2021

jurre commented Dec 31, 2021

mctofu commented Dec 31, 2021

landongrindheim Jan 4, 2022

mctofu Jan 4, 2022

landongrindheim Jan 4, 2022

mctofu Jan 4, 2022 •

edited

Loading

noseglid commented Feb 2, 2022

jurre commented Feb 2, 2022

noseglid commented Feb 3, 2022

landongrindheim commented Feb 4, 2022

Allow configuration of GOPRIVATE #4568

Allow configuration of GOPRIVATE #4568

Conversation

mctofu commented Dec 28, 2021 • edited Loading

jurre commented Dec 29, 2021 • edited Loading

mctofu commented Dec 29, 2021

jurre commented Dec 31, 2021

mctofu commented Dec 31, 2021

landongrindheim Jan 4, 2022

Choose a reason for hiding this comment

mctofu Jan 4, 2022

Choose a reason for hiding this comment

landongrindheim Jan 4, 2022

Choose a reason for hiding this comment

mctofu Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

noseglid commented Feb 2, 2022

jurre commented Feb 2, 2022

noseglid commented Feb 3, 2022

landongrindheim commented Feb 4, 2022

mctofu commented Dec 28, 2021 •

edited

Loading

jurre commented Dec 29, 2021 •

edited

Loading

mctofu Jan 4, 2022 •

edited

Loading