feat: propagate DG changes to all IMSIs #283

dariofaccin · 2025-01-14T14:39:08Z

This PR aims to fix the issue for which Access Mobility Data, Session Management Data, SMF Selection Data and Policy Data for subscribers are not refreshed when the Device Group is updated.

Before this PR, only subscribers that are added in the same API call of the Device Group update take the new information. This PR extends the refresh to all the subscribers currently belonging to the DG.

Current behaviour
A device group is created and its representation (only relevant part) in database is the following:

{
    [...]
    "imsis" : [ 
        "208930100007487"
    ],
    "ip-domain-expanded" : {
        "ue-dnn-qos" : {
            "dnn-mbr-uplink" : 20000000.0,
            "dnn-mbr-downlink" : 200000000.0,
            "bitrate-unit" : "bps",
            [...]
        },
        [...]
    }
}

The representation (only relevant part) of the subscriber associated to the device group is the following:

{
    [...]
    "subscribedUeAmbr" : {
        "uplink" : "20 Mbps",
        "downlink" : "200 Mbps"
    }
}

After updating the device group with new MBR data, the representation is the following:

{
    [...]
    "imsis" : [ 
        "208930100007487"
    ],
    "ip-domain-expanded" : {
        "ue-dnn-qos" : {
            "dnn-mbr-uplink" : 20000000.0,
            "dnn-mbr-downlink" : 300000000.0,
            "bitrate-unit" : "bps",
            [...]
        },
        [...]
    }
}

But subscriber data in all the collections is not updated:

{
    [...]
    "subscribedUeAmbr" : {
        "uplink" : "20 Mbps",
        "downlink" : "200 Mbps"
    }
}

The example is taken from the amData collection, but it applies also to smData and the others.

patriciareinoso · 2025-01-15T11:51:36Z

I created an issue to better understand the problem and the purpose of the PR. Please add the reference to it in the PR description. Would it be possible to capture a before and after of the DB content ? like this it would be clearer which fields we are talking about.

patriciareinoso · 2025-01-15T12:32:47Z

What do you mean by "only subscribers that are added in the same API call of the Device Group update take the new information" ? if the update api call does not include the list of subscribers it means that we are overwriting that list

patriciareinoso · 2025-01-15T14:04:54Z

What do you mean by "only subscribers that are added in the same API call of the Device Group update take the new information" ? if the update api call does not include the list of subscribers it means that we are overwriting that list

ok, looking at the code I think I understood. But I am worried about the case in which you add a DG and add a reference to a Subscriber in the imsis list but you haven't created the actual Subscriber yet, it will create entries in subscriptionData.provisionedData.amData, subscriptionData.provisionedData.smData, subscriptionData.provisionedData.smfSelectionSubscriptionData but not in subscriptionData.authenticationData.authenticationSubscription

patriciareinoso

Oh I thought I did not have permission to request changes. Please see the comments I added

updated Signed-off-by: Dario Faccin <[email protected]>

Signed-off-by: Dario Faccin <[email protected]>

patriciareinoso · 2025-01-20T10:02:25Z

proto/server/configEvtHandler.go

