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.

Sign up for GitHub

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

feat: propagate DG changes to all IMSIs #283

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

feat: propagate DG changes to all IMSIs #283

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5b6e40b
fix: update all subscribers belonging to the same DG when the DG is
dariofaccin Jan 13, 2025
cca215e
fix: update only already provisioned IMSIs
dariofaccin Jan 20, 2025
6ca96e2
fix: do not store provisioned IMSIs in memory
dariofaccin Jan 21, 2025
4e2bb9d
chore: add subscriber retrieval func
dariofaccin Jan 21, 2025
ca19748
chore: skip updating subscribers data during delete DG operation
dariofaccin Jan 21, 2025
4e8982e
fix: perform correct IMSI string check
dariofaccin Jan 22, 2025
ba37e35
chore: address review comments
dariofaccin Jan 23, 2025
91cd6d5
chore: address review comments
dariofaccin Jan 23, 2025
ddff02e
Merge branch 'main' into TELCO-1401-fix-editing-mbr
gab-arrobo Jan 23, 2025
bee58af
Merge branch 'main' into TELCO-1401-fix-editing-mbr
dariofaccin Jan 27, 2025
42a0eff
Merge branch 'main' into TELCO-1401-fix-editing-mbr
dariofaccin Feb 6, 2025
efcaa5e
Merge branch 'main' into TELCO-1401-fix-editing-mbr
dariofaccin Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion proto/server/clientEvtHandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@patriciareinoso patriciareinoso Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if authSubsData == nil {
client.clientLog.Infoln("SIM card details not found for IMSI ", imsi)
continue
Expand Down
64 changes: 47 additions & 17 deletions proto/server/configEvtHandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"encoding/json"
"fmt"
"os/exec"
"slices"
"strconv"
"strings"
"sync"
Expand Down Expand Up @@ -42,14 +43,9 @@ type Update5GSubscriberMsg struct {

var (
execCommand = exec.Command
imsiData map[string]*models.AuthenticationSubscription
rwLock sync.RWMutex
)

func init() {
imsiData = make(map[string]*models.AuthenticationSubscription)
}

func configHandler(configMsgChan chan *configmodels.ConfigMessage, configReceived chan bool) {
// Start Goroutine which will listens for subscriber config updates
// and update the mongoDB. Only for 5G
Expand All @@ -67,9 +63,6 @@ func configHandler(configMsgChan chan *configmodels.ConfigMessage, configReceive
if configMsg.MsgType == configmodels.Sub_data {
imsiVal := strings.ReplaceAll(configMsg.Imsi, "imsi-", "")
logger.ConfigLog.Infoln("received imsi from config channel:", imsiVal)
rwLock.Lock()
imsiData[imsiVal] = configMsg.AuthSubData
rwLock.Unlock()
logger.ConfigLog.Infof("received Imsi [%v] configuration from config channel", configMsg.Imsi)
handleSubscriberPost(configMsg)
if factory.WebUIConfig.Configuration.Mode5G {
Expand Down Expand Up @@ -355,9 +348,10 @@ func getAddedImsisList(group, prevGroup *configmodels.DeviceGroups) (aimsis []st
if group == nil {
return
}
provisionedSubscribers := getProvisionedSubscribers()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()).

for _, imsi := range group.Imsis {
if prevGroup == nil {
if imsiData[imsi] != nil {
if slices.Contains(provisionedSubscribers, imsi) {
aimsis = append(aimsis, imsi)
}
} else {
Expand Down Expand Up @@ -402,6 +396,36 @@ func getDeletedImsisList(group, prevGroup *configmodels.DeviceGroups) (dimsis []
return
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can inmmediately return here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can return an empty slice.

}
var provisionedSubscribers []string
for _, rawProvisionedSubscriber := range rawProvisionedSubscribers {
ueId, ok := rawProvisionedSubscriber["ueId"].(string)
if !ok {
logger.DbLog.Warnf("cannot retrieve ueId for subscriber: %v", rawProvisionedSubscriber)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

provisionedSubscribers = append(provisionedSubscribers, ueId)
}
return provisionedSubscribers
}

func getSubscriberAuthDataByUeId(ueId string) *models.AuthenticationSubscription {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can immediately return here

}
var subscriberAuthData models.AuthenticationSubscription
err := json.Unmarshal(configmodels.MapToByte(subscriberAuthDataInterface), &subscriberAuthData)
if err != nil {
logger.DbLog.Errorf("could not unmarshall subscriber %v", subscriberAuthDataInterface)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return &subscriberAuthData
}

func updateAmPolicyData(imsi string) {
// ampolicydata
var amPolicy models.AmPolicyData
Expand Down Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sVal, err := strconv.ParseUint(slice.SliceId.Sst, 10, 32)
if err != nil {
logger.DbLog.Errorf("could not parse SST %v", slice.SliceId.Sst)
Expand All @@ -616,14 +641,19 @@ func Config5GUpdateHandle(confChan chan *Update5GSubscriberMsg) {
Sst: int32(sVal),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

aimsis := getAddedImsisList(confData.Msg.DevGroup, confData.PrevDevGroup)
for _, imsi := range aimsis {
dnn := confData.Msg.DevGroup.IpDomainExpanded.Dnn
updateAmPolicyData(imsi)
updateSmPolicyData(snssai, dnn, imsi)
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)
/* skip delete case */
if confData.Msg.DevGroup != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please disregard my comment.

for _, imsi := range confData.Msg.DevGroup.Imsis {
/* update only if the imsi is provisioned */
if slices.Contains(provisionedSubscribers, "imsi-" + imsi) {
dnn := confData.Msg.DevGroup.IpDomainExpanded.Dnn
updateAmPolicyData(imsi)
updateSmPolicyData(snssai, dnn, imsi)
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)
}
}
}

dimsis := getDeletedImsisList(confData.Msg.DevGroup, confData.PrevDevGroup)
Expand Down