use logger.LogOnce to reduce printing disconnection logs (#15408)

fixes #15334

- re-use net/url parsed value for http.Request{}
- remove gosimple, structcheck and unusued due to https://github.com/golangci/golangci-lint/issues/2649
- unwrapErrs upto leafErr to ensure that we store exactly the correct errors

internal/rest/client.go

@@ -18,6 +18,7 @@
 package rest
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -26,6 +27,9 @@ import (
 	"math/rand"
 	"net/http"
 	"net/url"
+	"path"
+	"strings"
+	"sync"
 	"sync/atomic"
 	"time"
 
@@ -105,12 +109,10 @@ type Client struct {
 	httpClient   *http.Client
 	url          *url.URL
 	newAuthToken func(audience string) string
-}
 
-// URL query separator constants
-const (
-	querySep = "?"
-)
+	sync.RWMutex // mutex for lastErr
+	lastErr      error
+}
 
 type restError string
 
@@ -122,22 +124,112 @@ func (e restError) Timeout() bool {
 	return true
 }
 
-// Call - make a REST call with context.
-func (c *Client) Call(ctx context.Context, method string, values url.Values, body io.Reader, length int64) (reply io.ReadCloser, err error) {
-	if !c.IsOnline() {
-		return nil, &NetworkError{Err: &url.Error{Op: method, URL: c.url.String(), Err: restError("remote server offline")}}
+// Given a string of the form "host", "host:port", or "[ipv6::address]:port",
+// return true if the string includes a port.
+func hasPort(s string) bool { return strings.LastIndex(s, ":") > strings.LastIndex(s, "]") }
+
+// removeEmptyPort strips the empty port in ":port" to ""
+// as mandated by RFC 3986 Section 6.2.3.
+func removeEmptyPort(host string) string {
+	if hasPort(host) {
+		return strings.TrimSuffix(host, ":")
 	}
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.url.String()+method+querySep+values.Encode(), body)
-	if err != nil {
-		return nil, &NetworkError{err}
+	return host
+}
+
+// Copied from http.NewRequest but implemented to ensure we re-use `url.URL` instance.
+func (c *Client) newRequest(ctx context.Context, u *url.URL, body io.Reader) (*http.Request, error) {
+	rc, ok := body.(io.ReadCloser)
+	if !ok && body != nil {
+		rc = io.NopCloser(body)
 	}
+	u.Host = removeEmptyPort(u.Host)
+	// The host's colon:port should be normalized. See Issue 14836.
+	req := &http.Request{
+		Method:     http.MethodPost,
+		URL:        u,
+		Proto:      "HTTP/1.1",
+		ProtoMajor: 1,
+		ProtoMinor: 1,
+		Header:     make(http.Header),
+		Body:       rc,
+		Host:       u.Host,
+	}
+	req = req.WithContext(ctx)
+	if body != nil {
+		switch v := body.(type) {
+		case *bytes.Buffer:
+			req.ContentLength = int64(v.Len())
+			buf := v.Bytes()
+			req.GetBody = func() (io.ReadCloser, error) {
+				r := bytes.NewReader(buf)
+				return io.NopCloser(r), nil
+			}
+		case *bytes.Reader:
+			req.ContentLength = int64(v.Len())
+			snapshot := *v
+			req.GetBody = func() (io.ReadCloser, error) {
+				r := snapshot
+				return io.NopCloser(&r), nil
+			}
+		case *strings.Reader:
+			req.ContentLength = int64(v.Len())
+			snapshot := *v
+			req.GetBody = func() (io.ReadCloser, error) {
+				r := snapshot
+				return io.NopCloser(&r), nil
+			}
+		default:
+			// This is where we'd set it to -1 (at least
+			// if body != NoBody) to mean unknown, but
+			// that broke people during the Go 1.8 testing
+			// period. People depend on it being 0 I
+			// guess. Maybe retry later. See Issue 18117.
+		}
+		// For client requests, Request.ContentLength of 0
+		// means either actually 0, or unknown. The only way
+		// to explicitly say that the ContentLength is zero is
+		// to set the Body to nil. But turns out too much code
+		// depends on NewRequest returning a non-nil Body,
+		// so we use a well-known ReadCloser variable instead
+		// and have the http package also treat that sentinel
+		// variable to mean explicitly zero.
+		if req.GetBody != nil && req.ContentLength == 0 {
+			req.Body = http.NoBody
+			req.GetBody = func() (io.ReadCloser, error) { return http.NoBody, nil }
+		}
+	}
+
 	if c.newAuthToken != nil {
-		req.Header.Set("Authorization", "Bearer "+c.newAuthToken(req.URL.RawQuery))
+		req.Header.Set("Authorization", "Bearer "+c.newAuthToken(u.RawQuery))
 	}
 	req.Header.Set("X-Minio-Time", time.Now().UTC().Format(time.RFC3339))
 	if body != nil {
 		req.Header.Set("Expect", "100-continue")
 	}
+
+	return req, nil
+}
+
+// Call - make a REST call with context.
+func (c *Client) Call(ctx context.Context, method string, values url.Values, body io.Reader, length int64) (reply io.ReadCloser, err error) {
+	urlStr := c.url.String()
+	if !c.IsOnline() {
+		return nil, &NetworkError{c.LastError()}
+	}
+
+	u, err := url.Parse(urlStr)
+	if err != nil {
+		return nil, &NetworkError{Err: &url.Error{Op: method, URL: urlStr, Err: err}}
+	}
+
+	u.Path = path.Join(u.Path, method)
+	u.RawQuery = values.Encode()
+
+	req, err := c.newRequest(ctx, u, body)
+	if err != nil {
+		return nil, &NetworkError{err}
+	}
 	if length > 0 {
 		req.ContentLength = length
 	}
@@ -147,8 +239,8 @@ func (c *Client) Call(ctx context.Context, method string, values url.Values, bod
 			if !c.NoMetrics {
 				atomic.AddUint64(&networkErrsCounter, 1)
 			}
-			if c.MarkOffline() {
-				logger.LogIf(ctx, fmt.Errorf("Marking %s temporary offline; caused by %w", c.url.String(), err))
+			if c.MarkOffline(err) {
+				logger.LogOnceIf(ctx, fmt.Errorf("Marking %s offline temporarily; caused by %w", c.url.Host, err), c.url.Host)
 			}
 		}
 		return nil, &NetworkError{err}
@@ -171,8 +263,9 @@ func (c *Client) Call(ctx context.Context, method string, values url.Values, bod
 		// fully it should make sure to respond with '412'
 		// instead, see cmd/storage-rest-server.go for ideas.
 		if c.HealthCheckFn != nil && resp.StatusCode == http.StatusPreconditionFailed {
-			logger.LogIf(ctx, fmt.Errorf("Marking %s temporary offline; caused by PreconditionFailed with disk ID mismatch", c.url.String()))
-			c.MarkOffline()
+			err = fmt.Errorf("Marking %s offline temporarily; caused by PreconditionFailed with disk ID mismatch", c.url.Host)
+			logger.LogOnceIf(ctx, err, c.url.Host)
+			c.MarkOffline(err)
 		}
 		defer xhttp.DrainBody(resp.Body)
 		// Limit the ReadAll(), just in case, because of a bug, the server responds with large data.
@@ -182,8 +275,8 @@ func (c *Client) Call(ctx context.Context, method string, values url.Values, bod
 				if !c.NoMetrics {
 					atomic.AddUint64(&networkErrsCounter, 1)
 				}
-				if c.MarkOffline() {
-					logger.LogIf(ctx, fmt.Errorf("Marking %s temporary offline; caused by %w", c.url.String(), err))
+				if c.MarkOffline(err) {
+					logger.LogOnceIf(ctx, fmt.Errorf("Marking %s offline temporarily; caused by %w", c.url.Host, err), c.url.Host)
 				}
 			}
 			return nil, err
@@ -227,10 +320,20 @@ func (c *Client) LastConn() time.Time {
 	return time.Unix(0, atomic.LoadInt64(&c.lastConn))
 }
 
+// LastError returns previous error
+func (c *Client) LastError() error {
+	c.RLock()
+	defer c.RUnlock()
+	return c.lastErr
+}
+
 // MarkOffline - will mark a client as being offline and spawns
 // a goroutine that will attempt to reconnect if HealthCheckFn is set.
 // returns true if the node changed state from online to offline
-func (c *Client) MarkOffline() bool {
+func (c *Client) MarkOffline(err error) bool {
+	c.Lock()
+	c.lastErr = err
+	c.Unlock()
 	// Start goroutine that will attempt to reconnect.
 	// If server is already trying to reconnect this will have no effect.
 	if c.HealthCheckFn != nil && atomic.CompareAndSwapInt32(&c.connected, online, offline) {