fix: Audit tests on the correct response writer type (#9445)

cmd/generic-handlers.go

@@ -27,6 +27,7 @@ import (
 	"github.com/minio/minio/cmd/config/etcd/dns"
 	"github.com/minio/minio/cmd/crypto"
 	xhttp "github.com/minio/minio/cmd/http"
+	"github.com/minio/minio/cmd/http/stats"
 	"github.com/minio/minio/cmd/logger"
 	"github.com/minio/minio/pkg/handlers"
 	"github.com/rs/cors"
@@ -535,12 +536,21 @@ func setHTTPStatsHandler(h http.Handler) http.Handler {
 }
 
 func (h httpStatsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	isS3Request := !strings.HasPrefix(r.URL.Path, minioReservedBucketPath)
-	// record s3 connection stats.
-	r.Body = &recordTrafficRequest{ReadCloser: r.Body, isS3Request: isS3Request}
-	recordResponse := &recordTrafficResponse{ResponseWriter: w, isS3Request: isS3Request}
+	// Meters s3 connection stats.
+	meteredRequest := &stats.IncomingTrafficMeter{ReadCloser: r.Body}
+	meteredResponse := &stats.OutgoingTrafficMeter{ResponseWriter: w}
+
 	// Execute the request
-	h.handler.ServeHTTP(recordResponse, r)
+	r.Body = meteredRequest
+	h.handler.ServeHTTP(meteredResponse, r)
+
+	if strings.HasPrefix(r.URL.Path, minioReservedBucketPath) {
+		globalConnStats.incInputBytes(meteredRequest.BytesCount())
+		globalConnStats.incOutputBytes(meteredResponse.BytesCount())
+	} else {
+		globalConnStats.incS3InputBytes(meteredRequest.BytesCount())
+		globalConnStats.incS3OutputBytes(meteredResponse.BytesCount())
+	}
 }
 
 // requestValidityHandler validates all the incoming paths for

cmd/handler-utils.go

@@ -28,6 +28,7 @@ import (
 	"net/url"
 	"regexp"
 	"strings"
+	"time"
 
 	xhttp "github.com/minio/minio/cmd/http"
 	"github.com/minio/minio/cmd/logger"
@@ -35,6 +36,8 @@ import (
 	"github.com/minio/minio/pkg/bucket/object/tagging"
 	"github.com/minio/minio/pkg/handlers"
 	"github.com/minio/minio/pkg/madmin"
+
+	stats "github.com/minio/minio/cmd/http/stats"
 )
 
 const (
@@ -360,36 +363,19 @@ func httpTraceHdrs(f http.HandlerFunc) http.HandlerFunc {
 
 func collectAPIStats(api string, f http.HandlerFunc) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
+		globalHTTPStats.currentS3Requests.Inc(api)
+		defer globalHTTPStats.currentS3Requests.Dec(api)
 
-		isS3Request := !strings.HasPrefix(r.URL.Path, minioReservedBucketPath)
+		statsWriter := stats.NewRecordAPIStats(w)
 
-		// Time start before the call is about to start.
-		tBefore := UTCNow()
-
-		apiStatsWriter := &recordAPIStats{ResponseWriter: w, TTFB: tBefore, isS3Request: isS3Request}
-
-		if isS3Request {
-			globalHTTPStats.currentS3Requests.Inc(api)
-		}
-
-		// Execute the request
-		f.ServeHTTP(apiStatsWriter, r)
-
-		if isS3Request {
-			globalHTTPStats.currentS3Requests.Dec(api)
-		}
-
-		// Firstbyte read.
-		tAfter := apiStatsWriter.TTFB
+		f.ServeHTTP(statsWriter, r)
 
 		// Time duration in secs since the call started.
-		//
 		// We don't need to do nanosecond precision in this
 		// simply for the fact that it is not human readable.
-		durationSecs := tAfter.Sub(tBefore).Seconds()
+		durationSecs := time.Since(statsWriter.StartTime).Seconds()
 
-		// Update http statistics
-		globalHTTPStats.updateStats(api, r, apiStatsWriter, durationSecs)
+		globalHTTPStats.updateStats(api, r, statsWriter, durationSecs)
 	}
 }
 

cmd/http-stats.go

@@ -23,6 +23,7 @@ import (
 	"sync"
 	"time"
 
+	stats "github.com/minio/minio/cmd/http/stats"
 	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/atomic"
 )
@@ -166,18 +167,18 @@ func (st *HTTPStats) toServerHTTPStats() ServerHTTPStats {
 }
 
 // Update statistics from http request and response data
-func (st *HTTPStats) updateStats(api string, r *http.Request, w *recordAPIStats, durationSecs float64) {
+func (st *HTTPStats) updateStats(api string, r *http.Request, w *stats.RecordAPIStats, durationSecs float64) {
 	// A successful request has a 2xx response code
-	successReq := (w.respStatusCode >= 200 && w.respStatusCode < 300)
+	successReq := (w.RespStatusCode >= 200 && w.RespStatusCode < 300)
 
-	if w.isS3Request && !strings.HasSuffix(r.URL.Path, prometheusMetricsPath) {
+	if !strings.HasSuffix(r.URL.Path, prometheusMetricsPath) {
 		st.totalS3Requests.Inc(api)
-		if !successReq && w.respStatusCode != 0 {
+		if !successReq && w.RespStatusCode != 0 {
 			st.totalS3Errors.Inc(api)
 		}
 	}
 
-	if w.isS3Request && r.Method == "GET" {
+	if r.Method == "GET" {
 		// Increment the prometheus http request response histogram with appropriate label
 		httpRequestsDuration.With(prometheus.Labels{"api": api}).Observe(durationSecs)
 	}

cmd/http-traffic-recorder.go

@@ -1,91 +0,0 @@
-/*
- * MinIO Cloud Storage, (C) 2019 MinIO, Inc.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package cmd
-
-import (
-	"io"
-	"net/http"
-	"time"
-)
-
-// records the incoming bytes from the underlying request.Body.
-type recordTrafficRequest struct {
-	io.ReadCloser
-	isS3Request bool
-}
-
-// Records the bytes read.
-func (r *recordTrafficRequest) Read(p []byte) (n int, err error) {
-	n, err = r.ReadCloser.Read(p)
-	globalConnStats.incInputBytes(n)
-	if r.isS3Request {
-		globalConnStats.incS3InputBytes(n)
-	}
-	return n, err
-}
-
-// Records the outgoing bytes through the responseWriter.
-type recordTrafficResponse struct {
-	// wrapper for underlying http.ResponseWriter.
-	http.ResponseWriter
-	isS3Request bool
-}
-
-// Records the output bytes
-func (r *recordTrafficResponse) Write(p []byte) (n int, err error) {
-	n, err = r.ResponseWriter.Write(p)
-	globalConnStats.incOutputBytes(n)
-	// Check if it is s3 request
-	if r.isS3Request {
-		globalConnStats.incS3OutputBytes(n)
-	}
-	return n, err
-}
-
-// Calls the underlying Flush.
-func (r *recordTrafficResponse) Flush() {
-	r.ResponseWriter.(http.Flusher).Flush()
-}
-
-// Records the outgoing bytes through the responseWriter.
-type recordAPIStats struct {
-	http.ResponseWriter
-	TTFB           time.Time // TimeToFirstByte.
-	firstByteRead  bool
-	respStatusCode int
-	isS3Request    bool
-}
-
-// Calls the underlying WriteHeader.
-func (r *recordAPIStats) WriteHeader(i int) {
-	r.respStatusCode = i
-	r.ResponseWriter.WriteHeader(i)
-}
-
-// Records the TTFB on the first byte write.
-func (r *recordAPIStats) Write(p []byte) (n int, err error) {
-	if !r.firstByteRead {
-		r.TTFB = UTCNow()
-		r.firstByteRead = true
-	}
-	return r.ResponseWriter.Write(p)
-}
-
-// Calls the underlying Flush.
-func (r *recordAPIStats) Flush() {
-	r.ResponseWriter.(http.Flusher).Flush()
-}

cmd/http/stats/http-traffic-recorder.go

@@ -0,0 +1,107 @@
+/*
+ * MinIO Cloud Storage, (C) 2019-2020 MinIO, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package stats
+
+import (
+	"io"
+	"net/http"
+	"time"
+)
+
+// IncomingTrafficMeter counts the incoming bytes from the underlying request.Body.
+type IncomingTrafficMeter struct {
+	io.ReadCloser
+	countBytes int
+}
+
+// Read calls the underlying Read and counts the transferred bytes.
+func (r *IncomingTrafficMeter) Read(p []byte) (n int, err error) {
+	n, err = r.ReadCloser.Read(p)
+	r.countBytes += n
+	return n, err
+}
+
+// BytesCount returns the number of transferred bytes
+func (r IncomingTrafficMeter) BytesCount() int {
+	return r.countBytes
+}
+
+// OutgoingTrafficMeter counts the outgoing bytes through the responseWriter.
+type OutgoingTrafficMeter struct {
+	// wrapper for underlying http.ResponseWriter.
+	http.ResponseWriter
+	countBytes int
+}
+
+// Write calls the underlying write and counts the output bytes
+func (w *OutgoingTrafficMeter) Write(p []byte) (n int, err error) {
+	n, err = w.ResponseWriter.Write(p)
+	w.countBytes += n
+	return n, err
+}
+
+// Flush calls the underlying Flush.
+func (w *OutgoingTrafficMeter) Flush() {
+	w.ResponseWriter.(http.Flusher).Flush()
+}
+
+// BytesCount returns the number of transferred bytes
+func (w OutgoingTrafficMeter) BytesCount() int {
+	return w.countBytes
+}
+
+// RecordAPIStats is a response writer which stores
+// information of the underlying http response.
+type RecordAPIStats struct {
+	http.ResponseWriter
+	TTFB           time.Duration // TimeToFirstByte.
+	StartTime      time.Time
+	RespStatusCode int
+
+	firstByteRead bool
+}
+
+// NewRecordAPIStats creates a new response writer with
+// start time set to the function call time.
+func NewRecordAPIStats(w http.ResponseWriter) *RecordAPIStats {
+	return &RecordAPIStats{
+		ResponseWriter: w,
+		StartTime:      time.Now().UTC(),
+	}
+}
+
+// WriteHeader calls the underlying WriteHeader
+// and records the response status code.
+func (r *RecordAPIStats) WriteHeader(i int) {
+	r.RespStatusCode = i
+	r.ResponseWriter.WriteHeader(i)
+}
+
+// Write calls the underlying Write and updates TTFB and other info
+func (r *RecordAPIStats) Write(p []byte) (n int, err error) {
+	if !r.firstByteRead {
+		r.TTFB = time.Now().UTC().Sub(r.StartTime)
+		r.firstByteRead = true
+	}
+	n, err = r.ResponseWriter.Write(p)
+	return
+}
+
+// Flush calls the underlying Flush.
+func (r *RecordAPIStats) Flush() {
+	r.ResponseWriter.(http.Flusher).Flush()
+}

cmd/logger/audit.go

@@ -26,6 +26,8 @@ import (
 
 	"github.com/gorilla/mux"
 	"github.com/minio/minio/cmd/logger/message/audit"
+
+	stats "github.com/minio/minio/cmd/http/stats"
 )
 
 // ResponseWriter - is a wrapper to trap the http response status code.
@@ -132,14 +134,22 @@ func AddAuditTarget(t Target) {
 
 // AuditLog - logs audit logs to all audit targets.
 func AuditLog(w http.ResponseWriter, r *http.Request, api string, reqClaims map[string]interface{}) {
-	var statusCode int
-	var timeToResponse time.Duration
-	var timeToFirstByte time.Duration
-	lrw, ok := w.(*ResponseWriter)
+	// Fast exit if there is not audit target configured
+	if len(AuditTargets) == 0 {
+		return
+	}
+
+	var (
+		statusCode      int
+		timeToResponse  time.Duration
+		timeToFirstByte time.Duration
+	)
+
+	st, ok := w.(*stats.RecordAPIStats)
 	if ok {
-		statusCode = lrw.StatusCode
-		timeToResponse = time.Now().UTC().Sub(lrw.StartTime)
-		timeToFirstByte = lrw.TimeToFirstByte
+		statusCode = st.RespStatusCode
+		timeToResponse = time.Now().UTC().Sub(st.StartTime)
+		timeToFirstByte = st.TTFB
 	}
 
 	vars := mux.Vars(r)
@@ -149,16 +159,17 @@ func AuditLog(w http.ResponseWriter, r *http.Request, api string, reqClaims map[
 		object = vars["object"]
 	}
 
+	entry := audit.ToEntry(w, r, reqClaims, globalDeploymentID)
+	entry.API.Name = api
+	entry.API.Bucket = bucket
+	entry.API.Object = object
+	entry.API.Status = http.StatusText(statusCode)
+	entry.API.StatusCode = statusCode
+	entry.API.TimeToFirstByte = timeToFirstByte.String()
+	entry.API.TimeToResponse = timeToResponse.String()
+
 	// Send audit logs only to http targets.
 	for _, t := range AuditTargets {
-		entry := audit.ToEntry(w, r, reqClaims, globalDeploymentID)
-		entry.API.Name = api
-		entry.API.Bucket = bucket
-		entry.API.Object = object
-		entry.API.Status = http.StatusText(statusCode)
-		entry.API.StatusCode = statusCode
-		entry.API.TimeToFirstByte = timeToFirstByte.String()
-		entry.API.TimeToResponse = timeToResponse.String()
 		_ = t.Send(entry, string(All))
 	}
 }