rec: correct appliedPolicyTrigger value for IP matches #10842
Conversation
…ules (ns, client, response). The NetMaskTree has it.
pdns/filterpo.cc
Outdated
| key = fnd->first; | ||
| pol = fnd->second; |
There was a problem hiding this comment.
Would it make sense to update the d_trigger field of the returned policy here, instead of exposing the internal key?
| key = fnd->first; | |
| pol = fnd->second; | |
| const auto& key = fnd->first; | |
| pol = fnd->second; | |
| pol.d_trigger = Zone::maskToRPZ(key); |
There was a problem hiding this comment.
Yeah, that is a good idea.
| DNSFilterEngine::Policy zonePolicy; | ||
| BOOST_CHECK(zone->findNSIPPolicy(nsIP, zonePolicy)); | ||
| BOOST_CHECK(zone->findNSIPPolicy(nsIP, key, zonePolicy)); | ||
| BOOST_CHECK(key == nsIP); |
There was a problem hiding this comment.
It looks like all our tests are using an exact match (/32), perhaps we should test with at least one different mask? I know we do in the regression tests, though.
There was a problem hiding this comment.
Extended the tests and found another case where the trigger value was not set properly: findExactNSPolicy.
Now I'm wondering if d_hit should be set in the find methods as well. Plus looking at the assignments to d_trigger in getQueryPolicy. They look redundant now.
In a few cases (wildcard processing) the matched value is not the hit as seen by the find function and an overide is needed.
pdns/filterpo.cc
Outdated
| @@ -212,12 +222,9 @@ bool DNSFilterEngine::getProcessingPolicy(const ComboAddress& address, const std | |||
| continue; | |||
| } | |||
|
|
|||
| Netmask key; | |||
pdns/filterpo.cc
Outdated
| @@ -236,6 +243,7 @@ bool DNSFilterEngine::getClientPolicy(const ComboAddress& ca, const std::unorder | |||
| continue; | |||
| } | |||
|
|
|||
| Netmask key; | |||
pdns/filterpo.cc
Outdated
| @@ -355,10 +361,8 @@ bool DNSFilterEngine::getPostPolicy(const DNSRecord& record, const std::unordere | |||
| return false; | |||
| } | |||
|
|
|||
| Netmask key; | |||
pdns/filterpo.cc
Outdated
| return true; | ||
| } | ||
|
|
||
| for (const auto& wc : wcNames) { | ||
| if (z->findExactQNamePolicy(wc, pol)) { | ||
| // cerr<<"Had a hit on the name of the query"<<endl; | ||
| pol.d_trigger = wc; | ||
| // Hit is not arg to findExactQNamePolicy! |
There was a problem hiding this comment.
| // Hit is not arg to findExactQNamePolicy! | |
| // Hit is not the wildcard passed to findExactQNamePolicy but the actual qname! |
Short description
The key of the
NetmaskTreecontains the Netmask that matched and we can construct the trigger from it.Checklist
I have: