servicebroker icon indicating copy to clipboard operation
servicebroker copied to clipboard

Add `X-Api-Info-Location header` to the profile document

Open mattmcneeney opened this issue 6 years ago • 18 comments

What is the problem this PR solves? Closes #676

Checklist:

  • [x] The swagger.yaml doc has been updated with any required changes
  • [x] The openapi.yaml doc has been updated with any required changes

mattmcneeney avatar Jun 27 '19 08:06 mattmcneeney

:white_check_mark: Hey mattmcneeney! The commit authors and yourself have already signed the CLA.

cfdreddbot avatar Jun 27 '19 08:06 cfdreddbot

Thank you @mattmcneeney While it looks good to me, and because this spec change just follows the de-facto implementation in Cloud Foundry, there is a possibility for a confusion. As @waterlink mentioned in the discussion, CF sends that header in a different format:

X-Api-Info-Location: api.example.org/v2/info

I personally like the proposed format with an explicit platform type mentioned in the value, besides, it is consistent with the Originating Identity header.

vorbidan avatar Jun 27 '19 14:06 vorbidan

Ah, good point. I missed the bit about CF not sending the platform value today. @waterlink would this be easy to add?

mattmcneeney avatar Jun 27 '19 14:06 mattmcneeney

The other thing I don't like about this is the fact that all other headers start with X-Broker, and this one doesn't, but since this header already exists today and may be being used by brokers registered with CF, I didn't feel like we should change this.

mattmcneeney avatar Jun 27 '19 14:06 mattmcneeney

If this header is only supported by CF, the specification should only go into the profile document.

fmui avatar Jun 28 '19 13:06 fmui

It is currently supported by CF, I think the goal is to make it optional for other platforms (k8s) as well, but we have not heard from @jberkhahn if it is even possible...

Don't you think that because os these many questions it is a bit premature to be adding it to the spec PR?

vorbidan avatar Jun 28 '19 14:06 vorbidan

I responded in the issue that there really isn't a k8s equivalent to this, for several reasons. I don't think adding this to the spec is a good idea.

jberkhahn avatar Jul 01 '19 21:07 jberkhahn

That makes sense. Are you happy if I detail this header in the profile doc only?

mattmcneeney avatar Jul 02 '19 09:07 mattmcneeney

Updated the PR, and also removed the header from the openapi and swagger docs since this isn't part of the core spec. cc @jberkhahn

mattmcneeney avatar Jul 02 '19 09:07 mattmcneeney

Hey @vorbidan

We discussed this on today's call and spoke about whether or not we could, instead of using this existing header that CF sends, add a field to the context object along the lines of platform_url which is set to the URL of the platform making the request. That fits the existing pattern of sending platform-specific information on API calls.

What do you think?

mattmcneeney avatar Jul 09 '19 15:07 mattmcneeney

@mattmcneeney I agree - it makes the API cleaner and more explicit, besides it already has a place for platform-specific stuff.

vorbidan avatar Jul 10 '19 16:07 vorbidan

I've moved this header to the profile doc. Reviews please!

mattmcneeney avatar Jul 23 '19 15:07 mattmcneeney

Review please @fmui @tinygrasshopper

mattmcneeney avatar Jul 25 '19 09:07 mattmcneeney

@zrob pointed out on the call today that with the CF CAPI v2->v3 transition going on, the location and contents of this header may change. We might want to wait until we have this functionality in v3.

Heroku also may have a URL and other info that they may want to make available to service providers. This would help their service brokers support multiple platforms where they need to call back into them.

@waterlink mentioned that service brokers will also need to know how to interact with the platform API. The context object does inform brokers of the platform name, but that is only retrievable on certain API calls.

We could introduce a second header informing the service broker of the platform that the URL relates too, or we could include it in the encoded header value, e.g.:

X-Api-Info-Location: cloudfoundry api.example.org/v2/info

Let's continue the discussion on here.

