[asterisk-dev] Methodologies for validating dialplan

[asterisk at phreaknet.org]

On 1/5/2022 6:06 PM, Mark Murawski wrote:
> Hi!
>
> Throwing my .02 in here.  Adjust to .10 for inflation!
>
>
> On 1/4/22 14:53, asterisk at phreaknet.org wrote:
>> .... However, there are a lot of dialplan problems that represent 
>> potentially valid syntax that will cause an error at runtime, such as 
>> branching to somewhere that doesn't exist. The dialplan will reload 
>> with no errors, since there isn't a syntax issue, but at runtime, the 
>> call will fail (and most likely crash). I found over the years that a 
>> lot of these were often simple typos or issues that were easily fixed 
>> but wasted a lot of time in finding solely in the "test, test, test" 
>> approach. Another common grievance I hear time to time about the 
>> dialplan is most issues are caught at runtime, not "compile time" 
>> (i.e. dialplan reload).
>
> I love it!
>
> And... take an example from a basic cli call that has a validator 
> built-in:
>
> static char *meetme_kick_cmd(struct ast_cli_entry *e, int cmd, struct 
> ast_cli_args *a)
> {
>         switch (cmd) {
>         case CLI_INIT:
>                 e->command = "meetme kick";
>                 e->usage =
>                         "Usage: meetme kick <confno> all|<userno>\n"
>                         "       Kick a conference or a user in a 
> conference.\n";
>                 return NULL;
>         case CLI_GENERATE:
>                 return complete_meetmecmd_mute_kick(a->line, a->word, 
> a->pos, a->n);
>         }
>
>         if (a->argc != 4) {
>                 return CLI_SHOWUSAGE;
>         }
>
>         return meetme_cmd_helper(a);
> }
>
>
> This design pattern has self-contained validation.  And it's up to the 
> function to validate as much or as little as it wants to.
>
> We would need a new ast_register_application_xml2 and 
> ast_custom_function_register2 to have a new design pattern for 
> self-validating functions and applications.
>
> Each application would know what kind of parameters it takes, and 
> what's valid and not valid
>
> new-stle for say... Goto Application
>
> static int gosub_exec(int cmd, struct ast_channel *chan, const char 
> *data)
> {
>     if (cmd == APP_PARAMS_VALIDATE)
>         ... do some validation!
>
> ideally gosub knows it's being passed an exten/context/priority and 
> can call ast_exists_extension at dialplan parse time via the validator 
> flow

I think your line of thinking here makes a lot of sense. However, I 
don't think it makes sense to do validation in the actual execution 
function like CLI handlers, as you've shown.

I think an enhanced application (and function) register function would 
make sense, accepting another function to do the validation, e.g.:

ast_register_application_xml_validate(app, app_exec, app_validate);

Some of these validation functions can be quite involved, and separating 
them out that way makes more sense.

I slept on this overnight, and here are some other thoughts:

The current approach on the review is to have an external module 
(currently res_pbx_analyze) use core PBX APIs to crawl the dialplan and 
execute callbacks to find fallthroughs and missing audio files. 
Obviously, there are a lot of other things we could look for as well. 
Currently, my approach is looking for "types" of problems, rather than 
analyzing specific applications, but now that I think about it, the 
latter is really a subset of the former. Each application really has 
*one* type of validation that it's doing... Goto/Gosub etc. will check 
the location... MusicOnHold will check the MOH class... 
Playback/Background/Read will check the audio file... Voicemail would 
check the voicemail box, and so on... Dial would validate that a 
registered channel tech is being used, etc. (validating that any options 
use also exist could also be another component).

Some of these naturally fall into groups, but a good way to deal with 
this might be CLI aliases, e.g.:

dialplan analyze fallthrough => dialplan analyze Goto, dialplan analyze 
GotoIf, dialplan analyze Gosub, dialplan analyze GosubIf

dialplan analyze audio => dialplan analyze Playback, dialplan analyze 
ControlPlayback, dialplan analyze BackGround, dialplan analyze Read

Obviously, the validation functions for some of these are going to be 
almost identical. To deal with code duplication, each validation 
function could call the same function within the core, even though the 
entry point would be different.

Of course, for registering applications in the same module (like Goto 
and GotoIf), the same validation function could directly be use for 
both. We'd want to use the same function though from within app_stack 
for Gosub/GosubIf, most likely, so maybe for rare cases of multiple 
modules sharing validation functions, those could go in a core file like 
pbx_validate that contains common-to-many-modules validation functions. 
The vast majority of modules that do this, though, I expect, would have 
their own validation function within the module.

(By the way, I'm not set on dialplan analyze... still debating whether 
that or dialplan inspect or dialplan validate would make the most sense...)

Another reason I think doing validation in module is a better approach. 
For one, not all the internals of modules are exposed outside of it. For 
instance, say we want to verify that MusicOnHold() and 
StartMusicOnHold(), if they contain an argument, contain the name of a 
valid in-memory MOH class. This is only exposed within res_tonedetect 
currently. To do validation in another module would mean that needs to 
be exposed somehow. Obviously, this can get very messy.

If it's done in-module, then it just works out cleanly. So, we could do 
away with a dedicated external module and simply register validation 
functions from within particular modules.

> This would be a major undertaking to add validation for every function 
> and every application but since dialplan is so ubiquitous and is 
> probably never going away... would make huge inroads to making sure 
> everything is as correct as possible at parse/load time.

I don't really see this as something that would be done for every single 
module necessarily, but some that come to mind that could benefit:

Goto, GotoIf, Gosub, GosubIf, ExecIf, Verbose, Log, Dial, VoiceMail, 
ConfBridge, Playback, Read, BackGround, etc.

> There needs to be an externally-facing way to call 
> function/application validators for those of us who write dynamic 
> dialplan generators.  Maybe an AMI command?
Considering it would just be executing the registered validation 
function for each app, function, etc. should be doable.

At the moment, I'd envisioned it mainly be CLI commands, first and 
foremost, since that goes along nicely with dialplan reload. And, as I 
said above, there could be CLI aliases for each "group" of validation 
functions that naturally go well together, to find specific categories 
of errors. There could also be a "dialplan analyze all" to run all 
registered validation functions and report any problems.

> Strict: false    <-- allow for generic 'does it look okay' testing vs 
> does the destination actually exist 
I'm with you until here... this I'm not sure I understand the point as 
much of the "generic" validation. If we're not validating the 
destination actually exists, in this case, then we're not really doing 
much at all. At that point, it's basically counting commas and reporting 
whether there's no more than 2.
> And then more of a full validator via a new application and AMI Command
> ValidateDialplanFile(/tmp/new_extensions.conf,/tmp/path_to_errors.log); 

As far as specific error logging, currently I'm just throwing WARNINGs 
that would get sent out to all the relevant consoles and log files. A 
simple grep might be able to filter all of those out.