Missing service offerings are not removed upon `cf update-service-broker` and don't issue warnings

gberche-orange commented 4 years ago

Issue

a service broker that is evolving its catalog by removing some service offerings (but not service plans) never gets its old service offerings removed from CC

diagnostic message of failed cf update-service-broker is unfriendly in the following scenario

given a service broker has registered a service offering with name overview-service and guid G1, and plan small with guid S1
given an existing service instance for service overview-service
given a broker updates changes response from its v2/catalog to not return service with name overview-service-2 and guid G2, and plan small with guid S1
when admin executes cf update-service-broker
then the admin sees no warning
and service plan "small" with guid S1 appears within service offerings overview-service and overview-service-2

Context

As a CF admin

in order to be productive with CF API
I need helpful friendly diagnostic messages on catalog update errors

Steps to Reproduce

deploy broker from https://github.com/cloudfoundry/overview-broker
register the broker and enable its service plan
provision a service instance cf create-service overview-service small gberche
edit overview broker catalog and rename "overview" service to "overview-2" along with its guid
cf update-service-broker

Expected result

Warning: Service plans are missing from the broker's catalog (https://osb-reverse-proxy-1.internal-controlplane-cf.paas/v2/catalog) but can not be removed from Cloud Foundry while instances exist. The plans have been deactivated to prevent users from attempting to provision new instances of these plans. The broker should continue to support bind, unbind, and delete for existing instances; if these operations fail contact your broker provider.

Service Offering: overview-service (guid=G1)
Plans deactivated: small (guid=S1)

This would be similar to https://github.com/cloudfoundry/cloud_controller_ng/blob/17402a48fe38e2f0f8cc36a316e60427fc79f0f1/lib/services/service_brokers/service_manager.rb#L175-L181

Current result

No warning is issued

Users still see the old service offering in CF marketplace whose broker now reject

Creating service instance gberche2 in org service-sandbox / space osb-reverse-proxy-smoke-tests as coa-cf...
The service broker rejected the request. Status Code: 400 Bad Request, Body: {"error":"Could not find service b39f09f6-e54b-4762-954d-963366ef6ebc, plan 34df9076-6b13-48e7-9b6f-b2a2550ef8f9"}`

service access still shows the old removed service offering, without any disabled plans

cf service-access -b osb-reverse-proxy-1
Getting service access for broker osb-reverse-proxy-1 as coa-cf...
broker: osb-reverse-proxy-1
service              plan                                                        access    orgs
overview-service     allOf-with-two-levels-of-nesting                            limited   service-sandbox
overview-service     allOf                                                       limited   service-sandbox
overview-service     anyOf-with-two-levels-of-nesting                            limited   service-sandbox
overview-service     anyOf                                                       limited   service-sandbox
overview-service     default-value-and-not-required                              limited   service-sandbox
overview-service     default-value-and-required                                  limited   service-sandbox
overview-service     large                                                       limited   service-sandbox
overview-service     object-with-min-max-title-description                       limited   service-sandbox
overview-service     oneOf-with-two-levels-of-nesting                            limited   service-sandbox
overview-service     oneOf                                                       limited   service-sandbox
overview-service     optional-object-with-required-field                         limited   service-sandbox
overview-service     required-object-with-no-required-fields                     limited   service-sandbox
overview-service     required-object-with-required-fields                        limited   service-sandbox
overview-service     small                                                       limited   service-sandbox
overview-service     three-levels-of-nesting                                     limited   service-sandbox
overview-service     two-levels-of-nesting-with-required-field-in-second-level   limited   service-sandbox
overview-service-2   allOf-with-two-levels-of-nesting                            none      
overview-service-2   allOf                                                       none      
overview-service-2   anyOf-with-two-levels-of-nesting                            none      
overview-service-2   anyOf                                                       none      
overview-service-2   default-value-and-not-required                              none      
overview-service-2   default-value-and-required                                  none      
overview-service-2   large                                                       none      
overview-service-2   object-with-min-max-title-description                       none      
overview-service-2   oneOf-with-two-levels-of-nesting                            none      
overview-service-2   oneOf                                                       none      
overview-service-2   optional-object-with-required-field                         none      
overview-service-2   required-object-with-no-required-fields                     none      
overview-service-2   required-object-with-required-fields                        none      
overview-service-2   small                                                       none      
overview-service-2   three-levels-of-nesting                                     none      
overview-service-2   two-levels-of-nesting-with-required-field-in-second-level   none

cf-gitbot commented 4 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/175305440

The labels on this github issue will be updated when the story is started.

blgm commented 3 years ago

@gberche-orange, I have managed to re-create this using the instructions above using both V2 and V3 endpoints. The subtlety is to update the service name and ID, but not to change the plan name or ID.

This what OSBAPI says about the plan ID:

An identifier used to correlate this Service Plan in future requests to the Service Broker. This MUST be globally unique such that Platforms (and their users) MUST be able to assume that seeing the same value (no matter what Service Broker uses it) will always refer to this Service Plan and for the same Service Offering. MUST be a non-empty string. Using a GUID is RECOMMENDED.

Also see https://github.com/openservicebrokerapi/servicebroker/issues/306 for a discussion about the plan ID being globally unique.

Because a plan ID is globally unique, it is an error to change a service ID without changing the plan ID (because the same plan ID value will always refer to this Service Plan and for the same Service Offering). But how should a platform handle this error?

I'm thinking that catalog validation should fail in a way that's analogous to having a duplicate plan ID. Something like plan ID abc... is already used by another service. What are your thoughts?

gberche-orange commented 3 years ago

thanks @blgm for looking into this issue !

Because a plan ID is globally unique, it is an error to change a service ID without changing the plan ID (because the same plan ID value will always refer to this Service Plan and for the same Service Offering). But how should a platform handle this error?

I'm thinking that catalog validation should fail in a way that's analogous to having a duplicate plan ID. Something like plan ID abc... is already used by another service. What are your thoughts?

+1 for failing the service catalog in this case.

I think it would be useful to hint the service broker operator/author with a more specific message

helping diagnose the problem cause such as Unable to register service name=[service_name] with guid=[service_guid] has its plan name=[plan_name] guid=[plan_guid] is already used by service name=[existing_service_name] with guid=[existing_service_guid] offered by broker name=[broker_name]
helping finding a resolution/fix: such as The service broker catalog was not loaded. Please consider fixing the service broker catalog, or deleting the service broker with the conflicting planid

elenasharma commented 3 years ago

The fix for this issue was released in capi-release 1.105. Thank you for raising this issue!

gberche-orange commented 3 years ago

Great thanks @blgm and @elenasharma !

cloudfoundry / cloud_controller_ng