Use stdlib TLS dialer - #36687

cpuguy83 · 2018-03-24T18:54:44Z

Since go1.8, the stdlib TLS net.Conn implementation implements the
CloseWrite() interface.

boaz0

LGTM 👍

boaz0 · 2018-03-25T21:30:55Z

Um. On powerpc the test failed:

19:48:00 --- FAIL: TestTLSCloseWriter (0.00s)
19:48:00 panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: socket: The requested service provider could not be loaded or initialized. [recovered]
19:48:00 	panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: socket: The requested service provider could not be loaded or initialized.
19:48:00 
19:48:00 goroutine 135 [running]:
19:48:00 testing.tRunner.func1(0xc042337860)
19:48:00 	C:/go/src/testing/testing.go:711 +0x2d9
19:48:00 panic(0x99e2a0, 0xc0424392d0)
19:48:00 	C:/go/src/runtime/panic.go:491 +0x291
19:48:00 net/http/httptest.newLocalListener(0x180000, 0x0)
19:48:00 	C:/go/src/net/http/httptest/server.go:66 +0x205
19:48:00 net/http/httptest.NewUnstartedServer(0xd8ac40, 0xc0424392a0, 0xc042336001)
19:48:00 	C:/go/src/net/http/httptest/server.go:94 +0x2d
19:48:00 net/http/httptest.NewTLSServer(0xd8ac40, 0xc0424392a0, 0xd8448f)
19:48:00 	C:/go/src/net/http/httptest/server.go:161 +0x3c
19:48:00 github.com/docker/docker/client.TestTLSCloseWriter(0xc042337860)
19:48:00 	C:/go/src/github.com/docker/docker/client/hijack_test.go:22 +0xbb
19:48:00 testing.tRunner(0xc042337860, 0xaac8a8)
19:48:00 	C:/go/src/testing/testing.go:746 +0xd7
19:48:00 created by testing.(*T).Run
19:48:00 	C:/go/src/testing/testing.go:789 +0x2e5

yongtang

LGTM

cpuguy83 · 2018-03-26T13:42:08Z

Really strange powerpc failed like that now it's working but windows is failing the same way.

cpuguy83 · 2018-03-26T14:05:23Z

I'm going to assume it's hitting nodes without ipv6 or something.
I'm going to update the test with a manually configured server I can force it to use ipv4.

codecov · 2018-03-26T14:23:18Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5cb95f6). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #36687   +/-   ##
=========================================
  Coverage          ?   35.03%           
=========================================
  Files             ?      608           
  Lines             ?    44965           
  Branches          ?        0           
=========================================
  Hits              ?    15754           
  Misses            ?    27106           
  Partials          ?     2105

cpuguy83 · 2018-03-26T14:36:37Z

hijack_test.go:27: assertion failed: error is not nil: listen tcp4 127.0.0.1:0: socket: The requested service provider could not be loaded or initialized.

😡

boaz0 · 2018-03-26T15:43:37Z

Super odd!

cpuguy83 · 2018-05-07T21:21:24Z

The one CI I want to run won't actually run. Just sitting there:

