Skip to content

Commit b70c076

Browse files
nileboxVille Aikas
nilebox
authored and
Ville Aikas
committed
Reorder class and plan creation; test plan conflict handling (#1459)
1 parent 4bea012 commit b70c076

File tree

2 files changed

+113
-75
lines changed

2 files changed

+113
-75
lines changed

‎pkg/controller/controller_broker.go

+46-46
Original file line numberDiff line numberDiff line change
@@ -299,44 +299,44 @@ func (c *controller) reconcileClusterServiceBroker(broker *v1beta1.ClusterServic
299299
existingServiceClassMap := convertServiceClassListToMap(existingServiceClasses)
300300
existingServicePlanMap := convertServicePlanListToMap(existingServicePlans)
301301

302-
// reconcile the plans that were part of the broker's catalog payload
303-
for _, payloadServicePlan := range payloadServicePlans {
304-
existingServicePlan, _ := existingServicePlanMap[payloadServicePlan.Name]
305-
delete(existingServicePlanMap, payloadServicePlan.Name)
302+
// reconcile the serviceClasses that were part of the broker's catalog
303+
// payload
304+
for _, payloadServiceClass := range payloadServiceClasses {
305+
existingServiceClass, _ := existingServiceClassMap[payloadServiceClass.Name]
306+
delete(existingServiceClassMap, payloadServiceClass.Name)
306307

307-
glog.V(4).Infof(
308-
"ClusterServiceBroker %q: reconciling %s",
309-
broker.Name, pretty.ClusterServicePlanName(payloadServicePlan),
310-
)
311-
if err := c.reconcileClusterServicePlanFromClusterServiceBrokerCatalog(broker, payloadServicePlan, existingServicePlan); err != nil {
308+
glog.V(4).Info(pcb.Messagef("Reconciling %s", pretty.ClusterServiceClassName(payloadServiceClass)))
309+
if err := c.reconcileClusterServiceClassFromClusterServiceBrokerCatalog(broker, payloadServiceClass, existingServiceClass); err != nil {
312310
s := fmt.Sprintf(
313-
"Error reconciling %s: %s",
314-
pretty.ClusterServicePlanName(payloadServicePlan), err,
311+
"Error reconciling %s (broker %q): %s",
312+
pretty.ClusterServiceClassName(payloadServiceClass), broker.Name, err,
315313
)
316314
glog.Warning(pcb.Message(s))
317315
c.recorder.Eventf(broker, corev1.EventTypeWarning, errorSyncingCatalogReason, s)
318-
c.updateClusterServiceBrokerCondition(broker, v1beta1.ServiceBrokerConditionReady, v1beta1.ConditionFalse, errorSyncingCatalogReason,
319-
errorSyncingCatalogMessage+s)
316+
if err := c.updateClusterServiceBrokerCondition(broker, v1beta1.ServiceBrokerConditionReady, v1beta1.ConditionFalse, errorSyncingCatalogReason,
317+
errorSyncingCatalogMessage+s); err != nil {
318+
return err
319+
}
320320
return err
321321
}
322-
glog.V(5).Info(pcb.Messagef("Reconciled %s", pretty.ClusterServicePlanName(payloadServicePlan)))
323322

323+
glog.V(5).Info(pcb.Messagef("Reconciled %s", pretty.ClusterServiceClassName(payloadServiceClass)))
324324
}
325325

326-
// handle the servicePlans that were not in the broker's payload;
327-
// mark these as deleted
328-
for _, existingServicePlan := range existingServicePlanMap {
329-
if existingServicePlan.Status.RemovedFromBrokerCatalog {
326+
// handle the serviceClasses that were not in the broker's payload;
327+
// mark these as having been removed from the broker's catalog
328+
for _, existingServiceClass := range existingServiceClassMap {
329+
if existingServiceClass.Status.RemovedFromBrokerCatalog {
330330
continue
331331
}
332-
glog.V(4).Info(pcb.Messagef("%s has been removed from broker's catalog; marking", pretty.ClusterServicePlanName(existingServicePlan)))
333-
existingServicePlan.Status.RemovedFromBrokerCatalog = true
334-
_, err := c.serviceCatalogClient.ClusterServicePlans().UpdateStatus(existingServicePlan)
332+
333+
glog.V(4).Info(pcb.Messagef("%s has been removed from broker's catalog; marking", pretty.ClusterServiceClassName(existingServiceClass)))
334+
existingServiceClass.Status.RemovedFromBrokerCatalog = true
335+
_, err := c.serviceCatalogClient.ClusterServiceClasses().UpdateStatus(existingServiceClass)
335336
if err != nil {
336337
s := fmt.Sprintf(
337338
"Error updating status of %s: %v",
338-
pretty.ClusterServicePlanName(existingServicePlan),
339-
err,
339+
pretty.ClusterServiceClassName(existingServiceClass), err,
340340
)
341341
glog.Warning(pcb.Message(s))
342342
c.recorder.Eventf(broker, corev1.EventTypeWarning, errorSyncingCatalogReason, s)
@@ -348,44 +348,44 @@ func (c *controller) reconcileClusterServiceBroker(broker *v1beta1.ClusterServic
348348
}
349349
}
350350

351-
// reconcile the serviceClasses that were part of the broker's catalog
352-
// payload
353-
for _, payloadServiceClass := range payloadServiceClasses {
354-
existingServiceClass, _ := existingServiceClassMap[payloadServiceClass.Name]
355-
delete(existingServiceClassMap, payloadServiceClass.Name)
351+
// reconcile the plans that were part of the broker's catalog payload
352+
for _, payloadServicePlan := range payloadServicePlans {
353+
existingServicePlan, _ := existingServicePlanMap[payloadServicePlan.Name]
354+
delete(existingServicePlanMap, payloadServicePlan.Name)
356355

357-
glog.V(4).Info(pcb.Messagef("Reconciling %s", pretty.ClusterServiceClassName(payloadServiceClass)))
358-
if err := c.reconcileClusterServiceClassFromClusterServiceBrokerCatalog(broker, payloadServiceClass, existingServiceClass); err != nil {
356+
glog.V(4).Infof(
357+
"ClusterServiceBroker %q: reconciling %s",
358+
broker.Name, pretty.ClusterServicePlanName(payloadServicePlan),
359+
)
360+
if err := c.reconcileClusterServicePlanFromClusterServiceBrokerCatalog(broker, payloadServicePlan, existingServicePlan); err != nil {
359361
s := fmt.Sprintf(
360-
"Error reconciling %s (broker %q): %s",
361-
pretty.ClusterServiceClassName(payloadServiceClass), broker.Name, err,
362+
"Error reconciling %s: %s",
363+
pretty.ClusterServicePlanName(payloadServicePlan), err,
362364
)
363365
glog.Warning(pcb.Message(s))
364366
c.recorder.Eventf(broker, corev1.EventTypeWarning, errorSyncingCatalogReason, s)
365-
if err := c.updateClusterServiceBrokerCondition(broker, v1beta1.ServiceBrokerConditionReady, v1beta1.ConditionFalse, errorSyncingCatalogReason,
366-
errorSyncingCatalogMessage+s); err != nil {
367-
return err
368-
}
367+
c.updateClusterServiceBrokerCondition(broker, v1beta1.ServiceBrokerConditionReady, v1beta1.ConditionFalse, errorSyncingCatalogReason,
368+
errorSyncingCatalogMessage+s)
369369
return err
370370
}
371+
glog.V(5).Info(pcb.Messagef("Reconciled %s", pretty.ClusterServicePlanName(payloadServicePlan)))
371372

372-
glog.V(5).Info(pcb.Messagef("Reconciled %s", pretty.ClusterServiceClassName(payloadServiceClass)))
373373
}
374374

375-
// handle the serviceClasses that were not in the broker's payload;
376-
// mark these as having been removed from the broker's catalog
377-
for _, existingServiceClass := range existingServiceClassMap {
378-
if existingServiceClass.Status.RemovedFromBrokerCatalog {
375+
// handle the servicePlans that were not in the broker's payload;
376+
// mark these as deleted
377+
for _, existingServicePlan := range existingServicePlanMap {
378+
if existingServicePlan.Status.RemovedFromBrokerCatalog {
379379
continue
380380
}
381-
382-
glog.V(4).Info(pcb.Messagef("%s has been removed from broker's catalog; marking", pretty.ClusterServiceClassName(existingServiceClass)))
383-
existingServiceClass.Status.RemovedFromBrokerCatalog = true
384-
_, err := c.serviceCatalogClient.ClusterServiceClasses().UpdateStatus(existingServiceClass)
381+
glog.V(4).Info(pcb.Messagef("%s has been removed from broker's catalog; marking", pretty.ClusterServicePlanName(existingServicePlan)))
382+
existingServicePlan.Status.RemovedFromBrokerCatalog = true
383+
_, err := c.serviceCatalogClient.ClusterServicePlans().UpdateStatus(existingServicePlan)
385384
if err != nil {
386385
s := fmt.Sprintf(
387386
"Error updating status of %s: %v",
388-
pretty.ClusterServiceClassName(existingServiceClass), err,
387+
pretty.ClusterServicePlanName(existingServicePlan),
388+
err,
389389
)
390390
glog.Warning(pcb.Message(s))
391391
c.recorder.Eventf(broker, corev1.EventTypeWarning, errorSyncingCatalogReason, s)

‎pkg/controller/controller_broker_test.go

+67-29
Original file line numberDiff line numberDiff line change
@@ -232,9 +232,9 @@ func TestReconcileClusterServiceBrokerExistingServiceClassAndServicePlan(t *test
232232
assertNumberOfActions(t, actions, 6)
233233
assertList(t, actions[0], &v1beta1.ClusterServiceClass{}, listRestrictions)
234234
assertList(t, actions[1], &v1beta1.ClusterServicePlan{}, listRestrictions)
235-
assertCreate(t, actions[2], testClusterServicePlan)
236-
assertCreate(t, actions[3], testClusterServicePlanNonbindable)
237-
assertUpdate(t, actions[4], testClusterServiceClass)
235+
assertUpdate(t, actions[2], testClusterServiceClass)
236+
assertCreate(t, actions[3], testClusterServicePlan)
237+
assertCreate(t, actions[4], testClusterServicePlanNonbindable)
238238

239239
// 4 update action for broker status subresource
240240
updatedClusterServiceBroker := assertUpdateStatus(t, actions[5], getTestClusterServiceBroker())
@@ -281,10 +281,10 @@ func TestReconcileClusterServiceBrokerRemovedClusterServiceClass(t *testing.T) {
281281
assertNumberOfActions(t, actions, 7)
282282
assertList(t, actions[0], &v1beta1.ClusterServiceClass{}, listRestrictions)
283283
assertList(t, actions[1], &v1beta1.ClusterServicePlan{}, listRestrictions)
284-
assertCreate(t, actions[2], testClusterServicePlan)
285-
assertCreate(t, actions[3], testClusterServicePlanNonbindable)
286-
assertUpdate(t, actions[4], testClusterServiceClass)
287-
assertUpdateStatus(t, actions[5], testRemovedClusterServiceClass)
284+
assertUpdate(t, actions[2], testClusterServiceClass)
285+
assertUpdateStatus(t, actions[3], testRemovedClusterServiceClass)
286+
assertCreate(t, actions[4], testClusterServicePlan)
287+
assertCreate(t, actions[5], testClusterServicePlanNonbindable)
288288

289289
updatedClusterServiceBroker := assertUpdateStatus(t, actions[6], getTestClusterServiceBroker())
290290
assertClusterServiceBrokerReadyTrue(t, updatedClusterServiceBroker)
@@ -336,10 +336,10 @@ func TestReconcileClusterServiceBrokerRemovedClusterServicePlan(t *testing.T) {
336336
assertNumberOfActions(t, actions, 7)
337337
assertList(t, actions[0], &v1beta1.ClusterServiceClass{}, listRestrictions)
338338
assertList(t, actions[1], &v1beta1.ClusterServicePlan{}, listRestrictions)
339-
assertCreate(t, actions[2], testClusterServicePlan)
340-
assertCreate(t, actions[3], testClusterServicePlanNonbindable)
341-
assertUpdateStatus(t, actions[4], testRemovedClusterServicePlan)
342-
assertUpdate(t, actions[5], testClusterServiceClass)
339+
assertUpdate(t, actions[2], testClusterServiceClass)
340+
assertCreate(t, actions[3], testClusterServicePlan)
341+
assertCreate(t, actions[4], testClusterServicePlanNonbindable)
342+
assertUpdateStatus(t, actions[5], testRemovedClusterServicePlan)
343343

344344
updatedClusterServiceBroker := assertUpdateStatus(t, actions[6], getTestClusterServiceBroker())
345345
assertClusterServiceBrokerReadyTrue(t, updatedClusterServiceBroker)
@@ -358,15 +358,54 @@ func TestReconcileClusterServiceBrokerExistingClusterServiceClassDifferentBroker
358358
testClusterServiceClass := getTestClusterServiceClass()
359359
testClusterServiceClass.Spec.ClusterServiceBrokerName = "notTheSame"
360360

361+
sharedInformers.ClusterServiceClasses().Informer().GetStore().Add(testClusterServiceClass)
362+
363+
if err := testController.reconcileClusterServiceBroker(getTestClusterServiceBroker()); err == nil {
364+
t.Fatal("The same service class should not belong to two different brokers.")
365+
}
366+
367+
brokerActions := fakeClusterServiceBrokerClient.Actions()
368+
assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1)
369+
assertGetCatalog(t, brokerActions[0])
370+
371+
actions := fakeCatalogClient.Actions()
372+
assertNumberOfActions(t, actions, 3)
373+
374+
listRestrictions := clientgotesting.ListRestrictions{
375+
Labels: labels.Everything(),
376+
Fields: fields.OneTermEqualSelector("spec.clusterServiceBrokerName", "test-broker"),
377+
}
378+
assertList(t, actions[0], &v1beta1.ClusterServiceClass{}, listRestrictions)
379+
assertList(t, actions[1], &v1beta1.ClusterServicePlan{}, listRestrictions)
380+
updatedClusterServiceBroker := assertUpdateStatus(t, actions[2], getTestClusterServiceBroker())
381+
assertClusterServiceBrokerReadyFalse(t, updatedClusterServiceBroker)
382+
383+
// verify no kube resources created
384+
kubeActions := fakeKubeClient.Actions()
385+
assertNumberOfActions(t, kubeActions, 0)
386+
387+
events := getRecordedEvents(testController)
388+
assertNumEvents(t, events, 1)
389+
390+
expectedEvent := corev1.EventTypeWarning + " " + errorSyncingCatalogReason + ` Error reconciling ClusterServiceClass (K8S: "SCGUID" ExternalName: "test-serviceclass") (broker "test-broker"): ClusterServiceClass (K8S: "SCGUID" ExternalName: "test-serviceclass") already exists for Broker "notTheSame"`
391+
if e, a := expectedEvent, events[0]; e != a {
392+
t.Fatalf("Received unexpected event; expected\n%v, got\n%v", e, a)
393+
}
394+
}
395+
396+
// TestReconcileClusterServiceBrokerExistingClusterServicePlanDifferentClass simulates catalog
397+
// refresh where broker lists a service plan which matches an existing, already
398+
// cataloged service plan but the plan points to a different ClusterServiceClass. Results in an error.
399+
func TestReconcileClusterServiceBrokerExistingClusterServicePlanDifferentClass(t *testing.T) {
400+
fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, getTestCatalogConfig())
401+
361402
testClusterServicePlan := getTestClusterServicePlan()
362403
testClusterServicePlan.Spec.ClusterServiceBrokerName = "notTheSame"
404+
testClusterServicePlan.Spec.ClusterServiceClassRef = v1beta1.ClusterObjectReference{
405+
Name: "notTheSameClass",
406+
}
363407

364-
testClusterServicePlanNonbindable := getTestClusterServicePlanNonbindable()
365-
testClusterServicePlanNonbindable.Spec.ClusterServiceBrokerName = "notTheSame"
366-
367-
sharedInformers.ClusterServiceClasses().Informer().GetStore().Add(testClusterServiceClass)
368408
sharedInformers.ClusterServicePlans().Informer().GetStore().Add(testClusterServicePlan)
369-
sharedInformers.ClusterServicePlans().Informer().GetStore().Add(testClusterServicePlanNonbindable)
370409

371410
if err := testController.reconcileClusterServiceBroker(getTestClusterServiceBroker()); err == nil {
372411
t.Fatal("The same service class should not belong to two different brokers.")
@@ -377,15 +416,16 @@ func TestReconcileClusterServiceBrokerExistingClusterServiceClassDifferentBroker
377416
assertGetCatalog(t, brokerActions[0])
378417

379418
actions := fakeCatalogClient.Actions()
380-
assertNumberOfActions(t, actions, 3)
419+
assertNumberOfActions(t, actions, 4)
381420

382421
listRestrictions := clientgotesting.ListRestrictions{
383422
Labels: labels.Everything(),
384423
Fields: fields.OneTermEqualSelector("spec.clusterServiceBrokerName", "test-broker"),
385424
}
386425
assertList(t, actions[0], &v1beta1.ClusterServiceClass{}, listRestrictions)
387426
assertList(t, actions[1], &v1beta1.ClusterServicePlan{}, listRestrictions)
388-
updatedClusterServiceBroker := assertUpdateStatus(t, actions[2], getTestClusterServiceBroker())
427+
assertCreate(t, actions[2], getTestClusterServiceClass())
428+
updatedClusterServiceBroker := assertUpdateStatus(t, actions[3], getTestClusterServiceBroker())
389429
assertClusterServiceBrokerReadyFalse(t, updatedClusterServiceBroker)
390430

391431
// verify no kube resources created
@@ -737,28 +777,26 @@ func TestReconcileClusterServiceBrokerWithReconcileError(t *testing.T) {
737777
assertGetCatalog(t, brokerActions[0])
738778

739779
actions := fakeCatalogClient.Actions()
740-
assertNumberOfActions(t, actions, 6)
780+
assertNumberOfActions(t, actions, 4)
741781

742782
listRestrictions := clientgotesting.ListRestrictions{
743783
Labels: labels.Everything(),
744784
Fields: fields.OneTermEqualSelector("spec.clusterServiceBrokerName", broker.Name),
745785
}
746786
assertList(t, actions[0], &v1beta1.ClusterServiceClass{}, listRestrictions)
747787
assertList(t, actions[1], &v1beta1.ClusterServicePlan{}, listRestrictions)
748-
assertCreate(t, actions[2], getTestClusterServicePlan())
749-
assertCreate(t, actions[3], getTestClusterServicePlanNonbindable())
750788

751789
// the two plans in the catalog as two separate actions
752790

753-
createSCAction := actions[4].(clientgotesting.CreateAction)
791+
createSCAction := actions[2].(clientgotesting.CreateAction)
754792
createdSC, ok := createSCAction.GetObject().(*v1beta1.ClusterServiceClass)
755793
if !ok {
756794
t.Fatalf("couldn't convert to a ClusterServiceClass: %+v", createSCAction.GetObject())
757795
}
758796
if e, a := getTestClusterServiceClass(), createdSC; !reflect.DeepEqual(e, a) {
759797
t.Fatalf("unexpected diff for created ClusterServiceClass: %v,\n\nEXPECTED: %+v\n\nACTUAL: %+v", diff.ObjectReflectDiff(e, a), e, a)
760798
}
761-
updatedClusterServiceBroker := assertUpdateStatus(t, actions[5], broker)
799+
updatedClusterServiceBroker := assertUpdateStatus(t, actions[3], broker)
762800
assertClusterServiceBrokerReadyFalse(t, updatedClusterServiceBroker)
763801

764802
kubeActions := fakeKubeClient.Actions()
@@ -807,9 +845,9 @@ func TestReconcileClusterServiceBrokerSuccessOnFinalRetry(t *testing.T) {
807845

808846
assertList(t, actions[1], &v1beta1.ClusterServiceClass{}, listRestrictions)
809847
assertList(t, actions[2], &v1beta1.ClusterServicePlan{}, listRestrictions)
810-
assertCreate(t, actions[3], getTestClusterServicePlan())
811-
assertCreate(t, actions[4], getTestClusterServicePlanNonbindable())
812-
assertCreate(t, actions[5], testClusterServiceClass)
848+
assertCreate(t, actions[3], testClusterServiceClass)
849+
assertCreate(t, actions[4], getTestClusterServicePlan())
850+
assertCreate(t, actions[5], getTestClusterServicePlanNonbindable())
813851

814852
updatedClusterServiceBroker = assertUpdateStatus(t, actions[6], getTestClusterServiceBroker())
815853
assertClusterServiceBrokerReadyTrue(t, updatedClusterServiceBroker)
@@ -904,9 +942,9 @@ func TestReconcileClusterServiceBrokerWithStatusUpdateError(t *testing.T) {
904942

905943
assertList(t, actions[0], &v1beta1.ClusterServiceClass{}, listRestrictions)
906944
assertList(t, actions[1], &v1beta1.ClusterServicePlan{}, listRestrictions)
907-
assertCreate(t, actions[2], getTestClusterServicePlan())
908-
assertCreate(t, actions[3], getTestClusterServicePlanNonbindable())
909-
assertCreate(t, actions[4], testClusterServiceClass)
945+
assertCreate(t, actions[2], testClusterServiceClass)
946+
assertCreate(t, actions[3], getTestClusterServicePlan())
947+
assertCreate(t, actions[4], getTestClusterServicePlanNonbindable())
910948

911949
// 4 update action for broker status subresource
912950
updatedClusterServiceBroker := assertUpdateStatus(t, actions[5], getTestClusterServiceBroker())

0 commit comments

Comments
 (0)