-
Notifications
You must be signed in to change notification settings - Fork 23
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 MAX_SUBSCRIBE_ID to limit number of subscriptions #491
Conversation
@@ -501,6 +503,9 @@ code, as defined below: | |||
* Duplicate Track Alias: The endpoint attempted to use a Track Alias | |||
that was already in use. | |||
|
|||
* Too Many Subscribes: The session was closed because the subscriber used | |||
a Subscribe ID equal or larger than the current Maximum Subscribe ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it expected that a publisher MUST close an entire session under these circumstances? Or could it just not fulfill the subscription request? I guess maybe we don't have another way to signal the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern would be that the potential impact on aggregating subscribers (relays) could be substantial if they erroneously open too many subscriptions (or are somehow coerced into doing so).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, having a limit here is better than no limit, so I'd be fine revisiting this concern at a later date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aggregating subscribers exist in HTTP proxies as well, and those deal with stream limits on both upstream and downstream fine, so I'd suggest we revisit if it turns out to be a problem people experience.
@@ -993,6 +998,13 @@ client MUST set the PATH parameter to the `path-abempty` portion of the | |||
URI; if `query` is present, the client MUST concatenate `?`, followed by | |||
the `query` portion of the URI to the parameter. | |||
|
|||
#### MAX_SUBSCRIBE_ID {#max-subscribe-id} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary, since you can just send a MAX_SUBSCRIBE_ID immediately after the SETUP message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I added it because we have the equivalent in QUIC.
draft-ietf-moq-transport.md
Outdated
## MAX_SUBSCRIBE_ID {#message-max-subscribe-id} | ||
|
||
A publisher sends a MAX_SUBSCRIBE_ID message to increase the number of | ||
subscriptions a subscriber can create within a session. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if we need to add a non-normative note on what a publisher would like to do this and how do they pick such an value. I do understand that this is very application/implementation specific, however, a few notes on reasons causing the below will be beneficial ( can be in appendix too )
- choosing a value for it
- updating the value
draft-ietf-moq-transport.md
Outdated
|
||
The Maximum Subscribe Id MUST only increase within a session, and | ||
receipt of a MAX_SUBSCRIBE_ID message with an equal or smaller Subscribe ID | ||
value is a 'Protocol Violation'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a subscriber received MAX_SUBSCRIBE_ID with value lesser than previous MAX_SUBSCRIBE_ID sent by the publisher, are we saying here that the subscriber needs to close the session ? I am not sure if that is feasible to do or can be enforced.
Thanks @ianswett . this looks like a good improvement. I have few clarification questions and bit of extra work for you to add some informative text, if my comment seems ok to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a really complicated way to limit the subscribes. I understand how it mirror streams but that had pretty different constrains. Why don't we just have an subscribe error of "too many subscriptions"?
Unfortunately, that makes it much more difficult for the server to tell abusive clients from valid ones, as we discovered with the recent HTTP/2 rapid reset attacks. In the end we did develop heuristics, but they're imperfect and in the process we broke a bunch of applications in an effort to defend against DDoS attacks. In contrast, QUIC had a system like this and experienced no issues and we didn't need to develop any new heuristics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed open issues in the 7 August 2024 virtual, not recorded here. Remaining issue (just do a SUBSCRIBE_ERROR?) went to the list and chairs declared rough consensus, although at least 2 participants would prefer not to have this.
draft-06 encode/decode support for MaxSubscribeId MAX_SUBSCRIBE_ID control message - [PR 491](moq-wg/moq-transport#491) - [PR 528](moq-wg/moq-transport#528) - [§6.16](https://www.ietf.org/archive/id/draft-ietf-moq-transport-06.html#section-6.16) Just parsing for the control message so far, not wired up anywhere yet, will need error codes
draft-06 encode/decode support for MaxSubscribeId MAX_SUBSCRIBE_ID control message - [PR 491](moq-wg/moq-transport#491) - [PR 528](moq-wg/moq-transport#528) - [§6.16](https://www.ietf.org/archive/id/draft-ietf-moq-transport-06.html#section-6.16) Just parsing for the control message so far, not wired up anywhere yet, will need error codes
Fixes #272
Some diffs are due to me reflowing text to fit the line limits, so please ignore those.
Once we have this and a limit on announce IDs, it seems likely that we'll be able to get rid of 'Role', but for now it can stay.