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

Change truncateTC to defaulting to off #4859

Merged

Conversation

RobinGeuze
Copy link
Contributor

Short description

Having truncateTC enabled by default causes dnsdist to break compatibility with RFC 6891 by default. Closes #4857

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code

@@ -1406,7 +1406,7 @@ instantiate a server with additional parameters
* `addDelay(netmask, n)`: delay answers within that netmask by n milliseconds
* `addDelay({netmask, netmask}, n)`: delay answers within those netmasks (together) by n milliseconds
* Answer changing functions:
* `truncateTC(bool)`: if set (default) truncate TC=1 answers so they are actually empty. Fixes an issue for PowerDNS Authoritative Server 2.9.22.
* `truncateTC(bool)`: if set (default to no) truncate TC=1 answers so they are actually empty. Fixes an issue for PowerDNS Authoritative Server 2.9.22. Note: turning this on breaks compatibility with RFC 6891.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Defaults to no starting with dnsdist 1.2.0"

@rgacogne rgacogne added this to the dnsdist-1.2.0 milestone Jan 6, 2017
Make it more concise specifying when the default behaviour changed.
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits, LGTM otherwise.

@@ -366,7 +366,7 @@ const std::vector<ConsoleKeyword> g_consoleKeywords{
{ "topResponseRule", true, "", "move the last response rule to the first position" },
{ "topRule", true, "", "move the last rule to the first position" },
{ "topSlow", true, "[top][, limit][, labels]", "show `top` queries slower than `limit` milliseconds, grouped by last `labels` labels" },
{ "truncateTC", true, "bool", "if set (default) truncate TC=1 answers so they are actually empty. Fixes an issue for PowerDNS Authoritative Server 2.9.22" },
{ "truncateTC", true, "bool", "if set (defaults to no starting with dnsdist 1.2.0) truncate TC=1 answers so they are actually empty. Fixes an issue for PowerDNS Authoritative Server 2.9.22. Note: turning this one breaks compatibility with RFC 6891." },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/turning this one/turning this on/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -136,7 +136,7 @@ int g_tcpSendTimeout{2};
int g_udpTimeout{2};

bool g_servFailOnNoPolicy{false};
bool g_truncateTC{1};
bool g_truncateTC{0};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we use 0 or 1 in other places, but I'd prefer you use false here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants