On Wed, Dec 4, 2019 at 5:05 AM Joshua C. Colp <jc...@sangoma.com> wrote:
> On Wed, Dec 4, 2019 at 5:05 AM Jaco Kroon <j...@uls.co.za> wrote: > >> Hi All, >> >> In ast_apply_acl (main/acl.c) there is two lines that's issuing a >> LOG_WARNING when an ACL gets denied. >> >> The first happens if the ACL is invalid. I'm not too worried about this >> specific one, it's probably a good thing if this gets logged always. >> >> The latter, in the case of AST_SENSE_DENY is a bit problematic for me. >> I've submitted patches now to use ACLs in res_rtp_asterisk, and with large >> number of rejects this can quickly spam the logs, and frankly, confuse >> consumers. >> >> As I see it, there are two possible solutions: >> >> Solution 1: >> >> 1. Add AST_SENSE_INVALID as a possible return. >> 2. Rename the current function to ast_apply_acl_(silent|nolog), and >> remove the logging. >> 3. Add a replacement ast_apply_acl function which will generate the log >> entries as per current. >> >> Solution 2: >> >> Simply don't log at all if the purpose argument is NULL. >> >> Solution two is the simpler fix, but it's probably also the less ideal >> one. >> >> The adding of the AST_SENSE_INVALID will also mean that the replacement >> function will need to rewrite AST_SENSE_INVALID => AST_SENSE_DENY, or we >> need to audit all consumers of the function (there fortunately isn't a >> great many of these) and wherever ast_apply_acl(...) == AST_SENSE_DENY is >> found, it should be rewritten as ast_apply_acl(...) != AST_SENSE_ALLOW. >> >> Would dearly like some opinions on the matter. >> >> PS: The advantage for me on using ACL over HA is simply the named ACL >> functionality, so in rtp.conf I can state ice_acl = named_acl instead of >> having to embed the ACL into rtp.conf >> > > I would prefer #1 however the existing function should behave as it does > now. Each consumer should not need to be touched, unless they are to be > switched to silent. We have an obligation to maintain ABI and behavior of > API functions as best we can in case there are any outside consumers as > well. > Unfortunately due to how consumer possibly use of the current return value I think the only way to handle this is with #2. Or well by adding another function ast_apply_acl_silent, and removing the logging that is. Your new changes could call this function, and nothing else would be potentially affected. -- Kevin Harwell Senior Software Developer Sangoma Technologies Check us out at: https://sangoma.com & https://asterisk.org
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev