camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family
https://wiki.camaraproject.org/x/zwOeAQ
Apache License 2.0
42 stars 59 forks source link

Include support for /PATCH sessions #47

Closed maheshc01 closed 10 months ago

maheshc01 commented 2 years ago

Currently the API spec does not have support for making any updates once the qos session is created. Rather than assuming the data passed during session creation will always be static for the duration of the session, we need to consider use cases where either the UE details or the Application server details could change due to various factors and provide support for developers to make the updates via a PUT /sessions API.

GarethWGSMA commented 1 year ago

Would it not be something for commonalities to decide if we use "Put" or "PATCH" across the board. If possible it would be nice if we could have some consistency for developers, unless there is a good reason not to.

emil-cheung commented 1 year ago

Provide some additional information:

Considering the UE IP address cannot be modified, PATCH operation in CAMARA QoD may be more clearer, for example, we could create a 'PatchSession' datatype excluding the UE ID object.

emil-cheung commented 1 year ago

Let's discuss the changeable attributes firstly. Please check the following table:

QoD Session Attribute Modifiable in 3GPP Additional Comment Use Case in CAMARA
device No UE IP address cannot be changed in 3GPP AsSessionWithQos N/A
duration N/A The attribute is CAMARA specific Prolong the duration of an exisiting QoD session
notificationUrl N/A Even though it is not mapped to 3GPP AsSessionWithQos notificationDestination, we could refer to the 3GPP AsSessionWithQos that notificationDestionation is modifiable. API invoker updates the notification endpoint, e.g., when upgrading/migrating the application server
notificationAuthToken N/A The attribute is CAMARA specific Update the token due to security purpose
applicationServer Yes 3GPP AsSessionWithQos allows to change the flowInfo (5-tuple) except for the UE IP address UE fails over to other application server
devicePorts Yes 3GPP AsSessionWithQos allows to change the flowInfo (5-tuple) except for the UE IP address Need further discussion in the community
qosProfile Yes 3GPP AsSessionWithQos allows to change qosReference Need further discussion in the community
hdamker commented 1 year ago

Changed title from /PUT to /PATCH session as agreed.

As agreed we will add the support of a /PATCH session operation, with a minimum set of parameters which can be changed during the session. Criteria are either technical necessity (e.g. updating notificationAuthToken for security purposes) or developer demand (e.g. ability to change duration of an existing session). Further parameter to be discussed.

eric-murray commented 1 year ago

My view is that we should be able to patch the following parameters:

and consequently that we should not be able to patch applicationServer or notificationUrl.

If duration is patched , this should be the new session duration from the time of patching, not from the original session creation time.

My concern with allowing patching of the application server address(es) is that this complicates the business logic of the API. An API consumer might be granted enhanced QoS to a specific address, say 8.8.8.8, at a low cost because not much traffic is expected for that flow, and then they go and patch it to 0.0.0.0/0. If backup application servers need to be specified, that should be specified on session creation. The applicationServer property already supports subnets, but maybe that needs to be extended to allow subnet lists.

I don't see a reason to allow updating of notificationUrl, and am also concerned about potential implementation complexity if this was allowed.

emil-cheung commented 1 year ago

My view is that we should be able to patch the following parameters:

  • duration
  • devicePorts
  • applicationServerPorts
  • qosProfile
  • notificationAuthToken

and consequently that we should not be able to patch applicationServer or notificationUrl.

If duration is patched , this should be the new session duration from the time of patching, not from the original session creation time.

My concern with allowing patching of the application server address(es) is that this complicates the business logic of the API. An API consumer might be granted enhanced QoS to a specific address, say 8.8.8.8, at a low cost because not much traffic is expected for that flow, and then they go and patch it to 0.0.0.0/0. If backup application servers need to be specified, that should be specified on session creation. The applicationServer property already supports subnets, but maybe that needs to be extended to allow subnet lists.

