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 proposal for binding credential schema #96

Closed
wants to merge 3 commits into from

Conversation

bmelville
Copy link
Contributor

Service Binding outputs are a generic map of data, and must be
schematized in the same way that input parameters are. This allows
customers and tooling to reason about the expected output when binding
to a service instance of a particular service type.

@shalako
Copy link
Contributor

shalako commented Oct 11, 2016

The current spec specifies four fields that a broker can return; credentials, syslog_drain_url, route_service_url, and volume_mounts. http://docs.cloudfoundry.org/services/api.html#binding. The last three are not generic and quite specific. credentials is more generic but guidelines for a broker response is documented here: http://docs.cloudfoundry.org/services/binding-credentials.html.

I believe this use case is quite different than input parameters. Exposing a schema for input parameters could be used by a client to dynamically generate a user experience in order to construct the parameters field. Would you expect a client to do anything dynamic with the schema of the bind output? I would expect a broker client author (e.g. Cloud Foundry) will have to do custom development for each expected value.

It seems to me that all this accomplishes is a mechanism for documentation. It isn't actually a programmatic interface.

Further, by moving supported values from the API spec to a generic field outputs field, brokers can now return anything they want in the bind response (is that actually the use case you're trying to solve here?). This means that broker clients can no longer implement support for known responses, their behavior must now be custom to a particular service. You would then have service brokers that are compatible with Cloud Foundry, and service brokers that are compatible with Kubernetes.

The whole point of this effort from CF's perspective is to allow ISVs to implement one API and offer their service across platforms, encouraging growth of the ecosystem of service brokers. That means that all platforms potentially need to support any bind response. In order to achieve that, valid bind responses should be part of the spec.

As an example, if your broker's bind response included something Cloud Foundry doesn't know what to do with, then your broker is not compatible with Cloud Foundry.

Maybe you are coming from a perspective that an end-user developer is consuming the bind response from the broker. In Cloud Foundry, only the credentials field is passed through to the enduser (and only under some circumstances). Cloud Foundry does specific operations based on the presence of all four fields. If Cloud Foundry can't intermediate the response, then we're not able to provide platform value-add.

@bmelville
Copy link
Contributor Author

bmelville commented Oct 11, 2016

The use-cases are:

  1. the ability for tooling (e.g., UI) to convey credential output schemas to consumers of bindings
  2. the ability for a binding to output various different forms of credentials
  3. the future ability for "type" checking bindings with services that are consuming bindings (e.g., this binding outputs a connection string, rather than separate fields, but the consuming service expects something different)

The two main issues I have with your argument are:

  1. It seems that you are asserting that credentials are well-formed object, but the documentation specifies otherwise: "Choose from the following list of credential fields if possible, though you can provide additional fields as needed."
  2. Credentials come in vastly different forms than just username and password. For example, connection strings, OAuth tokens, API keys, base64 enconded rich JSON objects are all forms of credentials that I've seen used by services, which need to be able to be returned as part of a binding. This problem is already prevalent in the API, given that someone might set the "uri" field and not the other fields.

Finally, why would we rely on a service broker author to provide the correct documentation for what to expect from a binding when the binding can self describe in a way that is usable programmatically in visualization/configuration UIs and other forms of tooling?

@shalako
Copy link
Contributor

shalako commented Oct 11, 2016

Thank you for the use cases. Very helpful.

the ability for tooling (e.g., UI) to convey credential output schemas to consumers of bindings... why would we rely on a service broker author to provide the correct documentation for what to expect from a binding when the binding can self describe in a way that is usable programmatically in visualization/configuration UIs and other forms of tooling?

OK, I get it now. This would allow a UI client to flesh out the marketplace metadata with documentation on credentials only (there will still need to be documentation for the service elsewhere). If credentials exist; not all services return credentials. I agree this would improve the UX in some cases but is this really a blocker to adoption of the spec? There are other ways an end-user could discover the format of bind credentials (e.g. bind then look at the creds).

I agree broker API spec is not prescriptive with regard to format of credentials; the field is free-form object. In CF the field is opaque except in the buildpack community where there is a good deal of automation that depends on specific credentials fields. A broker is free to return any format for credentials they like, but the developer miss out on the benefit of this automation. So we provide guidance to brokers to use common fields where possible. I would expect the same would be true in other platforms. So for credentials, the broker response is platform agnostic.

My concerns with platform-specific brokers stem from interpreting this outputs field implying that a broker could return arbitrary top-level fields. Did I misinterpret the intent? Maybe I over-reacted, since I haven't seen a proposal to change the fields of the bind response.

syslog_drain_url, route_service_url, and volume_mounts trigger platform actions in CF (e.g. mount a volume in the container). These value of these fields are not exposed to end-users. Would you propose that these fields are also documented in the generic outputs property?

@pmorie
Copy link
Contributor

pmorie commented Oct 12, 2016

I'm still rolling this PR around in my head, but let me chime in on this:

My concerns with platform-specific brokers stem from interpreting this outputs field implying that a broker could return arbitrary top-level fields. Did I misinterpret the intent? Maybe I over-reacted, since I haven't seen a proposal to change the fields of the bind response.

Platform-specific brokers are an anti-goal, IMO.

@bmelville
Copy link
Contributor Author

bmelville commented Oct 12, 2016

I agree this would improve the UX in some cases but is this really a blocker to adoption of the spec? There are other ways an end-user could discover the format of bind credentials (e.g. bind then look at the creds).

I totally agree with this and this should be an optional field for service broker creators to add value for their customers.

My concerns with platform-specific brokers stem from interpreting this outputs field implying that a broker could return arbitrary top-level fields. Did I misinterpret the intent? Maybe I over-reacted, since I haven't seen a proposal to change the fields of the bind response.

Definitely have no intention of proposing changes to output fields for bindings, only schematize those outputs which need it. I think it is sufficient to have a credentials section.

syslog_drain_url, route_service_url, and volume_mounts trigger platform actions in CF (e.g. mount a volume in the container). These value of these fields are not exposed to end-users. Would you propose that these fields are also documented in the generic outputs property?

Perhaps having it named outputs is leading to the confusion. Somewhere in the history of the other PR, someone suggested renaming credentials to outputs.

I think this could go two ways: either there are multiple different binding types which may have outputs, in which case it may be advantageous to have schematization for each of them in the outputs section. e.g.,

schemas:
  service_bindings:
    outputs:
      credentials:
        ...

or credential bindings are the only type of binding with schematized outputs, in which case, we can move the field name back from outputs to credentials, like:

schemas:
  service_bindings:
    credentials:
      ...

@bmelville bmelville changed the title Add proposal for output schemas. Add proposal for binding credential schema Oct 12, 2016
@MHBauer MHBauer mentioned this pull request Oct 13, 2016
@shalako
Copy link
Contributor

shalako commented Oct 17, 2016

This makes sense to me:

schemas:
  service_bindings:
    outputs:
      credentials:
        ...

Especially if the PR for a schema for the parameters input field (#74) was schemas.service_instances.inputs.parameters

Signed-off-by: Brendan Melville <bmelville@google.com>
Service Binding outputs are a generic map of data, and must be
schematized in the same way that input parameters are. This allows
customers and tooling to reason about the expected output when binding
to a service instance of a particular service type.

Signed-off-by: Brendan Melville <bmelville@google.com>
@angarg12
Copy link
Contributor

angarg12 commented Nov 8, 2016

Is there any open question that should be addressed on this PR?

Also part of this PR seems to overlap with #74. Should we merge both together, or wait for #74 to be merged and then discuss this one as a follow up?

@shalako
Copy link
Contributor

shalako commented Nov 8, 2016

@angarg12 This PR is for a different use case, and I believe should be independent from #74. We must remain diligent to keep the scope of each change as small as possible, as it is easier to reason about and quicker to implement. #74 is higher priority and gives us a well scoped use case to work through many questions about how to enable brokers to declare a schema for things. Once all those questions are worked through, adding support for additional use cases, like this one may be easier.

@shalako
Copy link
Contributor

shalako commented Jan 25, 2017

Closing in favor of Issue #116, as this feature is still in proposal phase.

@shalako shalako closed this Jan 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants