Management: add reload-push-options & Fix PUSH_UPDATE logic
#916
Conversation
This adds a new management command 'reload-push-options' that allows reloading the push options from the configuration file without restarting the server. This is useful for updating routes or DNS settings for new clients without dropping existing connections. The command supports an optional 'sync' argument. When provided, the server will also synchronize the new options to currently connected clients by: 1. Calculating the difference between old and new push options. 2. Sending '-instruction' (e.g. -route) to remove old options. 3. Sending new options via PUSH_UPDATE. This includes a comprehensive integration test suite in tests/reload_push_options.
Previously, the logic for resetting push options (like 'route') was based on `update_options_found` which was local to `apply_push_options`. This meant that if a PUSH_UPDATE was split across multiple continuation messages, the state was lost, causing routes to be reset multiple times (once per message chunk) rather than once per update sequence. This patch moves the state tracking to `struct options` as `push_update_options_found`, allowing it to persist across the entire PUSH_UPDATE sequence. This fixes an issue where large route lists sent via PUSH_UPDATE would result in only the last chunk's routes being applied, or previous routes being continuously deleted and re-added. Added unit test `test_incoming_push_continuation_route_accumulation` to verify the fix.
|
Thanks for working on this, and thanks for sending all the good stuff to the At this point in time, close to the 2.7 release, the big feature enhancement might be a bit too much for us to digest. The bugfix patch really should go in before 2.7.0 - @mrbff can you review? |
|
Yes, I will review asap |
|
It looks bigger than it is; most of it is just end-to-end tests. The code change itself is not that much.... Please have a closer look at the gc_arena stuff; I hope I got that right. I'm basically creating a new options.push_list and then switching the current with the new one. |
| const msglvl_t msglevel = D_PUSH_ERRORS | M_OPTERR; | ||
| unsigned int update_options_found = 0; | ||
|
|
||
| /* |
There was a problem hiding this comment.
this comment could be moved above the update_option() call
| } | ||
| else | ||
| { | ||
| update_option(c, options, p, false, file, line_num, 0, msglevel, permission_mask, |
There was a problem hiding this comment.
Actually at this point the last parameter of update_option() could be removed as we pass the entire options struct anyway. but is just refactor anyway...
| goto error; | ||
| } | ||
| } | ||
| /* Clear push_update_options_found for next PUSH_UPDATE sequence */ |
There was a problem hiding this comment.
i would suggest to put this reset inside the else (is not a REPLY so is PUSH_UPDATE) and before the possible goto error;
| */ | ||
|
|
||
| /* Simulate route handling from update_option() in options.c */ | ||
| if (is_update && strncmp(&line[i], "route ", 6) == 0) |
There was a problem hiding this comment.
is_update is always true in the this test, no need to check it, only if (!strncmp(&line[i], "route ", 6)) is fine.
| options->push_update_options_found |= OPT_P_U_ROUTE; | ||
| } | ||
| route_add_count++; | ||
| } |
There was a problem hiding this comment.
actually to make the tests work you should add the push-continuation logic otherwise the testdriver will fail.
if (!strncmp(&line[i], "route ", 6))
{
...
}
/* Simulate add_option() push-continuation logic */
else if (!strcmp(&line[i], "push-continuation 2"))
{
options->push_continuation = 2;
}
else if (!strcmp(&line[i], "push-continuation 1"))
{
options->push_continuation = 1;
}
Build using make check to run the tests
| * continuation message, causing routes to be reset on each message instead | ||
| * of accumulating. | ||
| * | ||
| * Expected behavior: routes should only be reset ONCE (when -route is received), |
There was a problem hiding this comment.
actually the intended behavior is to reset routes (or an option in general) the first time it is encountered. So even if no -route is present, but just route 10.1.0.0 255.255.0.0 for example, all the already existing routes should be deleted. So the comment should be modified accordingly and the -route option in the first message can actually be removed.
|
|
||
| /* Message 1: removal + first batch of routes, continuation 2 (more coming) */ | ||
| struct buffer buf1 = alloc_buf(512); | ||
| const char *msg1 = "PUSH_UPDATE,-route, route 10.1.0.0 255.255.0.0, route 10.2.0.0 255.255.0.0, route 10.3.0.0 255.255.0.0,push-continuation 2"; |
| assert_int_equal(route_add_count, 9); | ||
|
|
||
| /* | ||
| * BUG CHECK: Routes should only be "reset" once (for the entire update sequence). |
There was a problem hiding this comment.
how about just "We assure that route option is reset only one time in the first message if a push-continuation is present" or something like that? "Currently it will show route_reset_count == 3, exposing the bug." sounds strange to read once the commit is merged.
| msg(M_CLIENT, " Ex. push-update-broad \"route something, -dns\""); | ||
| msg(M_CLIENT, "push-update-cid CID options : Send an update message to the client identified by CID."); | ||
| msg(M_CLIENT, "reload-push-options [sync] : Reload push options from config file for new clients."); | ||
| msg(M_CLIENT, " With 'sync': also sync options to connected clients (add new, remove old)."); |
There was a problem hiding this comment.
As we are adding new commands, we may need to increment MANAGEMENT_VERSION in manage.h so that the client can determine whether the option is supported.
4 participants
Overview
This PR introduces a new management command
reload-push-optionsto allow dynamic updating of client configurations (routes, DNS, etc.) without restarting the server or disconnecting clients. It also includes a critical fix forPUSH_UPDATEhandling when messages are split across multiple chunks (continuation messages).1. Feature:
reload-push-optionsAdministrators can now trigger a reload of push options from the server configuration file via the management interface.
Usage
pushoptions from the config file. New clients connecting after this will receive the new options. Existing clients are unaffected.sync: Reloads options AND synchronizes them to currently connected clients.Sync Logic
When
syncis used, the server:push-update-broad "-route") for option types that have changed or been removed.PUSH_UPDATE.This allows for seamless updates of routing tables and network settings on live VPNs.
2. Bug Fix:
PUSH_UPDATEContinuation LogicThe Issue
Previously, the state tracking for which option types had been "reset" (e.g.,
OPT_P_U_ROUTE) was stored in a local variableupdate_options_foundwithinapply_push_options().If a
PUSH_UPDATEmessage was large enough to be split into multiple fragments (usingpush-continuation), this state was lost between fragments.The Fix
The state variable has been moved to
struct optionsaspush_update_options_found. This ensures the "reset state" persists across the entirePUSH_UPDATEsequence and is only cleared once the full sequence is processed.Testing
Integration Tests
Added a full docker-based integration test suite in
tests/reload_push_options/.tests/reload_push_options/run.shUnit Tests
test_incoming_push_continuation_route_accumulationtotest_push_update_msg.cto verify that route lists accumulate correctly across multiple message chunks.