diff --git a/cmd/olympus/actions/app.go b/cmd/olympus/actions/app.go index ef3e6b86..8a23436d 100644 --- a/cmd/olympus/actions/app.go +++ b/cmd/olympus/actions/app.go @@ -160,12 +160,12 @@ func App(conf *config.Config) (*buffalo.App, error) { if err != nil { return nil, err } + lister := download.NewVCSLister(goBin, fs) st := stash.New(mf, storage) dpOpts := &download.Opts{ - Storage: storage, - Stasher: st, - GoBinPath: goBin, - Fs: fs, + Storage: storage, + Stasher: st, + Lister: lister, } dp := download.New(dpOpts) diff --git a/cmd/proxy/actions/app_proxy.go b/cmd/proxy/actions/app_proxy.go index 0fdcc8e6..0b4ced24 100644 --- a/cmd/proxy/actions/app_proxy.go +++ b/cmd/proxy/actions/app_proxy.go @@ -49,13 +49,14 @@ func addProxyRoutes( if err != nil { return err } + + lister := download.NewVCSLister(goBin, fs) st := stash.New(mf, s, stash.WithPool(goGetWorkers), stash.WithSingleflight) dpOpts := &download.Opts{ - Storage: s, - Stasher: st, - GoBinPath: goBin, - Fs: fs, + Storage: s, + Stasher: st, + Lister: lister, } dp := download.New(dpOpts, addons.WithPool(protocolWorkers)) diff --git a/pkg/download/list_merge_test.go b/pkg/download/list_merge_test.go new file mode 100644 index 00000000..849d6494 --- /dev/null +++ b/pkg/download/list_merge_test.go @@ -0,0 +1,158 @@ +package download + +import ( + "bytes" + "context" + "errors" + "io/ioutil" + "testing" + + athenserr "github.com/gomods/athens/pkg/errors" + "github.com/gomods/athens/pkg/storage" + "github.com/gomods/athens/pkg/storage/mem" + "github.com/stretchr/testify/require" +) + +const testOp athenserr.Op = "vcsLister.List" +const testModName = "happy tags" + +type listMergeTest struct { + name string + newStorage func() (storage.Backend, error) + module string + goVersions []string + goErr error + strVersions []string + strErr error + expected []string + expectedErr error +} + +type storageMock struct { + storage.Backend + versions []string + err error +} + +func (s *storageMock) List(ctx context.Context, module string) ([]string, error) { + return s.versions, s.err +} + +var listMergeTests = []listMergeTest{ + { + name: "go list full and storage full", + newStorage: mem.NewStorage, + goVersions: []string{"v1.0.0", "v1.0.2", "v1.0.3"}, + goErr: nil, + strVersions: []string{"v1.0.0", "v1.0.1", "v1.0.2"}, + expected: []string{"v1.0.0", "v1.0.1", "v1.0.2", "v1.0.3"}, + expectedErr: nil, + }, + { + name: "go list full and storage empty", + newStorage: mem.NewStorage, + goVersions: []string{"v1.0.0", "v1.0.1", "v1.0.2"}, + goErr: nil, + strVersions: []string{}, + strErr: nil, + expected: []string{"v1.0.0", "v1.0.1", "v1.0.2"}, + expectedErr: nil, + }, + { + name: "go list repo not found and storage full", + newStorage: mem.NewStorage, + goVersions: nil, + goErr: errors.New("remote: Repository not found"), + strVersions: []string{"v1.0.0", "v1.0.1", "v1.0.2"}, + strErr: nil, + expected: []string{"v1.0.0", "v1.0.1", "v1.0.2"}, + expectedErr: nil, + }, + { + name: "go list repo not found and storage empty", + newStorage: mem.NewStorage, + goVersions: nil, + goErr: errors.New("remote: Repository not found"), + strVersions: []string{}, + strErr: nil, + expected: nil, + expectedErr: athenserr.E(testOp, athenserr.M(testModName), athenserr.KindNotFound, errors.New("remote: Repository not found")), + }, + { + name: "unexpected go err", + newStorage: mem.NewStorage, + goVersions: nil, + goErr: errors.New("unexpected error"), + strVersions: []string{"1.1.1"}, + strErr: nil, + expected: nil, + expectedErr: athenserr.E(testOp, errors.New("unexpected error")), + }, + { + name: "unexpected storage err", + newStorage: func() (storage.Backend, error) { return &storageMock{err: errors.New("unexpected error")}, nil }, + goVersions: []string{"1.1.1"}, + goErr: nil, + strVersions: nil, + strErr: errors.New("unexpected error"), + expected: nil, + expectedErr: athenserr.E(testOp, errors.New("unexpected error")), + }, +} + +type listerMock struct { + versions []string + err error +} + +func (l *listerMock) List(mod string) (*storage.RevInfo, []string, error) { + return nil, l.versions, l.err +} + +func TestListMerge(t *testing.T) { + ctx := context.Background() + bts := []byte("123") + clearStorage := func(st storage.Backend, module string, versions []string) { + for _, v := range versions { + st.Delete(ctx, module, v) + } + } + + for _, tc := range listMergeTests { + t.Run(tc.name, func(t *testing.T) { + s, err := tc.newStorage() + if err != nil { + t.Fatal(err) + } + for _, v := range tc.strVersions { + s.Save(ctx, testModName, v, bts, ioutil.NopCloser(bytes.NewReader(bts)), bts) + } + defer clearStorage(s, testModName, tc.strVersions) + dp := New(&Opts{s, nil, &listerMock{versions: tc.goVersions, err: tc.goErr}}) + list, err := dp.List(ctx, testModName) + + if ok := testErrEq(tc.expectedErr, err); !ok { + t.Fatalf("expected err: %v, got: %v", tc.expectedErr, err) + } + if tc.expectedErr != nil { + require.Equal(t, athenserr.Kind(tc.expectedErr), athenserr.Kind(err)) + } + require.ElementsMatch(t, tc.expected, list, "expected list: %v, got: %v", tc.expected, list) + }) + } +} + +func testErrEq(a, b error) bool { + if a == nil && b == nil { + return true + } + + if (a == nil) != (b == nil) { + return false + } + + if a.Error() != b.Error() { + return false + } + return true +} diff --git a/pkg/download/protocol.go b/pkg/download/protocol.go index 33182aa5..c7959c1e 100644 --- a/pkg/download/protocol.go +++ b/pkg/download/protocol.go @@ -1,21 +1,13 @@ package download import ( - "bytes" "context" - "encoding/json" - "fmt" "io" - "os/exec" - "time" - "github.com/gomods/athens/pkg/config" "github.com/gomods/athens/pkg/errors" - "github.com/gomods/athens/pkg/module" "github.com/gomods/athens/pkg/observ" "github.com/gomods/athens/pkg/stash" "github.com/gomods/athens/pkg/storage" - "github.com/spf13/afero" ) // Protocol is the download protocol which mirrors @@ -42,10 +34,9 @@ type Wrapper func(Protocol) Protocol // Opts specifies download protocol options to avoid long func signature. type Opts struct { - Storage storage.Backend - Stasher stash.Stasher - GoBinPath string - Fs afero.Fs + Storage storage.Backend + Stasher stash.Stasher + Lister UpstreamLister } // New returns a full implementation of the download.Protocol @@ -54,7 +45,7 @@ type Opts struct { // The wrappers are applied in order, meaning the last wrapper // passed is the Protocol that gets hit first. func New(opts *Opts, wrappers ...Wrapper) Protocol { - var p Protocol = &protocol{opts.Storage, opts.Stasher, opts.GoBinPath, opts.Fs} + var p Protocol = &protocol{opts.Storage, opts.Stasher, opts.Lister} for _, w := range wrappers { p = w(p) } @@ -63,88 +54,49 @@ func New(opts *Opts, wrappers ...Wrapper) Protocol { } type protocol struct { - s storage.Backend - stasher stash.Stasher - goBinPath string - fs afero.Fs + s storage.Backend + stasher stash.Stasher + lister UpstreamLister } func (p *protocol) List(ctx context.Context, mod string) ([]string, error) { const op errors.Op = "protocol.List" ctx, span := observ.StartSpan(ctx, op.String()) defer span.End() - lr, err := p.list(op, mod) - if err != nil { - return nil, err + + strList, sErr := p.s.List(ctx, mod) + // if we got an unexpected storage err then we can not guarantee that the end result contains all versions + // a tag or repo could have been deleted + if sErr != nil { + return nil, errors.E(op, sErr) + } + _, goList, goErr := p.lister.List(mod) + isUnexpGoErr := goErr != nil && !errors.IsRepoNotFoundErr(goErr) + // if i.e. github is unavailable we should fail as well so that the behavior of the proxy is stable. + // otherwise we will get different results the next time because i.e. GH is up again + if isUnexpGoErr { + return nil, errors.E(op, goErr) } - return lr.Versions, nil + isRepoNotFoundErr := goErr != nil && errors.IsRepoNotFoundErr(goErr) + storageEmpty := len(strList) == 0 + if isRepoNotFoundErr && storageEmpty { + return nil, errors.E(op, errors.M(mod), errors.KindNotFound, goErr) + } + + return union(goList, strList), nil } func (p *protocol) Latest(ctx context.Context, mod string) (*storage.RevInfo, error) { const op errors.Op = "protocol.Latest" ctx, span := observ.StartSpan(ctx, op.String()) defer span.End() - lr, err := p.list(op, mod) + lr, _, err := p.lister.List(mod) if err != nil { return nil, errors.E(op, err) } - return &storage.RevInfo{ - Time: lr.Time, - Version: lr.Version, - }, nil -} - -type listResp struct { - Path string - Version string - Versions []string `json:",omitempty"` - Time time.Time -} - -func (p *protocol) list(op errors.Op, mod string) (*listResp, error) { - hackyPath, err := afero.TempDir(p.fs, "", "hackymod") - if err != nil { - return nil, errors.E(op, err) - } - defer p.fs.RemoveAll(hackyPath) - err = module.Dummy(p.fs, hackyPath) - if err != nil { - return nil, errors.E(op, err) - } - - cmd := exec.Command( - p.goBinPath, - "list", "-m", "-versions", "-json", - config.FmtModVer(mod, "latest"), - ) - cmd.Dir = hackyPath - stdout := &bytes.Buffer{} - stderr := &bytes.Buffer{} - cmd.Stdout = stdout - cmd.Stderr = stderr - - gopath, err := afero.TempDir(p.fs, "", "athens") - if err != nil { - return nil, errors.E(op, err) - } - defer module.ClearFiles(p.fs, gopath) - cmd.Env = module.PrepareEnv(gopath) - - err = cmd.Run() - if err != nil { - err = fmt.Errorf("%v: %s", err, stderr) - return nil, errors.E(op, err) - } - - var lr listResp - err = json.NewDecoder(stdout).Decode(&lr) - if err != nil { - return nil, errors.E(op, err) - } - - return &lr, nil + return lr, nil } func (p *protocol) Info(ctx context.Context, mod, ver string) ([]byte, error) { @@ -203,3 +155,23 @@ func (p *protocol) Zip(ctx context.Context, mod, ver string) (io.ReadCloser, err return zip, nil } + +// union concatenates two version lists and removes duplicates +func union(list1, list2 []string) []string { + if list1 == nil { + list1 = []string{} + } + if list2 == nil { + list2 = []string{} + } + list := append(list1, list2...) + unique := []string{} + m := make(map[string]struct{}) + for _, v := range list { + if _, ok := m[v]; !ok { + unique = append(unique, v) + m[v] = struct{}{} + } + } + return unique +} diff --git a/pkg/download/protocol_test.go b/pkg/download/protocol_test.go index de8f2f23..268c0053 100644 --- a/pkg/download/protocol_test.go +++ b/pkg/download/protocol_test.go @@ -39,7 +39,7 @@ func getDP(t *testing.T) Protocol { t.Fatal(err) } st := stash.New(mf, s) - return New(&Opts{s, st, goBin, fs}) + return New(&Opts{s, st, NewVCSLister(goBin, fs)}) } type listTest struct { @@ -57,6 +57,7 @@ var listTests = []listTest{ { name: "no tags", path: "github.com/athens-artifacts/no-tags", + tags: []string{}, }, } @@ -268,7 +269,7 @@ func TestDownloadProtocol(t *testing.T) { } mp := &mockFetcher{} st := stash.New(mp, s) - dp := New(&Opts{s, st, "", afero.NewMemMapFs()}) + dp := New(&Opts{s, st, nil}) ctx := context.Background() var eg errgroup.Group diff --git a/pkg/download/upstream_lister.go b/pkg/download/upstream_lister.go new file mode 100644 index 00000000..5c26d4c0 --- /dev/null +++ b/pkg/download/upstream_lister.go @@ -0,0 +1,85 @@ +package download + +import ( + "bytes" + "encoding/json" + "fmt" + "os/exec" + "time" + + "github.com/gomods/athens/pkg/config" + "github.com/gomods/athens/pkg/errors" + "github.com/gomods/athens/pkg/module" + "github.com/gomods/athens/pkg/storage" + "github.com/spf13/afero" +) + +// UpstreamLister retrieves a list o available module versions from upstream i.e. VCS, Olympus +type UpstreamLister interface { + List(mod string) (*storage.RevInfo, []string, error) +} + +type listResp struct { + Path string + Version string + Versions []string `json:",omitempty"` + Time time.Time +} + +type vcsLister struct { + goBinPath string + fs afero.Fs +} + +func (l *vcsLister) List(mod string) (*storage.RevInfo, []string, error) { + const op errors.Op = "vcsLister.List" + hackyPath, err := afero.TempDir(l.fs, "", "hackymod") + if err != nil { + return nil, nil, errors.E(op, err) + } + defer l.fs.RemoveAll(hackyPath) + err = module.Dummy(l.fs, hackyPath) + if err != nil { + return nil, nil, errors.E(op, err) + } + + cmd := exec.Command( + l.goBinPath, + "list", "-m", "-versions", "-json", + config.FmtModVer(mod, "latest"), + ) + cmd.Dir = hackyPath + stdout := &bytes.Buffer{} + stderr := &bytes.Buffer{} + cmd.Stdout = stdout + cmd.Stderr = stderr + + gopath, err := afero.TempDir(l.fs, "", "athens") + if err != nil { + return nil, nil, errors.E(op, err) + } + defer module.ClearFiles(l.fs, gopath) + cmd.Env = module.PrepareEnv(gopath) + + err = cmd.Run() + if err != nil { + err = fmt.Errorf("%v: %s", err, stderr) + return nil, nil, errors.E(op, err) + } + + var lr listResp + err = json.NewDecoder(stdout).Decode(&lr) + if err != nil { + return nil, nil, errors.E(op, err) + } + rev := storage.RevInfo{ + Time: lr.Time, + Version: lr.Version, + } + return &rev, lr.Versions, nil +} + +// NewVCSLister creates an UpstreamLister which uses VCS to fetch a list of available versions +func NewVCSLister(goBinPath string, fs afero.Fs) UpstreamLister { + return &vcsLister{goBinPath: goBinPath, fs: fs} +} diff --git a/pkg/errors/gocmd.go b/pkg/errors/gocmd.go new file mode 100644 index 00000000..2a002236 --- /dev/null +++ b/pkg/errors/gocmd.go @@ -0,0 +1,9 @@ +package errors + +import ( + "strings" +) + +func IsRepoNotFoundErr(err error) bool { + return strings.Contains(err.Error(), "remote: Repository not found") +} diff --git a/pkg/storage/fs/lister.go b/pkg/storage/fs/lister.go index fa44bc5f..c832ef61 100644 --- a/pkg/storage/fs/lister.go +++ b/pkg/storage/fs/lister.go @@ -16,12 +16,11 @@ func (l *storageImpl) List(ctx context.Context, module string) ([]string, error) loc := l.moduleLocation(module) fileInfos, err := afero.ReadDir(l.filesystem, loc) if err != nil { - kind := errors.KindUnexpected if os.IsNotExist(err) { - kind = errors.KindNotFound + return []string{}, nil } - return nil, errors.E(op, errors.M(module), err, kind) + return nil, errors.E(op, errors.M(module), err, errors.KindUnexpected) } ret := []string{} for _, fileInfo := range fileInfos { diff --git a/pkg/storage/gcp/gcp_test.go b/pkg/storage/gcp/gcp_test.go index 9623835a..d19fbb57 100644 --- a/pkg/storage/gcp/gcp_test.go +++ b/pkg/storage/gcp/gcp_test.go @@ -89,7 +89,8 @@ func (g *GcpTests) TestNotFounds() { }) g.T().Run("List not found", func(t *testing.T) { - _, err := g.store.List(g.context, "nothing/to/see/here") - r.True(errors.IsNotFoundErr(err)) + list, err := g.store.List(g.context, "nothing/to/see/here") + r.NoError(err) + r.Equal(0, len(list)) }) } diff --git a/pkg/storage/gcp/lister.go b/pkg/storage/gcp/lister.go index e3ec3748..4f2b1a3d 100644 --- a/pkg/storage/gcp/lister.go +++ b/pkg/storage/gcp/lister.go @@ -18,11 +18,7 @@ func (s *Storage) List(ctx context.Context, module string) ([]string, error) { if err != nil { return nil, errors.E(op, err, errors.M(module)) } - versions := extractVersions(paths) - if len(versions) < 1 { - return nil, errors.E(op, "empty versions list", errors.M(module), errors.KindNotFound) - } - return versions, nil + return extractVersions(paths), nil } func extractVersions(paths []string) []string { diff --git a/pkg/storage/minio/lister.go b/pkg/storage/minio/lister.go index 64ea7368..31f94dc6 100644 --- a/pkg/storage/minio/lister.go +++ b/pkg/storage/minio/lister.go @@ -31,10 +31,6 @@ func (l *storageImpl) List(ctx context.Context, module string) ([]string, error) } ret := []string{} - if len(dict) == 0 { - return ret, errors.E(op, errors.M(module), errors.KindNotFound) - } - for ver := range dict { ret = append(ret, ver) } diff --git a/pkg/storage/mongo/lister.go b/pkg/storage/mongo/lister.go index a032d213..3451b5b5 100644 --- a/pkg/storage/mongo/lister.go +++ b/pkg/storage/mongo/lister.go @@ -27,10 +27,6 @@ func (s *ModuleStore) List(ctx context.Context, module string) ([]string, error) } versions := make([]string, len(result)) - if len(result) == 0 { - return versions, errors.E(op, errors.M(module), errors.KindNotFound) - } - for i, r := range result { versions[i] = r.Version } diff --git a/pkg/storage/storage_tests/module_storage/storage_test.go b/pkg/storage/storage_tests/module_storage/storage_test.go index 8b1442d8..89cfc34b 100644 --- a/pkg/storage/storage_tests/module_storage/storage_test.go +++ b/pkg/storage/storage_tests/module_storage/storage_test.go @@ -66,7 +66,6 @@ func (d *TestSuites) SetupTest() { d.mod = []byte("123") d.zip = []byte("456") d.info = []byte("789") - } func TestModuleStorages(t *testing.T) { @@ -111,9 +110,9 @@ func (d *TestSuites) testKindNotFound(ts storage.TestSuite) { r.Error(err, hrn) r.Equal(errors.KindNotFound, errors.Kind(err), hrn) - _, err = s.List(ctx, mod) - r.Error(err, hrn) - r.Equal(errors.KindNotFound, errors.Kind(err), hrn) + vs, err := s.List(ctx, mod) + r.NoError(err) + r.Equal(0, len(vs)) _, err = s.Zip(ctx, mod, ver) r.Error(err, hrn)