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

@@ -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.)
@@ -118,7 +122,6 @@ type AdmissionController struct {
 	Options  ControllerOptions
 	Handlers map[schema.GroupVersionKind]GenericCRD
 	Logger   *zap.SugaredLogger
-	StatsReporter StatsReporter
 
 	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
 	}
 
+	if ac.Options.StatsReporter != nil {
 		// Only report valid requests
-	ac.StatsReporter.ReportRequest(review.Request, response.Response, time.Since(ttStart))
+		ac.Options.StatsReporter.ReportRequest(review.Request, response.Response, time.Since(ttStart))
+	}
 }
 
 func makeErrorStatus(reason string, args ...interface{}) *admissionv1beta1.AdmissionResponse {

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,14 +794,10 @@ func setUserAnnotation(userC, userU string) jsonpatch.JsonPatchOperation {
 	}
 }
 
-func NewAdmissionController(client kubernetes.Interface, options ControllerOptions,
+func NewTestAdmissionController(client kubernetes.Interface, options ControllerOptions,
-	logger *zap.SugaredLogger, statsReporter *StatsReporter) (*AdmissionController, error) {
+	logger *zap.SugaredLogger) (*AdmissionController, error) {
-	return &AdmissionController{
-		Client:                client,
-		Options:               options,
-		DisallowUnknownFields: true,
 	// Use different versions and domains, for coverage.
-		Handlers: map[schema.GroupVersionKind]GenericCRD{
+	handlers := map[schema.GroupVersionKind]GenericCRD{
 		{
 			Group:   "pkg.knative.dev",
 			Version: "v1alpha1",
@@ -826,8 +818,6 @@ func NewAdmissionController(client kubernetes.Interface, options ControllerOptio
 			Version: "v1alpha1",
 			Kind:    "InnerDefaultResource",
 		}: &InnerDefaultResource{},
-		},
+	}
-		Logger:        logger,
+	return NewAdmissionController(client, options, handlers, logger, nil, true)
-		StatsReporter: *statsReporter,
-	}, nil
 }