Add test coverage of the Service controller code.

Unlike the other controllers, this follows the pattern of OpenShift's Ingress controller (thanks to @smarterclayton for the pointer). This enables us to have a largely table-driven approach to testing this, which makes adding tests surprisingly easy.

pkg/controller/configuration/configuration.go

@@ -63,40 +63,28 @@ func NewController(
 
 	informers := []cache.SharedIndexInformer{informer.Informer(), revisionInformer.Informer()}
 
-	controller := &Controller{
+	c := &Controller{
 		Base:           controller.NewBase(opt, controllerAgentName, "Configurations", informers),
 		buildClientSet: buildClientSet,
 		lister:         informer.Lister(),
 		revisionLister: revisionInformer.Lister(),
 	}
 
-	controller.Logger.Info("Setting up event handlers")
+	c.Logger.Info("Setting up event handlers")
 	informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
-		AddFunc: controller.Enqueue,
-		UpdateFunc: func(old, new interface{}) {
-			controller.Enqueue(new)
-		},
-		DeleteFunc: controller.Enqueue,
+		AddFunc:    c.Enqueue,
+		UpdateFunc: controller.PassSecond(c.Enqueue),
+		DeleteFunc: c.Enqueue,
 	})
 
 	revisionInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
-		FilterFunc: func(obj interface{}) bool {
-			if object, ok := obj.(metav1.Object); ok {
-				owner := metav1.GetControllerOf(object)
-				return owner != nil &&
-					owner.APIVersion == v1alpha1.SchemeGroupVersion.String() &&
-					owner.Kind == "Configuration"
-			}
-			return false
-		},
+		FilterFunc: controller.Filter("Configuration"),
 		Handler: cache.ResourceEventHandlerFuncs{
-			AddFunc: controller.EnqueueControllerOf,
-			UpdateFunc: func(old, new interface{}) {
-				controller.EnqueueControllerOf(new)
-			},
+			AddFunc:    c.EnqueueControllerOf,
+			UpdateFunc: controller.PassSecond(c.Enqueue),
 		},
 	})
-	return controller
+	return c
 }
 
 // Run will set up the event handlers for types we are interested in, as well

pkg/controller/configuration/configuration_test.go

@@ -36,6 +36,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/rest"
 
 	buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1"
 	fakebuildclientset "github.com/knative/build/pkg/client/clientset/versioned/fake"
@@ -45,8 +46,6 @@ import (
 	informers "github.com/knative/serving/pkg/client/informers/externalversions"
 	ctrl "github.com/knative/serving/pkg/controller"
 
-	"k8s.io/client-go/rest"
-
 	kubeinformers "k8s.io/client-go/informers"
 	fakekubeclientset "k8s.io/client-go/kubernetes/fake"
 

pkg/controller/controller.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	clientset "github.com/knative/serving/pkg/client/clientset/versioned"
 	elascheme "github.com/knative/serving/pkg/client/clientset/versioned/scheme"
 	"github.com/knative/serving/pkg/logging/logkey"
@@ -48,6 +49,27 @@ func init() {
 	elascheme.AddToScheme(scheme.Scheme)
 }
 
+func PassSecond(f func(interface{})) func(interface{}, interface{}) {
+	return func(first, second interface{}) {
+		f(second)
+	}
+}
+
+// Filter makes it simple to create FilterFunc's for use with
+// cache.FilteringResourceEventHandler that filter based on the
+// kind of the controlling resources.
+func Filter(kind string) func(obj interface{}) bool {
+	return func(obj interface{}) bool {
+		if object, ok := obj.(metav1.Object); ok {
+			owner := metav1.GetControllerOf(object)
+			return owner != nil &&
+				owner.APIVersion == v1alpha1.SchemeGroupVersion.String() &&
+				owner.Kind == kind
+		}
+		return false
+	}
+}
+
 // Base implements most of the boilerplate and common code
 // we have in our controllers.
 type Base struct {

pkg/controller/service/service.go

@@ -25,7 +25,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/cache"
@@ -64,53 +63,37 @@ func NewController(
 	informers := []cache.SharedIndexInformer{informer.Informer(),
 		configurationInformer.Informer(), routeInformer.Informer()}
 
-	controller := &Controller{
+	c := &Controller{
 		Base:                controller.NewBase(opt, controllerAgentName, "Services", informers),
 		lister:              informer.Lister(),
 		configurationLister: configurationInformer.Lister(),
 		routeLister:         routeInformer.Lister(),
 	}
 
-	controller.Logger.Info("Setting up event handlers")
+	c.Logger.Info("Setting up event handlers")
 	informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
-		AddFunc: controller.Enqueue,
-		UpdateFunc: func(old, new interface{}) {
-			controller.Enqueue(new)
-		},
-		DeleteFunc: controller.Enqueue,
+		AddFunc:    c.Enqueue,
+		UpdateFunc: controller.PassSecond(c.Enqueue),
+		DeleteFunc: c.Enqueue,
 	})
 
 	configurationInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
-		FilterFunc: isControlled,
+		FilterFunc: controller.Filter("Service"),
 		Handler: cache.ResourceEventHandlerFuncs{
-			AddFunc: controller.EnqueueControllerOf,
-			UpdateFunc: func(old, new interface{}) {
-				controller.EnqueueControllerOf(new)
-			},
+			AddFunc:    c.EnqueueControllerOf,
+			UpdateFunc: controller.PassSecond(c.Enqueue),
 		},
 	})
 
 	routeInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
