Repository: knative/pkg
Pull request #1355 (2020-05-26 14:46:19)

Kubelet probes would result in the webhook writing the HTTP status twice
Doesn't seem like it affected anything - just writes out some extra
log messages

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1355#" title="Author self-approved">dprotaso</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/OWNERS)~~ [dprotaso]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[webhook/webhook.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1355/pull-knative-pkg-go-coverage/1265293300978421760/artifacts/line-cov.html#file0) | 83.6% | 86.8% | 3.2

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[webhook/webhook.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1355/pull-knative-pkg-go-coverage/1265297830751244289/artifacts/line-cov.html#file0) | 83.6% | 86.8% | 3.2

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[webhook/webhook.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1355/pull-knative-pkg-go-coverage/1265300473791909888/artifacts/line-cov.html#file0) | 83.6% | 86.8% | 3.2

dprotaso: /hold cancel

Patch:
diff --git a/webhook/webhook.go b/webhook/webhook.go
index e29ea9611..ef7c238da 100644
--- a/webhook/webhook.go
+++ b/webhook/webhook.go
@@ -253,6 +253,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		default:
 			w.WriteHeader(http.StatusOK)
 		}
+		return
 	}
 
 	// Verify the content type is accurate.


Test patch:
diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go
index bcdef62d9..19ce13a10 100644
--- a/webhook/webhook_test.go
+++ b/webhook/webhook_test.go
@@ -20,6 +20,8 @@ import (
 	"context"
 	"fmt"
 	"net"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 	"time"
 
@@ -96,3 +98,51 @@ func TestRegistrationStopChanFire(t *testing.T) {
 		t.Errorf("Unexpected success to dial to port %d", opts.Port)
 	}
 }
+
+func TestWebhookKubeletProbe(t *testing.T) {
+	opts := newDefaultOptions()
+	ctx, webhook, cancel := newNonRunningTestWebhook(t, opts)
+	defer cancel()
+
+	recorder := bombRecorder{ResponseRecorder: httptest.NewRecorder()}
+	probeReq := httptest.NewRequest("GET", "/", nil)
+	probeReq.Header.Set("User-Agent", "kube-probe/1.16")
+
+	webhook.ServeHTTP(&recorder, probeReq)
+
+	if got, want := recorder.Code, http.StatusOK; got != want {
+		t.Fatalf("Probe got HTTP status %d - expected %d", got, want)
+	}
+
+	if got, want := recorder.writeCount, 1; got != want {
+		t.Errorf("HTTP status was written %d times - expected only one write", got)
+	}
+
+	// Stop the webhook - which means probes should fail
+	//
+	// The steps below aren't obvious and requires you to
+	// know the implementation details
+	cancel()
+	webhook.Run(ctx.Done())
+
+	recorder = bombRecorder{ResponseRecorder: httptest.NewRecorder()}
+	webhook.ServeHTTP(&recorder, probeReq)
+
+	if got, want := recorder.Code, http.StatusInternalServerError; got != want {
+		t.Fatalf("Probe got HTTP status %d - expected %d", got, want)
+	}
+
+	if got, want := recorder.writeCount, 1; got != want {
+		t.Errorf("HTTP status was written %d times - expected only one write", got)
+	}
+}
+
+type bombRecorder struct {
+	*httptest.ResponseRecorder
+	writeCount int
+}
+
+func (rw *bombRecorder) WriteHeader(code int) {
+	rw.writeCount += 1
+	rw.ResponseRecorder.WriteHeader(code)
+}