[client-go] Renegotiate Streaming Content-Type #62175

DirectXMan12 · 2018-04-05T18:22:05Z

REST clients in client-go have two content type fields -- the "accepted"
content types, and the default content type. When we set up
serializers and such, we set up default decoders, etc for the default
content type, and set up a "renegotiation function" to renegotiate in
case the server sends some other content type.

However, in the case of streaming, we don't do renegotiation properly --
we assume whatever the server sent is compatible with our default stream
framing/decoding.

This is fine an dandy when it actually is compatible, and is generally
fine an dandy when using the JSON content type as the default, because
most things support JSON. However, if you set your default up to, say,
protobuf, and then try to use that REST config to construct a REST
client for talking to the custom resources API server, you're going to
have a bad time, since it doesn't support protobuf, only JSON.

This in turn causes weird behavior like watches starting succesfully,
but never actuall firing any watch events.

To fix this, this commit introduces a new renegotiation function for
streaming serialization and framing, and checks to see if it needs to be
used.

Release note:

Fix content-type negotiation in streaming watch requests in client-go

DirectXMan12 · 2018-04-05T18:22:34Z

cc @kubernetes/sig-api-machinery-bugs @kubernetes/sig-api-machinery-pr-reviews

liggitt · 2018-04-05T18:31:56Z

staging/src/k8s.io/client-go/rest/request.go

+		if err != nil {
+			return nil, err
+		}
+		normalDecoder, streamingSerializer, framer, err = r.serializers.RenegotiatedStreaming(mediaType, params)


this assumes RenegotiatedStreaming is non-nil?

the code seemed to assume StreamingSerializer was non-nil before. I'll add a check in to be safe.

liggitt · 2018-04-05T18:32:49Z

staging/src/k8s.io/client-go/rest/client.go

