feat: enable SPDY pings on connections

[knight42]

Signed-off-by: Jian Zeng zengjian.zj@bytedance.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

Enable SPDY pings on connections and the period is set to 5s.

Which issue(s) this PR fixes:

xref #94301

Special notes for your reviewer:

Both kubectl exec and kubectl portforward invoke spdy.RoundTripperFor transitively:

• kubectl exec:

  kubernetes/staging/src/k8s.io/client-go/tools/remotecommand/remotecommand.go

  Lines 73 to 74 in e1c617a

  	func NewSPDYExecutor(config *restclient.Config, method string, url *url.URL) (Executor, error) {
  	wrapper, upgradeRoundTripper, err := spdy.RoundTripperFor(config)

• kubectl portforward:

  kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/portforward/portforward.go

  Lines 138 to 139 in e1c617a

  	func (f *defaultPortForwarder) ForwardPorts(method string, url *url.URL, opts PortForwardOptions) error {
  	transport, upgrader, err := spdy.RoundTripperFor(opts.Config)

Does this PR introduce a user-facing change?:

Enable SPDY pings to keep connections alive, so that `kubectl exec` and `kubectl portforward` won't be interrupted.

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

[knight42]

/cc @caesarxuchao @lavalamp

[nielsbasjes]

Question: Is the kubectl logs -f case also fixed with this?

[knight42]

Question: Is the kubectl logs -f case also fixed with this?

@nielsbasjes I think this is a different problem, because kubectl logs -f is actually a simple HTTP request.

[aojea]

Why not enabling it by default as with http/2 #95981 ?

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

Lines 93 to 102 in 4332d34

	// NewRoundTripperWithProxy creates a new SpdyRoundTripper that will use the
	// specified tlsConfig and proxy func.
	func NewRoundTripperWithProxy(tlsConfig *tls.Config, followRedirects, requireSameHostRedirects bool, proxier func(*http.Request) (*url.URL, error)) *SpdyRoundTripper {
	return NewRoundTripperWithConfig(RoundTripperConfig{
	TLS: tlsConfig,
	FollowRedirects: followRedirects,
	RequireSameHostRedirects: requireSameHostRedirects,
	Proxier: proxier,
	})
	}

/cc @caesarxuchao @lavalamp

[knight42]

flake

--- FAIL: TestInitialSync (0.02s)
    api_test.go:171: Unexpected endpoints: []*v1.Endpoints{(*v1.Endpoints)(0xc00044b900)}, expected: []*v1.Endpoints{(*v1.Endpoints)(0xc0004ac280), (*v1.Endpoints)(0xc0004ac140)}

/test pull-kubernetes-bazel-test

[fedebongio]

/assign @caesarxuchao
/triage accepted

[tiloso]

Should NewRoundTripperWithProxy be removed from k8s.io/apimachinery/pkg/util/httpstream/spdy? I think it's not being used anywhere else.

[knight42]

Should NewRoundTripperWithProxy be removed from k8s.io/apimachinery/pkg/util/httpstream/spdy? I think it's not being used anywhere else.

I guess we'd better not, as NewRoundTripperWithProxy is an exported function, we might break other users if we suddenly remove it.

[caesarxuchao]

The 20 second is based on the bug report #94301. People might need a different pingPeriod to satisfy different load balancers.

I think we should expose this variable as a flag of kubectl exec and kubectl port-forward commands. That will need approvals from @kubernetes/sig-cli-api-reviews.

I'm ok with hard-code it to 20 seconds for now and make it configurable in a follow-up.

Can you remove the TODO?

[nielsbasjes]

Given the example config for a loadbalancer sets a timeout to 20 seconds having the ping every 20 seconds will most likely cause problems "in the wild". I see this simply as cutting it too close.

I would really prefer a ping interval of something like 5 or 10 seconds because of that.

[lavalamp]

I vote let's hard code to 5s and not make it configurable.

[knight42]

I have updated the ping period to 5s.

[fejta-bot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[lavalamp]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knight42, 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/client-go/OWNERS [lavalamp]

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

[aojea]

awesome,
I think this was the last one without keepalive , right?
we should see less complains about hanging connections ... hopefully 🤞 ;)

[caesarxuchao]

The unit test failure looks real:

1/1940 Tests Failed. | expand_less
-- | --
go_test: //staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/go_default_test:run_1_of_2 expand_less0sexec ${PAGER:-/usr/bin/less} "$0" \|\| exit 1 Executing tests from //staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy:go_default_test ----------------------------------------------------------------------------- --- FAIL: TestConnectionPings (0.04s)     connection_test.go:240: server: failed to send any pings (check logs) 2021/01/13 19:01:17 http: TLS handshake error from 127.0.0.1:33312: remote error: tls: bad certificate 2021/01/13 19:01:17 http: TLS handshake error from 127.0.0.1:34410: remote error: tls: bad certificate 2021/01/13 19:01:17 http: TLS handshake error from 127.0.0.1:52824: remote error: tls: bad certificate 2021/01/13 19:01:17 [001] WARN: Error copying to client: read tcp 127.0.0.1:37014->127.0.0.1:40645: read: connection reset by peer 2021/01/13 19:01:18 http: TLS handshake error from 127.0.0.1:47462: remote error: tls: bad certificate 2021/01/13 19:01:18 http: TLS handshake error from 127.0.0.1:60264: remote error: tls: bad certificate 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:58088->127.0.0.1:42433: read: connection reset by peer 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:37486->127.0.0.1:36923: read: connection reset by peer 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:33462->127.0.0.1:42509: read: connection reset by peer 2021/01/13 19:01:18 http: TLS handshake error from 127.0.0.1:48890: remote error: tls: bad certificate 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:32982->127.0.0.1:36415: read: connection reset by peer 2021/01/13 19:01:19 [001] WARN: Error copying to client: read tcp 127.0.0.1:45768->127.0.0.1:46097: read: connection reset by peer FAIL | go_test: //staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/go_default_test:run_1_of_2 expand_less | 0s | exec ${PAGER:-/usr/bin/less} "$0" \|\| exit 1 Executing tests from //staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy:go_default_test ----------------------------------------------------------------------------- --- FAIL: TestConnectionPings (0.04s)     connection_test.go:240: server: failed to send any pings (check logs) 2021/01/13 19:01:17 http: TLS handshake error from 127.0.0.1:33312: remote error: tls: bad certificate 2021/01/13 19:01:17 http: TLS handshake error from 127.0.0.1:34410: remote error: tls: bad certificate 2021/01/13 19:01:17 http: TLS handshake error from 127.0.0.1:52824: remote error: tls: bad certificate 2021/01/13 19:01:17 [001] WARN: Error copying to client: read tcp 127.0.0.1:37014->127.0.0.1:40645: read: connection reset by peer 2021/01/13 19:01:18 http: TLS handshake error from 127.0.0.1:47462: remote error: tls: bad certificate 2021/01/13 19:01:18 http: TLS handshake error from 127.0.0.1:60264: remote error: tls: bad certificate 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:58088->127.0.0.1:42433: read: connection reset by peer 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:37486->127.0.0.1:36923: read: connection reset by peer 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:33462->127.0.0.1:42509: read: connection reset by peer 2021/01/13 19:01:18 http: TLS handshake error from 127.0.0.1:48890: remote error: tls: bad certificate 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:32982->127.0.0.1:36415: read: connection reset by peer 2021/01/13 19:01:19 [001] WARN: Error copying to client: read tcp 127.0.0.1:45768->127.0.0.1:46097: read: connection reset by peer FAIL
go_test: //staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/go_default_test:run_1_of_2 expand_less | 0s
exec ${PAGER:-/usr/bin/less} "$0" \|\| exit 1 Executing tests from //staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy:go_default_test ----------------------------------------------------------------------------- --- FAIL: TestConnectionPings (0.04s)     connection_test.go:240: server: failed to send any pings (check logs) 2021/01/13 19:01:17 http: TLS handshake error from 127.0.0.1:33312: remote error: tls: bad certificate 2021/01/13 19:01:17 http: TLS handshake error from 127.0.0.1:34410: remote error: tls: bad certificate 2021/01/13 19:01:17 http: TLS handshake error from 127.0.0.1:52824: remote error: tls: bad certificate 2021/01/13 19:01:17 [001] WARN: Error copying to client: read tcp 127.0.0.1:37014->127.0.0.1:40645: read: connection reset by peer 2021/01/13 19:01:18 http: TLS handshake error from 127.0.0.1:47462: remote error: tls: bad certificate 2021/01/13 19:01:18 http: TLS handshake error from 127.0.0.1:60264: remote error: tls: bad certificate 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:58088->127.0.0.1:42433: read: connection reset by peer 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:37486->127.0.0.1:36923: read: connection reset by peer 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:33462->127.0.0.1:42509: read: connection reset by peer 2021/01/13 19:01:18 http: TLS handshake error from 127.0.0.1:48890: remote error: tls: bad certificate 2021/01/13 19:01:18 [001] WARN: Error copying to client: read tcp 127.0.0.1:32982->127.0.0.1:36415: read: connection reset by peer 2021/01/13 19:01:19 [001] WARN: Error copying to client: read tcp 127.0.0.1:45768->127.0.0.1:46097: read: connection reset by peer FAIL

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.