50342: Establish '406 Not Acceptable' response for protobuf serializa… #67041

tristanburgess · 2018-08-06T23:33:02Z

…tion 'errNotMarshalable'

 - Added metav1.Status() that enforces '406 Not Acceptable' response if
protobuf serialization is not fully supported for the API resource type.
 - JSON and YAML serialization are supposed to be more completely baked
in, so serialization involving those, and general errors with seralizing
protobuf, will return '500 Internal Server Error'.
- If serialization failure occurs and original HTTP status code is
error, use the original status code, else use the serialization failure
status code.
 - Write encoded API responses to intermediate buffer
 - Use apimachinery/runtime::Encode() instead of
apimachinery/runtime/protocol::Encode() in
apiserver/endpoints/handlers/responsewriters/writers::SerializeObject()
 - This allows for intended encoder error handling to fully work, facilitated by
apiserver/endpoints/handlers/responsewriters/status::ErrorToAPIResponse() before officially
writing to the http.ResponseWriter
 - The specific part that wasn't working by ErrorToAPIResponse() was the
HTTP status code set. A direct call to
http.ResponseWriter::WriteHeader(statusCode) was made in
SerializeObject() with the original response status code, before
performing the encode. Once this
method is called, it can not again update the status code at a later
time, with say, an erro status code due to encode failure.
 - Updated relevant apiserver unit test to reflect the new behavior
(TestWriteJSONDecodeError())
 - Add build deps from make update for protobuf serializer

What this PR does / why we need it:
This PR fixes a bug that was blocking extensible error handling in the case that serializing response data fails, and implements a '406 Not Acceptable' status code response if protobuf marshal definitions are not implemented for an API resource type. See commit message for further details.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #50342

Special notes for your reviewer:

Release note:

Fixed a bug that was blocking extensible error handling when serializing API responses error out. Previously, serialization failures always resulted in the status code of the original response being returned. Now, the following behavior occurs:
   - If the serialization type is application/vnd.kubernetes.protobuf, and protobuf marshaling is not implemented for the requested API resource type, a '406 Not Acceptable is returned'.
   - If the serialization type is 'application/json':
        - If serialization fails, and the original status code was an failure (e.g. 4xx or 5xx), the original status code will be returned.
        - If serialization fails, and the original status code was not a failure (e.g. 2xx), the status code of the serialization failure will be returned. By default, this is '500 Internal Server Error', because JSON serialization is our default, and not supposed to be implemented on a type-by-type basis.

k8s-ci-robot · 2018-08-06T23:33:37Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

tristanburgess · 2018-08-06T23:36:23Z

I have signed the CLA, should be all set with that now.

tristanburgess · 2018-08-07T00:43:28Z

One remaining concern is of unit tests: I couldn't find any protobuf specific unit tests in apiserver/pkg/endpoints/apiserver_test.go. apimachinery/pkg/test/runtime_serializer_protobuf_protobuf_test.go didn't seem like the right place either, because it covers unit tests for the actual encode/decode, and not of the higher level response behavior. Any thoughts on this, adding a unit test for the 406 returned by 'errNotMarshalable'?

