dnsdist: Warn on unsupported parameters #10115

cmouse · 2021-02-22T18:18:43Z

Short description

Change behavior of many commands to actually check the passed in vars / parameters that they are consumed fully. If they are not, we assume that they were not supported, and indicate first failing var / parameter.

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)

omoerbeek · 2021-02-23T06:58:58Z

This idea would be nice for the recursor as well. Some general thoughts:
The three lines

 if (vars->count("idleTimeout")) {
        frontend->d_idleTimeout = boost::get<int>((*vars)["idleTimeout"]);
        vars->erase("idleTimeout");
 }

are repeated over and over again.
I'd love to see a function in the line of

getOptionalValue<int>(frontend->d_idleTimeout , vars, "idleTimeout");

to avoid the repeating.
Also, the exceptions thrown mean that previously working config (with typos) now cause startup failure. That is likely too much, a warning would be better imo.

cmouse · 2021-03-08T21:28:03Z

Tested that it does indeed print warning

> pc = newPacketCache(100, {dontAget=true})
Unknown key 'dontAget' given - ignored

rgacogne

Looks like we often pass a copy of the variables, so we need to fix that at least. The general concept is awesome and the code looks good.

pdns/dnsdist-lua.cc

pdns/dnsdist-lua-actions.cc

pdns/dnsdist-lua.cc

pdns/dnsdist-lua.hh

pdns/dnsdist-lua.cc

rgacogne

I love the general idea, and the implementation looks good! A few issues remain and I would not mind a rebase on the current master to ensure the checks are still valid, but other than that I'd like to merge it soon!

rgacogne · 2021-05-19T08:11:44Z

pdns/dnsdist-lua.cc


      ComboAddress serverAddr;
      std::string serverAddressStr;
      if(auto addrStr = boost::get<string>(&pvars)) {
        serverAddressStr = *addrStr;
        if(qps) {
-          vars["qps"] = std::to_string(*qps);
+          (*vars)["qps"] = std::to_string(*qps);


Does that work? I would expect that de-referencing an unset boost::optional would be a problem.

I have to try.

rgacogne · 2021-05-19T08:18:15Z

pdns/dnsdist-lua.cc

+      getOptionalValue<bool>(vars, "mustResolve", ret->mustResolve);
+      getOptionalValue<bool>(vars, "useClientSubnet", ret->useECS);
+      getOptionalValue<bool>(vars, "useProxyProtocol", ret->useProxyProtocol);
+      getOptionalValue<bool>(vars, "disableZeroScoping", ret->disableZeroScope);


Suggested change

getOptionalValue<bool>(vars, "disableZeroScoping", ret->disableZeroScope);

getOptionalValue<bool>(vars, "disableZeroScope", ret->disableZeroScope);

hm. that would also change the parameter name no?

I believe the name is currently disableZeroScope?

rgacogne · 2021-05-19T08:21:35Z

pdns/dnsdist-lua.cc

-            boost::algorithm::to_lower(frontend->d_provider);
-          }
-
+          getOptionalValue<std::string>(vars, "provider", frontend->d_provider);


Isn't the to_lower part gone?

so it seems.

rgacogne · 2023-01-18T17:25:12Z

I'm in the process of (painfully, but that's on me) rebasing this pull request on master, just FYI.

Broken in b54e94d

rgacogne · 2023-01-26T15:35:03Z

Rebased to fix conflicts.

rgacogne · 2023-01-30T13:25:22Z

Rebased on master, and ready to be merged. A second review would be nice to have to make sure I did not mess up :)

chbruyand

Maybe we could unify a bit some integer values processing. Otherwise this lgtm and feels like a really good feature.

pdns/dnsdist-lua.cc

chbruyand · 2023-02-07T10:32:18Z

pdns/dnsdist-lua.cc

-                         if (vars.count("checkTimeout")) {
-                           config.checkTimeout = std::stoi(boost::get<string>(vars["checkTimeout"]));
+                         if (getOptionalValue<std::string>(vars, "checkClass", valueStr) > 0) {
+                           config.checkClass = std::stoi(valueStr);


As done a bit later, do we want to guard potential conversion exceptions for optional values ?

I'm also wondering if there is a good reason to not directly getOptionalValue<int>(vars, "checkClass", config.checkClass)

I'm also wondering if there is a good reason to not directly getOptionalValue<int>(vars, "checkClass", config.checkClass)

Wouldn't that fail because the Lua wrapper used the string type for that variable? We could extend the variant to allow integers, but then we need to change all the existing types and make sure the conversion works properly. One drawback that I did not notice before with the new code is that if we pass a string when we expect an integer, it is no longer considered a parsing error and we don't even warn, except at the end to say something like newServer: Unknown key 'checkClass' given - ignored, which is confusing: the key exists, but the type is wrong.

I pushed a new commit with better handling of numerical values, comments welcome!

As suggested by Charles-Henri Bruyand (thanks!).

chbruyand

lgtm!

rgacogne added dnsdist enhancement labels Feb 22, 2021

rgacogne added this to the dnsdist-1.7.0 milestone Mar 4, 2021

cmouse force-pushed the dnsdist-params branch 4 times, most recently from a0f2735 to f9ee369 Compare March 8, 2021 21:14

cmouse force-pushed the dnsdist-params branch 2 times, most recently from 4d10b80 to ea1d2d3 Compare March 9, 2021 09:44

cmouse force-pushed the dnsdist-params branch 4 times, most recently from 2705f81 to 8bc710d Compare March 24, 2021 08:53

cmouse changed the title ~~dnsdist: Fail on unsupported parameters~~ dnsdist: Warn on unsupported parameters Mar 24, 2021

rgacogne requested changes Mar 25, 2021

View reviewed changes

cmouse force-pushed the dnsdist-params branch 2 times, most recently from fc8bf8c to 31e4642 Compare March 25, 2021 13:46

cmouse force-pushed the dnsdist-params branch from 31e4642 to 91c3d96 Compare April 8, 2021 07:16

cmouse force-pushed the dnsdist-params branch from 91c3d96 to 364bfd7 Compare April 20, 2021 10:27

cmouse requested a review from rgacogne May 3, 2021 09:30

cmouse force-pushed the dnsdist-params branch from 364bfd7 to 3a21cb1 Compare May 3, 2021 09:30

rgacogne requested changes May 19, 2021

View reviewed changes

rgacogne modified the milestones: dnsdist-1.7.0, dnsdist-1.8.0 Nov 2, 2021

rgacogne self-requested a review November 30, 2022 17:00

rgacogne self-assigned this Nov 30, 2022

dnsdist: Warn on unsupported parameters

45f86d7

cmouse and others added 4 commits January 26, 2023 16:32

dnsdist-lua: Really ignore negative numberOfStoredSessions

a8921dd

Broken in b54e94d

test_Caching: ecsParsing is called parseECS

7b8cfb4

dnsdist-lua: newserver - Warn about invalid and unused parameters

c29eedc

dnsdist: Fix formatting in dnsdist-lua.cc

c7abe16

rgacogne force-pushed the dnsdist-params branch from 3a21cb1 to c7abe16 Compare January 26, 2023 15:34

dnsdist: Fix formatting (again)

aa69821

rgacogne approved these changes Jan 30, 2023

View reviewed changes

rgacogne requested a review from chbruyand January 30, 2023 13:24

chbruyand reviewed Feb 7, 2023

View reviewed changes

dnsdist: Better handling of failed numerical conversions

41366c4

As suggested by Charles-Henri Bruyand (thanks!).

chbruyand self-requested a review February 8, 2023 10:26

chbruyand approved these changes Feb 8, 2023

View reviewed changes

rgacogne merged commit 597a91a into PowerDNS:master Feb 8, 2023

cmouse deleted the dnsdist-params branch April 12, 2023 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dnsdist: Warn on unsupported parameters #10115

dnsdist: Warn on unsupported parameters #10115

cmouse commented Feb 22, 2021 •

edited

Loading

omoerbeek commented Feb 23, 2021

cmouse commented Mar 8, 2021

rgacogne left a comment

rgacogne left a comment

rgacogne May 19, 2021

cmouse Jul 7, 2021

rgacogne May 19, 2021

cmouse Jul 7, 2021

rgacogne Jul 7, 2021

rgacogne May 19, 2021

cmouse Jul 7, 2021

rgacogne commented Jan 18, 2023

rgacogne commented Jan 26, 2023

rgacogne commented Jan 30, 2023

chbruyand left a comment

chbruyand Feb 7, 2023

chbruyand Feb 7, 2023

rgacogne Feb 7, 2023

rgacogne Feb 7, 2023

chbruyand left a comment

	getOptionalValue<bool>(vars, "disableZeroScoping", ret->disableZeroScope);
	getOptionalValue<bool>(vars, "disableZeroScope", ret->disableZeroScope);

dnsdist: Warn on unsupported parameters #10115

dnsdist: Warn on unsupported parameters #10115

Conversation

cmouse commented Feb 22, 2021 • edited Loading

Short description

Checklist

omoerbeek commented Feb 23, 2021

cmouse commented Mar 8, 2021

rgacogne left a comment

Choose a reason for hiding this comment

rgacogne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgacogne commented Jan 18, 2023

rgacogne commented Jan 26, 2023

rgacogne commented Jan 30, 2023

chbruyand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chbruyand left a comment

Choose a reason for hiding this comment

cmouse commented Feb 22, 2021 •

edited

Loading