Merge pull request #163 from xmidt-org/denopink/refactoring/remove-we…

…bpa-common/logging

Remove `go-kit/kit/log` & `go-kit/log`

@johnabass @renaz6 same pr as #150 no need to re-review. merging since changes have been already approved

.github/workflows/ci.yml

@@ -20,6 +20,5 @@ jobs:
   ci:
     uses: xmidt-org/.github/.github/workflows/go-ci.yml@go-ci-v1
     with:
-      lint-skip: true
       style-skip: true
     secrets: inherit

.golangci.yaml

@@ -2,13 +2,16 @@
 linters-settings:
   misspell:
     locale: US
-
+  errorlint:
+    # Report non-wrapping error creation using fmt.Errorf
+    errorf: false
+
 linters:
   enable:
     - bodyclose
     - dupl
     - errorlint
-    - funlen
+    # - funlen
     - goconst
     - gosec
     - misspell
@@ -24,8 +27,3 @@ issues:
       linters:
         - dupl
         - funlen
-
-linters-settings:
-  errorlint:
-    # Report non-wrapping error creation using fmt.Errorf
-    errorf: false

CHANGELOG.md

@@ -5,13 +5,17 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+- [Enable & Fix Linter #149](https://github.com/xmidt-org/bascule/issues/149)
+- [Remove go-kit/kit & go-kit/log #148](https://github.com/xmidt-org/bascule/issues/148)
+- [Move to zap logger #103](https://github.com/xmidt-org/bascule/issues/103)
+- Security patch, remove debug logged token
 
 ## [v0.11.1]
 - [SetLogger Bug: Shared logger among requests creates repeating logging context #159](https://github.com/xmidt-org/bascule/issues/159)
 - [CVE-2022-32149 (High) detected in golang.org/x/text-v0.3.7 #153](https://github.com/xmidt-org/bascule/issues/153)
 
 ## [v0.11.0]
-- Refactored basculehttp to use Clortho instead of key package. [135](https://github.com/xmidt-org/bascule/pull/135)
+- Refactored basculehttp to use Clortho instead of key package [135](https://github.com/xmidt-org/bascule/pull/135)
 - Update dependencies. [131](https://github.com/xmidt-org/bascule/pull/131)
     -  [github.com/gorilla/sessions v1.2.1 cwe-613 no patch available](https://ossindex.sonatype.org/vulnerability/sonatype-2021-4899)
 - Update dependencies. [130](https://github.com/xmidt-org/bascule/pull/130)

acquire/bearer.go

@@ -19,7 +19,7 @@ package acquire
 
 import (
 	"fmt"
-	"io/ioutil"
+	"io"
 	"net/http"
 	"sync"
 	"time"
@@ -106,7 +106,7 @@ func (acquirer *RemoteBearerTokenAcquirer) Acquire() (string, error) {
 		return "", fmt.Errorf("received non 200 code acquiring Bearer: code %v", resp.Status)
 	}
 
-	respBody, errRead := ioutil.ReadAll(resp.Body)
+	respBody, errRead := io.ReadAll(resp.Body)
 	if errRead != nil {
 		return "", fmt.Errorf("error reading HTTP response body: %v", errRead)
 	}

basculehttp/constructor.go

@@ -24,11 +24,11 @@ import (
 	"net/http"
 	"strings"
 
-	"github.com/go-kit/kit/log"
-	"github.com/go-kit/kit/log/level"
 	"github.com/justinas/alice"
 	"github.com/xmidt-org/bascule"
+	"github.com/xmidt-org/sallust"
 	"go.uber.org/fx"
+	"go.uber.org/zap"
 )
 
 const (
@@ -77,13 +77,13 @@ type constructor struct {
 	headerName          string
 	headerDelimiter     string
 	authorizations      map[bascule.Authorization]TokenFactory
-	getLogger           func(context.Context) log.Logger
+	getLogger           sallust.GetLoggerFunc
 	parseURL            ParseURL
 	onErrorResponse     OnErrorResponse
 	onErrorHTTPResponse OnErrorHTTPResponse
 }
 
-func (c *constructor) authenticationOutput(logger log.Logger, request *http.Request) (bascule.Authentication, ErrorResponseReason, error) {
+func (c *constructor) authenticationOutput(logger *zap.Logger, request *http.Request) (bascule.Authentication, ErrorResponseReason, error) {
 	urlVal := *request.URL // copy the URL before modifying it
 	u, err := c.parseURL(&urlVal)
 	if err != nil {
@@ -124,17 +124,16 @@ func (c *constructor) decorate(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		logger := c.getLogger(r.Context())
 		if logger == nil {
-			logger = defaultGetLoggerFunc(r.Context())
+			logger = sallust.Get(r.Context())
 		}
 		auth, errReason, err := c.authenticationOutput(logger, r)
 		if err != nil {
-			level.Error(logger).Log(errorKey, err, "auth", r.Header.Get(c.headerName))
+			logger.Error(err.Error(), zap.String("auth", r.Header.Get(c.headerName)))
 			c.onErrorResponse(errReason, err)
 			c.onErrorHTTPResponse(w, errReason)
 			return
 		}
 		ctx := bascule.WithAuthentication(r.Context(), auth)
-		level.Debug(logger).Log("msg", "authentication added to context", "token", auth.Token, "key", auth.Authorization)
 		next.ServeHTTP(w, r.WithContext(ctx))
 	})
 }
@@ -147,7 +146,7 @@ func NewConstructor(options ...COption) func(http.Handler) http.Handler {
 		headerName:          DefaultHeaderName,
 		headerDelimiter:     DefaultHeaderDelimiter,
 		authorizations:      make(map[bascule.Authorization]TokenFactory),
-		getLogger:           defaultGetLoggerFunc,
+		getLogger:           sallust.Get,
 		parseURL:            DefaultParseURLFunc,
 		onErrorResponse:     DefaultOnErrorResponse,
 		onErrorHTTPResponse: DefaultOnErrorHTTPResponse,
@@ -193,7 +192,7 @@ func WithTokenFactory(key bascule.Authorization, tf TokenFactory) COption {
 
 // WithCLogger sets the function to use to get the logger from the context.
 // If no logger is set, nothing is logged.
-func WithCLogger(getLogger func(context.Context) log.Logger) COption {
+func WithCLogger(getLogger sallust.GetLoggerFunc) COption {
 	return func(c *constructor) {
 		if getLogger != nil {
 			c.getLogger = getLogger

basculehttp/constructor_test.go

@@ -18,14 +18,12 @@
 package basculehttp
 
 import (
-	"context"
 	"net/http"
 	"net/http/httptest"
-	"os"
 	"testing"
 
-	"github.com/go-kit/kit/log"
 	"github.com/stretchr/testify/assert"
+	"github.com/xmidt-org/sallust"
 )
 
 func TestConstructor(t *testing.T) {
@@ -37,17 +35,15 @@ func TestConstructor(t *testing.T) {
 		WithHeaderDelimiter(testDelimiter),
 		nil,
 		WithTokenFactory("Basic", BasicTokenFactory{"codex": "codex"}),
-		WithCLogger(func(_ context.Context) log.Logger {
-			return log.NewJSONLogger(log.NewSyncWriter(os.Stdout))
-		}),
+		WithCLogger(sallust.GetDefaultLogger),
 		WithParseURLFunc(CreateRemovePrefixURLFunc("/test", DefaultParseURLFunc)),
 		WithCErrorResponseFunc(DefaultOnErrorResponse),
 		WithCErrorHTTPResponseFunc(LegacyOnErrorHTTPResponse),
 	)
 	c2 := NewConstructor(
 		WithHeaderName(""),
 		WithHeaderDelimiter(""),
-		WithCLogger(func(_ context.Context) log.Logger { return nil }),
+		WithCLogger(sallust.GetNilLogger),
 		WithParseURLFunc(CreateRemovePrefixURLFunc("", nil)),
 	)
 	tests := []struct {

basculehttp/enforcer.go

@@ -18,15 +18,14 @@
 package basculehttp
 
 import (
-	"context"
 	"errors"
 	"net/http"
 
-	"github.com/go-kit/kit/log"
-	"github.com/go-kit/kit/log/level"
 	"github.com/justinas/alice"
 	"github.com/xmidt-org/bascule"
+	"github.com/xmidt-org/sallust"
 	"go.uber.org/fx"
+	"go.uber.org/zap"
 )
 
 //go:generate stringer -type=NotFoundBehavior
@@ -54,7 +53,7 @@ type EOptionsIn struct {
 type enforcer struct {
 	notFoundBehavior NotFoundBehavior
 	rules            map[bascule.Authorization]bascule.Validator
-	getLogger        func(context.Context) log.Logger
+	getLogger        sallust.GetLoggerFunc
 	onErrorResponse  OnErrorResponse
 }
 
@@ -63,22 +62,21 @@ func (e *enforcer) decorate(next http.Handler) http.Handler {
 		ctx := request.Context()
 		logger := e.getLogger(ctx)
 		if logger == nil {
-			logger = defaultGetLoggerFunc(ctx)
+			logger = sallust.Get(ctx)
 		}
 		auth, ok := bascule.FromContext(ctx)
 		if !ok {
 			err := errors.New("no authentication found")
-			logger.Log(level.Key(), level.ErrorValue(), errorKey, err.Error())
+			logger.Error(err.Error())
 			e.onErrorResponse(MissingAuthentication, err)
 			response.WriteHeader(http.StatusForbidden)
 			return
 		}
 		rules, ok := e.rules[auth.Authorization]
 		if !ok {
 			err := errors.New("no rules found for authorization")
-			logger.Log(level.Key(), level.ErrorValue(),
-				errorKey, err.Error(), "rules", rules,
-				"authorization", auth.Authorization, "behavior", e.notFoundBehavior)
+			logger.Error(err.Error(), zap.Any("rules", rules),
+				zap.String("authorization", string(auth.Authorization)), zap.Int("behavior", int(e.notFoundBehavior)))
 			switch e.notFoundBehavior {
 			case Forbid:
 				e.onErrorResponse(ChecksNotFound, err)
@@ -94,13 +92,13 @@ func (e *enforcer) decorate(next http.Handler) http.Handler {
 		} else {
 			err := rules.Check(ctx, auth.Token)
 			if err != nil {
-				logger.Log(level.Key(), level.ErrorValue(), errorKey, err)
+				logger.Error(err.Error())
 				e.onErrorResponse(ChecksFailed, err)
 				WriteResponse(response, http.StatusForbidden, err)
 				return
 			}
 		}
-		logger.Log(level.Key(), level.DebugValue(), "msg", "authentication accepted by enforcer")
+		logger.Debug("authentication accepted by enforcer")
 		next.ServeHTTP(response, request)
 	})
 }
@@ -111,7 +109,7 @@ func (e *enforcer) decorate(next http.Handler) http.Handler {
 func NewEnforcer(options ...EOption) func(http.Handler) http.Handler {
 	e := &enforcer{
 		rules:           make(map[bascule.Authorization]bascule.Validator),
-		getLogger:       defaultGetLoggerFunc,
+		getLogger:       sallust.Get,
 		onErrorResponse: DefaultOnErrorResponse,
 	}
 
@@ -146,7 +144,7 @@ func WithRules(key bascule.Authorization, v bascule.Validator) EOption {
 
 // WithELogger sets the function to use to get the logger from the context.
 // If no logger is set, nothing is logged.
-func WithELogger(getLogger func(context.Context) log.Logger) EOption {
+func WithELogger(getLogger sallust.GetLoggerFunc) EOption {
 	return func(e *enforcer) {
 		if getLogger != nil {
 			e.getLogger = getLogger

basculehttp/enforcer_test.go

@@ -21,25 +21,22 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
-	"os"
 	"testing"
 
-	"github.com/go-kit/kit/log"
 	"github.com/stretchr/testify/assert"
 	"github.com/xmidt-org/bascule"
 	"github.com/xmidt-org/bascule/basculechecks"
+	"github.com/xmidt-org/sallust"
 )
 
 func TestEnforcer(t *testing.T) {
 	e := NewEnforcer(
 		WithNotFoundBehavior(Allow),
-		WithELogger(func(_ context.Context) log.Logger { return nil }),
+		WithELogger(sallust.GetNilLogger),
 	)
 	e2 := NewEnforcer(
 		WithRules("jwt", bascule.Validators{basculechecks.NonEmptyType()}),
-		WithELogger(func(_ context.Context) log.Logger {
-			return log.NewJSONLogger(log.NewSyncWriter(os.Stdout))
-		}),
+		WithELogger(sallust.GetDefaultLogger),
 		WithEErrorResponseFunc(DefaultOnErrorResponse),
 	)
 	emptyAttributes := bascule.NewAttributes(map[string]interface{}{})

basculehttp/log.go

@@ -18,42 +18,18 @@
 package basculehttp
 
 import (
-	"context"
 	"net"
 	"net/http"
 	"strings"
 
-	"github.com/go-kit/kit/log"
 	"github.com/justinas/alice"
 	"github.com/xmidt-org/bascule"
 	"github.com/xmidt-org/candlelight"
 	"github.com/xmidt-org/sallust"
-	"github.com/xmidt-org/sallust/sallustkit"
 	"go.uber.org/fx"
 	"go.uber.org/zap"
 )
 
-var (
-	defaultLogger = log.NewNopLogger()
-
-	errorKey interface{} = "error"
-)
-
-// defaultGetLoggerFunc returns the default logger, which doesn't do anything.
-func defaultGetLoggerFunc(_ context.Context) log.Logger {
-	return defaultLogger
-}
-
-// getZapLogger converts a zap logger to a go-kit logger. This won't be needed
-// when basculehttp starts using the zap logger directly.
-func getZapLogger(f func(context.Context) *zap.Logger) func(context.Context) log.Logger {
-	return func(ctx context.Context) log.Logger {
-		return sallustkit.Logger{
-			Zap: f(ctx),
-		}
-	}
-}
-
 func sanitizeHeaders(headers http.Header) (filtered http.Header) {
 	filtered = headers.Clone()
 	if authHeader := filtered.Get("Authorization"); authHeader != "" {
@@ -86,7 +62,7 @@ func SetLogger(logger *zap.Logger) alice.Constructor {
 			}
 
 			l := logger.With(
-				zap.Reflect("request.Headers", sanitizeHeaders(r.Header)), //lgtm [go/clear-text-logging]
+				zap.Any("request.Headers", sanitizeHeaders(r.Header)), //lgtm [go/clear-text-logging]
 				zap.String("request.URL", r.URL.EscapedPath()),
 				zap.String("request.Method", r.Method),
 				zap.String("request.address", source),
@@ -124,16 +100,16 @@ func ProvideLogger() fx.Option {
 			// add logger constructor option
 			fx.Annotated{
 				Group: "bascule_constructor_options",
-				Target: func(getLogger func(context.Context) *zap.Logger) COption {
-					return WithCLogger(getZapLogger(getLogger))
+				Target: func(getLogger sallust.GetLoggerFunc) COption {
+					return WithCLogger(getLogger)
 				},
 			},
 
 			// add logger enforcer option
 			fx.Annotated{
 				Group: "bascule_enforcer_options",
-				Target: func(getLogger func(context.Context) *zap.Logger) EOption {
-					return WithELogger(getZapLogger(getLogger))
+				Target: func(getLogger sallust.GetLoggerFunc) EOption {
+					return WithELogger(getLogger)
 				},
 			},
 

basculehttp/log_test.go

@@ -18,26 +18,12 @@
 package basculehttp
 
 import (
-	"context"
 	"net/http"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"go.uber.org/zap"
 )
 
-func TestGetZapLogger(t *testing.T) {
-	l := zap.NewNop()
-	f := getZapLogger(func(_ context.Context) *zap.Logger {
-		return l
-	})
-	result := f(context.Background())
-	assert.NotNil(t, result)
-	assert.NotPanics(t, func() {
-		result.Log("msg", "testing", "error", "nope", "level", "debug")
-	})
-}
-
 func TestSanitizeHeaders(t *testing.T) {
 	testCases := []struct {
 		Description string