-
Notifications
You must be signed in to change notification settings - Fork 6
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
Refactor API for cleaner RESTful idioms and support pagination and batching #20
Conversation
Remove the action from the URI for the resource to allow individual referencing and collections of nested resources.
The current API namespace is shared across all the callers so responses will be filtered by visibility of the caller or denied in the absence of authorization to use the passed input parameters.
Implement a paginator over the list of sessions visible to a caller. The paginator is opportunistic as it will return an upper bounded set using the client hint, and opaque as it will make no commitments on the content of the pagination token.
The creation of sessions allows to submit a set of them to treat them as an aggregate when requesting to operate on a certain peering posture. This commit adds a field to the input and output series such that the client can univocally identify which sessions have been processed successfully or not without relying on implicit ordering of the input or result set.
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
I originally thought of opening 4 smaller PRs with one topic branch per change to comment/reject independently, but I noticed it complicates the parallelization of this merge, hence only one PR. I hope this is the expectation. If any comments require changes, please let me know if you want them added as newer commits, or prefer amending the offending ones. |
@caguado looks good, thanks! I have a couple questions for clarification. I'm confused on the difference between the
I just found that, if it's just the server vs initiator, I think we should name it accordingly so it's more intuitive, thoughts? How is
but don't see anything else referencing that. |
What if the server always issued both ids (for individual sessions and for batch configuration requests)? It might be simpler if the server were always the grantor of IDs, and the client the recipient of IDs (which they can map back internally as needed). If the client grants the individual item_ids, how do we handle the case where the server proposes to add additional sessions within a request? Should the server ID those sessions? Should they be ID-less, until the client agrees to configure them? |
I understood we had discarded that behaviour in the call 2 from Jan 10th where if the server wants to propose sessions outside of those requested would either fall the request or we would add another API resource for it. |
Implement a method to depeer by deleting existing sessions. The batched creation method does not create an obvious collection resource on the server side that may be used for a depeering situation therefore this commit only implements individual session deletion. To tackle a situation where depeering is desired for a whole metro or IX while respecting this RESTful interface, we will need to define a separate aggregate resource that can be used to operate atomically on the set of target sessions.
a849067
to
4e57929
Compare
This commit adds the soft deleted semantics described in the original spec where a set of sessions is requested for deletion and the response will include a status in the deletion of the request. This commit also recreates these semantics in the individual session deletion.
Regarding comments around IDs from the last call. I'm adding a description of the intent for the existing IDs, and the role of the additional ones. In every case, it is assumed that IDs are used to univocally identify the resources they refer to in their scope of existence. Existing IDs in the current spec proposed:
Discussed additional identifiers:
So to concrete my proposal, what if we:
|
Thanks for the updates! A few questions:
Let's say ClientA and ServerB want to peer. ClientA sends a request to peer on client_ip with server_ip. Server configures the request, and assigns it id_1. I would assume it ends when the session is deleted, and the server should give a new id, but wanted to confirm.
This is different from the session_id, in that, if (later, when the API supports it) the client wants to change the prefix limit on session (session_id_1), that request would be assigned item_id_1?
I don't think we need to define this--this identifier is exclusive to the client, so the client can choose to use it or not. I would vote we leave it out and let clients do as they wish. I don't see this getting passed back and forth between the client and server. (I guess the use case would be that the client could send a request for a session with the session_name attached, and the server would reply with session_id, and thus the client could easily match between session_name and session_ids?)
Is this necessary? Should we not just force the server to check for duplicate requests on their end?
Makes sense to me. Thanks for the detailed proposal! |
Thanks for all of the added details Carlos!
It sounds like we all agree on this and it that it MUST be globally unique for currently enabled sessions. :) I think having it unique over time should be up to the server, but I can see the argument for making it required if you all feel strongly about it. The rest mostly seem unnecessary to me.
I agree, at least in it the current implementation. If we were to add a request for A unique way to identify the session is always going to be local/remote IPs (and local/remote ASNs if a network has multiple ones) as there can be only one, so I don't think we need
I'm struggling to see the usefullness of this, we could do array offset of the request if we need to, otherwise we can still always fall back to local/remote IP to figure these out.
Again, it could be handy for troubleshooting, but users could just as easily to IP or ASN and timestamp, right? I think we should keep the initial API spec as simple as possible, it's a lot easier to add fields later than to remove them. |
That's the proposal, to make explicit not to recycle resource IDs or otherwise explain why.
I am proposing to make request and response item matching explicit for the duration of the request alone. If the agreement is not to use a unique identifier that only exists for such duration, then how a developer should match items the response to the items they requested? Joining request and response lists by offset? Matching IP-pairs for the session in the response body? I don't agree those are simpler than matching by the client-side-assigned identity to the resource (session_name, or this item_id if we don't agree to add such session_name). But if everyone agrees, we should make that expectation explicit in the doc.
The main use case is ease of use of the API for a human interacting with it. Imagine presenting a list of sessions to act on in a drop down menu in the UI, what would that drop down expect the user to know about the sessions? their random ID? the IPv6 pair concatenated string for the session? Surely, it is up to each consumer of the API to decide, but this field is aiming to provide them with an option that both initiator-receiver can independently agree and explicit.
My first comment on
I argue that not defining behavior explicitly makes it not simple as people will have to agree off spec. It will be easy so long those changes are backward compatible. We are not defining how versioning of the API will work either (content negotiation? embedded in resource path (i.e. |
As per the different discussions, this commit removes references to identifiers used to identify parts of an inflight request that disambiguate failure scenarios. They may be addressed later. It also removes the partial successful reponse conveyed through a redirect error code. The rationale to use only the 200 success code is that the response for a batched request has been successfully processed and the response may still contain references to items which were not successful. Clients will have to introspect the response to decide what to do with those failed items.
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.
Looks good!
This PR contains 4 stacked commits to modify the current API proposal to: