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

Restrict value range for weight parameter, avoid sum overflows dropping queries #6448

Merged
merged 1 commit into from Apr 6, 2018

Conversation

Projects
None yet
2 participants
@dmccombs
Contributor

dmccombs commented Apr 5, 2018

Short description

This restricts weight values on downstreams and caps the calculated sum of all weights to avoid the following issues:

  • Sum of all downstream weights combined exceeding the max value of int, causing queries to be dropped with no-policy
  • Negative values on some downstreams causing a sum of all downstream weights to be 0 or less, causing queries to be dropped with no-policy
  • Negative value weight preventing any queries from going to a downstream

Testing

  • Run regression tests as described in regression-tests.dnsdist/README, particularly test_Routing.py
  • Manually try adding downstreams with weights that are negative or greater than 2147483647 and observe the logged error and that they are not added, similar behavior to master when adding a bad address
  • Add multiple downstreams (with different address/port combinations) that have weights whose sum is greater than 2147483647, and note that no queries are dropped as no-policy shown in dumpStats(), as they are when testing this on master

Checklist

I have:

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

Notes

When very high value weights are specified for downstreams to where the sum is greater than the value of int, more queries will be sent to downstreams earlier in the list than later ones with the same weight, since the earlier downstream's weight will be a high percentage of the total sum. However, this seems preferable to having queries be dropped with no-policy in this edge case.

Dan McCombs
Restrict value range for weight parameter, avoid overflowing and drop…
…ping queries if the sum of all weights is greater than the max value of int.

@rgacogne rgacogne added this to the dnsdist-1.4.0 milestone Apr 6, 2018

@rgacogne

This comment has been minimized.

Member

rgacogne commented Apr 6, 2018

Unrelated authoritative tests failure in travis (ALIAS), restarted.

@@ -294,7 +294,8 @@ Servers
address="IP:PORT", -- IP and PORT of the backend server (mandatory)
qps=NUM, -- Limit the number of queries per second to NUM, when using the `firstAvailable` policy
order=NUM, -- The order of this server, used by the `leastOustanding` and `firstAvailable` policies
weight=NUM, -- The weight of this server, used by the `wrandom` and `whashed` policies
weight=NUM, -- The weight of this server, used by the `wrandom` and `whashed` policies, default: 1
-- Supported values are a minimum of 1, and a maximum of 2147483647.

This comment has been minimized.

@rgacogne

rgacogne Apr 6, 2018

Member

Technically a signed int maximum value could be as small as 32767, but let's assume we are not going to run on 16-bit.

This comment has been minimized.

@dmccombs

dmccombs Apr 6, 2018

Contributor

Yeah, the actual check is pulling the max dynamically, but I went with the common value for documentation here. If someone happens to run on 16-bit and tries to add something larger than it, at least the error log will say the correct max.

@rgacogne

This comment has been minimized.

Member

rgacogne commented Apr 6, 2018

Thank you for this PR!

@rgacogne rgacogne merged commit 062e900 into PowerDNS:master Apr 6, 2018

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