Enhance code quality analysis (#104)

* chore: add new Make targets.

* refactor: code review.

* chore(travis): go import path.
This commit is contained in:
Ludovic Fernandez
2017-12-18 10:25:18 +01:00
committed by Emile Vauge
parent 4685429df7
commit 1fe46d1aeb
24 changed files with 116 additions and 77 deletions
+10 -7
View File
@@ -7,15 +7,18 @@ go:
sudo: false
go_import_path: github.com/vulcand/oxy
notifications:
email:
on_success: never
on_failure: change
before_install:
- go get -u github.com/golang/lint/golint
- go get -u honnef.co/go/tools/cmd/{gosimple,staticcheck}
- go get -u github.com/client9/misspell/cmd/misspell
- curl -sI https://github.com/golang/dep/releases/latest | grep -Fi Location | tr -d '\r' | sed "s/tag/download/g" | awk -F " " '{ print $2 "/dep-linux-amd64"}' | wget --output-document=$GOPATH/bin/dep -i -
- chmod +x $GOPATH/bin/dep
install: make dependencies
script:
- PKGS=$(go list ./... | grep -v '/vendor/')
- GOFILES=$(go list -f '{{range $index, $element := .GoFiles}}{{$.Dir}}/{{$element}}{{"\n"}}{{end}}' ./... | grep -v '/vendor/')
- go test -cover -race $PKGS
- go vet $PKGS
- diff -u <(echo -n) <(gofmt -d $GOFILES)
+27
View File
@@ -1,8 +1,14 @@
.PHONY: all
PKGS := $(shell go list ./... | grep -v '/vendor/')
TXT_FILES := $(shell find * -type f -not -path 'vendor/**')
default: clean misspell vet test
test: clean
go test -race -cover $(PKGS)
test-verbose: clean
go test -v -race -cover $(PKGS)
dependencies:
@@ -10,6 +16,27 @@ dependencies:
clean:
find . -name flymake_* -delete
rm -f cover.out
lint:
echo "golint:"
golint -set_exit_status $(PKGS)
vet:
go vet $(PKGS)
checks: vet lint check-fmt
echo "staticcheck:"
staticcheck $(PKGS)
echo "gosimple:"
gosimple $(PKGS)
check-fmt: SHELL := /bin/bash
check-fmt:
diff -u <(echo -n) <(gofmt -d $(GOFILES))
misspell:
misspell -source=text -error $(TXT_FILES)
test-package: clean
go test -v ./$(p)
+10 -7
View File
@@ -1,5 +1,5 @@
/*
package buffer provides http.Handler middleware that solves several problems when dealing with http requests:
Package buffer provides http.Handler middleware that solves several problems when dealing with http requests:
Reads the entire request and response into buffer, optionally buffering it to disk for large requests.
Checks the limits for the requests and responses, rejecting in case if the limit was exceeded.
@@ -51,18 +51,18 @@ import (
)
const (
// Store up to 1MB in RAM
// DefaultMemBodyBytes Store up to 1MB in RAM
DefaultMemBodyBytes = 1048576
// No limit by default
// DefaultMaxBodyBytes No limit by default
DefaultMaxBodyBytes = -1
// Maximum retry attempts
// DefaultMaxRetryAttempts Maximum retry attempts
DefaultMaxRetryAttempts = 10
)
var errHandler utils.ErrorHandler = &SizeErrHandler{}
// Buffer is responsible for buffering requests and responses
// It buffers large reqeuests and responses to disk,
// It buffers large requests and responses to disk,
type Buffer struct {
maxRequestBodyBytes int64
memRequestBodyBytes int64
@@ -212,7 +212,10 @@ func (s *Buffer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
// and we don'w want to mess with standard library
defer func() {
if body != nil {
body.Close()
errClose := body.Close()
if errClose != nil {
log.Errorf("vulcand/oxy/buffer: failed to close body, err: %v", errClose)
}
}
}()
@@ -303,7 +306,7 @@ func (s *Buffer) copyRequest(req *http.Request, body io.ReadCloser, bodySize int
if body == nil {
o.Body = nil
} else {
o.Body = ioutil.NopCloser(body)
o.Body = ioutil.NopCloser(body.(io.Reader))
}
return &o
}
+3 -1
View File
@@ -85,6 +85,7 @@ func (s *BFSuite) TestChunkedEncodingSuccess(c *C) {
c.Assert(err, IsNil)
fmt.Fprintf(conn, "POST / HTTP/1.1\r\nHost: 127.0.0.1:8080\r\nTransfer-Encoding: chunked\r\n\r\n4\r\ntest\r\n5\r\ntest1\r\n5\r\ntest2\r\n0\r\n\r\n")
status, err := bufio.NewReader(conn).ReadString('\n')
c.Assert(err, IsNil)
c.Assert(reqBody, Equals, "testtest1test2")
c.Assert(status, Equals, "HTTP/1.1 200 OK\r\n")
@@ -116,8 +117,9 @@ func (s *BFSuite) TestChunkedEncodingLimitReached(c *C) {
conn, err := net.Dial("tcp", testutils.ParseURI(proxy.URL).Host)
c.Assert(err, IsNil)
fmt.Fprintf(conn, "POST / HTTP/1.1\r\nHost: 127.0.0.1:8080\r\nTransfer-Encoding: chunked\r\n\r\n4\r\ntest\r\n5\r\ntest1\r\n5\r\ntest2\r\n0\r\n\r\n")
fmt.Fprint(conn, "POST / HTTP/1.1\r\nHost: 127.0.0.1:8080\r\nTransfer-Encoding: chunked\r\n\r\n4\r\ntest\r\n5\r\ntest1\r\n5\r\ntest2\r\n0\r\n\r\n")
status, err := bufio.NewReader(conn).ReadString('\n')
c.Assert(err, IsNil)
c.Assert(status, Equals, "HTTP/1.1 413 Request Entity Too Large\r\n")
}
+1 -1
View File
@@ -1,4 +1,4 @@
// package cbreaker implements circuit breaker similar to https://github.com/Netflix/Hystrix/wiki/How-it-Works
// Package cbreaker implements circuit breaker similar to https://github.com/Netflix/Hystrix/wiki/How-it-Works
//
// Vulcan circuit breaker watches the error condtion to match
// after which it activates the fallback scenario, e.g. returns the response code
+7 -7
View File
@@ -90,7 +90,7 @@ func (s *CBSuite) TestFullCycle(c *C) {
cb.metrics = statsNetErrors(0.6)
s.advanceTime(defaultCheckPeriod + time.Millisecond)
re, _, err = testutils.Get(srv.URL)
_, _, err = testutils.Get(srv.URL)
c.Assert(err, IsNil)
c.Assert(cb.state, Equals, cbState(stateTripped))
@@ -139,7 +139,7 @@ func (s *CBSuite) TestRedirectWithPath(c *C) {
defer srv.Close()
cb.metrics = statsNetErrors(0.6)
re, _, err := testutils.Get(srv.URL)
_, _, err = testutils.Get(srv.URL)
c.Assert(err, IsNil)
client := &http.Client{
@@ -148,7 +148,7 @@ func (s *CBSuite) TestRedirectWithPath(c *C) {
},
}
re, err = client.Get(srv.URL + "/somePath")
re, err := client.Get(srv.URL + "/somePath")
c.Assert(err, NotNil)
c.Assert(re.StatusCode, Equals, http.StatusFound)
c.Assert(re.Header.Get("Location"), Equals, "http://localhost:6000/somePath")
@@ -166,7 +166,7 @@ func (s *CBSuite) TestRedirect(c *C) {
defer srv.Close()
cb.metrics = statsNetErrors(0.6)
re, _, err := testutils.Get(srv.URL)
_, _, err = testutils.Get(srv.URL)
c.Assert(err, IsNil)
client := &http.Client{
@@ -175,7 +175,7 @@ func (s *CBSuite) TestRedirect(c *C) {
},
}
re, err = client.Get(srv.URL + "/somePath")
re, err := client.Get(srv.URL + "/somePath")
c.Assert(err, NotNil)
c.Assert(re.StatusCode, Equals, http.StatusFound)
c.Assert(re.Header.Get("Location"), Equals, "http://localhost:5000")
@@ -193,13 +193,13 @@ func (s *CBSuite) TestTriggerDuringRecovery(c *C) {
defer srv.Close()
cb.metrics = statsNetErrors(0.6)
re, _, err := testutils.Get(srv.URL)
_, _, err = testutils.Get(srv.URL)
c.Assert(err, IsNil)
c.Assert(cb.state, Equals, cbState(stateTripped))
// We should be in recovering state by now
s.advanceTime(10*time.Second + time.Millisecond)
re, _, err = testutils.Get(srv.URL)
re, _, err := testutils.Get(srv.URL)
c.Assert(err, IsNil)
c.Assert(re.StatusCode, Equals, http.StatusServiceUnavailable)
c.Assert(cb.state, Equals, cbState(stateRecovering))
+8 -2
View File
@@ -39,7 +39,10 @@ func (f *ResponseFallback) ServeHTTP(w http.ResponseWriter, req *http.Request) {
}
w.Header().Set("Content-Length", strconv.Itoa(len(f.r.Body)))
w.WriteHeader(f.r.StatusCode)
w.Write(f.r.Body)
_, err := w.Write(f.r.Body)
if err != nil {
log.Errorf("vulcand/oxy/fallback/response: failed to write response, err: %v", err)
}
}
type Redirect struct {
@@ -74,5 +77,8 @@ func (f *RedirectFallback) ServeHTTP(w http.ResponseWriter, req *http.Request) {
w.Header().Set("Location", location)
w.WriteHeader(http.StatusFound)
w.Write([]byte(http.StatusText(http.StatusFound)))
_, err := w.Write([]byte(http.StatusText(http.StatusFound)))
if err != nil {
log.Errorf("vulcand/oxy/fallback/redirect: failed to write response, err: %v", err)
}
}
+2 -2
View File
@@ -78,7 +78,7 @@ func (cl *ConnLimiter) acquire(token string, amount int64) error {
}
cl.connections[token] += amount
cl.totalConnections += int64(amount)
cl.totalConnections += amount
return nil
}
@@ -87,7 +87,7 @@ func (cl *ConnLimiter) release(token string, amount int64) {
defer cl.mutex.Unlock()
cl.connections[token] -= amount
cl.totalConnections -= int64(amount)
cl.totalConnections -= amount
// Otherwise it would grow forever
if cl.connections[token] == 0 {
+2 -2
View File
@@ -38,8 +38,8 @@ func (s *ConnLimiterSuite) TestHitLimitAndRelease(c *C) {
defer srv.Close()
go func() {
re, _, err := testutils.Get(srv.URL, testutils.Header("Limit", "a"), testutils.Header("wait", "yes"))
c.Assert(err, IsNil)
re, _, errGet := testutils.Get(srv.URL, testutils.Header("Limit", "a"), testutils.Header("wait", "yes"))
c.Assert(errGet, IsNil)
c.Assert(re.StatusCode, Equals, http.StatusOK)
}()
+13 -14
View File
@@ -4,18 +4,17 @@
package forward
import (
"crypto/tls"
"io"
"net/http"
"net/http/httptest"
"net/http/httputil"
"net/url"
"os"
"reflect"
"strings"
"time"
"crypto/tls"
"net/http/httputil"
"reflect"
"github.com/gorilla/websocket"
log "github.com/sirupsen/logrus"
"github.com/vulcand/oxy/utils"
@@ -280,18 +279,18 @@ func (f *httpForwarder) serveWebSocket(w http.ResponseWriter, req *http.Request,
return
}
conn, _, err := hijacker.Hijack()
if err != nil {
conn, _, errHij := hijacker.Hijack()
if errHij != nil {
log.Errorf("vulcand/oxy/forward/websocket: Failed to hijack responseWriter")
ctx.errHandler.ServeHTTP(w, req, err)
ctx.errHandler.ServeHTTP(w, req, errHij)
return
}
defer conn.Close()
err = resp.Write(conn)
if err != nil {
errWrite := resp.Write(conn)
if errWrite != nil {
log.Errorf("vulcand/oxy/forward/websocket: Failed to forward response")
ctx.errHandler.ServeHTTP(w, req, err)
ctx.errHandler.ServeHTTP(w, req, errWrite)
return
}
}
@@ -314,13 +313,13 @@ func (f *httpForwarder) serveWebSocket(w http.ResponseWriter, req *http.Request,
errc := make(chan error, 2)
replicate := func(dst io.Writer, src io.Reader, dstName string, srcName string) {
_, err := io.Copy(dst, src)
if err != nil {
f.log.Errorf("vulcand/oxy/forward/websocket: Error when copying from %s to %s using io.Copy: %v", srcName, dstName, err)
_, errCopy := io.Copy(dst, src)
if errCopy != nil {
f.log.Errorf("vulcand/oxy/forward/websocket: Error when copying from %s to %s using io.Copy: %v", srcName, dstName, errCopy)
} else {
f.log.Infof("vulcand/oxy/forward/websocket: Copying from %s to %s using io.Copy completed without error.", srcName, dstName)
}
errc <- err
errc <- errCopy
}
go replicate(targetConn.UnderlyingConn(), underlyingConn.UnderlyingConn(), "backend", "client")
+1
View File
@@ -243,6 +243,7 @@ func (s *FwdSuite) TestEscapedURL(c *C) {
path := "/log/http%3A%2F%2Fwww.site.com%2Fsomething?a=b"
request, err := http.NewRequest("GET", proxy.URL, nil)
c.Assert(err, IsNil)
parsed := testutils.ParseURI(proxy.URL)
parsed.Opaque = path
request.URL = parsed
+3 -2
View File
@@ -290,16 +290,17 @@ func (s *FwdSuite) TestWebSocketUpgradeFailed(c *C) {
// First request works with 400
br := bufio.NewReader(conn)
resp, err := http.ReadResponse(br, req)
c.Assert(err, IsNil)
c.Assert(resp.StatusCode, Equals, 400)
req, err = http.NewRequest(http.MethodGet, "ws://127.0.0.1/ws2", nil)
c.Assert(err, IsNil)
req.Header.Add("upgrade", "websocket")
req.Header.Add("Connection", "upgrade")
req.Write(conn)
br = bufio.NewReader(conn)
resp, err = http.ReadResponse(br, req)
_, err = http.ReadResponse(br, req)
c.Assert(err, Equals, io.ErrUnexpectedEOF)
}
+1 -1
View File
@@ -14,7 +14,7 @@ type HeaderRewriter struct {
Hostname string
}
// clean up IP in case if it is ipv6 address and it has {zone} infromation in it, like "[fe80::d806:a55d:eb1b:49cc%vEthernet (vmxnet3 Ethernet Adapter - Virtual Switch)]:64692"
// clean up IP in case if it is ipv6 address and it has {zone} information in it, like "[fe80::d806:a55d:eb1b:49cc%vEthernet (vmxnet3 Ethernet Adapter - Virtual Switch)]:64692"
func ipv6fix(clientIP string) string {
return strings.Split(clientIP, "%")[0]
}
+1 -1
View File
@@ -40,7 +40,7 @@ func SplitRatios(values []float64) (good map[float64]bool, bad map[float64]bool)
}
// SplitFloat64 provides simple anomaly detection for skewed data sets with no particular distribution.
// In essense it applies the formula if(v > median(values) + threshold * medianAbsoluteDeviation) -> anomaly
// In essence it applies the formula if(v > median(values) + threshold * medianAbsoluteDeviation) -> anomaly
// There's a corner case where there are just 2 values, so by definition there's no value that exceeds the threshold.
// This case is solved by introducing additional value that we know is good, e.g. 0. That helps to improve the detection results
// on such data sets.
+1 -3
View File
@@ -71,9 +71,7 @@ func (c *RollingCounter) Clone() *RollingCounter {
lastBucket: c.lastBucket,
lastUpdated: c.lastUpdated,
}
for i, v := range c.values {
other.values[i] = v
}
copy(other.values, c.values)
return other
}
+2 -2
View File
@@ -179,8 +179,8 @@ func (r *RollingHDRHistogram) Merged() (*HDRHistogram, error) {
return m, err
}
for _, h := range r.buckets {
if m.Merge(h); err != nil {
return nil, err
if errMerge := m.Merge(h); errMerge != nil {
return nil, errMerge
}
}
return m, nil
+1 -6
View File
@@ -135,7 +135,7 @@ func (m *RTMetrics) CounterWindowSize() time.Duration {
}
// GetNetworkErrorRatio calculates the amont of network errors such as time outs and dropped connection
// that occured in the given time window compared to the total requests count.
// that occurred in the given time window compared to the total requests count.
func (m *RTMetrics) NetworkErrorRatio() float64 {
if m.total.Count() == 0 {
return 0
@@ -246,11 +246,6 @@ func (m *RTMetrics) Reset() {
m.statusCodes = make(map[int]*RollingCounter)
}
func (m *RTMetrics) recordNetError() error {
m.netErrors.Inc(1)
return nil
}
func (m *RTMetrics) recordLatency(d time.Duration) error {
m.histogramLock.Lock()
defer m.histogramLock.Unlock()
+2 -1
View File
@@ -172,7 +172,7 @@ func (s *BucketSuite) TestUpdateInvalidPeriod(c *C) {
// ...check that burst did not change
s.clock.Sleep(40 * time.Second)
delay, err = tb.consume(21)
_, err = tb.consume(21)
c.Assert(err, NotNil)
delay, err = tb.consume(20)
c.Assert(err, IsNil)
@@ -216,6 +216,7 @@ func (s *BucketSuite) TestUpdateRateChanged(c *C) {
tb.consume(15) // 5 tokens available
// When
err := tb.update(&rate{time.Second, 20, 20}) // still 5 tokens available
c.Assert(err, IsNil)
// Then
delay, err := tb.consume(20)
c.Assert(err, IsNil)
+1 -1
View File
@@ -96,7 +96,7 @@ func (s *LimiterSuite) TestFailure(c *C) {
c.Assert(re.StatusCode, Equals, http.StatusInternalServerError)
}
// Make sure rates from different ips are controlled separatedly
// Make sure rates from different ips are controlled separately
func (s *LimiterSuite) TestIsolation(c *C) {
handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
w.Write([]byte("hello"))
+1 -1
View File
@@ -44,7 +44,7 @@ const (
)
// Stream is responsible for buffering requests and responses
// It buffers large reqeuests and responses to disk,
// It buffers large requests and responses to disk,
type Stream struct {
maxRequestBodyBytes int64
+4 -2
View File
@@ -13,9 +13,10 @@ import (
"github.com/vulcand/oxy/forward"
"github.com/vulcand/oxy/testutils"
"time"
log "github.com/sirupsen/logrus"
. "gopkg.in/check.v1"
"time"
)
func TestStream(t *testing.T) { TestingT(t) }
@@ -107,10 +108,11 @@ func (s *STSuite) TestChunkedEncodingSuccess(c *C) {
conn, err := net.Dial("tcp", testutils.ParseURI(proxy.URL).Host)
c.Assert(err, IsNil)
fmt.Fprintf(conn, "POST / HTTP/1.1\r\nHost: 127.0.0.1\r\nTransfer-Encoding: chunked\r\n\r\n4\r\ntest\r\n5\r\ntest1\r\n5\r\ntest2\r\n0\r\n\r\n")
fmt.Fprint(conn, "POST / HTTP/1.1\r\nHost: 127.0.0.1\r\nTransfer-Encoding: chunked\r\n\r\n4\r\ntest\r\n5\r\ntest1\r\n5\r\ntest2\r\n0\r\n\r\n")
reader := bufio.NewReader(conn)
status, err := reader.ReadString('\n')
c.Assert(err, IsNil)
reader.ReadString('\n') //content type
reader.ReadString('\n') //Date
+7 -7
View File
@@ -2,7 +2,7 @@ package testutils
import (
"crypto/tls"
"fmt"
"errors"
"io/ioutil"
"net/http"
"net/http/httptest"
@@ -13,7 +13,7 @@ import (
)
func NewHandler(handler http.HandlerFunc) *httptest.Server {
return httptest.NewServer(http.HandlerFunc(handler))
return httptest.NewServer(handler)
}
func NewResponder(response string) *httptest.Server {
@@ -101,7 +101,7 @@ func MakeRequest(url string, opts ...ReqOption) (*http.Response, []byte, error)
}
if o.Method == "" {
o.Method = "GET"
o.Method = http.MethodGet
}
request, _ := http.NewRequest(o.Method, url, strings.NewReader(o.Body))
if o.Headers != nil {
@@ -131,18 +131,18 @@ func MakeRequest(url string, opts ...ReqOption) (*http.Response, []byte, error)
client := &http.Client{
Transport: tr,
CheckRedirect: func(req *http.Request, via []*http.Request) error {
return fmt.Errorf("No redirects")
return errors.New("no redirects")
},
}
response, err := client.Do(request)
if err == nil {
bodyBytes, err := ioutil.ReadAll(response.Body)
return response, bodyBytes, err
bodyBytes, errRead := ioutil.ReadAll(response.Body)
return response, bodyBytes, errRead
}
return response, nil, err
}
func Get(url string, opts ...ReqOption) (*http.Response, []byte, error) {
opts = append(opts, Method("GET"))
opts = append(opts, Method(http.MethodGet))
return MakeRequest(url, opts...)
}
+2 -1
View File
@@ -104,8 +104,9 @@ func (s *TraceSuite) TestTraceTLS(c *C) {
conn, err := tls.Dial("tcp", u.Host, config)
c.Assert(err, IsNil)
fmt.Fprintf(conn, "GET / HTTP/1.0\r\n\r\n")
fmt.Fprint(conn, "GET / HTTP/1.0\r\n\r\n")
status, err := bufio.NewReader(conn).ReadString('\n')
c.Assert(err, IsNil)
c.Assert(status, Equals, "HTTP/1.0 200 OK\r\n")
state := conn.ConnectionState()
conn.Close()
+6 -6
View File
@@ -22,18 +22,18 @@ func ParseAuthHeader(header string) (*BasicAuth, error) {
return nil, fmt.Errorf(fmt.Sprintf("Failed to parse header '%s'", header))
}
auth_type := strings.ToLower(values[0])
if auth_type != "basic" {
return nil, fmt.Errorf("Expected basic auth type, got '%s'", auth_type)
authType := strings.ToLower(values[0])
if authType != "basic" {
return nil, fmt.Errorf("Expected basic auth type, got '%s'", authType)
}
encoded_string := values[1]
decoded_string, err := base64.StdEncoding.DecodeString(encoded_string)
encodedString := values[1]
decodedString, err := base64.StdEncoding.DecodeString(encodedString)
if err != nil {
return nil, fmt.Errorf("Failed to parse header '%s', base64 failed: %s", header, err)
}
values = strings.SplitN(string(decoded_string), ":", 2)
values = strings.SplitN(string(decodedString), ":", 2)
if len(values) != 2 {
return nil, fmt.Errorf("Failed to parse header '%s', expected separator ':'", header)
}