mattmcneeney avatar Aug 20 '19 15:08 mattmcneeney

@mattmcneeney Good point on CF CAPI v3. There does not seem to be an equivalent of /v2/info resource. I still think it is a good idea to have something, which enables a platform callback, but it becomes a lot less trivial.

vorbidan avatar Aug 26 '19 15:08 vorbidan

No objections on today's call on postponing this until the CAPI v3 transition is over.

mattmcneeney avatar Sep 03 '19 15:09 mattmcneeney

@mattmcneeney

The X-Api-Info-Location header is not only necessary to identify distinct cloudfoundry platform clients, but also to enable brokers to discover the OpenIdConnect endpoints and CloudFoundry service instance permission authorization endpoint that are necessary to perform developer AuthN and AuthZ upon service dashboard web accesses.

Here are the current pains associated with using the CC API v2/info endpoint in the Osb cloudfoundry profile:

  • the endpoint is subject to change in CF API v3
  • service brokers have to make an outbound call to fetch the Json content, generating extra unnecessary network traffic
  • the endpoint response is lacking an href to the service instance permission endpoint. Service brokers have to make the assumption that the service instance permission endpoint lives on the same FQDN as the v2/info endpoint
  • the endpoint response has some content which is unrelated to OSB and should be ignored by service brokers, or redundant with other OSB spec fields
{
  "name": "",
  "build": "",
  "support": "https://support.run.pivotal.io",
  "version": 0,
  "description": "Cloud Foundry sponsored by Pivotal",
  "authorization_endpoint": "https://login.run.pivotal.io",
  "token_endpoint": "https://uaa.run.pivotal.io",
  "min_cli_version": "6.22.0",
  "min_recommended_cli_version": "latest",
  "app_ssh_endpoint": "ssh.run.pivotal.io:2222",
  "app_ssh_host_key_fingerprint": "e7:13:4e:32:ee:39:62:df:54:41:d7:f7:8b:b2:a7:6b",
  "app_ssh_oauth_client": "ssh-proxy",
  "doppler_logging_endpoint": "wss://doppler.run.pivotal.io:443",
  "api_version": "2.141.0",
  "osbapi_version": "2.15",
  "routing_endpoint": "https://api.run.pivotal.io/routing",
  "user": "526cc2f7-..."
}

I first considered having the X-Api-Info-Location header instead contain relevant content for brokers such as:

{
  "authorization_endpoint": "https://login.platform.example.com",
  "token_endpoint": "https://uaa.platform.example.com",
  "service_instance_permission_endpoint": "https://api.platform.example.com/v2/service_instances/:guid/permissions"
}

However, after a second thought, these fields might be better suited in the response of the PUT /v2/service_instances/:instance_id endpoint as part of the CloudFoundry context

Benefits of including OIDC and Service-instance-permission endpoints in the context object on the provisionning endpoint:

  • May loosen the OSB API dependency to a specific CC API version (V2 vs V3), and delaying this PR: the OSB CF profile can be kept backward compatible between V2 and V3 if the V3 service-instance-permission endpoint returns the same payload
  • Easier for service brokers authors to consumme OSB (avoid extra network calls and figuring out useful data in /v2/infoendpoint )
  • Since the contract is standard (OpenIdConnect + simple REST endpoint), the broker dependencies on cloudfoundry is reduced, and this same contract can be used for some other clients.
  • The model can be made consistent with K8S clients, which could choose to return the K8S SubjectAccessReview webhook as service instance permission endpoint. In which case, the fields could be made directly available in the PUT /v2/service_instances/:instance_id endpoint response body.

gberche-orange avatar Oct 10 '19 10:10 gberche-orange

Removing the v2.16 milestone from this, and leaving this blocked given the comment above:

No objections on today's call on postponing this until the CAPI v3 transition is over.

mattmcneeney avatar May 13 '20 11:05 mattmcneeney