-					updateSmfSelectionProviosionedData(snssai, slice.SiteInfo.Plmn.Mcc, slice.SiteInfo.Plmn.Mnc, dnn, imsi)
+				for _, imsi := range confData.Msg.DevGroup.Imsis {
+					/* update only if the imsi is provisioned */
+					if imsiData[imsi] != nil {


if imsiData is kept in memory. What happens if we add Subscriber A, B , C. and they exist in the DevGroup.Imsis. Then the pod dies and then we update the MBR ?

This issue looks to me independent from this PR, as the same logic is already in place. I can try to fix it in this same PR or open a new one, I don't have a strong opinion.

to me it's part of this PR. the fix would not be completed if we do not handle this edge case

patriciareinoso

if I try to build I get the error:

proto/server/clientEvtHandler.go:978:21: undefined: imsiData

Signed-off-by: Dario Faccin <[email protected]>

patriciareinoso · 2025-01-21T15:33:29Z

proto/server/configEvtHandler.go

-					updateAmProvisionedData(snssai, confData.Msg.DevGroup.IpDomainExpanded.UeDnnQos, slice.SiteInfo.Plmn.Mcc, slice.SiteInfo.Plmn.Mnc, imsi)
-					updateSmProvisionedData(snssai, confData.Msg.DevGroup.IpDomainExpanded.UeDnnQos, slice.SiteInfo.Plmn.Mcc, slice.SiteInfo.Plmn.Mnc, dnn, imsi)
-					updateSmfSelectionProviosionedData(snssai, slice.SiteInfo.Plmn.Mcc, slice.SiteInfo.Plmn.Mnc, dnn, imsi)
+				for _, imsi := range confData.Msg.DevGroup.Imsis {


This method is shared between the DELETE and the POST operation. In the delete operation confData.Msg.DevGroup is nil, so the program crashes if we try to delete a DG.

patriciareinoso

These modifications change the initial behavior of the following workflow in the NMS:

Create a NS
DG is automatically created
Create a Subscriber

Before

subscriptionData.provisionedData.amData, subscriptionData.provisionedData.smData, subscriptionData.provisionedData.smfSelectionSubscriptionData are filled

Now

subscriptionData.provisionedData.amData, subscriptionData.provisionedData.smData, subscriptionData.provisionedData.smfSelectionSubscriptionData are not filled

Signed-off-by: Dario Faccin <[email protected]>

patriciareinoso · 2025-01-22T12:09:28Z

proto/server/configEvtHandler.go

@@ -607,6 +631,7 @@ func Config5GUpdateHandle(confChan chan *Update5GSubscriberMsg) {
 			/* is this devicegroup part of any existing slice */
 			slice := isDeviceGroupExistInSlice(confData)
 			if slice != nil {
+				provisionedSubscribers := getProvisionedSubscribers()


i would make this in line 646: after the if and before the loop. In case of delete operation we would unnecessarily retrieving the list of subscribers

patriciareinoso · 2025-01-22T12:09:56Z

proto/server/configEvtHandler.go

+	return provisionedSubscribers
+}
+
+func getSubscriberAuthDataByUeId(ueId string) *models.AuthenticationSubscription {


i would move this function to clientEvtHandler.go since it is only used there

In clientEvtHandler we are not istantiating the DBClient, therefore moving this function there will increase the imports and in general the complexity. In the client handler we are also already importing getAddedImsisList() func from configEvtHandler

patriciareinoso · 2025-01-22T12:18:08Z

proto/server/configEvtHandler.go

+func getProvisionedSubscribers() []string {
+	rawProvisionedSubscribers, errGetMany := dbadapter.AuthDBClient.RestfulAPIGetMany(authSubsDataColl, nil)
+	if errGetMany != nil {
+		logger.DbLog.Warnln(errGetMany)


we can inmmediately return here

Agree, if we continue rawProvisionedSubscribers might be uninitialized and return an nil pointer dereference in line 405.

We can return an empty slice.

patriciareinoso · 2025-01-22T12:18:25Z

proto/server/configEvtHandler.go

+	filter := bson.M{"ueId": ueId}
+	subscriberAuthDataInterface, errGetOne := dbadapter.AuthDBClient.RestfulAPIGetOne(authSubsDataColl, filter)
+	if errGetOne != nil {
+		logger.DbLog.Warnln(errGetOne)


we can immediately return here

patriciareinoso · 2025-01-22T12:20:58Z

proto/server/configEvtHandler.go

@@ -355,9 +348,10 @@ func getAddedImsisList(group, prevGroup *configmodels.DeviceGroups) (aimsis []st
 	if group == nil {
 		return
 	}
+	provisionedSubscribers := getProvisionedSubscribers()


i am concerned that here you are using the information in authSubsDataColl but that table is only filled up in factory.WebUIConfig.Configuration.Mode5G Do you know what happens in all the function in clientEvtHandler that use getAddedImsisList? are all of them used when factory.WebUIConfig.Configuration.Mode5G ?

In clientEvtHandler when 5GMode is disabled there are other functions (in this case, postConfigHss) that take care of the subscribers and these are using another functions (in this case, addedImsis() and deletedImsis()).

gatici · 2025-01-22T16:05:54Z

proto/server/configEvtHandler.go

+func getProvisionedSubscribers() []string {
+	rawProvisionedSubscribers, errGetMany := dbadapter.AuthDBClient.RestfulAPIGetMany(authSubsDataColl, nil)
+	if errGetMany != nil {
+		logger.DbLog.Warnln(errGetMany)


Agree, if we continue rawProvisionedSubscribers might be uninitialized and return an nil pointer dereference in line 405.

gatici · 2025-01-22T16:09:28Z

proto/server/configEvtHandler.go

+		if !ok {
+			logger.DbLog.Warnf("cannot retrieve ueId for subscriber: %v", rawProvisionedSubscriber)
+		}


Suggested change

if !ok {

logger.DbLog.Warnf("cannot retrieve ueId for subscriber: %v", rawProvisionedSubscriber)

}

if !ok {

logger.DbLog.Warnf("cannot retrieve ueId for subscriber: %v", rawProvisionedSubscriber)

continue

}

If we can not get a proper ueId, we can skip it.

gatici · 2025-01-22T16:13:59Z

proto/server/configEvtHandler.go

+func getProvisionedSubscribers() []string {
+	rawProvisionedSubscribers, errGetMany := dbadapter.AuthDBClient.RestfulAPIGetMany(authSubsDataColl, nil)
+	if errGetMany != nil {
+		logger.DbLog.Warnln(errGetMany)


We can return an empty slice.

gatici · 2025-01-22T16:25:38Z

proto/server/configEvtHandler.go

+	if err != nil {
+		logger.DbLog.Errorf("could not unmarshall subscriber %v", subscriberAuthDataInterface)
+	}


Suggested change

if err != nil {

logger.DbLog.Errorf("could not unmarshall subscriber %v", subscriberAuthDataInterface)

}

if err != nil {

logger.DbLog.Errorf("could not unmarshall subscriber %v", subscriberAuthDataInterface)

return nil

}

We can return here.

I don't see the point of adding return nil here: in line 427 we are returning the pointer (return &subscriberAuthData) which will already be nil if the unmarshalling failed.

In the project L978 this function is called like below:

authSubsData := getSubscriberAuthDataByUeId("imsi-" + imsi) if authSubsData == nil { client.clientLog.Infoln("SIM card details not found for IMSI ", imsi) continue }

They do a nil check to skip or continue to execution.
If json.Unmarshal fails, &subscriberAuthData pointer will keep the location of an empty AuthenticationSubscription struct looking like:
{Opc:{...} PermanentKey:{...} SequenceNumber: ""}
And it will not be nil. Execution continue and In L983 config.Opc = authSubsData.Opc.OpcValue will cause a panic as OpcValue is not reachable.
Hence, instead of returning a pointer of empty struct, we can return nil in L428.

gatici · 2025-01-22T16:34:03Z

proto/server/configEvtHandler.go

@@ -616,14 +641,19 @@ func Config5GUpdateHandle(confChan chan *Update5GSubscriberMsg) {
 					Sst: int32(sVal),


We shoud return after line 637. If strconv.ParseUint fails, we can not calculate snssai properly.

gatici · 2025-01-22T16:38:45Z

proto/server/configEvtHandler.go

-					updateSmProvisionedData(snssai, confData.Msg.DevGroup.IpDomainExpanded.UeDnnQos, slice.SiteInfo.Plmn.Mcc, slice.SiteInfo.Plmn.Mnc, dnn, imsi)
-					updateSmfSelectionProviosionedData(snssai, slice.SiteInfo.Plmn.Mcc, slice.SiteInfo.Plmn.Mnc, dnn, imsi)
+				/* skip delete case */
+				if confData.Msg.DevGroup != nil {


We can move the first if clause outside of this block to increase readability.

if confData.Msg.DevGroup == nil {
logger.DbLog.Warn("DevGroup is nil in confData.Msg")
return
}

If DevGroup is nil we want to remove the subscribers data from the collections, but not update them. I don't fully understand the comment however.

Let's talk about it. I will ping you.

Please disregard my comment.

Signed-off-by: Dario Faccin <[email protected]>

gab-arrobo · 2025-01-23T23:33:20Z

@dariofaccin, I guess you have tested these changes, correct? I am asking this because I was running a test and I do not see these changes being used, or am I missing something?

dariofaccin · 2025-01-27T08:21:45Z

@gab-arrobo, these changes are used to update the subscriber data of a device group whenever the device group is updated. To see the changes the flow is the following:

create a device group (and network slice if needed)
create a subscriber associated to such device group
edit the device group
changes are reflected in the subscriber data (collections and data returned by GET /subscribers/{imsi})

Let me know, thanks

patriciareinoso · 2025-01-28T15:34:58Z

proto/server/clientEvtHandler.go

@@ -975,7 +975,7 @@ func postConfigHss(client *clientNF, lastDevGroup *configmodels.DeviceGroups, la
 				}
 				config.StartImsi = uint64(num)
 				config.EndImsi = uint64(num)
-				authSubsData := imsiData[imsi]
+				authSubsData := getSubscriberAuthDataByUeId("imsi-" + imsi)


this piece of code is executed by the 4G configuration , in which case the authSubsDataColl used in getSubscriberAuthDataByUeId will not be filled and authSubsData will be nil

And it's not 100% clear to me if getAddedImsisList if only used by 5G code since the code is shared between both configurations so I wouldn't touch it.

please do not merge these changes until thi is fixed

thakurajayL · 2025-02-07T22:56:10Z

Is this ready for review? I see its in draft state.

patriciareinoso mentioned this pull request Jan 15, 2025

Edit MBR upstream and downstream in Device Group does not propagate the values to subscribers in DB #284

Open

patriciareinoso suggested changes Jan 16, 2025

View reviewed changes

dariofaccin added 2 commits January 20, 2025 09:47

fix: update all subscribers belonging to the same DG when the DG is

5b6e40b

updated Signed-off-by: Dario Faccin <[email protected]>

fix: update only already provisioned IMSIs

cca215e

Signed-off-by: Dario Faccin <[email protected]>

dariofaccin force-pushed the TELCO-1401-fix-editing-mbr branch from 240599b to cca215e Compare January 20, 2025 08:57

dariofaccin requested a review from patriciareinoso January 20, 2025 09:57

patriciareinoso suggested changes Jan 20, 2025

View reviewed changes

dariofaccin requested a review from patriciareinoso January 21, 2025 10:53

patriciareinoso suggested changes Jan 21, 2025

View reviewed changes

dariofaccin added 2 commits January 21, 2025 14:33

fix: do not store provisioned IMSIs in memory

6ca96e2

Signed-off-by: Dario Faccin <[email protected]>

chore: add subscriber retrieval func

4e2bb9d

Signed-off-by: Dario Faccin <[email protected]>

dariofaccin force-pushed the TELCO-1401-fix-editing-mbr branch from 94b81a1 to 4e2bb9d Compare January 21, 2025 13:33

dariofaccin requested a review from patriciareinoso January 21, 2025 14:32

patriciareinoso suggested changes Jan 21, 2025

View reviewed changes

dariofaccin added 2 commits January 21, 2025 17:13

chore: skip updating subscribers data during delete DG operation

ca19748

Signed-off-by: Dario Faccin <[email protected]>

fix: perform correct IMSI string check

4e8982e

Signed-off-by: Dario Faccin <[email protected]>

patriciareinoso suggested changes Jan 22, 2025

View reviewed changes

gatici suggested changes Jan 22, 2025

View reviewed changes

chore: address review comments

ba37e35

Signed-off-by: Dario Faccin <[email protected]>

dariofaccin requested review from gatici and patriciareinoso January 23, 2025 08:49

chore: address review comments

91cd6d5

Signed-off-by: Dario Faccin <[email protected]>

gatici approved these changes Jan 23, 2025

View reviewed changes

patriciareinoso approved these changes Jan 23, 2025

View reviewed changes

Merge branch 'main' into TELCO-1401-fix-editing-mbr

ddff02e

Merge branch 'main' into TELCO-1401-fix-editing-mbr

bee58af

patriciareinoso suggested changes Jan 28, 2025

View reviewed changes

dariofaccin marked this pull request as draft January 29, 2025 14:05

Merge branch 'main' into TELCO-1401-fix-editing-mbr

42a0eff

Merge branch 'main' into TELCO-1401-fix-editing-mbr

efcaa5e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: propagate DG changes to all IMSIs #283

feat: propagate DG changes to all IMSIs #283

dariofaccin commented Jan 14, 2025 •

edited

Loading

patriciareinoso commented Jan 15, 2025

patriciareinoso commented Jan 15, 2025

patriciareinoso commented Jan 15, 2025 •

edited

Loading

patriciareinoso left a comment

patriciareinoso Jan 20, 2025 •

edited

Loading

dariofaccin Jan 20, 2025

patriciareinoso Jan 20, 2025

patriciareinoso left a comment

patriciareinoso Jan 21, 2025

patriciareinoso left a comment •

edited

Loading

patriciareinoso Jan 22, 2025

patriciareinoso Jan 22, 2025

dariofaccin Jan 22, 2025

patriciareinoso Jan 22, 2025

gatici Jan 22, 2025

gatici Jan 22, 2025

patriciareinoso Jan 22, 2025

patriciareinoso Jan 22, 2025

dariofaccin Jan 22, 2025

gatici Jan 22, 2025

gatici Jan 22, 2025

gatici Jan 22, 2025

gatici Jan 22, 2025

dariofaccin Jan 23, 2025

gatici Jan 23, 2025

gatici Jan 22, 2025

gatici Jan 22, 2025

dariofaccin Jan 23, 2025

gatici Jan 23, 2025

gatici Jan 23, 2025

gab-arrobo commented Jan 23, 2025

dariofaccin commented Jan 27, 2025

patriciareinoso Jan 28, 2025 •

edited

Loading

thakurajayL commented Feb 7, 2025

		@@ -616,14 +641,19 @@ func Config5GUpdateHandle(confChan chan *Update5GSubscriberMsg) {
		Sst: int32(sVal),

feat: propagate DG changes to all IMSIs #283

Are you sure you want to change the base?

feat: propagate DG changes to all IMSIs #283

Conversation

dariofaccin commented Jan 14, 2025 • edited Loading

patriciareinoso commented Jan 15, 2025

patriciareinoso commented Jan 15, 2025

patriciareinoso commented Jan 15, 2025 • edited Loading

patriciareinoso left a comment

Choose a reason for hiding this comment

patriciareinoso Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriciareinoso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriciareinoso left a comment • edited Loading

Choose a reason for hiding this comment

Before

Now

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gab-arrobo commented Jan 23, 2025

dariofaccin commented Jan 27, 2025

patriciareinoso Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

thakurajayL commented Feb 7, 2025

dariofaccin commented Jan 14, 2025 •

edited

Loading

patriciareinoso commented Jan 15, 2025 •

edited

Loading

patriciareinoso Jan 20, 2025 •

edited

Loading

patriciareinoso left a comment •

edited

Loading

patriciareinoso Jan 28, 2025 •

edited

Loading