Replace use of httputil in client hijack by dmcgowan · Pull Request #46920 · moby/moby

dmcgowan · 2023-12-10T06:21:48Z

addresses part of Fixme: SA1019: httputil.NewClientConn is deprecated #39926

Simplify the hijack process by just performing the http request/response on the connection and returning the raw conn after success. The client conn from httputil is deprecated and easily replaced.

Just a draft to see if the tests are ok with it.

thaJeztah · 2023-12-11T11:35:48Z

Thanks! Looks like this partially addresses Fixme: SA1019: httputil.NewClientConn is deprecated #39926

I haven't looked at the other uses of this; is it complicated to address those as well? If we could, we could remove these exceptions from the golangci-lint config;

moby/.golangci.yml

Lines 116 to 123 in 86cd6da

    
           # FIXME temporarily suppress these. See #39926 
        
           - text: "SA1019: httputil.NewClientConn" 
        
             linters: 
        
               - staticcheck 
        
           # FIXME temporarily suppress these (related to the ones above) 
        
           - text: "SA1019: httputil.ErrPersistEOF" 
        
             linters: 
        
               - staticcheck

thaJeztah · 2023-12-11T11:37:26Z

Failure is a flaky test;

Flaky test: TestLiveRestore flakyness #46508

=== Failed
=== FAIL: amd64.integration.daemon TestLiveRestore/volume_references/local_volume_with_mount_options (2.31s)
    daemon_test.go:581: assertion failed: error is not nil: Error response from daemon: remove test-live-restore-volume-references-local: volume has active mounts
        --- FAIL: TestLiveRestore/volume_references/local_volume_with_mount_options (2.31s)

=== FAIL: amd64.integration.daemon TestLiveRestore/volume_references (16.87s)
    --- FAIL: TestLiveRestore/volume_references (16.87s)

thaJeztah · 2023-12-11T11:40:22Z

oh; maybe the other bits were already addressed (see #39927) in;

integration-cli: Fix SA1019: httputil.ClientConn is deprecated #40074

thaJeztah · 2023-12-11T11:47:31Z

Okay, not entirely; the ErrPersistEOF is fixed by this PR, but the httputil.NewClientConn one still needs changes in integration/plugin/authz/;

integration/plugin/authz/authz_plugin_test.go:181:7: SA1019: httputil.NewClientConn has been deprecated since Go 1.0: Use the Client or Transport in package net/http instead. (staticcheck)
	c := httputil.NewClientConn(conn, nil)
	     ^
integration/plugin/authz/authz_plugin_test.go:476:12: SA1019: httputil.NewClientConn has been deprecated since Go 1.0: Use the Client or Transport in package net/http instead. (staticcheck)
	client := httputil.NewClientConn(conn, nil)
	          ^

Let me know if you want to address that last one in this PR as well, otherwise, we can already remove this one only;

moby/.golangci.yml

Lines 120 to 123 in 86cd6da

    
           # FIXME temporarily suppress these (related to the ones above) 
        
           - text: "SA1019: httputil.ErrPersistEOF" 
        
             linters: 
        
               - staticcheck

dmcgowan · 2023-12-11T14:03:18Z

@thaJeztah sure! I'll take a look. If others are comfortable with this approach I can complete this change. It is much simpler than the httputil.ClientConn implementation, but the complexity there is around handling multiple requests/responses, which we do not need for the upgrade/hijack flow.

dmcgowan · 2023-12-11T23:05:42Z

Updated with the lint exceptions removed and authz test updated.

corhere · 2023-12-12T00:54:31Z


 	// Server hijacks the connection, error 'connection closed' expected
-	resp, err := clientconn.Do(req)
+	resp, err := client.Do(req)


The workflow with httputil.ClientConn was (simplified): construct a ClientConn, round-trip one HTTP request, then take its net.Conn. Looking at the implementation, (*httputil.ClientConn).Do() looks uncannily similar to (*hijackedConn).RoundTrip(). And when you ignore the pipelining cruft in the httputil code (waiting until the last request has been fully written before writing the next one to the conn) which does not come into play when only a single round-trip is made with the instance, they are functionally indistinguishable.

http.Client does a lot of things above and beyond httputil.ClientConn. For instance, it handles 3xx redirect responses by internally making a follow-up request to the new location. For this to work as expected, the http.RoundTripper assigned to the client's Transport field has to adhere to the interface contract, particularly the ability to make independent requests to different servers depending on the request URL. I think it's safe to say that things would not go well if the HTTP client tried to follow a redirect using hijackedConn as the RoundTripper.

Suggested change

resp, err := client.Do(req)

resp, err := hc.RoundTrip(req)

As a bonus, it would then be possible to wrap hc in an otelhttp.Transport for injecting the span context into the request headers.

thaJeztah · 2023-12-13T09:22:26Z

@dmcgowan while working on this, perhaps you have time to look at these as well;

dmcgowan · 2023-12-14T00:47:26Z

@thaJeztah that code has changed and Brian added a case for the tls config when the otel change was put in. The original issue may no longer exist.

@corhere updated to just use RoundTrip, agreed it is better to just directly use since there is no extra logic we want in handling the request and response.

thaJeztah

some minor comments in the second commit, but looks good otherwise 😅

thaJeztah · 2023-12-18T20:39:04Z

-	req = req.WithContext(ctx)
-	resp, err := c.Do(req)
+	req.URL.Scheme = "http"
+	req.URL.Host = daemonURL.Path


So we need to use client.DummyHost here instead? (see #45942)

Suggested change

req.URL.Host = daemonURL.Path

req.URL.Host = client.DummyHost

I don't think our integration tests need to be compatible with Go 1.20.6, 1.20.7 or 1.21.0. It was a regression, and has been fixed since Go 1.20.8, 1.21.1. We don't have to work around it any longer.

DummyHost is maybe clearer, at least it would be much better for constructing a full URL. Without setting any of this it fails and setting the Host to path is still weird with the slashes encoded.

Simplify the hijack process by just performing the http request/response on the connection and returning the raw conn after success. The client conn from httputil is deprecated and easily replaced. Signed-off-by: Derek McGowan <[email protected]>

Signed-off-by: Derek McGowan <[email protected]>

dmcgowan · 2023-12-18T21:41:34Z

@thaJeztah updated the authz test to use DummyHost

thaJeztah

LGTM

dmcgowan mentioned this pull request Dec 10, 2023

vendor: OTEL v1.19.0 / v0.45.0, containerd v1.7.11 #46830

Merged

3 tasks

thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Dec 11, 2023

thaJeztah added this to the 25.0.0 milestone Dec 11, 2023

dmcgowan marked this pull request as ready for review December 11, 2023 23:08

corhere reviewed Dec 12, 2023

View reviewed changes

thaJeztah assigned dmcgowan Dec 12, 2023

dmcgowan force-pushed the client-hijack-cleanup branch from ea5e142 to 2543bf0 Compare December 14, 2023 00:44

corhere approved these changes Dec 14, 2023

View reviewed changes

Comment thread client/hijack.go Outdated

dmcgowan force-pushed the client-hijack-cleanup branch from 2543bf0 to 365c43d Compare December 14, 2023 00:49

thaJeztah requested changes Dec 18, 2023

View reviewed changes

dmcgowan added 4 commits December 18, 2023 13:16

Update authz plugin test to not use httputil

35a51fd

Signed-off-by: Derek McGowan <[email protected]>

Remove httputil lint exceptions

c84e889

Signed-off-by: Derek McGowan <[email protected]>

Update OTEL in client hijack to use transport

eb9ce77

Signed-off-by: Derek McGowan <[email protected]>

dmcgowan force-pushed the client-hijack-cleanup branch from 365c43d to eb9ce77 Compare December 18, 2023 21:26

thaJeztah approved these changes Dec 18, 2023

View reviewed changes

thaJeztah merged commit 0751141 into moby:master Dec 18, 2023

	req.URL.Host = daemonURL.Path
	req.URL.Host = client.DummyHost

Conversation

dmcgowan commented Dec 10, 2023 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Dec 11, 2023

Uh oh!

thaJeztah commented Dec 11, 2023

Uh oh!

thaJeztah commented Dec 11, 2023

Uh oh!

thaJeztah commented Dec 11, 2023

Uh oh!

dmcgowan commented Dec 11, 2023

Uh oh!

dmcgowan commented Dec 11, 2023

Uh oh!

corhere Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 13, 2023

Uh oh!

Uh oh!

dmcgowan commented Dec 14, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

corhere Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

dmcgowan Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dmcgowan commented Dec 18, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmcgowan commented Dec 10, 2023 •

edited by thaJeztah

Loading