| Message ID | 20210825210232.22509-2-selva.nair@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,v2,1/3] Add remote-count and remote-entry query via management | expand |
Hi, While implementing this feature in the GUI, I have come across some limitations of management-query-remote: (i) Using a remote selected by the user from a list requires skipping several remotes and accepting when the right one is reached. This is very slow when there are more than a dozen remote entries: about ~1 second per command. Solution: Extend "remote SKIP" to take an optional parameter to skip "n" entries by one command. Use of "remote MOD" is not useful here as it does not support change of protocol. It also has some "misbehaviour" due to the way resolved addresses are cached. (ii) remote-entry-get introduced here needs to be extended to support requesting multiple or all remote entries in one command. Please ignore the current version. v3 is coming. Selva <div dir="ltr"><div dir="ltr">Hi,</div><div dir="ltr"><br></div><div>While implementing this feature in the GUI, I have come across some limitations of management-query-remote:</div><div><br></div><div>(i) Using a remote selected by the user from a list requires skipping several remotes and accepting when the right one is reached. This is very slow when there are more than a dozen remote entries: about ~1 second per command.</div><div><br></div><div>Solution: Extend "remote SKIP" to take an optional parameter to skip "n" entries by one command.</div><div><br></div><div>Use of "remote MOD" is not useful here as it does not support change of protocol. It also has some "misbehaviour" due to the way resolved addresses are cached.</div><div><br></div><div>(ii) remote-entry-get introduced here needs to be extended to support requesting multiple or all remote entries in one command.</div><div><br></div><div>Please ignore the current version. v3 is coming.</div><div><br></div><div>Selva</div></div>
Hi, As this patch set is not getting any attention, I guess the need for this is not clear enough from the commit messages. To be blunt, --management-query-remote is pretty useless the way it stands. Let me explain: (1) It queries the user with one remote at a time taking about 1 second per query. With 100 remotes, if the user wants to select the 100th one, a UI has to show 100 popups and take ~100 seconds. So one wants two things in good UI (i) show a list of all available remotes to the user (ii) have a way to jump to a remote that user chooses For (i) one could scan the config and make a list but I'll come to why that doesn't work in a minute. Consider (ii) first: we currently have an option to skip remotes one by one or send a MOD request where the host and port can be changed. Skipping one by one is very slow, and MOD doesn't allow change of protocol. So, if the 100th item that user chooses is TCP and all other 99 are UDP, we have to wait until we get prompted with the right one. There are other reasons why MOD doesn't work but I want to keep this mail simple. Thus, we want a way to skip "N" remotes by one command (say skip 99, get prompted with the 100th which we accept). One of the three patches in this set does that. Now back to (i) about making a list. For "SKIP N" to work, the UI and the core must have an identical view of the list -- especially the order of entries. If remote-random is in use, the only way to assure that is by making the list by querying the core. Even otherwise independently parsing the list from the config is not the right approach when identity of content is important. This, hopefully, justifies the patch that allows the UI to query the core for a list of remotes and their indices. The third patch is to relax the restriction on the number of remote entries and connection lists. Currently it's hard coded to 64, there is no reason why we can't allow 100 or even 1000 remote entries. From experience, I know that patches that languish stay like that for months if not years. Please, either review and ACK or hate it at first sight and NAK. Or ask for clarification. A patch for the GUI is waiting for the fate of this. Thanks, Selva On Tue, Sep 7, 2021 at 6:27 PM Selva Nair <selva.nair@gmail.com> wrote: > Hi, > > While implementing this feature in the GUI, I have come across some > limitations of management-query-remote: > > (i) Using a remote selected by the user from a list requires skipping > several remotes and accepting when the right one is reached. This is very > slow when there are more than a dozen remote entries: about ~1 second per > command. > > Solution: Extend "remote SKIP" to take an optional parameter to skip "n" > entries by one command. > > Use of "remote MOD" is not useful here as it does not support change of > protocol. It also has some "misbehaviour" due to the way resolved addresses > are cached. > > (ii) remote-entry-get introduced here needs to be extended to support > requesting multiple or all remote entries in one command. > > Please ignore the current version. v3 is coming. > > Selva > <div dir="ltr">Hi,<div><br></div><div>As this patch set is not getting any attention, I guess the need for this is not clear enough from the commit messages. To be blunt, --management-query-remote is pretty useless the way it stands. Let me explain:</div><div><br></div><div>(1) It queries the user with one remote at a time taking about 1 second per query. With 100 remotes, if the user wants to select the 100th one, a UI has to show 100 popups and take ~100 seconds.</div><div><br></div><div>So one wants two things in good UI </div><div>(i) show a list of all available remotes to the user</div><div>(ii) have a way to jump to a remote that user chooses </div><div><br></div><div>For (i) one could scan the config and make a list but I'll come to why that doesn't work in a minute. Consider (ii) first: we currently have an option to skip remotes one by one or send a MOD request where the host and port can be changed. Skipping one by one is very slow, and MOD doesn't allow change of protocol. So, if the 100th item that user chooses is TCP and all other 99 are UDP, we have to wait until we get prompted with the right one. There are other reasons why MOD doesn't work but I want to keep this mail simple.</div><div><br></div><div>Thus, we want a way to skip "N" remotes by one command (say skip 99, get prompted with the 100th which we accept). One of the three patches in this set does that.</div><div><br></div><div>Now back to (i) about making a list. For "SKIP N" to work, the UI and the core must have an identical view of the list -- especially the order of entries. If remote-random is in use, the only way to assure that is by making the list by querying the core. Even otherwise independently parsing the list from the config is not the right approach when identity of content is important.</div><div><br></div><div>This, hopefully, justifies the patch that allows the UI to query the core for a list of remotes and their indices.</div><div><br></div><div>The third patch is to relax the restriction on the number of remote entries and connection lists. Currently it's hard coded to 64, there is no reason why we can't allow 100 or even 1000 remote entries.</div><div><br></div><div>From experience, I know that patches that languish stay like that for months if not years. Please, either review and ACK or hate it at first sight and NAK. Or ask for clarification. A patch for the GUI is waiting for the fate of this.</div><div><br></div><div>Thanks,</div><div><br>Selva</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 7, 2021 at 6:27 PM Selva Nair <<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">Hi,</div><div dir="ltr"><br></div><div>While implementing this feature in the GUI, I have come across some limitations of management-query-remote:</div><div><br></div><div>(i) Using a remote selected by the user from a list requires skipping several remotes and accepting when the right one is reached. This is very slow when there are more than a dozen remote entries: about ~1 second per command.</div><div><br></div><div>Solution: Extend "remote SKIP" to take an optional parameter to skip "n" entries by one command.</div><div><br></div><div>Use of "remote MOD" is not useful here as it does not support change of protocol. It also has some "misbehaviour" due to the way resolved addresses are cached.</div><div><br></div><div>(ii) remote-entry-get introduced here needs to be extended to support requesting multiple or all remote entries in one command.</div><div><br></div><div>Please ignore the current version. v3 is coming.</div><div><br></div><div>Selva</div></div> </blockquote></div>
diff --git a/Changes.rst b/Changes.rst index e5ac8098..6e3c535e 100644 --- a/Changes.rst +++ b/Changes.rst @@ -4,6 +4,8 @@ Overview of changes in 2.6 New features ------------ +Support unlimited number of connection entries and remote entries + New management commands to enumerate and list remote entries Use ``remote-entry-count`` and ``remote-entry-get index`` commands from the management interface to get the number of diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 0e398c0f..5d0aa8af 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -889,6 +889,14 @@ init_options(struct options *o, const bool init_gc) void uninit_options(struct options *o) { + if (o->connection_list) { + free(o->connection_list->array); + CLEAR(*o->connection_list); + } + if (o->remote_list) { + free(o->remote_list->array); + CLEAR(*o->remote_list); + } if (o->gc_owned) { gc_free(&o->gc); @@ -1947,10 +1955,17 @@ alloc_connection_entry(struct options *options, const int msglevel) struct connection_list *l = alloc_connection_list_if_undef(options); struct connection_entry *e; - if (l->len >= CONNECTION_LIST_SIZE) + if (l->len == l->capacity) { - msg(msglevel, "Maximum number of 'connection' options (%d) exceeded", CONNECTION_LIST_SIZE); - return NULL; + int capacity = l->capacity + CONNECTION_LIST_SIZE; + struct connection_entry **ce = realloc(l->array, capacity*sizeof(struct connection_entry *)); + if (ce == NULL) + { + msg(msglevel, "Unable to process more connection options: out of memory. Number of entries = %d", l->len); + return NULL; + } + l->array = ce; + l->capacity = capacity; } ALLOC_OBJ_GC(e, struct connection_entry, &options->gc); l->array[l->len++] = e; @@ -1973,10 +1988,17 @@ alloc_remote_entry(struct options *options, const int msglevel) struct remote_list *l = alloc_remote_list_if_undef(options); struct remote_entry *e; - if (l->len >= CONNECTION_LIST_SIZE) + if (l->len == l->capacity) { - msg(msglevel, "Maximum number of 'remote' options (%d) exceeded", CONNECTION_LIST_SIZE); - return NULL; + int capacity = l->capacity + CONNECTION_LIST_SIZE; + struct remote_entry **re = realloc(l->array, capacity*sizeof(struct remote_entry *)); + if (re == NULL) + { + msg(msglevel, "Unable to process more remote options: out of memory. Number of entries = %d", l->len); + return NULL; + } + l->array = re; + l->capacity = capacity; } ALLOC_OBJ_GC(e, struct remote_entry, &options->gc); l->array[l->len++] = e; diff --git a/src/openvpn/options.h b/src/openvpn/options.h index b0e40cb7..98977d41 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -168,15 +168,17 @@ struct remote_entry struct connection_list { + int capacity; int len; int current; - struct connection_entry *array[CONNECTION_LIST_SIZE]; + struct connection_entry **array; }; struct remote_list { + int capacity; int len; - struct remote_entry *array[CONNECTION_LIST_SIZE]; + struct remote_entry **array; }; enum vlan_acceptable_frames