VPN template conflict during template switching #977

shwetd19 · 2025-02-05T19:29:31Z

Resolves a bug where VPN configuration variables become unresolved when switching between VPN templates using the same VPN server

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
I have updated the documentation.

Reference to Existing Issue

Closes #973

Description of Changes

This PR fixes a race condition in the VPN client management code that occurs when switching between VPN templates that use the same VPN server. The bug popups when simultaneously removing one VPN template while adding another that uses the same VPN server.

The Problem

The issue was occuring due to how VPN clients are managed during template changes:

When adding a new template (post_add):
- Creating a new VPN client for the new template
- Deletes existing VPN clients not in the current template list
- This could delete VPN clients still needed by other templates
When removing a template (post_remove):
- Deletes VPN clients associated with the removed template
- This could delete VPN clients still needed by remaining templates

As a result, the configuration could end up with no VPN client object, causing VPN configuration variables to remain unresolved.

The Solution

The fix modifies the manage_vpn_clients method to:

Tracking all of current VPN templates and their associated VPNs upfront
For post_add:
- Create new VPN clients first
- Only delete VPN clients whose VPNs are not associated with any current template
For post_remove:
- Only delete VPN clients if their VPN is not used by any remaining template

This soln ensures VPN clients are preserved when switching between templates that use the same VPN server, maintaining proper configuration variable resolution.

Resolves a bug where VPN configuration variables become unresolved when switching between VPN templates using the same VPN server

pandafy

@shwetd19 kindly add a failing test for the bug described in the issue. A fix for this issue should not remove the existing features, i.e. the existing tests should not fail due to missing logic.

pandafy · 2025-02-06T11:03:34Z

openwisp_controller/config/base/config.py

@@ -245,7 +245,6 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
        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.ConfigConfig.connect_signals


Why did you remove the comments?

@pandafy Sorry for removing the comments - that was unintentional. I've restored the original docstring comments in my latest commit

pandafy · 2025-02-06T11:24:55Z

openwisp_controller/config/base/config.py

+            for template in templates.filter(type='vpn'):
+                if template.vpn_id not in current_vpns:
+                    instance.vpnclient_set.filter(vpn=template.vpn).delete()


Can this be changed into

Suggested change

for template in templates.filter(type='vpn'):

if template.vpn_id not in current_vpns:

instance.vpnclient_set.filter(vpn=template.vpn).delete()

instance.vpnclient_set.exclude(vpn__in=current_vpns).delete()

We need to ensure that after the operation, there are not two VpnClients for this config object.

@pandafy Thanks for catching this! I understand the concern about potential duplicate VPN clients. I've modified the code to use your suggested approach:

In post_add:

First create new VPN clients

Then cleanup any unused clients with exclude(vpn__in=current_vpns)

In post_remove:

Simply cleanup all unused clients with exclude(vpn__in=current_vpns)

This ensures we won't have duplicate VPN clients for the same config object while preserving the necessary clients. The full cleanup at the end handles any edge cases where duplicate clients might have been created.

We need a test which verifies this.

- Kept the original docstring comments - Modified the VPN client cleanup logic as suggested - Fixed the formatting issues (blank lines with whitespace) - Added a failing test case for the bug

shwetd19 · 2025-02-07T04:35:00Z

Hey @pandafy I've implemented the suggested changes, can you pls check once

shwetd19 · 2025-02-10T09:42:37Z

Hey @pandafy @nemesifier can you please reivew this PR ?
pls let me know if any changes are required, Thanks!

pandafy

@shwetd19 add a test. You can take reference from this existing test here

openwisp-controller/openwisp_controller/config/tests/test_admin.py

Lines 1623 to 1704 in 129a42f

    
           def test_vpn_clients_deleted(self): 
        
               def _update_template(templates): 
        
                   params.update( 
        
                       { 
        
                           'config-0-templates': ','.join( 
        
                               [str(template.pk) for template in templates] 
        
                           ) 
        
                       } 
        
                   ) 
        
                   response = self.client.post(path, data=params, follow=True) 
        
                   self.assertEqual(response.status_code, 200) 
        
                   for template in templates: 
        
                       self.assertContains( 
        
                           response, f'class="sortedm2m" checked> {template.name}' 
        
                       ) 
        
                   return response 
        
               vpn = self._create_vpn() 
        
               template = self._create_template() 
        
               vpn_template = self._create_template( 
        
                   name='vpn-test', 
        
                   type='vpn', 
        
                   vpn=vpn, 
        
                   auto_cert=True, 
        
               ) 
        
               cert_query = Cert.objects.exclude(pk=vpn.cert_id) 
        
               valid_cert_query = cert_query.filter(revoked=False) 
        
               revoked_cert_query = cert_query.filter(revoked=True) 
        
               # Add a new device 
        
               path = reverse(f'admin:{self.app_label}_device_add') 
        
               params = self._get_device_params(org=self._get_org()) 
        
               response = self.client.post(path, data=params, follow=True) 
        
               self.assertEqual(response.status_code, 200) 
        
               config = Device.objects.get(name=params['name']).config 
        
               self.assertEqual(config.vpnclient_set.count(), 0) 
        
               self.assertEqual(config.templates.count(), 0) 
        
               path = reverse(f'admin:{self.app_label}_device_change', args=[config.device_id]) 
        
               params.update( 
        
                   { 
        
                       'config-0-id': str(config.pk), 
        
                       'config-0-device': str(config.device_id), 
        
                       'config-INITIAL_FORMS': 1, 
        
                       '_continue': True, 
        
                   } 
        
               ) 
        
               with self.subTest('Adding only VpnClient template'): 
        
                   # Adding VpnClient template to the device 
        
                   _update_template(templates=[vpn_template]) 
        
                   self.assertEqual(config.templates.count(), 1) 
        
                   self.assertEqual(config.vpnclient_set.count(), 1) 
        
                   self.assertEqual(cert_query.count(), 1) 
        
                   self.assertEqual(valid_cert_query.count(), 1) 
        
                   # Remove VpnClient template from the device 
        
                   _update_template(templates=[]) 
        
                   self.assertEqual(config.templates.count(), 0) 
        
                   self.assertEqual(config.vpnclient_set.count(), 0) 
        
                   # Removing VPN template marks the related certificate as revoked 
        
                   self.assertEqual(revoked_cert_query.count(), 1) 
        
                   self.assertEqual(valid_cert_query.count(), 0) 
        
               with self.subTest('Add VpnClient template along with another template'): 
        
                   # Adding templates to the device 
        
                   _update_template(templates=[template, vpn_template]) 
        
                   self.assertEqual(config.templates.count(), 2) 
        
                   self.assertEqual(config.vpnclient_set.count(), 1) 
        
                   self.assertEqual(valid_cert_query.count(), 1) 
        
                   # Remove VpnClient template from the device 
        
                   _update_template(templates=[template]) 
        
                   self.assertEqual(config.templates.count(), 1) 
        
                   self.assertEqual(config.vpnclient_set.count(), 0) 
        
                   self.assertEqual(valid_cert_query.count(), 0) 
        
                   self.assertEqual(revoked_cert_query.count(), 2)

The test should do the following operations:

Create a VPN object
Create two templates with the same vpn server. (say template1 and template 2)

        vpn = self._create_vpn()
        template1 = self._create_template(
            name='vpn-test-1',
            type='vpn',
            vpn=vpn,
            config={},
            auto_cert=True,
            default=True, # This will auto-apply template1 to the device
        )
        template2 = self._create_template(
            name='vpn-test-2',
            type='vpn',
            vpn=vpn,
            config={},
            auto_cert=True,
        )

Using the Django admin, apply tmplate2 and remove template1 from the device
Verify there's only 1 VpnClient present for the config
Verify that the variables are correctly resolved in the config using the following:

       self.assertEqual(
            config.backend_instance.config['openvpn'][0]['cert'],
            f'/etc/x509/client-{vpn.pk.hex}.pem',
        )

We cannot accept a bugfix patch with a test. Ensure that the test fails without the changes you've made in openwisp_controller/config/base/config.py

nemesifier

@shwetd19 as @pandafy indicated, we need a tests which replicates the issue and fails without the patch. Please let us know if you have doubts on how to write it.

shwetd19 · 2025-02-12T13:12:49Z

Hey @pandafy @nemesifier ,

Thank you for the clarification. I understand that I need to write a test that replicates the issue and fails without the patch. I’ll follow the structure you provided and ensure the test verifies the presence of only one VpnClient and the correct resolution of configuration variables.

Just to confirm, should the test also include a check for the behavior when switching back and forth between template1 and template2 multiple times? Or is it sufficient to test a single switch from template1 to template2?

I’ll proceed with writing the test and will update the PR soon. If there’s anything else I should keep in mind, please let me know.

Thanks again for your guidance!

Best regards,
Shwet

This reply shows that you’re actively engaging with the feedback, seeking clarification to ensure you meet expectations, and demonstrating your commitment to addressing the issue properly.

nemesifier

@shwetd19

Just to confirm, should the test also include a check for the behavior when switching back and forth between template1 and template2 multiple times? Or is it sufficient to test a single switch from template1 to template2?

I’ll proceed with writing the test and will update the PR soon. If there’s anything else I should keep in mind, please let me know.

Switching back would from template2 to template1 would be nice and shouldn't involve a lot more effort.

Thanks again for your guidance!

Welcome!

PS: look at the existing tests to get inspiration on how to write your test, try to be consistent with the rest of the codebase please.

shwetd19 · 2025-02-13T18:08:27Z

Hey @pandafy @nemesifier

I've added the fix here as : Added a test to replicate the bug and verify the fix, which creates two VPN templates using the same VPN server, switches between them, and verifies that only one VpnClient object exists for the configuration and that VPN configuration variables are resolved correctly, ensuring the test fails without the fix and passes with it applied.

also I've tested it locally as here :

pandafy

@shwetd19 I don't see any progress on this PR after my last review.

Your recent commits have deleted the changes to the vpn_templates logic, kindly rectify it.

pandafy · 2025-02-14T10:26:47Z

openwisp_controller/config/base/config.py

+        config.templates.add(template1)
+        config.save()


We have used default=True while creating the template, thus we don't need to add the template1 to the Config object.

pandafy · 2025-02-14T10:29:41Z

openwisp_controller/config/base/config.py

@@ -812,5 +812,63 @@ def manage_backend_changed(cls, instance_id, old_backend, backend, **kwargs):
            old_templates = device_group.templates.filter(backend=old_backend)
        config.manage_group_templates(templates, old_templates, not created)

+    def test_vpn_template_switch(self):


@shwetd19 why did you add this test in the model definition.

pandafy · 2025-02-14T10:38:48Z

also I've tested it locally as here :

You ran the QA checks, not test suite. Read the docs on how to run the tests.

nemesifier

@shwetd19 have you verified that test_vpn_template_switch fails with a clear error message without the code fix?

nemesifier · 2025-02-14T19:34:40Z

openwisp_controller/config/base/config.py

@@ -256,7 +256,7 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):

        if action == 'post_clear':
            if instance.is_deactivating_or_deactivated():
-                # If the device is deactivated or in the process of deactivating, then
+                # If the device is deactivated or in the process of deactivatiing, then


Suggested change

# If the device is deactivated or in the process of deactivatiing, then

# If the device is deactivated or in the process of deactivating, then

nemesifier

PS: QA checks are failing.

[fix]VPN template conflict during template switching

6f1a7db

Resolves a bug where VPN configuration variables become unresolved when switching between VPN templates using the same VPN server

pandafy requested changes Feb 6, 2025

View reviewed changes

[fix] Updated VPN Template Management Code

5a3daf8

- Kept the original docstring comments - Modified the VPN client cleanup logic as suggested - Fixed the formatting issues (blank lines with whitespace) - Added a failing test case for the bug

shwetd19 requested a review from pandafy February 7, 2025 04:34

pandafy requested changes Feb 10, 2025

View reviewed changes

nemesifier requested changes Feb 11, 2025

View reviewed changes

nemesifier reviewed Feb 12, 2025

View reviewed changes

[fix] Resolve VPN template conflict during template switching

cff3227

shwetd19 requested review from nemesifier and pandafy February 13, 2025 18:42

pandafy requested changes Feb 14, 2025

View reviewed changes

nemesifier reviewed Feb 14, 2025

View reviewed changes

nemesifier requested changes Feb 14, 2025

View reviewed changes

shwetd19 added 2 commits February 15, 2025 13:34

[fix] Resolving VPN template conflict when switching templates

fe76352

[test] Adding test for VPN template switching

2aaf960

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPN template conflict during template switching #977

VPN template conflict during template switching #977

