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

Adding a Stream ID #4

Closed
FragLegs opened this issue Feb 5, 2022 · 17 comments
Closed

Adding a Stream ID #4

FragLegs opened this issue Feb 5, 2022 · 17 comments

Comments

@FragLegs
Copy link
Contributor

FragLegs commented Feb 5, 2022

In previous discussion, it was decided that we needed a way for there to be potentially more than one stream between a transmitter and receiver. Some use cases for this were the needs of GDPR to keep data localized, having different polling frequencies for different types of events, etc. What follows is a high-level proposal for how we could add stream IDs without significantly changing the structure of the SSE framework. I would like to have some discussion here so that the high-level details can be settled before I create a pull request for the change.

Support for default and/or named streams

First of all, because stream IDs dictate something about how the data model works, transmitters should be able to decide whether they support streams with IDs or not. Currently, there is a single default stream per receiver. I propose adding a new optional property to the transmitter configuration response from /.well-known/sse-configuration:

stream_types: [
  "https://schemas.openid.net/secevent/stream_type/default",
  "https://schemas.openid.net/secevent/stream_type/named"
]

This allows the transmitter to support both default and named streams. Many, perhaps most, receivers will only care about having a single stream, so continuing to support a default stream is user friendly.

The default stream does not need to be created. It exists as soon as there is a transmitter/receiver relations if the transmitter supports default streams. Any named streams must be explicitly created.

Changes to the configuration_endpoint

Changes to the stream configuration object

The most significant changes to the spec are centered around the configuration_endpoint, in the GET, POST and DELETE verbs, as well as in a new PATCH verb (detailed below).

With the exception of DELETE, all of these verbs either take a Stream Configuration object as an argument or return a Stream Configuration object. In all cases, we would update the Stream Configuration object to include a new optional parameter, stream_id:

  • stream_id (if missing, assume default stream)
  • iss (read only)
  • aud (read only)
  • events_supported (read only)
  • events_delivered (read only)
  • min_verification_interval (read only)
  • events_requested (read/write)
  • delivery (read/write)
  • format (read/write)

In some ways, you could think of a default stream as a stream whose stream_id is null.

Create a Stream - configuration_endpoint [POST]

Creates a new stream

Arguments

  • stream configuration object
    • All attributes of the object are optional (but will raise error if stream_id is missing)

Results

  • 200: New stream created, returns stream configuration object
  • 400: Request cannot be parsed or a read-only value is present and incorrect
  • 401: Authorization failed or is missing
  • 403: Event receiver is not allowed to create a stream
  • 409: A stream with stream_id already exists. Because the default stream always exists, calling this endpoint without a stream_id will always raise a 409 (or 501 if default streams are not supported)
  • 501: Not implemented. A stream_id is present and named streams are not supported, or stream_id is missing and default streams are not supported.

Get Stream Configuration - configuration_endpoint [GET]

Gets the configuration of a stream.

Arguments

  • stream_id (optional)

Results

  • 200: If the stream_id argument is provided, returns the stream configuration object with that stream_id. Otherwise, returns the stream configuration of the default stream.
  • 401: Authorization failed or is missing
  • 403: Event receiver is not allowed to get streams
  • 404: A stream_id is present and the stream does not exist, or stream_id is missing and default streams are not supported.

Update a Stream - configuration_endpoint [PATCH]

Updates a stream's configuration.

Arguments

  • A stream configuration object

Results

  • 200: Stream updated, returns stream configuration object
  • 400: Request cannot be parsed or read only property is incorrect
  • 401: Authorization failed or is missing
  • 403: If the Event Receiver is not allowed to update a stream
  • 404: If the stream_id is present and the stream does not exist or stream_id is missing and default streams not supported

Open question

How should we treat missing read/write values in the stream configuration object?

  1. PATCH semantics imply that missing values should be interpreted as "leave the value as it is".
  2. Current SSE framework interprets missing read/write values as "delete the value". But interprets missing read-only values as "leave the value as it is".
  3. Google best practice suggests including a field mask to indicate which fields are being updated. Do we want that?

Delete a Stream - configuration_endpoint [DELETE]

Deletes a named stream or resets a default stream to its default configuration. When a stream is deleted (or reset for default streams) all subjects that have been added to the stream, events that have been enqueued in the stream, or status that has been set on the stream should also be deleted.

Arguments

  • stream_id (optional)

Results

  • 200: Success
  • 401: Authorization failed or is missing
  • 403: Event receiver is not allowed to delete streams
  • 404: A stream_id is present and the stream does not exist, or stream_id is missing and default streams are not supported.

New endpoint: list_streams_endpoint

A new endpoint will be included in the transmitter configuration response from /.well-known/sse-configuration:

list_streams_endpoint: "https://tr.example.com/sse/mgmt/list_streams"

List stream IDs - list_streams_endpoint [GET]

Returns a list of the stream IDs in use

Arguments

  • None

Results

  • 200: A list of stream IDs. Note that the default stream has a null ID and will not be present in the list.
  • 401: Authorization failed or is missing
  • 403: Event receiver is not allowed to list streams

Other changes

The following endpoints all get an optional stream_id argument, either in the body of the POST endpoints or the URL params of the GET endpoint. If the stream_id argument is missing, assume the default stream is being indicated.

All five methods get a new possible return value:

  • 404: A stream_id is present and the stream does not exist, or stream_id is missing and default streams are not supported.
@randiepathirage
Copy link

This proposal is a good improvement and it covers most of the missing parts in the SSE 1.0 - draft 01.
I wonder what happens if we send a GET request to list_streams_endpoint when there are no "named streams" created, will it still respond with 200 OK and empty list?

@FragLegs
Copy link
Contributor Author

This proposal is a good improvement and it covers most of the missing parts in the SSE 1.0 - draft 01. I wonder what happens if we send a GET request to list_streams_endpoint when there are no "named streams" created, will it still respond with 200 OK and empty list?

That's a good question. If there are no named streams, it would be an empty list. Perhaps the endpoint should be list_named_streams_endpoint and it will throw a 501 if you don't have the "named streams" feature turned on. I think that might be a little less confusing. Something like this:

New endpoint: list_named_streams_endpoint

A new endpoint will be included in the transmitter configuration response from /.well-known/sse-configuration:

list_named_streams_endpoint: "https://tr.example.com/sse/mgmt/list_named_streams"

List named stream IDs - list_named_streams_endpoint [GET]

Returns a list of the stream IDs for any named streams in use

Arguments
None

Results

  • 200: A list of stream IDs. Will be an empty list if no named streams have been created yet.
  • 401: Authorization failed or is missing
  • 403: Event receiver is not allowed to list streams
  • 501: Not implemented. This transmitter does not have named_streams configured.

@FragLegs
Copy link
Contributor Author

FragLegs commented Mar 2, 2022

After further discussion, we decided to lean less on the need for backwards compatibility. Instead of having default and named streams, we will only have named streams. The receiver MUST create the stream before using it. The proposal is rewritten below with those updates in mind.

Changes to the configuration_endpoint

Changes to the stream configuration object

The most significant changes to the spec are centered around the configuration_endpoint, in the GET, POST and DELETE verbs, as well as in the new PATCH and PUT verbs (detailed below).

With the exception of DELETE, all of these verbs either take a Stream Configuration object as an argument or return one or more Stream Configuration objects. In all cases, we would update the Stream Configuration object to include a new parameter, stream_id:

  • stream_id (read only)
  • iss (read only)
  • aud (read only)
  • events_supported (read only)
  • events_delivered (read only)
  • min_verification_interval (read only)
  • events_requested (read/write)
  • delivery (read/write)
  • format (read/write)

Create a Stream - configuration_endpoint [POST]

The transmitter creates a new stream with a unique stream ID

Arguments

Only the optional read/write elements of the stream configuration object are allowed:

  • events_requested
  • delivery
  • format

Results

  • 200: New stream created, returns stream configuration object
  • 400: Request cannot be parsed
  • 401: Authorization failed or is missing
  • 403: Event receiver is not allowed to create a stream

Get Stream Configuration - configuration_endpoint [GET]

Gets the configuration(s) of a stream or streams.

Arguments

  • stream_id (optional)

Results

  • 200: Returns a list of stream configuration objects. If the stream_id argument is provided, the list only contains the stream configuration object with that stream_id.
  • 401: Authorization failed or is missing
  • 403: Event receiver is not allowed to get streams
  • 404: The stream_id does not map to an existing stream

Update a Stream - configuration_endpoint [PUT]

Replaces a stream's configuration. Missing read/write values are deleted. Missing read-only values cause an error.

Arguments

  • A stream configuration object

Results

  • 200: Stream updated, returns stream configuration object
  • 400: Request cannot be parsed or read only property is incorrect or missing
  • 401: Authorization failed or is missing
  • 403: If the Event Receiver is not allowed to update a stream
  • 404: If the stream_id does not map to an existing stream (takes precedence over the 400 error)

Update a Stream - configuration_endpoint [PATCH]

Partial update for a stream's configuration. Missing read/write values are ignored. Missing read-only values are ignored.

Arguments

  • A stream configuration object

