openservicebrokerapi / servicebroker

Open Service Broker API Specification
https://openservicebrokerapi.org/
Apache License 2.0
1.19k stars 436 forks source link

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

Open mattmcneeney opened 5 years ago

mattmcneeney commented 5 years ago

What is the problem this PR solves? Closes #676

Checklist:

cfdreddbot commented 5 years ago

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

vorbidan commented 5 years ago

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.

mattmcneeney commented 5 years ago

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

mattmcneeney commented 5 years ago

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.

fmui commented 5 years ago

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

vorbidan commented 5 years ago

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?

jberkhahn commented 5 years ago

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.

mattmcneeney commented 5 years ago

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

mattmcneeney commented 5 years ago

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 commented 5 years ago

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?

vorbidan commented 5 years ago

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

mattmcneeney commented 5 years ago

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

mattmcneeney commented 5 years ago

Review please @fmui @tinygrasshopper

mattmcneeney commented 5 years ago

@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.

vorbidan commented 5 years ago

@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.

mattmcneeney commented 5 years ago

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

gberche-orange commented 4 years ago

@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:

{
  "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:

mattmcneeney commented 4 years ago

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.