@@ -200,6 +201,14 @@ func createSerializers(config ContentConfig) (*Serializers, error) {
 	if info.StreamSerializer != nil {
 		s.StreamingSerializer = info.StreamSerializer.Serializer
 		s.Framer = info.StreamSerializer.Framer
+		s.RenegotiatedStreaming = func(contentType string, params map[string]string) (runtime.Decoder, runtime.Serializer, runtime.Framer, error) {


definitely needs tests to cover positive and negative cases around this

yep, will do. Wanted to make sure there wasn't something I missed with the approach.

pushed some basic tests, can do more if needed (will probably write a few more on monday anyway)

REST clients in client-go have two content type fields -- the "accepted" content types, and the default content type. When we set up serializers and such, we set up default decoders, etc for the default content type, and set up a "renegotiation function" to renegotiate in case the server sends some other content type. However, in the case of streaming, we don't do renegotiation properly -- we assume whatever the server sent is compatible with our default stream framing/decoding. This is fine an dandy when it actually is compatible, and is generally fine an dandy when using the JSON content type as the default, because most things support JSON. However, if you set your default up to, say, protobuf, and then try to use that REST config to construct a REST client for talking to the custom resources API server, you're going to have a bad time, since it doesn't support protobuf, only JSON. This in turn causes weird behavior like watches starting succesfully, but never actuall firing any watch events. To fix this, this commit introduces a new renegotiation function for streaming serialization and framing, and checks to see if it needs to be used.

wenjiaswe · 2018-04-12T20:11:31Z

cc @yliaog

k8s-ci-robot · 2018-04-26T22:53:43Z

@DirectXMan12: PR needs rebase.

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.

fejta-bot · 2018-07-25T23:01:55Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2018-08-24T23:48:36Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2018-09-24T00:41:11Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2018-09-24T00:41:18Z

@fejta-bot: Closing this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot · 2018-10-17T14:04:04Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`bd2f112`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-integration	`bd2f112`	link	`/test pull-kubernetes-integration`
pull-kubernetes-e2e-gce	`bd2f112`	link	`/test pull-kubernetes-e2e-gce`
pull-kubernetes-e2e-gce-device-plugin-gpu	`bd2f112`	link	`/test pull-kubernetes-e2e-gce-device-plugin-gpu`
pull-kubernetes-e2e-gce-100-performance	`bd2f112`	link	`/test pull-kubernetes-e2e-gce-100-performance`
pull-kubernetes-node-e2e	`bd2f112`	link	`/test pull-kubernetes-node-e2e`
pull-kubernetes-bazel-test	`bd2f112`	link	`/test pull-kubernetes-bazel-test`
pull-kubernetes-bazel-build	`bd2f112`	link	`/test pull-kubernetes-bazel-build`
pull-kubernetes-kubemark-e2e-gce-big	`bd2f112`	link	`/test pull-kubernetes-kubemark-e2e-gce-big`
pull-kubernetes-typecheck	`bd2f112`	link	`/test pull-kubernetes-typecheck`
pull-kubernetes-verify	`bd2f112`	link	`/test pull-kubernetes-verify`

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.

jsafrane · 2018-10-18T07:42:03Z

seems related to #67803 (comment)

Yes, symptoms described here is what I saw in #67803 (review). Client created there worked just fine with JSON, only watch events were sent by API server as protobuf and the client failed to parse them. My informer thus did not get any events.

DirectXMan12 · 2018-11-07T20:28:46Z

I'll try and rebase this soonish

This e2e test reproduces kubernetes#62175 and will be expanded to check for other negotiation related errors against real servers.

liggitt · 2019-11-11T02:17:47Z

/close

in favor of #84692

k8s-ci-robot · 2019-11-11T02:17:48Z

@liggitt: Closed this PR.

In response to this:

/close

in favor of #84692

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.

This e2e test reproduces kubernetes#62175 and will be expanded to check for other negotiation related errors against real servers.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 5, 2018

k8s-ci-robot requested review from errordeveloper and rmmh April 5, 2018 18:22

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/bug Categorizes issue or PR as related to a bug. labels Apr 5, 2018

liggitt reviewed Apr 5, 2018

View reviewed changes

k8s-ci-robot assigned liggitt Apr 5, 2018

k8s-ci-robot closed this Apr 5, 2018

k8s-ci-robot reopened this Apr 5, 2018

DirectXMan12 force-pushed the bug/renegotiate-streaming-content-type branch from 2ca9e67 to bd2f112 Compare April 5, 2018 22:23

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 5, 2018

rmmh removed their request for review April 6, 2018 21:56

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 25, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 24, 2018

k8s-ci-robot closed this Sep 24, 2018

liggitt added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Oct 17, 2018

liggitt mentioned this pull request Oct 18, 2018

WIP - Fix watch event handling for CRDs #69987

Closed

liggitt mentioned this pull request Nov 17, 2018

Make bootstrap client cert loading part of rotation #69890

Merged

smarterclayton mentioned this pull request Feb 11, 2019

Report a watch error instead of eating it when we can't decode #73937

Merged

smarterclayton mentioned this pull request Feb 11, 2019

Fix watch negotiation when using a non-default mime type in the client #73938

Closed

liggitt assigned smarterclayton Mar 7, 2019

liggitt mentioned this pull request Mar 15, 2019

Support Table and PartialObjectMetadata on watch #71548

Merged

4 tasks

liggitt removed their assignment Apr 2, 2019

smarterclayton mentioned this pull request Nov 3, 2019

Fix watch negotiation when using a non-default mime type in the client #84692

Merged

k8s-ci-robot closed this Nov 11, 2019

DirectXMan12 deleted the bug/renegotiate-streaming-content-type branch December 4, 2019 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[client-go] Renegotiate Streaming Content-Type #62175

[client-go] Renegotiate Streaming Content-Type #62175

DirectXMan12 commented Apr 5, 2018

DirectXMan12 commented Apr 5, 2018

liggitt Apr 5, 2018

DirectXMan12 Apr 5, 2018

liggitt Apr 5, 2018

DirectXMan12 Apr 5, 2018

DirectXMan12 Apr 5, 2018

wenjiaswe commented Apr 12, 2018

k8s-ci-robot commented Apr 26, 2018

fejta-bot commented Jul 25, 2018

fejta-bot commented Aug 24, 2018

fejta-bot commented Sep 24, 2018

k8s-ci-robot commented Sep 24, 2018

k8s-ci-robot commented Oct 17, 2018

jsafrane commented Oct 18, 2018

DirectXMan12 commented Nov 7, 2018

liggitt commented Nov 11, 2019

k8s-ci-robot commented Nov 11, 2019

[client-go] Renegotiate Streaming Content-Type #62175

[client-go] Renegotiate Streaming Content-Type #62175

Conversation

DirectXMan12 commented Apr 5, 2018

DirectXMan12 commented Apr 5, 2018

liggitt Apr 5, 2018

Choose a reason for hiding this comment

DirectXMan12 Apr 5, 2018

Choose a reason for hiding this comment

liggitt Apr 5, 2018

Choose a reason for hiding this comment

DirectXMan12 Apr 5, 2018

Choose a reason for hiding this comment

DirectXMan12 Apr 5, 2018

Choose a reason for hiding this comment

wenjiaswe commented Apr 12, 2018

k8s-ci-robot commented Apr 26, 2018

fejta-bot commented Jul 25, 2018

fejta-bot commented Aug 24, 2018

fejta-bot commented Sep 24, 2018

k8s-ci-robot commented Sep 24, 2018

k8s-ci-robot commented Oct 17, 2018

jsafrane commented Oct 18, 2018

DirectXMan12 commented Nov 7, 2018

liggitt commented Nov 11, 2019

k8s-ci-robot commented Nov 11, 2019