openvpn icon indicating copy to clipboard operation
openvpn copied to clipboard

Management: add `reload-push-options` & Fix `PUSH_UPDATE` logic

Open maurice2k opened this issue 1 month ago • 11 comments

Overview

This PR introduces a new management command reload-push-options to allow dynamic updating of client configurations (routes, DNS, etc.) without restarting the server or disconnecting clients. It also includes a critical fix for PUSH_UPDATE handling when messages are split across multiple chunks (continuation messages).

1. Feature: reload-push-options

Administrators can now trigger a reload of push options from the server configuration file via the management interface.

Usage

  • Without arguments: Reloads push options from the config file. New clients connecting after this will receive the new options. Existing clients are unaffected.
  • With update-clients: Reloads options AND synchronizes them to currently connected clients.

Update Logic

When update-clients is used, the server:

  1. Parses the new configuration.
  2. Calculates the difference between the old and new push lists.
  3. Sends removal commands (e.g., -route) for option types that have changed or been removed.
  4. Sends the new options via PUSH_UPDATE.

This allows for seamless updates of routing tables and network settings on live VPNs.

2. Bug Fix: PUSH_UPDATE Continuation Logic

The Issue

Previously, the state tracking for which option types had been "reset" (e.g., OPT_P_U_ROUTE) was stored in a local variable update_options_found within apply_push_options().

If a PUSH_UPDATE message was large enough to be split into multiple fragments (using push-continuation), this state was lost between fragments.

  • Result: The route table (or other list options) would be reset for every fragment that contained a route, rather than once for the entire update sequence.
  • Symptom: Large route lists would fail to apply correctly, often resulting in only the last chunk's routes being present, or significant churn/flickering of routes during the update.

The Fix

The state variable has been moved to struct options as push_update_options_found. This ensures the "reset state" persists across the entire PUSH_UPDATE sequence and is only cleared once the full sequence is processed.

Testing

Integration Tests

Added a Docker-based integration test suite in tests/reload_push_options/.

  • Coverage:
    • Basic reload (no update-clients).
    • Adding routes with update-clients.
    • Removing routes.
    • Replacing all routes.
    • Stress Test: Verifies handling of 500+ routes (which triggers the continuation logic).
  • How to run: tests/reload_push_options/run.sh

Unit Tests

  • Added test_incoming_push_continuation_route_accumulation to test_push_update_msg.c to verify that route lists accumulate correctly across multiple message chunks.

maurice2k avatar Nov 27 '25 00:11 maurice2k

Thanks for working on this, and thanks for sending all the good stuff to the openvpn-devel list right away.

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?

cron2 avatar Nov 27 '25 11:11 cron2

Yes, I will review asap

mrbff avatar Nov 27 '25 11:11 mrbff

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.

maurice2k avatar Nov 27 '25 13:11 maurice2k

regarding the management command:

reload-push-options [sync]

is the wording "sync" ok or should i change it to "broadcast" because there's "push-update-broad"?

maurice2k avatar Dec 01 '25 10:12 maurice2k

broadcast does not sound really intuitive. Maybe reload-push-options update-clients instead?

cron2 avatar Dec 01 '25 10:12 cron2

Changed the naming, adjusted everything mentioned in the comments. Anything else? Any chance for 2.7.0?

maurice2k avatar Dec 01 '25 12:12 maurice2k

The end result should be "one bugfix commit" (squashing all parts that are "bugfix" related), and "one feature commit" (new feature, with the 'sync' rename).

Bugfix can - and should - go into 2.7.0, "new feature" needs more review and (likely) more discussion.

cron2 avatar Dec 01 '25 13:12 cron2

ok @cron2, i'm opening a new PR here for bugfix only and send the diff to the mailing list

maurice2k avatar Dec 01 '25 13:12 maurice2k

@maurice2k please do not open a new pr. You can update/force push to this PR to update it.

schwabe avatar Dec 01 '25 16:12 schwabe

adjusted it here, merged the two patches (feature + bugfix) back into two commits. i made a new PR just for the bugfix (https://github.com/OpenVPN/openvpn/pull/925).

should i remove the bugfix commit from this PR? or remove the other PR?

maurice2k avatar Dec 01 '25 23:12 maurice2k

Final patches have to be submitted to the mailing list. Once they are there, PRs on GitHub are not that useful anymore (they are good for a preliminary review, but nothing else) and can be closed.

So it doesn't matter how PRs are structured here, as long as they get the needed attention and patches are then submitted to the openvpn-devel mailing list.

ordex avatar Dec 02 '25 08:12 ordex