diff --git a/pkg/endpoints/filters/audit.go b/pkg/endpoints/filters/audit.go index fe32ad363..5a97c7d47 100644 --- a/pkg/endpoints/filters/audit.go +++ b/pkg/endpoints/filters/audit.go @@ -32,6 +32,7 @@ import ( "k8s.io/apiserver/pkg/audit" "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/endpoints/responsewriter" ) // WithAudit decorates a http.Handler with audit logging information for all the @@ -178,19 +179,11 @@ func decorateResponseWriter(ctx context.Context, responseWriter http.ResponseWri omitStages: omitStages, } - // check if the ResponseWriter we're wrapping is the fancy one we need - // or if the basic is sufficient - //lint:file-ignore SA1019 Keep supporting deprecated http.CloseNotifier - _, cn := responseWriter.(http.CloseNotifier) - _, fl := responseWriter.(http.Flusher) - _, hj := responseWriter.(http.Hijacker) - if cn && fl && hj { - return &fancyResponseWriterDelegator{delegate} - } - return delegate + return responsewriter.WrapForHTTP1Or2(delegate) } var _ http.ResponseWriter = &auditResponseWriter{} +var _ responsewriter.UserProvidedDecorator = &auditResponseWriter{} // auditResponseWriter intercepts WriteHeader, sets it in the event. If the sink is set, it will // create immediately an event (for long running requests). @@ -203,6 +196,10 @@ type auditResponseWriter struct { omitStages []auditinternal.Stage } +func (a *auditResponseWriter) Unwrap() http.ResponseWriter { + return a.ResponseWriter +} + func (a *auditResponseWriter) processCode(code int) { a.once.Do(func() { if a.event.ResponseStatus == nil { @@ -228,28 +225,11 @@ func (a *auditResponseWriter) WriteHeader(code int) { a.ResponseWriter.WriteHeader(code) } -// fancyResponseWriterDelegator implements http.CloseNotifier, http.Flusher and -// http.Hijacker which are needed to make certain http operation (e.g. watch, rsh, etc) -// working. -type fancyResponseWriterDelegator struct { - *auditResponseWriter -} - -func (f *fancyResponseWriterDelegator) CloseNotify() <-chan bool { - return f.ResponseWriter.(http.CloseNotifier).CloseNotify() -} - -func (f *fancyResponseWriterDelegator) Flush() { - f.ResponseWriter.(http.Flusher).Flush() -} - -func (f *fancyResponseWriterDelegator) Hijack() (net.Conn, *bufio.ReadWriter, error) { +func (a *auditResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { // fake a response status before protocol switch happens - f.processCode(http.StatusSwitchingProtocols) + a.processCode(http.StatusSwitchingProtocols) - return f.ResponseWriter.(http.Hijacker).Hijack() + // the outer ResponseWriter object returned by WrapForHTTP1Or2 implements + // http.Hijacker if the inner object (a.ResponseWriter) implements http.Hijacker. + return a.ResponseWriter.(http.Hijacker).Hijack() } - -var _ http.CloseNotifier = &fancyResponseWriterDelegator{} -var _ http.Flusher = &fancyResponseWriterDelegator{} -var _ http.Hijacker = &fancyResponseWriterDelegator{} diff --git a/pkg/endpoints/filters/audit_test.go b/pkg/endpoints/filters/audit_test.go index 7c622e978..04199373d 100644 --- a/pkg/endpoints/filters/audit_test.go +++ b/pkg/endpoints/filters/audit_test.go @@ -17,9 +17,7 @@ limitations under the License. package filters import ( - "bufio" "context" - "net" "net/http" "net/http/httptest" "reflect" @@ -37,6 +35,7 @@ import ( "k8s.io/apiserver/pkg/audit/policy" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/endpoints/responsewriter" ) type fakeAuditSink struct { @@ -75,43 +74,46 @@ func (s *fakeAuditSink) Pop(timeout time.Duration) (*auditinternal.Event, error) return result, err } -type simpleResponseWriter struct{} - -var _ http.ResponseWriter = &simpleResponseWriter{} - -func (*simpleResponseWriter) WriteHeader(code int) {} -func (*simpleResponseWriter) Write(bs []byte) (int, error) { return len(bs), nil } -func (*simpleResponseWriter) Header() http.Header { return http.Header{} } - -type fancyResponseWriter struct { - simpleResponseWriter -} - -func (*fancyResponseWriter) CloseNotify() <-chan bool { return nil } - -func (*fancyResponseWriter) Flush() {} - -func (*fancyResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { return nil, nil, nil } - func TestConstructResponseWriter(t *testing.T) { - actual := decorateResponseWriter(context.Background(), &simpleResponseWriter{}, nil, nil, nil) + inner := &responsewriter.FakeResponseWriter{} + actual := decorateResponseWriter(context.Background(), inner, nil, nil, nil) switch v := actual.(type) { case *auditResponseWriter: default: t.Errorf("Expected auditResponseWriter, got %v", reflect.TypeOf(v)) } + if innerGot := actual.(responsewriter.UserProvidedDecorator).Unwrap(); inner != innerGot { + t.Errorf("Expected the decorator to return the inner http.ResponseWriter object") + } - actual = decorateResponseWriter(context.Background(), &fancyResponseWriter{}, nil, nil, nil) - switch v := actual.(type) { - case *fancyResponseWriterDelegator: - default: - t.Errorf("Expected fancyResponseWriterDelegator, got %v", reflect.TypeOf(v)) + actual = decorateResponseWriter(context.Background(), &responsewriter.FakeResponseWriterFlusherCloseNotifier{}, nil, nil, nil) + //lint:file-ignore SA1019 Keep supporting deprecated http.CloseNotifier + if _, ok := actual.(http.CloseNotifier); !ok { + t.Errorf("Expected http.ResponseWriter to implement http.CloseNotifier") + } + if _, ok := actual.(http.Flusher); !ok { + t.Errorf("Expected the wrapper to implement http.Flusher") + } + if _, ok := actual.(http.Hijacker); ok { + t.Errorf("Expected http.ResponseWriter not to implement http.Hijacker") + } + + actual = decorateResponseWriter(context.Background(), &responsewriter.FakeResponseWriterFlusherCloseNotifierHijacker{}, nil, nil, nil) + //lint:file-ignore SA1019 Keep supporting deprecated http.CloseNotifier + if _, ok := actual.(http.CloseNotifier); !ok { + t.Errorf("Expected http.ResponseWriter to implement http.CloseNotifier") + } + if _, ok := actual.(http.Flusher); !ok { + t.Errorf("Expected the wrapper to implement http.Flusher") + } + if _, ok := actual.(http.Hijacker); !ok { + t.Errorf("Expected http.ResponseWriter to implement http.Hijacker") } } func TestDecorateResponseWriterWithoutChannel(t *testing.T) { ev := &auditinternal.Event{} - actual := decorateResponseWriter(context.Background(), &simpleResponseWriter{}, ev, nil, nil) + actual := decorateResponseWriter(context.Background(), &responsewriter.FakeResponseWriter{}, ev, nil, nil) // write status. This will not block because firstEventSentCh is nil actual.WriteHeader(42) @@ -125,7 +127,7 @@ func TestDecorateResponseWriterWithoutChannel(t *testing.T) { func TestDecorateResponseWriterWithImplicitWrite(t *testing.T) { ev := &auditinternal.Event{} - actual := decorateResponseWriter(context.Background(), &simpleResponseWriter{}, ev, nil, nil) + actual := decorateResponseWriter(context.Background(), &responsewriter.FakeResponseWriter{}, ev, nil, nil) // write status. This will not block because firstEventSentCh is nil actual.Write([]byte("foo")) @@ -140,7 +142,7 @@ func TestDecorateResponseWriterWithImplicitWrite(t *testing.T) { func TestDecorateResponseWriterChannel(t *testing.T) { sink := &fakeAuditSink{} ev := &auditinternal.Event{} - actual := decorateResponseWriter(context.Background(), &simpleResponseWriter{}, ev, sink, nil) + actual := decorateResponseWriter(context.Background(), &responsewriter.FakeResponseWriter{}, ev, sink, nil) done := make(chan struct{}) go func() { diff --git a/pkg/endpoints/handlers/watch.go b/pkg/endpoints/handlers/watch.go index 47fe44b78..818d94ae4 100644 --- a/pkg/endpoints/handlers/watch.go +++ b/pkg/endpoints/handlers/watch.go @@ -31,7 +31,6 @@ import ( "k8s.io/apimachinery/pkg/watch" "k8s.io/apiserver/pkg/endpoints/handlers/negotiation" "k8s.io/apiserver/pkg/endpoints/metrics" - "k8s.io/apiserver/pkg/server/httplog" "k8s.io/apiserver/pkg/util/wsstream" "golang.org/x/net/websocket" @@ -166,8 +165,6 @@ func (s *WatchServer) ServeHTTP(w http.ResponseWriter, req *http.Request) { metrics.RegisteredWatchers.WithContext(req.Context()).WithLabelValues(kind.Group, kind.Version, kind.Kind).Inc() defer metrics.RegisteredWatchers.WithContext(req.Context()).WithLabelValues(kind.Group, kind.Version, kind.Kind).Dec() - w = httplog.Unlogged(req, w) - if wsstream.IsWebSocketRequest(req) { w.Header().Set("Content-Type", s.MediaType) websocket.Handler(s.HandleWS).ServeHTTP(w, req) diff --git a/pkg/endpoints/metrics/metrics.go b/pkg/endpoints/metrics/metrics.go index cb08facff..46e92404c 100644 --- a/pkg/endpoints/metrics/metrics.go +++ b/pkg/endpoints/metrics/metrics.go @@ -17,9 +17,7 @@ limitations under the License. package metrics import ( - "bufio" "context" - "net" "net/http" "net/url" "strconv" @@ -34,6 +32,7 @@ import ( "k8s.io/apiserver/pkg/audit" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/endpoints/responsewriter" "k8s.io/apiserver/pkg/features" utilfeature "k8s.io/apiserver/pkg/util/feature" compbasemetrics "k8s.io/component-base/metrics" @@ -486,16 +485,7 @@ func InstrumentRouteFunc(verb, group, version, resource, subresource, scope, com delegate := &ResponseWriterDelegator{ResponseWriter: response.ResponseWriter} - //lint:file-ignore SA1019 Keep supporting deprecated http.CloseNotifier - _, cn := response.ResponseWriter.(http.CloseNotifier) - _, fl := response.ResponseWriter.(http.Flusher) - _, hj := response.ResponseWriter.(http.Hijacker) - var rw http.ResponseWriter - if cn && fl && hj { - rw = &fancyResponseWriterDelegator{delegate} - } else { - rw = delegate - } + rw := responsewriter.WrapForHTTP1Or2(delegate) response.ResponseWriter = rw routeFunc(req, response) @@ -513,15 +503,7 @@ func InstrumentHandlerFunc(verb, group, version, resource, subresource, scope, c } delegate := &ResponseWriterDelegator{ResponseWriter: w} - - _, cn := w.(http.CloseNotifier) - _, fl := w.(http.Flusher) - _, hj := w.(http.Hijacker) - if cn && fl && hj { - w = &fancyResponseWriterDelegator{delegate} - } else { - w = delegate - } + w = responsewriter.WrapForHTTP1Or2(delegate) handler(w, req) @@ -612,6 +594,9 @@ func cleanDryRun(u *url.URL) string { return strings.Join(utilsets.NewString(dryRun...).List(), ",") } +var _ http.ResponseWriter = (*ResponseWriterDelegator)(nil) +var _ responsewriter.UserProvidedDecorator = (*ResponseWriterDelegator)(nil) + // ResponseWriterDelegator interface wraps http.ResponseWriter to additionally record content-length, status-code, etc. type ResponseWriterDelegator struct { http.ResponseWriter @@ -621,6 +606,10 @@ type ResponseWriterDelegator struct { wroteHeader bool } +func (r *ResponseWriterDelegator) Unwrap() http.ResponseWriter { + return r.ResponseWriter +} + func (r *ResponseWriterDelegator) WriteHeader(code int) { r.status = code r.wroteHeader = true @@ -644,22 +633,6 @@ func (r *ResponseWriterDelegator) ContentLength() int { return int(r.written) } -type fancyResponseWriterDelegator struct { - *ResponseWriterDelegator -} - -func (f *fancyResponseWriterDelegator) CloseNotify() <-chan bool { - return f.ResponseWriter.(http.CloseNotifier).CloseNotify() -} - -func (f *fancyResponseWriterDelegator) Flush() { - f.ResponseWriter.(http.Flusher).Flush() -} - -func (f *fancyResponseWriterDelegator) Hijack() (net.Conn, *bufio.ReadWriter, error) { - return f.ResponseWriter.(http.Hijacker).Hijack() -} - // Small optimization over Itoa func codeToString(s int) string { switch s { diff --git a/pkg/endpoints/metrics/metrics_test.go b/pkg/endpoints/metrics/metrics_test.go index 3728b4a5f..a120e9ed1 100644 --- a/pkg/endpoints/metrics/metrics_test.go +++ b/pkg/endpoints/metrics/metrics_test.go @@ -22,6 +22,7 @@ import ( "testing" "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/endpoints/responsewriter" ) func TestCleanVerb(t *testing.T) { @@ -181,3 +182,14 @@ func TestCleanScope(t *testing.T) { }) } } + +func TestResponseWriterDecorator(t *testing.T) { + decorator := &ResponseWriterDelegator{ + ResponseWriter: &responsewriter.FakeResponseWriter{}, + } + var w http.ResponseWriter = decorator + + if inner := w.(responsewriter.UserProvidedDecorator).Unwrap(); inner != decorator.ResponseWriter { + t.Errorf("Expected the decorator to return the inner http.ResponseWriter object") + } +} diff --git a/pkg/server/filters/timeout.go b/pkg/server/filters/timeout.go index 69e4fd4f2..7487fbf1e 100644 --- a/pkg/server/filters/timeout.go +++ b/pkg/server/filters/timeout.go @@ -30,6 +30,7 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apiserver/pkg/endpoints/metrics" apirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/endpoints/responsewriter" ) // WithTimeoutForNonLongRunningRequests times out non-long-running requests after the time given by timeout. @@ -90,7 +91,8 @@ func (t *timeoutHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // resultCh is used as both errCh and stopCh resultCh := make(chan interface{}) - tw := newTimeoutWriter(w) + var tw timeoutWriter + tw, w = newTimeoutWriter(w) go func() { defer func() { err := recover() @@ -105,7 +107,7 @@ func (t *timeoutHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } resultCh <- err }() - t.handler.ServeHTTP(tw, r) + t.handler.ServeHTTP(w, r) }() select { case err := <-resultCh: @@ -146,24 +148,16 @@ type timeoutWriter interface { timeout(*apierrors.StatusError) } -func newTimeoutWriter(w http.ResponseWriter) timeoutWriter { +func newTimeoutWriter(w http.ResponseWriter) (timeoutWriter, http.ResponseWriter) { base := &baseTimeoutWriter{w: w} + wrapped := responsewriter.WrapForHTTP1Or2(base) - _, notifiable := w.(http.CloseNotifier) - _, hijackable := w.(http.Hijacker) - - switch { - case notifiable && hijackable: - return &closeHijackTimeoutWriter{base} - case notifiable: - return &closeTimeoutWriter{base} - case hijackable: - return &hijackTimeoutWriter{base} - default: - return base - } + return base, wrapped } +var _ http.ResponseWriter = &baseTimeoutWriter{} +var _ responsewriter.UserProvidedDecorator = &baseTimeoutWriter{} + type baseTimeoutWriter struct { w http.ResponseWriter @@ -176,6 +170,10 @@ type baseTimeoutWriter struct { hijacked bool } +func (tw *baseTimeoutWriter) Unwrap() http.ResponseWriter { + return tw.w +} + func (tw *baseTimeoutWriter) Header() http.Header { tw.mu.Lock() defer tw.mu.Unlock() @@ -210,9 +208,9 @@ func (tw *baseTimeoutWriter) Flush() { return } - if flusher, ok := tw.w.(http.Flusher); ok { - flusher.Flush() - } + // the outer ResponseWriter object returned by WrapForHTTP1Or2 implements + // http.Flusher if the inner object (tw.w) implements http.Flusher. + tw.w.(http.Flusher).Flush() } func (tw *baseTimeoutWriter) WriteHeader(code int) { @@ -259,7 +257,7 @@ func (tw *baseTimeoutWriter) timeout(err *apierrors.StatusError) { } } -func (tw *baseTimeoutWriter) closeNotify() <-chan bool { +func (tw *baseTimeoutWriter) CloseNotify() <-chan bool { tw.mu.Lock() defer tw.mu.Unlock() @@ -269,47 +267,24 @@ func (tw *baseTimeoutWriter) closeNotify() <-chan bool { return done } + // the outer ResponseWriter object returned by WrapForHTTP1Or2 implements + // http.CloseNotifier if the inner object (tw.w) implements http.CloseNotifier. return tw.w.(http.CloseNotifier).CloseNotify() } -func (tw *baseTimeoutWriter) hijack() (net.Conn, *bufio.ReadWriter, error) { +func (tw *baseTimeoutWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { tw.mu.Lock() defer tw.mu.Unlock() if tw.timedOut { return nil, nil, http.ErrHandlerTimeout } + + // the outer ResponseWriter object returned by WrapForHTTP1Or2 implements + // http.Hijacker if the inner object (tw.w) implements http.Hijacker. conn, rw, err := tw.w.(http.Hijacker).Hijack() if err == nil { tw.hijacked = true } return conn, rw, err } - -type closeTimeoutWriter struct { - *baseTimeoutWriter -} - -func (tw *closeTimeoutWriter) CloseNotify() <-chan bool { - return tw.closeNotify() -} - -type hijackTimeoutWriter struct { - *baseTimeoutWriter -} - -func (tw *hijackTimeoutWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { - return tw.hijack() -} - -type closeHijackTimeoutWriter struct { - *baseTimeoutWriter -} - -func (tw *closeHijackTimeoutWriter) CloseNotify() <-chan bool { - return tw.closeNotify() -} - -func (tw *closeHijackTimeoutWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { - return tw.hijack() -} diff --git a/pkg/server/filters/timeout_test.go b/pkg/server/filters/timeout_test.go index 56220883b..32682c713 100644 --- a/pkg/server/filters/timeout_test.go +++ b/pkg/server/filters/timeout_test.go @@ -45,6 +45,7 @@ import ( "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/endpoints/responsewriter" "k8s.io/klog/v2" ) @@ -368,6 +369,17 @@ func TestErrConnKilledHTTP2(t *testing.T) { } } +func TestResponseWriterDecorator(t *testing.T) { + decorator := &baseTimeoutWriter{ + w: &responsewriter.FakeResponseWriter{}, + } + var w http.ResponseWriter = decorator + + if inner := w.(responsewriter.UserProvidedDecorator).Unwrap(); inner != decorator.w { + t.Errorf("Expected the decorator to return the inner http.ResponseWriter object") + } +} + func isStackTraceLoggedByRuntime(message string) bool { // Check the captured output for the following patterns to find out if the // stack trace is included in the log: diff --git a/pkg/server/healthz/healthz.go b/pkg/server/healthz/healthz.go index eb288239c..e19505e9c 100644 --- a/pkg/server/healthz/healthz.go +++ b/pkg/server/healthz/healthz.go @@ -29,7 +29,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/endpoints/metrics" - "k8s.io/apiserver/pkg/server/httplog" + "k8s.io/apiserver/pkg/endpoints/responsewriter" "k8s.io/klog/v2" ) @@ -255,7 +255,7 @@ func handleRootHealth(name string, firstTimeHealthy func(), checks ...HealthChec // always be verbose on failure if len(failedChecks) > 0 { klog.V(2).Infof("%s check failed: %s\n%v", strings.Join(failedChecks, ","), name, failedVerboseLogOutput.String()) - http.Error(httplog.Unlogged(r, w), fmt.Sprintf("%s%s check failed", individualCheckOutput.String(), name), http.StatusInternalServerError) + http.Error(responsewriter.GetOriginal(w), fmt.Sprintf("%s%s check failed", individualCheckOutput.String(), name), http.StatusInternalServerError) return } diff --git a/pkg/server/httplog/httplog.go b/pkg/server/httplog/httplog.go index 40a243bcc..a1e1ad97f 100644 --- a/pkg/server/httplog/httplog.go +++ b/pkg/server/httplog/httplog.go @@ -28,6 +28,7 @@ import ( "k8s.io/apiserver/pkg/endpoints/metrics" "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/endpoints/responsewriter" "k8s.io/klog/v2" ) @@ -66,12 +67,16 @@ type respLogger struct { logStacktracePred StacktracePred } +var _ http.ResponseWriter = &respLogger{} +var _ responsewriter.UserProvidedDecorator = &respLogger{} + +func (rl *respLogger) Unwrap() http.ResponseWriter { + return rl.w +} + // Simple logger that logs immediately when Addf is called type passthroughLogger struct{} -//lint:ignore SA1019 Interface implementation check to make sure we don't drop CloseNotifier again -var _ http.CloseNotifier = &respLogger{} - // Addf logs info immediately. func (passthroughLogger) Addf(format string, data ...interface{}) { klog.V(2).Info(fmt.Sprintf(format, data...)) @@ -95,13 +100,15 @@ func WithLogging(handler http.Handler, pred StacktracePred) http.Handler { startTime = receivedTimestamp } - rl := newLoggedWithStartTime(req, w, startTime).StacktraceWhen(pred) + var rl *respLogger + rl, w = newLoggedWithStartTime(req, w, startTime) + rl.StacktraceWhen(pred) req = req.WithContext(context.WithValue(ctx, respLoggerContextKey, rl)) if klog.V(3).Enabled() { defer rl.Log() } - handler.ServeHTTP(rl, req) + handler.ServeHTTP(w, req) }) } @@ -118,17 +125,20 @@ func respLoggerFromRequest(req *http.Request) *respLogger { return respLoggerFromContext(req.Context()) } -func newLoggedWithStartTime(req *http.Request, w http.ResponseWriter, startTime time.Time) *respLogger { - return &respLogger{ +func newLoggedWithStartTime(req *http.Request, w http.ResponseWriter, startTime time.Time) (*respLogger, http.ResponseWriter) { + logger := &respLogger{ startTime: startTime, req: req, w: w, logStacktracePred: DefaultStacktracePred, } + + rw := responsewriter.WrapForHTTP1Or2(logger) + return logger, rw } // newLogged turns a normal response writer into a logged response writer. -func newLogged(req *http.Request, w http.ResponseWriter) *respLogger { +func newLogged(req *http.Request, w http.ResponseWriter) (*respLogger, http.ResponseWriter) { return newLoggedWithStartTime(req, w, time.Now()) } @@ -253,32 +263,18 @@ func (rl *respLogger) Write(b []byte) (int, error) { return rl.w.Write(b) } -// Flush implements http.Flusher even if the underlying http.Writer doesn't implement it. -// Flush is used for streaming purposes and allows to flush buffered data to the client. -func (rl *respLogger) Flush() { - if flusher, ok := rl.w.(http.Flusher); ok { - flusher.Flush() - } else if klog.V(2).Enabled() { - klog.InfoDepth(1, fmt.Sprintf("Unable to convert %+v into http.Flusher", rl.w)) - } -} - // WriteHeader implements http.ResponseWriter. func (rl *respLogger) WriteHeader(status int) { rl.recordStatus(status) rl.w.WriteHeader(status) } -// Hijack implements http.Hijacker. func (rl *respLogger) Hijack() (net.Conn, *bufio.ReadWriter, error) { rl.hijacked = true - return rl.w.(http.Hijacker).Hijack() -} -// CloseNotify implements http.CloseNotifier -func (rl *respLogger) CloseNotify() <-chan bool { - //lint:ignore SA1019 There are places in the code base requiring the CloseNotifier interface to be implemented. - return rl.w.(http.CloseNotifier).CloseNotify() + // the outer ResponseWriter object returned by WrapForHTTP1Or2 implements + // http.Hijacker if the inner object (rl.w) implements http.Hijacker. + return rl.w.(http.Hijacker).Hijack() } func (rl *respLogger) recordStatus(status int) { diff --git a/pkg/server/httplog/httplog_test.go b/pkg/server/httplog/httplog_test.go index 62f5ed76d..845f85075 100644 --- a/pkg/server/httplog/httplog_test.go +++ b/pkg/server/httplog/httplog_test.go @@ -21,6 +21,8 @@ import ( "net/http/httptest" "reflect" "testing" + + "k8s.io/apiserver/pkg/endpoints/responsewriter" ) func TestDefaultStacktracePred(t *testing.T) { @@ -127,28 +129,22 @@ func TestUnlogged(t *testing.T) { } } -type testResponseWriter struct{} - -func (*testResponseWriter) Header() http.Header { return nil } -func (*testResponseWriter) Write([]byte) (int, error) { return 0, nil } -func (*testResponseWriter) WriteHeader(int) {} - func TestLoggedStatus(t *testing.T) { req, err := http.NewRequest("GET", "http://example.com", nil) if err != nil { t.Errorf("unexpected error: %v", err) } - var tw http.ResponseWriter = new(testResponseWriter) - logger := newLogged(req, tw) + var tw http.ResponseWriter = new(responsewriter.FakeResponseWriter) + logger, _ := newLogged(req, tw) logger.Write(nil) if logger.status != http.StatusOK { t.Errorf("expected status after write to be %v, got %v", http.StatusOK, logger.status) } - tw = new(testResponseWriter) - logger = newLogged(req, tw) + tw = new(responsewriter.FakeResponseWriter) + logger, _ = newLogged(req, tw) logger.WriteHeader(http.StatusForbidden) logger.Write(nil) @@ -156,3 +152,58 @@ func TestLoggedStatus(t *testing.T) { t.Errorf("expected status after write to remain %v, got %v", http.StatusForbidden, logger.status) } } + +func TestRespLoggerWithDecoratedResponseWriter(t *testing.T) { + req, err := http.NewRequest("GET", "http://example.com", nil) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + var tw http.ResponseWriter = new(responsewriter.FakeResponseWriter) + _, rwGot := newLogged(req, tw) + + switch v := rwGot.(type) { + case *respLogger: + default: + t.Errorf("Expected respLogger, got %v", reflect.TypeOf(v)) + } + + tw = new(responsewriter.FakeResponseWriterFlusherCloseNotifier) + _, rwGot = newLogged(req, tw) + + //lint:file-ignore SA1019 Keep supporting deprecated http.CloseNotifier + if _, ok := rwGot.(http.CloseNotifier); !ok { + t.Errorf("Expected http.ResponseWriter to implement http.CloseNotifier") + } + if _, ok := rwGot.(http.Flusher); !ok { + t.Errorf("Expected the wrapper to implement http.Flusher") + } + if _, ok := rwGot.(http.Hijacker); ok { + t.Errorf("Expected http.ResponseWriter not to implement http.Hijacker") + } + + tw = new(responsewriter.FakeResponseWriterFlusherCloseNotifierHijacker) + _, rwGot = newLogged(req, tw) + + //lint:file-ignore SA1019 Keep supporting deprecated http.CloseNotifier + if _, ok := rwGot.(http.CloseNotifier); !ok { + t.Errorf("Expected http.ResponseWriter to implement http.CloseNotifier") + } + if _, ok := rwGot.(http.Flusher); !ok { + t.Errorf("Expected the wrapper to implement http.Flusher") + } + if _, ok := rwGot.(http.Hijacker); !ok { + t.Errorf("Expected http.ResponseWriter to implement http.Hijacker") + } +} + +func TestResponseWriterDecorator(t *testing.T) { + decorator := &respLogger{ + w: &responsewriter.FakeResponseWriter{}, + } + var w http.ResponseWriter = decorator + + if inner := w.(responsewriter.UserProvidedDecorator).Unwrap(); inner != decorator.w { + t.Errorf("Expected the decorator to return the inner http.ResponseWriter object") + } +}