I don't see a reason to allow updating of notificationUrl, and am also concerned about potential implementation complexity if this was allowed.

@eric-murray Thanks for the comments. I have 2 further questions:

  1. Could you elaborate the use cases of changing device ports and application ports?
  2. In terms of application server address, when creating a session, will Camara layer (or specific implementation) authorize it based on the traffic? 3GPP will not check it as it is just part of the packet filters.
emil-cheung commented 1 year ago

@eric-murray

If QoS profile is allowed to change, we may need to think about how to reflect the QoS status. Let's investigate the following case:

(1) Create QoD session with QoS profile 'QOS_M'. (2) The network accepts the boost (NEF notifying SUCCESSFUL_RESOURCES_ALLOCATION event) and 'QOS_M' is AVAILABLE. (3) Patch the QoD session with QoS profile 'QOS_L'. (4a) If the network accepts the boost (NEF notifying SUCCESSFUL_RESOURCES_ALLOCATION event), then 'QOS_L' is AVAILABLE'. (4b) If the network rejects the boost (NEF notifying FAILED_RESOURCES_ALLOCATION event), the 'QOS_M' shall remain, and 'QOS_M' is AVAILABLE.

My question to the community, Shall we keep the current QoS status design which only reflects the available QoS profile? Or we enhance the current QoS status design to reflect more information, such as the changed QoS profile is rejected?

hdamker commented 1 year ago

I'm seeing clear use cases for patching

I don't see the use cases for devicePorts and applicationServerPorts but open for input.

Regarding qosProfile there might be use cases, but I see also a lot of complexity in implementation including proper notivications and understandable charging if different profiles have different prices.

My proposal would be to start with the above minimal set of durationand notificationAuthToken, and consider all other parameters as additional enhancements which need justification of the implementation effort and complexity in product design (assuming that we can add further parameters without breaking the backward compatibility).

jlurien commented 1 year ago

I agree to start with a minimal approach and wait for specific requirements for other use cases. To me the most evident is duration in order to extend and possibly reduce (but not terminate) an ongoing session. But this has some implications on the API design:

Possible logic:

Another approach would be to provide specific operations to extend o reduce session duration, for example:

POST /sessions/{session_id}/extend { durationExtension: extended_duration } -> duration modified to original_duration + extended_duration

This is beyond adding regular PATCH support and would require case by case operations.

Regarding modification of notifcationAuthToken, taking into account that this API creates adhoc notifications for relatively short sessions, it may not be that necessary. I think that for a subscription model it makes more sense.

RandyLevensalor commented 1 year ago

For security could we clarify that the patch be limited to authorized users for that session and not just anyone with that sessionID. This assumes some backend business logic, but I think that is expected across the API.

emil-cheung commented 1 year ago

I agree to start with a minimal approach and wait for specific requirements for other use cases. To me the most evident is duration in order to extend and possibly reduce (but not terminate) an ongoing session. But this has some implications on the API design:

  • duration is a property of the Session object, It can be retrieved at any time with GET /sessions/{sessionId} and it is expected that the value reflects the whole duration of a session, from the start.
  • In a CRUD model, a PATCH on an object only modifies specific attributes to the new values, so PATCH /sessions/{sessionId} { "duration": new_duration }, is expected to set duration to new_duration, and this new value would be returned by GET.

Possible logic:

  • If new_duration > original_duration -> extend session (if session is still alive)
  • If elapsed_duration < new_duration < original_duration -> reduce session
  • If new_duration < elapsed_duration -> terminate session or error

Another approach would be to provide specific operations to extend o reduce session duration, for example:

POST /sessions/{session_id}/extend { durationExtension: extended_duration } -> duration modified to original_duration + extended_duration

This is beyond adding regular PATCH support and would require case by case operations.

Regarding modification of notifcationAuthToken, taking into account that this API creates adhoc notifications for relatively short sessions, it may not be that necessary. I think that for a subscription model it makes more sense.

