Skip to content

Commit 2095919

Browse files
Ville Aikasarschles
Ville Aikas
authored and
arschles
committed
Handle default plan setting when using k8s names (#1405)
* Handle default plan setting when using k8s names * address golint issues
1 parent 607ba66 commit 2095919

File tree

3 files changed

+230
-38
lines changed

3 files changed

+230
-38
lines changed

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

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ func (d *defaultServicePlan) Admit(a admission.Attributes) error {
7878
}
7979

8080
// cannot find what we're trying to create an instance of
81-
sc, err := d.getClusterServiceClassByExternalName(a, instance.Spec.ExternalClusterServiceClassName)
81+
sc, err := d.getClusterServiceClassByPlanReference(a, &instance.Spec.PlanReference)
8282
if err != nil {
8383
if !apierrors.IsNotFound(err) {
8484
return admission.NewForbidden(a, err)
@@ -120,9 +120,13 @@ func (d *defaultServicePlan) Admit(a admission.Attributes) error {
120120
// otherwise, by default, pick the only plan that exists for the service class
121121

122122
p := plans[0]
123-
glog.V(4).Infof("Using default plan %q for Service Class %q for instance %s",
124-
p.Spec.ExternalName, sc.Spec.ExternalName, instance.Name)
125-
instance.Spec.ExternalClusterServicePlanName = p.Spec.ExternalName
123+
glog.V(4).Infof("Using default plan %q (K8S: %q) for Service Class %q for instance %s",
124+
p.Spec.ExternalName, p.Name, sc.Spec.ExternalName, instance.Name)
125+
if instance.Spec.ExternalClusterServiceClassName != "" {
126+
instance.Spec.ExternalClusterServicePlanName = p.Spec.ExternalName
127+
} else {
128+
instance.Spec.ClusterServicePlanName = p.Name
129+
}
126130
return nil
127131
}
128132

@@ -151,6 +155,18 @@ func (d *defaultServicePlan) Validate() error {
151155
return nil
152156
}
153157

158+
func (d *defaultServicePlan) getClusterServiceClassByPlanReference(a admission.Attributes, ref *servicecatalog.PlanReference) (*servicecatalog.ClusterServiceClass, error) {
159+
if ref.ExternalClusterServiceClassName != "" {
160+
return d.getClusterServiceClassByExternalName(a, ref.ExternalClusterServiceClassName)
161+
}
162+
return d.getClusterServiceClassByK8SName(a, ref.ClusterServiceClassName)
163+
}
164+
165+
func (d *defaultServicePlan) getClusterServiceClassByK8SName(a admission.Attributes, scK8SName string) (*servicecatalog.ClusterServiceClass, error) {
166+
glog.V(4).Infof("Fetching serviceclass by class k8s name %q", scK8SName)
167+
return d.scClient.Get(scK8SName, apimachineryv1.GetOptions{})
168+
}
169+
154170
func (d *defaultServicePlan) getClusterServiceClassByExternalName(a admission.Attributes, scName string) (*servicecatalog.ClusterServiceClass, error) {
155171
glog.V(4).Infof("Fetching serviceclass filtered by class external name %q", scName)
156172
fieldSet := fields.Set{

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

Lines changed: 164 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,10 @@ import (
2525
"github.com/golang/glog"
2626

2727
"k8s.io/api/core/v1"
28+
apierrors "k8s.io/apimachinery/pkg/api/errors"
2829
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2930
"k8s.io/apimachinery/pkg/runtime"
31+
"k8s.io/apimachinery/pkg/runtime/schema"
3032
"k8s.io/apimachinery/pkg/util/wait"
3133
"k8s.io/apiserver/pkg/admission"
3234
core "k8s.io/client-go/testing"
@@ -52,22 +54,34 @@ func newHandlerForTest(internalClient internalclientset.Interface) (admission.In
5254
}
5355

5456
// newFakeServiceCatalogClientForTest creates a fake clientset that returns a
55-
// ClusterServiceClassList with the given ClusterServiceClass as the single list item.
56-
func newFakeServiceCatalogClientForTest(sc *servicecatalog.ClusterServiceClass, sps []*servicecatalog.ClusterServicePlan, classFilter string) *fake.Clientset {
57+
// ClusterServiceClassList with the given ClusterServiceClass as the single list item
58+
// and list of ClusterServicePlan objects.
59+
// If classFilter is provided (as in not ""), then only sps with the
60+
// Spec.ClusterServiceClassRef.Name equaling that string will be added to the list.
61+
func newFakeServiceCatalogClientForTest(sc *servicecatalog.ClusterServiceClass, sps []*servicecatalog.ClusterServicePlan, classFilter string, useGet bool) *fake.Clientset {
5762
fakeClient := &fake.Clientset{}
5863

59-
// react to the given service classes
60-
scList := &servicecatalog.ClusterServiceClassList{
61-
ListMeta: metav1.ListMeta{
62-
ResourceVersion: "1",
63-
}}
64-
if sc != nil {
65-
scList.Items = append(scList.Items, *sc)
66-
}
64+
if useGet {
65+
fakeClient.AddReactor("get", "clusterserviceclasses", func(action core.Action) (bool, runtime.Object, error) {
66+
if sc != nil {
67+
return true, sc, nil
68+
}
69+
return true, nil, apierrors.NewNotFound(schema.GroupResource{}, "")
70+
})
71+
} else {
72+
// react to the given service class list
73+
scList := &servicecatalog.ClusterServiceClassList{
74+
ListMeta: metav1.ListMeta{
75+
ResourceVersion: "1",
76+
}}
77+
if sc != nil {
78+
scList.Items = append(scList.Items, *sc)
79+
}
6780

68-
fakeClient.AddReactor("list", "clusterserviceclasses", func(action core.Action) (bool, runtime.Object, error) {
69-
return true, scList, nil
70-
})
81+
fakeClient.AddReactor("list", "clusterserviceclasses", func(action core.Action) (bool, runtime.Object, error) {
82+
return true, scList, nil
83+
})
84+
}
7185

7286
// react to the given plans
7387
spList := &servicecatalog.ClusterServicePlanList{
@@ -166,10 +180,13 @@ func TestWithListFailure(t *testing.T) {
166180
} else if !strings.Contains(err.Error(), "simulated test failure") {
167181
t.Errorf("did not find expected error, got %q", err)
168182
}
183+
assertPlanReference(t,
184+
servicecatalog.PlanReference{ExternalClusterServiceClassName: "foo"},
185+
instance.Spec.PlanReference)
169186
}
170187

171188
func TestWithPlanWorks(t *testing.T) {
172-
fakeClient := newFakeServiceCatalogClientForTest(nil, newClusterServicePlans(1, false), "")
189+
fakeClient := newFakeServiceCatalogClientForTest(nil, newClusterServicePlans(1, false), "", false /* do not use get */)
173190
handler, informerFactory, err := newHandlerForTest(fakeClient)
174191
if err != nil {
175192
t.Errorf("unexpected error initializing handler: %v", err)
@@ -188,10 +205,13 @@ func TestWithPlanWorks(t *testing.T) {
188205
}
189206
t.Errorf("unexpected error %q returned from admission handler: %v", err, actions)
190207
}
208+
assertPlanReference(t,
209+
servicecatalog.PlanReference{ExternalClusterServiceClassName: "foo", ExternalClusterServicePlanName: "bar"},
210+
instance.Spec.PlanReference)
191211
}
192212

193213
func TestWithNoPlanFailsWithNoClusterServiceClass(t *testing.T) {
194-
fakeClient := newFakeServiceCatalogClientForTest(nil, newClusterServicePlans(1, false), "")
214+
fakeClient := newFakeServiceCatalogClientForTest(nil, newClusterServicePlans(1, false), "", false /* do not use get */)
195215
handler, informerFactory, err := newHandlerForTest(fakeClient)
196216
if err != nil {
197217
t.Errorf("unexpected error initializing handler: %v", err)
@@ -207,14 +227,17 @@ func TestWithNoPlanFailsWithNoClusterServiceClass(t *testing.T) {
207227
} else if !strings.Contains(err.Error(), "does not exist, can not figure") {
208228
t.Errorf("did not find expected error, got %q", err)
209229
}
230+
assertPlanReference(t,
231+
servicecatalog.PlanReference{ExternalClusterServiceClassName: "foobar"},
232+
instance.Spec.PlanReference)
210233
}
211234

212235
// checks that the defaulting action works when a service class only provides a single plan.
213236
func TestWithNoPlanWorksWithSinglePlan(t *testing.T) {
214237
sc := newClusterServiceClass("foo-id", "foo")
215238
sps := newClusterServicePlans(1, false)
216239
glog.V(4).Infof("Created Service as %+v", sc)
217-
fakeClient := newFakeServiceCatalogClientForTest(sc, sps, "")
240+
fakeClient := newFakeServiceCatalogClientForTest(sc, sps, "", false /* do not use get */)
218241

219242
handler, informerFactory, err := newHandlerForTest(fakeClient)
220243
if err != nil {
@@ -233,18 +256,17 @@ func TestWithNoPlanWorksWithSinglePlan(t *testing.T) {
233256
}
234257
t.Errorf("unexpected error %q returned from admission handler: %v", err, actions)
235258
}
236-
// Make sure the ServiceInstance has been mutated to include the service plan name
237-
if instance.Spec.ExternalClusterServicePlanName != "bar" {
238-
t.Errorf("PlanName was not modified for the default plan")
239-
}
259+
assertPlanReference(t,
260+
servicecatalog.PlanReference{ExternalClusterServiceClassName: "foo", ExternalClusterServicePlanName: "bar"},
261+
instance.Spec.PlanReference)
240262
}
241263

242264
// checks that defaulting fails when there are multiple plans to choose from.
243265
func TestWithNoPlanFailsWithMultiplePlans(t *testing.T) {
244266
sc := newClusterServiceClass("foo-id", "foo")
245267
sps := newClusterServicePlans(2, false)
246268
glog.V(4).Infof("Created Service as %+v", sc)
247-
fakeClient := newFakeServiceCatalogClientForTest(sc, sps, "")
269+
fakeClient := newFakeServiceCatalogClientForTest(sc, sps, "", false /* do not use get */)
248270
handler, informerFactory, err := newHandlerForTest(fakeClient)
249271
if err != nil {
250272
t.Errorf("unexpected error initializing handler: %v", err)
@@ -261,6 +283,9 @@ func TestWithNoPlanFailsWithMultiplePlans(t *testing.T) {
261283
} else if !strings.Contains(err.Error(), "has more than one plan, PlanName must be") {
262284
t.Errorf("did not find expected error, got %q", err)
263285
}
286+
assertPlanReference(t,
287+
servicecatalog.PlanReference{ExternalClusterServiceClassName: "foo"},
288+
instance.Spec.PlanReference)
264289
}
265290

266291
// checks that defaulting succeeds when there are multiple plans but only a
@@ -270,7 +295,7 @@ func TestWithNoPlanSucceedsWithMultiplePlansFromDifferentClasses(t *testing.T) {
270295
sps := newClusterServicePlans(2, true)
271296
glog.V(4).Infof("Created Service as %+v", sc)
272297
classFilter := "test-serviceclass"
273-
fakeClient := newFakeServiceCatalogClientForTest(sc, sps, classFilter)
298+
fakeClient := newFakeServiceCatalogClientForTest(sc, sps, classFilter, false /* do not use get */)
274299
handler, informerFactory, err := newHandlerForTest(fakeClient)
275300
if err != nil {
276301
t.Errorf("unexpected error initializing handler: %v", err)
@@ -288,8 +313,122 @@ func TestWithNoPlanSucceedsWithMultiplePlansFromDifferentClasses(t *testing.T) {
288313
}
289314
t.Errorf("unexpected error %q returned from admission handler: %v", err, actions)
290315
}
291-
// Make sure the ServiceInstance has been mutated to include the service plan name
292-
if instance.Spec.ExternalClusterServicePlanName != "bar" {
293-
t.Errorf("PlanName was not modified for the default plan")
316+
assertPlanReference(t,
317+
servicecatalog.PlanReference{ExternalClusterServiceClassName: "foo", ExternalClusterServicePlanName: "bar"},
318+
instance.Spec.PlanReference)
319+
}
320+
321+
func TestWithNoPlanFailsWithNoClusterServiceClassK8SName(t *testing.T) {
322+
fakeClient := newFakeServiceCatalogClientForTest(nil, newClusterServicePlans(1, false), "", true /* use get */)
323+
handler, informerFactory, err := newHandlerForTest(fakeClient)
324+
if err != nil {
325+
t.Errorf("unexpected error initializing handler: %v", err)
326+
}
327+
informerFactory.Start(wait.NeverStop)
328+
329+
instance := newServiceInstance("dummy")
330+
instance.Spec.ClusterServiceClassName = "foobar-id"
331+
332+
err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("ServiceInstance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("serviceinstances").WithVersion("version"), "", admission.Create, nil))
333+
if err == nil {
334+
t.Errorf("unexpected success with no plan specified and no serviceclass existing")
335+
} else if !strings.Contains(err.Error(), "does not exist, can not figure") {
336+
t.Errorf("did not find expected error, got %q", err)
337+
}
338+
assertPlanReference(t,
339+
servicecatalog.PlanReference{ClusterServiceClassName: "foobar-id"},
340+
instance.Spec.PlanReference)
341+
}
342+
343+
// checks that the defaulting action works when a service class only provides a single plan.
344+
func TestWithNoPlanWorksWithSinglePlanK8SName(t *testing.T) {
345+
sc := newClusterServiceClass("foo-id", "foo")
346+
sps := newClusterServicePlans(1, false)
347+
glog.V(4).Infof("Created Service as %+v", sc)
348+
fakeClient := newFakeServiceCatalogClientForTest(sc, sps, "", true /* use get */)
349+
350+
handler, informerFactory, err := newHandlerForTest(fakeClient)
351+
if err != nil {
352+
t.Errorf("unexpected error initializing handler: %v", err)
353+
}
354+
informerFactory.Start(wait.NeverStop)
355+
356+
instance := newServiceInstance("dummy")
357+
instance.Spec.ClusterServiceClassName = "foo-id"
358+
359+
err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("ServiceInstance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("serviceinstances").WithVersion("version"), "", admission.Create, nil))
360+
if err != nil {
361+
actions := ""
362+
for _, action := range fakeClient.Actions() {
363+
actions = actions + action.GetVerb() + ":" + action.GetResource().Resource + ":" + action.GetSubresource() + ", "
364+
}
365+
t.Errorf("unexpected error %q returned from admission handler: %v", err, actions)
366+
}
367+
assertPlanReference(t,
368+
servicecatalog.PlanReference{ClusterServiceClassName: "foo-id", ClusterServicePlanName: "bar-id"},
369+
instance.Spec.PlanReference)
370+
}
371+
372+
// checks that defaulting fails when there are multiple plans to choose from.
373+
func TestWithNoPlanFailsWithMultiplePlansK8SName(t *testing.T) {
374+
sc := newClusterServiceClass("foo-id", "foo")
375+
sps := newClusterServicePlans(2, false)
376+
glog.V(4).Infof("Created Service as %+v", sc)
377+
fakeClient := newFakeServiceCatalogClientForTest(sc, sps, "", true /* use get */)
378+
handler, informerFactory, err := newHandlerForTest(fakeClient)
379+
if err != nil {
380+
t.Errorf("unexpected error initializing handler: %v", err)
381+
}
382+
informerFactory.Start(wait.NeverStop)
383+
384+
instance := newServiceInstance("dummy")
385+
instance.Spec.ClusterServiceClassName = "foo-id"
386+
387+
err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("ServiceInstance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("serviceinstances").WithVersion("version"), "", admission.Create, nil))
388+
if err == nil {
389+
t.Errorf("unexpected success with no plan specified and no serviceclass existing")
390+
return
391+
} else if !strings.Contains(err.Error(), "has more than one plan, PlanName must be") {
392+
t.Errorf("did not find expected error, got %q", err)
393+
}
394+
assertPlanReference(t,
395+
servicecatalog.PlanReference{ClusterServiceClassName: "foo-id"},
396+
instance.Spec.PlanReference)
397+
}
398+
399+
// checks that defaulting succeeds when there are multiple plans but only a
400+
// single plan for the specified Service Class
401+
func TestWithNoPlanSucceedsWithMultiplePlansFromDifferentClassesK8SName(t *testing.T) {
402+
sc := newClusterServiceClass("foo-id", "foo")
403+
sps := newClusterServicePlans(2, true)
404+
glog.V(4).Infof("Created Service as %+v", sc)
405+
classFilter := "test-serviceclass"
406+
fakeClient := newFakeServiceCatalogClientForTest(sc, sps, classFilter, true /* use get */)
407+
handler, informerFactory, err := newHandlerForTest(fakeClient)
408+
if err != nil {
409+
t.Errorf("unexpected error initializing handler: %v", err)
410+
}
411+
informerFactory.Start(wait.NeverStop)
412+
413+
instance := newServiceInstance("dummy")
414+
instance.Spec.ClusterServiceClassName = "foo-id"
415+
416+
err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("ServiceInstance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("serviceinstances").WithVersion("version"), "", admission.Create, nil))
417+
if err != nil {
418+
actions := ""
419+
for _, action := range fakeClient.Actions() {
420+
actions = actions + action.GetVerb() + ":" + action.GetResource().Resource + ":" + action.GetSubresource() + ", "
421+
}
422+
t.Errorf("unexpected error %q returned from admission handler: %v", err, actions)
423+
}
424+
assertPlanReference(t,
425+
servicecatalog.PlanReference{ClusterServiceClassName: "foo-id", ClusterServicePlanName: "bar-id"},
426+
instance.Spec.PlanReference)
427+
}
428+
429+
// Compares expected and actual PlanReferences and reports with Errorf of any mismatch
430+
func assertPlanReference(t *testing.T, expected servicecatalog.PlanReference, actual servicecatalog.PlanReference) {
431+
if expected != actual {
432+
t.Errorf("PlanReference was not as expected: %+v actual: %+v", expected, actual)
294433
}
295434
}

‎test/e2e/walkthrough.go

Lines changed: 46 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -64,15 +64,16 @@ var _ = framework.ServiceCatalogDescribe("walkthrough", func() {
6464

6565
It("Run walkthrough-example ", func() {
6666
var (
67-
brokerName = upsbrokername
68-
serviceclassName = "user-provided-service"
69-
serviceclassID = "4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468"
70-
serviceplanID = "86064792-7ea2-467b-af93-ac9694d96d52"
71-
testns = "test-ns"
72-
instanceName = "ups-instance"
73-
bindingName = "ups-binding"
74-
instanceNameDef = "ups-instance-def"
75-
instanceNameK8sNames = "ups-instance-k8s-names"
67+
brokerName = upsbrokername
68+
serviceclassName = "user-provided-service"
69+
serviceclassID = "4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468"
70+
serviceplanID = "86064792-7ea2-467b-af93-ac9694d96d52"
71+
testns = "test-ns"
72+
instanceName = "ups-instance"
73+
bindingName = "ups-binding"
74+
instanceNameDef = "ups-instance-def"
75+
instanceNameK8sNames = "ups-instance-k8s-names"
76+
instanceNameK8sNamesDef = "ups-instance-k8s-names-def"
7677
)
7778

7879
// Broker and ClusterServiceClass should become ready
@@ -283,6 +284,42 @@ var _ = framework.ServiceCatalogDescribe("walkthrough", func() {
283284
err = util.WaitForInstanceToNotExist(f.ServiceCatalogClientSet.ServicecatalogV1beta1(), testnamespace.Name, instanceNameK8sNames)
284285
Expect(err).NotTo(HaveOccurred())
285286

287+
By("Creating a ServiceInstance using k8s name and default plan")
288+
instanceK8SNamesDef := &v1beta1.ServiceInstance{
289+
ObjectMeta: metav1.ObjectMeta{
290+
Name: instanceNameK8sNamesDef,
291+
Namespace: testnamespace.Name,
292+
},
293+
Spec: v1beta1.ServiceInstanceSpec{
294+
PlanReference: v1beta1.PlanReference{
295+
ClusterServiceClassName: serviceclassID,
296+
},
297+
},
298+
}
299+
instance, err = f.ServiceCatalogClientSet.ServicecatalogV1beta1().ServiceInstances(testnamespace.Name).Create(instanceK8SNamesDef)
300+
Expect(err).NotTo(HaveOccurred(), "failed to create instance with K8S name and default plan")
301+
Expect(instanceK8SNamesDef).NotTo(BeNil())
302+
303+
By("Waiting for ServiceInstance with k8s name and default plan to be ready")
304+
err = util.WaitForInstanceCondition(f.ServiceCatalogClientSet.ServicecatalogV1beta1(),
305+
testnamespace.Name,
306+
instanceNameK8sNamesDef,
307+
v1beta1.ServiceInstanceCondition{
308+
Type: v1beta1.ServiceInstanceConditionReady,
309+
Status: v1beta1.ConditionTrue,
310+
},
311+
)
312+
Expect(err).NotTo(HaveOccurred(), "failed to wait instance with k8s name and default plan to be ready")
313+
314+
// Deprovisioning the ServiceInstance with k8s name and default plan
315+
By("Deleting the ServiceInstance with k8s name and default plan")
316+
err = f.ServiceCatalogClientSet.ServicecatalogV1beta1().ServiceInstances(testnamespace.Name).Delete(instanceNameK8sNamesDef, nil)
317+
Expect(err).NotTo(HaveOccurred(), "failed to delete the instance with k8s name and default plan")
318+
319+
By("Waiting for ServiceInstance with k8s name and default plan to not exist")
320+
err = util.WaitForInstanceToNotExist(f.ServiceCatalogClientSet.ServicecatalogV1beta1(), testnamespace.Name, instanceNameK8sNamesDef)
321+
Expect(err).NotTo(HaveOccurred())
322+
286323
By("Deleting the test namespace")
287324
err = framework.DeleteKubeNamespace(f.KubeClientSet, testnamespace.Name)
288325
Expect(err).NotTo(HaveOccurred())

0 commit comments

Comments
 (0)