Skip to content

Bump go-connections for TLS 1.3 support #41042

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Bump go-connections for TLS 1.3 support #41042

wants to merge 1 commit into from

Conversation

@SamWhited
Copy link
Contributor

@SamWhited SamWhited commented May 28, 2020

See #40495

@SamWhited SamWhited requested a review from thaJeztah May 28, 2020 18:56
cpuguy83
cpuguy83 approved these changes May 28, 2020
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah added this to the 20.03.0 milestone Jun 3, 2020
thaJeztah
thaJeztah approved these changes Jun 3, 2020
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

left one suggestion; @SamWhited let me know if you think that's a good idea; not a blocker, so otherwise "good to go"

@thaJeztah thaJeztah mentioned this pull request Jun 3, 2020
Closed
4 tasks
@SamWhited
Copy link
Contributor Author

SamWhited commented Jun 3, 2020

@thaJeztah good idea, merged.

thaJeztah
thaJeztah approved these changes Jun 3, 2020
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

we'll revendor once v0.5.0 was tagged, but don't think we have to wait for that 👍

@thaJeztah
Copy link
Member

thaJeztah commented Jun 4, 2020

Why is Windows RS5 failing? Weird;

Failed
Stacktrace
failed to get info from daemon: Error response from daemon: client version 1.41 is too new. Maximum supported API version is 1.40
exit status 1
FAIL	github.com/docker/docker/integration/build	0.219s

@SamWhited
Copy link
Contributor Author

SamWhited commented Jun 4, 2020

Pushed a new version with the fixes @tiborvass just merged. I feel like I've seen that RS5 error a few times now, but I don't remember why. Let's see if it happens again when this CI run completes.

@thaJeztah
Copy link
Member

thaJeztah commented Jun 4, 2020

Yes, so somehow it seems to be running tests against an older daemon, but with a current version of the CLI (which is.. odd), and not performing API version negotiation. Definitely need to keep an eye on it.

Also on this PR to make sure it's not a regression in this package (I was about to tag v0.5.0, but then thought; let's first get this merged and tag later)

@thaJeztah
Copy link
Member

thaJeztah commented Jun 9, 2020

Looks like it's still failing on Windows. The failure originates from this part of the code;

moby/testutil/environment/environment.go

Lines 47 to 51 in 5bf33ad

