spdy: add optional periodic Pings on the connection #94170

awly · 2020-08-21T21:53:29Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

When an SPDY connection goes over an intermediate box (proxy or load
balancer, e.g. AWS ELB), it can get interrupted due to idleness (default
in ELB is 60s). For example, this happens with kubectl exec sessions
are left open without any activity for a while.

TCP-level keep-alives are not sufficient for all intermediate boxes,
they may pay attention to application-layer traffic only. SPDY pings
make the connection appear active, letting it survive a period of
idleness.

Note: this commit adds support for pings in
k8s.io/apimachinery/pkg/util/httpstream/spdy, but doesn't enable it
anywhere in the calling code. There is no behavior change for existing
callers.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

This will be used in https://github.com/gravitational/teleport/, which acts as a man-in-the-middle auth proxy in for k8s API. When teleport is fronted by AWS ELB (same probably applies to other load balancers), kubectl exec sessions get disconnected after 60s of idleness. This PR addresses that.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

N/A

awly · 2020-08-21T21:58:07Z

/assign @smarterclayton

fedebongio · 2020-08-25T20:12:09Z

/assign @caesarxuchao
(@Jefftree was safe because on vacay)

caesarxuchao

The transition from SPDY to HTTP/2 is blocked on #7452 (comment), so in the mean time, I think we can accept PRs maintaining SPDY. @deads2k @lavalamp what do you think?

@awly How's the user going to enable the ping?

This is similar to #81179. cc @liggitt who reviewed that PR.

caesarxuchao · 2020-08-25T21:27:40Z

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

+			// actually closed.
+		} else {
+			// Count successful pings, for testing.
+			atomic.AddInt64(&c.pingsSent, 1)


Can we avoid this testing-only code? Dose the SPDY library provide any testing support that exposes if a ping frame is received?

Unfortunately, the library handles incoming pings internally without any external hooks I can find.
This unexported field is the least intrusive way I could verify the behavior. Using sync/atomic, performance overhead should be negligible.

+1, if you want to test this, add a member variable of type function for calling Ping and let the test swap that out. It's not performance, it's just a code smell.

fair enough, added a swap-able function field in connection

awly

How's the user going to enable the ping?

@caesarxuchao this PR doesn't expose it to k8s users.
My motivation is for https://github.com/gravitational/teleport/ which uses this library to intercept kubectl exec connections for audit purposes (recording session traffic). For kube-apiserver, pings are usually unnecessary since it's generally not fronted by a strict load balancer killing idle connections.

If you'd like, I can plumb a flag up to kube-apiserver. But it adds unnecessary complexity, if nobody is asking to use it there.

awly · 2020-08-25T22:01:54Z

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

+			// actually closed.
+		} else {
+			// Count successful pings, for testing.
+			atomic.AddInt64(&c.pingsSent, 1)


Unfortunately, the library handles incoming pings internally without any external hooks I can find.
This unexported field is the least intrusive way I could verify the behavior. Using sync/atomic, performance overhead should be negligible.

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

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

lavalamp · 2020-08-25T23:01:04Z

I think I'm OK with adding this and I might even be OK with turning it on, as long as we do that early in the release cycle. But please don't change the function signatures. :)

awly

PTAL

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

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

awly · 2020-08-25T23:28:26Z

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

+			// actually closed.
+		} else {
+			// Count successful pings, for testing.
+			atomic.AddInt64(&c.pingsSent, 1)


fair enough, added a swap-able function field in connection

lavalamp · 2020-08-25T23:45:34Z

I think I'm OK with this now, @caesarxuchao can you do a final review?

awly · 2020-08-26T18:26:25Z

/retest

awly · 2020-08-31T16:34:39Z

@caesarxuchao could you take another look please?

caesarxuchao

Two nits. Otherwise lgtm.

caesarxuchao · 2020-09-01T21:59:24Z

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

+	Proxier func(*http.Request) (*url.URL, error)
+	// ConnPingPeriod is a period for sending SPDY Pings on the connection.
+	// Optional.
+	ConnPingPeriod time.Duration


nit: why not just naming it "PingPeriod"?

renamed, here an in upgrade.go

caesarxuchao · 2020-09-01T22:03:52Z

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

 	go conn.Serve(c.newSpdyStream)
+	if pingPeriod > 0 {


Also check if c.ping is nil?

When an SPDY connection goes over an intermediate box (proxy or load balancer, e.g. AWS ELB), it can get interrupted due to idleness (default in ELB is 60s). For example, this happens with `kubectl exec` sessions are left open without any activity for a while. TCP-level keep-alives are not sufficient for all intermediate boxes, they may pay attention to application-layer traffic only. SPDY pings make the connection appear active, letting it survive a period of idleness. Note: this commit adds support for pings in `k8s.io/apimachinery/pkg/util/httpstream/spdy`, but doesn't enable it anywhere in the calling code. There is no behavior change for existing callers.

awly

Thanks @caesarxuchao, nits fixed

awly · 2020-09-01T23:05:45Z

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

 	go conn.Serve(c.newSpdyStream)
+	if pingPeriod > 0 {


awly · 2020-09-01T23:06:01Z

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

+	Proxier func(*http.Request) (*url.URL, error)
+	// ConnPingPeriod is a period for sending SPDY Pings on the connection.
+	// Optional.
+	ConnPingPeriod time.Duration


renamed, here an in upgrade.go

knight42 · 2020-09-03T17:26:02Z

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

 }

 // newConnection returns a new connection wrapping conn. newStreamHandler
 // will be invoked when the server receives a newly created stream from the
 // client.
-func newConnection(conn *spdystream.Connection, newStreamHandler httpstream.NewStreamHandler) httpstream.Connection {
-	c := &connection{conn: conn, newStreamHandler: newStreamHandler}
+func newConnection(conn *spdystream.Connection, newStreamHandler httpstream.NewStreamHandler, pingPeriod time.Duration, pingFn func() (time.Duration, error)) httpstream.Connection {


nit: given that this is a private function, and it looks like pingFn is always spdyConn.Ping. Would it be better to omit the pingFn argument and invoke c.conn.Ping directly in c.sendPings?

Oh I overlook the test..

awly · 2020-09-14T20:21:51Z

Ping @caesarxuchao

caesarxuchao · 2020-09-14T21:23:01Z

/retest
/lgtm

awly · 2020-09-15T20:40:59Z

/milestone v1.20

k8s-ci-robot · 2020-09-15T20:41:01Z

@awly: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.20

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.

awly · 2020-09-17T15:12:51Z

/priority backlog

awly · 2020-09-17T15:14:53Z

@caesarxuchao this PR is still missing your approval, i think

awly · 2020-09-22T17:53:00Z

ping @caesarxuchao

lavalamp · 2020-09-22T22:41:33Z

/approve

k8s-ci-robot · 2020-09-22T22:42:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awly, lavalamp

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~~ [lavalamp]

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 requested review from mikedanese and sjenning August 21, 2020 21:53

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 21, 2020

awly mentioned this pull request Aug 21, 2020

kubectl exec session with ELB not respecting keep-alives and session not in UI gravitational/teleport#3765

Closed

k8s-ci-robot assigned smarterclayton Aug 21, 2020

awly force-pushed the spdy-pings branch from 873f33d to abac031 Compare August 21, 2020 22:15

k8s-ci-robot assigned caesarxuchao Aug 25, 2020

caesarxuchao reviewed Aug 25, 2020

View reviewed changes

awly commented Aug 25, 2020

View reviewed changes

lavalamp reviewed Aug 25, 2020

View reviewed changes

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

lavalamp reviewed Aug 25, 2020

View reviewed changes

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

lavalamp reviewed Aug 25, 2020

View reviewed changes

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

awly force-pushed the spdy-pings branch from abac031 to 28dfade Compare August 25, 2020 23:32

awly commented Aug 25, 2020

View reviewed changes

awly force-pushed the spdy-pings branch from 28dfade to b4017b8 Compare August 26, 2020 15:43

lavalamp mentioned this pull request Sep 1, 2020

Kubectl: Keepalive connection to API server for exec and logs. #94301

Closed

caesarxuchao reviewed Sep 1, 2020

View reviewed changes

awly force-pushed the spdy-pings branch from b4017b8 to 9269da5 Compare September 1, 2020 23:05

awly commented Sep 1, 2020

View reviewed changes

knight42 reviewed Sep 3, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2020

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 17, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2020

k8s-ci-robot merged commit 4332d34 into kubernetes:master Sep 22, 2020

k8s-ci-robot added this to the v1.20 milestone Sep 22, 2020

awly deleted the spdy-pings branch September 23, 2020 18:08

awly mentioned this pull request Sep 24, 2020

k8s: enable SPDY pings on in/outbound connections gravitational/teleport#4377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spdy: add optional periodic Pings on the connection #94170

spdy: add optional periodic Pings on the connection #94170

awly commented Aug 21, 2020

awly commented Aug 21, 2020

fedebongio commented Aug 25, 2020

caesarxuchao left a comment

caesarxuchao Aug 25, 2020

awly Aug 25, 2020

lavalamp Aug 25, 2020

awly Aug 25, 2020

awly left a comment

awly Aug 25, 2020

lavalamp commented Aug 25, 2020

awly left a comment

awly Aug 25, 2020

lavalamp commented Aug 25, 2020

awly commented Aug 26, 2020

awly commented Aug 31, 2020

caesarxuchao left a comment

caesarxuchao Sep 1, 2020

awly Sep 1, 2020

caesarxuchao Sep 1, 2020

awly Sep 1, 2020

awly left a comment

awly Sep 1, 2020

awly Sep 1, 2020

knight42 Sep 3, 2020

knight42 Sep 3, 2020

awly commented Sep 14, 2020

caesarxuchao commented Sep 14, 2020

awly commented Sep 15, 2020

k8s-ci-robot commented Sep 15, 2020

awly commented Sep 17, 2020

awly commented Sep 17, 2020

awly commented Sep 22, 2020

lavalamp commented Sep 22, 2020

k8s-ci-robot commented Sep 22, 2020

spdy: add optional periodic Pings on the connection #94170

spdy: add optional periodic Pings on the connection #94170

Conversation

awly commented Aug 21, 2020

awly commented Aug 21, 2020

fedebongio commented Aug 25, 2020

caesarxuchao left a 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

awly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Aug 25, 2020

awly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Aug 25, 2020

awly commented Aug 26, 2020

awly commented Aug 31, 2020

caesarxuchao left a 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

awly left a 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

awly commented Sep 14, 2020

caesarxuchao commented Sep 14, 2020

awly commented Sep 15, 2020

k8s-ci-robot commented Sep 15, 2020

awly commented Sep 17, 2020

awly commented Sep 17, 2020

awly commented Sep 22, 2020

lavalamp commented Sep 22, 2020

k8s-ci-robot commented Sep 22, 2020