Repository: knative/pkg
Pull request #2651 (2022-11-22 23:29:55)

[release-1.8] Fix nil pointer deref in drain when it's reset
This is an automated cherry-pick of #2645

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2651#" title="Author self-approved">knative-prow-robot</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign tcnghia for approval by writing `/assign @tcnghia` in a comment. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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/release-1.8/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":["tcnghia"]} -->
dprotaso: /lgtm
/approve
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2651#issuecomment-1324429252" title="Approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/2651#" title="Author self-approved">knative-prow-robot</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/release-1.8/network/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":[]} -->

Patch:
diff --git a/network/handlers/drain.go b/network/handlers/drain.go
index 5eee2ea42..3b2f28a07 100644
--- a/network/handlers/drain.go
+++ b/network/handlers/drain.go
@@ -196,7 +196,7 @@ func (d *Drainer) resetTimer() {
 
 	d.Lock()
 	defer d.Unlock()
-	if d.timer.Stop() {
+	if d.timer != nil && d.timer.Stop() {
 		d.timer.Reset(d.QuietPeriod)
 	}
 }


Test patch:
diff --git a/network/handlers/drain_test.go b/network/handlers/drain_test.go
index 082b8100d..88e4f07e5 100644
--- a/network/handlers/drain_test.go
+++ b/network/handlers/drain_test.go
@@ -546,3 +546,43 @@ func TestReset(t *testing.T) {
 	// Calling reset after a drain should succeed
 	d.Reset()
 }
+
+// https://github.com/knative/pkg/issues/2642
+func TestResetWithActiveRequests(t *testing.T) {
+	d := Drainer{
+		QuietPeriod: 5 * time.Second,
+		Inner:       http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}),
+	}
+
+	trafficStopped := make(chan struct{})
+	trafficStarted := make(chan struct{})
+	drainStarted := make(chan struct{})
+	defer close(trafficStopped)
+
+	go func() {
+		req, _ := http.NewRequest("GET", "knative.dev", nil)
+		rec := httptest.NewRecorder()
+
+		close(trafficStarted)
+		for {
+			select {
+			case <-trafficStopped:
+				return
+			default:
+				d.ServeHTTP(rec, req)
+			}
+		}
+	}()
+
+	go func() {
+		<-trafficStarted
+		close(drainStarted)
+		d.Drain()
+	}()
+
+	<-drainStarted
+	d.Reset()
+
+	// We need requests to be active for a bit
+	time.Sleep(time.Second)
+}