Fix issues in SPDY RoundTripper #109651

ash2k · 2022-04-25T12:36:04Z

What type of PR is this?

/kind bug
/sig api-machinery

What this PR does / why we need it:

Feature: Pass context when doing TLS dials
Bugfix: Don't leak connection when an error happens after a successful TLS dial
Cleanup: Don't clone headers twice
Cleanup: No need for MultiReader

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2022-04-25T12:36:06Z

Please note that we're already in Test Freeze for the release-1.24 branch. This means every merged PR has to be cherry-picked into the release branch to be part of the upcoming v1.24.0 release.

ash2k · 2022-04-25T12:38:12Z

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

+	netD := s.Dialer
+	if netD == nil {
+		netD = &net.Dialer{
+			Timeout: 30 * time.Second,


This matches timeout used on line 222: https://github.com/kubernetes/kubernetes/pull/109651/files#diff-be99fb028cbdec52a7ee54d6b9a97c60a76f288124640e5f41a6aa96807fb8bbL222

Let's not set a timeout here. The socks proxy dialer is ~new, so copying its timeout to this old code path is not compelling... since we're now plumbing the context, inheriting any timeout associated with it seems sufficient to me.

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

ash2k · 2022-04-25T12:40:14Z

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

 	if err != nil {
 		return nil, err
 	}
+	defer func() {
+		if !success {
+			c.Close()


The connection is now always closed on error. It was leaked before (on error).

I'm not sure is leaked, it looks to me that it will be closed later

// Dial implements k8s.io/apimachinery/pkg/util/net.Dialer. func (s *SpdyRoundTripper) Dial(req *http.Request) (net.Conn, error) { conn, err := s.dial(req) if err != nil { return nil, err } if err := req.Write(conn); err != nil { conn.Close() return nil, err } return conn, nil }

No, it will not be - s.dial(req) returns an error and function returns.

oh, yeah, you are right, do you mind if we leave the connection leaking for other PR?
it seems a whole topic in its own and I can't see tests for those

hmm, I still think the conn will not leak

func (s *SpdyRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { header := utilnet.CloneHeader(req.Header) header.Add(httpstream.HeaderConnection, httpstream.HeaderUpgrade) header.Add(httpstream.HeaderUpgrade, HeaderSpdy31) var ( conn net.Conn rawResponse []byte err error ) clone := utilnet.CloneRequest(req) clone.Header = header conn, err = s.Dial(clone) if err != nil { return nil, err }

is not that it runs in a goroutine, once the RoundTrip exits it will be destroyed, no?

A connection is a kind of object that needs to get it's lifetime managed by the program. GC will collect the object, but I don't know if there is a finalizer to clean up the unreleased underlying OS resources. It's poor practice to rely on finalizers anyway. I don't understand what is your concern. Why not close the connection if it's not needed (because an error is returned)?

because this code is too old that there are a lot of assumptions that we don't know and we may break something without noticing, I'm sure you are right, but what I'm not sure is about the consequences of the changes, that is why I'm trying to be conservative ... is not the first time that for fixing something a bug arise in other place of the stack, and then is very hard to solve these bugs ... we can open a bug for the conns leaks and follow up later, I think that is not blocking your problem, and I can work on the test if you want

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

aojea · 2022-04-25T20:21:46Z

too many different changes in a single commit?

ash2k · 2022-04-26T00:30:38Z

@aojea They are all one the same lines of code, so it was easier to just clean it all up in a single pass.

Need to fix tests...

aojea · 2022-04-26T06:47:01Z

Feature: Pass context when doing TLS dials
Bugfix: Don't leak connection when an error happens after a successful TLS dial
Cleanup: Don't clone headers twice
Cleanup: No need for MultiReader

I personally think that each item should be an independent commit because it is easier to backport the changes or revert if required.

Also, I think that the new feature and the bugfix should have unit tests

fedebongio · 2022-04-26T20:14:17Z

/assign @aojea
(thank you Antonio!)
/triage accepted

ash2k · 2022-04-29T06:04:55Z

@aojea I've split it into 3 commits, should be cleaner now. PTAL.

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

aojea · 2022-04-29T10:51:42Z

LGTM if we remove the "connection leaking" code so we can tackle that problem in a different PR verifying the leaks with tests, :)

ash2k · 2022-04-29T11:09:36Z

LGTM if we remove the "connection leaking" code so we can tackle that problem in a different PR verifying the leaks with tests, :)

I'm sorry but I'm not going to come back to this. The only reason I'm looking at this code is because I want to fix #89163 and I was learning how things work currently. I don't want to polish this code to look perfectly, I want to make it obsolete and eventually delete it. It's clear that there is a leak and to test that I'd need to inject a stub dialer (to return a mock connection and then check that Close() has been called). Honestly, I don't think it's worth the effort. Maybe you have a better idea of how to test it?

aojea · 2022-04-29T13:15:40Z

LGTM if we remove the "connection leaking" code so we can tackle that problem in a different PR verifying the leaks with tests, :)

I'm sorry but I'm not going to come back to this. The only reason I'm looking at this code is because I want to fix #89163 and I was learning how things work currently. I don't want to polish this code to look perfectly, I want to make it obsolete and eventually delete it. It's clear that there is a leak and to test that I'd need to inject a stub dialer (to return a mock connection and then check that Close() has been called). Honestly, I don't think it's worth the effort. Maybe you have a better idea of how to test it?

@ash2k I will work on the tests, I think that your commits add value, give me some days to come back to you

Regarding websockets, this comment will be interesting for you #7452 (comment)

ash2k · 2022-04-30T08:40:39Z

👍

aojea · 2022-05-02T11:05:54Z

the tls.Dial already performs the handshake and closes the underline connection if it fails https://cs.opensource.google/go/go/+/refs/tags/go1.18.1:src/crypto/tls/tls.go;l=156-161;drc=860704317e02d699e4e4a24103853c4782d746c1

	rawConn, err := netDialer.DialContext(ctx, network, addr)
	if err != nil {
		return nil, err
	}

	colonPos := strings.LastIndex(addr, ":")
	if colonPos == -1 {
		colonPos = len(addr)
	}
	hostname := addr[:colonPos]

	if config == nil {
		config = defaultConfig()
	}
	// If no ServerName is set, infer the ServerName
	// from the hostname we're connecting to.
	if config.ServerName == "" {
		// Make a copy to avoid polluting argument or default.
		c := config.Clone()
		c.ServerName = hostname
		config = c
	}

	conn := Client(rawConn, config)
	if err := conn.HandshakeContext(ctx); err != nil {
		rawConn.Close()
		return nil, err
	}
	return conn, nil

Another thing is that the code that verify the hostname seems not needed anymore, https://groups.google.com/g/golang-nuts/c/4vnt7NdLvVU/m/b1SJ4u0ikb0J

You can play with the different combinations using https://gist.github.com/aojea/4e172d7f3835cd3ffc6ea4f83eea6608

I've filed a new PR for that adding you as co-author #109750

ash2k · 2022-05-02T11:30:09Z

I'll rebased once your PR is merged. Cheers.

CloneRequest() clones headers too

ash2k · 2022-05-07T12:41:40Z

@aojea Rebased, PTAL.

/remove-kind bug
/kind cleanup

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

aojea · 2022-05-07T13:29:32Z

/retest
Unrelated failure k8s.io/kubernetes/pkg/kubelet/cm/devicemanager

/Lgtm
/assign @liggitt

For approval

Thanks

liggitt · 2022-05-10T13:33:18Z

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go

+			require.NoError(t, err)
+			spdyTransport := NewRoundTripper(&tls.Config{})
+			_, err = spdyTransport.Dial(req)
+			assert.EqualError(t, err, "dial tcp 127.0.0.1:1233: operation was canceled")


is there a way to check the type of this error rather than the string?

Dialer returns a *net.OpError but it's a generic thing. Within it there is an Err field which is the original error. In Go 1.18.x it's NOT possible to check it, but in the yet unreleased 1.19 it will be possible via errors.Is(). See golang/go#51428 (commit golang/go@85b5f86). In any case, I think it's not that important as it's a test only. String comparison is probably good enough for a test.

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go

liggitt · 2022-05-10T13:42:54Z

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

+	netD := s.Dialer
+	if netD == nil {
+		netD = &net.Dialer{
+			Timeout: 30 * time.Second,


Let's not set a timeout here. The socks proxy dialer is ~new, so copying its timeout to this old code path is not compelling... since we're now plumbing the context, inheriting any timeout associated with it seems sufficient to me.

ash2k · 2022-05-11T06:38:39Z

/retest

liggitt · 2022-05-15T02:33:34Z

/lgtm
/approve

k8s-ci-robot · 2022-05-15T02:34:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, liggitt

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:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [liggitt]

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

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 25, 2022

k8s-ci-robot requested review from caesarxuchao and sttts April 25, 2022 12:36

ash2k commented Apr 25, 2022

View reviewed changes

k8s-ci-robot assigned aojea Apr 26, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 26, 2022

ash2k force-pushed the ash2k/spdy-cleanup branch from fb08dc0 to 34b7240 Compare April 29, 2022 01:15

ash2k force-pushed the ash2k/spdy-cleanup branch from 34b7240 to d925c9c Compare April 29, 2022 06:04

aojea reviewed Apr 29, 2022

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go Outdated Show resolved Hide resolved

aojea reviewed Apr 29, 2022

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2022

Don't clone headers twice

6d74474

CloneRequest() clones headers too

ash2k force-pushed the ash2k/spdy-cleanup branch from 2d8048c to 4c13b46 Compare May 7, 2022 12:40

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2022

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/bug Categorizes issue or PR as related to a bug. labels May 7, 2022

aojea reviewed May 7, 2022

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go Show resolved Hide resolved

k8s-ci-robot assigned liggitt May 7, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2022

liggitt reviewed May 10, 2022

View reviewed changes

liggitt mentioned this pull request May 10, 2022

Deprecate net.Dialer by a dial function for SpdyRoundTripper #109135

Closed

Pass context for TLS dial

1dda915

ash2k force-pushed the ash2k/spdy-cleanup branch from 4c13b46 to 1dda915 Compare May 11, 2022 05:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2022

ash2k requested a review from liggitt May 13, 2022 06:49

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2022

k8s-ci-robot merged commit dfee09a into kubernetes:master May 15, 2022

k8s-ci-robot added this to the v1.25 milestone May 15, 2022

ash2k deleted the ash2k/spdy-cleanup branch May 15, 2022 09:27

aojea mentioned this pull request Nov 18, 2022

Support cancelable SPDY connection establishment #107605

Closed

piepmatz mentioned this pull request Mar 9, 2023

Support cancelable SPDY connection establishment #116412

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues in SPDY RoundTripper #109651

Fix issues in SPDY RoundTripper #109651

ash2k commented Apr 25, 2022

k8s-ci-robot commented Apr 25, 2022

ash2k Apr 25, 2022

liggitt May 10, 2022

ash2k Apr 25, 2022

aojea Apr 29, 2022

ash2k Apr 29, 2022

aojea Apr 29, 2022

aojea Apr 29, 2022

ash2k Apr 29, 2022

aojea Apr 29, 2022

aojea commented Apr 25, 2022 •

edited

ash2k commented Apr 26, 2022

aojea commented Apr 26, 2022 •

edited

fedebongio commented Apr 26, 2022

ash2k commented Apr 29, 2022

aojea commented Apr 29, 2022

ash2k commented Apr 29, 2022

aojea commented Apr 29, 2022 •

edited

ash2k commented Apr 30, 2022

aojea commented May 2, 2022

ash2k commented May 2, 2022

ash2k commented May 7, 2022

aojea commented May 7, 2022

liggitt May 10, 2022

ash2k May 11, 2022

liggitt May 10, 2022

ash2k commented May 11, 2022

liggitt commented May 15, 2022

k8s-ci-robot commented May 15, 2022

Fix issues in SPDY RoundTripper #109651

Fix issues in SPDY RoundTripper #109651

Conversation

ash2k commented Apr 25, 2022

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Apr 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Apr 25, 2022 • edited

ash2k commented Apr 26, 2022

aojea commented Apr 26, 2022 • edited

fedebongio commented Apr 26, 2022

ash2k commented Apr 29, 2022

aojea commented Apr 29, 2022

ash2k commented Apr 29, 2022

aojea commented Apr 29, 2022 • edited

ash2k commented Apr 30, 2022

aojea commented May 2, 2022

ash2k commented May 2, 2022

ash2k commented May 7, 2022

aojea commented May 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k commented May 11, 2022

liggitt commented May 15, 2022

k8s-ci-robot commented May 15, 2022

aojea commented Apr 25, 2022 •

edited

aojea commented Apr 26, 2022 •

edited

aojea commented Apr 29, 2022 •

edited