CaoShuFeng · 2018-08-07T06:17:43Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go

 	output, err := runtime.Encode(codec, status)
 	if err != nil {
-		w.WriteHeader(code)
+		w.Header().Set("Content-Type", "text/plain")


Should we modify header before calling WriteHeader?

From the document: https://golang.org/pkg/net/http/#ResponseWriter

// Changing the header map after a call to WriteHeader (or // Write) has no effect unless the modified headers are // trailers.

Yes, definitely, good catch. As the document implies, WriteHeader() and Write() are final in terms of header modification (andWriteHeader() seems to be intended to be called immediately before Write(), if it is used to modify the status code). I will make the change.

hzxuzhonghu · 2018-08-08T02:20:04Z

/ok-to-test

tristanburgess · 2018-08-13T22:15:19Z

/assign @liggitt

liggitt · 2018-08-16T13:44:53Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go


-	if err := encoder.Encode(object, w); err != nil {
-		errorJSONFatal(err, encoder, w)
+	if output, err := runtime.Encode(encoder, object); err != nil {


This means we will no longer serialize large responses (like list all nodes, list all pods, etc) in a streaming fashion.

@kubernetes/sig-scalability-pr-reviews @kubernetes/sig-api-machinery-pr-reviews might have opinions around losing that in order to get a better status code for something that is arguably a coding error.

I think it would make more sense to wrap the responsewriter we pass to encoder.Encode to call WriteHeader(statusCode) on first Write() (basically the same thing response writer already does, but with our custom status code instead of an implicit 200). Then, as long as an encoder error doesn't output anything before failure (which it shouldn't if the object can't be encoded at all), we still have the ability to write a serialization error code/response in failure cases.

Thanks, understood on the first point. Definitely don't think the tradeoff is worth it in that case. Good idea on the second point. I will look into getting that off the ground.

If we're going to make this change, I'd suggest we run some scalability tests first to ensure api call latencies aren't affected much.

@shyamjvs @liggitt I made a new commit that preserves the streaming fashion of serialization, while still allowing for more control over the header content. I believe this should take care of any scalability concern, but please let me know if there's tests I should run to ensure this. Thanks very much @liggitt for the review and idea.

liggitt · 2018-08-17T13:15:54Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go

 	}
-	w.Header().Set("Content-Type", "application/json")


why did this get dropped? I didn't expect errorJSONFatal to change much, given the new wrapped-writer approach. was the encoded status actually coming out in protobuf?

That particular line drop was from an earlier commit, and had no effect previously due to similar reasons as to why the status code wasn't changing. WriteHeader() had occurred before errJSONFatal, and afterwards prevents further modifications to the header. Further, the error status encoder above does encode the response to protobuf (or whatever other codec that's supported and used), and the request in the original issue filed did have vnd.kubernetes.protobuf in the Accept header, so a protobuf response is expected. Thus, this needed to be cleaned up, otherwise the error would be returned as JSON all the time, which is unexpected.

I see, thanks.

liggitt · 2018-08-17T13:44:13Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go


 	if err := encoder.Encode(object, w); err != nil {
-		errorJSONFatal(err, encoder, w)
+		errSerializationFatal(err, encoder, w)


I actually expected to pass innerW here, and for errorJSONFatal to remain more or less as-is. were there other issues with that?

If we pass innerW, we lose the original status code, which we still keep around if the original status code itself was an error. I chose this behavior based on similar implementation choices I found in this file; if the original status code was an error (4xx, 5xx etc), don't change the status code due to serialization problems. I also added the client expected media type as a separate field in the new type, so that we can control that part of header write time as well, the same way as the status code.

liggitt · 2018-08-17T15:00:07Z

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go

-	// still be an error object.  This seems ok, the alternative is to validate the object before
-	// encoding, but this really should never happen, so it's wasted compute for every API request.
-	status := expectApiStatus(t, "GET", server.URL, nil, http.StatusOK)
+	// This used to send the original response's HTTP status code, but the behavior in staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/status.go::ErrorToAPIStatus()


can drop the historical note from the comment. justification for the current behavior is fine.

liggitt · 2018-08-17T15:15:12Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go

+// HTTPResponseWriterWithInit wraps http.ResponseWriter, and implements the io.Writer interface to be used
+// with encoding. The purpose is to allow for encoding to a stream, while accommodating a custom HTTP status code
+// if encoding fails, and meeting the encoder's io.Writer interface requirement.
+type HTTPResponseWriterWithInit struct {


let's keep this unexported for now

liggitt · 2018-08-17T15:17:02Z

a couple nits, needs squashing, then LGTM. thanks for chasing this down

…tion 'errNotMarshalable' - Added metav1.Status() that enforces '406 Not Acceptable' response if protobuf serialization is not fully supported for the API resource type. - JSON and YAML serialization are supposed to be more completely baked in, so serialization involving those, and general errors with seralizing protobuf, will return '500 Internal Server Error'. - If serialization failure occurs and original HTTP status code is error, use the original status code, else use the serialization failure status code. - Write encoded API responses to intermediate buffer - Use apimachinery/runtime::Encode() instead of apimachinery/runtime/protocol::Encode() in apiserver/endpoints/handlers/responsewriters/writers::SerializeObject() - This allows for intended encoder error handling to fully work, facilitated by apiserver/endpoints/handlers/responsewriters/status::ErrorToAPIResponse() before officially writing to the http.ResponseWriter - The specific part that wasn't working by ErrorToAPIResponse() was the HTTP status code set. A direct call to http.ResponseWriter::WriteHeader(statusCode) was made in SerializeObject() with the original response status code, before performing the encode. Once this method is called, it can not again update the status code at a later time, with say, an erro status code due to encode failure. - Updated relevant apiserver unit test to reflect the new behavior (TestWriteJSONDecodeError()) - Add build deps from make update for protobuf serializer 50342: Code review suggestion impl - Ensure that http.ResponseWriter::Header().Set() is called before http.ResponseWriter::WriteHeader() - This will avert a potential issue where changing the response media type to text/plain wouldn't work. - We want to respond with plain text if serialization fails of the original response, and serialization also fails for the resultant error response. 50342: wrapper for http.ResponseWriter - Prevent potential performance regression caused by modifying encode to use a buffer instead of streaming - This is achieved by creating a wrapper type for http.ResponseWriter that will use WriteHeader(statusCode) on the first call to Write(). Thus, on encode success, Write() will write the original statusCode. On encode failure, we pass control onto responsewriters::errSerializationFatal(), which will process the error to obtain potentially a new status code, depending on whether or not the original status code was itself an error. 50342: code review suggestions - Remove historical note from unit test comment - Don't export httpResponseWriterWithInit type (for now)

tristanburgess · 2018-08-17T20:22:41Z

Thanks @liggitt, I committed the changes for the nits and squashed. Thanks for taking the time to review.

liggitt · 2018-08-18T00:06:59Z

/lgtm

k8s-ci-robot · 2018-08-18T00:07:04Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]
~~staging/src/k8s.io/apiserver/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-github-robot · 2018-08-18T00:50:55Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2018-08-18T01:50:35Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce	`bcdf3bb`	link	`/test pull-kubernetes-e2e-gce`

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.

k8s-github-robot · 2018-08-18T02:14:04Z

Automatic merge from submit-queue (batch tested with PRs 67041, 66948). If you want to cherry-pick this change to another branch, please follow the instructions here.

tristanburgess · 2018-08-18T11:23:50Z

Believe the test pull-kubernetes-e2e-gce failed related to #67295
/test pull-kubernetes-e2e-gce

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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 6, 2018

k8s-ci-robot requested review from hzxuzhonghu and thockin August 6, 2018 23:33

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 6, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 6, 2018

CaoShuFeng reviewed Aug 7, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 8, 2018

k8s-ci-robot assigned liggitt Aug 13, 2018

liggitt reviewed Aug 16, 2018

View reviewed changes

k8s-ci-robot added sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 16, 2018

liggitt reviewed Aug 17, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2018

k8s-github-robot merged commit 363e341 into kubernetes:master Aug 18, 2018

tristanburgess deleted the 50342_error_handling_refinement_for_serialization_encode branch August 18, 2018 11:23

This was referenced Dec 13, 2019

sample-apiserver returns "$type does not implement the protobuf marshalling interface and cannot be encoded to a protobuf message" error instead of falling back to second Accept type #86253

Open

wardle example e2e #84735

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

50342: Establish '406 Not Acceptable' response for protobuf serializa… #67041

50342: Establish '406 Not Acceptable' response for protobuf serializa… #67041

tristanburgess commented Aug 6, 2018

k8s-ci-robot commented Aug 6, 2018

tristanburgess commented Aug 6, 2018

tristanburgess commented Aug 7, 2018

CaoShuFeng Aug 7, 2018

tristanburgess Aug 7, 2018 •

edited

hzxuzhonghu commented Aug 8, 2018

tristanburgess commented Aug 13, 2018

liggitt Aug 16, 2018 •

edited

liggitt Aug 16, 2018

tristanburgess Aug 16, 2018

shyamjvs Aug 17, 2018

tristanburgess Aug 17, 2018 •

edited

liggitt Aug 17, 2018 •

edited

tristanburgess Aug 17, 2018

liggitt Aug 17, 2018

liggitt Aug 17, 2018

tristanburgess Aug 17, 2018 •

edited

liggitt Aug 17, 2018 •

edited

liggitt Aug 17, 2018

liggitt commented Aug 17, 2018

tristanburgess commented Aug 17, 2018

liggitt commented Aug 18, 2018

k8s-ci-robot commented Aug 18, 2018

k8s-github-robot commented Aug 18, 2018

k8s-ci-robot commented Aug 18, 2018

k8s-github-robot commented Aug 18, 2018

tristanburgess commented Aug 18, 2018

50342: Establish '406 Not Acceptable' response for protobuf serializa… #67041

50342: Establish '406 Not Acceptable' response for protobuf serializa… #67041

Conversation

tristanburgess commented Aug 6, 2018

k8s-ci-robot commented Aug 6, 2018

tristanburgess commented Aug 6, 2018

tristanburgess commented Aug 7, 2018

Choose a reason for hiding this comment

tristanburgess Aug 7, 2018 • edited

Choose a reason for hiding this comment

hzxuzhonghu commented Aug 8, 2018

tristanburgess commented Aug 13, 2018

liggitt Aug 16, 2018 • edited

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

tristanburgess Aug 17, 2018 • edited

Choose a reason for hiding this comment

liggitt Aug 17, 2018 • edited

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

tristanburgess Aug 17, 2018 • edited

Choose a reason for hiding this comment

liggitt Aug 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 17, 2018

tristanburgess commented Aug 17, 2018

liggitt commented Aug 18, 2018

k8s-ci-robot commented Aug 18, 2018

k8s-github-robot commented Aug 18, 2018

k8s-ci-robot commented Aug 18, 2018

k8s-github-robot commented Aug 18, 2018

tristanburgess commented Aug 18, 2018

tristanburgess Aug 7, 2018 •

edited

liggitt Aug 16, 2018 •

edited

tristanburgess Aug 17, 2018 •

edited

liggitt Aug 17, 2018 •

edited

tristanburgess Aug 17, 2018 •

edited

liggitt Aug 17, 2018 •

edited