Repository: knative/pkg
Pull request #1318 (2020-05-09 16:01:01)

Implement the K8s lifecycle in webhook.
The webhook never properly implemented the Kubernetes SIGTERM/SIGKILL lifecycle, and doesn't even really support readiness probes today.  This change enables folks to use a block like this on their webhook container:

```yaml
        readinessProbe: &probe
          periodSeconds: 1
          httpGet:
            scheme: HTTPS
            port: 8443
            httpHeaders:
            - name: k-kubelet-probe
              value: "webhook"
        livenessProbe: *probe
```

With this, the webhook won't report as `Ready` until a probe has succeeded, and when the SIGTERM is received, we will start failing probes for a grace period (so our Endpoint drops) before shutting down the webhook's HTTP Server.

This was uncovered by running the webhook across 10 replicas in Serving with the "Goose" (https://github.com/knative/pkg/pull/1316) enabled for the e2e tests.  The failure mode I saw was conversion webhook requests failing across random tests.

This also moves the Serving probe-detection function into PKG.

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1318#" title="Author self-approved">mattmoor</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)~~ [mattmoor]
- ~~[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/OWNERS)~~ [mattmoor]

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
---- |:------------:|:------------:|:-----:
[network/network.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1318/pull-knative-pkg-go-coverage/1259151549502853120/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[webhook/webhook.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1318/pull-knative-pkg-go-coverage/1259151549502853120/artifacts/line-cov.html#file1) | 83.6% | 80.6% | -3.0

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
---- |:------------:|:------------:|:-----:
[network/network.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1318/pull-knative-pkg-go-coverage/1259261399155085314/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[webhook/webhook.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1318/pull-knative-pkg-go-coverage/1259261399155085314/artifacts/line-cov.html#file1) | 83.6% | 80.6% | -3.0

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
---- |:------------:|:------------:|:-----:
[network/network.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1318/pull-knative-pkg-go-coverage/1259263412064489474/artifacts/line-cov.html#file0) | Do not exist | 100.0% | 
[webhook/webhook.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1318/pull-knative-pkg-go-coverage/1259263412064489474/artifacts/line-cov.html#file1) | 83.6% | 82.1% | -1.5


Patch:
diff --git a/network/network.go b/network/network.go
index 32015ada3..0c4749d49 100644
--- a/network/network.go
+++ b/network/network.go
@@ -17,6 +17,8 @@ limitations under the License.
 package network
 
 import (
+	"net/http"
+	"strings"
 	"time"
 )
 
@@ -42,4 +44,18 @@ const (
 	// with this header will not be passed to the user container or
 	// included in request metrics.
 	ProbeHeaderName = "K-Network-Probe"
+
+	// Since K8s 1.8, prober requests have
+	//   User-Agent = "kube-probe/{major-version}.{minor-version}".
+	KubeProbeUAPrefix = "kube-probe/"
+
+	// Istio with mTLS rewrites probes, but their probes pass a different
+	// user-agent.  So we augment the probes with this header.
+	KubeletProbeHeaderName = "K-Kubelet-Probe"
 )
+
+// IsKubeletProbe returns true if the request is a Kubernetes probe.
+func IsKubeletProbe(r *http.Request) bool {
+	return strings.HasPrefix(r.Header.Get("User-Agent"), KubeProbeUAPrefix) ||
+		r.Header.Get(KubeletProbeHeaderName) != ""
+}
diff --git a/webhook/webhook.go b/webhook/webhook.go
index 3315ba163..e29ea9611 100644
--- a/webhook/webhook.go
+++ b/webhook/webhook.go
@@ -22,6 +22,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"time"
 
 	// Injection stuff
 	kubeclient "knative.dev/pkg/client/injection/kube/client"
@@ -33,6 +34,7 @@ import (
 	"k8s.io/client-go/kubernetes"
 	corelisters "k8s.io/client-go/listers/core/v1"
 	"knative.dev/pkg/logging"
+	"knative.dev/pkg/network"
 	"knative.dev/pkg/system"
 	certresources "knative.dev/pkg/webhook/certificates/resources"
 )
@@ -71,6 +73,13 @@ const (
 	Connect Operation = admissionv1beta1.Connect
 )
 
+var (
+	// GracePeriod is the duration that the webhook will wait after it's
+	// context is cancelled (and probes are failing) before shutting down
+	// the http server.
+	GracePeriod = 30 * time.Second
+)
+
 // Webhook implements the external webhook for validation of
 // resources and configuration.
 type Webhook struct {
@@ -81,6 +90,12 @@ type Webhook struct {
 	// synced is function that is called when the informers have been synced.
 	synced context.CancelFunc
 
+	// stopCh is closed when we should start failing readiness probes.
+	stopCh chan struct{}
+	// grace period is how long to wait after failing readiness probes
+	// before shutting down.
+	gracePeriod time.Duration
+
 	mux          http.ServeMux
 	secretlister corelisters.SecretLister
 }
@@ -129,6 +144,8 @@ func New(
 		secretlister: secretInformer.Lister(),
 		Logger:       logger,
 		synced:       cancel,
+		stopCh:       make(chan struct{}),
+		gracePeriod:  GracePeriod,
 	}
 
 	webhook.mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
@@ -193,8 +210,6 @@ func (wh *Webhook) Run(stop <-chan struct{}) error {
 		},
 	}
 
-	logger.Info("Found certificates for webhook...")
-
 	eg, ctx := errgroup.WithContext(ctx)
 	eg.Go(func() error {
 		if err := server.ListenAndServeTLS("", ""); err != nil && err != http.ErrServerClosed {
@@ -206,16 +221,40 @@ func (wh *Webhook) Run(stop <-chan struct{}) error {
 
 	select {
 	case <-stop:
-		if err := server.Close(); err != nil {
-			return err
-		}
+		eg.Go(func() error {
+			// Start failing readiness probes immediately.
+			logger.Info("Starting to fail readiness probes...")
+			close(wh.stopCh)
+
+			// Wait for a grace period for the above to take effect and this Pod's
+			// endpoint to be removed from the webhook service's Endpoints.
+			// For this to be effective, it must be greater than the probe's
+			// periodSeconds times failureThreshold by a margin suitable to
+			// propagate the new Endpoints data across the cluster.
+			time.Sleep(wh.gracePeriod)
+
+			return server.Shutdown(context.Background())
+		})
+
+		// Wait for all outstanding go routined to terminate, including our new one.
 		return eg.Wait()
+
 	case <-ctx.Done():
 		return fmt.Errorf("webhook server bootstrap failed %w", ctx.Err())
 	}
 }
 
 func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	// Respond to probes regardless of path.
+	if network.IsKubeletProbe(r) {
+		select {
+		case <-wh.stopCh:
+			http.Error(w, "shutting down", http.StatusInternalServerError)
+		default:
+			w.WriteHeader(http.StatusOK)
+		}
+	}
+
 	// Verify the content type is accurate.
 	contentType := r.Header.Get("Content-Type")
 	if contentType != "application/json" {


Test patch:
diff --git a/network/network_test.go b/network/network_test.go
new file mode 100644
index 000000000..dc0eb2eb2
--- /dev/null
+++ b/network/network_test.go
@@ -0,0 +1,44 @@
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package network
+
+import (
+	"net/http"
+	"testing"
+)
+
+func TestIsKubeletProbe(t *testing.T) {
+	req, err := http.NewRequest(http.MethodGet, "http://example.com/", nil)
+	if err != nil {
+		t.Fatal("Error building request:", err)
+	}
+	if IsKubeletProbe(req) {
+		t.Error("Not a kubelet probe but counted as such")
+	}
+	req.Header.Set("User-Agent", KubeProbeUAPrefix+"1.14")
+	if !IsKubeletProbe(req) {
+		t.Error("kubelet probe but not counted as such")
+	}
+	req.Header.Del("User-Agent")
+	if IsKubeletProbe(req) {
+		t.Error("Not a kubelet probe but counted as such")
+	}
+	req.Header.Set(KubeletProbeHeaderName, "no matter")
+	if !IsKubeletProbe(req) {
+		t.Error("kubelet probe but not counted as such")
+	}
+}
diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go
index 3f2b29ed5..bcdef62d9 100644
--- a/webhook/webhook_test.go
+++ b/webhook/webhook_test.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"net"
 	"testing"
+	"time"
 
 	"golang.org/x/sync/errgroup"
 	"knative.dev/pkg/controller"
@@ -44,6 +45,11 @@ const (
 	user1            = "brutto@knative.dev"
 )
 
+func init() {
+	// Don't hang forever when running tests.
+	GracePeriod = 100 * time.Millisecond
+}
+
 func newNonRunningTestWebhook(t *testing.T, options Options, acs ...interface{}) (
 	ctx context.Context, ac *Webhook, cancel context.CancelFunc) {
 	t.Helper()