[asterisk-dev] Methodologies for validating dialplan

[Jaco Kroon]

Hi,

I find this discussion very interesting.  I do have one question, and
this was implied by another poster:

How do you intend to deal with dynamic stuff, for example:

exten => ??,n,Goto(foo-${bar})

In this case, what's the possible values for ${bar}?

Would it make sense to enhance the dialplan execution system to
basically collect error points?  For example, in the above, an entry to
the "failure" list could be added providing the context, ${EXTEN} and
values of expanded variables.  And number of times this has "hit", so
assuming the above is in context foobar, and ?? is something like _0Z.
and the actual value of exten was 0111, and bar=fneh, then the entry
could store:

{
   execution_point: foobar,_0Z.,n
   EXTEN=0111
   bar=fneh
}

This could be hashed in some form such that a hit counter for the tuple
can be created. Then something like "diaplan show failure points".  Not
sure it makes sense to log all variables, but it could, and possibly the
dialplan execution leading up to the failure, so perhaps just the call
id (C-??????) so that it can be looked up in the verbose logs (which we
do log as part of full, even if we only keep it for 72 hours).

Special handing for Hangup() and others that are expected to hangup the
channel could be made (I don't recall if there are different return
codes from functions/applications for indicating failure vs continue vs
hangup etc ...

This is a bulk catch-all, and can possibly get stats about dynamic stuff
failing too.  A static analyzer would still be great to catch obvious
failure cases such as Goto(invalid) at load time.

An API to add warnings/non-fatal errors from applications/functions
would be great too (eg, MOH not found in some cases won't cause a
failure, just silence instead of moh, but if there is an API then in
such a case an entry could still be logged, ideally then with an extra
variable such as message="MOH ${moh_class} wasn't found").

Kind Regards,
Jaco

On 2022/01/06 16:24, asterisk at phreaknet.org wrote:

> 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.
>
>