default plan admission controller: filter list of service plans/service classes by the class name (openshift#1351)

* filter list of service plans/service classes by the specified service class name

* add success test determining default plan when there are multiple plans but each from a different classes

Author: Jay Boyd

plugin/pkg/admission/serviceplan/defaultserviceplan/admission.go

@@ -25,13 +25,11 @@ import (
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	apimachineryv1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apiserver/pkg/admission"
 
 	"github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/internalclientset"
 	servicecataloginternalversion "github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/internalclientset/typed/servicecatalog/internalversion"
-	informers "github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/internalversion"
-	internalversion "github.com/kubernetes-incubator/service-catalog/pkg/client/listers_generated/servicecatalog/internalversion"
 
 	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
 	scadmission "github.com/kubernetes-incubator/service-catalog/pkg/apiserver/admission"
@@ -57,17 +55,12 @@ func Register(plugins *admission.Plugins) {
 type defaultServicePlan struct {
 	*admission.Handler
 	scClient servicecataloginternalversion.ClusterServiceClassInterface
-	spLister internalversion.ClusterServicePlanLister
+	spClient servicecataloginternalversion.ClusterServicePlanInterface
 }
 
-var _ = scadmission.WantsInternalServiceCatalogInformerFactory(&defaultServicePlan{})
 var _ = scadmission.WantsInternalServiceCatalogClientSet(&defaultServicePlan{})
 
 func (d *defaultServicePlan) Admit(a admission.Attributes) error {
-	// we need to wait for our caches to warm
-	if !d.WaitForReady() {
-		return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request"))
-	}
 
 	// We only care about service Instances
 	if a.GetResource().Group != servicecatalog.GroupName || a.GetResource().GroupResource() != servicecatalog.Resource("serviceinstances") {
@@ -85,7 +78,7 @@ func (d *defaultServicePlan) Admit(a admission.Attributes) error {
 	}
 
 	// cannot find what we're trying to create an instance of
-	sc, err := d.getServiceClassByExternalName(a, instance.Spec.ExternalClusterServiceClassName)
+	sc, err := d.getClusterServiceClassByExternalName(a, instance.Spec.ExternalClusterServiceClassName)
 	if err != nil {
 		if !apierrors.IsNotFound(err) {
 			return admission.NewForbidden(a, err)
@@ -103,11 +96,12 @@ func (d *defaultServicePlan) Admit(a admission.Attributes) error {
 	// the order changes, we will need to rethink the
 	// implementation of this controller.
 
-	// implement field selector
-
-	// loop over all service plans accumulate into slice
-	plans, err := d.spLister.List(labels.Everything())
-	// TODO filter `plans` down to only those owned by `sc`.
+	plans, err := d.getClusterServicePlansByClusterServiceClassName(sc.Name)
+	if err != nil {
+		msg := fmt.Sprintf("Error listing plans for service class (K8S: %v ExternalName: %v) - retry and specify desired ClusterServicePlan", sc.Name, instance.Spec.ExternalClusterServiceClassName)
+		glog.V(4).Info(msg)
+		return admission.NewForbidden(a, errors.New(msg))
+	}
 
 	// check if there were any service plans
 	// TODO: in combination with not allowing classes with no plans, this should be impossible
@@ -144,42 +138,55 @@ func NewDefaultClusterServicePlan() (admission.Interface, error) {
 
 func (d *defaultServicePlan) SetInternalServiceCatalogClientSet(f internalclientset.Interface) {
 	d.scClient = f.Servicecatalog().ClusterServiceClasses()
-}
-
-func (d *defaultServicePlan) SetInternalServiceCatalogInformerFactory(f informers.SharedInformerFactory) {
-	spInformer := f.Servicecatalog().InternalVersion().ClusterServicePlans()
-	d.spLister = spInformer.Lister()
-
-	readyFunc := func() bool {
-		return spInformer.Informer().HasSynced()
-	}
-
-	d.SetReadyFunc(readyFunc)
+	d.spClient = f.Servicecatalog().ClusterServicePlans()
 }
 
 func (d *defaultServicePlan) Validate() error {
 	if d.scClient == nil {
 		return errors.New("missing service class interface")
 	}
-	if d.spLister == nil {
-		return errors.New("missing service plan lister")
+	if d.spClient == nil {
+		return errors.New("missing serviceplan interface")
 	}
 	return nil
 }
 
-func (d *defaultServicePlan) getServiceClassByExternalName(a admission.Attributes, scName string) (*servicecatalog.ClusterServiceClass, error) {
-	glog.V(4).Infof("Fetching serviceclass as %q", scName)
-	listOpts := apimachineryv1.ListOptions{FieldSelector: "spec.externalName==" + scName}
-	servicePlans, err := d.scClient.List(listOpts)
+func (d *defaultServicePlan) getClusterServiceClassByExternalName(a admission.Attributes, scName string) (*servicecatalog.ClusterServiceClass, error) {
+	glog.V(4).Infof("Fetching serviceclass filtered by class external name %q", scName)
+	fieldSet := fields.Set{
+		"spec.externalName": scName,
+	}
+	fieldSelector := fields.SelectorFromSet(fieldSet).String()
+	listOpts := apimachineryv1.ListOptions{FieldSelector: fieldSelector}
+	serviceClasses, err := d.scClient.List(listOpts)
 	if err != nil {
 		glog.V(4).Infof("List failed %q", err)
 		return nil, err
 	}
-	if len(servicePlans.Items) == 1 {
-		glog.V(4).Infof("Found Single item as %+v", servicePlans.Items[0])
-		return &servicePlans.Items[0], nil
+	if len(serviceClasses.Items) == 1 {
+		glog.V(4).Infof("Found Single item as %+v", serviceClasses.Items[0])
+		return &serviceClasses.Items[0], nil
 	}
-	msg := fmt.Sprintf("Could not find a single ServiceClass with name %q", scName)
+	msg := fmt.Sprintf("Could not find a single ServiceClass with name %q, found %v", scName, len(serviceClasses.Items))
 	glog.V(4).Info(msg)
 	return nil, admission.NewNotFound(a)
 }
+
+// getClusterServicePlansByClusterServiceClassName() returns a list of
+// ServicePlans for the specified service class name
+func (d *defaultServicePlan) getClusterServicePlansByClusterServiceClassName(scName string) ([]servicecatalog.ClusterServicePlan, error) {
+	glog.V(4).Infof("Fetching ClusterServicePlans by class name %q", scName)
+	fieldSet := fields.Set{
+		"spec.clusterServiceClassRef.name": scName,
+	}
+	fieldSelector := fields.SelectorFromSet(fieldSet).String()
+	listOpts := apimachineryv1.ListOptions{FieldSelector: fieldSelector}
+	servicePlans, err := d.spClient.List(listOpts)
+	if err != nil {
+		glog.Infof("List failed %q", err)
+		return nil, err
+	}
+	glog.V(4).Infof("plans fetched by filtering classname: %+v", servicePlans.Items)
+	r := servicePlans.Items
+	return r, err
+}

plugin/pkg/admission/serviceplan/defaultserviceplan/admission_test.go

@@ -24,6 +24,7 @@ import (
 
 	"github.com/golang/glog"
 
+	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -52,7 +53,7 @@ func newHandlerForTest(internalClient internalclientset.Interface) (admission.In
 
 // newFakeServiceCatalogClientForTest creates a fake clientset that returns a
 // ClusterServiceClassList with the given ClusterServiceClass as the single list item.
-func newFakeServiceCatalogClientForTest(sc *servicecatalog.ClusterServiceClass, sps []*servicecatalog.ClusterServicePlan) *fake.Clientset {
+func newFakeServiceCatalogClientForTest(sc *servicecatalog.ClusterServiceClass, sps []*servicecatalog.ClusterServicePlan, classFilter string) *fake.Clientset {
 	fakeClient := &fake.Clientset{}
 
 	// react to the given service classes
@@ -74,7 +75,9 @@ func newFakeServiceCatalogClientForTest(sc *servicecatalog.ClusterServiceClass,
 			ResourceVersion: "1",
 		}}
 	for _, sp := range sps {
-		spList.Items = append(spList.Items, *sp)
+		if classFilter == "" || classFilter == sp.Spec.ClusterServiceClassRef.Name {
+			spList.Items = append(spList.Items, *sp)
+		}
 	}
 	fakeClient.AddReactor("list", "clusterserviceplans", func(action core.Action) (bool, runtime.Object, error) {
 		return true, spList, nil
@@ -104,21 +107,30 @@ func newClusterServiceClass(id string, name string) *servicecatalog.ClusterServi
 	return sc
 }
 
-// newClusterServiceClass returns a new serviceclass.
-func newClusterServicePlans(count uint) []*servicecatalog.ClusterServicePlan {
+// newClusterServicePlans returns new serviceplans.
+func newClusterServicePlans(count uint, useDifferentClasses bool) []*servicecatalog.ClusterServicePlan {
+	classname := "test-serviceclass"
 	sp1 := &servicecatalog.ClusterServicePlan{
 		ObjectMeta: metav1.ObjectMeta{Name: "bar-id"},
 		Spec: servicecatalog.ClusterServicePlanSpec{
 			ExternalName: "bar",
 			ExternalID:   "12345",
+			ClusterServiceClassRef: v1.LocalObjectReference{
+				Name: classname,
+			},
 		},
 	}
-
+	if useDifferentClasses {
+		classname = "different-serviceclass"
+	}
 	sp2 := &servicecatalog.ClusterServicePlan{
 		ObjectMeta: metav1.ObjectMeta{Name: "baz-id"},
 		Spec: servicecatalog.ClusterServicePlanSpec{
 			ExternalName: "baz",
 			ExternalID:   "23456",
+			ClusterServiceClassRef: v1.LocalObjectReference{
+				Name: classname,
+			},
 		},
 	}
 
@@ -157,7 +169,7 @@ func TestWithListFailure(t *testing.T) {
 }
 
 func TestWithPlanWorks(t *testing.T) {
-	fakeClient := newFakeServiceCatalogClientForTest(nil, newClusterServicePlans(1))
+	fakeClient := newFakeServiceCatalogClientForTest(nil, newClusterServicePlans(1, false), "")
 	handler, informerFactory, err := newHandlerForTest(fakeClient)
 	if err != nil {
 		t.Errorf("unexpected error initializing handler: %v", err)
@@ -179,7 +191,7 @@ func TestWithPlanWorks(t *testing.T) {
 }
 
 func TestWithNoPlanFailsWithNoClusterServiceClass(t *testing.T) {
-	fakeClient := newFakeServiceCatalogClientForTest(nil, newClusterServicePlans(1))
+	fakeClient := newFakeServiceCatalogClientForTest(nil, newClusterServicePlans(1, false), "")
 	handler, informerFactory, err := newHandlerForTest(fakeClient)
 	if err != nil {
 		t.Errorf("unexpected error initializing handler: %v", err)
@@ -200,9 +212,9 @@ func TestWithNoPlanFailsWithNoClusterServiceClass(t *testing.T) {
 // checks that the defaulting action works when a service class only provides a single plan.
 func TestWithNoPlanWorksWithSinglePlan(t *testing.T) {
 	sc := newClusterServiceClass("foo-id", "foo")
-	sps := newClusterServicePlans(1)
+	sps := newClusterServicePlans(1, false)
 	glog.V(4).Infof("Created Service as %+v", sc)
-	fakeClient := newFakeServiceCatalogClientForTest(sc, sps)
+	fakeClient := newFakeServiceCatalogClientForTest(sc, sps, "")
 
 	handler, informerFactory, err := newHandlerForTest(fakeClient)
 	if err != nil {
@@ -230,9 +242,9 @@ func TestWithNoPlanWorksWithSinglePlan(t *testing.T) {
 // checks that defaulting fails when there are multiple plans to choose from.
 func TestWithNoPlanFailsWithMultiplePlans(t *testing.T) {
 	sc := newClusterServiceClass("foo-id", "foo")
-	sps := newClusterServicePlans(2)
+	sps := newClusterServicePlans(2, false)
 	glog.V(4).Infof("Created Service as %+v", sc)
-	fakeClient := newFakeServiceCatalogClientForTest(sc, sps)
+	fakeClient := newFakeServiceCatalogClientForTest(sc, sps, "")
 	handler, informerFactory, err := newHandlerForTest(fakeClient)
 	if err != nil {
 		t.Errorf("unexpected error initializing handler: %v", err)
@@ -250,3 +262,34 @@ func TestWithNoPlanFailsWithMultiplePlans(t *testing.T) {
 		t.Errorf("did not find expected error, got %q", err)
 	}
 }
+
+// checks that defaulting succeeds when there are multiple plans but only a
+// single plan for the specified Service Class
+func TestWithNoPlanSucceedsWithMultiplePlansFromDifferentClasses(t *testing.T) {
+	sc := newClusterServiceClass("foo-id", "foo")
+	sps := newClusterServicePlans(2, true)
+	glog.V(4).Infof("Created Service as %+v", sc)
+	classFilter := "test-serviceclass"
+	fakeClient := newFakeServiceCatalogClientForTest(sc, sps, classFilter)
+	handler, informerFactory, err := newHandlerForTest(fakeClient)
+	if err != nil {
+		t.Errorf("unexpected error initializing handler: %v", err)
+	}
+	informerFactory.Start(wait.NeverStop)
+
+	instance := newServiceInstance("dummy")
+	instance.Spec.ExternalClusterServiceClassName = "foo"
+
+	err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("ServiceInstance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("serviceinstances").WithVersion("version"), "", admission.Create, nil))
+	if err != nil {
+		actions := ""
+		for _, action := range fakeClient.Actions() {
+			actions = actions + action.GetVerb() + ":" + action.GetResource().Resource + ":" + action.GetSubresource() + ", "
+		}
+		t.Errorf("unexpected error %q returned from admission handler: %v", err, actions)
+	}
+	// Make sure the ServiceInstance has been mutated to include the service plan name
+	if instance.Spec.ExternalClusterServicePlanName != "bar" {
+		t.Errorf("PlanName was not modified for the default plan")
+	}
+}