Skip to content
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.

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

Use Dial with context #60012

Merged
merged 1 commit into from May 19, 2018

Conversation

@ash2k
Copy link
Member

ash2k commented Feb 17, 2018

What this PR does / why we need it:
net/http/Transport.Dial field is deprecated:

// DialContext specifies the dial function for creating unencrypted TCP connections.
// If DialContext is nil (and the deprecated Dial below is also nil),
// then the transport dials using package net.
DialContext func(ctx context.Context, network, addr string) (net.Conn, error)

// Dial specifies the dial function for creating unencrypted TCP connections.
//
// Deprecated: Use DialContext instead, which allows the transport
// to cancel dials as soon as they are no longer needed.
// If both are set, DialContext takes priority.
Dial func(network, addr string) (net.Conn, error)

This PR switches all Dial usages to DialContext. Fixes #63455.

Special notes for your reviewer:
Also related: #59287 #58532 #815 kubernetes/community#1166 #58677 #57932

Release note:

HTTP transport now uses `context.Context` to cancel dial operations. k8s.io/client-go/transport/Config struct has been updated to accept a function with a `context.Context` parameter. This is a breaking change if you use this field in your code.

/sig api-machinery
/kind enhancement
/cc @sttts

@@ -496,9 +498,15 @@ func TestProxyUpgradeErrorResponse(t *testing.T) {
expectedErr = errors.New("EXPECTED")
)
proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
transport := http.DefaultTransport.(*http.Transport)
transport.Dial = func(network, addr string) (net.Conn, error) {

This comment has been minimized.

@ash2k

ash2k Feb 17, 2018

Author Member

Global mutable state is a bad idea. Mutating it is an even worse idea. Some other test was failing because of that.

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented Feb 18, 2018

/assign @smarterclayton

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Feb 22, 2018

/assign @yliaog

@ash2k ash2k force-pushed the atlassian:dial-with-context branch 2 times, most recently from c8db9de to 3437da2 Feb 23, 2018

@@ -68,25 +70,25 @@ func TestDialURL(t *testing.T) {

"insecure, custom dial": {
TLSConfig: &tls.Config{InsecureSkipVerify: true},
Dial: net.Dial,
Dial: d.DialContext,

This comment has been minimized.

@sttts

sttts Feb 27, 2018

Contributor

nit: DialContext:

transport := http.DefaultTransport.(*http.Transport)
transport.Dial = func(network, addr string) (net.Conn, error) {
return &fakeConn{err: expectedErr}, nil
transport := &http.Transport{

This comment has been minimized.

@sttts

sttts Feb 27, 2018

Contributor

don't we have a Transport copier somewhere? I believe we do.

This comment has been minimized.

@ash2k

ash2k Feb 28, 2018

Author Member

The closest thing I found is utilnet.SetOldTransportDefaults() which is used above. It does not matter, it is just a test. Here I just copied the definition of the default http transport. Transport contains a mutex so it is not safe to copy the whole struct.

This comment has been minimized.

@sttts

sttts Feb 28, 2018

Contributor

true, for the test it does not matter

@@ -53,7 +54,7 @@ type Config struct {
WrapTransport func(rt http.RoundTripper) http.RoundTripper

// Dial specifies the dial function for creating unencrypted TCP connections.
Dial func(network, addr string) (net.Conn, error)
Dial func(ctx context.Context, network, address string) (net.Conn, error)

This comment has been minimized.

@sttts

sttts Feb 27, 2018

Contributor

intentionally left as Dial and not DialContext?

This comment has been minimized.

@ash2k

ash2k Feb 27, 2018

Author Member

Yes. It's our interface and I thought if we're making a breaking change anyway, lets not make the name ugly.

This comment has been minimized.

@sttts

sttts Feb 28, 2018

Contributor

Ok, fine with me

This comment has been minimized.

@yliaog

yliaog Apr 3, 2018

Contributor

could we make sure this breaking change is captured in the release note somehow? probably start with documenting it in this PR's release note section.

This comment has been minimized.

@ash2k

ash2k Apr 4, 2018

Author Member

Added a release note to the PR description.

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented Mar 27, 2018

Would be great to get this merged @sttts

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented Mar 30, 2018

/test pull-kubernetes-typecheck

if s.client == nil {
return nil, errors.New("tunnel is not opened.")
}
// Cannot use context here

This comment has been minimized.

@sttts

sttts Apr 3, 2018

Contributor

is this a todo?

This comment has been minimized.

@ash2k

ash2k Apr 3, 2018

Author Member

No, just a comment, pointing out the sad fact that the library used here (golang.org/x/crypto/ssh) does not allow to pass a context for dial.

This comment has been minimized.

@sttts

sttts Apr 3, 2018

Contributor

maybe be a bit more explicit in the comment.

@@ -86,7 +88,7 @@ func DialURL(url *url.URL, transport http.RoundTripper) (net.Conn, error) {
}

} else {
// Dial
// Dial. Cannot use ctx

This comment has been minimized.

@sttts

sttts Apr 3, 2018

Contributor

is this a todo?

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu May 8, 2018

Member

How about using tls.DialWithDialer(dialer *net.Dialer, network, addr string, config *Config), then should first construct a dialer with Deadline?

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Apr 3, 2018

@yliaog can you please take a look at this? This needs more eyes and it's already assigned for 6 weeks.

@yliaog

This comment has been minimized.

Copy link
Contributor

yliaog commented Apr 3, 2018

The current tests simply pass in Context, it would be nice to add checks to ensure Context functions correctly, i.e., after the switch, the transport could cancel Dials as soon as they are no longer needed.

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented Apr 4, 2018

@yliaog tests for which file/object are you talking about? I don't immediately see what I can easily test.

@yliaog

This comment has been minimized.

Copy link
Contributor

yliaog commented Apr 4, 2018

@ash2k the tests i'm thinking about is to test the 'functionality' of having context.Context added actually works as intended. It's nice to have. It's ok to merge without it.

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented Apr 5, 2018

@yliaog I don't feel it is worth the effort, sorry. Would like to get it merged though :)
/retest

@yliaog

This comment has been minimized.

Copy link
Contributor

yliaog commented Apr 5, 2018

@ash2k fine with me.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 5, 2018

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented May 8, 2018

For approval please @lavalamp @smarterclayton
This has been open for almost 3 months...

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented May 10, 2018

/approve

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented May 10, 2018

@yliaog LGTM please - lost because of rebase.

@ash2k ash2k force-pushed the atlassian:dial-with-context branch from 0f8fdcc to ebaed6e May 17, 2018

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented May 17, 2018

/retest

1 similar comment
@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented May 18, 2018

/retest

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented May 18, 2018

@yliaog PTAL

@yliaog

This comment has been minimized.

Copy link
Contributor

yliaog commented May 18, 2018

could you please squash the commits? lgtm otherwise.

@ash2k ash2k force-pushed the atlassian:dial-with-context branch from ebaed6e to 5e8e570 May 18, 2018

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented May 18, 2018

rebased + squashed

@ash2k

This comment has been minimized.

Copy link
Member Author

ash2k commented May 19, 2018

/retest

@yliaog

This comment has been minimized.

Copy link
Contributor

yliaog commented May 19, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 19, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 19, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, lavalamp, sttts, yliaog

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 19, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 19, 2018

@ash2k: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-node-e2e 5e8e570 link /test pull-kubernetes-node-e2e
pull-kubernetes-e2e-kops-aws 5e8e570 link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-e2e-gce 5e8e570 link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 19, 2018

Automatic merge from submit-queue (batch tested with PRs 60012, 63692, 63977, 63960, 64008). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit ddf551c into kubernetes:master May 19, 2018

13 of 18 checks passed

pull-kubernetes-e2e-gce Job failed.
Details
pull-kubernetes-e2e-kops-aws Job failed.
Details
pull-kubernetes-node-e2e Job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation ash2k authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@ash2k ash2k deleted the atlassian:dial-with-context branch May 19, 2018

k8s-github-robot pushed a commit that referenced this pull request Jun 28, 2018

Kubernetes Submit Queue
Merge pull request #65547 from liggitt/dial-util
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Honor custom transport dialer

#60012 updated API machinery code to use context dial functions by default, but we should still fall back to honor transport.Dial if set

* SetOldTransportDefaults should not use the default http DialContext if a custom Dial method is already set
* DialerFor should prefer DialContext, but fall back to returning a custom Dial if set before returning nil

```release-note
api-machinery utility functions `SetTransportDefaults` and `DialerFor` once again respect custom Dial functions set on transports
```

k8s-publishing-bot added a commit to kubernetes/apimachinery that referenced this pull request Jun 28, 2018

Merge pull request #65547 from liggitt/dial-util
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Honor custom transport dialer

kubernetes/kubernetes#60012 updated API machinery code to use context dial functions by default, but we should still fall back to honor transport.Dial if set

* SetOldTransportDefaults should not use the default http DialContext if a custom Dial method is already set
* DialerFor should prefer DialContext, but fall back to returning a custom Dial if set before returning nil

```release-note
api-machinery utility functions `SetTransportDefaults` and `DialerFor` once again respect custom Dial functions set on transports
```

Kubernetes-commit: ee2e11a0d43c1534932f00b4dac233369d64f522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.