Merge pull request #224 from desimone/deployment/golangci-lint

development: replace vet/lint with golangci-lint

.golangci.yml

@@ -0,0 +1,210 @@
+# forked from istio
+service:
+  # When updating this, also update bin/linters.sh accordingly
+  golangci-lint-version: 1.17.x # use the fixed version to not introduce new linters unexpectedly
+run:
+  # timeout for analysis, e.g. 30s, 5m, default is 1m
+  deadline: 5m
+
+  # which dirs to skip: they won't be analyzed;
+  # can use regexp here: generated.*, regexp is applied on full path;
+  # default value is empty list, but next dirs are always skipped independently
+  # from this option's value:
+  #   	vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
+  skip-dirs:
+    - genfiles$
+    - vendor$
+
+  # which files to skip: they will be analyzed, but issues from them
+  # won't be reported. Default value is empty list, but there is
+  # no need to include all autogenerated files, we confidently recognize
+  # autogenerated files. If it's not please let us know.
+  skip-files:
+    - ".*\\.pb\\.go"
+    - ".*\\.gen\\.go"
+
+linters:
+  enable-all: true
+  disable:
+    - depguard
+    - dupl
+    - gochecknoglobals
+    - gochecknoinits
+    - goconst
+    - gocyclo
+    - nakedret
+    - prealloc
+    - scopelint
+    - maligned
+    - interfacer
+  fast: false
+
+linters-settings:
+  errcheck:
+    # report about not checking of errors in type assetions: `a := b.(MyStruct)`;
+    # default is false: such cases aren't reported by default.
+    check-type-assertions: false
+
+    # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`;
+    # default is false: such cases aren't reported by default.
+    check-blank: false
+
+  govet:
+    # report about shadowed variables
+    check-shadowing: false
+  golint:
+    # minimal confidence for issues, default is 0.8
+    min-confidence: 0.0
+  gofmt:
+    # simplify code: gofmt with `-s` option, true by default
+    simplify: true
+  misspell:
+    # Correct spellings using locale preferences for US or UK.
+    # Default is to use a neutral variety of English.
+    # Setting locale to US will correct the British spelling of 'colour' to 'color'.
+    locale: US
+  lll:
+    # max line length, lines longer will be reported. Default is 120.
+    # '\t' is counted as 1 character by default, and can be changed with the tab-width option
+    line-length: 160
+    # tab width in spaces. Default to 1.
+    tab-width: 1
+  unused:
+    # treat code as a program (not a library) and report unused exported identifiers; default is false.
+    # XXX: if you enable this setting, unused will report a lot of false-positives in text editors:
+    # if it's called for subdir of a project it can't find funcs usages. All text editor integrations
+    # with golangci-lint call it on a directory with the changed file.
+    check-exported: false
+  unparam:
+    # call graph construction algorithm (cha, rta). In general, use cha for libraries,
+    # and rta for programs with main packages. Default is cha.
+    algo: cha
+
+    # Inspect exported functions, default is false. Set to true if no external program/library imports your code.
+    # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors:
+    # if it's called for subdir of a project it can't find external interfaces. All text editor integrations
+    # with golangci-lint call it on a directory with the changed file.
+    check-exported: false
+  gocritic:
+    enabled-checks:
+      - appendCombine
+      - argOrder
+      - assignOp
+      - badCond
+      - boolExprSimplify
+      - builtinShadow
+      - captLocal
+      - caseOrder
+      - codegenComment
+      - commentedOutCode
+      - commentedOutImport
+      - defaultCaseOrder
+      - deprecatedComment
+      - docStub
+      - dupArg
+      - dupBranchBody
+      - dupCase
+      - dupSubExpr
+      - elseif
+      - emptyFallthrough
+      - equalFold
+      - flagDeref
+      - flagName
+      - hexLiteral
+      - indexAlloc
+      - initClause
+      - methodExprCall
+      - nilValReturn
+      - octalLiteral
+      - offBy1
+      - rangeExprCopy
+      - regexpMust
+      - sloppyLen
+      - stringXbytes
+      - switchTrue
+      - typeAssertChain
+      - typeSwitchVar
+      - typeUnparen
+      - underef
+      - unlambda
+      - unnecessaryBlock
+      - unslice
+      - valSwap
+      - weakCond
+      - yodaStyleExpr
+
+      # Unused
+      # - appendAssign
+      # - commentFormatting
+      # - emptyStringTest
+      # - exitAfterDefer
+      # - ifElseChain
+      # - hugeParam
+      # - importShadow
+      # - nestingReduce
+      # - paramTypeCombine
+      # - ptrToRefParam
+      # - rangeValCopy
+      # - singleCaseSwitch
+      # - sloppyReassign
+      # - unlabelStmt
+      # - unnamedResult
+      # - wrapperFunc
+
+issues:
+  # List of regexps of issue texts to exclude, empty list by default.
+  # But independently from this option we use default exclude patterns,
+  # it can be disabled by `exclude-use-default: false`. To list all
+  # excluded by default patterns execute `golangci-lint run --help`
+  exclude:
+    # Mostly harmless buffer writes where we skip error checking
+    # https://golang.org/pkg/bytes/#Buffer.Write
+    - "Error return value of `w.Write` is not checked"
+    - "Error return value of `io.WriteString` is not checked"
+    - "Error return value of `viper.BindEnv` is not checked"
+    - "Error return value of `h.Write` is not checked"
+    - "ExecuteTemplate` is not checked"
+
+    # go sec : we want to allow skipping tls auth
+    - "TLS InsecureSkipVerify set true."
+    - "goroutine calls T.Fatalf, which must be called in the same goroutine as the test"
+    # good job Protobuffs!
+    - "method XXX"
+
+  exclude-rules:
+    # Exclude some linters from running on test files.
+    - path: _test\.go$|^tests/|^samples/|templates\.go$
+      linters:
+        - errcheck
+        - maligned
+        - lll
+        - gosec
+        - bodyclose
+        - unparam
+    # erroneously thinks google api url is a cred
+    - path: internal/identity/google.go
+      text: "Potential hardcoded credentials"
+      linters:
+        - gosec
+    # deprecated but every example still uses New
+    - path: internal/identity/google.go
+      text: "please use NewService instead"
+      linters:
+        - staticcheck
+
+    # todo(bdd): replace in go 1.13
+    - path: proxy/proxy.go
+      text: "copylocks: assignment copies lock value to transport"
+      linters:
+        - govet
+  # Independently from option `exclude` we use default exclude patterns,
+  # it can be disabled by this option. To list all
+  # excluded by default patterns execute `golangci-lint run --help`.
+  # Default value for this option is true.
+  exclude-use-default: true
+
+  # Maximum issues count per one linter. Set to 0 to disable. Default is 50.
+  max-per-linter: 0
+
+  # Maximum count of issues with the same text. Set to 0 to disable. Default is 3.
+  max-same-issues: 0

Makefile

@@ -17,7 +17,7 @@ VERSION := $(shell cat VERSION)
 GITCOMMIT := $(shell git rev-parse --short HEAD)
 GITUNTRACKEDCHANGES := $(shell git status --porcelain --untracked-files=no)
 BUILDMETA:=
-ifneq ($(GITUNTRACKEDCHANGES),)
+ifneq ($(GITUNTRACKEDCHANGES),"")
 	BUILDMETA := dirty
 endif
 CTIMEVAR=-X $(PKG)/internal/version.GitCommit=$(GITCOMMIT) \
@@ -30,7 +30,7 @@ GOOSARCHES = linux/amd64 darwin/amd64 windows/amd64
 
 
 .PHONY: all
-all: clean build fmt lint vet test ## Runs a clean, build, fmt, lint, test, and vet.
+all: clean build lint test ## Runs a clean, build, fmt, lint, test, and vet.
 
 .PHONY: tag
 tag: ## Create a new git tag to prepare to build a release
@@ -42,26 +42,11 @@ build: ## Builds dynamic executables and/or packages.
 	@echo "==> $@"
 	@CGO_ENABLED=0 GO111MODULE=on go build -tags "$(BUILDTAGS)" ${GO_LDFLAGS} -o $(BINDIR)/$(NAME) ./cmd/"$(NAME)"
 
-.PHONY: fmt
-fmt: ## Verifies all files have been `gofmt`ed.
-	@echo "==> $@"
-	@gofmt -s -l . | grep -v '.pb.go:' | grep -v vendor | tee /dev/stderr
-
 .PHONY: lint
 lint: ## Verifies `golint` passes.
 	@echo "==> $@"
-	@go get golang.org/x/lint/golint
-	@golint ./... | grep -v '.pb.go:' | grep -v vendor | tee /dev/stderr
-
-.PHONY: staticcheck
-staticcheck: ## Verifies `staticcheck` passes
-	@echo "+ $@"
-	@staticcheck $(shell go list ./... | grep -v vendor) | grep -v '.pb.go:' | tee /dev/stderr
-
-.PHONY: vet
-vet: ## Verifies `go vet` passes.
-	@echo "==> $@"
-	@go vet $(shell go list ./... | grep -v vendor) | grep -v '.pb.go:' | tee /dev/stderr
+	@GO111MODULE=off go get -u github.com/golangci/golangci-lint/cmd/golangci-lint
+	@golangci-lint run ./...
 
 .PHONY: test
 test: ## Runs the go tests.

authenticate/authenticate.go

@@ -59,7 +59,7 @@ func New(opts config.Options) (*Authenticate, error) {
 		return nil, err
 	}
 	decodedCookieSecret, _ := base64.StdEncoding.DecodeString(opts.CookieSecret)
-	cipher, err := cryptutil.NewCipher([]byte(decodedCookieSecret))
+	cipher, err := cryptutil.NewCipher(decodedCookieSecret)
 	if err != nil {
 		return nil, err
 	}

authenticate/authenticate_test.go