Results

  • 200: Stream updated, returns stream configuration object
  • 400: Request cannot be parsed or read only property is incorrect
  • 401: Authorization failed or is missing
  • 403: If the Event Receiver is not allowed to update a stream
  • 404: If the stream_id is present and does not map to an existing stream (takes precedence over the 400 error)

Delete a Stream - configuration_endpoint [DELETE]

Deletes a stream. When a stream is deleted all subjects that have been added to the stream, events that have been enqueued in the stream, or status that has been set on the stream should also be deleted.

Arguments

  • stream_id (required)

Results

  • 201: Success
  • 401: Authorization failed or is missing
  • 403: Event receiver is not allowed to delete streams
  • 404: The stream_id does not map to an existing stream

Other changes

The following endpoints all get a required stream_id argument, either in the body of the POST endpoints or the URL params of the GET endpoint.

All five methods get a new possible return value:

  • 404: The stream_id does not map to an existing stream

@tulshi
Copy link
Contributor

tulshi commented Mar 2, 2022

I'm a bit concerned about the "PUT" method of updating a stream:

  1. If a Transmitter doesn't support a certain read-only value (e.g. min_verification_interval), then is it an error for the Receiver to call the PUT method without specifying the value?
  2. If a Transmitter modifies their implementation and starts supporting a new read-only value (either one that is defined now, or may be defined later as the spec evolves), then does the Receiver implementation break because it is not providing the read-only value?

@tulshi
Copy link
Contributor

tulshi commented Mar 3, 2022

It'll be good to clarify what happens when read-only values are specified in the PATCH (update) method. Right now the proposal states that the missing read-only values are ignored. The notes from the Jan 22 call do not make this clear, so we should probably discuss and close it in the next call. In those notes, we had talked about reviewing industry best practices. The Google aip.dev update description uses "update masks" to determine which values to update. My recommendation is that if the Receiver sends a read-only value in the request, it should be verified to match. If a read-only value is missing, it should be ignored. This is so that if the Receiver has a disconnect about the Transmitter's configuration, it should not be allowed to update that configuration.

@FragLegs
Copy link
Contributor Author

FragLegs commented Mar 4, 2022

If a Transmitter doesn't support a certain read-only value (e.g. min_verification_interval), then is it an error for the Receiver to call the PUT method without specifying the value?

This is a good point. For the PUT behavior, the expected use case is the Receiver calls GET, modifies the object that was returned, and then sends the modified object. I think we should make all of the read-only values REQUIRED in the data that is sent from the transmitter. If a transmitter doesn't want to support min_verification_interval they can set it to 0 seconds (which would be functionally the same as not having it).

If a Transmitter modifies their implementation and starts supporting a new read-only value (either one that is defined now, or may be defined later as the spec evolves), then does the Receiver implementation break because it is not providing the read-only value?

You would get an error in that case. The PUT behavior really expects that the Receiver gets the configuration from GET or POST first. So, if you try to PUT and get a 400 error, you may want to call GET and try again with the updated object.

The reason we need both PUT and PATCH is so that we can delete read-write values. Since PATCH is supposed to ignore missing values, there is no way to tell it to delete something. PUT gives us that ability.

My recommendation is that if the Receiver sends a read-only value in the request, it should be verified to match. If a read-only value is missing, it should be ignored. This is so that if the Receiver has a disconnect about the Transmitter's configuration, it should not be allowed to update that configuration.

I'm pretty sure that is what we had discussed on the call. And it is what the PATCH behavior does in this implementation (notice the 400 error that gets thrown if a read-only value is incorrect).

@FragLegs
Copy link
Contributor Author

FragLegs commented Mar 4, 2022

Also, what would y'all think about putting aud in the POST arguments? Let the Receiver specify at the time of Stream creation what the aud value should be.

Right now, two pieces of info need to be shared out of band: the bearer token and the aud value. It would be really nice to make it so that only the bearer token needed to be shared outside this framework.

@ghost
Copy link

ghost commented Nov 29, 2022

Just catching up on this...

Overall this looks like a good addition to enable multiple streams to be included within the spec. Up till now, handling multiple streams between two entities required an implementation-specific mechanism.

A few more detailed comments:

  • Stream ID:

    • Is there a reason for making stream_id a parameter, rather than part of the path. E.g.
      • GET /sse/stream/1234 - will fetch a single stream config.
      • GET /sse/stream - will fetch a list of stream IDs.
      • PUT/PATCH/DELETE all need to include the stream_id element of the path.
    • (Pardon me if this has been discussed to death before!) A more general question: is there a reason we don't make the API more generally REST-like? E.g. POST /sse/stream/1234/subject:add
  • Read-only values:

    • Why should the ER have to send read-only config values for POST/PUT/PATCH? Instead the ET could ignore read only values in these requests, allowing the ER to send the or not as they see fit.
    • Alternatively, separate out read-only config from editable config as they are "owned" by different parties in the exchange.

