Move dashboard_client to CF catalog extension described in profile.md #483

mattmcneeney · 2018-03-28T08:58:07Z

The dashboard_client object is only used by service brokers that are being used with Pivotal Cloud Foundry. We should therefore investigate if it is feasible to deprecate this field (similar to what we did with organization_guid and space_guid) and move it into the service-level metadata specific for Cloud Foundry environments.

Adding the do not merge flag as I need some feedback from the CF side before moving this into review.

cfdreddbot · 2018-03-28T08:58:08Z

Hey mattmcneeney!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

mattmcneeney · 2018-03-28T08:59:08Z

@zrob I'd appreciate your feedback on this one. We can handle this deprecation similar to what we've done for other fields in CAPI, but we will have to do a substantial outreach to broker authors to let them know that at some point in the future, the top-level field will be removed.

cc @Samze

n3wscott · 2018-03-28T17:01:13Z

spec.md

@@ -378,7 +378,7 @@ It is therefore RECOMMENDED that implementations avoid such strings.
 | requires | array of strings | A list of permissions that the user would have to give the service, if they provision it. The only permissions currently supported are `syslog_drain`, `route_forwarding` and `volume_mount`. |
 | bindable* | boolean | Specifies whether Service Instances of the service can be bound to applications. This specifies the default for all plans of this service. Plans can override this field (see [Plan Object](#plan-object)). |
 | metadata | object | An opaque object of metadata for a Service Offering. It is expected that Platforms will treat this as a blob. Note that there are [conventions](profile.md#service-metadata) in existing Service Brokers and Platforms for fields that aid in the display of catalog data. |
-| dashboard_client | [DashboardClient](#dashboard-client-object) | Contains the data necessary to activate the Dashboard SSO feature for this service. |
+| dashboard_client | [DashboardClient](profile.md#dashboard-client-object) | Deprecated; has been moved to the [Cloud Foundry Service Metadata](profile.md#cloud-foundry-service-metadata) object. Contains the data necessary to activate the Dashboard SSO feature for this service. |


perhaps remove Contains the data necessary to activate the Dashboard SSO feature for this service. as well?

n3wscott · 2018-03-28T17:02:54Z

LGTM, my only nit is to remove the description for the deprecated field so that someone goes to look at the new location inside of profile.md for more information. But I am ok as is as well...

zrob · 2018-03-29T23:36:49Z

I agree this is CF specific and should be moved from the top-level of the spec. I think there are a few things to address.

It should still be configurable per service
CF platform should be able to support both old and new location
It inherently holds sensitive information. This means that even in the metadata section this could appear on the Service object in k8s. So a broker that is multi-platform may be surprised at the visibility of this secret information.

I think this proposal meets the first two issues.

The third issue does not have a clear answer since we don't have a concept of "sensitive" in the spec that a platform could care about, so I don't think we should block on this, but maybe we need to consider this as a separate concern?

Before merging we should give CF a chance to update to use the new location.

n3wscott · 2018-03-30T15:14:52Z

Before merging we should give CF a chance to update to use the new location.

The field is just being marked deprecated, and not removed. So CF can still go on using the field in the current location even if the PR is merged.

In the future, perhaps CF should look into a Generic Action Auth call on the broker for a service and that would fix point 3? It might have to be a generic broker action as you auth before an instance is created I think.

duglin · 2018-04-03T16:38:04Z

Look to see if we can resolve #462 as part of this work

mattmcneeney · 2018-04-03T16:53:56Z

This is now covering #462

zrob · 2018-04-03T19:07:59Z

profile.md

+
+| Response Field | Type | Description |
+| --- | --- | --- |
+| id | string | The id of the oAuth client that the dashboard will use. If present, MUST be a globally unique non-empty string. |


oAuth -> OAuth

mattmcneeney · 2018-04-04T17:23:40Z

Any other thoughts on this @zrob and co? Can I get a 👍 if people are happy to move this into validation through implementation?

duglin · 2018-04-17T12:03:46Z

spec.md

@@ -393,15 +393,6 @@ company). Additionally, some Platforms might modify the service names before
 presenting them to their users. This specification places no requirements on
 how Platforms might expose these values to their users.

-##### Dashboard Client Object


If its just deprecated, I'm not sure we can fully remove it from the spec yet - don't we usually keep the old fields around?

Since we're still linking to it elsewhere, I'd be keen to remove this to keep things tidier. You can still find out what it is in the versioned profile.md - it's just moved. What do you think?

By that logic we should remove all CF-isms from the core spec today. :-) I think each version of a spec should be complete on its own. It feels wrong to mention something that is only defined in an old version of the spec.

duglin · 2018-04-17T12:04:16Z

spec.md

@@ -378,7 +378,7 @@ It is therefore RECOMMENDED that implementations avoid such strings.
 | requires | array of strings | A list of permissions that the user would have to give the service, if they provision it. The only permissions currently supported are `syslog_drain`, `route_forwarding` and `volume_mount`. |
 | bindable* | boolean | Specifies whether Service Instances of the service can be bound to applications. This specifies the default for all plans of this service. Plans can override this field (see [Plan Object](#plan-object)). |
 | metadata | object | An opaque object of metadata for a Service Offering. It is expected that Platforms will treat this as a blob. Note that there are [conventions](profile.md#service-metadata) in existing Service Brokers and Platforms for fields that aid in the display of catalog data. |
-| dashboard_client | [DashboardClient](#dashboard-client-object) | Contains the data necessary to activate the Dashboard SSO feature for this service. |
+| dashboard_client | [DashboardClient](profile.md#dashboard-client-object) | Deprecated (see the [Cloud Foundry Service Metadata](profile.md#cloud-foundry-service-metadata) object). |


I think this needs to point to the old stuff, until its fully removed, but keep the deprecation statement and pointer to the new stuff.

See above. Since both spec.md and profile.md are versioned, can we not move things between these without 'breaking' anything?

My point is that existing brokers might look at the spec and see this field and, even though its deprecated, they're still allowed to send it. Which means we need to still clearly define it. We can't leave a hole in the supported functionality. See how we dealt with org_id - we tagged it as deprecated in favor of context but still fully defined it.

duglin · 2018-04-17T12:04:57Z

SGTM just a minor comment on the spec changes.

n3wscott · 2018-04-17T17:12:35Z

how about adding a login call to OSBAPI?

pmorie · 2018-04-17T17:17:01Z

profile.md

@@ -421,3 +421,12 @@ specific behaviour.
 | Broker API Field | Type | Description |
 | --- | --- | --- |
 | metadata.shareable | string | Allows Service Instances to be shared across orgs and spaces. |
+| metadata.dashboard_client | [DashboardClient](#dashboard-client-object) | Contains the data necessary to activate the Dashboard SSO feature for this service. |


I think we should take a step back here - this doesn't seem like a great fit for metadata or the catalog endpoint in general, as-is.

In kubernetes, we take whatever's in metadata and put it into a user-visible field. AFAIK we don't have an expectation that the platform name is sent to the catalog endpoint. That means that when a broker returns this field, it will be visible to every user that can see the service resource. That means that if you return this information, you've opened it up to a wider audience than you probably expected.

I don't wish to stall this at all, but I think we need to begin reasoning about the catalog endpoint as returning information that may be visible to everyone, even users with a low degree of privilege. Could we consider moving this to a separate resource or subresource in the future?

So in this regard, CF and k8s are behaving differently. In CF, the platform uses information in metadata to understand behaviours but doesn't make this information visible to the end user. We also only show some information from the catalog in general, hence this kind of information has been stored here.

Do you think this is probably a good input for the v3 design?

In general it does seem like we may need to have a clean separation between data that's ok for end users to see and data that is not ok for end users to see - and each needs to be clearly identified in the spec so that Platforms can act appropriately. In this case, if we were to move this data into some "secretMetadata" section then Kube can know that it should do something special with it.

Actually CF and K8s are the same here. We expose all information returned in metadata on the CF API. Historically metadata has been populated with data for client tooling. This would be the first time we have to "hide" something from the response.

If we're considering a different abstraction for broker-to-platform metadata. Do we want to move it to the current metadata object just to have to move it again later?

I don't want to block this, but I think putting secret information into metadata is something that could have negative consequences. I would prefer not to accumulate fields that require filtering in metadata and I don't think we should set that example for users.

I have been thinking about this and haven't come up with a great solution yet :-/

mattmcneeney · 2018-04-24T17:14:09Z

Moving dashboard_client to metadata isn't a good solution as this field is visible to end users in both CF and k8s. However we do all agree that we shouldn't have platform-specific fields in the spec.

Next plan:

Wait for v3, or
Move this to a 'vendor extension'?

mattmcneeney · 2018-05-08T17:10:04Z

Thoughts on this rework @duglin @zrob ?

duglin · 2018-05-08T17:51:07Z

I can live with that.
LGTM

zrob · 2018-05-12T18:01:56Z

This makes sense to me. We also talked about using this as a chance to address #462. I'm happy to go with this or see one more update to mention the unique constraint.

zrob · 2018-05-15T16:37:40Z

LGTM

duglin · 2018-05-15T17:44:02Z

One more little review needed

duglin · 2018-06-05T14:37:39Z

rebase needed

mattmcneeney · 2018-06-05T15:04:25Z

@duglin rebased

duglin · 2018-07-12T18:33:55Z

LGTM

mattmcneeney added the do not merge label Mar 28, 2018

n3wscott reviewed Mar 28, 2018

View reviewed changes

duglin mentioned this pull request Apr 3, 2018

Dashboard Client ID should be globally unique #462

Closed

mattmcneeney force-pushed the move-dashboard-client-to-metadata branch from 88979dd to 2b63a7f Compare April 3, 2018 16:49

zrob reviewed Apr 3, 2018

View reviewed changes

mattmcneeney force-pushed the move-dashboard-client-to-metadata branch from 2b63a7f to 907c49b Compare April 4, 2018 08:28

n3wscott approved these changes Apr 16, 2018

View reviewed changes

duglin reviewed Apr 17, 2018

View reviewed changes

pmorie reviewed Apr 17, 2018

View reviewed changes

mattmcneeney added the needs discussion label Apr 26, 2018

mattmcneeney force-pushed the move-dashboard-client-to-metadata branch from 907c49b to d92fa48 Compare May 8, 2018 17:08

mattmcneeney changed the title ~~Deprecate dashboard_client and move to CF service metadata~~ Move dashboard_client to CF catalog extension described in profile.md May 8, 2018

mattmcneeney force-pushed the move-dashboard-client-to-metadata branch from d92fa48 to 26876a7 Compare May 8, 2018 17:11

n3wscott approved these changes May 14, 2018

View reviewed changes

mattmcneeney added 3 - reviewing PR and removed do not merge needs discussion labels May 21, 2018

mattmcneeney force-pushed the move-dashboard-client-to-metadata branch from 26876a7 to 2a51d82 Compare June 5, 2018 15:04

mattmcneeney force-pushed the move-dashboard-client-to-metadata branch 2 times, most recently from d1da943 to ca4707a Compare July 12, 2018 17:33

n3wscott approved these changes Jul 12, 2018

View reviewed changes

zrob approved these changes Jul 12, 2018

View reviewed changes

move dashboard_client to a cloud foundry extension section

3a536ed

mattmcneeney force-pushed the move-dashboard-client-to-metadata branch from ca4707a to 3a536ed Compare July 12, 2018 18:12

zrob approved these changes Jul 12, 2018

View reviewed changes

n3wscott approved these changes Jul 12, 2018

View reviewed changes

fmui approved these changes Jul 12, 2018

View reviewed changes

mattmcneeney merged commit 7bdc786 into openservicebrokerapi:master Jul 12, 2018

mattmcneeney deleted the move-dashboard-client-to-metadata branch July 12, 2018 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move dashboard_client to CF catalog extension described in profile.md #483

Move dashboard_client to CF catalog extension described in profile.md #483

mattmcneeney commented Mar 28, 2018

cfdreddbot commented Mar 28, 2018

mattmcneeney commented Mar 28, 2018

n3wscott Mar 28, 2018

n3wscott commented Mar 28, 2018 •

edited by avade

Loading

zrob commented Mar 29, 2018

n3wscott commented Mar 30, 2018 •

edited

Loading

duglin commented Apr 3, 2018

mattmcneeney commented Apr 3, 2018

zrob Apr 3, 2018

mattmcneeney Apr 4, 2018

mattmcneeney commented Apr 4, 2018

duglin Apr 17, 2018

mattmcneeney Apr 17, 2018

duglin Apr 17, 2018

duglin Apr 17, 2018

mattmcneeney Apr 17, 2018

duglin Apr 17, 2018

duglin commented Apr 17, 2018

n3wscott commented Apr 17, 2018

pmorie Apr 17, 2018

mattmcneeney Apr 17, 2018

duglin Apr 17, 2018

Samze Apr 24, 2018

pmorie Apr 24, 2018

mattmcneeney commented Apr 24, 2018

mattmcneeney commented May 8, 2018

duglin commented May 8, 2018 •

edited by avade

Loading

zrob commented May 12, 2018

zrob commented May 15, 2018 •

edited by avade

Loading

duglin commented May 15, 2018

duglin commented Jun 5, 2018

mattmcneeney commented Jun 5, 2018

duglin commented Jul 12, 2018 •

edited by avade

Loading

Move dashboard_client to CF catalog extension described in profile.md #483

Move dashboard_client to CF catalog extension described in profile.md #483

Conversation

mattmcneeney commented Mar 28, 2018

cfdreddbot commented Mar 28, 2018

mattmcneeney commented Mar 28, 2018

Choose a reason for hiding this comment

n3wscott commented Mar 28, 2018 • edited by avade Loading

zrob commented Mar 29, 2018

n3wscott commented Mar 30, 2018 • edited Loading

duglin commented Apr 3, 2018

mattmcneeney commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmcneeney commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Apr 17, 2018

n3wscott commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmcneeney commented Apr 24, 2018

mattmcneeney commented May 8, 2018

duglin commented May 8, 2018 • edited by avade Loading

zrob commented May 12, 2018

zrob commented May 15, 2018 • edited by avade Loading

duglin commented May 15, 2018

duglin commented Jun 5, 2018

mattmcneeney commented Jun 5, 2018

duglin commented Jul 12, 2018 • edited by avade Loading

n3wscott commented Mar 28, 2018 •

edited by avade

Loading

n3wscott commented Mar 30, 2018 •

edited

Loading

duglin commented May 8, 2018 •

edited by avade

Loading

zrob commented May 15, 2018 •

edited by avade

Loading

duglin commented Jul 12, 2018 •

edited by avade

Loading