webhook: Admission metrics are not reported by status code

[ahmetb]

This is several bugs in one bug since it appears multiple things are broken. Apologies if it gets too complicated.

Problem 1: It appears that controller-runtime webhooks always respond with HTTP status code 200 OK even if the request is completely malformed.

Example (sending a malformed request to a webhook endpoint):

curl -k -v https://localhost:9443/validate-ship-example-org-v1beta1-frigate
< HTTP/2 200
< ... other response headers ...
<
{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"contentType=, expected application/json","code":400}}}

---

Problem 2: controller-runtime webhooks do not use the metav1.Status.Code and always respond with HTTP status code 200 OK.

Example code I used to register my custom handler at /test:

webhookServer.Register("/test", &webhook.Admission{Handler: &myHandler{}})
...

type myHandler struct{}
func (*myHandler) Handle(context.Context, admission.Request) admission.Response {
	return admission.Response{
		AdmissionResponse: admissionv1.AdmissionResponse{
			Allowed: false,
			Result: &metav1.Status{
				Code:    403,
				Message: "things suck",
				Status:  "Failure",
				Reason:  metav1.StatusReasonForbidden}}}
}

And here's the query I am making:

curl -XPOST -k -v -H "Content-Type: application/json" https://localhost:9443/test

< HTTP/2 200
...
<
{"kind":"AdmissionReview","apiVersion":"admission.k8s.io/v1","response":{"uid":"","allowed":false,"status":{"metadata":{},"status":"Failure","message":"things suck","reason":"Forbidden","code":403}}}

Problem 3: controller-runtime doesn't expose any metrics about rejected requests and their .status.codes. This is likely because

1. we always report code=200 no matter the response (Problem 1 & 2)
2. it appears we're capping controller_runtime_webhook_requests_total{code=...} dimension to only 200 and 500 here:

   controller-runtime/pkg/webhook/internal/metrics/metrics.go

   Lines 70 to 76 in 4cae9df

   	lat := RequestLatency.MustCurryWith(lbl)
   	cnt := RequestTotal.MustCurryWith(lbl)
   	gge := RequestInFlight.With(lbl)
   	// Initialize the most likely HTTP status codes.
   	cnt.WithLabelValues("200")
   	cnt.WithLabelValues("500")

so even if I make a faulty request, only the code=200 metric goes up:

controller_runtime_webhook_requests_total{code="200",webhook="/test"} 4

In an ideal state, I'd like to see controller_runtime_webhook_requests_total metric actually let me understand things like how many requests I'm denying (non-200 codes, such as 403, 400, 429, ...).

/kind bug

[troy0820]

I believe the 200 response is if the webhook can create the response. There was an issue around this with failure policy and not sure if this helps with your issue #2662.

[alvaroaleman]

Pretty much what troy said. I do agree that this is confusing and that it would be helpful to have metrics around the status code in the admission response.

[ahmetb]

Indeed, after creating the issue I've realized HTTP response code must be 200 regardless of the admission response (which contains the status code the apiserver would return in its response body).

Maybe we could still add a metric that parses the code out from admission.Response? (I understand we can't help if someone actually decides to implement their own http.Handler).

[ahmetb]

/remove-kind bug
/kind feature