@adeinega
Copy link
Contributor

adeinega commented Nov 29, 2022

@frankt-vmware, I do wish to see more REST-like APIs, one thing that I would suggest using

POST /sse/stream/1234/subject

instead of

POST /sse/stream/1234/subject:add

The suffix "add" is unnecessary here, and if the specification still wants to use it, it should be using something like /sse/stream/1234/subject%3Aadd.

@ghost
Copy link

ghost commented Nov 29, 2022

@adeinega

POST /sse/stream/1234/subject

Even better.

@FragLegs
Copy link
Contributor Author

I like the idea of adding it to the path, except that we have to figure out how to encode that information in the /.well-known response. Right now, /.well-known returns something like this:

   {
     "issuer":
       "https://tr.example.com",
     "jwks_uri":
       "https://tr.example.com/jwks.json",
     "delivery_methods_supported": [
       "https://schemas.openid.net/secevent/risc/delivery-method/push",
       "https://schemas.openid.net/secevent/risc/delivery-method/poll"],
     "configuration_endpoint":
       "https://tr.example.com/sse/mgmt/stream",
     "status_endpoint":
       "https://tr.example.com/sse/mgmt/status",
     "add_subject_endpoint":
       "https://tr.example.com/sse/mgmt/subject:add",
     "remove_subject_endpoint":
       "https://tr.example.com/sse/mgmt/subject:remove",
     "verification_endpoint":
       "https://tr.example.com/sse/mgmt/verification",
     "critical_subject_members": [ "tenant", "user" ]
   }

where those specific endpoints are whatever the transmitter wants them to be. We would need to a) become prescriptive about what the endpoints must be, and b) figure out how to specific "stream id goes here" in the URL.

I'm not sure if those changes are non-starters for this discussion.

@adeinega
Copy link
Contributor

@FragLegs, add_subject_endpoint and remove_subject_endpoint aren't really REST-like endpoints, they both can be replaced by one, say subject_endpoint. A receiver/client would use either

POST /sse/stream/1234/subject

or

DELETE /sse/stream/1234/subject

depending on what it wants to achieve.

@FragLegs
Copy link
Contributor Author

Agreed. That doesn't fix the problem though.

@FragLegs
Copy link
Contributor Author

@adeinega I added an issue addressing your point about not needing distinct add/remove endpoints in #39

@ghost
Copy link

ghost commented Nov 29, 2022

@FragLegs Who owns the format of the contents of /.well-known? Is this something invented in the SSE/F spec, or have we imported it from elsewhere?

RFC8615 defines /.well-known/ (note the trailing /) as a container for well known URIs, but does not talk about /.well-known itself.

If we are free to make up the content, then we could follow typical REST API specs and use the :var notation for variables.

Giving:

   {
...
     "configuration_endpoint":
       "https://tr.example.com/sse/mgmt/stream/:stream_id",
     "status_endpoint":
       "https://tr.example.com/sse/mgmt/stream/:stream_id/status",
     "subject_endpoint":
       "https://tr.example.com/sse/mgmt/stream/:stream_id/subject",
     "verification_endpoint": [
       "https://tr.example.com/sse/mgmt/verification",
       "https://tr.example.com/sse/mgmt/stream/:stream_id/verification"
     ],
...
   }

Here the verification endpoint has two modes /verification to send a message to all streams for this ER, and /stream/:stream_id/verification to send for just a specific stream.

The caller would have to know that subject requires POST and DELETE.

Thoughts?

@adeinega
Copy link
Contributor

@frankt-vmware, I believe /.well-known/sse-configuration and its content is inspired by https://openid.net/specs/openid-connect-discovery-1_0.html and https://www.rfc-editor.org/rfc/rfc8414.

FragLegs added a commit that referenced this issue Jan 4, 2023
* Add stream_id to SSE Framework spec as per Issue 4: #4

* Update README with development instructions and fix error in Makefile

* Added note to PUSH/POLL section about uniqueness requirements for the URLs

* Add explanation about what an Event Stream is

* Change terms to Transmitter-Supplied and Receiver-Supplied
@tulshi tulshi closed this as completed Feb 23, 2023
@independentid
Copy link

Responding with 409 on create for a server that supports only one stream is incorrect. 409 is meant for situations like two clients submitting a put causing one to be out of date. 409 is inappropriate because it invites the client to try again.

403 is more appropriate because it is a policy decision (even though it may be technical) to support only one stream. 403 tells the client not to retry which is appropriate here.

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

No branches or pull requests

5 participants