-		FilterFunc: isControlled,
+		FilterFunc: controller.Filter("Service"),
 		Handler: cache.ResourceEventHandlerFuncs{
-			AddFunc: controller.EnqueueControllerOf,
-			UpdateFunc: func(old, new interface{}) {
-				controller.EnqueueControllerOf(new)
-			},
+			AddFunc:    c.EnqueueControllerOf,
+			UpdateFunc: controller.PassSecond(c.Enqueue),
 		},
 	})
 
-	return controller
-}
-
-func isControlled(obj interface{}) bool {
-	if object, ok := obj.(metav1.Object); ok {
-		owner := metav1.GetControllerOf(object)
-		return owner != nil &&
-			owner.APIVersion == v1alpha1.SchemeGroupVersion.String() &&
-			owner.Kind == "Service"
-	}
-	return false
+	return c
 }
 
 // Run will set up the event handlers for types we are interested in, as well
@@ -210,14 +193,14 @@ func (c *Controller) Reconcile(key string) error {
 }
 
 func (c *Controller) updateStatus(service *v1alpha1.Service) (*v1alpha1.Service, error) {
-	serviceClient := c.ElaClientSet.ServingV1alpha1().Services(service.Namespace)
-	existing, err := serviceClient.Get(service.Name, metav1.GetOptions{})
+	existing, err := c.lister.Services(service.Namespace).Get(service.Name)
 	if err != nil {
 		return nil, err
 	}
 	// Check if there is anything to update.
 	if !reflect.DeepEqual(existing.Status, service.Status) {
 		existing.Status = service.Status
+		serviceClient := c.ElaClientSet.ServingV1alpha1().Services(service.Namespace)
 		// TODO: for CRD there's no updatestatus, so use normal update.
 		return serviceClient.Update(existing)
 	}
@@ -226,12 +209,19 @@ func (c *Controller) updateStatus(service *v1alpha1.Service) (*v1alpha1.Service,
 
 func (c *Controller) createConfiguration(service *v1alpha1.Service) (*v1alpha1.Configuration, error) {
 	configClient := c.ElaClientSet.ServingV1alpha1().Configurations(service.Namespace)
-	return configClient.Create(MakeServiceConfiguration(service))
+	cfg, err := MakeServiceConfiguration(service)
+	if err != nil {
+		return nil, err
+	}
+	return configClient.Create(cfg)
 }
 
 func (c *Controller) reconcileConfiguration(service *v1alpha1.Service, config *v1alpha1.Configuration) (*v1alpha1.Configuration, error) {
 	logger := loggerWithServiceInfo(c.Logger, service.Namespace, service.Name)
-	desiredConfig := MakeServiceConfiguration(service)
+	desiredConfig, err := MakeServiceConfiguration(service)
+	if err != nil {
+		return nil, err
+	}
 
 	// TODO(#642): Remove this (needed to avoid continuous updates)
 	desiredConfig.Spec.Generation = config.Spec.Generation

pkg/controller/service/service_configuration.go

@@ -17,14 +17,16 @@ limitations under the License.
 package service
 
 import (
+	"errors"
+
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/knative/serving/pkg/controller"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // MakeServiceConfiguration creates a Configuration from a Service object.
-func MakeServiceConfiguration(service *v1alpha1.Service) *v1alpha1.Configuration {
+func MakeServiceConfiguration(service *v1alpha1.Service) (*v1alpha1.Configuration, error) {
 	c := &v1alpha1.Configuration{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      controller.GetServiceConfigurationName(service),
@@ -38,8 +40,10 @@ func MakeServiceConfiguration(service *v1alpha1.Service) *v1alpha1.Configuration
 
 	if service.Spec.RunLatest != nil {
 		c.Spec = service.Spec.RunLatest.Configuration
-	} else {
+	} else if service.Spec.Pinned != nil {
 		c.Spec = service.Spec.Pinned.Configuration
+	} else {
+		return nil, errors.New("malformed Service: one of runLatest or pinned must be present.")
 	}
-	return c
+	return c, nil
 }

pkg/controller/service/service_configuration_test.go

@@ -83,7 +83,7 @@ func createServiceWithPinned() *v1alpha1.Service {
 
 func TestRunLatest(t *testing.T) {
 	s := createServiceWithRunLatest()
-	c := MakeServiceConfiguration(s)
+	c, _ := MakeServiceConfiguration(s)
 	if got, want := c.Name, testServiceName; got != want {
 		t.Errorf("expected %q for service name got %q", want, got)
 	}
@@ -108,7 +108,7 @@ func TestRunLatest(t *testing.T) {
 
 func TestPinned(t *testing.T) {
 	s := createServiceWithPinned()
-	c := MakeServiceConfiguration(s)
+	c, _ := MakeServiceConfiguration(s)
 	if got, want := c.Name, testServiceName; got != want {
 		t.Errorf("expected %q for service name got %q", want, got)
 	}