@@ -85,9 +85,6 @@ func TestNew(t *testing.T) {
 				t.Errorf("New() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
-			// if !reflect.DeepEqual(got, tt.want) {
-			// 	t.Errorf("New() = %v, want %v", got, tt.want)
-			// }
 		})
 	}
 }

authenticate/grpc_test.go

@@ -93,12 +93,12 @@ func TestAuthenticate_Refresh(t *testing.T) {
 
 func TestAuthenticate_Authenticate(t *testing.T) {
 	secret := cryptutil.GenerateKey()
-	c, err := cryptutil.NewCipher([]byte(secret))
+	c, err := cryptutil.NewCipher(secret)
 	if err != nil {
 		t.Fatalf("expected to be able to create cipher: %v", err)
 	}
 	newSecret := cryptutil.GenerateKey()
-	c2, err := cryptutil.NewCipher([]byte(newSecret))
+	c2, err := cryptutil.NewCipher(newSecret)
 	if err != nil {
 		t.Fatalf("expected to be able to create cipher: %v", err)
 	}

authenticate/handlers.go

@@ -119,7 +119,7 @@ func (a *Authenticate) SignIn(w http.ResponseWriter, r *http.Request) {
 		httputil.ErrorResponse(w, r, httpErr)
 		return
 	}
-	http.Redirect(w, r, getAuthCodeRedirectURL(redirectURL, state, string(encrypted)), http.StatusFound)
+	http.Redirect(w, r, getAuthCodeRedirectURL(redirectURL, state, encrypted), http.StatusFound)
 }
 
 func getAuthCodeRedirectURL(redirectURL *url.URL, state, authCode string) string {

authenticate/handlers_test.go

@@ -224,7 +224,7 @@ func (a mockCipher) Decrypt(s []byte) ([]byte, error) {
 }
 func (a mockCipher) Marshal(s interface{}) (string, error) { return "ok", nil }
 func (a mockCipher) Unmarshal(s string, i interface{}) error {
-	if string(s) == "unmarshal error" || string(s) == "error" {
+	if s == "unmarshal error" || s == "error" {
 		return errors.New("error")
 	}
 	return nil

authorize/authorize_test.go

@@ -34,9 +34,6 @@ func TestNew(t *testing.T) {
 				t.Errorf("New() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
-			// if !reflect.DeepEqual(got, tt.want) {
-			// 	t.Errorf("New() = %v, want %v", got, tt.want)
-			// }
 		})
 	}
 }

cmd/pomerium/main.go

@@ -167,7 +167,7 @@ func newPromListener(addr string) {
 	log.Error().Err(metrics.NewPromHTTPListener(addr)).Str("MetricsAddr", addr).Msg("cmd/pomerium: could not start metrics exporter")
 }
 
-func wrapMiddleware(o *config.Options, mux *http.ServeMux) http.Handler {
+func wrapMiddleware(o *config.Options, mux http.Handler) http.Handler {
 	c := middleware.NewChain()
 	c = c.Append(metrics.HTTPMetricsHandler("proxy"))
 	c = c.Append(log.NewHandler(log.Logger))
@@ -202,7 +202,6 @@ func parseOptions(configFile string) (*config.Options, error) {
 	}
 	if o.Debug {
 		log.SetDebugMode()
-		// log.Debug().Interface("options", o).Msg("cmd/pomerium")
 	}
 	if o.LogLevel != "" {
 		log.SetLevel(o.LogLevel)

cmd/pomerium/main_test.go

@@ -268,7 +268,7 @@ func (m *mockService) UpdateOptions(o config.Options) error {
 
 	m.Updated = true
 	if m.fail {
-		return fmt.Errorf("Failed")
+		return fmt.Errorf("failed")
 	}
 	return nil
 }

internal/config/options.go

@@ -137,10 +137,10 @@ var defaultOptions = Options{
 	Services:               "all",
 	CookieHTTPOnly:         true,
 	CookieSecure:           true,
-	CookieExpire:           time.Duration(14) * time.Hour,
-	CookieRefresh:          time.Duration(30) * time.Minute,
+	CookieExpire:           14 * time.Hour,
+	CookieRefresh:          30 * time.Minute,
 	CookieName:             "_pomerium",
-	DefaultUpstreamTimeout: time.Duration(30) * time.Second,
+	DefaultUpstreamTimeout: 30 * time.Second,
 	Headers: map[string]string{
 		"X-Content-Type-Options":    "nosniff",
 		"X-Frame-Options":           "SAMEORIGIN",
@@ -154,7 +154,7 @@ var defaultOptions = Options{
 	ReadTimeout:       30 * time.Second,
 	WriteTimeout:      0, // support streaming by default
 	IdleTimeout:       5 * time.Minute,
-	RefreshCooldown:   time.Duration(5 * time.Minute),
+	RefreshCooldown:   5 * time.Minute,
 }
 
 // NewOptions returns a minimal options configuration built from default options.
@@ -263,11 +263,8 @@ func (o *Options) parsePolicy() error {
 		if err := yaml.Unmarshal(policyBytes, &policies); err != nil {
 			return fmt.Errorf("could not unmarshal policy yaml: %s", err)
 		}
-	} else {
-		// Parse from file
-		if err := viper.UnmarshalKey("policy", &policies); err != nil {
-			return err
-		}
+	} else if err := viper.UnmarshalKey("policy", &policies); err != nil {
+		return err
 	}
 	if len(policies) != 0 {
 		o.Policies = policies

internal/config/options_test.go

@@ -304,7 +304,7 @@ func Test_Checksum(t *testing.T) {
 		t.Error("Checksum() not returning data")
 	}
 
-	if o.Checksum() != o.Checksum() {
+	if o.Checksum() != newChecksum {
 		t.Error("Checksum() inconsistent")
 	}
 }

internal/cryptutil/encrypt.go

@@ -89,7 +89,7 @@ func (c *XChaCha20Cipher) Encrypt(plaintext []byte) (joined []byte, err error) {
 	ciphertext := c.aead.Seal(nil, nonce, plaintext, nil)
 
 	// we return the nonce as part of the returned value
-	joined = append(ciphertext[:], nonce[:]...)
+	joined = append(ciphertext, nonce...)
 	return joined, nil
 }
 

internal/cryptutil/encrypt_test.go

@@ -14,7 +14,7 @@ func TestEncodeAndDecodeAccessToken(t *testing.T) {
 	plaintext := []byte("my plain text value")
 
 	key := GenerateKey()
-	c, err := NewCipher([]byte(key))
+	c, err := NewCipher(key)
 	if err != nil {
 		t.Fatalf("unexpected err: %v", err)
 	}
@@ -43,7 +43,7 @@ func TestEncodeAndDecodeAccessToken(t *testing.T) {
 func TestMarshalAndUnmarshalStruct(t *testing.T) {
 	key := GenerateKey()
 
-	c, err := NewCipher([]byte(key))
+	c, err := NewCipher(key)
 	if err != nil {
 		t.Fatalf("unexpected err: %v", err)
 	}

internal/httputil/client.go

@@ -62,7 +62,6 @@ func Client(method, endpoint, userAgent string, headers map[string]string, param
 	if err != nil {
 		return err
 	}
-	// log.Info().Msgf("%s", respBody)
 	if resp.StatusCode != http.StatusOK {
 		switch resp.StatusCode {
 		case http.StatusBadRequest:

internal/httputil/https.go

@@ -14,8 +14,6 @@ import (
 
 	"github.com/pomerium/pomerium/internal/fileutil"
 	"github.com/pomerium/pomerium/internal/log"
-
-	"google.golang.org/grpc"
 )
 
 // Options contains the configurations settings for a TLS http server.
@@ -81,7 +79,7 @@ func (o *Options) applyDefaults() {
 
 // ListenAndServeTLS serves the provided handlers by HTTPS
 // using the provided options.
-func ListenAndServeTLS(opt *Options, httpHandler http.Handler, grpcHandler *grpc.Server) error {
+func ListenAndServeTLS(opt *Options, httpHandler http.Handler, grpcHandler http.Handler) error {
 	if opt == nil {
 		opt = defaultOptions
 	} else {
@@ -97,10 +95,7 @@ func ListenAndServeTLS(opt *Options, httpHandler http.Handler, grpcHandler *grpc
 	if err != nil {
 		return fmt.Errorf("https: failed loading x509 certificate: %v", err)
 	}
-	config, err := newDefaultTLSConfig(cert)
-	if err != nil {
-		return fmt.Errorf("https: setting up TLS config: %v", err)
-	}
+	config := newDefaultTLSConfig(cert)
 	ln, err := net.Listen("tcp", opt.Addr)
 	if err != nil {
 		return err
@@ -168,7 +163,7 @@ func readCertificateFile(certFile, certKeyFile string) (*tls.Certificate, error)
 // https://blog.cloudflare.com/exposing-go-on-the-internet/
 // https://github.com/ssllabs/research/wiki/SSL-and-TLS-Deployment-Best-Practices
 // https://github.com/golang/go/blob/df91b8044dbe790c69c16058330f545be069cc1f/src/crypto/tls/common.go#L919
-func newDefaultTLSConfig(cert *tls.Certificate) (*tls.Config, error) {
+func newDefaultTLSConfig(cert *tls.Certificate) *tls.Config {
 	tlsConfig := &tls.Config{
 		MinVersion: tls.VersionTLS12,
 		// Prioritize cipher suites sped up by AES-NI (AES-GCM)
@@ -191,12 +186,12 @@ func newDefaultTLSConfig(cert *tls.Certificate) (*tls.Config, error) {
 		NextProtos: []string{"h2"},
 	}
 	tlsConfig.BuildNameToCertificate()
-	return tlsConfig, nil
+	return tlsConfig
 }
 
 // grpcHandlerFunc splits request serving between gRPC and HTTPS depending on the request type.
 // Requires HTTP/2.
-func grpcHandlerFunc(rpcServer *grpc.Server, other http.Handler) http.Handler {
+func grpcHandlerFunc(rpcServer http.Handler, other http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		ct := r.Header.Get("Content-Type")
 		if r.ProtoMajor == 2 && strings.Contains(ct, "application/grpc") {

internal/log/log_test.go

@@ -22,7 +22,7 @@ func setup() {
 	// examples to pass, we need to override zerolog.TimestampFunc
 	// and log.Logger globals -- you would not normally need to do this
 	zerolog.TimestampFunc = func() time.Time {
-		return time.Date(2008, 1, 8, 17, 5, 05, 0, time.UTC)
+		return time.Date(2008, 1, 8, 17, 5, 5, 0, time.UTC)
 	}
 	log.Logger = zerolog.New(os.Stdout).With().Timestamp().Logger()
 }
@@ -95,7 +95,7 @@ func ExampleError() {
 // Example of a log at a particular "level" (in this case, "fatal")
 func ExampleFatal() {
 	setup()
-	err := errors.New("A repo man spends his life getting into tense situations")
+	err := errors.New("a repo man spends his life getting into tense situations")
 	service := "myservice"
 
 	log.Fatal().
@@ -103,7 +103,7 @@ func ExampleFatal() {
 		Str("service", service).
 		Msg("Cannot start")
 
-	// Outputs: {"level":"fatal","time":1199811905,"error":"A repo man spends his life getting into tense situations","service":"myservice","message":"Cannot start myservice"}
+	// Outputs: {"level":"fatal","time":1199811905,"error":"a repo man spends his life getting into tense situations","service":"myservice","message":"Cannot start myservice"}
 }
 
 // This example uses command-line flags to demonstrate various outputs

internal/log/middleware_test.go

@@ -24,19 +24,15 @@ func TestGenerateUUID(t *testing.T) {
 		if prev == id {
 			t.Fatalf("Should get a new ID!")
 		}
-		matched, err := regexp.MatchString("[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}", id)
-		if !matched || err != nil {
-			t.Fatalf("expected match %s %v %s", id, matched, err)
+		matched := regexp.MustCompile("[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}").MatchString(id)
+		if !matched {
+			t.Fatalf("expected match %s %v", id, matched)
 		}
 	}
 }
 
-func decodeIfBinary(out *bytes.Buffer) string {
-	// p := out.Bytes()
-	// if len(p) == 0 || p[0] < 0x7F {
-	// 	return out.String()
-	// }
-	return out.String() //cbor.DecodeObjectToStr(p) + "\n"
+func decodeIfBinary(out fmt.Stringer) string {
+	return out.String()
 }
 
 func TestNewHandler(t *testing.T) {
@@ -182,9 +178,6 @@ func TestRequestIDHandler(t *testing.T) {
 		if !ok {
 			t.Fatal("Missing id in request")
 		}
-		// if want, got := id.String(), w.Header().Get("Request-Id"); got != want {
-		// 	t.Errorf("Invalid Request-Id header, got: %s, want: %s", got, want)
-		// }
 		l := FromRequest(r)
 		l.Log().Msg("")
 		if want, got := fmt.Sprintf(`{"id":"%s"}`+"\n", id), decodeIfBinary(out); want != got {

internal/metrics/exporter.go

@@ -9,8 +9,8 @@ import (
 )
 
 //NewPromHTTPListener creates a prometheus exporter on ListenAddr
-func NewPromHTTPListener(ListenAddr string) error {
-	return http.ListenAndServe(ListenAddr, newPromHTTPHandler())
+func NewPromHTTPListener(addr string) error {
+	return http.ListenAndServe(addr, newPromHTTPHandler())
 }
 
 // newPromHTTPHandler creates a new prometheus exporter handler for /metrics

internal/metrics/interceptors.go

@@ -22,7 +22,8 @@ var (
 		100, 250, 500, 750, 1000,
 	)
 
-	// GRPCServerRequestCountView is an OpenCensus view which counts GRPC Server requests by pomerium service, grpc service, grpc method, and status
+	// GRPCServerRequestCountView is an OpenCensus view which counts GRPC Server
+	// requests by pomerium service, grpc service, grpc method, and status
 	GRPCServerRequestCountView = &view.View{
 		Name:        "grpc_server_requests_total",
 		Measure:     ocgrpc.ServerLatency,
@@ -31,7 +32,8 @@ var (
 		Aggregation: view.Count(),
 	}
 
-	// GRPCServerRequestDurationView is an OpenCensus view which tracks GRPC Server request duration by pomerium service, grpc service, grpc method, and status
+	// GRPCServerRequestDurationView is an OpenCensus view which tracks GRPC Server
+	// request duration by pomerium service, grpc service, grpc method, and status
 	GRPCServerRequestDurationView = &view.View{
 		Name:        "grpc_server_request_duration_ms",
 		Measure:     ocgrpc.ServerLatency,
@@ -40,7 +42,8 @@ var (
 		Aggregation: grcpLatencyDistribution,
 	}
 
-	// GRPCServerResponseSizeView is an OpenCensus view which tracks GRPC Server response size by pomerium service, grpc service, grpc method, and status
+	// GRPCServerResponseSizeView is an OpenCensus view which tracks GRPC Server
+	// response size by pomerium service, grpc service, grpc method, and status
 	GRPCServerResponseSizeView = &view.View{
 		Name:        "grpc_server_response_size_bytes",
 		Measure:     ocgrpc.ServerSentBytesPerRPC,
@@ -49,7 +52,8 @@ var (
 		Aggregation: grpcSizeDistribution,
 	}
 
-	// GRPCServerRequestSizeView is an OpenCensus view which tracks GRPC Server request size by pomerium service, grpc service, grpc method, and status
+	// GRPCServerRequestSizeView is an OpenCensus view which tracks GRPC Server
+	// request size by pomerium service, grpc service, grpc method, and status
 	GRPCServerRequestSizeView = &view.View{
 		Name:        "grpc_server_request_size_bytes",
 		Measure:     ocgrpc.ServerReceivedBytesPerRPC,
@@ -58,7 +62,8 @@ var (
 		Aggregation: grpcSizeDistribution,
 	}
 
-	// GRPCClientRequestCountView is an OpenCensus view which tracks GRPC Client requests by pomerium service, target host, grpc service, grpc method, and status
+	// GRPCClientRequestCountView is an OpenCensus view which tracks GRPC Client
+	// requests by pomerium service, target host, grpc service, grpc method, and status
 	GRPCClientRequestCountView = &view.View{
 		Name:        "grpc_client_requests_total",
 		Measure:     ocgrpc.ClientRoundtripLatency,
@@ -67,7 +72,8 @@ var (
 		Aggregation: view.Count(),
 	}
 
-	// GRPCClientRequestDurationView is an OpenCensus view which tracks GRPC Client request duration by pomerium service, target host, grpc service, grpc method, and status
+	// GRPCClientRequestDurationView is an OpenCensus view which tracks GRPC Client
+	// request duration by pomerium service, target host, grpc service, grpc method, and status
 	GRPCClientRequestDurationView = &view.View{
 		Name:        "grpc_client_request_duration_ms",
 		Measure:     ocgrpc.ClientRoundtripLatency,
@@ -76,7 +82,8 @@ var (
 		Aggregation: grcpLatencyDistribution,
 	}
 
-	// GRPCClientResponseSizeView  is an OpenCensus view which tracks GRPC Client response size by pomerium service, target host, grpc service, grpc method, and status
+	// GRPCClientResponseSizeView  is an OpenCensus view which tracks GRPC Client
+	// response size by pomerium service, target host, grpc service, grpc method, and status
 	GRPCClientResponseSizeView = &view.View{
 		Name:        "grpc_client_response_size_bytes",
 		Measure:     ocgrpc.ClientReceivedBytesPerRPC,
@@ -85,7 +92,8 @@ var (
 		Aggregation: grpcSizeDistribution,
 	}
 
-	// GRPCClientRequestSizeView  is an OpenCensus view which tracks GRPC Client request size by pomerium service, target host, grpc service, grpc method, and status
+	// GRPCClientRequestSizeView  is an OpenCensus view which tracks GRPC Client
+	// request size by pomerium service, target host, grpc service, grpc method, and status
 	GRPCClientRequestSizeView = &view.View{
 		Name:        "grpc_client_request_size_bytes",
 		Measure:     ocgrpc.ClientSentBytesPerRPC,
@@ -95,8 +103,8 @@ var (
 	}
 )
 
-// GRPCClientInterceptor creates a UnaryClientInterceptor which updates the RPC context with metric tag
-// metadata
+// GRPCClientInterceptor creates a UnaryClientInterceptor which updates the RPC
+// context with metric tag metadata
 func GRPCClientInterceptor(service string) grpc.UnaryClientInterceptor {
 	return func(
 		ctx context.Context,

internal/metrics/middleware.go

@@ -69,7 +69,7 @@ var (
 	// HTTPClientRequestCountView is an OpenCensus View that tracks HTTP client requests by pomerium service, destination, host, method and status
 	HTTPClientRequestCountView = &view.View{
 		Name:        "http_client_requests_total",
-		Measure:     ochttp.ClientLatency,
+		Measure:     ochttp.ClientRoundtripLatency,
 		Description: "Total HTTP Client Requests",
 		TagKeys:     []tag.Key{keyService, keyHost, keyHTTPMethod, ochttp.StatusCode, keyDestination},
 		Aggregation: view.Count(),

internal/metrics/tags.go

@@ -5,8 +5,8 @@ import (
 )
 
 var (
+	//	keyStatus      tag.Key = tag.MustNewKey("status")
 	keyHTTPMethod  tag.Key = tag.MustNewKey("http_method")
-	keyStatus      tag.Key = tag.MustNewKey("status")
 	keyService     tag.Key = tag.MustNewKey("service")
 	keyGRPCService tag.Key = tag.MustNewKey("grpc_service")
 	keyGRPCMethod  tag.Key = tag.MustNewKey("grpc_method")

internal/middleware/chain.go

@@ -18,7 +18,7 @@ type Chain struct {
 // New serves no other function,
 // constructors are only called upon a call to Then().
 func NewChain(constructors ...Constructor) Chain {
-	return Chain{append(([]Constructor)(nil), constructors...)}
+	return Chain{append([]Constructor(nil), constructors...)}
 }
 
 // Then chains the middleware and returns the final http.Handler.

internal/middleware/chain_test.go

@@ -77,7 +77,7 @@ func TestThenFuncConstructsHandlerFunc(t *testing.T) {
 
 	chained.ServeHTTP(rec, (*http.Request)(nil))
 
-	if reflect.TypeOf(chained) != reflect.TypeOf((http.HandlerFunc)(nil)) {
+	if reflect.TypeOf(chained) != reflect.TypeOf(http.HandlerFunc(nil)) {
 		t.Error("ThenFunc does not construct HandlerFunc")
 	}
 }

internal/middleware/middleware_test.go

@@ -56,12 +56,12 @@ func Test_ValidSignature(t *testing.T) {
 		secret      string
 		want        bool
 	}{
-		{"good signature", goodURL, string(sig), now, secretA, true},
-		{"empty redirect url", "", string(sig), now, secretA, false},
-		{"bad redirect url", "https://google.com^", string(sig), now, secretA, false},
-		{"malformed signature", goodURL, string(sig + "^"), now, "&*&@**($&#(", false},
-		{"malformed timestamp", goodURL, string(sig), now + "^", secretA, false},
-		{"stale timestamp", goodURL, string(sig), staleTime, secretA, false},
+		{"good signature", goodURL, sig, now, secretA, true},
+		{"empty redirect url", "", sig, now, secretA, false},
+		{"bad redirect url", "https://google.com^", sig, now, secretA, false},
+		{"malformed signature", goodURL, sig + "^", now, "&*&@**($&#(", false},
+		{"malformed timestamp", goodURL, sig, now + "^", secretA, false},
+		{"stale timestamp", goodURL, sig, staleTime, secretA, false},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

internal/middleware/reverse_proxy_test.go

@@ -84,7 +84,7 @@ func TestStripPomeriumCookie(t *testing.T) {
 				Name:  tt.pomeriumCookie,
 				Value: "pomerium cookie!",
 			})
-			req := &http.Request{Header: http.Header{"Cookie": rr.HeaderMap["Set-Cookie"]}}
+			req := &http.Request{Header: http.Header{"Cookie": rr.Header()["Set-Cookie"]}}
 
 			handler := StripPomeriumCookie(tt.pomeriumCookie)(testHandler)
 			handler.ServeHTTP(rr, req)

internal/sessions/cookie_store.go

@@ -140,7 +140,7 @@ func (s *CookieStore) setCookie(w http.ResponseWriter, cookie *http.Cookie) {
 		} else {
 			// subsequent parts will be postfixed with their part number
 			nc.Name = fmt.Sprintf("%s_%d", cookie.Name, i)
-			nc.Value = fmt.Sprintf("%s", c)
+			nc.Value = c
 		}
 		http.SetCookie(w, &nc)
 	}

internal/sessions/cookie_store_test.go

@@ -31,7 +31,7 @@ func (a mockCipher) Decrypt(s []byte) ([]byte, error) {
 }
 func (a mockCipher) Marshal(s interface{}) (string, error) { return "", errors.New("error") }
 func (a mockCipher) Unmarshal(s string, i interface{}) error {
-	if string(s) == "unmarshal error" || string(s) == "error" {
+	if s == "unmarshal error" || s == "error" {
 		return errors.New("error")
 	}
 	return nil

internal/sessions/rest_store_test.go

@@ -14,7 +14,7 @@ import (
 )
 
 func TestRestStore_SaveSession(t *testing.T) {
-	now := time.Date(2008, 1, 8, 17, 5, 05, 0, time.UTC)
+	now := time.Date(2008, 1, 8, 17, 5, 5, 0, time.UTC)
 
 	tests := []struct {
 		name             string

internal/sessions/session_state_test.go

@@ -13,7 +13,7 @@ import (
 
 func TestSessionStateSerialization(t *testing.T) {
 	secret := cryptutil.GenerateKey()
-	c, err := cryptutil.NewCipher([]byte(secret))
+	c, err := cryptutil.NewCipher(secret)
 	if err != nil {
 		t.Fatalf("expected to be able to create cipher: %v", err)
 	}
@@ -144,7 +144,7 @@ func TestSessionState_Impersonating(t *testing.T) {
 
 func TestMarshalSession(t *testing.T) {
 	secret := cryptutil.GenerateKey()
-	c, err := cryptutil.NewCipher([]byte(secret))
+	c, err := cryptutil.NewCipher(secret)
 	if err != nil {
 		t.Fatalf("expected to be able to create cipher: %v", err)
 	}

internal/tripper/chain.go

@@ -18,7 +18,7 @@ type Chain struct {
 // New serves no other function,
 // constructors are only called upon a call to Then().
 func NewChain(constructors ...Constructor) Chain {
-	return Chain{append(([]Constructor)(nil), constructors...)}
+	return Chain{append([]Constructor(nil), constructors...)}
 }
 
 // Then chains the trippers and returns the final http.RoundTripper.

proxy/clients/mock_clients_test.go

@@ -16,26 +16,26 @@ func TestMockAuthenticate(t *testing.T) {
 		RefreshToken: "RefreshToken",
 	}
 	ma := &MockAuthenticate{
-		RedeemError:    errors.New("RedeemError"),
+		RedeemError:    errors.New("redeem error"),
 		RedeemResponse: redeemResponse,
 		RefreshResponse: &sessions.SessionState{
 			AccessToken:  "AccessToken",
 			RefreshToken: "RefreshToken",
 		},
-		RefreshError:     errors.New("RefreshError"),
+		RefreshError:     errors.New("refresh error"),
 		ValidateResponse: true,
-		ValidateError:    errors.New("ValidateError"),
-		CloseError:       errors.New("CloseError"),
+		ValidateError:    errors.New("validate error"),
+		CloseError:       errors.New("close error"),
 	}
 	got, gotErr := ma.Redeem(context.Background(), "a")
-	if gotErr.Error() != "RedeemError" {
+	if gotErr.Error() != "redeem error" {
 		t.Errorf("unexpected value for gotErr %s", gotErr)
 	}
 	if !reflect.DeepEqual(redeemResponse, got) {
 		t.Errorf("unexpected value for redeemResponse %s", got)
 	}
 	newSession, gotErr := ma.Refresh(context.Background(), nil)
-	if gotErr.Error() != "RefreshError" {
+	if gotErr.Error() != "refresh error" {
 		t.Errorf("unexpected value for gotErr %s", gotErr)
 	}
 	if !reflect.DeepEqual(newSession, redeemResponse) {
@@ -46,11 +46,11 @@ func TestMockAuthenticate(t *testing.T) {
 	if !ok {
 		t.Errorf("unexpected value for ok : %t", ok)
 	}
-	if gotErr.Error() != "ValidateError" {
+	if gotErr.Error() != "validate error" {
 		t.Errorf("unexpected value for gotErr %s", gotErr)
 	}
 	gotErr = ma.Close()
-	if gotErr.Error() != "CloseError" {
+	if gotErr.Error() != "close error" {
 		t.Errorf("unexpected value for ma.CloseError %s", gotErr)
 	}
 

proxy/handlers.go

@@ -339,7 +339,7 @@ func (p *Proxy) UserDashboard(w http.ResponseWriter, r *http.Request) {
 		SignoutURL:       p.GetSignOutURL(p.AuthenticateURL, redirectURL).String(),
 		IsAdmin:          isAdmin,
 		ImpersonateEmail: session.ImpersonateEmail,
-		ImpersonateGroup: strings.Join(session.ImpersonateGroups[:], ","),
+		ImpersonateGroup: strings.Join(session.ImpersonateGroups, ","),
 		CSRF:             csrf.SessionID,
 	}
 	templates.New().ExecuteTemplate(w, "dashboard.html", t)

proxy/handlers_test.go

@@ -40,7 +40,7 @@ func (a mockCipher) Marshal(s interface{}) (string, error) {
 	return "ok", nil
 }
 func (a mockCipher) Unmarshal(s string, i interface{}) error {
-	if string(s) == "unmarshal error" || string(s) == "error" {
+	if s == "unmarshal error" || s == "error" {
 		return errors.New("error")
 	}
 	return nil
@@ -432,7 +432,6 @@ func TestProxy_Refresh(t *testing.T) {
 			p.Refresh(w, r)
 			if status := w.Code; status != tt.wantStatus {
 				t.Errorf("status code: got %v want %v", status, tt.wantStatus)
-				// t.Errorf("\n%+v", w.Body.String())
 				t.Errorf("\n%+v", opts)
 			}
 		})
@@ -494,7 +493,6 @@ func TestProxy_Impersonate(t *testing.T) {
 			p.Impersonate(w, r)
 			if status := w.Code; status != tt.wantStatus {
 				t.Errorf("status code: got %v want %v", status, tt.wantStatus)
-				// t.Errorf("\n%+v", w.Body.String())
 				t.Errorf("\n%+v", opts)
 			}
 		})

proxy/proxy.go

@@ -276,8 +276,7 @@ func (p *Proxy) customCAPool(cert string) (*x509.CertPool, error) {
 }
 
 // newReverseProxyHandler applies handler specific options to a given route.
-func (p *Proxy) newReverseProxyHandler(rp *httputil.ReverseProxy, route *config.Policy) (http.Handler, error) {
-	var handler http.Handler
+func (p *Proxy) newReverseProxyHandler(rp *httputil.ReverseProxy, route *config.Policy) (handler http.Handler, err error) {
 	handler = &UpstreamProxy{
 		name:    route.Destination.Host,
 		handler: rp,