Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auth: geoip - Fix static lookup when using weighted records on multiple record types #7219

Merged
merged 3 commits into from Mar 7, 2019

Conversation

Projects
None yet
5 participants
@Annih
Copy link
Contributor

Annih commented Nov 24, 2018

Short description

Computes the record weight/probability per QType and adapt the code to avoid empty answer in a mixed-weighted-Qtype situation.

Closes #7051.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@rgacogne rgacogne added the auth label Nov 26, 2018

rr.weight=static_cast<int>((static_cast<float>(rr.weight) / weight)*1000.0);
sum += rr.weight;
uint16_t rr_type = rr.qtype.getCode();
rr.weight=static_cast<int>((static_cast<float>(rr.weight) / weights[rr_type])*1000.0);

This comment has been minimized.

Copy link
@rgacogne

rgacogne Nov 26, 2018

Member

This was already the case with the existing code, but it looks like we would happily divide by 0.0 if the weight of all records for a given type is set to 0.

This comment has been minimized.

Copy link
@Annih

Annih Nov 26, 2018

Author Contributor

What would be the desired behavior in that case?
Maybe the yaml parser should just ignore records with a weight of 0?

Anyway it seems out of the scope of this review, what do you think @rgacogne?

This comment has been minimized.

Copy link
@rgacogne

rgacogne Nov 26, 2018

Member

I think the parser should error out on a weight of 0.

I agree it's a bit out of the scope so if you don't feel like fixing that in your PR just let me know and I'll either open a new PR or an issue so we don't forget to fix it.

This comment has been minimized.

Copy link
@Annih

Annih Nov 26, 2018

Author Contributor

OK I'm doing it in another review :) (#7227)

@Annih Annih referenced this pull request Nov 26, 2018

Merged

auth: geoip - forbid 0 as weight value #7227

4 of 7 tasks complete
@Annih

This comment has been minimized.

Copy link
Contributor Author

Annih commented Nov 28, 2018

@rgacogne I'm new in the pdns opensource world, in order to backport this change to the 4.1.x branch, what is the process?

I read I need first to wait that this is one is merged, but I still have some question:

  • Should I do the backport or a maintainer will do it?
  • Should all the fixes be backported, or only the main fix?
@Habbie

This comment has been minimized.

Copy link
Member

Habbie commented Nov 28, 2018

  • Should I do the backport or a maintainer will do it?

If you want it, it's best if you do it, instead of having to wait for us.

  • Should all the fixes be backported, or only the main fix?

We prefer minimal backports whenever possible.

@rgacogne
Copy link
Member

rgacogne left a comment

Looks good to me!

@rgacogne

This comment has been minimized.

Copy link
Member

rgacogne commented Nov 28, 2018

Perhaps @cmouse might want to review this before we merge it?