@jlurien Agree with the duration update logic. For the third condition:

In terms of using PATCH or introducing a new operation for extending duration, I support using PATCH to make it simple.

eric-murray commented 1 year ago

Having read the discussion above and considered all the points, my view now is that patching is not the best way to allow session parameter updates, the reasons being:

So my preference is to have dedicated endpoints for this (e.g. /sessions/{session_id}/extend and /sessions/{session_id}/updateNotificationAuthToken). However, if the consensus is that PATCH is the expected way of updating these two session parameters, then that would work, but patching duration would need to be well documented.

Expanding on my second bullet point, and to answer some questions above, I see the (admittedly weak) rationale to allow the other parameters I identified for patching as follows:

devicePorts In fact, I don't think this parameter is required at all. Why not just use a wildcard for device source ports? I suggested this when updating the API specification for PR #139, but this comment suggested that it might be required to explicitly specify device source ports for some use cases, so the parameter remains. It may be that we return to the discussion on whether devicePorts is needed at all, but as long as devicePorts can be specified, then it can be specified incorrectly or otherwise need to be updated. For example:

But these are all weak reasons. It is probably best to force the API consumer to delete and re-create sessions with configuration errors or create additional sessions rather than allow existing sessions to be patched in this way.

applicationServerPorts The use case here would be re-direction. Maybe a session is set-up for port 443, and the server then re-directs to 8443. Should the session be allowed to be patched to fix this, or should the API consumer be forced to delete and re-create the session given their new understanding of how the application server works? Probably the latter.

The only common port re-direction I'm aware of is the implicit 80 -> 443 when doing an http -> https re-direction, but there may be other cases where this is used. But unlikely to be enough to justify allowing such patching.

qosProfile Patching the selected QoS profile has the strongest rationale of the three additional parameters. If the enhanced QoS that was selected for a given flow is not "good enough", then how to change that "seamlessly" without having to delete the session (which will revert the flow to the default QoS) and create a new one?

But, as @emil-cheung pointed out, using PATCH for this creates its own problems as it cannot be enabled synchronously, and a "successful" PATCH might actually be followed by a reversion to the original QoS profile, or even a session termination. So I think it would be better to consider using a separate endpoint to change 'qosProfile` which would make it clearer that acknowledgement of the request to change the QoS profile is not the same as implementing it.

RandyLevensalor commented 1 year ago

Since we seem to be having issues are how to manage duration, what about deprecating during and instead transitioning to a termination date-time following the date-time OpenAPI format. This way, it an absolute value that can be easily changed.

If we are do end up allowing PATCH or another mechanism to update a session, should we also add an optional GET for the history /sessions/{session_id}/logs

Just a thought. This will break the APIs, though it should be fairly easy to add the code. I got this from ChatGPT in less than a minute.

from datetime import datetime, timedelta

def rfc3339_date_time(seconds_ahead):
    current_time_utc = datetime.utcnow()
    future_time_utc = current_time_utc + timedelta(seconds=seconds_ahead)
    return future_time_utc.isoformat('T') + 'Z'

# Example usage:
seconds_ahead = 3600
print(rfc3339_date_time(seconds_ahead))  # Will print the current date and time plus one hour, in RFC 3339 format
chrishowell commented 1 year ago

👋🏻 I don't think patching a duration is good idea.. it's just 'not obvious' what the behaviour would be. But having the ability to extend a session would be very useful - think a call going on longer than expected; I don't see the need to be able to reduce the session duration, that could easily be achieved using cancellation. So I think POST /sessions/{session_id}/extend {seconds} would be most ideal.

Being able to update the authToken is useful for security reasons, though in practice I guess it'll never be used...

Updating the profile would definitely be useful, doing this via a POST /sessions/{session_id}/profile {profile} would be easiest to to allow for an async flow (e.g. 202 ACCEPTED, and a callback to say whether the change succeeded/failed)

hdamker commented 10 months ago

MOM-2023-12-01: