apiserver: decorate http.ResponseWriter correctly

[tkashem]

What type of PR is this?

/kind bug

What this PR does / why we need it:

today there are four handlers that decoratehttp.ResponseWriter in apiserver filter chain:

• httplog
• timeout
• audit
• metrics

Additional interfaces like http.Flusher, http.Hijacker, http.CloseNotifier are also implemented by the basic response writer object provided to the request handler by net/http.

we want to establish an invariant that the decorator extends only the interfaces implemented by the original response writer object, so that check like the following always gives the correct answer:

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/watch.go#L177-L183

	flusher, ok := w.(http.Flusher)
	if !ok {
		err := fmt.Errorf("unable to start watch - can't get http.Flusher: %#v", w)
		utilruntime.HandleError(err)
		s.Scope.err(errors.NewInternalError(err), w, req)
		return
	}

our decorators have logic like this (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go#L490-L498)

func(w http.ResponseWriter, req *http.Request) {
                delegate := &ResponseWriterDelegator{ResponseWriter: w}

		_, closenotifiable := response.ResponseWriter.(http.CloseNotifier)
		_, flushable := response.ResponseWriter.(http.Flusher)
		_, hijackable := response.ResponseWriter.(http.Hijacker)
		var rw http.ResponseWriter
		if closenotifiable && flushable && hijackable {
			rw = &fancyResponseWriterDelegator{delegate}
		} else {
			rw = delegate
		}
}

for an http2 request, if closenotifiable && flushable && hijackable will never be true, since http2 does not implement http.Hijacker. The following is from go net/http doc:

The default ResponseWriter for HTTP/1.x connections supports Hijacker, but HTTP/2 connections 
intentionally do not. Handlers should always test for this ability at runtime.

• http/1.x request: closenotifiable && flushable && hijackable is true
• http2 request: closenotifiable && flushable is true

so the decorator drops Flusher and we see the following error for watch request if the decorator is used:

E0824 18:47:56.493042   56565 watch.go:180] unable to start watch - can't get http.Flusher: 
&metrics.ResponseWriterDelegator{ResponseWriter:(*http2.responseWriter)(0xc00efb3e30), status:0, written:0, 
wroteHeader:false}

Today, watch.go works around this issue by skipping the decorators and looking at the original response writer object provided by net/http
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/watch.go#L169

w = httplog.Unlogged(req, w)

This poses a couple of issues:

• we always need to initialize httplog constructs even though log level is below 3. This is blocking apiserver: construct logger for httplog only when log level is 3 #104557. We basically want to optimize httplog - initialize only when we need to.
• also, status code for watch requests do not show up in audit log or metric since the decorators are bypassed. Following is an output of request counter for WATCH :

apiserver_request_total{code="0",component="apiserver",dry_run="",group="",resource="configmaps",
scope="namespace",subresource="",verb="WATCH",version="v1"} 37

code is always 0 (we could not tap into the status code because decorators were skipped)

This PR ensures that:

• decorators do not "dummy" implement any interface that is not implemented by the original response writer object
• the handler can reliably do check similar to flusher, ok := w.(http.Flusher), we won't need to retrieve the original response writer object.
• all decorators get executed as intended (because they will no longer be skipped).

Which issue(s) this PR fixes:

Special notes for your reviewer:

If folks have an interest to have a reusable abstraction for this i am happy to look into it - I tried a few patterns but was not very happy with those.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[tkashem]

@wojtek-t @sttts can you please look at it when you have some time

/assign @wojtek-t @sttts

[tkashem]

cc @MikeSpreitzer

[tkashem]

httplog.Unlogged is called in 3 more places, we will need to extend the decorator so that the handler logic can get to the original http.ResponseWriter object from the decorator, thus eliminating the strong coupling we have with http.Unlogged

[MikeSpreitzer]

Yes, I think we should create and use a generic abstraction for this decoration. I tried drafting that, have a look at #104943 .

[tkashem]

Yes, I think we should create and use a generic abstraction for this decoration. I tried drafting that, have a look at #104943 .

@MikeSpreitzer thanks, I initially tried a similar pattern - I will update the PR with a reusable abstraction.

[MikeSpreitzer]

/LGTM
Thanks!

[tkashem]

Needs an approval from kubelet, @sjenning please look at it when you have some time

/assign @sjenning

[sttts]

why is this removal fine?

[sttts]

/lgtm
/approve

[tkashem]

/assign @mrunalp

[mrunalp]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MikeSpreitzer, mrunalp, sttts, tkashem

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:

• pkg/kubelet/OWNERS [mrunalp]
• staging/src/k8s.io/apiserver/OWNERS [sttts]
• vendor/OWNERS [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sttts]

/retest

[leoryu]

Will this pr be merged in v1.22.x?

[tkashem]

Will this pr be merged in v1.22.x?

It's not small enough or critical enough to be back ported to 1.22.x. Is there any particular reason you want it in 1.22.x?

[leoryu]

Will this pr be merged in v1.22.x?

It's not small enough or critical enough to be back ported to 1.22.x. Is there any particular reason you want it in 1.22.x?

My project is using Aggregation API to expand k8s, and it imports apiserver. After I update apiserver version to v0.22.3, I got err logs which is mentioned in this PR.