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.

Sign up for GitHub

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

GET MAY return creds unable to be used #517

Merged
merged 15 commits into from
Jul 12, 2018
Merged

GET MAY return creds unable to be used #517

Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e755cf2
GET may return creds unable to be used
MHBauer May 15, 2018
a859d31
all or nothing on credentials.
MHBauer May 15, 2018
94beff6
proposed wording that addresses asynchronous bind
MHBauer May 23, 2018
d3f3d3d
add same text to binding create
MHBauer Jul 3, 2018
f296861
accidental word wrapping
MHBauer Jul 3, 2018
0094af7
accidental deletion
MHBauer Jul 3, 2018
3868de4
Merge branch 'master' into get-no-creds
MHBauer Jul 12, 2018
430cea4
add to split response?
MHBauer Jul 12, 2018
41513fd
oops.
MHBauer Jul 12, 2018
5979f6e
move to new location
MHBauer Jul 12, 2018
9f70b38
make sure we're in a GET response
MHBauer Jul 12, 2018
67bdcd8
dug's suggested wording
MHBauer Jul 12, 2018
2fb4952
remove old text
MHBauer Jul 12, 2018
c86b130
move to overall binding block
MHBauer Jul 12, 2018
2ad9c34
hard 80 col wrap
MHBauer Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1374,7 +1374,7 @@ For success responses, the following fields are defined:

| Response Field | Type | Description |
| --- | --- | --- |
| credentials | object | A free-form hash of credentials that can be used by applications or users to access the service. |
| credentials | object | A free-form hash of credentials that can be used by applications or users to access the Service Instance. This field MAY be omitted if the Service Broker either cannot return the credentials or does not want to for security reasons. Note that during an asynchronous bind operation the Service Broker MUST include this field in at least the first response after the Service Binding has been successfully created, otherwise the Platform will not have the credentials to pass along to the application/user. |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@n3wscott does this address the async bind case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little weird to have async bind information in this PR since it may be submitted before #334

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattmcneeney does that mean you want it back out and a reminder in #334 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition introduces an inconstancy. If we allow the omission of the credentials here, we also have to allow the omission for the same reasons when a binding is (re-)created with the identical parameters (status code 200 OK).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MHBauer please make similar edits to the create per Florian's suggestion - then we'll discuss at the f2f

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. The door shuts eventually, but that doesn't mean it's shut from the start.

For synchronous binding the credentials can be returned the once before becoming irretrievable.

For async binding we have to preserve the response for only so long.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MHBauer this is about the case where a replayed create comes in, at that point its like a GET when the params are the same so the broker can then be free to not include the creds. I agree that a non-replayed create should return the creds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is very much in conflict with stateless-ness. It seems like it would now be saying "must be preserved forever". Being able to do this now implies that the credential generation is deterministic. I'm not sure this is clarifying anything anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the equivalent of MUST include this field in at least the first response? We now appear to be saying, "MUST include forever".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/application/Application

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And who is the 'user'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a GET endpoint, so I'm not sure it makes sense to talk about 'during an asyhnchronous bind operation'.

| syslog_drain_url | string | A URL to which logs MUST be streamed. `"requires":["syslog_drain"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the Platform MUST consider the response invalid. |
| route_service_url | string | A URL to which the Platform MUST proxy requests for the address sent with `bind_resource.route` in the request body. `"requires":["route_forwarding"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the Platform can consider the response invalid. |
| volume_mounts | array-of-objects | An array of configuration for mounting volumes. `"requires":["volume_mount"]` MUST be declared in the [Catalog](#catalog-management) endpoint or the Platform can consider the response invalid. |
Expand Down