shwetd19 commented Feb 5, 2025

pandafy left a comment

pandafy Feb 6, 2025

shwetd19 Feb 7, 2025

pandafy Feb 6, 2025

pandafy Feb 6, 2025

shwetd19 Feb 7, 2025

pandafy Feb 10, 2025

shwetd19 commented Feb 7, 2025

shwetd19 commented Feb 10, 2025

pandafy left a comment

nemesifier left a comment

shwetd19 commented Feb 12, 2025

nemesifier left a comment

shwetd19 commented Feb 13, 2025

pandafy left a comment

pandafy Feb 14, 2025

pandafy Feb 14, 2025

pandafy commented Feb 14, 2025

nemesifier left a comment

nemesifier Feb 14, 2025

nemesifier left a comment

	def test_vpn_clients_deleted(self):
	def _update_template(templates):
	params.update(
	{
	'config-0-templates': ','.join(
	[str(template.pk) for template in templates]
	)
	}
	)
	response = self.client.post(path, data=params, follow=True)
	self.assertEqual(response.status_code, 200)
	for template in templates:
	self.assertContains(
	response, f'class="sortedm2m" checked> {template.name}'
	)
	return response

	vpn = self._create_vpn()
	template = self._create_template()
	vpn_template = self._create_template(
	name='vpn-test',
	type='vpn',
	vpn=vpn,
	auto_cert=True,
	)
	cert_query = Cert.objects.exclude(pk=vpn.cert_id)
	valid_cert_query = cert_query.filter(revoked=False)
	revoked_cert_query = cert_query.filter(revoked=True)

	# Add a new device
	path = reverse(f'admin:{self.app_label}_device_add')
	params = self._get_device_params(org=self._get_org())
	response = self.client.post(path, data=params, follow=True)
	self.assertEqual(response.status_code, 200)

	config = Device.objects.get(name=params['name']).config
	self.assertEqual(config.vpnclient_set.count(), 0)
	self.assertEqual(config.templates.count(), 0)

	path = reverse(f'admin:{self.app_label}_device_change', args=[config.device_id])
	params.update(
	{
	'config-0-id': str(config.pk),
	'config-0-device': str(config.device_id),
	'config-INITIAL_FORMS': 1,
	'_continue': True,
	}
	)

	with self.subTest('Adding only VpnClient template'):
	# Adding VpnClient template to the device
	_update_template(templates=[vpn_template])

	self.assertEqual(config.templates.count(), 1)
	self.assertEqual(config.vpnclient_set.count(), 1)
	self.assertEqual(cert_query.count(), 1)
	self.assertEqual(valid_cert_query.count(), 1)

	# Remove VpnClient template from the device
	_update_template(templates=[])

	self.assertEqual(config.templates.count(), 0)
	self.assertEqual(config.vpnclient_set.count(), 0)
	# Removing VPN template marks the related certificate as revoked
	self.assertEqual(revoked_cert_query.count(), 1)
	self.assertEqual(valid_cert_query.count(), 0)

	with self.subTest('Add VpnClient template along with another template'):
	# Adding templates to the device
	_update_template(templates=[template, vpn_template])

	self.assertEqual(config.templates.count(), 2)
	self.assertEqual(config.vpnclient_set.count(), 1)
	self.assertEqual(valid_cert_query.count(), 1)

	# Remove VpnClient template from the device
	_update_template(templates=[template])

	self.assertEqual(config.templates.count(), 1)
	self.assertEqual(config.vpnclient_set.count(), 0)
	self.assertEqual(valid_cert_query.count(), 0)
	self.assertEqual(revoked_cert_query.count(), 2)

	# If the device is deactivated or in the process of deactivatiing, then
	# If the device is deactivated or in the process of deactivating, then

VPN template conflict during template switching #977

Are you sure you want to change the base?

VPN template conflict during template switching #977

Conversation

shwetd19 commented Feb 5, 2025

Checklist

Reference to Existing Issue

Description of Changes

The Problem

The Solution

pandafy left a 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

shwetd19 commented Feb 7, 2025

shwetd19 commented Feb 10, 2025

pandafy left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

shwetd19 commented Feb 12, 2025

nemesifier left a comment

Choose a reason for hiding this comment

shwetd19 commented Feb 13, 2025

pandafy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandafy commented Feb 14, 2025

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment