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

fix: enable http timeout #1777

Merged
merged 4 commits into from
Apr 4, 2024
Merged

fix: enable http timeout #1777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0d6ed3
fix: enable http timeout
willmurphyscode Apr 1, 2024
09449fa
separate timeouts for listing and database download
willmurphyscode Apr 2, 2024
7a21a6d
assign timeout after checking err is nil
willmurphyscode Apr 2, 2024
c2109ef
add unit test for timeout behavior
willmurphyscode Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions cmd/grype/cli/options/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,33 @@ import (
)

type Database struct {
Dir string `yaml:"cache-dir" json:"cache-dir" mapstructure:"cache-dir"`
UpdateURL string `yaml:"update-url" json:"update-url" mapstructure:"update-url"`
CACert string `yaml:"ca-cert" json:"ca-cert" mapstructure:"ca-cert"`
AutoUpdate bool `yaml:"auto-update" json:"auto-update" mapstructure:"auto-update"`
ValidateByHashOnStart bool `yaml:"validate-by-hash-on-start" json:"validate-by-hash-on-start" mapstructure:"validate-by-hash-on-start"`
ValidateAge bool `yaml:"validate-age" json:"validate-age" mapstructure:"validate-age"`
MaxAllowedBuiltAge time.Duration `yaml:"max-allowed-built-age" json:"max-allowed-built-age" mapstructure:"max-allowed-built-age"`
Dir string `yaml:"cache-dir" json:"cache-dir" mapstructure:"cache-dir"`
UpdateURL string `yaml:"update-url" json:"update-url" mapstructure:"update-url"`
CACert string `yaml:"ca-cert" json:"ca-cert" mapstructure:"ca-cert"`
AutoUpdate bool `yaml:"auto-update" json:"auto-update" mapstructure:"auto-update"`
ValidateByHashOnStart bool `yaml:"validate-by-hash-on-start" json:"validate-by-hash-on-start" mapstructure:"validate-by-hash-on-start"`
ValidateAge bool `yaml:"validate-age" json:"validate-age" mapstructure:"validate-age"`
MaxAllowedBuiltAge time.Duration `yaml:"max-allowed-built-age" json:"max-allowed-built-age" mapstructure:"max-allowed-built-age"`
UpdateAvailableTimeout time.Duration `yaml:"update-available-timeout" json:"update-available-timeout" mapstructure:"update-available-timeout"`
UpdateDownloadTimeout time.Duration `yaml:"update-download-timeout" json:"update-download-timeout" mapstructure:"update-download-timeout"`
}

const (
defaultMaxDBAge time.Duration = time.Hour * 24 * 5
defaultUpdateAvailableTimeout = time.Second * 30
defaultUpdateDownloadTimeout = time.Second * 120
)

func DefaultDatabase(id clio.Identification) Database {
return Database{
Dir: path.Join(xdg.CacheHome, id.Name, "db"),
UpdateURL: internal.DBUpdateURL,
AutoUpdate: true,
ValidateAge: true,
// After this period (5 days) the db data is considered stale
MaxAllowedBuiltAge: time.Hour * 24 * 5,
MaxAllowedBuiltAge: defaultMaxDBAge,
UpdateAvailableTimeout: defaultUpdateAvailableTimeout,
UpdateDownloadTimeout: defaultUpdateDownloadTimeout,
}
}

Expand All @@ -40,5 +50,7 @@ func (cfg Database) ToCuratorConfig() db.Config {
ValidateByHashOnGet: cfg.ValidateByHashOnStart,
ValidateAge: cfg.ValidateAge,
MaxAllowedBuiltAge: cfg.MaxAllowedBuiltAge,
ListingFileTimeout: cfg.UpdateAvailableTimeout,
UpdateTimeout: cfg.UpdateDownloadTimeout,
}
}
22 changes: 17 additions & 5 deletions grype/db/curator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,14 @@ type Config struct {
ValidateByHashOnGet bool
ValidateAge bool
MaxAllowedBuiltAge time.Duration
ListingFileTimeout time.Duration
UpdateTimeout time.Duration
}

