From a68e0090419f3121493c0652f5fcfe37fec1acd6 Mon Sep 17 00:00:00 2001
From: Annie Fu <16651409+anniefu@users.noreply.github.com>
Date: Tue, 9 Jul 2019 17:23:58 -0700
Subject: [PATCH] Add check for nil StatsReporter in webhook package (#518)

* Prevent nil StatsReporter for existing webhook package consumers

* Pass StatsReporter by pointer and have tests test constructor

* Make constructor return error instead of panicking

* Move StatsReporter to ControllerOptions to consolidate constructors
---
 webhook/webhook.go      | 46 ++++++++++++++++++++++++-----
 webhook/webhook_test.go | 64 +++++++++++++++++------------------------
 2 files changed, 66 insertions(+), 44 deletions(-)

diff --git a/webhook/webhook.go b/webhook/webhook.go
index 08ff6b902..3aa67dd96 100644
--- a/webhook/webhook.go
+++ b/webhook/webhook.go
@@ -99,6 +99,10 @@ type ControllerOptions struct {
 	// TLS Client Authentication.
 	// The default value is tls.NoClientCert.
 	ClientAuth tls.ClientAuthType
+
+	// StatsReporter reports metrics about the webhook.
+	// This will be automatically initialized by the constructor if left uninitialized.
+	StatsReporter StatsReporter
 }
 
 // ResourceCallback defines a signature for resource specific (Route, Configuration, etc.)
@@ -114,11 +118,10 @@ type ResourceDefaulter func(patches *[]jsonpatch.JsonPatchOperation, crd Generic
 // AdmissionController implements the external admission webhook for validation of
 // pilot configuration.
 type AdmissionController struct {
-	Client        kubernetes.Interface
-	Options       ControllerOptions
-	Handlers      map[schema.GroupVersionKind]GenericCRD
-	Logger        *zap.SugaredLogger
-	StatsReporter StatsReporter
+	Client   kubernetes.Interface
+	Options  ControllerOptions
+	Handlers map[schema.GroupVersionKind]GenericCRD
+	Logger   *zap.SugaredLogger
 
 	WithContext           func(context.Context) context.Context
 	DisallowUnknownFields bool
@@ -136,6 +139,33 @@ type GenericCRD interface {
 	runtime.Object
 }
 
+// NewAdmissionController constructs an AdmissionController
+func NewAdmissionController(
+	client kubernetes.Interface,
+	opts ControllerOptions,
+	handlers map[schema.GroupVersionKind]GenericCRD,
+	logger *zap.SugaredLogger,
+	ctx func(context.Context) context.Context,
+	disallowUnknownFields bool) (*AdmissionController, error) {
+
+	if opts.StatsReporter == nil {
+		reporter, err := NewStatsReporter()
+		if err != nil {
+			return nil, err
+		}
+		opts.StatsReporter = reporter
+	}
+
+	return &AdmissionController{
+		Client:                client,
+		Options:               opts,
+		Handlers:              handlers,
+		Logger:                logger,
+		WithContext:           ctx,
+		DisallowUnknownFields: disallowUnknownFields,
+	}, nil
+}
+
 // GetAPIServerExtensionCACert gets the Kubernetes aggregate apiserver
 // client CA cert used by validator.
 //
@@ -455,8 +485,10 @@ func (ac *AdmissionController) ServeHTTP(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	// Only report valid requests
-	ac.StatsReporter.ReportRequest(review.Request, response.Response, time.Since(ttStart))
+	if ac.Options.StatsReporter != nil {
+		// Only report valid requests
+		ac.Options.StatsReporter.ReportRequest(review.Request, response.Response, time.Since(ttStart))
+	}
 }
 
 func makeErrorStatus(reason string, args ...interface{}) *admissionv1beta1.AdmissionResponse {
diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go
index 9eb582472..ab0876593 100644
--- a/webhook/webhook_test.go
+++ b/webhook/webhook_test.go
@@ -73,12 +73,8 @@ func newNonRunningTestAdmissionController(t *testing.T, options ControllerOption
 	t.Helper()
 	// Create fake clients
 	kubeClient = fakekubeclientset.NewSimpleClientset()
-	statsReporter, srErr := NewStatsReporter()
-	if srErr != nil {
-		t.Fatalf("Failed to create new stats reporter: %v", srErr)
-	}
 
-	ac, err := NewAdmissionController(kubeClient, options, TestLogger(t), &statsReporter)
+	ac, err := NewTestAdmissionController(kubeClient, options, TestLogger(t))
 	if err != nil {
 		t.Fatalf("Failed to create new admission controller: %v", err)
 	}
@@ -798,36 +794,30 @@ func setUserAnnotation(userC, userU string) jsonpatch.JsonPatchOperation {
 	}
 }
 
-func NewAdmissionController(client kubernetes.Interface, options ControllerOptions,
-	logger *zap.SugaredLogger, statsReporter *StatsReporter) (*AdmissionController, error) {
-	return &AdmissionController{
-		Client:                client,
-		Options:               options,
-		DisallowUnknownFields: true,
-		// Use different versions and domains, for coverage.
-		Handlers: map[schema.GroupVersionKind]GenericCRD{
-			{
-				Group:   "pkg.knative.dev",
-				Version: "v1alpha1",
-				Kind:    "Resource",
-			}: &Resource{},
-			{
-				Group:   "pkg.knative.dev",
-				Version: "v1beta1",
-				Kind:    "Resource",
-			}: &Resource{},
-			{
-				Group:   "pkg.knative.dev",
-				Version: "v1alpha1",
-				Kind:    "InnerDefaultResource",
-			}: &InnerDefaultResource{},
-			{
-				Group:   "pkg.knative.io",
-				Version: "v1alpha1",
-				Kind:    "InnerDefaultResource",
-			}: &InnerDefaultResource{},
-		},
-		Logger:        logger,
-		StatsReporter: *statsReporter,
-	}, nil
+func NewTestAdmissionController(client kubernetes.Interface, options ControllerOptions,
+	logger *zap.SugaredLogger) (*AdmissionController, error) {
+	// Use different versions and domains, for coverage.
+	handlers := map[schema.GroupVersionKind]GenericCRD{
+		{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		}: &Resource{},
+		{
+			Group:   "pkg.knative.dev",
+			Version: "v1beta1",
+			Kind:    "Resource",
+		}: &Resource{},
+		{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "InnerDefaultResource",
+		}: &InnerDefaultResource{},
+		{
+			Group:   "pkg.knative.io",
+			Version: "v1alpha1",
+			Kind:    "InnerDefaultResource",
+		}: &InnerDefaultResource{},
+	}
+	return NewAdmissionController(client, options, handlers, logger, nil, true)
 }