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

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

Open
wants to merge 6 commits into
base: master
from

Conversation

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 11, 2019

This goes a bit further than #62175 and makes some other changes to RESTClient that have been delayed for a while, specifically adding an interface specific to client-> server interactions. That makes it easier to remove conversion specific logic from rest client (which is long overdue) behind that interface, as well as setting the stage for a single client to handle regular Kube objects in Protobuf but CRD objects in JSON (so we don't have to have two clients). For now, focus on streamlining how RESTClient converts objects to bytes and back.

The e2e test reproduces #62175 and will be expanded to check for other
negotiation related errors against real servers. Builds on top of #73937 to get an actual error back.

NONE
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Feb 12, 2019

Not complete yet, and this is slightly more invasive than Solly's previous, but after looking through this code I'm not happy with levels of indirection that have accumulated. This cuts through the somewhat baroque negotiation logic and uses a clean interface to find a content type. It also cleans up the Request and RESTClient constructors and makes Request more of a subordinate to RESTClient.

I want to use this to make internal clients have to explicitly request conversion (via the generator), and then remove any whiff of conversion logic in RESTClient or the constructors. At that point the core client code should have unwound the last internal constructs (no defaulting, no conversion, no references to internal types), and we can cut out some of that old code.

@smarterclayton smarterclayton force-pushed the smarterclayton:protocol_errors branch 3 times, most recently from e868c6e to 0f452fd Feb 12, 2019

@smarterclayton smarterclayton force-pushed the smarterclayton:protocol_errors branch 4 times, most recently from 94af9fb to ca0a5df Feb 12, 2019

@smarterclayton smarterclayton force-pushed the smarterclayton:protocol_errors branch 4 times, most recently from 8057f9e to 26459d3 Feb 12, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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

@smarterclayton smarterclayton force-pushed the smarterclayton:protocol_errors branch from 26459d3 to b83b56f Feb 13, 2019

@yliaog

This comment has been minimized.

Copy link
Contributor

yliaog commented Feb 14, 2019

/cc

@k8s-ci-robot k8s-ci-robot requested a review from yliaog Feb 14, 2019

@logicalhan

This comment has been minimized.

Copy link
Contributor

logicalhan commented Feb 14, 2019

/assign @caesarxuchao

smarterclayton added some commits Feb 11, 2019

Report a watch error instead of eating it when we can't decode
Clients are required to handle watch events of type ERROR, so instead
of eating the decoding error we should pass it on to the client. Use
NewGenericServerError with isUnexpectedResponse to indicate that we
didn't get the bytes from the server we were expecting. For watch, the
415 error code is roughly correct and we will return an error to the
client that makes debugging a failure in either server watch or client
machinery much easier.

We do not alter the behavior when it appears the response is an EOF
or other disconnection.
Detect watch protocol errors via an e2e test for apimachinery
This e2e test reproduces #62175 and will be expanded to check for other
negotiation related errors against real servers.
Create a shim for Codecs that handles client duties
Clients have to renegotiate frequently, and the shape of the interface
that a client wants is slightly different than the interface on the
server. Create a new ClientNegotiator interface that represents what
the client->server code would want to use (mostly, still evolving
Encoder) and move versioning details out of RESTClient. Move
DirectEncoder to pkg/runtime so it is more accessible.

In the long run, we want to remove internal clients and conversions
from clients. This takes a step in that direction and also makes sure
watch negotiation is consistent with the server.
Remove deprecated-dynamic client
It is now unused.
Always negotiate a decoder using ClientNegotiator
This commit performs two refactors and fixes a bug.

Refactor 1 changes the signature of Request to take a RESTClient, which
removes the extra copy of everything on RESTClient from Request. A pair
of optional constructors are added for testing. The major functional
change is that Request no longer has the shim HTTPClient interface and
so some test cases change slightly because we are now going through
http.Client code paths instead of direct to our test stubs.

Refactor 2 changes the signature of RESTClient to take a
ClientContentConfig instead of ContentConfig - the primary difference
being that ClientContentConfig uses ClientNegotiator instead of
NegotiatedSerializer and the old Serializers type. We also collapse
some redundancies (like the rate limiter can be created outside the
constructor). We also remove the public accessor to Throttle in
favor of the method, and change the only caller of it to simply
request a new unlimited client. Naming is changed to rateLimiter
consistently.

The bug fix is to negotiate the streaming content type on a Watch()
like we do for requests. We stop caching the decoder and simply
resolve it on the request. We also clean up the dynamic client
and remove the extra WatchSpecificVersions() method by providing
a properly wrapped dynamic client.

The dynamic client is also updated to use a single scheme and to
return metav1.Status instead of unstructured.Unstructured when
client side watch errors are returned. An additional test case
is added to verify mismatched mime types are ignored.
Use the simpler DirectEncoder method to make init a bit terser
Minor adjustment for getting a direct encoder factory.

@smarterclayton smarterclayton force-pushed the smarterclayton:protocol_errors branch from b83b56f to 53a9962 Feb 18, 2019

@smarterclayton smarterclayton changed the title WIP: Fix watch negotiation Fix watch negotiation when using a non-default mime type in the client Feb 18, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 18, 2019

@smarterclayton: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e-containerized 53a9962 link /test pull-kubernetes-local-e2e-containerized

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment