From ad400b0d22f2778579adcc3cfb1b49c67de61264 Mon Sep 17 00:00:00 2001 From: Michael Graf Date: Tue, 9 Jun 2015 17:30:23 -0700 Subject: [PATCH 1/7] Adds option to Allow requests on limiter error --- handlers/README.md | 6 ++++ handlers/http.go | 20 +++++++++--- handlers/http_test.go | 72 +++++++++++++++++++++++++++++++++++++++---- limitkeys/README.md | 2 +- main.go | 2 +- 5 files changed, 89 insertions(+), 13 deletions(-) diff --git a/handlers/README.md b/handlers/README.md index c48a93a..6e62b18 100644 --- a/handlers/README.md +++ b/handlers/README.md @@ -5,6 +5,12 @@ ## Usage +```go +const ( + StatusTooManyRequests = 429 // not in net/http package +) +``` + #### func NewHTTPLimiter ```go diff --git a/handlers/http.go b/handlers/http.go index 0a16f6d..53a3a79 100644 --- a/handlers/http.go +++ b/handlers/http.go @@ -12,6 +12,10 @@ import ( "strings" ) +const ( + StatusTooManyRequests = 429 // not in net/http package +) + func stringifyHeaders(headers http.Header) *bytes.Buffer { buf := &bytes.Buffer{} for header, values := range headers { @@ -35,8 +39,9 @@ func stringifyRequest(req common.Request) *bytes.Buffer { } type httpRateLimiter struct { - rateLimiter ratelimiter.RateLimiter - proxy http.Handler + rateLimiter ratelimiter.RateLimiter + proxy http.Handler + AllowOnError bool // Do not limit on errors when true } func (hrl httpRateLimiter) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -46,12 +51,17 @@ func (hrl httpRateLimiter) ServeHTTP(w http.ResponseWriter, r *http.Request) { matches, err := hrl.rateLimiter.Add(request) if err != nil && err != leakybucket.ErrorFull { log.Printf("[%s] ERROR: %s", guid, err) - w.WriteHeader(500) - return + if !hrl.AllowOnError { + log.Printf("[%s] WARNING: bypassing rate limiter due to Error") + w.WriteHeader(http.StatusInternalServerError) + return + } } + addRateLimitHeaders(w, matches) + if err == leakybucket.ErrorFull { - w.WriteHeader(429) + w.WriteHeader(StatusTooManyRequests) return } hrl.proxy.ServeHTTP(w, r) diff --git a/handlers/http_test.go b/handlers/http_test.go index 58e5302..edbe97c 100644 --- a/handlers/http_test.go +++ b/handlers/http_test.go @@ -184,8 +184,8 @@ func TestHandleWhenFull(t *testing.T) { limiter.ServeHTTP(w, r) compareStatusesToHeader(t, w.Header(), statuses) - if w.Code != 429 { - t.Fatalf("expected status 429, received %d", w.Code) + if w.Code != StatusTooManyRequests { + t.Fatalf("expected status %d, received %d", StatusTooManyRequests, w.Code) } limitMock.AssertExpectations(t) } @@ -205,8 +205,8 @@ func TestHandleWhenErrWithStatus(t *testing.T) { limiter.ServeHTTP(w, r) assertNoRateLimitHeaders(t, w.Header()) - if w.Code != 500 { - t.Fatalf("expected status 500, received %d", w.Code) + if w.Code != http.StatusInternalServerError { + t.Fatalf("expected status %d, received %d", http.StatusInternalServerError, w.Code) } limitMock.AssertExpectations(t) } @@ -226,8 +226,68 @@ func TestHandleWhenErrWithoutStatus(t *testing.T) { limiter.ServeHTTP(w, r) assertNoRateLimitHeaders(t, w.Header()) - if w.Code != 500 { - t.Fatalf("expected status 500, received %d", w.Code) + if w.Code != http.StatusInternalServerError { + t.Fatalf("expected status %d, received %d", http.StatusInternalServerError, w.Code) } limitMock.AssertExpectations(t) } + +// Test cases when an error occurs and either value of AllowOnError +var allowOnErrorCases = []struct { + AllowOnError bool + ExpectedCode int + RateLimiterStatus []ratelimiter.Status +}{ + // It should still block if AllowOnError == false + { + false, + http.StatusInternalServerError, + []ratelimiter.Status{}, + }, + // If AllowOnError == true and + { + true, + http.StatusOK, + []ratelimiter.Status{}, + }, + // It should still add headers if AllowOnError == true + { + true, + http.StatusOK, + []ratelimiter.Status{sphinxStatus}, + }, +} + +// Tests the AllowOnError flag feature +func TestAllowOnError(t *testing.T) { + for _, test := range allowOnErrorCases { + limiter := constructHTTPRateLimiter() + limiter.AllowOnError = test.AllowOnError + w := httptest.NewRecorder() + r, err := http.NewRequest("GET", "http://google.com", strings.NewReader("thebody")) + if err != nil { + t.Fatal(err) + } + + // Setup an error case (simulate redis connection error) + limitMock := limiter.rateLimiter.(*MockRateLimiter).Mock + limitMock.On("Add", anyRequest). + Return(test.RateLimiterStatus, errors.New("Expected Testing error - redis.conn error")). + Once() + + proxyMock := limiter.proxy.(*MockProxy).Mock + proxyMock.On("ServeHTTP", w, r).Return().Once() + limiter.ServeHTTP(w, r) + + // If no status, then + if len(test.RateLimiterStatus) == 0 { + assertNoRateLimitHeaders(t, w.Header()) + } else { + compareStatusesToHeader(t, w.Header(), test.RateLimiterStatus) + } + + if w.Code != test.ExpectedCode { + t.Fatalf("expected status %d, received %d", test.ExpectedCode, w.Code) + } + } +} diff --git a/limitkeys/README.md b/limitkeys/README.md index 103f250..74829c2 100644 --- a/limitkeys/README.md +++ b/limitkeys/README.md @@ -41,8 +41,8 @@ func (eke EmptyKeyError) Error() string ```go type LimitKey interface { - Key(common.Request) (string, error) Type() string + Key(common.Request) (string, error) } ``` diff --git a/main.go b/main.go index ecb2fdb..2c5d1a5 100644 --- a/main.go +++ b/main.go @@ -54,7 +54,7 @@ func sighupHandler(d daemon.Daemon) { log.Println("Reloaded config file") } -// setupSighupHandler craetes a channel to listen for HUP signals and process them. +// setupSighupHandler creates a channel to listen for HUP signals and process them. func setupSighupHandler(d daemon.Daemon, handler func(daemon.Daemon)) { sigc := make(chan os.Signal) signal.Notify(sigc, syscall.SIGHUP) From b2fd183cefbd8e16936b3a6c67c1fc0d95e3291e Mon Sep 17 00:00:00 2001 From: Michael Graf Date: Wed, 10 Jun 2015 17:59:56 -0700 Subject: [PATCH 2/7] Pass AllowOnError value from config to http.Handler --- config/README.md | 7 ++++--- config/config.go | 10 ++++++---- config/config_test.go | 13 ++++++++++--- daemon/daemon.go | 2 +- example.yaml | 1 + handlers/README.md | 2 +- handlers/http.go | 4 ++-- main_test.go | 2 +- 8 files changed, 26 insertions(+), 15 deletions(-) diff --git a/config/README.md b/config/README.md index 8ac46c9..564b5bc 100644 --- a/config/README.md +++ b/config/README.md @@ -81,9 +81,10 @@ Limit holds the yaml data for one of the limits in the config file ```go type Proxy struct { - Handler string - Host string - Listen string + Handler string + Host string + Listen string + AllowOnError bool `yaml:"allow-on-error"` } ``` diff --git a/config/config.go b/config/config.go index e484eac..6edd11a 100644 --- a/config/config.go +++ b/config/config.go @@ -21,9 +21,10 @@ type Config struct { // Proxy holds the yaml data for the proxy option in the config file type Proxy struct { - Handler string - Host string - Listen string + Handler string + Host string + Listen string + AllowOnError bool `yaml:"allow-on-error"` } // HealthCheck holds the yaml data for how to run the health check service. @@ -56,6 +57,7 @@ func LoadYaml(data []byte) (Config, error) { // ValidateConfig validates that a Config has all the required fields // TODO (z): These should all be private, but right now tests depend on parsing bytes into yaml func ValidateConfig(config Config) error { + // NOTE: tests depend on the order of these checks. if config.Proxy.Handler == "" { return fmt.Errorf("proxy.handler not set") } @@ -80,7 +82,7 @@ func ValidateConfig(config Config) error { } if len(config.Limits) < 1 { - return fmt.Errorf("no limits definied") + return fmt.Errorf("no limits defined") } for name, limit := range config.Limits { diff --git a/config/config_test.go b/config/config_test.go index 924a0e6..078b8b1 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -21,6 +21,10 @@ func TestConfigurationFileLoading(t *testing.T) { t.Error("expected http for Proxy.Handler") } + if config.Proxy.AllowOnError != true { + t.Error("expected true for proxy.allow-on-error: Yes") + } + if config.HealthCheck.Port != "60002" { t.Error("expected 60002 for HealthCheck.Port") } @@ -65,6 +69,7 @@ forward // Incorrect configuration file should return errors func TestInvalidProxyConfig(t *testing.T) { + // proxy.handler not set invalidConfig := []byte(` proxy: host: http://proxy.example.com @@ -74,6 +79,7 @@ proxy: t.Errorf("Expected proxy handler error. Got: %s", err.Error()) } + // proxy.listen not set invalidConfig = []byte(` proxy: handler: http @@ -91,7 +97,8 @@ proxy: listen: :8000 `) _, err = LoadAndValidateYaml(invalidConfig) - if err == nil || !strings.Contains(err.Error(), "proxy") { + + if err == nil || !strings.Contains(err.Error(), "proxy.host") { t.Errorf("Expected proxy host error. Got: %s", err.Error()) } } @@ -112,7 +119,7 @@ storage: limits: bearer/events: keys: - headers: + headers: - 'Authentication' `) _, err := LoadAndValidateYaml(configBuf.Bytes()) @@ -126,7 +133,7 @@ limits: bearer/events: interval: 10 keys: - headers: + headers: - 'Authentication' `) _, err = LoadAndValidateYaml(configBuf.Bytes()) diff --git a/daemon/daemon.go b/daemon/daemon.go index 4ef50ae..29d13bc 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -66,7 +66,7 @@ func (d *daemon) LoadConfig(newConfig config.Config) error { // Set the proxy and handler daemon fields switch d.proxy.Handler { case "http": - d.handler = handlers.NewHTTPLimiter(rateLimiter, proxy) + d.handler = handlers.NewHTTPLimiter(rateLimiter, proxy, d.proxy.AllowOnError) return nil case "httplogger": d.handler = handlers.NewHTTPLogger(rateLimiter, proxy) diff --git a/example.yaml b/example.yaml index 0da4d86..b4e0230 100644 --- a/example.yaml +++ b/example.yaml @@ -3,6 +3,7 @@ proxy: handler: http # can be {http,httplogger} host: http://httpbin.org # URI for the http(s) backend we are proxying to listen: :6634 # bind to host:port. default: height of the Great Sphinx of Giza + allow-on-error: Yes # become passive proxy if error detected? (optional, default=No) storage: type: memory # can be {redis,memory} diff --git a/handlers/README.md b/handlers/README.md index 6e62b18..6d02adf 100644 --- a/handlers/README.md +++ b/handlers/README.md @@ -14,7 +14,7 @@ const ( #### func NewHTTPLimiter ```go -func NewHTTPLimiter(rateLimiter ratelimiter.RateLimiter, proxy http.Handler) http.Handler +func NewHTTPLimiter(rateLimiter ratelimiter.RateLimiter, proxy http.Handler, allowOnError bool) http.Handler ``` NewHTTPLimiter returns an http.Handler that rate limits and proxies requests. diff --git a/handlers/http.go b/handlers/http.go index 53a3a79..3a41ed3 100644 --- a/handlers/http.go +++ b/handlers/http.go @@ -128,8 +128,8 @@ func addRateLimitHeaders(w http.ResponseWriter, statuses []ratelimiter.Status) { } // NewHTTPLimiter returns an http.Handler that rate limits and proxies requests. -func NewHTTPLimiter(rateLimiter ratelimiter.RateLimiter, proxy http.Handler) http.Handler { - return &httpRateLimiter{rateLimiter: rateLimiter, proxy: proxy} +func NewHTTPLimiter(rateLimiter ratelimiter.RateLimiter, proxy http.Handler, allowOnError bool) http.Handler { + return &httpRateLimiter{rateLimiter: rateLimiter, proxy: proxy, AllowOnError: allowOnError} } // NewHTTPLogger returns an http.Handler that logs the results of rate limiting requests, but diff --git a/main_test.go b/main_test.go index 2e43ca5..01a1343 100644 --- a/main_test.go +++ b/main_test.go @@ -45,7 +45,7 @@ func setUpHTTPLimiter(b *testing.B) { // ignore the url in the config and use localhost target, _ := url.Parse(host) proxy := httputil.NewSingleHostReverseProxy(target) - httpLimiter := handlers.NewHTTPLimiter(rateLimiter, proxy) + httpLimiter := handlers.NewHTTPLimiter(rateLimiter, proxy, false) go http.ListenAndServe(":8082", httpLimiter) } From 8689475522f3c91ed15ff513aadae933dfdc4dd3 Mon Sep 17 00:00:00 2001 From: Michael Graf Date: Thu, 18 Jun 2015 01:08:55 +0000 Subject: [PATCH 3/7] Move AllowOnError logging to correct spot --- handlers/http.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/handlers/http.go b/handlers/http.go index 3a41ed3..f3efe4a 100644 --- a/handlers/http.go +++ b/handlers/http.go @@ -52,10 +52,11 @@ func (hrl httpRateLimiter) ServeHTTP(w http.ResponseWriter, r *http.Request) { if err != nil && err != leakybucket.ErrorFull { log.Printf("[%s] ERROR: %s", guid, err) if !hrl.AllowOnError { - log.Printf("[%s] WARNING: bypassing rate limiter due to Error") w.WriteHeader(http.StatusInternalServerError) return } + // Else log and bypass + log.Printf("[%s] WARNING: bypassing rate limiter due to Error") } addRateLimitHeaders(w, matches) From c3a09fe17c7ccfaed55e24091d59d0214489a4a9 Mon Sep 17 00:00:00 2001 From: Michael Graf Date: Thu, 18 Jun 2015 16:59:51 -0700 Subject: [PATCH 4/7] Refactor hrl.ServeHttp for readability --- handlers/http.go | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/handlers/http.go b/handlers/http.go index f3efe4a..498fe82 100644 --- a/handlers/http.go +++ b/handlers/http.go @@ -49,23 +49,22 @@ func (hrl httpRateLimiter) ServeHTTP(w http.ResponseWriter, r *http.Request) { request := common.HTTPToSphinxRequest(r) log.Printf("[%s] REQUEST: %s", guid, stringifyRequest(request).String()) matches, err := hrl.rateLimiter.Add(request) - if err != nil && err != leakybucket.ErrorFull { + switch { + case err == leakybucket.ErrorFull: + addRateLimitHeaders(w, matches) + w.WriteHeader(StatusTooManyRequests) + case err != nil && hrl.AllowOnError: log.Printf("[%s] ERROR: %s", guid, err) - if !hrl.AllowOnError { - w.WriteHeader(http.StatusInternalServerError) - return - } - // Else log and bypass log.Printf("[%s] WARNING: bypassing rate limiter due to Error") - } - - addRateLimitHeaders(w, matches) + hrl.proxy.ServeHTTP(w, r) + case err != nil: + log.Printf("[%s] ERROR: %s", guid, err) + w.WriteHeader(http.StatusInternalServerError) - if err == leakybucket.ErrorFull { - w.WriteHeader(StatusTooManyRequests) - return + default: + addRateLimitHeaders(w, matches) + hrl.proxy.ServeHTTP(w, r) } - hrl.proxy.ServeHTTP(w, r) } type httpRateLogger httpRateLimiter From 1871bc6d0e5dedeb82acc4e79c54b390c09fcf07 Mon Sep 17 00:00:00 2001 From: Michael Graf Date: Thu, 18 Jun 2015 18:32:27 -0700 Subject: [PATCH 5/7] On error, do not add ratelimit headers --- handlers/http.go | 2 +- handlers/http_test.go | 34 +++++++--------------------------- 2 files changed, 8 insertions(+), 28 deletions(-) diff --git a/handlers/http.go b/handlers/http.go index 498fe82..604bca6 100644 --- a/handlers/http.go +++ b/handlers/http.go @@ -55,7 +55,7 @@ func (hrl httpRateLimiter) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.WriteHeader(StatusTooManyRequests) case err != nil && hrl.AllowOnError: log.Printf("[%s] ERROR: %s", guid, err) - log.Printf("[%s] WARNING: bypassing rate limiter due to Error") + log.Printf("[%s] WARNING: bypassing rate limiter due to Error", guid) hrl.proxy.ServeHTTP(w, r) case err != nil: log.Printf("[%s] ERROR: %s", guid, err) diff --git a/handlers/http_test.go b/handlers/http_test.go index edbe97c..850417f 100644 --- a/handlers/http_test.go +++ b/handlers/http_test.go @@ -234,28 +234,13 @@ func TestHandleWhenErrWithoutStatus(t *testing.T) { // Test cases when an error occurs and either value of AllowOnError var allowOnErrorCases = []struct { - AllowOnError bool - ExpectedCode int - RateLimiterStatus []ratelimiter.Status + AllowOnError bool + ExpectedCode int }{ // It should still block if AllowOnError == false - { - false, - http.StatusInternalServerError, - []ratelimiter.Status{}, - }, - // If AllowOnError == true and - { - true, - http.StatusOK, - []ratelimiter.Status{}, - }, - // It should still add headers if AllowOnError == true - { - true, - http.StatusOK, - []ratelimiter.Status{sphinxStatus}, - }, + {false, http.StatusInternalServerError}, + // If AllowOnError == true and no headers, should still StatusOK + {true, http.StatusOK}, } // Tests the AllowOnError flag feature @@ -272,19 +257,14 @@ func TestAllowOnError(t *testing.T) { // Setup an error case (simulate redis connection error) limitMock := limiter.rateLimiter.(*MockRateLimiter).Mock limitMock.On("Add", anyRequest). - Return(test.RateLimiterStatus, errors.New("Expected Testing error - redis.conn error")). + Return([]ratelimiter.Status{sphinxStatus}, errors.New("Expected Testing error - redis.conn error")). Once() proxyMock := limiter.proxy.(*MockProxy).Mock proxyMock.On("ServeHTTP", w, r).Return().Once() limiter.ServeHTTP(w, r) - // If no status, then - if len(test.RateLimiterStatus) == 0 { - assertNoRateLimitHeaders(t, w.Header()) - } else { - compareStatusesToHeader(t, w.Header(), test.RateLimiterStatus) - } + assertNoRateLimitHeaders(t, w.Header()) if w.Code != test.ExpectedCode { t.Fatalf("expected status %d, received %d", test.ExpectedCode, w.Code) From 6dfa83f546799ed488ec7f5efb524f2cc1084d09 Mon Sep 17 00:00:00 2001 From: Michael Graf Date: Fri, 19 Jun 2015 10:55:28 -0700 Subject: [PATCH 6/7] HttpRateLimiter.AllowOnError becomes private --- handlers/http.go | 6 +++--- handlers/http_test.go | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/handlers/http.go b/handlers/http.go index 604bca6..24a3550 100644 --- a/handlers/http.go +++ b/handlers/http.go @@ -41,7 +41,7 @@ func stringifyRequest(req common.Request) *bytes.Buffer { type httpRateLimiter struct { rateLimiter ratelimiter.RateLimiter proxy http.Handler - AllowOnError bool // Do not limit on errors when true + allowOnError bool // Do not limit on errors when true } func (hrl httpRateLimiter) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -53,7 +53,7 @@ func (hrl httpRateLimiter) ServeHTTP(w http.ResponseWriter, r *http.Request) { case err == leakybucket.ErrorFull: addRateLimitHeaders(w, matches) w.WriteHeader(StatusTooManyRequests) - case err != nil && hrl.AllowOnError: + case err != nil && hrl.allowOnError: log.Printf("[%s] ERROR: %s", guid, err) log.Printf("[%s] WARNING: bypassing rate limiter due to Error", guid) hrl.proxy.ServeHTTP(w, r) @@ -129,7 +129,7 @@ func addRateLimitHeaders(w http.ResponseWriter, statuses []ratelimiter.Status) { // NewHTTPLimiter returns an http.Handler that rate limits and proxies requests. func NewHTTPLimiter(rateLimiter ratelimiter.RateLimiter, proxy http.Handler, allowOnError bool) http.Handler { - return &httpRateLimiter{rateLimiter: rateLimiter, proxy: proxy, AllowOnError: allowOnError} + return &httpRateLimiter{rateLimiter: rateLimiter, proxy: proxy, allowOnError: allowOnError} } // NewHTTPLogger returns an http.Handler that logs the results of rate limiting requests, but diff --git a/handlers/http_test.go b/handlers/http_test.go index 850417f..39ef440 100644 --- a/handlers/http_test.go +++ b/handlers/http_test.go @@ -232,22 +232,22 @@ func TestHandleWhenErrWithoutStatus(t *testing.T) { limitMock.AssertExpectations(t) } -// Test cases when an error occurs and either value of AllowOnError +// Test cases when an error occurs and either value of allowOnError var allowOnErrorCases = []struct { - AllowOnError bool + allowOnError bool ExpectedCode int }{ - // It should still block if AllowOnError == false + // It should still block if allowOnError == false {false, http.StatusInternalServerError}, - // If AllowOnError == true and no headers, should still StatusOK + // If allowOnError == true and no headers, should still StatusOK {true, http.StatusOK}, } -// Tests the AllowOnError flag feature -func TestAllowOnError(t *testing.T) { +// Tests the allowOnError flag feature +func TestallowOnError(t *testing.T) { for _, test := range allowOnErrorCases { limiter := constructHTTPRateLimiter() - limiter.AllowOnError = test.AllowOnError + limiter.allowOnError = test.allowOnError w := httptest.NewRecorder() r, err := http.NewRequest("GET", "http://google.com", strings.NewReader("thebody")) if err != nil { From de4936715a3ec776e8d7ec5bdd7f82be1452e28c Mon Sep 17 00:00:00 2001 From: Michael Graf Date: Fri, 19 Jun 2015 11:25:43 -0700 Subject: [PATCH 7/7] Version bump --- deb/sphinx/DEBIAN/control | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deb/sphinx/DEBIAN/control b/deb/sphinx/DEBIAN/control index cfdcaf9..02ffaa2 100644 --- a/deb/sphinx/DEBIAN/control +++ b/deb/sphinx/DEBIAN/control @@ -1,5 +1,5 @@ Package: sphinx -Version: 0.3.1 +Version: 0.4.1 Section: base Priority: optional Architecture: amd64