Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattmcneeney
Copy link
Contributor

What is the problem this PR solves?
Closes #676

Checklist:

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

@cfdreddbot
Copy link

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

@vorbidan
Copy link

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
Copy link
Contributor Author

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

@mattmcneeney
Copy link
Contributor Author

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
Copy link
Member

fmui commented Jun 28, 2019

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

@vorbidan
Copy link

vorbidan commented Jun 28, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@mattmcneeney
Copy link
Contributor Author

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 mattmcneeney changed the title Add X-Api-Info-Location header Add X-Api-Info-Location header to the profile document Jul 2, 2019
@mattmcneeney mattmcneeney added this to the 2.16 milestone Jul 2, 2019
@mattmcneeney
Copy link
Contributor Author

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
Copy link

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

@mattmcneeney
Copy link
Contributor Author

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

@mattmcneeney
Copy link
Contributor Author

Review please @fmui @tinygrasshopper

@mattmcneeney
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor Author

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

@gberche-orange
Copy link
Contributor

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

@mattmcneeney
Copy link
Contributor Author

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 mattmcneeney removed this from the 2.16 milestone May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Inbox
Development

Successfully merging this pull request may close these issues.

Should the X-Api-Info-Location header be a part of the spec?
7 participants