From 657c9d04a35197a23667a064804deda38bcc0256 Mon Sep 17 00:00:00 2001 From: Manu Gupta Date: Wed, 31 Oct 2018 13:08:15 -0400 Subject: [PATCH] If a filter file is not found and is in config, throw an error (#788) * When the filter file is missing throw a fatal * Ignore path when file is not specified * Fix build failure on ci * Remove logs * Fix propagate and handle errors * Fix build error * Actually check filter file * Check for errors in the test suite as well * Improve test coverage * Add more tests * Simplify NewFilter function * Address review comments --- cmd/proxy/actions/app.go | 5 ++- pkg/config/proxy.go | 2 +- pkg/middleware/middleware_test.go | 37 +++++++++++++----- pkg/module/filter.go | 33 ++++++++-------- pkg/module/filter_test.go | 63 +++++++++++++++++++++++-------- 5 files changed, 98 insertions(+), 42 deletions(-) diff --git a/cmd/proxy/actions/app.go b/cmd/proxy/actions/app.go index b176d2c3..32e28d2b 100644 --- a/cmd/proxy/actions/app.go +++ b/cmd/proxy/actions/app.go @@ -120,7 +120,10 @@ func App(conf *config.Config) (*buffalo.App, error) { initializeAuth(app) if !conf.Proxy.FilterOff { - mf := module.NewFilter(conf.FilterFile) + mf, err := module.NewFilter(conf.FilterFile) + if err != nil { + lggr.Fatal(err) + } app.Use(mw.NewFilterMiddleware(mf, conf.Proxy.GlobalEndpoint)) } diff --git a/pkg/config/proxy.go b/pkg/config/proxy.go index 6e5dafa1..95872dfe 100644 --- a/pkg/config/proxy.go +++ b/pkg/config/proxy.go @@ -5,7 +5,7 @@ type ProxyConfig struct { StorageType string `validate:"required" envconfig:"ATHENS_STORAGE_TYPE"` GlobalEndpoint string `envconfig:"ATHENS_GLOBAL_ENDPOINT"` // This feature is not yet implemented Port string `validate:"required" envconfig:"PORT"` - FilterOff bool `validate:"required" envconfig:"PROXY_FILTER_OFF"` + FilterOff bool `envconfig:"PROXY_FILTER_OFF"` BasicAuthUser string `envconfig:"BASIC_AUTH_USER"` BasicAuthPass string `envconfig:"BASIC_AUTH_PASS"` ForceSSL bool `envconfig:"PROXY_FORCE_SSL"` diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index e2b9b6ae..3512b258 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -2,9 +2,10 @@ package middleware import ( "encoding/json" - "fmt" + "io/ioutil" "net/http" "net/http/httptest" + "os" "path/filepath" "testing" @@ -28,31 +29,43 @@ var ( testConfigFile = filepath.Join("..", "..", "config.dev.toml") ) -func middlewareFilterApp(filterFile, registryEndpoint string) *buffalo.App { +func middlewareFilterApp(filterFile, registryEndpoint string) (*buffalo.App, error) { h := func(c buffalo.Context) error { return c.Render(200, nil) } a := buffalo.New(buffalo.Options{}) - mf := newTestFilter(filterFile) + mf, err := newTestFilter(filterFile) + if err != nil { + return nil, err + } a.Use(NewFilterMiddleware(mf, registryEndpoint)) a.GET(pathList, h) a.GET(pathVersionInfo, h) - return a + return a, nil } -func newTestFilter(filterFile string) *module.Filter { - f := module.NewFilter(filterFile) +func newTestFilter(filterFile string) (*module.Filter, error) { + f, err := module.NewFilter(filterFile) + if err != nil { + return nil, err + } f.AddRule("github.com/gomods/athens/", module.Include) f.AddRule("github.com/athens-artifacts/no-tags", module.Exclude) f.AddRule("github.com/athens-artifacts", module.Direct) - return f + return f, nil } func Test_FilterMiddleware(t *testing.T) { r := require.New(t) + filter, err := ioutil.TempFile(os.TempDir(), "filter-") + if err != nil { + t.FailNow() + } + defer os.Remove(filter.Name()) + conf, err := config.GetConf(testConfigFile) if err != nil { t.Fatalf("Unable to parse config file: %s", err.Error()) @@ -60,7 +73,14 @@ func Test_FilterMiddleware(t *testing.T) { if conf.Proxy == nil { t.Fatalf("No Proxy configuration in test config") } - app := middlewareFilterApp(conf.FilterFile, conf.Proxy.GlobalEndpoint) + + // Test with a filter file not existing + app, err := middlewareFilterApp("nofsfile", conf.Proxy.GlobalEndpoint) + r.Nil(app, "app should be nil when a file not exisiting") + r.Error(err, "Expected error when a file not existing on the filesystem is given") + + app, err = middlewareFilterApp(filter.Name(), conf.Proxy.GlobalEndpoint) + r.NoError(err, "app should be succesfully created in the test") w := willie.New(app) // Public, expects to be redirected to the global registry endpoint, with and without a trailing slash @@ -114,7 +134,6 @@ type HookTestsSuite struct { } func (suite *HookTestsSuite) SetupSuite() { - fmt.Println("setup") suite.server = httptest.NewServer(&suite.mock) suite.w = willie.New(hookFilterApp(suite.server.URL)) } diff --git a/pkg/module/filter.go b/pkg/module/filter.go index 394c5aa6..dc4e1cf3 100644 --- a/pkg/module/filter.go +++ b/pkg/module/filter.go @@ -29,16 +29,15 @@ type Filter struct { // - // + github.com/a // will exclude all items from communication except github.com/a -func NewFilter(filterFilePath string) *Filter { - rn := newRule(Default) - modFilter := Filter{ - filePath: filterFilePath, +func NewFilter(filterFilePath string) (*Filter, error) { + // Do not return an error if the file path is empty + // Do not attempt to parse it as well. + if filterFilePath == "" { + return nil, nil } - modFilter.root = rn - modFilter.initFromConfig() + return initFromConfig(filterFilePath) - return &modFilter } // AddRule adds rule for specified path @@ -116,13 +115,18 @@ func (f *Filter) getAssociatedRule(path ...string) FilterRule { return f.root.rule } -func (f *Filter) initFromConfig() { - lines, err := getConfigLines(f.filePath) - - if err != nil || len(lines) == 0 { - return +func initFromConfig(filePath string) (*Filter, error) { + lines, err := getConfigLines(filePath) + if err != nil { + return nil, err } + rn := newRule(Default) + f := &Filter{ + filePath: filePath, + } + f.root = rn + for _, line := range lines { split := strings.Split(strings.TrimSpace(line), " ") if len(split) > 2 { @@ -151,6 +155,7 @@ func (f *Filter) initFromConfig() { path := strings.TrimSpace(split[1]) f.AddRule(path, rule) } + return f, nil } func getPathSegments(path string) []string { @@ -179,7 +184,6 @@ func getConfigLines(filterFile string) ([]string, error) { if err != nil { return nil, errors.E(op, err) } - defer f.Close() scanner := bufio.NewScanner(f) @@ -193,6 +197,5 @@ func getConfigLines(filterFile string) ([]string, error) { lines = append(lines, line) } - - return lines, nil + return lines, f.Close() } diff --git a/pkg/module/filter_test.go b/pkg/module/filter_test.go index d7235c54..4393e08d 100644 --- a/pkg/module/filter_test.go +++ b/pkg/module/filter_test.go @@ -1,10 +1,11 @@ package module import ( + "io/ioutil" + "os" "path/filepath" "testing" - "github.com/gomods/athens/pkg/config" "github.com/stretchr/testify/suite" ) @@ -20,10 +21,33 @@ func Test_Filter(t *testing.T) { suite.Run(t, new(FilterTests)) } +func (t *FilterTests) Test_NewFilter() { + r := t.Require() + mf, err := NewFilter("") + r.NoError(err, "When a file name is empty string return no error") + r.Nil(nil, mf) + + mf, err = NewFilter("nofile") + r.Nil(mf) + r.Error(err) + + filter := tempFilterFile(t.T()) + defer os.Remove(filter) + + mf, err = NewFilter(filter) + r.Equal(filter, mf.filePath) + r.NoError(err) + +} + func (t *FilterTests) Test_IgnoreSimple() { r := t.Require() - conf := GetConfLogErr(testConfigFile, t.T()) - f := NewFilter(conf.FilterFile) + + filter := tempFilterFile(t.T()) + defer os.Remove(filter) + + f, err := NewFilter(filter) + r.NoError(err) f.AddRule("github.com/a/b", Exclude) r.Equal(Include, f.Rule("github.com/a")) @@ -35,8 +59,12 @@ func (t *FilterTests) Test_IgnoreSimple() { func (t *FilterTests) Test_IgnoreParentAllowChildren() { r := t.Require() - conf := GetConfLogErr(testConfigFile, t.T()) - f := NewFilter(conf.FilterFile) + + filter := tempFilterFile(t.T()) + defer os.Remove(filter) + + f, err := NewFilter(filter) + r.NoError(err) f.AddRule("github.com/a/b", Exclude) f.AddRule("github.com/a/b/c", Include) @@ -50,8 +78,11 @@ func (t *FilterTests) Test_IgnoreParentAllowChildren() { func (t *FilterTests) Test_OnlyAllowed() { r := t.Require() - conf := GetConfLogErr(testConfigFile, t.T()) - f := NewFilter(conf.FilterFile) + filter := tempFilterFile(t.T()) + defer os.Remove(filter) + + f, err := NewFilter(filter) + r.NoError(err) f.AddRule("github.com/a/b", Include) f.AddRule("", Exclude) @@ -65,8 +96,11 @@ func (t *FilterTests) Test_OnlyAllowed() { func (t *FilterTests) Test_Direct() { r := t.Require() - conf := GetConfLogErr(testConfigFile, t.T()) - f := NewFilter(conf.FilterFile) + filter := tempFilterFile(t.T()) + defer os.Remove(filter) + + f, err := NewFilter(filter) + r.NoError(err) f.AddRule("github.com/a/b/c", Exclude) f.AddRule("github.com/a/b", Direct) f.AddRule("github.com/a", Include) @@ -77,13 +111,10 @@ func (t *FilterTests) Test_Direct() { r.Equal(Exclude, f.Rule("github.com/a/b/c/d")) } -// GetConfLogErr is similar to GetConf, except it logs a failure for the calling test -// if any errors are encountered -func GetConfLogErr(path string, t *testing.T) *config.Config { - c, err := config.GetConf(path) +func tempFilterFile(t *testing.T) (path string) { + filter, err := ioutil.TempFile(os.TempDir(), "filter-") if err != nil { - t.Fatalf("Unable to parse config file: %s", err.Error()) - return nil + t.FailNow() } - return c + return filter.Name() }