It's not that the issue itself needs a test. AskDNS and/or tag handling currently have no tests at all. In addition there's some open bugs looking at the ways to use AskDNS and it's shortcomings (empty tags, meta tests breaking on askdns etc). TLDR: It actually requires a lot of work, don't have time or stamina right now.
On Thu, Nov 28, 2019 at 05:33:03AM -0500, Kevin A. McGrail wrote: > I never would have found that. Nice job. > > Any chance you can create a regression test for the issue ? > > On Thu, Nov 28, 2019, 05:09 Henrik K <[1]h...@hege.li> wrote: > > > Fixed: > [2]http://svn.apache.org/viewvc?view=revision&revision=1870552 > > On Thu, Nov 28, 2019 at 11:29:19AM +0200, Henrik K wrote: > > > > Trunk has already many revamps and fixes for these codes, works there. > It > > seems 3.4 askdns has trouble using those, investigating minimal fix.. > > > > > > On Thu, Nov 28, 2019 at 10:20:39AM +0100, Tobi <[3]jahli...@gmx.ch> > wrote: > > > Henrik > > > > > > But my current testing clearly shows that without the explicit set_tag > > > _LASTEXTERNALRDNS_ and _LASTEXTERNALHELO_ won't be set. > > > I'm testing this using spamassassin in debug mode to scan a > testmessage. > > > As soon as I re-add the set_tag to PerMsgStatus.pm the tags are set > and > > > tests based on that tags are run. Removing the lines from pm and debug > > > shows that the tests are **not** run anymore. > > > > > > So I somewhat doubt that the set_tag "code is redundant and should be > > > removed" :-) > > > > > > Using: SA 3.4.2 on centos7 / perl 5.16.3 > > > > > > > > > Cheers > > > > > > tobi > > > > > > Am 28.11.19 um 08:36 schrieb Henrik K: > > > > > > > > There is nothing missing. All the LASTEXT* tags are already > dynamically > > > > returning functions. If anything, that if ($lasthop) set_tag code > is > > > > completely redundant and should be removed. > > > > > > > > > > > > BEGIN { > > > > LASTEXTERNALIP => sub { > > > > my $pms = shift; > > > > my $lasthop = $pms->{relays_external}->[0]; > > > > $lasthop ? $lasthop->{ip} : ''; > > > > }, > > > > > > > > LASTEXTERNALRDNS => sub { > > > > my $pms = shift; > > > > my $lasthop = $pms->{relays_external}->[0]; > > > > $lasthop ? $lasthop->{rdns} : ''; > > > > }, > > > > > > > > LASTEXTERNALHELO => sub { > > > > my $pms = shift; > > > > my $lasthop = $pms->{relays_external}->[0]; > > > > $lasthop ? $lasthop->{helo} : ''; > > > > }, > > > > > > > > > > > > On Wed, Nov 27, 2019 at 10:18:20AM -0500, Kevin A. McGrail wrote: > > > >> After a 10 minute or so study of the issue and comparing 3.4 and > trunk, > > > >> it definitely looks like the code is missing. I am not 100% sure > your > > > >> fix works but it's better than it currently exists :-) > > > >> > > > >> Have you been using that change in production? > > > >> > > > >> Regards, > > > >> > > > >> KAM > > > >> > > > >> On 11/27/2019 6:59 AM, Tobi <[4]jahli...@gmx.ch> wrote: > > > >>> Hi, > > > >>> > > > >>> is there any specific reason why the two tags mentioned in subject > are > > > >>> not set in SA? It took me a while to find out why an askdns test > was not > > > >>> running. The test relies on _LASTEXTERNALRDNS_ but after running > with > > > >>> --debug I found that those tags are not set by SA. Although > > > >>> _LASTEXTERNALIP_ is set. Also the man page states that the two > tags > > > >>> should exist. > > > >>> > > > >>> In PerMsgStatus.pm I saw that everything is prepared for the two > tags > > > >>> but they finally not set (around line 1721). So I changed to > > > >>> > > > >>>> if ($lasthop) { > > > >>>> $self->set_tag('LASTEXTERNALIP', $lasthop->{ip}); > > > >>>> $self->set_tag('LASTEXTERNALRDNS', $lasthop->{rdns}); > > > >>>> $self->set_tag('LASTEXTERNALHELO', $lasthop->{helo}); > > > >>>> } > > > >>> > > > >>> and --debug shows that the tags are now set and the askdns test is > run. > > > >>> > > > >>> So I wonder if the code has just been forgotten or if there are > deeper > > > >>> reasons to not set the tags? > > > >>> If no deeper reasons exist it would be nice to have those two tags > set > > > >>> as default in PerMsgStatus.pm > > > >>> > > > >>> > > > >>> Cheers > > > >>> > > > >>> -- > > > >>> > > > >>> tobi > > > >> > > > >> -- > > > >> Kevin A. McGrail > > > >> kmcgr...@apache.org > > > >> > > > >> Member, Apache Software Foundation > > > >> Chair Emeritus Apache SpamAssassin Project > > > >> [5]https://www.linkedin.com/in/kmcgrail - 703.798.0171 > > > > > > > References: > > [1] mailto:h...@hege.li > [2] http://svn.apache.org/viewvc?view=revision&revision=1870552 > [3] mailto:jahli...@gmx.ch > [4] mailto:jahli...@gmx.ch > [5] https://www.linkedin.com/in/kmcgrail
