[bug] Sortedm2m causes vpnclients to be destroyed and recreated every time the configuration or templates are changed

[nemesifier]

How to replicate:

• create a device
• create a configuration, assign a template of type "VPN client" which has auto-cert enabled, save
• go to the certificates list, you'll see a new certificate has been created
• change the details of the configuration (eg: the context or the config), or alternatively, add a new template

Expected result:

The certificate created in the previous steps should still exists.

Actual result:

The certificate is destroyed and a new one is created.

Cause:

This is caused by the annoying implementation of django-sortedm2m, which implements the reodreding of m2m relationships by clearing templates and adding them back again, which causes signal receivers of openwisp to destroy and recreate many different things.

My thoughts:

I'm not sure if we can solve it without forking django-sortedm2m and implement the reordering in a different way, nor I'm sure if it's worth pursuing this now, but I want to leave this here for others to know so at least we can be aware of it.

[nemesifier]

I thought that we could solve it in a different way:

• avoid clearing vpnclients on post_clear
• when creating new vpnclients, check if they already exists for that config and vpn, if they do, skip

[Saurav-Shrivastav]

I will start working on this and make a PR soon.

[devkapilbansal]

I thought that we could solve it in a different way:

• avoid clearing vpnclients on post_clear
• when creating new vpnclients, check if they already exists for that config and vpn, if they do, skip

But if we just skip, then how the ordering will be maintained 🤔

[nemesifier]

@devkapilbansal good question, but there's no concept of ordering in VPN clients, so template m2m relationships (config.templates) have to be deleted and recreated, because those one do have the concept of order, what we have to avoid being deleted and recreted are VpnClient instances, VpnClient is a custom m2m through model.

So, templates through emit m2m_changed when they're changed, we connect a receiver to this signal:

openwisp-controller/openwisp_controller/config/apps.py

Lines 62 to 66 in 32c4bc9

	m2m_changed.connect(
	self.config_model.manage_vpn_clients,
	sender=self.config_model.templates.through,
	dispatch_uid='config.manage_vpn_clients',
	)

The receiver function (actually a class method) is here:

openwisp-controller/openwisp_controller/config/base/config.py

Lines 207 to 241 in 32c4bc9

	@classmethod
	def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
	"""
	automatically manages associated vpn clients if the
	instance is using templates which have type set to "VPN"
	and "auto_cert" set to True.
	This method is called from a django signal (m2m_changed)
	see config.apps.DjangoNetjsonconfigApp.connect_signals
	"""
	if action not in ['post_add', 'post_remove', 'post_clear']:
	return
	vpn_client_model = cls.vpn.through
	# coming from signal
	if isinstance(pk_set, set):
	template_model = cls.get_template_model()
	templates = template_model.objects.filter(pk__in=list(pk_set))
	# coming from admin ModelForm
	else:
	templates = pk_set
	# when clearing all templates
	if action == 'post_clear':
	for client in instance.vpnclient_set.all():
	client.delete()
	return
	# when adding or removing specific templates
	for template in templates.filter(type='vpn'):
	if action == 'post_add':
	client = vpn_client_model(
	config=instance, vpn=template.vpn, auto_cert=template.auto_cert
	)
	client.full_clean()
	client.save()
	elif action == 'post_remove':
	for client in instance.vpnclient_set.filter(vpn=template.vpn):
	client.delete()

To anyone wanting to work on this: please write a test first, eg:

• create a vpn server
• create a vpn client template
• create a new device and config and assign it the template created at the previous step
• ensure the vpnclient instance is created and with it the x509 Cert instance
• now call device.config.templates.clear()
• ensure the related VpnClient and Cert are not deleted
• now assign the template to the config object again and ensure no additional VpnClient is created and everything runs fine

Please do not forget updating the docs, that is, we have to state clearly in the README that vpnclients relationships are not deleted when clearing templates because of how sortedm2m works to reorder templates.

[Saurav-Shrivastav]

@nemesisdesign Very naive question - Which file should be ideally used for writing the test for this solution?

[nemesifier]

@nemesisdesign Very naive question - Which file should be ideally used for writing the test for this solution?

@Saurav-Shrivastav take a look at https://github.com/openwisp/openwisp-controller/blob/master/openwisp_controller/config/tests/test_config.py#L297-L379.

There's also one test which will have to be changed (the one which tests the clear() operation).