Browse code

integration-cli: Fix `SA1019: httputil.ClientConn is deprecated`

Rewrite sockRequestHijack to requestHijack which use writable
Transport's Response.Body to replace deprecated hijacked httputil.ClientConn.
```
// As of Go 1.12, the Body will also implement io.Writer
// on a successful "101 Switching Protocols" response,
// as used by WebSockets and HTTP/2's "h2c" mode.
Body io.ReadCloser
```.

TestPostContainersAttach and TestExecResizeImmediatelyAfterExecStart
replace all sockRequestHijack to requestHijack.

Signed-off-by: HuanHuan Ye <logindaveye@gmail.com>

HuanHuan Ye authored on 2019/10/11 18:54:35
Showing 3 changed files
... ...
@@ -86,10 +86,6 @@ issues:
86 86
     - text: "SA1019: httputil.NewClientConn is deprecated"
87 87
       linters:
88 88
         - staticcheck
89
-    # FIXME temporarily suppress these. See #39927
90
-    - text: "SA1019: httputil.ClientConn is deprecated"
91
-      linters:
92
-        - staticcheck
93 89
     # FIXME temporarily suppress these (related to the ones above)
94 90
     - text: "SA1019: httputil.ErrPersistEOF is deprecated"
95 91
       linters:
... ...
@@ -4,11 +4,9 @@ import (
4 4
 	"bufio"
5 5
 	"bytes"
6 6
 	"context"
7
-	"fmt"
8 7
 	"io"
9 8
 	"net"
10 9
 	"net/http"
11
-	"net/http/httputil"
12 10
 	"strings"
13 11
 	"testing"
14 12
 	"time"
... ...
@@ -17,6 +15,7 @@ import (
17 17
 	"github.com/docker/docker/client"
18 18
 	"github.com/docker/docker/pkg/stdcopy"
19 19
 	"github.com/docker/docker/testutil/request"
20
+	"github.com/docker/go-connections/sockets"
20 21
 	"github.com/pkg/errors"
21 22
 	"golang.org/x/net/websocket"
22 23
 	"gotest.tools/assert"
... ...
@@ -100,19 +99,18 @@ func (s *DockerSuite) TestGetContainersWsAttachContainerNotFound(c *testing.T) {
100 100
 func (s *DockerSuite) TestPostContainersAttach(c *testing.T) {
101 101
 	testRequires(c, DaemonIsLinux)
102 102
 
103
-	expectSuccess := func(conn net.Conn, br *bufio.Reader, stream string, tty bool) {
104
-		defer conn.Close()
103
+	expectSuccess := func(wc io.WriteCloser, br *bufio.Reader, stream string, tty bool) {
104
+		defer wc.Close()
105 105
 		expected := []byte("success")
106
-		_, err := conn.Write(expected)
106
+		_, err := wc.Write(expected)
107 107
 		assert.NilError(c, err)
108 108
 
109
-		conn.SetReadDeadline(time.Now().Add(time.Second))
110 109
 		lenHeader := 0
111 110
 		if !tty {
112 111
 			lenHeader = 8
113 112
 		}
114 113
 		actual := make([]byte, len(expected)+lenHeader)
115
-		_, err = io.ReadFull(br, actual)
114
+		_, err = readTimeout(br, actual, time.Second)
116 115
 		assert.NilError(c, err)
117 116
 		if !tty {
118 117
 			fdMap := map[string]byte{
... ...
@@ -125,57 +123,56 @@ func (s *DockerSuite) TestPostContainersAttach(c *testing.T) {
125 125
 		assert.Assert(c, is.DeepEqual(actual[lenHeader:], expected), "Attach didn't return the expected data from %s", stream)
126 126
 	}
127 127
 
128
-	expectTimeout := func(conn net.Conn, br *bufio.Reader, stream string) {
129
-		defer conn.Close()
130
-		_, err := conn.Write([]byte{'t'})
128
+	expectTimeout := func(wc io.WriteCloser, br *bufio.Reader, stream string) {
129
+		defer wc.Close()
130
+		_, err := wc.Write([]byte{'t'})
131 131
 		assert.NilError(c, err)
132 132
 
133
-		conn.SetReadDeadline(time.Now().Add(time.Second))
134 133
 		actual := make([]byte, 1)
135
-		_, err = io.ReadFull(br, actual)
136
-		opErr, ok := err.(*net.OpError)
137
-		assert.Assert(c, ok, "Error is expected to be *net.OpError, got %v", err)
138
-		assert.Assert(c, opErr.Timeout(), "Read from %s is expected to timeout", stream)
134
+		_, err = readTimeout(br, actual, time.Second)
135
+		assert.Assert(c, err.Error() == "Timeout", "Read from %s is expected to timeout", stream)
139 136
 	}
140 137
 
141 138
 	// Create a container that only emits stdout.
142 139
 	cid, _ := dockerCmd(c, "run", "-di", "busybox", "cat")
143 140
 	cid = strings.TrimSpace(cid)
141
+
144 142
 	// Attach to the container's stdout stream.
145
-	conn, br, err := sockRequestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stdout=1", nil, "text/plain", request.DaemonHost())
143
+	wc, br, err := requestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stdout=1", nil, "text/plain", request.DaemonHost())
146 144
 	assert.NilError(c, err)
147 145
 	// Check if the data from stdout can be received.
148
-	expectSuccess(conn, br, "stdout", false)
146
+	expectSuccess(wc, br, "stdout", false)
147
+
149 148
 	// Attach to the container's stderr stream.
150
-	conn, br, err = sockRequestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stderr=1", nil, "text/plain", request.DaemonHost())
149
+	wc, br, err = requestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stderr=1", nil, "text/plain", request.DaemonHost())
151 150
 	assert.NilError(c, err)
152 151
 	// Since the container only emits stdout, attaching to stderr should return nothing.
153
-	expectTimeout(conn, br, "stdout")
152
+	expectTimeout(wc, br, "stdout")
154 153
 
155 154
 	// Test the similar functions of the stderr stream.
156 155
 	cid, _ = dockerCmd(c, "run", "-di", "busybox", "/bin/sh", "-c", "cat >&2")
157 156
 	cid = strings.TrimSpace(cid)
158
-	conn, br, err = sockRequestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stderr=1", nil, "text/plain", request.DaemonHost())
157
+	wc, br, err = requestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stderr=1", nil, "text/plain", request.DaemonHost())
159 158
 	assert.NilError(c, err)
160
-	expectSuccess(conn, br, "stderr", false)
161
-	conn, br, err = sockRequestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stdout=1", nil, "text/plain", request.DaemonHost())
159
+	expectSuccess(wc, br, "stderr", false)
160
+	wc, br, err = requestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stdout=1", nil, "text/plain", request.DaemonHost())
162 161
 	assert.NilError(c, err)
163
-	expectTimeout(conn, br, "stderr")
162
+	expectTimeout(wc, br, "stderr")
164 163
 
165 164
 	// Test with tty.
166 165
 	cid, _ = dockerCmd(c, "run", "-dit", "busybox", "/bin/sh", "-c", "cat >&2")
167 166
 	cid = strings.TrimSpace(cid)
168 167
 	// Attach to stdout only.
169
-	conn, br, err = sockRequestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stdout=1", nil, "text/plain", request.DaemonHost())
168
+	wc, br, err = requestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stdout=1", nil, "text/plain", request.DaemonHost())
170 169
 	assert.NilError(c, err)
171
-	expectSuccess(conn, br, "stdout", true)
170
+	expectSuccess(wc, br, "stdout", true)
172 171
 
173 172
 	// Attach without stdout stream.
174
-	conn, br, err = sockRequestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stderr=1", nil, "text/plain", request.DaemonHost())
173
+	wc, br, err = requestHijack(http.MethodPost, "/containers/"+cid+"/attach?stream=1&stdin=1&stderr=1", nil, "text/plain", request.DaemonHost())
175 174
 	assert.NilError(c, err)
176 175
 	// Nothing should be received because both the stdout and stderr of the container will be
177 176
 	// sent to the client as stdout when tty is enabled.
178
-	expectTimeout(conn, br, "stdout")
177
+	expectTimeout(wc, br, "stdout")
179 178
 
180 179
 	// Test the client API
181 180
 	client, err := client.NewClientWithOpts(client.FromEnv)
... ...
@@ -220,35 +217,22 @@ func (s *DockerSuite) TestPostContainersAttach(c *testing.T) {
220 220
 	assert.Equal(c, outBuf.String(), "hello\nsuccess")
221 221
 }
222 222
 
223
-// SockRequestHijack creates a connection to specified host (with method, contenttype, …) and returns a hijacked connection
224
-// and the output as a `bufio.Reader`
225
-func sockRequestHijack(method, endpoint string, data io.Reader, ct string, daemon string, modifiers ...func(*http.Request)) (net.Conn, *bufio.Reader, error) {
226
-	req, client, err := newRequestClient(method, endpoint, data, ct, daemon, modifiers...)
227
-	if err != nil {
228
-		return nil, nil, err
229
-	}
223
+// requestHijack create a http requst to specified host with `Upgrade` header (with method
224
+// , contenttype, …), if receive a successful "101 Switching Protocols" response return
225
+// a `io.WriteCloser` and `bufio.Reader`
226
+func requestHijack(method, endpoint string, data io.Reader, ct, daemon string, modifiers ...func(*http.Request)) (io.WriteCloser, *bufio.Reader, error) {
230 227
 
231
-	client.Do(req)
232
-	conn, br := client.Hijack()
233
-	return conn, br, nil
234
-}
235
-
236
-// FIXME(vdemeester) httputil.ClientConn is deprecated, use http.Client instead (closer to actual client)
237
-// Deprecated: Use New instead of NewRequestClient
238
-// Deprecated: use request.Do (or Get, Delete, Post) instead
239
-func newRequestClient(method, endpoint string, data io.Reader, ct, daemon string, modifiers ...func(*http.Request)) (*http.Request, *httputil.ClientConn, error) {
240
-	c, err := request.SockConn(10*time.Second, daemon)
228
+	hostURL, err := client.ParseHostURL(daemon)
241 229
 	if err != nil {
242
-		return nil, nil, fmt.Errorf("could not dial docker daemon: %v", err)
230
+		return nil, nil, errors.Wrap(err, "parse daemon host error")
243 231
 	}
244 232
 
245
-	client := httputil.NewClientConn(c, nil)
246
-
247 233
 	req, err := http.NewRequest(method, endpoint, data)
248 234
 	if err != nil {
249
-		client.Close()
250
-		return nil, nil, fmt.Errorf("could not create new request: %v", err)
235
+		return nil, nil, errors.Wrap(err, "could not create new request")
251 236
 	}
237
+	req.URL.Scheme = "http"
238
+	req.URL.Host = hostURL.Host
252 239
 
253 240
 	for _, opt := range modifiers {
254 241
 		opt(req)
... ...
@@ -257,5 +241,52 @@ func newRequestClient(method, endpoint string, data io.Reader, ct, daemon string
257 257
 	if ct != "" {
258 258
 		req.Header.Set("Content-Type", ct)
259 259
 	}
260
-	return req, client, nil
260
+
261
+	// must have Upgrade header
262
+	// server api return 101 Switching Protocols
263
+	req.Header.Set("Upgrade", "tcp")
264
+
265
+	// new client
266
+	// FIXME use testutil/request newHTTPClient
267
+	transport := &http.Transport{}
268
+	err = sockets.ConfigureTransport(transport, hostURL.Scheme, hostURL.Host)
269
+	if err != nil {
270
+		return nil, nil, errors.Wrap(err, "configure Transport error")
271
+	}
272
+
273
+	client := http.Client{
274
+		Transport: transport,
275
+	}
276
+
277
+	resp, err := client.Do(req)
278
+	if err != nil {
279
+		return nil, nil, errors.Wrap(err, "client.Do")
280
+	}
281
+
282
+	if !bodyIsWritable(resp) {
283
+		return nil, nil, errors.New("response.Body not writable")
284
+	}
285
+
286
+	return resp.Body.(io.WriteCloser), bufio.NewReader(resp.Body), nil
287
+}
288
+
289
+// bodyIsWritable check Response.Body is writable
290
+func bodyIsWritable(r *http.Response) bool {
291
+	_, ok := r.Body.(io.Writer)
292
+	return ok
293
+}
294
+
295
+// readTimeout read from io.Reader with timeout
296
+func readTimeout(r io.Reader, buf []byte, timeout time.Duration) (n int, err error) {
297
+	ch := make(chan bool)
298
+	go func() {
299
+		n, err = io.ReadFull(r, buf)
300
+		ch <- true
301
+	}()
302
+	select {
303
+	case <-ch:
304
+		return
305
+	case <-time.After(timeout):
306
+		return 0, errors.New("Timeout")
307
+	}
261 308
 }
... ...
@@ -65,11 +65,11 @@ func (s *DockerSuite) TestExecResizeImmediatelyAfterExecStart(c *testing.T) {
65 65
 		}
66 66
 
67 67
 		payload := bytes.NewBufferString(`{"Tty":true}`)
68
-		conn, _, err := sockRequestHijack(http.MethodPost, fmt.Sprintf("/exec/%s/start", execID), payload, "application/json", request.DaemonHost())
68
+		wc, _, err := requestHijack(http.MethodPost, fmt.Sprintf("/exec/%s/start", execID), payload, "application/json", request.DaemonHost())
69 69
 		if err != nil {
70 70
 			return errors.Wrap(err, "failed to start the exec")
71 71
 		}
72
-		defer conn.Close()
72
+		defer wc.Close()
73 73
 
74 74
 		_, rc, err := request.Post(fmt.Sprintf("/exec/%s/resize?h=24&w=80", execID), request.ContentType("text/plain"))
75 75
 		if err != nil {