func FromClient(c *client.Client) (*Execution, error) {
info, err := c.Info(context.Background())
if err != nil {
return nil, errors.Wrapf(err, "failed to get info from daemon")
}

The client there is setup using this code

moby/testutil/environment/environment.go

Lines 36 to 39 in 5bf33ad

// New creates a new Execution struct
// This is configured using the env client (see client.FromEnv)
func New() (*Execution, error) {
c, err := client.NewClientWithOpts(client.FromEnv)

It looks like API version negotiation isn't working (but only on Windows?), or is it because the integration tests on Windows are running on the host?

@thaJeztah thaJeztah mentioned this pull request Jun 9, 2020
Merged
@thaJeztah
Copy link
Member

thaJeztah commented Jun 9, 2020

giving #41084 a test (still not sure why this worked before, and not in this PR)

@thaJeztah
Copy link
Member

thaJeztah commented Jun 9, 2020

@SamWhited @AkihiroSuda did some more digging in #41084 (see comments I left there)

@thaJeztah thaJeztah modified the milestones: 20.10.0, 20.10.1, 20.10.2 Dec 9, 2020
@thaJeztah thaJeztah mentioned this pull request Jan 4, 2021
Closed
@thaJeztah thaJeztah modified the milestones: 20.10.2, 20.10.3 Jan 5, 2021
@thaJeztah thaJeztah removed this from the 20.10.3 milestone Feb 2, 2021
@thaJeztah thaJeztah modified the milestones: 20.10.4, 21.xx Feb 2, 2021
thaJeztah added a commit to thaJeztah/go-connections that referenced this pull request Jul 22, 2021
@thaJeztah
socket: restore deprecated Transport.Dial (for now)
f536900 
This restores the deprecated Transport.Dial, which were removed in commits:

- 61039d0 (Replace deprecated Transport.Dial with Transport.DialContext)
- fb772cf (Fix problems introduced by 61039d0)

While we should still look at removing these, the moby code currently looks to
be depending on their behavior. Removing them caused CI to fail, which blocked
us from updating to the current version of this package.

With those changes, Windows clients in CI were connecting with the wrong daemon,
causing CI failures:

    Failed
    failed to get info from daemon: Error response from daemon: client version 1.41 is too new. Maximum supported API version is 1.40
    exit status 1
    FAIL	github.com/docker/docker/integration/build	0.219s

More details on moby/moby#41042 and moby/moby#41084

This patch restores the deprecated parts, but keeps the new variants as well,
so that we can perform the migration in Moby when possible (after which they
can be removed again)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/go-connections that referenced this pull request Jul 22, 2021
@thaJeztah
socket: restore deprecated Transport.Dial (for now)
cb4314b 
This restores the deprecated Transport.Dial, which were removed in commits:

- 61039d0 (Replace deprecated Transport.Dial with Transport.DialContext)
- fb772cf (Fix problems introduced by 61039d0)

While we should still look at removing these, the moby code currently looks to
be depending on their behavior. Removing them caused CI to fail, which blocked
us from updating to the current version of this package.

With those changes, Windows clients in CI were connecting with the wrong daemon,
causing CI failures:

    Failed
    failed to get info from daemon: Error response from daemon: client version 1.41 is too new. Maximum supported API version is 1.40
    exit status 1
    FAIL	github.com/docker/docker/integration/build	0.219s

More details on moby/moby#41042 and moby/moby#41084

This patch restores the deprecated parts, but keeps the new variants as well,
so that we can perform the migration in Moby when possible (after which they
can be removed again)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah mentioned this pull request Jul 22, 2021
Draft
thaJeztah added a commit to thaJeztah/go-connections that referenced this pull request Jul 22, 2021
@thaJeztah
socket: restore deprecated Transport.Dial (for now)
548eb07 
This restores the deprecated Transport.Dial, which were removed in commits:

- 61039d0 (Replace deprecated Transport.Dial with Transport.DialContext)
- fb772cf (Fix problems introduced by 61039d0)

While we should still look at removing these, the moby code currently looks to
be depending on their behavior. Removing them caused CI to fail, which blocked
us from updating to the current version of this package.

With those changes, Windows clients in CI were connecting with the wrong daemon,
causing CI failures:

    Failed
    failed to get info from daemon: Error response from daemon: client version 1.41 is too new. Maximum supported API version is 1.40
    exit status 1
    FAIL	github.com/docker/docker/integration/build	0.219s

More details on moby/moby#41042 and moby/moby#41084

This patch restores the deprecated parts, but keeps the new variants as well,
so that we can perform the migration in Moby when possible (after which they
can be removed again)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/go-connections that referenced this pull request Jul 22, 2021
@thaJeztah
socket: restore deprecated Transport.Dial (for now)
a80c254 
This restores the deprecated Transport.Dial, which were removed in commits:

- 61039d0 (Replace deprecated Transport.Dial with Transport.DialContext)
- fb772cf (Fix problems introduced by 61039d0)

While we should still look at removing these, the moby code currently looks to
be depending on their behavior. Removing them caused CI to fail, which blocked
us from updating to the current version of this package.

With those changes, Windows clients in CI were connecting with the wrong daemon,
causing CI failures:

    Failed
    failed to get info from daemon: Error response from daemon: client version 1.41 is too new. Maximum supported API version is 1.40
    exit status 1
    FAIL	github.com/docker/docker/integration/build	0.219s

More details on moby/moby#41042 and moby/moby#41084

This patch restores the deprecated parts, but keeps the new variants as well,
so that we can perform the migration in Moby when possible (after which they
can be removed again)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/go-connections that referenced this pull request Jul 22, 2021
@thaJeztah
socket: restore deprecated Transport.Dial (for now)
e74611c 
This restores the deprecated Transport.Dial, which were removed in commits:

- 61039d0 (Replace deprecated Transport.Dial with Transport.DialContext)
- fb772cf (Fix problems introduced by 61039d0)

While we should still look at removing these, the moby code currently looks to
be depending on their behavior. Removing them caused CI to fail, which blocked
us from updating to the current version of this package.

With those changes, Windows clients in CI were connecting with the wrong daemon,
causing CI failures:

    Failed
    failed to get info from daemon: Error response from daemon: client version 1.41 is too new. Maximum supported API version is 1.40
    exit status 1
    FAIL	github.com/docker/docker/integration/build	0.219s

More details on moby/moby#41042 and moby/moby#41084

This patch restores the deprecated parts, but keeps the new variants as well,
so that we can perform the migration in Moby when possible (after which they
can be removed again)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/go-connections that referenced this pull request Jul 26, 2021
@thaJeztah
socket: restore deprecated Transport.Dial (for now)
b353ece 
This restores the deprecated Transport.Dial, which were removed in commits:

- 61039d0 (Replace deprecated Transport.Dial with Transport.DialContext)
- fb772cf (Fix problems introduced by 61039d0)

While we should still look at removing these, the moby code currently looks to
be depending on their behavior. Removing them caused CI to fail, which blocked
us from updating to the current version of this package.

With those changes, Windows clients in CI were connecting with the wrong daemon,
causing CI failures:

    Failed
    failed to get info from daemon: Error response from daemon: client version 1.41 is too new. Maximum supported API version is 1.40
    exit status 1
    FAIL	github.com/docker/docker/integration/build	0.219s

More details on moby/moby#41042 and moby/moby#41084

This patch restores the deprecated parts, but keeps the new variants as well,
so that we can perform the migration in Moby when possible (after which they
can be removed again)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah mentioned this pull request Jan 19, 2022
Closed
@thaJeztah thaJeztah modified the milestones: 22.06.0, v-next Aug 18, 2022
@rumpl rumpl modified the milestones: 24.0.0, v-future Apr 24, 2023
@thaJeztah
Copy link
Member

thaJeztah commented Dec 13, 2023

@thaJeztah thaJeztah removed this from the v-future milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SamWhited @thaJeztah @cpuguy83 @rumpl