Merge pull request #67041 from tristanburgess/50342_error_handling_refinement_for_serialization_encode

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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

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

…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**:

```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.

```

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/protobuf/BUILD

@@ -14,6 +14,7 @@ go_library(
     importmap = "k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/runtime/serializer/protobuf",
     importpath = "k8s.io/apimachinery/pkg/runtime/serializer/protobuf",
     deps = [
+        "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/serializer/recognizer:go_default_library",

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/protobuf/protobuf.go

@@ -20,10 +20,12 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"net/http"
 	"reflect"
 
 	"github.com/gogo/protobuf/proto"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer/recognizer"
@@ -50,6 +52,15 @@ func (e errNotMarshalable) Error() string {
 	return fmt.Sprintf("object %v does not implement the protobuf marshalling interface and cannot be encoded to a protobuf message", e.t)
 }
 
+func (e errNotMarshalable) Status() metav1.Status {
+	return metav1.Status{
+		Status:  metav1.StatusFailure,
+		Code:    http.StatusNotAcceptable,
+		Reason:  metav1.StatusReason("NotAcceptable"),
+		Message: e.Error(),
+	}
+}
+
 func IsNotMarshalable(err error) bool {
 	_, ok := err.(errNotMarshalable)
 	return err != nil && ok

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

@@ -3667,10 +3667,12 @@ func TestWriteJSONDecodeError(t *testing.T) {
 		responsewriters.WriteObjectNegotiated(codecs, newGroupVersion, w, req, http.StatusOK, &UnregisteredAPIObject{"Undecodable"})
 	}))
 	defer server.Close()
-	// We send a 200 status code before we encode the object, so we expect OK, but there will
-	// 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)
+	// Decode error response behavior is dictated by
+	// apiserver/pkg/endpoints/handlers/responsewriters/status.go::ErrorToAPIStatus().
+	// Unless specific metav1.Status() parameters are implemented for the particular error in question, such that
+	// the status code is defined, metav1 errors where error.status == metav1.StatusFailure
+	// will throw a '500 Internal Server Error'. Non-metav1 type errors will always throw a '500 Internal Server Error'.
+	status := expectApiStatus(t, "GET", server.URL, nil, http.StatusInternalServerError)
 	if status.Reason != metav1.StatusReasonUnknown {
 		t.Errorf("unexpected reason %#v", status)
 	}

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

@@ -35,6 +35,26 @@ import (
 	"k8s.io/apiserver/pkg/util/wsstream"
 )
 
+// 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 {
+	hasWritten bool
+	mediaType  string
+	statusCode int
+	innerW     http.ResponseWriter
+}
+
+func (w httpResponseWriterWithInit) Write(b []byte) (n int, err error) {
+	if !w.hasWritten {
+		w.innerW.Header().Set("Content-Type", w.mediaType)
+		w.innerW.WriteHeader(w.statusCode)
+		w.hasWritten = true
+	}
+
+	return w.innerW.Write(b)
+}
+
 // WriteObject renders a returned runtime.Object to the response as a stream or an encoded object. If the object
 // returned by the response implements rest.ResourceStreamer that interface will be used to render the
 // response. The Accept header and current API version will be passed in, and the output will be copied
@@ -90,12 +110,11 @@ func StreamObject(statusCode int, gv schema.GroupVersion, s runtime.NegotiatedSe
 
 // SerializeObject renders an object in the content type negotiated by the client using the provided encoder.
 // The context is optional and can be nil.
-func SerializeObject(mediaType string, encoder runtime.Encoder, w http.ResponseWriter, req *http.Request, statusCode int, object runtime.Object) {
-	w.Header().Set("Content-Type", mediaType)
-	w.WriteHeader(statusCode)
+func SerializeObject(mediaType string, encoder runtime.Encoder, innerW http.ResponseWriter, req *http.Request, statusCode int, object runtime.Object) {
+	w := httpResponseWriterWithInit{mediaType: mediaType, innerW: innerW, statusCode: statusCode}
 
 	if err := encoder.Encode(object, w); err != nil {
-		errorJSONFatal(err, encoder, w)
+		errSerializationFatal(err, encoder, w)
 	}
 }
 
@@ -143,22 +162,23 @@ func ErrorNegotiated(err error, s runtime.NegotiatedSerializer, gv schema.GroupV
 	return code
 }
 
-// errorJSONFatal renders an error to the response, and if codec fails will render plaintext.
+// errSerializationFatal renders an error to the response, and if codec fails will render plaintext.
 // Returns the HTTP status code of the error.
-func errorJSONFatal(err error, codec runtime.Encoder, w http.ResponseWriter) int {
+func errSerializationFatal(err error, codec runtime.Encoder, w httpResponseWriterWithInit) {
 	utilruntime.HandleError(fmt.Errorf("apiserver was unable to write a JSON response: %v", err))
 	status := ErrorToAPIStatus(err)
-	code := int(status.Code)
+	candidateStatusCode := int(status.Code)
+	// If original statusCode was not successful, we need to return the original error.
+	// We cannot hide it behind serialization problems
+	if w.statusCode >= http.StatusOK && w.statusCode < http.StatusBadRequest {
+		w.statusCode = candidateStatusCode
+	}
 	output, err := runtime.Encode(codec, status)
 	if err != nil {
-		w.WriteHeader(code)
-		fmt.Fprintf(w, "%s: %s", status.Reason, status.Message)
-		return code
+		w.mediaType = "text/plain"
+		output = []byte(fmt.Sprintf("%s: %s", status.Reason, status.Message))
 	}
-	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(code)
 	w.Write(output)
-	return code
 }
 
 // WriteRawJSON writes a non-API object in JSON.