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.

Sign up for GitHub

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

Jump to bottom

Update go-getter client options #111

Merged
merged 5 commits into from Jun 9, 2022
Merged

Update go-getter client options #111

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
da23d8b
Update go-getter client options to disable symlinks
nywilken May 25, 2022
ee2fe87
Set resonable timeouts for remote storage getters
nywilken Jun 1, 2022
ffa44ec
Replace default Getters with explicit list of Getters
nywilken Jun 2, 2022
faee09d
Update CHANGELOG
nywilken Jun 2, 2022
900cdcf
Bump default to accommodate long downloads
nywilken Jun 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 34 additions & 6 deletions multistep/commonsteps/step_download.go
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@ import (
"path/filepath"
"runtime"
"strings"
"time"

gcs "github.com/hashicorp/go-getter/gcs/v2"
s3 "github.com/hashicorp/go-getter/s3/v2"
Expand Down Expand Up @@ -56,12 +57,39 @@ type StepDownload struct {
}

var defaultGetterClient = getter.Client{
Getters: getter.Getters,
}

func init() {
defaultGetterClient.Getters = append(defaultGetterClient.Getters, new(gcs.Getter))
defaultGetterClient.Getters = append(defaultGetterClient.Getters, new(s3.Getter))
// Disable writing and reading through symlinks
DisableSymlinks: true,
// The order of the Getters in the list may affect the result
// depending if the Request.Src is detected as valid by multiple getters
Getters: []getter.Getter{
Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

&getter.GitGetter{
Timeout: 5 * time.Minute,
Detectors: []getter.Detector{
new(getter.GitHubDetector),
new(getter.GitDetector),
new(getter.BitBucketDetector),
new(getter.GitLabDetector),
},
},
&getter.HgGetter{
Timeout: 5 * time.Minute,
},
new(getter.SmbClientGetter),
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Should this getter have a timeout? I see this getter only exists in v2, and doesn't have that option currently. Just curious to your thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - I think it should. It was not caught in the go-getter updates. I will follow up with a separate PR for that change.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR opened for the SmbClientGetter hashicorp/go-getter#369

new(getter.SmbMountGetter),
&getter.HttpGetter{
Netrc: true,
XTerraformGetDisabled: true,
HeadFirstTimeout: 10 * time.Second,
ReadTimeout: 30 * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

The other getter timeouts are 5 minutes, while this one is essentially 30 seconds. Maybe it'd make more sense to have a uniform timeout across the getters, or somehow make these configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea of the uniform timeout. TBH 30 seconds seems really short given the use of go-getter in Packer. I struggled a bit to find good defaults. But ultimately think that going high 30 minutes is a good base line. We can make these configurable for the step so that plugins could override if needed.

That said there is an open issue for improving iso_config to support some addition go-getter Headers, which could also include setting some of these values. Taking a page out of Nomad's book here.

},
new(getter.FileGetter),
&gcs.Getter{
Timeout: 5 * time.Minute,
},
&s3.Getter{
Timeout: 5 * time.Minute,
},
},
}

func (s *StepDownload) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {
Expand Down