Refactor messages as per modified API #14
Conversation
This commit adapts the structures and wording around the changes made to the reference OpenAPI spec from [1]. [1] bgp/autopeer#17
draft-ramseyer-grow-peering-api.md
Outdated
| 2. TimeWindow: Time window indicating when sessions will be configured after being notified (may be 0 if sessions are already configured on receiver side) | ||
| 3. isInboundFiltered: optional bool that indicates whether prefixes will be filtered inbound. If this is set to true, the time window should be set for how long the prefixes will be filtered. | ||
| 4. isOutboundFiltered: optional bool that indicates whether prefixes will be filtered outbound. If this is set to true, the time window should be set for how long the prefixes will be filtered. If the outbound limit is longer than the inbound limit time, the time window should be set to the max of inbound versus outbound. | ||
| * Server returns and error message which indicates that all of the sessions requested have been rejected and the reason for it. |
draft-ramseyer-grow-peering-api.md
Outdated
| * Given a list of Session Arrays, remove the sessions in that list. | ||
| * This API serves as a notification to the server, as the client may remove the sessions before sending the request to the server. | ||
| * Server replies back with request ID and deletion status (complete, in-progress). | ||
| * /sessions: ADD/RETRIEVE sessions visible to the calling PEER |
There was a problem hiding this comment.
Since REMOVE IX peer is deleted, shall we add /DELETE here as well?
Line 240 was the only place we mentioned delete as an option for PX sessions, so let's mention it here as well.
Something like:
'''
- Batch delete existing session resources
- Delete BGP sessions between peers, at the desired exchange.
- Below is based on OpenAPI specification: https://github.com/bgp/autopeer/blob/main/api/openapi.yaml
- DELETE: /sessions
- Request body: Session Array
- Responses:
- 200 OK:
- Contents: Session Array (all sessions in request accepted for deletion).
- 300:
- Contents: Modified Session Array, with rejected or additional sessions.
- 400:
- Error
- 403:
- Unauthorized to perform the operation
'''
- Unauthorized to perform the operation
- 200 OK:
There was a problem hiding this comment.
Sure, I thought was a gap from the doc to the OpenAPI spec and cleared the ref. Idem for the TimeWindow et al. attributes that did not appear in the spec.
I can add this back with the DELETE as you describe so that folks comment on its expected batch behaviour dual to the batched creation.
There was a problem hiding this comment.
Good catch, we need to be able to depeer. :)
There was a problem hiding this comment.
When reading through the proposal, I realized I missed the soft-delete semantics implied where the status of the session deletion is reported back so to make the deletion idempotent, I'm adding all this back to the API changes in bgp/autopeer#20
draft-ramseyer-grow-peering-api.md
Outdated
| @@ -310,7 +291,7 @@ If the client configured B as well, it will not come up. | |||
|
|
|||
| This draft encourages peers to set up garbage collection for unconfigured or down peering sessions, to remove stale configuration and maintain good router hygiene. | |||
|
|
|||
| Related to rejection, if the server would like to configure additional sessions with the client, the server may reply back with additional peering sessions D and E. | |||
| Related to rejection, if the server would like to configure additional sessions with the client, the server may either reject all the session that do not meet the criteria caused by such absence in the client's request or issue a separate request to the client's server requesting those additional peering sessions D and E. | |||
There was a problem hiding this comment.
Why split into a separate request?
I suppose it makes the handshaking simpler, in that, if the server doesn't agree, it just rejects, but I worry it causes a few problems:
- splitting up more config requests results in additional config pushes to routers (maybe this doesn't matter)
- client and server end up in a weird looping state of "client requests ABC". Server would accept ABC, but not D. Server rejects all sessions and submits request to client for ABCD. Client rejects ABC, and sends another request for ABC. This loops forever, and ABC never gets configured. I suppose that ties in to "how do we escalate to a human."
We will have to develop some notion of handshaking for LOAs, unfiltering, etc, later, so it seems OK to handshake here as well on the list of sessions.
But, I think this way is OK as well, and I'm happy to go with it--if this is the REST-API standard way to do it, that's fine by me.
In that case, I propose one clarification:
" issue a separate request to the client's server requesting those additional peering sessions D and E." -> "approve the client's request and issue a separate request to the client's server requesting those additional peering sessions D and E."
There was a problem hiding this comment.
Why split into a separate request?
I suppose it makes the handshaking simpler, in that, if the server doesn't agree, it just rejects, but I worry it causes a few problems:
- splitting up more config requests results in additional config pushes to routers (maybe this doesn't matter)
Certainly config pushes and overall turnup velocity may be affected, although this is an implementation detail of how each peer may implement their deployment system. IMO the API should reduce ambiguity between what is proposed and what is accepted without implying additional API calls to be made to finish the handover over the same API method.
If we aim to create a two-phase-commit-kind semantics, what if we create an additional resource (e.g. /proposals/{proposal_id} which tracks a changeable set of sessions and has a state machine attached to reflect the negotiation status such that when the proposal is accepted by both parties, this triggers the creation of the /sessions/{session_id} on the receiver and possibly on the initiator via a back channel mechanism?
- client and server end up in a weird looping state of "client requests ABC". Server would accept ABC, but not D. Server rejects all sessions and submits request to client for ABCD. Client rejects ABC, and sends another request for ABC. This loops forever, and ABC never gets configured. I suppose that ties in to "how do we escalate to a human."
Exactly why IMO this API is meant to be atomic and model the underlying resources such that initiators will get a clear yes/no response from the receiver. If we intend to track negotiation over time, I think we should model explicitly with a dedicated resource.
We will have to develop some notion of handshaking for LOAs, unfiltering, etc, later, so it seems OK to handshake here as well on the list of sessions.
IMO, this is separate from the sessions over an IX because in those situations the fabric over which the BGP speakers interconnect is already set by both parties so all pertaining to physical layer, LOAs, LAGs and their shape, capacity, IP allocations, etc. would be modelled separately in a /ports/{port_id} or /speaker/{speaker_id} resource when modelling new IXs or PNIs.
So I would actually leave all that ambiguity of the physical layer on the side right now and focus the message only creating the API scaffold for BGP sessions, and somewhat negotiating their cardinality, and the identity federation part.
But, I think this way is OK as well, and I'm happy to go with it--if this is the REST-API standard way to do it, that's fine by me.
In that case, I propose one clarification: " issue a separate request to the client's server requesting those additional peering sessions D and E." -> "approve the client's request and issue a separate request to the client's server requesting those additional peering sessions D and E."
Ack, let me edit it as proposed. In light of our concern on work required on smaller peers, I wasn't sure either if AS1 exposing an Autopeer endpoint implies that AS2 must also expose Autopeer endpoint for this backchannel to exist prior to any handshake being possible, hence being a bit ambiguous about it.
There was a problem hiding this comment.
OK, reading your comments, this makes sense to me, and I'm convinced :). I am happy going ahead with your more atomic suggestion--don't think we need to add a proposal object just yet, if this keeps it simpler overall.
You raise an interesting point about the /ports and /speaker endpoints for new PNIs. Seems sensible to focus on BGP for now, let's leave those for later (maybe we should discuss in a separate issue?)
I don't think we require both sides necessarily to have autopeer endpoints? Although I think we can strongly encourage it. You're right that the server wouldn't be able to send a request back to the client for additional sessions then, but I think we can leave it as an edge case.
|
|
||
|
|
There was a problem hiding this comment.
Please keep this space, it is required for formatting the built files
There was a problem hiding this comment.
You can check the built files here: https://github.com/bgp/draft-ietf-peering-api/actions/runs/7655174182
caguado
force-pushed
the
contrib/crlsa/refactor/restful
branch
from
9788673
to
27bf85f
Compare
Address the review feedback to reintroduce delete methods on session batches and clarify the expectations where receivers need additional sessions to be created.
caguado
force-pushed
the
contrib/crlsa/refactor/restful
branch
from
27bf85f
to
d4273e1
Compare
| * Below is based on OpenAPI specification: [https://github.com/bgp/autopeer/blob/main/api/openapi.yaml](https://github.com/bgp/autopeer/blob/main/api/openapi.yaml) | ||
| * `GET /sessions/{session_id}` | ||
| * Request parameters: | ||
| * session_id returned by the server on creation or through the session list operation. |
There was a problem hiding this comment.
nice, the session list operation would cover the case where it was configured outside the API.
| * The session referred by the specified session_id does not exist or is not visible to the caller | ||
|
|
||
| * Delete a session. | ||
| * Given a session ID, delete it which effectively triggers an depeering from the initiator. |
There was a problem hiding this comment.
an -> a, I think
(sorry for the nit)
|
Ack thanks. I did check the job output to verify I don't add regressions. Merging then. |
This commit adapts the structures and wording around the changes made to the reference OpenAPI spec from [1].
[1] bgp/autopeer#17