feat: propagate Subscriber deletion to Device Groups #299

dariofaccin · 2025-02-04T13:00:31Z

Issue

When we DELETE a subscriber, information is removed from subscriptionData.provisionedData.amData and subscriptionData.authenticationData.authenticationSubscription. However, if a device group contains a reference to this subscriber in imsis list, the reference is not removed.

Description

This PR propagates the DELETE subscriber operation to all the device groups that contains the deletedsubscriber. This way, the removal of the subscriber is done in a single operation, no need to manually update the device groups to remove the dangling reference anymore.
This PR improves the response when the list of subscribers is empty: now it returns an empty list ([]) instead of null.
This PR also improves the response in case of subscriber deletion failure: now it returns 500 Internal Server Error.

How the code works

Webconsole behavior on Subscriber POST before this PR

When a subscriber is created (POST), it creates entries in:

subscriptionData.authenticationData.authenticationSubscription
subscriptionData.provisionedData.amData

Webconsole behavior on Subscriber DELETE before this PR

When a subscriber is deleted (DELETE) , it removes entries for each subscriber in:

subscriptionData.authenticationData.authenticationSubscription
subscriptionData.provisionedData.amData

This PR

retrieves all the device groups where the subscriber to be deleted is present
creates a new (in memory) configuration for all the device groups on previous point, where the subscriber is removed from imsis
sends a message in the config channel for the subscriber removal
sends a message in the config channel for the device groups update

How to reproduce

Create a subscriber, a device group containing such subscriber and a network slice containing such device group
Check the DB entries for that device group in webconsoleData.snapshots.devGroupData (imsis list)
Delete the subscriber

What you should see

The subscriber reference is removed from the device group(s) imsis list
The subscriber information is removed from subscriptionData.authenticationData.authenticationSubscription
The subscriber information with the PLMN belonging to the modified device groups is removed from:
subscriptionData.provisionedData.amData
subscriptionData.provisionedData.smData
subscriptionData.provisionedData.smfSelectionSubscriptionData
policyData.ues.amData
policyData.ues.smData

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

patriciareinoso · 2025-02-06T09:56:53Z

configapi/api_sub_config_test.go

+		{
+			name:      "Create a new subscriber success",
+			route:     "/api/subscriber/imsi-208930100007487",
+			inputData: `{"UeId":"208930100007487", "plmnId":"12345", "opc":"981d464c7c52eb6e5036234984ad0bcf","key":"5122250214c33e723a5dd523fc145fc0", "sequenceNumber":"16f3b3f70fc2"}`,


Suggested change

inputData: `{"UeId":"208930100007487", "plmnId":"12345", "opc":"981d464c7c52eb6e5036234984ad0bcf","key":"5122250214c33e723a5dd523fc145fc0", "sequenceNumber":"16f3b3f70fc2"}`,

inputData: `{"plmnID":"12345", "opc":"981d464c7c52eb6e5036234984ad0bcf","key":"5122250214c33e723a5dd523fc145fc0", "sequenceNumber":"16f3b3f70fc2"}`,

patriciareinoso · 2025-02-06T09:59:29Z

configapi/api_sub_config_test.go

+		expectedCode    int
+		expectedBody    string


Suggested change

expectedCode int

expectedBody string

patriciareinoso · 2025-02-06T10:06:21Z

configapi/api_sub_config_test.go

+				MsgMethod: configmodels.Post_op,
+				AuthSubData: &models.AuthenticationSubscription{
+					AuthenticationManagementField: "8000",
+					AuthenticationMethod:          "5G_AKA", // "5G_AKA", "EAP_AKA_PRIME"


Suggested change

AuthenticationMethod: "5G_AKA", // "5G_AKA", "EAP_AKA_PRIME"

AuthenticationMethod: "5G_AKA",

patriciareinoso · 2025-02-06T10:06:57Z

configapi/api_sub_config_test.go

+						Op: &models.Op{
+							EncryptionAlgorithm: 0,
+							EncryptionKey:       0,
+							OpValue:             "", // Required


Replace OpValue with real expected value and remove comment

patriciareinoso · 2025-02-06T10:07:32Z

configapi/api_sub_config_test.go

+					Opc: &models.Opc{
+						EncryptionAlgorithm: 0,
+						EncryptionKey:       0,
+						// OpcValue:            "8e27b6af0e692e750f32667a3b14605d", // Required


replace OpcValue with real expected value and remove comment

patriciareinoso · 2025-02-06T10:18:00Z

configapi/api_sub_config_test.go

+				if tc.expectedMessage.AuthSubData != nil {
+					if msg.AuthSubData == nil {
+						t.Errorf("expected AuthSubData %+v, but got nil", tc.expectedMessage.AuthSubData)
+					}
+					if tc.expectedMessage.Imsi != msg.Imsi {
+						t.Errorf("expected IMSI %+v, but got %+v", tc.expectedMessage.Imsi, msg.Imsi)
+					}
+				}


Suggested change

if tc.expectedMessage.AuthSubData != nil {

if msg.AuthSubData == nil {

t.Errorf("expected AuthSubData %+v, but got nil", tc.expectedMessage.AuthSubData)

}

if tc.expectedMessage.Imsi != msg.Imsi {

t.Errorf("expected IMSI %+v, but got %+v", tc.expectedMessage.Imsi, msg.Imsi)

}

}

if tc.expectedMessage.Imsi != msg.Imsi {

t.Errorf("expected IMSI %+v, but got %+v", tc.expectedMessage.Imsi, msg.Imsi)

}

otherwise you are not really checking the imsi

you can also add a check to verify that the AuthSubData in the received message is nil. Or maybe use reflect.DeepEqual to check the whole message in one step instead of checking each field

patriciareinoso · 2025-02-06T10:20:27Z

configapi/api_sub_config_test.go

+	}
+}
+
+func TestSubscriberDeleteHandlers(t *testing.T) {


The test in which a Subscriber belongs to a DG is missing

configapi/api_sub_config.go

+	rawDeviceGroups, err := dbadapter.CommonDBClient.RestfulAPIGetMany(devGroupDataColl, filterByImsi)
+	if err != nil {
+		logger.WebUILog.Errorf("failed to fetch device groups: %w", err)
+		return


patriciareinoso · 2025-02-06T10:30:41Z

configapi/api_sub_config.go

+		var deviceGroup configmodels.DeviceGroups
+		if err = json.Unmarshal(configmodels.MapToByte(rawDeviceGroup), &deviceGroup); err != nil {
+			logger.WebUILog.Errorf("error unmarshaling device group: %v", err)
+			return


Suggested change

return

continue

configapi/api_sub_config.go

+		}
+		deviceGroupUpdateMessages = append(deviceGroupUpdateMessages, &deviceGroupUpdateMessage)
+	}
+	for _, msg := range deviceGroupUpdateMessages {


patriciareinoso · 2025-02-06T10:36:47Z

Please add to the description the fact that you are changing the behavior of the getter to return [ ] instead of null

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

patriciareinoso · 2025-02-10T16:06:44Z

configapi/api_sub_config_test.go

+		{
+			name:      "Create a new subscriber success",
+			route:     "/api/subscriber/imsi-208930100007487",
+			inputData: `{"plmnId":"12345", "opc":"8e27b6af0e692e750f32667a3b14605d","key":"8baf473f2f8fd09487cccbd7097c6862", "sequenceNumber":"16f3b3f70fc2"}`,


Suggested change

inputData: `{"plmnId":"12345", "opc":"8e27b6af0e692e750f32667a3b14605d","key":"8baf473f2f8fd09487cccbd7097c6862", "sequenceNumber":"16f3b3f70fc2"}`,

inputData: `{"plmnID":"12345", "opc":"8e27b6af0e692e750f32667a3b14605d","key":"8baf473f2f8fd09487cccbd7097c6862", "sequenceNumber":"16f3b3f70fc2"}`,

the json representation is plmnID. This does not affect the test but if in the future if someone wants to do something with the plmnID, this subtlety can be a struggling point.

patriciareinoso · 2025-02-10T16:12:23Z

configapi/api_sub_config_test.go

+				if tc.expectedMessage.AuthSubData != nil {
+					if msg.AuthSubData == nil {
+						t.Errorf("expected AuthSubData %+v, but got nil", tc.expectedMessage.AuthSubData)
+					}
+					if tc.expectedMessage.Imsi != msg.Imsi {
+						t.Errorf("expected IMSI %+v, but got %+v", tc.expectedMessage.Imsi, msg.Imsi)
+					}


this part of code is never executed because tc.expectedMessage.AuthSubData != nil condition is never true.

it's not recommended to have complicated logic in the checks.
We need to test that in the first tc only one message is sent, and in the second tc we need to check that 2 messages are sent, if the logic becomes complicated it's better to separate en 2 tests.
Complicated logic in tests is error prone, and hard to read

patriciareinoso · 2025-02-10T16:17:51Z

configapi/api_sub_config_test.go

+}
+
+func deviceGroupWithImsis(name string, imsis []string) configmodels.DeviceGroups {
+	traffic_class := configmodels.TrafficClassInfo{


Suggested change

traffic_class := configmodels.TrafficClassInfo{

trafficClass := configmodels.TrafficClassInfo{

patriciareinoso · 2025-02-10T16:18:20Z

configapi/api_sub_config_test.go

+		DnnMbrUplink:   10000000,
+		DnnMbrDownlink: 10000000,
+		BitrateUnit:    "kbps",
+		TrafficClass:   &traffic_class,


Suggested change

TrafficClass: &traffic_class,

TrafficClass: &trafficClass,

patriciareinoso · 2025-02-10T16:18:36Z

configapi/api_sub_config_test.go

+		BitrateUnit:    "kbps",
+		TrafficClass:   &traffic_class,
+	}
+	ipdomain := configmodels.DeviceGroupsIpDomainExpanded{


Suggested change

ipdomain := configmodels.DeviceGroupsIpDomainExpanded{

ipDomain := configmodels.DeviceGroupsIpDomainExpanded{

patriciareinoso · 2025-02-10T16:18:49Z

configapi/api_sub_config_test.go

+		Imsis:            imsis,
+		SiteInfo:         "demo",
+		IpDomainName:     "pool1",
+		IpDomainExpanded: ipdomain,


Suggested change

IpDomainExpanded: ipdomain,

IpDomainExpanded: ipDomain,

patriciareinoso · 2025-02-10T16:30:23Z

configapi/api_sub_config_test.go

@@ -0,0 +1,354 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright 2024 Canonical Ltd.


Suggested change

// Copyright 2024 Canonical Ltd.

// Copyright 2025 Canonical Ltd.

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

patriciareinoso · 2025-02-11T12:50:42Z

configapi/api_sub_config_test.go

+			}
+		default:
+			t.Error("expected message in configChannel, but none received")
+	}


check over the second message is missing

patriciareinoso · 2025-02-11T12:54:42Z

configapi/api_sub_config_test.go

+		default:
+			t.Error("expected message in configChannel, but none received")
+	}
+}


we can check that only one message is sent: read again from the channel and if it does not go to the default clause, it should raise an error.

We need to try to read message again from the channel and msg should not exist.

gatici · 2025-02-11T21:10:58Z

configapi/api_sub_config.go

+	filterByImsi := bson.M{
+		"imsis": imsi,
+	}
+	rawDeviceGroups, err := dbadapter.CommonDBClient.RestfulAPIGetMany(devGroupDataColl, filterByImsi)


In RestfulAPIGetMany var resultArray []map[string]interface{} initial value is nil. According to cur, err := collection.Find(ctx, filter), if filter does not match it can stay as nil and it can return rawDeviceGroups as nil and err as nil.
In line 509, as we only check the err is nil or not, execution continue.
On line 513, iterating over rawDeviceGroups which is nil may cause panic.

Initial value of var resultArray []map[string]interface{} is not nil but []. Actually, there is a test (TestSubscriberDeleteNoDeviceGroup) where the IMSI to be deleted does not belong to any device group and it does not panic. From the debugger: []map[string]interface {}=[]

You are right. Please discard this comment.

gatici · 2025-02-11T21:30:49Z

configapi/api_sub_config.go

+	for _, rawDeviceGroup := range rawDeviceGroups {
+		var deviceGroup configmodels.DeviceGroups
+		if err = json.Unmarshal(configmodels.MapToByte(rawDeviceGroup), &deviceGroup); err != nil {
+			logger.WebUILog.Errorf("error unmarshaling device group: %v", err)


we can improve logging here by improving device group name.

At this point we are not sure that the DeviceGroupName field is available in the object we are trying to unmarshal: logging the raw object could simplify the debugging operations.

gatici · 2025-02-11T21:45:59Z

configapi/api_sub_config.go

@@ -485,6 +489,9 @@ func DeleteSubscriberByID(c *gin.Context) {

 	c.JSON(http.StatusNoContent, gin.H{})

+	imsi := strings.TrimPrefix(ueId, "imsi-")
+	updateSubscriberInDeviceGroups(imsi)


According to line 490, before doing any operation, DeleteSubscriberByID function send HTTP 204 no content response, then it calls updateSubscriberInDeviceGroups(imsi) method. I saw two problems here:

Order: response is sent without checking the success of device group update

Error propagation: updateSubscriberInDeviceGroups log the errors but does not propagate them to caller method. The caller always report a successful operation regardless from what happened in the updateSubscriberInDeviceGroups.

gatici · 2025-02-11T21:50:41Z

configapi/api_sub_config.go

+	filterByImsi := bson.M{
+		"imsis": imsi,


I suggest to validate the existence of imsi here.

gab-arrobo · 2025-02-12T16:05:48Z

@dariofaccin, you need to sign every commit you make (-s), otherwise, the DCO will fail

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

gatici

Looks good.

gatici · 2025-02-13T13:14:28Z

configapi/api_sub_config_test.go

+		default:
+			t.Error("expected message in configChannel, but none received")
+	}
+}


We need to try to read message again from the channel and msg should not exist.

gatici · 2025-02-13T13:27:10Z

configapi/api_sub_config_test.go

+	dbAdapter := &MockMongoClientDeviceGroupsWithSubscriber{}
+	dbadapter.CommonDBClient = dbAdapter


Generally speaking, we are accessing the dbadapter.CommonDBClient and configChannel global variables and changing their states in the tests. I saw some tests doing it but some are not. All the tests that accessing those global variables need to save their original states and after test, we need to restore them.
For example:

// Save original state origDBClient := dbadapter.CommonDBClient origConfigChannel := configChannel // Restore them after the test finishes defer func() { dbadapter.CommonDBClient = origDBClient configChannel = origConfigChannel }() // Set up a mock database adapter dbAdapter := &MockMongoClientDeviceGroupsWithSubscriber{} dbadapter.CommonDBClient = dbAdapter // Create a new llocal config channel for the test configChannel = make(chan *configmodels.ConfigMessage, 2) ...

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

dariofaccin added 11 commits January 29, 2025 11:42

feat: propagate subscriber deletion to device groups

a43c3ba

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

Merge branch 'main' into TELCO-1556-propagate-subscriber-deletion

4eeb21f

fix: return empty subscribers list and handle DB failure

bebae5a

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

chore: fix typos and restore return codes

d14f2ed

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

chore: add unit tests

1aeb1bd

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

revert changes

15a3a76

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

fix: return empty subscribers list and handle DB failure

58eb6d5

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

chore: update unit tests

b81328a

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

feat: send update DGs message to propagate Subscriber deletion

b9e7ddf

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

fix: use imsi instead of ueId

69f69fe

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

fix: update device groups before deleting the subscriber

e65064e

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

patriciareinoso suggested changes Feb 6, 2025

View reviewed changes

chore: address review comments

a092729

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

dariofaccin requested a review from patriciareinoso February 9, 2025 10:22

patriciareinoso suggested changes Feb 10, 2025

View reviewed changes

patriciareinoso reviewed Feb 10, 2025

View reviewed changes

chore: refactor unit tests

3fcddbd

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

dariofaccin requested a review from patriciareinoso February 11, 2025 10:30

patriciareinoso suggested changes Feb 11, 2025

View reviewed changes

gatici reviewed Feb 11, 2025

View reviewed changes

Merge branch 'main' into TELCO-1556-propagate-subscriber-deletion

e5c3c3b

dariofaccin requested review from gatici and patriciareinoso February 12, 2025 13:07

dariofaccin added 2 commits February 13, 2025 09:13

chore: improve deletion handling

709b066

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

chore: improve error response

fe11864

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

dariofaccin force-pushed the TELCO-1556-propagate-subscriber-deletion branch from dcd98c9 to fe11864 Compare February 13, 2025 08:13

gatici reviewed Feb 13, 2025

View reviewed changes

chore: restore state of global variables after each test

6597b57

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

chore: improve config channel messages validation

0cafddb

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

dariofaccin requested a review from gatici February 13, 2025 17:37

	inputData: `{"UeId":"208930100007487", "plmnId":"12345", "opc":"981d464c7c52eb6e5036234984ad0bcf","key":"5122250214c33e723a5dd523fc145fc0", "sequenceNumber":"16f3b3f70fc2"}`,
	inputData: `{"plmnID":"12345", "opc":"981d464c7c52eb6e5036234984ad0bcf","key":"5122250214c33e723a5dd523fc145fc0", "sequenceNumber":"16f3b3f70fc2"}`,

	AuthenticationMethod: "5G_AKA", // "5G_AKA", "EAP_AKA_PRIME"
	AuthenticationMethod: "5G_AKA",

	inputData: `{"plmnId":"12345", "opc":"8e27b6af0e692e750f32667a3b14605d","key":"8baf473f2f8fd09487cccbd7097c6862", "sequenceNumber":"16f3b3f70fc2"}`,
	inputData: `{"plmnID":"12345", "opc":"8e27b6af0e692e750f32667a3b14605d","key":"8baf473f2f8fd09487cccbd7097c6862", "sequenceNumber":"16f3b3f70fc2"}`,

	traffic_class := configmodels.TrafficClassInfo{
	trafficClass := configmodels.TrafficClassInfo{

	ipdomain := configmodels.DeviceGroupsIpDomainExpanded{
	ipDomain := configmodels.DeviceGroupsIpDomainExpanded{

		@@ -0,0 +1,354 @@
		// SPDX-License-Identifier: Apache-2.0
		// Copyright 2024 Canonical Ltd.

	// Copyright 2024 Canonical Ltd.
	// Copyright 2025 Canonical Ltd.

		dbAdapter := &MockMongoClientDeviceGroupsWithSubscriber{}
		dbadapter.CommonDBClient = dbAdapter

feat: propagate Subscriber deletion to Device Groups #299

Are you sure you want to change the base?

feat: propagate Subscriber deletion to Device Groups #299

Conversation

dariofaccin commented Feb 4, 2025 • edited Loading

Issue

Description

How the code works

Webconsole behavior on Subscriber POST before this PR

Webconsole behavior on Subscriber DELETE before this PR

This PR

How to reproduce

What you should see

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

This comment was marked as outdated.

Choose a reason for hiding this comment

This comment was marked as outdated.

patriciareinoso commented Feb 6, 2025

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 Feb 12, 2025 • edited Loading

gatici left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariofaccin commented Feb 4, 2025 •

edited

Loading

gab-arrobo commented Feb 12, 2025 •

edited

Loading