From c1aabbbf68e7a94073e7cc9da20997ecc7f20b86 Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Thu, 14 Jun 2018 21:53:24 +0000 Subject: [PATCH 1/2] 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 | 30 +- .../configuration/configuration_test.go | 3 +- pkg/controller/controller.go | 22 + pkg/controller/service/service.go | 56 +-- .../service/service_configuration.go | 10 +- .../service/service_configuration_test.go | 4 +- pkg/controller/service/service_test.go | 463 +++++++++++++++++- pkg/controller/testing/listers.go | 175 +++++++ 8 files changed, 699 insertions(+), 64 deletions(-) create mode 100644 pkg/controller/testing/listers.go diff --git a/pkg/controller/configuration/configuration.go b/pkg/controller/configuration/configuration.go index a6403d9b9c43..ee38a273f615 100644 --- a/pkg/controller/configuration/configuration.go +++ b/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 diff --git a/pkg/controller/configuration/configuration_test.go b/pkg/controller/configuration/configuration_test.go index a8576d759532..5aa1730434d2 100644 --- a/pkg/controller/configuration/configuration_test.go +++ b/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" diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 196efbc57725..77c5a3e0c3e6 100644 --- a/pkg/controller/controller.go +++ b/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 { diff --git a/pkg/controller/service/service.go b/pkg/controller/service/service.go index 899e13e62eb9..89af7c72a6d7 100644 --- a/pkg/controller/service/service.go +++ b/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 diff --git a/pkg/controller/service/service_configuration.go b/pkg/controller/service/service_configuration.go index 1efb18fb62e3..499031325a32 100644 --- a/pkg/controller/service/service_configuration.go +++ b/pkg/controller/service/service_configuration.go @@ -17,6 +17,8 @@ limitations under the License. package service import ( + "errors" + "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/knative/serving/pkg/controller" @@ -24,7 +26,7 @@ import ( ) // 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 } diff --git a/pkg/controller/service/service_configuration_test.go b/pkg/controller/service/service_configuration_test.go index b1832232ce80..9401c8a8db61 100644 --- a/pkg/controller/service/service_configuration_test.go +++ b/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) } diff --git a/pkg/controller/service/service_test.go b/pkg/controller/service/service_test.go index 2f22f856ef18..18009cee92ef 100644 --- a/pkg/controller/service/service_test.go +++ b/pkg/controller/service/service_test.go @@ -14,10 +14,467 @@ limitations under the License. package service import ( + "strings" "testing" + + "github.com/google/go-cmp/cmp" + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + fakekubeclientset "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/rest" + clientgotesting "k8s.io/client-go/testing" + "k8s.io/client-go/tools/cache" + + "github.com/knative/serving/pkg/apis/serving" + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + fakeclientset "github.com/knative/serving/pkg/client/clientset/versioned/fake" + informers "github.com/knative/serving/pkg/client/informers/externalversions" + // listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" + "github.com/knative/serving/pkg/controller" + + . "github.com/knative/serving/pkg/controller/testing" ) -func testNothing(t *testing.T) { - // TODO(vaikas): Implement controller tests - return +var ( + boolTrue = true + configSpec = v1alpha1.ConfigurationSpec{ + RevisionTemplate: v1alpha1.RevisionTemplateSpec{ + Spec: v1alpha1.RevisionSpec{ + Container: corev1.Container{ + Image: "busybox", + }, + }, + }, + } +) + +// This is heavily based on the way the OpenShift Ingress controller tests its reconciliation method. +func TestReconcile(t *testing.T) { + type fields struct { + s *ServiceLister + r *RouteLister + c *ConfigurationLister + } + tests := []struct { + name string + fields fields + key string + wantErr bool + wantCreates []metav1.Object + wantUpdates []clientgotesting.UpdateActionImpl + wantDeletes []clientgotesting.DeleteActionImpl + wantQueue []string + }{{ + name: "bad workqueue key", + fields: fields{ + s: &ServiceLister{}, + r: &RouteLister{}, + c: &ConfigurationLister{}, + }, + key: "too/many/parts", + }, { + name: "key not found", + fields: fields{ + s: &ServiceLister{}, + r: &RouteLister{}, + c: &ConfigurationLister{}, + }, + key: "foo/not-found", + }, { + name: "incomplete service", + fields: fields{ + s: &ServiceLister{ + Items: []*v1alpha1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "incomplete", + Namespace: "foo", + }, + // There is no spec.{runLatest,pinned} in this + // Service to trigger the error condition. + }}, + }, + r: &RouteLister{}, + c: &ConfigurationLister{}, + }, + key: "foo/incomplete", + wantErr: true, + }, { + name: "runLatest - create route and service", + fields: fields{ + s: &ServiceLister{ + Items: []*v1alpha1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "run-latest", + Namespace: "foo", + }, + Spec: v1alpha1.ServiceSpec{ + RunLatest: &v1alpha1.RunLatestType{Configuration: configSpec}, + }, + }}, + }, + r: &RouteLister{}, + c: &ConfigurationLister{}, + }, + key: "foo/run-latest", + wantCreates: []metav1.Object{ + &v1alpha1.Configuration{ + ObjectMeta: om("foo", "run-latest"), + Spec: configSpec, + }, + &v1alpha1.Route{ + ObjectMeta: om("foo", "run-latest"), + Spec: runLatestSpec("run-latest"), + }, + }, + wantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: &v1alpha1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "run-latest", + Namespace: "foo", + }, + Spec: v1alpha1.ServiceSpec{ + RunLatest: &v1alpha1.RunLatestType{Configuration: configSpec}, + }, + Status: v1alpha1.ServiceStatus{ + Conditions: []v1alpha1.ServiceCondition{{ + Type: v1alpha1.ServiceConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionConfigurationReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionRouteReady, + Status: corev1.ConditionUnknown, + }}, + }, + }, + }}, + }, { + name: "pinned - create route and service", + fields: fields{ + s: &ServiceLister{ + Items: []*v1alpha1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinned", + Namespace: "foo", + }, + Spec: v1alpha1.ServiceSpec{ + Pinned: &v1alpha1.PinnedType{RevisionName: "pinned-0001", Configuration: configSpec}, + }, + }}, + }, + r: &RouteLister{}, + c: &ConfigurationLister{}, + }, + key: "foo/pinned", + wantCreates: []metav1.Object{ + &v1alpha1.Configuration{ + ObjectMeta: om("foo", "pinned"), + Spec: configSpec, + }, + &v1alpha1.Route{ + ObjectMeta: om("foo", "pinned"), + Spec: pinnedSpec("pinned-0001"), + }, + }, + wantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: &v1alpha1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinned", + Namespace: "foo", + }, + Spec: v1alpha1.ServiceSpec{ + Pinned: &v1alpha1.PinnedType{RevisionName: "pinned-0001", Configuration: configSpec}, + }, + Status: v1alpha1.ServiceStatus{ + Conditions: []v1alpha1.ServiceCondition{{ + Type: v1alpha1.ServiceConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionConfigurationReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionRouteReady, + Status: corev1.ConditionUnknown, + }}, + }, + }, + }}, + }, { + name: "runLatest - no updates", + fields: fields{ + s: &ServiceLister{ + Items: []*v1alpha1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-updates", + Namespace: "foo", + }, + Spec: v1alpha1.ServiceSpec{ + RunLatest: &v1alpha1.RunLatestType{Configuration: configSpec}, + }, + Status: v1alpha1.ServiceStatus{ + Conditions: []v1alpha1.ServiceCondition{{ + Type: v1alpha1.ServiceConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionConfigurationReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionRouteReady, + Status: corev1.ConditionUnknown, + }}, + }, + }}, + }, + r: &RouteLister{ + Items: []*v1alpha1.Route{{ + ObjectMeta: om("foo", "no-updates"), + Spec: runLatestSpec("no-updates"), + }}, + }, + c: &ConfigurationLister{ + Items: []*v1alpha1.Configuration{{ + ObjectMeta: om("foo", "no-updates"), + Spec: configSpec, + }}, + }, + }, + key: "foo/no-updates", + }, { + name: "runLatest - update route and service", + fields: fields{ + s: &ServiceLister{ + Items: []*v1alpha1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "update-route-and-config", + Namespace: "foo", + }, + Spec: v1alpha1.ServiceSpec{ + RunLatest: &v1alpha1.RunLatestType{Configuration: configSpec}, + }, + Status: v1alpha1.ServiceStatus{ + Conditions: []v1alpha1.ServiceCondition{{ + Type: v1alpha1.ServiceConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionConfigurationReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionRouteReady, + Status: corev1.ConditionUnknown, + }}, + }, + }}, + }, + // Update the skeletal Config/Route to have the appropriate {Config,Route}Specs + r: &RouteLister{ + Items: []*v1alpha1.Route{{ObjectMeta: om("foo", "update-route-and-config")}}, + }, + c: &ConfigurationLister{ + Items: []*v1alpha1.Configuration{{ObjectMeta: om("foo", "update-route-and-config")}}, + }, + }, + key: "foo/update-route-and-config", + wantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: &v1alpha1.Configuration{ + ObjectMeta: om("foo", "update-route-and-config"), + Spec: configSpec, + }, + }, { + Object: &v1alpha1.Route{ + ObjectMeta: om("foo", "update-route-and-config"), + Spec: runLatestSpec("update-route-and-config"), + }, + }}, + }, { + name: "runLatest - bad config update", + fields: fields{ + s: &ServiceLister{ + Items: []*v1alpha1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-config-update", + Namespace: "foo", + }, + Status: v1alpha1.ServiceStatus{ + Conditions: []v1alpha1.ServiceCondition{{ + Type: v1alpha1.ServiceConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionConfigurationReady, + Status: corev1.ConditionUnknown, + }, { + Type: v1alpha1.ServiceConditionRouteReady, + Status: corev1.ConditionUnknown, + }}, + }, + }}, + }, + // Update the skeletal Config/Route to have the appropriate {Config,Route}Specs + r: &RouteLister{ + Items: []*v1alpha1.Route{{ObjectMeta: om("foo", "bad-config-update")}}, + }, + c: &ConfigurationLister{ + Items: []*v1alpha1.Configuration{{ObjectMeta: om("foo", "bad-config-update")}}, + }, + }, + key: "foo/bad-config-update", + wantErr: true, + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var names []string + + var objs []runtime.Object + for _, s := range tt.fields.s.Items { + objs = append(objs, s) + } + for _, c := range tt.fields.c.Items { + objs = append(objs, c) + } + for _, r := range tt.fields.r.Items { + objs = append(objs, r) + } + + client := fakeclientset.NewSimpleClientset(objs...) + c := &Controller{ + Base: controller.NewBase(controller.Options{ + KubeClientSet: fakekubeclientset.NewSimpleClientset(), + ServingClientSet: client, + Logger: zap.NewNop().Sugar(), + }, controllerAgentName, "Services", nil), + lister: tt.fields.s, + configurationLister: tt.fields.c, + routeLister: tt.fields.r, + } + + if err := c.Reconcile(tt.key); (err != nil) != tt.wantErr { + t.Errorf("Reconcile() error = %v, wantErr %v", err, tt.wantErr) + } + expectedNamespace, _, _ := cache.SplitMetaNamespaceKey(tt.key) + + c.WorkQueue.ShutDown() + var hasQueue []string + for { + key, shutdown := c.WorkQueue.Get() + if shutdown { + break + } + hasQueue = append(hasQueue, key.(string)) + } + if diff := cmp.Diff(tt.wantQueue, hasQueue); diff != "" { + t.Errorf("unexpected queue (-want +got): %s", diff) + } + + actions := client.Actions() + + for i := range tt.wantCreates { + if i > len(actions)-1 { + t.Fatalf("Reconcile() unexpected actions: %#v", client.Actions()) + } + if actions[i].GetVerb() != "create" { + t.Fatalf("Reconcile() unexpected actions: %#v", client.Actions()) + } + action := actions[i].(clientgotesting.CreateAction) + if action.GetNamespace() != expectedNamespace { + t.Errorf("unexpected action[%d]: %#v", i, action) + } + obj := action.GetObject() + if tt.wantCreates[i].GetName() == "" { + tt.wantCreates[i].SetName(names[0]) + names = names[1:] + } + if diff := cmp.Diff(tt.wantCreates[i], obj, ignoreLastTransitionTime); diff != "" { + t.Errorf("unexpected create (-want +got): %s", diff) + } + } + actions = actions[len(tt.wantCreates):] + + for i := range tt.wantUpdates { + if i > len(actions)-1 { + t.Fatalf("Reconcile() unexpected actions: %#v", client.Actions()) + } + if actions[i].GetVerb() != "update" { + t.Fatalf("Reconcile() unexpected actions: %#v", client.Actions()) + } + action := actions[i].(clientgotesting.UpdateAction) + if diff := cmp.Diff(tt.wantUpdates[i].GetObject(), action.GetObject(), ignoreLastTransitionTime); diff != "" { + t.Errorf("unexpected update (-want +got): %s", diff) + } + } + actions = actions[len(tt.wantUpdates):] + + for i := range tt.wantDeletes { + if i > len(actions)-1 { + t.Fatalf("Reconcile() unexpected actions: %#v", client.Actions()) + } + if actions[i].GetVerb() != "delete" { + t.Fatalf("Reconcile() unexpected actions: %#v", client.Actions()) + } + action := actions[i].(clientgotesting.DeleteAction) + if action.GetName() != tt.wantDeletes[i].Name || action.GetNamespace() != expectedNamespace { + t.Errorf("unexpected action[%d]: %#v", i, action) + } + } + actions = actions[len(tt.wantDeletes):] + + if len(actions) != 0 { + t.Fatalf("Reconcile() unexpected actions: %#v", actions) + } + }) + } +} + +func TestNew(t *testing.T) { + kubeClient := fakekubeclientset.NewSimpleClientset() + servingClient := fakeclientset.NewSimpleClientset() + servingInformer := informers.NewSharedInformerFactory(servingClient, 0) + _ = NewController(controller.Options{ + KubeClientSet: kubeClient, + ServingClientSet: servingClient, + Logger: zap.NewNop().Sugar(), + }, servingInformer, &rest.Config{}) +} + +var ignoreLastTransitionTime = cmp.FilterPath(func(p cmp.Path) bool { + return strings.HasSuffix(p.String(), "LastTransitionTime.Time") +}, cmp.Ignore()) + +func runLatestSpec(name string) v1alpha1.RouteSpec { + return v1alpha1.RouteSpec{ + Traffic: []v1alpha1.TrafficTarget{{ + ConfigurationName: name, + Percent: 100, + }}, + } +} + +func pinnedSpec(name string) v1alpha1.RouteSpec { + return v1alpha1.RouteSpec{ + Traffic: []v1alpha1.TrafficTarget{{ + RevisionName: name, + Percent: 100, + }}, + } +} + +func or(name string) []metav1.OwnerReference { + return []metav1.OwnerReference{{ + APIVersion: v1alpha1.SchemeGroupVersion.String(), + Kind: "Service", + Name: name, + Controller: &boolTrue, + BlockOwnerDeletion: &boolTrue, + }} +} + +func om(namespace, name string) metav1.ObjectMeta { + return metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{serving.ServiceLabelKey: name}, + OwnerReferences: or(name), + } } diff --git a/pkg/controller/testing/listers.go b/pkg/controller/testing/listers.go new file mode 100644 index 000000000000..81624e821477 --- /dev/null +++ b/pkg/controller/testing/listers.go @@ -0,0 +1,175 @@ +/* +Copyright 2018 Google LLC +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 testing + +import ( + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" +) + +// ServiceLister is a lister.ServiceLister fake for testing. +type ServiceLister struct { + Err error + Items []*v1alpha1.Service +} + +// Assert that our fake implements the interface it is faking. +var _ listers.ServiceLister = (*ServiceLister)(nil) + +func (r *ServiceLister) List(selector labels.Selector) (ret []*v1alpha1.Service, err error) { + return r.Items, r.Err +} + +func (r *ServiceLister) Services(namespace string) listers.ServiceNamespaceLister { + return &nsServiceLister{r: r, ns: namespace} +} + +type nsServiceLister struct { + r *ServiceLister + ns string +} + +// Assert that our fake implements the interface it is faking. +var _ listers.ServiceNamespaceLister = (*nsServiceLister)(nil) + +func (r *nsServiceLister) List(selector labels.Selector) (ret []*v1alpha1.Service, err error) { + return r.r.Items, r.r.Err +} + +func (r *nsServiceLister) Get(name string) (*v1alpha1.Service, error) { + for _, s := range r.r.Items { + if s.Name == name && r.ns == s.Namespace { + return s, nil + } + } + return nil, errors.NewNotFound(schema.GroupResource{}, name) +} + +// RouteLister is a lister.RouteLister fake for testing. +type RouteLister struct { + Err error + Items []*v1alpha1.Route +} + +// Assert that our fake implements the interface it is faking. +var _ listers.RouteLister = (*RouteLister)(nil) + +func (r *RouteLister) List(selector labels.Selector) (ret []*v1alpha1.Route, err error) { + return r.Items, r.Err +} + +func (r *RouteLister) Routes(namespace string) listers.RouteNamespaceLister { + return &nsRouteLister{r: r, ns: namespace} +} + +type nsRouteLister struct { + r *RouteLister + ns string +} + +// Assert that our fake implements the interface it is faking. +var _ listers.RouteNamespaceLister = (*nsRouteLister)(nil) + +func (r *nsRouteLister) List(selector labels.Selector) (ret []*v1alpha1.Route, err error) { + return r.r.Items, r.r.Err +} + +func (r *nsRouteLister) Get(name string) (*v1alpha1.Route, error) { + for _, s := range r.r.Items { + if s.Name == name && r.ns == s.Namespace { + return s, nil + } + } + return nil, errors.NewNotFound(schema.GroupResource{}, name) +} + +// ConfigurationLister is a lister.ConfigurationLister fake for testing. +type ConfigurationLister struct { + Err error + Items []*v1alpha1.Configuration +} + +// Assert that our fake implements the interface it is faking. +var _ listers.ConfigurationLister = (*ConfigurationLister)(nil) + +func (r *ConfigurationLister) List(selector labels.Selector) (ret []*v1alpha1.Configuration, err error) { + return r.Items, r.Err +} + +func (r *ConfigurationLister) Configurations(namespace string) listers.ConfigurationNamespaceLister { + return &nsConfigurationLister{r: r, ns: namespace} +} + +type nsConfigurationLister struct { + r *ConfigurationLister + ns string +} + +// Assert that our fake implements the interface it is faking. +var _ listers.ConfigurationNamespaceLister = (*nsConfigurationLister)(nil) + +func (r *nsConfigurationLister) List(selector labels.Selector) (ret []*v1alpha1.Configuration, err error) { + return r.r.Items, r.r.Err +} + +func (r *nsConfigurationLister) Get(name string) (*v1alpha1.Configuration, error) { + for _, s := range r.r.Items { + if s.Name == name && r.ns == s.Namespace { + return s, nil + } + } + return nil, errors.NewNotFound(schema.GroupResource{}, name) +} + +// RevisionLister is a lister.RevisionLister fake for testing. +type RevisionLister struct { + Err error + Items []*v1alpha1.Revision +} + +// Assert that our fake implements the interface it is faking. +var _ listers.RevisionLister = (*RevisionLister)(nil) + +func (r *RevisionLister) List(selector labels.Selector) (ret []*v1alpha1.Revision, err error) { + return r.Items, r.Err +} + +func (r *RevisionLister) Revisions(namespace string) listers.RevisionNamespaceLister { + return &nsRevisionLister{r: r, ns: namespace} +} + +type nsRevisionLister struct { + r *RevisionLister + ns string +} + +// Assert that our fake implements the interface it is faking. +var _ listers.RevisionNamespaceLister = (*nsRevisionLister)(nil) + +func (r *nsRevisionLister) List(selector labels.Selector) (ret []*v1alpha1.Revision, err error) { + return r.r.Items, r.r.Err +} + +func (r *nsRevisionLister) Get(name string) (*v1alpha1.Revision, error) { + for _, s := range r.r.Items { + if s.Name == name && r.ns == s.Namespace { + return s, nil + } + } + return nil, errors.NewNotFound(schema.GroupResource{}, name) +} From 984af5f52fd0c62502508a9e41c26eb51d46e414 Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Fri, 15 Jun 2018 14:01:25 +0000 Subject: [PATCH 2/2] Incorporate review feedback. Dropped named returns from listers.go Rename and document the UpdateFunc helper. --- pkg/controller/configuration/configuration.go | 4 ++-- pkg/controller/controller.go | 6 +++++- pkg/controller/service/service.go | 6 +++--- pkg/controller/testing/listers.go | 16 ++++++++-------- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/pkg/controller/configuration/configuration.go b/pkg/controller/configuration/configuration.go index ee38a273f615..99696c8acb90 100644 --- a/pkg/controller/configuration/configuration.go +++ b/pkg/controller/configuration/configuration.go @@ -73,7 +73,7 @@ func NewController( c.Logger.Info("Setting up event handlers") informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: c.Enqueue, - UpdateFunc: controller.PassSecond(c.Enqueue), + UpdateFunc: controller.PassNew(c.Enqueue), DeleteFunc: c.Enqueue, }) @@ -81,7 +81,7 @@ func NewController( FilterFunc: controller.Filter("Configuration"), Handler: cache.ResourceEventHandlerFuncs{ AddFunc: c.EnqueueControllerOf, - UpdateFunc: controller.PassSecond(c.Enqueue), + UpdateFunc: controller.PassNew(c.Enqueue), }, }) return c diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 77c5a3e0c3e6..c80e1bec3189 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -49,7 +49,11 @@ func init() { elascheme.AddToScheme(scheme.Scheme) } -func PassSecond(f func(interface{})) func(interface{}, interface{}) { +// PassNew makes it simple to create an UpdateFunc for use with +// cache.ResourceEventHandlerFuncs that can delegate the same methods +// as AddFunc/DeleteFunc but passing through only the second argument +// (which is the "new" object). +func PassNew(f func(interface{})) func(interface{}, interface{}) { return func(first, second interface{}) { f(second) } diff --git a/pkg/controller/service/service.go b/pkg/controller/service/service.go index 89af7c72a6d7..8b21ef06b7ee 100644 --- a/pkg/controller/service/service.go +++ b/pkg/controller/service/service.go @@ -73,7 +73,7 @@ func NewController( c.Logger.Info("Setting up event handlers") informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: c.Enqueue, - UpdateFunc: controller.PassSecond(c.Enqueue), + UpdateFunc: controller.PassNew(c.Enqueue), DeleteFunc: c.Enqueue, }) @@ -81,7 +81,7 @@ func NewController( FilterFunc: controller.Filter("Service"), Handler: cache.ResourceEventHandlerFuncs{ AddFunc: c.EnqueueControllerOf, - UpdateFunc: controller.PassSecond(c.Enqueue), + UpdateFunc: controller.PassNew(c.Enqueue), }, }) @@ -89,7 +89,7 @@ func NewController( FilterFunc: controller.Filter("Service"), Handler: cache.ResourceEventHandlerFuncs{ AddFunc: c.EnqueueControllerOf, - UpdateFunc: controller.PassSecond(c.Enqueue), + UpdateFunc: controller.PassNew(c.Enqueue), }, }) diff --git a/pkg/controller/testing/listers.go b/pkg/controller/testing/listers.go index 81624e821477..499c252daf66 100644 --- a/pkg/controller/testing/listers.go +++ b/pkg/controller/testing/listers.go @@ -31,7 +31,7 @@ type ServiceLister struct { // Assert that our fake implements the interface it is faking. var _ listers.ServiceLister = (*ServiceLister)(nil) -func (r *ServiceLister) List(selector labels.Selector) (ret []*v1alpha1.Service, err error) { +func (r *ServiceLister) List(selector labels.Selector) ([]*v1alpha1.Service, error) { return r.Items, r.Err } @@ -47,7 +47,7 @@ type nsServiceLister struct { // Assert that our fake implements the interface it is faking. var _ listers.ServiceNamespaceLister = (*nsServiceLister)(nil) -func (r *nsServiceLister) List(selector labels.Selector) (ret []*v1alpha1.Service, err error) { +func (r *nsServiceLister) List(selector labels.Selector) ([]*v1alpha1.Service, error) { return r.r.Items, r.r.Err } @@ -69,7 +69,7 @@ type RouteLister struct { // Assert that our fake implements the interface it is faking. var _ listers.RouteLister = (*RouteLister)(nil) -func (r *RouteLister) List(selector labels.Selector) (ret []*v1alpha1.Route, err error) { +func (r *RouteLister) List(selector labels.Selector) ([]*v1alpha1.Route, error) { return r.Items, r.Err } @@ -85,7 +85,7 @@ type nsRouteLister struct { // Assert that our fake implements the interface it is faking. var _ listers.RouteNamespaceLister = (*nsRouteLister)(nil) -func (r *nsRouteLister) List(selector labels.Selector) (ret []*v1alpha1.Route, err error) { +func (r *nsRouteLister) List(selector labels.Selector) ([]*v1alpha1.Route, error) { return r.r.Items, r.r.Err } @@ -107,7 +107,7 @@ type ConfigurationLister struct { // Assert that our fake implements the interface it is faking. var _ listers.ConfigurationLister = (*ConfigurationLister)(nil) -func (r *ConfigurationLister) List(selector labels.Selector) (ret []*v1alpha1.Configuration, err error) { +func (r *ConfigurationLister) List(selector labels.Selector) ([]*v1alpha1.Configuration, error) { return r.Items, r.Err } @@ -123,7 +123,7 @@ type nsConfigurationLister struct { // Assert that our fake implements the interface it is faking. var _ listers.ConfigurationNamespaceLister = (*nsConfigurationLister)(nil) -func (r *nsConfigurationLister) List(selector labels.Selector) (ret []*v1alpha1.Configuration, err error) { +func (r *nsConfigurationLister) List(selector labels.Selector) ([]*v1alpha1.Configuration, error) { return r.r.Items, r.r.Err } @@ -145,7 +145,7 @@ type RevisionLister struct { // Assert that our fake implements the interface it is faking. var _ listers.RevisionLister = (*RevisionLister)(nil) -func (r *RevisionLister) List(selector labels.Selector) (ret []*v1alpha1.Revision, err error) { +func (r *RevisionLister) List(selector labels.Selector) ([]*v1alpha1.Revision, error) { return r.Items, r.Err } @@ -161,7 +161,7 @@ type nsRevisionLister struct { // Assert that our fake implements the interface it is faking. var _ listers.RevisionNamespaceLister = (*nsRevisionLister)(nil) -func (r *nsRevisionLister) List(selector labels.Selector) (ret []*v1alpha1.Revision, err error) { +func (r *nsRevisionLister) List(selector labels.Selector) ([]*v1alpha1.Revision, error) { return r.r.Items, r.r.Err }