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

Common proposal to tackle subscription-based open issues. #185

Closed
bigludo7 opened this issue Apr 16, 2024 · 7 comments · Fixed by #189, #198 or #219
Closed

Common proposal to tackle subscription-based open issues. #185

bigludo7 opened this issue Apr 16, 2024 · 7 comments · Fixed by #189, #198 or #219
Labels
enhancement New feature or request

Comments

@bigludo7
Copy link
Collaborator

Problem description
We have several subscription-related issues for some time. (Issues check list 140 155 153 154 155 150 163 149 176 159)
In order to speed up their resolution and with CAMARA first meta-release in front us, it is critical to move forward on these issues.
This proposal has been build by @shilpa-padgaonkar @rartych @hdamker @PedroDiez @patrice-conil and myself.

Possible evolution

We have a meeting with to tackle long time open issues related to events & subscriptions and craft a common proposal.

Here is the sum-up of our proposal and then you have the detail below.

Comments are welcome in particular for blocker (as the issues are opened for several weeks). The plan is to craft a Design guideline proposal via a PR.

A TSC presentation of this proposal is also planned.

item1: subscription model
#140
#155

Proposal: align our subscription model with CloudsEvents one

All agreed that this is the 'last' time to do it and better to align with CloudEvents subscription model.

  • We need to have a standardized solution for subscription template (build from CloudEvents or from ourselves)
  • CloudsEvents is close to our draft propsoal but open some features that we might need in future like support multiple protocol or use filter.
  • The fact the CloudEvents model for subscription is draft is not an issue. We align with this model and assess fitire modification (if any)

item2: Manage lifecycle of subscriptions
issue #153

Proposal: Introduce a status attribute in the template as we have fair UCs that can leverage this (Retrieve expired subscriptions for monitoring, deactivate a subscription if an user revoked her/his consent, etc...). API subproject could decide to use it or not. A enumeration status list could be proposed

item3: Allow event consumers to subscribe to more than one event types with a single subscription
Issue #154

Proposal: Switch to an array in the model but at least for the first meta-release we enforce to have only event type per subscription. After this first meta release decision to handle several event types in one subscription request should be discussed at API sub projet level.

item4: Support filter option for subscriptions
#155

Proposal:
add filters and it's up to API Project to use it. We recommend to be very cautious as it add complexity so it should be keep for very relevant UC.
Add initialEvent in config as well to manage request to get event when current situation of a device corresponds to the subscriptions type. initialEvent is not in the template but pre-defined and it's up to each subproject to decide to use or not. We need to expliclty describe this in the guideline.

item5: Change the scope pattern for explicit subscriptions
#163

Proposal: We keep as it for now but discussion on the issue not close. In particular we're looking for @jlurien feedback as José crafted the scope part.
Linked to the Wild Card discussion - @shilpa-padgaonkar craft an issue: #184 (comment)

item6: Extension Authcontext for certain event types
#150

Proposal: Postponed - tag it as backlog (Put the label)

item7: Align Common Design Model for Cloud Event specversion field in APIs Design
#176

Proposal: As stated by Pedro better to use an enum
@PedroDiez will craft the PR then we will create issue when API projet did not follow the pattern

item8:x-correlator in CloudsEvent notification?
#164

PR to merged (#170)

Proposal: @shilpa-padgaonkar / @patrice-conil to take a look and review.

item9:CloudEvent type enum causes code generation problem
#159

Good news as it works for now so we can close this one and opened it again


DETAILLED ITEM DESCRIPTION & DISCUSSION

item1: subscription model

Some issues like #140 or #155 raised the point about alignement with cloudEvents subscription model.

We have a good example of the shift in issue 155.

We **absolutely ** need to make a decision on this before meta release 1

Change:

  • sink instead of webhook
  • add protocol and protocolSetting - we enforce protocol to HTTP
  • add config structure. This structure will gather existing subscriptionDetail, subscriptionExpireTime, subscriptionMaxEvents, startsAt and expiresAt + we can add initialEvent as suggested in Add initialEvent management in subscription #140
  • add filters but not manage this as of now (see 155)

In order to discuss on an existing case, if we try to realign our subscription model to CloudsEvent one we are going from:

{
  "webhook": {
    "notificationUrl": "https://application-server.com",
    "notificationAuthToken": "c8974e592c2fa383d4a3960714"
  },
  "subscriptionDetail": {
    "device": {
      "phoneNumber": "123456789"
      }
    },
    "area": {
       "areaType": "CIRCLE",
      "center": {
        "latitude": 50.735851,
        "longitude": 7.10066
    },
    "type": "org.camaraproject.geofencing.v0.area-entered"
  },
  "subscriptionExpireTime": "2024-06-08T09:42:23.807Z",
  "subscriptionMaxEvents ": "1",
  "initalEvent ": true,
  "subscriptionId": "3fa85f64-5717-4562-b3fc-2c963f66afa6",
  "startsAt": "2024-03-08T09:42:23.807Z",
  "expiresAt": "2024-06-08T09:42:23.807Z"
}
}

to

{
  "id": "3fa85f64-5717-4562-b3fc-2c963f66afa6", 
  "protocol": "HTTP",   **Only HTTP
  "protocolSettings": {},
  "sink": "https://application-server.com",
  "sinkCredential": {  **To be studied
    "credentialType": "OauthToken"
    "credential": "c8974e592c2fa383d4a3960714"
  },
  "types": [  **depending on other discussion if 1 or *
    "org.camaraproject.geofencing.v0.area-entered"
 ], 
"config": { 
    "subscriptionDetail": {
      "device": {
        "phoneNumber": "123456789"
      },
    },
    "area": {
       "areaType": "CIRCLE",
      "center": {
        "latitude": 50.735851,
        "longitude": 7.10066
    },
    "type": "org.camaraproject.geofencing.v0.area-entered"
  },
    "subscriptionExpireTime": "2024-06-08T09:42:23.807Z",
    "subscriptionMaxEvents ": "1",
    "initalEvent ": true,
     "startsAt": "2024-03-08T09:42:23.807Z",
    "expiresAt": "2024-06-08T09:42:23.807Z"
} }

_Important impact on 4 APIs (device location, Device status, SIM Swa, Connectivity Insight)

Meeting discussion:
All agreed that this is the 'last' time to do it.

Orange: No issue till we do not have to add filter behavior.
DT: Better to switch to this new model now
Telefonica: Technically it is good to move forward + agreed to have a template

  • This is still WIP in CloudEvents so we have risk that it could change.
  • No use filter at beginning till we have a concrete UC for it.

Positive point for a change:

  • Have a standardized solution for subscription template (build from CE or from ourselves)
    • We might need in future to support multiple protocol, to use filter

Next steps:
Inform the TSC (next meeting) - Ping for Connectivity Insights (Shilpa will do)
Create an issue to drop objection for each subproject.

item2: Manage lifecycle of subscriptions

issue #153
No status in CloudEvents subscription model

Async creation as Monitoring of terminated subscription could leverage a status attribute

Is it enough to manage a state engine?

Orange: humm not sure we need it right now
DT: we can postpone
Telefonica: Does it relevant to show to the consumer expired/terminated subscription?

DT: how we manage if the user revokes the consent associated to the subscription?
Pedro: This is a status - the subscription should not be termianted but more 'on hold' mode

Proposal: Introduce a status attribute in the template as we have fair UC (expired, deactivated). Then subproject could decide to use it or not.

item3: Allow event consumers to subscribe to more than one event types with a single subscription

Issue #154

Latest proposal:

Do we need to be restrictive on this point? ... if a Telco is able to manage multiple event type in one request while another Telco is able to manage only one is there risk on API adoption ?
Proposal: change the eventType as an enum but Telco is free to indicate in the documentation that only one eventType should be provided if this telco has specific reason to offer granularity 1,1

Patrice: de facto is we move to the CloudEvents the attribute should be an array
Orange: Technically not an issue
DT: As we have close event type this is not an issue to have this multiple types
Telefonica: prefer to not have to support multiple events. As of now we have no buisness requirement. At least we must allow Telco to have a restriction to no manage multiple.

Proposal:
We can switch to an array but at least for the first meta release we enforce to have only event type per subscription. After this first meta release decision to handle several event type could discussed at sub projet level.

item4: Support filter option for subscriptions

#155
If we shift to cloudsEvent structure we open the possibility to use filters.

Filtershas the drawback to require a 'grammar' and server side to implement it.

As long as we do not have clear uc requiring it I will probably prefer to not use it (have filters but not use if)
For geofencing or device status probably a initialEvent attribute is more straighforward

Orange : add filters and it's up to API Project to use it. Add initialEvent in config as well.
DT: How to manage that a consumer does not want to get the termination subscription?
Telefonica: not use for filters as of now.
OK for the initialEvent. It has impact for the implementation.

OK for the initialEvent (in the config structure) but not in the template but pre-defined and it's up to each sub project to decide to use or not. We need to explicitly describe this in the guideline.

item5: Change the scope pattern for explicit subscriptions

#163
Shilpa crafted PR
Not far to a conclusion - need to be discussed with Pedro

Pedro: How we manage a request for all event type for a given APi in one call
Related to the discussion on the wild card scope
It was left hanging.
Nobody has documented.
We need a general issue for Wild card scope (Shilpa will create an issue in Commonalities & referes it in Identity & Consent)

Proposal: As of now we strictly follow the pattern for eventType
Have feedback from José will be helpful (Pedro will do a follow-up)

item6: Extension Authcontext for certain event types
#150

CloudsEvent spec specified that this This extension is purely informational and is not intended to secure CloudEvents.

Orange : I'm afraid it will bring more confusion thant solving problem - I prefer to not add this
Telefonica: same
DT: Same

To be postponed - tag it as backlog (Put the label)

item7: Align Common Design Model for Cloud Event specversion field in APIs Design

#176
It has been detected a misaligment regarding the design format used to reflect specversion field in API that manage notifications. Do we use an enum or a open string
Proposal: As stated by Pedro better to use an enum - Orange & DT agrees
Pedro will craft the PR
Then we will create issue when projet did not follow the pattern

item8:x-correlator in CloudsEvent notification?
#164

PR to merged (#170)

@shilpa-padgaonkar / @patrice-conil to take a look and review.

item9:CloudEvent type enum causes code generation problem
#159

As in camaraproject/DeviceStatus#107 reported using the CloudEvent type identifier as an enum e.g. org.camaraproject.device-status.v0.roaming-status causes problems in the code generation for Java and probably other languages too.

An enum can't contain either a dot or (in Java) lowercase letters.

We have a proposal on the table by Lyle Bertz that need to be assessed quickly.

Good news as it works for now so we can close this one and opened it again

Additional context
Presentation to be done in next TSC

@shilpa-padgaonkar
Copy link
Collaborator

@bigludo7 and @PedroDiez As we agreed today to have a common template, we probably need to also agree where this template yaml file should reside.

@bigludo7
Copy link
Collaborator Author

@shilpa-padgaonkar I tend t think here: https://github.com/camaraproject/Commonalities/tree/main/artifacts

I will craft a draft PR to propose this file.

@shilpa-padgaonkar
Copy link
Collaborator

@bigludo7 Sounds good to me. Maybe you could create a folder named camara-cloudevents under artifacts, and we use it to place both notifications and subscription yaml templates?

@PedroDiez
Copy link
Collaborator

@bigludo7, @shilpa-padgaonkar as part of that template I think it would be also good point to model the POST callback, to have a reference of the model for CAMARA cloudevents profile as well as the traversal case of "subscription-ends"

@PedroDiez
Copy link
Collaborator

Hi,

@bigludo7, @shilpa-padgaonkar within Issue 155 context

Proposal:
add filters and it's up to API Project to use it. We recommend to be very cautious as it add complexity so it should be keep for very relevant UC.

I understood we do not consider filters from the beginning as we use currently "subscriptionDetail" object as filter criteria, and this can generate complex rules to manage filtering

@bigludo7
Copy link
Collaborator Author

Hello @PedroDiez @shilpa-padgaonkar
Agreed Pedro but I noted that we keep the filters in the model but we recommend to not use till good UC for that as very complex.

If both You and Shilpa prefer to remove it from the model I'm fine.

bigludo7 added a commit to bigludo7/Commonalities that referenced this issue Apr 18, 2024
Create subscription-notification-template.yaml aligned with CloudEvents subscription model. 
Refer to camaraproject#185
@bigludo7
Copy link
Collaborator Author

bigludo7 commented Apr 18, 2024

@PedroDiez @shilpa-padgaonkar
We crafted with @patrice-conil the yaml.
We took the CloudEvent specification in input but we slightly updated it to follow CAMARA pattern regarding use of polymorphic pattern and class heritage.
We keep filters but open to removing it you prefer.

@PedroDiez We also described the callback part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants