Skip to content

Commit 0c7aade

Browse files
pandafynemesifier
andauthored
[fix] Exclude location_id from radius_acc extra_tags if missing
Avoid including the `location_id` tag in `radius_acc` metrics when the device has no associated `DeviceLocation`. InfluxDB does not support tags with `None` values, which causes errors when attempting to delete such metrics. --------- Co-authored-by: Federico Capoano <f.capoano@openwisp.io>
1 parent 3394744 commit 0c7aade

2 files changed

Lines changed: 54 additions & 14 deletions

File tree

openwisp_radius/integrations/monitoring/tasks.py

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,12 @@ def post_save_radiusaccounting(
195195
else:
196196
registration_method = clean_registration_method(registration_method)
197197
device_lookup = Q(mac_address__iexact=called_station_id.replace('-', ':'))
198+
extra_tags = {
199+
'method': registration_method,
200+
'organization_id': organization_id,
201+
'calling_station_id': sha1_hash(calling_station_id),
202+
'called_station_id': called_station_id,
203+
}
198204
# Do not use organization_id for device lookup if shared accounting is enabled
199205
if not app_settings.SHARED_ACCOUNTING:
200206
device_lookup &= Q(organization_id=organization_id)
@@ -212,30 +218,21 @@ def post_save_radiusaccounting(
212218
)
213219
object_id = None
214220
content_type = None
215-
location_id = None
216221
if app_settings.SHARED_ACCOUNTING:
217-
organization_id = None
222+
extra_tags.pop('organization_id')
218223
else:
219224
object_id = str(device.id)
220225
content_type = ContentType.objects.get_for_model(Device)
221226
if hasattr(device, 'devicelocation'):
222-
location_id = str(device.devicelocation.location_id)
223-
else:
224-
location_id = None
227+
extra_tags['location_id'] = str(device.devicelocation.location_id)
225228

226229
metric, created = Metric._get_or_create(
227230
configuration='radius_acc',
228231
name='RADIUS Accounting',
229232
key='radius_acc',
230233
object_id=object_id,
231234
content_type=content_type,
232-
extra_tags={
233-
'organization_id': organization_id,
234-
'method': registration_method,
235-
'calling_station_id': sha1_hash(calling_station_id),
236-
'called_station_id': called_station_id,
237-
'location_id': location_id,
238-
},
235+
extra_tags=extra_tags,
239236
)
240237
metric.write(
241238
input_octets,

openwisp_radius/integrations/monitoring/tests/test_metrics.py

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,51 @@ def test_post_save_radiusaccounting(self, *args):
8686
self.assertEqual(points['traces'][0][1][-1], 1)
8787
self.assertEqual(points['summary'], {'mobile_phone': 1})
8888

89+
@patch('logging.Logger.warning')
90+
def test_post_save_radiusaccounting_device_without_location(self, *args):
91+
user = self._create_user()
92+
reg_user = self._create_registered_user(user=user)
93+
device = self._create_device()
94+
options = _RADACCT.copy()
95+
options.update(
96+
{
97+
'unique_id': '117',
98+
'username': user.username,
99+
'called_station_id': device.mac_address.replace('-', ':').upper(),
100+
'calling_station_id': '00:00:00:00:00:00',
101+
'input_octets': '8000000000',
102+
'output_octets': '9000000000',
103+
}
104+
)
105+
options['stop_time'] = options['start_time']
106+
self._create_radius_accounting(**options)
107+
with self.subTest('location_id should not be set'):
108+
self.assertEqual(
109+
self.metric_model.objects.filter(
110+
configuration='radius_acc',
111+
name='RADIUS Accounting',
112+
key='radius_acc',
113+
object_id=str(device.id),
114+
content_type=ContentType.objects.get_for_model(self.device_model),
115+
extra_tags={
116+
'called_station_id': device.mac_address,
117+
'calling_station_id': sha1_hash('00:00:00:00:00:00'),
118+
'method': reg_user.method,
119+
'organization_id': str(self.default_org.id),
120+
},
121+
).count(),
122+
1,
123+
)
124+
with self.subTest('Deleting device without location_id should not fail'):
125+
self.device_model.objects.all().delete()
126+
self.assertEqual(self.device_model.objects.count(), 0)
127+
self.assertEqual(
128+
self.metric_model.objects.filter(
129+
key='radius_acc', object_id=str(device.id)
130+
).count(),
131+
0,
132+
)
133+
89134
@patch('openwisp_radius.integrations.monitoring.tasks.post_save_radiusaccounting')
90135
def test_post_save_radiusaccouting_open_session(self, mocked_task):
91136
radius_options = _RADACCT.copy()
@@ -157,7 +202,6 @@ def test_post_save_radius_accounting_shared_accounting(self, mocked_logger):
157202
extra_tags={
158203
'called_station_id': device.mac_address,
159204
'calling_station_id': sha1_hash('00:00:00:00:00:00'),
160-
'location_id': None,
161205
'method': reg_user.method,
162206
'organization_id': str(self.default_org.id),
163207
},
@@ -222,7 +266,6 @@ def test_post_save_radius_accounting_device_not_found(self, mocked_logger):
222266
extra_tags={
223267
'called_station_id': '11:22:33:44:55:66',
224268
'calling_station_id': sha1_hash('00:00:00:00:00:00'),
225-
'location_id': None,
226269
'method': reg_user.method,
227270
'organization_id': str(self.default_org.id),
228271
},

0 commit comments

Comments
 (0)