if (rr.has_weight) {
gl.netmask = (v6?128:32);
int comp = cumul_probability;
cumul_probability += rr.weight;
int comp = cumul_probabilities[rr.qtype.getCode()];

This comment has been minimized.

Copy link
@cmouse

cmouse Nov 28, 2018

Contributor

I am bit worried that QType::ANY will no longer work here.

This comment has been minimized.

Copy link
@Annih

Annih Nov 28, 2018

Author Contributor

Do you mean in the case where someone put a ANY record in the yaml? (I would say this is not valid)

Otherwise I would actually say that this patch fixes the behaviour with the QType::ANY. Before this patch a ANY request with mixed QType records would only return 1 record (or zero due to the bug in #7051)
i.e. as I tried to demonstrate with the new regression test, if you specify A and TXT values in a record, I would expect the ANY question to return 1 record for the A type and another for the TXT record.

This comment has been minimized.

Copy link
@cmouse

cmouse Nov 28, 2018

Contributor

I am wondering if this still makes sense, because now it can return "different" TXT-A pair if there are more than one specified.

Consider following example:

  • A 1.2.3.4, weight = 10
  • A 4.3.2.1, weight = 10
  • TXT "1-2-3-4", weight = 10
  • TXT "4-3-2-1", weight = 10

If I am reading this code correctly, it can end up returning four different kinds of results.

So, is the requirement to have corresponding records returned, or is it ok for them to behave independently of each other?

Seems that ANY query is handled correctly after all, I read it wrong the first round. But only question now remains is that is it OK that records are chosen randomly by type? If it's OK, then this works correctly.

This comment has been minimized.

Copy link
@Annih

Annih Nov 28, 2018

Author Contributor

Yes I confirm with your example the new implementation can return 4 different kinds of results. As I said this is the behavior I would expect to get, so IMHO this is OK and "better" than having only one QType returned. Now … who should decide? :P

This comment has been minimized.

Copy link
@Annih

Annih Dec 8, 2018

Author Contributor

@Habbie @rgacogne or @cmouse who should take this decision?

Could we move forward? I'll send the backport review once this is merged.

This comment has been minimized.

Copy link
@cmouse

cmouse Jan 25, 2019

Contributor

I think the result-set needs to contain one of TXT and one of A in this configuration to be correct.

This comment has been minimized.

Copy link
@Annih

Annih Jan 26, 2019

Author Contributor

That's what the new implementation produces; a single random record per QTYPE.

This comment has been minimized.

Copy link
@cmouse

cmouse Jan 26, 2019

Contributor

so can you paste what dig any name.geo.example.com gives?

This comment has been minimized.

Copy link
@Annih

Annih Jan 26, 2019

Author Contributor

Yes certainly I can :), this is exactly the behavior I tried to demonstrate in the added regression test with the ANY query, but due to the randomness of the result I had to simplify the data.

So with the a setup similar to what you described:

    name.geo.example.com:
      - a:
          content: 1.2.3.4
          weight: 10
      - a:
          content: 4.2.3.1
          weight: 10
      - txt:
          content: 1-2-3-4
          weight: 10
      - txt:
          content: 4-2-3-1
          weight: 10

Here are the results:

#1:
name.geo.example.com.   30      IN      TXT     "4-2-3-1"
name.geo.example.com.   30      IN      A       4.2.3.1
#2:
name.geo.example.com.   30      IN      TXT     "4-2-3-1"
name.geo.example.com.   30      IN      A       4.2.3.1
#3:
name.geo.example.com.   30      IN      TXT     "1-2-3-4"
name.geo.example.com.   30      IN      A       1.2.3.4
#4:
name.geo.example.com.   30      IN      TXT     "4-2-3-1"
name.geo.example.com.   30      IN      A       4.2.3.1
#5:
name.geo.example.com.   30      IN      TXT     "1-2-3-4"
name.geo.example.com.   30      IN      A       1.2.3.4

Interestingly the pairs are preserved, but I'm not sure this is a feature that should be advertised ^^

This comment has been minimized.

Copy link
@cmouse

cmouse Jan 28, 2019

Contributor

Well, if you use the random rnd for choosing value, it's no surprise. but yeah, I'm ok now.

weight: 10
- txt:
content: text
weight: 10

This comment has been minimized.

Copy link
@cmouse

cmouse Nov 28, 2018

Contributor

this test does not really test the functionality

This comment has been minimized.

Copy link
@Annih

Annih Nov 28, 2018

Author Contributor

Could you be more specific on the "functionality" which should be tested and which is not?

Is there something unclear in the commit message where I introduce the test? If yes, how should I reword it to make it clearer?

My goal is to demonstrate we do not regress/retrigger #7051, and ensure that the ANY question answer both records, the A question return the A record and the the TXT return the TXT.
Do you think my test is covering this goal?
Do you think my test is enough?

This comment has been minimized.

Copy link
@cmouse

cmouse Nov 28, 2018

Contributor

although not sure if it's even possible to test it well.

Annih added some commits Nov 24, 2018

auth: geoip, add regression test for issue #7051
The added test does not validate that the weight system is working in
a pseudo-random way. It use a single entry per QType to ensure that
they are all queryable individually or via a ANY question.

The ANY test is done using TCP to avoid test failure due to exceeded
buffer size.
auth: geoip, compute weight per QType
As seen in #7051 the former behavior of the geoip backend in mixed
weighted QType cases, was erroneous.
Having per QType weights allows proper static lookup.
It also enables to question with ANY and retrieve one record per QType.

Closes #7051
auth: geoip, check weight only on matching QTypes
This is a micro-optimization to avoid storing & computing probabilities
of non matching QTypes.

@Annih Annih force-pushed the Annih:auth-issue-7051 branch from cbb3953 to d129860 Jan 25, 2019

@Annih

This comment has been minimized.

Copy link
Contributor Author

Annih commented Jan 25, 2019

@rgacogne & @cmouse any thought about this?
I know you are working hard on 4.2.0, it would be great if this change could be included in the release.

@aerique

This comment has been minimized.

Copy link
Member

aerique commented Feb 19, 2019

@rgacogne do you still approve and @cmouse do you approve?

@Annih

This comment has been minimized.

Copy link
Contributor Author

Annih commented Mar 6, 2019

Thanks @aerique for the reminder, it has been a while that this PR didn't move :)
@cmouse could you please tell us if you approve or not? If you don't, could you drive me on the right path to fix this patch and move forward?

@cmouse

This comment has been minimized.

Copy link
Contributor

cmouse commented Mar 6, 2019

i already wrote in earlier code that i approve, but lets approve here too

@Annih

This comment has been minimized.

Copy link
Contributor Author

Annih commented Mar 6, 2019

Thanks @cmouse :)
So what is required to merge this, who is allowed to do it?

@aerique

This comment has been minimized.

Copy link
Member

aerique commented Mar 6, 2019

So what is required to merge this, who is allowed to do it?

I or one of the other PowerDNS people will merge it in the coming days.

@rgacogne rgacogne merged commit ac36a2c into PowerDNS:master Mar 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.