Repository: knative/pkg
Pull request #1916 (2020-11-18 20:23:07)

Add  tags to knative provided reconcile metrics
This is related to Issue https://github.com/knative/pkg/issues/1894

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1916#" title="Author self-approved">rusde</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **grantr** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @grantr` 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:

- **[controller/OWNERS](https://github.com/knative/pkg/blob/master/controller/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":["grantr"]} -->
rusde: /cc yanweiguo
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/1916?src=pr&el=h1) Report
> Merging [#1916](https://codecov.io/gh/knative/pkg/pull/1916?src=pr&el=desc) (8a16fde) into [master](https://codecov.io/gh/knative/pkg/commit/ab1a398f669ce2d3d10d876e63ca4a7641f09fff?el=desc) (ab1a398) will **increase** coverage by `0.03%`.
> The diff coverage is `87.50%`.

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

```diff
@@            Coverage Diff             @@
##           master    #1916      +/-   ##
==========================================
+ Coverage   69.45%   69.49%   +0.03%     
==========================================
  Files         207      207              
  Lines        8752     8760       +8     
==========================================
+ Hits         6079     6088       +9     
+ Misses       2402     2400       -2     
- Partials      271      272       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/1916?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [controller/testing/fake\_stats\_reporter.go](https://codecov.io/gh/knative/pkg/pull/1916/diff?src=pr&el=tree#diff-Y29udHJvbGxlci90ZXN0aW5nL2Zha2Vfc3RhdHNfcmVwb3J0ZXIuZ28=) | `100.00% <ø> (ø)` | |
| [network/h2c.go](https://codecov.io/gh/knative/pkg/pull/1916/diff?src=pr&el=tree#diff-bmV0d29yay9oMmMuZ28=) | `20.00% <0.00%> (+4.61%)` | :arrow_up: |
| [network/network.go](https://codecov.io/gh/knative/pkg/pull/1916/diff?src=pr&el=tree#diff-bmV0d29yay9uZXR3b3JrLmdv) | `100.00% <ø> (ø)` | |
| [controller/controller.go](https://codecov.io/gh/knative/pkg/pull/1916/diff?src=pr&el=tree#diff-Y29udHJvbGxlci9jb250cm9sbGVyLmdv) | `88.88% <100.00%> (ø)` | |
| [controller/stats\_reporter.go](https://codecov.io/gh/knative/pkg/pull/1916/diff?src=pr&el=tree#diff-Y29udHJvbGxlci9zdGF0c19yZXBvcnRlci5nbw==) | `96.05% <100.00%> (+0.27%)` | :arrow_up: |
| [test/upgrade/functions.go](https://codecov.io/gh/knative/pkg/pull/1916/diff?src=pr&el=tree#diff-dGVzdC91cGdyYWRlL2Z1bmN0aW9ucy5nbw==) | `100.00% <100.00%> (ø)` | |
| [test/gcs/mock/mock.go](https://codecov.io/gh/knative/pkg/pull/1916/diff?src=pr&el=tree#diff-dGVzdC9nY3MvbW9jay9tb2NrLmdv) | `90.32% <0.00%> (-1.08%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/1916?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/1916?src=pr&el=footer). Last update [ab1a398...4b7929a](https://codecov.io/gh/knative/pkg/pull/1916?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

markusthoemmes: This explodes the cardinality of these metrics doesn't it? We had huge issues with memory pressure due to this and I think in general prometheus wants you to keep the labels on the metrics relatively bounded.
rusde: Hmm without these tags these metrics does not have enough information to be in sync with `knative_revision` metric schema. https://github.com/knative/pkg/blob/master/metrics/metricskey/constants_serving.go#L43-L51

Without that we are not able to ingest these metrics into our system.
rusde: Is it fine if this is behind a config map setting to add these tags?

CM: https://github.com/knative/serving/blob/master/config/core/configmaps/observability.yaml#L25

Saying `metrics.controller-add-knative-revision-tags` 
vagababov: This is `pkg` so it is used outside of Knative Serving proper, so the flag should be more generic `enable-detailed-flags`, or along those lines (I am notoriously bad with naming).

Now, question is also how this is going to be consumed downstream, e.g. do you have in mind how activator or autoscaler will receive this flag, and attach the additional tags. A POC would be super useful.

Generally speaking, I like more dimensions than fewer, but we have to balance against system capabilities as well. 

Patch:
diff --git a/controller/controller.go b/controller/controller.go
index b0ecb2e9b..941117c71 100644
--- a/controller/controller.go
+++ b/controller/controller.go
@@ -493,7 +493,7 @@ func (c *Impl) processNextWorkItem() bool {
 		if err != nil {
 			status = falseString
 		}
-		c.statsReporter.ReportReconcile(time.Since(startTime), status)
+		c.statsReporter.ReportReconcile(time.Since(startTime), status, key)
 
 		// We call Done here so the workqueue knows we have finished
 		// processing this item. We also must remember to call Forget if
diff --git a/controller/stats_reporter.go b/controller/stats_reporter.go
index 7ffa80d7b..92a5848a1 100644
--- a/controller/stats_reporter.go
+++ b/controller/stats_reporter.go
@@ -25,10 +25,12 @@ import (
 	"go.opencensus.io/stats/view"
 	"go.opencensus.io/tag"
 	"go.uber.org/zap"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/cache"
 	kubemetrics "k8s.io/client-go/tools/metrics"
 	"k8s.io/client-go/util/workqueue"
 	"knative.dev/pkg/metrics"
+	"knative.dev/pkg/metrics/metricskey"
 )
 
 var (
@@ -47,6 +49,7 @@ var (
 	// - characters are printable US-ASCII
 	reconcilerTagKey = tag.MustNewKey("reconciler")
 	successTagKey    = tag.MustNewKey("success")
+	NamespaceTagKey  = tag.MustNewKey(metricskey.LabelNamespaceName)
 )
 
 func init() {
@@ -170,12 +173,12 @@ func init() {
 		Description: "Number of reconcile operations",
 		Measure:     reconcileCountStat,
 		Aggregation: view.Count(),
-		TagKeys:     []tag.Key{reconcilerTagKey, successTagKey},
+		TagKeys:     []tag.Key{reconcilerTagKey, successTagKey, NamespaceTagKey},
 	}, {
 		Description: "Latency of reconcile operations",
 		Measure:     reconcileLatencyStat,
 		Aggregation: reconcileDistribution,
-		TagKeys:     []tag.Key{reconcilerTagKey, successTagKey},
+		TagKeys:     []tag.Key{reconcilerTagKey, successTagKey, NamespaceTagKey},
 	}}
 	views = append(views, wp.DefaultViews()...)
 	views = append(views, rp.DefaultViews()...)
@@ -195,7 +198,7 @@ type StatsReporter interface {
 	ReportQueueDepth(v int64) error
 
 	// ReportReconcile reports the count and latency metrics for a reconcile operation
-	ReportReconcile(duration time.Duration, success string) error
+	ReportReconcile(duration time.Duration, success string, key types.NamespacedName) error
 }
 
 // Reporter holds cached metric objects to report metrics
@@ -237,11 +240,14 @@ func (r *reporter) ReportQueueDepth(v int64) error {
 }
 
 // ReportReconcile reports the count and latency metrics for a reconcile operation
-func (r *reporter) ReportReconcile(duration time.Duration, success string) error {
+func (r *reporter) ReportReconcile(duration time.Duration, success string, key types.NamespacedName) error {
 	ctx, err := tag.New(
 		context.Background(),
 		tag.Insert(reconcilerTagKey, r.reconciler),
-		tag.Insert(successTagKey, success))
+		tag.Insert(successTagKey, success),
+		tag.Insert(NamespaceTagKey, key.Namespace),
+	)
+
 	if err != nil {
 		return err
 	}
diff --git a/go.mod b/go.mod
index fcaf0faaf..229ef3537 100644
--- a/go.mod
+++ b/go.mod
@@ -74,6 +74,6 @@ require (
 	k8s.io/klog v1.0.0
 	k8s.io/kube-openapi v0.0.0-20200410145947-bcb3869e6f29 // indirect
 	k8s.io/utils v0.0.0-20200603063816-c1c6865ac451 // indirect
-	knative.dev/hack v0.0.0-20201112185459-01a34c573bd8
+	knative.dev/hack v0.0.0-20201120192952-353db687ec5b
 	sigs.k8s.io/structured-merge-diff/v3 v3.0.1-0.20200706213357-43c19bbb7fba // indirect
 )
diff --git a/go.sum b/go.sum
index 7de2c8fe0..cf77665d0 100644
--- a/go.sum
+++ b/go.sum
@@ -1120,8 +1120,8 @@ k8s.io/kube-openapi v0.0.0-20200410145947-bcb3869e6f29/go.mod h1:F+5wygcW0wmRTnM
 k8s.io/utils v0.0.0-20200324210504-a9aa75ae1b89/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew=
 k8s.io/utils v0.0.0-20200603063816-c1c6865ac451 h1:v8ud2Up6QK1lNOKFgiIVrZdMg7MpmSnvtrOieolJKoE=
 k8s.io/utils v0.0.0-20200603063816-c1c6865ac451/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
-knative.dev/hack v0.0.0-20201112185459-01a34c573bd8 h1:RNbZsAjhswBPtl4C5C5gEFX5/GfWIOZQxfYD9DhkHdY=
-knative.dev/hack v0.0.0-20201112185459-01a34c573bd8/go.mod h1:PHt8x8yX5Z9pPquBEfIj0X66f8iWkWfR0S/sarACJrI=
+knative.dev/hack v0.0.0-20201120192952-353db687ec5b h1:Lc+AKgwhAZUD98mN++qTHeeaP6FRmS8fcwc/rXkP8G0=
+knative.dev/hack v0.0.0-20201120192952-353db687ec5b/go.mod h1:PHt8x8yX5Z9pPquBEfIj0X66f8iWkWfR0S/sarACJrI=
 pgregory.net/rapid v0.3.3 h1:jCjBsY4ln4Atz78QoBWxUEvAHaFyNDQg9+WU62aCn1U=
 pgregory.net/rapid v0.3.3/go.mod h1:UYpPVyjFHzYBGHIxLFoupi8vwk6rXNzRY9OMvVxFIOU=
 rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
diff --git a/metrics/prometheus_exporter.go b/metrics/prometheus_exporter.go
index 784eab2e8..7c177b129 100644
--- a/metrics/prometheus_exporter.go
+++ b/metrics/prometheus_exporter.go
@@ -48,7 +48,7 @@ func newPrometheusExporter(config *metricsConfig, logger *zap.SugaredLogger) (vi
 		logger.Errorw("Failed to create the Prometheus exporter.", zap.Error(err))
 		return nil, nil, err
 	}
-	logger.Infof("Created Opencensus Prometheus exporter with config: %v. Start the server for Prometheus exporter.", config)
+	logger.Infof("Created Prometheus exporter with config: %v. Start the server for Prometheus exporter.", config)
 	// Start the server for Prometheus scraping
 	go func() {
 		srv := startNewPromSrv(e, config.prometheusHost, config.prometheusPort)
diff --git a/vendor/knative.dev/hack/library.sh b/vendor/knative.dev/hack/library.sh
index f37b3f8df..9b90b0025 100644
--- a/vendor/knative.dev/hack/library.sh
+++ b/vendor/knative.dev/hack/library.sh
@@ -551,8 +551,11 @@ function go_update_deps() {
   echo "--- Go mod tidy and vendor"
 
   # Prune modules.
-  go mod tidy
-  go mod vendor
+  local orig_pipefail_opt=$(shopt -p -o pipefail)
+  set -o pipefail
+  go mod tidy 2>&1 | grep -v "ignoring symlink" || true
+  go mod vendor 2>&1 |  grep -v "ignoring symlink" || true
+  eval "$orig_pipefail_opt"
 
   echo "--- Removing unwanted vendor files"
 
@@ -741,6 +744,14 @@ function shellcheck_new_files() {
   fi
 }
 
+function latest_version() {
+  local semver=$(git describe --match "v[0-9]*" --abbrev=0)
+  local major_minor=$(echo "$semver" | cut -d. -f1-2)
+
+  # Get the latest patch release for the major minor
+  git tag -l "${major_minor}*" | sort -r --version-sort | head -n1
+}
+
 # Initializations that depend on previous functions.
 # These MUST come last.
 
diff --git a/vendor/knative.dev/hack/release.sh b/vendor/knative.dev/hack/release.sh
index 4dcfa9158..bd0fdda7d 100644
--- a/vendor/knative.dev/hack/release.sh
+++ b/vendor/knative.dev/hack/release.sh
@@ -468,7 +468,6 @@ function parse_flags() {
   readonly RELEASE_BRANCH
   readonly RELEASE_GCS_BUCKET
   readonly RELEASE_DIR
-  readonly KO_DOCKER_REPO
   readonly VALIDATION_TESTS
   readonly FROM_NIGHTLY_RELEASE
 }
@@ -478,11 +477,16 @@ function parse_flags() {
 function run_validation_tests() {
   (( SKIP_TESTS )) && return
   banner "Running release validation tests"
+  # Unset KO_DOCKER_REPO and restore it after the tests are finished.
+  # This will allow the tests to define their own KO_DOCKER_REPO.
+  local old_docker_repo="${KO_DOCKER_REPO}"
+  unset KO_DOCKER_REPO
   # Run tests.
   if ! $1; then
     banner "Release validation tests failed, aborting"
     abort "release validation tests failed"
   fi
+  export KO_DOCKER_REPO="${old_docker_repo}"
 }
 
 # Publishes the generated artifacts to directory, GCS, GitHub, etc.
diff --git a/vendor/modules.txt b/vendor/modules.txt
index 71df6aa7c..d2b9ad606 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -955,7 +955,7 @@ k8s.io/utils/buffer
 k8s.io/utils/integer
 k8s.io/utils/pointer
 k8s.io/utils/trace
-# knative.dev/hack v0.0.0-20201112185459-01a34c573bd8
+# knative.dev/hack v0.0.0-20201120192952-353db687ec5b
 ## explicit
 knative.dev/hack
 # sigs.k8s.io/structured-merge-diff/v3 v3.0.1-0.20200706213357-43c19bbb7fba
diff --git a/webhook/env.go b/webhook/env.go
index 9e59f7cfd..cd038b40e 100644
--- a/webhook/env.go
+++ b/webhook/env.go
@@ -24,6 +24,9 @@ import (
 
 const portEnvKey = "WEBHOOK_PORT"
 
+// Webhook is the name of the override key used inside of the logging config for Webhook Controller.
+const webhookNameEnv = "WEBHOOK_NAME"
+
 // PortFromEnv returns the webhook port set by portEnvKey, or default port if env var is not set.
 func PortFromEnv(defaultPort int) int {
 	if os.Getenv(portEnvKey) == "" {
@@ -37,3 +40,17 @@ func PortFromEnv(defaultPort int) int {
 	}
 	return port
 }
+
+func NameFromEnv() string {
+	if webhook := os.Getenv(webhookNameEnv); webhook != "" {
+		return webhook
+	}
+
+	panic(fmt.Sprintf(`The environment variable %q is not set.
+This should be unique for the webhooks in a namespace
+If this is a process running on Kubernetes, then initialize this variable via:
+  env:
+  - name: WEBHOOK_NAME
+    value: webhook
+`, webhookNameEnv))
+}
diff --git a/websocket/connection.go b/websocket/connection.go
index 00feceb50..7493d5712 100644
--- a/websocket/connection.go
+++ b/websocket/connection.go
@@ -22,6 +22,7 @@ import (
 	"errors"
 	"io"
 	"io/ioutil"
+	"net/http/httputil"
 	"sync"
 	"time"
 
@@ -133,9 +134,15 @@ func NewDurableConnection(target string, messageChan chan []byte, logger *zap.Su
 			// by restarting the serving side of the connection behind a Kubernetes Service.
 			HandshakeTimeout: 3 * time.Second,
 		}
-		conn, _, err := dialer.Dial(target, nil)
+		conn, resp, err := dialer.Dial(target, nil)
 		if err != nil {
-			logger.Errorw("Websocket connection could not be established", zap.Error(err))
+			if resp != nil {
+				dresp, _ := httputil.DumpResponse(resp, true /*body*/) // This is for logging so don't care if it fails.
+				logger.Errorw("Websocket connection could not be established", zap.Error(err),
+					zap.String("request", string(dresp)))
+			} else {
+				logger.Errorw("Websocket connection could not be established", zap.Error(err))
+			}
 		}
 		return conn, err
 	}


Test patch:
diff --git a/controller/stats_reporter_test.go b/controller/stats_reporter_test.go
index fb3d3dd71..a9e99eb31 100644
--- a/controller/stats_reporter_test.go
+++ b/controller/stats_reporter_test.go
@@ -23,6 +23,7 @@ import (
 	"time"
 
 	"go.opencensus.io/stats/view"
+	"k8s.io/apimachinery/pkg/types"
 	"knative.dev/pkg/metrics/metricstest"
 	_ "knative.dev/pkg/metrics/testing"
 )
@@ -66,9 +67,12 @@ func TestReportQueueDepth(t *testing.T) {
 
 func TestReportReconcile(t *testing.T) {
 	r, _ := NewStatsReporter("testreconciler")
+	rName := "test_resource"
+	rNamespace := "default"
 	wantTags := map[string]string{
-		"reconciler": "testreconciler",
-		"success":    "true",
+		"reconciler":     "testreconciler",
+		"success":        "true",
+		"namespace_name": rNamespace,
 	}
 
 	initialReconcileCount := int64(0)
@@ -82,15 +86,19 @@ func TestReportReconcile(t *testing.T) {
 		initialMin, initialMax = dd.Min, dd.Max
 		initialDistributionCount = dd.Count
 	}
+	key := types.NamespacedName{
+		Name:      rName,
+		Namespace: rNamespace,
+	}
 
 	slow, fast := initialMax+5, initialMin-5
 
-	expectSuccess(t, func() error { return r.ReportReconcile(time.Duration(fast)*time.Millisecond, "true") })
+	expectSuccess(t, func() error { return r.ReportReconcile(time.Duration(fast)*time.Millisecond, "true", key) })
 	metricstest.CheckCountData(t, "reconcile_count", wantTags, initialReconcileCount+1)
 	metricstest.CheckDistributionData(t, "reconcile_latency", wantTags, initialDistributionCount+1,
 		fast, initialMax)
 
-	expectSuccess(t, func() error { return r.ReportReconcile(time.Duration(slow)*time.Millisecond, "true") })
+	expectSuccess(t, func() error { return r.ReportReconcile(time.Duration(slow)*time.Millisecond, "true", key) })
 	metricstest.CheckCountData(t, "reconcile_count", wantTags, initialReconcileCount+2)
 	metricstest.CheckDistributionData(t, "reconcile_latency", wantTags, initialDistributionCount+2,
 		fast, slow)
diff --git a/controller/testing/fake_stats_reporter.go b/controller/testing/fake_stats_reporter.go
index 528a854b3..68a23999e 100644
--- a/controller/testing/fake_stats_reporter.go
+++ b/controller/testing/fake_stats_reporter.go
@@ -19,6 +19,8 @@ package testing
 import (
 	"sync"
 	"time"
+
+	"k8s.io/apimachinery/pkg/types"
 )
 
 // FakeStatsReporter is a fake implementation of StatsReporter
@@ -43,7 +45,7 @@ func (r *FakeStatsReporter) ReportQueueDepth(v int64) error {
 }
 
 // ReportReconcile records the call and returns success.
-func (r *FakeStatsReporter) ReportReconcile(duration time.Duration, success string) error {
+func (r *FakeStatsReporter) ReportReconcile(duration time.Duration, success string, _ types.NamespacedName) error {
 	r.Lock.Lock()
 	defer r.Lock.Unlock()
 	r.reconcileData = append(r.reconcileData, FakeReconcileStatData{duration, success})
diff --git a/controller/testing/fake_stats_reporter_test.go b/controller/testing/fake_stats_reporter_test.go
index c57f14780..6630ac1ca 100644
--- a/controller/testing/fake_stats_reporter_test.go
+++ b/controller/testing/fake_stats_reporter_test.go
@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"k8s.io/apimachinery/pkg/types"
 
 	"knative.dev/pkg/controller"
 )
@@ -38,7 +39,10 @@ func TestReportQueueDepth(t *testing.T) {
 
 func TestReportReconcile(t *testing.T) {
 	r := &FakeStatsReporter{}
-	r.ReportReconcile(time.Duration(123), "False")
+	r.ReportReconcile(time.Duration(123), "False", types.NamespacedName{
+		Name:      "test_name",
+		Namespace: "default",
+	})
 	if got, want := r.GetReconcileData(), []FakeReconcileStatData{{time.Duration(123), "False"}}; !reflect.DeepEqual(want, got) {
 		t.Errorf("reconcile data len: want: %v, got: %v", want, got)
 	}
diff --git a/test/request.go b/test/request.go
index 8b546d970..ecaecd68f 100644
--- a/test/request.go
+++ b/test/request.go
@@ -20,16 +20,12 @@ package test
 
 import (
 	"context"
-	"fmt"
-	"net/http"
 	"net/url"
 	"strings"
-	"sync"
 	"time"
 
 	"knative.dev/pkg/test/flags"
 
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/kubernetes"
 	"knative.dev/pkg/test/logging"
 	"knative.dev/pkg/test/spoof"
@@ -37,107 +33,37 @@ import (
 
 // RequestOption enables configuration of requests
 // when polling for endpoint states.
-type RequestOption func(*http.Request)
+type RequestOption = spoof.RequestOption
 
 // WithHeader will add the provided headers to the request.
-func WithHeader(header http.Header) RequestOption {
-	return func(r *http.Request) {
-		if r.Header == nil {
-			r.Header = header
-			return
-		}
-		for key, values := range header {
-			for _, value := range values {
-				r.Header.Add(key, value)
-			}
-		}
-	}
-}
+//
+// Deprecated: Use the spoof package version
+var WithHeader = spoof.WithHeader
 
 // Retrying modifies a ResponseChecker to retry certain response codes.
-func Retrying(rc spoof.ResponseChecker, codes ...int) spoof.ResponseChecker {
-	return func(resp *spoof.Response) (bool, error) {
-		for _, code := range codes {
-			if resp.StatusCode == code {
-				// Returning (false, nil) causes SpoofingClient.Poll to retry.
-				// sc.logger.Info("Retrying for code ", resp.StatusCode)
-				return false, nil
-			}
-		}
-
-		// If we didn't match any retryable codes, invoke the ResponseChecker that we wrapped.
-		return rc(resp)
-	}
-}
+//
+// Deprecated: Use the spoof package version
+var Retrying = spoof.Retrying
 
 // IsOneOfStatusCodes checks that the response code is equal to the given one.
-func IsOneOfStatusCodes(codes ...int) spoof.ResponseChecker {
-	return func(resp *spoof.Response) (bool, error) {
-		for _, code := range codes {
-			if resp.StatusCode == code {
-				return true, nil
-			}
-		}
-
-		return true, fmt.Errorf("status = %d %s, want one of: %v", resp.StatusCode, resp.Status, codes)
-	}
-}
+//
+// Deprecated: Use the spoof package version
+var IsOneOfStatusCodes = spoof.IsOneOfStatusCodes
 
 // IsStatusOK checks that the response code is a 200.
-func IsStatusOK(resp *spoof.Response) (bool, error) {
-	return IsOneOfStatusCodes(http.StatusOK)(resp)
-}
+//
+// Deprecated: Use the spoof package version
+var IsStatusOK = spoof.IsStatusOK
 
 // MatchesAllBodies checks that the *first* response body matches the "expected" body, otherwise failing.
-func MatchesAllBodies(all ...string) spoof.ResponseChecker {
-	var m sync.Mutex
-	// This helps with two things:
-	// 1. we can use Equal on sets
-	// 2. it will collapse the duplicates
-	want := sets.NewString(all...)
-	seen := make(sets.String, len(all))
-
-	return func(resp *spoof.Response) (bool, error) {
-		bs := string(resp.Body)
-		for expected := range want {
-			if !strings.Contains(bs, expected) {
-				// See if the next one matches.
-				continue
-			}
-
-			m.Lock()
-			defer m.Unlock()
-			seen.Insert(expected)
-
-			// Stop once we've seen them all.
-			return want.Equal(seen), nil
-		}
-
-		// Returning (true, err) causes SpoofingClient.Poll to fail.
-		return true, fmt.Errorf("body = %s, want one of: %s", bs, all)
-	}
-}
+//
+// Deprecated: Use the spoof package version
+var MatchesAllBodies = spoof.MatchesAllBodies
 
 // MatchesBody checks that the *first* response body matches the "expected" body, otherwise failing.
-func MatchesBody(expected string) spoof.ResponseChecker {
-	return func(resp *spoof.Response) (bool, error) {
-		if !strings.Contains(string(resp.Body), expected) {
-			// Returning (true, err) causes SpoofingClient.Poll to fail.
-			return true, fmt.Errorf("body = %s, want: %s", string(resp.Body), expected)
-		}
-
-		return true, nil
-	}
-}
-
-// EventuallyMatchesBody checks that the response body *eventually* matches the expected body.
-// TODO(#1178): Delete me. We don't want to need this; we should be waiting for an appropriate Status instead.
-func EventuallyMatchesBody(expected string) spoof.ResponseChecker {
-	return func(resp *spoof.Response) (bool, error) {
-		// Returning (false, nil) causes SpoofingClient.Poll to retry.
-		return strings.Contains(string(resp.Body), expected), nil
-	}
-}
+//
+// Deprecated: Use the spoof package version
+var MatchesBody = spoof.MatchesBody
 
 // MatchesAllOf combines multiple ResponseCheckers to one ResponseChecker with a logical AND. The
 // checkers are executed in order. The first function to trigger an error or a retry will short-circuit
@@ -147,14 +73,16 @@ func EventuallyMatchesBody(expected string) spoof.ResponseChecker {
 // MatchesAllOf(IsStatusOK, MatchesBody("test"))
 //
 // The MatchesBody check will only be executed after the IsStatusOK has passed.
-func MatchesAllOf(checkers ...spoof.ResponseChecker) spoof.ResponseChecker {
+//
+// Deprecated: Use the spoof package version
+var MatchesAllOf = spoof.MatchesAllOf
+
+// EventuallyMatchesBody checks that the response body *eventually* matches the expected body.
+// TODO(#1178): Delete me. We don't want to need this; we should be waiting for an appropriate Status instead.
+func EventuallyMatchesBody(expected string) spoof.ResponseChecker {
 	return func(resp *spoof.Response) (bool, error) {
-		for _, checker := range checkers {
-			if done, err := checker(resp); err != nil || !done {
-				return done, err
-			}
-		}
-		return true, nil
+		// Returning (false, nil) causes SpoofingClient.Poll to retry.
+		return strings.Contains(string(resp.Body), expected), nil
 	}
 }
 
@@ -193,24 +121,16 @@ func WaitForEndpointStateWithTimeout(
 	resolvable bool,
 	timeout time.Duration,
 	opts ...interface{}) (*spoof.Response, error) {
-	defer logging.GetEmitableSpan(ctx, "WaitForEndpointState/"+desc).End()
-
-	if url.Scheme == "" || url.Host == "" {
-		return nil, fmt.Errorf("invalid URL: %q", url.String())
-	}
-
-	req, err := http.NewRequest(http.MethodGet, url.String(), nil)
-	if err != nil {
-		return nil, err
-	}
 
 	var tOpts []spoof.TransportOption
+	var rOpts []spoof.RequestOption
+
 	for _, opt := range opts {
-		rOpt, ok := opt.(RequestOption)
-		if ok {
-			rOpt(req)
-		} else if tOpt, ok := opt.(spoof.TransportOption); ok {
-			tOpts = append(tOpts, tOpt)
+		switch o := opt.(type) {
+		case spoof.RequestOption:
+			rOpts = append(rOpts, o)
+		case spoof.TransportOption:
+			tOpts = append(tOpts, o)
 		}
 	}
 
@@ -220,5 +140,5 @@ func WaitForEndpointStateWithTimeout(
 	}
 	client.RequestTimeout = timeout
 
-	return client.Poll(req, inState)
+	return client.WaitForEndpointState(ctx, url, inState, desc, rOpts...)
 }
diff --git a/test/spoof/request.go b/test/spoof/request.go
new file mode 100644
index 000000000..df26e8fae
--- /dev/null
+++ b/test/spoof/request.go
@@ -0,0 +1,38 @@
+/*
+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
+
+    http://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 spoof
+
+import "net/http"
+
+// RequestOption enables configuration of requests
+// when polling for endpoint states.
+type RequestOption func(*http.Request)
+
+// WithHeader will add the provided headers to the request.
+func WithHeader(header http.Header) RequestOption {
+	return func(r *http.Request) {
+		if r.Header == nil {
+			r.Header = header
+			return
+		}
+		for key, values := range header {
+			for _, value := range values {
+				r.Header.Add(key, value)
+			}
+		}
+	}
+}
diff --git a/test/spoof/response_checks.go b/test/spoof/response_checks.go
new file mode 100644
index 000000000..d5e96258f
--- /dev/null
+++ b/test/spoof/response_checks.go
@@ -0,0 +1,121 @@
+/*
+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
+
+    http://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 spoof
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+	"sync"
+
+	"k8s.io/apimachinery/pkg/util/sets"
+)
+
+// MatchesBody checks that the *first* response body matches the "expected" body, otherwise failing.
+func MatchesBody(expected string) ResponseChecker {
+	return func(resp *Response) (bool, error) {
+		if !strings.Contains(string(resp.Body), expected) {
+			// Returning (true, err) causes SpoofingClient.Poll to fail.
+			return true, fmt.Errorf("body = %s, want: %s", string(resp.Body), expected)
+		}
+
+		return true, nil
+	}
+}
+
+// MatchesAllOf combines multiple ResponseCheckers to one ResponseChecker with a logical AND. The
+// checkers are executed in order. The first function to trigger an error or a retry will short-circuit
+// the other functions (they will not be executed).
+//
+// This is useful for combining a body with a status check like:
+// MatchesAllOf(IsStatusOK, MatchesBody("test"))
+//
+// The MatchesBody check will only be executed after the IsStatusOK has passed.
+func MatchesAllOf(checkers ...ResponseChecker) ResponseChecker {
+	return func(resp *Response) (bool, error) {
+		for _, checker := range checkers {
+			if done, err := checker(resp); err != nil || !done {
+				return done, err
+			}
+		}
+		return true, nil
+	}
+}
+
+// MatchesAllBodies checks that the *first* response body matches the "expected" body, otherwise failing.
+func MatchesAllBodies(all ...string) ResponseChecker {
+	var m sync.Mutex
+	// This helps with two things:
+	// 1. we can use Equal on sets
+	// 2. it will collapse the duplicates
+	want := sets.NewString(all...)
+	seen := make(sets.String, len(all))
+
+	return func(resp *Response) (bool, error) {
+		bs := string(resp.Body)
+		for expected := range want {
+			if !strings.Contains(bs, expected) {
+				// See if the next one matches.
+				continue
+			}
+
+			m.Lock()
+			defer m.Unlock()
+			seen.Insert(expected)
+
+			// Stop once we've seen them all.
+			return want.Equal(seen), nil
+		}
+
+		// Returning (true, err) causes SpoofingClient.Poll to fail.
+		return true, fmt.Errorf("body = %s, want one of: %s", bs, all)
+	}
+}
+
+// IsStatusOK checks that the response code is a 200.
+func IsStatusOK(resp *Response) (bool, error) {
+	return IsOneOfStatusCodes(http.StatusOK)(resp)
+}
+
+// IsOneOfStatusCodes checks that the response code is equal to the given one.
+func IsOneOfStatusCodes(codes ...int) ResponseChecker {
+	return func(resp *Response) (bool, error) {
+		for _, code := range codes {
+			if resp.StatusCode == code {
+				return true, nil
+			}
+		}
+
+		return true, fmt.Errorf("status = %d %s, want one of: %v", resp.StatusCode, resp.Status, codes)
+	}
+}
+
+// Retrying modifies a ResponseChecker to retry certain response codes.
+func Retrying(rc ResponseChecker, codes ...int) ResponseChecker {
+	return func(resp *Response) (bool, error) {
+		for _, code := range codes {
+			if resp.StatusCode == code {
+				// Returning (false, nil) causes SpoofingClient.Poll to retry.
+				// sc.logger.Info("Retrying for code ", resp.StatusCode)
+				return false, nil
+			}
+		}
+
+		// If we didn't match any retryable codes, invoke the ResponseChecker that we wrapped.
+		return rc(resp)
+	}
+}
diff --git a/test/spoof/spoof.go b/test/spoof/spoof.go
index e69b39c28..9d00bad9e 100644
--- a/test/spoof/spoof.go
+++ b/test/spoof/spoof.go
@@ -26,6 +26,7 @@ import (
 	"io/ioutil"
 	"net"
 	"net/http"
+	"net/url"
 	"time"
 
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -245,3 +246,28 @@ func (sc *SpoofingClient) logZipkinTrace(spoofResp *Response) {
 
 	sc.Logf("%s", json)
 }
+
+func (sc *SpoofingClient) WaitForEndpointState(
+	ctx context.Context,
+	url *url.URL,
+	inState ResponseChecker,
+	desc string,
+	opts ...RequestOption) (*Response, error) {
+
+	defer logging.GetEmitableSpan(ctx, "WaitForEndpointState/"+desc).End()
+
+	if url.Scheme == "" || url.Host == "" {
+		return nil, fmt.Errorf("invalid URL: %q", url.String())
+	}
+
+	req, err := http.NewRequest(http.MethodGet, url.String(), nil)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, opt := range opts {
+		opt(req)
+	}
+
+	return sc.Poll(req, inState)
+}
diff --git a/vendor/knative.dev/hack/e2e-tests.sh b/vendor/knative.dev/hack/e2e-tests.sh
index 19a7b6eec..0b8548f66 100644
--- a/vendor/knative.dev/hack/e2e-tests.sh
+++ b/vendor/knative.dev/hack/e2e-tests.sh
@@ -69,8 +69,12 @@ function setup_test_cluster() {
   # Acquire cluster admin role for the current user.
   acquire_cluster_admin_role "${k8s_cluster}"
 
-  # Setup KO_DOCKER_REPO if it is a GKE cluster. Incorporate an element of randomness to ensure that each run properly publishes images.
-  [[ "${k8s_cluster}" =~ ^gke_.* ]] && export KO_DOCKER_REPO=gcr.io/${E2E_PROJECT_ID}/${REPO_NAME}-e2e-img/${RANDOM}
+  # Setup KO_DOCKER_REPO if it is a GKE cluster. Incorporate an element of 
+  # randomness to ensure that each run properly publishes images. Don't 
+  # owerwrite KO_DOCKER_REPO if already set.
+  [ -z "${KO_DOCKER_REPO}" ] && \
+    [[ "${k8s_cluster}" =~ ^gke_.* ]] && \
+    export KO_DOCKER_REPO=gcr.io/${E2E_PROJECT_ID}/${REPO_NAME}-e2e-img/${RANDOM}
 
   # Safety checks
   is_protected_gcr "${KO_DOCKER_REPO}" && \
diff --git a/webhook/env_test.go b/webhook/env_test.go
index 4ffc8956c..c08eaba77 100644
--- a/webhook/env_test.go
+++ b/webhook/env_test.go
@@ -33,6 +33,13 @@ type portTest struct {
 	wantPanic bool
 }
 
+type webhookNameTest struct {
+	name      string
+	in        string
+	want      string
+	wantPanic bool
+}
+
 func TestPort(t *testing.T) {
 	tests := []portTest{{
 		name: testMissingInputName,
@@ -81,3 +88,37 @@ func TestPort(t *testing.T) {
 		})
 	}
 }
+
+func TestWebhookName(t *testing.T) {
+	tests := []webhookNameTest{{
+		name:      "EmptyInput",
+		in:        "",
+		wantPanic: true,
+	}, {
+		name: "ValidInput",
+		in:   "mywebhook",
+		want: "mywebhook",
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// webhookNameEnv is unset when testing missing input.
+			if tc.name != testMissingInputName {
+				os.Setenv(webhookNameEnv, tc.in)
+			}
+
+			defer func() {
+				if r := recover(); r == nil && tc.wantPanic {
+					t.Error("Did not panic")
+				} else if r != nil && !tc.wantPanic {
+					t.Error("Got unexpected panic")
+				}
+				os.Unsetenv(webhookNameEnv)
+			}()
+
+			if got := NameFromEnv(); got != tc.want {
+				t.Errorf("NameFromEnv = %s, want: %s", got, tc.want)
+			}
+		})
+	}
+}
diff --git a/websocket/connection_test.go b/websocket/connection_test.go
index ea3d6124e..d8712c31f 100644
--- a/websocket/connection_test.go
+++ b/websocket/connection_test.go
@@ -94,7 +94,7 @@ func errConnFactory(err error) func() (rawConnection, error) {
 }
 
 func TestRetriesWhileConnect(t *testing.T) {
-	wantConnects := 2
+	const wantConnects = 2
 	gotConnects := 0
 
 	spy := &inspectableConnection{