18:54:52 INFO: Docker info of control daemon
18:54:52 
18:54:52 Containers: 0
18:54:52  Running: 0
18:54:52  Paused: 0
18:54:52  Stopped: 0
18:54:52 Images: 7
18:54:52 Server Version: master-dockerproject-2018-03-18
18:54:52 Storage Driver: windowsfilter
18:54:52  Windows: 
18:54:52 Logging Driver: json-file
18:54:52 Plugins:
18:54:52  Volume: local
18:54:52  Network: ics l2bridge l2tunnel nat null overlay transparent
18:54:52  Log: awslogs etwlogs fluentd gelf json-file logentries splunk syslog
18:54:52 Swarm: inactive
18:54:52 Default Isolation: process
18:54:52 Kernel Version: 10.0 14393 (14393.2125.amd64fre.rs1_release.180301-2139)
18:54:52 Operating System: Windows Server 2016 Datacenter Version 1607 (OS Build 14393.2125)
18:54:52 OSType: windows
18:54:52 Architecture: x86_64
18:54:52 CPUs: 4
18:54:52 Total Memory: 14GiB
18:54:52 Name: jenkins-rs1-7
18:54:52 ID: SVRR:JK6X:JQU5:FHMB:27UY:JJ5F:7WTR:TBXC:J6OF:5X57:OWBA:QRFA
18:54:52 Docker Root Dir: D:\control
18:54:52 Debug Mode (client): false
18:54:52 Debug Mode (server): false
18:54:52 Registry: https://index.docker.io/v1/
18:54:52 Labels:
18:54:52 Experimental: false
18:54:52 Insecure Registries:
18:54:52  127.0.0.0/8
18:54:52 Live Restore Enabled: false
18:54:52 
18:54:52 
18:54:52 INFO: Commit hash is 0a9988b61
18:54:52 INFO: Nuke-Everything...
18:54:52 INFO: Container count on control daemon to delete is 0

thaJeztah

LGTM

cpuguy83 · 2018-05-08T16:20:05Z

Ok... I have no idea why Windows will not listen on a TCP port.
Is there some security policy involved? Should I just skip the test on Windows?

cpuguy83 · 2018-05-08T16:20:37Z

I've tried ports in different ranges, it's trying thousands of ports on each test run.

thaJeztah · 2018-05-08T18:04:36Z

ping @johnstep @salah-khan

johnstep · 2018-06-07T00:40:15Z

TestTLSCloseWriter fails on Windows when the following line is run first, which can happen even if TestNegotiateAPIVersionEmpty starts later because the failing test pauses and continues:
https://github.com/moby/moby/blob/7fb456cfa719c02b41926a29d9bc70b4a1e33abc/client/client_test.go#L197

$ go test .\client -v -run 'TLS'
=== RUN   TestTLSCloseWriter
=== PAUSE TestTLSCloseWriter
=== CONT  TestTLSCloseWriter
--- PASS: TestTLSCloseWriter (0.01s)
PASS
ok      github.com/docker/docker/client (cached)

$ go test .\client -v -run 'TLS|VersionEmpty'
=== RUN   TestNegotiateAPIVersionEmpty
--- PASS: TestNegotiateAPIVersionEmpty (0.00s)
=== RUN   TestTLSCloseWriter
=== PAUSE TestTLSCloseWriter
=== CONT  TestTLSCloseWriter
--- FAIL: TestTLSCloseWriter (2.17s)
        hijack_test.go:64: assertion failed: error is not nil: listen tcp4 127.0.0.1:9999: socket: The requested service provider could not be loaded or initialized.
FAIL
FAIL    github.com/docker/docker/client 2.236s

It also passes when all tests run except for TestNegotiateAPIVersionEmpty.

It looks like PatchAll should clean up the environment after the test. @dnephin @vdemeester, any idea why this environment stuff might be leading to issues in tests run later?

dnephin · 2018-06-07T02:28:08Z

Yes, it looks like there is a problem with that line. It should be deferring the return value of PatchAll. There's a missing () at the end of the line. Right now it's doing nothing for the test, then setting the environment after the test is done.

Does TestTLSCloseWriter depend on the value of DOCKER_API_VERSION ? I guess it should patch as well if an unexpected value can break the test.

johnstep · 2018-06-08T02:44:03Z

Thanks, that makes sense.

TestTLSCloseWriter does not depend on the value of DOCKER_API_VERSION but does require that the standard SystemRoot environment variables is set. On Windows, clearing the entire environment is almost always a bad idea as program code and system libraries use some of them.

johnstep · 2018-06-08T03:08:46Z

This should pass on Windows once #37236 is merged.

Since go1.8, the stdlib TLS net.Conn implementation implements the `CloseWrite()` interface. Signed-off-by: Brian Goff <cpuguy83@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2018-06-08T21:25:54Z

rebased to trigger CI

cpuguy83 requested a review from dnephin as a code owner March 24, 2018 18:54

cpuguy83 force-pushed the remove_custom_tls_dialer branch from 5421290 to 2ec7f9b Compare March 24, 2018 18:54

GordonTheTurtle added the status/0-triage label Mar 24, 2018

cpuguy83 force-pushed the remove_custom_tls_dialer branch 3 times, most recently from ee4763f to 7396048 Compare March 24, 2018 19:41

boaz0 added status/2-code-review and removed status/0-triage labels Mar 25, 2018

boaz0 approved these changes Mar 25, 2018

View reviewed changes

yongtang approved these changes Mar 25, 2018

View reviewed changes

yongtang added rebuild/windowsRS1 labels Mar 25, 2018

GordonTheTurtle removed rebuild/powerpc labels Mar 25, 2018

AkihiroSuda added the rebuild/windowsRS1 label Mar 26, 2018

GordonTheTurtle removed the rebuild/windowsRS1 label Mar 26, 2018

cpuguy83 force-pushed the remove_custom_tls_dialer branch from 7396048 to 5e82141 Compare March 26, 2018 14:23

cpuguy83 force-pushed the remove_custom_tls_dialer branch from 5e82141 to 45e70b6 Compare March 28, 2018 13:59

GordonTheTurtle assigned dnephin Apr 8, 2018

cpuguy83 force-pushed the remove_custom_tls_dialer branch from 45e70b6 to f63eed2 Compare April 26, 2018 14:23

cpuguy83 force-pushed the remove_custom_tls_dialer branch from f63eed2 to 0a9988b Compare May 7, 2018 18:53

cpuguy83 added the rebuild/windowsRS1 label May 7, 2018

GordonTheTurtle removed the rebuild/windowsRS1 label May 7, 2018

thaJeztah approved these changes May 7, 2018

View reviewed changes

thaJeztah added the rebuild/* label May 7, 2018

GordonTheTurtle removed the rebuild/* label May 7, 2018

cpuguy83 force-pushed the remove_custom_tls_dialer branch 3 times, most recently from af807c5 to 7fb456c Compare May 8, 2018 15:17

thaJeztah assigned johnstep and unassigned dnephin Jun 6, 2018

johnstep mentioned this pull request Jun 8, 2018

Clean up in TestNegotiateAPIVersionEmpty #37236

Merged

Use stdlib TLS dialer

2ac277a

Since go1.8, the stdlib TLS net.Conn implementation implements the `CloseWrite()` interface. Signed-off-by: Brian Goff <cpuguy83@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the remove_custom_tls_dialer branch from 7fb456c to 2ac277a Compare June 8, 2018 21:25

vdemeester added status/4-merge rebuild/powerpc and removed status/2-code-review labels Jun 9, 2018

GordonTheTurtle removed the rebuild/powerpc label Jun 9, 2018

vdemeester merged commit f5e60bb into moby:master Jun 9, 2018

Conversation

cpuguy83 commented Mar 24, 2018

Uh oh!

boaz0 left a comment

Choose a reason for hiding this comment

Uh oh!

boaz0 commented Mar 25, 2018

Uh oh!

yongtang left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Mar 26, 2018

Uh oh!

cpuguy83 commented Mar 26, 2018

Uh oh!

codecov Bot commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cpuguy83 commented Mar 26, 2018

Uh oh!

boaz0 commented Mar 26, 2018

Uh oh!

cpuguy83 commented May 7, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented May 8, 2018

Uh oh!

cpuguy83 commented May 8, 2018

Uh oh!

thaJeztah commented May 8, 2018

Uh oh!

johnstep commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Jun 7, 2018

Uh oh!

johnstep commented Jun 8, 2018

Uh oh!

johnstep commented Jun 8, 2018

Uh oh!

thaJeztah commented Jun 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov Bot commented Mar 26, 2018 •

edited

Loading

johnstep commented Jun 7, 2018 •

edited

Loading