Skip to content

Commit

Permalink
default plan admission controller: filter list of service plans/servi…
Browse files Browse the repository at this point in the history 
…ce 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
  • Loading branch information
@pmorie
Jay Boyd authored and pmorie committed Oct 13, 2017
1 parent 6648c0e commit e64bbd1
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 47 deletions.
79 changes: 43 additions & 36 deletions plugin/pkg/admission/serviceplan/defaultserviceplan/admission.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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") {
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
}
65 changes: 54 additions & 11 deletions plugin/pkg/admission/serviceplan/defaultserviceplan/admission_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
},
},
}

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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")
}
}

0 comments on commit e64bbd1

Please sign in to comment.