type Curator struct {
fs afero.Fs
downloader file.Getter
listingDownloader file.Getter
updateDownloader file.Getter
targetSchema int
dbDir string
dbPath string
Expand All @@ -55,15 +58,23 @@ func NewCurator(cfg Config) (Curator, error) {
dbDir := path.Join(cfg.DBRootDir, strconv.Itoa(vulnerability.SchemaVersion))

fs := afero.NewOsFs()
httpClient, err := defaultHTTPClient(fs, cfg.CACert)
listingClient, err := defaultHTTPClient(fs, cfg.CACert)
willmurphyscode marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return Curator{}, err
}
listingClient.Timeout = cfg.ListingFileTimeout

dbClient, err := defaultHTTPClient(fs, cfg.CACert)
if err != nil {
return Curator{}, err
}
dbClient.Timeout = cfg.UpdateTimeout

return Curator{
fs: fs,
targetSchema: vulnerability.SchemaVersion,
downloader: file.NewGetter(httpClient),
listingDownloader: file.NewGetter(listingClient),
updateDownloader: file.NewGetter(dbClient),
dbDir: dbDir,
dbPath: path.Join(dbDir, FileName),
listingURL: cfg.ListingURL,
Expand Down Expand Up @@ -283,7 +294,7 @@ func (c *Curator) download(listing *ListingEntry, downloadProgress *progress.Man
url.RawQuery = query.Encode()

// go-getter will automatically extract all files within the archive to the temp dir
err = c.downloader.GetToDir(tempDir, listing.URL.String(), downloadProgress)
err = c.updateDownloader.GetToDir(tempDir, listing.URL.String(), downloadProgress)
if err != nil {
return "", fmt.Errorf("unable to download db: %w", err)
}
Expand Down Expand Up @@ -375,7 +386,7 @@ func (c Curator) ListingFromURL() (Listing, error) {
}()

// download the listing file
err = c.downloader.GetFile(tempFile.Name(), c.listingURL)
err = c.listingDownloader.GetFile(tempFile.Name(), c.listingURL)
if err != nil {
return Listing{}, fmt.Errorf("unable to download listing: %w", err)
}
Expand All @@ -390,6 +401,7 @@ func (c Curator) ListingFromURL() (Listing, error) {

func defaultHTTPClient(fs afero.Fs, caCertPath string) (*http.Client, error) {
httpClient := cleanhttp.DefaultClient()
httpClient.Timeout = 30 * time.Second
if caCertPath != "" {
rootCAs := x509.NewCertPool()

Expand Down
89 changes: 86 additions & 3 deletions grype/db/curator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@ package db

import (
"bufio"
"errors"
"fmt"
"io"
"net/http"
"net/http/httptest"
"net/url"
"os"
"os/exec"
"path"
"path/filepath"
"strconv"
"strings"
"syscall"
"testing"
"time"
Expand Down Expand Up @@ -68,13 +71,14 @@ func newTestCurator(tb testing.TB, fs afero.Fs, getter file.Getter, dbDir, metad

require.NoError(tb, err)

c.downloader = getter
c.listingDownloader = getter
c.updateDownloader = getter
c.fs = fs

return c
}

func Test_defaultHTTPClient(t *testing.T) {
func Test_defaultHTTPClientHasCert(t *testing.T) {
tests := []struct {
name string
hasCert bool
Expand Down Expand Up @@ -105,11 +109,16 @@ func Test_defaultHTTPClient(t *testing.T) {
} else {
assert.Nil(t, httpClient.Transport.(*http.Transport).TLSClientConfig)
}

})
}
}

func Test_defaultHTTPClientTimeout(t *testing.T) {
c, err := defaultHTTPClient(afero.NewMemMapFs(), "")
require.NoError(t, err)
assert.Equal(t, 30*time.Second, c.Timeout)
}

func generateCertFixture(t *testing.T) string {
path := "test-fixtures/tls/server.crt"
if _, err := os.Stat(path); !os.IsNotExist(err) {
Expand Down Expand Up @@ -364,3 +373,77 @@ func TestCurator_validateStaleness(t *testing.T) {
})
}
}

func TestCuratorTimeoutBehavior(t *testing.T) {
failAfter := 10 * time.Second
success := make(chan struct{})
errs := make(chan error)
timeout := time.After(failAfter)

hangForeverHandler := func(w http.ResponseWriter, r *http.Request) {
select {} // hang forever
}

ts := httptest.NewServer(http.HandlerFunc(hangForeverHandler))

cfg := Config{
DBRootDir: "",
ListingURL: fmt.Sprintf("%s/listing.json", ts.URL),
CACert: "",
ValidateByHashOnGet: false,
ValidateAge: false,
MaxAllowedBuiltAge: 0,
ListingFileTimeout: 400 * time.Millisecond,
UpdateTimeout: 400 * time.Millisecond,
}

curator, err := NewCurator(cfg)
require.NoError(t, err)

u, err := url.Parse(fmt.Sprintf("%s/some-db.tar.gz", ts.URL))
require.NoError(t, err)

entry := ListingEntry{
Built: time.Now(),
Version: 5,
URL: u,
Checksum: "83b52a2aa6aff35d208520f40dd36144",
}

downloadProgress := progress.NewManual(10)
importProgress := progress.NewManual(10)
stage := progress.NewAtomicStage("some-stage")

runTheTest := func(success chan struct{}, errs chan error) {
_, _, _, err = curator.IsUpdateAvailable()
if err == nil {
errs <- errors.New("expected timeout error but got nil")
return
}
if !strings.Contains(err.Error(), "Timeout exceeded") {
errs <- fmt.Errorf("expected %q but got %q", "Timeout exceeded", err.Error())
return
}

err = curator.UpdateTo(&entry, downloadProgress, importProgress, stage)
if err == nil {
errs <- errors.New("expected timeout error but got nil")
return
}
if !strings.Contains(err.Error(), "Timeout exceeded") {
errs <- fmt.Errorf("expected %q but got %q", "Timeout exceeded", err.Error())
return
}
success <- struct{}{}
}
go runTheTest(success, errs)

select {
case <-success:
return
case err := <-errs:
t.Error(err)
case <-timeout:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're doing a test that combines the calls of IsUpdateAvailable and UpdateTo is there a chance this can race incorrectly?

I guess the gap is so large from the configured timeouts and failAfter (more than 10x) that it doesn't matter.

LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spiffcs I wanted to make sure I understand what you mean by "race incorrectly." I think you're saying that since (1) the test fails on a timeout and (2) the assertion expects a different timeout, we could get the wrong timeout. Is that correct?

I think that since we're effectively racing a 0.8 second timer against a 10 second timer, the test should be pretty stable. I put in the 10 second timer because otherwise removing one of the timeouts causes the test suite to hang for 10 minutes while the overall testing timer runs out, which seems like worse behavior.

Please let me know if I've addressed these concerns adequately and then I'll merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! That's exactly what I considered (0.8 vs 10). Test looks good and I don't think it will give us trouble in the future. LGTM

t.Fatalf("timeout exceeded (%v)", failAfter)
}
}
Loading