Repository: knative/pkg
Pull request #2038 (2021-02-25 01:07:00)

Move IsKProbe to drainer, since it's the only place it's used
And hide it, since no need to export it, if nobody's going to use it.

🧹 

 /assign @tcnghia 

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

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **vagababov** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @vagababov` in a comment when ready.

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

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

- **[network/OWNERS](https://github.com/knative/pkg/blob/master/network/OWNERS)**

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":["vagababov"]} -->
knative-prow-robot: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | 3fdcc4b5b4d27d2fef1557483a27ee385342d26a | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/2038/pull-knative-pkg-unit-tests/1364743685299769344) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/2038). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2038?src=pr&el=h1) Report
> Merging [#2038](https://codecov.io/gh/knative/pkg/pull/2038?src=pr&el=desc) (3fdcc4b) into [master](https://codecov.io/gh/knative/pkg/commit/5367a43eef749917ad7c0bbee3cbd0b896887b11?el=desc) (5367a43) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/2038/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO)](https://codecov.io/gh/knative/pkg/pull/2038?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #2038   +/-   ##
=======================================
  Coverage   67.16%   67.16%           
=======================================
  Files         214      214           
  Lines        9028     9028           
=======================================
  Hits         6064     6064           
  Misses       2689     2689           
  Partials      275      275           
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2038?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [network/network.go](https://codecov.io/gh/knative/pkg/pull/2038/diff?src=pr&el=tree#diff-bmV0d29yay9uZXR3b3JrLmdv) | `100.00% <ø> (ø)` | |
| [network/handlers/drain.go](https://codecov.io/gh/knative/pkg/pull/2038/diff?src=pr&el=tree#diff-bmV0d29yay9oYW5kbGVycy9kcmFpbi5nbw==) | `83.72% <100.00%> (+0.38%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/2038?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/2038?src=pr&el=footer). Last update [5367a43...3fdcc4b](https://codecov.io/gh/knative/pkg/pull/2038?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

vagababov: /test pull-knative-pkg-unit-tests

metrics :/
vagababov: Added one more method which is only used in drain.
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2038#pullrequestreview-598175263" title="LGTM">tcnghia</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:

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

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":[]} -->

Patch:
diff --git a/network/handlers/drain.go b/network/handlers/drain.go
index 0ed37ba6d..b8dc6ff1b 100644
--- a/network/handlers/drain.go
+++ b/network/handlers/drain.go
@@ -17,6 +17,7 @@ limitations under the License.
 package handlers
 
 import (
+	"fmt"
 	"net/http"
 	"sync"
 	"time"
@@ -90,11 +91,11 @@ func (d *Drainer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 		return
 	}
-	if network.IsKProbe(r) {
+	if isKProbe(r) {
 		if d.draining() {
 			http.Error(w, "shutting down", http.StatusServiceUnavailable)
 		} else {
-			network.ServeKProbe(w, r)
+			serveKProbe(w, r)
 		}
 		return
 	}
@@ -146,3 +147,21 @@ func (d *Drainer) draining() bool {
 	defer d.RUnlock()
 	return d.timer != nil
 }
+
+// isKProbe returns true if the request is a knatvie probe.
+func isKProbe(r *http.Request) bool {
+	return r.Header.Get(network.ProbeHeaderName) == network.ProbeHeaderValue
+}
+
+// serveKProbe serve KProbe requests.
+func serveKProbe(w http.ResponseWriter, r *http.Request) {
+	hh := r.Header.Get(network.HashHeaderName)
+	if hh == "" {
+		http.Error(w,
+			fmt.Sprintf("a probe request must contain a non-empty %q header", network.HashHeaderName),
+			http.StatusBadRequest)
+		return
+	}
+	w.Header().Set(network.HashHeaderName, hh)
+	w.WriteHeader(http.StatusOK)
+}
diff --git a/network/network.go b/network/network.go
index ee5ce7c48..cf5feaa8f 100644
--- a/network/network.go
+++ b/network/network.go
@@ -17,7 +17,6 @@ limitations under the License.
 package network
 
 import (
-	"fmt"
 	"net/http"
 	"strings"
 	"time"
@@ -68,19 +67,3 @@ func IsKubeletProbe(r *http.Request) bool {
 	return strings.HasPrefix(r.Header.Get(UserAgentKey), KubeProbeUAPrefix) ||
 		r.Header.Get(KubeletProbeHeaderName) != ""
 }
-
-// IsKProbe returns true if the request is a knatvie probe.
-func IsKProbe(r *http.Request) bool {
-	return r.Header.Get(ProbeHeaderName) == ProbeHeaderValue
-}
-
-// ServeKProbe serve KProbe requests.
-func ServeKProbe(w http.ResponseWriter, r *http.Request) {
-	hh := r.Header.Get(HashHeaderName)
-	if hh == "" {
-		http.Error(w, fmt.Sprintf("a probe request must contain a non-empty %q header", HashHeaderName), http.StatusBadRequest)
-		return
-	}
-	w.Header().Set(HashHeaderName, hh)
-	w.WriteHeader(http.StatusOK)
-}


Test patch:
diff --git a/network/handlers/drain_test.go b/network/handlers/drain_test.go
index 0cfcdb510..0d0ead0ac 100644
--- a/network/handlers/drain_test.go
+++ b/network/handlers/drain_test.go
@@ -362,3 +362,58 @@ func TestHealthCheck(t *testing.T) {
 		t.Errorf("Probe status = %d, wanted %d", got, want)
 	}
 }
+
+func TestIsKProbe(t *testing.T) {
+	req, err := http.NewRequest(http.MethodGet, "http://example.com/", nil)
+	if err != nil {
+		t.Fatal("Error building request:", err)
+	}
+	if isKProbe(req) {
+		t.Error("Not a knative probe but counted as such")
+	}
+	req.Header.Set(network.ProbeHeaderName, network.ProbeHeaderValue)
+	if !isKProbe(req) {
+		t.Error("knative probe but not counted as such")
+	}
+	req.Header.Del(network.ProbeHeaderName)
+	if isKProbe(req) {
+		t.Error("Not a knative probe but counted as such")
+	}
+	req.Header.Set(network.ProbeHeaderName, "no matter")
+	if isKProbe(req) {
+		t.Error("Not a knative probe but counted as such")
+	}
+}
+
+func TestServeKProbe(t *testing.T) {
+	var (
+		kprobehash = "hash"
+		kprobe     = &http.Request{
+			Header: http.Header{
+				network.ProbeHeaderName: []string{network.ProbeHeaderValue},
+				network.HashHeaderName:  []string{kprobehash},
+			},
+		}
+		kprobeerr = &http.Request{
+			Header: http.Header{
+				network.ProbeHeaderName: []string{network.ProbeHeaderValue},
+			},
+		}
+	)
+
+	resp := httptest.NewRecorder()
+	serveKProbe(resp, kprobe)
+	if got, want := resp.Code, http.StatusOK; got != want {
+		t.Errorf("Probe status = %d, wanted %d", got, want)
+	}
+
+	if got, want := resp.Header().Get(network.HashHeaderName), kprobehash; got != want {
+		t.Errorf("KProbe hash = %s, wanted %s", got, want)
+	}
+
+	resp = httptest.NewRecorder()
+	serveKProbe(resp, kprobeerr)
+	if got, want := resp.Code, http.StatusBadRequest; got != want {
+		t.Errorf("Probe status = %d, wanted %d", got, want)
+	}
+}
diff --git a/network/network_test.go b/network/network_test.go
index 3ed1cd9d2..f1a84e775 100644
--- a/network/network_test.go
+++ b/network/network_test.go
@@ -18,7 +18,6 @@ package network
 
 import (
 	"net/http"
-	"net/http/httptest"
 	"testing"
 )
 
@@ -43,58 +42,3 @@ func TestIsKubeletProbe(t *testing.T) {
 		t.Error("kubelet probe but not counted as such")
 	}
 }
-
-func TestIsKnativeProbe(t *testing.T) {
-	req, err := http.NewRequest(http.MethodGet, "http://example.com/", nil)
-	if err != nil {
-		t.Fatal("Error building request:", err)
-	}
-	if IsKProbe(req) {
-		t.Error("Not a knative probe but counted as such")
-	}
-	req.Header.Set(ProbeHeaderName, ProbeHeaderValue)
-	if !IsKProbe(req) {
-		t.Error("knative probe but not counted as such")
-	}
-	req.Header.Del(ProbeHeaderName)
-	if IsKProbe(req) {
-		t.Error("Not a knative probe but counted as such")
-	}
-	req.Header.Set(ProbeHeaderName, "no matter")
-	if IsKProbe(req) {
-		t.Error("Not a knative probe but counted as such")
-	}
-}
-
-func TestServeKProbe(t *testing.T) {
-	var (
-		kprobehash = "hash"
-		kprobe     = &http.Request{
-			Header: http.Header{
-				ProbeHeaderName: []string{ProbeHeaderValue},
-				HashHeaderName:  []string{kprobehash},
-			},
-		}
-		kprobeerr = &http.Request{
-			Header: http.Header{
-				ProbeHeaderName: []string{ProbeHeaderValue},
-			},
-		}
-	)
-
-	resp := httptest.NewRecorder()
-	ServeKProbe(resp, kprobe)
-	if got, want := resp.Code, http.StatusOK; got != want {
-		t.Errorf("Probe status = %d, wanted %d", got, want)
-	}
-
-	if got, want := resp.Header().Get(HashHeaderName), kprobehash; got != want {
-		t.Errorf("KProbe hash = %s, wanted %s", got, want)
-	}
-
-	resp = httptest.NewRecorder()
-	ServeKProbe(resp, kprobeerr)
-	if got, want := resp.Code, http.StatusBadRequest; got != want {
-		t.Errorf("Probe status = %d, wanted %d", got, want)
-	}
-}