IPSec: multiple improvements #1485
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements improvements to IPSec tunnel management by refactoring the edit_tunnel function to update tunnels in-place rather than deleting and recreating them, changing the default closeaction value from 'none' to 'trap', and allowing custom closeaction values. The PR also includes changes to HA (High Availability) components that are not mentioned in the PR description.
Changes:
- Refactored
edit_tunnelto update tunnels in-place, only recreating tunnel sections when subnets change - Changed
closeactiondefault value from 'none' to 'trap' and added support for custom values - Modified HA script to skip dnsmasq stop during startup (unrelated to IPSec)
- Changed keepalived default enabled state from 1 to 0 (unrelated to IPSec)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/ns-ha/files/500-dhcp-force | Prevents dnsmasq from being stopped during startup by adding startup check to elif condition |
| packages/ns-api/files/ns.ipsectunnel | Refactors edit_tunnel to update in-place, adds closeaction parameter with 'trap' default, updates get_tunnel and documentation |
| packages/keepalived/files/keepalived.init | Changes keepalived default enabled value from 1 to 0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| current_remote = current_remote | set(tmpr) | ||
| except: | ||
| continue | ||
|
|
There was a problem hiding this comment.
If no existing tunnels are found for this remote ID, if_id will remain None. This could cause issues when creating the dname variable (line 259, 292) which would become 'ipsecNone'. The function should handle the case where no existing tunnels are found, either by raising an error or by obtaining if_id from the remote section's tunnel list.
| # Ensure we have a valid interface id for subsequent operations. | |
| # If no existing tunnels were found for this ID, we cannot safely proceed. | |
| if if_id is None: | |
| raise ValueError(f"No existing IPSec tunnels found for id '{id}' to derive if_id") |
packages/ns-api/files/ns.ipsectunnel
Outdated
| tmpr = u.get_all('ipsec', t, 'remote_subnet') | ||
| current_remote = current_remote | set(tmpr) |
There was a problem hiding this comment.
The variable name 'tmpr' (temporary remote) is not descriptive. Consider using a more meaningful name like 'remote_subnets' for better code readability.
| tmpr = u.get_all('ipsec', t, 'remote_subnet') | |
| current_remote = current_remote | set(tmpr) | |
| remote_subnets = u.get_all('ipsec', t, 'remote_subnet') | |
| current_remote = current_remote | set(remote_subnets) |
packages/ns-api/files/ns.ipsectunnel
Outdated
| for ls in args['local_subnet']: | ||
| for rs in args['remote_subnet']: | ||
| tunnel = f'{tunnel_base}_{ti}' | ||
| u.set('ipsec', tunnel, 'tunnel') | ||
| for opt in ['ipcomp', 'dpdaction']: | ||
| u.set('ipsec', tunnel, opt, args[opt]) | ||
| u.set('ipsec', tunnel, 'local_subnet', [ls]) | ||
| u.set('ipsec', tunnel, 'remote_subnet', [rs]) | ||
| u.set('ipsec', tunnel, 'rekeytime', args['esp']['rekeytime']) | ||
| u.set('ipsec', tunnel, 'crypto_proposal', [esp_p]) | ||
| u.set('ipsec', tunnel, 'closeaction', args.get('closeaction', 'trap')) | ||
| u.set('ipsec', tunnel, 'startaction', 'start') | ||
| u.set('ipsec', tunnel, 'if_id', if_id) | ||
| u.set('ipsec', tunnel, 'ns_link', link) | ||
| tunnels.append(tunnel) | ||
| ti = ti + 1 |
There was a problem hiding this comment.
This tunnel creation logic in lines 238-253 is duplicated from the setup_tunnel function (lines 120-136). Consider extracting this into a shared helper function to avoid code duplication and maintain consistency between add and edit operations.
packages/ns-api/files/ns.ipsectunnel
Outdated
| subnets = 1 | ||
| for net in args['remote_subnet']: | ||
| rname = f'{id}_route_{subnets}' | ||
| u.set('network', rname, 'route') | ||
| u.set('network', rname, 'target', net) | ||
| u.set('network', rname, 'interface', dname) | ||
| u.set('network', rname, 'ns_link', link) | ||
| u.set('network', rname, 'disabled', '0' if args['enabled'] == '1' else '1') | ||
| subnets = subnets + 1 |
There was a problem hiding this comment.
This route creation logic in lines 266-274 is duplicated from the setup_tunnel function (lines 163-171). Consider extracting this into a shared helper function to avoid code duplication and ensure consistency in route handling.
packages/ns-api/files/ns.ipsectunnel
Outdated
| tmpl = u.get_all('ipsec', t, 'local_subnet') | ||
| current_local = current_local | set(tmpl) | ||
| tmpr = u.get_all('ipsec', t, 'remote_subnet') | ||
| current_remote = current_remote | set(tmpr) |
There was a problem hiding this comment.
The variable name 'tmpl' (temporary local) is not descriptive. Consider using a more meaningful name like 'local_subnets' for better code readability.
| tmpl = u.get_all('ipsec', t, 'local_subnet') | |
| current_local = current_local | set(tmpl) | |
| tmpr = u.get_all('ipsec', t, 'remote_subnet') | |
| current_remote = current_remote | set(tmpr) | |
| local_subnets = u.get_all('ipsec', t, 'local_subnet') | |
| current_local = current_local | set(local_subnets) | |
| remote_subnets = u.get_all('ipsec', t, 'remote_subnet') | |
| current_remote = current_remote | set(remote_subnets) |
|
|
||
| config_load 'keepalived' | ||
| config_get_bool enabled globals enabled 1 | ||
| config_get_bool enabled globals enabled 0 |
There was a problem hiding this comment.
Changing the default enabled value from 1 to 0 means keepalived will be disabled by default instead of enabled by default. This is a significant behavior change that could affect existing deployments that rely on the default enabled state. This change also appears unrelated to the IPSec improvements mentioned in the PR title and description. Please verify this change is intentional and document it in the PR description if it should be included.
| config_get_bool enabled globals enabled 0 | |
| config_get_bool enabled globals enabled 1 |
When a tunnel is completely deleted during edit, swanctl can't handle the changes without a full restart. To avoid this issue, just edit changed fields.
- New default value for closaction is 'trap' that tries to recreate the CHILD_SA - Make the closeaction value editable
gsanchietti
force-pushed
the
improve_ipsec
branch
from
f5bb078 to
f11f8c8
Compare
Changes: